* [PATCH 0/2] hw/cxl: Fix decoder commit and uncommit handling
@ 2023-03-21 18:00 Jonathan Cameron via
2023-03-21 18:00 ` [PATCH 1/2] hw/cxl: Fix endian handling for decoder commit Jonathan Cameron via
2023-03-21 18:00 ` [PATCH 2/2] hw/cxl: Fix incorrect reset of commit and associated clearing of committed Jonathan Cameron via
0 siblings, 2 replies; 5+ messages in thread
From: Jonathan Cameron via @ 2023-03-21 18:00 UTC (permalink / raw)
To: Michael Tsirkin, qemu-devel; +Cc: linuxarm, Fan Ni, Dave Jiang, linux-cxl
Issue reported in discussion of:
https://lore.kernel.org/all/20230228224014.1402545-1-fan.ni@samsung.com/
The committed bit for HDM decoders is expected reset when commit transitions
from 1->0. Whilst looking at that it was noticed that hardware was resetting
the commit bit which is not an option allowed by the CXL spec.
In common with many other areas the code did not take into account
big endian architectures, so fix that whilst we are here.
Note testing this exposed a kernel bug around repeated attempts to clear
a decoder out of order. That's been reported but not yet fixed.
Jonathan Cameron (2):
hw/cxl: Fix endian handling for decoder commit.
hw/cxl: Fix incorrect reset of commit and associated clearing of
committed.
hw/cxl/cxl-component-utils.c | 14 ++++++++------
hw/mem/cxl_type3.c | 28 +++++++++++++++++++++++++---
2 files changed, 33 insertions(+), 9 deletions(-)
--
2.37.2
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/2] hw/cxl: Fix endian handling for decoder commit. 2023-03-21 18:00 [PATCH 0/2] hw/cxl: Fix decoder commit and uncommit handling Jonathan Cameron via @ 2023-03-21 18:00 ` Jonathan Cameron via 2023-03-21 19:28 ` Fan Ni 2023-03-21 18:00 ` [PATCH 2/2] hw/cxl: Fix incorrect reset of commit and associated clearing of committed Jonathan Cameron via 1 sibling, 1 reply; 5+ messages in thread From: Jonathan Cameron via @ 2023-03-21 18:00 UTC (permalink / raw) To: Michael Tsirkin, qemu-devel; +Cc: linuxarm, Fan Ni, Dave Jiang, linux-cxl Not a real problem yet as all supported architectures are little endian, but continue to tidy these up when touching code for other reasons. Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- hw/cxl/cxl-component-utils.c | 10 ++++------ hw/mem/cxl_type3.c | 9 ++++++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c index b665d4f565..a3e6cf75cf 100644 --- a/hw/cxl/cxl-component-utils.c +++ b/hw/cxl/cxl-component-utils.c @@ -47,14 +47,12 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset, break; } - memory_region_transaction_begin(); - stl_le_p((uint8_t *)cache_mem + offset, value); if (should_commit) { - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0); - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0); - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0); + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0); + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); } - memory_region_transaction_commit(); + stl_le_p((uint8_t *)cache_mem + offset, value); } static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value, diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index abe60b362c..846089ccda 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -314,14 +314,17 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which) { ComponentRegisters *cregs = &ct3d->cxl_cstate.crb; uint32_t *cache_mem = cregs->cache_mem_registers; + uint32_t ctrl; assert(which == 0); + ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL); /* TODO: Sanity checks that the decoder is possible */ - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0); - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0); + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0); + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0); + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); + stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl); } static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err) -- 2.37.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] hw/cxl: Fix endian handling for decoder commit. 2023-03-21 18:00 ` [PATCH 1/2] hw/cxl: Fix endian handling for decoder commit Jonathan Cameron via @ 2023-03-21 19:28 ` Fan Ni 2023-03-22 10:29 ` Jonathan Cameron via 0 siblings, 1 reply; 5+ messages in thread From: Fan Ni @ 2023-03-21 19:28 UTC (permalink / raw) To: Jonathan Cameron Cc: Michael Tsirkin, qemu-devel@nongnu.org, linuxarm@huawei.com, Dave Jiang, linux-cxl@vger.kernel.org On Tue, Mar 21, 2023 at 06:00:11PM +0000, Jonathan Cameron wrote: > Not a real problem yet as all supported architectures are > little endian, but continue to tidy these up when touching > code for other reasons. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Hi Jonathan, Did you forget to send the other patch in this series by any chance? Fan > --- > hw/cxl/cxl-component-utils.c | 10 ++++------ > hw/mem/cxl_type3.c | 9 ++++++--- > 2 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c > index b665d4f565..a3e6cf75cf 100644 > --- a/hw/cxl/cxl-component-utils.c > +++ b/hw/cxl/cxl-component-utils.c > @@ -47,14 +47,12 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset, > break; > } > > - memory_region_transaction_begin(); > - stl_le_p((uint8_t *)cache_mem + offset, value); > if (should_commit) { > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0); > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0); > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); > + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0); > + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0); > + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); > } > - memory_region_transaction_commit(); > + stl_le_p((uint8_t *)cache_mem + offset, value); > } > > static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value, > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index abe60b362c..846089ccda 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -314,14 +314,17 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which) > { > ComponentRegisters *cregs = &ct3d->cxl_cstate.crb; > uint32_t *cache_mem = cregs->cache_mem_registers; > + uint32_t ctrl; > > assert(which == 0); > > + ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL); > /* TODO: Sanity checks that the decoder is possible */ > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0); > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0); > + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0); > + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0); > + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); > > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); > + stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl); > } > > static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err) > -- > 2.37.2 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] hw/cxl: Fix endian handling for decoder commit. 2023-03-21 19:28 ` Fan Ni @ 2023-03-22 10:29 ` Jonathan Cameron via 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Cameron via @ 2023-03-22 10:29 UTC (permalink / raw) To: Fan Ni Cc: Michael Tsirkin, qemu-devel@nongnu.org, linuxarm@huawei.com, Dave Jiang, linux-cxl@vger.kernel.org On Tue, 21 Mar 2023 19:28:02 +0000 Fan Ni <fan.ni@samsung.com> wrote: > On Tue, Mar 21, 2023 at 06:00:11PM +0000, Jonathan Cameron wrote: > > Not a real problem yet as all supported architectures are > > little endian, but continue to tidy these up when touching > > code for other reasons. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Hi Jonathan, > Did you forget to send the other patch in this series by any chance? No. All 3 emails went out, but not all seem to have made it to the lists. I have them via an internal reflector so maybe a Huawei network glitch. Odd. I'll try a resend. Thanks for pointing this out! Jonathan > > Fan > > --- > > hw/cxl/cxl-component-utils.c | 10 ++++------ > > hw/mem/cxl_type3.c | 9 ++++++--- > > 2 files changed, 10 insertions(+), 9 deletions(-) > > > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c > > index b665d4f565..a3e6cf75cf 100644 > > --- a/hw/cxl/cxl-component-utils.c > > +++ b/hw/cxl/cxl-component-utils.c > > @@ -47,14 +47,12 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset, > > break; > > } > > > > - memory_region_transaction_begin(); > > - stl_le_p((uint8_t *)cache_mem + offset, value); > > if (should_commit) { > > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0); > > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0); > > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); > > + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0); > > + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0); > > + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); > > } > > - memory_region_transaction_commit(); > > + stl_le_p((uint8_t *)cache_mem + offset, value); > > } > > > > static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value, > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > index abe60b362c..846089ccda 100644 > > --- a/hw/mem/cxl_type3.c > > +++ b/hw/mem/cxl_type3.c > > @@ -314,14 +314,17 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which) > > { > > ComponentRegisters *cregs = &ct3d->cxl_cstate.crb; > > uint32_t *cache_mem = cregs->cache_mem_registers; > > + uint32_t ctrl; > > > > assert(which == 0); > > > > + ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL); > > /* TODO: Sanity checks that the decoder is possible */ > > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0); > > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0); > > + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0); > > + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0); > > + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); > > > > - ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); > > + stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl); > > } > > > > static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err) > > -- > > 2.37.2 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] hw/cxl: Fix incorrect reset of commit and associated clearing of committed. 2023-03-21 18:00 [PATCH 0/2] hw/cxl: Fix decoder commit and uncommit handling Jonathan Cameron via 2023-03-21 18:00 ` [PATCH 1/2] hw/cxl: Fix endian handling for decoder commit Jonathan Cameron via @ 2023-03-21 18:00 ` Jonathan Cameron via 1 sibling, 0 replies; 5+ messages in thread From: Jonathan Cameron via @ 2023-03-21 18:00 UTC (permalink / raw) To: Michael Tsirkin, qemu-devel; +Cc: linuxarm, Fan Ni, Dave Jiang, linux-cxl The hardware clearing the commit bit is not spec compliant. Clearing of committed bit when commit is cleared is not specifically stated in the CXL spec, but is the expected (and simplest) permitted behaviour so use that for QEMU emulation. Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- hw/cxl/cxl-component-utils.c | 6 +++++- hw/mem/cxl_type3.c | 21 ++++++++++++++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c index a3e6cf75cf..378f1082ce 100644 --- a/hw/cxl/cxl-component-utils.c +++ b/hw/cxl/cxl-component-utils.c @@ -38,19 +38,23 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset, ComponentRegisters *cregs = &cxl_cstate->crb; uint32_t *cache_mem = cregs->cache_mem_registers; bool should_commit = false; + bool should_uncommit = false; switch (offset) { case A_CXL_HDM_DECODER0_CTRL: should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT); + should_uncommit = !should_commit; break; default: break; } if (should_commit) { - value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0); value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0); value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); + } else if (should_uncommit) { + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0); + value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 0); } stl_le_p((uint8_t *)cache_mem + offset, value); } diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index 846089ccda..9598d584ac 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -320,13 +320,28 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which) ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL); /* TODO: Sanity checks that the decoder is possible */ - ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0); ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0); ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl); } +static void hdm_decoder_uncommit(CXLType3Dev *ct3d, int which) +{ + ComponentRegisters *cregs = &ct3d->cxl_cstate.crb; + uint32_t *cache_mem = cregs->cache_mem_registers; + uint32_t ctrl; + + assert(which == 0); + + ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL); + + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0); + ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 0); + + stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl); +} + static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err) { switch (qmp_err) { @@ -395,6 +410,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value, CXLType3Dev *ct3d = container_of(cxl_cstate, CXLType3Dev, cxl_cstate); uint32_t *cache_mem = cregs->cache_mem_registers; bool should_commit = false; + bool should_uncommit = false; int which_hdm = -1; assert(size == 4); @@ -403,6 +419,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value, switch (offset) { case A_CXL_HDM_DECODER0_CTRL: should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT); + should_uncommit = !should_commit; which_hdm = 0; break; case A_CXL_RAS_UNC_ERR_STATUS: @@ -489,6 +506,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value, stl_le_p((uint8_t *)cache_mem + offset, value); if (should_commit) { hdm_decoder_commit(ct3d, which_hdm); + } else if (should_uncommit) { + hdm_decoder_uncommit(ct3d, which_hdm); } } -- 2.37.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-22 16:29 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-21 18:00 [PATCH 0/2] hw/cxl: Fix decoder commit and uncommit handling Jonathan Cameron via 2023-03-21 18:00 ` [PATCH 1/2] hw/cxl: Fix endian handling for decoder commit Jonathan Cameron via 2023-03-21 19:28 ` Fan Ni 2023-03-22 10:29 ` Jonathan Cameron via 2023-03-21 18:00 ` [PATCH 2/2] hw/cxl: Fix incorrect reset of commit and associated clearing of committed Jonathan Cameron via
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).