Re: [PATCH] hw/vfio/common: Fix a small boundary issue of a trace

[chenxiang (M)]

Hi Damien,


在 2022/4/6 23:22, Damien Hedde 写道:
> On 4/6/22 10:14, chenxiang via wrote:
> > From: Xiang Chen <chenxiang66@hisilicon.com>
> >
> > Right now the trace of vfio_region_sparse_mmap_entry is as follows:
> > vfio_region_sparse_mmap_entry sparse entry 0 [0x1000 - 0x9000]
> > Actually the range it wants to show is [0x1000 - 0x8fff]，so fix it.
> >
> > Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> > ---
> >   hw/vfio/common.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index 080046e3f5..0b3808caf8 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -1546,7 +1546,7 @@ static int 
> > vfio_setup_region_sparse_mmaps(VFIORegion *region,
> >       for (i = 0, j = 0; i < sparse->nr_areas; i++) {
> >           trace_vfio_region_sparse_mmap_entry(i, 
> > sparse->areas[i].offset,
> > sparse->areas[i].offset +
> > - sparse->areas[i].size);
> > + sparse->areas[i].size - 1);
> >             if (sparse->areas[i].size) {
> >               region->mmaps[j].offset = sparse->areas[i].offset;
>
> If the size if zero, the trace will be weird with an underflow if 
> offset is zero as well.

Yes, that's a issue.

> Maybe just change the trace by inverting the right bracket ?
> eg: [0x1000 - 0x9000[
> Or don't trace in that case ? (but I am not maintainer of this, so 
> maybe that does not make sense).

But it uses [offset, offset + size - 1] in other places such as 
trace_vfio_region_region_mmap()/trace_vfio_subregion_unmap()/trace_vfio_region_mmap_fault() 
in vfio code.
Maybe it is better to move this trace to the brace of "if 
(sparse->areas[i].size)" which ensures size != 0.

> --
> Damien
> .