| Message ID | 20230630151245.1318-1-yangx.jy@fujitsu.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [NDCTL] cxl/region: Always use the correct target position | expand |
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. 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));
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));
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));
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(-)