From: Gabriel Somlo <somlo@cmu.edu> Date: 2016-03-08 18:31:43
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(+)
@@ -77,12 +77,28 @@ static inline u16 fw_cfg_sel_endianness(u16 key)staticinlinevoidfw_cfg_read_blob(u16key,void*buf,loff_tpos,size_tcount){+u32glk;+acpi_statusstatus;++/* If we have ACPI, ensure mutual exclusion against any potential+*deviceaccessbythefirmware,e.g.viaAMLmethods:+*/+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 */
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-03-09 09:13:35
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>
quoted hunk
---
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(+)
@@ -77,12 +77,28 @@ static inline u16 fw_cfg_sel_endianness(u16 key)staticinlinevoidfw_cfg_read_blob(u16key,void*buf,loff_tpos,size_tcount){+u32glk;+acpi_statusstatus;++/* If we have ACPI, ensure mutual exclusion against any potential+*deviceaccessbythefirmware,e.g.viaAMLmethods:+*/+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 */
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-03-16 16:57:16
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 <redacted>
Signed-off-by: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
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?
quoted hunk
---
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(+)
@@ -77,12 +77,28 @@ static inline u16 fw_cfg_sel_endianness(u16 key)staticinlinevoidfw_cfg_read_blob(u16key,void*buf,loff_tpos,size_tcount){+u32glk;+acpi_statusstatus;++/* If we have ACPI, ensure mutual exclusion against any potential+*deviceaccessbythefirmware,e.g.viaAMLmethods:+*/+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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2016-03-17 13:14:09
On 16/03/2016 17:57, Michael S. Tsirkin wrote:
On Tue, Mar 08, 2016 at 01:30:50PM -0500, Gabriel Somlo wrote:
quoted
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 <redacted>
Signed-off-by: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
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
From: "Gabriel L. Somlo" <somlo@cmu.edu> Date: 2016-03-17 13:34:29
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:
quoted
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 <redacted>
Signed-off-by: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
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
quoted
---
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(+)
@@ -77,12 +77,28 @@ static inline u16 fw_cfg_sel_endianness(u16 key)staticinlinevoidfw_cfg_read_blob(u16key,void*buf,loff_tpos,size_tcount){+u32glk;+acpi_statusstatus;++/* If we have ACPI, ensure mutual exclusion against any potential+*deviceaccessbythefirmware,e.g.viaAMLmethods:+*/+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 */
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-03-17 14:03:58
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:
quoted
On Tue, Mar 08, 2016 at 01:30:50PM -0500, Gabriel Somlo wrote:
quoted
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 <redacted>
Signed-off-by: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
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.
quoted
quoted
---
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(+)
@@ -77,12 +77,28 @@ static inline u16 fw_cfg_sel_endianness(u16 key)staticinlinevoidfw_cfg_read_blob(u16key,void*buf,loff_tpos,size_tcount){+u32glk;+acpi_statusstatus;++/* If we have ACPI, ensure mutual exclusion against any potential+*deviceaccessbythefirmware,e.g.viaAMLmethods:+*/+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 */
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-04-03 12:12:46
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 <redacted>
Signed-off-by: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
Greg, could you merge this please?
I'm rather worried that this goes out without.
quoted hunk
---
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(+)
@@ -77,12 +77,28 @@ static inline u16 fw_cfg_sel_endianness(u16 key)staticinlinevoidfw_cfg_read_blob(u16key,void*buf,loff_tpos,size_tcount){+u32glk;+acpi_statusstatus;++/* If we have ACPI, ensure mutual exclusion against any potential+*deviceaccessbythefirmware,e.g.viaAMLmethods:+*/+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 */
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-04-05 08:54:36
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:
quoted
On Tue, Mar 08, 2016 at 01:30:50PM -0500, Gabriel Somlo wrote:
quoted
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 <redacted>
Signed-off-by: Gabriel Somlo <somlo-D+Gtc/HYRWM@public.gmane.org>
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.
quoted
quoted
---
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(+)
@@ -77,12 +77,28 @@ static inline u16 fw_cfg_sel_endianness(u16 key)staticinlinevoidfw_cfg_read_blob(u16key,void*buf,loff_tpos,size_tcount){+u32glk;+acpi_statusstatus;++/* If we have ACPI, ensure mutual exclusion against any potential+*deviceaccessbythefirmware,e.g.viaAMLmethods:+*/+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 */
From: "Gabriel L. Somlo" <somlo@cmu.edu> Date: 2016-04-11 13:13:56
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:
quoted
On Wed, Mar 16, 2016 at 06:57:01PM +0200, Michael S. Tsirkin wrote:
quoted
On Tue, Mar 08, 2016 at 01:30:50PM -0500, Gabriel Somlo wrote:
quoted
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.
quoted
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
quoted
quoted
quoted
---
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(+)
@@ -77,12 +77,28 @@ static inline u16 fw_cfg_sel_endianness(u16 key)staticinlinevoidfw_cfg_read_blob(u16key,void*buf,loff_tpos,size_tcount){+u32glk;+acpi_statusstatus;++/* If we have ACPI, ensure mutual exclusion against any potential+*deviceaccessbythefirmware,e.g.viaAMLmethods:+*/+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 */
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-04-11 13:48:20
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:
quoted
On Thu, Mar 17, 2016 at 09:33:40AM -0400, Gabriel L. Somlo wrote:
quoted
On Wed, Mar 16, 2016 at 06:57:01PM +0200, Michael S. Tsirkin wrote:
quoted
On Tue, Mar 08, 2016 at 01:30:50PM -0500, Gabriel Somlo wrote:
quoted
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.
quoted
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.
quoted
quoted
quoted
quoted
---
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(+)
@@ -77,12 +77,28 @@ static inline u16 fw_cfg_sel_endianness(u16 key)staticinlinevoidfw_cfg_read_blob(u16key,void*buf,loff_tpos,size_tcount){+u32glk;+acpi_statusstatus;++/* If we have ACPI, ensure mutual exclusion against any potential+*deviceaccessbythefirmware,e.g.viaAMLmethods:+*/+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 */