Re: [PATCH for-4.17 v2 4/5] pci: do not disable memory decoding for devices

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

On Wed, Oct 26, 2022 at 02:35:44PM +0200, Jan Beulich wrote:
> On 25.10.2022 16:44, Roger Pau Monne wrote:
> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -121,7 +121,9 @@ static void modify_decoding(const struct pci_dev *pdev, 
> > uint16_t cmd,
> >          }
> >  
> >          if ( !rom_only &&
> > -             (bar->type != VPCI_BAR_ROM || header->rom_enabled) )
> > +             (bar->type != VPCI_BAR_ROM || header->rom_enabled) &&
> > +             pci_check_bar(pdev, _mfn(PFN_DOWN(bar->addr)),
> > +                           _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
> >              bar->enabled = map;
> >      }
> 
> What about the ROM handling immediately above from here?

Needs fixing indeed.  I had plans to short circuit the ROM only
mapping path earlier if the BAR wasn't correctly positioned, but
decided it was too complicated and then forgot to adjust the
conditional.

> > @@ -234,9 +236,25 @@ static int modify_bars(const struct pci_dev *pdev, 
> > uint16_t cmd, bool rom_only)
> >  
> >          if ( !MAPPABLE_BAR(bar) ||
> >               (rom_only ? bar->type != VPCI_BAR_ROM
> > -                       : (bar->type == VPCI_BAR_ROM && 
> > !header->rom_enabled)) )
> > +                       : (bar->type == VPCI_BAR_ROM && 
> > !header->rom_enabled)) ||
> > +             /* Skip BARs already in the requested state. */
> > +             bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
> >              continue;
> >  
> > +        /*
> > +         * Only do BAR position checks for the hardware domain, for guests 
> > it's
> > +         * assumed that the hardware domain has changed the position of any
> > +         * problematic BARs.
> > +         */
> > +        if ( is_hardware_domain(pdev->domain) &&
> > +             !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
> > +        {
> > +            printk(XENLOG_G_WARNING
> > +                   "%pp: not mapping BAR [%lx, %lx] invalid position\n",
> > +                   &pdev->sbdf, start, end);
> > +            continue;
> > +        }
> 
> I'm not convinced of it being appropriate to skip the check for DomU.
> I'd rather consider this a "fixme", as (perhaps somewhere else) we
> should return an error if a misconfigured device was passed. We cannot
> blindly leave the security of the system to tool stack + Dom0 kernel
> imo.
> 
> And then, if this is Dom0-only, I think it wants to be XENLOG_WARNING,
> i.e. without the G infix.

OK, I don't mind removing the is_hardware_domain() condition, it's
still not clear how we want to handle all this when domU support is
added.

Thanks, Roger.