Re: [PATCH v3 2/4] firmware: use acpi to detect QEMU fw_cfg device for sysfs fw_cfg driver

[Gabriel L. Somlo]

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:
> > On Sat, Oct 03, 2015 at 07:28:07PM -0400, Gabriel L. Somlo wrote:
> > > 
> > > 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@xxxxxxx>
> > 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/