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 04:06:40PM +0200, Jan Beulich wrote:
> On 26.10.2022 15:58, Roger Pau Monné wrote:
> > On Wed, Oct 26, 2022 at 02:47:43PM +0200, Jan Beulich wrote:
> >> On 25.10.2022 16:44, Roger Pau Monne wrote:
> >>> @@ -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.
> 
> It's not so much a matter of being clearer, but a matter of consistency:
> Why does the same consideration not apply in rom_write()? In fact both
> uses there are (already before the change) combined with further
> conditions (checking header->rom_enabled and new_enabled). If the
> inconsistency is on purpose (and perhaps necessary), I'd like to first
> understand why that is before deciding what to do about it. A comment
> like you suggest it _may_ be the route to go.

ROM register is more complex to handle, because the same register
that's used to store the address also contains the bit that can
trigger whether it's mapped into the guest p2m or not
(PCI_ROM_ADDRESS_ENABLE).  So ROM BAR writes with the ROM BAR mapped
and the PCI_ROM_ADDRESS_ENABLE bit also set in the to be written value
will be rejected, because we only allow to first disable the ROM and
then change the address (which is likely to not play well with OSes,
but so far I haven't been able to test ROM BAR register usage on PVH).

I do think for consistency it would be better to use rom->enabled in
the first conditional of rom_write() check, so it would be:

    if ( rom->enabled && new_enabled )
    {
        gprintk(XENLOG_WARNING,
                "%pp: ignored ROM BAR write while mapped\n",
                &pdev->sbdf);
        return;
    }

So that we also allow changing the address of ROM BARs even with
memory decoding and PCI_ROM_ADDRESS_ENABLE as long as the ROM BAR is
not mapped in the p2m.

Would you be fine with the comment in the previous email added and
rom_write() adjusted as suggested above?

Thanks, Roger.