From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751838AbbJDU2H (ORCPT ); Sun, 4 Oct 2015 16:28:07 -0400 Received: from SMTP.ANDREW.CMU.EDU ([128.2.105.203]:50711 "EHLO smtp.andrew.cmu.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751756AbbJDU2F (ORCPT ); Sun, 4 Oct 2015 16:28:05 -0400 Date: Sun, 4 Oct 2015 16:27:46 -0400 From: "Gabriel L. Somlo" To: "Michael S. Tsirkin" Cc: gregkh@linuxfoundation.org, paul@pwsan.com, galak@codeaurora.org, will.deacon@arm.com, agross@codeaurora.org, mark.rutland@arm.com, zajec5@gmail.com, hanjun.guo@linaro.org, catalin.marinas@arm.com, linux-api@vger.kernel.org, linux-kernel@vger.kernel.org, kernelnewbies@kernelnewbies.org, matt.fleming@intel.com, lersek@redhat.com, jordan.l.justen@intel.com, peter.maydell@linaro.org, leif.lindholm@linaro.org, ard.biesheuvel@linaro.org, pbonzini@redhat.com, kraxel@redhat.com, qemu-devel@nongnu.org Subject: Re: [PATCH v3 2/4] firmware: use acpi to detect QEMU fw_cfg device for sysfs fw_cfg driver Message-ID: <20151004202746.GE1977@HEDWIG.INI.CMU.EDU> References: <1443914889-9619-1-git-send-email-somlo@cmu.edu> <1443914889-9619-3-git-send-email-somlo@cmu.edu> <20151004103555-mutt-send-email-mst@redhat.com> <20151004202400.GD1977@HEDWIG.INI.CMU.EDU> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151004202400.GD1977@HEDWIG.INI.CMU.EDU> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.5.23 (2014-03-12) X-PMX-Version: 6.0.3.2322014, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2015.1.28.224819 X-SMTP-Spam-Clean: 8% ( MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODY_SIZE_4000_4999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, DATE_TZ_NA 0, FROM_EDU_TLD 0, NO_URI_HTTPS 0, REFERENCES 0, __ANY_URI 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __CD 0, __CT 0, __CT_TEXT_PLAIN 0, __FORWARDED_MSG 0, __HAS_FROM 0, __HAS_MSGID 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __PHISH_SPEAR_STRUCTURE_1 0, __REFERENCES 0, __SANE_MSGID 0, __STOCK_PHRASE_7 0, __SUBJ_ALPHA_END 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 0, __URI_NO_PATH 0, __URI_NO_WWW 0, __URI_NS , __USER_AGENT 0) X-SMTP-Spam-Score: 8% Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Oct 04, 2015 at 04:24:00PM -0400, Gabriel L. Somlo wrote: > On Sun, Oct 04, 2015 at 10:54:57AM +0300, Michael S. Tsirkin wrote: > > On Sat, Oct 03, 2015 at 07:28:07PM -0400, Gabriel L. Somlo wrote: > > > > > > Instead of blindly probing fw_cfg registers at known IOport and MMIO > > > locations, use the ACPI subsystem to determine whether a QEMU fw_cfg > > > device is present, and, if found, to initialize it. > > > > > > This limits portability to architectures which support ACPI (x86 and > > > UEFI-enabled aarch64), but avoids touching hardware registers before > > > being certain that our device is present. > > > > > > NOTE: The standard way to verify the presence of fw_cfg on arm VMs > > > would have been to use the device tree, but that would have left out > > > x86, which is the primary architecture targeted by this patch. > > > > > > Signed-off-by: Gabriel Somlo > > > > IMHO it's not a good idea to probe registers provided > > by CRS like this. > > It seems quite reasonable that we'd want to add some > > extra registers in the future, and this probing will break. > > > > Further, accessing registers directly means that there's > > no way to have ACPI code access them as that would > > cause race conditions. > > > > Maybe we should provide access methods in ACPI instead? > > OK, I think I understand what you meant by "don't poke CRS" in the > other thread... > > So, you're proposing I move the follwing bits: > > /* atomic access to fw_cfg device (potentially slow i/o, so using > * mutex) */ > static DEFINE_MUTEX(fw_cfg_dev_lock); > > /* pick appropriate endianness for selector key */ > static inline u16 fw_cfg_sel_endianness(u16 key) > { > return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key); > } > > /* type for fw_cfg "directory scan" visitor/callback function */ > typedef int (*fw_cfg_file_callback)(const struct fw_cfg_file *f); > > /* run a given callback on each fw_cfg directory entry */ > static int fw_cfg_scan_dir(fw_cfg_file_callback callback) > { > int ret = 0; > u32 count, i; > struct fw_cfg_file f; > > mutex_lock(&fw_cfg_dev_lock); > iowrite16(fw_cfg_sel_endianness(FW_CFG_FILE_DIR), fw_cfg_reg_ctrl); > ioread8_rep(fw_cfg_reg_data, &count, sizeof(count)); > for (i = 0; i < be32_to_cpu(count); i++) { > ioread8_rep(fw_cfg_reg_data, &f, sizeof(f)); > ret = callback(&f); > if (ret) > break; > } > mutex_unlock(&fw_cfg_dev_lock); > return ret; > } > > /* read chunk of given fw_cfg blob (caller responsible for > * sanity-check) */ > static inline void fw_cfg_read_blob(u16 key, > void *buf, loff_t pos, size_t count) > { > mutex_lock(&fw_cfg_dev_lock); > iowrite16(fw_cfg_sel_endianness(key), fw_cfg_reg_ctrl); > while (pos-- > 0) > ioread8(fw_cfg_reg_data); > ioread8_rep(fw_cfg_reg_data, buf, count); > mutex_unlock(&fw_cfg_dev_lock); > } > > into the FWCF, "QEMU0002" node as an AML method ? Have ACPI provide > mutual exclusion against competing readers, and somehow figure out how > to call the ACPI/AML code from the guest-side kernel driver whenever > I need to call fw_cfg_read_blob() ? > > I guess I could implement fw_cfg_scan_dir() using fw_cfg_read_blob(): > > u32 count; > size_t bufsize; > void *buf; > fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(u32)); > bufsize = sizeof(u32) + count * sizeof(struct fw_cfg_file); > buf = kalloc(bufsize); > fw_cfg_read_blob(FW_CFG_FILE_DIR, buf, 0, bufsize); > ... > /* now read all the blob meta-data from buf ... */ > > It would be 100% atomic, but since we can safely assume the fw_cfg > contents never change, it'd be OK. I meant "wouldn't be 100% atomic", as in "it would be a case of verify-then-use"... Sorry, --Gabriel > > The atomicity of the ACPI version of fw_cfg_read_blob(), picking the > right endianness for the selector, etc. would have to be done in AML > within the QEMU host-side patch. > > If you know of anything I can look at for a good ASL example, please > point it out to me. I'm going to go away now and spend some quality > time with the ACPI spec :) > > Thanks, > --Gabriel