Re: [PATCH v1 14/19] cxl: Add support for fwctl RPC command to enable CXL feature commands

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

> > +}
> > +
> > +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