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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 8/14/25 12:03, Roger Pau Monne wrote:
> 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. This is not the current behavior for domUs, so add
> a note that it would preferably done that way.
> 

Nit: I think you want:
Resolves: https://gitlab.com/xen-project/xen/-/issues/98

> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  xen/drivers/vpci/header.c | 42 +++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 1035dcca242d..1a501a0ba47e 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -163,8 +163,6 @@ static void modify_decoding(const struct pci_dev *pdev, 
> uint16_t cmd,
>              if ( pci_check_bar(pdev, _mfn(PFN_DOWN(bar->addr)),
>                                 _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
>                  bar->enabled = map;
> -            header->rom_enabled = map;
> -            pci_conf_write32(pdev->sbdf, rom_pos, val);

rom_pos and val are unused

drivers/vpci/header.c: In function ‘modify_decoding’:
drivers/vpci/header.c:160:22: error: unused variable ‘val’ 
[-Werror=unused-variable]
  160 |             uint32_t val = bar->addr |
      |                      ^~~
drivers/vpci/header.c:158:26: error: unused variable ‘rom_pos’ 
[-Werror=unused-variable]
  158 |             unsigned int rom_pos = (i == PCI_HEADER_NORMAL_NR_BARS)
      |                          ^~~~~~~

Other than that the patch looks good to me

>              return;
>          }
>  
> @@ -174,14 +172,6 @@ static void modify_decoding(const struct pci_dev *pdev, 
> uint16_t cmd,
>                             _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
>              bar->enabled = map;
>      }
> -
> -    if ( !rom_only )
> -    {
> -        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> -        header->bars_mapped = map;
> -    }
> -    else
> -        ASSERT_UNREACHABLE();
>  }
>  
>  bool vpci_process_pending(struct vcpu *v)
> @@ -547,16 +537,23 @@ 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 != !!(cmd & PCI_COMMAND_MEMORY) )
> +    if ( header->bars_mapped != !!(cmd & PCI_COMMAND_MEMORY) &&
> +         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;
> +
> +    /*
> +     * 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 = cmd & PCI_COMMAND_MEMORY;
> +    pci_conf_write16(pdev->sbdf, reg, cmd);
>  }
>  
>  static uint32_t cf_check guest_cmd_read(
> @@ -712,17 +709,11 @@ static void cf_check rom_write(
>          rom->guest_addr = rom->addr;
>      }
>  
> -    if ( !header->bars_mapped || rom->enabled == new_enabled )
> -    {
> -        /* Just update the ROM BAR field. */
> -        header->rom_enabled = new_enabled;
> -        pci_conf_write32(pdev->sbdf, reg, val);
> -    }
>      /*
>       * Pass PCI_COMMAND_MEMORY or 0 to signal a map/unmap request, note that
>       * this fabricated command is never going to be written to the register.
>       */
> -    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
> +    if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
>          /*
>           * No memory has been added or removed from the p2m (because the 
> actual
>           * p2m changes are deferred in defer_map) and the ROM enable bit has
> @@ -733,6 +724,8 @@ static void cf_check rom_write(
>           */
>          return;
>  
> +    header->rom_enabled = new_enabled;
> +    pci_conf_write32(pdev->sbdf, reg, val);
>      if ( !new_enabled )
>      {
>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
> @@ -1057,6 +1050,13 @@ int vpci_init_header(struct pci_dev *pdev)
>              goto fail;
>      }
>  
> +    if ( cmd & PCI_COMMAND_MEMORY )
> +    {
> +        /* 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;
>  
>   fail: