When vpci fails to initialize a extended capability of device for dom0,
it just return error instead of catching and processing exception. That
makes the entire device unusable.

So, add new a function to hide extended capability when initialization
fails. And remove the failed extended capability handler from vpci
extended capability list.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v2->v3 changes:
* Separated from the last version patch "vpci: Hide capability when it fails to initialize".
* Whole implementation changed because last version is wrong.
  This version gets target handler and previous handler from vpci->handlers, then remove the target.
* Note: a case in function vpci_ext_capability_mask() needs to be discussed,
  because it may change the offset of next capability when the offset of target
  capability is 0x100U(the first extended capability), my implementation is just to
  ignore and let hardware to handle the target capability.

v1->v2 changes:
* Removed the "priorities" of initializing capabilities since it isn't used anymore.
* Added new function vpci_capability_mask() and vpci_ext_capability_mask() to
  remove failed capability from list.
* Called vpci_make_msix_hole() in the end of init_msix().

Best regards,
Jiqian Chen.
---
 xen/drivers/vpci/vpci.c    | 79 ++++++++++++++++++++++++++++++++++++++
 xen/include/xen/pci_regs.h |  1 +
 2 files changed, 80 insertions(+)

diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index f97c7cc460a0..8ff5169bdd18 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -183,6 +183,83 @@ static void vpci_capability_mask(struct pci_dev *pdev,
     xfree(next_r);
 }
 
+static struct vpci_register *vpci_get_previous_ext_cap_register
+                (struct vpci *vpci, const unsigned int offset)
+{
+    uint32_t header;
+    unsigned int pos = PCI_CFG_SPACE_SIZE;
+    struct vpci_register *r;
+
+    if ( offset <= PCI_CFG_SPACE_SIZE )
+        return NULL;
+
+    r = vpci_get_register(vpci, pos, 4);
+    ASSERT(r);
+
+    header = (uint32_t)(uintptr_t)r->private;
+    pos = PCI_EXT_CAP_NEXT(header);
+    while ( pos > PCI_CFG_SPACE_SIZE && pos != offset )
+    {
+        r = vpci_get_register(vpci, pos, 4);
+        ASSERT(r);
+        header = (uint32_t)(uintptr_t)r->private;
+        pos = PCI_EXT_CAP_NEXT(header);
+    }
+
+    if ( pos <= PCI_CFG_SPACE_SIZE )
+        return NULL;
+
+    return r;
+}
+
+static void vpci_ext_capability_mask(struct pci_dev *pdev,
+                                     const unsigned int cap)
+{
+    const unsigned int offset = pci_find_ext_capability(pdev->sbdf, cap);
+    struct vpci_register *rm, *prev_r;
+    struct vpci *vpci = pdev->vpci;
+    uint32_t header, pre_header;
+
+    spin_lock(&vpci->lock);
+    rm = vpci_get_register(vpci, offset, 4);
+    if ( !rm )
+    {
+        spin_unlock(&vpci->lock);
+        return;
+    }
+
+    header = (uint32_t)(uintptr_t)rm->private;
+    if ( offset == PCI_CFG_SPACE_SIZE )
+    {
+        if ( PCI_EXT_CAP_NEXT(header) <= PCI_CFG_SPACE_SIZE )
+            rm->private = (void *)(uintptr_t)0;
+        else
+            /*
+             * Else case needs to remove the capability in position 0x100U and
+             * moves the next capability to be in position 0x100U, that would
+             * cause the offset of next capability in vpci different from the
+             * hardware, then cause error accesses, so just ignore it here and
+             * hope hardware would handle the capability well.
+            */
+            printk(XENLOG_ERR "%pd %pp: ext cap %u is first cap, can't mask it\n",
+                   pdev->domain, &pdev->sbdf, cap);
+        spin_unlock(&vpci->lock);
+        return;
+    }
+
+    prev_r = vpci_get_previous_ext_cap_register(vpci, offset);
+    ASSERT(prev_r);
+
+    pre_header = (uint32_t)(uintptr_t)prev_r->private;
+    prev_r->private = (void *)(uintptr_t)((pre_header &
+                                           ~PCI_EXT_CAP_NEXT_MASK) |
+                                          (header & PCI_EXT_CAP_NEXT_MASK));
+
+    list_del(&rm->node);
+    spin_unlock(&vpci->lock);
+    xfree(rm);
+}
+
 static void vpci_init_capabilities(struct pci_dev *pdev)
 {
     for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ )
@@ -216,6 +293,8 @@ static void vpci_init_capabilities(struct pci_dev *pdev)
                    is_ext ? "extended" : "legacy", cap, rc);
             if ( !is_ext )
                 vpci_capability_mask(pdev, cap);
+            else
+                vpci_ext_capability_mask(pdev, cap);
         }
     }
 }
diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
index 27b4f44eedf3..5fe6653fded4 100644
--- a/xen/include/xen/pci_regs.h
+++ b/xen/include/xen/pci_regs.h
@@ -449,6 +449,7 @@
 #define PCI_EXT_CAP_ID(header)		((header) & 0x0000ffff)
 #define PCI_EXT_CAP_VER(header)		(((header) >> 16) & 0xf)
 #define PCI_EXT_CAP_NEXT(header)	(((header) >> 20) & 0xffc)
+#define PCI_EXT_CAP_NEXT_MASK		0xFFC00000U
 
 #define PCI_EXT_CAP_ID_ERR	1
 #define PCI_EXT_CAP_ID_VC	2
-- 
2.34.1

On Mon, Apr 21, 2025 at 02:18:59PM +0800, Jiqian Chen wrote:
> When vpci fails to initialize a extended capability of device for dom0,
> it just return error instead of catching and processing exception. That
> makes the entire device unusable.
> 
> So, add new a function to hide extended capability when initialization
> fails. And remove the failed extended capability handler from vpci
> extended capability list.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> v2->v3 changes:
> * Separated from the last version patch "vpci: Hide capability when it fails to initialize".
> * Whole implementation changed because last version is wrong.
>   This version gets target handler and previous handler from vpci->handlers, then remove the target.
> * Note: a case in function vpci_ext_capability_mask() needs to be discussed,
>   because it may change the offset of next capability when the offset of target
>   capability is 0x100U(the first extended capability), my implementation is just to
>   ignore and let hardware to handle the target capability.
> 
> v1->v2 changes:
> * Removed the "priorities" of initializing capabilities since it isn't used anymore.
> * Added new function vpci_capability_mask() and vpci_ext_capability_mask() to
>   remove failed capability from list.
> * Called vpci_make_msix_hole() in the end of init_msix().
> 
> Best regards,
> Jiqian Chen.
> ---
>  xen/drivers/vpci/vpci.c    | 79 ++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/pci_regs.h |  1 +
>  2 files changed, 80 insertions(+)
> 
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index f97c7cc460a0..8ff5169bdd18 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -183,6 +183,83 @@ static void vpci_capability_mask(struct pci_dev *pdev,
>      xfree(next_r);
>  }
>  
> +static struct vpci_register *vpci_get_previous_ext_cap_register
> +                (struct vpci *vpci, const unsigned int offset)
> +{
> +    uint32_t header;
> +    unsigned int pos = PCI_CFG_SPACE_SIZE;
> +    struct vpci_register *r;
> +
> +    if ( offset <= PCI_CFG_SPACE_SIZE )
> +        return NULL;
> +
> +    r = vpci_get_register(vpci, pos, 4);
> +    ASSERT(r);
> +
> +    header = (uint32_t)(uintptr_t)r->private;
> +    pos = PCI_EXT_CAP_NEXT(header);
> +    while ( pos > PCI_CFG_SPACE_SIZE && pos != offset )
> +    {
> +        r = vpci_get_register(vpci, pos, 4);
> +        ASSERT(r);
> +        header = (uint32_t)(uintptr_t)r->private;
> +        pos = PCI_EXT_CAP_NEXT(header);
> +    }
> +
> +    if ( pos <= PCI_CFG_SPACE_SIZE )
> +        return NULL;
> +
> +    return r;
> +}
> +
> +static void vpci_ext_capability_mask(struct pci_dev *pdev,
> +                                     const unsigned int cap)
> +{
> +    const unsigned int offset = pci_find_ext_capability(pdev->sbdf, cap);
> +    struct vpci_register *rm, *prev_r;
> +    struct vpci *vpci = pdev->vpci;
> +    uint32_t header, pre_header;

Maybe sanity check that offset is correct?

> +    spin_lock(&vpci->lock);
> +    rm = vpci_get_register(vpci, offset, 4);
> +    if ( !rm )
> +    {
> +        spin_unlock(&vpci->lock);
> +        return;
> +    }
> +
> +    header = (uint32_t)(uintptr_t)rm->private;
> +    if ( offset == PCI_CFG_SPACE_SIZE )
> +    {
> +        if ( PCI_EXT_CAP_NEXT(header) <= PCI_CFG_SPACE_SIZE )
> +            rm->private = (void *)(uintptr_t)0;
> +        else
> +            /*
> +             * Else case needs to remove the capability in position 0x100U and
> +             * moves the next capability to be in position 0x100U, that would
> +             * cause the offset of next capability in vpci different from the
> +             * hardware, then cause error accesses, so just ignore it here and
> +             * hope hardware would handle the capability well.
> +            */
> +            printk(XENLOG_ERR "%pd %pp: ext cap %u is first cap, can't mask it\n",
> +                   pdev->domain, &pdev->sbdf, cap);

In this case, could you maybe replace just the capability ID part of
the header to return 0?  That will likely cause the OS to continue
scanning the list, while ID 0 won't match which any known
capability.

Thanks, Roger.

On 2025/5/7 00:21, Roger Pau Monné wrote:
> On Mon, Apr 21, 2025 at 02:18:59PM +0800, Jiqian Chen wrote:
>> When vpci fails to initialize a extended capability of device for dom0,
>> it just return error instead of catching and processing exception. That
>> makes the entire device unusable.
>>
>> So, add new a function to hide extended capability when initialization
>> fails. And remove the failed extended capability handler from vpci
>> extended capability list.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
>> ---
>> v2->v3 changes:
>> * Separated from the last version patch "vpci: Hide capability when it fails to initialize".
>> * Whole implementation changed because last version is wrong.
>>   This version gets target handler and previous handler from vpci->handlers, then remove the target.
>> * Note: a case in function vpci_ext_capability_mask() needs to be discussed,
>>   because it may change the offset of next capability when the offset of target
>>   capability is 0x100U(the first extended capability), my implementation is just to
>>   ignore and let hardware to handle the target capability.
>>
>> v1->v2 changes:
>> * Removed the "priorities" of initializing capabilities since it isn't used anymore.
>> * Added new function vpci_capability_mask() and vpci_ext_capability_mask() to
>>   remove failed capability from list.
>> * Called vpci_make_msix_hole() in the end of init_msix().
>>
>> Best regards,
>> Jiqian Chen.
>> ---
>>  xen/drivers/vpci/vpci.c    | 79 ++++++++++++++++++++++++++++++++++++++
>>  xen/include/xen/pci_regs.h |  1 +
>>  2 files changed, 80 insertions(+)
>>
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index f97c7cc460a0..8ff5169bdd18 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -183,6 +183,83 @@ static void vpci_capability_mask(struct pci_dev *pdev,
>>      xfree(next_r);
>>  }
>>  
>> +static struct vpci_register *vpci_get_previous_ext_cap_register
>> +                (struct vpci *vpci, const unsigned int offset)
>> +{
>> +    uint32_t header;
>> +    unsigned int pos = PCI_CFG_SPACE_SIZE;
>> +    struct vpci_register *r;
>> +
>> +    if ( offset <= PCI_CFG_SPACE_SIZE )
>> +        return NULL;
>> +
>> +    r = vpci_get_register(vpci, pos, 4);
>> +    ASSERT(r);
>> +
>> +    header = (uint32_t)(uintptr_t)r->private;
>> +    pos = PCI_EXT_CAP_NEXT(header);
>> +    while ( pos > PCI_CFG_SPACE_SIZE && pos != offset )
>> +    {
>> +        r = vpci_get_register(vpci, pos, 4);
>> +        ASSERT(r);
>> +        header = (uint32_t)(uintptr_t)r->private;
>> +        pos = PCI_EXT_CAP_NEXT(header);
>> +    }
>> +
>> +    if ( pos <= PCI_CFG_SPACE_SIZE )
>> +        return NULL;
>> +
>> +    return r;
>> +}
>> +
>> +static void vpci_ext_capability_mask(struct pci_dev *pdev,
>> +                                     const unsigned int cap)
>> +{
>> +    const unsigned int offset = pci_find_ext_capability(pdev->sbdf, cap);
>> +    struct vpci_register *rm, *prev_r;
>> +    struct vpci *vpci = pdev->vpci;
>> +    uint32_t header, pre_header;
> 
> Maybe sanity check that offset is correct?
What do you mean sanity check?
Do I need to add something?

> 
>> +    spin_lock(&vpci->lock);
>> +    rm = vpci_get_register(vpci, offset, 4);
>> +    if ( !rm )
>> +    {
>> +        spin_unlock(&vpci->lock);
>> +        return;
>> +    }
>> +
>> +    header = (uint32_t)(uintptr_t)rm->private;
>> +    if ( offset == PCI_CFG_SPACE_SIZE )
>> +    {
>> +        if ( PCI_EXT_CAP_NEXT(header) <= PCI_CFG_SPACE_SIZE )
>> +            rm->private = (void *)(uintptr_t)0;
>> +        else
>> +            /*
>> +             * Else case needs to remove the capability in position 0x100U and
>> +             * moves the next capability to be in position 0x100U, that would
>> +             * cause the offset of next capability in vpci different from the
>> +             * hardware, then cause error accesses, so just ignore it here and
>> +             * hope hardware would handle the capability well.
>> +            */
>> +            printk(XENLOG_ERR "%pd %pp: ext cap %u is first cap, can't mask it\n",
>> +                   pdev->domain, &pdev->sbdf, cap);
> 
> In this case, could you maybe replace just the capability ID part of
> the header to return 0?  That will likely cause the OS to continue
> scanning the list, while ID 0 won't match which any known
> capability.
OK, will do in next version.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

On Wed, May 07, 2025 at 07:26:21AM +0000, Chen, Jiqian wrote:
> On 2025/5/7 00:21, Roger Pau Monné wrote:
> > On Mon, Apr 21, 2025 at 02:18:59PM +0800, Jiqian Chen wrote:
> >> When vpci fails to initialize a extended capability of device for dom0,
> >> it just return error instead of catching and processing exception. That
> >> makes the entire device unusable.
> >>
> >> So, add new a function to hide extended capability when initialization
> >> fails. And remove the failed extended capability handler from vpci
> >> extended capability list.
> >>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> >> ---
> >> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> >> ---
> >> v2->v3 changes:
> >> * Separated from the last version patch "vpci: Hide capability when it fails to initialize".
> >> * Whole implementation changed because last version is wrong.
> >>   This version gets target handler and previous handler from vpci->handlers, then remove the target.
> >> * Note: a case in function vpci_ext_capability_mask() needs to be discussed,
> >>   because it may change the offset of next capability when the offset of target
> >>   capability is 0x100U(the first extended capability), my implementation is just to
> >>   ignore and let hardware to handle the target capability.
> >>
> >> v1->v2 changes:
> >> * Removed the "priorities" of initializing capabilities since it isn't used anymore.
> >> * Added new function vpci_capability_mask() and vpci_ext_capability_mask() to
> >>   remove failed capability from list.
> >> * Called vpci_make_msix_hole() in the end of init_msix().
> >>
> >> Best regards,
> >> Jiqian Chen.
> >> ---
> >>  xen/drivers/vpci/vpci.c    | 79 ++++++++++++++++++++++++++++++++++++++
> >>  xen/include/xen/pci_regs.h |  1 +
> >>  2 files changed, 80 insertions(+)
> >>
> >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> >> index f97c7cc460a0..8ff5169bdd18 100644
> >> --- a/xen/drivers/vpci/vpci.c
> >> +++ b/xen/drivers/vpci/vpci.c
> >> @@ -183,6 +183,83 @@ static void vpci_capability_mask(struct pci_dev *pdev,
> >>      xfree(next_r);
> >>  }
> >>  
> >> +static struct vpci_register *vpci_get_previous_ext_cap_register
> >> +                (struct vpci *vpci, const unsigned int offset)
> >> +{
> >> +    uint32_t header;
> >> +    unsigned int pos = PCI_CFG_SPACE_SIZE;
> >> +    struct vpci_register *r;
> >> +
> >> +    if ( offset <= PCI_CFG_SPACE_SIZE )
> >> +        return NULL;
> >> +
> >> +    r = vpci_get_register(vpci, pos, 4);
> >> +    ASSERT(r);
> >> +
> >> +    header = (uint32_t)(uintptr_t)r->private;
> >> +    pos = PCI_EXT_CAP_NEXT(header);
> >> +    while ( pos > PCI_CFG_SPACE_SIZE && pos != offset )
> >> +    {
> >> +        r = vpci_get_register(vpci, pos, 4);
> >> +        ASSERT(r);
> >> +        header = (uint32_t)(uintptr_t)r->private;
> >> +        pos = PCI_EXT_CAP_NEXT(header);
> >> +    }
> >> +
> >> +    if ( pos <= PCI_CFG_SPACE_SIZE )
> >> +        return NULL;
> >> +
> >> +    return r;
> >> +}
> >> +
> >> +static void vpci_ext_capability_mask(struct pci_dev *pdev,
> >> +                                     const unsigned int cap)
> >> +{
> >> +    const unsigned int offset = pci_find_ext_capability(pdev->sbdf, cap);
> >> +    struct vpci_register *rm, *prev_r;
> >> +    struct vpci *vpci = pdev->vpci;
> >> +    uint32_t header, pre_header;
> > 
> > Maybe sanity check that offset is correct?
> What do you mean sanity check?
> Do I need to add something?

I would probably do something like:

if ( !offset )
{
    ASSERT_UNREACHABLE();
    return;
}

Thanks, Roger.

On 2025/5/7 16:09, Roger Pau Monné wrote:
> On Wed, May 07, 2025 at 07:26:21AM +0000, Chen, Jiqian wrote:
>> On 2025/5/7 00:21, Roger Pau Monné wrote:
>>> On Mon, Apr 21, 2025 at 02:18:59PM +0800, Jiqian Chen wrote:
>>>> When vpci fails to initialize a extended capability of device for dom0,
>>>> it just return error instead of catching and processing exception. That
>>>> makes the entire device unusable.
>>>>
>>>> So, add new a function to hide extended capability when initialization
>>>> fails. And remove the failed extended capability handler from vpci
>>>> extended capability list.
>>>>
>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>>> ---
>>>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
>>>> ---
>>>> v2->v3 changes:
>>>> * Separated from the last version patch "vpci: Hide capability when it fails to initialize".
>>>> * Whole implementation changed because last version is wrong.
>>>>   This version gets target handler and previous handler from vpci->handlers, then remove the target.
>>>> * Note: a case in function vpci_ext_capability_mask() needs to be discussed,
>>>>   because it may change the offset of next capability when the offset of target
>>>>   capability is 0x100U(the first extended capability), my implementation is just to
>>>>   ignore and let hardware to handle the target capability.
>>>>
>>>> v1->v2 changes:
>>>> * Removed the "priorities" of initializing capabilities since it isn't used anymore.
>>>> * Added new function vpci_capability_mask() and vpci_ext_capability_mask() to
>>>>   remove failed capability from list.
>>>> * Called vpci_make_msix_hole() in the end of init_msix().
>>>>
>>>> Best regards,
>>>> Jiqian Chen.
>>>> ---
>>>>  xen/drivers/vpci/vpci.c    | 79 ++++++++++++++++++++++++++++++++++++++
>>>>  xen/include/xen/pci_regs.h |  1 +
>>>>  2 files changed, 80 insertions(+)
>>>>
>>>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>>>> index f97c7cc460a0..8ff5169bdd18 100644
>>>> --- a/xen/drivers/vpci/vpci.c
>>>> +++ b/xen/drivers/vpci/vpci.c
>>>> @@ -183,6 +183,83 @@ static void vpci_capability_mask(struct pci_dev *pdev,
>>>>      xfree(next_r);
>>>>  }
>>>>  
>>>> +static struct vpci_register *vpci_get_previous_ext_cap_register
>>>> +                (struct vpci *vpci, const unsigned int offset)
>>>> +{
>>>> +    uint32_t header;
>>>> +    unsigned int pos = PCI_CFG_SPACE_SIZE;
>>>> +    struct vpci_register *r;
>>>> +
>>>> +    if ( offset <= PCI_CFG_SPACE_SIZE )
>>>> +        return NULL;
>>>> +
>>>> +    r = vpci_get_register(vpci, pos, 4);
>>>> +    ASSERT(r);
>>>> +
>>>> +    header = (uint32_t)(uintptr_t)r->private;
>>>> +    pos = PCI_EXT_CAP_NEXT(header);
>>>> +    while ( pos > PCI_CFG_SPACE_SIZE && pos != offset )
>>>> +    {
>>>> +        r = vpci_get_register(vpci, pos, 4);
>>>> +        ASSERT(r);
>>>> +        header = (uint32_t)(uintptr_t)r->private;
>>>> +        pos = PCI_EXT_CAP_NEXT(header);
>>>> +    }
>>>> +
>>>> +    if ( pos <= PCI_CFG_SPACE_SIZE )
>>>> +        return NULL;
>>>> +
>>>> +    return r;
>>>> +}
>>>> +
>>>> +static void vpci_ext_capability_mask(struct pci_dev *pdev,
>>>> +                                     const unsigned int cap)
>>>> +{
>>>> +    const unsigned int offset = pci_find_ext_capability(pdev->sbdf, cap);
>>>> +    struct vpci_register *rm, *prev_r;
>>>> +    struct vpci *vpci = pdev->vpci;
>>>> +    uint32_t header, pre_header;
>>>
>>> Maybe sanity check that offset is correct?
>> What do you mean sanity check?
>> Do I need to add something?
> 
> I would probably do something like:
> 
> if ( !offset )
> {
>     ASSERT_UNREACHABLE();
>     return;
> }
How about adding check?

    if ( offset < PCI_CFG_SPACE_SIZE )
    {
        ASSERT_UNREACHABLE();
        return -EINVAL;
    }

Do I need to add similar check in vpci_capability_mask()?

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

On Wed, May 07, 2025 at 08:49:46AM +0000, Chen, Jiqian wrote:
> On 2025/5/7 16:09, Roger Pau Monné wrote:
> > On Wed, May 07, 2025 at 07:26:21AM +0000, Chen, Jiqian wrote:
> >> On 2025/5/7 00:21, Roger Pau Monné wrote:
> >>> On Mon, Apr 21, 2025 at 02:18:59PM +0800, Jiqian Chen wrote:
> >>>> When vpci fails to initialize a extended capability of device for dom0,
> >>>> it just return error instead of catching and processing exception. That
> >>>> makes the entire device unusable.
> >>>>
> >>>> So, add new a function to hide extended capability when initialization
> >>>> fails. And remove the failed extended capability handler from vpci
> >>>> extended capability list.
> >>>>
> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> >>>> ---
> >>>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> >>>> ---
> >>>> v2->v3 changes:
> >>>> * Separated from the last version patch "vpci: Hide capability when it fails to initialize".
> >>>> * Whole implementation changed because last version is wrong.
> >>>>   This version gets target handler and previous handler from vpci->handlers, then remove the target.
> >>>> * Note: a case in function vpci_ext_capability_mask() needs to be discussed,
> >>>>   because it may change the offset of next capability when the offset of target
> >>>>   capability is 0x100U(the first extended capability), my implementation is just to
> >>>>   ignore and let hardware to handle the target capability.
> >>>>
> >>>> v1->v2 changes:
> >>>> * Removed the "priorities" of initializing capabilities since it isn't used anymore.
> >>>> * Added new function vpci_capability_mask() and vpci_ext_capability_mask() to
> >>>>   remove failed capability from list.
> >>>> * Called vpci_make_msix_hole() in the end of init_msix().
> >>>>
> >>>> Best regards,
> >>>> Jiqian Chen.
> >>>> ---
> >>>>  xen/drivers/vpci/vpci.c    | 79 ++++++++++++++++++++++++++++++++++++++
> >>>>  xen/include/xen/pci_regs.h |  1 +
> >>>>  2 files changed, 80 insertions(+)
> >>>>
> >>>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> >>>> index f97c7cc460a0..8ff5169bdd18 100644
> >>>> --- a/xen/drivers/vpci/vpci.c
> >>>> +++ b/xen/drivers/vpci/vpci.c
> >>>> @@ -183,6 +183,83 @@ static void vpci_capability_mask(struct pci_dev *pdev,
> >>>>      xfree(next_r);
> >>>>  }
> >>>>  
> >>>> +static struct vpci_register *vpci_get_previous_ext_cap_register
> >>>> +                (struct vpci *vpci, const unsigned int offset)
> >>>> +{
> >>>> +    uint32_t header;
> >>>> +    unsigned int pos = PCI_CFG_SPACE_SIZE;
> >>>> +    struct vpci_register *r;
> >>>> +
> >>>> +    if ( offset <= PCI_CFG_SPACE_SIZE )
> >>>> +        return NULL;
> >>>> +
> >>>> +    r = vpci_get_register(vpci, pos, 4);
> >>>> +    ASSERT(r);
> >>>> +
> >>>> +    header = (uint32_t)(uintptr_t)r->private;
> >>>> +    pos = PCI_EXT_CAP_NEXT(header);
> >>>> +    while ( pos > PCI_CFG_SPACE_SIZE && pos != offset )
> >>>> +    {
> >>>> +        r = vpci_get_register(vpci, pos, 4);
> >>>> +        ASSERT(r);
> >>>> +        header = (uint32_t)(uintptr_t)r->private;
> >>>> +        pos = PCI_EXT_CAP_NEXT(header);
> >>>> +    }
> >>>> +
> >>>> +    if ( pos <= PCI_CFG_SPACE_SIZE )
> >>>> +        return NULL;
> >>>> +
> >>>> +    return r;
> >>>> +}
> >>>> +
> >>>> +static void vpci_ext_capability_mask(struct pci_dev *pdev,
> >>>> +                                     const unsigned int cap)
> >>>> +{
> >>>> +    const unsigned int offset = pci_find_ext_capability(pdev->sbdf, cap);
> >>>> +    struct vpci_register *rm, *prev_r;
> >>>> +    struct vpci *vpci = pdev->vpci;
> >>>> +    uint32_t header, pre_header;
> >>>
> >>> Maybe sanity check that offset is correct?
> >> What do you mean sanity check?
> >> Do I need to add something?
> > 
> > I would probably do something like:
> > 
> > if ( !offset )
> > {
> >     ASSERT_UNREACHABLE();
> >     return;
> > }
> How about adding check?
> 
>     if ( offset < PCI_CFG_SPACE_SIZE )
>     {
>         ASSERT_UNREACHABLE();
>         return -EINVAL;
>     }

That would work also, however note that pci_find_ext_capability()
should only return 0 if the capability is not found, and other callers
already assume that != 0 implies a valid position.  I will simply
check !offset as that's inline with all the other checks Xen does for
return values of pci_find_ext_capability().

> Do I need to add similar check in vpci_capability_mask()?

Possibly - seems like I didn't comment on that one, sorry.

Regards, Roger.

On 21.04.2025 08:18, Jiqian Chen wrote:
> --- a/xen/include/xen/pci_regs.h
> +++ b/xen/include/xen/pci_regs.h
> @@ -449,6 +449,7 @@
>  #define PCI_EXT_CAP_ID(header)		((header) & 0x0000ffff)
>  #define PCI_EXT_CAP_VER(header)		(((header) >> 16) & 0xf)
>  #define PCI_EXT_CAP_NEXT(header)	(((header) >> 20) & 0xffc)
> +#define PCI_EXT_CAP_NEXT_MASK		0xFFC00000U

To avoid introducing redundancy, imo this addition calls for

#define PCI_EXT_CAP_NEXT(header)	MASK_EXTR(header, PCI_EXT_CAP_NEXT_MASK)

now.

Jan

On 2025/4/23 00:06, Jan Beulich wrote:
> On 21.04.2025 08:18, Jiqian Chen wrote:
>> --- a/xen/include/xen/pci_regs.h
>> +++ b/xen/include/xen/pci_regs.h
>> @@ -449,6 +449,7 @@
>>  #define PCI_EXT_CAP_ID(header)		((header) & 0x0000ffff)
>>  #define PCI_EXT_CAP_VER(header)		(((header) >> 16) & 0xf)
>>  #define PCI_EXT_CAP_NEXT(header)	(((header) >> 20) & 0xffc)
>> +#define PCI_EXT_CAP_NEXT_MASK		0xFFC00000U
> 
> To avoid introducing redundancy, imo this addition calls for
> 
> #define PCI_EXT_CAP_NEXT(header)	MASK_EXTR(header, PCI_EXT_CAP_NEXT_MASK)
When I tested this locally, I encountered errors: every next position address loss two bits zero.
The next register has 12 bits, according to PCI spec, the bottom two bits are reserved zero,
so "#define PCI_EXT_CAP_NEXT_MASK 0xFFC00000U" is fine,
but if change this "#define PCI_EXT_CAP_NEXT(header) MASK_EXTR(header, PCI_EXT_CAP_NEXT_MASK)",
I need to change PCI_EXT_CAP_NEXT_MASK to be 0xFFF00000U, is it fine?

> 
> now.
> 
> Jan

-- 
Best regards,
Jiqian Chen.

On Thu, May 08, 2025 at 09:16:49AM +0000, Chen, Jiqian wrote:
> On 2025/4/23 00:06, Jan Beulich wrote:
> > On 21.04.2025 08:18, Jiqian Chen wrote:
> >> --- a/xen/include/xen/pci_regs.h
> >> +++ b/xen/include/xen/pci_regs.h
> >> @@ -449,6 +449,7 @@
> >>  #define PCI_EXT_CAP_ID(header)		((header) & 0x0000ffff)
> >>  #define PCI_EXT_CAP_VER(header)		(((header) >> 16) & 0xf)
> >>  #define PCI_EXT_CAP_NEXT(header)	(((header) >> 20) & 0xffc)
> >> +#define PCI_EXT_CAP_NEXT_MASK		0xFFC00000U
> > 
> > To avoid introducing redundancy, imo this addition calls for
> > 
> > #define PCI_EXT_CAP_NEXT(header)	MASK_EXTR(header, PCI_EXT_CAP_NEXT_MASK)
> When I tested this locally, I encountered errors: every next position address loss two bits zero.
> The next register has 12 bits, according to PCI spec, the bottom two bits are reserved zero,
> so "#define PCI_EXT_CAP_NEXT_MASK 0xFFC00000U" is fine,
> but if change this "#define PCI_EXT_CAP_NEXT(header) MASK_EXTR(header, PCI_EXT_CAP_NEXT_MASK)",
> I need to change PCI_EXT_CAP_NEXT_MASK to be 0xFFF00000U, is it fine?

Oh, I see.  You might want to do:

#define PCI_EXT_CAP_NEXT_MASK		0xFFF00000U
/* Bottom two bits of next capability position are reserved. */
#define PCI_EXT_CAP_NEXT(header)	(MASK_EXTR(header,
					           PCI_EXT_CAP_NEXT_MASK)
					 & 0xFFCU)

The spec says:

"The bottom 2 bits of this offset are Reserved and must be implemented
as 00b although software must mask them to allow for future uses of
these bits."

So we need to make sure they are masked.

Thanks, Roger.