Date: Thu, 17 Mar 2016 09:33:40 -0400
From: "Gabriel L. Somlo" <somlo@cmu.edu>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Greg KH <gregkh@linuxfoundation.org>, 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>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@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 <mst@redhat.com>
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> 
> 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