| Message ID | 20240221182020.1086096-5-nifan.cxl@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enabling DCD emulation support in Qemu | expand |
On Wed, 21 Feb 2024 10:15:57 -0800 nifan.cxl@gmail.com wrote: > 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 > I'd be tempted to delete the rest of this description. > However, we cannot really read/write to the DC regions due to lack of "However, we cannot yet read/write ..." > 1. host backend and address space setup for DC regions; > 2. mailbox command support for adding/releasing DC extents. "This will be added later in the series." > > Signed-off-by: Fan Ni <fan.ni@samsung.com> Without the above additions it sounds like this patch series only partly does the job :) A forwards reference is good, or just don't mention it. No guarantee of code doing anything useful, beyond not crashing at mid point of a series. Jonathan
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. > + 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
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
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; + } + 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(), };