Re: [PATCH v4 6/6] xen/vpci: header: filter PCI capabilities

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.08.2023 19:56, Stewart Hildebrand wrote:
> Currently, Xen vPCI only supports virtualizing the MSI and MSI-X capabilities.
> Hide all other PCI capabilities (including extended capabilities) from domUs 
> for
> now, even though there may be certain devices/drivers that depend on being 
> able
> to discover certain capabilities.
> 
> We parse the physical PCI capabilities linked list and add vPCI register
> handlers for the next elements, inserting our own next value, thus presenting 
> a
> modified linked list to the domU.
> 
> Introduce helper functions vpci_hw_read8 and vpci_read_val. The vpci_read_val
> helper function returns a fixed value, which may be used for RAZ registers, or
> registers whose value doesn't change.
> 
> Introduce pci_find_next_cap_ttl() helper while adapting the logic from
> pci_find_next_cap() to suit our needs, and implement the existing
> pci_find_next_cap() in terms of the new helper.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Nevertheless a couple of remarks:

> --- a/xen/drivers/pci/pci.c
> +++ b/xen/drivers/pci/pci.c
> @@ -39,31 +39,44 @@ unsigned int pci_find_cap_offset(pci_sbdf_t sbdf, 
> unsigned int cap)
>      return 0;
>  }
>  
> -unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos,
> -                               unsigned int cap)
> +unsigned int pci_find_next_cap_ttl(pci_sbdf_t sbdf, unsigned int pos,
> +                                   bool (*is_match)(unsigned int, unsigned 
> int),
> +                                   unsigned int userdata, unsigned int *ttl)
>  {
> -    u8 id;
> -    int ttl = 48;
> +    unsigned int id;
>  
> -    while ( ttl-- )
> +    while ( (*ttl)-- )
>      {
>          pos = pci_conf_read8(sbdf, pos);
>          if ( pos < 0x40 )
>              break;
>  
> -        pos &= ~3;
> -        id = pci_conf_read8(sbdf, 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, userdata) )
>              return pos;
>  
> -        pos += PCI_CAP_LIST_NEXT;
> +        pos = (pos & ~3) + PCI_CAP_LIST_NEXT;
>      }
> +
>      return 0;
>  }
>  
> +static bool cf_check is_cap_match(unsigned int id1, unsigned int id2)
> +{
> +    return id1 == id2;
> +}

Personally I would have preferred to get away without yet another hook
function here, by ...

> +unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos,
> +                               unsigned int cap)
> +{
> +    unsigned int ttl = 48;
> +
> +    return pci_find_next_cap_ttl(sbdf, pos, is_cap_match, cap, &ttl) & ~3;

... passing NULL here and then suitably handling the case in that
common helper.

> @@ -561,6 +573,71 @@ 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) )
> +        {
> +            /* RAZ/WI */
> +            rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> +                                   PCI_CAPABILITY_LIST, 1, (void *)0);
> +            if ( rc )
> +                return rc;
> +        }
> +        else
> +        {
> +            /* Only expose capabilities to the guest that vPCI can handle. */
> +            uint8_t next;

If this was "unsigned long", ...

> +            unsigned int ttl = 48;
> +
> +            next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST,
> +                                         vpci_cap_supported, 0, &ttl);
> +
> +            rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> +                                   PCI_CAPABILITY_LIST, 1,
> +                                   (void *)(uintptr_t)next);

... you'd avoid the need for the double cast here and again below. Yet
then I realize that Misra would take offence at us doing so ...

Jan