Message-ID: <20151004202400.GD1977@HEDWIG.INI.CMU.EDU>
Date:	Sun, 4 Oct 2015 16:24:00 -0400
From:	"Gabriel L. Somlo" <somlo@....edu>
To:	"Michael S. Tsirkin" <mst@...hat.com>
Cc:	gregkh@...uxfoundation.org, paul@...an.com, galak@...eaurora.org,
	will.deacon@....com, agross@...eaurora.org, mark.rutland@....com,
	zajec5@...il.com, hanjun.guo@...aro.org, catalin.marinas@....com,
	linux-api@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernelnewbies@...nelnewbies.org, matt.fleming@...el.com,
	lersek@...hat.com, jordan.l.justen@...el.com,
	peter.maydell@...aro.org, leif.lindholm@...aro.org,
	ard.biesheuvel@...aro.org, pbonzini@...hat.com, kraxel@...hat.com,
	qemu-devel@...gnu.org
Subject: Re: [PATCH v3 2/4] firmware: use acpi to detect QEMU fw_cfg device
 for sysfs fw_cfg driver

On Sun, Oct 04, 2015 at 10:54:57AM +0300, Michael S. Tsirkin wrote:
> On Sat, Oct 03, 2015 at 07:28:07PM -0400, Gabriel L. Somlo wrote:
> > From: Gabriel Somlo <somlo@....edu>
> > 
> > Instead of blindly probing fw_cfg registers at known IOport and MMIO
> > locations, use the ACPI subsystem to determine whether a QEMU fw_cfg
> > device is present, and, if found, to initialize it.
> > 
> > This limits portability to architectures which support ACPI (x86 and
> > UEFI-enabled aarch64), but avoids touching hardware registers before
> > being certain that our device is present.
> > 
> > NOTE: The standard way to verify the presence of fw_cfg on arm VMs
> > would have been to use the device tree, but that would have left out
> > x86, which is the primary architecture targeted by this patch.
> > 
> > Signed-off-by: Gabriel Somlo <somlo@....edu>
> 
> IMHO it's not a good idea to probe registers provided
> by CRS like this.
> It seems quite reasonable that we'd want to add some
> extra registers in the future, and this probing will break.
> 
> Further, accessing registers directly means that there's
> no way to have ACPI code access them as that would
> cause race conditions.
> 
> Maybe we should provide access methods in ACPI instead?

OK, I think I understand what you meant by "don't poke CRS" in the
other thread...

So, you're proposing I move the follwing bits:

  /* atomic access to fw_cfg device (potentially slow i/o, so using
   * mutex) */
  static DEFINE_MUTEX(fw_cfg_dev_lock);

  /* pick appropriate endianness for selector key */
  static inline u16 fw_cfg_sel_endianness(u16 key)
  {
          return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
  }

  /* type for fw_cfg "directory scan" visitor/callback function */
  typedef int (*fw_cfg_file_callback)(const struct fw_cfg_file *f);

  /* run a given callback on each fw_cfg directory entry */
  static int fw_cfg_scan_dir(fw_cfg_file_callback callback)
  {
          int ret = 0;
          u32 count, i;
          struct fw_cfg_file f;

          mutex_lock(&fw_cfg_dev_lock);
          iowrite16(fw_cfg_sel_endianness(FW_CFG_FILE_DIR), fw_cfg_reg_ctrl);
          ioread8_rep(fw_cfg_reg_data, &count, sizeof(count));
          for (i = 0; i < be32_to_cpu(count); i++) {
                  ioread8_rep(fw_cfg_reg_data, &f, sizeof(f));
                  ret = callback(&f);
                  if (ret)
                          break;
          }
          mutex_unlock(&fw_cfg_dev_lock);
          return ret;
  }

  /* read chunk of given fw_cfg blob (caller responsible for
   * sanity-check) */
  static inline void fw_cfg_read_blob(u16 key,
                                      void *buf, loff_t pos, size_t count)
  {
          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);
  }

into the FWCF, "QEMU0002" node as an AML method ? Have ACPI provide
mutual exclusion against competing readers, and somehow figure out how
to call the ACPI/AML code from the guest-side kernel driver whenever
I need to call fw_cfg_read_blob() ?

I guess I could implement fw_cfg_scan_dir() using fw_cfg_read_blob():

  u32 count;
  size_t  bufsize;
  void *buf;
  fw_cfg_read_blob(FW_CFG_FILE_DIR, &count, 0, sizeof(u32));
  bufsize = sizeof(u32) + count * sizeof(struct fw_cfg_file);
  buf = kalloc(bufsize);
  fw_cfg_read_blob(FW_CFG_FILE_DIR, buf, 0, bufsize);
  ...
  /* now read all the blob meta-data from buf ... */

It would be 100% atomic, but since we can safely assume the fw_cfg
contents never change, it'd be OK.

The atomicity of the ACPI version of fw_cfg_read_blob(), picking the
right endianness for the selector, etc. would have to be done in AML
within the QEMU host-side patch.

If you know of anything I can look at for a good ASL example, please
point it out to me. I'm going to go away now and spend some quality
time with the ACPI spec :)

Thanks,
--Gabriel

> 
> 
> > ---
> >  .../ABI/testing/sysfs-firmware-qemu_fw_cfg         |   4 +
> >  drivers/firmware/Kconfig                           |   2 +-
> >  drivers/firmware/qemu_fw_cfg.c                     | 201 +++++++++++----------
> >  3 files changed, 113 insertions(+), 94 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-firmware-qemu_fw_cfg b/Documentation/ABI/testing/sysfs-firmware-qemu_fw_cfg
> > index f1ef44e..e9761bf 100644
> > --- a/Documentation/ABI/testing/sysfs-firmware-qemu_fw_cfg
> > +++ b/Documentation/ABI/testing/sysfs-firmware-qemu_fw_cfg
> > @@ -76,6 +76,10 @@ Description:
> >  		the port number of the control register. I.e., the two ports
> >  		are overlapping, and can not be mapped separately.
> >  
> > +		NOTE 2. QEMU publishes the register details in the device tree
> > +		on arm guests, and in ACPI (under _HID "QEMU0002") on x86 and
> > +		select arm (aarch64) VM types.
> > +
> >  		=== Firmware Configuration Items of Interest ===
> >  
> >  		Originally, the index key, size, and formatting of blobs in
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index 0466e80..bc12d31 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -137,7 +137,7 @@ config ISCSI_IBFT
> >  
> >  config FW_CFG_SYSFS
> >  	tristate "QEMU fw_cfg device support in sysfs"
> > -	depends on SYSFS
> > +	depends on SYSFS && ACPI
> >  	default n
> >  	help
> >  	  Say Y or M here to enable the exporting of the QEMU firmware
> > diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> > index 3a67a16..f935afb 100644
> > --- a/drivers/firmware/qemu_fw_cfg.c
> > +++ b/drivers/firmware/qemu_fw_cfg.c
> > @@ -8,6 +8,7 @@
> >   */
> >  
> >  #include <linux/module.h>
> > +#include <linux/acpi.h>
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> >  #include <linux/ioport.h>
> > @@ -35,53 +36,10 @@ struct fw_cfg_file {
> >  	char name[FW_CFG_MAX_FILE_PATH];
> >  };
> >  
> > -/* fw_cfg device i/o access options type */
> > -struct fw_cfg_access {
> > -	const char *name;
> > -	phys_addr_t base;
> > -	u8 size;
> > -	u8 ctrl_offset;
> > -	u8 data_offset;
> > -	bool is_mmio;
> > -};
> > -
> > -/* table of fw_cfg device i/o access options for known architectures */
> > -static struct fw_cfg_access fw_cfg_modes[] = {
> > -	{
> > -		.name = "fw_cfg IOport on i386, sun4u",
> > -		.base = 0x510,
> > -		.size = 0x02,
> > -		.ctrl_offset = 0x00,
> > -		.data_offset = 0x01,
> > -		.is_mmio = false,
> > -	}, {
> > -		.name = "fw_cfg MMIO on arm",
> > -		.base = 0x9020000,
> > -		.size = 0x0a,
> > -		.ctrl_offset = 0x08,
> > -		.data_offset = 0x00,
> > -		.is_mmio = true,
> > -	}, {
> > -		.name = "fw_cfg MMIO on sun4m",
> > -		.base = 0xd00000510,
> > -		.size = 0x03,
> > -		.ctrl_offset = 0x00,
> > -		.data_offset = 0x02,
> > -		.is_mmio = true,
> > -	}, {
> > -		.name = "fw_cfg MMIO on ppc/mac",
> > -		.base = 0xf0000510,
> > -		.size = 0x03,
> > -		.ctrl_offset = 0x00,
> > -		.data_offset = 0x02,
> > -		.is_mmio = true,
> > -	}, { } /* END */
> > -};
> > -
> > -/* fw_cfg device i/o currently selected option set */
> > -static struct fw_cfg_access *fw_cfg_mode;
> > -
> >  /* fw_cfg device i/o register addresses */
> > +static bool fw_cfg_is_mmio;
> > +static phys_addr_t fw_cfg_phys_base;
> > +static u32 fw_cfg_phys_size;
> >  static void __iomem *fw_cfg_dev_base;
> >  static void __iomem *fw_cfg_reg_ctrl;
> >  static void __iomem *fw_cfg_reg_data;
> > @@ -92,7 +50,7 @@ static DEFINE_MUTEX(fw_cfg_dev_lock);
> >  /* pick appropriate endianness for selector key */
> >  static inline u16 fw_cfg_sel_endianness(u16 key)
> >  {
> > -	return fw_cfg_mode->is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
> > +	return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
> >  }
> >  
> >  /* type for fw_cfg "directory scan" visitor/callback function */
> > @@ -133,60 +91,100 @@ static inline void fw_cfg_read_blob(u16 key,
> >  /* clean up fw_cfg device i/o */
> >  static void fw_cfg_io_cleanup(void)
> >  {
> > -	if (fw_cfg_mode->is_mmio) {
> > +	if (fw_cfg_is_mmio) {
> >  		iounmap(fw_cfg_dev_base);
> > -		release_mem_region(fw_cfg_mode->base, fw_cfg_mode->size);
> > +		release_mem_region(fw_cfg_phys_base, fw_cfg_phys_size);
> >  	} else {
> >  		ioport_unmap(fw_cfg_dev_base);
> > -		release_region(fw_cfg_mode->base, fw_cfg_mode->size);
> > +		release_region(fw_cfg_phys_base, fw_cfg_phys_size);
> >  	}
> >  }
> >  
> > -/* probe and map fw_cfg device */
> > -static int __init fw_cfg_io_probe(void)
> > +/* configure fw_cfg device i/o from ACPI _CRS method */
> > +static acpi_status fw_cfg_walk_crs(struct acpi_resource *r, void *context)
> > +{
> > +	struct acpi_resource_io *io;
> > +	struct acpi_resource_fixed_memory32 *mmio;
> > +
> > +	switch (r->type) {
> > +	case ACPI_RESOURCE_TYPE_END_TAG:
> > +		return AE_OK;
> > +	case ACPI_RESOURCE_TYPE_IO:
> > +		io = &r->data.io;
> > +		/* physical base addr should NOT be already set */
> > +		if (fw_cfg_phys_base)
> > +			return AE_ERROR;
> > +		if (!request_region(io->minimum,
> > +				    io->address_length, "fw_cfg_io"))
> > +			return AE_ERROR;
> > +		fw_cfg_dev_base = ioport_map(io->minimum, io->address_length);
> > +		if (!fw_cfg_dev_base) {
> > +			release_region(io->minimum, io->address_length);
> > +			return AE_ERROR;
> > +		}
> > +		fw_cfg_phys_base = io->minimum;
> > +		fw_cfg_phys_size = io->address_length;
> > +		fw_cfg_is_mmio = false;
> > +		/* set register addresses (pc/i386 offsets) */
> > +		fw_cfg_reg_ctrl = fw_cfg_dev_base + 0x00;
> > +		fw_cfg_reg_data = fw_cfg_dev_base + 0x01;
> > +		return AE_OK;
> > +	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> > +		mmio = &r->data.fixed_memory32;
> > +		/* physical base addr should NOT be already set */
> > +		if (fw_cfg_phys_base)
> > +			return AE_ERROR;
> > +		/* MMIO and ACPI, but not on ARM ?!?! */
> > +		if (mmio->address_length < 0x0a)
> > +			return AE_ERROR;
> > +		if (!request_mem_region(mmio->address,
> > +					mmio->address_length, "fw_cfg_mem"))
> > +			return AE_ERROR;
> > +		fw_cfg_dev_base = ioremap(mmio->address, mmio->address_length);
> > +		if (!fw_cfg_dev_base) {
> > +			release_mem_region(mmio->address, mmio->address_length);
> > +			return AE_ERROR;
> > +		}
> > +		fw_cfg_phys_base = mmio->address;
> > +		fw_cfg_phys_size = mmio->address_length;
> > +		fw_cfg_is_mmio = true;
> > +		/* set register addresses (arm offsets) */
> > +		fw_cfg_reg_ctrl = fw_cfg_dev_base + 0x08;
> > +		fw_cfg_reg_data = fw_cfg_dev_base + 0x00;
> > +		return AE_OK;
> > +	default:
> > +		return AE_ERROR;
> > +	}
> > +}
> > +
> > +/* initialize fw_cfg device i/o from ACPI data */
> > +static int fw_cfg_acpi_init(struct acpi_device *dev)
> >  {
> >  	char sig[FW_CFG_SIG_SIZE];
> > +	acpi_status status;
> > +	int err;
> >  
> > -	for (fw_cfg_mode = &fw_cfg_modes[0];
> > -	     fw_cfg_mode->base; fw_cfg_mode++) {
> > -
> > -		phys_addr_t base = fw_cfg_mode->base;
> > -		u8 size = fw_cfg_mode->size;
> > -
> > -		/* reserve and map mmio or ioport region */
> > -		if (fw_cfg_mode->is_mmio) {
> > -			if (!request_mem_region(base, size, fw_cfg_mode->name))
> > -				continue;
> > -			fw_cfg_dev_base = ioremap(base, size);
> > -			if (!fw_cfg_dev_base) {
> > -				release_mem_region(base, size);
> > -				continue;
> > -			}
> > -		} else {
> > -			if (!request_region(base, size, fw_cfg_mode->name))
> > -				continue;
> > -			fw_cfg_dev_base = ioport_map(base, size);
> > -			if (!fw_cfg_dev_base) {
> > -				release_region(base, size);
> > -				continue;
> > -			}
> > -		}
> > +	err = acpi_bus_get_status(dev);
> > +	if (err < 0)
> > +		return err;
> >  
> > -		/* set control and data register addresses */
> > -		fw_cfg_reg_ctrl = fw_cfg_dev_base + fw_cfg_mode->ctrl_offset;
> > -		fw_cfg_reg_data = fw_cfg_dev_base + fw_cfg_mode->data_offset;
> > +	if (!(dev->status.enabled && dev->status.functional))
> > +		return -ENODEV;
> >  
> > -		/* verify fw_cfg device signature */
> > -		fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE);
> > -		if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) == 0)
> > -			/* success, we're done */
> > -			return 0;
> > +	/* extract device i/o details from _CRS */
> > +	status = acpi_walk_resources(dev->handle, METHOD_NAME__CRS,
> > +				     fw_cfg_walk_crs, NULL);
> > +	if (status != AE_OK || !fw_cfg_phys_base)
> > +		return -ENODEV;
> >  
> > -		/* clean up before probing next access mode */
> > +	/* verify fw_cfg device signature */
> > +	fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE);
> > +	if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> >  		fw_cfg_io_cleanup();
> > +		return -ENODEV;
> >  	}
> >  
> > -	return -ENODEV;
> > +	return 0;
> >  }
> >  
> >  /* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
> > @@ -353,7 +351,7 @@ static struct kobject *fw_cfg_top_ko;
> >  static struct kobject *fw_cfg_sel_ko;
> >  
> >  /* callback function to register an individual fw_cfg file */
> > -static int __init fw_cfg_register_file(const struct fw_cfg_file *f)
> > +static int fw_cfg_register_file(const struct fw_cfg_file *f)
> >  {
> >  	int err;
> >  	struct fw_cfg_sysfs_entry *entry;
> > @@ -397,12 +395,12 @@ static inline void fw_cfg_kobj_cleanup(struct kobject *kobj)
> >  	kobject_put(kobj);
> >  }
> >  
> > -static int __init fw_cfg_sysfs_init(void)
> > +static int fw_cfg_sysfs_add(struct acpi_device *dev)
> >  {
> >  	int err;
> >  
> > -	/* probe for the fw_cfg "hardware" */
> > -	err = fw_cfg_io_probe();
> > +	/* initialize fw_cfg device i/o from ACPI data */
> > +	err = fw_cfg_acpi_init(dev);
> >  	if (err)
> >  		return err;
> >  
> > @@ -443,14 +441,31 @@ err_top:
> >  	return err;
> >  }
> >  
> > -static void __exit fw_cfg_sysfs_exit(void)
> > +static int fw_cfg_sysfs_remove(struct acpi_device *dev)
> >  {
> >  	pr_debug("fw_cfg: unloading.\n");
> >  	fw_cfg_sysfs_cache_cleanup();
> >  	fw_cfg_kobj_cleanup(fw_cfg_sel_ko);
> >  	fw_cfg_kobj_cleanup(fw_cfg_top_ko);
> >  	fw_cfg_io_cleanup();
> > +	return 0;
> >  }
> >  
> > -module_init(fw_cfg_sysfs_init);
> > -module_exit(fw_cfg_sysfs_exit);
> > +static const struct acpi_device_id fw_cfg_sysfs_device_ids[] = {
> > +	{ "QEMU0002", 0 },
> > +	{ "", 0 },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, fw_cfg_sysfs_device_ids);
> > +
> > +static struct acpi_driver fw_cfg_sysfs_driver = {
> > +	.name =		"fw_cfg",
> > +	.class =	"QEMU",
> > +	.ids =		fw_cfg_sysfs_device_ids,
> > +	.ops =		{
> > +				.add =		fw_cfg_sysfs_add,
> > +				.remove =	fw_cfg_sysfs_remove,
> > +			},
> > +	.owner =	THIS_MODULE,
> > +};
> > +
> > +module_acpi_driver(fw_cfg_sysfs_driver);
> > -- 
> > 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists