Re: [RFC PATCH 1/2] hw/cxl/type3: add missing flag bit for GMER

[Jonathan Cameron]

On Fri,  9 Feb 2024 19:54:11 +0800
Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote:

> The "Volatile" should be set if current device is a volatile memory.
> Per CXL Spec r3.0 8.2.9.2.1.1, Table 8-43.

Whilst true, we haven't previously adjusted the injected record
to conform to the device.  I don't think there is anything preventing
you setting this bit in the dpa fields whilst injecting.

I'm not against filling this in automatically but we should be
masking the relevant bits out of what is injected and filling it in based
on the dpa that is provided and which memory that falls within.


> 
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> ---
>  hw/mem/cxl_type3.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 52647b4ac7..d8fb63b1de 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1285,6 +1285,9 @@ static const QemuUUID memory_module_uuid = {
>                   0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74),
>  };
>  
> +#define CXL_GMER_DPA_VOLATILE                           BIT(0)
> +#define CXL_GMER_DPA_NOT_REPAIRABLE                     BIT(1)
> +
>  #define CXL_GMER_VALID_CHANNEL                          BIT(0)
>  #define CXL_GMER_VALID_RANK                             BIT(1)
>  #define CXL_GMER_VALID_DEVICE                           BIT(2)
> @@ -1348,6 +1351,9 @@ void qmp_cxl_inject_general_media_event(const char 
> *path, CxlEventLog log,
>      cxl_assign_event_header(hdr, &gen_media_uuid, flags, sizeof(gem),
>                              cxl_device_get_timestamp(&ct3d->cxl_dstate));
>  
> +    if (ct3d->hostvmem) {
> +        dpa |= CXL_GMER_DPA_VOLATILE;
> +    }

The presence of volatile memory isn't sufficient.  You may have a device
with both volatile and persistent memory. It will get even fiddlier with
Dynamic Capacity which will need checking as well once that support is
ready.



>      stq_le_p(&gem.phys_addr, dpa);
>      gem.descriptor = descriptor;
>      gem.type = type;