Re: [PATCH] softmmu/memory: fix memory_region_ioeventfd_equal()

[Stefan Hajnoczi]

On Sat, Oct 17, 2020 at 02:01:02PM -0700, Elena Afanasova wrote:
> Eventfd can be registered with a zero length when fast_mmio is true.
> Handle this case properly when dispatching through QEMU.
> 
> Signed-off-by: Elena Afanasova <eafanas...@gmail.com>
> ---
>  softmmu/memory.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 403ff3abc9..3ca2154a64 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -203,10 +203,17 @@ static bool 
> memory_region_ioeventfd_before(MemoryRegionIoeventfd *a,
>  }
>  
>  static bool memory_region_ioeventfd_equal(MemoryRegionIoeventfd *a,
> -                                          MemoryRegionIoeventfd *b)
> -{
> -    return !memory_region_ioeventfd_before(a, b)
> -        && !memory_region_ioeventfd_before(b, a);
> +                                          MemoryRegionIoeventfd *mrb)

Why rename b -> mrb?

> +{
> +    if (int128_eq(a->addr.start, mrb->addr.start) &&
> +            (!int128_nz(mrb->addr.size) ||
> +             int128_eq(a->addr.size, mrb->addr.size)) &&
> +            (a->match_data == mrb->match_data) &&
> +            ((mrb->match_data && (a->data == mrb->data)) || 
> !mrb->match_data) &&
> +            (a->e == mrb->e))
> +        return true;

The kernel behaves differently in that a and b are symmetric:

  static bool
  ioeventfd_check_collision(struct kvm *kvm, struct _ioeventfd *p)
  {
        struct _ioeventfd *_p;

        list_for_each_entry(_p, &kvm->ioeventfds, list)
                if (_p->bus_idx == p->bus_idx &&
                    _p->addr == p->addr &&
                    (!_p->length || !p->length ||
                     (_p->length == p->length &&
                      (_p->wildcard || p->wildcard ||
                       _p->datamatch == p->datamatch))))
                        return true;

        return false;
  }

For example, if a->addr.size == 0 then the ioeventfd matches whereas
this patch only does it for b.

Please match the kernel behavior.

A different approach is setting ioeventfd.addr.size to zero when
mr->ioeventfds[i].addr.size is zero in the
memory_region_dispatch_write_eventfds() loop. That way
memory_region_ioeventfd_equal() doesn't need to be modified.

signature.asc
Description: PGP signature