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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 05, 2022 at 05:14:14PM +0200, Jan Beulich wrote:
> 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).

Sorry, should have been clearer. My question was to use pdev->sbdf for
the second instance.  I see now that you don't print the function, so
that's fine.

Thanks, Roger.