Re: [PATCH v4 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry

[Chen Yu <yu.c.chen@intel.com>]

On Sat, Oct 16, 2021 at 05:10:25PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Oct 16, 2021 at 06:44:31PM +0800, Chen Yu wrote:
> > Platform Firmware Runtime Update(PFRU) Telemetry Service is part of RoT
> > (Root of Trust), which allows PFRU handler and other PFRU drivers to
> > produce telemetry data to upper layer OS consumer at runtime.
> > 
> > The linux provides interfaces for the user to query the parameters of
> > telemetry data, and the user could read out the telemetry data
> > accordingly.
> 
> What type of interface is this?  How does userspace interact with it?
> 
> > 
> > Also add the ABI documentation.
> 
> Add it where?
>
They are all ioctl interfaces, and was introduced in previous patch in
Documentation/ABI/testing/pfru. The way userspace interace with it is
introduced in next patch in the man page. I'll revise the commit log to
better describe how user could use it.
> > 
> > Typical log looks like:
> > [SmmRuntimeUpdateHandler.ProcessSmmRuntimeUpdate]
> > ProcessSmmRuntimeUpdate = START, Action = 2
> > [SmmRuntimeUpdateHandler.ProcessSmmRuntimeUpdate]
> > FwVersion = 0, CodeInjectionVersion = 1
> > [ShadowSmmRuntimeUpdateImage]
> > Image = 0x74D9B000, ImageSize = 0x1172
> > [ProcessSmmRuntimeUpdate]
> > ShadowSmmRuntimeUpdateImage.Status = Success
> > [ValidateSmmRuntimeUpdateImage]
> > CapsuleHeader.CapsuleGuid = 6DCBD5ED-E82D-4C44-BDA1-7194199AD92A
> > [ValidateSmmRuntimeUpdateImage]
> > FmpCapHeader.Version = 1
> > [ValidateSmmRuntimeUpdateImage]
> > FmpCapImageHeader.UpdateImageTypeId = B2F84B79-7B6E-4E45-885F-3FB9BB185402
> > [ValidateSmmRuntimeUpdateImage]
> > SmmRuntimeUpdateVerifyImageWithDenylist.Status = Success
> > [ValidateSmmRuntimeUpdateImage]
> > SmmRuntimeUpdateVerifyImageWithAllowlist.Status = Success
> > [SmmCodeInjectionVerifyPayloadHeader]
> > PayloadHeader.Signature = 0x31494353
> > [SmmCodeInjectionVerifyPayloadHeader]
> > PayloadHeader.PlatformId = 63462139-A8B1-AA4E-9024-F2BB53EA4723
> > [SmmCodeInjectionVerifyPayloadHeader]
> > PayloadHeader.SupportedSmmFirmwareVersion = 0,
> > PayloadHeader.SmmCodeInjectionRuntimeVersion = 1
> > [ProcessSmmRuntimeUpdate]
> > ValidateSmmRuntimeUpdateImage.Status = Success
> > CPU CSR[0B102D28] Before = 7FBF830E
> > CPU CSR[0B102D28] After = 7FBF8310
> > [ProcessSmmRuntimeUpdate] ProcessSmmCodeInjection.Status = Success
> > [SmmRuntimeUpdateHandler.ProcessSmmRuntimeUpdate]
> > ProcessSmmRuntimeUpdate = End, Status = Success
> 
> This log does not make any sense to me, where is it from?  Why the odd
> line-wrapping?
>
It is from the telemetry log generated by the BIOS. Since this content is
platform specific, I'll remove the log in next version.
> > +};
> > +
> > +
[snip...]
> > +ssize_t pfru_log_read(struct file *filp, char __user *ubuf,
> > +		      size_t size, loff_t *off)
> > +{
> > +	struct pfru_log_data_info info;
> > +	phys_addr_t base_addr;
> > +	int buf_size, ret;
> > +	char *buf_ptr;
> > +
> > +	if (!pfru_log_dev)
> > +		return -ENODEV;
> > +
> > +	if (*off < 0)
> > +		return -EINVAL;
> > +
> > +	ret = get_pfru_log_data_info(&info, pfru_log_dev->info.log_type);
> > +	if (ret)
> > +		return ret;
> > +
> > +	base_addr = (phys_addr_t)(info.chunk2_addr_lo | (info.chunk2_addr_hi << 32));
> > +	/* pfru update has not been launched yet.*/
> > +	if (!base_addr)
> > +		return -EBUSY;
> > +
> > +	buf_size = info.max_data_size;
> > +	if (*off >= buf_size)
> > +		return 0;
> > +
> > +	buf_ptr = memremap(base_addr, buf_size, MEMREMAP_WB);
> > +	if (IS_ERR(buf_ptr))
> > +		return PTR_ERR(buf_ptr);
> > +
> > +	size = min_t(size_t, size, buf_size - *off);
> > +	if (copy_to_user(ubuf, buf_ptr + *off, size))
> > +		ret = -EFAULT;
> > +	else
> > +		ret = 0;
> 
> As all you are doing is mapping some memory and reading from it, why do
> you need a read() file operation at all?  Why not just use mmap?
> 
In the beginning mmap() interface was provided to the user. Then it was
realized that there is no guarantee in the spec that, the physical address
provided by the BIOS would remain unchanged. So instead of asking the user
to mmap the file each time before reading the log, the read() is leveraged
here to always memremap() the latest address.

thanks,
Chenyu
> thanks,
> 
> greg k-h