Re: [PATCH v3 08/11] vpci: Refactor vpci_remove_register to remove matched registers

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Apr 21, 2025 at 02:19:00PM +0800, 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. And vpci_remove_register() is only used for test currently.
> So, refactor it to support removing all matched registers in a
> calling time.
> 
> And it is no matter to remove a non exist register, so remove the
> __must_check prefix.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> ---
> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
> 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 | 34 ++++++++++++++++++++--------------
>  xen/include/xen/vpci.h  |  4 ++--
>  3 files changed, 24 insertions(+), 18 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 8ff5169bdd18..904770628a2a 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -497,34 +497,40 @@ 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;
> +    unsigned int end = start + size;
>  
>      spin_lock(&vpci->lock);
>      list_for_each_entry ( rm, &vpci->handlers, node )

You might want to use list_for_each_entry_safe ( ) so that...

>      {
> -        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 )
>          {
> +            struct vpci_register *prev =
> +                list_entry(rm->node.prev, struct vpci_register, node);

... you don't need to find prev here.

>              list_del(&rm->node);
> -            spin_unlock(&vpci->lock);
>              xfree(rm);
> -            return 0;
> +            rm = prev;
> +            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 -EINVAL;

-ERANGE might be more descriptive here.

Thanks, Roger.