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

[Jonathan Cameron]

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.

> +}
> +
> +/*
> + * 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);
> +}
> +