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 <[email protected]>
---
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:
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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.