Re: [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.05.2022 17:00, Roger Pau Monné wrote:
> On Fri, Apr 29, 2022 at 03:05:32PM +0200, Jan Beulich wrote:
>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
>> unless you have verified the sender and know the content is safe.
>>
>> IOMMU code mapping / unmapping devices and interrupts will misbehave if
>> a wrong command line option declared a function "phantom" when there's a
>> real device at that position. Warn about this and adjust the specified
>> stride (in the worst case ignoring the option altogether).
>>
>> Requested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> FWIW, I would be fine with just discarding the stride option if one of
> the phantom devices happen to report vendor/device IDs on the config
> space.

Well, I thought I'd try a best-effort adjustment rather than simply
ignoring an option.

>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -451,7 +451,24 @@ static struct pci_dev *alloc_pdev(struct
>>                           phantom_devs[i].slot == PCI_SLOT(devfn) &&
>>                           phantom_devs[i].stride > PCI_FUNC(devfn) )
>>                      {
>> -                        pdev->phantom_stride = phantom_devs[i].stride;
>> +                        pci_sbdf_t sbdf = pdev->sbdf;
>> +                        unsigned int stride = phantom_devs[i].stride;
>> +
>> +                        while ( (sbdf.fn += stride) > PCI_FUNC(devfn) )
>> +                        {
>> +                            if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) == 
>> 0xffff &&
>> +                                 pci_conf_read16(sbdf, PCI_DEVICE_ID) == 
>> 0xffff )
>> +                                continue;
>> +                            stride <<= 1;
>> +                            printk(XENLOG_WARNING
>> +                                   "%pp looks to be a real device; bumping 
>> %04x:%02x:%02x stride to %u\n",
>> +                                   &sbdf, phantom_devs[i].seg,
>> +                                   phantom_devs[i].bus, 
>> phantom_devs[i].slot,
> 
> Can't you use pdev->sbdf here?

No - sbdf was altered from pdev->sbdf (and is also shorter to use),
and for the 2nd item I'm intentionally omitting the function part
(to match the command line option).

Jan