On Thu, Apr 18, 2024 at 04:10:57PM -0700, nifan....@gmail.com wrote:
> From: Fan Ni <fan...@samsung.com>
>
> Add (file/memory backed) host backend for DCD. 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, the following support is added:
> 1. Add a new property to type3 device "volatile-dc-memdev" to point to host
> memory backend for dynamic capacity. Currently, all DC regions share one
> host backend;
> 2. Add namespace for dynamic capacity for read/write support;
> 3. Create cdat entries for each dynamic capacity region.
>
> Signed-off-by: Fan Ni <fan...@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 16 ++--
> hw/mem/cxl_type3.c | 172 +++++++++++++++++++++++++++++-------
> include/hw/cxl/cxl_device.h | 8 ++
> 3 files changed, 160 insertions(+), 36 deletions(-)
>
A couple general comments in line for discussion, but patch looks good
otherwise. Notes are mostly on improvements we could make that should
not block this patch.
Reviewed-by: Gregory Price <gregory.pr...@memverge.com>
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index a1fe268560..ac87398089 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -45,7 +45,8 @@ enum {
>
> static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> int dsmad_handle, uint64_t size,
> - bool is_pmem, uint64_t dpa_base)
> + bool is_pmem, bool is_dynamic,
> + uint64_t dpa_base)
We should probably change the is_* fields into a flags field and do some
error checking on the combination of flags.
> {
> CDATDsmas *dsmas;
> CDATDslbis *dslbis0;
> @@ -61,7 +62,8 @@ static void 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),
For example, as noted elsewhere in the code, is_pmem+is_dynamic is not
presently supported, so this shouldn't even be allowed in this function.
> + if (dc_mr) {
> + int i;
> + uint64_t region_base = vmr_size + pmr_size;
> +
> + /*
> + * TODO: we assume the dynamic capacity to be volatile for now.
> + * Non-volatile dynamic capacity will be added if needed in the
> + * future.
> + */
Probably don't need to mark this TODO, can just leave it as a note.
Non-volatile dynamic capacity will coincide with shared memory, so it'll
end up handled. So this isn't really a TODO for this current work, and
should read more like:
"Dynamic Capacity is always volatile, until shared memory is
implemented"
> + } else if (ct3d->hostpmem) {
> range1_size_hi = ct3d->hostpmem->size >> 32;
> range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
> (ct3d->hostpmem->size & 0xF0000000);
> + } else {
> + /*
> + * For DCD with no static memory, set memory active, memory class
> bits.
> + * No range is set.
> + */
> + range1_size_lo = (2 << 5) | (2 << 2) | 0x3;
We should probably add defs for these fields at some point. Can be
tabled for later work though.
> + /*
> + * TODO: set dc as volatile for now, non-volatile support can be
> added
> + * in the future if needed.
> + */
> + memory_region_set_nonvolatile(dc_mr, false);
Again can probably drop the TODO and just leave a statement.
~Gregory
