Re: [PATCH v3 3/4] xen/vpci: header: filter PCI capabilities

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.08.2023 03:29, 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>
> ---
> v2->v3:
> * get rid of > 0 in loop condition
> * implement pci_find_next_cap in terms of new pci_find_next_cap_ttl function 
> so
>   that hypothetical future callers wouldn't be required to pass &ttl.
> * change NULL to (void *)0 for RAZ value passed to vpci_read_val
> * change type of ttl to unsigned int
> * remember to mask off the low 2 bits of next in the initial loop iteration
> * change return type of pci_find_next_cap and pci_find_next_cap_ttl
> * avoid wrapping the PCI_STATUS_CAP_LIST condition by using ! instead of == 0

Looks mostly okay to me now, just two things (pointed out before):

> --- a/xen/drivers/pci/pci.c
> +++ b/xen/drivers/pci/pci.c
> @@ -39,30 +39,38 @@ int pci_find_cap_offset(pci_sbdf_t sbdf, u8 cap)
>      return 0;
>  }
>  
> -int pci_find_next_cap(pci_sbdf_t sbdf, u8 pos, int cap)
> +uint8_t pci_find_next_cap_ttl(pci_sbdf_t sbdf, uint8_t pos,
> +                              bool (*is_match)(uint8_t), unsigned int *ttl)
>  {
> -    u8 id;
> -    int ttl = 48;
> +    uint8_t 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) )
>              return pos;
>  
> -        pos += PCI_CAP_LIST_NEXT;
> +        pos = (pos & ~3) + PCI_CAP_LIST_NEXT;
>      }
> +
>      return 0;
>  }
>  
> +uint8_t pci_find_next_cap(pci_sbdf_t sbdf, uint8_t pos,
> +                          bool (*is_match)(uint8_t))
> +{
> +    unsigned int ttl = 48;
> +
> +    return pci_find_next_cap_ttl(sbdf, pos, is_match, &ttl) & ~3;
> +}

You still change the original function's signature. In this patch, the
prototype for it should not need touching at all.

The other is the imo excessive use of fixed width types. "pos" has no
business being uint8_t (but that'll be taken care of in the earlier
patch for the case here), and similarly e.g. id doesn't need to be (in
the earlier function). But I can see that at least some of the cases
here are on the edge ...

Jan