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

[Matt Fleming]

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
>    address@hidden 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
(address@hidden) since he polices things related to sysfs,
the x86 maintainers (address@hidden), and address@hidden
The closest thing we have to a Linux system firmware list is probably
address@hidden

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().