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

[Dan Williams <dan.j.williams@intel.com>]

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.