* [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-19 14:17 [PATCH v6 0/4] hw/cxl: Poison get, inject, clear Jonathan Cameron via
@ 2023-05-19 14:18 ` Jonathan Cameron via
2023-05-19 16:08 ` Philippe Mathieu-Daudé
2023-05-20 12:37 ` Peter Maydell
2023-05-19 14:18 ` [PATCH v6 2/4] hw/cxl: QMP based poison injection support Jonathan Cameron via
` (2 subsequent siblings)
3 siblings, 2 replies; 17+ messages in thread
From: Jonathan Cameron via @ 2023-05-19 14:18 UTC (permalink / raw)
To: qemu-devel, Michael Tsirkin, Fan Ni
Cc: linux-cxl, linuxarm, Ira Weiny, Michael Roth,
Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
Marc-André Lureau, Thomas Huth
From: Ira Weiny <ira.weiny@intel.com>
CXL has 24 bit unaligned fields which need to be stored to. CXL is
specified as little endian.
Define st24_le_p() and the supporting functions to store such a field
from a 32 bit host native value.
The use of b, w, l, q as the size specifier is limiting. So "24" was
used for the size part of the function name.
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
docs/devel/loads-stores.rst | 1 +
include/qemu/bswap.h | 27 +++++++++++++++++++++++++++
2 files changed, 28 insertions(+)
diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
index d2cefc77a2..82a79e91d9 100644
--- a/docs/devel/loads-stores.rst
+++ b/docs/devel/loads-stores.rst
@@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
``size``
- ``b`` : 8 bits
- ``w`` : 16 bits
+ - ``24`` : 24 bits
- ``l`` : 32 bits
- ``q`` : 64 bits
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 15a78c0db5..f546b1fc06 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -8,11 +8,25 @@
#undef bswap64
#define bswap64(_x) __builtin_bswap64(_x)
+static inline uint32_t bswap24(uint32_t x)
+{
+ assert((x & 0xff000000U) == 0);
+
+ return (((x & 0x000000ffU) << 16) |
+ ((x & 0x0000ff00U) << 0) |
+ ((x & 0x00ff0000U) >> 16));
+}
+
static inline void bswap16s(uint16_t *s)
{
*s = __builtin_bswap16(*s);
}
+static inline void bswap24s(uint32_t *s)
+{
+ *s = bswap24(*s & 0x00ffffffU);
+}
+
static inline void bswap32s(uint32_t *s)
{
*s = __builtin_bswap32(*s);
@@ -26,11 +40,13 @@ static inline void bswap64s(uint64_t *s)
#if HOST_BIG_ENDIAN
#define be_bswap(v, size) (v)
#define le_bswap(v, size) glue(__builtin_bswap, size)(v)
+#define le_bswap24(v) bswap24(v)
#define be_bswaps(v, size)
#define le_bswaps(p, size) \
do { *p = glue(__builtin_bswap, size)(*p); } while (0)
#else
#define le_bswap(v, size) (v)
+#define le_bswap24(v) (v)
#define be_bswap(v, size) glue(__builtin_bswap, size)(v)
#define le_bswaps(v, size)
#define be_bswaps(p, size) \
@@ -176,6 +192,7 @@ CPU_CONVERT(le, 64, uint64_t)
* size is:
* b: 8 bits
* w: 16 bits
+ * 24: 24 bits
* l: 32 bits
* q: 64 bits
*
@@ -248,6 +265,11 @@ static inline void stw_he_p(void *ptr, uint16_t v)
__builtin_memcpy(ptr, &v, sizeof(v));
}
+static inline void st24_he_p(void *ptr, uint32_t v)
+{
+ __builtin_memcpy(ptr, &v, 3);
+}
+
static inline int ldl_he_p(const void *ptr)
{
int32_t r;
@@ -297,6 +319,11 @@ static inline void stw_le_p(void *ptr, uint16_t v)
stw_he_p(ptr, le_bswap(v, 16));
}
+static inline void st24_le_p(void *ptr, uint32_t v)
+{
+ st24_he_p(ptr, le_bswap24(v));
+}
+
static inline void stl_le_p(void *ptr, uint32_t v)
{
stl_he_p(ptr, le_bswap(v, 32));
--
2.39.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-19 14:18 ` [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field Jonathan Cameron via
@ 2023-05-19 16:08 ` Philippe Mathieu-Daudé
2023-05-19 16:24 ` Jonathan Cameron via
2023-05-20 12:37 ` Peter Maydell
1 sibling, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-05-19 16:08 UTC (permalink / raw)
To: Jonathan Cameron, qemu-devel, Michael Tsirkin, Fan Ni
Cc: linux-cxl, linuxarm, Ira Weiny, Michael Roth, Dave Jiang,
Markus Armbruster, Daniel P . Berrangé, Eric Blake,
Mike Maslenkin, Marc-André Lureau, Thomas Huth
On 19/5/23 16:18, Jonathan Cameron wrote:
> From: Ira Weiny <ira.weiny@intel.com>
>
> CXL has 24 bit unaligned fields which need to be stored to. CXL is
> specified as little endian.
>
> Define st24_le_p() and the supporting functions to store such a field
> from a 32 bit host native value.
>
> The use of b, w, l, q as the size specifier is limiting. So "24" was
> used for the size part of the function name.
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> docs/devel/loads-stores.rst | 1 +
> include/qemu/bswap.h | 27 +++++++++++++++++++++++++++
> 2 files changed, 28 insertions(+)
>
> diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
> index d2cefc77a2..82a79e91d9 100644
> --- a/docs/devel/loads-stores.rst
> +++ b/docs/devel/loads-stores.rst
> @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
> ``size``
> - ``b`` : 8 bits
> - ``w`` : 16 bits
> + - ``24`` : 24 bits
> - ``l`` : 32 bits
> - ``q`` : 64 bits
>
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 15a78c0db5..f546b1fc06 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -8,11 +8,25 @@
> #undef bswap64
> #define bswap64(_x) __builtin_bswap64(_x)
>
> +static inline uint32_t bswap24(uint32_t x)
> +{
> + assert((x & 0xff000000U) == 0);
Asserting here is a bit violent. In particular because there is no
contract description that x should be less than N bits for bswapN()
in "qemu/bswap.h" API.
But if you rather to assert ...
> +
> + return (((x & 0x000000ffU) << 16) |
> + ((x & 0x0000ff00U) << 0) |
> + ((x & 0x00ff0000U) >> 16));
> +}
> +
> static inline void bswap16s(uint16_t *s)
> {
> *s = __builtin_bswap16(*s);
> }
>
> +static inline void bswap24s(uint32_t *s)
> +{
> + *s = bswap24(*s & 0x00ffffffU);
... and sanitize the value here ...
> +}
> +
> static inline void bswap32s(uint32_t *s)
> {
> *s = __builtin_bswap32(*s);
> @@ -26,11 +40,13 @@ static inline void bswap64s(uint64_t *s)
> #if HOST_BIG_ENDIAN
> #define be_bswap(v, size) (v)
> #define le_bswap(v, size) glue(__builtin_bswap, size)(v)
> +#define le_bswap24(v) bswap24(v)
... then shouldn't you sanitize also here?
Personally I'd just drop the assertion.
> #define be_bswaps(v, size)
> #define le_bswaps(p, size) \
> do { *p = glue(__builtin_bswap, size)(*p); } while (0)
> #else
> #define le_bswap(v, size) (v)
> +#define le_bswap24(v) (v)
> #define be_bswap(v, size) glue(__builtin_bswap, size)(v)
> #define le_bswaps(v, size)
> #define be_bswaps(p, size) \
> @@ -176,6 +192,7 @@ CPU_CONVERT(le, 64, uint64_t)
> * size is:
> * b: 8 bits
> * w: 16 bits
> + * 24: 24 bits
> * l: 32 bits
> * q: 64 bits
> *
> @@ -248,6 +265,11 @@ static inline void stw_he_p(void *ptr, uint16_t v)
> __builtin_memcpy(ptr, &v, sizeof(v));
> }
>
> +static inline void st24_he_p(void *ptr, uint32_t v)
> +{
> + __builtin_memcpy(ptr, &v, 3);
> +}
> +
> static inline int ldl_he_p(const void *ptr)
> {
> int32_t r;
> @@ -297,6 +319,11 @@ static inline void stw_le_p(void *ptr, uint16_t v)
> stw_he_p(ptr, le_bswap(v, 16));
> }
>
> +static inline void st24_le_p(void *ptr, uint32_t v)
> +{
> + st24_he_p(ptr, le_bswap24(v));
> +}
> +
> static inline void stl_le_p(void *ptr, uint32_t v)
> {
> stl_he_p(ptr, le_bswap(v, 32));
Conditional to removing the assertion in bswap24():
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-19 16:08 ` Philippe Mathieu-Daudé
@ 2023-05-19 16:24 ` Jonathan Cameron via
2023-05-19 16:43 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron via @ 2023-05-19 16:24 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
Ira Weiny, Michael Roth, Dave Jiang, Markus Armbruster,
Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
Marc-André Lureau, Thomas Huth
On Fri, 19 May 2023 18:08:30 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> On 19/5/23 16:18, Jonathan Cameron wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > CXL has 24 bit unaligned fields which need to be stored to. CXL is
> > specified as little endian.
> >
> > Define st24_le_p() and the supporting functions to store such a field
> > from a 32 bit host native value.
> >
> > The use of b, w, l, q as the size specifier is limiting. So "24" was
> > used for the size part of the function name.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > docs/devel/loads-stores.rst | 1 +
> > include/qemu/bswap.h | 27 +++++++++++++++++++++++++++
> > 2 files changed, 28 insertions(+)
> >
> > diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
> > index d2cefc77a2..82a79e91d9 100644
> > --- a/docs/devel/loads-stores.rst
> > +++ b/docs/devel/loads-stores.rst
> > @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
> > ``size``
> > - ``b`` : 8 bits
> > - ``w`` : 16 bits
> > + - ``24`` : 24 bits
> > - ``l`` : 32 bits
> > - ``q`` : 64 bits
> >
> > diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> > index 15a78c0db5..f546b1fc06 100644
> > --- a/include/qemu/bswap.h
> > +++ b/include/qemu/bswap.h
> > @@ -8,11 +8,25 @@
> > #undef bswap64
> > #define bswap64(_x) __builtin_bswap64(_x)
> >
> > +static inline uint32_t bswap24(uint32_t x)
> > +{
> > + assert((x & 0xff000000U) == 0);
>
> Asserting here is a bit violent. In particular because there is no
> contract description that x should be less than N bits for bswapN()
> in "qemu/bswap.h" API.
>
> But if you rather to assert ...
I'm fine either way. You asked for it when reviewing v4...
https://lore.kernel.org/all/28a9d97a-b252-a33f-1ac0-cd36264b29ab@linaro.org/
>
> > +
> > + return (((x & 0x000000ffU) << 16) |
> > + ((x & 0x0000ff00U) << 0) |
> > + ((x & 0x00ff0000U) >> 16));
> > +}
> > +
> > static inline void bswap16s(uint16_t *s)
> > {
> > *s = __builtin_bswap16(*s);
> > }
> >
> > +static inline void bswap24s(uint32_t *s)
> > +{
> > + *s = bswap24(*s & 0x00ffffffU);
>
> ... and sanitize the value here ...
>
> > +}
> > +
> > static inline void bswap32s(uint32_t *s)
> > {
> > *s = __builtin_bswap32(*s);
> > @@ -26,11 +40,13 @@ static inline void bswap64s(uint64_t *s)
> > #if HOST_BIG_ENDIAN
> > #define be_bswap(v, size) (v)
> > #define le_bswap(v, size) glue(__builtin_bswap, size)(v)
> > +#define le_bswap24(v) bswap24(v)
>
> ... then shouldn't you sanitize also here?
>
Good point. I forgot that detail whilst fighting with s390
cross builds earlier ;)
> Personally I'd just drop the assertion.
I'm fine with doing that.
Jonathan
>
> > #define be_bswaps(v, size)
> > #define le_bswaps(p, size) \
> > do { *p = glue(__builtin_bswap, size)(*p); } while (0)
> > #else
> > #define le_bswap(v, size) (v)
> > +#define le_bswap24(v) (v)
> > #define be_bswap(v, size) glue(__builtin_bswap, size)(v)
> > #define le_bswaps(v, size)
> > #define be_bswaps(p, size) \
> > @@ -176,6 +192,7 @@ CPU_CONVERT(le, 64, uint64_t)
> > * size is:
> > * b: 8 bits
> > * w: 16 bits
> > + * 24: 24 bits
> > * l: 32 bits
> > * q: 64 bits
> > *
> > @@ -248,6 +265,11 @@ static inline void stw_he_p(void *ptr, uint16_t v)
> > __builtin_memcpy(ptr, &v, sizeof(v));
> > }
> >
> > +static inline void st24_he_p(void *ptr, uint32_t v)
> > +{
> > + __builtin_memcpy(ptr, &v, 3);
> > +}
> > +
> > static inline int ldl_he_p(const void *ptr)
> > {
> > int32_t r;
> > @@ -297,6 +319,11 @@ static inline void stw_le_p(void *ptr, uint16_t v)
> > stw_he_p(ptr, le_bswap(v, 16));
> > }
> >
> > +static inline void st24_le_p(void *ptr, uint32_t v)
> > +{
> > + st24_he_p(ptr, le_bswap24(v));
> > +}
> > +
> > static inline void stl_le_p(void *ptr, uint32_t v)
> > {
> > stl_he_p(ptr, le_bswap(v, 32));
>
> Conditional to removing the assertion in bswap24():
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-19 16:24 ` Jonathan Cameron via
@ 2023-05-19 16:43 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-05-19 16:43 UTC (permalink / raw)
To: Jonathan Cameron
Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
Ira Weiny, Michael Roth, Dave Jiang, Markus Armbruster,
Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
Marc-André Lureau, Thomas Huth
On 19/5/23 18:24, Jonathan Cameron wrote:
> On Fri, 19 May 2023 18:08:30 +0200
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
>> On 19/5/23 16:18, Jonathan Cameron wrote:
>>> From: Ira Weiny <ira.weiny@intel.com>
>>>
>>> CXL has 24 bit unaligned fields which need to be stored to. CXL is
>>> specified as little endian.
>>>
>>> Define st24_le_p() and the supporting functions to store such a field
>>> from a 32 bit host native value.
>>>
>>> The use of b, w, l, q as the size specifier is limiting. So "24" was
>>> used for the size part of the function name.
>>>
>>> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> ---
>>> docs/devel/loads-stores.rst | 1 +
>>> include/qemu/bswap.h | 27 +++++++++++++++++++++++++++
>>> 2 files changed, 28 insertions(+)
>>>
>>> diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
>>> index d2cefc77a2..82a79e91d9 100644
>>> --- a/docs/devel/loads-stores.rst
>>> +++ b/docs/devel/loads-stores.rst
>>> @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
>>> ``size``
>>> - ``b`` : 8 bits
>>> - ``w`` : 16 bits
>>> + - ``24`` : 24 bits
>>> - ``l`` : 32 bits
>>> - ``q`` : 64 bits
>>>
>>> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
>>> index 15a78c0db5..f546b1fc06 100644
>>> --- a/include/qemu/bswap.h
>>> +++ b/include/qemu/bswap.h
>>> @@ -8,11 +8,25 @@
>>> #undef bswap64
>>> #define bswap64(_x) __builtin_bswap64(_x)
>>>
>>> +static inline uint32_t bswap24(uint32_t x)
>>> +{
>>> + assert((x & 0xff000000U) == 0);
>>
>> Asserting here is a bit violent. In particular because there is no
>> contract description that x should be less than N bits for bswapN()
>> in "qemu/bswap.h" API.
>>
>> But if you rather to assert ...
>
>
> I'm fine either way. You asked for it when reviewing v4...
> https://lore.kernel.org/all/28a9d97a-b252-a33f-1ac0-cd36264b29ab@linaro.org/
Never too late to improve oneself ;)
One month later I'm afraid the assertion would fire too often,
resulting in unnecessary pain to developers (in particular the
non-QEMU ones).
>>
>>> +
>>> + return (((x & 0x000000ffU) << 16) |
>>> + ((x & 0x0000ff00U) << 0) |
>>> + ((x & 0x00ff0000U) >> 16));
>>> +}
>>> +
>>> static inline void bswap16s(uint16_t *s)
>>> {
>>> *s = __builtin_bswap16(*s);
>>> }
>>>
>>> +static inline void bswap24s(uint32_t *s)
>>> +{
>>> + *s = bswap24(*s & 0x00ffffffU);
>>
>> ... and sanitize the value here ...
>>
>>> +}
>>> +
>>> static inline void bswap32s(uint32_t *s)
>>> {
>>> *s = __builtin_bswap32(*s);
>>> @@ -26,11 +40,13 @@ static inline void bswap64s(uint64_t *s)
>>> #if HOST_BIG_ENDIAN
>>> #define be_bswap(v, size) (v)
>>> #define le_bswap(v, size) glue(__builtin_bswap, size)(v)
>>> +#define le_bswap24(v) bswap24(v)
>>
>> ... then shouldn't you sanitize also here?
>>
>
> Good point. I forgot that detail whilst fighting with s390
> cross builds earlier ;)
>
>> Personally I'd just drop the assertion.
>
> I'm fine with doing that.
>
> Jonathan
>
>>
>>> #define be_bswaps(v, size)
>>> #define le_bswaps(p, size) \
>>> do { *p = glue(__builtin_bswap, size)(*p); } while (0)
>>> #else
>>> #define le_bswap(v, size) (v)
>>> +#define le_bswap24(v) (v)
>>> #define be_bswap(v, size) glue(__builtin_bswap, size)(v)
>>> #define le_bswaps(v, size)
>>> #define be_bswaps(p, size) \
>>> @@ -176,6 +192,7 @@ CPU_CONVERT(le, 64, uint64_t)
>>> * size is:
>>> * b: 8 bits
>>> * w: 16 bits
>>> + * 24: 24 bits
>>> * l: 32 bits
>>> * q: 64 bits
>>> *
>>> @@ -248,6 +265,11 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>>> __builtin_memcpy(ptr, &v, sizeof(v));
>>> }
>>>
>>> +static inline void st24_he_p(void *ptr, uint32_t v)
>>> +{
>>> + __builtin_memcpy(ptr, &v, 3);
>>> +}
>>> +
>>> static inline int ldl_he_p(const void *ptr)
>>> {
>>> int32_t r;
>>> @@ -297,6 +319,11 @@ static inline void stw_le_p(void *ptr, uint16_t v)
>>> stw_he_p(ptr, le_bswap(v, 16));
>>> }
>>>
>>> +static inline void st24_le_p(void *ptr, uint32_t v)
>>> +{
>>> + st24_he_p(ptr, le_bswap24(v));
>>> +}
>>> +
>>> static inline void stl_le_p(void *ptr, uint32_t v)
>>> {
>>> stl_he_p(ptr, le_bswap(v, 32));
>>
>> Conditional to removing the assertion in bswap24():
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-19 14:18 ` [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field Jonathan Cameron via
2023-05-19 16:08 ` Philippe Mathieu-Daudé
@ 2023-05-20 12:37 ` Peter Maydell
2023-05-20 13:15 ` BALATON Zoltan
2023-05-22 11:59 ` Jonathan Cameron via
1 sibling, 2 replies; 17+ messages in thread
From: Peter Maydell @ 2023-05-20 12:37 UTC (permalink / raw)
To: Jonathan Cameron
Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
Ira Weiny, Michael Roth, Philippe Mathieu-Daudé, Dave Jiang,
Markus Armbruster, Daniel P . Berrangé, Eric Blake,
Mike Maslenkin, Marc-André Lureau, Thomas Huth
On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
<qemu-devel@nongnu.org> wrote:
>
> From: Ira Weiny <ira.weiny@intel.com>
>
> CXL has 24 bit unaligned fields which need to be stored to. CXL is
> specified as little endian.
>
> Define st24_le_p() and the supporting functions to store such a field
> from a 32 bit host native value.
>
> The use of b, w, l, q as the size specifier is limiting. So "24" was
> used for the size part of the function name.
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> docs/devel/loads-stores.rst | 1 +
> include/qemu/bswap.h | 27 +++++++++++++++++++++++++++
> 2 files changed, 28 insertions(+)
>
> diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
> index d2cefc77a2..82a79e91d9 100644
> --- a/docs/devel/loads-stores.rst
> +++ b/docs/devel/loads-stores.rst
> @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
> ``size``
> - ``b`` : 8 bits
> - ``w`` : 16 bits
> + - ``24`` : 24 bits
> - ``l`` : 32 bits
> - ``q`` : 64 bits
Can you also update the "Regexes for git grep" section
below to account for the new size value, please?
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-20 12:37 ` Peter Maydell
@ 2023-05-20 13:15 ` BALATON Zoltan
2023-05-20 15:15 ` Richard Henderson
2023-05-22 11:59 ` Jonathan Cameron via
1 sibling, 1 reply; 17+ messages in thread
From: BALATON Zoltan @ 2023-05-20 13:15 UTC (permalink / raw)
To: Peter Maydell
Cc: Jonathan Cameron, qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl,
linuxarm, Ira Weiny, Michael Roth, Philippe Mathieu-Daudé,
Dave Jiang, Markus Armbruster, Daniel P . Berrangé,
Eric Blake, Mike Maslenkin, Marc-André Lureau, Thomas Huth
On Sat, 20 May 2023, Peter Maydell wrote:
> On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
> <qemu-devel@nongnu.org> wrote:
>>
>> From: Ira Weiny <ira.weiny@intel.com>
>>
>> CXL has 24 bit unaligned fields which need to be stored to. CXL is
>> specified as little endian.
>>
>> Define st24_le_p() and the supporting functions to store such a field
>> from a 32 bit host native value.
>>
>> The use of b, w, l, q as the size specifier is limiting. So "24" was
>> used for the size part of the function name.
Maybe it's clearer to use 24 but if we want to keep these somewhat
consistent how about using t for Triplet, Three-bytes or Twenty-four?
Regards,
BALATON Zoltan
>> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> ---
>> docs/devel/loads-stores.rst | 1 +
>> include/qemu/bswap.h | 27 +++++++++++++++++++++++++++
>> 2 files changed, 28 insertions(+)
>>
>> diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
>> index d2cefc77a2..82a79e91d9 100644
>> --- a/docs/devel/loads-stores.rst
>> +++ b/docs/devel/loads-stores.rst
>> @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
>> ``size``
>> - ``b`` : 8 bits
>> - ``w`` : 16 bits
>> + - ``24`` : 24 bits
>> - ``l`` : 32 bits
>> - ``q`` : 64 bits
>
> Can you also update the "Regexes for git grep" section
> below to account for the new size value, please?
>
> thanks
> -- PMM
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-20 13:15 ` BALATON Zoltan
@ 2023-05-20 15:15 ` Richard Henderson
2023-05-20 17:08 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2023-05-20 15:15 UTC (permalink / raw)
To: BALATON Zoltan, Peter Maydell
Cc: Jonathan Cameron, qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl,
linuxarm, Ira Weiny, Michael Roth, Philippe Mathieu-Daudé,
Dave Jiang, Markus Armbruster, Daniel P . Berrangé,
Eric Blake, Mike Maslenkin, Marc-André Lureau, Thomas Huth
On 5/20/23 06:15, BALATON Zoltan wrote:
> On Sat, 20 May 2023, Peter Maydell wrote:
>> On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
>> <qemu-devel@nongnu.org> wrote:
>>>
>>> From: Ira Weiny <ira.weiny@intel.com>
>>>
>>> CXL has 24 bit unaligned fields which need to be stored to. CXL is
>>> specified as little endian.
>>>
>>> Define st24_le_p() and the supporting functions to store such a field
>>> from a 32 bit host native value.
>>>
>>> The use of b, w, l, q as the size specifier is limiting. So "24" was
>>> used for the size part of the function name.
>
> Maybe it's clearer to use 24 but if we want to keep these somewhat consistent how about
> using t for Triplet, Three-bytes or Twenty-four?
I think it's clearer to use '3'.
When I added 128-bit support I used cpu_ld16_mmu.
I think it would be clearer to not use letters anywhere, and to use units of bytes instead
of units of bits (no one can store just a bit), but changing everything is a big job.
r~
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-20 15:15 ` Richard Henderson
@ 2023-05-20 17:08 ` Philippe Mathieu-Daudé
2023-05-21 9:05 ` Michael S. Tsirkin
2023-05-22 12:09 ` Jonathan Cameron via
0 siblings, 2 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-05-20 17:08 UTC (permalink / raw)
To: Richard Henderson, BALATON Zoltan, Peter Maydell
Cc: Jonathan Cameron, qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl,
linuxarm, Ira Weiny, Michael Roth, Dave Jiang, Markus Armbruster,
Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
Marc-André Lureau, Thomas Huth
On 20/5/23 17:15, Richard Henderson wrote:
> On 5/20/23 06:15, BALATON Zoltan wrote:
>> On Sat, 20 May 2023, Peter Maydell wrote:
>>> On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
>>> <qemu-devel@nongnu.org> wrote:
>>>>
>>>> From: Ira Weiny <ira.weiny@intel.com>
>>>>
>>>> CXL has 24 bit unaligned fields which need to be stored to. CXL is
>>>> specified as little endian.
>>>>
>>>> Define st24_le_p() and the supporting functions to store such a field
>>>> from a 32 bit host native value.
>>>>
>>>> The use of b, w, l, q as the size specifier is limiting. So "24" was
>>>> used for the size part of the function name.
>>
>> Maybe it's clearer to use 24 but if we want to keep these somewhat
>> consistent how about using t for Triplet, Three-bytes or Twenty-four?
>
> I think it's clearer to use '3'.
> When I added 128-bit support I used cpu_ld16_mmu.
There is also ld8u / ld8s / st8.
> I think it would be clearer to not use letters anywhere, and to use
> units of bytes instead of units of bits (no one can store just a bit),
> but changing everything is a big job.
So:
ldub -> ld1u,
lduw_le -> ld2u_le,
virtio_stl -> virtio_st4,
stq_be -> st8_be.
Right?
Also we have:
cpu_ld/st_*
virtio_ld/st_*
ld/st_*_phys
ld/st_*_pci_dma
address_space_ld/st
While mass-changing, we could use FOO_ld/st_BAR with FOO
for API and BAR for API variant (endian, mmuidx, ra, ...):
So:
ld/st_*_pci_dma -> pci_dma_ld/st_*
for ld/st_*_phys I'm not sure.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-20 17:08 ` Philippe Mathieu-Daudé
@ 2023-05-21 9:05 ` Michael S. Tsirkin
2023-05-22 12:09 ` Jonathan Cameron via
1 sibling, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2023-05-21 9:05 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Richard Henderson, BALATON Zoltan, Peter Maydell,
Jonathan Cameron, qemu-devel, Fan Ni, linux-cxl, linuxarm,
Ira Weiny, Michael Roth, Dave Jiang, Markus Armbruster,
Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
Marc-André Lureau, Thomas Huth
On Sat, May 20, 2023 at 07:08:22PM +0200, Philippe Mathieu-Daudé wrote:
> On 20/5/23 17:15, Richard Henderson wrote:
> > On 5/20/23 06:15, BALATON Zoltan wrote:
> > > On Sat, 20 May 2023, Peter Maydell wrote:
> > > > On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
> > > > <qemu-devel@nongnu.org> wrote:
> > > > >
> > > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > >
> > > > > CXL has 24 bit unaligned fields which need to be stored to. CXL is
> > > > > specified as little endian.
> > > > >
> > > > > Define st24_le_p() and the supporting functions to store such a field
> > > > > from a 32 bit host native value.
> > > > >
> > > > > The use of b, w, l, q as the size specifier is limiting. So "24" was
> > > > > used for the size part of the function name.
> > >
> > > Maybe it's clearer to use 24 but if we want to keep these somewhat
> > > consistent how about using t for Triplet, Three-bytes or
> > > Twenty-four?
> >
> > I think it's clearer to use '3'.
> > When I added 128-bit support I used cpu_ld16_mmu.
>
> There is also ld8u / ld8s / st8.
>
> > I think it would be clearer to not use letters anywhere, and to use
> > units of bytes instead of units of bits (no one can store just a bit),
> > but changing everything is a big job.
>
> So:
>
> ldub -> ld1u,
>
> lduw_le -> ld2u_le,
>
> virtio_stl -> virtio_st4,
>
> stq_be -> st8_be.
>
> Right?
No, using bits is so ingrained by now, that people will think
st8 is a single byte.
And yes, you can store a bit - you have to read modify write but
hey.
> Also we have:
>
> cpu_ld/st_*
> virtio_ld/st_*
> ld/st_*_phys
> ld/st_*_pci_dma
> address_space_ld/st
>
> While mass-changing, we could use FOO_ld/st_BAR with FOO
> for API and BAR for API variant (endian, mmuidx, ra, ...):
>
> So:
>
> ld/st_*_pci_dma -> pci_dma_ld/st_*
>
> for ld/st_*_phys I'm not sure.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-20 17:08 ` Philippe Mathieu-Daudé
2023-05-21 9:05 ` Michael S. Tsirkin
@ 2023-05-22 12:09 ` Jonathan Cameron via
1 sibling, 0 replies; 17+ messages in thread
From: Jonathan Cameron via @ 2023-05-22 12:09 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Richard Henderson, BALATON Zoltan, Peter Maydell, qemu-devel,
Michael Tsirkin, Fan Ni, linux-cxl, linuxarm, Ira Weiny,
Michael Roth, Dave Jiang, Markus Armbruster,
Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
Marc-André Lureau, Thomas Huth
On Sat, 20 May 2023 19:08:22 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> On 20/5/23 17:15, Richard Henderson wrote:
> > On 5/20/23 06:15, BALATON Zoltan wrote:
> >> On Sat, 20 May 2023, Peter Maydell wrote:
> >>> On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
> >>> <qemu-devel@nongnu.org> wrote:
> >>>>
> >>>> From: Ira Weiny <ira.weiny@intel.com>
> >>>>
> >>>> CXL has 24 bit unaligned fields which need to be stored to. CXL is
> >>>> specified as little endian.
> >>>>
> >>>> Define st24_le_p() and the supporting functions to store such a field
> >>>> from a 32 bit host native value.
> >>>>
> >>>> The use of b, w, l, q as the size specifier is limiting. So "24" was
> >>>> used for the size part of the function name.
> >>
> >> Maybe it's clearer to use 24 but if we want to keep these somewhat
> >> consistent how about using t for Triplet, Three-bytes or Twenty-four?
> >
> > I think it's clearer to use '3'.
> > When I added 128-bit support I used cpu_ld16_mmu.
As an aside on that - you didn't update the docs when you added that
(I was looking for it to copy your regex ;)
>
> There is also ld8u / ld8s / st8.
>
> > I think it would be clearer to not use letters anywhere, and to use
> > units of bytes instead of units of bits (no one can store just a bit),
> > but changing everything is a big job.
>
> So:
>
> ldub -> ld1u,
>
> lduw_le -> ld2u_le,
>
> virtio_stl -> virtio_st4,
>
> stq_be -> st8_be.
>
> Right?
>
> Also we have:
>
> cpu_ld/st_*
> virtio_ld/st_*
> ld/st_*_phys
> ld/st_*_pci_dma
> address_space_ld/st
>
> While mass-changing, we could use FOO_ld/st_BAR with FOO
> for API and BAR for API variant (endian, mmuidx, ra, ...):
>
> So:
>
> ld/st_*_pci_dma -> pci_dma_ld/st_*
>
> for ld/st_*_phys I'm not sure.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field
2023-05-20 12:37 ` Peter Maydell
2023-05-20 13:15 ` BALATON Zoltan
@ 2023-05-22 11:59 ` Jonathan Cameron via
1 sibling, 0 replies; 17+ messages in thread
From: Jonathan Cameron via @ 2023-05-22 11:59 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
Ira Weiny, Michael Roth, Philippe Mathieu-Daudé, Dave Jiang,
Markus Armbruster, Daniel P . Berrangé, Eric Blake,
Mike Maslenkin, Marc-André Lureau, Thomas Huth
On Sat, 20 May 2023 13:37:42 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:
> On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
> <qemu-devel@nongnu.org> wrote:
> >
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > CXL has 24 bit unaligned fields which need to be stored to. CXL is
> > specified as little endian.
> >
> > Define st24_le_p() and the supporting functions to store such a field
> > from a 32 bit host native value.
> >
> > The use of b, w, l, q as the size specifier is limiting. So "24" was
> > used for the size part of the function name.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > docs/devel/loads-stores.rst | 1 +
> > include/qemu/bswap.h | 27 +++++++++++++++++++++++++++
> > 2 files changed, 28 insertions(+)
> >
> > diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
> > index d2cefc77a2..82a79e91d9 100644
> > --- a/docs/devel/loads-stores.rst
> > +++ b/docs/devel/loads-stores.rst
> > @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
> > ``size``
> > - ``b`` : 8 bits
> > - ``w`` : 16 bits
> > + - ``24`` : 24 bits
> > - ``l`` : 32 bits
> > - ``q`` : 64 bits
>
> Can you also update the "Regexes for git grep" section
> below to account for the new size value, please?
Ok. My regex isn't great, but I think this would require either some
separate entries or a switch to git grep -E to allow for the multiple
character matching.
So I've added
- ``\st24\(_[hbl]e\)\?_p\>``
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 2/4] hw/cxl: QMP based poison injection support
2023-05-19 14:17 [PATCH v6 0/4] hw/cxl: Poison get, inject, clear Jonathan Cameron via
2023-05-19 14:18 ` [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field Jonathan Cameron via
@ 2023-05-19 14:18 ` Jonathan Cameron via
2023-05-22 11:20 ` Jonathan Cameron via
2023-05-19 14:18 ` [PATCH v6 3/4] hw/cxl: Add poison injection via the mailbox Jonathan Cameron via
2023-05-19 14:18 ` [PATCH v6 4/4] hw/cxl: Add clear poison mailbox command support Jonathan Cameron via
3 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron via @ 2023-05-19 14:18 UTC (permalink / raw)
To: qemu-devel, Michael Tsirkin, Fan Ni
Cc: linux-cxl, linuxarm, Ira Weiny, Michael Roth,
Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
Marc-André Lureau, Thomas Huth
Inject poison using qmp command cxl-inject-poison to add an entry to the
poison list.
For now, the poison is not returned CXL.mem reads, but only via the
mailbox command Get Poison List. So a normal memory read to an address
that is on the poison list will not yet result in a synchronous exception
(and similar for partial cacheline writes).
That is left for a future patch.
See CXL rev 3.0, sec 8.2.9.8.4.1 Get Poison list (Opcode 4300h)
Kernel patches to use this interface here:
https://lore.kernel.org/linux-cxl/cover.1665606782.git.alison.schofield@intel.com/
To inject poison using qmp (telnet to the qmp port)
{ "execute": "qmp_capabilities" }
{ "execute": "cxl-inject-poison",
"arguments": {
"path": "/machine/peripheral/cxl-pmem0",
"start": 2048,
"length": 256
}
}
Adjusted to select a device on your machine.
Note that the poison list supported is kept short enough to avoid the
complexity of state machine that is needed to handle the MORE flag.
Reviewed-by: Fan Ni <fan.ni@samsung.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
hw/cxl/cxl-mailbox-utils.c | 90 +++++++++++++++++++++++++++++++++++++
hw/mem/cxl_type3.c | 56 +++++++++++++++++++++++
hw/mem/cxl_type3_stubs.c | 6 +++
include/hw/cxl/cxl.h | 1 +
include/hw/cxl/cxl_device.h | 20 +++++++++
qapi/cxl.json | 18 ++++++++
6 files changed, 191 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 702e16ca20..1f74b26ea2 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -62,6 +62,8 @@ enum {
#define GET_PARTITION_INFO 0x0
#define GET_LSA 0x2
#define SET_LSA 0x3
+ MEDIA_AND_POISON = 0x43,
+ #define GET_POISON_LIST 0x0
};
/* 8.2.8.4.5.1 Command Return Codes */
@@ -295,6 +297,10 @@ static CXLRetCode cmd_identify_memory_device(struct cxl_cmd *cmd,
stq_le_p(&id->persistent_capacity, cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
stq_le_p(&id->volatile_capacity, cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
stl_le_p(&id->lsa_size, cvc->get_lsa_size(ct3d));
+ /* 256 poison records */
+ st24_le_p(id->poison_list_max_mer, 256);
+ /* No limit - so limited by main poison record limit */
+ stw_le_p(&id->inject_poison_limit, 0);
*len = sizeof(*id);
return CXL_MBOX_SUCCESS;
@@ -384,6 +390,88 @@ static CXLRetCode cmd_ccls_set_lsa(struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+/*
+ * This is very inefficient, but good enough for now!
+ * Also the payload will always fit, so no need to handle the MORE flag and
+ * make this stateful. We may want to allow longer poison lists to aid
+ * testing that kernel functionality.
+ */
+static CXLRetCode cmd_media_get_poison_list(struct cxl_cmd *cmd,
+ CXLDeviceState *cxl_dstate,
+ uint16_t *len)
+{
+ struct get_poison_list_pl {
+ uint64_t pa;
+ uint64_t length;
+ } QEMU_PACKED;
+
+ struct get_poison_list_out_pl {
+ uint8_t flags;
+ uint8_t rsvd1;
+ uint64_t overflow_timestamp;
+ uint16_t count;
+ uint8_t rsvd2[0x14];
+ struct {
+ uint64_t addr;
+ uint32_t length;
+ uint32_t resv;
+ } QEMU_PACKED records[];
+ } QEMU_PACKED;
+
+ struct get_poison_list_pl *in = (void *)cmd->payload;
+ struct get_poison_list_out_pl *out = (void *)cmd->payload;
+ CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
+ uint16_t record_count = 0, i = 0;
+ uint64_t query_start, query_length;
+ CXLPoisonList *poison_list = &ct3d->poison_list;
+ CXLPoison *ent;
+ uint16_t out_pl_len;
+
+ query_start = ldq_le_p(&in->pa);
+ /* 64 byte alignemnt required */
+ if (query_start & 0x3f) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+ query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
+
+ QLIST_FOREACH(ent, poison_list, node) {
+ /* Check for no overlap */
+ if (ent->start >= query_start + query_length ||
+ ent->start + ent->length <= query_start) {
+ continue;
+ }
+ record_count++;
+ }
+ out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
+ assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
+
+ memset(out, 0, out_pl_len);
+ QLIST_FOREACH(ent, poison_list, node) {
+ uint64_t start, stop;
+
+ /* Check for no overlap */
+ if (ent->start >= query_start + query_length ||
+ ent->start + ent->length <= query_start) {
+ continue;
+ }
+
+ /* Deal with overlap */
+ start = MAX(ROUND_DOWN(ent->start, 64ull), query_start);
+ stop = MIN(ROUND_DOWN(ent->start, 64ull) + ent->length,
+ query_start + query_length);
+ stq_le_p(&out->records[i].addr, start | (ent->type & 0x7));
+ stl_le_p(&out->records[i].length, (stop - start) / CXL_CACHE_LINE_SIZE);
+ i++;
+ }
+ if (ct3d->poison_list_overflowed) {
+ out->flags = (1 << 1);
+ stq_le_p(&out->overflow_timestamp, ct3d->poison_list_overflow_ts);
+ }
+ stw_le_p(&out->count, record_count);
+ *len = out_pl_len;
+ return CXL_MBOX_SUCCESS;
+}
+
#define IMMEDIATE_CONFIG_CHANGE (1 << 1)
#define IMMEDIATE_DATA_CHANGE (1 << 2)
#define IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -411,6 +499,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
[CCLS][GET_LSA] = { "CCLS_GET_LSA", cmd_ccls_get_lsa, 8, 0 },
[CCLS][SET_LSA] = { "CCLS_SET_LSA", cmd_ccls_set_lsa,
~0, IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE },
+ [MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST",
+ cmd_media_get_poison_list, 16, 0 },
};
void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 2adacbd01b..ab600735eb 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -947,6 +947,62 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size,
*/
}
+void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d)
+{
+ ct3d->poison_list_overflowed = true;
+ ct3d->poison_list_overflow_ts =
+ cxl_device_get_timestamp(&ct3d->cxl_dstate);
+}
+
+void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
+ Error **errp)
+{
+ Object *obj = object_resolve_path(path, NULL);
+ CXLType3Dev *ct3d;
+ CXLPoison *p;
+
+ if (length % 64) {
+ error_setg(errp, "Poison injection must be in multiples of 64 bytes");
+ return;
+ }
+ if (start % 64) {
+ error_setg(errp, "Poison start address must be 64 byte aligned");
+ return;
+ }
+ if (!obj) {
+ error_setg(errp, "Unable to resolve path");
+ return;
+ }
+ if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
+ error_setg(errp, "Path does not point to a CXL type 3 device");
+ return;
+ }
+
+ ct3d = CXL_TYPE3(obj);
+
+ QLIST_FOREACH(p, &ct3d->poison_list, node) {
+ if (((start >= p->start) && (start < p->start + p->length)) ||
+ ((start + length > p->start) &&
+ (start + length <= p->start + p->length))) {
+ error_setg(errp, "Overlap with existing poisoned region not supported");
+ return;
+ }
+ }
+
+ if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
+ cxl_set_poison_list_overflowed(ct3d);
+ return;
+ }
+
+ p = g_new0(CXLPoison, 1);
+ p->length = length;
+ p->start = start;
+ p->type = CXL_POISON_TYPE_INTERNAL; /* Different from injected via the mbox */
+
+ QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
+ ct3d->poison_list_cnt++;
+}
+
/* For uncorrectable errors include support for multiple header recording */
void qmp_cxl_inject_uncorrectable_errors(const char *path,
CXLUncorErrorRecordList *errors,
diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
index d574c58f9a..fd1166a610 100644
--- a/hw/mem/cxl_type3_stubs.c
+++ b/hw/mem/cxl_type3_stubs.c
@@ -3,6 +3,12 @@
#include "qapi/error.h"
#include "qapi/qapi-commands-cxl.h"
+void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
+ Error **errp)
+{
+ error_setg(errp, "CXL Type 3 support is not compiled in");
+}
+
void qmp_cxl_inject_uncorrectable_errors(const char *path,
CXLUncorErrorRecordList *errors,
Error **errp)
diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
index c453983e83..56c9e7676e 100644
--- a/include/hw/cxl/cxl.h
+++ b/include/hw/cxl/cxl.h
@@ -18,6 +18,7 @@
#include "cxl_component.h"
#include "cxl_device.h"
+#define CXL_CACHE_LINE_SIZE 64
#define CXL_COMPONENT_REG_BAR_IDX 0
#define CXL_DEVICE_REG_BAR_IDX 2
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 02befda0f6..32c234ea91 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -242,6 +242,18 @@ typedef struct CXLError {
typedef QTAILQ_HEAD(, CXLError) CXLErrorList;
+typedef struct CXLPoison {
+ uint64_t start, length;
+ uint8_t type;
+#define CXL_POISON_TYPE_EXTERNAL 0x1
+#define CXL_POISON_TYPE_INTERNAL 0x2
+#define CXL_POISON_TYPE_INJECTED 0x3
+ QLIST_ENTRY(CXLPoison) node;
+} CXLPoison;
+
+typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
+#define CXL_POISON_LIST_LIMIT 256
+
struct CXLType3Dev {
/* Private */
PCIDevice parent_obj;
@@ -264,6 +276,12 @@ struct CXLType3Dev {
/* Error injection */
CXLErrorList error_list;
+
+ /* Poison Injection - cache */
+ CXLPoisonList poison_list;
+ unsigned int poison_list_cnt;
+ bool poison_list_overflowed;
+ uint64_t poison_list_overflow_ts;
};
#define TYPE_CXL_TYPE3 "cxl-type3"
@@ -289,4 +307,6 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
uint64_t cxl_device_get_timestamp(CXLDeviceState *cxlds);
+void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d);
+
#endif
diff --git a/qapi/cxl.json b/qapi/cxl.json
index b21c9b4c1c..dcf9e7b715 100644
--- a/qapi/cxl.json
+++ b/qapi/cxl.json
@@ -5,6 +5,24 @@
# = CXL devices
##
+##
+# @cxl-inject-poison:
+#
+# Poison records indicate that a CXL memory device knows that a particular
+# memory region may be corrupted. This may be because of locally detected
+# errors (e.g. ECC failure) or poisoned writes received from other components
+# in the system. This injection mechanism enables testing of the OS handling
+# of poison records which may be queried via the CXL mailbox.
+#
+# @path: CXL type 3 device canonical QOM path
+# @start: Start address - must be 64 byte aligned.
+# @length: Length of poison to inject - must be a multiple of 64 bytes.
+#
+# Since: 8.1
+##
+{ 'command': 'cxl-inject-poison',
+ 'data': { 'path': 'str', 'start': 'uint64', 'length': 'uint64' }}
+
##
# @CxlUncorErrorType:
#
--
2.39.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 2/4] hw/cxl: QMP based poison injection support
2023-05-19 14:18 ` [PATCH v6 2/4] hw/cxl: QMP based poison injection support Jonathan Cameron via
@ 2023-05-22 11:20 ` Jonathan Cameron via
0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron via @ 2023-05-22 11:20 UTC (permalink / raw)
To: qemu-devel, Michael Tsirkin, Fan Ni
Cc: linux-cxl, Ira Weiny, Michael Roth, Philippe Mathieu-Daudé,
Dave Jiang, Markus Armbruster, Daniel P . Berrangé,
Eric Blake, Mike Maslenkin, Marc-André Lureau, Thomas Huth
On Fri, 19 May 2023 15:18:01 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> Inject poison using qmp command cxl-inject-poison to add an entry to the
> poison list.
>
> For now, the poison is not returned CXL.mem reads, but only via the
> mailbox command Get Poison List. So a normal memory read to an address
> that is on the poison list will not yet result in a synchronous exception
> (and similar for partial cacheline writes).
> That is left for a future patch.
>
> See CXL rev 3.0, sec 8.2.9.8.4.1 Get Poison list (Opcode 4300h)
>
> Kernel patches to use this interface here:
> https://lore.kernel.org/linux-cxl/cover.1665606782.git.alison.schofield@intel.com/
>
> To inject poison using qmp (telnet to the qmp port)
> { "execute": "qmp_capabilities" }
>
> { "execute": "cxl-inject-poison",
> "arguments": {
> "path": "/machine/peripheral/cxl-pmem0",
> "start": 2048,
> "length": 256
> }
> }
>
> Adjusted to select a device on your machine.
>
> Note that the poison list supported is kept short enough to avoid the
> complexity of state machine that is needed to handle the MORE flag.
>
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
For reference. Markus Armbruster gave some feedback on v5 that crossed this this.
I'll incorporate into v7.
Thanks,
Jonathan
> ---
> hw/cxl/cxl-mailbox-utils.c | 90 +++++++++++++++++++++++++++++++++++++
> hw/mem/cxl_type3.c | 56 +++++++++++++++++++++++
> hw/mem/cxl_type3_stubs.c | 6 +++
> include/hw/cxl/cxl.h | 1 +
> include/hw/cxl/cxl_device.h | 20 +++++++++
> qapi/cxl.json | 18 ++++++++
> 6 files changed, 191 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 702e16ca20..1f74b26ea2 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -62,6 +62,8 @@ enum {
> #define GET_PARTITION_INFO 0x0
> #define GET_LSA 0x2
> #define SET_LSA 0x3
> + MEDIA_AND_POISON = 0x43,
> + #define GET_POISON_LIST 0x0
> };
>
> /* 8.2.8.4.5.1 Command Return Codes */
> @@ -295,6 +297,10 @@ static CXLRetCode cmd_identify_memory_device(struct cxl_cmd *cmd,
> stq_le_p(&id->persistent_capacity, cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
> stq_le_p(&id->volatile_capacity, cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
> stl_le_p(&id->lsa_size, cvc->get_lsa_size(ct3d));
> + /* 256 poison records */
> + st24_le_p(id->poison_list_max_mer, 256);
> + /* No limit - so limited by main poison record limit */
> + stw_le_p(&id->inject_poison_limit, 0);
>
> *len = sizeof(*id);
> return CXL_MBOX_SUCCESS;
> @@ -384,6 +390,88 @@ static CXLRetCode cmd_ccls_set_lsa(struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * This is very inefficient, but good enough for now!
> + * Also the payload will always fit, so no need to handle the MORE flag and
> + * make this stateful. We may want to allow longer poison lists to aid
> + * testing that kernel functionality.
> + */
> +static CXLRetCode cmd_media_get_poison_list(struct cxl_cmd *cmd,
> + CXLDeviceState *cxl_dstate,
> + uint16_t *len)
> +{
> + struct get_poison_list_pl {
> + uint64_t pa;
> + uint64_t length;
> + } QEMU_PACKED;
> +
> + struct get_poison_list_out_pl {
> + uint8_t flags;
> + uint8_t rsvd1;
> + uint64_t overflow_timestamp;
> + uint16_t count;
> + uint8_t rsvd2[0x14];
> + struct {
> + uint64_t addr;
> + uint32_t length;
> + uint32_t resv;
> + } QEMU_PACKED records[];
> + } QEMU_PACKED;
> +
> + struct get_poison_list_pl *in = (void *)cmd->payload;
> + struct get_poison_list_out_pl *out = (void *)cmd->payload;
> + CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> + uint16_t record_count = 0, i = 0;
> + uint64_t query_start, query_length;
> + CXLPoisonList *poison_list = &ct3d->poison_list;
> + CXLPoison *ent;
> + uint16_t out_pl_len;
> +
> + query_start = ldq_le_p(&in->pa);
> + /* 64 byte alignemnt required */
> + if (query_start & 0x3f) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> + query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
> +
> + QLIST_FOREACH(ent, poison_list, node) {
> + /* Check for no overlap */
> + if (ent->start >= query_start + query_length ||
> + ent->start + ent->length <= query_start) {
> + continue;
> + }
> + record_count++;
> + }
> + out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> + assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> +
> + memset(out, 0, out_pl_len);
> + QLIST_FOREACH(ent, poison_list, node) {
> + uint64_t start, stop;
> +
> + /* Check for no overlap */
> + if (ent->start >= query_start + query_length ||
> + ent->start + ent->length <= query_start) {
> + continue;
> + }
> +
> + /* Deal with overlap */
> + start = MAX(ROUND_DOWN(ent->start, 64ull), query_start);
> + stop = MIN(ROUND_DOWN(ent->start, 64ull) + ent->length,
> + query_start + query_length);
> + stq_le_p(&out->records[i].addr, start | (ent->type & 0x7));
> + stl_le_p(&out->records[i].length, (stop - start) / CXL_CACHE_LINE_SIZE);
> + i++;
> + }
> + if (ct3d->poison_list_overflowed) {
> + out->flags = (1 << 1);
> + stq_le_p(&out->overflow_timestamp, ct3d->poison_list_overflow_ts);
> + }
> + stw_le_p(&out->count, record_count);
> + *len = out_pl_len;
> + return CXL_MBOX_SUCCESS;
> +}
> +
> #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> #define IMMEDIATE_DATA_CHANGE (1 << 2)
> #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -411,6 +499,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
> [CCLS][GET_LSA] = { "CCLS_GET_LSA", cmd_ccls_get_lsa, 8, 0 },
> [CCLS][SET_LSA] = { "CCLS_SET_LSA", cmd_ccls_set_lsa,
> ~0, IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE },
> + [MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST",
> + cmd_media_get_poison_list, 16, 0 },
> };
>
> void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 2adacbd01b..ab600735eb 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -947,6 +947,62 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size,
> */
> }
>
> +void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d)
> +{
> + ct3d->poison_list_overflowed = true;
> + ct3d->poison_list_overflow_ts =
> + cxl_device_get_timestamp(&ct3d->cxl_dstate);
> +}
> +
> +void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
> + Error **errp)
> +{
> + Object *obj = object_resolve_path(path, NULL);
> + CXLType3Dev *ct3d;
> + CXLPoison *p;
> +
> + if (length % 64) {
> + error_setg(errp, "Poison injection must be in multiples of 64 bytes");
> + return;
> + }
> + if (start % 64) {
> + error_setg(errp, "Poison start address must be 64 byte aligned");
> + return;
> + }
> + if (!obj) {
> + error_setg(errp, "Unable to resolve path");
> + return;
> + }
> + if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
> + error_setg(errp, "Path does not point to a CXL type 3 device");
> + return;
> + }
> +
> + ct3d = CXL_TYPE3(obj);
> +
> + QLIST_FOREACH(p, &ct3d->poison_list, node) {
> + if (((start >= p->start) && (start < p->start + p->length)) ||
> + ((start + length > p->start) &&
> + (start + length <= p->start + p->length))) {
> + error_setg(errp, "Overlap with existing poisoned region not supported");
> + return;
> + }
> + }
> +
> + if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
> + cxl_set_poison_list_overflowed(ct3d);
> + return;
> + }
> +
> + p = g_new0(CXLPoison, 1);
> + p->length = length;
> + p->start = start;
> + p->type = CXL_POISON_TYPE_INTERNAL; /* Different from injected via the mbox */
> +
> + QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
> + ct3d->poison_list_cnt++;
> +}
> +
> /* For uncorrectable errors include support for multiple header recording */
> void qmp_cxl_inject_uncorrectable_errors(const char *path,
> CXLUncorErrorRecordList *errors,
> diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
> index d574c58f9a..fd1166a610 100644
> --- a/hw/mem/cxl_type3_stubs.c
> +++ b/hw/mem/cxl_type3_stubs.c
> @@ -3,6 +3,12 @@
> #include "qapi/error.h"
> #include "qapi/qapi-commands-cxl.h"
>
> +void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
> + Error **errp)
> +{
> + error_setg(errp, "CXL Type 3 support is not compiled in");
> +}
> +
> void qmp_cxl_inject_uncorrectable_errors(const char *path,
> CXLUncorErrorRecordList *errors,
> Error **errp)
> diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
> index c453983e83..56c9e7676e 100644
> --- a/include/hw/cxl/cxl.h
> +++ b/include/hw/cxl/cxl.h
> @@ -18,6 +18,7 @@
> #include "cxl_component.h"
> #include "cxl_device.h"
>
> +#define CXL_CACHE_LINE_SIZE 64
> #define CXL_COMPONENT_REG_BAR_IDX 0
> #define CXL_DEVICE_REG_BAR_IDX 2
>
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 02befda0f6..32c234ea91 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -242,6 +242,18 @@ typedef struct CXLError {
>
> typedef QTAILQ_HEAD(, CXLError) CXLErrorList;
>
> +typedef struct CXLPoison {
> + uint64_t start, length;
> + uint8_t type;
> +#define CXL_POISON_TYPE_EXTERNAL 0x1
> +#define CXL_POISON_TYPE_INTERNAL 0x2
> +#define CXL_POISON_TYPE_INJECTED 0x3
> + QLIST_ENTRY(CXLPoison) node;
> +} CXLPoison;
> +
> +typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> +#define CXL_POISON_LIST_LIMIT 256
> +
> struct CXLType3Dev {
> /* Private */
> PCIDevice parent_obj;
> @@ -264,6 +276,12 @@ struct CXLType3Dev {
>
> /* Error injection */
> CXLErrorList error_list;
> +
> + /* Poison Injection - cache */
> + CXLPoisonList poison_list;
> + unsigned int poison_list_cnt;
> + bool poison_list_overflowed;
> + uint64_t poison_list_overflow_ts;
> };
>
> #define TYPE_CXL_TYPE3 "cxl-type3"
> @@ -289,4 +307,6 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
>
> uint64_t cxl_device_get_timestamp(CXLDeviceState *cxlds);
>
> +void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d);
> +
> #endif
> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index b21c9b4c1c..dcf9e7b715 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -5,6 +5,24 @@
> # = CXL devices
> ##
>
> +##
> +# @cxl-inject-poison:
> +#
> +# Poison records indicate that a CXL memory device knows that a particular
> +# memory region may be corrupted. This may be because of locally detected
> +# errors (e.g. ECC failure) or poisoned writes received from other components
> +# in the system. This injection mechanism enables testing of the OS handling
> +# of poison records which may be queried via the CXL mailbox.
> +#
> +# @path: CXL type 3 device canonical QOM path
> +# @start: Start address - must be 64 byte aligned.
> +# @length: Length of poison to inject - must be a multiple of 64 bytes.
> +#
> +# Since: 8.1
> +##
> +{ 'command': 'cxl-inject-poison',
> + 'data': { 'path': 'str', 'start': 'uint64', 'length': 'uint64' }}
> +
> ##
> # @CxlUncorErrorType:
> #
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 3/4] hw/cxl: Add poison injection via the mailbox.
2023-05-19 14:17 [PATCH v6 0/4] hw/cxl: Poison get, inject, clear Jonathan Cameron via
2023-05-19 14:18 ` [PATCH v6 1/4] bswap: Add the ability to store to an unaligned 24 bit field Jonathan Cameron via
2023-05-19 14:18 ` [PATCH v6 2/4] hw/cxl: QMP based poison injection support Jonathan Cameron via
@ 2023-05-19 14:18 ` Jonathan Cameron via
2023-05-19 14:18 ` [PATCH v6 4/4] hw/cxl: Add clear poison mailbox command support Jonathan Cameron via
3 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron via @ 2023-05-19 14:18 UTC (permalink / raw)
To: qemu-devel, Michael Tsirkin, Fan Ni
Cc: linux-cxl, linuxarm, Ira Weiny, Michael Roth,
Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
Marc-André Lureau, Thomas Huth
Very simple implementation to allow testing of corresponding
kernel code. Note that for now we track each 64 byte section
independently. Whilst a valid implementation choice, it may
make sense to fuse entries so as to prove out more complex
corners of the kernel code.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
hw/cxl/cxl-mailbox-utils.c | 42 ++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 1f74b26ea2..6c476ad7f4 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -64,6 +64,7 @@ enum {
#define SET_LSA 0x3
MEDIA_AND_POISON = 0x43,
#define GET_POISON_LIST 0x0
+ #define INJECT_POISON 0x1
};
/* 8.2.8.4.5.1 Command Return Codes */
@@ -472,6 +473,45 @@ static CXLRetCode cmd_media_get_poison_list(struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+static CXLRetCode cmd_media_inject_poison(struct cxl_cmd *cmd,
+ CXLDeviceState *cxl_dstate,
+ uint16_t *len_unused)
+{
+ CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
+ CXLPoisonList *poison_list = &ct3d->poison_list;
+ CXLPoison *ent;
+ struct inject_poison_pl {
+ uint64_t dpa;
+ };
+ struct inject_poison_pl *in = (void *)cmd->payload;
+ uint64_t dpa = ldq_le_p(&in->dpa);
+ CXLPoison *p;
+
+ QLIST_FOREACH(ent, poison_list, node) {
+ if (dpa >= ent->start &&
+ dpa + CXL_CACHE_LINE_SIZE <= ent->start + ent->length) {
+ return CXL_MBOX_SUCCESS;
+ }
+ }
+
+ if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
+ return CXL_MBOX_INJECT_POISON_LIMIT;
+ }
+ p = g_new0(CXLPoison, 1);
+
+ p->length = CXL_CACHE_LINE_SIZE;
+ p->start = dpa;
+ p->type = CXL_POISON_TYPE_INJECTED;
+
+ /*
+ * Possible todo: Merge with existing entry if next to it and if same type
+ */
+ QLIST_INSERT_HEAD(poison_list, p, node);
+ ct3d->poison_list_cnt++;
+
+ return CXL_MBOX_SUCCESS;
+}
+
#define IMMEDIATE_CONFIG_CHANGE (1 << 1)
#define IMMEDIATE_DATA_CHANGE (1 << 2)
#define IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -501,6 +541,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
~0, IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE },
[MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST",
cmd_media_get_poison_list, 16, 0 },
+ [MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON",
+ cmd_media_inject_poison, 8, 0 },
};
void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
--
2.39.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 4/4] hw/cxl: Add clear poison mailbox command support.
2023-05-19 14:17 [PATCH v6 0/4] hw/cxl: Poison get, inject, clear Jonathan Cameron via
` (2 preceding siblings ...)
2023-05-19 14:18 ` [PATCH v6 3/4] hw/cxl: Add poison injection via the mailbox Jonathan Cameron via
@ 2023-05-19 14:18 ` Jonathan Cameron via
2023-05-19 17:48 ` Ira Weiny
3 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron via @ 2023-05-19 14:18 UTC (permalink / raw)
To: qemu-devel, Michael Tsirkin, Fan Ni
Cc: linux-cxl, linuxarm, Ira Weiny, Michael Roth,
Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
Marc-André Lureau, Thomas Huth
Current implementation is very simple so many of the corner
cases do not exist (e.g. fragmenting larger poison list entries)
Reviewed-by: Fan Ni <fan.ni@samsung.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
hw/cxl/cxl-mailbox-utils.c | 82 +++++++++++++++++++++++++++++++++++++
hw/mem/cxl_type3.c | 37 +++++++++++++++++
include/hw/cxl/cxl_device.h | 1 +
3 files changed, 120 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 6c476ad7f4..e3401b6be8 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -65,6 +65,7 @@ enum {
MEDIA_AND_POISON = 0x43,
#define GET_POISON_LIST 0x0
#define INJECT_POISON 0x1
+ #define CLEAR_POISON 0x2
};
/* 8.2.8.4.5.1 Command Return Codes */
@@ -512,6 +513,85 @@ static CXLRetCode cmd_media_inject_poison(struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+static CXLRetCode cmd_media_clear_poison(struct cxl_cmd *cmd,
+ CXLDeviceState *cxl_dstate,
+ uint16_t *len_unused)
+{
+ CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
+ CXLPoisonList *poison_list = &ct3d->poison_list;
+ CXLType3Class *cvc = CXL_TYPE3_GET_CLASS(ct3d);
+ struct clear_poison_pl {
+ uint64_t dpa;
+ uint8_t data[64];
+ };
+ CXLPoison *ent;
+ uint64_t dpa;
+
+ struct clear_poison_pl *in = (void *)cmd->payload;
+
+ dpa = ldq_le_p(&in->dpa);
+ if (dpa + CXL_CACHE_LINE_SIZE > cxl_dstate->mem_size) {
+ return CXL_MBOX_INVALID_PA;
+ }
+
+ /* Clearing a region with no poison is not an error so always do so */
+ if (cvc->set_cacheline) {
+ if (!cvc->set_cacheline(ct3d, dpa, in->data)) {
+ return CXL_MBOX_INTERNAL_ERROR;
+ }
+ }
+
+ QLIST_FOREACH(ent, poison_list, node) {
+ /*
+ * Test for contained in entry. Simpler than general case
+ * as clearing 64 bytes and entries 64 byte aligned
+ */
+ if ((dpa >= ent->start) && (dpa < ent->start + ent->length)) {
+ break;
+ }
+ }
+ if (!ent) {
+ return CXL_MBOX_SUCCESS;
+ }
+
+ QLIST_REMOVE(ent, node);
+ ct3d->poison_list_cnt--;
+
+ if (dpa > ent->start) {
+ CXLPoison *frag;
+ /* Cannot overflow as replacing existing entry */
+
+ frag = g_new0(CXLPoison, 1);
+
+ frag->start = ent->start;
+ frag->length = dpa - ent->start;
+ frag->type = ent->type;
+
+ QLIST_INSERT_HEAD(poison_list, frag, node);
+ ct3d->poison_list_cnt++;
+ }
+
+ if (dpa + CXL_CACHE_LINE_SIZE < ent->start + ent->length) {
+ CXLPoison *frag;
+
+ if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
+ cxl_set_poison_list_overflowed(ct3d);
+ } else {
+ frag = g_new0(CXLPoison, 1);
+
+ frag->start = dpa + CXL_CACHE_LINE_SIZE;
+ frag->length = ent->start + ent->length - frag->start;
+ frag->type = ent->type;
+ QLIST_INSERT_HEAD(poison_list, frag, node);
+ ct3d->poison_list_cnt++;
+ }
+ }
+ /* Any fragments have been added, free original entry */
+ g_free(ent);
+
+ return CXL_MBOX_SUCCESS;
+}
+
#define IMMEDIATE_CONFIG_CHANGE (1 << 1)
#define IMMEDIATE_DATA_CHANGE (1 << 2)
#define IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -543,6 +623,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
cmd_media_get_poison_list, 16, 0 },
[MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON",
cmd_media_inject_poison, 8, 0 },
+ [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON",
+ cmd_media_clear_poison, 72, 0 },
};
void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index ab600735eb..84022d7ae3 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -947,6 +947,42 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size,
*/
}
+static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
+{
+ MemoryRegion *vmr = NULL, *pmr = NULL;
+ AddressSpace *as;
+
+ if (ct3d->hostvmem) {
+ vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+ }
+ if (ct3d->hostpmem) {
+ pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+ }
+
+ if (!vmr && !pmr) {
+ return false;
+ }
+
+ if (dpa_offset + 64 > ct3d->cxl_dstate.mem_size) {
+ return false;
+ }
+
+ if (vmr) {
+ if (dpa_offset < memory_region_size(vmr)) {
+ as = &ct3d->hostvmem_as;
+ } else {
+ as = &ct3d->hostpmem_as;
+ dpa_offset -= memory_region_size(vmr);
+ }
+ } else {
+ as = &ct3d->hostpmem_as;
+ }
+
+ address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, &data,
+ CXL_CACHE_LINE_SIZE);
+ return true;
+}
+
void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d)
{
ct3d->poison_list_overflowed = true;
@@ -1168,6 +1204,7 @@ static void ct3_class_init(ObjectClass *oc, void *data)
cvc->get_lsa_size = get_lsa_size;
cvc->get_lsa = get_lsa;
cvc->set_lsa = set_lsa;
+ cvc->set_cacheline = set_cacheline;
}
static const TypeInfo ct3d_info = {
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 32c234ea91..73328a52cf 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -298,6 +298,7 @@ struct CXLType3Class {
uint64_t offset);
void (*set_lsa)(CXLType3Dev *ct3d, const void *buf, uint64_t size,
uint64_t offset);
+ bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data);
};
MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
--
2.39.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 4/4] hw/cxl: Add clear poison mailbox command support.
2023-05-19 14:18 ` [PATCH v6 4/4] hw/cxl: Add clear poison mailbox command support Jonathan Cameron via
@ 2023-05-19 17:48 ` Ira Weiny
0 siblings, 0 replies; 17+ messages in thread
From: Ira Weiny @ 2023-05-19 17:48 UTC (permalink / raw)
To: Jonathan Cameron, qemu-devel, Michael Tsirkin, Fan Ni
Cc: linux-cxl, linuxarm, Ira Weiny, Michael Roth,
Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
Marc-André Lureau, Thomas Huth
Jonathan Cameron wrote:
> Current implementation is very simple so many of the corner
> cases do not exist (e.g. fragmenting larger poison list entries)
>
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
Minor nit below. Otherwise looks good.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index ab600735eb..84022d7ae3 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -947,6 +947,42 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size,
> */
> }
>
> +static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
> +{
> + MemoryRegion *vmr = NULL, *pmr = NULL;
> + AddressSpace *as;
> +
> + if (ct3d->hostvmem) {
> + vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> + }
> + if (ct3d->hostpmem) {
> + pmr = host_memory_backend_get_memory(ct3d->hostpmem);
> + }
> +
> + if (!vmr && !pmr) {
> + return false;
> + }
> +
> + if (dpa_offset + 64 > ct3d->cxl_dstate.mem_size) {
NIT: s/64/CXL_CACHE_LINE_SIZE/
Ira
^ permalink raw reply [flat|nested] 17+ messages in thread