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

[Shiju Jose via <qemu-devel@nongnu.org>]

Hi Fan,

Thanks for the feedbacks.

>-----Original Message-----
>From: fan <nifan.cxl@gmail.com>
>Sent: 15 February 2024 18:06
>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>
>Subject: Re: [PATCH v2 1/3] hw/cxl/cxl-mailbox-utils: Add support for feature
>commands (8.2.9.6)
>
>On Fri, Nov 24, 2023 at 09:53:35PM +0800, 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.
>>
>> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>> ---
>
>Updated the references to align with cxl spec r3.1, other than that looks good to
>me.

I had posted v3 of this series updated for spec r3.1. Please find here,
https://lore.kernel.org/qemu-devel/20240215110146.1444-1-shiju.jose@huawei.com/T/#t

>
>Fan
>
>>  hw/cxl/cxl-mailbox-utils.c | 167
>> +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 167 insertions(+)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index 6184f44339..1bbc9a48a6 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,157 @@ 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
>> +};
>> +
>> +/* Get Feature CXL 3.0 Spec 8.2.9.6.2 */
>> +/*
>> + * Get Feature input payload
>> + * CXL rev 3.0 section 8.2.9.6.2; Table 8-79  */
>> +/* Get Feature : Payload in selection */ enum
>> +CXL_GET_FEATURE_SELECTION {
>> +    CXL_GET_FEATURE_SEL_CURRENT_VALUE = 0x0,
>> +    CXL_GET_FEATURE_SEL_DEFAULT_VALUE = 0x1,
>> +    CXL_GET_FEATURE_SEL_SAVED_VALUE = 0x2,
>> +    CXL_GET_FEATURE_SEL_MAX
>> +};
>> +
>> +/* Set Feature CXL 3.0 Spec 8.2.9.6.3 */
>> +/*
>> + * Set Feature input payload
>> + * CXL rev 3.0 section 8.2.9.6.3; Table 8-81  */ typedef struct
>> +CXLSetFeatureInHeader {
>> +        QemuUUID uuid;
>> +        uint32_t flags;
>> +        uint16_t offset;
>> +        uint8_t version;
>> +        uint8_t rsvd[9];
>> +} QEMU_PACKED QEMU_ALIGNED(16) CXLSetFeatureInHeader;
>> +
>> +/* Set Feature : Payload in flags */
>> +#define CXL_SET_FEATURE_FLAG_DATA_TRANSFER_MASK   0x7
>> +enum CXL_SET_FEATURE_FLAG_DATA_TRANSFER {
>> +    CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER = 0x0,
>> +    CXL_SET_FEATURE_FLAG_INITIATE_DATA_TRANSFER = 0x1,
>> +    CXL_SET_FEATURE_FLAG_CONTINUE_DATA_TRANSFER = 0x2,
>> +    CXL_SET_FEATURE_FLAG_FINISH_DATA_TRANSFER = 0x3,
>> +    CXL_SET_FEATURE_FLAG_ABORT_DATA_TRANSFER = 0x4,
>> +    CXL_SET_FEATURE_FLAG_DATA_TRANSFER_MAX
>> +};
>> +
>> +/* 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) * get_feats_out = (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;
>> +    }
>> +    req_entries = (get_feats_in->count -
>> +                   sizeof(CXLSupportedFeatureHeader)) /
>> +                   sizeof(CXLSupportedFeatureEntry);
>> +    req_entries = MIN(req_entries, CXL_FEATURE_MAX);
>> +    index = get_feats_in->start_index;
>> +
>> +    entry = 0;
>> +    while (entry < req_entries) {
>> +        switch (index) {
>> +        default:
>> +            break;
>> +        }
>> +        index++;
>> +        entry++;
>> +    }
>> +
>> +    get_feats_out->hdr.nsuppfeats_dev = CXL_FEATURE_MAX;
>> +    get_feats_out->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->selection != CXL_GET_FEATURE_SEL_CURRENT_VALUE) {
>> +        return CXL_MBOX_UNSUPPORTED;
>> +    }
>> +    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 +2109,18 @@ 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 |
>> +                                 CXL_MBOX_IMMEDIATE_DATA_CHANGE |
>> +                                 CXL_MBOX_IMMEDIATE_POLICY_CHANGE |
>> +                                 CXL_MBOX_IMMEDIATE_LOG_CHANGE |
>> +                                 CXL_MBOX_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
>>

Thanks,
Shiju