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

["Roger Pau Monné" <roger.pau@citrix.com>]

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

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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.

> --- 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?

Thanks, Roger.