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

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

On Wed, Apr 09, 2025 at 02:45:25PM +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 it is only used for test. 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>
> ---
> v1->v2 changes:
> new patch
> 
> Best regards,
> Jiqian Chen.
> ---
>  tools/tests/vpci/main.c |  4 ++--
>  xen/drivers/vpci/vpci.c | 23 ++++++++++++-----------
>  xen/include/xen/vpci.h  |  4 ++--
>  3 files changed, 16 insertions(+), 15 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 f1f125bfdab1..115d3c5f0c84 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -418,34 +418,35 @@ 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 offset,
> +                          unsigned int size)
>  {
>      const struct vpci_register r = { .offset = offset, .size = size };
>      struct vpci_register *rm;
> +    int rc = -ENOENT;

Thinking about this, not sure returning ENOENT makes much sense now,
as the (new) purpose of the function is to zap all handlers from a
range, without possibly prior knowledge whether there are any
handlers in the range.

>  
>      spin_lock(&vpci->lock);
>      list_for_each_entry ( rm, &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 )
> +        if ( !cmp )
>          {
> +            struct vpci_register *prev =
> +                list_entry(rm->node.prev, struct vpci_register, node);
> +
>              list_del(&rm->node);
> -            spin_unlock(&vpci->lock);
>              xfree(rm);
> -            return 0;
> +            rm = prev;
> +            rc = 0;

I think you want some extra checks here to ensure you are not removing
partially overlapping registers.  IOW, you need to assert the register
in rm is fully contained inside the range to zap.

It might be best to simply not use vpci_register_cmp(), and instead
open code the comparison here to ensure "rm" is fuly contained inside
the [offset, offset + size - 1] range.  You will need to return an
error if there's a partially overlapping register.

Thanks, Roger.