Re: [PATCH v3 5/9] hw/mem/cxl_type3: Add host backend and address space handling for DC regions
> > > *cdat_table = g_steal_pointer();
> > > @@ -445,11 +492,24 @@ static void build_dvsecs(CXLType3Dev *ct3d)
> > > range2_size_hi = ct3d->hostpmem->size >> 32;
> > > range2_size_lo = (2 << 5) | (2 << 2) | 0x3 |
> > > (ct3d->hostpmem->size & 0xF000);
> > > +} else if (ct3d->dc.host_dc) {
> > > +range2_size_hi = ct3d->dc.host_dc->size >> 32;
> > > +range2_size_lo = (2 << 5) | (2 << 2) | 0x3 |
> > > + (ct3d->dc.host_dc->size & 0xF000);
> >
> > I've forgotten if we came to a conclusion on whether these should include
> > DC or not... My gut feeling is no because we don't know what to do
> > if they are both already in use.
> >
>
> QUESTION:
>
> If we do not include DC, and there is no static ram/pmem capacity and
> only dynamic capacity, then the range registers will not be set, is that
> what we want?
I think that's a valid interpretation of the specification.
So for now go with that.
p.s. Sorry for slow response. Debugging had me distracted from catching
up with the list.
Re: [PATCH v3 5/9] hw/mem/cxl_type3: Add host backend and address space handling for DC regions
On Wed, Jan 24, 2024 at 03:47:21PM +, Jonathan Cameron wrote:
> On Tue, 7 Nov 2023 10:07:09 -0800
> nifan@gmail.com wrote:
>
> > From: Fan Ni
> >
> > Add (file/memory backed) host backend, all the dynamic capacity regions
> > will share a single, large enough host backend. Set up address space for
> > DC regions to support read/write operations to dynamic capacity for DCD.
> >
> > With the change, following supports are added:
> > 1. Add a new property to type3 device "nonvolatile-dc-memdev" to point to
> > host
> >memory backend for dynamic capacity. Currently, all dc regions share one
> >one host backend.
> > 2. Add namespace for dynamic capacity for read/write support;
> > 3. Create cdat entries for each dynamic capacity region;
> > 4. Fix dvsec range registers to include DC regions.
> >
> > Signed-off-by: Fan Ni
> Some minor comments inline, mostly suggesting pulling refactors out before
> you do the new stuff.
>
> Thanks,
>
> Jonathan
Hi Jonathan,
One question about DVSEC setting inline.
Please search ""QUESTION:"
>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 16 ++-
> > hw/mem/cxl_type3.c | 198 +---
> > include/hw/cxl/cxl_device.h | 4 +
> > 3 files changed, 179 insertions(+), 39 deletions(-)
> >
>
>
>
> >
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 2d67d2015c..152a51306d 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -31,6 +31,7 @@
> > #include "hw/pci/spdm.h"
> >
> > #define DWORD_BYTE 4
> > +#define CXL_CAPACITY_MULTIPLIER (256 * MiB)
> >
> > /* Default CDAT entries for a memory region */
> > enum {
> > @@ -44,8 +45,9 @@ enum {
> > };
> >
> > static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> > - int dsmad_handle, MemoryRegion
> > *mr,
> > - bool is_pmem, uint64_t dpa_base)
> > + int dsmad_handle, uint64_t size,
> > + bool is_pmem, bool is_dynamic,
> > + uint64_t dpa_base)
> > {
> > g_autofree CDATDsmas *dsmas = NULL;
> > g_autofree CDATDslbis *dslbis0 = NULL;
> > @@ -64,9 +66,10 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader
> > **cdat_table,
> > .length = sizeof(*dsmas),
> > },
> > .DSMADhandle = dsmad_handle,
> > -.flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0,
> > +.flags = (is_pmem ? CDAT_DSMAS_FLAG_NV : 0) |
> > +(is_dynamic ? CDAT_DSMAS_FLAG_DYNAMIC_CAP : 0),
> > .DPA_base = dpa_base,
> > -.DPA_length = memory_region_size(mr),
> > +.DPA_length = size,
> > };
> >
> > /* For now, no memory side cache, plausiblish numbers */
> > @@ -150,7 +153,7 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader
> > **cdat_table,
> > */
> > .EFI_memory_type_attr = is_pmem ? 2 : 1,
> > .DPA_offset = 0,
> > -.DPA_length = memory_region_size(mr),
> > +.DPA_length = size,
> > };
>
> Might be better to make the change to this function as a precursor patch
> before
> you introduce the new users. Will separate the DC bits out from the rest.
>
> >
> > /* Header always at start of structure */
> > @@ -169,21 +172,28 @@ static int ct3_build_cdat_table(CDATSubHeader
> > ***cdat_table, void *priv)
> > g_autofree CDATSubHeader **table = NULL;
> > CXLType3Dev *ct3d = priv;
> > MemoryRegion *volatile_mr = NULL, *nonvolatile_mr = NULL;
> > +MemoryRegion *dc_mr = NULL;
> > int dsmad_handle = 0;
> > int cur_ent = 0;
> > int len = 0;
> > int rc, i;
> > +uint64_t vmr_size = 0, pmr_size = 0;
>
> Put these next to the memory region definitions above given they are
> referring to the
> same regions.
>
> >
> > -if (!ct3d->hostpmem && !ct3d->hostvmem) {
> > +if (!ct3d->hostpmem && !ct3d->hostvmem && !ct3d->dc.num_regions) {
> > return 0;
> > }
> >
> > +if (ct3d->hostpmem && ct3d->hostvmem && ct3d->dc.host_dc) {
> > +warn_report("The device has static ram and pmem and dynamic
> > capacity");
>
> This is the whole how many DVSEC ranges question?
> I hope we resolved that so we don't care about this...
>
> > +}
> > +
> > if (ct3d->hostvmem) {
> > volatile_mr = host_memory_backend_get_memory(ct3d->hostvmem);
> > if (!volatile_mr) {
> > return -EINVAL;
> > }
> > len += CT3_CDAT_NUM_ENTRIES;
> > +vmr_size = memory_region_size(volatile_mr);
> > }
> >
> > if (ct3d->hostpmem) {
>
>
>
> > @@ -210,14 +233,38 @@ static int ct3_build_cdat_table(CDATSubHeader
> > ***cdat_table, void *priv)
> > }
> >
> > if (nonvolatile_mr) {
> > -uint64_t base = volatile_mr ? memory_region_size(volatile_mr) : 0;
> > rc =
Re: [PATCH v3 5/9] hw/mem/cxl_type3: Add host backend and address space handling for DC regions
On Tue, 7 Nov 2023 10:07:09 -0800
nifan@gmail.com wrote:
> From: Fan Ni
>
> Add (file/memory backed) host backend, all the dynamic capacity regions
> will share a single, large enough host backend. Set up address space for
> DC regions to support read/write operations to dynamic capacity for DCD.
>
> With the change, following supports are added:
> 1. Add a new property to type3 device "nonvolatile-dc-memdev" to point to host
>memory backend for dynamic capacity. Currently, all dc regions share one
>one host backend.
> 2. Add namespace for dynamic capacity for read/write support;
> 3. Create cdat entries for each dynamic capacity region;
> 4. Fix dvsec range registers to include DC regions.
>
> Signed-off-by: Fan Ni
Some minor comments inline, mostly suggesting pulling refactors out before
you do the new stuff.
Thanks,
Jonathan
> ---
> hw/cxl/cxl-mailbox-utils.c | 16 ++-
> hw/mem/cxl_type3.c | 198 +---
> include/hw/cxl/cxl_device.h | 4 +
> 3 files changed, 179 insertions(+), 39 deletions(-)
>
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 2d67d2015c..152a51306d 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -31,6 +31,7 @@
> #include "hw/pci/spdm.h"
>
> #define DWORD_BYTE 4
> +#define CXL_CAPACITY_MULTIPLIER (256 * MiB)
>
> /* Default CDAT entries for a memory region */
> enum {
> @@ -44,8 +45,9 @@ enum {
> };
>
> static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> - int dsmad_handle, MemoryRegion *mr,
> - bool is_pmem, uint64_t dpa_base)
> + int dsmad_handle, uint64_t size,
> + bool is_pmem, bool is_dynamic,
> + uint64_t dpa_base)
> {
> g_autofree CDATDsmas *dsmas = NULL;
> g_autofree CDATDslbis *dslbis0 = NULL;
> @@ -64,9 +66,10 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader
> **cdat_table,
> .length = sizeof(*dsmas),
> },
> .DSMADhandle = dsmad_handle,
> -.flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0,
> +.flags = (is_pmem ? CDAT_DSMAS_FLAG_NV : 0) |
> +(is_dynamic ? CDAT_DSMAS_FLAG_DYNAMIC_CAP : 0),
> .DPA_base = dpa_base,
> -.DPA_length = memory_region_size(mr),
> +.DPA_length = size,
> };
>
> /* For now, no memory side cache, plausiblish numbers */
> @@ -150,7 +153,7 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader
> **cdat_table,
> */
> .EFI_memory_type_attr = is_pmem ? 2 : 1,
> .DPA_offset = 0,
> -.DPA_length = memory_region_size(mr),
> +.DPA_length = size,
> };
Might be better to make the change to this function as a precursor patch before
you introduce the new users. Will separate the DC bits out from the rest.
>
> /* Header always at start of structure */
> @@ -169,21 +172,28 @@ static int ct3_build_cdat_table(CDATSubHeader
> ***cdat_table, void *priv)
> g_autofree CDATSubHeader **table = NULL;
> CXLType3Dev *ct3d = priv;
> MemoryRegion *volatile_mr = NULL, *nonvolatile_mr = NULL;
> +MemoryRegion *dc_mr = NULL;
> int dsmad_handle = 0;
> int cur_ent = 0;
> int len = 0;
> int rc, i;
> +uint64_t vmr_size = 0, pmr_size = 0;
Put these next to the memory region definitions above given they are referring
to the
same regions.
>
> -if (!ct3d->hostpmem && !ct3d->hostvmem) {
> +if (!ct3d->hostpmem && !ct3d->hostvmem && !ct3d->dc.num_regions) {
> return 0;
> }
>
> +if (ct3d->hostpmem && ct3d->hostvmem && ct3d->dc.host_dc) {
> +warn_report("The device has static ram and pmem and dynamic
> capacity");
This is the whole how many DVSEC ranges question?
I hope we resolved that so we don't care about this...
> +}
> +
> if (ct3d->hostvmem) {
> volatile_mr = host_memory_backend_get_memory(ct3d->hostvmem);
> if (!volatile_mr) {
> return -EINVAL;
> }
> len += CT3_CDAT_NUM_ENTRIES;
> +vmr_size = memory_region_size(volatile_mr);
> }
>
> if (ct3d->hostpmem) {
> @@ -210,14 +233,38 @@ static int ct3_build_cdat_table(CDATSubHeader
> ***cdat_table, void *priv)
> }
>
> if (nonvolatile_mr) {
> -uint64_t base = volatile_mr ? memory_region_size(volatile_mr) : 0;
> rc = ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
> - nonvolatile_mr, true, base);
> + pmr_size, true, false, vmr_size);
> if (rc < 0) {
> goto error_cleanup;
> }
> cur_ent += CT3_CDAT_NUM_ENTRIES;
> }
> +
> +if (dc_mr) {
> +uint64_t region_base = vmr_size + pmr_size;
> +
> +/*
> +
[PATCH v3 5/9] hw/mem/cxl_type3: Add host backend and address space handling for DC regions
From: Fan Ni
Add (file/memory backed) host backend, all the dynamic capacity regions
will share a single, large enough host backend. Set up address space for
DC regions to support read/write operations to dynamic capacity for DCD.
With the change, following supports are added:
1. Add a new property to type3 device "nonvolatile-dc-memdev" to point to host
memory backend for dynamic capacity. Currently, all dc regions share one
one host backend.
2. Add namespace for dynamic capacity for read/write support;
3. Create cdat entries for each dynamic capacity region;
4. Fix dvsec range registers to include DC regions.
Signed-off-by: Fan Ni
---
hw/cxl/cxl-mailbox-utils.c | 16 ++-
hw/mem/cxl_type3.c | 198 +---
include/hw/cxl/cxl_device.h | 4 +
3 files changed, 179 insertions(+), 39 deletions(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 707fd9fe7f..1f512b3e6b 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -596,7 +596,8 @@ static CXLRetCode cmd_firmware_update_get_info(const struct
cxl_cmd *cmd,
size_t *len_out,
CXLCCI *cci)
{
-CXLDeviceState *cxl_dstate = _TYPE3(cci->d)->cxl_dstate;
+CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+CXLDeviceState *cxl_dstate = >cxl_dstate;
struct {
uint8_t slots_supported;
uint8_t slot_info;
@@ -610,7 +611,8 @@ static CXLRetCode cmd_firmware_update_get_info(const struct
cxl_cmd *cmd,
QEMU_BUILD_BUG_ON(sizeof(*fw_info) != 0x50);
if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
-(cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {
+(cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER) ||
+(ct3d->dc.total_capacity < CXL_CAPACITY_MULTIPLIER)) {
return CXL_MBOX_INTERNAL_ERROR;
}
@@ -764,7 +766,8 @@ static CXLRetCode cmd_identify_memory_device(const struct
cxl_cmd *cmd,
CXLDeviceState *cxl_dstate = >cxl_dstate;
if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) ||
-(!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) {
+(!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER)) ||
+(!QEMU_IS_ALIGNED(ct3d->dc.total_capacity, CXL_CAPACITY_MULTIPLIER))) {
return CXL_MBOX_INTERNAL_ERROR;
}
@@ -805,9 +808,11 @@ static CXLRetCode cmd_ccls_get_partition_info(const struct
cxl_cmd *cmd,
uint64_t next_pmem;
} QEMU_PACKED *part_info = (void *)payload_out;
QEMU_BUILD_BUG_ON(sizeof(*part_info) != 0x20);
+CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) ||
-(!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) {
+(!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER)) ||
+(!QEMU_IS_ALIGNED(ct3d->dc.total_capacity, CXL_CAPACITY_MULTIPLIER))) {
return CXL_MBOX_INTERNAL_ERROR;
}
@@ -1149,7 +1154,8 @@ static CXLRetCode cmd_media_clear_poison(const struct
cxl_cmd *cmd,
struct clear_poison_pl *in = (void *)payload_in;
dpa = ldq_le_p(>dpa);
-if (dpa + CXL_CACHE_LINE_SIZE > cxl_dstate->static_mem_size) {
+if (dpa + CXL_CACHE_LINE_SIZE > cxl_dstate->static_mem_size +
+ct3d->dc.total_capacity) {
return CXL_MBOX_INVALID_PA;
}
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 2d67d2015c..152a51306d 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -31,6 +31,7 @@
#include "hw/pci/spdm.h"
#define DWORD_BYTE 4
+#define CXL_CAPACITY_MULTIPLIER (256 * MiB)
/* Default CDAT entries for a memory region */
enum {
@@ -44,8 +45,9 @@ enum {
};
static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
- int dsmad_handle, MemoryRegion *mr,
- bool is_pmem, uint64_t dpa_base)
+ int dsmad_handle, uint64_t size,
+ bool is_pmem, bool is_dynamic,
+ uint64_t dpa_base)
{
g_autofree CDATDsmas *dsmas = NULL;
g_autofree CDATDslbis *dslbis0 = NULL;
@@ -64,9 +66,10 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader
**cdat_table,
.length = sizeof(*dsmas),
},
.DSMADhandle = dsmad_handle,
-.flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0,
+.flags = (is_pmem ? CDAT_DSMAS_FLAG_NV : 0) |
+(is_dynamic ? CDAT_DSMAS_FLAG_DYNAMIC_CAP : 0),
.DPA_base = dpa_base,
-.DPA_length = memory_region_size(mr),
+.DPA_length = size,
};
/* For now, no memory side cache, plausiblish numbers */
@@ -150,7 +153,7 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader
**cdat_table,
