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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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@xxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

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.