RE: [PATCH] mem/cxl_type3: fix hpa to dpa logic

[Xingtao Yao (Fujitsu)]

Jonathan

thanks for your reply!

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Wednesday, March 27, 2024 9:28 PM
> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>
> Cc: fan.ni@samsung.com; qemu-devel@nongnu.org; Cao, Quanquan/曹 全全
> <caoqq@fujitsu.com>
> Subject: Re: [PATCH] mem/cxl_type3: fix hpa to dpa logic
> 
> On Tue, 26 Mar 2024 21:46:53 -0400
> Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote:
> 
> > In 3, 6, 12 interleave ways, we could not access cxl memory properly,
> > and when the process is running on it, a 'segmentation fault' error will
> > occur.
> >
> > According to the CXL specification '8.2.4.20.13 Decoder Protection',
> > there are two branches to convert HPA to DPA:
> > b1: Decoder[m].IW < 8 (for 1, 2, 4, 8, 16 interleave ways)
> > b2: Decoder[m].IW >= 8 (for 3, 6, 12 interleave ways)
> >
> > but only b1 has been implemented.
> >
> > To solve this issue, we should implement b2:
> >   DPAOffset[51:IG+8]=HPAOffset[51:IG+IW] / 3
> >   DPAOffset[IG+7:0]=HPAOffset[IG+7:0]
> >   DPA=DPAOffset + Decoder[n].DPABase
> >
> > Links:
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> u.com/
> > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> 
> Not implementing this was intentional (shouldn't seg fault obviously) but
> I thought we were not advertising EP support for 3, 6, 12?  The HDM Decoder
> configuration checking is currently terrible so we don't prevent
> the bits being set (adding device side sanity checks for those decoders
> has been on the todo list for a long time).  There are a lot of ways of
> programming those that will blow up.
> 
> Can you confirm that the emulation reports they are supported.
> https://elixir.bootlin.com/qemu/v9.0.0-rc1/source/hw/cxl/cxl-component-utils.c
> #L246
> implies it shouldn't and so any software using them is broken.
yes, the feature is not supported by QEMU, but I can still create a 
6-interleave-ways region on kernel layer.

I checked the source code of kernel, and found that the kernel did not check 
this bit when committing decoder.
we may add some check on kernel side.

> 
> The non power of 2 decodes always made me nervous as the maths is more
> complex and any changes to that decode will need careful checking.
> For the power of 2 cases it was a bunch of writes to edge conditions etc
> and checking the right data landed in the backing stores.
after applying this modification, I tested some command by using these memory, 
like 'ls', 'top'..
and they can be executed normally, maybe there are some other problems I 
haven't met yet.

> 
> Joanthan
> 
> 
> > ---
> >  hw/mem/cxl_type3.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index b0a7e9f11b..2c1218fb12 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -805,10 +805,17 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr
> host_addr, uint64_t *dpa)
> >              continue;
> >          }
> >
> > -        *dpa = dpa_base +
> > -            ((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > -             ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) & hpa_offset)
> > -              >> iw));
> > +        if (iw < 8) {
> > +            *dpa = dpa_base +
> > +                ((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > +                 ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> hpa_offset)
> > +                  >> iw));
> > +        } else {
> > +            *dpa = dpa_base +
> > +                ((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > +                 ((((MAKE_64BIT_MASK(ig + iw, 64 - ig - iw) & hpa_offset)
> > +                   >> (ig + iw)) / 3) << (ig + 8)));
> > +        }
> >
> >          return true;
> >      }