Re: [PATCH v7 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

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

On Wed, Oct 27, 2021 at 01:20:33PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 27, 2021 at 03:07:51PM +0800, Chen Yu wrote:
> > Introduce the pfru_update driver which can be used for Platform Firmware
> > Runtime code injection and driver update [1]. The user is expected to
> > provide the update firmware in the form of capsule file, and pass it to
> > the driver via ioctl. Then the driver would hand this capsule file to the
> > Platform Firmware Runtime Update via the ACPI device _DSM method. At last
> > the low level Management Mode would do the firmware update.
> > 
> > The corresponding userspace tool and man page will be introduced at
> > tools/power/acpi/pfru.
> 
> ...
> 
> > +static int get_image_type(struct efi_manage_capsule_image_header *img_hdr,
> > +			  struct pfru_device *pfru_dev)
> > +{
> > +	guid_t *image_type_id = &img_hdr->image_type_id;
> 
> efi_guid_t ?
>
efi_guid_t is a 32-bit aligned guid_t, which is for the case when
efi_guid_t* arguments are 32-bit aligned. And it is for 32-bit ARM.
Since this code injection is only for 64-bit, the guid is not required
to be strictly 32-bit aligned I suppose?
> > +	/* check whether this is a code injection or driver update */
> > +	if (guid_equal(image_type_id, &pfru_dev->code_uuid))
> > +		return CODE_INJECT_TYPE;
> > +
> > +	if (guid_equal(image_type_id, &pfru_dev->drv_uuid))
> > +		return DRIVER_UPDATE_TYPE;
> > +
> > +	return -EINVAL;
> > +}
> 
> ...
> 
> > +static bool valid_version(const void *data, struct pfru_update_cap_info *cap,
> > +			  struct pfru_device *pfru_dev)
> > +{
> > +	struct pfru_payload_hdr *payload_hdr;
> > +	efi_capsule_header_t *cap_hdr;
> > +	struct efi_manage_capsule_header *m_hdr;
> > +	struct efi_manage_capsule_image_header *m_img_hdr;
> > +	struct efi_image_auth *auth;
> > +	int type, size;
> > +
> > +	/*
> > +	 * Sanity check if the capsule image has a newer version
> > +	 * than current one.
> > +	 */
> > +	cap_hdr = (efi_capsule_header_t *)data;
> 
> Why casting?
> 
Will remove this in next version.
> > +	size = cap_hdr->headersize;
> > +	m_hdr = (struct efi_manage_capsule_header *)(data + size);
> > +	/*
> > +	 * Current data structure size plus variable array indicated
> > +	 * by number of (emb_drv_cnt + payload_cnt)
> > +	 */
> > +	size += sizeof(struct efi_manage_capsule_header) +
> > +		      (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
> > +	m_img_hdr = (struct efi_manage_capsule_image_header *)(data + size);
> > +
> > +	type = get_image_type(m_img_hdr, pfru_dev);
> > +	if (type < 0)
> > +		return false;
> > +
> > +	size = adjust_efi_size(m_img_hdr, size);
> > +	if (size < 0)
> > +		return false;
> > +
> > +	auth = (struct efi_image_auth *)(data + size);
> > +	size += sizeof(u64) + auth->auth_info.hdr.len;
> > +	payload_hdr = (struct pfru_payload_hdr *)(data + size);
> > +
> > +	/* finally compare the version */
> > +	if (type == CODE_INJECT_TYPE)
> > +		return payload_hdr->rt_ver >= cap->code_rt_version;
> > +	else
> > +		return payload_hdr->rt_ver >= cap->drv_rt_version;
> > +}
> 
> ...
> 
> > +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > +	struct pfru_update_cap_info cap;
> > +	struct pfru_device *pfru_dev;
> > +	void __user *p;
> > +	int ret, rev;
> 
> > +	pfru_dev = to_pfru_dev(file);
> > +	p = (void __user *)arg;
> 
> Can be combined with definitions above. Ditto for the rest cases in the code.
>
Ok, will do. 
> > +}
> 
> ...
> 
> > +	phy_addr = (phys_addr_t)(info.addr_lo | (info.addr_hi << 32));
> 
> Does it compile without warnings for 32-bit target?
> 
> ...
>
The code injection depends on Kconfig 64-bit and is not supposed to work
on 32-bit AFAIK. 
> > +	ret = guid_parse(PFRU_UUID, &pfru_dev->uuid);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = guid_parse(PFRU_CODE_INJ_UUID, &pfru_dev->code_uuid);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = guid_parse(PFRU_DRV_UPDATE_UUID, &pfru_dev->drv_uuid);
> > +	if (ret)
> > +		return ret;
> 
> Why do you need to keep zillions of copies of the data which seems
> is not going to be changed? Three global variables should be enough,
> no?
>
The guid information is embedded in each pfru_dev and only calculated
once during probe. I thought people try to avoid using global variables
if possible?
> > +	ret = ida_alloc(&pfru_ida, GFP_KERNEL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pfru_dev->index = ret;
> 
> ...
> 
> > +	/* default rev id is 1 */
> 
> Shouldn't you rather define this magic and drop this doubtful comment?
> 
Ok, will do.
> > +	pfru_dev->rev_id = 1;
> 
> ...
> 
> > +failed:
> 
> Make you labeling consistent. The usual pattern is to explain what will be
> happened when goto to the certain label, for example, here is 'err_free_ida'.
> Also, add an empty line everywhere before labels.
> 
Ok, got it, will do.
> > +	ida_free(&pfru_ida, pfru_dev->index);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +#define UUID_SIZE 16
> 
> It must not be here at all.
> Or it should be properly namespaced.
> 
Ok. Would __u8 uuid[16] applicable? There are some examples in uapi headers.

thanks,
Chenyu
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>