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.c...@amd.com> > >> Reviewed-by: Roger Pau Monné <roger....@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.
