On Wed, Jan 24, 2024 at 02:51:18PM +0000, Jonathan Cameron wrote:
> On Tue, 7 Nov 2023 10:07:06 -0800
> nifan....@gmail.com wrote:
>
> > From: Fan Ni <fan...@samsung.com>
> >
> > Per cxl spec 3.0, add dynamic capacity region representative based on
> > Table 8-126 and extend the cxl type3 device definition to include dc region
> > information. Also, based on info in 8.2.9.8.9.1, add 'Get Dynamic Capacity
> > Configuration' mailbox support.
> >
> > Note: decode_len of a dc region is aligned to 256*MiB, need to be divided by
> > 256 * MiB before returned to the host for "Get Dynamic Capacity
> > Configuration"
> > mailbox command.
> >
> > Signed-off-by: Fan Ni <fan...@samsung.com>
>
> Hi Fan,
>
> I'm looking at how to move these much earlier in my tree on basis that
> they should be our main focus for merging in this QEMU cycle.
>
> Whilst I do that rebase, I'm taking a closer look at the code.
> I'm targetting rebasing on upstream qemu + the two patch sets I just
> sent out:
> [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup.
> [PATCH 0/5 qemu] hw/cxl: Update CXL emulation to reflect and reference r3.1
>
> It would be good to document why these commands should be optional (which I
> think
> comes down to the annoying fact that Get Dynamic Capacity Configuration isn't
> allowed to return 0 regions, but instead should not be available as a command
> if DCD isn't supported.
>
> Note this requires us to carry Gregory's patches to make the CCI command list
> constructed at runtime rather than baked in ahead of this set.
>
> So another question is should we jump directly to the r3.1 version of DCD?
> I think we probably should as it includes some additions that are necessary
> for a bunch of the potential use cases.
>
Based on cxl spec r3.1, the get dynamic capacity configuration output
payload (Table 8-164) have 4 extra items after the variable region configuration
structure. That is not allowed by the compiler, should we move the
new-added 4 items before the variable region configuration structures?
Fan
>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 80 +++++++++++++++++++++++++++++++++++++
> > hw/mem/cxl_type3.c | 6 +++
> > include/hw/cxl/cxl_device.h | 17 ++++++++
> > 3 files changed, 103 insertions(+)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 8eceedfa87..f80dd6474f 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -80,6 +80,8 @@ enum {
> > #define GET_POISON_LIST 0x0
> > #define INJECT_POISON 0x1
> > #define CLEAR_POISON 0x2
> > + DCD_CONFIG = 0x48,
> > + #define GET_DC_CONFIG 0x0
> > PHYSICAL_SWITCH = 0x51,
> > #define IDENTIFY_SWITCH_DEVICE 0x0
> > #define GET_PHYSICAL_PORT_STATE 0x1
> > @@ -1210,6 +1212,74 @@ static CXLRetCode cmd_media_clear_poison(const
> > struct cxl_cmd *cmd,
> > return CXL_MBOX_SUCCESS;
> > }
> >
> > +/*
> > + * CXL r3.0 section 8.2.9.8.9.1: Get Dynamic Capacity Configuration
>
> As per the patch set I just sent out, I want to standardize on references
> to r3.1 because it's all that is easy to get. However if we decide to r3.0
> DCD first the upgrade it later, then clearly these need to stick to r3.0 for
> now.
>
> > + * (Opcode: 4800h)
> > + */
> > +static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> > + uint8_t *payload_in,
> > + size_t len_in,
> > + uint8_t *payload_out,
> > + size_t *len_out,
> > + CXLCCI *cci)
> > +{
> > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > + struct get_dyn_cap_config_in_pl {
> > + uint8_t region_cnt;
> > + uint8_t start_region_id;
> > + } QEMU_PACKED;
> > +
> > + struct get_dyn_cap_config_out_pl {
> > + uint8_t num_regions;
> > + uint8_t rsvd1[7];
>
> This changed in r3.1 (errata? - I haven't checked)
> Should be 'regions returned' in first byte.
>
> > + struct {
> > + uint64_t base;
> > + uint64_t decode_len;
> > + uint64_t region_len;
> > + uint64_t block_size;
> > + uint32_t dsmadhandle;
>
> > + uint8_t flags;
> > + uint8_t rsvd2[3];
> > + } QEMU_PACKED records[];
>
> There are two fields after this as well.
> Total number of supported extents and number of available extents.
>
> That annoyingly means we can't use the structure to tell us where
> to find all the fields...
>
>
> > + } QEMU_PACKED;
> > +
> > + struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
> > + struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
> > + uint16_t record_count = 0, i;
>
> Better to split that on to 2 lines. Never hide setting a value
> in the middle of a set of declarations.
>
> > + uint16_t out_pl_len;
> > + uint8_t start_region_id = in->start_region_id;
> > +
> > + if (start_region_id >= ct3d->dc.num_regions) {
> > + return CXL_MBOX_INVALID_INPUT;
> > + }
> > +
> > + record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
> > + in->region_cnt);
> > +
> > + out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
>
> For r3.1 + 8 for the two trailing fields.
>
> > + assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> > +
> > + memset(out, 0, out_pl_len);
>
> As part of the cci rework we started zeroing the whole mailbox payload space
> after copying out the input payload.
> https://elixir.bootlin.com/qemu/latest/source/hw/cxl/cxl-device-utils.c#L204
>
> So shouldn't need this (unless we have a bug)
>
> Jonathan
