From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47732) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFLMu-0003yF-VJ for qemu-devel@nongnu.org; Wed, 15 Jul 2015 08:00:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZFLMr-0005Q3-NO for qemu-devel@nongnu.org; Wed, 15 Jul 2015 08:00:56 -0400 Received: from mga11.intel.com ([192.55.52.93]:19317) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFLMr-0005Pj-EG for qemu-devel@nongnu.org; Wed, 15 Jul 2015 08:00:53 -0400 Message-ID: <1436961645.25906.20.camel@intel.com> From: Matt Fleming Date: Wed, 15 Jul 2015 13:00:45 +0100 In-Reply-To: <20150713200936.GK1606@HEDWIG.INI.CMU.EDU> References: <20150713200936.GK1606@HEDWIG.INI.CMU.EDU> Content-Type: text/plain; charset="UTF-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] RFC: guest-side retrieval of fw_cfg file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gabriel L. Somlo" Cc: mdroth@linux.vnet.ibm.com, rjones@redhat.com, jordan.l.justen@intel.com, "Gabriel L. Somlo" , qemu-devel@nongnu.org, gleb@cloudius-systems.com, kraxel@redhat.com, pbonzini@redhat.com, Laszlo Ersek 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().