On Tue, Feb 23, 2016 at 07:07:36AM +0200, Michael S. Tsirkin wrote:

On Mon, Feb 22, 2016 at 03:26:23PM -0500, Gabriel L. Somlo wrote:

quoted

On Mon, Feb 22, 2016 at 10:14:50PM +0200, Michael S. Tsirkin wrote:

quoted

On Sun, Feb 21, 2016 at 08:06:17AM -0500, Gabriel L. Somlo wrote:

quoted

quoted

quoted

+static void fw_cfg_io_cleanup(void)
+{
+	if (fw_cfg_is_mmio) {
+		iounmap(fw_cfg_dev_base);
+		release_mem_region(fw_cfg_p_base, fw_cfg_p_size);
+	} else {
+		ioport_unmap(fw_cfg_dev_base);
+		release_region(fw_cfg_p_base, fw_cfg_p_size);
+	}
+}
+
+/* arch-specific ctrl & data register offsets are not available in ACPI, DT */

So for all arches which support ACPI, I think this driver
should just rely on ACPI.

There was a discussion about that a few versions ago, and IIRC the
conclusion was not to expect the firmware to contend for fw_cfg access
after the guest kernel boots:

https://lkml.org/lkml/2015/10/5/283

So it looks like NVDIMM at least wants to pass label data to guest -
for which fw cfg might be a reasonable choice.

I suspect things changed - fw cfg used to be very slow but we now have
DMA interface which makes it useful for a range of applications.

Comment on this? I'm really worried we'll release linux
without a way to access fw cfg from aml.
How about taking acpi lock around all accesses?

You mean something like this (haven't tried compiling it yet, so it
might be a bit more complicated, but just for the purpose of this
conversation):

diff --git a/drivers/firmware/qemu_fw_cfg.c

b/drivers/firmware/qemu_fw_cfg.c
index fedbff5..3462a2c 100644

--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c

@@ -77,12 +77,18 @@ 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)
 {
+#ifdef CONFIG_ACPI
+       acpi_os_acquire_mutex(acpi_gbl_osi_mutex, ACPI_WAIT_FOREVER);
+#endif
        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);
+#ifdef CONFIG_ACPI
+       acpi_os_release_mutex(acpi_gbl_osi_mutex);
+#endif
 }
 
 /* clean up fw_cfg device i/o */

I wouldn't particularly *mind* doing that, but I'd still like to hear
from other QEMU devs on whether it's really necessary.

quoted

quoted

quoted

(I even had a prototype version doing what you suggested, but per the above
reference decided to drop it -- which IMHO is for the better, since otherwise
I'd have had to ifdef between ACPI and non-ACPI versions of the driver --
see https://lkml.org/lkml/2015/11/4/534 )

I'm not sure why you have these ifdefs - they are on the host, are they
not?

Think of those as "pseudocode" ifdefs, they're there to distinguish
between AML that would be generated on MMIO vs. IOPORT systems
(specifically, arm vs. x86, respectively)

Some of the AML is the same, but obviously the _CRS, and
OperationRegion + Field are different, and I wanted to point that out
somehow :)

Cheers,
--Gabriel

You can do ifs as well.

Yeah, but the AML is generated from arch-specific locations in QEMU,
so we'd be doing MMIO-only from e.g. hw/arm/virt-acpi-build.c, and
IOPORT-only from hw/i386/acpi-build.c, etc. I wouldnt need to write a
generic AML blob with 'if' statements and insert it the same way on
all architectures, or would I ? Not sure what the best practice would
be for that :)

Speaking of AML, if we were to implement a "RDBL" (read-blob) method
for fw_cfg in AML, and call it from the guest-side kernel module,
we'll never be able to make it use DMA on ACPI systems. The way
fw_cfg_read_blob is written now, we could patch that in at some later
point. So that's an argument in favor of *at most* wrapping
acpi_os_acquire_mutex() around the current fw_cfg_read_blob, rather
than including an acpi-specific version implemented on top of an
AML call.

Thanks,
--Gabriel