| Message ID | 20231107180907.553451-5-nifan.cxl@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enabling DCD emulation support in Qemu | expand |
On Tue, 7 Nov 2023 10:07:08 -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 as 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 > > Signed-off-by: Fan Ni <fan.ni@samsung.com> Hi Fan, a few comments inline. Jonathan > --- > hw/mem/cxl_type3.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index 754c885cd1..2d67d2015c 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -721,6 +721,36 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value, > } > } > > +static int cxl_create_dc_regions(CXLType3Dev *ct3d) > +{ > + int i; > + uint64_t region_base = 0; > + uint64_t region_len = 2 * GiB; > + uint64_t decode_len = 8; /* 8*256MB */ If decode len is going to be div 256MiB then we need a name for that field that makes it clear that it is. decode_len_256mbytes or something like that and maybe region_len_bytes to keep things consistent. Why the spec didn't make our life easier and define decode length in bytes with some bits that must be zero is beyond me... I think we need to make this at least optionally configurable or based in some fashion on the provided memory backend (divide that up by number of regions with appropriate rounding perhaps?) > + 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 0; Given it doesn't fail (even after the rest of this series is applied), why return anything? Make it void and we can drop the checks below.. > +} > + > static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp) > { > DeviceState *ds = DEVICE(ct3d); > @@ -789,6 +819,10 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp) > g_free(p_name); > } > > + if (cxl_create_dc_regions(ct3d)) { > + return false; > + } > + > return true; > } > > @@ -1108,6 +1142,7 @@ static Property ct3_props[] = { > DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL), > DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename), > DEFINE_PROP_UINT16("spdm", CXLType3Dev, spdm_port, 0), > + DEFINE_PROP_UINT8("num-dc-regions", CXLType3Dev, dc.num_regions, 0), > DEFINE_PROP_END_OF_LIST(), > }; >
On Wed, 24 Jan 2024 15:23:16 +0000 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Tue, 7 Nov 2023 10:07:08 -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 as 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 > > > > Signed-off-by: Fan Ni <fan.ni@samsung.com> > Hi Fan, a few comments inline. > > Jonathan > > > --- > > hw/mem/cxl_type3.c | 35 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > index 754c885cd1..2d67d2015c 100644 > > --- a/hw/mem/cxl_type3.c > > +++ b/hw/mem/cxl_type3.c > > @@ -721,6 +721,36 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value, > > } > > } > > > > +static int cxl_create_dc_regions(CXLType3Dev *ct3d) > > +{ > > + int i; > > + uint64_t region_base = 0; > > + uint64_t region_len = 2 * GiB; > > + uint64_t decode_len = 8; /* 8*256MB */ > > If decode len is going to be div 256MiB then we need > a name for that field that makes it clear that it is. > > decode_len_256mbytes or something like that and maybe > region_len_bytes to keep things consistent. > > Why the spec didn't make our life easier and define decode length > in bytes with some bits that must be zero is beyond me... > > > I think we need to make this at least optionally configurable or based > in some fashion on the provided memory backend (divide that up > by number of regions with appropriate rounding perhaps?) This seems to be a mid patch set confusion.. It's fixed in patch 7. Whilst applying I've made this 2GiB here. Jonathan
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index 754c885cd1..2d67d2015c 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -721,6 +721,36 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value, } } +static int cxl_create_dc_regions(CXLType3Dev *ct3d) +{ + int i; + uint64_t region_base = 0; + uint64_t region_len = 2 * GiB; + uint64_t decode_len = 8; /* 8*256MB */ + 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 0; +} + static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp) { DeviceState *ds = DEVICE(ct3d); @@ -789,6 +819,10 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp) g_free(p_name); } + if (cxl_create_dc_regions(ct3d)) { + return false; + } + return true; } @@ -1108,6 +1142,7 @@ static Property ct3_props[] = { DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL), DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename), DEFINE_PROP_UINT16("spdm", CXLType3Dev, spdm_port, 0), + DEFINE_PROP_UINT8("num-dc-regions", CXLType3Dev, dc.num_regions, 0), DEFINE_PROP_END_OF_LIST(), };