Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sun, 2013-01-27 at 18:53 -0600, Anthony Liguori wrote: Are you just trying to persist a single blob of a fixed maximum size? That would suffice. Why not just have a second flash device then? Mostly because flash devices don't actually *work* with KVM. Should I be looking at fixing *that*, instead? -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
David Woodhouse dw...@infradead.org writes: On Sun, 2013-01-27 at 18:53 -0600, Anthony Liguori wrote: Are you just trying to persist a single blob of a fixed maximum size? That would suffice. Why not just have a second flash device then? Mostly because flash devices don't actually *work* with KVM. They absolutely do. What doesn't work is executing ROM from flash if the ROM cannot be treated as read-only memory. That's because all we get is a PF in the kernel when trying to execute from unmapped ROM. There's no way to turn that into MMIO to userspace without switching to running fully in emulation mode. The x86 emulator is pretty close to complete but work would be needed to fully complete it to make this work. We normally handle this by mapping the ROM memory read-only so it can be executed without PF'ing but since the BIOS area is subject to PAM, we can't use this trick for that particular ROM. SeaBIOS has hack to just not use PAM to do BIOS shadowing when running under KVM/QEMU but presumably OVMF lacks this. But in this case, you're using the flash device purely for read/write, not for execution, so there's no limitation at all. Regards, Anthony Liguori Should I be looking at fixing *that*, instead? -- dwmw2
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Mon, 2013-01-28 at 10:10 -0600, Anthony Liguori wrote: Mostly because flash devices don't actually *work* with KVM. They absolutely do. What doesn't work is executing ROM from flash if the ROM cannot be treated as read-only memory. Ah, great. In that case, that seems like the best approach. It should make it easier to integrate into OVMF too, because it'll just be using the 'standard' flash storage code with a different physical location. Hopefully. Can we get away with exposing an additional flash device in the physical memory map immediately below the BIOS ROM and giving the firmware a way to discover its location? Or do we need to do it cleanly by making it a BAR of a PCI device? -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Mon, Jan 28, 2013 at 8:10 AM, Anthony Liguori anth...@codemonkey.ws wrote: David Woodhouse dw...@infradead.org writes: On Sun, 2013-01-27 at 18:53 -0600, Anthony Liguori wrote: Are you just trying to persist a single blob of a fixed maximum size? That would suffice. Why not just have a second flash device then? Mostly because flash devices don't actually *work* with KVM. They absolutely do. What doesn't work is executing ROM from flash if the ROM cannot be treated as read-only memory. What is need is for pflash_cfi01 to start in plain rom/executable mode while firmware executes from it during early boot. Then later, after the rom has been shadowed, firmware will want to write to that memory space to program it. At that point it no longer needs to be executable. So the question is, can it start out in rom/executable mode, but change into a non-executable mode if a write occurs? Will qemu get a chance to respond if something is written to a rom region, or is it silently ignored? Also, once the 'read-array' command is written to it after programming is finished, can it revert to executable rom mode? -Jordan That's because all we get is a PF in the kernel when trying to execute from unmapped ROM. There's no way to turn that into MMIO to userspace without switching to running fully in emulation mode. The x86 emulator is pretty close to complete but work would be needed to fully complete it to make this work. We normally handle this by mapping the ROM memory read-only so it can be executed without PF'ing but since the BIOS area is subject to PAM, we can't use this trick for that particular ROM. SeaBIOS has hack to just not use PAM to do BIOS shadowing when running under KVM/QEMU but presumably OVMF lacks this. But in this case, you're using the flash device purely for read/write, not for execution, so there's no limitation at all.
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Mon, 2013-01-28 at 08:36 -0800, Jordan Justen wrote: What is need is for pflash_cfi01 to start in plain rom/executable mode while firmware executes from it during early boot. Then later, after the rom has been shadowed, firmware will want to write to that memory space to program it. At that point it no longer needs to be executable. So the question is, can it start out in rom/executable mode, but change into a non-executable mode if a write occurs? Will qemu get a chance to respond if something is written to a rom region, or is it silently ignored? Also, once the 'read-array' command is written to it after programming is finished, can it revert to executable rom mode? We often have separate gating in hardware to enable the write line (or Vpp) to flash chips. Can we emulate that and use it to switch the flash between executable and MMIO mode? Rather than being able to trap the first write and see what it was... -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
Jordan Justen jljus...@gmail.com writes: On Mon, Jan 28, 2013 at 8:10 AM, Anthony Liguori anth...@codemonkey.ws wrote: David Woodhouse dw...@infradead.org writes: On Sun, 2013-01-27 at 18:53 -0600, Anthony Liguori wrote: Are you just trying to persist a single blob of a fixed maximum size? That would suffice. Why not just have a second flash device then? Mostly because flash devices don't actually *work* with KVM. They absolutely do. What doesn't work is executing ROM from flash if the ROM cannot be treated as read-only memory. What is need is for pflash_cfi01 to start in plain rom/executable mode while firmware executes from it during early boot. Then later, after the rom has been shadowed, firmware will want to write to that memory space to program it. At that point it no longer needs to be executable. So the question is, can it start out in rom/executable mode, but change into a non-executable mode if a write occurs? It's a matter of how you setup the memory region but yes, I see no problem with this although it may need to be a qdev option. chance to respond if something is written to a rom region, or is it silently ignored? Also, once the 'read-array' command is written to it after programming is finished, can it revert to executable rom mode? Reverting to executable means remapping the region as read-only memory verses MMIO. So yes, it's technically possible. I suspect that you want to make this behavior a flag for the device though. Regards, Anthony Liguori -Jordan That's because all we get is a PF in the kernel when trying to execute from unmapped ROM. There's no way to turn that into MMIO to userspace without switching to running fully in emulation mode. The x86 emulator is pretty close to complete but work would be needed to fully complete it to make this work. We normally handle this by mapping the ROM memory read-only so it can be executed without PF'ing but since the BIOS area is subject to PAM, we can't use this trick for that particular ROM. SeaBIOS has hack to just not use PAM to do BIOS shadowing when running under KVM/QEMU but presumably OVMF lacks this. But in this case, you're using the flash device purely for read/write, not for execution, so there's no limitation at all.
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Mon, Jan 28, 2013 at 10:10:06AM -0600, Anthony Liguori wrote: David Woodhouse dw...@infradead.org writes: On Sun, 2013-01-27 at 18:53 -0600, Anthony Liguori wrote: Are you just trying to persist a single blob of a fixed maximum size? That would suffice. Why not just have a second flash device then? Mostly because flash devices don't actually *work* with KVM. They absolutely do. What doesn't work is executing ROM from flash if the ROM cannot be treated as read-only memory. That's because all we get is a PF in the kernel when trying to execute from unmapped ROM. There's no way to turn that into MMIO to userspace without switching to running fully in emulation mode. The x86 emulator is pretty close to complete but work would be needed to fully complete it to make this work. The x86 emulator cannot fetch from MMIO memory today. And protected mode emulation is not so complete. We rarely, if ever, need to emulate protected mode instructions without memory operands. We normally handle this by mapping the ROM memory read-only so it can be executed without PF'ing but since the BIOS area is subject to PAM, we can't use this trick for that particular ROM. Up until kernel 3.7 there was no support for read-only memory slots. As far as I know QEMU still maps ROM as regular RAM to KVM. SeaBIOS has hack to just not use PAM to do BIOS shadowing when running under KVM/QEMU but presumably OVMF lacks this. Not sure that such hack exists and why is it needed. BIOS area is always writable in KVM. But in this case, you're using the flash device purely for read/write, not for execution, so there's no limitation at all. Yes, on newer kernel we can use read-only slots to emulate flash. They behaves like ROMD: memory on read MMIO on write. On older kernels we can use pure MMIO. -- Gleb.
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
Gleb Natapov g...@redhat.com writes:
On Mon, Jan 28, 2013 at 10:10:06AM -0600, Anthony Liguori wrote:
David Woodhouse dw...@infradead.org writes:
On Sun, 2013-01-27 at 18:53 -0600, Anthony Liguori wrote:
Are you just trying to persist a single blob of a fixed maximum size?
That would suffice.
Why not just have a second flash device then?
Mostly because flash devices don't actually *work* with KVM.
They absolutely do. What doesn't work is executing ROM from flash if
the ROM cannot be treated as read-only memory.
That's because all we get is a PF in the kernel when trying to execute
from unmapped ROM. There's no way to turn that into MMIO to userspace
without switching to running fully in emulation mode. The x86 emulator
is pretty close to complete but work would be needed to fully complete
it to make this work.
The x86 emulator cannot fetch from MMIO memory today. And protected mode
emulation is not so complete. We rarely, if ever, need to emulate
protected mode instructions without memory operands.
Ack.
We normally handle this by mapping the ROM memory read-only so it can be
executed without PF'ing but since the BIOS area is subject to PAM, we
can't use this trick for that particular ROM.
Up until kernel 3.7 there was no support for read-only memory slots. As
far as I know QEMU still maps ROM as regular RAM to KVM.
Ack.
SeaBIOS has hack to just not use PAM to do BIOS shadowing when running
under KVM/QEMU but presumably OVMF lacks this.
Not sure that such hack exists and why is it needed. BIOS area is always
writable in KVM.
shadow.c:
static void
make_bios_writable_intel(u16 bdf, u32 pam0)
{
int reg = pci_config_readb(bdf, pam0);
if (!(reg 0x10)) {
// QEMU doesn't fully implement the piix shadow capabilities -
// if ram isn't backing the bios segment when shadowing is
// disabled, the code itself wont be in memory. So, run the
// code from the high-memory flash location.
Normally when shadowing, you set the PAM registers to send read requests
to ROM and write requests to RAM. You then read the code segment (that
you're executing from) and write that out to RAM and switch to executing
from there.
That's just not possible without treating ROMs as MMIO and sending all
requests down to QEMU.
But in this case, you're using the flash device purely for read/write,
not for execution, so there's no limitation at all.
Yes, on newer kernel we can use read-only slots to emulate flash. They
behaves like ROMD: memory on read MMIO on write. On older kernels we can
use pure MMIO.
Ack.
Regards,
Anthony Liguori
--
Gleb.
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Mon, Jan 28, 2013 at 12:48:43PM -0600, Anthony Liguori wrote:
Gleb Natapov g...@redhat.com writes:
On Mon, Jan 28, 2013 at 10:10:06AM -0600, Anthony Liguori wrote:
David Woodhouse dw...@infradead.org writes:
On Sun, 2013-01-27 at 18:53 -0600, Anthony Liguori wrote:
Are you just trying to persist a single blob of a fixed maximum size?
That would suffice.
Why not just have a second flash device then?
Mostly because flash devices don't actually *work* with KVM.
They absolutely do. What doesn't work is executing ROM from flash if
the ROM cannot be treated as read-only memory.
That's because all we get is a PF in the kernel when trying to execute
from unmapped ROM. There's no way to turn that into MMIO to userspace
without switching to running fully in emulation mode. The x86 emulator
is pretty close to complete but work would be needed to fully complete
it to make this work.
The x86 emulator cannot fetch from MMIO memory today. And protected mode
emulation is not so complete. We rarely, if ever, need to emulate
protected mode instructions without memory operands.
Ack.
We normally handle this by mapping the ROM memory read-only so it can be
executed without PF'ing but since the BIOS area is subject to PAM, we
can't use this trick for that particular ROM.
Up until kernel 3.7 there was no support for read-only memory slots. As
far as I know QEMU still maps ROM as regular RAM to KVM.
Ack.
SeaBIOS has hack to just not use PAM to do BIOS shadowing when running
under KVM/QEMU but presumably OVMF lacks this.
Not sure that such hack exists and why is it needed. BIOS area is always
writable in KVM.
shadow.c:
static void
make_bios_writable_intel(u16 bdf, u32 pam0)
{
int reg = pci_config_readb(bdf, pam0);
if (!(reg 0x10)) {
// QEMU doesn't fully implement the piix shadow capabilities -
// if ram isn't backing the bios segment when shadowing is
// disabled, the code itself wont be in memory. So, run the
// code from the high-memory flash location.
Normally when shadowing, you set the PAM registers to send read requests
to ROM and write requests to RAM. You then read the code segment (that
you're executing from) and write that out to RAM and switch to executing
from there.
That's just not possible without treating ROMs as MMIO and sending all
requests down to QEMU.
Ack.
But in this case, you're using the flash device purely for read/write,
not for execution, so there's no limitation at all.
Yes, on newer kernel we can use read-only slots to emulate flash. They
behaves like ROMD: memory on read MMIO on write. On older kernels we can
use pure MMIO.
Ack.
Regards,
Anthony Liguori
--
Gleb.
--
Gleb.
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sat, Jan 26, 2013 at 8:43 PM, David Woodhouse dw...@infradead.org wrote:
For OVMF we really want to have a way to store non-volatile variables,
other than the dirty hack that currently puts them on a file in the EFI
system partition.
It looks like we've supported writing to fw_cfg items fairly much since
they were introduced, but we've never actually made use of that.
This WIP patch kills the existing fw_cfg_add_callback() because I can't
see how it would ever be useful, and it doesn't seem to have been used
for years, if ever. And adds something slightly more useful.
Other then the obvious bits that need finishing, any objections?
It looks like this duplicates rom_add_file() and fw_cfg_add_file(), so
I don't see the point.
Removing unused fw_cfg_add_callback() should be a separate patch and
that would be OK.
In general, QEMU uses different coding style from kernel, so please
read our CODING_STYLE and use checkpatch.pl to catch obvious problems,
like missing braces and C99 comments.
diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 3b31d77..1318a2e 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -33,6 +33,9 @@
#define FW_CFG_SIZE 2
#define FW_CFG_DATA_SIZE 1
+struct FWCfgEntry;
+typedef void (*FWCfgCallback)(struct FWCfgState *s, struct FWCfgEntry *e,
int value);
+
typedef struct FWCfgEntry {
uint32_t len;
uint8_t *data;
@@ -206,20 +209,19 @@ static void fw_cfg_write(FWCfgState *s, uint8_t value)
trace_fw_cfg_write(s, value);
-if (s-cur_entry FW_CFG_WRITE_CHANNEL e-callback
-s-cur_offset e-len) {
-e-data[s-cur_offset++] = value;
-if (s-cur_offset == e-len) {
-e-callback(e-callback_opaque, e-data);
-s-cur_offset = 0;
-}
-}
+if (s-cur_entry FW_CFG_WRITE_CHANNEL e-callback)
+e-callback(s, e, value);
}
static int fw_cfg_select(FWCfgState *s, uint16_t key)
{
+int arch = !!(s-cur_entry FW_CFG_ARCH_LOCAL);
+FWCfgEntry *e = s-entries[arch][s-cur_entry FW_CFG_ENTRY_MASK];
int ret;
+if (e-callback)
+e-callback(s, e, -1);
+
s-cur_offset = 0;
if ((key FW_CFG_ENTRY_MASK) = FW_CFG_MAX_ENTRY) {
s-cur_entry = FW_CFG_INVALID;
@@ -419,8 +421,8 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t
value)
fw_cfg_add_bytes(s, key, copy, sizeof(value));
}
-void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
- void *callback_opaque, void *data, size_t len)
+static void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback
callback,
+void *callback_opaque, void *data, size_t
len)
{
int arch = !!(key FW_CFG_ARCH_LOCAL);
@@ -436,8 +438,9 @@ void fw_cfg_add_callback(FWCfgState *s, uint16_t key,
FWCfgCallback callback,
s-entries[arch][key].callback = callback;
}
-void fw_cfg_add_file(FWCfgState *s, const char *filename,
- void *data, size_t len)
+static void fw_cfg_add_file_writeable(FWCfgState *s, const char *filename,
+ void *data, size_t len,
+ FWCfgCallback callback, void *cb_opaque)
{
int i, index;
size_t dsize;
@@ -451,7 +454,8 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename,
index = be32_to_cpu(s-files-count);
assert(index FW_CFG_FILE_SLOTS);
-fw_cfg_add_bytes(s, FW_CFG_FILE_FIRST + index, data, len);
+fw_cfg_add_callback(s, FW_CFG_WRITE_CHANNEL + FW_CFG_FILE_FIRST + index,
+callback, cb_opaque, data, len);
pstrcpy(s-files-f[index].name, sizeof(s-files-f[index].name),
filename);
@@ -464,11 +468,74 @@ void fw_cfg_add_file(FWCfgState *s, const char
*filename,
s-files-f[index].size = cpu_to_be32(len);
s-files-f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
+if (callback)
+ s-files-f[index].select |= cpu_to_be16(FW_CFG_WRITE_CHANNEL);
trace_fw_cfg_add_file(s, index, s-files-f[index].name, len);
s-files-count = cpu_to_be32(index+1);
}
+void fw_cfg_add_file(FWCfgState *s, const char *filename,
+ void *data, size_t len)
+{
+fw_cfg_add_file_writeable(s, filename, data, len, NULL, NULL);
+}
+
+// Lifted from pc.c
+static long get_file_size(FILE *f)
+{
+long where, size;
+
+/* XXX: on Unix systems, using fstat() probably makes more sense */
+
+where = ftell(f);
+fseek(f, 0, SEEK_END);
+size = ftell(f);
+fseek(f, where, SEEK_SET);
+
+return size;
+}
+
+static void nvstorage_callback(struct FWCfgState *s, struct FWCfgEntry *e,
int value)
+
+{
+if (value == -1) {
+FILE *f = fopen(e-callback_opaque, wb);
+if (f) {
+fwrite(e-data, e-len, 1, f);
+fclose(f);
+}
+ return;
+}
+if (s-cur_offset ==
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sun, 2013-01-27 at 15:14 +, Blue Swirl wrote: It looks like this duplicates rom_add_file() and fw_cfg_add_file(), so I don't see the point. Both of those are read-only, surely? The firmware inside the guest can't use them for non-volatile storage. It doesn't duplicate fw_cfg_add_file(); it extends it to allow a writeable mode too. fw_cfg_add_file() becomes a simple wrapper around fw_cfg_add_file_writeable(), which actually contains most of the contents of the original function. -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sun, Jan 27, 2013 at 3:50 PM, David Woodhouse dw...@infradead.org wrote: On Sun, 2013-01-27 at 15:14 +, Blue Swirl wrote: It looks like this duplicates rom_add_file() and fw_cfg_add_file(), so I don't see the point. Both of those are read-only, surely? The firmware inside the guest can't use them for non-volatile storage. Right. But why do we need PV non-volatile storage, instead of normal flash? It doesn't duplicate fw_cfg_add_file(); it extends it to allow a writeable mode too. fw_cfg_add_file() becomes a simple wrapper around fw_cfg_add_file_writeable(), which actually contains most of the contents of the original function. OK, however most of the loading logic is now in loader.c and that should not be duplicated either. -- dwmw2
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
David Woodhouse dw...@infradead.org writes:
For OVMF we really want to have a way to store non-volatile variables,
other than the dirty hack that currently puts them on a file in the EFI
system partition.
It looks like we've supported writing to fw_cfg items fairly much since
they were introduced, but we've never actually made use of that.
This WIP patch kills the existing fw_cfg_add_callback() because I can't
see how it would ever be useful, and it doesn't seem to have been used
for years, if ever. And adds something slightly more useful.
Other then the obvious bits that need finishing, any objections?
The main issue is malicious guests. The administrator has to be aware
of and in control of how much disk space the guest can use. The
secondary issue is how to tie into the block layer so things like live
migration work.
This is why use a flash device is an attractive answer here because it
gives a fixed sized storage pool that's configurable by the
administrator. Since it's backed by a block device, it supports all of
the features needed for non-volatile storage (snapshotting, live copy,
etc.).
The only real downside is that it's opaque to the host. If it's
desirable to have something that's visible to the host, then I think we
would still need to make use of BlockDriverState as the means to make it
non-volatile.
That essentially involves writing a file system in QEMU on top of
BlockDriverState and then having a PV inteface with the guest to get/set
file content. Would be useful to have, but so far no one has cared
enough about making these stores non-opaque to do it.
Regards,
Anthony Liguori
diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 3b31d77..1318a2e 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -33,6 +33,9 @@
#define FW_CFG_SIZE 2
#define FW_CFG_DATA_SIZE 1
+struct FWCfgEntry;
+typedef void (*FWCfgCallback)(struct FWCfgState *s, struct FWCfgEntry *e,
int value);
+
typedef struct FWCfgEntry {
uint32_t len;
uint8_t *data;
@@ -206,20 +209,19 @@ static void fw_cfg_write(FWCfgState *s, uint8_t value)
trace_fw_cfg_write(s, value);
-if (s-cur_entry FW_CFG_WRITE_CHANNEL e-callback
-s-cur_offset e-len) {
-e-data[s-cur_offset++] = value;
-if (s-cur_offset == e-len) {
-e-callback(e-callback_opaque, e-data);
-s-cur_offset = 0;
-}
-}
+if (s-cur_entry FW_CFG_WRITE_CHANNEL e-callback)
+e-callback(s, e, value);
}
static int fw_cfg_select(FWCfgState *s, uint16_t key)
{
+int arch = !!(s-cur_entry FW_CFG_ARCH_LOCAL);
+FWCfgEntry *e = s-entries[arch][s-cur_entry FW_CFG_ENTRY_MASK];
int ret;
+if (e-callback)
+e-callback(s, e, -1);
+
s-cur_offset = 0;
if ((key FW_CFG_ENTRY_MASK) = FW_CFG_MAX_ENTRY) {
s-cur_entry = FW_CFG_INVALID;
@@ -419,8 +421,8 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t
value)
fw_cfg_add_bytes(s, key, copy, sizeof(value));
}
-void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
- void *callback_opaque, void *data, size_t len)
+static void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback
callback,
+void *callback_opaque, void *data, size_t
len)
{
int arch = !!(key FW_CFG_ARCH_LOCAL);
@@ -436,8 +438,9 @@ void fw_cfg_add_callback(FWCfgState *s, uint16_t key,
FWCfgCallback callback,
s-entries[arch][key].callback = callback;
}
-void fw_cfg_add_file(FWCfgState *s, const char *filename,
- void *data, size_t len)
+static void fw_cfg_add_file_writeable(FWCfgState *s, const char *filename,
+ void *data, size_t len,
+ FWCfgCallback callback, void *cb_opaque)
{
int i, index;
size_t dsize;
@@ -451,7 +454,8 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename,
index = be32_to_cpu(s-files-count);
assert(index FW_CFG_FILE_SLOTS);
-fw_cfg_add_bytes(s, FW_CFG_FILE_FIRST + index, data, len);
+fw_cfg_add_callback(s, FW_CFG_WRITE_CHANNEL + FW_CFG_FILE_FIRST + index,
+callback, cb_opaque, data, len);
pstrcpy(s-files-f[index].name, sizeof(s-files-f[index].name),
filename);
@@ -464,11 +468,74 @@ void fw_cfg_add_file(FWCfgState *s, const char
*filename,
s-files-f[index].size = cpu_to_be32(len);
s-files-f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
+if (callback)
+ s-files-f[index].select |= cpu_to_be16(FW_CFG_WRITE_CHANNEL);
trace_fw_cfg_add_file(s, index, s-files-f[index].name, len);
s-files-count = cpu_to_be32(index+1);
}
+void fw_cfg_add_file(FWCfgState *s, const char *filename,
+ void *data, size_t len)
+{
+fw_cfg_add_file_writeable(s, filename, data, len,
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sun, 2013-01-27 at 16:02 +, Blue Swirl wrote: On Sun, Jan 27, 2013 at 3:50 PM, David Woodhouse dw...@infradead.org wrote: On Sun, 2013-01-27 at 15:14 +, Blue Swirl wrote: It looks like this duplicates rom_add_file() and fw_cfg_add_file(), so I don't see the point. Both of those are read-only, surely? The firmware inside the guest can't use them for non-volatile storage. Right. But why do we need PV non-volatile storage, instead of normal flash? People have looked at adding normal flash to the PC target, but it has a number of problems. Firstly, it doesn't even *work* with KVM enabled. But even if we were to somehow fix that, the simple approach of emulating a BIOS flash is also very suboptimal; it puts the whole of the firmware *into* the flash and lets it store its non-volatile variables in the same emulated flash chip. So if you want to update the firmware (which would normally be just a case of updating the bios.bin file), you end up having to do each guest system individually *and* the update is painful because you have to preserve the non-volatile storage while updating the other parts. It's much better to have a separate storage for the non-volatile variables. Yes, we *could* teach it to use a dedicated flash chip for that rather than using flash for the firmware *and* storage as we would on real hardware. But if we're going to do something different for the virtualised case, a writeable fw_cfg file seems a whole lot easier and saner. It doesn't duplicate fw_cfg_add_file(); it extends it to allow a writeable mode too. fw_cfg_add_file() becomes a simple wrapper around fw_cfg_add_file_writeable(), which actually contains most of the contents of the original function. OK, however most of the loading logic is now in loader.c and that should not be duplicated either. I actually got file_get_size() from pc.c, and had marked it with a horrid C99 comment so we couldn't miss the duplication. I'll sort out that kind of detail later, if we can reach consensus on the basic approach. -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sun, 2013-01-27 at 10:29 -0600, Anthony Liguori wrote: David Woodhouse dw...@infradead.org writes: For OVMF we really want to have a way to store non-volatile variables, other than the dirty hack that currently puts them on a file in the EFI system partition. It looks like we've supported writing to fw_cfg items fairly much since they were introduced, but we've never actually made use of that. This WIP patch kills the existing fw_cfg_add_callback() because I can't see how it would ever be useful, and it doesn't seem to have been used for years, if ever. And adds something slightly more useful. Other then the obvious bits that need finishing, any objections? The main issue is malicious guests. The administrator has to be aware of and in control of how much disk space the guest can use. The secondary issue is how to tie into the block layer so things like live migration work. This is why use a flash device is an attractive answer here because it gives a fixed sized storage pool that's configurable by the administrator. That part is fixable simply by imposing a maximum size, surely? Since it's backed by a block device, it supports all of the features needed for non-volatile storage (snapshotting, live copy, etc.). The only real downside is that it's opaque to the host. If it's desirable to have something that's visible to the host, then I think we would still need to make use of BlockDriverState as the means to make it non-volatile. I don't really care about it being visible to the host. The important thing is that it's easily usable from the guest firmware at runtime, and not on a device that the OS might be trying to use. Their current trick is a file on the EFI system partition, which is definitely *not* acceptable. It doesn't work after the OS has booted, so even basic tasks like install an OS and hope the installer can set the boot variables to boot into it are broken. For exposing it to the guest, writeable fw_cfg definitely seems like the most attractive answer. I'll look at backing it with BlockDriverState. I don't even think we need a file system on it; it can just be a fixed-size device exposed as-is, surely? -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sun, Jan 27, 2013 at 4:38 PM, David Woodhouse dw...@infradead.org wrote: On Sun, 2013-01-27 at 16:02 +, Blue Swirl wrote: On Sun, Jan 27, 2013 at 3:50 PM, David Woodhouse dw...@infradead.org wrote: On Sun, 2013-01-27 at 15:14 +, Blue Swirl wrote: It looks like this duplicates rom_add_file() and fw_cfg_add_file(), so I don't see the point. Both of those are read-only, surely? The firmware inside the guest can't use them for non-volatile storage. Right. But why do we need PV non-volatile storage, instead of normal flash? People have looked at adding normal flash to the PC target, but it has a number of problems. Firstly, it doesn't even *work* with KVM enabled. But even if we were to somehow fix that, the simple approach of emulating a BIOS flash is also very suboptimal; it puts the whole of the firmware *into* the flash and lets it store its non-volatile variables in the same emulated flash chip. So if you want to update the firmware (which would normally be just a case of updating the bios.bin file), you end up having to do each guest system individually *and* the update is painful because you have to preserve the non-volatile storage while updating the other parts. That problem could be easily solved by allowing a combination of two images with different RO/RW settings, for example -bios bios.bin[,offset=0,ro] -bios flash.img, offset=0x8000,rw. It's much better to have a separate storage for the non-volatile variables. Yes, we *could* teach it to use a dedicated flash chip for that rather than using flash for the firmware *and* storage as we would on real hardware. But if we're going to do something different for the virtualised case, a writeable fw_cfg file seems a whole lot easier and saner. It doesn't duplicate fw_cfg_add_file(); it extends it to allow a writeable mode too. fw_cfg_add_file() becomes a simple wrapper around fw_cfg_add_file_writeable(), which actually contains most of the contents of the original function. OK, however most of the loading logic is now in loader.c and that should not be duplicated either. I actually got file_get_size() from pc.c, and had marked it with a horrid C99 comment so we couldn't miss the duplication. I'll sort out that kind of detail later, if we can reach consensus on the basic approach. So the duplication already exists. :-( -- dwmw2
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sun, 2013-01-27 at 16:55 +, Blue Swirl wrote: That problem could be easily solved by allowing a combination of two images with different RO/RW settings, for example -bios bios.bin[,offset=0,ro] -bios flash.img, offset=0x8000,rw. /me shudders at the idea of co-ordinating that with the range of the flash chip that the latest build of the firmware actually wants to use for its non-volatile storage. Seriously, keep them separate. -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sun, 2013-01-27 at 16:59 +, Blue Swirl wrote: For example hw/spapr_nvram.c implements a similar device. If the user does not specify any backing file for nvram, its contents will not be saved. I'll look at that; thanks. -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sun, Jan 27, 2013 at 4:47 PM, David Woodhouse dw...@infradead.org wrote: On Sun, 2013-01-27 at 10:29 -0600, Anthony Liguori wrote: David Woodhouse dw...@infradead.org writes: For OVMF we really want to have a way to store non-volatile variables, other than the dirty hack that currently puts them on a file in the EFI system partition. It looks like we've supported writing to fw_cfg items fairly much since they were introduced, but we've never actually made use of that. This WIP patch kills the existing fw_cfg_add_callback() because I can't see how it would ever be useful, and it doesn't seem to have been used for years, if ever. And adds something slightly more useful. Other then the obvious bits that need finishing, any objections? The main issue is malicious guests. The administrator has to be aware of and in control of how much disk space the guest can use. The secondary issue is how to tie into the block layer so things like live migration work. This is why use a flash device is an attractive answer here because it gives a fixed sized storage pool that's configurable by the administrator. That part is fixable simply by imposing a maximum size, surely? Since it's backed by a block device, it supports all of the features needed for non-volatile storage (snapshotting, live copy, etc.). The only real downside is that it's opaque to the host. If it's desirable to have something that's visible to the host, then I think we would still need to make use of BlockDriverState as the means to make it non-volatile. I don't really care about it being visible to the host. The important thing is that it's easily usable from the guest firmware at runtime, and not on a device that the OS might be trying to use. Their current trick is a file on the EFI system partition, which is definitely *not* acceptable. It doesn't work after the OS has booted, so even basic tasks like install an OS and hope the installer can set the boot variables to boot into it are broken. For exposing it to the guest, writeable fw_cfg definitely seems like the most attractive answer. I'll look at backing it with BlockDriverState. I don't even think we need a file system on it; it can just be a fixed-size device exposed as-is, surely? For example hw/spapr_nvram.c implements a similar device. If the user does not specify any backing file for nvram, its contents will not be saved. -- dwmw2
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
On Sun, Jan 27, 2013 at 8:38 AM, David Woodhouse dw...@infradead.org wrote: On Sun, 2013-01-27 at 16:02 +, Blue Swirl wrote: On Sun, Jan 27, 2013 at 3:50 PM, David Woodhouse dw...@infradead.org wrote: On Sun, 2013-01-27 at 15:14 +, Blue Swirl wrote: It looks like this duplicates rom_add_file() and fw_cfg_add_file(), so I don't see the point. Both of those are read-only, surely? The firmware inside the guest can't use them for non-volatile storage. Right. But why do we need PV non-volatile storage, instead of normal flash? People have looked at adding normal flash to the PC target, FWIW, I do have non-volatile variables working for OVMF using http://wiki.qemu.org/Features/PC_System_Flash but it has a number of problems. Firstly, it doesn't even *work* with KVM enabled. Avi tells me KVM_MEM_READONLY may be a big step in solving this. But, I'll admit that I've not made much progress on this part of the task. But even if we were to somehow fix that, the simple approach of emulating a BIOS flash is also very suboptimal; it puts the whole of the firmware *into* the flash and lets it store its non-volatile variables in the same emulated flash chip. So if you want to update the firmware (which would normally be just a case of updating the bios.bin file), you end up having to do each guest system individually *and* the update is painful because you have to preserve the non-volatile storage while updating the other parts. It is a good point, and I originally thought of keeping them separate, but it seems like we ended up heading down the PC System Flash path. I think at one point being able to keep the firmware image static when QEMU updated might have been seen as an advantage. Based on several years experience, I think OVMF builds have continued to work well with older and newer versions of QEMU. Yet, for the user that just wants to have the latest firmware, well we all probably know how much fun bios upgrades can be. :) Anyway, I think OVMF probably will end up supporting PC System Flash for non-volatile variables in addition to what ever comes of this discussion. It's much better to have a separate storage for the non-volatile variables. Yes, we *could* teach it to use a dedicated flash chip for that rather than using flash for the firmware *and* storage as we would on real hardware. But if we're going to do something different for the virtualised case, a writeable fw_cfg file seems a whole lot easier and saner. I just wish fw_cfg had a more flash-friendly interface. The EDK II variable storage prefers random access to the non-volatile storage bytes. The infrastructure generally is built around random writes and block erases. -Jordan
Re: [Qemu-devel] [RFC] Writeable files in fw_cfg
David Woodhouse dw...@infradead.org writes: On Sun, 2013-01-27 at 10:29 -0600, Anthony Liguori wrote: David Woodhouse dw...@infradead.org writes: For OVMF we really want to have a way to store non-volatile variables, other than the dirty hack that currently puts them on a file in the EFI system partition. It looks like we've supported writing to fw_cfg items fairly much since they were introduced, but we've never actually made use of that. This WIP patch kills the existing fw_cfg_add_callback() because I can't see how it would ever be useful, and it doesn't seem to have been used for years, if ever. And adds something slightly more useful. Other then the obvious bits that need finishing, any objections? The main issue is malicious guests. The administrator has to be aware of and in control of how much disk space the guest can use. The secondary issue is how to tie into the block layer so things like live migration work. This is why use a flash device is an attractive answer here because it gives a fixed sized storage pool that's configurable by the administrator. That part is fixable simply by imposing a maximum size, surely? Are you just trying to persist a single blob of a fixed maximum size? Why not just have a second flash device then? Since it's backed by a block device, it supports all of the features needed for non-volatile storage (snapshotting, live copy, etc.). The only real downside is that it's opaque to the host. If it's desirable to have something that's visible to the host, then I think we would still need to make use of BlockDriverState as the means to make it non-volatile. I don't really care about it being visible to the host. The important thing is that it's easily usable from the guest firmware at runtime, and not on a device that the OS might be trying to use. Their current trick is a file on the EFI system partition, which is definitely *not* acceptable. It doesn't work after the OS has booted, so even basic tasks like install an OS and hope the installer can set the boot variables to boot into it are broken. For exposing it to the guest, writeable fw_cfg definitely seems like the most attractive answer. I'll look at backing it with BlockDriverState. I don't even think we need a file system on it; it can just be a fixed-size device exposed as-is, surely? If you just want to write out a blob, I'm not sure why you prefer to use fw_cfg vs. a flash interface. Regards, Anthony Liguori -- dwmw2
