Re: [PATCH v7 06/12] hw/mem/cxl_type3: Add host backend and address space handling for DC regions

[Gregory Price]

On Thu, Apr 18, 2024 at 04:10:57PM -0700, nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@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.ni@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.price@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