On Mon, 19 Feb 2024 23:00:23 +0800
<shiju.j...@huawei.com> wrote:
> From: Shiju Jose <shiju.j...@huawei.com>
>
> CXL spec 3.1 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 <d...@stgolabs.net>
> Reviewed-by: Fan Ni <fan...@samsung.com>
> Signed-off-by: Shiju Jose <shiju.j...@huawei.com>
Hi Shiju,
Sorry I've taken so long to get to this!
Looks good. One small comment inline.
Jonathan
> +
> +/* CXL r3.1 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);
I'm struggling a bit with the Specification text.
I says that count is
"Count in bytes of the supported Feature data to return in the
output payload."
I suppose that includes the header but it's not entirely clear
to me. Let's go with what we have here unless we get a spec
clarification.
> + req_entries = MIN(req_entries, CXL_FEATURE_MAX);
> + index = get_feats_in->start_index;
> +
> + entry = 0;
> + while (entry < req_entries) {
Given it's a known set of bounds a for loops should be
easier to read.
for (index = get_feats_in->start_index;
index < req_entries;
index++) {
switch (index) {
default:
__builtin_unreachable();
}
}
> + 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;
> +}
