| Message ID | 20221107212153.745993-1-fan.ni@samsung.com |
|---|---|
| State | Accepted |
| Commit | f04facfb993de47e2133b2b842d72b97b1c50162 |
| Headers | show |
| Series | cxl/region: Fix memdev reuse check | expand |
Fan Ni wrote: > Memdev reuse in a region currently does not iterate over all of the > interleave targets. Fix this by using existing iterator for memdev reuse > check. Not enough detail, what does this actually fix in practice? For example, if an end user encountered this bug, what would they see as the symptoms? I could probably figure out, but for bugs I should not have to, and more importantly downstream OSV kernel maintainers, who do not have the same context as CXL developers, also need that information to decide if this is a fix they want to backport into their kernel. > > Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders") > Signed-off-by: Fan Ni <fan.ni@samsung.com> > --- > drivers/cxl/core/region.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index f9ae5ad284ff..c7152b4bd9eb 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1226,7 +1226,7 @@ static int cxl_region_attach(struct cxl_region *cxlr, > struct cxl_endpoint_decoder *cxled_target; > struct cxl_memdev *cxlmd_target; > > - cxled_target = p->targets[pos]; > + cxled_target = p->targets[i]; > if (!cxled_target) > continue; > > -- > 2.25.1
On Mon, Nov 07, 2022 at 03:40:36PM -0800, Dan Williams wrote: > Fan Ni wrote: > > Memdev reuse in a region currently does not iterate over all of the > > interleave targets. Fix this by using existing iterator for memdev reuse > > check. > > Not enough detail, what does this actually fix in practice? For example, > if an end user encountered this bug, what would they see as the > symptoms? I could probably figure out, but for bugs I should not have > to, and more importantly downstream OSV kernel maintainers, who do not > have the same context as CXL developers, also need that information to > decide if this is a fix they want to backport into their kernel. > Hi Dan, Thanks for the feedback. Here are more details about the patch, and I will refine the patch. cxlmd_target = cxled_to_memdev(cxled_target); if (cxlmd_target == cxlmd) { dev_dbg(&cxlr->dev, "%s already specified at position %d via: %s\n", dev_name(&cxlmd->dev), pos, dev_name(&cxled_target->cxld.dev)); return -EBUSY; } Before the patch, the check of whether or not a memdev has already been used as a target for the region (above code piece) will always be skipped. Given a memdev with more than one HDM decoder, an interleaved region can be created that maps multiple HPAs to the same DPA. According to CXL spec 3.0 8.1.3.8.4, "Aliasing (mapping more than one Host Physical Address (HPA) to a single Device Physical Address) is forbidden." The CXL specification allows a device to have more than one HDM decoder ("The number of decoders implemented by a component are enumerated via the CXL HDM Decoder Capability register (see Section 8.2.4.19.1"). If a CXL device has multiple HDM decoders the current code allows to create memory regions that map multiple HPAs to a single DPA. Fan > > > > Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders") > > Signed-off-by: Fan Ni <fan.ni@samsung.com> > > --- > > drivers/cxl/core/region.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index f9ae5ad284ff..c7152b4bd9eb 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -1226,7 +1226,7 @@ static int cxl_region_attach(struct cxl_region *cxlr, > > struct cxl_endpoint_decoder *cxled_target; > > struct cxl_memdev *cxlmd_target; > > > > - cxled_target = p->targets[pos]; > > + cxled_target = p->targets[i]; > > if (!cxled_target) > > continue; > > > > -- > > 2.25.1 > >
Fan Ni wrote: > On Mon, Nov 07, 2022 at 03:40:36PM -0800, Dan Williams wrote: > > > Fan Ni wrote: > > > Memdev reuse in a region currently does not iterate over all of the > > > interleave targets. Fix this by using existing iterator for memdev reuse > > > check. > > > > Not enough detail, what does this actually fix in practice? For example, > > if an end user encountered this bug, what would they see as the > > symptoms? I could probably figure out, but for bugs I should not have > > to, and more importantly downstream OSV kernel maintainers, who do not > > have the same context as CXL developers, also need that information to > > decide if this is a fix they want to backport into their kernel. > > > > Hi Dan, > Thanks for the feedback. Here are more details about the patch, and > I will refine the patch. > > cxlmd_target = cxled_to_memdev(cxled_target); > if (cxlmd_target == cxlmd) { > dev_dbg(&cxlr->dev, > "%s already specified at position %d via: %s\n", > dev_name(&cxlmd->dev), pos, > dev_name(&cxled_target->cxld.dev)); > return -EBUSY; > } > > Before the patch, the check of whether or not a memdev has already been > used as a target for the region (above code piece) will always be skipped. > Given a memdev with more than one HDM decoder, an interleaved region can be > created that maps multiple HPAs to the same DPA. According to CXL spec 3.0 > 8.1.3.8.4, "Aliasing (mapping more than one Host Physical Address (HPA) to a > single Device Physical Address) is forbidden." > > The CXL specification allows a device to have more than one HDM decoder > ("The number of decoders implemented by a component are enumerated via the CXL > HDM Decoder Capability register (see Section 8.2.4.19.1"). If a CXL device > has multiple HDM decoders the current code allows to create memory regions > that map multiple HPAs to a single DPA. > > Fan Thanks for this explanation I folded this into the changelog and applied it for v6.2.
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index f9ae5ad284ff..c7152b4bd9eb 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1226,7 +1226,7 @@ static int cxl_region_attach(struct cxl_region *cxlr, struct cxl_endpoint_decoder *cxled_target; struct cxl_memdev *cxlmd_target; - cxled_target = p->targets[pos]; + cxled_target = p->targets[i]; if (!cxled_target) continue;
Memdev reuse in a region currently does not iterate over all of the interleave targets. Fix this by using existing iterator for memdev reuse check. Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders") Signed-off-by: Fan Ni <fan.ni@samsung.com> --- drivers/cxl/core/region.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)