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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.08.2023 21:12, Stewart Hildebrand wrote:
> Xen vPCI only supports virtualizing the MSI and MSI-X capabilities, so all 
> other
> PCI capabilities should be hidden from a domU for now.

I'm not sure about "should"; imo this would need evaluating for every cap
type separately. I'm okay though to take this as a starting point. What
needs considering (and mentioning here) is that there may be (iirc: are)
drivers which depend on being able to find/access certain capabilities.

Also - what about extended capabilities? Don't we want to hide them all
then as well (by returning 0 for the 32-bit value at 0x100)?

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -513,6 +513,36 @@ static void cf_check rom_write(
>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>  }
>  
> +static uint8_t vpci_find_next_cap(pci_sbdf_t sbdf, uint8_t pos)
> +{
> +    uint8_t id;
> +    int ttl;
> +
> +    if ( pos < 0x40 )
> +        pos = pci_conf_read8(sbdf, PCI_CAPABILITY_LIST);
> +    else
> +        pos = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_NEXT);

How about avoiding the if/else by having the caller pass in a useful
value, rather than PCI_CAPABILITY_LIST? I.e.

#define PCI_CAP_LIST_FIRST (PCI_CAPABILITY_LIST - PCI_CAP_LIST_NEXT)

> +    for ( ttl = 48; ttl > 0; ttl-- )
> +    {
> +        if ( pos < 0x40 )
> +            break;
> +
> +        pos &= ~3;
> +        id = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_ID);
> +
> +        if ( id == 0xff )
> +            break;
> +
> +        if ( id == PCI_CAP_ID_MSI ||
> +             id == PCI_CAP_ID_MSIX )
> +            return pos;

Can this please start out as switch() right away?

> +        pos = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_NEXT);
> +    }
> +    return 0;
> +}

Nit: Blank line please ahead of main function return point.

I also notice that the function isn't really vPCI-specific in any way
(except for the specific PCI_CAP_ID_* compared against). Would it
perhaps make sense to have it be a general utility function, living in
xen/drivers/pci/pci.c next to its relatives?

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

In vpci_find_next_cap() the low 2 bits were masked off. While reserved
at present, I wonder whether we wouldn't be better off passing them
"through".

> +            if ( rc )
> +                return rc;
> +
> +            for ( ttl = 48; ttl > 0; ttl-- )

vpci_find_next_cap() already bounds its loops; you effectively allow for
48*48 iterations here. It would seem better if the ttl applied globally,
and would hence want to be an in/out for vpci_find_next_cap().

Also note that according to ./CODING_STYLE ttl shouldn't be uint<N>_t
(this likely extends to other uses of such types here), and plain int
also isn't really appropriate for a value which can't go negative.

Jan