Re: [PATCH] PCI: pci-stub: accept exceptions to the ID- and class-based matching

[Laszlo Ersek <lersek@redhat.com>]

On 11/07/16 17:49, Alex Williamson wrote:
> On Tue, 25 Oct 2016 21:24:25 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 10/25/16 20:42, Alex Williamson wrote:
>>> On Tue, 25 Oct 2016 20:24:19 +0200
>>> Laszlo Ersek <lersek@redhat.com> wrote:
>>>   
>>>> Some systems have multiple instances of the exact same kind of PCI device
>>>> installed. When VFIO users intend to assign these devices to VMs, they
>>>> occasionally don't want to assign all of them; they'd keep a few for
>>>> host-side use. The current ID- and class-based matching in pci-stub
>>>> doesn't accommodate this use case, so users are left with either
>>>> rc.local-style host boot scripts, or QEMU wrapper scripts (which are
>>>> inferior to a pure libvirt environment).
>>>>
>>>> Introduce the "except" module parameter for pci-stub. In addition to
>>>> "ids", users can specify a list of Domain:Bus:Device.Function tuples. The
>>>> tuples are parsed and saved before pci_add_dynid() is called. The pci-stub
>>>> probe function will fail for the listed devices, for the initial and all
>>>> later (explicit) binding attempts.
>>>>
>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>> Cc: Andrei Grigore <andrei.grg@gmail.com>
>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>>> Cc: Jayme Howard <g.prime@gmail.com>
>>>> Reported-by: Andrei Grigore <andrei.grg@gmail.com>
>>>> Ref: https://www.redhat.com/archives/vfio-users/2016-October/msg00121.html
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>> ---
>>>>  drivers/pci/pci-stub.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 63 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci-stub.c b/drivers/pci/pci-stub.c
>>>> index 886fb3570278..120c29609c44 100644
>>>> --- a/drivers/pci/pci-stub.c
>>>> +++ b/drivers/pci/pci-stub.c
>>>> @@ -26,8 +26,44 @@ MODULE_PARM_DESC(ids, "Initial PCI IDs to add to the stub driver, format is "
>>>>  		 "\"vendor:device[:subvendor[:subdevice[:class[:class_mask]]]]\""
>>>>  		 " and multiple comma separated entries can be specified");
>>>>  
>>>> +#define MAX_EXCEPT 16
>>>> +
>>>> +static unsigned num_except;
>>>> +static struct except {
>>>> +	u16 domain;
>>>> +	u16 devid;
>>>> +} except[MAX_EXCEPT];
>>>> +
>>>> +/*
>>>> + * Accommodate substrings like "0000:00:1c.4," MAX_EXCEPT times, with the comma
>>>> + * replaced with '\0' in the last instance
>>>> + */
>>>> +static char except_str[13 * MAX_EXCEPT] __initdata;
>>>> +
>>>> +module_param_string(except, except_str, sizeof except_str, 0);
>>>> +MODULE_PARM_DESC(except, "Comma-separated list of PCI addresses to except "
>>>> +		 "from the ID- and class-based binding. The address format is "
>>>> +		 "Domain:Bus:Device.Function (all components are required and "
>>>> +		 "written in hex), for example, 0000:00:1c.4. At most "
>>>> +		 __stringify(MAX_EXCEPT) " exceptions are supported.");  
>>>
>>> So a user needs to specify both a set of set of vendor:device ids AND an
>>> exception list?  Wouldn't it be easier to make a list of _included_
>>> devices by address, w/o needing an ids= list?  
>>
>> First, I didn't want to drop the ids=... parameter for compatibility
>> reasons.
>>
>> Second (because I realize you're likely not suggesting to *drop* "ids",
>> just to provide a positive-sense replacement for it), I have no idea how
>> to influence the PCI subsystem like this. As far as I know (which is
>> very little, admittedly), the only way to get the PCI subsystem to call
>> back into a specific driver probe function is to provide
>> device/vendor/subsystem IDs, and class patterns, with that device driver.
>>
>> If I don't provide those IDs (either statically or dynamically), then
>> the driver will bind nothing, because the core won't invoke the probe
>> function for any device.
>>
>> If I provided a match-all pattern (not sure how), then the core would
>> call the probe function for all devices. While that might help move the
>> actual positive filtering into the stub probe function (i.e., without
>> the "ids" parameter), I don't think it would be appreciated.
> 
> This is why we have a driver_override available for devices, it
> supersedes the PCI ID match table.  I'd rather see work towards making
> a commandline interface to driver_override, which potentially benefits
> drivers beyond pci-stub, beyond PCI actually (in fact, we can pretty
> much eliminate pci-stub altogether simply by specifying a dummy
> driver_override that doesn't match any drivers, ex. "NONE").
> Regardless of an exception list being the easiest, or understood way to
> currently code pci-stub to get what you want, I still consider an
> id+exception list to be convoluted. Thanks,

driver_override sounds superior, indeed. I didn't know about it, I've
now read up on it in "ABI/testing/sysfs-bus-pci".

Unfortunately, I don't think I can personally tackle the kernel cmdline
enablement of driver_override.

Feel free to drop this patch (and thanks for the information).

Laszlo

>>> FWIW, I think the reason
>>> this hasn't been done to date is that PCI bus addresses (except for
>>> root bus devices) are not stable.  Depending on the system, the address
>>> of a given device may change, not only based on the slot where the
>>> device is installed, but whether other devices in other slots are
>>> populated.  
>>
>> I agree.
>>
>> However, while the addresses are not stable in the face of hardware
>> changes, I think the addresses don't change haphazardly (that is,
>> without hardware changes).
>>
>> So, if you plug in another card, your current pci-stub.except=...
>> parameter might become invalid; but that's not very different from the
>> case when you plug in the second instance of a preexistent card right
>> now -- then the pci-stub.ids=... filter won't match uniquely anymore,
>> and assignment vs. host-side use might not work as intented.
>>
>>> This is why when ACPI needs to describe a PCI device (such
>>> as in the DMAR tables), it does so via paths.  We don't know the bus
>>> number that will be assigned to a device, but we do know
>>> deterministically how to traverse to it, for instance root bus -> root
>>> dev.fn -> intermediate dev.fn -> target dev.fn.  Thanks,  
>>
>> Yes, UEFI device paths follow this model as well. In UEFI, device paths
>> (which cover a lot more than PCI) are very clearly separated from
>> domain/bus/device/function quadruplets.
>>
>> Are there utilities in the kernel for parsing a textual device path into
>> a binary representation, and then locating the PCI device with the
>> binary devpath? (This is doable in UEFI.)
>>
>> ... Anyhow, when I started working on this patch, the first thing I
>> searched for was existing practice. There is "prior art" for specifying
>> PCI BDFs on the kernel command line; please see the following commits:
>>
>> ea9e9d802902 Specify PCI based UART for earlyprintk
>> c43088e3b8ca Documentation/kernel-parameters: add missing pciserial to
>>              the earlyprintk
>>
>> Thanks
>> Laszlo
>>
>>>   
>>>> +
>>>> +static inline bool exception_matches(const struct except *ex,
>>>> +				     const struct pci_dev *dev)
>>>> +{
>>>> +	return ex->domain == pci_domain_nr(dev->bus) &&
>>>> +	       ex->devid == PCI_DEVID(dev->bus->number, dev->devfn);
>>>> +}
>>>> +
>>>>  static int pci_stub_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>>  {
>>>> +	unsigned i;
>>>> +
>>>> +	for (i = 0; i < num_except; i++)
>>>> +		if (exception_matches(&except[i], dev)) {
>>>> +			dev_info(&dev->dev, "skipped by stub\n");
>>>> +			return -EPERM;
>>>> +		}
>>>> +
>>>>  	dev_info(&dev->dev, "claimed by stub\n");
>>>>  	return 0;
>>>>  }
>>>> @@ -47,6 +83,33 @@ static int __init pci_stub_init(void)
>>>>  	if (rc)
>>>>  		return rc;
>>>>  
>>>> +	/* parse exceptions */
>>>> +	p = except_str;
>>>> +	while ((id = strsep(&p, ","))) {
>>>> +		int fields;
>>>> +		unsigned domain, bus, dev, fn;
>>>> +
>>>> +		if (*id == '\0')
>>>> +			continue;
>>>> +
>>>> +		fields = sscanf(id, "%x:%x:%x.%x", &domain, &bus, &dev, &fn);
>>>> +		if (fields != 4 || domain > 0xffff || bus > 0xff ||
>>>> +		    dev > 0x1f || fn > 0x7) {
>>>> +			printk(KERN_WARNING
>>>> +			       "pci-stub: invalid exception \"%s\"\n", id);
>>>> +			continue;
>>>> +		}
>>>> +
>>>> +		if (num_except < MAX_EXCEPT) {
>>>> +			struct except *ex = &except[num_except++];
>>>> +
>>>> +			ex->domain = domain;
>>>> +			ex->devid = PCI_DEVID(bus, PCI_DEVFN(dev, fn));
>>>> +		} else
>>>> +			printk(KERN_WARNING
>>>> +			       "pci-stub: no room for exception \"%s\"\n", id);
>>>> +	}
>>>> +
>>>>  	/* no ids passed actually */
>>>>  	if (ids[0] == '\0')
>>>>  		return 0;  
>>>   
>>
>