linux-kernel - Re: [PATCH v3 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZxUWlSFEPDCCXaq0@x1-carbon.lan>
Date: Sun, 20 Oct 2024 16:41:25 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Frank Li <Frank.li@....com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	imx@...ts.linux.dev, dlemoal@...nel.org, maz@...nel.org,
	tglx@...utronix.de, jdmason@...zu.us
Subject: Re: [PATCH v3 4/6] PCI: endpoint: pci-epf-test: Add doorbell test
 support

On Fri, Oct 18, 2024 at 11:13:34AM -0400, Frank Li wrote:
> On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote:
> > > +	/* Only revid >=1 support RC-to-EP Door bell */
> > > +	ret = epf->header->revid > 0 ?  pci_epf_alloc_doorbell(epf, 1) : -EINVAL;
> >
> > I really, really don't like this idea.
> >
> > This means that you would need to write a revid > 1 in configfs to test this.
> > I also don't think that it is right that pci-epf-test takes ownership of "rev".
> >
> > How about something like this instead:
> >
> > My thinking is that you add a doorbell_capable struct member to epc_features,
> > and then populate CAPS_DOORBELL_SUPPORT based on epc_features in
> > pci_epf_test_init_caps() (similar to how my proposal sets CAPS_MSI_SUPPORT).
> 
> The primary issue is that the doorbell is not a capability of the EPC
> itself; rather, it's a capability of the entire system that requires an
> external MSI/ITS controller. The CAPS_DOORBELL_SUPPORT should handle this
> feature. Even we needn't CAPS_DOORBELL_SUPPORT, just call
> pci_epf_alloc_doorbell(), if error return, means not support DOORBELL.

Well, the idea is that CAPS_DOORBELL_SUPPORT bit is to tell the host side
driver (pci-endpoint-test.c) that the EPF supports doorbell.

In other words, if pcitest -B is executed, but the EP (pci-epf-test) does
not set the CAPS_DOORBELL_SUPPORT bit to one in the CAPS register,
the host side driver (pci-endpoint-test.c) can error out immediately,
no need to even trying to send any command to the EP.

(We can do the same with MSI and MSI-X, no need to send a command to the EP
if the EP has CAPS (CAPS_MAGIC is set), but does not indicate support for
MSI/MSI-X.)


> To use the doorbell functionality, the revid can clearly inform users that
> this feature breaks previous compatibility. Users will need to update the
> host-side driver, PID/VID values, and the pcitest tools accordingly.

I still really don't like the revid idea.


> One potential problem is that if the EPC supports CAPS_DOORBELL_SUPPORT,
> but the user continues to use older PID/VID values to enable EPF testing,
> the pcitest tool may treat the doorbell BAR as a normal BAR. This could
> lead to confusion for users as to why their system breaks after a kernel
> update.

How about we add a new pcitest --set-doorbell-mode option
(that is similar to pcitest -i which sets the interrupt mode to use).

That way, we can do:
./pcitest --set-doorbell-mode 1
(This will enable doorbell for e.g. BAR0, pci-epf-test will call
pci_epf_alloc_doorbell() when receiving this command from the RC side.
The command will return failure if pci_epf_alloc_doorbell() returned failure.)

./pcitest -B
(This will perform the doorbell test)

./pcitest --set-doorbell-mode 0
(This will disable the doorbell for BAR0,
so it will again not trigger IRQs when BAR0 is written,
and pcitest's tests to read/write the BARs will again behave as expected.)

(We probably also need another option pcitest --get-doorbell-mode.)

I think this should solve all your concerns. Thoughts?


Kind regards,
Niklas