1. Patchew
  2. Xen
  3. [PATCH v1 0/5] vpci: allow 32-bit BAR writes with memory decoding enabled
  4. View patch

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

Stewart Hildebrand posted 5 patches 5 months ago
There is a newer version of this series
[PATCH v1 1/5] vpci: const-ify some pdev instances
Posted by Stewart Hildebrand 5 months ago 
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
Posted by Roger Pau Monné 4 months, 4 weeks ago 
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
Posted by Stewart Hildebrand 4 months, 3 weeks ago 
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
Posted by Stewart Hildebrand 3 months, 3 weeks ago 
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.
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.