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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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@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.

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.
>