Re: [PATCH v3 4/9] hw/mem/cxl_type3: Add support to create DC regions to type3 memory devices

[Jonathan Cameron]

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