On 6/17/2021 2:33 AM, Jason Gunthorpe wrote:
On Thu, Jun 17, 2021 at 02:28:36AM +0300, Max Gurtovoy wrote:
On 6/16/2021 3:34 AM, Jason Gunthorpe wrote:
On Tue, Jun 15, 2021 at 06:22:45PM -0600, Alex Williamson wrote:
On Tue, 15 Jun 2021 20:32:57 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
On Tue, Jun 15, 2021 at 05:22:42PM -0600, Alex Williamson wrote:
b) alone is a functional, runtime difference.
I would state b) differently:
b) Ignore the driver-override-only match entries in the ID table.
No, pci_match_device() returns NULL if a match is found that is marked
driver-override-only and a driver_override is not specified. That's
the same as no match at all. We don't then go on to search past that
match in the table, we fail to bind the driver. That's effectively an
anti-match when there's no driver_override on the device.
anti-match isn't the intention. The deployment will have match tables
where all entires are either flags=0 or are driver-override-only.
I'd expect pci-pf-stub to have one of each, an any-id with
override-only flag and the one device ID currently in the table with
no flag.
Oh Hum. Actually I think this shows the anti-match behavior is
actually a bug.. :(
For something like pci_pf_stub_whitelist, if we add a
driver_override-only using the PCI any id then it effectively disables
new_id completely because the match search will alway find the
driver_override match first and stop searching. There is no chance to
see things new_id adds.
Actually the dynamic table is the first table the driver search. So new_id
works exactly the same AFAIU.
Oh, even better, so it isn't really an issue
But you're right for static mixed table (I assumed that this will never
happen I guess).
Me too, we could organize the driver-overrides to be last
Yes we could, but in 2 years from now I'll forget this rule :)
And others may not be aware of it.
- found_id = pci_match_id(drv->id_table, dev);
- if (found_id) {
+ ids = drv->id_table;
+ while ((found_id = pci_match_id(ids, dev))) {
Yeah, keep searching makes logical sense to me
diff --git a/drivers/pci/pci-pf-stub.c b/drivers/pci/pci-pf-stub.c
index 45855a5e9fca..49544ba9a7af 100644
+++ b/drivers/pci/pci-pf-stub.c
@@ -19,6 +19,7 @@
*/
static const struct pci_device_id pci_pf_stub_whitelist[] = {
{ PCI_VDEVICE(AMAZON, 0x0053) },
+ { PCI_DEVICE_FLAGS(PCI_ANY_ID, PCI_ANY_ID,
PCI_ID_F_STUB_DRIVER_OVERRIDE) }, /* match all by default (override) */
/* required last entry */
{ 0 }
And we don't really want this change any more right? No reason to put
pci_stub in the module.alias file?
I actually did it in the patches I attached earlier.
It will look like:
stub_pci:v*d*sv*sd*bc*sc*i*
pci:v00001D0Fd00000053sv*sd*bc*sc*i*
I think it's good practice to avoid matching automatically and auto
loading any_id_override and vfio_override drivers in general.
Do you see a reason not adding this alias for stub drivers but adding it
to vfio_pci drivers ?
Thanks,
Jason
