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

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

[Yao Xingtao via]

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@fujitsu.com/
Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
---
 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;
     }
-- 
2.37.3

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

[Jonathan Cameron via]

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@fujitsu.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.

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.

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;
>      }

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

[Xingtao Yao (Fujitsu) via]

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;
> >      }

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

[Jonathan Cameron via]

On Thu, 28 Mar 2024 06:24:24 +0000
"Xingtao Yao (Fujitsu)" <yaoxt.fnst@fujitsu.com> wrote:

> 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.

ouch.  We definitely want that check!  The decoder commit will fail
anyway (which QEMU doesn't yet because we don't do all the sanity checks
we should). However failing on commit is nasty as the reason should have
been detected earlier.

> 
> > 
> > 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.

I usually run a bunch of manual tests with devmem2 to ensure the edge cases are handled
correctly, but I've not really seen any errors that didn't also show up in running
stressors (e.g. stressng) or just memhog on the memory.

Jonathan

> 
> > 
> > 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;
> > >      }  
> 

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

[Jonathan Cameron via]

On Mon, 1 Apr 2024 17:00:50 +0100
Jonathan Cameron via <qemu-devel@nongnu.org> wrote:

> On Thu, 28 Mar 2024 06:24:24 +0000
> "Xingtao Yao (Fujitsu)" <yaoxt.fnst@fujitsu.com> wrote:
> 
> > 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.  
> 
> ouch.  We definitely want that check!  The decoder commit will fail
> anyway (which QEMU doesn't yet because we don't do all the sanity checks
> we should). However failing on commit is nasty as the reason should have
> been detected earlier.
> 
> >   
> > > 
> > > 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.  
> 
> I usually run a bunch of manual tests with devmem2 to ensure the edge cases are handled
> correctly, but I've not really seen any errors that didn't also show up in running
> stressors (e.g. stressng) or just memhog on the memory.


Hi Yao,

If you have time, please spin a v2 that also sets the relevant
flag to say the QEMU emulation supports this interleave.

Whilst we test the kernel fixes, we can just drop that patch but longer term I'm
find with having this support in general in the QEMU emulation - so I won't queue
it up as a fix, but instead as a feature.

Thanks,

Jonathan

> 
> Jonathan
> 
> >   
> > > 
> > > 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;
> > > >      }    
> >   
> 
> 

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

[Xingtao Yao (Fujitsu) via]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Saturday, April 6, 2024 12:46 AM
> To: Jonathan Cameron via <qemu-devel@nongnu.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>; Yao, Xingtao/姚 幸涛
> <yaoxt.fnst@fujitsu.com>; fan.ni@samsung.com; Cao, Quanquan/曹 全全
> <caoqq@fujitsu.com>
> Subject: Re: [PATCH] mem/cxl_type3: fix hpa to dpa logic
> 
> On Mon, 1 Apr 2024 17:00:50 +0100
> Jonathan Cameron via <qemu-devel@nongnu.org> wrote:
> 
> > On Thu, 28 Mar 2024 06:24:24 +0000
> > "Xingtao Yao (Fujitsu)" <yaoxt.fnst@fujitsu.com> wrote:
> >
> > > 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.
> >
> > ouch.  We definitely want that check!  The decoder commit will fail
> > anyway (which QEMU doesn't yet because we don't do all the sanity checks
> > we should). However failing on commit is nasty as the reason should have
> > been detected earlier.
> >
> > >
> > > >
> > > > 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.
> >
> > I usually run a bunch of manual tests with devmem2 to ensure the edge cases are
> handled
> > correctly, but I've not really seen any errors that didn't also show up in running
> > stressors (e.g. stressng) or just memhog on the memory.
> 
> 
> Hi Yao,
> 
> If you have time, please spin a v2 that also sets the relevant
> flag to say the QEMU emulation supports this interleave.
OK, I will.
> 
> Whilst we test the kernel fixes, we can just drop that patch but longer term I'm
> find with having this support in general in the QEMU emulation - so I won't queue
> it up as a fix, but instead as a feature.
> 
> Thanks,
> 
> Jonathan
> 
> >
> > Jonathan
> >
> > >
> > > >
> > > > 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;
> > > > >      }
> > >
> >
> >