Re: [PATCH] Add support for FMAPI Get Mutliheaded Head info opcode (5501h)

[Alok Rathore <alok.rathore@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 7636 bytes --]

On 29/05/25 01:13PM, Jonathan Cameron wrote:
>On Thu, 22 May 2025 12:01:35 +0530
>Vinayak Holikatti <vinayak.kh@samsung.com> wrote:
>
>> CXL spec 3.2 section 7.6.7.5.2  describes Get Head Info.
>>
>> Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com>
>
>Hi Vinayak,
>
>Some code simplification suggestions below.

Thanks Jonathan for reviewing the patch.

Alok
>
>> ---
>> This patch is generated against Jonathan Cameron's branch cxl-2025-03-20
>>
>>  hw/cxl/cxl-mailbox-utils.c  | 21 +++++++++
>>  hw/cxl/mhsld/mhsld.c        | 92 ++++++++++++++++++++++++++++++++++++-
>>  hw/cxl/mhsld/mhsld.h        | 26 +++++++++++
>>  include/hw/cxl/cxl_device.h |  6 +++
>>  4 files changed, 144 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index a02d130926..4f25caecea 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>
>> diff --git a/hw/cxl/mhsld/mhsld.c b/hw/cxl/mhsld/mhsld.c
>> index 9f633b3bed..981546b5ff 100644
>> --- a/hw/cxl/mhsld/mhsld.c
>> +++ b/hw/cxl/mhsld/mhsld.c
>> @@ -61,9 +61,57 @@ static CXLRetCode cmd_mhd_get_info(const struct cxl_cmd *cmd,
>>      return CXL_MBOX_SUCCESS;
>>  }
>>
>> +/*
>> + * CXL r3.2 section 7.6.7.5.2 - Get Head Info (Opcode 5501h)
>> + *
>> + * This command retrieves the number of heads, number of supported LDs,
>> + * and Head-to-LD mapping of a Multi-Headed device.
>> + */
>> +static CXLRetCode cmd_mhd_get_head_info(const struct cxl_cmd *cmd,
>> +                                        uint8_t *payload_in, size_t len_in,
>> +                                        uint8_t *payload_out, size_t *len_out,
>> +                                        CXLCCI *cci)
>> +{
>> +    CXLMHSLDState *s = CXL_MHSLD(cci->d);
>> +    MHDGetHeadInfoInput *input = (void *)payload_in;
>> +    MHDGetHeadInfoOutput *output = (void *)payload_out;
>> +    int i = 0;
>> +
>> +    if (input->start_head > MHSLD_HEADS) {
>> +        return CXL_MBOX_INVALID_INPUT;
>> +    }
>> +
>> +    output->nr_heads = MIN((MHSLD_HEADS - input->start_head), input->nr_heads);
>> +    for (i = input->start_head; i < input->start_head + output->nr_heads; i++) {
>
>Can we get away with a memcpy()?  Any endian issues on any of these?

Sure, I’ll check with memcpy()

>
>> +        output->head_info_list[i].port_number =
>> +                                 s->mhd_state->head_info_blocks[i].port_number;
>> +        output->head_info_list[i].max_link_width =
>> +                              s->mhd_state->head_info_blocks[i].max_link_width;
>> +        output->head_info_list[i].nego_link_width =
>> +                             s->mhd_state->head_info_blocks[i].nego_link_width;
>> +        output->head_info_list[i].supp_link_speeds_vector =
>> +                     s->mhd_state->head_info_blocks[i].supp_link_speeds_vector;
>> +        output->head_info_list[i].max_link_speed =
>> +                              s->mhd_state->head_info_blocks[i].max_link_speed;
>> +        output->head_info_list[i].current_link_speed =
>> +                          s->mhd_state->head_info_blocks[i].current_link_speed;
>> +        output->head_info_list[i].ltssm_state =
>> +                                 s->mhd_state->head_info_blocks[i].ltssm_state;
>> +        output->head_info_list[i].first_nego_lane_num =
>> +                         s->mhd_state->head_info_blocks[i].first_nego_lane_num;
>> +        output->head_info_list[i].link_state_flags =
>> +                            s->mhd_state->head_info_blocks[i].link_state_flags;
>> +    }
>> +
>> +    *len_out = sizeof(*output) + output->nr_heads * sizeof(MHDHeadInfoBlock);
>> +    return CXL_MBOX_SUCCESS;
>> +}
>
>>  static const Property cxl_mhsld_props[] = {
>> @@ -166,6 +214,47 @@ static void cxl_mhsld_state_initialize(CXLMHSLDState *s, size_t dc_size)
>>      s->mhd_state->nr_blocks = dc_size / MHSLD_BLOCK_SZ;
>>  }
>>
>> +
>> +static void cxl_mhsld_init_head_info(CXLMHSLDState *s, PCIDevice *pdev)
>> +{
>> +    uint16_t lnksta = 0;
>
>No need to initialize when they are always set below.

Sure, I’ll fix in V1 patch.

>
>> +    uint16_t current_link_speed = 0;
>> +    uint16_t negotiated_link_width = 0;
>> +    uint16_t lnkcap = 0, lnkcap2 = 0;
>> +    uint16_t max_link_width = 0;
>> +    uint16_t max_link_speed = 0;
>
>Once you drop the unnecessary init combine width and speed on one line.
>Or as below, get rid of most of these local variables entirely.
>
>> +    uint16_t supported_link_speeds_vector = 0;
>> +
>> +    lnksta = pdev->config_read(pdev,
>> +                               pdev->exp.exp_cap + PCI_EXP_LNKSTA,
>> +                               sizeof(lnksta));
>> +    lnkcap = pdev->config_read(pdev,
>> +                               pdev->exp.exp_cap + PCI_EXP_LNKCAP,
>> +                               sizeof(lnkcap));
>> +    lnkcap2 = pdev->config_read(pdev,
>> +                                pdev->exp.exp_cap + PCI_EXP_LNKCAP2,
>> +                                sizeof(lnkcap2));
>> +    supported_link_speeds_vector = (lnkcap2 & 0xFE) >> 1;
>Worth considering adding defines for that to incluw/hw/pci/pcie_regs.h

Okay. I’ll add in V1 patch.
>
>> +    max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
>
>Use PCI_EXP_LNK_MLW_SHIFT to extract that.
>(we should also tidy this up in physical port state.

Sure. I’ll fix in V1 patch.

>
>
>> +    max_link_speed = lnkcap & PCI_EXP_LNKCAP_SLS;
>> +    current_link_speed = lnksta & PCI_EXP_LNKSTA_CLS;
>> +    negotiated_link_width = (lnksta & PCI_EXP_LNKSTA_NLW) >> 4;
>Similar - there should be a suitable define for that shift.
>> +
>> +    s->mhd_state->head_info_blocks[s->mhd_head].port_number = s->mhd_head;
>
>I would use something like
>
>    s->mhd_state->head_info_blocks[s->mhd_head] = (MHDHeadInfoBlock) {
>        .max_link_width = lnkcap & PCI_EXP_LNKCAP_SLS,
>        .nego_link_width = (lnksta & PCI_EXP_LNKSTA_NLW) >> 4, //with the define
>        .supp_link_speeds_vector = (lnkcap2 & 0xFE) >> 1,
>etc
>    };

Sure, I’ll remove local variable and assign in this way.

>> +    s->mhd_state->head_info_blocks[s->mhd_head].max_link_width = max_link_width;
>> +    s->mhd_state->head_info_blocks[s->mhd_head].nego_link_width =
>> +                                                          negotiated_link_width;
>> +    s->mhd_state->head_info_blocks[s->mhd_head].supp_link_speeds_vector =
>> +                                                   supported_link_speeds_vector;
>> +    s->mhd_state->head_info_blocks[s->mhd_head].max_link_speed =
>> +                                                                 max_link_speed;
>> +    s->mhd_state->head_info_blocks[s->mhd_head].current_link_speed =
>> +                                                             current_link_speed;
>> +    s->mhd_state->head_info_blocks[s->mhd_head].ltssm_state = 0x7;
>> +    s->mhd_state->head_info_blocks[s->mhd_head].first_nego_lane_num = 0;
>> +    s->mhd_state->head_info_blocks[s->mhd_head].link_state_flags = 0;
>> +}
>> +
>>  /* Returns starting index of region in MHD map. */
>>  static inline size_t cxl_mhsld_find_dc_region_start(PCIDevice *d,
>>                                                      CXLDCRegion *r)
>> @@ -376,7 +465,7 @@ static void cxl_mhsld_realize(PCIDevice *pci_dev, Error **errp)
>>      }
>>
>>      cxl_mhsld_state_initialize(s, dc_size);
>> -
>> +    cxl_mhsld_init_head_info(s, pci_dev);
>Avoid the white space noise by leaving a blank line here.

Okay, Will fix in V1 patch

>
>>      /* Set the LD ownership for this head to this system */
>>      s->mhd_state->ldmap[s->mhd_head] = s->mhd_head;
>>      return;

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]