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

[fan <nifan.cxl@gmail.com>]

On Wed, Jan 24, 2024 at 04:58:15PM +0000, Jonathan Cameron wrote:
> On Tue,  7 Nov 2023 10:07:13 -0800
> nifan.cxl@gmail.com wrote:
> 
> > From: Fan Ni <fan.ni@samsung.com>
> > 
> > Not all dpa range in the dc regions is valid to access until an extent
> DPA ... DC etc
> 
> > covering the range has been added. Add a bitmap for each region to
> > record whether a dc block in the region has been backed by dc extent.
> > For the bitmap, a bit in the bitmap represents a dc block. When a dc
> > extent is added, all the bits of the blocks in the extent will be set,
> > which will be cleared when the extent is released.
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> 
> Hi Fan, one query inline and a few comments.
> 
> Jonathan
> 
> > 
> > --
> > JC changes:
> > - Rebase on what will be next gitlab.com/jic23/qemu CXL staging tree.
> > - Drop unnecessary handling of failed bitmap allocations. In common with
> >   most QEMU allocations they fail hard anyway.
> > - Use previously factored out cxl_find_region() helper
> > - Minor editorial stuff in comments such as spec version references
> >   according to the standard form I'm trying to push through the code.
> > Picked up Jørgen's fix:
> > https://lore.kernel.org/qemu-devel/d0d7ca1d-81bc-19b3-4904-d60046ded844@wdc.com/T/#u
> > ---
> >  hw/cxl/cxl-mailbox-utils.c  | 31 +++++++++------
> >  hw/mem/cxl_type3.c          | 78 +++++++++++++++++++++++++++++++++++++
> >  include/hw/cxl/cxl_device.h | 15 +++++--
> >  3 files changed, 109 insertions(+), 15 deletions(-)
> > 
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 8e6a98753a..6be92fb5ba 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -1401,10 +1401,9 @@ CXLDCDRegion *cxl_find_dc_region(CXLType3Dev *ct3d, uint64_t dpa, uint64_t len)
> >  }
> >  
> >  void cxl_insert_extent_to_extent_list(CXLDCDExtentList *list,
> > -                                             uint64_t dpa,
> > -                                             uint64_t len,
> > -                                             uint8_t *tag,
> > -                                             uint16_t shared_seq)
> > +                                      uint64_t dpa, uint64_t len,
> > +                                      uint8_t *tag,
> > +                                      uint16_t shared_seq)
> 
> avoid noisy whitespace changes like this.
> 
> 
> > 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.

Fan



> > +}
> > +
> > +/*
> > + * Mark the DPA range [dpa, dap + len) to be unbacked and inaccessible. This
> > + * happens when a dc extent is return by the host.
> > + */
> > +void ct3_clear_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;
> > +    }
> > +
> > +    nr = (dpa - region->base) / region->block_size;
> > +    nbits = len / region->block_size;
> > +    bitmap_clear(region->blk_bitmap, nr, nbits);
> > +}
> > +
>