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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2022 08:21, Jan Beulich wrote:
> On 05.05.2022 21:10, Andrew Cooper wrote:
>> On 29/04/2022 14:05, 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>
>>>
>>> --- 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) )
>>
>> I'm fairly sure this doesn't do what you want it to.
>>
>> .fn is a 3 bit bitfield, meaning the += will be truncated before the
>> compare.
> 
> And this is precisely what I'm after: I want to stop once the value
> has wrapped.
> 
>>> +                        {
>>> +                            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,
>>> +                                   stride);
>>> +                            sbdf = pdev->sbdf;
>>> +                        }
>>> +                        if ( PCI_FUNC(stride) )
>>
>> This is an obfuscated way of writing stride < 8.
> 
> And intentionally so, matching a few other similar instances elsewhere.
> An open-coded 8 here doesn't really make clear where that 8 would be
> coming from. The use of PCI_FUNC(), otoh, documents what's meant.
> 
>> Given the printk(), if we actually find an 8-function device, what gets
>> printed (AFAICT) will be "bumping to 8" when in fact we mean "totally
>> ignoring the option".  I think this really wants an else clause.
> 
> Yes, "bumping to 8" is what is being printed in that case. I did
> realize the slight anomaly when writing the code and I observed
> (verified) it also in testing. But I didn't see a good reason for an
> "else" here - 8 being mentioned in the log message is clear enough
> for anyone vaguely understanding phantom functions. But if you strongly
> think we need to make the code yet larger and indentation yet
> unhelpfully deeper, then I will (begrudgingly) do what you ask for. But
> please explicitly confirm.

Like for the first few patches of the IOMMU large page series, I'm
going to put this in (with Roger's R-b) by the end of the week on
the assumption that my reply (above) did address your concerns.

Jan