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 <[email protected]>
> Signed-off-by: Jan Beulich <[email protected]>
>
> --- 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.
> + {
> + 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.
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.
~Andrew
