Re: [PATCH v4 4/4] vpci: allow 32-bit BAR writes with memory decoding enabled

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

On Mon, Apr 06, 2026 at 03:11:58PM -0400, Stewart Hildebrand wrote:
> Currently, Xen vPCI refuses BAR writes if the BAR is mapped in p2m. If
> firmware initializes a 32-bit BAR to a bad address, Linux may try to
> write a new address to the 32-bit BAR without disabling memory decoding.
> Since Xen refuses such writes, the BAR (and thus PCI device) will be
> non-functional.
> 
> Allow the hardware domain to issue 32-bit BAR writes with memory
> decoding enabled. This increases the compatibility of PVH dom0 with more
> hardware.
> 
> Note that Linux aims at disabling memory decoding before writing 64-bit
> BARs. Continue to refuse 64-bit BAR writes in Xen while those BARs are
> mapped for now to avoid mapping half-updated BARs in p2m.

While Linux does the disabling for 64bit BARs, I wonder if at some we
will need to handle 64bit BAR writes anyway for other OSes, specially
with domU support.

> Resolves: https://gitlab.com/xen-project/xen/-/issues/197
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> ---
> v3->v4:
> * rebase on dynamically allocated map queue
> 
> v2->v3:
> * minor tweaks for fixed number of map/unmap slots
> 
> v1->v2:
> * rework on top of queued BAR map/unmap operation machinery
> 
> RFC->v1:
> * keep memory decoding enabled in hardware
> * allow write while memory decoding is enabled for 32-bit BARs only
> * rework BAR mapping machinery to support unmap-then-map operation
> ---
>  xen/drivers/vpci/header.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 20fe380552f4..dc4f585b4e40 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -670,6 +670,7 @@ static void cf_check bar_write(
>  {
>      struct vpci_bar *bar = data;
>      bool hi = false;
> +    uint16_t cmd = 0;
>  
>      ASSERT(is_hardware_domain(pdev->domain));
>  
> @@ -683,19 +684,29 @@ static void cf_check bar_write(
>          val &= PCI_BASE_ADDRESS_MEM_MASK;
>  
>      /*
> -     * 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.
> +     * Allow 64-bit BAR writes only when the BAR is not mapped in p2m. Always
> +     * allow 32-bit BAR writes.
>       */
>      if ( bar->enabled )
>      {
> -        /* If the value written is the current one avoid printing a warning. 
> */
> -        if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
> -            gprintk(XENLOG_WARNING,
> -                    "%pp: ignored BAR %zu write while mapped\n",
> -                    &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
> -        return;
> -    }
> +        if ( bar->type == VPCI_BAR_MEM32 )
> +        {
> +            if ( val == bar->addr )
> +                return;
>  
> +            cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> +            modify_bars(pdev, cmd, false, false);
> +        }
> +        else
> +        {
> +            /* If the value written is the same avoid printing a warning. */
> +            if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
> +                gprintk(XENLOG_WARNING,
> +                        "%pp: ignored BAR %zu write while mapped\n",
> +                        &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
> +            return;
> +        }
> +    }
>  
>      /*
>       * Update the cached address, so that when memory decoding is enabled
> @@ -715,6 +726,9 @@ static void cf_check bar_write(
>      }
>  
>      pci_conf_write32(pdev->sbdf, reg, val);

I don't think it matters a lot, but here we are changing the position
of the BAR in the host memory map while the mappings are still active.

> +
> +    if ( bar->enabled )
> +        modify_bars(pdev, cmd, false, true);
>  }
>  
>  static void cf_check guest_mem_bar_write(const struct pci_dev *pdev,

As it's in context here - we possibly want to do something similar
with guest writes now that we can?

While we might not have a user-case for domUs ATM, I think it's vest
if we also fix that use case at the same time that dom0 is fixed
(unless there's something that prevents it).

Thanks, Roger.