Re: [PATCH v1 5/8] xen/pci: introduce has_vpci_bridge

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2025 09:59, Mykyta Poturai wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> 
> has_vpci_bridge is a macro to check if the domain is a domU or is dom0
> with vPCI (pci-scan=yes) enabled.

Hmm. Why would DomU-s, now and forever, not have (virtual) bridges? Wasn't them
gaining (virtual) bridges actually the longer-term plan?

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -230,7 +230,7 @@ bool vpci_process_pending(struct vcpu *v)
>  
>              read_unlock(&v->domain->pci_lock);
>  
> -            if ( !is_hardware_domain(v->domain) )
> +            if ( has_vpci_bridge(v->domain) )
>                  domain_crash(v->domain);
>  
>              return false;
> @@ -492,7 +492,7 @@ static int modify_bars(const struct pci_dev *pdev, 
> uint16_t cmd, bool rom_only)
>              }
>          }
>  
> -        if ( !is_hardware_domain(d) )
> +        if ( has_vpci_bridge(d) )
>              break;
>  
>          d = dom_xen;
> @@ -522,7 +522,7 @@ static void cf_check cmd_write(
>  {
>      struct vpci_header *header = data;
>  
> -    if ( !is_hardware_domain(pdev->domain) )
> +    if ( has_vpci_bridge(pdev->domain) )
>      {
>          const struct vpci *vpci = pdev->vpci;
>  
> @@ -564,7 +564,7 @@ static void cf_check bar_write(
>      struct vpci_bar *bar = data;
>      bool hi = false;
>  
> -    ASSERT(is_hardware_domain(pdev->domain));
> +    ASSERT(!has_vpci_bridge(pdev->domain));
>  
>      if ( bar->type == VPCI_BAR_MEM64_HI )
>      {
> @@ -747,7 +747,7 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
>  {
>      int rc;
>      bool mask_cap_list = false;
> -    bool is_hwdom = is_hardware_domain(pdev->domain);
> +    bool is_hwdom = !has_vpci_bridge(pdev->domain);
>  
>      if ( pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST )
>      {
> @@ -829,7 +829,7 @@ static int vpci_init_ext_capability_list(const struct 
> pci_dev *pdev)
>  {
>      unsigned int pos = PCI_CFG_SPACE_SIZE;
>  
> -    if ( !is_hardware_domain(pdev->domain) )
> +    if ( has_vpci_bridge(pdev->domain) )
>          /* Extended capabilities read as zero, write ignore for DomU */
>          return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>                                   pos, 4, (void *)0);
> @@ -866,7 +866,7 @@ int vpci_init_header(struct pci_dev *pdev)
>      struct vpci_header *header = &pdev->vpci->header;
>      struct vpci_bar *bars = header->bars;
>      int rc;
> -    bool is_hwdom = is_hardware_domain(pdev->domain);
> +    bool is_hwdom = !has_vpci_bridge(pdev->domain);
>  
>      ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));

For none of the changes (also further ones) it is clear (to me) why the
substitution is (logically) correct. For this last instance the variable
name also ends up wrong after the replacement.

> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -339,6 +339,15 @@ static inline int __must_check vpci_reset_device(struct 
> pci_dev *pdev)
>      return vpci_assign_device(pdev);
>  }
>  
> +#ifdef CONFIG_ARM
> +#include <asm/pci.h>
> +
> +#define has_vpci_bridge(d) (!is_hardware_domain(d) || \
> +                           (is_hardware_domain(d) && pci_scan_enabled))

Nit: Off-by-1 indentation.

Jan