[v9] Support hiding capability when its initialization fails

[PATCH v9 5/8] vpci: Refactor vpci_remove_register to remove matched registers

Posted by Jiqian Chen 6 months, 2 weeks ago

vpci_remove_register() only supports removing a register in a time,
but the follow-on changes need to remove all registers within a range.
So, refactor it to support removing all registers in a given region.

And it is no issue to remove a non exist register, so remove the
__must_check prefix.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
cc: Anthony PERARD <anthony.perard@vates.tech>
---
v8->v9 changes:
v7->v8 changes:
v6->v7 changes:
No.

v5->v6 changes:
* Modify commit message.
* Add Roger's Reviewed-by.

v4->v5 changes:
No.

v3->v4 changes:
* Use list_for_each_entry_safe instead of list_for_each_entry.
* Return ERANGE if overlap.

v2->v3 changes:
* Add new check to return error if registers overlap but not inside range.

v1->v2 changes:
new patch

Best regards,
Jiqian Chen.
---
 tools/tests/vpci/main.c |  4 ++--
 xen/drivers/vpci/vpci.c | 38 ++++++++++++++++++++------------------
 xen/include/xen/vpci.h  |  4 ++--
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/tools/tests/vpci/main.c b/tools/tests/vpci/main.c
index 33223db3eb77..ca72877d60cd 100644
--- a/tools/tests/vpci/main.c
+++ b/tools/tests/vpci/main.c
@@ -132,10 +132,10 @@ static void vpci_write32_mask(const struct pci_dev *pdev, unsigned int reg,
                                   rsvdz_mask))
 
 #define VPCI_REMOVE_REG(off, size)                                          \
-    assert(!vpci_remove_register(test_pdev.vpci, off, size))
+    assert(!vpci_remove_registers(test_pdev.vpci, off, size))
 
 #define VPCI_REMOVE_INVALID_REG(off, size)                                  \
-    assert(vpci_remove_register(test_pdev.vpci, off, size))
+    assert(vpci_remove_registers(test_pdev.vpci, off, size))
 
 /* Read a 32b register using all possible sizes. */
 void multiread4_check(unsigned int reg, uint32_t val)
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 01f5746b64df..4b8e6b28bd07 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -152,7 +152,7 @@ static int vpci_capability_hide(const struct pci_dev *pdev, unsigned int cap)
 
     prev_r->private = next_r->private;
     /*
-     * Not calling vpci_remove_register() here is to avoid redoing
+     * Not calling vpci_remove_registers() here is to avoid redoing
      * the register search.
      */
     list_del(&next_r->node);
@@ -160,7 +160,7 @@ static int vpci_capability_hide(const struct pci_dev *pdev, unsigned int cap)
     xfree(next_r);
 
     if ( !is_hardware_domain(pdev->domain) )
-        return vpci_remove_register(vpci, offset + PCI_CAP_LIST_ID, 1);
+        return vpci_remove_registers(vpci, offset + PCI_CAP_LIST_ID, 1);
 
     return 0;
 }
@@ -549,34 +549,36 @@ int vpci_add_register_mask(struct vpci *vpci, vpci_read_t *read_handler,
     return 0;
 }
 
-int vpci_remove_register(struct vpci *vpci, unsigned int offset,
-                         unsigned int size)
+int vpci_remove_registers(struct vpci *vpci, unsigned int start,
+                          unsigned int size)
 {
-    const struct vpci_register r = { .offset = offset, .size = size };
-    struct vpci_register *rm;
+    struct vpci_register *rm, *tmp;
+    unsigned int end = start + size;
 
     spin_lock(&vpci->lock);
-    list_for_each_entry ( rm, &vpci->handlers, node )
+    list_for_each_entry_safe ( rm, tmp, &vpci->handlers, node )
     {
-        int cmp = vpci_register_cmp(&r, rm);
-
-        /*
-         * NB: do not use a switch so that we can use break to
-         * get out of the list loop earlier if required.
-         */
-        if ( !cmp && rm->offset == offset && rm->size == size )
+        /* Remove rm if rm is inside the range. */
+        if ( rm->offset >= start && rm->offset + rm->size <= end )
         {
             list_del(&rm->node);
-            spin_unlock(&vpci->lock);
             xfree(rm);
-            return 0;
+            continue;
         }
-        if ( cmp <= 0 )
+
+        /* Return error if registers overlap but not inside. */
+        if ( rm->offset + rm->size > start && rm->offset < end )
+        {
+            spin_unlock(&vpci->lock);
+            return -ERANGE;
+        }
+
+        if ( start < rm->offset )
             break;
     }
     spin_unlock(&vpci->lock);
 
-    return -ENOENT;
+    return 0;
 }
 
 /* Wrappers for performing reads/writes to the underlying hardware. */
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 17cfecb0aabf..514a0ce39133 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -70,8 +70,8 @@ static inline int __must_check vpci_add_register(struct vpci *vpci,
                                   size, data, 0, 0, 0, 0);
 }
 
-int __must_check vpci_remove_register(struct vpci *vpci, unsigned int offset,
-                                      unsigned int size);
+int vpci_remove_registers(struct vpci *vpci, unsigned int start,
+                          unsigned int size);
 
 /* Generic read/write handlers for the PCI config space. */
 uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size);
-- 
2.34.1

Re: [PATCH v9 5/8] vpci: Refactor vpci_remove_register to remove matched registers

Posted by Andrew Cooper 6 months, 2 weeks ago

On 28/07/2025 6:03 am, Jiqian Chen wrote:
> vpci_remove_register() only supports removing a register in a time,
> but the follow-on changes need to remove all registers within a range.
> So, refactor it to support removing all registers in a given region.
>
> And it is no issue to remove a non exist register, so remove the
> __must_check prefix.
>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Bisection says this causes an assertion failure in the unit test.

Running /usr/lib/xen/tests/test_vpci
Assertion failed: vpci_remove_registers(test_pdev.vpci, 16, 2) (main.c:
main: 407)
FAILED: /usr/lib/xen/tests/test_vpci

Full logs, not that they're of any more help:

https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1956817587

~Andrew

Re: [PATCH v9 5/8] vpci: Refactor vpci_remove_register to remove matched registers

Posted by Chen, Jiqian 6 months, 1 week ago

On 2025/7/30 19:23, Andrew Cooper wrote:
> On 28/07/2025 6:03 am, Jiqian Chen wrote:
>> vpci_remove_register() only supports removing a register in a time,
>> but the follow-on changes need to remove all registers within a range.
>> So, refactor it to support removing all registers in a given region.
>>
>> And it is no issue to remove a non exist register, so remove the
>> __must_check prefix.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Bisection says this causes an assertion failure in the unit test.
> 
> Running /usr/lib/xen/tests/test_vpci
> Assertion failed: vpci_remove_registers(test_pdev.vpci, 16, 2) (main.c:
> main: 407)
> FAILED: /usr/lib/xen/tests/test_vpci
Thanks Andrew.
This is because new function vpci_remove_registers() removes all registers inside (offset, offset + size) and returns failure when overlap happens.
For tools/tests/vpci/main.c, there are layout:
     *
     * 32    24    16     8     0
     *  +------+------+------+------+
     *  |            r0             | 0
     *  +------+------+------+------+
     *  |  r7  |  r6  |  r5  |//////| 4
     *  +------+------+------+------|
     *  |///////////////////////////| 8
     *  +-------------+-------------+
     *  |/////////////|    r12      | 12
     *  +------+------+------+------+
     *  |r16[3]|r16[2]|r16[1]|r16[0]| 16
     *  +------+------+------+------+
     *  |    r20[1]   |    r20[0]   | 20
     *  +-------------+-------------|
     *  |            r24            | 24
     *  +------+------+------+------+
     *  |//////|  r30 |//////|  r28 | 28
     *  +------+------+------+------+
     *  |rsvdz |rsvdp | rw1c |  ro  | 32
     *  +------+------+------+------+
     *
As for the last three test cases:
    VPCI_REMOVE_INVALID_REG(20, 1);
    This can success as this overlap with r20[0]
    VPCI_REMOVE_INVALID_REG(16, 2);
    VPCI_REMOVE_INVALID_REG(30, 2);
    These two fail as there are r16[0], r16[1] and r30 inside them, so remove success and test cases fail.
So, I think we need to change these two test cases to match the new function's logic, like:
VPCI_REMOVE_INVALID_REG(0, 2);
VPCI_REMOVE_INVALID_REG(2, 2);
Or delete them directly.

Hi Roger, what's your opinion?

> 
> Full logs, not that they're of any more help:
> 
> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1956817587
> 
> ~Andrew

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v9 5/8] vpci: Refactor vpci_remove_register to remove matched registers

Posted by Roger Pau Monné 6 months, 1 week ago

On Thu, Jul 31, 2025 at 06:28:14AM +0000, Chen, Jiqian wrote:
> On 2025/7/30 19:23, Andrew Cooper wrote:
> > On 28/07/2025 6:03 am, Jiqian Chen wrote:
> >> vpci_remove_register() only supports removing a register in a time,
> >> but the follow-on changes need to remove all registers within a range.
> >> So, refactor it to support removing all registers in a given region.
> >>
> >> And it is no issue to remove a non exist register, so remove the
> >> __must_check prefix.
> >>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> >> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > Bisection says this causes an assertion failure in the unit test.
> > 
> > Running /usr/lib/xen/tests/test_vpci
> > Assertion failed: vpci_remove_registers(test_pdev.vpci, 16, 2) (main.c:
> > main: 407)
> > FAILED: /usr/lib/xen/tests/test_vpci
> Thanks Andrew.
> This is because new function vpci_remove_registers() removes all registers inside (offset, offset + size) and returns failure when overlap happens.
> For tools/tests/vpci/main.c, there are layout:
>      *
>      * 32    24    16     8     0
>      *  +------+------+------+------+
>      *  |            r0             | 0
>      *  +------+------+------+------+
>      *  |  r7  |  r6  |  r5  |//////| 4
>      *  +------+------+------+------|
>      *  |///////////////////////////| 8
>      *  +-------------+-------------+
>      *  |/////////////|    r12      | 12
>      *  +------+------+------+------+
>      *  |r16[3]|r16[2]|r16[1]|r16[0]| 16
>      *  +------+------+------+------+
>      *  |    r20[1]   |    r20[0]   | 20
>      *  +-------------+-------------|
>      *  |            r24            | 24
>      *  +------+------+------+------+
>      *  |//////|  r30 |//////|  r28 | 28
>      *  +------+------+------+------+
>      *  |rsvdz |rsvdp | rw1c |  ro  | 32
>      *  +------+------+------+------+
>      *
> As for the last three test cases:
>     VPCI_REMOVE_INVALID_REG(20, 1);
>     This can success as this overlap with r20[0]
>     VPCI_REMOVE_INVALID_REG(16, 2);
>     VPCI_REMOVE_INVALID_REG(30, 2);
>     These two fail as there are r16[0], r16[1] and r30 inside them, so remove success and test cases fail.

Yes, given that vpci_remove_registers() can now remove multiple
handlers in one call those two tests are simply not correct given the
new behavior of the function.

> So, I think we need to change these two test cases to match the new function's logic, like:
> VPCI_REMOVE_INVALID_REG(0, 2);
> VPCI_REMOVE_INVALID_REG(2, 2);

Those two test exactly the same as the (20, 1) call, so I think they
don't add value to the testing.

I would convert them into valid test cases, so:

VPCI_REMOVE_REG(16, 2);
VPCI_REMOVE_REG(30, 2);

Because they now test the new functionality of removing multiple
handlers with a single call (or removing a handler while dealing with
padding on the sides).

Thanks, Roger.

[PATCH v9 1/8] vpci/header: Emulate extended capability list for dom0
[PATCH v9 2/8] vpci: Refactor REGISTER_VPCI_INIT
[PATCH v9 3/8] vpci: Hide legacy capability when it fails to initialize
[PATCH v9 4/8] vpci: Hide extended capability when it fails to initialize
[PATCH v9 5/8] vpci: Refactor vpci_remove_register to remove matched registers
[PATCH v9 6/8] vpci/rebar: Free Rebar resources when init_rebar() fails
[PATCH v9 7/8] vpci/msi: Free MSI resources when init_msi() fails
[PATCH v9 8/8] vpci/msix: Free MSIX resources when init_msix() fails