diff mbox

[RFC] fw-cfg: support writeable blobs

Message ID 1456144729-17196-1-git-send-email-mst@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michael S. Tsirkin Feb. 22, 2016, 12:41 p.m. UTC
Useful to send guest data back to QEMU.
The write interface is restricted to DMA.

Suggested-by: Kevin O'Connor <kevin@koconnor.net>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

 hw/lm32/lm32_hwsetup.h    |  2 +-
 include/hw/loader.h       |  4 ++--
 include/hw/nvram/fw_cfg.h |  3 ++-
 hw/arm/virt-acpi-build.c  |  2 +-
 hw/core/loader.c          | 19 ++++++++++++-------
 hw/i386/acpi-build.c      |  4 ++--
 hw/nvram/fw_cfg.c         | 36 ++++++++++++++++++++++++++++--------
 7 files changed, 48 insertions(+), 22 deletions(-)

Comments

Gerd Hoffmann Feb. 22, 2016, 2:26 p.m. UTC | #1
On Mo, 2016-02-22 at 14:41 +0200, Michael S. Tsirkin wrote:
> Useful to send guest data back to QEMU.

Use case?

cheers,
  Gerd
Kevin O'Connor Feb. 22, 2016, 2:47 p.m. UTC | #2
On Mon, Feb 22, 2016 at 02:41:38PM +0200, Michael S. Tsirkin wrote:
> Useful to send guest data back to QEMU.
> The write interface is restricted to DMA.
> 
> Suggested-by: Kevin O'Connor <kevin@koconnor.net>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
>  hw/lm32/lm32_hwsetup.h    |  2 +-
>  include/hw/loader.h       |  4 ++--
>  include/hw/nvram/fw_cfg.h |  3 ++-
>  hw/arm/virt-acpi-build.c  |  2 +-
>  hw/core/loader.c          | 19 ++++++++++++-------
>  hw/i386/acpi-build.c      |  4 ++--
>  hw/nvram/fw_cfg.c         | 36 ++++++++++++++++++++++++++++--------
>  7 files changed, 48 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
> index 838754d..805b445 100644
> --- a/hw/lm32/lm32_hwsetup.h
> +++ b/hw/lm32/lm32_hwsetup.h
> @@ -74,7 +74,7 @@ static inline void hwsetup_create_rom(HWSetup *hw,
>          hwaddr base)
>  {
>      rom_add_blob("hwsetup", hw->data, TARGET_PAGE_SIZE,
> -                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL);
> +                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL, true);
>  }

Instead of supporting "writable blobs", I wonder if it would be
simpler for both firmware and qemu to implement an "ioctl" like
interface.  That is, just directly invoke a new fw_cfg callback from
fw_cfg_dma_transfer() with the data obtained via dma_memory_read().  I
think that may be more flexible because then the QEMU code doesn't
have to check for changes to a blob - it would instead be immediately
invoked upon a change.

-Kevin
Michael S. Tsirkin Feb. 22, 2016, 7:15 p.m. UTC | #3
On Mon, Feb 22, 2016 at 09:47:49AM -0500, Kevin O'Connor wrote:
> On Mon, Feb 22, 2016 at 02:41:38PM +0200, Michael S. Tsirkin wrote:
> > Useful to send guest data back to QEMU.
> > The write interface is restricted to DMA.
> > 
> > Suggested-by: Kevin O'Connor <kevin@koconnor.net>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> >  hw/lm32/lm32_hwsetup.h    |  2 +-
> >  include/hw/loader.h       |  4 ++--
> >  include/hw/nvram/fw_cfg.h |  3 ++-
> >  hw/arm/virt-acpi-build.c  |  2 +-
> >  hw/core/loader.c          | 19 ++++++++++++-------
> >  hw/i386/acpi-build.c      |  4 ++--
> >  hw/nvram/fw_cfg.c         | 36 ++++++++++++++++++++++++++++--------
> >  7 files changed, 48 insertions(+), 22 deletions(-)
> > 
> > diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
> > index 838754d..805b445 100644
> > --- a/hw/lm32/lm32_hwsetup.h
> > +++ b/hw/lm32/lm32_hwsetup.h
> > @@ -74,7 +74,7 @@ static inline void hwsetup_create_rom(HWSetup *hw,
> >          hwaddr base)
> >  {
> >      rom_add_blob("hwsetup", hw->data, TARGET_PAGE_SIZE,
> > -                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL);
> > +                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL, true);
> >  }
> 
> Instead of supporting "writable blobs", I wonder if it would be
> simpler for both firmware and qemu to implement an "ioctl" like
> interface.  That is, just directly invoke a new fw_cfg callback from
> fw_cfg_dma_transfer() with the data obtained via dma_memory_read().  I
> think that may be more flexible because then the QEMU code doesn't
> have to check for changes to a blob - it would instead be immediately
> invoked upon a change.
> 
> -Kevin

So far, I didn't see any need to invoke callbacks.
All users seem happy with simply using data
written by guest.

OTOH if there's no blob to write, users need to add code
to migrate it manually, blobs are migrated automatically.
Michael S. Tsirkin Feb. 22, 2016, 8:12 p.m. UTC | #4
On Mon, Feb 22, 2016 at 03:26:56PM +0100, Gerd Hoffmann wrote:
> On Mo, 2016-02-22 at 14:41 +0200, Michael S. Tsirkin wrote:
> > Useful to send guest data back to QEMU.
> 
> Use case?
> 
> cheers,
>   Gerd

VM GEN ID at least wants to pass address of some blob
in guest memory to host.

Apparently, that's also useful for nvdimm.

A reasonable way to do that seems to be to write it into
fw cfg file, this way it's also migrated automatically.
Gerd Hoffmann Feb. 23, 2016, 7:07 a.m. UTC | #5
On Mo, 2016-02-22 at 22:12 +0200, Michael S. Tsirkin wrote:
> On Mon, Feb 22, 2016 at 03:26:56PM +0100, Gerd Hoffmann wrote:
> > On Mo, 2016-02-22 at 14:41 +0200, Michael S. Tsirkin wrote:
> > > Useful to send guest data back to QEMU.
> > 
> > Use case?
> > 
> > cheers,
> >   Gerd
> 
> VM GEN ID at least wants to pass address of some blob
> in guest memory to host.
> 
> Apparently, that's also useful for nvdimm.
> 
> A reasonable way to do that seems to be to write it into
> fw cfg file, this way it's also migrated automatically.

Both seem to be about acpi aml code talking to qemu backend.

Not sure fw_cfg files are reasonable for that, given that you have to
fetch the directory listing, parse it to figure the entry index, ...

I want see a patch actually using that before going to merge it.

cheers,
  Gerd
Michael S. Tsirkin Feb. 23, 2016, 9:09 a.m. UTC | #6
On Tue, Feb 23, 2016 at 08:07:17AM +0100, Gerd Hoffmann wrote:
> On Mo, 2016-02-22 at 22:12 +0200, Michael S. Tsirkin wrote:
> > On Mon, Feb 22, 2016 at 03:26:56PM +0100, Gerd Hoffmann wrote:
> > > On Mo, 2016-02-22 at 14:41 +0200, Michael S. Tsirkin wrote:
> > > > Useful to send guest data back to QEMU.
> > > 
> > > Use case?
> > > 
> > > cheers,
> > >   Gerd
> > 
> > VM GEN ID at least wants to pass address of some blob
> > in guest memory to host.
> > 
> > Apparently, that's also useful for nvdimm.
> > 
> > A reasonable way to do that seems to be to write it into
> > fw cfg file, this way it's also migrated automatically.
> 
> Both seem to be about acpi aml code talking to qemu backend.

At least not for gen id - there will be a linker patch
to initialize that.

> Not sure fw_cfg files are reasonable for that, given that you have to
> fetch the directory listing, parse it to figure the entry index, ...
> 
> I want see a patch actually using that before going to merge it.
> 
> cheers,
>   Gerd

Absolutely, this makes sense.
diff mbox

Patch

diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
index 838754d..805b445 100644
--- a/hw/lm32/lm32_hwsetup.h
+++ b/hw/lm32/lm32_hwsetup.h
@@ -74,7 +74,7 @@  static inline void hwsetup_create_rom(HWSetup *hw,
         hwaddr base)
 {
     rom_add_blob("hwsetup", hw->data, TARGET_PAGE_SIZE,
-                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL);
+                 TARGET_PAGE_SIZE, base, NULL, NULL, NULL, true);
 }
 
 static inline void hwsetup_add_u8(HWSetup *hw, uint8_t u)
diff --git a/include/hw/loader.h b/include/hw/loader.h
index f7b43ab..14d99e3 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -72,7 +72,7 @@  MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                            size_t max_len, hwaddr addr,
                            const char *fw_file_name,
                            FWCfgReadCallback fw_callback,
-                           void *callback_opaque);
+                           void *callback_opaque, bool read_only);
 int rom_add_elf_program(const char *name, void *data, size_t datasize,
                         size_t romsize, hwaddr addr);
 int rom_check_and_register_reset(void);
@@ -84,7 +84,7 @@  void hmp_info_roms(Monitor *mon, const QDict *qdict);
 #define rom_add_file_fixed(_f, _a, _i)          \
     rom_add_file(_f, NULL, _a, _i, false)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
-    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL)
+    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, true)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 664eaf6..371e75e 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -173,6 +173,7 @@  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
  * @callback_opaque: argument to be passed into callback function
  * @data: pointer to start of item data
  * @len: size of item data
+ * @read_only: is file read only
  *
  * Add a new NAMED fw_cfg item as a raw "blob" of the given size. The data
  * referenced by the starting pointer is only linked, NOT copied, into the
@@ -188,7 +189,7 @@  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
  */
 void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
-                              void *data, size_t len);
+                              void *data, size_t len, bool read_only);
 
 /**
  * fw_cfg_modify_file:
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index b8b3ece..c076bbf 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -679,7 +679,7 @@  static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
                                        uint64_t max_size)
 {
     return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
-                        name, virt_acpi_build_update, build_state);
+                        name, virt_acpi_build_update, build_state, true);
 }
 
 static const VMStateDescription vmstate_virt_acpi_build = {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 3a57415..ee05c7a 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -731,7 +731,7 @@  static void fw_cfg_resized(const char *id, uint64_t length, void *host)
     }
 }
 
-static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
+static void *rom_set_mr(Rom *rom, Object *owner, const char *name, bool ro)
 {
     void *data;
 
@@ -740,7 +740,7 @@  static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
                                       rom->datasize, rom->romsize,
                                       fw_cfg_resized,
                                       &error_fatal);
-    memory_region_set_readonly(rom->mr, true);
+    memory_region_set_readonly(rom->mr, ro);
     vmstate_register_ram_global(rom->mr);
 
     data = memory_region_get_ram_ptr(rom->mr);
@@ -811,7 +811,7 @@  int rom_add_file(const char *file, const char *fw_dir,
         snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
 
         if ((!option_rom || mc->option_rom_has_mr) && mc->rom_file_has_mr) {
-            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
+            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, true);
         } else {
             data = rom->data;
         }
@@ -836,7 +836,8 @@  err:
 
 MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                    size_t max_len, hwaddr addr, const char *fw_file_name,
-                   FWCfgReadCallback fw_callback, void *callback_opaque)
+                   FWCfgReadCallback fw_callback, void *callback_opaque,
+                   bool read_only)
 {
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
     Rom *rom;
@@ -854,10 +855,14 @@  MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
         char devpath[100];
         void *data;
 
-        snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
+        if (read_only) {
+            snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
+        } else {
+            snprintf(devpath, sizeof(devpath), "/ram@%s", fw_file_name);
+        }
 
         if (mc->rom_file_has_mr) {
-            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
+            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, read_only);
             mr = rom->mr;
         } else {
             data = rom->data;
@@ -865,7 +870,7 @@  MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
 
         fw_cfg_add_file_callback(fw_cfg, fw_file_name,
                                  fw_callback, callback_opaque,
-                                 data, rom->datasize);
+                                 data, rom->datasize, read_only);
     }
     return mr;
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 52c9470..b654b0d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2791,7 +2791,7 @@  static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
                                        uint64_t max_size)
 {
     return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
-                        name, acpi_build_update, build_state);
+                        name, acpi_build_update, build_state, true);
 }
 
 static const VMStateDescription vmstate_acpi_build = {
@@ -2856,7 +2856,7 @@  void acpi_setup(void)
         build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size);
         fw_cfg_add_file_callback(pcms->fw_cfg, ACPI_BUILD_RSDP_FILE,
                                  acpi_build_update, build_state,
-                                 build_state->rsdp, rsdp_size);
+                                 build_state->rsdp, rsdp_size, true);
         build_state->rsdp_mr = NULL;
     } else {
         build_state->rsdp = NULL;
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 79c5742..b5d9577 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -53,11 +53,13 @@ 
 #define FW_CFG_DMA_CTL_READ    0x02
 #define FW_CFG_DMA_CTL_SKIP    0x04
 #define FW_CFG_DMA_CTL_SELECT  0x08
+#define FW_CFG_DMA_CTL_WRITE   0x10
 
 #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
 
 typedef struct FWCfgEntry {
     uint32_t len;
+    bool allow_write;
     uint8_t *data;
     void *callback_opaque;
     FWCfgReadCallback read_callback;
@@ -322,7 +324,7 @@  static void fw_cfg_dma_transfer(FWCfgState *s)
     FWCfgDmaAccess dma;
     int arch;
     FWCfgEntry *e;
-    int read;
+    int read = 0, write = 0;
     dma_addr_t dma_addr;
 
     /* Reset the address before the next access */
@@ -349,8 +351,13 @@  static void fw_cfg_dma_transfer(FWCfgState *s)
 
     if (dma.control & FW_CFG_DMA_CTL_READ) {
         read = 1;
+        write = 0;
+    } else if (dma.control & FW_CFG_DMA_CTL_WRITE) {
+        read = 0;
+        write = 1;
     } else if (dma.control & FW_CFG_DMA_CTL_SKIP) {
         read = 0;
+        write = 0;
     } else {
         dma.length = 0;
     }
@@ -370,7 +377,9 @@  static void fw_cfg_dma_transfer(FWCfgState *s)
                     dma.control |= FW_CFG_DMA_CTL_ERROR;
                 }
             }
-
+            if (write) {
+                dma.control |= FW_CFG_DMA_CTL_ERROR;
+            }
         } else {
             if (dma.length <= (e->len - s->cur_offset)) {
                 len = dma.length;
@@ -387,6 +396,13 @@  static void fw_cfg_dma_transfer(FWCfgState *s)
                     dma.control |= FW_CFG_DMA_CTL_ERROR;
                 }
             }
+            if (write) {
+                if (!e->allow_write ||
+                    dma_memory_read(s->dma_as, dma.address,
+                                    &e->data[s->cur_offset], len)) {
+                    dma.control |= FW_CFG_DMA_CTL_ERROR;
+                }
+            }
 
             s->cur_offset += len;
         }
@@ -582,7 +598,8 @@  static const VMStateDescription vmstate_fw_cfg = {
 static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
                                            FWCfgReadCallback callback,
                                            void *callback_opaque,
-                                           void *data, size_t len)
+                                           void *data, size_t len,
+                                           bool read_only)
 {
     int arch = !!(key & FW_CFG_ARCH_LOCAL);
 
@@ -595,6 +612,7 @@  static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
     s->entries[arch][key].len = (uint32_t)len;
     s->entries[arch][key].read_callback = callback;
     s->entries[arch][key].callback_opaque = callback_opaque;
+    s->entries[arch][key].allow_write = !read_only;
 }
 
 static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
@@ -612,13 +630,14 @@  static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = len;
     s->entries[arch][key].callback_opaque = NULL;
+    s->entries[arch][key].allow_write = false;
 
     return ptr;
 }
 
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
 {
-    fw_cfg_add_bytes_read_callback(s, key, NULL, NULL, data, len);
+    fw_cfg_add_bytes_read_callback(s, key, NULL, NULL, data, len, true);
 }
 
 void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
@@ -667,7 +686,7 @@  void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
 
 void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
-                              void *data, size_t len)
+                              void *data, size_t len, bool read_only)
 {
     int i, index;
     size_t dsize;
@@ -692,7 +711,8 @@  void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
     }
 
     fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index,
-                                   callback, callback_opaque, data, len);
+                                   callback, callback_opaque, data, len,
+                                   read_only);
 
     s->files->f[index].size   = cpu_to_be32(len);
     s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
@@ -704,7 +724,7 @@  void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
 void fw_cfg_add_file(FWCfgState *s,  const char *filename,
                      void *data, size_t len)
 {
-    fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len);
+    fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len, true);
 }
 
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
@@ -727,7 +747,7 @@  void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
         }
     }
     /* add new one */
-    fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len);
+    fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len, true);
     return NULL;
 }