Jonathan thanks for your reply!
> -----Original Message----- > From: Jonathan Cameron <jonathan.came...@huawei.com> > Sent: Wednesday, March 27, 2024 9:28 PM > To: Yao, Xingtao/姚 幸涛 <yaoxt.f...@fujitsu.com> > Cc: fan...@samsung.com; qemu-devel@nongnu.org; Cao, Quanquan/曹 全全 > <ca...@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.f...@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.f...@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; > > }
