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

Commit Message

fan Nov. 7, 2023, 6:07 p.m. UTC

From: Fan Ni <[e-mail]>

Not all dpa range in the dc regions is valid to access until an extent
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 <[e-mail]>

--
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/[e-mail]/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(-)

Comments

Jonathan Cameron Jan. 24, 2024, 4:58 p.m. UTC | #1

On Tue,  7 Nov 2023 10:07:13 -0800
[e-mail] wrote:

> From: Fan Ni <[e-mail]>
> 
> 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 <[e-mail]>

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/[e-mail]/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);
> +}
> +

fan Feb. 9, 2024, 7:04 p.m. UTC | #2

On Wed, Jan 24, 2024 at 04:58:15PM +0000, Jonathan Cameron wrote:
> On Tue,  7 Nov 2023 10:07:13 -0800
> [e-mail] wrote:
> 
> > From: Fan Ni <[e-mail]>
> > 
> > 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 <[e-mail]>
> 
> 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/[e-mail]/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);
> > +}
> > +
>

Jonathan Cameron Feb. 13, 2024, 9:31 a.m. UTC | #3

> > > 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

Patch

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)
 {
     CXLDCDExtent *extent;
 
@@ -1421,6 +1420,13 @@  void cxl_insert_extent_to_extent_list(CXLDCDExtentList *list,
     QTAILQ_INSERT_TAIL(list, extent, node);
 }
 
+static void cxl_remove_extent_to_extent_list(CXLDCDExtentList *list,
+                                             CXLDCDExtent *ent)
+{
+    QTAILQ_REMOVE(list, ent, node);
+    g_free(ent);
+}
+
 /*
  * CXL r3.0 Table 8-129: Add Dynamic Capacity Response Input Payload
  * CXL r3.0 Table 8-131: Release Dynamic Capacity Input Payload
@@ -1545,14 +1551,15 @@  static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
             }
         }
         if (ent) {
-            QTAILQ_REMOVE(&ct3d->dc.extents_pending_to_add, ent, node);
-            g_free(ent);
+            cxl_remove_extent_to_extent_list(&ct3d->dc.extents_pending_to_add,
+                                             ent);
         } else {
             return CXL_MBOX_INVALID_PA;
         }
 
         cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
         ct3d->dc.total_extent_count += 1;
+        ct3_set_region_block_backed(ct3d, dpa, len);
     }
 
     /*
@@ -1601,16 +1608,22 @@  static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
                 uint64_t len1 = dpa - ent->start_dpa;
                 uint64_t len2 = ent->start_dpa + ent->len - dpa - len;
 
+                cxl_remove_extent_to_extent_list(extent_list, ent);
+                ct3d->dc.total_extent_count -= 1;
+                ct3_clear_region_block_backed(ct3d, dpa, len);
+
                 if (len1) {
                     cxl_insert_extent_to_extent_list(extent_list,
                                                      ent->start_dpa, len1,
                                                      NULL, 0);
                     ct3d->dc.total_extent_count += 1;
+                    ct3_set_region_block_backed(ct3d, dpa, len);
                 }
                 if (len2) {
                     cxl_insert_extent_to_extent_list(extent_list, dpa + len,
                                                      len2, NULL, 0);
                     ct3d->dc.total_extent_count += 1;
+                    ct3_set_region_block_backed(ct3d, dpa, len);
                 }
                 break;
                 /*Currently we reject the attempt to remove a superset*/
@@ -1621,11 +1634,7 @@  static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
             }
         }
 
-        if (ent) {
-            QTAILQ_REMOVE(extent_list, ent, node);
-            g_free(ent);
-            ct3d->dc.total_extent_count -= 1;
-        } else {
+        if (!ent) {
             /* Try to remove a non-existing extent */
             return CXL_MBOX_INVALID_PA;
         }
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
@@ -810,6 +810,7 @@  static int cxl_create_dc_regions(CXLType3Dev *ct3d)
         /* dsmad_handle is set when creating cdat table entries */
         region->flags = 0;
 
+        region->blk_bitmap = bitmap_new(region->len / region->block_size);
         region_base += region->len;
     }
     QTAILQ_INIT(&ct3d->dc.extents);
@@ -818,6 +819,17 @@  static int cxl_create_dc_regions(CXLType3Dev *ct3d)
     return 0;
 }
 
+static void cxl_destroy_dc_regions(CXLType3Dev *ct3d)
+{
+    int i;
+    struct CXLDCDRegion *region;
+
+    for (i = 0; i < ct3d->dc.num_regions; i++) {
+        region = &ct3d->dc.regions[i];
+        g_free(region->blk_bitmap);
+    }
+}
+
 static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
 {
     DeviceState *ds = DEVICE(ct3d);
@@ -1046,6 +1058,7 @@  err_free_special_ops:
     g_free(regs->special_ops);
 err_address_space_free:
     if (ct3d->dc.host_dc) {
+        cxl_destroy_dc_regions(ct3d);
         address_space_destroy(&ct3d->dc.host_dc_as);
     }
     if (ct3d->hostpmem) {
@@ -1068,6 +1081,7 @@  static void ct3_exit(PCIDevice *pci_dev)
     spdm_sock_fini(ct3d->doe_spdm.socket);
     g_free(regs->special_ops);
     if (ct3d->dc.host_dc) {
+        cxl_destroy_dc_regions(ct3d);
         address_space_destroy(&ct3d->dc.host_dc_as);
     }
     if (ct3d->hostpmem) {
@@ -1078,6 +1092,66 @@  static void ct3_exit(PCIDevice *pci_dev)
     }
 }
 
+/*
+ * Mark the DPA range [dpa, dap + len) to be backed and accessible. This
+ * happens when a DC extent is added and accepted by the host.
+ */
+void ct3_set_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+                                 uint64_t len)
+{
+    CXLDCDRegion *region;
+
+    region = cxl_find_dc_region(ct3d, dpa, len);
+    if (!region) {
+        return;
+    }
+
+    bitmap_set(region->blk_bitmap, (dpa - region->base) / region->block_size,
+               len / region->block_size);
+}
+
+/*
+ * 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;
+}
+
+/*
+ * 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);
+}
+
 static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
 {
     int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
@@ -1178,6 +1252,10 @@  static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
         *as = &ct3d->hostpmem_as;
         *dpa_offset -= vmr_size;
     } else {
+        if (!ct3_test_region_block_backed(ct3d, *dpa_offset, size)) {
+            return -ENODEV;
+        }
+
         *as = &ct3d->dc.host_dc_as;
         *dpa_offset -= (vmr_size + pmr_size);
     }
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index ca4f824b11..b71b09700a 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -447,6 +447,7 @@  typedef struct CXLDCDRegion {
     uint64_t block_size;
     uint32_t dsmadhandle;
     uint8_t flags;
+    unsigned long *blk_bitmap;
 } CXLDCDRegion;
 
 struct CXLType3Dev {
@@ -552,9 +553,15 @@  void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d);
 
 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);
 bool test_any_bits_set(const unsigned long *addr, int nr, int size);
+void ct3_set_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+                                  uint64_t len);
+void ct3_clear_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+                                   uint64_t len);
+bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+                                  uint64_t len);
 #endif