RE: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
ping again.
> -Original Message-
> From: Yao, Xingtao/姚 幸涛
> Sent: Friday, May 24, 2024 5:31 PM
> To: Yao, Xingtao/姚 幸涛 ;
> jonathan.came...@huawei.com; fan...@samsung.com
> Cc: qemu-devel@nongnu.org
> Subject: RE: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> ping.
>
> > -Original Message-
> > From: Yao Xingtao
> > Sent: Wednesday, May 8, 2024 8:53 AM
> > To: jonathan.came...@huawei.com; fan...@samsung.com
> > Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
> > Subject: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
> >
> > Since the kernel does not check the interleave capability, a
> > 3-way, 6-way, 12-way or 16-way region can be create normally.
> >
> > Applications can access the memory of 16-way region normally because
> > qemu can convert hpa to dpa correctly for the power of 2 interleave
> > ways, after kernel implementing the check, this kind of region will
> > not be created any more.
> >
> > For non power of 2 interleave ways, applications could not access the
> > memory normally and may occur some unexpected behaviors, such as
> > segmentation fault.
> >
> > So implements this feature is needed.
> >
> > Link:
> >
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> > u.com/
> > Signed-off-by: Yao Xingtao
> > ---
> > hw/cxl/cxl-component-utils.c | 9 +++--
> > hw/mem/cxl_type3.c | 15 +++
> > 2 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index cd116c0401..473895948b 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -243,8 +243,13 @@ static void hdm_init_common(uint32_t *reg_state,
> > uint32_t *write_msk,
> > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > INTERLEAVE_4K, 1);
> > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > POISON_ON_ERR_CAP, 0);
> > -ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > 3_6_12_WAY, 0);
> > -ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 16_WAY,
> > 0);
> > +if (type == CXL2_TYPE3_DEVICE) {
> > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > 3_6_12_WAY, 1);
> > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > 16_WAY, 1);
> > +} else {
> > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > 3_6_12_WAY, 0);
> > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > 16_WAY, 0);
> > +}
> > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, UIO,
> 0);
> > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > UIO_DECODER_COUNT, 0);
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 3e42490b6c..b755318838 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -804,10 +804,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 v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
ping.
> -Original Message-
> From: Yao Xingtao
> Sent: Wednesday, May 8, 2024 8:53 AM
> To: jonathan.came...@huawei.com; fan...@samsung.com
> Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
> Subject: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> Since the kernel does not check the interleave capability, a
> 3-way, 6-way, 12-way or 16-way region can be create normally.
>
> Applications can access the memory of 16-way region normally because
> qemu can convert hpa to dpa correctly for the power of 2 interleave
> ways, after kernel implementing the check, this kind of region will
> not be created any more.
>
> For non power of 2 interleave ways, applications could not access the
> memory normally and may occur some unexpected behaviors, such as
> segmentation fault.
>
> So implements this feature is needed.
>
> Link:
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> u.com/
> Signed-off-by: Yao Xingtao
> ---
> hw/cxl/cxl-component-utils.c | 9 +++--
> hw/mem/cxl_type3.c | 15 +++
> 2 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index cd116c0401..473895948b 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -243,8 +243,13 @@ static void hdm_init_common(uint32_t *reg_state,
> uint32_t *write_msk,
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> INTERLEAVE_4K, 1);
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> POISON_ON_ERR_CAP, 0);
> -ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 3_6_12_WAY, 0);
> -ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, 16_WAY,
> 0);
> +if (type == CXL2_TYPE3_DEVICE) {
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 3_6_12_WAY, 1);
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 16_WAY, 1);
> +} else {
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 3_6_12_WAY, 0);
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 16_WAY, 0);
> +}
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, UIO, 0);
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> UIO_DECODER_COUNT, 0);
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 3e42490b6c..b755318838 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -804,10 +804,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 v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
> -Original Message-
> From: Jonathan Cameron
> Sent: Wednesday, May 8, 2024 12:31 AM
> To: Yao, Xingtao/姚 幸涛
> Cc: fan...@samsung.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> On Tue, 7 May 2024 00:22:00 +
> "Xingtao Yao (Fujitsu)" wrote:
>
> > > -Original Message-
> > > From: Jonathan Cameron
> > > Sent: Tuesday, April 30, 2024 10:43 PM
> > > To: Yao, Xingtao/姚 幸涛
> > > Cc: fan...@samsung.com; qemu-devel@nongnu.org
> > > Subject: Re: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave
> ways
> > >
> > > On Wed, 24 Apr 2024 01:36:56 +
> > > "Xingtao Yao (Fujitsu)" wrote:
> > >
> > > > ping.
> > > >
> > > > > -Original Message-
> > > > > From: Yao Xingtao
> > > > > Sent: Sunday, April 7, 2024 11:07 AM
> > > > > To: jonathan.came...@huawei.com; fan...@samsung.com
> > > > > Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
> > >
> > > > > Subject: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave
> ways
> > > > >
> > > > > Since the kernel does not check the interleave capability, a
> > > > > 3-way, 6-way, 12-way or 16-way region can be create normally.
> > > > >
> > > > > Applications can access the memory of 16-way region normally because
> > > > > qemu can convert hpa to dpa correctly for the power of 2 interleave
> > > > > ways, after kernel implementing the check, this kind of region will
> > > > > not be created any more.
> > > > >
> > > > > For non power of 2 interleave ways, applications could not access the
> > > > > memory normally and may occur some unexpected behaviors, such as
> > > > > segmentation fault.
> > > > >
> > > > > So implements this feature is needed.
> > > > >
> > > > > Link:
> > > > >
> > >
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> > > > > u.com/
> > > > > Signed-off-by: Yao Xingtao
> > > > > ---
> > > > > hw/mem/cxl_type3.c | 18 ++
> > > > > 1 file changed, 14 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > > > index b0a7e9f11b..d6ef784e96 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;
> > > > > }
> > > > > @@ -906,6 +913,9 @@ static void ct3d_reset(DeviceState *dev)
> > > > > uint32_t *write_msk =
> > > ct3d->cxl_cstate.crb.cache_mem_regs_write_mask;
> > > > >
> > > > > cxl_component_register_init_common(reg_state, write_msk,
> > > > > CXL2_TYPE3_DEVICE);
> > > > > +ARRAY_FIELD_DP32(reg_state,
> CXL_HDM_DECODER_CAPABILITY,
> > > > > 3_6_12_WAY, 1);
> > > > > +ARRAY_FIELD_DP32(reg_state,
> CXL_HDM_DECODER_CAPABILITY,
> > > > > 16_WAY, 1);
> > > > > +
> > >
> > > Why here rather than in hdm_reg_init_common()?
> > > It's constant data and is currently being set to 0 in there.
> >
> > according to the CXL specifications (8.2.4.20.1 CXL HDM Decoder Capability
> Register (Offset 00h)),
> > this feature is only applicable to cxl.mem, upstream switch port and CXL
> > host
> bridges shall hardwrite
> > these bits to 0.
> >
> > so I think it would be more appropriate to set these bits here.
> I don't follow. hdm_init_common() (sorry wrong function name above)
> has some type specific stuff already to show how this can be done.
> I'd prefer to minimize what we set directly in the ct3d_reset() call
> because it loses the connection to the rest of the register setup.
thanks, got it.
>
> Jonathan
>
>
>
> Jonathan
>
>
> >
> > >
> > > > > cxl_device_register_init_t3(ct3d);
> > > > >
> > > > > /*
> > > > > --
> > > > > 2.37.3
> > > >
> >
RE: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
> -Original Message-
> From: Jonathan Cameron
> Sent: Tuesday, April 30, 2024 10:43 PM
> To: Yao, Xingtao/姚 幸涛
> Cc: fan...@samsung.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> On Wed, 24 Apr 2024 01:36:56 +
> "Xingtao Yao (Fujitsu)" wrote:
>
> > ping.
> >
> > > -Original Message-
> > > From: Yao Xingtao
> > > Sent: Sunday, April 7, 2024 11:07 AM
> > > To: jonathan.came...@huawei.com; fan...@samsung.com
> > > Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
>
> > > Subject: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
> > >
> > > Since the kernel does not check the interleave capability, a
> > > 3-way, 6-way, 12-way or 16-way region can be create normally.
> > >
> > > Applications can access the memory of 16-way region normally because
> > > qemu can convert hpa to dpa correctly for the power of 2 interleave
> > > ways, after kernel implementing the check, this kind of region will
> > > not be created any more.
> > >
> > > For non power of 2 interleave ways, applications could not access the
> > > memory normally and may occur some unexpected behaviors, such as
> > > segmentation fault.
> > >
> > > So implements this feature is needed.
> > >
> > > Link:
> > >
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> > > u.com/
> > > Signed-off-by: Yao Xingtao
> > > ---
> > > hw/mem/cxl_type3.c | 18 ++
> > > 1 file changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > index b0a7e9f11b..d6ef784e96 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;
> > > }
> > > @@ -906,6 +913,9 @@ static void ct3d_reset(DeviceState *dev)
> > > uint32_t *write_msk =
> ct3d->cxl_cstate.crb.cache_mem_regs_write_mask;
> > >
> > > cxl_component_register_init_common(reg_state, write_msk,
> > > CXL2_TYPE3_DEVICE);
> > > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > > 3_6_12_WAY, 1);
> > > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > > 16_WAY, 1);
> > > +
>
> Why here rather than in hdm_reg_init_common()?
> It's constant data and is currently being set to 0 in there.
according to the CXL specifications (8.2.4.20.1 CXL HDM Decoder Capability
Register (Offset 00h)),
this feature is only applicable to cxl.mem, upstream switch port and CXL host
bridges shall hardwrite
these bits to 0.
so I think it would be more appropriate to set these bits here.
>
> > > cxl_device_register_init_t3(ct3d);
> > >
> > > /*
> > > --
> > > 2.37.3
> >
RE: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
ping.
> -Original Message-
> From: Yao Xingtao
> Sent: Sunday, April 7, 2024 11:07 AM
> To: jonathan.came...@huawei.com; fan...@samsung.com
> Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
> Subject: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> Since the kernel does not check the interleave capability, a
> 3-way, 6-way, 12-way or 16-way region can be create normally.
>
> Applications can access the memory of 16-way region normally because
> qemu can convert hpa to dpa correctly for the power of 2 interleave
> ways, after kernel implementing the check, this kind of region will
> not be created any more.
>
> For non power of 2 interleave ways, applications could not access the
> memory normally and may occur some unexpected behaviors, such as
> segmentation fault.
>
> So implements this feature is needed.
>
> Link:
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> u.com/
> Signed-off-by: Yao Xingtao
> ---
> hw/mem/cxl_type3.c | 18 ++
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b0a7e9f11b..d6ef784e96 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;
> }
> @@ -906,6 +913,9 @@ static void ct3d_reset(DeviceState *dev)
> uint32_t *write_msk = ct3d->cxl_cstate.crb.cache_mem_regs_write_mask;
>
> cxl_component_register_init_common(reg_state, write_msk,
> CXL2_TYPE3_DEVICE);
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 3_6_12_WAY, 1);
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 16_WAY, 1);
> +
> cxl_device_register_init_t3(ct3d);
>
> /*
> --
> 2.37.3
RE: [PATCH] mem/cxl_type3: fix hpa to dpa logic
> -Original Message- > From: Jonathan Cameron > Sent: Saturday, April 6, 2024 12:46 AM > To: Jonathan Cameron via > Cc: Jonathan Cameron ; Yao, Xingtao/姚 幸涛 > ; fan...@samsung.com; Cao, Quanquan/曹 全全 > > Subject: Re: [PATCH] mem/cxl_type3: fix hpa to dpa logic > > On Mon, 1 Apr 2024 17:00:50 +0100 > Jonathan Cameron via wrote: > > > On Thu, 28 Mar 2024 06:24:24 + > > "Xingtao Yao (Fujitsu)" wrote: > > > > > Jonathan > > > > > > thanks for your reply! > > > > > > > -Original Message- > > > > From: Jonathan Cameron > > > > Sent: Wednesday, March 27, 2024 9:28 PM > > > > To: Yao, Xingtao/姚 幸涛 > > > > Cc: fan...@samsung.com; qemu-devel@nongnu.org; Cao, Quanquan/曹 全 > 全 > > > > > > > > Subject: Re: [PATCH] mem/cxl_type3: fix hpa to dpa logic > > > > > > > > On Tue, 26 Mar 2024 21:46:53 -0400 > > > > Yao Xingtao 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 > > > > > > > > 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
RE: [PATCH] mem/cxl_type3: fix hpa to dpa logic
Jonathan
thanks for your reply!
> -Original Message-
> From: Jonathan Cameron
> Sent: Wednesday, March 27, 2024 9:28 PM
> To: Yao, Xingtao/姚 幸涛
> Cc: fan...@samsung.com; qemu-devel@nongnu.org; Cao, Quanquan/曹 全全
>
> Subject: Re: [PATCH] mem/cxl_type3: fix hpa to dpa logic
>
> On Tue, 26 Mar 2024 21:46:53 -0400
> Yao Xingtao 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
>
> 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 v3] contrib/plugins/execlog: Fix compiler warning
> -Original Message-
> From: Philippe Mathieu-Daudé
> Sent: Tuesday, March 26, 2024 8:04 PM
> To: Peter Maydell
> Cc: Pierrick Bouvier ; Yao, Xingtao/姚 幸涛
> ; qemu-devel@nongnu.org; alex.ben...@linaro.org;
> erdn...@crans.org; ma.mando...@gmail.com
> Subject: Re: [PATCH v3] contrib/plugins/execlog: Fix compiler warning
>
> On 26/3/24 11:33, Peter Maydell wrote:
> > On Tue, 26 Mar 2024 at 09:54, Philippe Mathieu-Daudé
> wrote:
> >>
> >> On 26/3/24 04:33, Pierrick Bouvier wrote:
> >>> On 3/26/24 05:52, Yao Xingtao wrote:
> 1. The g_pattern_match_string() is deprecated when glib2 version >=
> 2.70.
> Use g_pattern_spec_match_string() instead to avoid this
> problem.
>
> 2. The type of second parameter in g_ptr_array_add() is
> 'gpointer' {aka 'void *'}, but the type of reg->name is 'const
> char*'.
> Cast the type of reg->name to 'gpointer' to avoid this problem.
>
> compiler warning message:
> /root/qemu/contrib/plugins/execlog.c:330:17: warning:
> ‘g_pattern_match_string’
> is deprecated: Use 'g_pattern_spec_match_string'
> instead [-Wdeprecated-declarations]
> 330 | if (g_pattern_match_string(pat, rd->name)
> ||
> | ^~
> In file included from /usr/include/glib-2.0/glib.h:67,
> from /root/qemu/contrib/plugins/execlog.c:9:
> /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> 57 | gboolean g_pattern_match_string (GPatternSpec
> *pspec,
> | ^~
> /root/qemu/contrib/plugins/execlog.c:331:21: warning:
> ‘g_pattern_match_string’
> is deprecated: Use 'g_pattern_spec_match_string'
> instead [-Wdeprecated-declarations]
> 331 | g_pattern_match_string(pat, rd_lower))
> {
> | ^~
> /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> 57 | gboolean g_pattern_match_string (GPatternSpec
> *pspec,
> | ^~
> /root/qemu/contrib/plugins/execlog.c:339:63: warning: passing
> argument
> 2 of
> ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target
> type [-Wdiscarded-qualifiers]
> 339 |
> g_ptr_array_add(all_reg_names,
> reg->name);
> |
> ~~~^~
> In file included from /usr/include/glib-2.0/glib.h:33:
> /usr/include/glib-2.0/glib/garray.h:198:62: note: expected
> ‘gpointer’ {aka ‘void *’} but argument is of type ‘const char *’
> 198 |gpointer
> data);
> |
> ~~^~~~
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2210
> Signed-off-by: Yao Xingtao
> ---
> contrib/plugins/execlog.c | 24 +---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
> index a1dfd59ab7..fab18113d4 100644
> --- a/contrib/plugins/execlog.c
> +++ b/contrib/plugins/execlog.c
> @@ -311,6 +311,24 @@ static Register
> *init_vcpu_register(qemu_plugin_reg_descriptor *desc)
> return reg;
> }
> +/*
> + * g_pattern_match_string has been deprecated in Glib since 2.70
> +and
> + * will complain about it if you try to use it. Fortunately the
> + * signature of both functions is the same making it easy to work
> + * around.
> + */
> +static inline
> +gboolean g_pattern_spec_match_string_qemu(GPatternSpec *pspec,
> + const gchar *string) {
> +#if GLIB_CHECK_VERSION(2, 70, 0)
> +return g_pattern_spec_match_string(pspec, string); #else
> +return g_pattern_match_string(pspec, string); #endif };
> +#define g_pattern_spec_match_string(p, s)
> g_pattern_spec_match_string_qemu(p, s)
> +
> static GPtrArray *registers_init(int vcpu_index)
> {
> g_autoptr(GPtrArray) registers = g_ptr_array_new(); @@
> -327,8 +345,8 @@ static GPtrArray *registers_init(int vcpu_index)
> for (int p = 0; p < rmatches->len; p++) {
> g_autoptr(GPatternSpec) pat =
> g_pattern_spec_new(rmatches->pdata[p]);
> g_autofree gchar *rd_lower =
> g_utf8_strdown(rd->name, -1);
> -if (g_pattern_match_string(pat, rd->name) ||
> -g_pattern_match_string(pat, rd_lower)) {
> +if (g_pattern_spec_match_string(pat, rd->name) ||
> +g_pattern_spec_match_string(pat, rd_lower)) {
> Register *reg = init_vcpu_register(rd);
> g_ptr_array_add(registers,
RE: [PATCH] contrib/plugins/execlog: Fix compiler warning
Hi Pierrick,
thanks for your reply, and I will modify and push the patch later.
thanks
Xingtao
> -Original Message-
> From: Pierrick Bouvier
> Sent: Monday, March 25, 2024 12:31 PM
> To: Yao, Xingtao/姚 幸涛 ; Peter Maydell
>
> Cc: alex.ben...@linaro.org; erdn...@crans.org; ma.mando...@gmail.com;
> qemu-devel@nongnu.org
> Subject: Re: [PATCH] contrib/plugins/execlog: Fix compiler warning
>
> On 3/25/24 07:00, Xingtao Yao (Fujitsu) wrote:
> > Pete:
> > Thanks for your comment.
> >
> > I also find a similar patch written by Pierrick:
> > https://lore.kernel.org/qemu-devel/20240118032400.3762658-15-pierrick.
> > bouv...@linaro.org/ but for some reason, the patch was not merged yet.
> >
> > shall I need to continue tracking the fixes of this bug?
> >
>
> Hi Xingtao,
>
> you're doing the right thing here. In my original patch, there was no
> consideration for backward compatibility, to the opposite of what you did.
>
> Alex will be out for several weeks, so it might take some time to get this
> merged,
> but I'm definitely voting for this .
>
> Pierrick
>
> >> -Original Message-
> >> From: Peter Maydell
> >> Sent: Friday, March 22, 2024 7:50 PM
> >> To: Yao, Xingtao/姚 幸涛
> >> Cc: alex.ben...@linaro.org; erdn...@crans.org; ma.mando...@gmail.com;
> >> pierrick.bouv...@linaro.org; qemu-devel@nongnu.org
> >> Subject: Re: [PATCH] contrib/plugins/execlog: Fix compiler warning
> >>
> >> On Wed, 20 Mar 2024 at 02:05, Yao Xingtao via
> >> wrote:
> >>>
> >>> 1. The g_pattern_match_string() is deprecated when glib2 version >=
> 2.70.
> >>> Use g_pattern_spec_match_string() instead to avoid this problem.
> >>>
> >>> 2. The type of second parameter in g_ptr_array_add() is
> >>> 'gpointer' {aka 'void *'}, but the type of reg->name is 'const char*'.
> >>> Cast the type of reg->name to 'gpointer' to avoid this problem.
> >>>
> >>> compiler warning message:
> >>> /root/qemu/contrib/plugins/execlog.c:330:17: warning:
> >> ‘g_pattern_match_string’
> >>> is deprecated: Use 'g_pattern_spec_match_string'
> >>> instead [-Wdeprecated-declarations]
> >>>330 | if (g_pattern_match_string(pat, rd->name) ||
> >>>| ^~
> >>> In file included from /usr/include/glib-2.0/glib.h:67,
> >>> from /root/qemu/contrib/plugins/execlog.c:9:
> >>> /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> >>> 57 | gboolean g_pattern_match_string (GPatternSpec
> *pspec,
> >>>| ^~
> >>> /root/qemu/contrib/plugins/execlog.c:331:21: warning:
> >> ‘g_pattern_match_string’
> >>> is deprecated: Use 'g_pattern_spec_match_string'
> >>> instead [-Wdeprecated-declarations]
> >>>331 | g_pattern_match_string(pat, rd_lower)) {
> >>>| ^~
> >>> /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> >>> 57 | gboolean g_pattern_match_string (GPatternSpec
> *pspec,
> >>>| ^~
> >>> /root/qemu/contrib/plugins/execlog.c:339:63: warning: passing
> >>> argument
> >>> 2 of ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer
> >>> target type [-Wdiscarded-qualifiers]
> >>>339 | g_ptr_array_add(all_reg_names,
> >> reg->name);
> >>>|
> >> ~~~^~
> >>> In file included from /usr/include/glib-2.0/glib.h:33:
> >>> /usr/include/glib-2.0/glib/garray.h:198:62: note: expected ‘gpointer’
> >>> {aka ‘void *’} but argument is of type ‘const char *’
> >>>198 |gpointer
> >> data);
> >>>|
> >> ~~^~~~
> >>>
> >>
> >> Hi; thanks for this patch.
> >>
> >> This fixes a bug reported in the bug tracker so we should put
> >>
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2210
> >>
> >> in the commit message just above your signed-off-by tag.
> >>
> >>
> >>> Signed-off-by: Yao Xingtao
> > I will if needed.
> >
> >>> ---
> >>> contrib/plugins/execlog.c
RE: [PATCH] contrib/plugins/execlog: Fix compiler warning
Pete:
Thanks for your comment.
I also find a similar patch written by Pierrick:
https://lore.kernel.org/qemu-devel/20240118032400.3762658-15-pierrick.bouv...@linaro.org/
but for some reason, the patch was not merged yet.
shall I need to continue tracking the fixes of this bug?
> -Original Message-
> From: Peter Maydell
> Sent: Friday, March 22, 2024 7:50 PM
> To: Yao, Xingtao/姚 幸涛
> Cc: alex.ben...@linaro.org; erdn...@crans.org; ma.mando...@gmail.com;
> pierrick.bouv...@linaro.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH] contrib/plugins/execlog: Fix compiler warning
>
> On Wed, 20 Mar 2024 at 02:05, Yao Xingtao via
> wrote:
> >
> > 1. The g_pattern_match_string() is deprecated when glib2 version >= 2.70.
> >Use g_pattern_spec_match_string() instead to avoid this problem.
> >
> > 2. The type of second parameter in g_ptr_array_add() is
> >'gpointer' {aka 'void *'}, but the type of reg->name is 'const char*'.
> >Cast the type of reg->name to 'gpointer' to avoid this problem.
> >
> > compiler warning message:
> > /root/qemu/contrib/plugins/execlog.c:330:17: warning:
> ‘g_pattern_match_string’
> > is deprecated: Use 'g_pattern_spec_match_string'
> > instead [-Wdeprecated-declarations]
> > 330 | if (g_pattern_match_string(pat, rd->name) ||
> > | ^~
> > In file included from /usr/include/glib-2.0/glib.h:67,
> > from /root/qemu/contrib/plugins/execlog.c:9:
> > /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> >57 | gboolean g_pattern_match_string (GPatternSpec *pspec,
> > | ^~
> > /root/qemu/contrib/plugins/execlog.c:331:21: warning:
> ‘g_pattern_match_string’
> > is deprecated: Use 'g_pattern_spec_match_string'
> > instead [-Wdeprecated-declarations]
> > 331 | g_pattern_match_string(pat, rd_lower)) {
> > | ^~
> > /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> >57 | gboolean g_pattern_match_string (GPatternSpec *pspec,
> > | ^~
> > /root/qemu/contrib/plugins/execlog.c:339:63: warning: passing argument
> > 2 of ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target
> > type [-Wdiscarded-qualifiers]
> > 339 | g_ptr_array_add(all_reg_names,
> reg->name);
> > |
> ~~~^~
> > In file included from /usr/include/glib-2.0/glib.h:33:
> > /usr/include/glib-2.0/glib/garray.h:198:62: note: expected ‘gpointer’
> > {aka ‘void *’} but argument is of type ‘const char *’
> > 198 |gpointer
> data);
> > |
> ~~^~~~
> >
>
> Hi; thanks for this patch.
>
> This fixes a bug reported in the bug tracker so we should put
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2210
>
> in the commit message just above your signed-off-by tag.
>
>
> > Signed-off-by: Yao Xingtao
I will if needed.
> > ---
> > contrib/plugins/execlog.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
> > index a1dfd59ab7..41f6774116 100644
> > --- a/contrib/plugins/execlog.c
> > +++ b/contrib/plugins/execlog.c
> > @@ -327,8 +327,13 @@ static GPtrArray *registers_init(int vcpu_index)
> > for (int p = 0; p < rmatches->len; p++) {
> > g_autoptr(GPatternSpec) pat =
> g_pattern_spec_new(rmatches->pdata[p]);
> > g_autofree gchar *rd_lower = g_utf8_strdown(rd->name,
> > -1);
> > +#if GLIB_VERSION_MAX_ALLOWED < G_ENCODE_VERSION(2, 70)
>
> Elsewhere we do glib version checks with
>
> #if GLIB_CHECK_VERSION(2, 70, 0)
> code for 2.70.0 and up;
> #else
> code for older versions
> #endif
>
> so I think we should probably do that here too.
>
> > if (g_pattern_match_string(pat, rd->name) ||
> > g_pattern_match_string(pat, rd_lower)) {
> > +#else
> > +if (g_pattern_spec_match_string(pat, rd->name) ||
> > +g_pattern_spec_match_string(pat, rd_lower)) {
> > +#endif
thanks, I got it.
>
> Rather than putting this ifdef in the middle of this function, I think it
> would be
> easier to read if we abstract out a function which does the pattern matching
> and whose body calls the right glib function depending on glib version. We
> generally call these functions the same as the glib function but with a _qemu
> suffix (compare the ones in include/glib-compat.h), so here that would be
> g_pattern_spec_match_string_qemu().
>
> > Register *reg = init_vcpu_register(rd);
> > g_ptr_array_add(registers, reg);
> >
> > @@ -336,7 +341,7 @@ static GPtrArray *registers_init(int vcpu_index)
> > if (disas_assist) {
> > g_mutex_lock(_reg_name_lock);
> >
