On Mon, Mar 04, 2024 at 01:10:37PM +0000, Jørgen Hansen wrote:
> On 2/21/24 19:15, nifan....@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...@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...@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
