Re: [PATCH 5/5] xen/vpci: only check BAR validity once

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2025 18:03, Roger Pau Monne wrote:
> Avoid multiple calls to pci_check_bar() for the same memory decoding
> related operation, as each call can possibly print a warning message avoid
> a BAR being in an invalid position.

s/avoid/about/ ?

> @@ -217,13 +179,13 @@ bool vpci_process_pending(struct vcpu *v)
>           * rangeset_consume_ranges() itself doesn't generate any errors.
>           */
>          rangeset_purge(bar->mem);
> +
> + next:
> +        if ( bar->valid )
> +            bar->enabled = v->vpci.cmd & PCI_COMMAND_MEMORY;

Isn't it at least latently risky to possibly leave ->enabled set to true
when ->valid is false?

> @@ -243,10 +205,8 @@ 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) )
> -            continue;
> -
> -        while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
> +        while ( bar->mem &&
> +                (rc = rangeset_consume_ranges(bar->mem, map_range,
>                                                &data)) == -ERESTART )
>          {
>              /*
> @@ -258,9 +218,10 @@ static int __init apply_map(struct domain *d, const 
> struct pci_dev *pdev,
>              process_pending_softirqs();
>              write_lock(&d->pci_lock);
>          }
> +
> +        if ( bar->valid )
> +            bar->enabled = true;
>      }
> -    if ( !rc )
> -        modify_decoding(pdev, cmd, false);
>  
>      return rc;
>  }
> @@ -326,6 +287,9 @@ static int modify_bars(const struct pci_dev *pdev, 
> uint16_t cmd, bool rom_only)
>           */
>          rangeset_purge(bar->mem);
>  
> +        /* Reset whether the BAR is valid, will be checked below. */
> +        bar->valid = false;

Just that I can't spot any check further down. It's only ...

> @@ -341,6 +305,8 @@ static int modify_bars(const struct pci_dev *pdev, 
> uint16_t cmd, bool rom_only)
>              continue;
>          }
>  
> +        bar->valid = true;

... this setting to true.

> @@ -539,6 +505,7 @@ static void cf_check cmd_write(
>      if ( (cmd & PCI_COMMAND_MEMORY) && vpci_make_msix_hole(pdev) )
>          return;
>  #endif
> +
>      /*
>       * FIXME: for domUs we don't want the guest toggling the memory decoding
>       * bit.  It should be set in vpci_init_header() and guest attempts to

This change was probably meant to go into an earlier patch?

Jan