[RFC PATCH 1/3] hw/cxl/cxl-mailbox-utils: Add support for feature commands (8.2.9.6)

shiju.jose--- via posted 3 patches 1 year ago
Maintainers: Jonathan Cameron <jonathan.cameron@huawei.com>, Fan Ni <fan.ni@samsung.com>
There is a newer version of this series
[RFC PATCH 1/3] hw/cxl/cxl-mailbox-utils: Add support for feature commands (8.2.9.6)
Posted by shiju.jose--- via 1 year ago
From: Shiju Jose <shiju.jose@huawei.com>

CXL spec 3.0 section 8.2.9.6 describes optional device specific features.
CXL devices supports features with changeable attributes.
Get Supported Features retrieves the list of supported device specific
features. The settings of a feature can be retrieved using Get Feature and
optionally modified using Set Feature.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 140 +++++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 6184f44339..93960afd44 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -66,6 +66,10 @@ enum {
     LOGS        = 0x04,
         #define GET_SUPPORTED 0x0
         #define GET_LOG       0x1
+    FEATURES    = 0x05,
+        #define GET_SUPPORTED 0x0
+        #define GET_FEATURE   0x1
+        #define SET_FEATURE   0x2
     IDENTIFY    = 0x40,
         #define MEMORY_DEVICE 0x0
     CCLS        = 0x41,
@@ -785,6 +789,135 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+/* CXL r3.0 section 8.2.9.6: Features */
+typedef struct CXLSupportedFeatureHeader {
+    uint16_t entries;
+    uint16_t nsuppfeats_dev;
+    uint32_t reserved;
+} QEMU_PACKED CXLSupportedFeatureHeader;
+
+typedef struct CXLSupportedFeatureEntry {
+    QemuUUID uuid;
+    uint16_t feat_index;
+    uint16_t get_feat_size;
+    uint16_t set_feat_size;
+    uint32_t attrb_flags;
+    uint8_t get_feat_version;
+    uint8_t set_feat_version;
+    uint16_t set_feat_effects;
+    uint8_t rsvd[18];
+} QEMU_PACKED CXLSupportedFeatureEntry;
+
+enum CXL_SUPPORTED_FEATURES_LIST {
+    CXL_FEATURE_MAX
+};
+
+typedef struct CXLSetFeatureInHeader {
+        QemuUUID uuid;
+        uint32_t flags;
+        uint16_t offset;
+        uint8_t version;
+        uint8_t rsvd[9];
+} QEMU_PACKED QEMU_ALIGNED(16) CXLSetFeatureInHeader;
+
+#define CXL_SET_FEATURE_FLAG_DATA_TRANSFER_MASK   0x7
+#define CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER    0
+#define CXL_SET_FEATURE_FLAG_INITIATE_DATA_TRANSFER    1
+#define CXL_SET_FEATURE_FLAG_CONTINUE_DATA_TRANSFER    2
+#define CXL_SET_FEATURE_FLAG_FINISH_DATA_TRANSFER    3
+#define CXL_SET_FEATURE_FLAG_ABORT_DATA_TRANSFER    4
+
+/* CXL r3.0 section 8.2.9.6.1: Get Supported Features (Opcode 0500h) */
+static CXLRetCode cmd_features_get_supported(const struct cxl_cmd *cmd,
+                                             uint8_t *payload_in,
+                                             size_t len_in,
+                                             uint8_t *payload_out,
+                                             size_t *len_out,
+                                             CXLCCI *cci)
+{
+    struct {
+        uint32_t count;
+        uint16_t start_index;
+        uint16_t reserved;
+    } QEMU_PACKED QEMU_ALIGNED(16) * get_feats_in = (void *)payload_in;
+
+    struct {
+        CXLSupportedFeatureHeader hdr;
+        CXLSupportedFeatureEntry feat_entries[];
+    } QEMU_PACKED QEMU_ALIGNED(16) * supported_feats = (void *)payload_out;
+    uint16_t index;
+    uint16_t entry, req_entries;
+    uint16_t feat_entries = 0;
+
+    if (get_feats_in->count < sizeof(CXLSupportedFeatureHeader) ||
+        get_feats_in->start_index > CXL_FEATURE_MAX) {
+        return CXL_MBOX_INVALID_INPUT;
+    } else {
+        req_entries = (get_feats_in->count -
+                        sizeof(CXLSupportedFeatureHeader)) /
+                        sizeof(CXLSupportedFeatureEntry);
+    }
+    if (req_entries > CXL_FEATURE_MAX) {
+        req_entries = CXL_FEATURE_MAX;
+    }
+    supported_feats->hdr.nsuppfeats_dev = CXL_FEATURE_MAX;
+    index = get_feats_in->start_index;
+
+    entry = 0;
+    while (entry < req_entries) {
+        switch (index) {
+        default:
+            break;
+        }
+        index++;
+        entry++;
+    }
+
+    supported_feats->hdr.entries = feat_entries;
+    *len_out = sizeof(CXLSupportedFeatureHeader) +
+                      feat_entries * sizeof(CXLSupportedFeatureEntry);
+
+    return CXL_MBOX_SUCCESS;
+}
+
+/* CXL r3.0 section 8.2.9.6.2: Get Feature (Opcode 0501h) */
+static CXLRetCode cmd_features_get_feature(const struct cxl_cmd *cmd,
+                                           uint8_t *payload_in,
+                                           size_t len_in,
+                                           uint8_t *payload_out,
+                                           size_t *len_out,
+                                           CXLCCI *cci)
+{
+    struct {
+        QemuUUID uuid;
+        uint16_t offset;
+        uint16_t count;
+        uint8_t selection;
+    } QEMU_PACKED QEMU_ALIGNED(16) * get_feature;
+    uint16_t bytes_to_copy = 0;
+
+    get_feature = (void *)payload_in;
+
+    if (get_feature->offset + get_feature->count > cci->payload_max) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    *len_out = bytes_to_copy;
+
+    return CXL_MBOX_SUCCESS;
+}
+
+/* CXL r3.0 section 8.2.9.6.3: Set Feature (Opcode 0502h) */
+static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
+                                           uint8_t *payload_in,
+                                           size_t len_in,
+                                           uint8_t *payload_out,
+                                           size_t *len_out,
+                                           CXLCCI *cci)
+{
+    return CXL_MBOX_SUCCESS;
+}
+
 /* 8.2.9.5.1.1 */
 static CXLRetCode cmd_identify_memory_device(const struct cxl_cmd *cmd,
                                              uint8_t *payload_in,
@@ -1954,6 +2087,13 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
     [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported,
                               0, 0 },
     [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 },
+    [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
+                                  cmd_features_get_supported, 0x8, 0 },
+    [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
+                                cmd_features_get_feature, 0x15, 0 },
+    [FEATURES][SET_FEATURE] = { "FEATURES_SET_FEATURE",
+                                cmd_features_set_feature,
+                                ~0, CXL_MBOX_IMMEDIATE_CONFIG_CHANGE },
     [IDENTIFY][MEMORY_DEVICE] = { "IDENTIFY_MEMORY_DEVICE",
         cmd_identify_memory_device, 0, 0 },
     [CCLS][GET_PARTITION_INFO] = { "CCLS_GET_PARTITION_INFO",
-- 
2.34.1
Re: [RFC PATCH 1/3] hw/cxl/cxl-mailbox-utils: Add support for feature commands (8.2.9.6)
Posted by Davidlohr Bueso 1 year ago
On Tue, 14 Nov 2023, shiju.jose@huawei.com wrote:

>From: Shiju Jose <shiju.jose@huawei.com>
>
>CXL spec 3.0 section 8.2.9.6 describes optional device specific features.
>CXL devices supports features with changeable attributes.
>Get Supported Features retrieves the list of supported device specific
>features. The settings of a feature can be retrieved using Get Feature and
>optionally modified using Set Feature.
>
>Signed-off-by: Shiju Jose <shiju.jose@huawei.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

... with some comments below.

>---
> hw/cxl/cxl-mailbox-utils.c | 140 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 140 insertions(+)
>
>diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>index 6184f44339..93960afd44 100644
>--- a/hw/cxl/cxl-mailbox-utils.c
>+++ b/hw/cxl/cxl-mailbox-utils.c
>@@ -66,6 +66,10 @@ enum {
>     LOGS        = 0x04,
>         #define GET_SUPPORTED 0x0
>         #define GET_LOG       0x1
>+    FEATURES    = 0x05,
>+        #define GET_SUPPORTED 0x0
>+        #define GET_FEATURE   0x1
>+        #define SET_FEATURE   0x2
>     IDENTIFY    = 0x40,
>         #define MEMORY_DEVICE 0x0
>     CCLS        = 0x41,
>@@ -785,6 +789,135 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd,
>     return CXL_MBOX_SUCCESS;
> }
>
>+/* CXL r3.0 section 8.2.9.6: Features */
>+typedef struct CXLSupportedFeatureHeader {
>+    uint16_t entries;
>+    uint16_t nsuppfeats_dev;
>+    uint32_t reserved;
>+} QEMU_PACKED CXLSupportedFeatureHeader;
>+
>+typedef struct CXLSupportedFeatureEntry {
>+    QemuUUID uuid;
>+    uint16_t feat_index;
>+    uint16_t get_feat_size;
>+    uint16_t set_feat_size;
>+    uint32_t attrb_flags;
>+    uint8_t get_feat_version;
>+    uint8_t set_feat_version;
>+    uint16_t set_feat_effects;
>+    uint8_t rsvd[18];
>+} QEMU_PACKED CXLSupportedFeatureEntry;
>+
>+enum CXL_SUPPORTED_FEATURES_LIST {
>+    CXL_FEATURE_MAX
>+};
>+
>+typedef struct CXLSetFeatureInHeader {
>+        QemuUUID uuid;
>+        uint32_t flags;
>+        uint16_t offset;
>+        uint8_t version;
>+        uint8_t rsvd[9];
>+} QEMU_PACKED QEMU_ALIGNED(16) CXLSetFeatureInHeader;
>+
>+#define CXL_SET_FEATURE_FLAG_DATA_TRANSFER_MASK   0x7
>+#define CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER    0
>+#define CXL_SET_FEATURE_FLAG_INITIATE_DATA_TRANSFER    1
>+#define CXL_SET_FEATURE_FLAG_CONTINUE_DATA_TRANSFER    2
>+#define CXL_SET_FEATURE_FLAG_FINISH_DATA_TRANSFER    3
>+#define CXL_SET_FEATURE_FLAG_ABORT_DATA_TRANSFER    4

Maybe enum here?

>+
>+/* CXL r3.0 section 8.2.9.6.1: Get Supported Features (Opcode 0500h) */
>+static CXLRetCode cmd_features_get_supported(const struct cxl_cmd *cmd,
>+                                             uint8_t *payload_in,
>+                                             size_t len_in,
>+                                             uint8_t *payload_out,
>+                                             size_t *len_out,
>+                                             CXLCCI *cci)
>+{
>+    struct {
>+        uint32_t count;
>+        uint16_t start_index;
>+        uint16_t reserved;
>+    } QEMU_PACKED QEMU_ALIGNED(16) * get_feats_in = (void *)payload_in;
>+
>+    struct {
>+        CXLSupportedFeatureHeader hdr;
>+        CXLSupportedFeatureEntry feat_entries[];
>+    } QEMU_PACKED QEMU_ALIGNED(16) * supported_feats = (void *)payload_out;

s/supported_feats/get_feats_out.

>+    uint16_t index;
>+    uint16_t entry, req_entries;
>+    uint16_t feat_entries = 0;
>+
>+    if (get_feats_in->count < sizeof(CXLSupportedFeatureHeader) ||
>+        get_feats_in->start_index > CXL_FEATURE_MAX) {

Ah I see you update this to '>=' in the next patch.

>+        return CXL_MBOX_INVALID_INPUT;
>+    } else {

This branch is not needed.

>+        req_entries = (get_feats_in->count -
>+                        sizeof(CXLSupportedFeatureHeader)) /
>+                        sizeof(CXLSupportedFeatureEntry);
>+    }
>+    if (req_entries > CXL_FEATURE_MAX) {
>+        req_entries = CXL_FEATURE_MAX;
>+    }

min()?

>+    supported_feats->hdr.nsuppfeats_dev = CXL_FEATURE_MAX;

Logically this should go below, when setting the feature entries.

>+    index = get_feats_in->start_index;
>+
>+    entry = 0;
>+    while (entry < req_entries) {
>+        switch (index) {
>+        default:
>+            break;
>+        }
>+        index++;
>+        entry++;
>+    }
>+
>+    supported_feats->hdr.entries = feat_entries;
>+    *len_out = sizeof(CXLSupportedFeatureHeader) +
>+                      feat_entries * sizeof(CXLSupportedFeatureEntry);
>+
>+    return CXL_MBOX_SUCCESS;
>+}
>+
>+/* CXL r3.0 section 8.2.9.6.2: Get Feature (Opcode 0501h) */
>+static CXLRetCode cmd_features_get_feature(const struct cxl_cmd *cmd,
>+                                           uint8_t *payload_in,
>+                                           size_t len_in,
>+                                           uint8_t *payload_out,
>+                                           size_t *len_out,
>+                                           CXLCCI *cci)
>+{
>+    struct {
>+        QemuUUID uuid;
>+        uint16_t offset;
>+        uint16_t count;
>+        uint8_t selection;
>+    } QEMU_PACKED QEMU_ALIGNED(16) * get_feature;
>+    uint16_t bytes_to_copy = 0;
>+
>+    get_feature = (void *)payload_in;
>+
>+    if (get_feature->offset + get_feature->count > cci->payload_max) {
>+        return CXL_MBOX_INVALID_INPUT;
>+    }

For now maybe return unsupported if a non-zero selection is passed?

>+
>+    *len_out = bytes_to_copy;
>+
>+    return CXL_MBOX_SUCCESS;
>+}
>+
>+/* CXL r3.0 section 8.2.9.6.3: Set Feature (Opcode 0502h) */
>+static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>+                                           uint8_t *payload_in,
>+                                           size_t len_in,
>+                                           uint8_t *payload_out,
>+                                           size_t *len_out,
>+                                           CXLCCI *cci)
>+{
>+    return CXL_MBOX_SUCCESS;
>+}
>+
> /* 8.2.9.5.1.1 */
> static CXLRetCode cmd_identify_memory_device(const struct cxl_cmd *cmd,
>                                              uint8_t *payload_in,
>@@ -1954,6 +2087,13 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>     [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported,
>                               0, 0 },
>     [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 },
>+    [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
>+                                  cmd_features_get_supported, 0x8, 0 },
>+    [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
>+                                cmd_features_get_feature, 0x15, 0 },
>+    [FEATURES][SET_FEATURE] = { "FEATURES_SET_FEATURE",
>+                                cmd_features_set_feature,
>+                                ~0, CXL_MBOX_IMMEDIATE_CONFIG_CHANGE },

I don't think we are actually doing anything with these, but in addition to
the config, set_feature would need IMMEDIATE_DATA_CHANGE, IMMEDIATE_POLICY_CHANGE,
IMMEDIATE_LOG_CHANGE and SECURITY_STATE_CHANGE.

>     [IDENTIFY][MEMORY_DEVICE] = { "IDENTIFY_MEMORY_DEVICE",
>         cmd_identify_memory_device, 0, 0 },
>     [CCLS][GET_PARTITION_INFO] = { "CCLS_GET_PARTITION_INFO",
>--
>2.34.1
>
RE: [RFC PATCH 1/3] hw/cxl/cxl-mailbox-utils: Add support for feature commands (8.2.9.6)
Posted by Shiju Jose via 1 year ago
Hi Davidlohr,

Thanks for reviewing and for the comments.

>-----Original Message-----
>From: Davidlohr Bueso <dave@stgolabs.net>
>Sent: 20 November 2023 19:45
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: qemu-devel@nongnu.org; linux-cxl@vger.kernel.org; Jonathan Cameron
><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>;
>Zengtao (B) <prime.zeng@hisilicon.com>; Linuxarm <linuxarm@huawei.com>;
>fan.ni@samsung.com; a.manzanares@samsung.com
>Subject: Re: [RFC PATCH 1/3] hw/cxl/cxl-mailbox-utils: Add support for feature
>commands (8.2.9.6)
>
>On Tue, 14 Nov 2023, shiju.jose@huawei.com wrote:
>
>>From: Shiju Jose <shiju.jose@huawei.com>
>>
>>CXL spec 3.0 section 8.2.9.6 describes optional device specific features.
>>CXL devices supports features with changeable attributes.
>>Get Supported Features retrieves the list of supported device specific
>>features. The settings of a feature can be retrieved using Get Feature
>>and optionally modified using Set Feature.
>>
>>Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>
>Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>
>... with some comments below.
>
>>---
>> hw/cxl/cxl-mailbox-utils.c | 140 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 140 insertions(+)
>>
>>diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>>index 6184f44339..93960afd44 100644
>>--- a/hw/cxl/cxl-mailbox-utils.c
>>+++ b/hw/cxl/cxl-mailbox-utils.c
>>@@ -66,6 +66,10 @@ enum {
>>     LOGS        = 0x04,
>>         #define GET_SUPPORTED 0x0
>>         #define GET_LOG       0x1
>>+    FEATURES    = 0x05,
>>+        #define GET_SUPPORTED 0x0
>>+        #define GET_FEATURE   0x1
>>+        #define SET_FEATURE   0x2
>>     IDENTIFY    = 0x40,
>>         #define MEMORY_DEVICE 0x0
>>     CCLS        = 0x41,
>>@@ -785,6 +789,135 @@ static CXLRetCode cmd_logs_get_log(const struct
>cxl_cmd *cmd,
>>     return CXL_MBOX_SUCCESS;
>> }
>>
>>+/* CXL r3.0 section 8.2.9.6: Features */ typedef struct
>>+CXLSupportedFeatureHeader {
>>+    uint16_t entries;
>>+    uint16_t nsuppfeats_dev;
>>+    uint32_t reserved;
>>+} QEMU_PACKED CXLSupportedFeatureHeader;
>>+
>>+typedef struct CXLSupportedFeatureEntry {
>>+    QemuUUID uuid;
>>+    uint16_t feat_index;
>>+    uint16_t get_feat_size;
>>+    uint16_t set_feat_size;
>>+    uint32_t attrb_flags;
>>+    uint8_t get_feat_version;
>>+    uint8_t set_feat_version;
>>+    uint16_t set_feat_effects;
>>+    uint8_t rsvd[18];
>>+} QEMU_PACKED CXLSupportedFeatureEntry;
>>+
>>+enum CXL_SUPPORTED_FEATURES_LIST {
>>+    CXL_FEATURE_MAX
>>+};
>>+
>>+typedef struct CXLSetFeatureInHeader {
>>+        QemuUUID uuid;
>>+        uint32_t flags;
>>+        uint16_t offset;
>>+        uint8_t version;
>>+        uint8_t rsvd[9];
>>+} QEMU_PACKED QEMU_ALIGNED(16) CXLSetFeatureInHeader;
>>+
>>+#define CXL_SET_FEATURE_FLAG_DATA_TRANSFER_MASK   0x7
>>+#define CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER    0
>>+#define CXL_SET_FEATURE_FLAG_INITIATE_DATA_TRANSFER    1
>>+#define CXL_SET_FEATURE_FLAG_CONTINUE_DATA_TRANSFER    2
>>+#define CXL_SET_FEATURE_FLAG_FINISH_DATA_TRANSFER    3
>>+#define CXL_SET_FEATURE_FLAG_ABORT_DATA_TRANSFER    4
>
>Maybe enum here?
Sure. I will change.

>
>>+
>>+/* CXL r3.0 section 8.2.9.6.1: Get Supported Features (Opcode 0500h)
>>+*/ static CXLRetCode cmd_features_get_supported(const struct cxl_cmd
>*cmd,
>>+                                             uint8_t *payload_in,
>>+                                             size_t len_in,
>>+                                             uint8_t *payload_out,
>>+                                             size_t *len_out,
>>+                                             CXLCCI *cci) {
>>+    struct {
>>+        uint32_t count;
>>+        uint16_t start_index;
>>+        uint16_t reserved;
>>+    } QEMU_PACKED QEMU_ALIGNED(16) * get_feats_in = (void
>>+*)payload_in;
>>+
>>+    struct {
>>+        CXLSupportedFeatureHeader hdr;
>>+        CXLSupportedFeatureEntry feat_entries[];
>>+    } QEMU_PACKED QEMU_ALIGNED(16) * supported_feats = (void
>>+ *)payload_out;
>
>s/supported_feats/get_feats_out.
Will change.

>
>>+    uint16_t index;
>>+    uint16_t entry, req_entries;
>>+    uint16_t feat_entries = 0;
>>+
>>+    if (get_feats_in->count < sizeof(CXLSupportedFeatureHeader) ||
>>+        get_feats_in->start_index > CXL_FEATURE_MAX) {
>
>Ah I see you update this to '>=' in the next patch.
>
>>+        return CXL_MBOX_INVALID_INPUT;
>>+    } else {
>
>This branch is not needed.
Ok.

>
>>+        req_entries = (get_feats_in->count -
>>+                        sizeof(CXLSupportedFeatureHeader)) /
>>+                        sizeof(CXLSupportedFeatureEntry);
>>+    }
>>+    if (req_entries > CXL_FEATURE_MAX) {
>>+        req_entries = CXL_FEATURE_MAX;
>>+    }
>
>min()?
Sure.

>
>>+    supported_feats->hdr.nsuppfeats_dev = CXL_FEATURE_MAX;
>
>Logically this should go below, when setting the feature entries.
>
>>+    index = get_feats_in->start_index;
>>+
>>+    entry = 0;
>>+    while (entry < req_entries) {
>>+        switch (index) {
>>+        default:
>>+            break;
>>+        }
>>+        index++;
>>+        entry++;
>>+    }
>>+
>>+    supported_feats->hdr.entries = feat_entries;
>>+    *len_out = sizeof(CXLSupportedFeatureHeader) +
>>+                      feat_entries * sizeof(CXLSupportedFeatureEntry);
>>+
>>+    return CXL_MBOX_SUCCESS;
>>+}
>>+
>>+/* CXL r3.0 section 8.2.9.6.2: Get Feature (Opcode 0501h) */ static
>>+CXLRetCode cmd_features_get_feature(const struct cxl_cmd *cmd,
>>+                                           uint8_t *payload_in,
>>+                                           size_t len_in,
>>+                                           uint8_t *payload_out,
>>+                                           size_t *len_out,
>>+                                           CXLCCI *cci) {
>>+    struct {
>>+        QemuUUID uuid;
>>+        uint16_t offset;
>>+        uint16_t count;
>>+        uint8_t selection;
>>+    } QEMU_PACKED QEMU_ALIGNED(16) * get_feature;
>>+    uint16_t bytes_to_copy = 0;
>>+
>>+    get_feature = (void *)payload_in;
>>+
>>+    if (get_feature->offset + get_feature->count > cci->payload_max) {
>>+        return CXL_MBOX_INVALID_INPUT;
>>+    }
>
>For now maybe return unsupported if a non-zero selection is passed?
Sure.

>
>>+
>>+    *len_out = bytes_to_copy;
>>+
>>+    return CXL_MBOX_SUCCESS;
>>+}
>>+
>>+/* CXL r3.0 section 8.2.9.6.3: Set Feature (Opcode 0502h) */ static
>>+CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>>+                                           uint8_t *payload_in,
>>+                                           size_t len_in,
>>+                                           uint8_t *payload_out,
>>+                                           size_t *len_out,
>>+                                           CXLCCI *cci) {
>>+    return CXL_MBOX_SUCCESS;
>>+}
>>+
>> /* 8.2.9.5.1.1 */
>> static CXLRetCode cmd_identify_memory_device(const struct cxl_cmd *cmd,
>>                                              uint8_t *payload_in, @@
>>-1954,6 +2087,13 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>>     [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED",
>cmd_logs_get_supported,
>>                               0, 0 },
>>     [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 },
>>+    [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
>>+                                  cmd_features_get_supported, 0x8, 0 },
>>+    [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
>>+                                cmd_features_get_feature, 0x15, 0 },
>>+    [FEATURES][SET_FEATURE] = { "FEATURES_SET_FEATURE",
>>+                                cmd_features_set_feature,
>>+                                ~0, CXL_MBOX_IMMEDIATE_CONFIG_CHANGE
>>+ },
>
>I don't think we are actually doing anything with these, but in addition to the
>config, set_feature would need IMMEDIATE_DATA_CHANGE,
>IMMEDIATE_POLICY_CHANGE, IMMEDIATE_LOG_CHANGE and
>SECURITY_STATE_CHANGE.
Sure. I will change.

>
>>     [IDENTIFY][MEMORY_DEVICE] = { "IDENTIFY_MEMORY_DEVICE",
>>         cmd_identify_memory_device, 0, 0 },
>>     [CCLS][GET_PARTITION_INFO] = { "CCLS_GET_PARTITION_INFO",
>>--
>>2.34.1
>>

Thanks,
Shiju