Re: [PATCH for-4.17 v2 5/5] vpci: refuse BAR writes only if the BAR is mapped

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

On Wed, Oct 26, 2022 at 02:47:43PM +0200, Jan Beulich wrote:
> On 25.10.2022 16:44, Roger Pau Monne wrote:
> > Writes to the BARs are ignored if memory decoding is enabled for the
> > device, and the same happen with ROM BARs if the write is an attempt
> > to change the position of the BAR without disabling it first.
> > 
> > The reason of ignoring such writes is a limitation in Xen, as it would
> > need to unmap the BAR, change the address, and remap the BAR at the
> > new position, which the current logic doesn't support.
> > 
> > Some devices however seem to (wrongly) have the memory decoding bit
> > hardcoded to enabled, and attempts to disable it don't get reflected
> > on the command register.
> > 
> > This causes issues for well behaved guests that disable memory
> > decoding and then try to size the BARs, as vPCI will think memory
> > decoding is still enabled and ignore the write.
> 
> Just to avoid misunderstandings: "guests" here includes Dom0? In such
> cases we typically prefer to say "domains". This then also affects
> the next (final) paragraph.

Right, will adjust.

> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -128,7 +128,10 @@ static void modify_decoding(const struct pci_dev 
> > *pdev, uint16_t cmd,
> >      }
> >  
> >      if ( !rom_only )
> > +    {
> >          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> > +        header->bars_mapped = map;
> > +    }
> >      else
> >          ASSERT_UNREACHABLE();
> >  }
> > @@ -355,13 +358,13 @@ static int modify_bars(const struct pci_dev *pdev, 
> > uint16_t cmd, bool rom_only)
> >  static void cf_check cmd_write(
> >      const struct pci_dev *pdev, unsigned int reg, uint32_t cmd, void *data)
> >  {
> > -    uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
> > +    struct vpci_header *header = data;
> >  
> >      /*
> >       * Let Dom0 play with all the bits directly except for the memory
> >       * decoding one.
> >       */
> > -    if ( (cmd ^ current_cmd) & PCI_COMMAND_MEMORY )
> > +    if ( header->bars_mapped != !!(cmd & PCI_COMMAND_MEMORY) )
> >          /*
> >           * Ignore the error. No memory has been added or removed from the 
> > p2m
> >           * (because the actual p2m changes are deferred in defer_map) and 
> > the
> > @@ -388,12 +391,12 @@ static void cf_check bar_write(
> >      else
> >          val &= PCI_BASE_ADDRESS_MEM_MASK;
> >  
> > -    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
> > +    if ( bar->enabled )
> 
> In 3 of the 4 cases you use header->bars_mapped as replacement. Since it's
> not clear to me why you don't here, could you explain this to me? (I'm
> therefore undecided whether this is merely a cosmetic [consistency] issue.)

No, it's intended to use bar->enabled here rather than
header->bars_mapped.

It's possible to have header->bars_mapped == true, but bar->enabled ==
false if memory decoding is enabled, but this BAR specifically has
failed to be mapped in the guest p2m, which means dom0 is safe to move
it for what Xen cares (ie: it won't mess with p2m mappings because
there are none for the BAR).

We could be more strict and use header->bars_mapped, but I don't think
there's a need for it.

What about adding a comment with:

/*
 * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
 * writes as long as the BAR is not mapped into the p2m.
 */

Otherwise I can switch to using header->bars_mapped if you think
that's clearer.

Thanks, Roger.