Re: [PATCH v3 9/9] hw/mem/cxl_type3: Add dpa range validation for accesses to dc regions

[Jonathan Cameron via <qemu-devel@nongnu.org>]

> > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > index 43cea3d818..4ec65a751a 100644
> > > --- a/hw/mem/cxl_type3.c
> > > +++ b/hw/mem/cxl_type3.c  
> >   
> > > +/*
> > > + * Check whether a DPA range [dpa, dpa + len) has been backed with DC extents.
> > > + * Used when validating read/write to dc regions
> > > + */
> > > +bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> > > +                                  uint64_t len)
> > > +{
> > > +    CXLDCDRegion *region;
> > > +    uint64_t nbits;
> > > +    long nr;
> > > +
> > > +    region = cxl_find_dc_region(ct3d, dpa, len);
> > > +    if (!region) {
> > > +        return false;
> > > +    }
> > > +
> > > +    nr = (dpa - region->base) / region->block_size;
> > > +    nbits = DIV_ROUND_UP(len, region->block_size);
> > > +    return find_next_zero_bit(region->blk_bitmap, nr + nbits, nr) == nr + nbits;  
> > I'm not sure how this works... Is it taking a size or an end point?
> > 
> > Linux equivalent takes size, so I'd expect
> > 
> >     return find_next_zero_bit(region->blk_bitmap, nbits, nr);
> > Perhaps a comment would avoid any future confusion on this.
> >   
> 
> My understanding is that the size is the size of the bitmap, which is
> also end of the range to check, not the length of the range to check.
> 
> The function find_next_zero_bit(bitmap, size, offset) checks the bitmap range
> [offset, size) to find the next unset bit, for the above test, we want to
> check range [nr, nr + nbits), so the arguments passed to the function
> should be right.
> 
> In the definition of the function, whenever offset >= size, it returns size
> because size is the end of the range, So if we pass nbits and nr
> to the function and nr >= nbits, which can be common, meaning (dpa-region_base)
> \> len, the function will always return true; that is not what we want.  
> 
> To sum up, the second parameter of the function should always be the end
> of the range to check, for our case, it is nr + nbits.
Ok. Thanks for the explanation. That sounds good to me

Jonathan

> 
> Fan