Re: [PATCH 1/4] tsm: Add TVM Measurement Register support

[Huang, Kai]

On Mon, 2025-02-17 at 13:17 +1300, Huang, Kai wrote:
> Hi Cedric,
> 
> [...]
> 
> > +static ssize_t tmr_digest_read(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
> > +			       char *page, loff_t off, size_t count)
> 
> Better to rename 'page' to 'buffer'?
> 
> Since page normally implies 4KB alignment but I don't see we need the 
> alignment here.
> 
> > +{
> > +	const struct tsm_measurement_register *mr;
> > +	struct tmr_provider *pvd;
> > +	int rc;
> > +
> > +	if (off < 0 || off > attr->size)
> > +		return -EINVAL;
> > +
> > +	count = min(count, attr->size - (size_t)off);
> > +	if (!count)
> > +		return count;
> > +
> > +	mr = (typeof(mr))attr->private;
> > +	pvd = tmr_mr_to_provider(mr, kobj);
> > +	rc = down_read_interruptible(&pvd->rwsem);
> > +	if (rc)
> > +		return rc;
> > +
> > +	if ((mr->mr_flags & TSM_MR_F_L) && !pvd->in_sync) {
> > +		up_read(&pvd->rwsem);
> > +
> > +		rc = down_write_killable(&pvd->rwsem);
> > +		if (rc)
> > +			return rc;
> > +
> > +		if (!pvd->in_sync) {
> > +			rc = tmr_call_refresh(pvd, mr);
> > +			pvd->in_sync = !rc;
> > +		}
> > +
> > +		downgrade_write(&pvd->rwsem);
> > +	}
> > +
> > +	if (!rc)
> > +		memcpy(page, mr->mr_value + off, count);
> > +
> > +	up_read(&pvd->rwsem);
> > +	return rc ?: count;
> > +}
> > +
> > +static ssize_t tmr_digest_write(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
> > +				char *page, loff_t off, size_t count)
> > +{
> > +	const struct tsm_measurement_register *mr;
> > +	struct tmr_provider *pvd;
> > +	ssize_t rc;
> > +
> > +	if (off != 0 || count != attr->size)
> > +		return -EINVAL;
> > +
> > +	mr = (typeof(mr))attr->private;
> > +	pvd = tmr_mr_to_provider(mr, kobj);
> > +	rc = down_write_killable(&pvd->rwsem);
> > +	if (rc)
> > +		return rc;
> > +
> > +	if (mr->mr_flags & TSM_MR_F_X)
> > +		rc = tmr_call_extend(pvd, mr, page);
> > +	else
> > +		memcpy(mr->mr_value, page, count);
> > +
> > +	if (!rc)
> > +		pvd->in_sync = false;
> > +
> > +	up_write(&pvd->rwsem);
> > +	return rc ?: count;
> > +}
> 
> The logic around using pvd->in_sync is kinda complicated.  MR operations 
> seem like a classic reader/writer contention problem and I am not sure 
> why pvd->in_sync is needed.  Could you help to clarify?
> 
> [...]

[...]

> > 
> > +#define TSM_MR_(mr, hash)                                                           \
> > +	.mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash = HASH_ALGO_##hash, \
> > +	.mr_flags = TSM_MR_F_R
> > +
> > +/**
> > + * struct tsm_measurement - define CC specific MRs and methods for updating them
> > + * @name: name of the measurement provider
> > + * @mrs: array of MR definitions ending with mr_name set to %NULL
> > + * @refresh: invoked to update the specified MR
> > + * @extend: invoked to extend the specified MR with mr_size bytes
> > + */
> > +struct tsm_measurement {
> > +	const char *name;
> > +	const struct tsm_measurement_register *mrs;
> > +	int (*refresh)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr);
> > +	int (*extend)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr,
> > +		      const u8 *data);
> > +};
> 
>  From the description above, I don't quite follow what does ->refresh() 
> do exactly.  Could you clarify why we need it?
> 

After reading patch 4, I figured out the purpose of the pvd->in_sync and the
refresh() myself:

Each call of ->extend() will update the value of the MR, but only in the
hardware/firmware.  The MR value maintained by the kernel is unchanged so it
will be out-of-date after the ->extend().  Therefore the kernel needs to keep
them synced first when userspace reads the MR, using the ->refresh().

It's also feasible to always sync/refresh after each extend(), eliminating the
pvd->in_sync logic completely, but it's not desired -- because the sync
operation talks to the hardware/firmware thus could be time-consuming, and we in
general only needs to sync before reading the value to the userspace.  E.g.,
this saves a lot of sync operation if userspace extends the MR multi-times with
large chunk of data and then reads the final value once.

So the pvd->in_sync logic seems useful to me, but I think it would be better to
a comment to explain this in both tmr_digest_read() and tmr_digest_write(). 
E.g., something like below before the code which checks pvd->in_sync in the
tmr_digest_read()?

	/*
	 * Each write-and-extend to the MR will update the MR, but only in
	 * TSM hardware/firmware, leaving the MR value maintained by the kernel
	 * out-of-date.  Sync MR value in TSM hardware/firmware to the kernel 
	 * before returning to the userspace in this case.
	 */
	if ((mr->mr_flags & TSM_MR_F_L) && !pvd->in_sync) {
		...

The description of the refresh() callback is also not clear to me.  Perhaps
something like below?

	/*
	 * ...
	 * @refresh: sync MR value in TSM hardware/firmware to the kernel
buffer
	 * ...
	 */