Re: [NDCTL PATCH] cxl/region: Always use the correct target position

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Sun, 2023-07-09 at 19:50 +0800, Xiao Yang wrote:
> Hi all,
> 
> Kindly ping.
> 
> This patch can only fixes the case that 2 way interleave is enabled 
> across 2 CXL host bridges and each host bridge has 1 CXL Root Port.
> PS: In other word, this patch is wrong when 2 way interleave is enabled 
> across 2 CXL host bridges and each host bridge has 2 CXL Root Ports.
> 
> I am trying to find a better solution. If you have any suggestion, 
> please let me know.

Hi Xiao,

The current behavior is that if a list of memdevs is supplied on the
command line, then that ordering is how the targets are assigned. The
plan is to eventually add a mode where cxl-create-region is allowed to
reorder the memdevs to fit the target rules. This should fix the issue
you are hitting, but that's a bigger piece of work. For now the
workaround is that if the automatic ordering fails (and it isn't
guaranteed to work), figure out the right ordering externally (using
cxl-list) and feed the correct order to cxl-create-region.

> 
> Best Regards,
> Xiao Yang
> 
> On 2023/6/30 23:12, Xiao Yang wrote:
> > create_region() uses the wrong target position in some cases.
> > For example, cxl create-region command fails to create a new
> > region in 2 way interleave set when mem0 connects target1(position:1)
> > and mem1 connects target0(position:0):
> > 
> > $ cxl list -M -P -D -T -u
> > [
> >    {
> >      "ports":[
> >        {
> >          "port":"port1",
> >          "host":"pci0000:16",
> >          "depth":1,
> >          "nr_dports":1,
> >          "dports":[
> >            {
> >              "dport":"0000:16:00.0",
> >              "id":"0"
> >            }
> >          ],
> >          "memdevs:port1":[
> >            {
> >              "memdev":"mem0",
> >              "ram_size":"512.00 MiB (536.87 MB)",
> >              "serial":"0",
> >              "host":"0000:17:00.0"
> >            }
> >          ]
> >        },
> >        {
> >          "port":"port2",
> >          "host":"pci0000:0c",
> >          "depth":1,
> >          "nr_dports":1,
> >          "dports":[
> >            {
> >              "dport":"0000:0c:00.0",
> >              "id":"0"
> >            }
> >          ],
> >          "memdevs:port2":[
> >            {
> >              "memdev":"mem1",
> >              "ram_size":"512.00 MiB (536.87 MB)",
> >              "serial":"0",
> >              "host":"0000:0d:00.0"
> >            }
> >          ]
> >        }
> >      ]
> >    },
> >    {
> >      "root decoders":[
> >        {
> >          "decoder":"decoder0.0",
> >          "resource":"0x750000000",
> >          "size":"4.00 GiB (4.29 GB)",
> >          "interleave_ways":2,
> >          "interleave_granularity":8192,
> >          "max_available_extent":"4.00 GiB (4.29 GB)",
> >          "pmem_capable":true,
> >          "volatile_capable":true,
> >          "accelmem_capable":true,
> >          "nr_targets":2,
> >          "targets":[
> >            {
> >              "target":"pci0000:16",
> >              "alias":"ACPI0016:00",
> >              "position":1,
> >              "id":"0x16"
> >            },
> >            {
> >              "target":"pci0000:0c",
> >              "alias":"ACPI0016:01",
> >              "position":0,
> >              "id":"0xc"
> >            }
> >          ]
> >        }
> >      ]
> >    }
> > ]
> > 
> > $ cxl create-region -t ram -d decoder0.0 -m mem0 mem1
> > cxl region: create_region: region0: failed to set target0 to mem0
> > cxl region: cmd_create_region: created 0 regions
> > 
> > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> > ---
> >   cxl/region.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cxl/region.c b/cxl/region.c
> > index 07ce4a3..946b5ff 100644
> > --- a/cxl/region.c
> > +++ b/cxl/region.c
> > @@ -667,6 +667,8 @@ static int create_region(struct cxl_ctx *ctx, int *count,
> >                 struct json_object *jobj =
> >                         json_object_array_get_idx(p->memdevs, i);
> >                 struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> > +               struct cxl_target *target = cxl_decoder_get_target_by_memdev(p->root_decoder,
> > +                                                                               memdev);
> >   
> >                 ep_decoder = cxl_memdev_find_decoder(memdev);
> >                 if (!ep_decoder) {
> > @@ -683,7 +685,7 @@ static int create_region(struct cxl_ctx *ctx, int *count,
> >                         try(cxl_decoder, set_mode, ep_decoder, p->mode);
> >                 }
> >                 try(cxl_decoder, set_dpa_size, ep_decoder, size/p->ways);
> > -               rc = cxl_region_set_target(region, i, ep_decoder);
> > +               rc = cxl_region_set_target(region, cxl_target_get_position(target), ep_decoder);
> >                 if (rc) {
> >                         log_err(&rl, "%s: failed to set target%d to %s\n",
> >                                 devname, i, cxl_memdev_get_devname(memdev));