Re: [PATCH v2 1/2] xen/vpci: header: filter PCI capabilities

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2023 20:50, Stewart Hildebrand wrote:
> If there are no capabilities to be exposed to the guest, a future status
> register handler likely would want to mask the PCI_STATUS_CAP_LIST bit. See 
> [1]
> for a suggestion of how this might be tracked in struct vpci_header.

Can we actually get away without doing this right away? I'm not sure
consumers are required to range check what they read from PCI_CAPABILITY_LIST.

> --- a/xen/drivers/pci/pci.c
> +++ b/xen/drivers/pci/pci.c
> @@ -39,27 +39,27 @@ int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 func, 
> u8 cap)
>      return 0;
>  }
>  
> -int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap)
> +int pci_find_next_cap(pci_sbdf_t sbdf, uint8_t pos, bool 
> (*is_match)(uint8_t),
> +                      int *ttl)

Why plain int? When values can't go negative, respective variables generally
want to be of unsigned types.

>  {
> -    u8 id;
> -    int ttl = 48;
> +    uint8_t id;
>  
> -    while ( ttl-- )
> +    while ( (*ttl)-- > 0 )

I don't see why you add "> 0" here.

>      {
> -        pos = pci_conf_read8(PCI_SBDF(seg, bus, devfn), pos);
> +        pos = pci_conf_read8(sbdf, pos);
>          if ( pos < 0x40 )
>              break;
>  
> -        pos &= ~3;
> -        id = pci_conf_read8(PCI_SBDF(seg, bus, devfn), pos + 
> PCI_CAP_LIST_ID);
> +        id = pci_conf_read8(sbdf, (pos & ~3) + PCI_CAP_LIST_ID);
>  
>          if ( id == 0xff )
>              break;
> -        if ( id == cap )
> +        if ( is_match(id) )
>              return pos;
>  
> -        pos += PCI_CAP_LIST_NEXT;
> +        pos = (pos & ~3) + PCI_CAP_LIST_NEXT;
>      }
> +
>      return 0;
>  }

As said in context of v1, this function should remain usable for its
original purpose. That, to me, includes the caller not needing to care about
ttl. I could see you convert the original function the way you do, but under
a new name, and then implement the original one simply in terms of this more
general purpose function.

Also, while I appreciate the sbdf conversion, that wants to be a separate
patch, which would then want to take care of the sibling functions as well.

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -513,6 +513,18 @@ static void cf_check rom_write(
>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>  }
>  
> +static bool cf_check vpci_cap_supported(uint8_t id)
> +{
> +    switch ( id )
> +    {
> +    case PCI_CAP_ID_MSI:
> +    case PCI_CAP_ID_MSIX:
> +        return true;
> +    default:
> +        return false;
> +    }
> +}
> +
>  static int cf_check init_bars(struct pci_dev *pdev)
>  {
>      uint16_t cmd;
> @@ -544,6 +556,60 @@ static int cf_check init_bars(struct pci_dev *pdev)
>      if ( rc )
>          return rc;
>  
> +    if ( !is_hardware_domain(pdev->domain) )
> +    {
> +        if ( (pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST)
> +             == 0 )

This fits on a single line when written this more commonly used way:

        if ( !(pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST) )

Otherwise it needs wrapping differently - binary operators at a wrapping
point belong on the earlier line in our style.

> +        {
> +            /* RAZ/WI */
> +            rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> +                                   PCI_CAPABILITY_LIST, 1, NULL);

This last NULL is likely misleading to readers: It does not obviously
represent the value 0 cast to void *. (Same then for the extended cap
handler at the end.)

> +            if ( rc )
> +                return rc;
> +        }
> +        else
> +        {
> +            /* Only expose capabilities to the guest that vPCI can handle. */
> +            uint8_t next;
> +            int ttl = 48;
> +
> +            next = pci_find_next_cap(pdev->sbdf, PCI_CAPABILITY_LIST,
> +                                     vpci_cap_supported, &ttl);
> +
> +            rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> +                                   PCI_CAPABILITY_LIST, 1,
> +                                   (void *)(uintptr_t)next);
> +            if ( rc )
> +                return rc;
> +
> +            while ( next && (ttl > 0) )

Don't you need to mask off the low two bits first (rather than [only] ...

> +            {
> +                uint8_t pos = next;
> +
> +                next = pci_find_next_cap(pdev->sbdf, pos + PCI_CAP_LIST_NEXT,
> +                                         vpci_cap_supported, &ttl);
> +
> +                rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
> +                                       pos + PCI_CAP_LIST_ID, 1, NULL);
> +                if ( rc )
> +                    return rc;
> +
> +                rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> +                                       pos + PCI_CAP_LIST_NEXT, 1,
> +                                       (void *)(uintptr_t)next);
> +                if ( rc )
> +                    return rc;
> +
> +                next &= ~3;

... here)?

Jan