On Sun, Oct 04, 2015 at 04:24:00PM -0400, Gabriel L. Somlo wrote:

On Sun, Oct 04, 2015 at 10:54:57AM +0300, Michael S. Tsirkin wrote:

quoted

On Sat, Oct 03, 2015 at 07:28:07PM -0400, Gabriel L. Somlo wrote:

quoted

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@cmu.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.

I meant "wouldn't be 100% atomic", as in "it would be a case of
verify-then-use"...

Sorry,
--Gabriel

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