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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.