Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030795AbcCQNe3 (ORCPT ); Thu, 17 Mar 2016 09:34:29 -0400 Received: from SMTP.ANDREW.CMU.EDU ([128.2.105.204]:42949 "EHLO smtp.andrew.cmu.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030487AbcCQNe1 (ORCPT ); Thu, 17 Mar 2016 09:34:27 -0400 Date: Thu, 17 Mar 2016 09:33:40 -0400 From: "Gabriel L. Somlo" To: "Michael S. Tsirkin" Cc: Greg KH , robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, arnd@arndb.de, lersek@redhat.com, ralf@linux-mips.org, rmk+kernel@arm.linux.org.uk, eric@anholt.net, hanjun.guo@linaro.org, zajec5@gmail.com, sudeep.holla@arm.com, agross@codeaurora.org, linux-api@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, qemu-devel@nongnu.org, imammedo@redhat.com, peter.maydell@linaro.org, leif.lindholm@linaro.org, ard.biesheuvel@linaro.org, pbonzini@redhat.com, kraxel@redhat.com, ehabkost@redhat.com, luto@amacapital.net, stefanha@gmail.com, revol@free.fr, matt@codeblueprint.co.uk, rth@twiddle.net Subject: Re: [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access Message-ID: <20160317133340.GJ12454@HEDWIG.INI.CMU.EDU> References: <20160308183050.GJ2049@HEDWIG.INI.CMU.EDU> <20160316185440-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160316185440-mutt-send-email-mst@redhat.com> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.5.24 (2015-08-30) X-PMX-Version: 6.0.3.2322014, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2016.3.17.132417 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, __SUBJ_ALPHA_END 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 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 Content-Length: 4047 Lines: 106 On Wed, Mar 16, 2016 at 06:57:01PM +0200, Michael S. Tsirkin wrote: > On Tue, Mar 08, 2016 at 01:30:50PM -0500, Gabriel Somlo wrote: > > Allowing for the future possibility of implementing AML-based > > (i.e., firmware-triggered) access to the QEMU fw_cfg device, > > acquire the global ACPI lock when accessing the device on behalf > > of the guest-side sysfs driver, to prevent any potential race > > conditions. > > > > Suggested-by: Michael S. Tsirkin > > Signed-off-by: Gabriel Somlo > > So this patch makes sense of course. > > > Given the recent discussion on QEMU mailing list, > I think there is an additional patch that we need: > filter the files exposed to userspace by "opt/" prefix. > > This will ensure that we can change all other fw cfg files > at will without breaking guest scripts. > > Gabriel, could you code this up? Or do you see a > pressing need to expose internal QEMU registers to > userspace? I'd be happy to update the docs to (better) emphasisze that: 1 the only way to guarantee any particular item shows up in guest-side fw_cfg sysfs is manually putting it there via the host-side command line - and BTW, unless you prefixed it with "opt/..." you are off the reservation, and it might collide with qemu->firmware communications. 2 anything one didn't place there themselves via the qemu command line is informational only, might change or go away at any time, and developing expectations about it based on past observation is done at the observer's own risk. While I don't *personally* care about items outside of "opt/", I'm a bit uncomfortable actively *hiding* them from userspace -- I could easily imagine the ability to see (read-only) fw_cfg content from userspace being a handy debugging/troubleshooting tool. It's back to separating between mechanism and policy: hiding things from userspace would IMHO fall into the policy enforcement side of things, and I'm still unclear about the failure scenario we'd be trying to prevent, and its likelihood. Thanks, --Gabriel > > --- > > > > Changes since v1: > > - no more "#ifdef CONFIG_ACPI"; instead we proceed if > > acpi_acquire_global_lock() returns either OK or NOT_CONFIGURED, > > and only throw a warning/error message otherwise. > > > > - didn't get any *negative* feedback from the QEMU crowd, so > > this is now a bona-fide "please apply this", rather than just > > an RFC :) > > > > - tested on ACPI-enabled x86_64, and acpi_less ARM (32 and 64 bit) > > QEMU VMs (I don't have handy access to an ACPI-enabled ARM VM) > > > > Thanks much, > > --Gabriel > > > > drivers/firmware/qemu_fw_cfg.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c > > index 7bba76c..a44dc32 100644 > > --- a/drivers/firmware/qemu_fw_cfg.c > > +++ b/drivers/firmware/qemu_fw_cfg.c > > @@ -77,12 +77,28 @@ static inline u16 fw_cfg_sel_endianness(u16 key) > > static inline void fw_cfg_read_blob(u16 key, > > void *buf, loff_t pos, size_t count) > > { > > + u32 glk; > > + acpi_status status; > > + > > + /* If we have ACPI, ensure mutual exclusion against any potential > > + * device access by the firmware, e.g. via AML methods: > > + */ > > + status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk); > > + if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) { > > + /* Should never get here */ > > + WARN(1, "fw_cfg_read_blob: Failed to lock ACPI!\n"); > > + memset(buf, 0, count); > > + return; > > + } > > + > > 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); > > + > > + acpi_release_global_lock(glk); > > } > > > > /* clean up fw_cfg device i/o */ > > -- > > 2.4.3