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.coop...@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>>>
>>> --- 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
