Re: [PATCH v4 6/6] xen/vpci: header: filter PCI capabilities
On 8/31/23 08:11, Jan Beulich wrote:
> 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
>
> Reviewed-by: Jan Beulich >
> 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, ) & ~3;
>
> ... passing NULL here and then suitably handling the case in that
> common helper.
Thinking in terms of reducing the amount of dead code, I'll change it
>> @@ -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, );
>> +
>> +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 ...
As ugly as that double cast is, I think I prefer the next and pos declarations
have consistent types (which I had intended to be unsigned int to match the
prior patches, not uint8_t). As well as not making the MISRA situation any
worse. The casts, after all, make it excruciatingly obvious what we're doing
here, I hope.
Stew
Re: [PATCH v4 6/6] xen/vpci: header: filter PCI capabilities
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
Reviewed-by: Jan Beulich
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, ) & ~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, );
> +
> +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
Re: [PATCH v4 6/6] xen/vpci: header: filter PCI capabilities
On 8/28/23 13:56, Stewart Hildebrand wrote:
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 4a4dbb69ab1c..919addbfa630 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -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;
s/uint8_t/unsigned int/
> +unsigned int ttl = 48;
> +
> +next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST,
> + vpci_cap_supported, 0, );
> +
> +rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> + PCI_CAPABILITY_LIST, 1,
> + (void *)(uintptr_t)next);
> +if ( rc )
> +return rc;
> +
> +next &= ~3;
> +
> +if ( !next )
> +/*
> + * If we don't have any supported capabilities to expose to
> the
> + * guest, mask the PCI_STATUS_CAP_LIST bit in the status
> + * register.
> + */
> +header->mask_cap_list = true;
> +
> +while ( next && ttl )
> +{
> +uint8_t pos = next;
s/uint8_t/unsigned int/
