Re: [Qemu-devel] RFC: guest-side retrieval of fw_cfg file

[Matt Fleming <matt.fleming@intel.com>]

On Mon, 2015-07-13 at 16:09 -0400, Gabriel L. Somlo wrote:
> 
> 3. I'm currently only handling x86 and I/O ports. I could drop the
>    fw_cfg_dmi_whitelist and just check the signature, using mmio where
>    appropriate, but I don't have a handy-dandy set of VMs for those
>    architectures on which I could test. Wondering if that's something
>    we should have before I officially try to submit this to the kernel,
>    or whether it could wait for a second iteration.
> 
>    Speaking of the kernel: My default plan is to subscribe to
>    kernelnewbies@kernelnewbies.org and submit it there (this is after
>    all baby's first kernel module :) but if there's a better route for
>    pushing it upstream, please feel free to suggest it to me.

Congrats on your first kernel patch! Cc'ing kernelnewbies is OK but this
patches warrants a wider audience. Cc Greg Kroah-Hartman
(gregkh@linuxfoundation.org) since he polices things related to sysfs,
the x86 maintainers (x86@kernel.org), and linux-kernel@vger.kernel.org.
The closest thing we have to a Linux system firmware list is probably
linux-efi@vger.kernel.org.

By all means, mention that it's your first kernel patch when submitting
this.

Oh, you're also going to want to add documentation under
Documentation/ABI/testing/sysfs-firmware-fw_cfg as part of your patch
series.


> /* provide atomic read access to hardware fw_cfg device
>  * (critical section involves potentially lengthy i/o, using mutex) */
> static DEFINE_MUTEX(fw_cfg_dev_lock);

Just a very small nitpick, but,

	/*
	 * Multi-line comments should look like this
	 * in the kernel.
	 */


> static int __init fw_cfg_sysfs_init(void)
> {
> 	int ret = 0;
> 	uint32_t count, i;
> 	char sig[FW_CFG_SIG_SIZE];
> 	struct fw_cfg_sysfs_entry *entry;
> 
> 	/* only install on matching "hardware" */
> 	if (!dmi_check_system(fw_cfg_whitelist)) {
> 		pr_warn("Supported QEMU Standard PC hardware not found!\n");
> 		return -ENODEV;
> 	}

I would suggest deleting this warning because it's just noise if I build
this code into my kernel and boot it on non-qemu hardware.

> 	/* verify fw_cfg device signature */
> 	fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE);
> 	if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> 		pr_warn("QEMU fw_cfg signature not found!\n");
> 		return -ENODEV;
> 	}
> 
> 	/* create fw_cfg folder in sysfs */
> 	fw_cfg_kset = kset_create_and_add("fw_cfg", NULL, firmware_kobj);
> 	if (!fw_cfg_kset)
> 		return -ENOMEM;
> 
> 	/* process fw_cfg file directory entry */
> 	mutex_lock(&fw_cfg_dev_lock);
> 	outw(FW_CFG_FILE_DIR, FW_CFG_PORT_CTL);	/* select fw_cfg directory */
> 	insb(FW_CFG_PORT_DATA, &count, sizeof(count)); /* get file count */
> 	count = be32_to_cpu(count);
> 	/* create & register a sysfs node for each file in the directory */
> 	for (i = 0; i < count; i++) {
> 		/* allocate new entry */
> 		entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> 		if (!entry) {
> 			ret = -ENOMEM;
> 			break;
> 		}
> 
> 		/* acquire file information from directory */
> 		insb(FW_CFG_PORT_DATA, &entry->f, sizeof(struct fw_cfg_file));
> 		entry->f.size = be32_to_cpu(entry->f.size);
> 		entry->f.select = be16_to_cpu(entry->f.select);
> 
> 		/* register sysfs entry for this file */
> 		entry->kobj.kset = fw_cfg_kset;
> 		ret = kobject_init_and_add(&entry->kobj,
> 					   &fw_cfg_sysfs_entry_ktype, NULL,
> 					   /* NOTE: '/' represented as '!' */
> 					   "%s", entry->f.name);
> 		if (ret)
> 			break;

If you take this error path, aren't you leaking 'entry'? It isn't yet on
the 'fw_cfg_entry_cache' list and so won't be freed in
fw_cfg_sysfs_cache_cleanup().