[PATCH v1 0/5] vpci: allow 32-bit BAR writes with memory decoding enabled

[PATCH v1 0/5] vpci: allow 32-bit BAR writes with memory decoding enabled

[Stewart Hildebrand]

Pipeline: https://gitlab.com/xen-project/people/stewarthildebrand/xen/-/pipelines/1845628953

RFC->v1:
* rework BAR mapping machinery to support unmap-then-map operation

RFC: https://lore.kernel.org/xen-devel/20250312195019.382926-1-stewart.hildebrand@amd.com/T/#t

Stewart Hildebrand (5):
  vpci: const-ify some pdev instances
  vpci: rework error path in vpci_process_pending()
  vpci: introduce map_bars()
  vpci: use separate rangeset for BAR unmapping
  vpci: allow 32-bit BAR writes with memory decoding enabled

 xen/drivers/vpci/header.c | 220 ++++++++++++++++++++++++++------------
 xen/drivers/vpci/vpci.c   |   5 +-
 xen/include/xen/vpci.h    |  10 +-
 3 files changed, 162 insertions(+), 73 deletions(-)


base-commit: 96a587a057363e519ca74498882fac42d72670b6
-- 
2.49.0

[PATCH v1 1/5] vpci: const-ify some pdev instances

[Stewart Hildebrand]

Since 622bdd962822 ("vpci/header: handle p2m range sets per BAR"), a
non-const pdev is no longer needed for error handling in
vpci_process_pending(). Const-ify pdev in vpci_process_pending(),
defer_map(), and struct vpci_vcpu.

Get rid of const-removal workaround in modify_bars().

Take the opportunity to remove an unused parameter in defer_map().

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
This is prerequisite for ("vpci: use separate rangeset for BAR
unmapping") in order to call defer_map() with a const pdev.
---
 xen/drivers/vpci/header.c | 16 ++++------------
 xen/include/xen/vpci.h    |  2 +-
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 1f48f2aac64e..e42c8efa2302 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -175,7 +175,7 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
 bool vpci_process_pending(struct vcpu *v)
 {
-    struct pci_dev *pdev = v->vpci.pdev;
+    const struct pci_dev *pdev = v->vpci.pdev;
     struct vpci_header *header = NULL;
     unsigned int i;
 
@@ -283,8 +283,7 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
     return rc;
 }
 
-static void defer_map(struct domain *d, struct pci_dev *pdev,
-                      uint16_t cmd, bool rom_only)
+static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 {
     struct vcpu *curr = current;
 
@@ -308,7 +307,7 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
 static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 {
     struct vpci_header *header = &pdev->vpci->header;
-    struct pci_dev *tmp, *dev = NULL;
+    struct pci_dev *tmp;
     const struct domain *d;
     const struct vpci_msix *msix = pdev->vpci->msix;
     unsigned int i, j;
@@ -450,11 +449,6 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 
             if ( tmp == pdev )
             {
-                /*
-                 * Need to store the device so it's not constified and defer_map
-                 * can modify it in case of error.
-                 */
-                dev = tmp;
                 if ( !rom_only )
                     /*
                      * If memory decoding is toggled avoid checking against the
@@ -507,8 +501,6 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         d = dom_xen;
     }
 
-    ASSERT(dev);
-
     if ( system_state < SYS_STATE_active )
     {
         /*
@@ -523,7 +515,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         return apply_map(pdev->domain, pdev, cmd);
     }
 
-    defer_map(dev->domain, dev, cmd, rom_only);
+    defer_map(pdev, cmd, rom_only);
 
     return 0;
 }
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 475981cb8155..27eebdcef170 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -194,7 +194,7 @@ struct vpci {
 
 struct vpci_vcpu {
     /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
-    struct pci_dev *pdev;
+    const struct pci_dev *pdev;
     uint16_t cmd;
     bool rom_only : 1;
 };
-- 
2.49.0

Re: [PATCH v1 1/5] vpci: const-ify some pdev instances

[Roger Pau Monné]

On Sat, May 31, 2025 at 08:53:59AM -0400, Stewart Hildebrand wrote:
> Since 622bdd962822 ("vpci/header: handle p2m range sets per BAR"), a
> non-const pdev is no longer needed for error handling in
> vpci_process_pending(). Const-ify pdev in vpci_process_pending(),
> defer_map(), and struct vpci_vcpu.
> 
> Get rid of const-removal workaround in modify_bars().
> 
> Take the opportunity to remove an unused parameter in defer_map().
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

One further simplification below.

> ---
> This is prerequisite for ("vpci: use separate rangeset for BAR
> unmapping") in order to call defer_map() with a const pdev.
> ---
>  xen/drivers/vpci/header.c | 16 ++++------------
>  xen/include/xen/vpci.h    |  2 +-
>  2 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 1f48f2aac64e..e42c8efa2302 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -175,7 +175,7 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
>  
>  bool vpci_process_pending(struct vcpu *v)
>  {
> -    struct pci_dev *pdev = v->vpci.pdev;
> +    const struct pci_dev *pdev = v->vpci.pdev;
>      struct vpci_header *header = NULL;
>      unsigned int i;
>  
> @@ -283,8 +283,7 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
>      return rc;
>  }
>  
> -static void defer_map(struct domain *d, struct pci_dev *pdev,
> -                      uint16_t cmd, bool rom_only)
> +static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>  {
>      struct vcpu *curr = current;
>  
> @@ -308,7 +307,7 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>  static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>  {
>      struct vpci_header *header = &pdev->vpci->header;
> -    struct pci_dev *tmp, *dev = NULL;
> +    struct pci_dev *tmp;
>      const struct domain *d;
>      const struct vpci_msix *msix = pdev->vpci->msix;
>      unsigned int i, j;
> @@ -450,11 +449,6 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>  
>              if ( tmp == pdev )
>              {
> -                /*
> -                 * Need to store the device so it's not constified and defer_map
> -                 * can modify it in case of error.
> -                 */
> -                dev = tmp;
>                  if ( !rom_only )

You can now join this with the previous if, and reduce one level of
indentation:

if ( tmp == pdev && !rom_only )
    /* comment text */
    continue;

Thanks, Roger.

Re: [PATCH v1 1/5] vpci: const-ify some pdev instances

[Stewart Hildebrand]

On 6/5/25 05:47, Roger Pau Monné wrote:
> On Sat, May 31, 2025 at 08:53:59AM -0400, Stewart Hildebrand wrote:
>> Since 622bdd962822 ("vpci/header: handle p2m range sets per BAR"), a
>> non-const pdev is no longer needed for error handling in
>> vpci_process_pending(). Const-ify pdev in vpci_process_pending(),
>> defer_map(), and struct vpci_vcpu.
>>
>> Get rid of const-removal workaround in modify_bars().
>>
>> Take the opportunity to remove an unused parameter in defer_map().
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks!

> One further simplification below.
> 
>> ---
>> This is prerequisite for ("vpci: use separate rangeset for BAR
>> unmapping") in order to call defer_map() with a const pdev.
>> ---
>>  xen/drivers/vpci/header.c | 16 ++++------------
>>  xen/include/xen/vpci.h    |  2 +-
>>  2 files changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index 1f48f2aac64e..e42c8efa2302 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -175,7 +175,7 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
>>  
>>  bool vpci_process_pending(struct vcpu *v)
>>  {
>> -    struct pci_dev *pdev = v->vpci.pdev;
>> +    const struct pci_dev *pdev = v->vpci.pdev;
>>      struct vpci_header *header = NULL;
>>      unsigned int i;
>>  
>> @@ -283,8 +283,7 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
>>      return rc;
>>  }
>>  
>> -static void defer_map(struct domain *d, struct pci_dev *pdev,
>> -                      uint16_t cmd, bool rom_only)
>> +static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>  {
>>      struct vcpu *curr = current;
>>  
>> @@ -308,7 +307,7 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>>  static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>  {
>>      struct vpci_header *header = &pdev->vpci->header;
>> -    struct pci_dev *tmp, *dev = NULL;
>> +    struct pci_dev *tmp;
>>      const struct domain *d;
>>      const struct vpci_msix *msix = pdev->vpci->msix;
>>      unsigned int i, j;
>> @@ -450,11 +449,6 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>  
>>              if ( tmp == pdev )
>>              {
>> -                /*
>> -                 * Need to store the device so it's not constified and defer_map
>> -                 * can modify it in case of error.
>> -                 */
>> -                dev = tmp;
>>                  if ( !rom_only )
> 
> You can now join this with the previous if, and reduce one level of
> indentation:
> 
> if ( tmp == pdev && !rom_only )
>     /* comment text */
>     continue;

Will do. I'll plan to keep your R-b tag for v2 since this is a trivial
change.

Re: [PATCH v1 1/5] vpci: const-ify some pdev instances

[Stewart Hildebrand]

On 6/11/25 15:28, Stewart Hildebrand wrote:
> On 6/5/25 05:47, Roger Pau Monné wrote:
>> On Sat, May 31, 2025 at 08:53:59AM -0400, Stewart Hildebrand wrote:
>>> Since 622bdd962822 ("vpci/header: handle p2m range sets per BAR"), a
>>> non-const pdev is no longer needed for error handling in
>>> vpci_process_pending(). Const-ify pdev in vpci_process_pending(),
>>> defer_map(), and struct vpci_vcpu.
>>>
>>> Get rid of const-removal workaround in modify_bars().
>>>
>>> Take the opportunity to remove an unused parameter in defer_map().
>>>
>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>
>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks!
> 
>> One further simplification below.
>>
>>> ---
>>> This is prerequisite for ("vpci: use separate rangeset for BAR
>>> unmapping") in order to call defer_map() with a const pdev.

I'm trying a somewhat different approach for the series for v2, and this
patch will no longer strictly be prerequisite. However, this patch seems
to be a desirable cleanup by itself, so I'll send it independently.

>>> ---
>>>  xen/drivers/vpci/header.c | 16 ++++------------
>>>  xen/include/xen/vpci.h    |  2 +-
>>>  2 files changed, 5 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>>> index 1f48f2aac64e..e42c8efa2302 100644
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -175,7 +175,7 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
>>>  
>>>  bool vpci_process_pending(struct vcpu *v)
>>>  {
>>> -    struct pci_dev *pdev = v->vpci.pdev;
>>> +    const struct pci_dev *pdev = v->vpci.pdev;
>>>      struct vpci_header *header = NULL;
>>>      unsigned int i;
>>>  
>>> @@ -283,8 +283,7 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
>>>      return rc;
>>>  }
>>>  
>>> -static void defer_map(struct domain *d, struct pci_dev *pdev,
>>> -                      uint16_t cmd, bool rom_only)
>>> +static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>  {
>>>      struct vcpu *curr = current;
>>>  
>>> @@ -308,7 +307,7 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>>>  static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>  {
>>>      struct vpci_header *header = &pdev->vpci->header;
>>> -    struct pci_dev *tmp, *dev = NULL;
>>> +    struct pci_dev *tmp;
>>>      const struct domain *d;
>>>      const struct vpci_msix *msix = pdev->vpci->msix;
>>>      unsigned int i, j;
>>> @@ -450,11 +449,6 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>  
>>>              if ( tmp == pdev )
>>>              {
>>> -                /*
>>> -                 * Need to store the device so it's not constified and defer_map
>>> -                 * can modify it in case of error.
>>> -                 */
>>> -                dev = tmp;
>>>                  if ( !rom_only )
>>
>> You can now join this with the previous if, and reduce one level of
>> indentation:
>>
>> if ( tmp == pdev && !rom_only )
>>     /* comment text */
>>     continue;
> 
> Will do. I'll plan to keep your R-b tag for v2 since this is a trivial
> change.
> 

[PATCH v1 2/5] vpci: rework error path in vpci_process_pending()

[Stewart Hildebrand]

This will make further refactoring simpler.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
 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 e42c8efa2302..c1463d2ce076 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -214,27 +214,7 @@ bool vpci_process_pending(struct vcpu *v)
         }
 
         if ( rc )
-        {
-            spin_lock(&pdev->vpci->lock);
-            /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
-            spin_unlock(&pdev->vpci->lock);
-
-            /* Clean all the rangesets */
-            for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-                if ( !rangeset_is_empty(header->bars[i].mem) )
-                     rangeset_purge(header->bars[i].mem);
-
-            v->vpci.pdev = NULL;
-
-            read_unlock(&v->domain->pci_lock);
-
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
-
-            return false;
-        }
+            goto fail;
     }
     v->vpci.pdev = NULL;
 
@@ -245,6 +225,26 @@ bool vpci_process_pending(struct vcpu *v)
     read_unlock(&v->domain->pci_lock);
 
     return false;
+
+ fail:
+    spin_lock(&pdev->vpci->lock);
+    /* Disable memory decoding unconditionally on failure. */
+    modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY, false);
+    spin_unlock(&pdev->vpci->lock);
+
+    /* Clean all the rangesets */
+    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+        if ( !rangeset_is_empty(header->bars[i].mem) )
+             rangeset_purge(header->bars[i].mem);
+
+    v->vpci.pdev = NULL;
+
+    read_unlock(&v->domain->pci_lock);
+
+    if ( !is_hardware_domain(v->domain) )
+        domain_crash(v->domain);
+
+    return false;
 }
 
 static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
-- 
2.49.0

Re: [PATCH v1 2/5] vpci: rework error path in vpci_process_pending()

[Roger Pau Monné]

On Sat, May 31, 2025 at 08:54:00AM -0400, Stewart Hildebrand wrote:
> This will make further refactoring simpler.

I think you want to add:

No functional change intended.

To the commit message.

> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

The success and error paths have some shared code, that might be good
to unify, but that requires further rework.

Thanks, Roger.

Re: [PATCH v1 2/5] vpci: rework error path in vpci_process_pending()

[Stewart Hildebrand]

On 6/5/25 05:53, Roger Pau Monné wrote:
> On Sat, May 31, 2025 at 08:54:00AM -0400, Stewart Hildebrand wrote:
>> This will make further refactoring simpler.
> 
> I think you want to add:
> 
> No functional change intended.
> 
> To the commit message.

Yep, will do.

> 
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks!

> The success and error paths have some shared code, that might be good
> to unify, but that requires further rework.
> 
> Thanks, Roger.

[PATCH v1 3/5] vpci: introduce map_bars()

[Stewart Hildebrand]

Move some logic to a new function to enable code reuse.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
 xen/drivers/vpci/header.c | 56 ++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index c1463d2ce076..b09ccc5e6be6 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -173,11 +173,38 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
         ASSERT_UNREACHABLE();
 }
 
+static int map_bars(struct vpci_header *header, struct domain *d, bool map)
+{
+    unsigned int i;
+
+    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    {
+        struct vpci_bar *bar = &header->bars[i];
+        struct map_data data = {
+            .d = d,
+            .map = map,
+            .bar = bar,
+        };
+        int rc;
+
+        if ( rangeset_is_empty(bar->mem) )
+            continue;
+
+        rc = rangeset_consume_ranges(bar->mem, map_range, &data);
+
+        if ( rc )
+            return rc;
+    }
+
+    return 0;
+}
+
 bool vpci_process_pending(struct vcpu *v)
 {
     const struct pci_dev *pdev = v->vpci.pdev;
     struct vpci_header *header = NULL;
     unsigned int i;
+    int rc;
 
     if ( !pdev )
         return false;
@@ -192,30 +219,17 @@ bool vpci_process_pending(struct vcpu *v)
     }
 
     header = &pdev->vpci->header;
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-    {
-        struct vpci_bar *bar = &header->bars[i];
-        struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
-            .bar = bar,
-        };
-        int rc;
-
-        if ( rangeset_is_empty(bar->mem) )
-            continue;
+    rc = map_bars(header, v->domain, v->vpci.cmd & PCI_COMMAND_MEMORY);
 
-        rc = rangeset_consume_ranges(bar->mem, map_range, &data);
+    if ( rc == -ERESTART )
+    {
+        read_unlock(&v->domain->pci_lock);
+        return true;
+    }
 
-        if ( rc == -ERESTART )
-        {
-            read_unlock(&v->domain->pci_lock);
-            return true;
-        }
+    if ( rc )
+        goto fail;
 
-        if ( rc )
-            goto fail;
-    }
     v->vpci.pdev = NULL;
 
     spin_lock(&pdev->vpci->lock);
-- 
2.49.0

Re: [PATCH v1 3/5] vpci: introduce map_bars()

[Roger Pau Monné]

On Sat, May 31, 2025 at 08:54:01AM -0400, Stewart Hildebrand wrote:
> Move some logic to a new function to enable code reuse.

Like with the previous changes, it's helpful if you explicitly note
that no functional change is intended in the commit message (which I
think it's the case here).

> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
>  xen/drivers/vpci/header.c | 56 ++++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index c1463d2ce076..b09ccc5e6be6 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -173,11 +173,38 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
>          ASSERT_UNREACHABLE();
>  }
>  
> +static int map_bars(struct vpci_header *header, struct domain *d, bool map)
> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +    {
> +        struct vpci_bar *bar = &header->bars[i];
> +        struct map_data data = {
> +            .d = d,
> +            .map = map,
> +            .bar = bar,
> +        };
> +        int rc;
> +
> +        if ( rangeset_is_empty(bar->mem) )
> +            continue;
> +
> +        rc = rangeset_consume_ranges(bar->mem, map_range, &data);
> +
> +        if ( rc )
> +            return rc;
> +    }
> +
> +    return 0;
> +}
> +
>  bool vpci_process_pending(struct vcpu *v)
>  {
>      const struct pci_dev *pdev = v->vpci.pdev;
>      struct vpci_header *header = NULL;
>      unsigned int i;

Maybe I'm missing something, but don't you get complains from the
compiler here about i being unused after this change?

Thanks, Roger.

Re: [PATCH v1 3/5] vpci: introduce map_bars()

[Stewart Hildebrand]

On 6/5/25 06:16, Roger Pau Monné wrote:
> On Sat, May 31, 2025 at 08:54:01AM -0400, Stewart Hildebrand wrote:
>> Move some logic to a new function to enable code reuse.
> 
> Like with the previous changes, it's helpful if you explicitly note
> that no functional change is intended in the commit message (which I
> think it's the case here).

OK, will do.

>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>>  xen/drivers/vpci/header.c | 56 ++++++++++++++++++++++++---------------
>>  1 file changed, 35 insertions(+), 21 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index c1463d2ce076..b09ccc5e6be6 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -173,11 +173,38 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
>>          ASSERT_UNREACHABLE();
>>  }
>>  
>> +static int map_bars(struct vpci_header *header, struct domain *d, bool map)
>> +{
>> +    unsigned int i;
>> +
>> +    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>> +    {
>> +        struct vpci_bar *bar = &header->bars[i];
>> +        struct map_data data = {
>> +            .d = d,
>> +            .map = map,
>> +            .bar = bar,
>> +        };
>> +        int rc;
>> +
>> +        if ( rangeset_is_empty(bar->mem) )
>> +            continue;
>> +
>> +        rc = rangeset_consume_ranges(bar->mem, map_range, &data);
>> +
>> +        if ( rc )
>> +            return rc;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  bool vpci_process_pending(struct vcpu *v)
>>  {
>>      const struct pci_dev *pdev = v->vpci.pdev;
>>      struct vpci_header *header = NULL;
>>      unsigned int i;
> 
> Maybe I'm missing something, but don't you get complains from the
> compiler here about i being unused after this change?

No, i is still used in the error path.

[PATCH v1 4/5] vpci: use separate rangeset for BAR unmapping

[Stewart Hildebrand]

Introduce a new per-BAR rangeset, unmap_mem, for p2m unmapping. Rename
existing mem rangeset to map_mem, which is now only used for mapping.
Populate unmap_mem by moving just-mapped ranges from map_mem to
unmap_mem. In modify_bars(), skip recalculating the ranges when
unmapping as they are already stored in unmap_mem.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
 xen/drivers/vpci/header.c | 74 +++++++++++++++++++++++++++++----------
 xen/drivers/vpci/vpci.c   |  5 ++-
 xen/include/xen/vpci.h    |  3 +-
 3 files changed, 62 insertions(+), 20 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index b09ccc5e6be6..c9519c804d97 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -90,6 +90,8 @@ static int cf_check map_range(
         if ( rc == 0 )
         {
             *c += size;
+            if ( map->map )
+                rc = rangeset_add_range(map->bar->unmap_mem, s, e);
             break;
         }
         if ( rc < 0 )
@@ -102,6 +104,13 @@ static int cf_check map_range(
         }
         ASSERT(rc < size);
         *c += rc;
+        if ( map->map )
+        {
+            int rc2 = rangeset_add_range(map->bar->unmap_mem, s, s + rc);
+
+            if ( rc2 )
+                return rc2;
+        }
         s += rc;
         if ( general_preempt_check() )
                 return -ERESTART;
@@ -185,12 +194,13 @@ static int map_bars(struct vpci_header *header, struct domain *d, bool map)
             .map = map,
             .bar = bar,
         };
+        struct rangeset *r = map ? bar->map_mem : bar->unmap_mem;
         int rc;
 
-        if ( rangeset_is_empty(bar->mem) )
+        if ( rangeset_is_empty(r) )
             continue;
 
-        rc = rangeset_consume_ranges(bar->mem, map_range, &data);
+        rc = rangeset_consume_ranges(r, map_range, &data);
 
         if ( rc )
             return rc;
@@ -248,8 +258,13 @@ bool vpci_process_pending(struct vcpu *v)
 
     /* Clean all the rangesets */
     for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-        if ( !rangeset_is_empty(header->bars[i].mem) )
-             rangeset_purge(header->bars[i].mem);
+    {
+        if ( !rangeset_is_empty(header->bars[i].map_mem) )
+             rangeset_purge(header->bars[i].map_mem);
+
+        if ( !rangeset_is_empty(header->bars[i].unmap_mem) )
+             rangeset_purge(header->bars[i].unmap_mem);
+    }
 
     v->vpci.pdev = NULL;
 
@@ -275,10 +290,10 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
         struct vpci_bar *bar = &header->bars[i];
         struct map_data data = { .d = d, .map = true, .bar = bar };
 
-        if ( rangeset_is_empty(bar->mem) )
+        if ( rangeset_is_empty(bar->map_mem) )
             continue;
 
-        while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
+        while ( (rc = rangeset_consume_ranges(bar->map_mem, map_range,
                                               &data)) == -ERESTART )
         {
             /*
@@ -329,6 +344,13 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 
     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 
+    if ( !(cmd & PCI_COMMAND_MEMORY) )
+    {
+        defer_map(pdev, cmd, rom_only);
+
+        return 0;
+    }
+
     /*
      * Create a rangeset per BAR that represents the current device memory
      * region and compare it against all the currently active BAR memory
@@ -349,7 +371,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         unsigned long start_guest = PFN_DOWN(bar->guest_addr);
         unsigned long end_guest = PFN_DOWN(bar->guest_addr + bar->size - 1);
 
-        if ( !bar->mem )
+        if ( !bar->map_mem || !bar->unmap_mem )
             continue;
 
         if ( !MAPPABLE_BAR(bar) ||
@@ -367,7 +389,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             continue;
         }
 
-        ASSERT(rangeset_is_empty(bar->mem));
+        ASSERT(rangeset_is_empty(bar->map_mem));
 
         /*
          * Make sure that the guest set address has the same page offset
@@ -382,7 +404,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             return -EINVAL;
         }
 
-        rc = rangeset_add_range(bar->mem, start_guest, end_guest);
+        rc = rangeset_add_range(bar->map_mem, start_guest, end_guest);
         if ( rc )
         {
             printk(XENLOG_G_WARNING "Failed to add [%lx, %lx]: %d\n",
@@ -395,10 +417,10 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         {
             struct vpci_bar *prev_bar = &header->bars[j];
 
-            if ( rangeset_is_empty(prev_bar->mem) )
+            if ( rangeset_is_empty(prev_bar->map_mem) )
                 continue;
 
-            rc = rangeset_remove_range(prev_bar->mem, start_guest, end_guest);
+            rc = rangeset_remove_range(prev_bar->map_mem, start_guest, end_guest);
             if ( rc )
             {
                 gprintk(XENLOG_WARNING,
@@ -408,7 +430,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             }
         }
 
-        rc = pci_sanitize_bar_memory(bar->mem);
+        rc = pci_sanitize_bar_memory(bar->map_mem);
         if ( rc )
         {
             gprintk(XENLOG_WARNING,
@@ -429,10 +451,10 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         {
             const struct vpci_bar *bar = &header->bars[j];
 
-            if ( rangeset_is_empty(bar->mem) )
+            if ( rangeset_is_empty(bar->map_mem) )
                 continue;
 
-            rc = rangeset_remove_range(bar->mem, start, end);
+            rc = rangeset_remove_range(bar->map_mem, start, end);
             if ( rc )
             {
                 gprintk(XENLOG_WARNING,
@@ -486,7 +508,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
                 {
                     const struct vpci_bar *bar = &header->bars[j];
 
-                    if ( !rangeset_overlaps_range(bar->mem, start, end) ||
+                    if ( !rangeset_overlaps_range(bar->map_mem, start, end) ||
                          /*
                           * If only the ROM enable bit is toggled check against
                           * other BARs in the same device for overlaps, but not
@@ -497,7 +519,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
                           bar->type == VPCI_BAR_ROM) )
                         continue;
 
-                    rc = rangeset_remove_range(bar->mem, start, end);
+                    rc = rangeset_remove_range(bar->map_mem, start, end);
                     if ( rc )
                     {
                         gprintk(XENLOG_WARNING,
@@ -752,12 +774,28 @@ static int bar_add_rangeset(const struct pci_dev *pdev, struct vpci_bar *bar,
                             unsigned int i)
 {
     char str[32];
+    int rc = 0;
 
     snprintf(str, sizeof(str), "%pp:BAR%u", &pdev->sbdf, i);
 
-    bar->mem = rangeset_new(pdev->domain, str, RANGESETF_no_print);
+    bar->map_mem = rangeset_new(pdev->domain, str, RANGESETF_no_print);
+    bar->unmap_mem = rangeset_new(pdev->domain, str, RANGESETF_no_print);
+
+    if ( !bar->map_mem )
+        rc = -ENOMEM;
+
+    if ( !bar->unmap_mem )
+        rc = -ENOMEM;
 
-    return !bar->mem ? -ENOMEM : 0;
+    if ( rc == -ENOMEM )
+    {
+        rangeset_destroy(bar->map_mem);
+        rangeset_destroy(bar->unmap_mem);
+        bar->map_mem = NULL;
+        bar->unmap_mem = NULL;
+    }
+
+    return rc;
 }
 
 static int cf_check init_header(struct pci_dev *pdev)
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index d2f0f97e0a04..c0198aa1b08d 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -118,7 +118,10 @@ void vpci_deassign_device(struct pci_dev *pdev)
     }
 
     for ( i = 0; i < ARRAY_SIZE(pdev->vpci->header.bars); i++ )
-        rangeset_destroy(pdev->vpci->header.bars[i].mem);
+    {
+        rangeset_destroy(pdev->vpci->header.bars[i].map_mem);
+        rangeset_destroy(pdev->vpci->header.bars[i].unmap_mem);
+    }
 
     xfree(pdev->vpci->msix);
     xfree(pdev->vpci->msi);
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 27eebdcef170..e74359848440 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -101,7 +101,8 @@ struct vpci {
             uint64_t guest_addr;
             uint64_t size;
             uint64_t resizable_sizes;
-            struct rangeset *mem;
+            struct rangeset *map_mem;
+            struct rangeset *unmap_mem;
             enum {
                 VPCI_BAR_EMPTY,
                 VPCI_BAR_IO,
-- 
2.49.0

Re: [PATCH v1 4/5] vpci: use separate rangeset for BAR unmapping

[Roger Pau Monné]

On Sat, May 31, 2025 at 08:54:02AM -0400, Stewart Hildebrand wrote:
> Introduce a new per-BAR rangeset, unmap_mem, for p2m unmapping. Rename
> existing mem rangeset to map_mem, which is now only used for mapping.
> Populate unmap_mem by moving just-mapped ranges from map_mem to
> unmap_mem. In modify_bars(), skip recalculating the ranges when
> unmapping as they are already stored in unmap_mem.

I'm afraid the copying of ranges from map_mem to unmap_mem is not fully
correct.

The calculations in modify_bars() take into account overlaps between
BARs.  Given the following set of actions:

1. Map dev#0 BAR0.
2. Cache mapped regions for dev#0 BAR0.
3. Map dev#1 BAR0 that overlaps with dev#0 BAR0.
4. Unmap dev#0 BAR0 using the cached regions.

The unmap of dev#0 BAR0 will also have the side-effect of unmapping
dev#1 BAR0, which is wrong.  That's why modify_bars() must be called
for every map and unmap operation.

Thanks, Roger.

Re: [PATCH v1 4/5] vpci: use separate rangeset for BAR unmapping

[Jan Beulich]

On 31.05.2025 14:54, Stewart Hildebrand wrote:
> Introduce a new per-BAR rangeset, unmap_mem, for p2m unmapping. Rename
> existing mem rangeset to map_mem, which is now only used for mapping.
> Populate unmap_mem by moving just-mapped ranges from map_mem to
> unmap_mem. In modify_bars(), skip recalculating the ranges when
> unmapping as they are already stored in unmap_mem.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Already when per-BAR rangsets were introduced I questioned that, resource
efficiency wise: A BAR fundamentally is a single range. For e.g. the MSI-X
table we punch a hole, but it then is still questionable if representing
the result as a rangeset is appropriate. Now you further extend the waste.
Since unmapping something that was never mapped should not be a problem,
does what needs unmapping really need representing as a rangeset, rather
than as a plain range?

Jan

[PATCH v1 5/5] vpci: allow 32-bit BAR writes with memory decoding enabled

[Stewart Hildebrand]

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 BAR without disabling memory decoding. Since
Xen refuses such writes, the BAR (and thus PCI device) will be
non-functional.

Currently the deferred mapping machinery supports only map or unmap
operations. Rework the deferred mapping machinery to support
unmap-then-map (VPCI_MOVE) operations.

Allow the hardware domain to issue 32-bit BAR writes with memory
decoding enabled, using the VPCI_MOVE operation to remap the BAR in p2m.

Take the opportunity to remove a stray newline in bar_write().

Resolves: https://gitlab.com/xen-project/xen/-/issues/197
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
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 | 86 +++++++++++++++++++++++++++------------
 xen/include/xen/vpci.h    |  5 +++
 2 files changed, 66 insertions(+), 25 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index c9519c804d97..f2ffad2ace32 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -214,7 +214,6 @@ bool vpci_process_pending(struct vcpu *v)
     const struct pci_dev *pdev = v->vpci.pdev;
     struct vpci_header *header = NULL;
     unsigned int i;
-    int rc;
 
     if ( !pdev )
         return false;
@@ -229,16 +228,34 @@ bool vpci_process_pending(struct vcpu *v)
     }
 
     header = &pdev->vpci->header;
-    rc = map_bars(header, v->domain, v->vpci.cmd & PCI_COMMAND_MEMORY);
 
-    if ( rc == -ERESTART )
+    if ( v->vpci.map_op == VPCI_UNMAP || v->vpci.map_op == VPCI_MOVE )
     {
-        read_unlock(&v->domain->pci_lock);
-        return true;
+        int rc = map_bars(header, v->domain, false);
+
+        if ( rc == -ERESTART )
+        {
+            read_unlock(&v->domain->pci_lock);
+            return true;
+        }
+
+        if ( rc )
+            goto fail;
     }
 
-    if ( rc )
-        goto fail;
+    if ( v->vpci.map_op == VPCI_MAP || v->vpci.map_op == VPCI_MOVE )
+    {
+        int rc = map_bars(header, v->domain, true);
+
+        if ( rc == -ERESTART )
+        {
+            read_unlock(&v->domain->pci_lock);
+            return true;
+        }
+
+        if ( rc )
+            goto fail;
+    }
 
     v->vpci.pdev = NULL;
 
@@ -312,7 +329,8 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
     return rc;
 }
 
-static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
+static void defer_map(const struct pci_dev *pdev, uint16_t cmd,
+                      enum vpci_map_op map_op, bool rom_only)
 {
     struct vcpu *curr = current;
 
@@ -324,6 +342,7 @@ static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
      */
     curr->vpci.pdev = pdev;
     curr->vpci.cmd = cmd;
+    curr->vpci.map_op = map_op;
     curr->vpci.rom_only = rom_only;
     /*
      * Raise a scheduler softirq in order to prevent the guest from resuming
@@ -333,7 +352,8 @@ static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
     raise_softirq(SCHEDULE_SOFTIRQ);
 }
 
-static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
+static int modify_bars(const struct pci_dev *pdev, uint16_t cmd,
+                       enum vpci_map_op map_op, bool rom_only)
 {
     struct vpci_header *header = &pdev->vpci->header;
     struct pci_dev *tmp;
@@ -344,9 +364,9 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 
     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 
-    if ( !(cmd & PCI_COMMAND_MEMORY) )
+    if ( map_op == VPCI_UNMAP )
     {
-        defer_map(pdev, cmd, rom_only);
+        defer_map(pdev, cmd, map_op, rom_only);
 
         return 0;
     }
@@ -378,7 +398,8 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
              (rom_only ? bar->type != VPCI_BAR_ROM
                        : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) ||
              /* Skip BARs already in the requested state. */
-             bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
+             (bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) &&
+              map_op != VPCI_MOVE) )
             continue;
 
         if ( !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
@@ -551,7 +572,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         return apply_map(pdev->domain, pdev, cmd);
     }
 
-    defer_map(pdev, cmd, rom_only);
+    defer_map(pdev, cmd, map_op, rom_only);
 
     return 0;
 }
@@ -584,7 +605,8 @@ static void cf_check cmd_write(
          * 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);
+        modify_bars(pdev, cmd, cmd & PCI_COMMAND_MEMORY ? VPCI_MAP : VPCI_UNMAP,
+                    false);
     else
         pci_conf_write16(pdev->sbdf, reg, cmd);
 }
@@ -615,20 +637,27 @@ 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, but skip unnecessary p2m operations when mapped.
      */
     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;
+        }
+        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
      * Xen can map the BAR into the guest p2m.
@@ -647,6 +676,10 @@ static void cf_check bar_write(
     }
 
     pci_conf_write32(pdev->sbdf, reg, val);
+
+    if ( bar->enabled )
+        modify_bars(pdev, pci_conf_read16(pdev->sbdf, PCI_COMMAND), VPCI_MOVE,
+                    false);
 }
 
 static void cf_check guest_mem_bar_write(const struct pci_dev *pdev,
@@ -752,7 +785,8 @@ static void cf_check rom_write(
      * 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) )
+    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0,
+                          new_enabled ? VPCI_MAP : VPCI_UNMAP, 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
@@ -1054,7 +1088,9 @@ static int cf_check init_header(struct pci_dev *pdev)
             goto fail;
     }
 
-    return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;
+    return (cmd & PCI_COMMAND_MEMORY)
+           ? modify_bars(pdev, cmd, VPCI_MAP, false)
+           : 0;
 
  fail:
     pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index e74359848440..2ddfb147e7b7 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -197,6 +197,11 @@ struct vpci_vcpu {
     /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
     const struct pci_dev *pdev;
     uint16_t cmd;
+    enum vpci_map_op {
+        VPCI_MAP,
+        VPCI_UNMAP,
+        VPCI_MOVE,
+    } map_op;
     bool rom_only : 1;
 };
 
-- 
2.49.0

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

[Jan Beulich]

On 31.05.2025 14:54, 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 BAR without disabling memory decoding. Since
> Xen refuses such writes, the BAR (and thus PCI device) will be
> non-functional.

Doing this for 32-bit BARs only, with not even an outline what to do about
the same issue with 64-bit ones, seems like it won't buy us very much.

Jan

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

[Stewart Hildebrand]

On 6/5/25 06:41, Jan Beulich wrote:
> On 31.05.2025 14:54, 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 BAR without disabling memory decoding. Since
>> Xen refuses such writes, the BAR (and thus PCI device) will be
>> non-functional.
> 
> Doing this for 32-bit BARs only, with not even an outline what to do about
> the same issue with 64-bit ones, seems like it won't buy us very much.

It buys us quite a lot: it means the difference between booting vs.
booting with degraded functionality or not booting at all with PVH dom0
on some platforms with certain PCI devices plugged in.

The plan for 64-bit BARs for now is to continue to refuse the write(s)
when the 64-bit BAR is mapped to avoid mapping half-updated BARs in p2m.

I'll add something to this effect to the commit message.

Also see https://gitlab.com/xen-project/xen/-/issues/197

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

[Jan Beulich]

On 11.06.2025 22:22, Stewart Hildebrand wrote:
> On 6/5/25 06:41, Jan Beulich wrote:
>> On 31.05.2025 14:54, 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 BAR without disabling memory decoding. Since
>>> Xen refuses such writes, the BAR (and thus PCI device) will be
>>> non-functional.
>>
>> Doing this for 32-bit BARs only, with not even an outline what to do about
>> the same issue with 64-bit ones, seems like it won't buy us very much.
> 
> It buys us quite a lot: it means the difference between booting vs.
> booting with degraded functionality or not booting at all with PVH dom0
> on some platforms with certain PCI devices plugged in.
> 
> The plan for 64-bit BARs for now is to continue to refuse the write(s)
> when the 64-bit BAR is mapped to avoid mapping half-updated BARs in p2m.
> 
> I'll add something to this effect to the commit message.

Yes please, in particular ...

> Also see https://gitlab.com/xen-project/xen/-/issues/197

... to make clear that Linux indeed aims at disabling memory decode when
fiddling with 64-bit BARs. That reduces the set of remaining cases quite a
bit.

Jan

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

[Roger Pau Monné]

On Thu, Jun 05, 2025 at 12:41:06PM +0200, Jan Beulich wrote:
> On 31.05.2025 14:54, 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 BAR without disabling memory decoding. Since
> > Xen refuses such writes, the BAR (and thus PCI device) will be
> > non-functional.
> 
> Doing this for 32-bit BARs only, with not even an outline what to do about
> the same issue with 64-bit ones, seems like it won't buy us very much.

IIRC Linux will disable decoding in the common case when updating the
position of a 64bit BAR.  However it won't disable decoding for 32bit
BARs.  I think that's why Stewart cares more about the 32bit case than
the 64bit one.

Regards, Roger.