* [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI
@ 2016-12-01 17:06 Laszlo Ersek
2016-12-01 17:06 ` [Qemu-arm] [PATCH v4 1/7] fw-cfg: support writeable blobs Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Laszlo Ersek @ 2016-12-01 17:06 UTC (permalink / raw)
To: qemu devel list
Cc: Peter Maydell, Eduardo Habkost, Michael S. Tsirkin,
Gabriel L. Somlo, Shannon Zhao, Michael Walle, qemu-arm,
Gerd Hoffmann, Paolo Bonzini, Igor Mammedov
* This is version 4 of the series; the last version was at
<http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03582.html>.
This version is practically a rewrite from scratch, seeking to address
the v3 feedback. Here's what the individual patches do:
- Patch #1 rebases and extends Michael's "writeable fw_cfg blobs"
patch from Feb/March 2016. The changes relative to the original
patch are documented in the commit message (the code changes are
minimal).
- Patches #2 and #3 turn the FW_CFG_FILE_SLOTS constant into a device
property, and expose the desired number of fw_cfg file slots to
board code. This is meant to address a concern raised by Paolo in
the v2 review
<http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03125.html>,
namely that we're about to run out of FW_CFG_FILE_SLOTS.
- Patch #4 introduces the "pc-q35-2.9" and "pc-i440fx-2.9" machine
types, which are allowed to take advantage of the new default for
fw_cfg file slots (0x20 rather than 0x10).
- Patch #5 introduces SMI feature negotiation via fw_cfg, with the
following new files:
- etc/smi/host-features
- etc/smi/guest-features
- etc/smi/features-ok
This is supposed to follow Michael's recommendation re: imitating
virtio
<http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03218.html>.
The guest-features file is freely writeable by the guest (no write
callbacks in fw_cfg), and the feature validation & lockdown occurs
when the guest selects the features-ok file (for reading).
Board code is allowed to choose the host feature bitmap to
advertise.
This patch doesn't add any specific SMI features yet.
- Patch #6 adds the ICH9_LPC_SMI_F_BROADCAST feature.
- Patch #7 introduces the PCMachineClass.get_smi_host_features method,
and implements it for "pc-q35-2.9" and later. The idea is to tie the
SMI host features to machine types, and let the machine types
calculate their host features with code (i.e., not just a constant).
In this patch, the "pc-q35-2.9" machine type exposes the
ICH9_LPC_SMI_F_BROADCAST feature iff (smp_cpus == max_cpus), that
is, when VCPU hotplug is not possible. (Also from Paolo's v3
feedback, in
<http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg04919.html>.)
* I've written the OVMF side patches too and tested them together
(including gdb / debug messages for "white box" testing).
* Note that this version depends on the following PULL req from Michael:
[Qemu-devel] [PULL 0/5] virtio, vhost, pc: fixes
http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05503.html
In particular on the following patch:
"loader: fix handling of custom address spaces when adding ROM blobs"
Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Michael Walle <michael@walle.cc>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: qemu-arm@nongnu.org
Thanks
Laszlo
Laszlo Ersek (6):
fw-cfg: turn FW_CFG_FILE_SLOTS into a device property
fw-cfg: expose "file_slots" parameter in fw_cfg_init_io_dma()
hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots
hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg
hw/isa/lpc_ich9: add broadcast SMI feature
hw/i386/pc_q35: advertise broadcast SMI if VCPU hotplug is turned off
Michael S. Tsirkin (1):
fw-cfg: support writeable blobs
docs/specs/fw_cfg.txt | 36 +++++++++---
hw/lm32/lm32_hwsetup.h | 2 +-
include/hw/compat.h | 11 ++++
include/hw/i386/ich9.h | 15 ++++-
include/hw/i386/pc.h | 4 ++
include/hw/loader.h | 7 ++-
include/hw/nvram/fw_cfg.h | 5 +-
include/hw/nvram/fw_cfg_keys.h | 7 ++-
hw/arm/virt-acpi-build.c | 2 +-
hw/core/loader.c | 18 +++---
hw/i386/acpi-build.c | 4 +-
hw/i386/pc.c | 3 +-
hw/i386/pc_piix.c | 16 +++++-
hw/i386/pc_q35.c | 37 ++++++++++++-
hw/isa/lpc_ich9.c | 92 +++++++++++++++++++++++++++++-
hw/nvram/fw_cfg.c | 123 +++++++++++++++++++++++++++++++++++------
16 files changed, 326 insertions(+), 56 deletions(-)
--
2.9.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-arm] [PATCH v4 1/7] fw-cfg: support writeable blobs
2016-12-01 17:06 [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI Laszlo Ersek
@ 2016-12-01 17:06 ` Laszlo Ersek
2016-12-20 15:58 ` [Qemu-arm] [Qemu-devel] " Marcel Apfelbaum
2016-12-20 23:01 ` [Qemu-arm] [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI no-reply
2017-01-10 15:06 ` [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI Michael S. Tsirkin
2 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2016-12-01 17:06 UTC (permalink / raw)
To: qemu devel list
Cc: Peter Maydell, Michael S. Tsirkin, Gabriel L. Somlo, Shannon Zhao,
Michael Walle, qemu-arm, Gerd Hoffmann, Paolo Bonzini,
Igor Mammedov
From: "Michael S. Tsirkin" <mst@redhat.com>
Useful to send guest data back to QEMU.
Changes from Laszlo Ersek <lersek@redhat.com>:
- rebase the patch from Michael Tsirkin's original postings at [1] and [2]
to the following patches:
- loader: Allow a custom AddressSpace when loading ROMs
- loader: Add AddressSpace loading support to uImages
- loader: fix handling of custom address spaces when adding ROM blobs
- reject such writes immediately that would exceed the end of the array,
rather than performing a partial write before setting the error bit: see
the (len != dma.length) condition
- document the write interface
[1] http://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg04968.html
[2] http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg02735.html
Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Michael Walle <michael@walle.cc>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Cc: qemu-arm@nongnu.org
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
docs/specs/fw_cfg.txt | 32 +++++++++++++++++++++++++-------
hw/lm32/lm32_hwsetup.h | 2 +-
include/hw/loader.h | 7 ++++---
include/hw/nvram/fw_cfg.h | 3 ++-
hw/arm/virt-acpi-build.c | 2 +-
hw/core/loader.c | 18 +++++++++++-------
hw/i386/acpi-build.c | 4 ++--
hw/nvram/fw_cfg.c | 37 +++++++++++++++++++++++++++++--------
8 files changed, 75 insertions(+), 30 deletions(-)
diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 7a5f8c7824ce..a19e2adbe1c6 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -31,21 +31,25 @@ register. In other words, configuration write mode is enabled when
the selector value is between 0x4000-0x7fff or 0xc000-0xffff.
NOTE: As of QEMU v2.4, writes to the fw_cfg data register are no
longer supported, and will be ignored (treated as no-ops)!
+NOTE: As of QEMU v2.9, writes are reinstated, but only through the DMA
+ interface (see below). Furthermore, writeability of any specific item is
+ governed independently of Bit14 in the selector key value.
+
Bit15 of the selector register indicates whether the configuration
setting is architecture specific. A value of 0 means the item is a
generic configuration item. A value of 1 means the item is specific
to a particular architecture. In other words, generic configuration
items are accessed with a selector value between 0x0000-0x7fff, and
architecture specific configuration items are accessed with a selector
value between 0x8000-0xffff.
== Data Register ==
-* Read/Write (writes ignored as of QEMU v2.4)
+* Read/Write (writes ignored as of QEMU v2.4, but see the DMA interface)
* Location: platform dependent (IOport [*] or MMIO)
* Width: 8-bit (if IOport), 8/16/32/64-bit (if MMIO)
* Endianness: string-preserving
[*] On platforms where the data register is exposed as an IOport, its
@@ -132,23 +136,25 @@ struct FWCfgFile { /* an individual file entry, 64 bytes total */
char name[56]; /* fw_cfg item name, NUL-terminated ascii */
};
=== All Other Data Items ===
-Please consult the QEMU source for the most up-to-date and authoritative
-list of selector keys and their respective items' purpose and format.
+Please consult the QEMU source for the most up-to-date and authoritative list
+of selector keys and their respective items' purpose, format and writeability.
=== Ranges ===
Theoretically, there may be up to 0x4000 generic firmware configuration
items, and up to 0x4000 architecturally specific ones.
Selector Reg. Range Usage
--------------- -----------
-0x0000 - 0x3fff Generic (0x0000 - 0x3fff, RO)
+0x0000 - 0x3fff Generic (0x0000 - 0x3fff, generally RO, possibly RW through
+ the DMA interface in QEMU v2.9+)
0x4000 - 0x7fff Generic (0x0000 - 0x3fff, RW, ignored in QEMU v2.4+)
-0x8000 - 0xbfff Arch. Specific (0x0000 - 0x3fff, RO)
+0x8000 - 0xbfff Arch. Specific (0x0000 - 0x3fff, generally RO, possibly RW
+ through the DMA interface in QEMU v2.9+)
0xc000 - 0xffff Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
In practice, the number of allowed firmware configuration items is given
by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
@@ -180,21 +186,31 @@ address is the "control" field.
The "control" field has the following bits:
- Bit 0: Error
- Bit 1: Read
- Bit 2: Skip
- Bit 3: Select. The upper 16 bits are the selected index.
+ - Bit 4: Write
When an operation is triggered, if the "control" field has bit 3 set, the
upper 16 bits are interpreted as an index of a firmware configuration item.
This has the same effect as writing the selector register.
If the "control" field has bit 1 set, a read operation will be performed.
"length" bytes for the current selector and offset will be copied into the
physical RAM address specified by the "address" field.
-If the "control" field has bit 2 set (and not bit 1), a skip operation will be
-performed. The offset for the current selector will be advanced "length" bytes.
+If the "control" field has bit 4 set (and not bit 1), a write operation will be
+performed. "length" bytes will be copied from the physical RAM address
+specified by the "address" field to the current selector and offset. QEMU
+prevents starting or finishing the write beyond the end of the item associated
+with the current selector (i.e., the item cannot be resized). Truncated writes
+are dropped entirely. Writes to read-only items are also rejected. All of these
+write errors set bit 0 (the error bit) in the "control" field.
+
+If the "control" field has bit 2 set (and neither bit 1 nor bit 4), a skip
+operation will be performed. The offset for the current selector will be
+advanced "length" bytes.
To check the result, read the "control" field:
error bit set -> something went wrong.
all bits cleared -> transfer finished successfully.
otherwise -> transfer still in progress (doesn't happen
@@ -232,5 +248,7 @@ For historical reasons, "opt/ovmf/" is reserved for OVMF firmware.
Prefix "opt/org.qemu/" is reserved for QEMU itself.
Use of names not beginning with "opt/" is potentially dangerous and
entirely unsupported. QEMU will warn if you try.
+
+All externally provided fw_cfg items are read-only to the guest.
diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
index 23e18784dffd..a01f6bc5dfeb 100644
--- a/hw/lm32/lm32_hwsetup.h
+++ b/hw/lm32/lm32_hwsetup.h
@@ -73,11 +73,11 @@ static inline void hwsetup_free(HWSetup *hw)
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, NULL);
+ TARGET_PAGE_SIZE, base, NULL, NULL, NULL, NULL, true);
}
static inline void hwsetup_add_u8(HWSetup *hw, uint8_t u)
{
stb_p(hw->ptr, u);
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 0c864cfd6046..0dbd8d6bf37a 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -178,11 +178,12 @@ int rom_add_file(const char *file, const char *fw_dir,
bool option_rom, MemoryRegion *mr, AddressSpace *as);
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, AddressSpace *as);
+ void *callback_opaque, AddressSpace *as,
+ bool read_only);
int rom_add_elf_program(const char *name, void *data, size_t datasize,
size_t romsize, hwaddr addr, AddressSpace *as);
int rom_check_and_register_reset(void);
void rom_set_fw(FWCfgState *f);
void rom_set_order_override(int order);
@@ -192,19 +193,19 @@ void *rom_ptr(hwaddr addr);
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, NULL, NULL)
#define rom_add_blob_fixed(_f, _b, _l, _a) \
- rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL)
+ rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL, true)
#define rom_add_file_mr(_f, _mr, _i) \
rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
#define rom_add_file_as(_f, _as, _i) \
rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
#define rom_add_file_fixed_as(_f, _a, _i, _as) \
rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
#define rom_add_blob_fixed_as(_f, _b, _l, _a, _as) \
- rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as)
+ rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
#define PC_ROM_MIN_VGA 0xc0000
#define PC_ROM_MIN_OPTION 0xc8000
#define PC_ROM_MAX 0xe0000
#define PC_ROM_ALIGN 0x800
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 5c27a1f0d50b..b980cbaebf43 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -134,10 +134,11 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
* @filename: name of new fw_cfg file item
* @callback: callback function
* @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
* data structure of the fw_cfg device.
* The next available (unused) selector key starting at FW_CFG_FILE_FIRST
@@ -149,11 +150,11 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
* the fw_cfg control register, or passed to QEMU in FWCfgDmaAccess.control
* with FW_CFG_DMA_CTL_SELECT).
*/
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:
* @s: fw_cfg device being modified
* @filename: name of new fw_cfg file item
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index d4160dfa7d34..542fb67239db 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -807,11 +807,11 @@ static void virt_acpi_build_reset(void *build_opaque)
static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
GArray *blob, const char *name,
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, NULL);
+ name, virt_acpi_build_update, build_state, NULL, true);
}
static const VMStateDescription vmstate_virt_acpi_build = {
.name = "virt_acpi_build",
.version_id = 1,
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 45742494e6fd..ee5abd6eb7f4 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -851,20 +851,20 @@ static void fw_cfg_resized(const char *id, uint64_t length, void *host)
if (fw_cfg) {
fw_cfg_modify_file(fw_cfg, id + strlen("/rom@"), host, length);
}
}
-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;
rom->mr = g_malloc(sizeof(*rom->mr));
memory_region_init_resizeable_ram(rom->mr, owner, 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);
memcpy(data, rom->data, rom->datasize);
@@ -940,11 +940,11 @@ int rom_add_file(const char *file, const char *fw_dir,
snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
basename);
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;
}
fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
@@ -977,11 +977,11 @@ 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,
- AddressSpace *as)
+ AddressSpace *as, bool read_only)
{
MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
Rom *rom;
MemoryRegion *mr = NULL;
@@ -996,22 +996,26 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
rom_insert(rom);
if (fw_file_name && fw_cfg) {
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;
}
fw_cfg_add_file_callback(fw_cfg, fw_file_name,
fw_callback, callback_opaque,
- data, rom->datasize);
+ data, rom->datasize, read_only);
}
return mr;
}
/* This function is specific for elf program because we don't need to allocate
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9708cdc463df..965cb4cd4c51 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2934,11 +2934,11 @@ static void acpi_build_reset(void *build_opaque)
static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
GArray *blob, const char *name,
uint64_t max_size)
{
return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
- name, acpi_build_update, build_state, NULL);
+ name, acpi_build_update, build_state, NULL, true);
}
static const VMStateDescription vmstate_acpi_build = {
.name = "acpi_build",
.version_id = 1,
@@ -3000,11 +3000,11 @@ void acpi_setup(void)
uint32_t rsdp_size = acpi_data_len(tables.rsdp);
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;
build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
ACPI_BUILD_RSDP_FILE, 0);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 3ebecb22606a..e0145c11a19b 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -52,15 +52,17 @@
/* FW_CFG_DMA_CONTROL bits */
#define FW_CFG_DMA_CTL_ERROR 0x01
#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;
} FWCfgEntry;
@@ -324,11 +326,11 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
{
dma_addr_t len;
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 */
dma_addr = s->dma_addr;
s->dma_addr = 0;
@@ -351,12 +353,17 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
&s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
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;
}
dma.control = 0;
@@ -372,11 +379,13 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
if (read) {
if (dma_memory_set(s->dma_as, dma.address, 0, len)) {
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;
} else {
len = (e->len - s->cur_offset);
@@ -389,10 +398,18 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
if (dma_memory_write(s->dma_as, dma.address,
&e->data[s->cur_offset], len)) {
dma.control |= FW_CFG_DMA_CTL_ERROR;
}
}
+ if (write) {
+ if (!e->allow_write ||
+ len != dma.length ||
+ 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;
}
dma.address += len;
@@ -584,11 +601,12 @@ 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);
key &= FW_CFG_ENTRY_MASK;
@@ -597,10 +615,11 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
s->entries[arch][key].data = data;
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,
void *data, size_t len)
{
@@ -614,17 +633,18 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
/* return the old data to the function caller, avoid memory leak */
ptr = s->entries[arch][key].data;
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)
{
size_t sz = strlen(value) + 1;
@@ -747,11 +767,11 @@ static int get_fw_cfg_order(FWCfgState *s, const char *name)
return FW_CFG_ORDER_OVERRIDE_LAST;
}
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, count;
size_t dsize;
MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
int order = 0;
@@ -809,11 +829,12 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
exit(1);
}
}
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);
s->entry_order[index] = order;
trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
@@ -822,11 +843,11 @@ 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,
void *data, size_t len)
{
@@ -845,11 +866,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
s->files->f[i].size = cpu_to_be32(len);
return ptr;
}
}
/* 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;
}
static void fw_cfg_machine_reset(void *opaque)
{
--
2.9.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] [Qemu-devel] [PATCH v4 1/7] fw-cfg: support writeable blobs
2016-12-01 17:06 ` [Qemu-arm] [PATCH v4 1/7] fw-cfg: support writeable blobs Laszlo Ersek
@ 2016-12-20 15:58 ` Marcel Apfelbaum
2017-01-10 13:33 ` Laszlo Ersek
0 siblings, 1 reply; 17+ messages in thread
From: Marcel Apfelbaum @ 2016-12-20 15:58 UTC (permalink / raw)
To: Laszlo Ersek, qemu devel list
Cc: Peter Maydell, Michael S. Tsirkin, Gabriel L. Somlo,
Michael Walle, qemu-arm, Gerd Hoffmann, Shannon Zhao,
Paolo Bonzini, Igor Mammedov
On 12/01/2016 07:06 PM, Laszlo Ersek wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
>
> Useful to send guest data back to QEMU.
>
> Changes from Laszlo Ersek <lersek@redhat.com>:
> - rebase the patch from Michael Tsirkin's original postings at [1] and [2]
> to the following patches:
> - loader: Allow a custom AddressSpace when loading ROMs
> - loader: Add AddressSpace loading support to uImages
> - loader: fix handling of custom address spaces when adding ROM blobs
> - reject such writes immediately that would exceed the end of the array,
> rather than performing a partial write before setting the error bit: see
> the (len != dma.length) condition
> - document the write interface
>
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg04968.html
> [2] http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg02735.html
>
> Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> docs/specs/fw_cfg.txt | 32 +++++++++++++++++++++++++-------
> hw/lm32/lm32_hwsetup.h | 2 +-
> include/hw/loader.h | 7 ++++---
> include/hw/nvram/fw_cfg.h | 3 ++-
> hw/arm/virt-acpi-build.c | 2 +-
> hw/core/loader.c | 18 +++++++++++-------
> hw/i386/acpi-build.c | 4 ++--
> hw/nvram/fw_cfg.c | 37 +++++++++++++++++++++++++++++--------
> 8 files changed, 75 insertions(+), 30 deletions(-)
>
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 7a5f8c7824ce..a19e2adbe1c6 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -31,21 +31,25 @@ register. In other words, configuration write mode is enabled when
> the selector value is between 0x4000-0x7fff or 0xc000-0xffff.
>
> NOTE: As of QEMU v2.4, writes to the fw_cfg data register are no
> longer supported, and will be ignored (treated as no-ops)!
>
> +NOTE: As of QEMU v2.9, writes are reinstated, but only through the DMA
> + interface (see below). Furthermore, writeability of any specific item is
> + governed independently of Bit14 in the selector key value.
> +
> Bit15 of the selector register indicates whether the configuration
> setting is architecture specific. A value of 0 means the item is a
> generic configuration item. A value of 1 means the item is specific
> to a particular architecture. In other words, generic configuration
> items are accessed with a selector value between 0x0000-0x7fff, and
> architecture specific configuration items are accessed with a selector
> value between 0x8000-0xffff.
>
> == Data Register ==
>
> -* Read/Write (writes ignored as of QEMU v2.4)
> +* Read/Write (writes ignored as of QEMU v2.4, but see the DMA interface)
> * Location: platform dependent (IOport [*] or MMIO)
> * Width: 8-bit (if IOport), 8/16/32/64-bit (if MMIO)
> * Endianness: string-preserving
>
> [*] On platforms where the data register is exposed as an IOport, its
> @@ -132,23 +136,25 @@ struct FWCfgFile { /* an individual file entry, 64 bytes total */
> char name[56]; /* fw_cfg item name, NUL-terminated ascii */
> };
>
> === All Other Data Items ===
>
> -Please consult the QEMU source for the most up-to-date and authoritative
> -list of selector keys and their respective items' purpose and format.
> +Please consult the QEMU source for the most up-to-date and authoritative list
> +of selector keys and their respective items' purpose, format and writeability.
>
> === Ranges ===
>
> Theoretically, there may be up to 0x4000 generic firmware configuration
> items, and up to 0x4000 architecturally specific ones.
>
> Selector Reg. Range Usage
> --------------- -----------
> -0x0000 - 0x3fff Generic (0x0000 - 0x3fff, RO)
> +0x0000 - 0x3fff Generic (0x0000 - 0x3fff, generally RO, possibly RW through
> + the DMA interface in QEMU v2.9+)
> 0x4000 - 0x7fff Generic (0x0000 - 0x3fff, RW, ignored in QEMU v2.4+)
> -0x8000 - 0xbfff Arch. Specific (0x0000 - 0x3fff, RO)
> +0x8000 - 0xbfff Arch. Specific (0x0000 - 0x3fff, generally RO, possibly RW
> + through the DMA interface in QEMU v2.9+)
> 0xc000 - 0xffff Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
>
> In practice, the number of allowed firmware configuration items is given
> by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
>
> @@ -180,21 +186,31 @@ address is the "control" field.
> The "control" field has the following bits:
> - Bit 0: Error
> - Bit 1: Read
> - Bit 2: Skip
> - Bit 3: Select. The upper 16 bits are the selected index.
> + - Bit 4: Write
>
> When an operation is triggered, if the "control" field has bit 3 set, the
> upper 16 bits are interpreted as an index of a firmware configuration item.
> This has the same effect as writing the selector register.
>
> If the "control" field has bit 1 set, a read operation will be performed.
> "length" bytes for the current selector and offset will be copied into the
> physical RAM address specified by the "address" field.
>
> -If the "control" field has bit 2 set (and not bit 1), a skip operation will be
> -performed. The offset for the current selector will be advanced "length" bytes.
> +If the "control" field has bit 4 set (and not bit 1), a write operation will be
> +performed. "length" bytes will be copied from the physical RAM address
> +specified by the "address" field to the current selector and offset. QEMU
> +prevents starting or finishing the write beyond the end of the item associated
> +with the current selector (i.e., the item cannot be resized). Truncated writes
> +are dropped entirely. Writes to read-only items are also rejected. All of these
> +write errors set bit 0 (the error bit) in the "control" field.
> +
> +If the "control" field has bit 2 set (and neither bit 1 nor bit 4), a skip
> +operation will be performed. The offset for the current selector will be
> +advanced "length" bytes.
>
> To check the result, read the "control" field:
> error bit set -> something went wrong.
> all bits cleared -> transfer finished successfully.
> otherwise -> transfer still in progress (doesn't happen
> @@ -232,5 +248,7 @@ For historical reasons, "opt/ovmf/" is reserved for OVMF firmware.
>
> Prefix "opt/org.qemu/" is reserved for QEMU itself.
>
> Use of names not beginning with "opt/" is potentially dangerous and
> entirely unsupported. QEMU will warn if you try.
> +
> +All externally provided fw_cfg items are read-only to the guest.
> diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
> index 23e18784dffd..a01f6bc5dfeb 100644
> --- a/hw/lm32/lm32_hwsetup.h
> +++ b/hw/lm32/lm32_hwsetup.h
> @@ -73,11 +73,11 @@ static inline void hwsetup_free(HWSetup *hw)
>
> 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, NULL);
> + TARGET_PAGE_SIZE, base, NULL, NULL, NULL, NULL, true);
> }
>
> static inline void hwsetup_add_u8(HWSetup *hw, uint8_t u)
> {
> stb_p(hw->ptr, u);
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 0c864cfd6046..0dbd8d6bf37a 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -178,11 +178,12 @@ int rom_add_file(const char *file, const char *fw_dir,
> bool option_rom, MemoryRegion *mr, AddressSpace *as);
> 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, AddressSpace *as);
> + void *callback_opaque, AddressSpace *as,
> + bool read_only);
> int rom_add_elf_program(const char *name, void *data, size_t datasize,
> size_t romsize, hwaddr addr, AddressSpace *as);
> int rom_check_and_register_reset(void);
> void rom_set_fw(FWCfgState *f);
> void rom_set_order_override(int order);
> @@ -192,19 +193,19 @@ void *rom_ptr(hwaddr addr);
> 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, NULL, NULL)
> #define rom_add_blob_fixed(_f, _b, _l, _a) \
> - rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL)
> + rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL, true)
> #define rom_add_file_mr(_f, _mr, _i) \
> rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
> #define rom_add_file_as(_f, _as, _i) \
> rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
> #define rom_add_file_fixed_as(_f, _a, _i, _as) \
> rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
> #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as) \
> - rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as)
> + rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
>
> #define PC_ROM_MIN_VGA 0xc0000
> #define PC_ROM_MIN_OPTION 0xc8000
> #define PC_ROM_MAX 0xe0000
> #define PC_ROM_ALIGN 0x800
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 5c27a1f0d50b..b980cbaebf43 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -134,10 +134,11 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
> * @filename: name of new fw_cfg file item
> * @callback: callback function
> * @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
> * data structure of the fw_cfg device.
> * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
> @@ -149,11 +150,11 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
> * the fw_cfg control register, or passed to QEMU in FWCfgDmaAccess.control
> * with FW_CFG_DMA_CTL_SELECT).
> */
> 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:
> * @s: fw_cfg device being modified
> * @filename: name of new fw_cfg file item
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index d4160dfa7d34..542fb67239db 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -807,11 +807,11 @@ static void virt_acpi_build_reset(void *build_opaque)
> static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
> GArray *blob, const char *name,
> 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, NULL);
> + name, virt_acpi_build_update, build_state, NULL, true);
> }
>
> static const VMStateDescription vmstate_virt_acpi_build = {
> .name = "virt_acpi_build",
> .version_id = 1,
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 45742494e6fd..ee5abd6eb7f4 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -851,20 +851,20 @@ static void fw_cfg_resized(const char *id, uint64_t length, void *host)
> if (fw_cfg) {
> fw_cfg_modify_file(fw_cfg, id + strlen("/rom@"), host, length);
> }
> }
>
> -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;
>
> rom->mr = g_malloc(sizeof(*rom->mr));
> memory_region_init_resizeable_ram(rom->mr, owner, 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);
> memcpy(data, rom->data, rom->datasize);
>
> @@ -940,11 +940,11 @@ int rom_add_file(const char *file, const char *fw_dir,
> snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
> basename);
> 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;
> }
>
> fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
> @@ -977,11 +977,11 @@ 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,
> - AddressSpace *as)
> + AddressSpace *as, bool read_only)
> {
> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> Rom *rom;
> MemoryRegion *mr = NULL;
>
> @@ -996,22 +996,26 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
> rom_insert(rom);
> if (fw_file_name && fw_cfg) {
> 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;
> }
>
> fw_cfg_add_file_callback(fw_cfg, fw_file_name,
> fw_callback, callback_opaque,
> - data, rom->datasize);
> + data, rom->datasize, read_only);
> }
> return mr;
> }
>
> /* This function is specific for elf program because we don't need to allocate
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9708cdc463df..965cb4cd4c51 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2934,11 +2934,11 @@ static void acpi_build_reset(void *build_opaque)
> static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
> GArray *blob, const char *name,
> uint64_t max_size)
> {
> return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
> - name, acpi_build_update, build_state, NULL);
> + name, acpi_build_update, build_state, NULL, true);
> }
>
> static const VMStateDescription vmstate_acpi_build = {
> .name = "acpi_build",
> .version_id = 1,
> @@ -3000,11 +3000,11 @@ void acpi_setup(void)
> uint32_t rsdp_size = acpi_data_len(tables.rsdp);
>
> 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;
> build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
> ACPI_BUILD_RSDP_FILE, 0);
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 3ebecb22606a..e0145c11a19b 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -52,15 +52,17 @@
> /* FW_CFG_DMA_CONTROL bits */
> #define FW_CFG_DMA_CTL_ERROR 0x01
> #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;
> } FWCfgEntry;
>
> @@ -324,11 +326,11 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
> {
> dma_addr_t len;
> 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 */
> dma_addr = s->dma_addr;
> s->dma_addr = 0;
> @@ -351,12 +353,17 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
> e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
> &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
>
> 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;
> }
>
> dma.control = 0;
> @@ -372,11 +379,13 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
> if (read) {
> if (dma_memory_set(s->dma_as, dma.address, 0, len)) {
> 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;
> } else {
> len = (e->len - s->cur_offset);
> @@ -389,10 +398,18 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
> if (dma_memory_write(s->dma_as, dma.address,
> &e->data[s->cur_offset], len)) {
> dma.control |= FW_CFG_DMA_CTL_ERROR;
> }
> }
> + if (write) {
> + if (!e->allow_write ||
> + len != dma.length ||
> + 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;
> }
>
> dma.address += len;
> @@ -584,11 +601,12 @@ 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);
>
> key &= FW_CFG_ENTRY_MASK;
>
> @@ -597,10 +615,11 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
>
> s->entries[arch][key].data = data;
> 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,
> void *data, size_t len)
> {
> @@ -614,17 +633,18 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
> /* return the old data to the function caller, avoid memory leak */
> ptr = s->entries[arch][key].data;
> 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)
> {
> size_t sz = strlen(value) + 1;
> @@ -747,11 +767,11 @@ static int get_fw_cfg_order(FWCfgState *s, const char *name)
> return FW_CFG_ORDER_OVERRIDE_LAST;
> }
>
> 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, count;
> size_t dsize;
> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> int order = 0;
> @@ -809,11 +829,12 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
> exit(1);
> }
> }
>
> 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);
> s->entry_order[index] = order;
> trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
> @@ -822,11 +843,11 @@ 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,
> void *data, size_t len)
> {
> @@ -845,11 +866,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
> s->files->f[i].size = cpu_to_be32(len);
> return ptr;
> }
> }
> /* 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;
> }
>
> static void fw_cfg_machine_reset(void *opaque)
> {
>
Hi Laszlo,
The 'write' documentation is very helpful, thanks.
I would add that QEMU will 'use' the written values
when the guest selects another file (for reading),
that being protocol specific, of course.
But maybe is not connected directly.
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI
2016-12-01 17:06 [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI Laszlo Ersek
2016-12-01 17:06 ` [Qemu-arm] [PATCH v4 1/7] fw-cfg: support writeable blobs Laszlo Ersek
@ 2016-12-20 23:01 ` no-reply
2016-12-21 15:22 ` [Qemu-arm] checkpatch.pl false positive (was Re: [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI) Eduardo Habkost
2017-01-10 15:06 ` [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI Michael S. Tsirkin
2 siblings, 1 reply; 17+ messages in thread
From: no-reply @ 2016-12-20 23:01 UTC (permalink / raw)
To: lersek
Cc: peter.maydell, famz, ehabkost, mst, somlo, qemu-devel, michael,
qemu-arm, kraxel, zhaoshenglong, pbonzini, imammedo
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI
Message-id: 20161201170624.26496-1-lersek@redhat.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update] patchew/1480926904-17596-1-git-send-email-zhang.zhanghailiang@huawei.com -> patchew/1480926904-17596-1-git-send-email-zhang.zhanghailiang@huawei.com
- [tag update] patchew/1481625384-15077-1-git-send-email-peter.maydell@linaro.org -> patchew/1481625384-15077-1-git-send-email-peter.maydell@linaro.org
- [tag update] patchew/1481856403-23599-1-git-send-email-zhangchen.fnst@cn.fujitsu.com -> patchew/1481856403-23599-1-git-send-email-zhangchen.fnst@cn.fujitsu.com
* [new tag] patchew/1482137486-9843-1-git-send-email-douly.fnst@cn.fujitsu.com -> patchew/1482137486-9843-1-git-send-email-douly.fnst@cn.fujitsu.com
* [new tag] patchew/1482140507-23607-1-git-send-email-vlad.lungu@windriver.com -> patchew/1482140507-23607-1-git-send-email-vlad.lungu@windriver.com
* [new tag] patchew/1482145554-88823-1-git-send-email-wei.w.wang@intel.com -> patchew/1482145554-88823-1-git-send-email-wei.w.wang@intel.com
* [new tag] patchew/1482156623-9111-1-git-send-email-peterx@redhat.com -> patchew/1482156623-9111-1-git-send-email-peterx@redhat.com
* [new tag] patchew/148215768690.13973.9042496691140000163.stgit@bahia -> patchew/148215768690.13973.9042496691140000163.stgit@bahia
* [new tag] patchew/1482158486-18597-1-git-send-email-peterx@redhat.com -> patchew/1482158486-18597-1-git-send-email-peterx@redhat.com
* [new tag] patchew/1482187106-85065-1-git-send-email-sochin.jiang@huawei.com -> patchew/1482187106-85065-1-git-send-email-sochin.jiang@huawei.com
* [new tag] patchew/1482255793-19057-1-git-send-email-ehabkost@redhat.com -> patchew/1482255793-19057-1-git-send-email-ehabkost@redhat.com
* [new tag] patchew/1482268300-10082-1-git-send-email-andrew.smirnov@gmail.com -> patchew/1482268300-10082-1-git-send-email-andrew.smirnov@gmail.com
* [new tag] patchew/1482268598-27422-1-git-send-email-thuth@redhat.com -> patchew/1482268598-27422-1-git-send-email-thuth@redhat.com
* [new tag] patchew/1482269164-14642-1-git-send-email-minyard@acm.org -> patchew/1482269164-14642-1-git-send-email-minyard@acm.org
* [new tag] patchew/20161201170624.26496-1-lersek@redhat.com -> patchew/20161201170624.26496-1-lersek@redhat.com
- [tag update] patchew/20161212224325.20790-1-marcandre.lureau@redhat.com -> patchew/20161212224325.20790-1-marcandre.lureau@redhat.com
- [tag update] patchew/20161214070156.23368-1-ppandit@redhat.com -> patchew/20161214070156.23368-1-ppandit@redhat.com
* [new tag] patchew/20161219205054.4677-1-kirkseraph@gmail.com -> patchew/20161219205054.4677-1-kirkseraph@gmail.com
* [new tag] patchew/20161220163139.12016-1-famz@redhat.com -> patchew/20161220163139.12016-1-famz@redhat.com
* [new tag] patchew/b0619e40-daca-9b28-2c78-1e2709d9f0ad@tuxfamily.org -> patchew/b0619e40-daca-9b28-2c78-1e2709d9f0ad@tuxfamily.org
* [new tag] patchew/cover.1482143215.git.vpalatin@chromium.org -> patchew/cover.1482143215.git.vpalatin@chromium.org
* [new tag] patchew/cover.1482164622.git.vpalatin@chromium.org -> patchew/cover.1482164622.git.vpalatin@chromium.org
* [new tag] patchew/cover.1482187052.git.ben@skyportsystems.com -> patchew/cover.1482187052.git.ben@skyportsystems.com
Switched to a new branch 'test'
2d2326b hw/i386/pc_q35: advertise broadcast SMI if VCPU hotplug is turned off
3443a68 hw/isa/lpc_ich9: add broadcast SMI feature
3863642 hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg
12b2efd hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots
5387420 fw-cfg: expose "file_slots" parameter in fw_cfg_init_io_dma()
c550b2c fw-cfg: turn FW_CFG_FILE_SLOTS into a device property
1f40c84 fw-cfg: support writeable blobs
=== OUTPUT BEGIN ===
Checking PATCH 1/7: fw-cfg: support writeable blobs...
Checking PATCH 2/7: fw-cfg: turn FW_CFG_FILE_SLOTS into a device property...
Checking PATCH 3/7: fw-cfg: expose "file_slots" parameter in fw_cfg_init_io_dma()...
Checking PATCH 4/7: hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots...
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#126: FILE: include/hw/compat.h:4:
+#define HW_COMPAT_2_8 \
+ {\
+ .driver = "fw_cfg_mem",\
+ .property = "file_slots",\
+ .value = stringify(0x10),\
+ },{\
+ .driver = "fw_cfg_io",\
+ .property = "file_slots",\
+ .value = stringify(0x10),\
+ },
total: 1 errors, 0 warnings, 119 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 5/7: hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg...
Checking PATCH 6/7: hw/isa/lpc_ich9: add broadcast SMI feature...
Checking PATCH 7/7: hw/i386/pc_q35: advertise broadcast SMI if VCPU hotplug is turned off...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-arm] checkpatch.pl false positive (was Re: [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI)
2016-12-20 23:01 ` [Qemu-arm] [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI no-reply
@ 2016-12-21 15:22 ` Eduardo Habkost
2016-12-21 15:22 ` [Qemu-devel] checkpatch.pl false positive (was " Eduardo Habkost
2016-12-21 17:47 ` [Qemu-arm] " Paolo Bonzini
0 siblings, 2 replies; 17+ messages in thread
From: Eduardo Habkost @ 2016-12-21 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, famz, mst, Eric Blake, somlo, qemu-devel,
Markus Armbruster, michael, qemu-arm, kraxel, zhaoshenglong,
pbonzini, lersek, imammedo
On Tue, Dec 20, 2016 at 03:01:17PM -0800, no-reply@patchew.org wrote:
[...]
> Checking PATCH 4/7: hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots...
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #126: FILE: include/hw/compat.h:4:
> +#define HW_COMPAT_2_8 \
> + {\
> + .driver = "fw_cfg_mem",\
> + .property = "file_slots",\
> + .value = stringify(0x10),\
> + },{\
> + .driver = "fw_cfg_io",\
> + .property = "file_slots",\
> + .value = stringify(0x10),\
> + },
>
> total: 1 errors, 0 warnings, 119 lines checked
>
> Your patch has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
It is a false positive, but how exactly can we fix it? Should it
become a warning instead of an error?
--
Eduardo
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] checkpatch.pl false positive (was Re: [PATCH v4 0/7] q35: add negotiable broadcast SMI)
2016-12-21 15:22 ` [Qemu-arm] checkpatch.pl false positive (was Re: [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI) Eduardo Habkost
@ 2016-12-21 15:22 ` Eduardo Habkost
2016-12-21 17:47 ` [Qemu-arm] " Paolo Bonzini
1 sibling, 0 replies; 17+ messages in thread
From: Eduardo Habkost @ 2016-12-21 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, famz, mst, somlo, qemu-devel, Markus Armbruster,
michael, qemu-arm, kraxel, zhaoshenglong, pbonzini, lersek,
imammedo
On Tue, Dec 20, 2016 at 03:01:17PM -0800, no-reply@patchew.org wrote:
[...]
> Checking PATCH 4/7: hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots...
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #126: FILE: include/hw/compat.h:4:
> +#define HW_COMPAT_2_8 \
> + {\
> + .driver = "fw_cfg_mem",\
> + .property = "file_slots",\
> + .value = stringify(0x10),\
> + },{\
> + .driver = "fw_cfg_io",\
> + .property = "file_slots",\
> + .value = stringify(0x10),\
> + },
>
> total: 1 errors, 0 warnings, 119 lines checked
>
> Your patch has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
It is a false positive, but how exactly can we fix it? Should it
become a warning instead of an error?
--
Eduardo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] checkpatch.pl false positive (was Re: [PATCH v4 0/7] q35: add negotiable broadcast SMI)
2016-12-21 15:22 ` [Qemu-arm] checkpatch.pl false positive (was Re: [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI) Eduardo Habkost
2016-12-21 15:22 ` [Qemu-devel] checkpatch.pl false positive (was " Eduardo Habkost
@ 2016-12-21 17:47 ` Paolo Bonzini
2016-12-21 18:01 ` Eduardo Habkost
1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-12-21 17:47 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: peter.maydell, famz, mst, somlo, Markus Armbruster, michael,
qemu-arm, kraxel, zhaoshenglong, imammedo, lersek
On 21/12/2016 16:22, Eduardo Habkost wrote:
> On Tue, Dec 20, 2016 at 03:01:17PM -0800, no-reply@patchew.org wrote:
> [...]
>> Checking PATCH 4/7: hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots...
>> ERROR: Macros with multiple statements should be enclosed in a do - while loop
>> #126: FILE: include/hw/compat.h:4:
>> +#define HW_COMPAT_2_8 \
>> + {\
>> + .driver = "fw_cfg_mem",\
>> + .property = "file_slots",\
>> + .value = stringify(0x10),\
>> + },{\
>> + .driver = "fw_cfg_io",\
>> + .property = "file_slots",\
>> + .value = stringify(0x10),\
>> + },
>>
>> total: 1 errors, 0 warnings, 119 lines checked
>>
>> Your patch has style problems, please review. If any of these errors
>> are false positives report them to the maintainer, see
>> CHECKPATCH in MAINTAINERS.
>
> It is a false positive, but how exactly can we fix it? Should it
> become a warning instead of an error?
It should already be treated as an exception:
my $exceptions = qr{
$Declare|
module_param_named|
MODULE_PARAM_DESC|
DECLARE_PER_CPU|
DEFINE_PER_CPU|
__typeof__\(|
union|
struct|
\.$Ident\s*=\s*| # <---- see here
^\"|\"$
}x;
#print "REST<$rest> dstat<$dstat> ctx<$ctx>\n";
if ($rest ne '' && $rest ne ',') {
if ($rest !~ /while\s*\(/ &&
$dstat !~ /$exceptions/)
{
ERROR("Macros with multiple statements should be enclosed in a do - while loop\n" . "$here\n$ctx\n");
}
so I guess the first step is debugging it. :)
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] checkpatch.pl false positive (was Re: [PATCH v4 0/7] q35: add negotiable broadcast SMI)
2016-12-21 17:47 ` [Qemu-arm] " Paolo Bonzini
@ 2016-12-21 18:01 ` Eduardo Habkost
2016-12-21 18:08 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Eduardo Habkost @ 2016-12-21 18:01 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, famz, mst, somlo, qemu-devel, Markus Armbruster,
michael, qemu-arm, kraxel, zhaoshenglong, imammedo, lersek
On Wed, Dec 21, 2016 at 06:47:01PM +0100, Paolo Bonzini wrote:
>
>
> On 21/12/2016 16:22, Eduardo Habkost wrote:
> > On Tue, Dec 20, 2016 at 03:01:17PM -0800, no-reply@patchew.org wrote:
> > [...]
> >> Checking PATCH 4/7: hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots...
> >> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> >> #126: FILE: include/hw/compat.h:4:
> >> +#define HW_COMPAT_2_8 \
> >> + {\
> >> + .driver = "fw_cfg_mem",\
> >> + .property = "file_slots",\
> >> + .value = stringify(0x10),\
> >> + },{\
> >> + .driver = "fw_cfg_io",\
> >> + .property = "file_slots",\
> >> + .value = stringify(0x10),\
> >> + },
> >>
> >> total: 1 errors, 0 warnings, 119 lines checked
> >>
> >> Your patch has style problems, please review. If any of these errors
> >> are false positives report them to the maintainer, see
> >> CHECKPATCH in MAINTAINERS.
> >
> > It is a false positive, but how exactly can we fix it? Should it
> > become a warning instead of an error?
>
> It should already be treated as an exception:
>
> my $exceptions = qr{
> $Declare|
> module_param_named|
> MODULE_PARAM_DESC|
> DECLARE_PER_CPU|
> DEFINE_PER_CPU|
> __typeof__\(|
> union|
> struct|
> \.$Ident\s*=\s*| # <---- see here
> ^\"|\"$
> }x;
> #print "REST<$rest> dstat<$dstat> ctx<$ctx>\n";
> if ($rest ne '' && $rest ne ',') {
> if ($rest !~ /while\s*\(/ &&
> $dstat !~ /$exceptions/)
> {
> ERROR("Macros with multiple statements should be enclosed in a do - while loop\n" . "$here\n$ctx\n");
> }
>
>
> so I guess the first step is debugging it. :)
The following code replaces the whole "{ .driver = ... }" block
with "1":
# Flatten any parentheses and braces
while ($dstat =~ s/\([^\(\)]*\)/1/ ||
$dstat =~ s/\{[^\{\}]*\}/1/ ||
$dstat =~ s/\[[^\{\}]*\]/1/)
{
}
The following change fixes the bug, but I don't know if it has
unwanted side-effects:
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f084542..0aab3ac 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2200,6 +2200,10 @@ sub process {
$dstat =~ s/^\s*//s;
$dstat =~ s/\s*$//s;
+ # remove braces that cover the whole block, if any:
+ $dstat =~ s/^\{//;
+ $dstat =~ s/\}$//;
+
# Flatten any parentheses and braces
while ($dstat =~ s/\([^\(\)]*\)/1/ ||
$dstat =~ s/\{[^\{\}]*\}/1/ ||
--
Eduardo
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] checkpatch.pl false positive (was Re: [PATCH v4 0/7] q35: add negotiable broadcast SMI)
2016-12-21 18:01 ` Eduardo Habkost
@ 2016-12-21 18:08 ` Paolo Bonzini
2016-12-21 18:19 ` [Qemu-devel] " Eduardo Habkost
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-12-21 18:08 UTC (permalink / raw)
To: Eduardo Habkost
Cc: peter.maydell, famz, mst, somlo, qemu-devel, Markus Armbruster,
michael, qemu-arm, kraxel, zhaoshenglong, imammedo, lersek
On 21/12/2016 19:01, Eduardo Habkost wrote:
> The following code replaces the whole "{ .driver = ... }" block
> with "1":
>
> # Flatten any parentheses and braces
> while ($dstat =~ s/\([^\(\)]*\)/1/ ||
> $dstat =~ s/\{[^\{\}]*\}/1/ ||
> $dstat =~ s/\[[^\{\}]*\]/1/)
> {
> }
Maybe change it like
- $dstat =~ s/\{[^\{\}]*\}/1/ ||
+ $dstat =~ s/\{[^\{\}]*;[^\{\}]*\}/1;/ ||
so that it requires a statement? It would have a false positive on
strings containing a semicolon, but that's not a big deal (or could be
fixed by similarly turning strings into just "").
Paolo
> The following change fixes the bug, but I don't know if it has
> unwanted side-effects:
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f084542..0aab3ac 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2200,6 +2200,10 @@ sub process {
> $dstat =~ s/^\s*//s;
> $dstat =~ s/\s*$//s;
>
> + # remove braces that cover the whole block, if any:
> + $dstat =~ s/^\{//;
> + $dstat =~ s/\}$//;
> +
> # Flatten any parentheses and braces
> while ($dstat =~ s/\([^\(\)]*\)/1/ ||
> $dstat =~ s/\{[^\{\}]*\}/1/ ||
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] checkpatch.pl false positive (was Re: [PATCH v4 0/7] q35: add negotiable broadcast SMI)
2016-12-21 18:08 ` Paolo Bonzini
@ 2016-12-21 18:19 ` Eduardo Habkost
0 siblings, 0 replies; 17+ messages in thread
From: Eduardo Habkost @ 2016-12-21 18:19 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, famz, mst, somlo, qemu-devel, Markus Armbruster,
michael, qemu-arm, kraxel, zhaoshenglong, imammedo, lersek
On Wed, Dec 21, 2016 at 07:08:29PM +0100, Paolo Bonzini wrote:
>
>
> On 21/12/2016 19:01, Eduardo Habkost wrote:
> > The following code replaces the whole "{ .driver = ... }" block
> > with "1":
> >
> > # Flatten any parentheses and braces
> > while ($dstat =~ s/\([^\(\)]*\)/1/ ||
> > $dstat =~ s/\{[^\{\}]*\}/1/ ||
> > $dstat =~ s/\[[^\{\}]*\]/1/)
> > {
> > }
>
> Maybe change it like
>
> - $dstat =~ s/\{[^\{\}]*\}/1/ ||
> + $dstat =~ s/\{[^\{\}]*;[^\{\}]*\}/1;/ ||
>
> so that it requires a statement?
I am not sure the point of this line is to detect multi-statement
blocks, but to replace other types of expressions containing {}
that could incorrectly match $exceptions, like:
#define FOO { \
Foo f = { .somefield = 1 }; \
some_statement(); \
another_statement();
}
In this case, $dstat would become:
"{ Foo f = 1; some_statement(); another_statement(); }"
> It would have a false positive on
> strings containing a semicolon, but that's not a big deal (or could be
> fixed by similarly turning strings into just "").
String contents are already replaced with "XXX" by some other
code. These are the contents of $dstat:
'{ .driver = "XXXXXXXXXX", .property = "XXXXXXXXXX", .value = stringify1, }'
>
> Paolo
>
> > The following change fixes the bug, but I don't know if it has
> > unwanted side-effects:
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index f084542..0aab3ac 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2200,6 +2200,10 @@ sub process {
> > $dstat =~ s/^\s*//s;
> > $dstat =~ s/\s*$//s;
> >
> > + # remove braces that cover the whole block, if any:
> > + $dstat =~ s/^\{//;
> > + $dstat =~ s/\}$//;
> > +
> > # Flatten any parentheses and braces
> > while ($dstat =~ s/\([^\(\)]*\)/1/ ||
> > $dstat =~ s/\{[^\{\}]*\}/1/ ||
>
>
> Paolo
--
Eduardo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] [Qemu-devel] [PATCH v4 1/7] fw-cfg: support writeable blobs
2016-12-20 15:58 ` [Qemu-arm] [Qemu-devel] " Marcel Apfelbaum
@ 2017-01-10 13:33 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-01-10 13:33 UTC (permalink / raw)
To: marcel, qemu devel list
Cc: Peter Maydell, Michael S. Tsirkin, Gabriel L. Somlo,
Michael Walle, qemu-arm, Gerd Hoffmann, Shannon Zhao,
Paolo Bonzini, Igor Mammedov
On 12/20/16 16:58, Marcel Apfelbaum wrote:
> On 12/01/2016 07:06 PM, Laszlo Ersek wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>
>> Useful to send guest data back to QEMU.
>>
>> Changes from Laszlo Ersek <lersek@redhat.com>:
>> - rebase the patch from Michael Tsirkin's original postings at [1] and
>> [2]
>> to the following patches:
>> - loader: Allow a custom AddressSpace when loading ROMs
>> - loader: Add AddressSpace loading support to uImages
>> - loader: fix handling of custom address spaces when adding ROM blobs
>> - reject such writes immediately that would exceed the end of the array,
>> rather than performing a partial write before setting the error bit:
>> see
>> the (len != dma.length) condition
>> - document the write interface
>>
>> [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg04968.html
>> [2] http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg02735.html
>>
>> Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Michael Walle <michael@walle.cc>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
>> Cc: qemu-arm@nongnu.org
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> docs/specs/fw_cfg.txt | 32 +++++++++++++++++++++++++-------
>> hw/lm32/lm32_hwsetup.h | 2 +-
>> include/hw/loader.h | 7 ++++---
>> include/hw/nvram/fw_cfg.h | 3 ++-
>> hw/arm/virt-acpi-build.c | 2 +-
>> hw/core/loader.c | 18 +++++++++++-------
>> hw/i386/acpi-build.c | 4 ++--
>> hw/nvram/fw_cfg.c | 37 +++++++++++++++++++++++++++++--------
>> 8 files changed, 75 insertions(+), 30 deletions(-)
>>
>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>> index 7a5f8c7824ce..a19e2adbe1c6 100644
>> --- a/docs/specs/fw_cfg.txt
>> +++ b/docs/specs/fw_cfg.txt
>> @@ -31,21 +31,25 @@ register. In other words, configuration write mode
>> is enabled when
>> the selector value is between 0x4000-0x7fff or 0xc000-0xffff.
>>
>> NOTE: As of QEMU v2.4, writes to the fw_cfg data register are no
>> longer supported, and will be ignored (treated as no-ops)!
>>
>> +NOTE: As of QEMU v2.9, writes are reinstated, but only through the DMA
>> + interface (see below). Furthermore, writeability of any
>> specific item is
>> + governed independently of Bit14 in the selector key value.
>> +
>> Bit15 of the selector register indicates whether the configuration
>> setting is architecture specific. A value of 0 means the item is a
>> generic configuration item. A value of 1 means the item is specific
>> to a particular architecture. In other words, generic configuration
>> items are accessed with a selector value between 0x0000-0x7fff, and
>> architecture specific configuration items are accessed with a selector
>> value between 0x8000-0xffff.
>>
>> == Data Register ==
>>
>> -* Read/Write (writes ignored as of QEMU v2.4)
>> +* Read/Write (writes ignored as of QEMU v2.4, but see the DMA interface)
>> * Location: platform dependent (IOport [*] or MMIO)
>> * Width: 8-bit (if IOport), 8/16/32/64-bit (if MMIO)
>> * Endianness: string-preserving
>>
>> [*] On platforms where the data register is exposed as an IOport, its
>> @@ -132,23 +136,25 @@ struct FWCfgFile { /* an individual file
>> entry, 64 bytes total */
>> char name[56]; /* fw_cfg item name, NUL-terminated ascii */
>> };
>>
>> === All Other Data Items ===
>>
>> -Please consult the QEMU source for the most up-to-date and authoritative
>> -list of selector keys and their respective items' purpose and format.
>> +Please consult the QEMU source for the most up-to-date and
>> authoritative list
>> +of selector keys and their respective items' purpose, format and
>> writeability.
>>
>> === Ranges ===
>>
>> Theoretically, there may be up to 0x4000 generic firmware configuration
>> items, and up to 0x4000 architecturally specific ones.
>>
>> Selector Reg. Range Usage
>> --------------- -----------
>> -0x0000 - 0x3fff Generic (0x0000 - 0x3fff, RO)
>> +0x0000 - 0x3fff Generic (0x0000 - 0x3fff, generally RO, possibly RW
>> through
>> + the DMA interface in QEMU v2.9+)
>> 0x4000 - 0x7fff Generic (0x0000 - 0x3fff, RW, ignored in QEMU v2.4+)
>> -0x8000 - 0xbfff Arch. Specific (0x0000 - 0x3fff, RO)
>> +0x8000 - 0xbfff Arch. Specific (0x0000 - 0x3fff, generally RO,
>> possibly RW
>> + through the DMA interface in QEMU
>> v2.9+)
>> 0xc000 - 0xffff Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
>>
>> In practice, the number of allowed firmware configuration items is given
>> by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
>>
>> @@ -180,21 +186,31 @@ address is the "control" field.
>> The "control" field has the following bits:
>> - Bit 0: Error
>> - Bit 1: Read
>> - Bit 2: Skip
>> - Bit 3: Select. The upper 16 bits are the selected index.
>> + - Bit 4: Write
>>
>> When an operation is triggered, if the "control" field has bit 3 set,
>> the
>> upper 16 bits are interpreted as an index of a firmware configuration
>> item.
>> This has the same effect as writing the selector register.
>>
>> If the "control" field has bit 1 set, a read operation will be
>> performed.
>> "length" bytes for the current selector and offset will be copied
>> into the
>> physical RAM address specified by the "address" field.
>>
>> -If the "control" field has bit 2 set (and not bit 1), a skip
>> operation will be
>> -performed. The offset for the current selector will be advanced
>> "length" bytes.
>> +If the "control" field has bit 4 set (and not bit 1), a write
>> operation will be
>> +performed. "length" bytes will be copied from the physical RAM address
>> +specified by the "address" field to the current selector and offset.
>> QEMU
>> +prevents starting or finishing the write beyond the end of the item
>> associated
>> +with the current selector (i.e., the item cannot be resized).
>> Truncated writes
>> +are dropped entirely. Writes to read-only items are also rejected.
>> All of these
>> +write errors set bit 0 (the error bit) in the "control" field.
>> +
>> +If the "control" field has bit 2 set (and neither bit 1 nor bit 4), a
>> skip
>> +operation will be performed. The offset for the current selector will be
>> +advanced "length" bytes.
>>
>> To check the result, read the "control" field:
>> error bit set -> something went wrong.
>> all bits cleared -> transfer finished successfully.
>> otherwise -> transfer still in progress (doesn't happen
>> @@ -232,5 +248,7 @@ For historical reasons, "opt/ovmf/" is reserved
>> for OVMF firmware.
>>
>> Prefix "opt/org.qemu/" is reserved for QEMU itself.
>>
>> Use of names not beginning with "opt/" is potentially dangerous and
>> entirely unsupported. QEMU will warn if you try.
>> +
>> +All externally provided fw_cfg items are read-only to the guest.
>> diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
>> index 23e18784dffd..a01f6bc5dfeb 100644
>> --- a/hw/lm32/lm32_hwsetup.h
>> +++ b/hw/lm32/lm32_hwsetup.h
>> @@ -73,11 +73,11 @@ static inline void hwsetup_free(HWSetup *hw)
>>
>> 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, NULL);
>> + TARGET_PAGE_SIZE, base, NULL, NULL, NULL, NULL, true);
>> }
>>
>> static inline void hwsetup_add_u8(HWSetup *hw, uint8_t u)
>> {
>> stb_p(hw->ptr, u);
>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>> index 0c864cfd6046..0dbd8d6bf37a 100644
>> --- a/include/hw/loader.h
>> +++ b/include/hw/loader.h
>> @@ -178,11 +178,12 @@ int rom_add_file(const char *file, const char
>> *fw_dir,
>> bool option_rom, MemoryRegion *mr, AddressSpace *as);
>> 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, AddressSpace *as);
>> + void *callback_opaque, AddressSpace *as,
>> + bool read_only);
>> int rom_add_elf_program(const char *name, void *data, size_t datasize,
>> size_t romsize, hwaddr addr, AddressSpace *as);
>> int rom_check_and_register_reset(void);
>> void rom_set_fw(FWCfgState *f);
>> void rom_set_order_override(int order);
>> @@ -192,19 +193,19 @@ void *rom_ptr(hwaddr addr);
>> 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, NULL, NULL)
>> #define rom_add_blob_fixed(_f, _b, _l, _a) \
>> - rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL)
>> + rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL, true)
>> #define rom_add_file_mr(_f, _mr, _i) \
>> rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
>> #define rom_add_file_as(_f, _as, _i) \
>> rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
>> #define rom_add_file_fixed_as(_f, _a, _i, _as) \
>> rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
>> #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as) \
>> - rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as)
>> + rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
>>
>> #define PC_ROM_MIN_VGA 0xc0000
>> #define PC_ROM_MIN_OPTION 0xc8000
>> #define PC_ROM_MAX 0xe0000
>> #define PC_ROM_ALIGN 0x800
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index 5c27a1f0d50b..b980cbaebf43 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -134,10 +134,11 @@ void fw_cfg_add_file(FWCfgState *s, const char
>> *filename, void *data,
>> * @filename: name of new fw_cfg file item
>> * @callback: callback function
>> * @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
>> * data structure of the fw_cfg device.
>> * The next available (unused) selector key starting at
>> FW_CFG_FILE_FIRST
>> @@ -149,11 +150,11 @@ void fw_cfg_add_file(FWCfgState *s, const char
>> *filename, void *data,
>> * the fw_cfg control register, or passed to QEMU in
>> FWCfgDmaAccess.control
>> * with FW_CFG_DMA_CTL_SELECT).
>> */
>> 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:
>> * @s: fw_cfg device being modified
>> * @filename: name of new fw_cfg file item
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index d4160dfa7d34..542fb67239db 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -807,11 +807,11 @@ static void virt_acpi_build_reset(void
>> *build_opaque)
>> static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
>> GArray *blob, const char *name,
>> 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,
>> NULL);
>> + name, virt_acpi_build_update, build_state,
>> NULL, true);
>> }
>>
>> static const VMStateDescription vmstate_virt_acpi_build = {
>> .name = "virt_acpi_build",
>> .version_id = 1,
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 45742494e6fd..ee5abd6eb7f4 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -851,20 +851,20 @@ static void fw_cfg_resized(const char *id,
>> uint64_t length, void *host)
>> if (fw_cfg) {
>> fw_cfg_modify_file(fw_cfg, id + strlen("/rom@"), host, length);
>> }
>> }
>>
>> -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;
>>
>> rom->mr = g_malloc(sizeof(*rom->mr));
>> memory_region_init_resizeable_ram(rom->mr, owner, 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);
>> memcpy(data, rom->data, rom->datasize);
>>
>> @@ -940,11 +940,11 @@ int rom_add_file(const char *file, const char
>> *fw_dir,
>> snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s",
>> rom->fw_dir,
>> basename);
>> 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;
>> }
>>
>> fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
>> @@ -977,11 +977,11 @@ 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,
>> - AddressSpace *as)
>> + AddressSpace *as, bool read_only)
>> {
>> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>> Rom *rom;
>> MemoryRegion *mr = NULL;
>>
>> @@ -996,22 +996,26 @@ MemoryRegion *rom_add_blob(const char *name,
>> const void *blob, size_t len,
>> rom_insert(rom);
>> if (fw_file_name && fw_cfg) {
>> 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;
>> }
>>
>> fw_cfg_add_file_callback(fw_cfg, fw_file_name,
>> fw_callback, callback_opaque,
>> - data, rom->datasize);
>> + data, rom->datasize, read_only);
>> }
>> return mr;
>> }
>>
>> /* This function is specific for elf program because we don't need to
>> allocate
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 9708cdc463df..965cb4cd4c51 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2934,11 +2934,11 @@ static void acpi_build_reset(void *build_opaque)
>> static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
>> GArray *blob, const char *name,
>> uint64_t max_size)
>> {
>> return rom_add_blob(name, blob->data, acpi_data_len(blob),
>> max_size, -1,
>> - name, acpi_build_update, build_state, NULL);
>> + name, acpi_build_update, build_state, NULL,
>> true);
>> }
>>
>> static const VMStateDescription vmstate_acpi_build = {
>> .name = "acpi_build",
>> .version_id = 1,
>> @@ -3000,11 +3000,11 @@ void acpi_setup(void)
>> uint32_t rsdp_size = acpi_data_len(tables.rsdp);
>>
>> 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;
>> build_state->rsdp_mr = acpi_add_rom_blob(build_state,
>> tables.rsdp,
>>
>> ACPI_BUILD_RSDP_FILE, 0);
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 3ebecb22606a..e0145c11a19b 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -52,15 +52,17 @@
>> /* FW_CFG_DMA_CONTROL bits */
>> #define FW_CFG_DMA_CTL_ERROR 0x01
>> #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;
>> } FWCfgEntry;
>>
>> @@ -324,11 +326,11 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
>> {
>> dma_addr_t len;
>> 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 */
>> dma_addr = s->dma_addr;
>> s->dma_addr = 0;
>> @@ -351,12 +353,17 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
>> e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
>> &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
>>
>> 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;
>> }
>>
>> dma.control = 0;
>> @@ -372,11 +379,13 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
>> if (read) {
>> if (dma_memory_set(s->dma_as, dma.address, 0, len)) {
>> 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;
>> } else {
>> len = (e->len - s->cur_offset);
>> @@ -389,10 +398,18 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
>> if (dma_memory_write(s->dma_as, dma.address,
>> &e->data[s->cur_offset], len)) {
>> dma.control |= FW_CFG_DMA_CTL_ERROR;
>> }
>> }
>> + if (write) {
>> + if (!e->allow_write ||
>> + len != dma.length ||
>> + 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;
>> }
>>
>> dma.address += len;
>> @@ -584,11 +601,12 @@ 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);
>>
>> key &= FW_CFG_ENTRY_MASK;
>>
>> @@ -597,10 +615,11 @@ static void
>> fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
>>
>> s->entries[arch][key].data = data;
>> 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,
>> void *data, size_t len)
>> {
>> @@ -614,17 +633,18 @@ static void *fw_cfg_modify_bytes_read(FWCfgState
>> *s, uint16_t key,
>> /* return the old data to the function caller, avoid memory leak */
>> ptr = s->entries[arch][key].data;
>> 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)
>> {
>> size_t sz = strlen(value) + 1;
>> @@ -747,11 +767,11 @@ static int get_fw_cfg_order(FWCfgState *s, const
>> char *name)
>> return FW_CFG_ORDER_OVERRIDE_LAST;
>> }
>>
>> 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, count;
>> size_t dsize;
>> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>> int order = 0;
>> @@ -809,11 +829,12 @@ void fw_cfg_add_file_callback(FWCfgState *s,
>> const char *filename,
>> exit(1);
>> }
>> }
>>
>> 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);
>> s->entry_order[index] = order;
>> trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
>> @@ -822,11 +843,11 @@ 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,
>> void *data, size_t len)
>> {
>> @@ -845,11 +866,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const
>> char *filename,
>> s->files->f[i].size = cpu_to_be32(len);
>> return ptr;
>> }
>> }
>> /* 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;
>> }
>>
>> static void fw_cfg_machine_reset(void *opaque)
>> {
>>
>
> Hi Laszlo,
> The 'write' documentation is very helpful, thanks.
> I would add that QEMU will 'use' the written values
> when the guest selects another file (for reading),
> that being protocol specific, of course.
> But maybe is not connected directly.
I think it's not connected; it's a general characteristic that callbacks
are invoked at item selection.
Also, the data that a given callback consumes to produce the selected
item's new contents may or may not include any fw_cfg data written
previously (for example, with the ACPI linker/loader, the regenerated
contents are independent of any fw_cfg writes).
>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks!
I'm about to rebase / rework this series; if this patch applies with at
most minor updates, I'll keep your R-b.
Cheers,
Laszlo
> Thanks,
> Marcel
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI
2016-12-01 17:06 [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI Laszlo Ersek
2016-12-01 17:06 ` [Qemu-arm] [PATCH v4 1/7] fw-cfg: support writeable blobs Laszlo Ersek
2016-12-20 23:01 ` [Qemu-arm] [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI no-reply
@ 2017-01-10 15:06 ` Michael S. Tsirkin
2017-01-10 15:23 ` Laszlo Ersek
2 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2017-01-10 15:06 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Peter Maydell, Eduardo Habkost, Gabriel L. Somlo, Shannon Zhao,
qemu devel list, Michael Walle, qemu-arm, Gerd Hoffmann,
Paolo Bonzini, Igor Mammedov
On Thu, Dec 01, 2016 at 06:06:17PM +0100, Laszlo Ersek wrote:
> * This is version 4 of the series; the last version was at
> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03582.html>.
>
> This version is practically a rewrite from scratch, seeking to address
> the v3 feedback. Here's what the individual patches do:
>
> - Patch #1 rebases and extends Michael's "writeable fw_cfg blobs"
> patch from Feb/March 2016. The changes relative to the original
> patch are documented in the commit message (the code changes are
> minimal).
>
> - Patches #2 and #3 turn the FW_CFG_FILE_SLOTS constant into a device
> property, and expose the desired number of fw_cfg file slots to
> board code. This is meant to address a concern raised by Paolo in
> the v2 review
> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03125.html>,
> namely that we're about to run out of FW_CFG_FILE_SLOTS.
>
> - Patch #4 introduces the "pc-q35-2.9" and "pc-i440fx-2.9" machine
> types, which are allowed to take advantage of the new default for
> fw_cfg file slots (0x20 rather than 0x10).
>
> - Patch #5 introduces SMI feature negotiation via fw_cfg, with the
> following new files:
>
> - etc/smi/host-features
> - etc/smi/guest-features
> - etc/smi/features-ok
>
> This is supposed to follow Michael's recommendation re: imitating
> virtio
> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03218.html>.
>
> The guest-features file is freely writeable by the guest (no write
> callbacks in fw_cfg), and the feature validation & lockdown occurs
> when the guest selects the features-ok file (for reading).
>
> Board code is allowed to choose the host feature bitmap to
> advertise.
>
> This patch doesn't add any specific SMI features yet.
>
> - Patch #6 adds the ICH9_LPC_SMI_F_BROADCAST feature.
>
> - Patch #7 introduces the PCMachineClass.get_smi_host_features method,
> and implements it for "pc-q35-2.9" and later. The idea is to tie the
> SMI host features to machine types, and let the machine types
> calculate their host features with code (i.e., not just a constant).
>
> In this patch, the "pc-q35-2.9" machine type exposes the
> ICH9_LPC_SMI_F_BROADCAST feature iff (smp_cpus == max_cpus), that
> is, when VCPU hotplug is not possible. (Also from Paolo's v3
> feedback, in
> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg04919.html>.)
>
> * I've written the OVMF side patches too and tested them together
> (including gdb / debug messages for "white box" testing).
>
> * Note that this version depends on the following PULL req from Michael:
>
> [Qemu-devel] [PULL 0/5] virtio, vhost, pc: fixes
> http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05503.html
>
> In particular on the following patch:
> "loader: fix handling of custom address spaces when adding ROM blobs"
>
> Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
> Cc: qemu-arm@nongnu.org
>
> Thanks
> Laszlo
So I reviewed fw cfg stuff, looks good to me. I'd prefer Paolo to review
and merge broadcast SMI stuff as appropriate. Paolo, makes sense?
> Laszlo Ersek (6):
> fw-cfg: turn FW_CFG_FILE_SLOTS into a device property
> fw-cfg: expose "file_slots" parameter in fw_cfg_init_io_dma()
> hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots
> hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg
> hw/isa/lpc_ich9: add broadcast SMI feature
> hw/i386/pc_q35: advertise broadcast SMI if VCPU hotplug is turned off
>
> Michael S. Tsirkin (1):
> fw-cfg: support writeable blobs
>
> docs/specs/fw_cfg.txt | 36 +++++++++---
> hw/lm32/lm32_hwsetup.h | 2 +-
> include/hw/compat.h | 11 ++++
> include/hw/i386/ich9.h | 15 ++++-
> include/hw/i386/pc.h | 4 ++
> include/hw/loader.h | 7 ++-
> include/hw/nvram/fw_cfg.h | 5 +-
> include/hw/nvram/fw_cfg_keys.h | 7 ++-
> hw/arm/virt-acpi-build.c | 2 +-
> hw/core/loader.c | 18 +++---
> hw/i386/acpi-build.c | 4 +-
> hw/i386/pc.c | 3 +-
> hw/i386/pc_piix.c | 16 +++++-
> hw/i386/pc_q35.c | 37 ++++++++++++-
> hw/isa/lpc_ich9.c | 92 +++++++++++++++++++++++++++++-
> hw/nvram/fw_cfg.c | 123 +++++++++++++++++++++++++++++++++++------
> 16 files changed, 326 insertions(+), 56 deletions(-)
>
> --
> 2.9.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI
2017-01-10 15:06 ` [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI Michael S. Tsirkin
@ 2017-01-10 15:23 ` Laszlo Ersek
2017-01-10 16:05 ` Michael S. Tsirkin
2017-01-10 16:21 ` [Qemu-arm] " Igor Mammedov
0 siblings, 2 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-01-10 15:23 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, Eduardo Habkost, Gabriel L. Somlo, Shannon Zhao,
qemu devel list, Michael Walle, qemu-arm, Gerd Hoffmann,
Paolo Bonzini, Igor Mammedov
On 01/10/17 16:06, Michael S. Tsirkin wrote:
> On Thu, Dec 01, 2016 at 06:06:17PM +0100, Laszlo Ersek wrote:
>> * This is version 4 of the series; the last version was at
>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03582.html>.
>>
>> This version is practically a rewrite from scratch, seeking to address
>> the v3 feedback. Here's what the individual patches do:
>>
>> - Patch #1 rebases and extends Michael's "writeable fw_cfg blobs"
>> patch from Feb/March 2016. The changes relative to the original
>> patch are documented in the commit message (the code changes are
>> minimal).
>>
>> - Patches #2 and #3 turn the FW_CFG_FILE_SLOTS constant into a device
>> property, and expose the desired number of fw_cfg file slots to
>> board code. This is meant to address a concern raised by Paolo in
>> the v2 review
>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03125.html>,
>> namely that we're about to run out of FW_CFG_FILE_SLOTS.
>>
>> - Patch #4 introduces the "pc-q35-2.9" and "pc-i440fx-2.9" machine
>> types, which are allowed to take advantage of the new default for
>> fw_cfg file slots (0x20 rather than 0x10).
>>
>> - Patch #5 introduces SMI feature negotiation via fw_cfg, with the
>> following new files:
>>
>> - etc/smi/host-features
>> - etc/smi/guest-features
>> - etc/smi/features-ok
>>
>> This is supposed to follow Michael's recommendation re: imitating
>> virtio
>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03218.html>.
>>
>> The guest-features file is freely writeable by the guest (no write
>> callbacks in fw_cfg), and the feature validation & lockdown occurs
>> when the guest selects the features-ok file (for reading).
>>
>> Board code is allowed to choose the host feature bitmap to
>> advertise.
>>
>> This patch doesn't add any specific SMI features yet.
>>
>> - Patch #6 adds the ICH9_LPC_SMI_F_BROADCAST feature.
>>
>> - Patch #7 introduces the PCMachineClass.get_smi_host_features method,
>> and implements it for "pc-q35-2.9" and later. The idea is to tie the
>> SMI host features to machine types, and let the machine types
>> calculate their host features with code (i.e., not just a constant).
>>
>> In this patch, the "pc-q35-2.9" machine type exposes the
>> ICH9_LPC_SMI_F_BROADCAST feature iff (smp_cpus == max_cpus), that
>> is, when VCPU hotplug is not possible. (Also from Paolo's v3
>> feedback, in
>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg04919.html>.)
>>
>> * I've written the OVMF side patches too and tested them together
>> (including gdb / debug messages for "white box" testing).
>>
>> * Note that this version depends on the following PULL req from Michael:
>>
>> [Qemu-devel] [PULL 0/5] virtio, vhost, pc: fixes
>> http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05503.html
>>
>> In particular on the following patch:
>> "loader: fix handling of custom address spaces when adding ROM blobs"
>>
>> Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Michael Walle <michael@walle.cc>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
>> Cc: qemu-arm@nongnu.org
>>
>> Thanks
>> Laszlo
>
> So I reviewed fw cfg stuff, looks good to me.
Thank you.
I should note that Igor suggested / requested changes for patches #2 and
#3; the sub-threads start at
http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00695.html
msg-id: <20161206115038.4495a217@nial.brq.redhat.com>
http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00706.html
msg-id: <20161206124906.76a41896@nial.brq.redhat.com>
I was about to rework the patches based on his feedback. Should we
resume those discussions?
> I'd prefer Paolo to review
> and merge broadcast SMI stuff as appropriate. Paolo, makes sense?
If it helps, I can split the series into two "waves", and deal only with
fw_cfg first.
Thanks!
Laszlo
>
>
>> Laszlo Ersek (6):
>> fw-cfg: turn FW_CFG_FILE_SLOTS into a device property
>> fw-cfg: expose "file_slots" parameter in fw_cfg_init_io_dma()
>> hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots
>> hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg
>> hw/isa/lpc_ich9: add broadcast SMI feature
>> hw/i386/pc_q35: advertise broadcast SMI if VCPU hotplug is turned off
>>
>> Michael S. Tsirkin (1):
>> fw-cfg: support writeable blobs
>>
>> docs/specs/fw_cfg.txt | 36 +++++++++---
>> hw/lm32/lm32_hwsetup.h | 2 +-
>> include/hw/compat.h | 11 ++++
>> include/hw/i386/ich9.h | 15 ++++-
>> include/hw/i386/pc.h | 4 ++
>> include/hw/loader.h | 7 ++-
>> include/hw/nvram/fw_cfg.h | 5 +-
>> include/hw/nvram/fw_cfg_keys.h | 7 ++-
>> hw/arm/virt-acpi-build.c | 2 +-
>> hw/core/loader.c | 18 +++---
>> hw/i386/acpi-build.c | 4 +-
>> hw/i386/pc.c | 3 +-
>> hw/i386/pc_piix.c | 16 +++++-
>> hw/i386/pc_q35.c | 37 ++++++++++++-
>> hw/isa/lpc_ich9.c | 92 +++++++++++++++++++++++++++++-
>> hw/nvram/fw_cfg.c | 123 +++++++++++++++++++++++++++++++++++------
>> 16 files changed, 326 insertions(+), 56 deletions(-)
>>
>> --
>> 2.9.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI
2017-01-10 15:23 ` Laszlo Ersek
@ 2017-01-10 16:05 ` Michael S. Tsirkin
2017-01-10 16:19 ` [Qemu-devel] " Laszlo Ersek
2017-01-10 16:21 ` [Qemu-arm] " Igor Mammedov
1 sibling, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2017-01-10 16:05 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Peter Maydell, Eduardo Habkost, Gabriel L. Somlo, Shannon Zhao,
qemu devel list, Michael Walle, qemu-arm, Gerd Hoffmann,
Paolo Bonzini, Igor Mammedov
On Tue, Jan 10, 2017 at 04:23:41PM +0100, Laszlo Ersek wrote:
> On 01/10/17 16:06, Michael S. Tsirkin wrote:
> > On Thu, Dec 01, 2016 at 06:06:17PM +0100, Laszlo Ersek wrote:
> >> * This is version 4 of the series; the last version was at
> >> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03582.html>.
> >>
> >> This version is practically a rewrite from scratch, seeking to address
> >> the v3 feedback. Here's what the individual patches do:
> >>
> >> - Patch #1 rebases and extends Michael's "writeable fw_cfg blobs"
> >> patch from Feb/March 2016. The changes relative to the original
> >> patch are documented in the commit message (the code changes are
> >> minimal).
> >>
> >> - Patches #2 and #3 turn the FW_CFG_FILE_SLOTS constant into a device
> >> property, and expose the desired number of fw_cfg file slots to
> >> board code. This is meant to address a concern raised by Paolo in
> >> the v2 review
> >> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03125.html>,
> >> namely that we're about to run out of FW_CFG_FILE_SLOTS.
> >>
> >> - Patch #4 introduces the "pc-q35-2.9" and "pc-i440fx-2.9" machine
> >> types, which are allowed to take advantage of the new default for
> >> fw_cfg file slots (0x20 rather than 0x10).
> >>
> >> - Patch #5 introduces SMI feature negotiation via fw_cfg, with the
> >> following new files:
> >>
> >> - etc/smi/host-features
> >> - etc/smi/guest-features
> >> - etc/smi/features-ok
> >>
> >> This is supposed to follow Michael's recommendation re: imitating
> >> virtio
> >> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03218.html>.
> >>
> >> The guest-features file is freely writeable by the guest (no write
> >> callbacks in fw_cfg), and the feature validation & lockdown occurs
> >> when the guest selects the features-ok file (for reading).
> >>
> >> Board code is allowed to choose the host feature bitmap to
> >> advertise.
> >>
> >> This patch doesn't add any specific SMI features yet.
> >>
> >> - Patch #6 adds the ICH9_LPC_SMI_F_BROADCAST feature.
> >>
> >> - Patch #7 introduces the PCMachineClass.get_smi_host_features method,
> >> and implements it for "pc-q35-2.9" and later. The idea is to tie the
> >> SMI host features to machine types, and let the machine types
> >> calculate their host features with code (i.e., not just a constant).
> >>
> >> In this patch, the "pc-q35-2.9" machine type exposes the
> >> ICH9_LPC_SMI_F_BROADCAST feature iff (smp_cpus == max_cpus), that
> >> is, when VCPU hotplug is not possible. (Also from Paolo's v3
> >> feedback, in
> >> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg04919.html>.)
> >>
> >> * I've written the OVMF side patches too and tested them together
> >> (including gdb / debug messages for "white box" testing).
> >>
> >> * Note that this version depends on the following PULL req from Michael:
> >>
> >> [Qemu-devel] [PULL 0/5] virtio, vhost, pc: fixes
> >> http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05503.html
> >>
> >> In particular on the following patch:
> >> "loader: fix handling of custom address spaces when adding ROM blobs"
> >>
> >> Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Eduardo Habkost <ehabkost@redhat.com>
> >> Cc: Gerd Hoffmann <kraxel@redhat.com>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Cc: Michael Walle <michael@walle.cc>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Peter Maydell <peter.maydell@linaro.org>
> >> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
> >> Cc: qemu-arm@nongnu.org
> >>
> >> Thanks
> >> Laszlo
> >
> > So I reviewed fw cfg stuff, looks good to me.
>
> Thank you.
>
> I should note that Igor suggested / requested changes for patches #2 and
> #3; the sub-threads start at
>
> http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00695.html
> msg-id: <20161206115038.4495a217@nial.brq.redhat.com>
>
> http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00706.html
> msg-id: <20161206124906.76a41896@nial.brq.redhat.com>
>
> I was about to rework the patches based on his feedback. Should we
> resume those discussions?
If you both agree, pls go ahead and make these changes.
> > I'd prefer Paolo to review
> > and merge broadcast SMI stuff as appropriate. Paolo, makes sense?
>
> If it helps, I can split the series into two "waves", and deal only with
> fw_cfg first.
>
> Thanks!
> Laszlo
I don't mind either way.
> >
> >
> >> Laszlo Ersek (6):
> >> fw-cfg: turn FW_CFG_FILE_SLOTS into a device property
> >> fw-cfg: expose "file_slots" parameter in fw_cfg_init_io_dma()
> >> hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots
> >> hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg
> >> hw/isa/lpc_ich9: add broadcast SMI feature
> >> hw/i386/pc_q35: advertise broadcast SMI if VCPU hotplug is turned off
> >>
> >> Michael S. Tsirkin (1):
> >> fw-cfg: support writeable blobs
> >>
> >> docs/specs/fw_cfg.txt | 36 +++++++++---
> >> hw/lm32/lm32_hwsetup.h | 2 +-
> >> include/hw/compat.h | 11 ++++
> >> include/hw/i386/ich9.h | 15 ++++-
> >> include/hw/i386/pc.h | 4 ++
> >> include/hw/loader.h | 7 ++-
> >> include/hw/nvram/fw_cfg.h | 5 +-
> >> include/hw/nvram/fw_cfg_keys.h | 7 ++-
> >> hw/arm/virt-acpi-build.c | 2 +-
> >> hw/core/loader.c | 18 +++---
> >> hw/i386/acpi-build.c | 4 +-
> >> hw/i386/pc.c | 3 +-
> >> hw/i386/pc_piix.c | 16 +++++-
> >> hw/i386/pc_q35.c | 37 ++++++++++++-
> >> hw/isa/lpc_ich9.c | 92 +++++++++++++++++++++++++++++-
> >> hw/nvram/fw_cfg.c | 123 +++++++++++++++++++++++++++++++++++------
> >> 16 files changed, 326 insertions(+), 56 deletions(-)
> >>
> >> --
> >> 2.9.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI
2017-01-10 16:05 ` Michael S. Tsirkin
@ 2017-01-10 16:19 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-01-10 16:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, Eduardo Habkost, Gabriel L. Somlo, Shannon Zhao,
qemu devel list, Michael Walle, qemu-arm, Gerd Hoffmann,
Paolo Bonzini, Igor Mammedov
On 01/10/17 17:05, Michael S. Tsirkin wrote:
> On Tue, Jan 10, 2017 at 04:23:41PM +0100, Laszlo Ersek wrote:
>> On 01/10/17 16:06, Michael S. Tsirkin wrote:
>>> On Thu, Dec 01, 2016 at 06:06:17PM +0100, Laszlo Ersek wrote:
>>>> * This is version 4 of the series; the last version was at
>>>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03582.html>.
>>>>
>>>> This version is practically a rewrite from scratch, seeking to address
>>>> the v3 feedback. Here's what the individual patches do:
>>>>
>>>> - Patch #1 rebases and extends Michael's "writeable fw_cfg blobs"
>>>> patch from Feb/March 2016. The changes relative to the original
>>>> patch are documented in the commit message (the code changes are
>>>> minimal).
>>>>
>>>> - Patches #2 and #3 turn the FW_CFG_FILE_SLOTS constant into a device
>>>> property, and expose the desired number of fw_cfg file slots to
>>>> board code. This is meant to address a concern raised by Paolo in
>>>> the v2 review
>>>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03125.html>,
>>>> namely that we're about to run out of FW_CFG_FILE_SLOTS.
>>>>
>>>> - Patch #4 introduces the "pc-q35-2.9" and "pc-i440fx-2.9" machine
>>>> types, which are allowed to take advantage of the new default for
>>>> fw_cfg file slots (0x20 rather than 0x10).
>>>>
>>>> - Patch #5 introduces SMI feature negotiation via fw_cfg, with the
>>>> following new files:
>>>>
>>>> - etc/smi/host-features
>>>> - etc/smi/guest-features
>>>> - etc/smi/features-ok
>>>>
>>>> This is supposed to follow Michael's recommendation re: imitating
>>>> virtio
>>>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03218.html>.
>>>>
>>>> The guest-features file is freely writeable by the guest (no write
>>>> callbacks in fw_cfg), and the feature validation & lockdown occurs
>>>> when the guest selects the features-ok file (for reading).
>>>>
>>>> Board code is allowed to choose the host feature bitmap to
>>>> advertise.
>>>>
>>>> This patch doesn't add any specific SMI features yet.
>>>>
>>>> - Patch #6 adds the ICH9_LPC_SMI_F_BROADCAST feature.
>>>>
>>>> - Patch #7 introduces the PCMachineClass.get_smi_host_features method,
>>>> and implements it for "pc-q35-2.9" and later. The idea is to tie the
>>>> SMI host features to machine types, and let the machine types
>>>> calculate their host features with code (i.e., not just a constant).
>>>>
>>>> In this patch, the "pc-q35-2.9" machine type exposes the
>>>> ICH9_LPC_SMI_F_BROADCAST feature iff (smp_cpus == max_cpus), that
>>>> is, when VCPU hotplug is not possible. (Also from Paolo's v3
>>>> feedback, in
>>>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg04919.html>.)
>>>>
>>>> * I've written the OVMF side patches too and tested them together
>>>> (including gdb / debug messages for "white box" testing).
>>>>
>>>> * Note that this version depends on the following PULL req from Michael:
>>>>
>>>> [Qemu-devel] [PULL 0/5] virtio, vhost, pc: fixes
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05503.html
>>>>
>>>> In particular on the following patch:
>>>> "loader: fix handling of custom address spaces when adding ROM blobs"
>>>>
>>>> Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Cc: Michael Walle <michael@walle.cc>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>>> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
>>>> Cc: qemu-arm@nongnu.org
>>>>
>>>> Thanks
>>>> Laszlo
>>>
>>> So I reviewed fw cfg stuff, looks good to me.
>>
>> Thank you.
>>
>> I should note that Igor suggested / requested changes for patches #2 and
>> #3; the sub-threads start at
>>
>> http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00695.html
>> msg-id: <20161206115038.4495a217@nial.brq.redhat.com>
>>
>> http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00706.html
>> msg-id: <20161206124906.76a41896@nial.brq.redhat.com>
>>
>> I was about to rework the patches based on his feedback. Should we
>> resume those discussions?
>
> If you both agree, pls go ahead and make these changes.
Okay, will do.
>
>>> I'd prefer Paolo to review
>>> and merge broadcast SMI stuff as appropriate. Paolo, makes sense?
>>
>> If it helps, I can split the series into two "waves", and deal only with
>> fw_cfg first.
>>
>> Thanks!
>> Laszlo
>
> I don't mind either way.
I'll see then how I feel about splitting v5 when I'm ready to post it. :)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI
2017-01-10 15:23 ` Laszlo Ersek
2017-01-10 16:05 ` Michael S. Tsirkin
@ 2017-01-10 16:21 ` Igor Mammedov
2017-01-10 16:30 ` Laszlo Ersek
1 sibling, 1 reply; 17+ messages in thread
From: Igor Mammedov @ 2017-01-10 16:21 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Peter Maydell, Eduardo Habkost, Michael S. Tsirkin,
Gabriel L. Somlo, qemu devel list, Michael Walle, qemu-arm,
Gerd Hoffmann, Shannon Zhao, Paolo Bonzini
On Tue, 10 Jan 2017 16:23:41 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/10/17 16:06, Michael S. Tsirkin wrote:
> > On Thu, Dec 01, 2016 at 06:06:17PM +0100, Laszlo Ersek wrote:
> >> * This is version 4 of the series; the last version was at
> >> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03582.html>.
> >>
> >> This version is practically a rewrite from scratch, seeking to address
> >> the v3 feedback. Here's what the individual patches do:
> >>
> >> - Patch #1 rebases and extends Michael's "writeable fw_cfg blobs"
> >> patch from Feb/March 2016. The changes relative to the original
> >> patch are documented in the commit message (the code changes are
> >> minimal).
> >>
> >> - Patches #2 and #3 turn the FW_CFG_FILE_SLOTS constant into a device
> >> property, and expose the desired number of fw_cfg file slots to
> >> board code. This is meant to address a concern raised by Paolo in
> >> the v2 review
> >> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03125.html>,
> >> namely that we're about to run out of FW_CFG_FILE_SLOTS.
> >>
> >> - Patch #4 introduces the "pc-q35-2.9" and "pc-i440fx-2.9" machine
> >> types, which are allowed to take advantage of the new default for
> >> fw_cfg file slots (0x20 rather than 0x10).
> >>
> >> - Patch #5 introduces SMI feature negotiation via fw_cfg, with the
> >> following new files:
> >>
> >> - etc/smi/host-features
> >> - etc/smi/guest-features
> >> - etc/smi/features-ok
> >>
> >> This is supposed to follow Michael's recommendation re: imitating
> >> virtio
> >> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03218.html>.
> >>
> >> The guest-features file is freely writeable by the guest (no write
> >> callbacks in fw_cfg), and the feature validation & lockdown occurs
> >> when the guest selects the features-ok file (for reading).
> >>
> >> Board code is allowed to choose the host feature bitmap to
> >> advertise.
> >>
> >> This patch doesn't add any specific SMI features yet.
> >>
> >> - Patch #6 adds the ICH9_LPC_SMI_F_BROADCAST feature.
> >>
> >> - Patch #7 introduces the PCMachineClass.get_smi_host_features method,
> >> and implements it for "pc-q35-2.9" and later. The idea is to tie the
> >> SMI host features to machine types, and let the machine types
> >> calculate their host features with code (i.e., not just a constant).
> >>
> >> In this patch, the "pc-q35-2.9" machine type exposes the
> >> ICH9_LPC_SMI_F_BROADCAST feature iff (smp_cpus == max_cpus), that
> >> is, when VCPU hotplug is not possible. (Also from Paolo's v3
> >> feedback, in
> >> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg04919.html>.)
> >>
> >> * I've written the OVMF side patches too and tested them together
> >> (including gdb / debug messages for "white box" testing).
> >>
> >> * Note that this version depends on the following PULL req from Michael:
> >>
> >> [Qemu-devel] [PULL 0/5] virtio, vhost, pc: fixes
> >> http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05503.html
> >>
> >> In particular on the following patch:
> >> "loader: fix handling of custom address spaces when adding ROM blobs"
> >>
> >> Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Eduardo Habkost <ehabkost@redhat.com>
> >> Cc: Gerd Hoffmann <kraxel@redhat.com>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Cc: Michael Walle <michael@walle.cc>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Peter Maydell <peter.maydell@linaro.org>
> >> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
> >> Cc: qemu-arm@nongnu.org
> >>
> >> Thanks
> >> Laszlo
> >
> > So I reviewed fw cfg stuff, looks good to me.
>
> Thank you.
>
> I should note that Igor suggested / requested changes for patches #2 and
> #3; the sub-threads start at
>
> http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00695.html
> msg-id: <20161206115038.4495a217@nial.brq.redhat.com>
>
> http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00706.html
> msg-id: <20161206124906.76a41896@nial.brq.redhat.com>
>
> I was about to rework the patches based on his feedback. Should we
> resume those discussions?
>
> > I'd prefer Paolo to review
> > and merge broadcast SMI stuff as appropriate. Paolo, makes sense?
>
> If it helps, I can split the series into two "waves", and deal only with
> fw_cfg first.
2 waves looks better as vmgeneration stuff would depend on fw_cfg patches
so it would be good to first. And SMI could be follow up.
>
> Thanks!
> Laszlo
>
> >
> >
> >> Laszlo Ersek (6):
> >> fw-cfg: turn FW_CFG_FILE_SLOTS into a device property
> >> fw-cfg: expose "file_slots" parameter in fw_cfg_init_io_dma()
> >> hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots
> >> hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg
> >> hw/isa/lpc_ich9: add broadcast SMI feature
> >> hw/i386/pc_q35: advertise broadcast SMI if VCPU hotplug is turned off
> >>
> >> Michael S. Tsirkin (1):
> >> fw-cfg: support writeable blobs
> >>
> >> docs/specs/fw_cfg.txt | 36 +++++++++---
> >> hw/lm32/lm32_hwsetup.h | 2 +-
> >> include/hw/compat.h | 11 ++++
> >> include/hw/i386/ich9.h | 15 ++++-
> >> include/hw/i386/pc.h | 4 ++
> >> include/hw/loader.h | 7 ++-
> >> include/hw/nvram/fw_cfg.h | 5 +-
> >> include/hw/nvram/fw_cfg_keys.h | 7 ++-
> >> hw/arm/virt-acpi-build.c | 2 +-
> >> hw/core/loader.c | 18 +++---
> >> hw/i386/acpi-build.c | 4 +-
> >> hw/i386/pc.c | 3 +-
> >> hw/i386/pc_piix.c | 16 +++++-
> >> hw/i386/pc_q35.c | 37 ++++++++++++-
> >> hw/isa/lpc_ich9.c | 92 +++++++++++++++++++++++++++++-
> >> hw/nvram/fw_cfg.c | 123 +++++++++++++++++++++++++++++++++++------
> >> 16 files changed, 326 insertions(+), 56 deletions(-)
> >>
> >> --
> >> 2.9.2
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-arm] [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI
2017-01-10 16:21 ` [Qemu-arm] " Igor Mammedov
@ 2017-01-10 16:30 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-01-10 16:30 UTC (permalink / raw)
To: Igor Mammedov
Cc: Peter Maydell, Eduardo Habkost, Michael S. Tsirkin,
Gabriel L. Somlo, qemu devel list, Michael Walle, qemu-arm,
Gerd Hoffmann, Shannon Zhao, Paolo Bonzini
On 01/10/17 17:21, Igor Mammedov wrote:
> On Tue, 10 Jan 2017 16:23:41 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> On 01/10/17 16:06, Michael S. Tsirkin wrote:
>>> On Thu, Dec 01, 2016 at 06:06:17PM +0100, Laszlo Ersek wrote:
>>>> * This is version 4 of the series; the last version was at
>>>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03582.html>.
>>>>
>>>> This version is practically a rewrite from scratch, seeking to address
>>>> the v3 feedback. Here's what the individual patches do:
>>>>
>>>> - Patch #1 rebases and extends Michael's "writeable fw_cfg blobs"
>>>> patch from Feb/March 2016. The changes relative to the original
>>>> patch are documented in the commit message (the code changes are
>>>> minimal).
>>>>
>>>> - Patches #2 and #3 turn the FW_CFG_FILE_SLOTS constant into a device
>>>> property, and expose the desired number of fw_cfg file slots to
>>>> board code. This is meant to address a concern raised by Paolo in
>>>> the v2 review
>>>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03125.html>,
>>>> namely that we're about to run out of FW_CFG_FILE_SLOTS.
>>>>
>>>> - Patch #4 introduces the "pc-q35-2.9" and "pc-i440fx-2.9" machine
>>>> types, which are allowed to take advantage of the new default for
>>>> fw_cfg file slots (0x20 rather than 0x10).
>>>>
>>>> - Patch #5 introduces SMI feature negotiation via fw_cfg, with the
>>>> following new files:
>>>>
>>>> - etc/smi/host-features
>>>> - etc/smi/guest-features
>>>> - etc/smi/features-ok
>>>>
>>>> This is supposed to follow Michael's recommendation re: imitating
>>>> virtio
>>>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg03218.html>.
>>>>
>>>> The guest-features file is freely writeable by the guest (no write
>>>> callbacks in fw_cfg), and the feature validation & lockdown occurs
>>>> when the guest selects the features-ok file (for reading).
>>>>
>>>> Board code is allowed to choose the host feature bitmap to
>>>> advertise.
>>>>
>>>> This patch doesn't add any specific SMI features yet.
>>>>
>>>> - Patch #6 adds the ICH9_LPC_SMI_F_BROADCAST feature.
>>>>
>>>> - Patch #7 introduces the PCMachineClass.get_smi_host_features method,
>>>> and implements it for "pc-q35-2.9" and later. The idea is to tie the
>>>> SMI host features to machine types, and let the machine types
>>>> calculate their host features with code (i.e., not just a constant).
>>>>
>>>> In this patch, the "pc-q35-2.9" machine type exposes the
>>>> ICH9_LPC_SMI_F_BROADCAST feature iff (smp_cpus == max_cpus), that
>>>> is, when VCPU hotplug is not possible. (Also from Paolo's v3
>>>> feedback, in
>>>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg04919.html>.)
>>>>
>>>> * I've written the OVMF side patches too and tested them together
>>>> (including gdb / debug messages for "white box" testing).
>>>>
>>>> * Note that this version depends on the following PULL req from Michael:
>>>>
>>>> [Qemu-devel] [PULL 0/5] virtio, vhost, pc: fixes
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg05503.html
>>>>
>>>> In particular on the following patch:
>>>> "loader: fix handling of custom address spaces when adding ROM blobs"
>>>>
>>>> Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Cc: Michael Walle <michael@walle.cc>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>>> Cc: Shannon Zhao <zhaoshenglong@huawei.com>
>>>> Cc: qemu-arm@nongnu.org
>>>>
>>>> Thanks
>>>> Laszlo
>>>
>>> So I reviewed fw cfg stuff, looks good to me.
>>
>> Thank you.
>>
>> I should note that Igor suggested / requested changes for patches #2 and
>> #3; the sub-threads start at
>>
>> http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00695.html
>> msg-id: <20161206115038.4495a217@nial.brq.redhat.com>
>>
>> http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00706.html
>> msg-id: <20161206124906.76a41896@nial.brq.redhat.com>
>>
>> I was about to rework the patches based on his feedback. Should we
>> resume those discussions?
>>
>>> I'd prefer Paolo to review
>>> and merge broadcast SMI stuff as appropriate. Paolo, makes sense?
>>
>> If it helps, I can split the series into two "waves", and deal only with
>> fw_cfg first.
> 2 waves looks better as vmgeneration stuff would depend on fw_cfg patches
> so it would be good to first. And SMI could be follow up.
Makes sense.
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-01-10 16:34 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-01 17:06 [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI Laszlo Ersek
2016-12-01 17:06 ` [Qemu-arm] [PATCH v4 1/7] fw-cfg: support writeable blobs Laszlo Ersek
2016-12-20 15:58 ` [Qemu-arm] [Qemu-devel] " Marcel Apfelbaum
2017-01-10 13:33 ` Laszlo Ersek
2016-12-20 23:01 ` [Qemu-arm] [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI no-reply
2016-12-21 15:22 ` [Qemu-arm] checkpatch.pl false positive (was Re: [Qemu-devel] [PATCH v4 0/7] q35: add negotiable broadcast SMI) Eduardo Habkost
2016-12-21 15:22 ` [Qemu-devel] checkpatch.pl false positive (was " Eduardo Habkost
2016-12-21 17:47 ` [Qemu-arm] " Paolo Bonzini
2016-12-21 18:01 ` Eduardo Habkost
2016-12-21 18:08 ` Paolo Bonzini
2016-12-21 18:19 ` [Qemu-devel] " Eduardo Habkost
2017-01-10 15:06 ` [Qemu-arm] [PATCH v4 0/7] q35: add negotiable broadcast SMI Michael S. Tsirkin
2017-01-10 15:23 ` Laszlo Ersek
2017-01-10 16:05 ` Michael S. Tsirkin
2017-01-10 16:19 ` [Qemu-devel] " Laszlo Ersek
2017-01-10 16:21 ` [Qemu-arm] " Igor Mammedov
2017-01-10 16:30 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).