Re: [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to type3 memory devices

[fan <nifan.cxl@gmail.com>]

On Mon, Mar 04, 2024 at 01:10:37PM +0000, Jørgen Hansen wrote:
> On 2/21/24 19:15, nifan.cxl@gmail.com wrote:
> > CAUTION: This email originated from outside of Western Digital. Do not click on links or open attachments unless you recognize the sender and know that the content is safe.
> > 
> > 
> > From: Fan Ni <fan.ni@samsung.com>
> > 
> > With the change, when setting up memory for type3 memory device, we can
> > create DC regions.
> > A property 'num-dc-regions' is added to ct3_props to allow users to pass the
> > number of DC regions to create. To make it easier, other region parameters
> > like region base, length, and block size are hard coded. If needed,
> > these parameters can be added easily.
> > 
> > With the change, we can create DC regions with proper kernel side
> > support like below:
> > 
> > region=$(cat /sys/bus/cxl/devices/decoder0.0/create_dc_region)
> > echo $region > /sys/bus/cxl/devices/decoder0.0/create_dc_region
> > echo 256 > /sys/bus/cxl/devices/$region/interleave_granularity
> > echo 1 > /sys/bus/cxl/devices/$region/interleave_ways
> > 
> > echo "dc0" >/sys/bus/cxl/devices/decoder2.0/mode
> > echo 0x40000000 >/sys/bus/cxl/devices/decoder2.0/dpa_size
> > 
> > echo 0x40000000 > /sys/bus/cxl/devices/$region/size
> > echo  "decoder2.0" > /sys/bus/cxl/devices/$region/target0
> > echo 1 > /sys/bus/cxl/devices/$region/commit
> > echo $region > /sys/bus/cxl/drivers/cxl_region/bind
> > 
> > However, we cannot really read/write to the DC regions due to lack of
> > 1. host backend and address space setup for DC regions;
> > 2. mailbox command support for adding/releasing DC extents.
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> >   hw/mem/cxl_type3.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 40 insertions(+)
> > 
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 244d2b5fd5..c61cd2b5ac 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -567,6 +567,40 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> >       }
> >   }
> > 
> > +/*
> > + * TODO: dc region configuration will be updated once host backend and address
> > + * space support is added for DCD.
> > + */
> > +static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
> > +{
> > +    int i;
> > +    uint64_t region_base = 0;
> > +    uint64_t region_len =  2 * GiB;
> > +    uint64_t decode_len = 2 * GiB;
> > +    uint64_t blk_size = 2 * MiB;
> > +    CXLDCDRegion *region;
> > +
> > +    if (ct3d->hostvmem) {
> > +        region_base += ct3d->hostvmem->size;
> > +    }
> > +    if (ct3d->hostpmem) {
> > +        region_base += ct3d->hostpmem->size;
> > +    }
> 
> You could use memory_region_size() here instead of accessing field directly.
> 
> Also, it doesn't look like there is any enforcement of hostvmem and 
> hostpmem being a multiple of 256MB, so region_base isn't necessarily 
> aligned to 256MB either. This probably should be fixed separately for 
> vmem and pmem, such that they check the size similar to what you do, as 
> the existing code also assumes that they are multiples of 256MB.

Yeah. I will update the code and add check here to make sure the region
base is aligned with 256MB.

Fan

> 
> > +    for (i = 0; i < ct3d->dc.num_regions; i++) {
> > +        region = &ct3d->dc.regions[i];
> > +        region->base = region_base;
> > +        region->decode_len = decode_len;
> > +        region->len = region_len;
> > +        region->block_size = blk_size;
> > +        /* dsmad_handle is set when creating cdat table entries */
> > +        region->flags = 0;
> > +
> > +        region_base += region->len;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >   static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
> >   {
> >       DeviceState *ds = DEVICE(ct3d);
> > @@ -635,6 +669,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
> >           g_free(p_name);
> >       }
> > 
> > +    if (!cxl_create_dc_regions(ct3d, errp)) {
> > +        error_setg(errp, "setup DC regions failed");
> > +        return false;
> > +    }
> > +
> >       return true;
> >   }
> > 
> > @@ -930,6 +969,7 @@ static Property ct3_props[] = {
> >                        HostMemoryBackend *),
> >       DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL),
> >       DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename),
> > +    DEFINE_PROP_UINT8("num-dc-regions", CXLType3Dev, dc.num_regions, 0),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> > 
> > --
> > 2.43.0
> > 
> > 
> 
> Thanks,
> Jorgen