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

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

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>

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.