| Message ID | 20250122235159.2716036-15-dave.jiang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: Add CXL feature commands support via fwctl | expand |
On Wed, 22 Jan 2025 16:50:45 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls > to a device. The cxl fwctl driver will start by supporting the CXL > feature commands: Get Supported Features, Get Feature, and Set Feature. > > The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where > it indicates the security scope of the call. The Get Supported Features > and Get Feature calls can be executed with the scope of > FWCTL_RPC_CONFIGRATION. The Set Feature call is gated by the effects > of the feature reported by Get Supported Features call for the specific > feature. > > Only "get supported features" is supported in this patch. Additional > commands will be added in follow on patches. "Get supported features" > will filter the features that are exclusive to the kernel and only > report out features that are not kernel only. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> A few comments inline. Jonathan > > diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c > index cc72e73ae8d6..e65fc8479d21 100644 > --- a/drivers/cxl/features.c > +++ b/drivers/cxl/features.c > +static struct cxl_mem_command * > +cxlctl_get_valid_hw_command(struct cxl_features_state *cfs, > + const struct fwctl_rpc_cxl *rpc_in, > + enum fwctl_rpc_scope scope) > +{ > + struct cxl_mem_command *cmd; > + > + if (!cfs->num_features) > + return ERR_PTR(-EOPNOTSUPP); > + > + cmd = cxl_find_feature_command_by_id(rpc_in->command_id); > + if (!cmd) > + return ERR_PTR(-ENOENT); > + > + if (!cxl_feature_enabled(cfs, cmd->opcode)) > + return ERR_PTR(-EPERM); > + > + switch (cmd->opcode) { > + case CXL_MBOX_OP_GET_SUPPORTED_FEATURES: > + case CXL_MBOX_OP_GET_FEATURE: > + if (scope >= FWCTL_RPC_CONFIGURATION) > + return cmd; > + break; > + case CXL_MBOX_OP_SET_FEATURE: > + if (cxlctl_validate_set_features(cfs, rpc_in, scope)) > + return cmd; > + break; return ERR_PTR(-EINVAL); here perhaps. > + default: > + return ERR_PTR(-EINVAL); > + } > + > + return ERR_PTR(-EINVAL); Maybe better to return in the error cases above to make it clear locally to them that they are errors. > +} > diff --git a/include/uapi/cxl/features.h b/include/uapi/cxl/features.h > index 18b74252058a..55fa5b2a5d17 100644 > --- a/include/uapi/cxl/features.h > +++ b/include/uapi/cxl/features.h > @@ -14,6 +14,19 @@ > #include <linux/uuid.h> > #endif > > +/* CXL spec r3.2 Table 8-87 command effects */ > +#define CXL_CMD_CONFIG_CHANGE_COLD_RESET BIT(0) These are duplicated with those in include/cxl/features. Can we avoid that? > +#define CXL_CMD_CONFIG_CHANGE_IMMEDIATE BIT(1) > +#define CXL_CMD_DATA_CHANGE_IMMEDIATE BIT(2) > +#define CXL_CMD_POLICY_CHANGE_IMMEDIATE BIT(3) > +#define CXL_CMD_LOG_CHANGE_IMMEDIATE BIT(4) > +#define CXL_CMD_SECURITY_STATE_CHANGE BIT(5) > +#define CXL_CMD_BACKGROUND BIT(6) > +#define CXL_CMD_BGCMD_ABORT_SUPPORTED BIT(7) > +#define CXL_CMD_EFFECTS_EXTEND BIT(9) > +#define CXL_CMD_CONFIG_CHANGE_CONV_RESET BIT(10) > +#define CXL_CMD_CONFIG_CHANGE_CXL_RESET BIT(11) > + > /* Get Supported Features (0x500h) CXL r3.1 8.2.9.6.1 */ > struct cxl_mbox_get_sup_feats_in { > __le32 count;
Dave Jiang wrote: > fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls > to a device. The cxl fwctl driver will start by supporting the CXL > feature commands: Get Supported Features, Get Feature, and Set Feature. > > The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where > it indicates the security scope of the call. The Get Supported Features > and Get Feature calls can be executed with the scope of > FWCTL_RPC_CONFIGRATION. The Set Feature call is gated by the effects > of the feature reported by Get Supported Features call for the specific > feature. > > Only "get supported features" is supported in this patch. Additional > commands will be added in follow on patches. "Get supported features" > will filter the features that are exclusive to the kernel and only > report out features that are not kernel only. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/mbox.c | 12 +++ > drivers/cxl/features.c | 198 +++++++++++++++++++++++++++++++++++- > include/cxl/features.h | 1 + > include/uapi/cxl/features.h | 13 +++ > include/uapi/fwctl/cxl.h | 29 ++++++ > 5 files changed, 250 insertions(+), 3 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 0b4946205910..f818e452dbca 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -246,6 +246,18 @@ struct cxl_mem_command *cxl_find_feature_command(u16 opcode) > } > EXPORT_SYMBOL_NS_GPL(cxl_find_feature_command, "CXL"); > > +struct cxl_mem_command *cxl_find_feature_command_by_id(u32 id) > +{ > + struct cxl_mem_command *c; > + > + cxl_for_each_feature_cmd(c) > + if (c->info.id == id) > + return c; > + > + return NULL; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_find_feature_command_by_id, "CXL"); I assume this will go with the deletion of a cxl_mem_command array for Features. Just do a simple hard-coded switch statement. > + > static const char *cxl_mem_opcode_to_name(u16 opcode) > { > struct cxl_mem_command *c; > diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c > index cc72e73ae8d6..e65fc8479d21 100644 > --- a/drivers/cxl/features.c > +++ b/drivers/cxl/features.c > @@ -50,11 +50,203 @@ static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length) > return info; > } > > +static struct cxl_feat_entry * > +get_support_feature_info(struct cxl_features_state *cfs, > + const struct fwctl_rpc_cxl *rpc_in) > +{ > + struct cxl_feat_entry *feat; > + uuid_t uuid; > + > + if (rpc_in->op_size < sizeof(uuid)) > + return ERR_PTR(-EINVAL); > + > + if (copy_from_user(&uuid, u64_to_user_ptr(rpc_in->in_payload), > + sizeof(uuid))) > + return ERR_PTR(-EFAULT); > + > + for (int i = 0; i < cfs->num_features; i++) { > + feat = &cfs->entries[i]; > + if (uuid_equal(&uuid, &feat->uuid)) > + return feat; > + } > + > + return ERR_PTR(-ENOENT); I expect user space to be surprised that it is getting "No such file or directory" after successfully opening the fwctl fd. Just use EINVAL for attempts to access Features that were not captured via the init-time Get Supported Features. > +} > + > +static void *cxlctl_get_supported_features(struct cxl_features_state *cfs, > + const struct fwctl_rpc_cxl *rpc_in, > + size_t *out_len) > +{ > + struct cxl_mbox_get_sup_feats_out *feat_out; > + struct cxl_mbox_get_sup_feats_in feat_in; > + struct cxl_feat_entry *saved, *pos; > + int requested, copied; > + size_t out_size; > + u32 count; > + u16 start; > + > + if (rpc_in->op_size != sizeof(feat_in)) > + return ERR_PTR(-EINVAL); > + > + if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload), > + rpc_in->op_size)) > + return ERR_PTR(-EFAULT); > + > + count = le32_to_cpu(feat_in.count); > + start = le16_to_cpu(feat_in.start_idx); > + requested = count / sizeof(*pos); > + > + /* > + * Make sure that the total requested number of entries is not greater > + * than the total number of supported features allowed for userspace. > + */ > + if (start >= cfs->num_user_features) > + return ERR_PTR(-EINVAL); > + > + requested = min_t(int, requested, cfs->num_user_features - start); > + > + out_size = sizeof(struct fwctl_rpc_cxl_out) + sizeof(*feat_out) + > + requested * sizeof(*pos); > + > + struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) = > + kvzalloc(out_size, GFP_KERNEL); > + if (!rpc_out) > + return ERR_PTR(-ENOMEM); > + > + rpc_out->size = sizeof(*feat_out) + requested * sizeof(*pos); > + feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload; > + if (requested == 0) { > + feat_out->num_entries = cpu_to_le16(requested); > + feat_out->supported_feats = cpu_to_le16(cfs->num_user_features); > + rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS; > + *out_len = out_size; > + return no_free_ptr(rpc_out); > + } > + > + pos = &feat_out->ents[0]; > + saved = &cfs->entries[0]; > + > + copied = 0; > + for (int i = 0; i < cfs->num_features; i++, saved++) { > + if (is_cxl_feature_exclusive(saved)) > + continue; I think it's fine to let userspace see that exclusive features are present, just need to return EBUSY if userspace actually tries to use them. > + > + memcpy(pos, saved, sizeof(*saved)); > + copied++; > + if (copied == requested) > + break; > + pos++; > + } > + > + feat_out->num_entries = cpu_to_le16(requested); > + feat_out->supported_feats = cpu_to_le16(cfs->num_user_features); > + rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS; > + *out_len = out_size; > + > + return no_free_ptr(rpc_out); > +} > + > +static bool cxlctl_validate_set_features(struct cxl_features_state *cfs, > + const struct fwctl_rpc_cxl *rpc_in, > + enum fwctl_rpc_scope scope) > +{ > + struct cxl_feat_entry *feat; > + u16 effects, mask; > + u32 flags; > + > + feat = get_support_feature_info(cfs, rpc_in); > + if (IS_ERR(feat)) > + return false; > + > + /* Ensure that the attribute is changeable */ > + flags = le32_to_cpu(feat->flags); > + if (!(flags & CXL_FEATURE_F_CHANGEABLE)) > + return false; > + > + effects = le16_to_cpu(feat->effects); > + /* Currently no user background command support */ > + if (effects & CXL_CMD_BACKGROUND) > + return false; > + > + mask = CXL_CMD_CONFIG_CHANGE_IMMEDIATE | > + CXL_CMD_DATA_CHANGE_IMMEDIATE | > + CXL_CMD_POLICY_CHANGE_IMMEDIATE | > + CXL_CMD_LOG_CHANGE_IMMEDIATE; > + if (effects & mask && scope >= FWCTL_RPC_DEBUG_WRITE_FULL) > + return true; > + > + /* These effects supported for all scope */ > + if ((effects & CXL_CMD_CONFIG_CHANGE_COLD_RESET || > + (effects & CXL_CMD_EFFECTS_EXTEND && > + (effects & CXL_CMD_CONFIG_CHANGE_CONV_RESET || > + effects & CXL_CMD_CONFIG_CHANGE_CXL_RESET))) && > + scope >= FWCTL_RPC_DEBUG_WRITE) > + return true; Looks good for the known bits, but this needs to return false for the currently reserved bits because the driver can not assume a security model for future effects. If a future spec adds FWCTL_RPC_DEBUG_WRITE-safe effects, a new kernel is needed to allow those Feature commands through. Sidenote: I wonder why the spec wasted one of its bits on an extend bit, but here we are. The 'extend' concept is typically something like "bit15: go look at this other field in this payload as this 16-bit field was exhausted", not "bit9: the bits above this originally defined 16 bit field now has more bits", oh well. > + > + return false; > +} > + > +static struct cxl_mem_command * > +cxlctl_get_valid_hw_command(struct cxl_features_state *cfs, > + const struct fwctl_rpc_cxl *rpc_in, > + enum fwctl_rpc_scope scope) > +{ > + struct cxl_mem_command *cmd; > + > + if (!cfs->num_features) > + return ERR_PTR(-EOPNOTSUPP); > + > + cmd = cxl_find_feature_command_by_id(rpc_in->command_id); > + if (!cmd) > + return ERR_PTR(-ENOENT); > + > + if (!cxl_feature_enabled(cfs, cmd->opcode)) > + return ERR_PTR(-EPERM); Why "Permission denied", because the base command is missing? The cxl_fwctl interface would not even be availble to userspace if the above was true. No need to worry about optional Set Feature becuase that should be consistent with the Set Feature Size data in Get Supported Features. > + Per above, need to add EBUSY for exclusive features in this path. > + switch (cmd->opcode) { > + case CXL_MBOX_OP_GET_SUPPORTED_FEATURES: > + case CXL_MBOX_OP_GET_FEATURE: I would still check if a Get Feature has a configuration change side-effect. You never know if someone purposefully or accidentally introduces "read causes side effects" commands. [..]
> > +} > > + > > +static void *cxlctl_get_supported_features(struct cxl_features_state *cfs, > > + const struct fwctl_rpc_cxl *rpc_in, > > + size_t *out_len) > > +{ > > + struct cxl_mbox_get_sup_feats_out *feat_out; > > + struct cxl_mbox_get_sup_feats_in feat_in; > > + struct cxl_feat_entry *saved, *pos; > > + int requested, copied; > > + size_t out_size; > > + u32 count; > > + u16 start; > > + > > + if (rpc_in->op_size != sizeof(feat_in)) > > + return ERR_PTR(-EINVAL); > > + > > + if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload), > > + rpc_in->op_size)) > > + return ERR_PTR(-EFAULT); > > + > > + count = le32_to_cpu(feat_in.count); > > + start = le16_to_cpu(feat_in.start_idx); > > + requested = count / sizeof(*pos); > > + > > + /* > > + * Make sure that the total requested number of entries is not greater > > + * than the total number of supported features allowed for userspace. > > + */ > > + if (start >= cfs->num_user_features) > > + return ERR_PTR(-EINVAL); > > + > > + requested = min_t(int, requested, cfs->num_user_features - start); > > + > > + out_size = sizeof(struct fwctl_rpc_cxl_out) + sizeof(*feat_out) + > > + requested * sizeof(*pos); > > + > > + struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) = > > + kvzalloc(out_size, GFP_KERNEL); > > + if (!rpc_out) > > + return ERR_PTR(-ENOMEM); > > + > > + rpc_out->size = sizeof(*feat_out) + requested * sizeof(*pos); > > + feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload; > > + if (requested == 0) { > > + feat_out->num_entries = cpu_to_le16(requested); > > + feat_out->supported_feats = cpu_to_le16(cfs->num_user_features); > > + rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS; > > + *out_len = out_size; > > + return no_free_ptr(rpc_out); > > + } > > + > > + pos = &feat_out->ents[0]; > > + saved = &cfs->entries[0]; > > + > > + copied = 0; > > + for (int i = 0; i < cfs->num_features; i++, saved++) { > > + if (is_cxl_feature_exclusive(saved)) > > + continue; > > I think it's fine to let userspace see that exclusive features are > present, just need to return EBUSY if userspace actually tries to use > them. To me, a poke it and see interface is really ugly. In many cases we could let "get" through even if the we are using the interface via some other kernel path and have it as exclusive. (I don't know how useful that is, but maybe it makes sense). If we ever do that, the only way to discover if an interface is available will be to try the set interface. Depending on design of feature that might have side effects - hopefully get never does! Alternatives: 1. Flag. Maybe add something that makes it discoverable if a feature is in exclusive mode or not. 2. Query type interface. So a way to actually ask if a given feature is usable. 3. What we have here. To me the simplest solution is hide what we can't be used. > > + /* These effects supported for all scope */ > > + if ((effects & CXL_CMD_CONFIG_CHANGE_COLD_RESET || > > + (effects & CXL_CMD_EFFECTS_EXTEND && > > + (effects & CXL_CMD_CONFIG_CHANGE_CONV_RESET || > > + effects & CXL_CMD_CONFIG_CHANGE_CXL_RESET))) && > > + scope >= FWCTL_RPC_DEBUG_WRITE) > > + return true; > > Looks good for the known bits, but this needs to return false for the > currently reserved bits because the driver can not assume a security > model for future effects. If a future spec adds > FWCTL_RPC_DEBUG_WRITE-safe effects, a new kernel is needed to allow > those Feature commands through. > > Sidenote: I wonder why the spec wasted one of its bits on an extend bit, > but here we are. The 'extend' concept is typically something like > "bit15: go look at this other field in this payload as this 16-bit field > was exhausted", not "bit9: the bits above this originally defined 16 bit > field now has more bits", oh well. It's odd but corner case of going from 'unknown' state for the remaining pair of bits to 0 means this and 1 means this. Naming though doesn't match the spec that calls it CEL[11:10] valid. Would be good to name it closer to that as we may well have something in bits 12 and 15 in future and it doesn't refer to them. Jonathan
Jonathan Cameron wrote: > > > > +} > > > + > > > +static void *cxlctl_get_supported_features(struct cxl_features_state *cfs, > > > + const struct fwctl_rpc_cxl *rpc_in, > > > + size_t *out_len) > > > +{ > > > + struct cxl_mbox_get_sup_feats_out *feat_out; > > > + struct cxl_mbox_get_sup_feats_in feat_in; > > > + struct cxl_feat_entry *saved, *pos; > > > + int requested, copied; > > > + size_t out_size; > > > + u32 count; > > > + u16 start; > > > + > > > + if (rpc_in->op_size != sizeof(feat_in)) > > > + return ERR_PTR(-EINVAL); > > > + > > > + if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload), > > > + rpc_in->op_size)) > > > + return ERR_PTR(-EFAULT); > > > + > > > + count = le32_to_cpu(feat_in.count); > > > + start = le16_to_cpu(feat_in.start_idx); > > > + requested = count / sizeof(*pos); > > > + > > > + /* > > > + * Make sure that the total requested number of entries is not greater > > > + * than the total number of supported features allowed for userspace. > > > + */ > > > + if (start >= cfs->num_user_features) > > > + return ERR_PTR(-EINVAL); > > > + > > > + requested = min_t(int, requested, cfs->num_user_features - start); > > > + > > > + out_size = sizeof(struct fwctl_rpc_cxl_out) + sizeof(*feat_out) + > > > + requested * sizeof(*pos); > > > + > > > + struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) = > > > + kvzalloc(out_size, GFP_KERNEL); > > > + if (!rpc_out) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + rpc_out->size = sizeof(*feat_out) + requested * sizeof(*pos); > > > + feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload; > > > + if (requested == 0) { > > > + feat_out->num_entries = cpu_to_le16(requested); > > > + feat_out->supported_feats = cpu_to_le16(cfs->num_user_features); > > > + rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS; > > > + *out_len = out_size; > > > + return no_free_ptr(rpc_out); > > > + } > > > + > > > + pos = &feat_out->ents[0]; > > > + saved = &cfs->entries[0]; > > > + > > > + copied = 0; > > > + for (int i = 0; i < cfs->num_features; i++, saved++) { > > > + if (is_cxl_feature_exclusive(saved)) > > > + continue; > > > > I think it's fine to let userspace see that exclusive features are > > present, just need to return EBUSY if userspace actually tries to use > > them. > > To me, a poke it and see interface is really ugly. That smells more like a matter of documentation. "Doctor it hurts when I try to use the documented kernel-exclusive commands?" > In many cases we could let "get" through even if the we are using the interface > via some other kernel path and have it as exclusive. > (I don't know how useful that is, but maybe it makes sense). > > If we ever do that, the only way to discover if an interface is available > will be to try the set interface. Depending on design of feature > that might have side effects - hopefully get never does! I would not put it past some future device to make that mistake. > > Alternatives: > 1. Flag. Maybe add something that makes it discoverable if a feature is > in exclusive mode or not. I notice that all existing defined Features set a non-zero "Get Feature Size" in their Supported Feature Entry. I would not say "no" to just zero-ing out Get Feature Size as a hint that "you might get EBUSY due to kernel exclusivity with this command", but that still feels like overkill compared to documentation. > 2. Query type interface. So a way to actually ask if a given feature is > usable. Not sure we really need a programmatic way to read the documentation. The CXL_MEM_COMMAND_FLAG_EXCLUSIVE flag is for cases where the exclusivity is transient. For these features the exclusivity is permanent, and I hope we never need to cross that transient-exclusivity-bridge for Features. > 3. What we have here. To me the simplest solution is hide what we can't > be used. It is inconsistent that we do not do this for the other kernel exclusive commands in userspace retreived Command Effects Log. The ABI here is raw Get Supported Features payload. > > > + /* These effects supported for all scope */ > > > + if ((effects & CXL_CMD_CONFIG_CHANGE_COLD_RESET || > > > + (effects & CXL_CMD_EFFECTS_EXTEND && > > > + (effects & CXL_CMD_CONFIG_CHANGE_CONV_RESET || > > > + effects & CXL_CMD_CONFIG_CHANGE_CXL_RESET))) && > > > + scope >= FWCTL_RPC_DEBUG_WRITE) > > > + return true; > > > > Looks good for the known bits, but this needs to return false for the > > currently reserved bits because the driver can not assume a security > > model for future effects. If a future spec adds > > FWCTL_RPC_DEBUG_WRITE-safe effects, a new kernel is needed to allow > > those Feature commands through. > > > > Sidenote: I wonder why the spec wasted one of its bits on an extend bit, > > but here we are. The 'extend' concept is typically something like > > "bit15: go look at this other field in this payload as this 16-bit field > > was exhausted", not "bit9: the bits above this originally defined 16 bit > > field now has more bits", oh well. > > It's odd but corner case of going from 'unknown' state for the remaining > pair of bits to 0 means this and 1 means this. I don't understand. 0 means no effect to worry about whether it is defined or not. > Naming though doesn't match the spec that calls it CEL[11:10] valid. > Would be good to name it closer to that as we may well have something > in bits 12 and 15 in future and it doesn't refer to them. Hopefully we can head off another "valid2" mistake, and I don't think Linux needs to define anything for this bit. That bit's definition is: "Bit[9]: 1 is recommended, 0 is permitted (CEL[11:10] Valid)" ...which translates to "useless". If 11 or 10 are set, I don't care what value 9 has. If 12:15 are set, I don't care if there is a future valid2 bit gating whether or not to use them. Valid bits are for cases that go outside of what Reserved 0 compatibility rules can convey, and I think Reserved 0 compatiblity fully covers us in this case. So, if a device use case breaks because they set 10, but clear 9 and expect software to ignore 10 then they get to keep all the pieces because they have already broken the expectations of Reserved 0 compat-software created before 9 existed.
On Mon, 27 Jan 2025 16:40:48 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > Jonathan Cameron wrote: > > > > > > +} > > > > + > > > > +static void *cxlctl_get_supported_features(struct cxl_features_state *cfs, > > > > + const struct fwctl_rpc_cxl *rpc_in, > > > > + size_t *out_len) > > > > +{ > > > > + struct cxl_mbox_get_sup_feats_out *feat_out; > > > > + struct cxl_mbox_get_sup_feats_in feat_in; > > > > + struct cxl_feat_entry *saved, *pos; > > > > + int requested, copied; > > > > + size_t out_size; > > > > + u32 count; > > > > + u16 start; > > > > + > > > > + if (rpc_in->op_size != sizeof(feat_in)) > > > > + return ERR_PTR(-EINVAL); > > > > + > > > > + if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload), > > > > + rpc_in->op_size)) > > > > + return ERR_PTR(-EFAULT); > > > > + > > > > + count = le32_to_cpu(feat_in.count); > > > > + start = le16_to_cpu(feat_in.start_idx); > > > > + requested = count / sizeof(*pos); > > > > + > > > > + /* > > > > + * Make sure that the total requested number of entries is not greater > > > > + * than the total number of supported features allowed for userspace. > > > > + */ > > > > + if (start >= cfs->num_user_features) > > > > + return ERR_PTR(-EINVAL); > > > > + > > > > + requested = min_t(int, requested, cfs->num_user_features - start); > > > > + > > > > + out_size = sizeof(struct fwctl_rpc_cxl_out) + sizeof(*feat_out) + > > > > + requested * sizeof(*pos); > > > > + > > > > + struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) = > > > > + kvzalloc(out_size, GFP_KERNEL); > > > > + if (!rpc_out) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + rpc_out->size = sizeof(*feat_out) + requested * sizeof(*pos); > > > > + feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload; > > > > + if (requested == 0) { > > > > + feat_out->num_entries = cpu_to_le16(requested); > > > > + feat_out->supported_feats = cpu_to_le16(cfs->num_user_features); > > > > + rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS; > > > > + *out_len = out_size; > > > > + return no_free_ptr(rpc_out); > > > > + } > > > > + > > > > + pos = &feat_out->ents[0]; > > > > + saved = &cfs->entries[0]; > > > > + > > > > + copied = 0; > > > > + for (int i = 0; i < cfs->num_features; i++, saved++) { > > > > + if (is_cxl_feature_exclusive(saved)) > > > > + continue; > > > > > > I think it's fine to let userspace see that exclusive features are > > > present, just need to return EBUSY if userspace actually tries to use > > > them. > > > > To me, a poke it and see interface is really ugly. > > That smells more like a matter of documentation. "Doctor it hurts when I > try to use the documented kernel-exclusive commands?" To me this is a nasty interface design. If I'm writing a tool to enumerate what is exposed etc then it will have to poke every get command just to list if an interface is available. Hopefully none of them have side effects! > > > In many cases we could let "get" through even if the we are using the interface > > via some other kernel path and have it as exclusive. > > (I don't know how useful that is, but maybe it makes sense). > > > > If we ever do that, the only way to discover if an interface is available > > will be to try the set interface. Depending on design of feature > > that might have side effects - hopefully get never does! > > I would not put it past some future device to make that mistake. True. Though I'd be up for a quirk list to block such commands if we see them. Can't deal with them until we know though. > > > > > Alternatives: > > 1. Flag. Maybe add something that makes it discoverable if a feature is > > in exclusive mode or not. > > I notice that all existing defined Features set a non-zero "Get Feature > Size" in their Supported Feature Entry. I would not say "no" to just > zero-ing out Get Feature Size as a hint that "you might get EBUSY due to > kernel exclusivity with this command", but that still feels like > overkill compared to documentation. 'might' is no use. Would have to be definitely as otherwise userspace can't know the size that field conveyed. > > > 2. Query type interface. So a way to actually ask if a given feature is > > usable. > > Not sure we really need a programmatic way to read the documentation. > > The CXL_MEM_COMMAND_FLAG_EXCLUSIVE flag is for cases where the > exclusivity is transient. For these features the exclusivity is > permanent, and I hope we never need to cross that > transient-exclusivity-bridge for Features. It's permanent today, but I can definitely see that not always being the case - we may well have future kernel does things in X fashion but for legacy support disable that CONFIG option. Not nice but definitely plausible. > > > 3. What we have here. To me the simplest solution is hide what we can't > > be used. > > It is inconsistent that we do not do this for the other kernel exclusive > commands in userspace retreived Command Effects Log. The ABI here is raw > Get Supported Features payload. If they were exposed via similar paths I'd agree consistency matters but I 'hope' no one is going to have a tool that mixes fwctl and the legacy path. In my head we add all the useful commands to fwctl and that legacy path ends up effectively deprecated. Anyhow, I don't feel that strongly about this, it's just a case of doesn't smell of roses to me. > > > > > + /* These effects supported for all scope */ > > > > + if ((effects & CXL_CMD_CONFIG_CHANGE_COLD_RESET || > > > > + (effects & CXL_CMD_EFFECTS_EXTEND && > > > > + (effects & CXL_CMD_CONFIG_CHANGE_CONV_RESET || > > > > + effects & CXL_CMD_CONFIG_CHANGE_CXL_RESET))) && > > > > + scope >= FWCTL_RPC_DEBUG_WRITE) > > > > + return true; > > > > > > Looks good for the known bits, but this needs to return false for the > > > currently reserved bits because the driver can not assume a security > > > model for future effects. If a future spec adds > > > FWCTL_RPC_DEBUG_WRITE-safe effects, a new kernel is needed to allow > > > those Feature commands through. > > > > > > Sidenote: I wonder why the spec wasted one of its bits on an extend bit, > > > but here we are. The 'extend' concept is typically something like > > > "bit15: go look at this other field in this payload as this 16-bit field > > > was exhausted", not "bit9: the bits above this originally defined 16 bit > > > field now has more bits", oh well. > > > > It's odd but corner case of going from 'unknown' state for the remaining > > pair of bits to 0 means this and 1 means this. > > I don't understand. 0 means no effect to worry about whether it is > defined or not. > > > Naming though doesn't match the spec that calls it CEL[11:10] valid. > > Would be good to name it closer to that as we may well have something > > in bits 12 and 15 in future and it doesn't refer to them. > > Hopefully we can head off another "valid2" mistake, and I don't think > Linux needs to define anything for this bit. That bit's definition is: > > "Bit[9]: 1 is recommended, 0 is permitted (CEL[11:10] Valid)" > > ...which translates to "useless". If 11 or 10 are set, I don't care what > value 9 has. > > If 12:15 are set, I don't care if there is a future valid2 > bit gating whether or not to use them. Valid bits are for cases that go > outside of what Reserved 0 compatibility rules can convey, and I think > Reserved 0 compatiblity fully covers us in this case. Seems the spec authors disagreed. (obviously I can't comment on that discussion). Using just what anyone can see (if they have the spec) It was a clear spec hole and there wasn't an obvious default for 0 to mean so it was a 'read your device docs and act appropriately' case before this stuff was added. There may be corner cases where the right answer if we know the feature is not persisted over a reset but instead panic or take some heavy weight action. Same can be true the other way around in that we may have to do something heavy to manually reset something we don't want to persist over reset. Hopefully not but we'll see. > > So, if a device use case breaks because they set 10, but clear 9 and > expect software to ignore 10 then they get to keep all the pieces > because they have already broken the expectations of Reserved 0 > compat-software created before 9 existed. True, but a compliance test should have dealt with that (seems they are yet to catch up with this though). Jonathan
On 1/28/25 5:01 AM, Jonathan Cameron wrote: > On Mon, 27 Jan 2025 16:40:48 -0800 > Dan Williams <dan.j.williams@intel.com> wrote: > >> Jonathan Cameron wrote: >>> >>>>> +} >>>>> + >>>>> +static void *cxlctl_get_supported_features(struct cxl_features_state *cfs, >>>>> + const struct fwctl_rpc_cxl *rpc_in, >>>>> + size_t *out_len) >>>>> +{ >>>>> + struct cxl_mbox_get_sup_feats_out *feat_out; >>>>> + struct cxl_mbox_get_sup_feats_in feat_in; >>>>> + struct cxl_feat_entry *saved, *pos; >>>>> + int requested, copied; >>>>> + size_t out_size; >>>>> + u32 count; >>>>> + u16 start; >>>>> + >>>>> + if (rpc_in->op_size != sizeof(feat_in)) >>>>> + return ERR_PTR(-EINVAL); >>>>> + >>>>> + if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload), >>>>> + rpc_in->op_size)) >>>>> + return ERR_PTR(-EFAULT); >>>>> + >>>>> + count = le32_to_cpu(feat_in.count); >>>>> + start = le16_to_cpu(feat_in.start_idx); >>>>> + requested = count / sizeof(*pos); >>>>> + >>>>> + /* >>>>> + * Make sure that the total requested number of entries is not greater >>>>> + * than the total number of supported features allowed for userspace. >>>>> + */ >>>>> + if (start >= cfs->num_user_features) >>>>> + return ERR_PTR(-EINVAL); >>>>> + >>>>> + requested = min_t(int, requested, cfs->num_user_features - start); >>>>> + >>>>> + out_size = sizeof(struct fwctl_rpc_cxl_out) + sizeof(*feat_out) + >>>>> + requested * sizeof(*pos); >>>>> + >>>>> + struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) = >>>>> + kvzalloc(out_size, GFP_KERNEL); >>>>> + if (!rpc_out) >>>>> + return ERR_PTR(-ENOMEM); >>>>> + >>>>> + rpc_out->size = sizeof(*feat_out) + requested * sizeof(*pos); >>>>> + feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload; >>>>> + if (requested == 0) { >>>>> + feat_out->num_entries = cpu_to_le16(requested); >>>>> + feat_out->supported_feats = cpu_to_le16(cfs->num_user_features); >>>>> + rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS; >>>>> + *out_len = out_size; >>>>> + return no_free_ptr(rpc_out); >>>>> + } >>>>> + >>>>> + pos = &feat_out->ents[0]; >>>>> + saved = &cfs->entries[0]; >>>>> + >>>>> + copied = 0; >>>>> + for (int i = 0; i < cfs->num_features; i++, saved++) { >>>>> + if (is_cxl_feature_exclusive(saved)) >>>>> + continue; >>>> >>>> I think it's fine to let userspace see that exclusive features are >>>> present, just need to return EBUSY if userspace actually tries to use >>>> them. >>> >>> To me, a poke it and see interface is really ugly. >> >> That smells more like a matter of documentation. "Doctor it hurts when I >> try to use the documented kernel-exclusive commands?" > > To me this is a nasty interface design. > If I'm writing a tool to enumerate what is exposed etc then it will > have to poke every get command just to list if an interface is available. > Hopefully none of them have side effects! > >> >>> In many cases we could let "get" through even if the we are using the interface >>> via some other kernel path and have it as exclusive. >>> (I don't know how useful that is, but maybe it makes sense). >>> >>> If we ever do that, the only way to discover if an interface is available >>> will be to try the set interface. Depending on design of feature >>> that might have side effects - hopefully get never does! >> >> I would not put it past some future device to make that mistake. > > True. Though I'd be up for a quirk list to block such commands if we > see them. Can't deal with them until we know though. > >> >>> >>> Alternatives: >>> 1. Flag. Maybe add something that makes it discoverable if a feature is >>> in exclusive mode or not. >> >> I notice that all existing defined Features set a non-zero "Get Feature >> Size" in their Supported Feature Entry. I would not say "no" to just >> zero-ing out Get Feature Size as a hint that "you might get EBUSY due to >> kernel exclusivity with this command", but that still feels like >> overkill compared to documentation. > > 'might' is no use. Would have to be definitely as otherwise userspace > can't know the size that field conveyed. We could zero the "Set Feature Size" in the "Get Supported Features Supported Feature Entry". To indicate that the Feature cannot be changed. That would be spec compliant. Or is there concern that reading the feature may do something for some devices decide to be weird? DJ > >> >>> 2. Query type interface. So a way to actually ask if a given feature is >>> usable. >> >> Not sure we really need a programmatic way to read the documentation. >> >> The CXL_MEM_COMMAND_FLAG_EXCLUSIVE flag is for cases where the >> exclusivity is transient. For these features the exclusivity is >> permanent, and I hope we never need to cross that >> transient-exclusivity-bridge for Features. > > It's permanent today, but I can definitely see that not always being > the case - we may well have future kernel does things in X fashion but > for legacy support disable that CONFIG option. Not nice but definitely > plausible. > >> >>> 3. What we have here. To me the simplest solution is hide what we can't >>> be used. >> >> It is inconsistent that we do not do this for the other kernel exclusive >> commands in userspace retreived Command Effects Log. The ABI here is raw >> Get Supported Features payload. > > If they were exposed via similar paths I'd agree consistency matters > but I 'hope' no one is going to have a tool that mixes fwctl and the > legacy path. In my head we add all the useful commands to fwctl > and that legacy path ends up effectively deprecated. > > Anyhow, I don't feel that strongly about this, it's just a case > of doesn't smell of roses to me. > >> >>>>> + /* These effects supported for all scope */ >>>>> + if ((effects & CXL_CMD_CONFIG_CHANGE_COLD_RESET || >>>>> + (effects & CXL_CMD_EFFECTS_EXTEND && >>>>> + (effects & CXL_CMD_CONFIG_CHANGE_CONV_RESET || >>>>> + effects & CXL_CMD_CONFIG_CHANGE_CXL_RESET))) && >>>>> + scope >= FWCTL_RPC_DEBUG_WRITE) >>>>> + return true; >>>> >>>> Looks good for the known bits, but this needs to return false for the >>>> currently reserved bits because the driver can not assume a security >>>> model for future effects. If a future spec adds >>>> FWCTL_RPC_DEBUG_WRITE-safe effects, a new kernel is needed to allow >>>> those Feature commands through. >>>> >>>> Sidenote: I wonder why the spec wasted one of its bits on an extend bit, >>>> but here we are. The 'extend' concept is typically something like >>>> "bit15: go look at this other field in this payload as this 16-bit field >>>> was exhausted", not "bit9: the bits above this originally defined 16 bit >>>> field now has more bits", oh well. >>> >>> It's odd but corner case of going from 'unknown' state for the remaining >>> pair of bits to 0 means this and 1 means this. >> >> I don't understand. 0 means no effect to worry about whether it is >> defined or not. >> >>> Naming though doesn't match the spec that calls it CEL[11:10] valid. >>> Would be good to name it closer to that as we may well have something >>> in bits 12 and 15 in future and it doesn't refer to them. >> >> Hopefully we can head off another "valid2" mistake, and I don't think >> Linux needs to define anything for this bit. That bit's definition is: >> >> "Bit[9]: 1 is recommended, 0 is permitted (CEL[11:10] Valid)" >> >> ...which translates to "useless". If 11 or 10 are set, I don't care what >> value 9 has. >> >> If 12:15 are set, I don't care if there is a future valid2 >> bit gating whether or not to use them. Valid bits are for cases that go >> outside of what Reserved 0 compatibility rules can convey, and I think >> Reserved 0 compatiblity fully covers us in this case. > > Seems the spec authors disagreed. (obviously I can't comment on that > discussion). > > Using just what anyone can see (if they have the spec) > It was a clear spec hole and there wasn't an obvious default for 0 to > mean so it was a 'read your device docs and act appropriately' case > before this stuff was added. > > There may be corner cases where the right answer if we know the > feature is not persisted over a reset but instead panic or take > some heavy weight action. Same can be true the other way around > in that we may have to do something heavy to manually reset something > we don't want to persist over reset. Hopefully not but we'll see. > >> >> So, if a device use case breaks because they set 10, but clear 9 and >> expect software to ignore 10 then they get to keep all the pieces >> because they have already broken the expectations of Reserved 0 >> compat-software created before 9 existed. > > True, but a compliance test should have dealt with that (seems they > are yet to catch up with this though). > > Jonathan >
On Tue, 28 Jan 2025 08:55:57 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > On 1/28/25 5:01 AM, Jonathan Cameron wrote: > > On Mon, 27 Jan 2025 16:40:48 -0800 > > Dan Williams <dan.j.williams@intel.com> wrote: > > > >> Jonathan Cameron wrote: > >>> > >>>>> +} > >>>>> + > >>>>> +static void *cxlctl_get_supported_features(struct cxl_features_state *cfs, > >>>>> + const struct fwctl_rpc_cxl *rpc_in, > >>>>> + size_t *out_len) > >>>>> +{ > >>>>> + struct cxl_mbox_get_sup_feats_out *feat_out; > >>>>> + struct cxl_mbox_get_sup_feats_in feat_in; > >>>>> + struct cxl_feat_entry *saved, *pos; > >>>>> + int requested, copied; > >>>>> + size_t out_size; > >>>>> + u32 count; > >>>>> + u16 start; > >>>>> + > >>>>> + if (rpc_in->op_size != sizeof(feat_in)) > >>>>> + return ERR_PTR(-EINVAL); > >>>>> + > >>>>> + if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload), > >>>>> + rpc_in->op_size)) > >>>>> + return ERR_PTR(-EFAULT); > >>>>> + > >>>>> + count = le32_to_cpu(feat_in.count); > >>>>> + start = le16_to_cpu(feat_in.start_idx); > >>>>> + requested = count / sizeof(*pos); > >>>>> + > >>>>> + /* > >>>>> + * Make sure that the total requested number of entries is not greater > >>>>> + * than the total number of supported features allowed for userspace. > >>>>> + */ > >>>>> + if (start >= cfs->num_user_features) > >>>>> + return ERR_PTR(-EINVAL); > >>>>> + > >>>>> + requested = min_t(int, requested, cfs->num_user_features - start); > >>>>> + > >>>>> + out_size = sizeof(struct fwctl_rpc_cxl_out) + sizeof(*feat_out) + > >>>>> + requested * sizeof(*pos); > >>>>> + > >>>>> + struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) = > >>>>> + kvzalloc(out_size, GFP_KERNEL); > >>>>> + if (!rpc_out) > >>>>> + return ERR_PTR(-ENOMEM); > >>>>> + > >>>>> + rpc_out->size = sizeof(*feat_out) + requested * sizeof(*pos); > >>>>> + feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload; > >>>>> + if (requested == 0) { > >>>>> + feat_out->num_entries = cpu_to_le16(requested); > >>>>> + feat_out->supported_feats = cpu_to_le16(cfs->num_user_features); > >>>>> + rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS; > >>>>> + *out_len = out_size; > >>>>> + return no_free_ptr(rpc_out); > >>>>> + } > >>>>> + > >>>>> + pos = &feat_out->ents[0]; > >>>>> + saved = &cfs->entries[0]; > >>>>> + > >>>>> + copied = 0; > >>>>> + for (int i = 0; i < cfs->num_features; i++, saved++) { > >>>>> + if (is_cxl_feature_exclusive(saved)) > >>>>> + continue; > >>>> > >>>> I think it's fine to let userspace see that exclusive features are > >>>> present, just need to return EBUSY if userspace actually tries to use > >>>> them. > >>> > >>> To me, a poke it and see interface is really ugly. > >> > >> That smells more like a matter of documentation. "Doctor it hurts when I > >> try to use the documented kernel-exclusive commands?" > > > > To me this is a nasty interface design. > > If I'm writing a tool to enumerate what is exposed etc then it will > > have to poke every get command just to list if an interface is available. > > Hopefully none of them have side effects! > > > >> > >>> In many cases we could let "get" through even if the we are using the interface > >>> via some other kernel path and have it as exclusive. > >>> (I don't know how useful that is, but maybe it makes sense). > >>> > >>> If we ever do that, the only way to discover if an interface is available > >>> will be to try the set interface. Depending on design of feature > >>> that might have side effects - hopefully get never does! > >> > >> I would not put it past some future device to make that mistake. > > > > True. Though I'd be up for a quirk list to block such commands if we > > see them. Can't deal with them until we know though. > > > >> > >>> > >>> Alternatives: > >>> 1. Flag. Maybe add something that makes it discoverable if a feature is > >>> in exclusive mode or not. > >> > >> I notice that all existing defined Features set a non-zero "Get Feature > >> Size" in their Supported Feature Entry. I would not say "no" to just > >> zero-ing out Get Feature Size as a hint that "you might get EBUSY due to > >> kernel exclusivity with this command", but that still feels like > >> overkill compared to documentation. > > > > 'might' is no use. Would have to be definitely as otherwise userspace > > can't know the size that field conveyed. > > We could zero the "Set Feature Size" in the "Get Supported Features Supported Feature Entry". To indicate that the Feature cannot be changed. That would be spec compliant. Or is there concern that reading the feature may do something for some devices decide to be weird? Let's hope not. I'm fine with this suggestion. Seems like a reasonable compromise. Jonathan > > DJ > > > > > >> > >>> 2. Query type interface. So a way to actually ask if a given feature is > >>> usable. > >> > >> Not sure we really need a programmatic way to read the documentation. > >> > >> The CXL_MEM_COMMAND_FLAG_EXCLUSIVE flag is for cases where the > >> exclusivity is transient. For these features the exclusivity is > >> permanent, and I hope we never need to cross that > >> transient-exclusivity-bridge for Features. > > > > It's permanent today, but I can definitely see that not always being > > the case - we may well have future kernel does things in X fashion but > > for legacy support disable that CONFIG option. Not nice but definitely > > plausible. > > > >> > >>> 3. What we have here. To me the simplest solution is hide what we can't > >>> be used. > >> > >> It is inconsistent that we do not do this for the other kernel exclusive > >> commands in userspace retreived Command Effects Log. The ABI here is raw > >> Get Supported Features payload. > > > > If they were exposed via similar paths I'd agree consistency matters > > but I 'hope' no one is going to have a tool that mixes fwctl and the > > legacy path. In my head we add all the useful commands to fwctl > > and that legacy path ends up effectively deprecated. > > > > Anyhow, I don't feel that strongly about this, it's just a case > > of doesn't smell of roses to me. > > > >> > >>>>> + /* These effects supported for all scope */ > >>>>> + if ((effects & CXL_CMD_CONFIG_CHANGE_COLD_RESET || > >>>>> + (effects & CXL_CMD_EFFECTS_EXTEND && > >>>>> + (effects & CXL_CMD_CONFIG_CHANGE_CONV_RESET || > >>>>> + effects & CXL_CMD_CONFIG_CHANGE_CXL_RESET))) && > >>>>> + scope >= FWCTL_RPC_DEBUG_WRITE) > >>>>> + return true; > >>>> > >>>> Looks good for the known bits, but this needs to return false for the > >>>> currently reserved bits because the driver can not assume a security > >>>> model for future effects. If a future spec adds > >>>> FWCTL_RPC_DEBUG_WRITE-safe effects, a new kernel is needed to allow > >>>> those Feature commands through. > >>>> > >>>> Sidenote: I wonder why the spec wasted one of its bits on an extend bit, > >>>> but here we are. The 'extend' concept is typically something like > >>>> "bit15: go look at this other field in this payload as this 16-bit field > >>>> was exhausted", not "bit9: the bits above this originally defined 16 bit > >>>> field now has more bits", oh well. > >>> > >>> It's odd but corner case of going from 'unknown' state for the remaining > >>> pair of bits to 0 means this and 1 means this. > >> > >> I don't understand. 0 means no effect to worry about whether it is > >> defined or not. > >> > >>> Naming though doesn't match the spec that calls it CEL[11:10] valid. > >>> Would be good to name it closer to that as we may well have something > >>> in bits 12 and 15 in future and it doesn't refer to them. > >> > >> Hopefully we can head off another "valid2" mistake, and I don't think > >> Linux needs to define anything for this bit. That bit's definition is: > >> > >> "Bit[9]: 1 is recommended, 0 is permitted (CEL[11:10] Valid)" > >> > >> ...which translates to "useless". If 11 or 10 are set, I don't care what > >> value 9 has. > >> > >> If 12:15 are set, I don't care if there is a future valid2 > >> bit gating whether or not to use them. Valid bits are for cases that go > >> outside of what Reserved 0 compatibility rules can convey, and I think > >> Reserved 0 compatiblity fully covers us in this case. > > > > Seems the spec authors disagreed. (obviously I can't comment on that > > discussion). > > > > Using just what anyone can see (if they have the spec) > > It was a clear spec hole and there wasn't an obvious default for 0 to > > mean so it was a 'read your device docs and act appropriately' case > > before this stuff was added. > > > > There may be corner cases where the right answer if we know the > > feature is not persisted over a reset but instead panic or take > > some heavy weight action. Same can be true the other way around > > in that we may have to do something heavy to manually reset something > > we don't want to persist over reset. Hopefully not but we'll see. > > > >> > >> So, if a device use case breaks because they set 10, but clear 9 and > >> expect software to ignore 10 then they get to keep all the pieces > >> because they have already broken the expectations of Reserved 0 > >> compat-software created before 9 existed. > > > > True, but a compliance test should have dealt with that (seems they > > are yet to catch up with this though). > > > > Jonathan > > > >
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 0b4946205910..f818e452dbca 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -246,6 +246,18 @@ struct cxl_mem_command *cxl_find_feature_command(u16 opcode) } EXPORT_SYMBOL_NS_GPL(cxl_find_feature_command, "CXL"); +struct cxl_mem_command *cxl_find_feature_command_by_id(u32 id) +{ + struct cxl_mem_command *c; + + cxl_for_each_feature_cmd(c) + if (c->info.id == id) + return c; + + return NULL; +} +EXPORT_SYMBOL_NS_GPL(cxl_find_feature_command_by_id, "CXL"); + static const char *cxl_mem_opcode_to_name(u16 opcode) { struct cxl_mem_command *c; diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c index cc72e73ae8d6..e65fc8479d21 100644 --- a/drivers/cxl/features.c +++ b/drivers/cxl/features.c @@ -50,11 +50,203 @@ static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length) return info; } +static struct cxl_feat_entry * +get_support_feature_info(struct cxl_features_state *cfs, + const struct fwctl_rpc_cxl *rpc_in) +{ + struct cxl_feat_entry *feat; + uuid_t uuid; + + if (rpc_in->op_size < sizeof(uuid)) + return ERR_PTR(-EINVAL); + + if (copy_from_user(&uuid, u64_to_user_ptr(rpc_in->in_payload), + sizeof(uuid))) + return ERR_PTR(-EFAULT); + + for (int i = 0; i < cfs->num_features; i++) { + feat = &cfs->entries[i]; + if (uuid_equal(&uuid, &feat->uuid)) + return feat; + } + + return ERR_PTR(-ENOENT); +} + +static void *cxlctl_get_supported_features(struct cxl_features_state *cfs, + const struct fwctl_rpc_cxl *rpc_in, + size_t *out_len) +{ + struct cxl_mbox_get_sup_feats_out *feat_out; + struct cxl_mbox_get_sup_feats_in feat_in; + struct cxl_feat_entry *saved, *pos; + int requested, copied; + size_t out_size; + u32 count; + u16 start; + + if (rpc_in->op_size != sizeof(feat_in)) + return ERR_PTR(-EINVAL); + + if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload), + rpc_in->op_size)) + return ERR_PTR(-EFAULT); + + count = le32_to_cpu(feat_in.count); + start = le16_to_cpu(feat_in.start_idx); + requested = count / sizeof(*pos); + + /* + * Make sure that the total requested number of entries is not greater + * than the total number of supported features allowed for userspace. + */ + if (start >= cfs->num_user_features) + return ERR_PTR(-EINVAL); + + requested = min_t(int, requested, cfs->num_user_features - start); + + out_size = sizeof(struct fwctl_rpc_cxl_out) + sizeof(*feat_out) + + requested * sizeof(*pos); + + struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) = + kvzalloc(out_size, GFP_KERNEL); + if (!rpc_out) + return ERR_PTR(-ENOMEM); + + rpc_out->size = sizeof(*feat_out) + requested * sizeof(*pos); + feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload; + if (requested == 0) { + feat_out->num_entries = cpu_to_le16(requested); + feat_out->supported_feats = cpu_to_le16(cfs->num_user_features); + rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS; + *out_len = out_size; + return no_free_ptr(rpc_out); + } + + pos = &feat_out->ents[0]; + saved = &cfs->entries[0]; + + copied = 0; + for (int i = 0; i < cfs->num_features; i++, saved++) { + if (is_cxl_feature_exclusive(saved)) + continue; + + memcpy(pos, saved, sizeof(*saved)); + copied++; + if (copied == requested) + break; + pos++; + } + + feat_out->num_entries = cpu_to_le16(requested); + feat_out->supported_feats = cpu_to_le16(cfs->num_user_features); + rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS; + *out_len = out_size; + + return no_free_ptr(rpc_out); +} + +static bool cxlctl_validate_set_features(struct cxl_features_state *cfs, + const struct fwctl_rpc_cxl *rpc_in, + enum fwctl_rpc_scope scope) +{ + struct cxl_feat_entry *feat; + u16 effects, mask; + u32 flags; + + feat = get_support_feature_info(cfs, rpc_in); + if (IS_ERR(feat)) + return false; + + /* Ensure that the attribute is changeable */ + flags = le32_to_cpu(feat->flags); + if (!(flags & CXL_FEATURE_F_CHANGEABLE)) + return false; + + effects = le16_to_cpu(feat->effects); + /* Currently no user background command support */ + if (effects & CXL_CMD_BACKGROUND) + return false; + + mask = CXL_CMD_CONFIG_CHANGE_IMMEDIATE | + CXL_CMD_DATA_CHANGE_IMMEDIATE | + CXL_CMD_POLICY_CHANGE_IMMEDIATE | + CXL_CMD_LOG_CHANGE_IMMEDIATE; + if (effects & mask && scope >= FWCTL_RPC_DEBUG_WRITE_FULL) + return true; + + /* These effects supported for all scope */ + if ((effects & CXL_CMD_CONFIG_CHANGE_COLD_RESET || + (effects & CXL_CMD_EFFECTS_EXTEND && + (effects & CXL_CMD_CONFIG_CHANGE_CONV_RESET || + effects & CXL_CMD_CONFIG_CHANGE_CXL_RESET))) && + scope >= FWCTL_RPC_DEBUG_WRITE) + return true; + + return false; +} + +static struct cxl_mem_command * +cxlctl_get_valid_hw_command(struct cxl_features_state *cfs, + const struct fwctl_rpc_cxl *rpc_in, + enum fwctl_rpc_scope scope) +{ + struct cxl_mem_command *cmd; + + if (!cfs->num_features) + return ERR_PTR(-EOPNOTSUPP); + + cmd = cxl_find_feature_command_by_id(rpc_in->command_id); + if (!cmd) + return ERR_PTR(-ENOENT); + + if (!cxl_feature_enabled(cfs, cmd->opcode)) + return ERR_PTR(-EPERM); + + switch (cmd->opcode) { + case CXL_MBOX_OP_GET_SUPPORTED_FEATURES: + case CXL_MBOX_OP_GET_FEATURE: + if (scope >= FWCTL_RPC_CONFIGURATION) + return cmd; + break; + case CXL_MBOX_OP_SET_FEATURE: + if (cxlctl_validate_set_features(cfs, rpc_in, scope)) + return cmd; + break; + default: + return ERR_PTR(-EINVAL); + } + + return ERR_PTR(-EINVAL); +} + +static void *cxlctl_handle_commands(struct cxl_features_state *cfs, + const struct fwctl_rpc_cxl *rpc_in, + size_t *out_len, struct cxl_mem_command *cmd) +{ + switch (cmd->opcode) { + case CXL_MBOX_OP_GET_SUPPORTED_FEATURES: + return cxlctl_get_supported_features(cfs, rpc_in, out_len); + case CXL_MBOX_OP_GET_FEATURE: + case CXL_MBOX_OP_SET_FEATURE: + default: + return ERR_PTR(-EOPNOTSUPP); + } +} + static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope, - void *rpc_in, size_t in_len, size_t *out_len) + void *in, size_t in_len, size_t *out_len) { - /* Place holder */ - return ERR_PTR(-EOPNOTSUPP); + struct fwctl_device *fwctl = uctx->fwctl; + struct cxl_features_state *cfs = to_cxl_features_state(fwctl); + const struct fwctl_rpc_cxl *rpc_in = in; + struct cxl_mem_command *cmd; + + cmd = cxlctl_get_valid_hw_command(cfs, rpc_in, scope); + if (IS_ERR(cmd)) + return (void *)cmd; + + return cxlctl_handle_commands(cfs, rpc_in, out_len, cmd); } static const struct fwctl_ops cxlctl_ops = { diff --git a/include/cxl/features.h b/include/cxl/features.h index 9f234659ca2d..b5dacc1665c7 100644 --- a/include/cxl/features.h +++ b/include/cxl/features.h @@ -69,6 +69,7 @@ size_t cxl_get_feature(struct cxl_features *features, const uuid_t feat_uuid, int cxl_set_feature(struct cxl_features *features, const uuid_t feat_uuid, u8 feat_version, void *feat_data, size_t feat_data_size, u32 feat_flag, u16 offset, u16 *return_code); +struct cxl_mem_command *cxl_find_feature_command_by_id(u32 id); bool is_cxl_feature_exclusive(struct cxl_feat_entry *entry); #endif diff --git a/include/uapi/cxl/features.h b/include/uapi/cxl/features.h index 18b74252058a..55fa5b2a5d17 100644 --- a/include/uapi/cxl/features.h +++ b/include/uapi/cxl/features.h @@ -14,6 +14,19 @@ #include <linux/uuid.h> #endif +/* CXL spec r3.2 Table 8-87 command effects */ +#define CXL_CMD_CONFIG_CHANGE_COLD_RESET BIT(0) +#define CXL_CMD_CONFIG_CHANGE_IMMEDIATE BIT(1) +#define CXL_CMD_DATA_CHANGE_IMMEDIATE BIT(2) +#define CXL_CMD_POLICY_CHANGE_IMMEDIATE BIT(3) +#define CXL_CMD_LOG_CHANGE_IMMEDIATE BIT(4) +#define CXL_CMD_SECURITY_STATE_CHANGE BIT(5) +#define CXL_CMD_BACKGROUND BIT(6) +#define CXL_CMD_BGCMD_ABORT_SUPPORTED BIT(7) +#define CXL_CMD_EFFECTS_EXTEND BIT(9) +#define CXL_CMD_CONFIG_CHANGE_CONV_RESET BIT(10) +#define CXL_CMD_CONFIG_CHANGE_CXL_RESET BIT(11) + /* Get Supported Features (0x500h) CXL r3.1 8.2.9.6.1 */ struct cxl_mbox_get_sup_feats_in { __le32 count; diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h index 79b822dbfafd..ad23c65ce724 100644 --- a/include/uapi/fwctl/cxl.h +++ b/include/uapi/fwctl/cxl.h @@ -28,4 +28,33 @@ enum feature_cmds { struct fwctl_info_cxl { __u32 cmd_mask; }; + +/** + * struct fwctl_rpc_cxl - ioctl(FWCTL_RPC) input for CXL + * @command_id: the defined command id by 'enum feature_cmds' + * @flags: Flags for the command (input). + * @op_size: Size of input payload. + * @reserved1: Reserved. Must be 0s. + * @in_payload: User address of the hardware op input structure + */ +struct fwctl_rpc_cxl { + __u32 command_id; + __u32 flags; + __u32 op_size; + __u32 reserved1; + __aligned_u64 in_payload; +}; + +/** + * struct fwctl_rpc_cxl_out - ioctl(FWCTL_RPC) output for CXL + * @size: Size of the output payload + * @retval: Return value from device + * @payload: Return data from device + */ +struct fwctl_rpc_cxl_out { + __u32 size; + __u32 retval; + __u8 payload[] __counted_by(size); +}; + #endif
fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls to a device. The cxl fwctl driver will start by supporting the CXL feature commands: Get Supported Features, Get Feature, and Set Feature. The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where it indicates the security scope of the call. The Get Supported Features and Get Feature calls can be executed with the scope of FWCTL_RPC_CONFIGRATION. The Set Feature call is gated by the effects of the feature reported by Get Supported Features call for the specific feature. Only "get supported features" is supported in this patch. Additional commands will be added in follow on patches. "Get supported features" will filter the features that are exclusive to the kernel and only report out features that are not kernel only. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/cxl/core/mbox.c | 12 +++ drivers/cxl/features.c | 198 +++++++++++++++++++++++++++++++++++- include/cxl/features.h | 1 + include/uapi/cxl/features.h | 13 +++ include/uapi/fwctl/cxl.h | 29 ++++++ 5 files changed, 250 insertions(+), 3 deletions(-)