Re: [PATCH v5 5/9] vpci: simplify handling of memory decoding and ROM enable writes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.07.2026 23:02, Stewart Hildebrand wrote:
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> 
> Deferring the actual write of the PCI register bit, either the memory
> decoding or the ROM enable is not helpful, and adds an unnecessary amount of
> complexity to the preemptible handling of BAR related p2m modifications.
> 
> In the hardware domain case, whether the PCI register write is done ahead
> or after the p2m changes doesn't matter, a hardware domain has plenty of
> ways to mess with the PCI register state if it wants to.  Any poking at the
> BAR p2m regions ahead of the guest write having completed will be
> undefined.
> 
> On the other hand, for domUs the memory decoding bit shouldn't really
> change as a result of guest actions, and should always be enabled.  Guest
> toggling the memory decoding command register should only result in p2m
> modifications, but no propagation to the device PCI registers.  Having
> memory decoding unconditionally enabled ensures the domU attempting to
> perform p2m accesses while the p2m changes are taking place will always
> access the BAR contents.

I'm not quite following in particular this last aspect: Doing the P2M change
isn't atomic. How will it be possible, in principle, to guarantee accesses
will always access BAR contents (perhaps really: the contents of what BARs
point at)?

> @@ -534,22 +487,29 @@ static void cf_check cmd_write(
>       * decoding one. Bits that are not allowed for DomU are already
>       * handled above and by the rsvdp_mask.
>       */
> -    if ( header->bars_mapped != new_enabled )
> +    if ( header->bars_mapped != new_enabled &&
> +         modify_bars(pdev, cmd, false) )
>          /*
>           * 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
>           * memory decoding bit has not been changed, so leave everything 
> as-is,
>           * hoping the guest will realize and try again.
>           */
> -        modify_bars(pdev, cmd, false);
> -    else
> -        pci_conf_write16(pdev->sbdf, reg, cmd);
> +        return;
>  
>  #ifdef CONFIG_HAS_PCI_MSI
>      /* Unpopulate MSI-X table region, so accesses trap into Xen. */
>      if ( !header->bars_mapped && new_enabled && vpci_make_msix_hole(pdev) )
>          return;
>  #endif
> +
> +    /*
> +     * FIXME: for domUs we don't want the guest toggling the memory decoding
> +     * bit.  It should be set in vpci_init_header() and guest attempts to
> +     * modify it should only lead to guest p2m changes.
> +     */
> +    header->bars_mapped = new_enabled;
> +    pci_conf_write16(pdev->sbdf, reg, cmd);
>  }

First, to avoid any ambiguity, "the memory decoding bit" wants clarifying that
it's the physical bit which is meant. The virtual one we really should leave
for the guest to play with as it sees fit.

Then, disallowing the turning off of memory decoding may be problematic. I
have a vague recollection that dealing with certain quirks may require doing
so. Plus why would we force decoding to be on when it's not needed by the
domain driving the device? I'm saying this not the least because in principle
memory and I/O decoding bits should be treated equally. Yet especially on
platforms without the concept of I/O ports disallowing the turning off of that
bit would feel pretty irritating.

> @@ -927,6 +884,13 @@ int vpci_init_header(struct pci_dev *pdev)
>              goto fail;
>      }
>  
> +    if ( cmd & PCI_COMMAND_MEMORY )

Fold this condition with ...

> +    {
> +        /* Restore command register value. */
> +        header->bars_mapped = true;
> +        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> +    }
> +
>      return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;

... the pre-existing one here (perhaps by inverting and having a simply
"return 0" there)? Then modify_bars() could also be called before restoring
the command register, at which point it almost looks as if you could then
drop the justification for re-ordering the two operations.

Jan