[Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Gabriel Somlo]

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>
---

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

Re: [Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Michael S. Tsirkin]

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>

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> ---
> 
> 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

Re: [Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Michael S. Tsirkin]

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?

> ---
> 
> 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

Re: [Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Paolo Bonzini]

On 16/03/2016 17:57, 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.

That makes no sense, all other fw_cfg files are firmware ABI so we
cannot change them anyway.

Paolo

Re: [Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Gabriel L. Somlo]

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

Re: [Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Michael S. Tsirkin]

On Thu, Mar 17, 2016 at 09:33:40AM -0400, Gabriel L. Somlo wrote:
> 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

We are changing QEMU design right now.  Let's converge on that first.


> > > ---
> > > 
> > > 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

Re: [Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Michael S. Tsirkin]

On Thu, Mar 17, 2016 at 09:33:40AM -0400, Gabriel L. Somlo wrote:
> 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:

Well my experience shows people do not read the docs.
And really, good interfaces should be self-documenting.

>         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

Mostly, we can change internal qemu/firmware interfaces
as long as we verify that firmware that ships with QEMU
does not rely on them.

I'm fine with exposing stuff for debugging purposes
but I would like a cleaner separation between the two,
and self-documenting interfaces.
How about:
	- place everything that is under "opt/" in e.g. "supported"
	  directory, or at root
	- place everything that is not under "opt/" in e.g. "unsupported"
	  directory

Abstracting hardware is what OS is all about, this is not policy.

> > > ---
> > > 
> > > 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

Re: [Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Gabriel L. Somlo]

On Tue, Apr 05, 2016 at 11:54:19AM +0300, Michael S. Tsirkin wrote:
> On Thu, Mar 17, 2016 at 09:33:40AM -0400, Gabriel L. Somlo wrote:
> > 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:
> 
> Well my experience shows people do not read the docs.
> And really, good interfaces should be self-documenting.
> 
> >         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
> 
> Mostly, we can change internal qemu/firmware interfaces
> as long as we verify that firmware that ships with QEMU
> does not rely on them.
> 
> I'm fine with exposing stuff for debugging purposes
> but I would like a cleaner separation between the two,
> and self-documenting interfaces.
> How about:
> 	- place everything that is under "opt/" in e.g. "supported"
> 	  directory, or at root
> 	- place everything that is not under "opt/" in e.g. "unsupported"
> 	  directory
> 
> Abstracting hardware is what OS is all about, this is not policy.

I'm not sure I agree with this last point:

Arguably, fw_cfg could be viewed as a glorified out-of-band USB stick
with special files prepared by QEMU for the guest VM.

The sysfs driver is a mechanism to list/access these files, and is
IMHO the only thing one can reasonably construe as "kernel interface".

What you're suggesting boils down to adding a translation layer between
what QEMU names the files when preparing this magic USB stick, and what
we tell users the names are (by adding additional folders named e.g.
"supported" and "unsupported").

That to me looks like injecting policy ("look *here*, NOT there!") by
doing this, instead of sticking with mechanism only ("here's what qemu
wrote to fw_cfg, look at it if you want, or don't...").

While I understand your concerns, I'm not sure we should have to go
through this level of convolution to protect people from their own
mistakes (such as assuming certain content on the magic USB stick will
always be there, and writing some sort of script which would break if
said content mysteriously disappears, then reasonably complain about
either the sysfs kernel driver or qemu itself). Presence or absence of
some file on a magic USB stick does NOT (again, IMHO) an interface make...

I was thinking of maybe adding a module parameter, let's call it
"show-all", off by default, and which would cause the "by-name" folder
to only be populated by things starting with "opt" when off, and
all fw-cfg files when enabled. But I'm not sure I like having to do it
in the first place (particularly hardcoding the string "opt" anywhere
in the driver :) ) so let me think about this a bit more (additional
pro/con thoughs and opinions welcome!)...

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

Re: [Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Michael S. Tsirkin]

On Mon, Apr 11, 2016 at 09:13:00AM -0400, Gabriel L. Somlo wrote:
> On Tue, Apr 05, 2016 at 11:54:19AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Mar 17, 2016 at 09:33:40AM -0400, Gabriel L. Somlo wrote:
> > > 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:
> > 
> > Well my experience shows people do not read the docs.
> > And really, good interfaces should be self-documenting.
> > 
> > >         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
> > 
> > Mostly, we can change internal qemu/firmware interfaces
> > as long as we verify that firmware that ships with QEMU
> > does not rely on them.
> > 
> > I'm fine with exposing stuff for debugging purposes
> > but I would like a cleaner separation between the two,
> > and self-documenting interfaces.
> > How about:
> > 	- place everything that is under "opt/" in e.g. "supported"
> > 	  directory, or at root
> > 	- place everything that is not under "opt/" in e.g. "unsupported"
> > 	  directory
> > 
> > Abstracting hardware is what OS is all about, this is not policy.
> 
> I'm not sure I agree with this last point:
> 
> Arguably, fw_cfg could be viewed as a glorified out-of-band USB stick
> with special files prepared by QEMU for the guest VM.
> 
> The sysfs driver is a mechanism to list/access these files, and is
> IMHO the only thing one can reasonably construe as "kernel interface".
> 
> What you're suggesting boils down to adding a translation layer between
> what QEMU names the files when preparing this magic USB stick, and what
> we tell users the names are (by adding additional folders named e.g.
> "supported" and "unsupported").
> 
> That to me looks like injecting policy ("look *here*, NOT there!") by
> doing this, instead of sticking with mechanism only ("here's what qemu
> wrote to fw_cfg, look at it if you want, or don't...").

It's a mechanism really. We have a mechanism to affect the
names of files in guest.  What I'm saying is it would be nice to
have a mechanism for QEMU to tell guest "hidden file".

Consider that ACPI has a "hidden" attribute for devices.
This is more of the same.


> While I understand your concerns, I'm not sure we should have to go
> through this level of convolution to protect people from their own
> mistakes (such as assuming certain content on the magic USB stick will
> always be there, and writing some sort of script which would break if
> said content mysteriously disappears, then reasonably complain about
> either the sysfs kernel driver or qemu itself). Presence or absence of
> some file on a magic USB stick does NOT (again, IMHO) an interface make...
> 
> I was thinking of maybe adding a module parameter, let's call it
> "show-all", off by default, and which would cause the "by-name" folder
> to only be populated by things starting with "opt" when off, and
> all fw-cfg files when enabled. But I'm not sure I like having to do it
> in the first place (particularly hardcoding the string "opt" anywhere
> in the driver :) ) so let me think about this a bit more (additional
> pro/con thoughs and opinions welcome!)...
> 
> Thanks,
> --Gabriel

the "opt/" string is part of hardware - you already hardcore
e.g. the acpi ID, correct? That's just more of the same IMHO.


> > 
> > > > > ---
> > > > > 
> > > > > 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

Re: [Qemu-devel] [PATCH v2] firmware: qemu_fw_cfg.c: hold ACPI global lock during device access

[Michael S. Tsirkin]

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>

Greg, could you merge this please?
I'm rather worried that this goes out without.


> ---
> 
> 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