[Qemu-devel] [PATCH v3 1/3] hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500

[Rashmica Gupta]

GPIO pins are arranged in groups of 8 pins labeled A,B,..,Y,Z,AA,AB,AC.
(Note that the ast2400 controller only goes up to group AB).
A set has four groups (except set AC which only has one) and is
referred to by the groups it is composed of (eg ABCD,EFGH,...,YZAAAB).
Each set is accessed and controlled by a bank of 14 registers.

These registers operate on a per pin level where each bit in the register
corresponds to a pin, except for the command source registers. The command
source registers operate on a per group level where bits 24, 16, 8 and 0
correspond to each group in the set.

 eg. registers for set ABCD:
 |D7...D0|C7...C0|B7...B0|A7...A0| <- GPIOs
 |31...24|23...16|158|7.0| <- bit position

Note that there are a couple of groups that only have 4 pins.

There are two ways that this model deviates from the behaviour of the
actual controller:
(1) The only control source driving the GPIO pins in the model is the ARM
model (as there currently aren't models for the LPC or Coprocessor).

(2) None of the registers in the model are reset tolerant (needs
integration with the watchdog).

Signed-off-by: Rashmica Gupta 
---
 hw/gpio/Makefile.objs |   1 +
 hw/gpio/aspeed_gpio.c | 923 ++
 include/hw/gpio/aspeed_gpio.h |  91 
 3 files changed, 1015 insertions(+)
 create mode 100644 hw/gpio/aspeed_gpio.c
 create mode 100644 include/hw/gpio/aspeed_gpio.h

diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
index e5da0cb54f..d305b3b24b 100644
--- a/hw/gpio/Makefile.objs
+++ b/hw/gpio/Makefile.objs
@@ -9,3 +9,4 @@ obj-$(CONFIG_OMAP) += omap_gpio.o
 obj-$(CONFIG_IMX) += imx_gpio.o
 obj-$(CONFIG_RASPI) += bcm2835_gpio.o
 obj-$(CONFIG_NRF51_SOC) += nrf51_gpio.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_gpio.o
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
new file mode 100644
index 00..4f1546a900
--- /dev/null
+++ b/hw/gpio/aspeed_gpio.c
@@ -0,0 +1,923 @@
+/*
+ *  ASPEED GPIO Controller
+ *
+ *  Copyright (C) 2017-2019 IBM Corp.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/gpio/aspeed_gpio.h"
+#include "include/hw/misc/aspeed_scu.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+
+#define GPIOS_PER_REG 32
+#define GPIOS_PER_SET GPIOS_PER_REG
+#define GPIO_3_6V_REG_ARRAY_SIZE  (0x1f0 >> 2)
+#define GPIO_PIN_GAP_SIZE 4
+#define GPIOS_PER_GROUP 8
+#define GPIO_GROUP_SHIFT 3
+
+/* GPIO Source Types */
+#define ASPEED_CMD_SRC_MASK 0x01010101
+#define ASPEED_SOURCE_ARM   0
+#define ASPEED_SOURCE_LPC   1
+#define ASPEED_SOURCE_COPROCESSOR   2
+#define ASPEED_SOURCE_RESERVED  3
+
+/* GPIO Interrupt Triggers */
+/*
+ *  For each set of gpios there are three sensitivity registers that control
+ *  the interrupt trigger mode.
+ *
+ *  | 2 | 1 | 0 | trigger mode
+ *  -
+ *  | 0 | 0 | 0 | falling-edge
+ *  | 0 | 0 | 1 | rising-edge
+ *  | 0 | 1 | 0 | level-low
+ *  | 0 | 1 | 1 | level-high
+ *  | 1 | X | X | dual-edge
+ */
+#define ASPEED_FALLING_EDGE 0
+#define ASPEED_RISING_EDGE  1
+#define ASPEED_LEVEL_LOW2
+#define ASPEED_LEVEL_HIGH   3
+#define ASPEED_DUAL_EDGE4
+
+/* GPIO Register Address Offsets */
+#define GPIO_ABCD_DATA_VALUE   (0x000 >> 2)
+#define GPIO_ABCD_DIRECTION(0x004 >> 2)
+#define GPIO_ABCD_INT_ENABLE   (0x008 >> 2)
+#define GPIO_ABCD_INT_SENS_0   (0x00C >> 2)
+#define GPIO_ABCD_INT_SENS_1   (0x010 >> 2)
+#define GPIO_ABCD_INT_SENS_2   (0x014 >> 2)
+#define GPIO_ABCD_INT_STATUS   (0x018 >> 2)
+#define GPIO_ABCD_RESET_TOLERANT   (0x01C >> 2)
+#define GPIO_EFGH_DATA_VALUE   (0x020 >> 2)
+#define GPIO_EFGH_DIRECTION(0x024 >> 2)
+#define GPIO_EFGH_INT_ENABLE   (0x028 >> 2)
+#define GPIO_EFGH_INT_SENS_0   (0x02C >> 2)
+#define GPIO_EFGH_INT_SENS_1   (0x030 >> 2)
+#define GPIO_EFGH_INT_SENS_2   (0x034 >> 2)
+#define GPIO_EFGH_INT_STATUS   (0x038 >> 2)
+#define GPIO_EFGH_RESET_TOLERANT   (0x03C >> 2)
+#define GPIO_ABCD_DEBOUNCE_1   (0x040 >> 2)
+#define GPIO_ABCD_DEBOUNCE_2   (0x044 >> 2)
+#define GPIO_EFGH_DEBOUNCE_1   (0x048 >> 2)
+#define GPIO_EFGH_DEBOUNCE_2   (0x04C >> 2)
+#define GPIO_DEBOUNCE_TIME_1   (0x050 >> 2)
+#define GPIO_DEBOUNCE_TIME_2   (0x054 >> 2)
+#define GPIO_DEBOUNCE_TIME_3   (0x058 >> 2)
+#define GPIO_ABCD_COMMAND_SRC_0(0x060 >> 2)
+#define GPIO_ABCD_COMMAND_SRC_1(0x064 >> 2)
+#define GPIO_EFGH_COMMAND_SRC_0(0x068 >> 2)
+#define GPIO_EFGH_COMMAND_SRC_1(0x06C >> 2)
+#define GPIO_IJKL_DATA_VALUE   (0x070 >> 2)
+#define GPIO_IJKL_DIRECTION(0x074 >> 2)
+#define GPIO_MNOP_DATA_VALUE   (0x078 >> 2)
+#define GPIO_MNOP_DIRECTION(0x07C >> 2)
+#define GPIO_QRST_DATA_VALUE   (0x080 >> 2)
+#define GPIO_QRST_DIRECTION(0x084 >> 2)
+#define GPIO_UVWX_DATA_VALUE   (0x088 >> 2)
+#define GPIO_UWVX_DIRECTION(0x08C >> 2)
+#define 

[Qemu-devel] [PATCH v3 3/3] hw/gpio: Add in AST2600 specific implementation

[Rashmica Gupta]

The AST2600 has the same sets of 3.6v gpios as the AST2400 plus an
addtional two sets of 1.8V gpios.

Signed-off-by: Rashmica Gupta 
---
 hw/gpio/aspeed_gpio.c | 186 +-
 include/hw/gpio/aspeed_gpio.h |   2 +-
 2 files changed, 184 insertions(+), 4 deletions(-)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 4f1546a900..a94b5f2780 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -16,6 +16,7 @@
 #define GPIOS_PER_REG 32
 #define GPIOS_PER_SET GPIOS_PER_REG
 #define GPIO_3_6V_REG_ARRAY_SIZE  (0x1f0 >> 2)
+#define GPIO_1_8V_REG_ARRAY_SIZE  ((0x9d8 - 0x800) >> 2)
 #define GPIO_PIN_GAP_SIZE 4
 #define GPIOS_PER_GROUP 8
 #define GPIO_GROUP_SHIFT 3
@@ -145,6 +146,7 @@
 #define GPIO_YZAAAB_DEBOUNCE_1 (0x190 >> 2)
 #define GPIO_YZAAAB_DEBOUNCE_2 (0x194 >> 2)
 #define GPIO_YZAAAB_INPUT_MASK (0x198 >> 2)
+/* AST2500 only */
 #define GPIO_AC_COMMAND_SRC_0  (0x1A0 >> 2)
 #define GPIO_AC_COMMAND_SRC_1  (0x1A4 >> 2)
 #define GPIO_AC_INT_ENABLE (0x1A8 >> 2)
@@ -163,6 +165,47 @@
 #define GPIO_AC_DATA_VALUE (0x1E8 >> 2)
 #define GPIO_AC_DIRECTION  (0x1EC >> 2)
 
+
+/* AST2600 only - 1.8V gpios */
+/*
+ * The AST2600 has same 3.6V gpios as the AST2400 (memory offsets 0x0-0x198)
+ * and addtional 1.8V gpios (memory offsets 0x800-0x9D4). We create one
+ * GPIOState for the 3.6V gpios mapped at 0x0 and another GPIOState for the
+ * 1.8V gpios mapped at 0x800.
+ */
+#define GPIO_1_8_REG_OFFSET  0x800
+#define GPIO_1_8_ABCD_DATA_VALUE ((0x800 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_DIRECTION  ((0x804 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_INT_ENABLE ((0x808 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_INT_SENS_0 ((0x80C - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_INT_SENS_1 ((0x810 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_INT_SENS_2 ((0x814 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_INT_STATUS ((0x818 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_RESET_TOLERANT ((0x81C - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_DATA_VALUE((0x820 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_DIRECTION ((0x824 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_INT_ENABLE((0x828 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_INT_SENS_0((0x82C - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_INT_SENS_1((0x830 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_INT_SENS_2((0x834 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_INT_STATUS((0x838 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_RESET_TOLERANT((0x83C - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_DEBOUNCE_1 ((0x840 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_DEBOUNCE_2 ((0x844 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_DEBOUNCE_1((0x848 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_DEBOUNCE_2((0x84C - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_DEBOUNCE_TIME_1 ((0x850 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_DEBOUNCE_TIME_2 ((0x854 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_DEBOUNCE_TIME_3 ((0x858 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_COMMAND_SRC_0  ((0x860 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_COMMAND_SRC_1  ((0x864 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_COMMAND_SRC_0 ((0x868 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_COMMAND_SRC_1 ((0x86C - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_DATA_READ  ((0x8C0 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_DATA_READ ((0x8C4 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_ABCD_INPUT_MASK ((0x9D0 - GPIO_1_8_REG_OFFSET) >> 2)
+#define GPIO_1_8_E_INPUT_MASK((0x9D4 - GPIO_1_8_REG_OFFSET) >> 2)
+
 static int aspeed_evaluate_irq(GPIOSets *regs, int gpio_prev_high, int gpio)
 {
 uint32_t falling_edge = 0, rising_edge = 0;
@@ -626,6 +669,39 @@ static const AspeedGPIOReg 
aspeed_3_6v_gpios[GPIO_3_6V_REG_ARRAY_SIZE] = {
 [GPIO_AC_INPUT_MASK] = {7, read_input_mask, write_input_mask},
 };
 
+static const AspeedGPIOReg aspeed_1_8v_gpios[GPIO_1_8V_REG_ARRAY_SIZE] = {
+/* 1.8V Set ABCD */
+[GPIO_1_8_ABCD_DATA_VALUE] = {0, read_data_value, write_data_value},
+[GPIO_1_8_ABCD_DIRECTION] =  {0, read_direction, write_direction},
+[GPIO_1_8_ABCD_INT_ENABLE] = {0, read_int_enable, write_int_enable},
+[GPIO_1_8_ABCD_INT_SENS_0] = {0, read_int_sens_0, write_int_sens_0},
+[GPIO_1_8_ABCD_INT_SENS_1] = {0, read_int_sens_1, write_int_sens_1},
+[GPIO_1_8_ABCD_INT_SENS_2] = {0, read_int_sens_2, write_int_sens_2},
+[GPIO_1_8_ABCD_INT_STATUS] = {0, read_int_status, write_int_status},
+[GPIO_1_8_ABCD_RESET_TOLERANT] = {0, read_reset_tol, write_reset_tol},
+[GPIO_1_8_ABCD_DEBOUNCE_1] = {0, read_debounce_1, write_debounce_1},
+[GPIO_1_8_ABCD_DEBOUNCE_2] =

[Qemu-devel] [PATCH v3 0/3] Add Aspeed GPIO controller model

[Rashmica Gupta]

There are a couple of things I'm not confident about here:
- what should be in init vs realize?
- should the irq state be in vmstate?
- is there a better way to do composition of classes (patch 3)?

v3:
- didn't have each gpio set up as an irq 
- now can't access set AC on ast2400 (only exists on ast2500)
- added ast2600 implementation (patch 3)
- renamed a couple of variables for clarity


v2: Addressed Andrew's feedback, added debounce regs, renamed get/set to
read/write to minimise confusion with a 'set' of registers.

Rashmica Gupta (3):
  hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500
  aspeed: add a GPIO controller to the SoC
  hw/gpio: Add in AST2600 specific implementation

 hw/arm/aspeed_soc.c   |   17 +
 hw/gpio/Makefile.objs |1 +
 hw/gpio/aspeed_gpio.c | 1103 +
 include/hw/arm/aspeed_soc.h   |3 +
 include/hw/gpio/aspeed_gpio.h |   91 +++
 5 files changed, 1215 insertions(+)
 create mode 100644 hw/gpio/aspeed_gpio.c
 create mode 100644 include/hw/gpio/aspeed_gpio.h

-- 
2.20.1

[Qemu-devel] [PATCH v3 2/3] aspeed: add a GPIO controller to the SoC

[Rashmica Gupta]

Signed-off-by: Rashmica Gupta 
---
 hw/arm/aspeed_soc.c | 17 +
 include/hw/arm/aspeed_soc.h |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index c6fb3700f2..ff422c8ad1 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -124,6 +124,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
 .spis_num = 1,
 .fmc_typename = "aspeed.smc.fmc",
 .spi_typename = aspeed_soc_ast2400_typenames,
+.gpio_typename = "aspeed.gpio-ast2400",
 .wdts_num = 2,
 .irqmap   = aspeed_soc_ast2400_irqmap,
 .memmap   = aspeed_soc_ast2400_memmap,
@@ -136,6 +137,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
 .spis_num = 1,
 .fmc_typename = "aspeed.smc.fmc",
 .spi_typename = aspeed_soc_ast2400_typenames,
+.gpio_typename = "aspeed.gpio-ast2400",
 .wdts_num = 2,
 .irqmap   = aspeed_soc_ast2400_irqmap,
 .memmap   = aspeed_soc_ast2400_memmap,
@@ -148,6 +150,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
 .spis_num = 1,
 .fmc_typename = "aspeed.smc.fmc",
 .spi_typename = aspeed_soc_ast2400_typenames,
+.gpio_typename = "aspeed.gpio-ast2400",
 .wdts_num = 2,
 .irqmap   = aspeed_soc_ast2400_irqmap,
 .memmap   = aspeed_soc_ast2400_memmap,
@@ -160,6 +163,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
 .spis_num = 2,
 .fmc_typename = "aspeed.smc.ast2500-fmc",
 .spi_typename = aspeed_soc_ast2500_typenames,
+.gpio_typename = "aspeed.gpio-ast2500",
 .wdts_num = 3,
 .irqmap   = aspeed_soc_ast2500_irqmap,
 .memmap   = aspeed_soc_ast2500_memmap,
@@ -246,6 +250,9 @@ static void aspeed_soc_init(Object *obj)
 
 sysbus_init_child_obj(obj, "xdma", OBJECT(>xdma), sizeof(s->xdma),
   TYPE_ASPEED_XDMA);
+
+sysbus_init_child_obj(obj, "gpio", OBJECT(>gpio), sizeof(s->gpio),
+  sc->info->gpio_typename);
 }
 
 static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -425,6 +432,16 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
 sc->info->memmap[ASPEED_XDMA]);
 sysbus_connect_irq(SYS_BUS_DEVICE(>xdma), 0,
aspeed_soc_get_irq(s, ASPEED_XDMA));
+
+/* GPIO */
+object_property_set_bool(OBJECT(>gpio), true, "realized", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(>gpio), 0, 
sc->info->memmap[ASPEED_GPIO]);
+sysbus_connect_irq(SYS_BUS_DEVICE(>gpio), 0,
+   aspeed_soc_get_irq(s, ASPEED_GPIO));
 }
 static Property aspeed_soc_properties[] = {
 DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index cef605ad6b..fa04abddd8 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -22,6 +22,7 @@
 #include "hw/ssi/aspeed_smc.h"
 #include "hw/watchdog/wdt_aspeed.h"
 #include "hw/net/ftgmac100.h"
+#include "hw/gpio/aspeed_gpio.h"
 
 #define ASPEED_SPIS_NUM  2
 #define ASPEED_WDTS_NUM  3
@@ -47,6 +48,7 @@ typedef struct AspeedSoCState {
 AspeedSDMCState sdmc;
 AspeedWDTState wdt[ASPEED_WDTS_NUM];
 FTGMAC100State ftgmac100[ASPEED_MACS_NUM];
+AspeedGPIOState gpio;
 } AspeedSoCState;
 
 #define TYPE_ASPEED_SOC "aspeed-soc"
@@ -60,6 +62,7 @@ typedef struct AspeedSoCInfo {
 int spis_num;
 const char *fmc_typename;
 const char **spi_typename;
+const char *gpio_typename;
 int wdts_num;
 const int *irqmap;
 const hwaddr *memmap;
-- 
2.20.1

[Qemu-devel] [PATCH] migration: always initial ram_counters for a new migration

[Ivan Ren]

From: Ivan Ren 

This patch fix a multifd migration bug in migration speed calculation, this
problem can be reproduced as follows:
1. start a vm and give a heavy memory write stress to prevent the vm be
   successfully migrated to destination
2. begin a migration with multifd
3. migrate for a long time [actually, this can be measured by transferred bytes]
4. migrate cancel
5. begin a new migration with multifd, the migration will directly run into
   migration_completion phase

Reason as follows:

Migration update bandwidth and s->threshold_size in function
migration_update_counters after BUFFER_DELAY time:

current_bytes = migration_total_bytes(s);
transferred = current_bytes - s->iteration_initial_bytes;
time_spent = current_time - s->iteration_start_time;
bandwidth = (double)transferred / time_spent;
s->threshold_size = bandwidth * s->parameters.downtime_limit;

In multifd migration, migration_total_bytes function return
qemu_ftell(s->to_dst_file) + ram_counters.multifd_bytes.
s->iteration_initial_bytes will be initialized to 0 at every new migration,
but ram_counters is a global variable, and history migration data will be
accumulated. So if the ram_counters.multifd_bytes is big enough, it may lead
pending_size >= s->threshold_size become false in migration_iteration_run
after the first migration_update_counters.

Signed-off-by: Ivan Ren 
---
 migration/migration.c | 15 ++-
 migration/savevm.c|  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 8a607fe1e2..d35a6ae6f9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1908,6 +1908,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, 
bool blk_inc,
 }
 
 migrate_init(s);
+/*
+ * set ram_counters memory to zero for a
+ * new migration
+ */
+memset(_counters, 0, sizeof(ram_counters));
 
 return true;
 }
@@ -3187,6 +3192,10 @@ static void *migration_thread(void *opaque)
 
 object_ref(OBJECT(s));
 s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+/*
+ * Update s->iteration_initial_bytes to match s->iteration_start_time.
+ */
+s->iteration_initial_bytes = migration_total_bytes(s);
 
 qemu_savevm_state_header(s->to_dst_file);
 
@@ -3252,7 +3261,11 @@ static void *migration_thread(void *opaque)
  * breaking transferred_bytes and bandwidth calculation
  */
 s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-s->iteration_initial_bytes = 0;
+/*
+ * Update s->iteration_initial_bytes to current size to
+ * avoid historical data lead wrong bandwidth.
+ */
+s->iteration_initial_bytes = migration_total_bytes(s);
 }
 
 current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
diff --git a/migration/savevm.c b/migration/savevm.c
index 79ed44d475..480c511b19 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1424,6 +1424,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 }
 
 migrate_init(ms);
+memset(_counters, 0, sizeof(ram_counters));
 ms->to_dst_file = f;
 
 qemu_mutex_unlock_iothread();
-- 
2.17.2 (Apple Git-113)

[Qemu-devel] [PATCH v3 1/3] migration: add qemu_file_update_transfer interface

[Ivan Ren]

From: Ivan Ren 

Add qemu_file_update_transfer for just update bytes_xfer for speed
limitation. This will be used for further migration feature such as
multifd migration.

Signed-off-by: Ivan Ren 
Reviewed-by: Wei Yang  
---
 migration/qemu-file.c | 5 +
 migration/qemu-file.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 0431585502..18f480529a 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -615,6 +615,11 @@ void qemu_file_reset_rate_limit(QEMUFile *f)
 f->bytes_xfer = 0;
 }
 
+void qemu_file_update_transfer(QEMUFile *f, int64_t len)
+{
+f->bytes_xfer += len;
+}
+
 void qemu_put_be16(QEMUFile *f, unsigned int v)
 {
 qemu_put_byte(f, v >> 8);
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 13baf896bd..5de9fa2e96 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -147,6 +147,7 @@ int qemu_peek_byte(QEMUFile *f, int offset);
 void qemu_file_skip(QEMUFile *f, int size);
 void qemu_update_position(QEMUFile *f, size_t size);
 void qemu_file_reset_rate_limit(QEMUFile *f);
+void qemu_file_update_transfer(QEMUFile *f, int64_t len);
 void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
 int64_t qemu_file_get_rate_limit(QEMUFile *f);
 void qemu_file_set_error(QEMUFile *f, int ret);
-- 
2.17.2 (Apple Git-113)

[Qemu-devel] [PATCH v3 3/3] migration: update ram_counters for multifd sync packet

[Ivan Ren]

From: Ivan Ren 

Multifd sync will send MULTIFD_FLAG_SYNC flag info to destination, add
these bytes to ram_counters record.

Signed-off-by: Ivan Ren 
Suggested-by: Wei Yang 
---
 migration/ram.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index 88ddd2bbe2..20b6eebb7c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1085,6 +1085,8 @@ static void multifd_send_sync_main(RAMState *rs)
 p->flags |= MULTIFD_FLAG_SYNC;
 p->pending_job++;
 qemu_file_update_transfer(rs->f, p->packet_len);
+ram_counters.multifd_bytes += p->packet_len;
+ram_counters.transferred += p->packet_len;
 qemu_mutex_unlock(>mutex);
 qemu_sem_post(>sem);
 }
-- 
2.17.2 (Apple Git-113)

[Qemu-devel] [PATCH v2 0/3] migration: add speed limit for multifd migration

[Ivan Ren]

From: Ivan Ren 

Currently multifd migration has not been limited and it will consume
the whole bandwidth of Nic. These two patches add speed limitation to
it.

This is the v3 patches:

v3 VS v2:
Add Reviewed info and Suggested info.

v2 VS v1:
1. change qemu_file_update_rate_transfer interface name
   to qemu_file_update_transfer
2. add a new patch to update ram_counters for multifd sync packet

Ivan Ren (3):
  migration: add qemu_file_update_transfer interface
  migration: add speed limit for multifd migration
  migration: update ram_counters for multifd sync packet

 migration/qemu-file.c |  5 +
 migration/qemu-file.h |  1 +
 migration/ram.c   | 24 ++--
 3 files changed, 20 insertions(+), 10 deletions(-)

-- 
2.17.2 (Apple Git-113)

[Qemu-devel] [PATCH v3 2/3] migration: add speed limit for multifd migration

[Ivan Ren]

From: Ivan Ren 

Limit the speed of multifd migration through common speed limitation
qemu file.

Signed-off-by: Ivan Ren 
---
 migration/ram.c | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 889148dd84..88ddd2bbe2 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -922,7 +922,7 @@ struct {
  * false.
  */
 
-static int multifd_send_pages(void)
+static int multifd_send_pages(RAMState *rs)
 {
 int i;
 static int next_channel;
@@ -954,6 +954,7 @@ static int multifd_send_pages(void)
 multifd_send_state->pages = p->pages;
 p->pages = pages;
 transferred = ((uint64_t) pages->used) * TARGET_PAGE_SIZE + p->packet_len;
+qemu_file_update_transfer(rs->f, transferred);
 ram_counters.multifd_bytes += transferred;
 ram_counters.transferred += transferred;;
 qemu_mutex_unlock(>mutex);
@@ -962,7 +963,7 @@ static int multifd_send_pages(void)
 return 1;
 }
 
-static int multifd_queue_page(RAMBlock *block, ram_addr_t offset)
+static int multifd_queue_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
 {
 MultiFDPages_t *pages = multifd_send_state->pages;
 
@@ -981,12 +982,12 @@ static int multifd_queue_page(RAMBlock *block, ram_addr_t 
offset)
 }
 }
 
-if (multifd_send_pages() < 0) {
+if (multifd_send_pages(rs) < 0) {
 return -1;
 }
 
 if (pages->block != block) {
-return  multifd_queue_page(block, offset);
+return  multifd_queue_page(rs, block, offset);
 }
 
 return 1;
@@ -1054,7 +1055,7 @@ void multifd_save_cleanup(void)
 multifd_send_state = NULL;
 }
 
-static void multifd_send_sync_main(void)
+static void multifd_send_sync_main(RAMState *rs)
 {
 int i;
 
@@ -1062,7 +1063,7 @@ static void multifd_send_sync_main(void)
 return;
 }
 if (multifd_send_state->pages->used) {
-if (multifd_send_pages() < 0) {
+if (multifd_send_pages(rs) < 0) {
 error_report("%s: multifd_send_pages fail", __func__);
 return;
 }
@@ -1083,6 +1084,7 @@ static void multifd_send_sync_main(void)
 p->packet_num = multifd_send_state->packet_num++;
 p->flags |= MULTIFD_FLAG_SYNC;
 p->pending_job++;
+qemu_file_update_transfer(rs->f, p->packet_len);
 qemu_mutex_unlock(>mutex);
 qemu_sem_post(>sem);
 }
@@ -2079,7 +2081,7 @@ static int ram_save_page(RAMState *rs, PageSearchStatus 
*pss, bool last_stage)
 static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
  ram_addr_t offset)
 {
-if (multifd_queue_page(block, offset) < 0) {
+if (multifd_queue_page(rs, block, offset) < 0) {
 return -1;
 }
 ram_counters.normal++;
@@ -3482,7 +3484,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 ram_control_before_iterate(f, RAM_CONTROL_SETUP);
 ram_control_after_iterate(f, RAM_CONTROL_SETUP);
 
-multifd_send_sync_main();
+multifd_send_sync_main(*rsp);
 qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
 qemu_fflush(f);
 
@@ -3570,7 +3572,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 ram_control_after_iterate(f, RAM_CONTROL_ROUND);
 
 out:
-multifd_send_sync_main();
+multifd_send_sync_main(rs);
 qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
 qemu_fflush(f);
 ram_counters.transferred += 8;
@@ -3629,7 +3631,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
 
 rcu_read_unlock();
 
-multifd_send_sync_main();
+multifd_send_sync_main(rs);
 qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
 qemu_fflush(f);
 
-- 
2.17.2 (Apple Git-113)

Re: [Qemu-devel] [PATCH v2 3/3] migration: update ram_counters for multifd sync packet

[Ivan Ren]

Thanks
I'll send a new version with these suggest info and review info.

On Tue, Jul 30, 2019 at 8:42 AM Wei Yang 
wrote:

> On Mon, Jul 29, 2019 at 04:01:21PM +0800, Ivan Ren wrote:
> >Multifd sync will send MULTIFD_FLAG_SYNC flag info to destination, add
> >these bytes to ram_counters record.
> >
> >Signed-off-by: Ivan Ren 
>
> Also this is suggested by me, it would be better to add Suggested-by.
>
> >---
> > migration/ram.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> >diff --git a/migration/ram.c b/migration/ram.c
> >index 88ddd2bbe2..20b6eebb7c 100644
> >--- a/migration/ram.c
> >+++ b/migration/ram.c
> >@@ -1085,6 +1085,8 @@ static void multifd_send_sync_main(RAMState *rs)
> > p->flags |= MULTIFD_FLAG_SYNC;
> > p->pending_job++;
> > qemu_file_update_transfer(rs->f, p->packet_len);
> >+ram_counters.multifd_bytes += p->packet_len;
> >+ram_counters.transferred += p->packet_len;
> > qemu_mutex_unlock(>mutex);
> > qemu_sem_post(>sem);
> > }
> >--
> >2.17.2 (Apple Git-113)
> >
>
> --
> Wei Yang
> Help you, Help me
>

Re: [Qemu-devel] [PATCH 04/67] target/arm: Remove offset argument to gen_exception_internal_insn

[Richard Henderson]

On 7/29/19 6:52 AM, Peter Maydell wrote:
> I'm not so convinced about this one -- gen_exception_insn()
> and gen_exception_internal_insn() shouldn't have the
> same pattern of function prototype but different semantics
> like this, it's confusing. It happens that both the cases
> of wanting to generate an "internal" exception happen to want
> it to be taken with the PC being for the following insn,
> not the current one, but that seems more coincidence to
> me than anything else.

I don't like "offsets", because they don't work as expected between different
modes.  Would you prefer the pc in full be passed in?  Would you prefer that
the previous patches also pass in a pc, instead of implicitly using
base.pc_next (you had rationale vs patch 2 for why it was ok as-is).

Shall we shuffle these patches later, after the Great Renaming of Things Named
PC, as discussed wrt patch 6 (pc_read and friends), so that the "offset"
parameter immediately becomes the Right Sort of PC, rather than some
intermediary confusion?


r~

[Qemu-devel] [PATCH] ati-vga: Add limited support for big endian frame buffer aperture

[BALATON Zoltan]

Set frame buffer endianness according to requested endianness for
frame buffer aperture 0. This fixes inverted colors with Xorg frame
buffer driver. Setting endianness of aperture 1 and reg aperture are
not implemented.

Signed-off-by: BALATON Zoltan 
---
 hw/display/ati.c  | 9 -
 hw/display/ati_int.h  | 1 +
 hw/display/ati_regs.h | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index d1db07dd2f..84961d193f 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -90,7 +90,8 @@ static void ati_vga_switch_mode(ATIVGAState *s)
 DPRINTF("Switching to %dx%d %d %d @ %x\n", h, v, stride, bpp, 
offs);
 vbe_ioport_write_index(>vga, 0, VBE_DISPI_INDEX_ENABLE);
 vbe_ioport_write_data(>vga, 0, VBE_DISPI_DISABLED);
-s->vga.big_endian_fb = false;
+s->vga.big_endian_fb = (s->regs.config_cntl & APER_0_ENDIAN ?
+true : false);
 /* reset VBE regs then set up mode */
 s->vga.vbe_regs[VBE_DISPI_INDEX_XRES] = h;
 s->vga.vbe_regs[VBE_DISPI_INDEX_YRES] = v;
@@ -311,6 +312,9 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, 
unsigned int size)
 case PALETTE_DATA:
 val = vga_ioport_read(>vga, VGA_PEL_D);
 break;
+case CNFG_CNTL:
+val = s->regs.config_cntl;
+break;
 case CNFG_MEMSIZE:
 val = s->vga.vram_size;
 break;
@@ -605,6 +609,9 @@ static void ati_mm_write(void *opaque, hwaddr addr,
 data >>= 8;
 vga_ioport_write(>vga, VGA_PEL_D, data & 0xff);
 break;
+case CNFG_CNTL:
+s->regs.config_cntl = data;
+break;
 case CRTC_H_TOTAL_DISP:
 s->regs.crtc_h_total_disp = data & 0x07ff07ff;
 break;
diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index 31a1927b3e..5b4d3be1e6 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -39,6 +39,7 @@ typedef struct ATIVGARegs {
 uint32_t gpio_vga_ddc;
 uint32_t gpio_dvi_ddc;
 uint32_t gpio_monid;
+uint32_t config_cntl;
 uint32_t crtc_h_total_disp;
 uint32_t crtc_h_sync_strt_wid;
 uint32_t crtc_v_total_disp;
diff --git a/hw/display/ati_regs.h b/hw/display/ati_regs.h
index 81fb5302c0..11a54caa83 100644
--- a/hw/display/ati_regs.h
+++ b/hw/display/ati_regs.h
@@ -313,6 +313,7 @@
 #define X_MPLL_REF_DIV_MASK 0x00FF
 
 /* Config control values (CONFIG_CNTL) */
+#define APER_0_ENDIAN   0x0003
 #define CFG_VGA_IO_DIS  0x0400
 
 /* CRTC control values (CRTC_GEN_CNTL) */
-- 
2.13.7

Re: [Qemu-devel] [PATCH 01/67] decodetree: Allow !function with no input bits

[Richard Henderson]

On 7/29/19 6:43 AM, Peter Maydell wrote:
> On Fri, 26 Jul 2019 at 18:50, Richard Henderson
>  wrote:
>>
>> With this, we can have the function return a value from the DisasContext.
>>
>> Signed-off-by: Richard Henderson 
>> ---
>>  scripts/decodetree.py | 5 -
>>  tests/decode/succ_function.decode | 2 ++
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/decode/succ_function.decode
>>
>> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
>> index d7a59d63ac..4259d87a95 100755
>> --- a/scripts/decodetree.py
>> +++ b/scripts/decodetree.py
>> @@ -195,7 +195,10 @@ class MultiField:
>>  """Class representing a compound instruction field"""
>>  def __init__(self, subs, mask):
>>  self.subs = subs
>> -self.sign = subs[0].sign
>> +if len(subs):
>> +self.sign = subs[0].sign
>> +else:
>> +self.sign = 0
>>  self.mask = mask
>>
>>  def __str__(self):
>> diff --git a/tests/decode/succ_function.decode 
>> b/tests/decode/succ_function.decode
>> new file mode 100644
>> index 00..632a9de252
>> --- /dev/null
>> +++ b/tests/decode/succ_function.decode
>> @@ -0,0 +1,2 @@
>> +%foo  !function=foo
>> +foo    %foo
>> --
>> 2.17.1
> 
> Could you also update the documentation in docs/devel/decodetree.rst ?
> 
> This code change looks like it also now allows definitions
> of fields that specify nothing at all (ie there's no check
> that a field definition with no "unnamed_field" parts has
> a !function specifier) -- what do they do, or should they
> be made syntax errors ?

Ah good point.  Should be syntax errors.

> Is one of these functions which just returns a constant
> from no input bits still a "static int func(DisasContext *s, int x)"
> taking a pointless input argument, or is it now a
> "static int func(DisasContext *s)" ? I guess from the fact
> this code doesn't change the way a call is output that it
> is the former, but would the latter be cleaner ?

Right on both counts.  Because of how the loop in MultiField is set up, x will
always be given 0.  I'll see about cleaning this up.

In the meantime, fyi, this is used for setting the S bit for thumb1 insns,
where S=0 iff the insn is within an IT block.


r~

Re: [Qemu-devel] [PATCH 13/67] target/arm: Convert Data Processing (reg, reg-shifted-reg, imm)

[Richard Henderson]

On 7/29/19 8:25 AM, Peter Maydell wrote:
> I'm afraid this patch is too big for me to digest :-(
> 
> I just spent about half an hour trying to figure out whether
> the changes just to the thumb dp-immediate insns were right
> and didn't manage to work through it all.

Hmm.  It is probably the largest of the patches, and is caused by me trying to
do all of the insns handled by that one A32 code block.

I could perhaps split this to add fewer insns at a time, but I'd have to leave
the legacy decoder alone until the last patch, when it would go away all at
once.  I'm not sure if that is more or less reviewable.


> Why do we split it up into all these different kinds of patterns
> where some insns have special cases for rn==15 and some have
> special cases for rd==15 ?
> The legacy decoder doesn't seem to do that -- it treats everything
> the same.

It sorta special cases them, without actually diagnosing the UNPREDICTABLE
cases of invalid unused operands.

ARM MOV + MVN special cases:

> -if (op1 != 0x0f && op1 != 0x0d) {
> -rn = (insn >> 16) & 0xf;
> -tmp = load_reg(s, rn);
> -} else {
> -tmp = NULL;
> -}
...
> -if (op1 != 0x0f && op1 != 0x0d) {
> -tcg_temp_free_i32(tmp2);
> -}

Here we have TST, TEQ, CMP, CMN:

> -case 0x08:
> -if (set_cc) {
> -tcg_gen_and_i32(tmp, tmp, tmp2);
> -gen_logic_CC(tmp);
> -}
> -tcg_temp_free_i32(tmp);
> -break;
> -case 0x09:
> -if (set_cc) {
> -tcg_gen_xor_i32(tmp, tmp, tmp2);
> -gen_logic_CC(tmp);
> -}
> -tcg_temp_free_i32(tmp);
> -break;
> -case 0x0a:
> -if (set_cc) {
> -gen_sub_CC(tmp, tmp, tmp2);
> -}
> -tcg_temp_free_i32(tmp);
> -break;
> -case 0x0b:
> -if (set_cc) {
> -gen_add_CC(tmp, tmp, tmp2);
> -}
> -tcg_temp_free_i32(tmp);
> -break;

I don't see bit 20 (set_cc) as even being optional in the decode, though rd is
RES0/SBZ and is thus ignoring it is valid for CONSTRAINED UNPREDICTABLE.

Thumb2 MOV, MVN (and the rest UNPREDICTABLE):

>  /* Data processing register constant shift.  */
> -if (rn == 15) {
> -tmp = tcg_temp_new_i32();
> -tcg_gen_movi_i32(tmp, 0);
> -} else {
> -tmp = load_reg(s, rn);
> -}
...
> -if (rn == 15) {
> -tmp = tcg_temp_new_i32();
> -tcg_gen_movi_i32(tmp, 0);
> -} else {
> -tmp = load_reg(s, rn);
> -}

TST, TEQ, CMP, CMN (and the rest UNPREDICTABLE):

> -} else if (rd != 15) {
> -store_reg(s, rd, tmp);
> -} else {
> -tcg_temp_free_i32(tmp);
> -}

r~

Re: [Qemu-devel] [for-4.2 PATCH 2/2] hw/i386: AMD-Vi IVRS DMA alias support

[Peter Xu]

On Mon, Jul 29, 2019 at 01:04:41PM -0600, Alex Williamson wrote:
> On Mon, 29 Jul 2019 16:26:46 +0800
> Peter Xu  wrote:
> 
> > On Fri, Jul 26, 2019 at 06:55:53PM -0600, Alex Williamson wrote:
> > > When we account for DMA aliases in the PCI address space, we can no
> > > longer use a single IVHD entry in the IVRS covering all devices.  We
> > > instead need to walk the PCI bus and create alias ranges when we find
> > > a conventional bus.  These alias ranges cannot overlap with a "Select
> > > All" range (as currently implemented), so we also need to enumerate
> > > each device with IVHD entries.
> > > 
> > > Importantly, the IVHD entries used here include a Device ID, which is
> > > simply the PCI BDF (Bus/Device/Function).  The guest firmware is
> > > responsible for programming bus numbers, so the final revision of this
> > > table depends on the update mechanism (acpi_build_update) to be called
> > > after guest PCI enumeration.  
> > 
> > Ouch... so the ACPI build procedure is after those guest PCI code!
> > Could I ask how do you find this? :) It seems much easier for sure
> > this way...
> 
> I believe this is what MST was referring to with the MCFG update,
> acpi_build() is called from both acpi_setup() and acpi_build_update(),
> the latter is setup in numerous places to be called via a mechanism
> that re-writes the table on certain guest actions.  For instance
> acpi_add_rom_blob() passes this function as a callback which turns into
> a select_cb in fw_cfg, such that (aiui) the tables are updated before
> the guest reads them.  I added some fprintfs in the PCI config write
> path to confirm that the update callback occurs after PCI enumeration
> for both SeaBIOS and OVMF.  Since we seem to have other dependencies on
> this ordering elsewhere, I don't think that the IVRS update is unique
> in this regard.

Agreed.

[...]

> > We've implmented the similar logic for multiple times:
> > 
> >   - When we want to do DMA (pci_requester_id)
> >   - When we want to fetch the DMA address space (the previous patch)
> >   - When we fill in the AMD ACPI table (this patch)
> > 
> > Do you think we can generalize them somehow?  I'm thinking how about
> > we directly fetch RID in the 2nd/3rd use case using pci_requester_id()
> > (which existed already) and simply use it?
> 
> For this patch, I think we could use pci_requester_id() for dev_id_b
> above, but we still need to walk the buses and devices, so it really
> only simplifies the 'if (pci_is_express...' code block above, and
> barely at that since we're at the point in the topology where such a
> decision is made.  For the previous patch, pci_requester_id() returns a
> BDF and that code is executed before bus numbers are programmed.  We
> might still make use of requester_id_cache, but a different interface
> would be necessary.  Also note how pci_req_id_cache_get() assumes we're
> looking for the requester ID as seen from the root bus while
> pci_device_iommu_address_space() is from the bus hosting iommu_fn.
> While these are generally the same in practice, it's not required.  I'd
> therefore tend to leave that to some future consolidation.  I can
> update the comment to include that justification in the previous patch.

Yes, we can work on top in the future if needed.  I see that Michael
already plan to merge this version, then it may not worth a repost for
the comment (unless there will be a repost, we could mark a TODO).

> 
> > > +/*
> > > + * A PCI bus walk, for each PCI host bridge, is necessary to create a
> > > + * complete set of IVHD entries.  Do this into a separate blob so 
> > > that we
> > > + * can calculate the total IVRS table length here and then append 
> > > the new
> > > + * blob further below.  Fall back to an entry covering all devices, 
> > > which
> > > + * is sufficient when no aliases are present.
> > > + */
> > > +object_child_foreach_recursive(object_get_root(),
> > > +   ivrs_host_bridges, ivhd_blob);
> > > +
> > > +if (!ivhd_blob->len) {
> > > +/*
> > > + *   Type 1 device entry reporting all devices
> > > + *   These are 4-byte device entries currently reporting the 
> > > range of
> > > + *   Refer to Spec - Table 95:IVHD Device Entry Type 
> > > Codes(4-byte)
> > > + */
> > > +build_append_int_noprefix(ivhd_blob, 0x001, 4);
> > > +}  
> > 
> > Is there a real use case for ivhd_blob->len==0?
> 
> It was mostly paranoia, but I believe it's really only an Intel
> convention that the PCI host bridge appears as a device on the bus.  It
> seems possible that we could have a host bridge with no devices, in
> which case we'd insert this select-all entry to make the IVRS valid.
> Perhaps in combination with AMD exposing their IOMMU as a device on the
> PCI bus this is not really an issue, but it's a trivial safety net.
> Thanks,

That question was only for curiousity.  This code path will only
trigger when 

Re: [Qemu-devel] [PATCH 10/67] target/arm: Move test for AL into arm_skip_unless

[Richard Henderson]

On 7/29/19 7:32 AM, Peter Maydell wrote:
> On Fri, 26 Jul 2019 at 18:50, Richard Henderson
>  wrote:
>>
>> We will shortly be calling this function much more often.
>>
>> Signed-off-by: Richard Henderson 
>> ---
>>  target/arm/translate.c | 28 
>>  1 file changed, 12 insertions(+), 16 deletions(-)
>>
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index 53c46fcdc4..36419025db 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -7705,8 +7705,14 @@ static void arm_gen_condlabel(DisasContext *s)
>>  /* Skip this instruction if the ARM condition is false */
>>  static void arm_skip_unless(DisasContext *s, uint32_t cond)
>>  {
>> -arm_gen_condlabel(s);
>> -arm_gen_test_cc(cond ^ 1, s->condlabel);
>> +/*
>> + * Conditionally skip the insn. Note that both 0xe and 0xf mean
>> + * "always"; 0xf is not "never".
>> + */
>> +if (cond < 0xe) {
>> +arm_gen_condlabel(s);
>> +arm_gen_test_cc(cond ^ 1, s->condlabel);
>> +}
>>  }
>>
>>  static void disas_arm_insn(DisasContext *s, unsigned int insn)
>> @@ -7944,11 +7950,9 @@ static void disas_arm_insn(DisasContext *s, unsigned 
>> int insn)
>>  }
>>  goto illegal_op;
>>  }
>> -if (cond != 0xe) {
>> -/* if not always execute, we generate a conditional jump to
>> -   next instruction */
>> -arm_skip_unless(s, cond);
>> -}
>> +
>> +arm_skip_unless(s, cond);
>> +
>>  if ((insn & 0x0f90) == 0x0300) {
>>  if ((insn & (1 << 21)) == 0) {
>>  ARCH(6T2);
>> @@ -12095,15 +12099,7 @@ static void 
>> thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>>  dc->insn = insn;
>>
>>  if (dc->condexec_mask && !thumb_insn_is_unconditional(dc, insn)) {
>> -uint32_t cond = dc->condexec_cond;
>> -
>> -/*
>> - * Conditionally skip the insn. Note that both 0xe and 0xf mean
>> - * "always"; 0xf is not "never".
>> - */
>> -if (cond < 0x0e) {
>> -arm_skip_unless(dc, cond);
>> -}
>> +arm_skip_unless(dc, dc->condexec_cond);
>>  }
> 
> In the other callsites for arm_skip_unless() the cond argument
> can never be 0xe or 0xf.
> 
> Reviewed-by: Peter Maydell 

In my original version I included cond in the fields collected by decodetree,
and so every single trans_* function called arm_skip_unless, so that would not
be the case there.

I discarded that in the version posted here, but I still think it might be a
cleaner design overall.

In the short term, maybe I should just discard this patch?


r~

Re: [Qemu-devel] [PATCH v8 02/11] numa: move numa global variable nb_numa_nodes into MachineState

[Tao Xu]

On 7/29/2019 9:09 PM, Igor Mammedov wrote:

On Mon, 29 Jul 2019 14:31:18 +0800
Tao Xu  wrote:


Add struct NumaState in MachineState and move existing numa global
nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
numa_support into MachineClass to decide which submachines support NUMA.

Suggested-by: Igor Mammedov 
Suggested-by: Eduardo Habkost 
Signed-off-by: Tao Xu 
---

Changes in v8:
 - Add check if numa->numa_state is NULL in pxb_dev_realize_common
 - Use nb_nodes in spapr_populate_memory() (Igor)
---

[...]

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 821f0d4a49..1c7c12c415 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -331,7 +331,7 @@ static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState 
*spapr)
  return ret;
  }
  
-if (nb_numa_nodes > 1) {

+if (ms->numa_state->num_nodes > 1) {
  ret = spapr_fixup_cpu_numa_dt(fdt, offset, cpu);
  if (ret < 0) {
  return ret;
@@ -351,9 +351,9 @@ static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState 
*spapr)
  
  static hwaddr spapr_node0_size(MachineState *machine)

  {
-if (nb_numa_nodes) {
+if (machine->numa_state->num_nodes) {
  int i;
-for (i = 0; i < nb_numa_nodes; ++i) {
+for (i = 0; i < machine->numa_state->num_nodes; ++i) {
  if (numa_info[i].node_mem) {
  return MIN(pow2floor(numa_info[i].node_mem),
 machine->ram_size);
@@ -398,13 +398,14 @@ static int spapr_populate_memory(SpaprMachineState 
*spapr, void *fdt)
  {
  MachineState *machine = MACHINE(spapr);
  hwaddr mem_start, node_size;
-int i, nb_nodes = nb_numa_nodes;
+int i, nb_nodes = machine->numa_state->num_nodes;
  NodeInfo *nodes = numa_info;
  NodeInfo ramnode;
  
  /* No NUMA nodes, assume there is just one node with whole RAM */

-if (!nb_numa_nodes) {
+if (!nb_nodes) {
  nb_nodes = 1;
+machine->numa_state->num_nodes = nb_nodes;

You've not addressed a v7 comment
On Tue, 23 Jul 2019 16:56:41 +0200
Igor Mammedov  wrote:


I don't like user fixing up generic machine data that came from CLI
(or luck of such)

[...]

I'd keep fixup local (i.e. using nb_nodes)


i.e. do not modify machine->numa_state->num_nodes and just use value local
like current code does.


But modify machine->numa_state->num_nodes can fix the issue:
spapr_populate_memory() creates a implicit node and info
temporarily but then spapr_validate_node_memory() will use
global nb_numa_nodes which is 0 and skip check.

Or if I should modify the check part, i.e.
static void spapr_validate_node_memory(MachineState *machine, Error **errp)
{
[...]
for (i = 0; machine->numa_state->nodes[i] == NULL; i++) {
if (machine->numa_state->nodes[i].node_mem % 
SPAPR_MEMORY_BLOCK_SIZE) {

error_setg(errp,
   "Node %d memory size 0x%" PRIx64
   " is not aligned to %" PRIu64 " MiB",
   i, machine->numa_state->nodes[i].node_mem,
   SPAPR_MEMORY_BLOCK_SIZE / MiB);)




  ramnode.node_mem = machine->ram_size;
  nodes = 
  }

[...]

Re: [Qemu-devel] [PATCH] docs/nvdimm: add example on persistent backend setup

[Wei Yang]

Hi, Stefan

Thanks for your comments :-)

On Mon, Jul 29, 2019 at 02:58:59PM +0100, Stefan Hajnoczi wrote:
>On Wed, Jul 24, 2019 at 03:03:07PM +0800, Wei Yang wrote:
>> Persistent backend setup requires some knowledge about nvdimm and ndctl
>> tool. Some users report they may struggle to gather these knowledge and
>> have difficulty to setup it properly.
>> 
>> Here we provide two examples for persistent backend and gives the link
>> to ndctl. By doing so, user could try it directly and do more
>> investigation on persistent backend setup with ndctl.
>> 
>> Signed-off-by: Wei Yang 
>> ---
>>  docs/nvdimm.txt | 28 
>>  1 file changed, 28 insertions(+)
>> 
>> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
>> index b531cacd35..baba7a940d 100644
>> --- a/docs/nvdimm.txt
>> +++ b/docs/nvdimm.txt
>> @@ -171,6 +171,32 @@ guest software that this vNVDIMM device contains a 
>> region that cannot
>>  accept persistent writes. In result, for example, the guest Linux
>>  NVDIMM driver, marks such vNVDIMM device as read-only.
>>  
>> +Backend File Setup Example
>> +..

Here I use '-' because I want to say this is a sub-section of "Guest Data
Persistence".

Actually, I struggled a little to pick up a proper character. Do you think '-'
is the proper one?

>
>For consistency with the rest of the document please use '-' instead of
>'.'.
>
>> +
>> +Here is two examples for how to setup these persistent backend on
>> +linux, which leverages the tool ndctl [3].
>
>Small grammar tweaks:
>
>  Here are two examples showing how to set up persistent backends on
>  Linux using the tool ndctl [3].
>
>> +
>> +It is easy to setup DAX device backend file.
>
>Please move this into the "A. DAX device" section and use it as an
>introduction to explain what this section is about:
>
>  Use the following command to set up /dev/dax0.0 so that the entirety
>  of namespace0.0 can be exposed as an emulated NVDIMM to the guest:
>
>> +
>> +A. DAX device
>> +
>> +ndctl create-namespace -f -e namespace0.0 -m devdax
>> +
>> +The /dev/dax0.0 could be used directly in "mem-path" option.
>> +
>> +For DAX file, it is more than creating the proper namespace. The
>> +block device should be partitioned and mounted (with dax option).
>
>Please move this into "B. DAX file":
>
>  Individual files on a DAX host file system can be exposed as emulated
>  NVDIMMS.  First an fsdax block device is created, partitioned, and
>  then mounted with the "dax" mount option:
>
>> +
>> +B. DAX file
>> +
>> +ndctl create-namespace -f -e namespace0.0 -m fsdax
>> +(partition /dev/pmem0 with name pmem0p1)
>> +mount -o dax /dev/pmem0p1 /mnt
>> +(dd a file with proper size in /mnt)
>
>"dd a file" could be "create or copy a disk image file with qemu-img(1),
>cp(1), or dd(1) in /mnt".



-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 07/67] target/arm: Introduce add_reg_for_lit

[Richard Henderson]

On 7/29/19 7:15 AM, Peter Maydell wrote:
> On Fri, 26 Jul 2019 at 18:50, Richard Henderson
>  wrote:
>>
>> Used only on the thumb side so far, but will be more obvious
>> once we start unifying the implementation of A32+T32.
>>
>> Signed-off-by: Richard Henderson 
>> ---
>>  target/arm/translate-vfp.inc.c |  34 +--
>>  target/arm/translate.c | 163 +++--
>>  2 files changed, 76 insertions(+), 121 deletions(-)
>>
>> diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
>> index e7389bc057..4066b2febf 100644
>> --- a/target/arm/translate-vfp.inc.c
>> +++ b/target/arm/translate-vfp.inc.c
>> @@ -941,14 +941,7 @@ static bool trans_VLDR_VSTR_sp(DisasContext *s, 
>> arg_VLDR_VSTR_sp *a)
>>  offset = -offset;
>>  }
>>
>> -if (s->thumb && a->rn == 15) {
>> -/* This is actually UNPREDICTABLE */
>> -addr = tcg_temp_new_i32();
>> -tcg_gen_movi_i32(addr, s->pc & ~2);
>> -} else {
>> -addr = load_reg(s, a->rn);
>> -}
>> -tcg_gen_addi_i32(addr, addr, offset);
>> +addr = add_reg_for_lit(s, a->rn, offset);
>>  tmp = tcg_temp_new_i32();
>>  if (a->l) {
>>  gen_aa32_ld32u(s, tmp, addr, get_mem_index(s));
>> @@ -983,14 +976,7 @@ static bool trans_VLDR_VSTR_dp(DisasContext *s, 
>> arg_VLDR_VSTR_dp *a)
>>  offset = -offset;
>>  }
>>
>> -if (s->thumb && a->rn == 15) {
>> -/* This is actually UNPREDICTABLE */
>> -addr = tcg_temp_new_i32();
>> -tcg_gen_movi_i32(addr, s->pc & ~2);
>> -} else {
>> -addr = load_reg(s, a->rn);
>> -}
>> -tcg_gen_addi_i32(addr, addr, offset);
>> +addr = add_reg_for_lit(s, a->rn, offset);
>>  tmp = tcg_temp_new_i64();
>>  if (a->l) {
>>  gen_aa32_ld64(s, tmp, addr, get_mem_index(s));
>> @@ -1029,13 +1015,7 @@ static bool trans_VLDM_VSTM_sp(DisasContext *s, 
>> arg_VLDM_VSTM_sp *a)
>>  return true;
>>  }
>>
>> -if (s->thumb && a->rn == 15) {
>> -/* This is actually UNPREDICTABLE */
>> -addr = tcg_temp_new_i32();
>> -tcg_gen_movi_i32(addr, s->pc & ~2);
>> -} else {
>> -addr = load_reg(s, a->rn);
>> -}
>> +addr = add_reg_for_lit(s, a->rn, 0);
>>  if (a->p) {
>>  /* pre-decrement */
>>  tcg_gen_addi_i32(addr, addr, -(a->imm << 2));
>> @@ -1112,13 +1092,7 @@ static bool trans_VLDM_VSTM_dp(DisasContext *s, 
>> arg_VLDM_VSTM_dp *a)
>>  return true;
>>  }
>>
>> -if (s->thumb && a->rn == 15) {
>> -/* This is actually UNPREDICTABLE */
>> -addr = tcg_temp_new_i32();
>> -tcg_gen_movi_i32(addr, s->pc & ~2);
>> -} else {
>> -addr = load_reg(s, a->rn);
>> -}
>> +addr = add_reg_for_lit(s, a->rn, 0);
>>  if (a->p) {
>>  /* pre-decrement */
>>  tcg_gen_addi_i32(addr, addr, -(a->imm << 2));
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index a48e9a90f8..5e2dd8bb16 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -214,6 +214,23 @@ static inline TCGv_i32 load_reg(DisasContext *s, int 
>> reg)
>>  return tmp;
>>  }
>>
>> +/*
>> + * Create a new temp, incremented by OFS, except PC is aligned but not
>> + * incremented for thumb.  This is used for load/store for which use of
>> + * PC implies (literal), or ADD that implies ADR.
>> + */
>> +static TCGv_i32 add_reg_for_lit(DisasContext *s, int reg, int ofs)
>> +{
>> +TCGv_i32 tmp = tcg_temp_new_i32();
>> +
>> +if (reg == 15) {
>> +tcg_gen_movi_i32(tmp, (s->pc_read & ~3) + ofs);
>> +} else {
>> +tcg_gen_addi_i32(tmp, cpu_R[reg], ofs);
>> +}
>> +return tmp;
>> +}
> 
> This is losing the information in the comments about the UNPREDICTABLE
> cases. Are there callsites where the new function is called where
> "thumb and reg == 15" is not UNPREDICTABLE, or are they all
> that way?

These call sites are that way, but this function will eventually be used for
LDR (literal) and ADR, which obviously are not UNPREDICTABLE.

I don't think this comment attached to this code is useful as-is.  Either we do
the natural a32-ish behaviour and use ALIGN(PC,4), or we should
gen_illegal_op() and be done with it.

Would you prefer a function like

/* Use of PC is UNPREDICTABLE in thumb mode, but allowed in arm mode. */
static TCGv_i32 load_reg_nothumbpc(DisasContext *s, int reg)
{
if (unlikely(reg == 15) && s->thumb) {
gen_illegal_op(s);
/* Unreachable tcg ops will be deleted but must still be legal. */
return tcg_const_i32(0);
}
return load_reg(s, reg);
}

for these specific usages?


r~

Re: [Qemu-devel] [PATCH v2 0/3] migration: add speed limit for multifd migration

[Wei Yang]

On Mon, Jul 29, 2019 at 04:01:18PM +0800, Ivan Ren wrote:
>Currently multifd migration has not been limited and it will consume
>the whole bandwidth of Nic. These two patches add speed limitation to
>it.
>
>This is the v2 patches, differences with v1:
>1. change qemu_file_update_rate_transfer interface name
>   to qemu_file_update_transfer
>2. add a new patch to update ram_counters for multifd sync packet

Usually we cc those who gave us comment.

>
>Ivan Ren (3):
>  migration: add qemu_file_update_transfer interface
>  migration: add speed limit for multifd migration
>  migration: update ram_counters for multifd sync packet
>
> migration/qemu-file.c |  5 +
> migration/qemu-file.h |  1 +
> migration/ram.c   | 24 ++--
> 3 files changed, 20 insertions(+), 10 deletions(-)
>
>-- 
>2.17.2 (Apple Git-113)
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 3/3] migration: update ram_counters for multifd sync packet

[Wei Yang]

On Mon, Jul 29, 2019 at 04:01:21PM +0800, Ivan Ren wrote:
>Multifd sync will send MULTIFD_FLAG_SYNC flag info to destination, add
>these bytes to ram_counters record.
>
>Signed-off-by: Ivan Ren 

Also this is suggested by me, it would be better to add Suggested-by.

>---
> migration/ram.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/migration/ram.c b/migration/ram.c
>index 88ddd2bbe2..20b6eebb7c 100644
>--- a/migration/ram.c
>+++ b/migration/ram.c
>@@ -1085,6 +1085,8 @@ static void multifd_send_sync_main(RAMState *rs)
> p->flags |= MULTIFD_FLAG_SYNC;
> p->pending_job++;
> qemu_file_update_transfer(rs->f, p->packet_len);
>+ram_counters.multifd_bytes += p->packet_len;
>+ram_counters.transferred += p->packet_len;
> qemu_mutex_unlock(>mutex);
> qemu_sem_post(>sem);
> }
>-- 
>2.17.2 (Apple Git-113)
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 1/3] migration: add qemu_file_update_transfer interface

[Wei Yang]

On Mon, Jul 29, 2019 at 04:01:19PM +0800, Ivan Ren wrote:
>Add qemu_file_update_transfer for just update bytes_xfer for speed
>limitation. This will be used for further migration feature such as
>multifd migration.
>
>Signed-off-by: Ivan Ren 

Well I have reviewed this patch, suppose you need add my Reviewed-by.

>---
> migration/qemu-file.c | 5 +
> migration/qemu-file.h | 1 +
> 2 files changed, 6 insertions(+)
>
>diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>index 0431585502..18f480529a 100644
>--- a/migration/qemu-file.c
>+++ b/migration/qemu-file.c
>@@ -615,6 +615,11 @@ void qemu_file_reset_rate_limit(QEMUFile *f)
> f->bytes_xfer = 0;
> }
> 
>+void qemu_file_update_transfer(QEMUFile *f, int64_t len)
>+{
>+f->bytes_xfer += len;
>+}
>+
> void qemu_put_be16(QEMUFile *f, unsigned int v)
> {
> qemu_put_byte(f, v >> 8);
>diff --git a/migration/qemu-file.h b/migration/qemu-file.h
>index 13baf896bd..5de9fa2e96 100644
>--- a/migration/qemu-file.h
>+++ b/migration/qemu-file.h
>@@ -147,6 +147,7 @@ int qemu_peek_byte(QEMUFile *f, int offset);
> void qemu_file_skip(QEMUFile *f, int size);
> void qemu_update_position(QEMUFile *f, size_t size);
> void qemu_file_reset_rate_limit(QEMUFile *f);
>+void qemu_file_update_transfer(QEMUFile *f, int64_t len);
> void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
> int64_t qemu_file_get_rate_limit(QEMUFile *f);
> void qemu_file_set_error(QEMUFile *f, int ret);
>-- 
>2.17.2 (Apple Git-113)
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 06/67] target/arm: Introduce pc_read

[Richard Henderson]

On 7/29/19 7:05 AM, Peter Maydell wrote:
> On Fri, 26 Jul 2019 at 18:50, Richard Henderson
>  wrote:
>>
>> We currently have 3 different ways of computing the architectural
>> value of "PC" as seen in the ARM ARM.
>>
>> The value of s->pc has been incremented past the current insn,
>> but that is all.  Thus for a32, PC = s->pc + 4; for t32, PC = s->pc;
>> for t16, PC = s->pc + 2.  These differing computations make it
>> impossible at present to unify the various code paths.
>>
>> Let s->pc_read hold the architectural value of PC for all cases.
>>
>> Signed-off-by: Richard Henderson 
>> ---
>>  target/arm/translate.h | 10 
>>  target/arm/translate.c | 53 ++
>>  2 files changed, 32 insertions(+), 31 deletions(-)
>>
>> diff --git a/target/arm/translate.h b/target/arm/translate.h
>> index a20f6e2056..2dfdd8ca66 100644
>> --- a/target/arm/translate.h
>> +++ b/target/arm/translate.h
>> @@ -9,7 +9,17 @@ typedef struct DisasContext {
>>  DisasContextBase base;
>>  const ARMISARegisters *isar;
>>
>> +/*
>> + * Summary of the various values for "PC":
>> + * base.pc_next -- the start of the current insn
>> + * pc   -- the start of the next insn
> 
> These are confusingly named -- logically "pc_next" ought to
> be the PC of the next instruction and "pc" ought to be
> that of the current one...

Yes, well.  I don't quite remember why "pc_next" was chosen for this field.  It
is the "next" upon entry to tr_foo_disas_insn().  Often the target will
increment s->base.pc_next immediately, so it will also be the "next" insn
throughout translation.  Though that isn't currently the case for ARM.

Once most of the uses of s->pc get moved to s->pc_read, it might be reasonable
to rename the remaining "s->base.pc_next" -> "s->pc_orig" and "s->pc" ->
"s->base.pc_next".  Perhaps that would be clearer, I'm not sure.


>> + * pc_read  -- the value for "PC" in the ARM ARM;
> 
> nit: this line should end with a colon, rather than a semicolon

Oops, typo.


>> + * in arm mode, the current insn + 8;
>> + * in thumb mode, the current insn + 4;
>> + * in aa64 mode, unused.
>> + */
>>  target_ulong pc;
>> +target_ulong pc_read;
>>  target_ulong page_start;
>>  uint32_t insn;
> 
> Why target_ulong rather than uint32_t, given this is
> aarch32 only?

I didn't know if it would stay aarch32 only and it seemed more natural to match
the types.


r~

[Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr

[Wei Yang]

When we iterate the memory-device list to get the available range, it is not
necessary to iterate the whole list.

1) no more overlap for hinted range if tmp exceed it

v2:
   * remove #2 as suggested by Igor and David
   * add some comment to inform address assignment stay the same as before
 this change 

Wei Yang (2):
  memory-device: not necessary to use goto for the last check
  memory-device: break the loop if tmp exceed the hinted range

 hw/mem/memory-device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[Wei Yang]

We are already at the last condition check.

Signed-off-by: Wei Yang 
Reviewed-by: Igor Mammedov 
Reviewed-by: David Hildenbrand 
---
 hw/mem/memory-device.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 5f2c408036..df3261b32a 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -186,7 +186,6 @@ static uint64_t memory_device_get_free_addr(MachineState 
*ms,
 if (!range_contains_range(, )) {
 error_setg(errp, "could not find position in guest address space for "
"memory device - memory fragmented due to alignments");
-goto out;
 }
 out:
 g_slist_free(list);
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range

[Wei Yang]

The memory-device list built by memory_device_build_list is ordered by
its address, this means if the tmp range exceed the hinted range, all
the following range will not overlap with it.

And this won't change default pc-dimm mapping and address assignment stay
the same as before this change.

Signed-off-by: Wei Yang 
---
 hw/mem/memory-device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index df3261b32a..df4e338b83 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -180,6 +180,8 @@ static uint64_t memory_device_get_free_addr(MachineState 
*ms,
 range_make_empty();
 break;
 }
+} else if (range_lob() > range_upb()) {
+break;
 }
 }
 
-- 
2.17.1

Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference

[piaojun]

On 2019/7/29 23:41, Stefan Hajnoczi wrote:
> On Mon, Jul 29, 2019 at 08:35:36PM +0800, piaojun wrote:
>> Hi Stefan,
>>
>> On 2019/7/26 17:11, Stefan Hajnoczi wrote:
>>> Most lo_do_lookup() have already checked that the parent inode exists.
>>> lo_lookup() hasn't and can therefore hit a NULL pointer dereference when
>>> lo_inode(req, parent) returns NULL.
>>>
>>> Signed-off-by: Stefan Hajnoczi 
>>> ---
>>>  contrib/virtiofsd/passthrough_ll.c | 4 
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/contrib/virtiofsd/passthrough_ll.c 
>>> b/contrib/virtiofsd/passthrough_ll.c
>>> index 9ae1381618..277a17fc03 100644
>>> --- a/contrib/virtiofsd/passthrough_ll.c
>>> +++ b/contrib/virtiofsd/passthrough_ll.c
>>> @@ -766,6 +766,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t 
>>> parent, const char *name,
>>> struct lo_data *lo = lo_data(req);
>>> struct lo_inode *inode, *dir = lo_inode(req, parent);
>>>  
>>> +   if (!dir) {
>>> +   return EBADF;
>>> +   }
>>> +
>>
>> I worry about that dir will be released or set NULL just after NULL
>> checking. Or could we use some lock to prevent the simultaneity?
> 
> Yes, I agree.  I haven't audited lo_inode yet, but it needs a refcount
> and/or lock to ensure accesses are safe.  I'll do that and other things
> in a separate patch series.
> 
> Stefan

OK, that sounds good.

Jun

> 

[Qemu-devel] [PATCH] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined

[piaojun]

Use F_GETLK for fcntl when F_OFD_GETLK not defined.

Signed-off-by: Jun Piao 
---
 contrib/virtiofsd/passthrough_ll.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/contrib/virtiofsd/passthrough_ll.c 
b/contrib/virtiofsd/passthrough_ll.c
index 9ae1381..757785b 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1619,7 +1619,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino,
return;
}

+#ifdef F_OFD_GETLK
ret = fcntl(plock->fd, F_OFD_GETLK, lock);
+#else
+   ret = fcntl(plock->fd, F_GETLK, lock);
+#endif
if (ret == -1)
saverr = errno;
pthread_mutex_unlock(>plock_mutex);
@@ -1668,7 +1672,12 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino,

/* TODO: Is it alright to modify flock? */
lock->l_pid = 0;
+
+#ifdef F_OFD_GETLK
ret = fcntl(plock->fd, F_OFD_SETLK, lock);
+#else
+   ret = fcntl(plock->fd, F_SETLK, lock);
+#endif
if (ret == -1) {
saverr = errno;
}
-- 

Re: [Qemu-devel] [PATCH for 4.2 v2 0/6] target/mips: Misc patches for 4.2

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/1564424746-11065-1-git-send-email-aleksandar.marko...@rt-rk.com/



Hi,

This series failed build test on s390x host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e

echo
echo "=== ENV ==="
env

echo
echo "=== PACKAGES ==="
rpm -qa

echo
echo "=== UNAME ==="
uname -a

CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/1564424746-11065-1-git-send-email-aleksandar.marko...@rt-rk.com/testing.s390x/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[Qemu-devel] [PATCH] Revert "ide/ahci: Check for -ECANCELED in aio callbacks"

[John Snow]

This reverts commit 0d910cfeaf2076b116b4517166d5deb0fea76394.

It's not correct to just ignore an error code in a callback; we need to
handle that error and possible report failure to the guest so that they
don't wait indefinitely for an operation that will now never finish.

This ought to help cases reported by Nutanix where iSCSI returns a
legitimate -ECANCELED for certain operations which should be propagated
normally.

Reported-by: Shaju Abraham 
Signed-off-by: John Snow 
---
 hw/ide/ahci.c |  3 ---
 hw/ide/core.c | 14 --
 2 files changed, 17 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 00ba422a48..6aaf66534a 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1023,9 +1023,6 @@ static void ncq_cb(void *opaque, int ret)
 IDEState *ide_state = _tfs->drive->port.ifs[0];
 
 ncq_tfs->aiocb = NULL;
-if (ret == -ECANCELED) {
-return;
-}
 
 if (ret < 0) {
 bool is_read = ncq_tfs->cmd == READ_FPDMA_QUEUED;
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 6afadf894f..8e1624f7ce 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -722,9 +722,6 @@ static void ide_sector_read_cb(void *opaque, int ret)
 s->pio_aiocb = NULL;
 s->status &= ~BUSY_STAT;
 
-if (ret == -ECANCELED) {
-return;
-}
 if (ret != 0) {
 if (ide_handle_rw_error(s, -ret, IDE_RETRY_PIO |
 IDE_RETRY_READ)) {
@@ -840,10 +837,6 @@ static void ide_dma_cb(void *opaque, int ret)
 uint64_t offset;
 bool stay_active = false;
 
-if (ret == -ECANCELED) {
-return;
-}
-
 if (ret == -EINVAL) {
 ide_dma_error(s);
 return;
@@ -975,10 +968,6 @@ static void ide_sector_write_cb(void *opaque, int ret)
 IDEState *s = opaque;
 int n;
 
-if (ret == -ECANCELED) {
-return;
-}
-
 s->pio_aiocb = NULL;
 s->status &= ~BUSY_STAT;
 
@@ -1058,9 +1047,6 @@ static void ide_flush_cb(void *opaque, int ret)
 
 s->pio_aiocb = NULL;
 
-if (ret == -ECANCELED) {
-return;
-}
 if (ret < 0) {
 /* XXX: What sector number to set here? */
 if (ide_handle_rw_error(s, -ret, IDE_RETRY_FLUSH)) {
-- 
2.21.0

Re: [Qemu-devel] [PATCH] dma-helpers: ensure AIO callback is invoked after cancellation

[John Snow]

On 7/29/19 5:51 PM, Paolo Bonzini wrote:
> On 29/07/19 23:46, John Snow wrote:
>>> @@ -111,15 +112,12 @@ static void dma_complete(DMAAIOCB *dbs, int ret)
>>>  {
>>>  trace_dma_complete(dbs, ret, dbs->common.cb);
>>>  
>>> +assert(!dbs->acb && !dbs->bh);
>>>  dma_blk_unmap(dbs);
>>>  if (dbs->common.cb) {
>>>  dbs->common.cb(dbs->common.opaque, ret);
>>>  }
>>>  qemu_iovec_destroy(>iov);
>>> -if (dbs->bh) {
>>> -qemu_bh_delete(dbs->bh);
>>> -dbs->bh = NULL;
>>> -}
>>
>> Now presumably handled by dma_aio_cancel,
> 
> No, it simply could never happen.  dma_complete is called here in dma_blk_cb:
> 
> dbs->acb = NULL;
> dbs->offset += dbs->iov.size;
> 
> if (dbs->sg_cur_index == dbs->sg->nsg || ret < 0) {
> dma_complete(dbs, ret);
> return;
> }
> 
> and the only way to reach that when dbs->bh becomes non-NULL is through 
> reschedule_dma, which clears dbs->bh before invoking dma_blk_cb.
> 
>>>  if (dbs->acb) {
>>> +/* This will invoke dma_blk_cb.  */
>>
>> uhh, does it?
> 
> Yes:
> 
> /* Async version of aio cancel. The caller is not blocked if the acb 
> implements
>  * cancel_async, otherwise we do nothing and let the request normally 
> complete.
>  * In either case the completion callback must be called. */
> 

Right, right -- the comment can SAY anything it likes about what the
"contract" is ...

OK, so it's more as if EITHER the cancel callback will invoke
dma_blk_cb, OR the acb object there will ... eventually ... through
normal execution.

OK, ok, ok. Getting it, slowly, slowly.

I think this comment is confusing, actually -- because dma_blk_cb might
not actually get invoked in the stack below this call. We only know it
might.

>> this is maybe where I got lost reading this code.
>> Isn't dbs->acb going to be what was returned from e.g.
>> dma_blk_read_io_func, which ultimately uses blk_aio_em_aiocb_info, that
>> has no cancel callback?
> 
> Right therefore the I/O will complete and the callback will be invoked.
> 

>From the other email:

***oh***.

>> Well, here at least I am now on terra-firma that we're going to call the
>> original callback with ECANCELED, which is a step towards code that
>> isn't surprising my sensibilities.
> 
> Good. :)
> 
> Paolo
> 

OK, this seems right to me then; the last puzzle piece is that Fam added
no-op returns for ECANCELED to the IDE originators of such DMA requests,
but now that I see the pathways beneath here I think it'd be /never/
right to ignore them.

If you cancel IDE's DMA out from under it, I think the IDE state machine
ought to treat it as an error, yes.

Thanks for the help, Paolo!
--js

Re: [Qemu-devel] [PATCH] dma-helpers: ensure AIO callback is invoked after cancellation

[Paolo Bonzini]

On 29/07/19 23:46, John Snow wrote:
>> @@ -111,15 +112,12 @@ static void dma_complete(DMAAIOCB *dbs, int ret)
>>  {
>>  trace_dma_complete(dbs, ret, dbs->common.cb);
>>  
>> +assert(!dbs->acb && !dbs->bh);
>>  dma_blk_unmap(dbs);
>>  if (dbs->common.cb) {
>>  dbs->common.cb(dbs->common.opaque, ret);
>>  }
>>  qemu_iovec_destroy(>iov);
>> -if (dbs->bh) {
>> -qemu_bh_delete(dbs->bh);
>> -dbs->bh = NULL;
>> -}
> 
> Now presumably handled by dma_aio_cancel,

No, it simply could never happen.  dma_complete is called here in dma_blk_cb:

dbs->acb = NULL;
dbs->offset += dbs->iov.size;

if (dbs->sg_cur_index == dbs->sg->nsg || ret < 0) {
dma_complete(dbs, ret);
return;
}

and the only way to reach that when dbs->bh becomes non-NULL is through 
reschedule_dma, which clears dbs->bh before invoking dma_blk_cb.

>>  if (dbs->acb) {
>> +/* This will invoke dma_blk_cb.  */
> 
> uhh, does it?

Yes:

/* Async version of aio cancel. The caller is not blocked if the acb implements
 * cancel_async, otherwise we do nothing and let the request normally complete.
 * In either case the completion callback must be called. */

> this is maybe where I got lost reading this code.
> Isn't dbs->acb going to be what was returned from e.g.
> dma_blk_read_io_func, which ultimately uses blk_aio_em_aiocb_info, that
> has no cancel callback?

Right therefore the I/O will complete and the callback will be invoked.

> Well, here at least I am now on terra-firma that we're going to call the
> original callback with ECANCELED, which is a step towards code that
> isn't surprising my sensibilities.

Good. :)

Paolo

Re: [Qemu-devel] [PATCH] Fix Guest VM crash due to iSCSI Sense Key error

[Paolo Bonzini]

On 29/07/19 23:37, John Snow wrote:
>>>
>>> If it does -- if there are indeed no places in the code today that
>>> artificially inject -ECANCELED -- I need to remove these special stanzas
>>> from the IDE code and allow the IDE state machine to handle these errors
>>> as true errors.
>> The bug is that there is no place to inject -ECANCELED in the dbs->bh
>> case.  I've sent an obviously^W untested patch.
>
> Where does it inject -ECANCELED in the non-dbs->bh case?

It's simply part of the contract of bdrv_aio_cancel_async that the
completion callback will be invoked (most of the time the I/O will just
go on without any cancellation, as you noted).

Paolo

[Qemu-devel] [PATCH untested for-4.2] memory: fix race between TCG and accesses to dirty bitmap

[Paolo Bonzini]

The race is as follows:

  vCPU thread  reader thread
  ---  ---
  TLB check -> slow path
notdirty_mem_write
  write to RAM
  set dirty flag
   clear dirty flag
  TLB check -> fast path
   read memory
write to RAM

and the second write is missed by the reader.

Fortunately, in order to fix it, no change is required to the
vCPU thread.  However, the reader thread must delay the read after
the vCPU thread has finished the write.  This can be approximated
conservatively by run_on_cpu, which waits for the end of the current
translation block.

A similar technique is used by KVM, which has to do a synchronous TLB
flush after doing a test-and-clear of the dirty-page flags.

Reported-by: Dr. David Alan Gilbert 
Signed-off-by: Paolo Bonzini 
---
I tested this some time ago, and enough has changed that I don't
really trust those old results.  Nevertheless, I am throwing out
the patch so that it is not forgotten.

 exec.c| 31 +++
 include/exec/memory.h | 12 
 memory.c  | 10 +-
 migration/ram.c   |  1 +
 4 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 3e78de3b8f..ae68f72da4 100644
--- a/exec.c
+++ b/exec.c
@@ -198,6 +198,7 @@ typedef struct subpage_t {
 
 static void io_mem_init(void);
 static void memory_map_init(void);
+static void tcg_log_global_after_sync(MemoryListener *listener);
 static void tcg_commit(MemoryListener *listener);
 
 static MemoryRegion io_mem_watch;
@@ -906,6 +907,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
 newas->cpu = cpu;
 newas->as = as;
 if (tcg_enabled()) {
+newas->tcg_as_listener.log_global_after_sync = 
tcg_log_global_after_sync;
 newas->tcg_as_listener.commit = tcg_commit;
 memory_listener_register(>tcg_as_listener, as);
 }
@@ -3143,6 +3145,35 @@ void address_space_dispatch_free(AddressSpaceDispatch *d)
 g_free(d);
 }
 
+static void do_nothing(CPUState *cpu, run_on_cpu_data d)
+{
+}
+
+static void tcg_log_global_after_sync(MemoryListener *listener)
+{
+CPUAddressSpace *cpuas;
+
+/* Wait for the CPU to end the current TB.  This avoids the following
+ * incorrect race:
+ *
+ *  vCPU migration
+ *  --   -
+ *  TLB check -> slow path
+ *notdirty_mem_write
+ *  write to RAM
+ *  mark dirty
+ *   clear dirty flag
+ *  TLB check -> fast path
+ *   read memory
+ *write to RAM
+ *
+ * by pushing the migration thread's memory read after the vCPU thread has
+ * written the memory.
+ */
+cpuas = container_of(listener, CPUAddressSpace, tcg_as_listener);
+run_on_cpu(cpuas->cpu, do_nothing, RUN_ON_CPU_NULL);
+}
+
 static void tcg_commit(MemoryListener *listener)
 {
 CPUAddressSpace *cpuas;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index bb0961ddb9..b6bcf31b0a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -419,6 +419,7 @@ struct MemoryListener {
 void (*log_clear)(MemoryListener *listener, MemoryRegionSection *section);
 void (*log_global_start)(MemoryListener *listener);
 void (*log_global_stop)(MemoryListener *listener);
+void (*log_global_after_sync)(MemoryListener *listener);
 void (*eventfd_add)(MemoryListener *listener, MemoryRegionSection *section,
 bool match_data, uint64_t data, EventNotifier *e);
 void (*eventfd_del)(MemoryListener *listener, MemoryRegionSection *section,
@@ -1681,6 +1682,17 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
  */
 void memory_global_dirty_log_sync(void);
 
+/**
+ * memory_global_dirty_log_sync: synchronize the dirty log for all memory
+ *
+ * Synchronizes the vCPUs with a thread that is reading the dirty bitmap.
+ * This function must be called after the dirty log bitmap is cleared, and
+ * before dirty guest memory pages are read.  If you are using
+ * #DirtyBitmapSnapshot, memory_region_snapshot_and_clear_dirty() takes
+ * care of doing this.
+ */
+void memory_global_after_dirty_log_sync(void);
+
 /**
  * memory_region_transaction_begin: Start a transaction.
  *
diff --git a/memory.c b/memory.c
index e42d63a3a0..edd0c13c38 100644
--- a/memory.c
+++ b/memory.c
@@ -2127,9 +2127,12 @@ DirtyBitmapSnapshot 
*memory_region_snapshot_and_clear_dirty(MemoryRegion *mr,
 hwaddr size,
 unsigned client)
 {
+DirtyBitmapSnapshot *snapshot;
 assert(mr->ram_block);
 memory_region_sync_dirty_bitmap(mr);
-return 

[Qemu-devel] [PATCH] memory: introduce memory_global_after_dirty_log_sync

[Paolo Bonzini]

There is a race between TCG and accesses to the dirty log:

  vCPU thread  reader thread
  ---  ---
  TLB check -> slow path
notdirty_mem_write
  write to RAM
  set dirty flag
   clear dirty flag
  TLB check -> fast path
   read memory
write to RAM

Fortunately, in order to fix it, no change is required to the
vCPU thread.  However, the reader thread must delay the read after
the vCPU thread has finished the write.  This can be approximated
conservatively by run_on_cpu, which waits for the end of the current
translation block.

A similar technique is used by KVM, which has to do a synchronous TLB
flush after doing a test-and-clear of the dirty-page flags.

Reported-by: Dr. David Alan Gilbert 
Signed-off-by: Paolo Bonzini 
---
 exec.c| 31 +++
 include/exec/memory.h | 12 
 memory.c  | 10 +-
 migration/ram.c   |  1 +
 4 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 3e78de3b8f..ae68f72da4 100644
--- a/exec.c
+++ b/exec.c
@@ -198,6 +198,7 @@ typedef struct subpage_t {
 
 static void io_mem_init(void);
 static void memory_map_init(void);
+static void tcg_log_global_after_sync(MemoryListener *listener);
 static void tcg_commit(MemoryListener *listener);
 
 static MemoryRegion io_mem_watch;
@@ -906,6 +907,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
 newas->cpu = cpu;
 newas->as = as;
 if (tcg_enabled()) {
+newas->tcg_as_listener.log_global_after_sync = 
tcg_log_global_after_sync;
 newas->tcg_as_listener.commit = tcg_commit;
 memory_listener_register(>tcg_as_listener, as);
 }
@@ -3143,6 +3145,35 @@ void address_space_dispatch_free(AddressSpaceDispatch *d)
 g_free(d);
 }
 
+static void do_nothing(CPUState *cpu, run_on_cpu_data d)
+{
+}
+
+static void tcg_log_global_after_sync(MemoryListener *listener)
+{
+CPUAddressSpace *cpuas;
+
+/* Wait for the CPU to end the current TB.  This avoids the following
+ * incorrect race:
+ *
+ *  vCPU migration
+ *  --   -
+ *  TLB check -> slow path
+ *notdirty_mem_write
+ *  write to RAM
+ *  mark dirty
+ *   clear dirty flag
+ *  TLB check -> fast path
+ *   read memory
+ *write to RAM
+ *
+ * by pushing the migration thread's memory read after the vCPU thread has
+ * written the memory.
+ */
+cpuas = container_of(listener, CPUAddressSpace, tcg_as_listener);
+run_on_cpu(cpuas->cpu, do_nothing, RUN_ON_CPU_NULL);
+}
+
 static void tcg_commit(MemoryListener *listener)
 {
 CPUAddressSpace *cpuas;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index bb0961ddb9..b6bcf31b0a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -419,6 +419,7 @@ struct MemoryListener {
 void (*log_clear)(MemoryListener *listener, MemoryRegionSection *section);
 void (*log_global_start)(MemoryListener *listener);
 void (*log_global_stop)(MemoryListener *listener);
+void (*log_global_after_sync)(MemoryListener *listener);
 void (*eventfd_add)(MemoryListener *listener, MemoryRegionSection *section,
 bool match_data, uint64_t data, EventNotifier *e);
 void (*eventfd_del)(MemoryListener *listener, MemoryRegionSection *section,
@@ -1681,6 +1682,17 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
  */
 void memory_global_dirty_log_sync(void);
 
+/**
+ * memory_global_dirty_log_sync: synchronize the dirty log for all memory
+ *
+ * Synchronizes the vCPUs with a thread that is reading the dirty bitmap.
+ * This function must be called after the dirty log bitmap is cleared, and
+ * before dirty guest memory pages are read.  If you are using
+ * #DirtyBitmapSnapshot, memory_region_snapshot_and_clear_dirty() takes
+ * care of doing this.
+ */
+void memory_global_after_dirty_log_sync(void);
+
 /**
  * memory_region_transaction_begin: Start a transaction.
  *
diff --git a/memory.c b/memory.c
index e42d63a3a0..edd0c13c38 100644
--- a/memory.c
+++ b/memory.c
@@ -2127,9 +2127,12 @@ DirtyBitmapSnapshot 
*memory_region_snapshot_and_clear_dirty(MemoryRegion *mr,
 hwaddr size,
 unsigned client)
 {
+DirtyBitmapSnapshot *snapshot;
 assert(mr->ram_block);
 memory_region_sync_dirty_bitmap(mr);
-return cpu_physical_memory_snapshot_and_clear_dirty(mr, addr, size, 
client);
+snapshot = cpu_physical_memory_snapshot_and_clear_dirty(mr, addr, size, 
client);
+memory_global_after_dirty_log_sync();
+return 

Re: [Qemu-devel] [PATCH] dma-helpers: ensure AIO callback is invoked after cancellation

[John Snow]

On 7/29/19 5:34 PM, Paolo Bonzini wrote:
> dma_aio_cancel unschedules the BH if there is one, which corresponds
> to the reschedule_dma case of dma_blk_cb.  This can stall the DMA
> permanently, because dma_complete will never get invoked and therefore
> nobody will ever invoke the original AIO callback in dbs->common.cb.
> 
> Fix this by invoking the callback (which is ensured to happen after
> a bdrv_aio_cancel_async, or done manually in the dbs->bh case), and
> add assertions to check that the DMA state machine is indeed waiting
> for dma_complete or reschedule_dma, but never both.
> 
> Reported-by: John Snow 
> Signed-off-by: Paolo Bonzini 
> ---
>  dma-helpers.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 2d7e02d..d3871dc 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -90,6 +90,7 @@ static void reschedule_dma(void *opaque)
>  {
>  DMAAIOCB *dbs = (DMAAIOCB *)opaque;
>  
> +assert(!dbs->acb && dbs->bh);
>  qemu_bh_delete(dbs->bh);
>  dbs->bh = NULL;
>  dma_blk_cb(dbs, 0);
> @@ -111,15 +112,12 @@ static void dma_complete(DMAAIOCB *dbs, int ret)
>  {
>  trace_dma_complete(dbs, ret, dbs->common.cb);
>  
> +assert(!dbs->acb && !dbs->bh);
>  dma_blk_unmap(dbs);
>  if (dbs->common.cb) {
>  dbs->common.cb(dbs->common.opaque, ret);
>  }
>  qemu_iovec_destroy(>iov);
> -if (dbs->bh) {
> -qemu_bh_delete(dbs->bh);
> -dbs->bh = NULL;
> -}

Now presumably handled by dma_aio_cancel,

>  qemu_aio_unref(dbs);
>  }
>  
> @@ -179,14 +177,21 @@ static void dma_aio_cancel(BlockAIOCB *acb)
>  
>  trace_dma_aio_cancel(dbs);
>  
> +assert(!(dbs->acb && dbs->bh));
>  if (dbs->acb) {
> +/* This will invoke dma_blk_cb.  */

uhh, does it? this is maybe where I got lost reading this code.
Isn't dbs->acb going to be what was returned from e.g.
dma_blk_read_io_func, which ultimately uses blk_aio_em_aiocb_info, that
has no cancel callback?

I thought this was just going to NOP entirely. No?

>  blk_aio_cancel_async(dbs->acb);
> +return;
>  }
> +
>  if (dbs->bh) {
>  cpu_unregister_map_client(dbs->bh);
>  qemu_bh_delete(dbs->bh);
>  dbs->bh = NULL;
>  }
> +if (dbs->common.cb) {
> +dbs->common.cb(dbs->common.opaque, -ECANCELED);
> +}

Well, here at least I am now on terra-firma that we're going to call the
original callback with ECANCELED, which is a step towards code that
isn't surprising my sensibilities.

>  }
>  
>  static AioContext *dma_get_aio_context(BlockAIOCB *acb)
> 

[Qemu-devel] [PATCH v2 1/3] iotests: add script_initialize

[John Snow]

Like script_main, but doesn't require a single point of entry.
Replace all existing initialization sections with this drop-in replacement.

This brings debug support to all existing script-style iotests.

Note: supported_oses=['linux'] was omitted, as it is a default argument.
Signed-off-by: John Snow 
---
 tests/qemu-iotests/149|  3 +-
 tests/qemu-iotests/194|  3 +-
 tests/qemu-iotests/202|  3 +-
 tests/qemu-iotests/203|  3 +-
 tests/qemu-iotests/206|  2 +-
 tests/qemu-iotests/207|  2 +-
 tests/qemu-iotests/208|  2 +-
 tests/qemu-iotests/209|  2 +-
 tests/qemu-iotests/210|  2 +-
 tests/qemu-iotests/211|  2 +-
 tests/qemu-iotests/212|  2 +-
 tests/qemu-iotests/213|  2 +-
 tests/qemu-iotests/216|  3 +-
 tests/qemu-iotests/218|  2 +-
 tests/qemu-iotests/219|  2 +-
 tests/qemu-iotests/222|  5 ++-
 tests/qemu-iotests/224|  3 +-
 tests/qemu-iotests/228|  3 +-
 tests/qemu-iotests/234|  3 +-
 tests/qemu-iotests/235|  4 +--
 tests/qemu-iotests/236|  2 +-
 tests/qemu-iotests/237|  2 +-
 tests/qemu-iotests/238|  2 ++
 tests/qemu-iotests/242|  2 +-
 tests/qemu-iotests/246|  2 +-
 tests/qemu-iotests/248|  2 +-
 tests/qemu-iotests/254|  2 +-
 tests/qemu-iotests/255|  2 +-
 tests/qemu-iotests/256|  2 +-
 tests/qemu-iotests/iotests.py | 58 +++
 30 files changed, 70 insertions(+), 59 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 4f363f295f..9fa97966c5 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -383,8 +383,7 @@ def test_once(config, qemu_img=False):
 
 
 # Obviously we only work with the luks image format
-iotests.verify_image_format(supported_fmts=['luks'])
-iotests.verify_platform()
+iotests.script_initialize(supported_fmts=['luks'])
 
 # We need sudo in order to run cryptsetup to create
 # dm-crypt devices. This is safe to use on any
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index d746ab1e21..c8aeb6d0e4 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,8 +21,7 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'])
 
 with iotests.FilePath('source.img') as source_img_path, \
  iotests.FilePath('dest.img') as dest_img_path, \
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 581ca34d79..1271ac9459 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -24,8 +24,7 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index 4874a1a0d8..c40fe231ea 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -24,8 +24,7 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index 5bb738bf23..23ff2f624b 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 def blockdev_create(vm, options):
 result = vm.qmp_log('blockdev-create',
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index ec8c1d06f0..ab9e3b6747 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,7 +24,7 @@ import iotests
 import subprocess
 import re
 
-iotests.verify_image_format(supported_fmts=['raw'])
+iotests.script_initialize(supported_fmts=['raw'])
 iotests.verify_protocol(supported=['ssh'])
 
 def filter_hash(qmsg):
diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 1e202388dc..dfce6f9fe4 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -22,7 +22,7 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['generic'])
+iotests.script_initialize(supported_fmts=['generic'])
 
 with iotests.FilePath('disk.img') as disk_img_path, \
  iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
diff --git a/tests/qemu-iotests/209 b/tests/qemu-iotests/209
index 259e991ec6..a77f884166 100755
--- a/tests/qemu-iotests/209
+++ b/tests/qemu-iotests/209
@@ -22,7 +22,7 @@ import iotests
 from iotests import qemu_img_create, qemu_io, qemu_img_verbose, qemu_nbd, \
 file_path
 

[Qemu-devel] [PATCH v2 3/3] iotests: use python logging for iotests.log()

[John Snow]

We can turn logging on/off globally instead of per-function.

Remove use_log from run_job, and use python logging to turn on
diffable output when we run through a script entry point.

iotest 245 changes output order due to buffering reasons.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/030|  4 +--
 tests/qemu-iotests/245|  1 +
 tests/qemu-iotests/245.out| 24 +-
 tests/qemu-iotests/iotests.py | 47 +--
 4 files changed, 43 insertions(+), 33 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1b69f318c6..a382cb430b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -411,8 +411,8 @@ class TestParallelOps(iotests.QMPTestCase):
 result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
 self.assert_qmp(result, 'return', {})
 
-self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
-self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
+self.vm.run_job(job='drive0', auto_dismiss=True)
+self.vm.run_job(job='node4', auto_dismiss=True)
 self.assert_no_active_block_jobs()
 
 # Test a block-stream and a block-commit job in parallel
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index bc1ceb9792..3bc29acb33 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1000,4 +1000,5 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 self.reopen(opts, {'backing': 'hd2'})
 
 if __name__ == '__main__':
+iotests.activate_logging()
 iotests.main(supported_fmts=["qcow2"])
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index a19de5214d..15c3630e92 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -1,17 +1,17 @@
+{"execute": "job-finalize", "arguments": {"id": "commit0"}}
+{"return": {}}
+{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
 ..
 --
 Ran 18 tests
 
 OK
-{"execute": "job-finalize", "arguments": {"id": "commit0"}}
-{"return": {}}
-{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
-{"execute": "job-finalize", "arguments": {"id": "stream0"}}
-{"return": {}}
-{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
-{"execute": "job-finalize", "arguments": {"id": "stream0"}}
-{"return": {}}
-{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index d15260e5ad..8ce1329558 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -35,6 +35,13 @@ from collections import OrderedDict
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
+# Use this logger for logging messages directly from the iotests module
+logger = logging.getLogger(__name__)
+logger.addHandler(logging.NullHandler())
+
+# Use this logger for messages that ought to be used for diff output.
+test_logger = logging.getLogger('.'.join((__name__, 'iotest')))

Re: [Qemu-devel] [PATCH] Fix Guest VM crash due to iSCSI Sense Key error

[John Snow]

On 7/29/19 5:32 PM, Paolo Bonzini wrote:
> On 29/07/19 21:45, John Snow wrote:
>> Next, we'll unschedule the BH if there is one. I think the only case
>> where there is one is the reschedule_dma case of dma_blk_cb. (I'm not
>> too familiar with these DMA helpers: in what cases do we expect the iov
>> to be empty?)
> 
> When there is another I/O that is using the DMA bounce buffer (the one
> case that comes to mind in which you do DMA from MMIO areas is
> loading/saving VGA RAM).
> 
>> So it looks like this cancellation will produce one of two effects,
>> depending on when it's invoked:
>>
>> 1) We'll stall the DMA permanently by deleting that BH, because
>> dma_complete will never get invoked and therefore nobody will ever call
>> ide_dma_cb with any return value of any kind. The IDE state machine
>> likely just hangs waiting for the DMA to finish until the guest OS
>> decides to reset the errant controller.
>>
>> 2) The DMA will continue blissfully unaware it was canceled, because the
>> lower AIOCB has no cancel method, and so will finish, call back to
>> dma_blk_cb, and continue the transfer loop unaware.
>>
>>
>> ... Does your reading align with mine?
>>
>>
>> If it does -- if there are indeed no places in the code today that
>> artificially inject -ECANCELED -- I need to remove these special stanzas
>> from the IDE code and allow the IDE state machine to handle these errors
>> as true errors.
> 
> The bug is that there is no place to inject -ECANCELED in the dbs->bh
> case.  I've sent an obviously^W untested patch.
> 

Where does it inject -ECANCELED in the non-dbs->bh case?

--js

[Qemu-devel] [PATCH v2 2/3] iotests: add protocol support to initialization info

[John Snow]

This will add supported_protocols and unsupported_protocols to all of
iotests.main, iotests.script_main, and iotests.script_initialize.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/207| 4 ++--
 tests/qemu-iotests/210| 4 ++--
 tests/qemu-iotests/211| 4 ++--
 tests/qemu-iotests/212| 4 ++--
 tests/qemu-iotests/213| 4 ++--
 tests/qemu-iotests/iotests.py | 5 -
 6 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index ab9e3b6747..35d98f2736 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,8 +24,8 @@ import iotests
 import subprocess
 import re
 
-iotests.script_initialize(supported_fmts=['raw'])
-iotests.verify_protocol(supported=['ssh'])
+iotests.script_initialize(supported_fmts=['raw'],
+  supported_protocols=['ssh'])
 
 def filter_hash(qmsg):
 def _filter(key, value):
diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
index 5a7013cd34..d9fe780c07 100755
--- a/tests/qemu-iotests/210
+++ b/tests/qemu-iotests/210
@@ -23,8 +23,8 @@
 import iotests
 from iotests import imgfmt
 
-iotests.script_initialize(supported_fmts=['luks'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['luks'],
+  supported_protocols=['file'])
 
 def blockdev_create(vm, options):
 result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
index 4d6aac497f..155fac4e87 100755
--- a/tests/qemu-iotests/211
+++ b/tests/qemu-iotests/211
@@ -23,8 +23,8 @@
 import iotests
 from iotests import imgfmt
 
-iotests.script_initialize(supported_fmts=['vdi'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['vdi'],
+  supported_protocols=['file'])
 
 def blockdev_create(vm, options):
 result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/212 b/tests/qemu-iotests/212
index ec35bceb11..67e5a1dbb5 100755
--- a/tests/qemu-iotests/212
+++ b/tests/qemu-iotests/212
@@ -23,8 +23,8 @@
 import iotests
 from iotests import imgfmt
 
-iotests.script_initialize(supported_fmts=['parallels'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['parallels'],
+  supported_protocols=['file'])
 
 def blockdev_create(vm, options):
 result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/213 b/tests/qemu-iotests/213
index 3d2c024375..23f387ab63 100755
--- a/tests/qemu-iotests/213
+++ b/tests/qemu-iotests/213
@@ -23,8 +23,8 @@
 import iotests
 from iotests import imgfmt
 
-iotests.script_initialize(supported_fmts=['vhdx'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['vhdx'],
+  supported_protocols=['file'])
 
 def blockdev_create(vm, options):
 result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0229ffee0e..d15260e5ad 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -909,7 +909,9 @@ def execute_unittest(debug=False):
 def execute_setup_common(supported_fmts=[],
  supported_oses=['linux'],
  supported_cache_modes=[],
- unsupported_fmts=[]):
+ unsupported_fmts=[],
+ supported_protocols=[],
+ unsupported_protocols=[]):
 """
 Perform necessary setup for either script-style or unittest-style tests.
 """
@@ -925,6 +927,7 @@ def execute_setup_common(supported_fmts=[],
 verify_image_format(supported_fmts, unsupported_fmts)
 verify_platform(supported_oses)
 verify_cache_mode(supported_cache_modes)
+verify_protocol(supported_protocols, unsupported_protocols)
 
 debug = '-d' in sys.argv
 if debug:
-- 
2.21.0

[Qemu-devel] [PATCH v2 0/3] iotests: use python logging

[John Snow]

Based-on: https://github.com/jnsnow/qemu/tree/bitmaps

This series uses python logging to enable output conditionally on
iotests.log(). We unify an initialization call (which also enables
debugging output for those tests with -d) and then make the switch
inside of iotests.

It will help alleviate the need to create logged/unlogged versions
of all the various helpers we have made.

Depends on my bitmaps branch, because of the existing iotests
refactoring I have done there.

V2:
 - Added all of the other python tests I missed to use script_initialize
 - Refactored the common setup as per Ehabkost's suggestion
 - Added protocol arguments to common initialization,
   but this isn't strictly required.

John Snow (3):
  iotests: add script_initialize
  iotests: add protocol support to initialization info
  iotests: use python logging for iotests.log()

 tests/qemu-iotests/030|   4 +-
 tests/qemu-iotests/149|   3 +-
 tests/qemu-iotests/194|   3 +-
 tests/qemu-iotests/202|   3 +-
 tests/qemu-iotests/203|   3 +-
 tests/qemu-iotests/206|   2 +-
 tests/qemu-iotests/207|   4 +-
 tests/qemu-iotests/208|   2 +-
 tests/qemu-iotests/209|   2 +-
 tests/qemu-iotests/210|   4 +-
 tests/qemu-iotests/211|   4 +-
 tests/qemu-iotests/212|   4 +-
 tests/qemu-iotests/213|   4 +-
 tests/qemu-iotests/216|   3 +-
 tests/qemu-iotests/218|   2 +-
 tests/qemu-iotests/219|   2 +-
 tests/qemu-iotests/222|   5 +-
 tests/qemu-iotests/224|   3 +-
 tests/qemu-iotests/228|   3 +-
 tests/qemu-iotests/234|   3 +-
 tests/qemu-iotests/235|   4 +-
 tests/qemu-iotests/236|   2 +-
 tests/qemu-iotests/237|   2 +-
 tests/qemu-iotests/238|   2 +
 tests/qemu-iotests/242|   2 +-
 tests/qemu-iotests/245|   1 +
 tests/qemu-iotests/245.out|  24 
 tests/qemu-iotests/246|   2 +-
 tests/qemu-iotests/248|   2 +-
 tests/qemu-iotests/254|   2 +-
 tests/qemu-iotests/255|   2 +-
 tests/qemu-iotests/256|   2 +-
 tests/qemu-iotests/iotests.py | 108 ++
 33 files changed, 121 insertions(+), 97 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH] dma-helpers: ensure AIO callback is invoked after cancellation

[Paolo Bonzini]

dma_aio_cancel unschedules the BH if there is one, which corresponds
to the reschedule_dma case of dma_blk_cb.  This can stall the DMA
permanently, because dma_complete will never get invoked and therefore
nobody will ever invoke the original AIO callback in dbs->common.cb.

Fix this by invoking the callback (which is ensured to happen after
a bdrv_aio_cancel_async, or done manually in the dbs->bh case), and
add assertions to check that the DMA state machine is indeed waiting
for dma_complete or reschedule_dma, but never both.

Reported-by: John Snow 
Signed-off-by: Paolo Bonzini 
---
 dma-helpers.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 2d7e02d..d3871dc 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -90,6 +90,7 @@ static void reschedule_dma(void *opaque)
 {
 DMAAIOCB *dbs = (DMAAIOCB *)opaque;
 
+assert(!dbs->acb && dbs->bh);
 qemu_bh_delete(dbs->bh);
 dbs->bh = NULL;
 dma_blk_cb(dbs, 0);
@@ -111,15 +112,12 @@ static void dma_complete(DMAAIOCB *dbs, int ret)
 {
 trace_dma_complete(dbs, ret, dbs->common.cb);
 
+assert(!dbs->acb && !dbs->bh);
 dma_blk_unmap(dbs);
 if (dbs->common.cb) {
 dbs->common.cb(dbs->common.opaque, ret);
 }
 qemu_iovec_destroy(>iov);
-if (dbs->bh) {
-qemu_bh_delete(dbs->bh);
-dbs->bh = NULL;
-}
 qemu_aio_unref(dbs);
 }
 
@@ -179,14 +177,21 @@ static void dma_aio_cancel(BlockAIOCB *acb)
 
 trace_dma_aio_cancel(dbs);
 
+assert(!(dbs->acb && dbs->bh));
 if (dbs->acb) {
+/* This will invoke dma_blk_cb.  */
 blk_aio_cancel_async(dbs->acb);
+return;
 }
+
 if (dbs->bh) {
 cpu_unregister_map_client(dbs->bh);
 qemu_bh_delete(dbs->bh);
 dbs->bh = NULL;
 }
+if (dbs->common.cb) {
+dbs->common.cb(dbs->common.opaque, -ECANCELED);
+}
 }
 
 static AioContext *dma_get_aio_context(BlockAIOCB *acb)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] Fix Guest VM crash due to iSCSI Sense Key error

[Paolo Bonzini]

On 29/07/19 21:45, John Snow wrote:
> Next, we'll unschedule the BH if there is one. I think the only case
> where there is one is the reschedule_dma case of dma_blk_cb. (I'm not
> too familiar with these DMA helpers: in what cases do we expect the iov
> to be empty?)

When there is another I/O that is using the DMA bounce buffer (the one
case that comes to mind in which you do DMA from MMIO areas is
loading/saving VGA RAM).

> So it looks like this cancellation will produce one of two effects,
> depending on when it's invoked:
> 
> 1) We'll stall the DMA permanently by deleting that BH, because
> dma_complete will never get invoked and therefore nobody will ever call
> ide_dma_cb with any return value of any kind. The IDE state machine
> likely just hangs waiting for the DMA to finish until the guest OS
> decides to reset the errant controller.
> 
> 2) The DMA will continue blissfully unaware it was canceled, because the
> lower AIOCB has no cancel method, and so will finish, call back to
> dma_blk_cb, and continue the transfer loop unaware.
> 
> 
> ... Does your reading align with mine?
> 
> 
> If it does -- if there are indeed no places in the code today that
> artificially inject -ECANCELED -- I need to remove these special stanzas
> from the IDE code and allow the IDE state machine to handle these errors
> as true errors.

The bug is that there is no place to inject -ECANCELED in the dbs->bh
case.  I've sent an obviously^W untested patch.

Paolo

> I'm just not confident enough in my unwinding of the DMA callback
> spaghetti, though.
> 
> --js
> 

Re: [Qemu-devel] [PATCH v3 3/3] nbd: improve CMD_CACHE: use BDRV_REQ_PREFETCH

[Eric Blake]

On 7/25/19 5:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> This helps to avoid extra io, allocations and memory copying.
> We assume here that CMD_CACHE is always used with copy-on-read, as
> otherwise it's a noop.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  nbd/server.c | 43 +++
>  1 file changed, 35 insertions(+), 8 deletions(-)

libnbd is able to test NBD_CMD_CACHE requests, and I've confirmed that
behavior looks sane under requests issued that way.  I'm wondering if
qemu-io should also be able to drive this mode of operation, so that we
can give it iotest coverage without depending on libnbd.

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/4] virtio/block: handle zoned backing devices

[John Snow]

On 7/26/19 7:42 PM, Dmitry Fomichev wrote:
> John, please see inline...
> 
> Regards,
> Dmitry
> 
> On Thu, 2019-07-25 at 13:58 -0400, John Snow wrote:
>>
>> On 7/23/19 6:19 PM, Dmitry Fomichev wrote:
>>> Currently, attaching zoned block devices (i.e., storage devices
>>> compliant to ZAC/ZBC standards) using several virtio methods doesn't
>>> work properly as zoned devices appear as regular block devices at the
>>> guest. This may cause unexpected i/o errors and, potentially, some
>>> data corruption.
>>>
>>
>> Hi, I'm quite uninitiated here, what's a zoned block device? What are
>> the ZAC/ZBC standards?
> Zoned block devices (ZBDs) are HDDs that use SMR (shingled magnetic
> recording). This type of recording, if applied to the entire drive, would
> only allow the drive to be written sequentially. To make such devices more
> practical, the entire LBA range of the drive is divided into zones. All
> writes within a particular zone must be sequential, but writing different
> zones can be done concurrently in random manner. This sounds like a lot of
> hassle, but in return SMR can achieve up to 20% better areal data density
> compared to the most common PMR recording.
> 
> The same zoned model is used in up-and-coming NVMe ZNS standard, even
> though the reason for using it in ZNS is different compared to SMR HDDs -
> easier flash erase block management.
> 
> ZBC is an INCITS T10 (SCSI) standard and ZAC is the corresponding T13 (ATA)
> standard.
> 
> The lack of limelight for these standards is explained by the fact that
> these devices are mostly used by cloud infrastructure providers for "cold"
> data storage, a purely enterprise application. Currently, both WDC and
> Seagate produce SMR drives in significant quantities and Toshiba has
> announced support for ZBDs in their future products.
> 
>>>
>> I've found this:
>> https://www.snia.org/sites/default/files/SDC/2016/presentations/smr/DamienLeMoal_ZBC-ZAC_Linux.pdf
>>
> AFAIK, the most useful collection of public resources about zoned block
> devices is this website -
> http://zonedstorage.io
> The site is maintained by our group at WDC (shameless plug here :) ).
> BTW, here is the page containing the links to T10/T13 standards
> (the access might be restricted for non-members of T10/T13 committees) -
> http://zonedstorage.io/introduction/smr/#governing-standards
> 
>> It looks like ZAC/ZBC are new commands -- what happens if we just don't
>> use them, exactly?
> The standards define three models of zoned block devices: drive-managed,
> host-aware and host-managed.
> 
> Drive-managed zoned devices behave just like regular SCSI/ATA devices and
> don't require any additional support. There is no point for manufacturers
> to market such devices as zoned. Host-managed and host-aware devices can
> read data exactly the same way as common SCSI/ATA drives, but there are
> I/O pattern limitations in the write path that the host must adhere to.
> 
> Host-aware drives will work without I/O errors under purely random write
> workload, but their performance might be significantly degraded
> compared to running them under zone-sequential workload. With
> host-managed drives, any non-sequential writes within zones will lead
> to an I/O error, most likely, "unaligned write".
> 
> It is important to mention that almost all zoned devices that are
> currently on the market are host-managed.
> 

OK, understood.

> ZAC/ZBC standards do add some new commands to the common SCSI/ACS command
> sets, but, at least for the host-managed model, it wouldn't be sufficient
> just to never issue these commands to be able to utilize these devices.
> 
>>
>>> To be more precise, attaching a zoned device via virtio-pci-blk,
>>> virtio-scsi-pci/scsi-disk or virtio-scsi-pci/scsi-hd demonstrates the
>>> above behavior. The virtio-scsi-pci/scsi-block method works with a
>>> recent patch. The virtio-scsi-pci/scsi-generic method also appears to
>>> handle zoned devices without problems.
>>>
>>
>> What exactly fails, out of curiosity?
> The current Linux kernel is able to recognize zoned block devices and
> provide some means for the user to see that a particular device is zoned.
> For example, lsscsi will show "zbc" instead of "disk" for zoned devices.
> Another useful value is the "zoned" sysfs attribute that carries the
> zoned model of the drive. Without this patch, the attachment methods
> mentioned above present host-managed drives as regular drives at the
> guest system. There is no way for the user to figure out that they are
> dealing with a ZBD besides starting I/O and getting "unaligned write"
> error.
> 

Mmhmm...

> The folks who designed ZAC/ZBC were very careful about this not to
> happen and this doesn't happen on bare metal. Host-managed drives have
> a distinctive SCSI device type, 0x14, and old kernels without zoned
> device support simply are not be able to classify these drives during
> the device scan. The kernels with ZBD support are able to recognize
> a host-managed 

Re: [Qemu-devel] [PATCH v3 2/3] block/stream: use BDRV_REQ_PREFETCH

[Eric Blake]

On 7/25/19 5:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> This helps to avoid extra io, allocations and memory copying.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/stream.c | 20 +++-
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 6ac1e7bec4..170e3b43be 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -22,11 +22,11 @@
>  
>  enum {
>  /*
> - * Size of data buffer for populating the image file.  This should be 
> large
> + * Maximum chunk size to feed it to copy-on-read.  This should be large

s/feed it/feed/

Will fix during staging

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PULL 0/3] virtio, pc: fixes

[Michael S. Tsirkin]

I'm sending this out now as these patches are ready,
but it seems likely we'll need another patch for pci,
and as it deals with migration compat it might be a blocker.
Will know more tomorrow :(


The following changes since commit fff3159900d2b95613a9cb75fc3703e67a674729:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190726' 
into staging (2019-07-26 16:23:07 +0100)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 22235bb609c18547cf6b215bad1f9d2ec56ad371:

  pc-dimm: fix crash when invalid slot number is used (2019-07-29 16:57:27 
-0400)


virtio, pc: fixes

A couple of last minute bugfixes.

Signed-off-by: Michael S. Tsirkin 


Dr. David Alan Gilbert (2):
  Revert "Revert "globals: Allow global properties to be optional""
  Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only 
devs"

Igor Mammedov (1):
  pc-dimm: fix crash when invalid slot number is used

 hw/virtio/virtio-pci.h| 31 ++-
 include/hw/qdev-core.h|  3 +++
 hw/core/machine.c | 23 +++
 hw/display/virtio-gpu-pci.c   |  4 +---
 hw/display/virtio-vga.c   |  4 +---
 hw/mem/pc-dimm.c  |  7 +++
 hw/virtio/virtio-crypto-pci.c |  4 +---
 hw/virtio/virtio-input-pci.c  |  4 +---
 hw/virtio/virtio-pci.c| 26 ++
 qom/object.c  |  3 +++
 10 files changed, 36 insertions(+), 73 deletions(-)

Re: [Qemu-devel] [PATCH v3 1/3] block: implement BDRV_REQ_PREFETCH

[Eric Blake]

On 7/25/19 5:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> Do effective copy-on-read request when we don't need data actually. It
> will be used for block-stream and NBD_CMD_CACHE.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/block.h |  8 +++-
>  block/io.c| 18 --
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 50a07c1c33..73c3fc4daa 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -88,8 +88,14 @@ typedef enum {
>   * fallback. */
>  BDRV_REQ_NO_FALLBACK= 0x100,
>  
> +/*
> + * BDRV_REQ_PREFETCH may be used only together with BDRV_REQ_COPY_ON_READ
> + * on read request and means that caller don't really need data to be

doesn't

can fix up while staging.


> +++ b/block/io.c
> @@ -1167,7 +1167,8 @@ bdrv_driver_pwritev_compressed(BlockDriverState *bs, 
> uint64_t offset,
>  }
>  
>  static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
> -int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
> +int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
> +int flags)

We're bad about this, but 'int flags' does not play well with the C
language and well-defined behavior when it comes to 1<<31 (bit
operations and unsigned types have guaranteed behavior, bit operations
and negative signed types can cause the compiler to do differently than
you expect).  Not a problem for uses where we don't have 32 flags to OR
together, so I won't change it, so much as point it out for a bigger
task of auditing the entire code base if we are worried.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4] qapi: Add InetSocketAddress member keep-alive

[Eric Blake]

On 7/25/19 4:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> It's needed to provide keepalive for nbd client to track server
> availability.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---

> +++ b/util/qemu-sockets.c
> @@ -219,6 +219,12 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>  bool socket_created = false;
>  Error *err = NULL;
>  
> +if (saddr->keep_alive) {
> +error_setg(errp, "keep-alive options is not supported for passive "

option

(will fix as part of staging)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/2] iotests: add script_initialize

[John Snow]

On 7/28/19 8:07 PM, Eduardo Habkost wrote:
> On Fri, Jul 26, 2019 at 06:52:00PM -0400, John Snow wrote:
>> Like script_main, but doesn't require a single point of entry.
>> Replace all existing initialization sections with this drop-in replacement.
>>
>> This brings debug support to all existing script-style iotests.
>>
>> Note: supported_oses=['linux'] was omitted, as it is a default argument.
>> Signed-off-by: John Snow 
> [...]
> 
> Looks good overall, I just have one comment:
> 
> 
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 727730422f..5e9b2989dd 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -891,9 +891,8 @@ def execute_unittest(output, verbosity, debug):
>>  sys.stderr.write(re.sub(r'Ran (\d+) tests? in [\d.]+s',
>>  r'Ran \1 tests', output.getvalue()))
>>  
>> -def execute_test(test_function=None,
>> - supported_fmts=[], supported_oses=['linux'],
>> - supported_cache_modes=[], unsupported_fmts=[]):
>> +def execute_setup_common(supported_fmts=[], supported_oses=['linux'],
>> + supported_cache_modes=[], unsupported_fmts=[]):
>>  """Run either unittest or script-style tests."""
>>  
>>  # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
>> @@ -925,16 +924,28 @@ def execute_test(test_function=None,
>>  output = io.BytesIO()
>>  
>>  logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
>> +return output, verbosity, debug
> 
> Can't we make this simpler?
> 
> What about just returning `debug`, and letting execute_unittest()
> take care of `verbosity` and `output`?
> 

"yes," it turns out. v2 soon.

Re: [Qemu-devel] [PATCH 22/28] Include hw/boards.h a bit less

[Eduardo Habkost]

On Fri, Jul 26, 2019 at 02:05:36PM +0200, Markus Armbruster wrote:
> hw/boards.h pulls in almost 60 headers.  The less we include it into
> headers, the better.  As a first step, drop superfluous inclusions,
> and downgrade some more to what's actually needed.  Gets rid of just
> one inclusion into a header.
> 
> Cc: Eduardo Habkost 
> Cc: Marcel Apfelbaum 
> Signed-off-by: Markus Armbruster 
> ---

The following files use the MACHINE macro and require
hw/boards.h, but are touched by this patch:

hw/acpi/cpu.c:MachineState *machine = MACHINE(qdev_get_machine());
hw/acpi/memory_hotplug.c:MachineState *machine = 
MACHINE(qdev_get_machine());
hw/i386/intel_iommu.c:MachineState *ms = MACHINE(qdev_get_machine());
hw/i386/x86-iommu.c:MachineState *ms = MACHINE(qdev_get_machine());
hw/ppc/spapr_rtas.c:MachineState *ms = MACHINE(qdev_get_machine());
hw/s390x/s390-stattrib-kvm.c:MachineState *machine = 
MACHINE(qdev_get_machine());
hw/s390x/s390-stattrib-kvm.c:MachineState *machine = 
MACHINE(qdev_get_machine());

Maybe there are other files touched by this patch that require
struct MachineClass or struct MachineState contents to be
defined, but this is a bit trickier to verify.

-- 
Eduardo

Re: [Qemu-devel] [Qemu-stable] [PATCH 00/36] Patch Round-up for stable 3.1.1, freeze on 2019-07-29

[Bruce Rogers]

On Tue, 2019-07-23 at 12:00 -0500, Michael Roth wrote:
> Hi
> everyone,
>   
> 
> The following new patches are queued for QEMU stable v3.1.1:
> 
>   https://github.com/mdroth/qemu/commits/stable-3.1-staging
> 
> The release is planned for 2019-08-01:
> 
>   https://wiki.qemu.org/Planning/3.1
> 
> Please respond here or CC qemu-sta...@nongnu.org on any patches you
> think should be included in the release.
> 
> Note that this update falls outside the normal stable release support
> window (~1 development cycle), but is being released now since it was
> delayed from its intended release date.
> 
> Thanks!
> 
> 

Beyond what has already been suggested, here are some patches we've
added to our v3.1.0 based QEMU releases:

commit 2c858ce5da8ae6689c75182b73bc455a291cad41
Author: Prasad J Pandit 
Date:   Thu Dec 13 01:00:36 2018 +0530

pvrdma: check number of pages when creating rings


commit f1e2e38ee0136b7710a2caa347049818afd57a1b
Author: Prasad J Pandit 
Date:   Thu Dec 13 01:00:39 2018 +0530

pvrdma: check return value from pvrdma_idx_ring_has_ routines


commit 509f57c98e7536905bb4902363d0cba66ce7e089
Author: Prasad J Pandit 
Date:   Thu Dec 13 01:00:37 2018 +0530

pvrdma: release ring object in case of an error


commit 4720cbeea1f42fd905fc69338fd42b191e58b412
Author: Kevin Wolf 
Date:   Mon Jan 7 13:02:48 2019 +0100

block: Fix hangs in synchronous APIs with iothreads


commit 6f5d8671225dc77190647f18a27a0d156d4ca97a
Author: Prasad J Pandit 
Date:   Tue Jul 23 16:17:52 2019 +0530

qemu-bridge-helper: restrict interface name to IFNAMSIZ


Thanks,

Bruce

Re: [Qemu-devel] Exploring Sphinx, autodoc, apidoc, and coverage tools for python/qemu

[John Snow]

On 7/26/19 5:16 PM, Eduardo Habkost wrote:
> CCing Cleber and Gabriel.  Comments at the "conclusions" section
> below:
> 
> On Wed, Jul 24, 2019 at 05:06:41PM -0400, John Snow wrote:
>> Has anyone on this list experimented with these tools?
>>
>> I was hoping to use them to document things like the python/machine.py
>> and python/qmp.py modules to help demonstrate some of our internal
>> tooling API (for test writers, GSoC/Outreachy interns, folks who want to
>> script QEMU at a level between writing a CLI driver and using libvirt.)
>>
>> What follows below is my process trying to enable this and some of the
>> problems I'm still stuck with, summarized below at the end of this more
>> exploratory text.
>>
>>
>> Enabling autodoc:
>>
>> First, it appears as if enabling the "sphinx-autodoc" tool is not
>> sufficient for actually generating anything at all when you invoke the
>> sphinx-generated "make html" target. It just enables understanding
>> certain directives.
>>
>> So apparently you need to generate module "stubs" using sphinx-autodoc.
>> Sphinx uses the sphinx-autodoc extension to understand how to consume
>> the directives in these stubs.
>>
>> That strikes me as odd, because these stubs might need to be changed
>> frequently as code comes and goes; it seems strange that it isn't
>> integrated at the top level. (Do I have the wrong idea on how these
>> tools should be used?)
>>
>> So you need to run:
>>> sphinx-apidoc --separate --module-first -o docs/ python/qemu/
>>
>> which generates stubs to docs:
>>
>> Creating file docs/qemu.machine.rst.
>> Creating file docs/qemu.qmp.rst.
>> Creating file docs/qemu.qtest.rst.
>> Creating file docs/qemu.rst.
>> Creating file docs/modules.rst.
>>
>> And then you can edit e.g. the top-level index.rst TOC in docs/index.rst
>> to look like this:
>>
>> ```
>> .. toctree::
>>:maxdepth: 2
>>:caption: Contents:
>>
>>interop/index
>>devel/index
>>specs/index
>>modules
>> ```
>>
>> And then finally generating the build; manually removing the -W option
>> from the Makefile: there are a lot of warnings in here.
>>
>>> sphinx-build -n -b html -D version=4.0.92 -D release="4.0.92
>> (v4.1.0-rc2-34-g160802eb07-dirty)" -d .doctrees/
>> /home/bos/jhuston/src/qemu/docs/ docs/
>>
>> Great! that will generate output to docs/index.html which indeed shows
>> APIdoc comments generated from our Python files. Good.
>>
>> However, where this gets a little strange is if you look at the
>> generated stubs. For instance, qemu.machine.rst looks like this:
>>
>> ```
>> .. automodule:: qemu.machine
>> :members:
>> :undoc-members:
>> :show-inheritance:
>> ```
>>
>> :undoc-members: says that we want to "document" any members that don't
>> have a matching apidoc comment by generating a stub.
>>
>> Oops, but the presence of that stub will cause the sphinx coverage tool
>> to happily report 100% coverage.
>>
>> Further oops, pylint doesn't understand apidoc comments and can't be
>> used as the linter in this case, either.
>>
>> You can edit the stubs to remove these directives, but these stubs are
>> generated -- and it doesn't appear like there's a command line option to
>> change this behavior. ...Hmm.
>>
>> And either way, the coverage tool only generates a report and not
>> something with an error code that I could use to gate the build. Same
>> goes for the general build: if I remove the :undoc-members: parameter,
>> there's nothing in the autodoc module that appears to throw warnings
>> when it encounters undocumented parameters or members.
>>
>> That seems disappointing, because it's hard to keep docstrings up to
>> date unless they are checked conclusively at build time.
>>
>>
>> Conclusions:
>>
>> - the autodoc documentation page doesn't seem to document examples of
>> how you're expected to write meaningful docstrings for the tool to extract.
> 
> I had the same impression when I read sphinx/autodoc
> documentation.
> 
>>
>> - autodoc fools the coverage tool into reporting 100% coverage.
>>
>> - autodoc can be configured to omit non-documented members to allow the
>> coverage tool to work, but the configuration is auto-generated and
>> defaults to always generating documentation for these entities.
>>
>> - coverage tool doesn't appear like it can be used for gating the build
>> natively for missing python docs; it only generates a report.
>>
>> - Even if we script to block on a non-empty report, the coverage tool
>> only works at the function/class level and does not understand the
>> concept of missing parameter or return value tags.
>>
>> - It would seem that it would be the Autodoc module's job to be
>> responsible for understanding incomplete documentation, but doesn't
>> appear to. The :param name: syntax is just a ReST "field list" and isn't
>> parsed semantically by autodoc, sadly.
> 
> I wonder if there are other Python documentation coverage tools
> outside Sphinx.  Googling for [python docstring coverage]
> resulted in a few different 

Re: [Qemu-devel] [PATCH 22/28] Include hw/boards.h a bit less

[Eduardo Habkost]

On Fri, Jul 26, 2019 at 02:05:36PM +0200, Markus Armbruster wrote:
> hw/boards.h pulls in almost 60 headers.  The less we include it into
> headers, the better.  As a first step, drop superfluous inclusions,
> and downgrade some more to what's actually needed.  Gets rid of just
> one inclusion into a header.
> 
> Cc: Eduardo Habkost 
> Cc: Marcel Apfelbaum 
> Signed-off-by: Markus Armbruster 
[...]
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c58a8e594e..2c9c93983a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -15,7 +15,6 @@
>  #include "qapi/qmp/qerror.h"
>  #include "sysemu/replay.h"
>  #include "qemu/units.h"
> -#include "hw/boards.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-visit-common.h"
>  #include "qapi/visitor.h"

This doesn't look right.  hw/core/machine.c contains the
implementation of most functions declared at hw/boards.h, and
surely requires struct MachineClass and struct MachineState to be
defined.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 25/28] numa: Move remaining NUMA declarations from sysemu.h to numa.h

[Eduardo Habkost]

On Fri, Jul 26, 2019 at 02:05:39PM +0200, Markus Armbruster wrote:
> Commit e35704ba9c "numa: Move NUMA declarations from sysemu.h to
> numa.h" left a few NUMA-related macros behind.  Move them now.
> 
> Cc: Eduardo Habkost 
> Cc: Marcel Apfelbaum 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 24/28] Include sysemu/hostmem.h less

[Eduardo Habkost]

On Fri, Jul 26, 2019 at 02:05:38PM +0200, Markus Armbruster wrote:
> Move the HostMemoryBackend typedef from sysemu/hostmem.h to
> qemu/typedefs.h.  This renders a few inclusions of sysemu/hostmem.h
> superflouous; drop them.
> 
> Cc: Eduardo Habkost 
> Cc: Igor Mammedov 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Palmer Dabbelt]

On Mon, 22 Jul 2019 22:30:15 PDT (-0700), bmeng...@gmail.com wrote:

Hi Palmer,

On Sat, Jul 20, 2019 at 9:47 AM Palmer Dabbelt  wrote:


On Fri, 14 Jun 2019 08:15:51 PDT (-0700), bmeng...@gmail.com wrote:
> This adds a reset opcode for sifive_test device to trigger a system
> reset for testing purpose.
>
> Signed-off-by: Bin Meng 
> ---
>
>  hw/riscv/sifive_test.c | 4 
>  include/hw/riscv/sifive_test.h | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> index 24a04d7..cd86831 100644
> --- a/hw/riscv/sifive_test.c
> +++ b/hw/riscv/sifive_test.c
> @@ -21,6 +21,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "qemu/module.h"
> +#include "sysemu/sysemu.h"
>  #include "target/riscv/cpu.h"
>  #include "hw/riscv/sifive_test.h"
>
> @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
>  exit(code);
>  case FINISHER_PASS:
>  exit(0);
> +case FINISHER_RESET:
> +qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +return;
>  default:
>  break;
>  }
> diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
> index 71d4c9f..c186a31 100644
> --- a/include/hw/riscv/sifive_test.h
> +++ b/include/hw/riscv/sifive_test.h
> @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
>
>  enum {
>  FINISHER_FAIL = 0x,
> -FINISHER_PASS = 0x
> +FINISHER_PASS = 0x,
> +FINISHER_RESET = 0x
>  };
>
>  DeviceState *sifive_test_create(hwaddr addr);

Reviewed-by: Palmer Dabbelt 


Thanks a lot!


Sorry this took a while, but it's in the hardware now.  I'll merge this, but
I'm considering it a new feature so it'll be held off a bit.


"but it's in the hardware now", do you mean the code I added (0x)
is now supported by a newer version SiFive test device with compatible
string "sifive,test1", and can actually do the system wide reset?


No, the hardware is still a "sifive,test0" as plumbing through the reset is
trickier than I wanted to take on.  I just reserved the 0x code and
implemented it by triggering an unsupported function error, so we don't
accidentally use it for something else later.

Re: [Qemu-devel] [PATCH] Fix Guest VM crash due to iSCSI Sense Key error

[John Snow]

On 7/29/19 6:09 AM, Stefan Hajnoczi wrote:
> On Fri, Jul 26, 2019 at 04:18:46PM -0400, John Snow wrote:
>> Paolo, Stefan and Kevin: can I loop you in here? I'm quite uncertain
>> about this and I'd like to clear this up quickly if it's possible:
>>
>> On 7/25/19 8:58 PM, John Snow wrote:
>>>
>>>
>>> On 7/7/19 10:55 PM, shaju.abra...@nutanix.com wrote:
 From: Shaju Abraham 

 During the  IDE DMA transfer for a ISCSI target,when libiscsi encounters
 a SENSE KEY error, it sets the task->sense to  the value "COMMAND ABORTED".
 The function iscsi_translate_sense() later translaters this error to 
 -ECANCELED
 and this value is passed to the callback function. In the case of  IDE DMA 
 read
 or write, the callback function returns immediately if the value of the ret
 argument is -ECANCELED.
 Later when ide_cancel_dma_sync() function is invoked  the assertion
 "s->bus->dma->aiocb == ((void *)0)" fails and the qemu process gets 
 terminated.
 Fix the issue by making the value of s->bus->dma->aiocb = NULL when
 -ECANCELED is passed to the callback.

 Signed-off-by: Shaju Abraham 
 ---
  hw/ide/core.c | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/hw/ide/core.c b/hw/ide/core.c
 index 6afadf8..78ea357 100644
 --- a/hw/ide/core.c
 +++ b/hw/ide/core.c
 @@ -841,6 +841,7 @@ static void ide_dma_cb(void *opaque, int ret)
  bool stay_active = false;
  
  if (ret == -ECANCELED) {
 +s->bus->dma->aiocb = NULL;
  return;
  }
  

>>>
>>> The part that makes me nervous here is that I can't remember why we do
>>> NO cleanup whatsoever for the ECANCELED case.
>>>
>>> commit 0d910cfeaf2076b116b4517166d5deb0fea76394
>>> Author: Fam Zheng 
>>> Date:   Thu Sep 11 13:41:07 2014 +0800
>>>
>>> ide/ahci: Check for -ECANCELED in aio callbacks
>>>
>>>
>>> ... This looks like we never expected the aio callbacks to ever get
>>> called with ECANCELED, so we treat this as a QEMU-internal signal.
>>>
>>> It looks like we expect these callbacks to do NOTHING in this case; but
>>> I'm not sure where the IDE state machine does its cleanup otherwise.
>>> (The DMA might have been canceled, but the DMA and IDE state machines
>>> still need to exit their loop.)
>>>
>>> If you take a look at this patch from 2014 though, there are many other
>>> spots where we have littered ECANCELED checks that might also cause
>>> problems if we're receiving error codes we thought we couldn't get normally.
>>>
>>> I am worried this patch papers over something worse.
>>>
>> I'm not clear why Fam's patch adds a do-nothing return to the ide_dma_cb
>> if it's invoked with ECANCELED: shouldn't it be the case that the IDE
>> state machine needs to know that a transfer it was relying on to service
>> an ATA command was canceled and treat it like an error?
>>
>> Why was it ever correct to ignore these? Is it because we only ever
>> canceled DMA during reset/shutdown/etc?
>>
>> It appears as if iscsi requests can actually genuinely return an
>> ECANCELED errno, so there are likely several places in the IDE code that
>> need to accommodate this from happening.
>>
>> The easiest fix LOOKS like just deleting the special-casing of ECANCELED
>> altogether and letting the error pathways handle things as normal.
>>
>> Am I mistaken?
> 
> I think your instincts are right that there are deeper issues.  The
> first step would be test cases, then you can be sure various scenarios
> have been handled correctly.
> 

Suggestions? I'm not sure what's supposed to work and in what way here.
I guess this stuff was introduced for bdrv_aio_cancel_async, but it's
not immediately clear what's supposed to happen when you call that.

> I noticed that ide_sector_read_cb(), ide_sector_write_cb(), and
> ide_flush_cb() all differ in whether they reset s->pio_aiocb and
> s->status before returning early due to -ECANCELED.  That must be a bug.
> 
> I didn't look at the ide_dma_cb() code path.
> 
> Stefan
> 

Hm ...

It looks like canceling the ide_dma_cb AIOCB objects doesn't do anything
too useful?

dma_blk_io and friends will establish dma_aio_cancel as the async cancel
callback. So if we do cancel these objects, we're going to call this
function:

static void dma_aio_cancel(BlockAIOCB *acb)
{
DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common);

trace_dma_aio_cancel(dbs);

if (dbs->acb) {
blk_aio_cancel_async(dbs->acb);
}
if (dbs->bh) {
cpu_unregister_map_client(dbs->bh);
qemu_bh_delete(dbs->bh);
dbs->bh = NULL;
}
}

but there's no cancel callback for the lower layer in dbs->acb, so
that's just a nop, I think -- blk_aio_prwv doesn't offer an asynchronous
cancel mechanism.

Next, we'll unschedule the BH if there is one. I think the only case
where there is one is the reschedule_dma case of dma_blk_cb. (I'm not
too familiar with these DMA helpers: in what cases do we expect 

Re: [Qemu-devel] [PATCH 23/28] numa: Don't include hw/boards.h into sysemu/numa.h

[Eduardo Habkost]

On Fri, Jul 26, 2019 at 02:05:37PM +0200, Markus Armbruster wrote:
> sysemu/numa.h includes hw/boards.h just for the CPUArchId typedef, at
> the cost of pulling in more than two dozen extra headers indirectly.
> 
> I could move the typedef from hw/boards.h to qemu/typedefs.h.  But
> it's used in just two headers: boards.h and numa.h.
> 
> I could move it to another header both its users include.
> exec/cpu-common.h seems to be the least bad fit.
> 
> But I'm keeping this simple & stupid: declare the struct tag in
> numa.h.
> 
> Cc: Eduardo Habkost 
> Cc: Marcel Apfelbaum 
> Signed-off-by: Markus Armbruster 
> ---
>  exec.c | 1 +
>  hw/core/machine-qmp-cmds.c | 1 +
>  hw/core/machine.c  | 1 +
>  hw/mem/pc-dimm.c   | 1 +
>  include/hw/boards.h| 2 +-
>  include/sysemu/numa.h  | 9 +++--
>  6 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 6d60fdfb1f..4d9e146c79 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -41,6 +41,7 @@
>  #if defined(CONFIG_USER_ONLY)
>  #include "qemu.h"
>  #else /* !CONFIG_USER_ONLY */
> +#include "hw/boards.h"
>  #include "exec/memory.h"
>  #include "exec/ioport.h"
>  #include "sysemu/dma.h"
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index c83e8954e1..d8284671f0 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -9,6 +9,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "cpu.h"
> +#include "hw/boards.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-machine.h"
>  #include "qapi/qmp/qerror.h"
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2c9c93983a..901f3fa905 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -23,6 +23,7 @@
>  #include "sysemu/numa.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/qtest.h"
> +#include "hw/boards.h"
>  #include "hw/pci/pci.h"
>  #include "hw/mem/nvdimm.h"
>  
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 0c83dcd61e..fa90d4fc6c 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "hw/boards.h"
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 67e551636a..739d109fe1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -86,7 +86,7 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass 
> *mc, const char *type);
>   * @props - CPU object properties, initialized by board
>   * #vcpus_count - number of threads provided by @cpu object
>   */
> -typedef struct {
> +typedef struct CPUArchId {
>  uint64_t arch_id;
>  int64_t vcpus_count;
>  CpuInstanceProperties props;
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 01a263eba2..4c4c1dee9b 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -4,7 +4,10 @@
>  #include "qemu/bitmap.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/hostmem.h"
> -#include "hw/boards.h"
> +#include "qapi/qapi-types-machine.h"

This seems to be needed because of NodeInfo.

> +#include "exec/cpu-common.h"

This seems to be needed because of ram_addr_t.

Reviewed-by: Eduardo Habkost 


> +
> +struct CPUArchId;
>  

I wonder if we should do the same for other struct types like
NodeInfo.

Why is it bad to require the inclusion of hw/boards.h just
because of CPUArchId, but acceptable to require the inclusion of
qapi-types-machine.h just to be able to write "NodeInfo *nodes"
instead of "struct NodeInfo *nodes" below?

>  extern int nb_numa_nodes;   /* Number of NUMA nodes */
>  extern bool have_numa_distance;
> @@ -32,5 +35,7 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo 
> *nodes,
>   int nb_nodes, ram_addr_t size);
>  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>int nb_nodes, ram_addr_t size);
> -void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error 
> **errp);
> +void numa_cpu_pre_plug(const struct CPUArchId *slot, DeviceState *dev,
> +   Error **errp);
> +
>  #endif
> -- 
> 2.21.0
> 

-- 
Eduardo

[Qemu-devel] [PATCH] xen: cleanup IOREQ server on exit

[Igor Druzhinin]

Device model is supposed to destroy IOREQ server for itself.

Signed-off-by: Igor Druzhinin 
---
 hw/i386/xen/xen-hvm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index e8e79e0..30a5948 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1242,6 +1242,8 @@ static void xen_exit_notifier(Notifier *n, void *data)
 {
 XenIOState *state = container_of(n, XenIOState, exit);
 
+xen_destroy_ioreq_server(xen_domid, state->ioservid);
+
 xenevtchn_close(state->xce_handle);
 xs_daemon_close(state->xenstore);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 21/28] Include hw/qdev-properties.h less

[Eduardo Habkost]

On Fri, Jul 26, 2019 at 02:05:35PM +0200, Markus Armbruster wrote:
> In my "build everything" tree, changing hw/qdev-properties.h triggers
> a recompile of some 2700 out of 6600 objects (not counting tests and
> objects that don't depend on qemu/osdep.h).
> 
> Many places including hw/qdev-properties.h (directly or via hw/qdev.h)
> actually need only hw/qdev-core.h.  Include hw/qdev-core.h there
> instead.
> 
> hw/qdev.h is actually pointless: all it does is include hw/qdev-core.h
> and hw/qdev-properties.h, which in turn includes hw/qdev-core.h.
> Replace the remaining uses of hw/qdev.h by hw/qdev-properties.h.
> 
> While there, delete a few superfluous inclusions of hw/qdev-core.h.
> 
> Touching hw/qdev-properties.h now recompiles some 1200 objects.

Impressive, thanks for doing this.

I only see superfluous whitespace changes at
hw/sd/sdhci-internal.h and hw/qdev-dma.h, below.
They are harmless, though, so:

Reviewed-by: Eduardo Habkost 


[...]
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index 34141400f8..1d9053c183 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -21,6 +21,7 @@
>   * You should have received a copy of the GNU General Public License along
>   * with this program; if not, see .
>   */
> +
>  #ifndef SDHCI_INTERNAL_H
>  #define SDHCI_INTERNAL_H
>  
[...]
> diff --git a/include/hw/qdev-dma.h b/include/hw/qdev-dma.h
> index b00391aa0c..002705c57d 100644
> --- a/include/hw/qdev-dma.h
> +++ b/include/hw/qdev-dma.h
> @@ -10,6 +10,7 @@
>  #ifndef HW_QDEV_DMA_H
>  #define HW_QDEV_DMA_H
>  
> +
>  #define DEFINE_PROP_DMAADDR(_n, _s, _f, _d)   \
>  DEFINE_PROP_UINT64(_n, _s, _f, _d)
>  
[...]

-- 
Eduardo

Re: [Qemu-devel] [for-4.2 PATCH 0/2] PCI DMA alias support

[Michael S. Tsirkin]

On Fri, Jul 26, 2019 at 06:55:27PM -0600, Alex Williamson wrote:
> Please see patch 1/ for the motivation and utility of this series.
> This v1 submission improves on the previous RFC with revised commit
> logs, comments, and more testing, and the missing IVRS support for DMA
> alias ranges is now included.  Testing has been done with Linux guests
> with both SeaBIOS and OVMF with configurations of intel-iommu and
> amd-iommu.  Intel-iommu testing includes device assignment, amd-iommu
> is necessarily limited to emulated devices with interrupt remapping
> disabled and iommu=pt in the guest (enabling interrupt remapping or
> disabling guest passthrough mode fails to work regardless of this
> series).  This series is NOT intended for QEMU v4.1.  Thanks,
> 
> Alex


series looks good to me.
pls ping when 4.1 is out and I'll queue it.

> ---
> 
> Alex Williamson (2):
>   pci: Use PCI aliases when determining device IOMMU address space
>   hw/i386: AMD-Vi IVRS DMA alias support
> 
> 
>  hw/i386/acpi-build.c |  127 
> +++---
>  hw/pci/pci.c |   43 -
>  2 files changed, 160 insertions(+), 10 deletions(-)

Re: [Qemu-devel] [for-4.2 PATCH 2/2] hw/i386: AMD-Vi IVRS DMA alias support

[Alex Williamson]

On Mon, 29 Jul 2019 16:26:46 +0800
Peter Xu  wrote:

> On Fri, Jul 26, 2019 at 06:55:53PM -0600, Alex Williamson wrote:
> > When we account for DMA aliases in the PCI address space, we can no
> > longer use a single IVHD entry in the IVRS covering all devices.  We
> > instead need to walk the PCI bus and create alias ranges when we find
> > a conventional bus.  These alias ranges cannot overlap with a "Select
> > All" range (as currently implemented), so we also need to enumerate
> > each device with IVHD entries.
> > 
> > Importantly, the IVHD entries used here include a Device ID, which is
> > simply the PCI BDF (Bus/Device/Function).  The guest firmware is
> > responsible for programming bus numbers, so the final revision of this
> > table depends on the update mechanism (acpi_build_update) to be called
> > after guest PCI enumeration.  
> 
> Ouch... so the ACPI build procedure is after those guest PCI code!
> Could I ask how do you find this? :) It seems much easier for sure
> this way...

I believe this is what MST was referring to with the MCFG update,
acpi_build() is called from both acpi_setup() and acpi_build_update(),
the latter is setup in numerous places to be called via a mechanism
that re-writes the table on certain guest actions.  For instance
acpi_add_rom_blob() passes this function as a callback which turns into
a select_cb in fw_cfg, such that (aiui) the tables are updated before
the guest reads them.  I added some fprintfs in the PCI config write
path to confirm that the update callback occurs after PCI enumeration
for both SeaBIOS and OVMF.  Since we seem to have other dependencies on
this ordering elsewhere, I don't think that the IVRS update is unique
in this regard.
 
> This looks very nice to me already, though I still have got a few
> questions, please see below.
> 
> [...]
> 
> > +if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> > +PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
> > +uint8_t sec = pci_bus_num(sec_bus);
> > +uint8_t sub = dev->config[PCI_SUBORDINATE_BUS];
> > +
> > +if (pci_bus_is_express(sec_bus)) {
> > +/*
> > + * Walk the bus if there are subordinates, otherwise use a 
> > range
> > + * to cover an entire leaf bus.  We could potentially also use 
> > a
> > + * range for traversed buses, but we'd need to take care not to
> > + * create both Select and Range entries covering the same 
> > device.
> > + * This is easier and potentially more compact.
> > + *
> > + * An example bare metal system seems to use Select entries for
> > + * root ports without a slot (ie. built-ins) and Range entries
> > + * when there is a slot.  The same system also only hard-codes
> > + * the alias range for an onboard PCIe-to-PCI bridge, 
> > apparently
> > + * making no effort to support nested bridges.  We attempt to
> > + * be more thorough here.
> > + */
> > +if (sec == sub) { /* leaf bus */
> > +/* "Start of Range" IVHD entry, type 0x3 */
> > +entry = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0)) << 8 | 0x3;
> > +build_append_int_noprefix(table_data, entry, 4);
> > +/* "End of Range" IVHD entry, type 0x4 */
> > +entry = PCI_BUILD_BDF(sub, PCI_DEVFN(31, 7)) << 8 | 0x4;
> > +build_append_int_noprefix(table_data, entry, 4);
> > +} else {
> > +pci_for_each_device(sec_bus, sec, insert_ivhd, table_data);
> > +}
> > +} else {
> > +/*
> > + * If the secondary bus is conventional, then we need to 
> > create an
> > + * Alias range for everything downstream.  The range covers the
> > + * first devfn on the secondary bus to the last devfn on the
> > + * subordinate bus.  The alias target depends on legacy versus
> > + * express bridges, just as in 
> > pci_device_iommu_address_space().
> > + * DeviceIDa vs DeviceIDb as per the AMD IOMMU spec.
> > + */
> > +uint16_t dev_id_a, dev_id_b;
> > +
> > +dev_id_a = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0));
> > +
> > +if (pci_is_express(dev) &&
> > +pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
> > +dev_id_b = dev_id_a;
> > +} else {
> > +dev_id_b = PCI_BUILD_BDF(pci_bus_num(bus), dev->devfn);
> > +}
> > +
> > +/* "Alias Start of Range" IVHD entry, type 0x43, 8 bytes */
> > +build_append_int_noprefix(table_data, dev_id_a << 8 | 0x43, 4);
> > +build_append_int_noprefix(table_data, dev_id_b << 8 | 0x0, 4);
> > +
> > +/* "End of Range" IVHD entry, type 0x4 */
> > +entry = PCI_BUILD_BDF(sub, PCI_DEVFN(31, 7)) << 8 | 0x4;
> > + 

[Qemu-devel] [PATCH for 4.2 v2 3/6] target/mips: Implement Global Invalidate TLB instruction

[Aleksandar Markovic]

From: Yongbok Kim 

Implement Global Invalidate TLB instruction. As QEMU doesn't support
caches and Virtualization, this implementation only cover the GINVT
(Global Invalidate TLB) instruction.

Signed-off-by: Yongbok Kim 
Signed-off-by: Aleksandar Markovic 
---
 disas/mips.c|   2 +
 target/mips/helper.c|  24 --
 target/mips/helper.h|   2 +
 target/mips/internal.h  |   1 +
 target/mips/op_helper.c | 122 ++--
 target/mips/translate.c |  50 +++-
 6 files changed, 179 insertions(+), 22 deletions(-)

diff --git a/disas/mips.c b/disas/mips.c
index dfefe5e..c3a3059 100644
--- a/disas/mips.c
+++ b/disas/mips.c
@@ -1409,6 +1409,8 @@ const struct mips_opcode mips_builtin_opcodes[] =
 {"dvp","t", 0x41600024, 0xffe0, TRAP|WR_t,0, 
I32R6},
 {"evp","",  0x4164, 0x, TRAP, 0, 
I32R6},
 {"evp","t", 0x4164, 0xffe0, TRAP|WR_t,0, 
I32R6},
+{"ginvi",  "v", 0x7c3d, 0xfc1ffcff, TRAP | INSN_TLB,  0, 
I32R6},
+{"ginvt",  "v", 0x7cbd, 0xfc1ffcff, TRAP | INSN_TLB,  0, 
I32R6},
 
 /* MSA */
 {"sll.b",   "+d,+e,+f", 0x780d, 0xffe0003f, WR_VD|RD_VS|RD_VT,  0, MSA},
diff --git a/target/mips/helper.c b/target/mips/helper.c
index a2b6459..6e583d3 100644
--- a/target/mips/helper.c
+++ b/target/mips/helper.c
@@ -70,7 +70,12 @@ int r4k_map_address (CPUMIPSState *env, hwaddr *physical, 
int *prot,
  target_ulong address, int rw, int access_type)
 {
 uint16_t ASID = env->CP0_EntryHi & env->CP0_EntryHi_ASID_mask;
+uint32_t MMID = env->CP0_MemoryMapID;
+bool mi = !!((env->CP0_Config5 >> CP0C5_MI) & 1);
 int i;
+uint32_t tlb_mmid;
+
+MMID = mi ? MMID : (uint32_t) ASID;
 
 for (i = 0; i < env->tlb->tlb_in_use; i++) {
 r4k_tlb_t *tlb = >tlb->mmu.r4k.tlb[i];
@@ -82,8 +87,9 @@ int r4k_map_address (CPUMIPSState *env, hwaddr *physical, int 
*prot,
 tag &= env->SEGMask;
 #endif
 
-/* Check ASID, virtual page number & size */
-if ((tlb->G == 1 || tlb->ASID == ASID) && VPN == tag && !tlb->EHINV) {
+/* Check ASID/MMID, virtual page number & size */
+tlb_mmid = mi ? tlb->MMID : (uint32_t) tlb->ASID;
+if ((tlb->G == 1 || tlb_mmid == MMID) && VPN == tag && !tlb->EHINV) {
 /* TLB match */
 int n = !!(address & mask & ~(mask >> 1));
 /* Check access rights */
@@ -1397,12 +1403,20 @@ void r4k_invalidate_tlb (CPUMIPSState *env, int idx, 
int use_extra)
 target_ulong addr;
 target_ulong end;
 uint16_t ASID = env->CP0_EntryHi & env->CP0_EntryHi_ASID_mask;
+uint32_t MMID = env->CP0_MemoryMapID;
 target_ulong mask;
+bool mi = !!((env->CP0_Config5 >> CP0C5_MI) & 1);
+uint32_t tlb_mmid;
+
+MMID = mi ? MMID : (uint32_t) ASID;
 
 tlb = >tlb->mmu.r4k.tlb[idx];
-/* The qemu TLB is flushed when the ASID changes, so no need to
-   flush these entries again.  */
-if (tlb->G == 0 && tlb->ASID != ASID) {
+/*
+ * The qemu TLB is flushed when the ASID/MMID changes, so no need to
+ * flush these entries again.
+ */
+tlb_mmid = mi ? tlb->MMID : (uint32_t) tlb->ASID;
+if (tlb->G == 0 && tlb_mmid != MMID) {
 return;
 }
 
diff --git a/target/mips/helper.h b/target/mips/helper.h
index aad0951..c7d35bd 100644
--- a/target/mips/helper.h
+++ b/target/mips/helper.h
@@ -120,6 +120,7 @@ DEF_HELPER_2(mtc0_tcschefback, void, env, tl)
 DEF_HELPER_2(mttc0_tcschefback, void, env, tl)
 DEF_HELPER_2(mtc0_entrylo1, void, env, tl)
 DEF_HELPER_2(mtc0_context, void, env, tl)
+DEF_HELPER_2(mtc0_memorymapid, void, env, tl)
 DEF_HELPER_2(mtc0_pagemask, void, env, tl)
 DEF_HELPER_2(mtc0_pagegrain, void, env, tl)
 DEF_HELPER_2(mtc0_segctl0, void, env, tl)
@@ -376,6 +377,7 @@ DEF_HELPER_1(ei, tl, env)
 DEF_HELPER_1(eret, void, env)
 DEF_HELPER_1(eretnc, void, env)
 DEF_HELPER_1(deret, void, env)
+DEF_HELPER_3(ginvt, void, env, tl, i32)
 #endif /* !CONFIG_USER_ONLY */
 DEF_HELPER_1(rdhwr_cpunum, tl, env)
 DEF_HELPER_1(rdhwr_synci_step, tl, env)
diff --git a/target/mips/internal.h b/target/mips/internal.h
index f6d0d7a..d9216fb 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -92,6 +92,7 @@ struct r4k_tlb_t {
 target_ulong VPN;
 uint32_t PageMask;
 uint16_t ASID;
+uint32_t MMID;
 unsigned int G:1;
 unsigned int C0:3;
 unsigned int C1:3;
diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 52853e9..9b3bf4b 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -1409,6 +1409,17 @@ void helper_mtc0_context(CPUMIPSState *env, target_ulong 
arg1)
 env->CP0_Context = (env->CP0_Context & 0x007F) | (arg1 & ~0x007F);
 }
 
+void helper_mtc0_memorymapid(CPUMIPSState *env, target_ulong arg1)
+{
+int32_t old;
+old = env->CP0_MemoryMapID;
+env->CP0_MemoryMapID = (int32_t) arg1;
+

[Qemu-devel] [PATCH for 4.2 v2 1/6] target/mips: Add support for DSPRAM

[Aleksandar Markovic]

From: Yongbok Kim 

The optional Data Scratch Pad RAM (DSPRAM) block provides a general scratch pad 
RAM
used for temporary storage of data. The DSPRAM provides a connection to on-chip
memory or memory-mapped registers, which are accessed in parallel with the L1 
data
cache to minimize access latency

Signed-off-by: Yongbok Kim 
Signed-off-by: Aleksandar Markovic 
---
 default-configs/mips-softmmu-common.mak |   1 +
 hw/mips/cps.c   |  28 +-
 hw/misc/Makefile.objs   |   1 +
 hw/misc/mips_dspram.c   | 153 
 include/hw/mips/cps.h   |   2 +
 include/hw/misc/mips_dspram.h   |  46 ++
 target/mips/cpu.h   |   9 +-
 target/mips/internal.h  |   3 +-
 target/mips/op_helper.c |  14 +++
 target/mips/translate.c |   8 ++
 target/mips/translate_init.inc.c|   2 +
 11 files changed, 262 insertions(+), 5 deletions(-)
 create mode 100644 hw/misc/mips_dspram.c
 create mode 100644 include/hw/misc/mips_dspram.h

diff --git a/default-configs/mips-softmmu-common.mak 
b/default-configs/mips-softmmu-common.mak
index da29c6c..99b8df6 100644
--- a/default-configs/mips-softmmu-common.mak
+++ b/default-configs/mips-softmmu-common.mak
@@ -33,6 +33,7 @@ CONFIG_MC146818RTC=y
 CONFIG_EMPTY_SLOT=y
 CONFIG_MIPS_CPS=y
 CONFIG_MIPS_ITU=y
+CONFIG_MIPS_DSPRAM=y
 CONFIG_R4K=y
 CONFIG_MALTA=y
 CONFIG_PCNET_PCI=y
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 0d459c4..76d2a93 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -91,7 +91,8 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
 
 cpu = MIPS_CPU(first_cpu);
 env = >env;
-saar_present = (bool)env->saarp;
+saar_present = env->saarp;
+bool dspram_present = env->dspramp;
 
 /* Inter-Thread Communication Unit */
 if (itu_present) {
@@ -102,7 +103,8 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
 object_property_set_bool(OBJECT(>itu), saar_present, "saar-present",
  );
 if (saar_present) {
-qdev_prop_set_ptr(DEVICE(>itu), "saar", (void *)>CP0_SAAR);
+qdev_prop_set_ptr(DEVICE(>itu), "saar",
+  (void *) >CP0_SAAR[0]);
 }
 object_property_set_bool(OBJECT(>itu), true, "realized", );
 if (err != NULL) {
@@ -113,6 +115,28 @@ static void mips_cps_realize(DeviceState *dev, Error 
**errp)
 memory_region_add_subregion(>container, 0,
sysbus_mmio_get_region(SYS_BUS_DEVICE(>itu), 0));
 }
+env->dspram = g_new0(MIPSDSPRAMState, 1);
+
+/* Data Scratch Pad RAM */
+if (dspram_present) {
+if (!saar_present) {
+error_report("%s: DSPRAM requires SAAR registers", __func__);
+return;
+}
+object_initialize(>dspram, sizeof(MIPSDSPRAMState),
+  TYPE_MIPS_DSPRAM);
+qdev_set_parent_bus(DEVICE(>dspram), sysbus_get_default());
+qdev_prop_set_ptr(DEVICE(>dspram), "saar",
+  >CP0_SAAR[1]);
+object_property_set_bool(OBJECT(>dspram), true, "realized", );
+if (err != NULL) {
+error_report("%s: DSPRAM initialisation failed", __func__);
+error_propagate(errp, err);
+return;
+}
+memory_region_add_subregion(>container, 0,
+sysbus_mmio_get_region(SYS_BUS_DEVICE(>dspram), 0));
+}
 
 /* Cluster Power Controller */
 sysbus_init_child_obj(OBJECT(dev), "cpc", >cpc, sizeof(s->cpc),
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index e9aab51..5fcb4db 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -60,6 +60,7 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
 obj-$(CONFIG_MIPS_CPS) += mips_cmgcr.o
 obj-$(CONFIG_MIPS_CPS) += mips_cpc.o
 obj-$(CONFIG_MIPS_ITU) += mips_itu.o
+obj-$(CONFIG_MIPS_DSPRAM) += mips_dspram.o
 obj-$(CONFIG_MPS2_FPGAIO) += mps2-fpgaio.o
 obj-$(CONFIG_MPS2_SCC) += mps2-scc.o
 
diff --git a/hw/misc/mips_dspram.c b/hw/misc/mips_dspram.c
new file mode 100644
index 000..9bc155b
--- /dev/null
+++ b/hw/misc/mips_dspram.c
@@ -0,0 +1,153 @@
+/*
+ * Data Scratch Pad RAM
+ *
+ * Copyright (c) 2017 Imagination Technologies
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if 

[Qemu-devel] [PATCH for 4.2 v2 4/6] target/mips: Add emulation of CRC32 instructions

[Aleksandar Markovic]

From: Yongbok Kim 

Add emulation of MIPS' CRC32 (Cyclic Redundancy Check) instructions.
Reuse zlib crc32() and Linux crc32c(). Note that, at the time being,
there is no MIPS CPU that supports CRC32 instructions (they are an
optional part of MIPS64/32 R6 anf nanoMIPS ISAs).

Signed-off-by: Yongbok Kim 
Signed-off-by: Aleksandar Markovic 
---
 disas/mips.c|  8 
 target/mips/helper.h|  2 ++
 target/mips/op_helper.c | 22 ++
 target/mips/translate.c | 41 +
 4 files changed, 73 insertions(+)

diff --git a/disas/mips.c b/disas/mips.c
index c3a3059..b9a5204 100644
--- a/disas/mips.c
+++ b/disas/mips.c
@@ -1411,6 +1411,14 @@ const struct mips_opcode mips_builtin_opcodes[] =
 {"evp","t", 0x4164, 0xffe0, TRAP|WR_t,0, 
I32R6},
 {"ginvi",  "v", 0x7c3d, 0xfc1ffcff, TRAP | INSN_TLB,  0, 
I32R6},
 {"ginvt",  "v", 0x7cbd, 0xfc1ffcff, TRAP | INSN_TLB,  0, 
I32R6},
+{"crc32b", "t,v,t", 0x7c0f, 0xfc00ff3f, WR_d | RD_s | RD_t,   0, 
I32R6},
+{"crc32h", "t,v,t", 0x7c4f, 0xfc00ff3f, WR_d | RD_s | RD_t,   0, 
I32R6},
+{"crc32w", "t,v,t", 0x7c8f, 0xfc00ff3f, WR_d | RD_s | RD_t,   0, 
I32R6},
+{"crc32d", "t,v,t", 0x7ccf, 0xfc00ff3f, WR_d | RD_s | RD_t,   0, 
I64R6},
+{"crc32cb","t,v,t", 0x7c00010f, 0xfc00ff3f, WR_d | RD_s | RD_t,   0, 
I32R6},
+{"crc32ch","t,v,t", 0x7c00014f, 0xfc00ff3f, WR_d | RD_s | RD_t,   0, 
I32R6},
+{"crc32cw","t,v,t", 0x7c00018f, 0xfc00ff3f, WR_d | RD_s | RD_t,   0, 
I32R6},
+{"crc32cd","t,v,t", 0x7c0001cf, 0xfc00ff3f, WR_d | RD_s | RD_t,   0, 
I64R6},
 
 /* MSA */
 {"sll.b",   "+d,+e,+f", 0x780d, 0xffe0003f, WR_VD|RD_VS|RD_VT,  0, MSA},
diff --git a/target/mips/helper.h b/target/mips/helper.h
index c7d35bd..0e61043 100644
--- a/target/mips/helper.h
+++ b/target/mips/helper.h
@@ -40,6 +40,8 @@ DEF_HELPER_FLAGS_1(bitswap, TCG_CALL_NO_RWG_SE, tl, tl)
 DEF_HELPER_FLAGS_1(dbitswap, TCG_CALL_NO_RWG_SE, tl, tl)
 #endif
 
+DEF_HELPER_3(crc32, tl, tl, tl, i32)
+DEF_HELPER_3(crc32c, tl, tl, tl, i32)
 DEF_HELPER_FLAGS_4(rotx, TCG_CALL_NO_RWG_SE, tl, tl, i32, i32, i32)
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 9b3bf4b..93f5ee5 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -25,6 +25,8 @@
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "sysemu/kvm.h"
+#include "qemu/crc32c.h"
+#include 
 
 /*/
 /* Exceptions processing helpers */
@@ -343,6 +345,26 @@ target_ulong helper_rotx(target_ulong rs, uint32_t shift, 
uint32_t shiftx,
 return (int64_t)(int32_t)(uint32_t)tmp5;
 }
 
+/* these crc32 functions are based on target/arm/helper-a64.c */
+target_ulong helper_crc32(target_ulong val, target_ulong m, uint32_t sz)
+{
+uint8_t buf[8];
+target_ulong mask = ((sz * 8) == 64) ? -1ULL : ((1ULL << (sz * 8)) - 1);
+
+m &= mask;
+stq_le_p(buf, m);
+return (int32_t) (crc32(val ^ 0x, buf, sz) ^ 0x);
+}
+
+target_ulong helper_crc32c(target_ulong val, target_ulong m, uint32_t sz)
+{
+uint8_t buf[8];
+target_ulong mask = ((sz * 8) == 64) ? -1ULL : ((1ULL << (sz * 8)) - 1);
+m &= mask;
+stq_le_p(buf, m);
+return (int32_t) (crc32c(val, buf, sz) ^ 0x);
+}
+
 #ifndef CONFIG_USER_ONLY
 
 static inline hwaddr do_translate_address(CPUMIPSState *env,
diff --git a/target/mips/translate.c b/target/mips/translate.c
index c612787..8c578d8 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -452,6 +452,7 @@ enum {
 OPC_LWE= 0x2F | OPC_SPECIAL3,
 
 /* R6 */
+OPC_CRC32  = 0x0F | OPC_SPECIAL3,
 R6_OPC_PREF= 0x35 | OPC_SPECIAL3,
 R6_OPC_CACHE   = 0x25 | OPC_SPECIAL3,
 R6_OPC_LL  = 0x36 | OPC_SPECIAL3,
@@ -2550,6 +2551,7 @@ typedef struct DisasContext {
 bool saar;
 int gi;
 bool mi;
+bool crcp;
 } DisasContext;
 
 #define DISAS_STOP   DISAS_TARGET_0
@@ -27041,6 +27043,33 @@ static void decode_opc_special2_legacy(CPUMIPSState 
*env, DisasContext *ctx)
 }
 }
 
+static void gen_crc32(DisasContext *ctx, int rd, int rs, int rt, int sz,
+  int crc32c)
+{
+TCGv t0;
+TCGv t1;
+TCGv_i32 tsz = tcg_const_i32(1 << sz);
+if (rd == 0) {
+/* Treat as NOP. */
+return;
+}
+t0 = tcg_temp_new();
+t1 = tcg_temp_new();
+
+gen_load_gpr(t0, rt);
+gen_load_gpr(t1, rs);
+
+if (crc32c) {
+gen_helper_crc32c(cpu_gpr[rd], t0, t1, tsz);
+} else {
+gen_helper_crc32(cpu_gpr[rd], t0, t1, tsz);
+}
+
+tcg_temp_free(t0);
+tcg_temp_free(t1);
+tcg_temp_free_i32(tsz);
+}
+
 static void decode_opc_special3_r6(CPUMIPSState *env, DisasContext *ctx)
 {
 int rs, rt, rd, sa;
@@ -27055,6 +27084,17 @@ static void decode_opc_special3_r6(CPUMIPSState 

[Qemu-devel] [PATCH for 4.2 v2 5/6] tests/tcg: target/mips: Add optional printing of more detailed failure info

[Aleksandar Markovic]

From: Aleksandar Markovic 

There is a need for printing input and output data for failure cases,
for debugging purpose. This is achieved by this patch, and only if a
preprocessor constant is manually set to 1. (Assumption is that the
need for such printout is relatively rare.)

Signed-off-by: Aleksandar Markovic 
---
 tests/tcg/mips/include/test_utils_128.h | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/tests/tcg/mips/include/test_utils_128.h 
b/tests/tcg/mips/include/test_utils_128.h
index 2fea610..0dd3868 100644
--- a/tests/tcg/mips/include/test_utils_128.h
+++ b/tests/tcg/mips/include/test_utils_128.h
@@ -27,7 +27,8 @@
 #include 
 #include 
 
-#define PRINT_RESULTS 0
+#define PRINT_RESULTS0
+#define PRINT_FAILURES   0
 
 
 static inline int32_t check_results_128(const char *isa_ase_name,
@@ -65,6 +66,26 @@ static inline int32_t check_results_128(const char 
*isa_ase_name,
 (b128_result[2 * i + 1] == b128_expect[2 * i + 1])) {
 pass_count++;
 } else {
+#if PRINT_FAILURES
+uint32_t ii;
+uint64_t a, b;
+
+printf("\n");
+
+printf("FAILURE for test case %d!\n", i);
+
+memcpy(, (b128_expect + 2 * i), 8);
+memcpy(, (b128_expect + 2 * i + 1), 8);
+printf("Expected result : { 0x%016llxULL, 0x%016llxULL, },\n",
+   a, b);
+
+memcpy(, (b128_result + 2 * i), 8);
+memcpy(, (b128_result + 2 * i + 1), 8);
+printf("Actual result   : { 0x%016llxULL, 0x%016llxULL, },\n",
+   a, b);
+
+printf("\n");
+#endif
 fail_count++;
 }
 }
-- 
2.7.4

[Qemu-devel] [PATCH for 4.2 v2 2/6] target/mips: Extend WatchHi registers

[Aleksandar Markovic]

From: Yongbok Kim 

WatchHi is extended by the field MemoryMapID with the GINVT instruction.
The field is accessible by MTHC0/MFHC0 in 32-bit architectures and DMTC0/
DMFC0 in 64-bit architectures.

Signed-off-by: Yongbok Kim 
Signed-off-by: Aleksandar Markovic 
---
 target/mips/cpu.h   |  2 +-
 target/mips/helper.h|  3 +++
 target/mips/machine.c   |  2 +-
 target/mips/op_helper.c | 23 +--
 target/mips/translate.c | 40 +++-
 5 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 90a2ed8..6406ba8 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -898,7 +898,7 @@ struct CPUMIPSState {
 /*
  * CP0 Register 19
  */
-int32_t CP0_WatchHi[8];
+uint64_t CP0_WatchHi[8];
 #define CP0WH_ASID 16
 /*
  * CP0 Register 20
diff --git a/target/mips/helper.h b/target/mips/helper.h
index 51f0e1c..aad0951 100644
--- a/target/mips/helper.h
+++ b/target/mips/helper.h
@@ -78,6 +78,7 @@ DEF_HELPER_1(mfc0_maar, tl, env)
 DEF_HELPER_1(mfhc0_maar, tl, env)
 DEF_HELPER_2(mfc0_watchlo, tl, env, i32)
 DEF_HELPER_2(mfc0_watchhi, tl, env, i32)
+DEF_HELPER_2(mfhc0_watchhi, tl, env, i32)
 DEF_HELPER_1(mfc0_debug, tl, env)
 DEF_HELPER_1(mftc0_debug, tl, env)
 #ifdef TARGET_MIPS64
@@ -89,6 +90,7 @@ DEF_HELPER_1(dmfc0_tcschefback, tl, env)
 DEF_HELPER_1(dmfc0_lladdr, tl, env)
 DEF_HELPER_1(dmfc0_maar, tl, env)
 DEF_HELPER_2(dmfc0_watchlo, tl, env, i32)
+DEF_HELPER_2(dmfc0_watchhi, tl, env, i32)
 DEF_HELPER_1(dmfc0_saar, tl, env)
 #endif /* TARGET_MIPS64 */
 
@@ -159,6 +161,7 @@ DEF_HELPER_2(mthc0_maar, void, env, tl)
 DEF_HELPER_2(mtc0_maari, void, env, tl)
 DEF_HELPER_3(mtc0_watchlo, void, env, tl, i32)
 DEF_HELPER_3(mtc0_watchhi, void, env, tl, i32)
+DEF_HELPER_3(mthc0_watchhi, void, env, tl, i32)
 DEF_HELPER_2(mtc0_xcontext, void, env, tl)
 DEF_HELPER_2(mtc0_framemask, void, env, tl)
 DEF_HELPER_2(mtc0_debug, void, env, tl)
diff --git a/target/mips/machine.c b/target/mips/machine.c
index eb2d970..ff8cb98 100644
--- a/target/mips/machine.c
+++ b/target/mips/machine.c
@@ -297,7 +297,7 @@ const VMStateDescription vmstate_mips_cpu = {
 VMSTATE_INT32(env.CP0_MAARI, MIPSCPU),
 VMSTATE_UINTTL(env.lladdr, MIPSCPU),
 VMSTATE_UINTTL_ARRAY(env.CP0_WatchLo, MIPSCPU, 8),
-VMSTATE_INT32_ARRAY(env.CP0_WatchHi, MIPSCPU, 8),
+VMSTATE_UINT64_ARRAY(env.CP0_WatchHi, MIPSCPU, 8),
 VMSTATE_UINTTL(env.CP0_XContext, MIPSCPU),
 VMSTATE_INT32(env.CP0_Framemask, MIPSCPU),
 VMSTATE_INT32(env.CP0_Debug, MIPSCPU),
diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index f7b8c4d..52853e9 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -979,7 +979,12 @@ target_ulong helper_mfc0_watchlo(CPUMIPSState *env, 
uint32_t sel)
 
 target_ulong helper_mfc0_watchhi(CPUMIPSState *env, uint32_t sel)
 {
-return env->CP0_WatchHi[sel];
+return (int32_t) env->CP0_WatchHi[sel];
+}
+
+target_ulong helper_mfhc0_watchhi(CPUMIPSState *env, uint32_t sel)
+{
+return env->CP0_WatchHi[sel] >> 32;
 }
 
 target_ulong helper_mfc0_debug(CPUMIPSState *env)
@@ -1055,6 +1060,11 @@ target_ulong helper_dmfc0_saar(CPUMIPSState *env)
 }
 return 0;
 }
+
+target_ulong helper_dmfc0_watchhi(CPUMIPSState *env, uint32_t sel)
+{
+return env->CP0_WatchHi[sel];
+}
 #endif /* TARGET_MIPS64 */
 
 void helper_mtc0_index(CPUMIPSState *env, target_ulong arg1)
@@ -1892,11 +1902,20 @@ void helper_mtc0_watchlo(CPUMIPSState *env, 
target_ulong arg1, uint32_t sel)
 
 void helper_mtc0_watchhi(CPUMIPSState *env, target_ulong arg1, uint32_t sel)
 {
-int mask = 0x4FF8 | (env->CP0_EntryHi_ASID_mask << CP0WH_ASID);
+uint64_t mask = 0x4FF8 | (env->CP0_EntryHi_ASID_mask << CP0WH_ASID);
+if ((env->CP0_Config5 >> CP0C5_MI) & 1) {
+mask |= 0xULL; /* MMID */
+}
 env->CP0_WatchHi[sel] = arg1 & mask;
 env->CP0_WatchHi[sel] &= ~(env->CP0_WatchHi[sel] & arg1 & 0x7);
 }
 
+void helper_mthc0_watchhi(CPUMIPSState *env, target_ulong arg1, uint32_t sel)
+{
+env->CP0_WatchHi[sel] = ((uint64_t) (arg1) << 32) |
+(env->CP0_WatchHi[sel] & 0xULL);
+}
+
 void helper_mtc0_xcontext(CPUMIPSState *env, target_ulong arg1)
 {
 target_ulong mask = (1ULL << (env->SEGBITS - 7)) - 1;
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 4ebeabe..778461c 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -6680,6 +6680,25 @@ static void gen_mfhc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
 goto cp0_unimplemented;
 }
 break;
+case CP0_REGISTER_19:
+switch (sel) {
+case 0:
+case 1:
+case 2:
+case 3:
+case 4:
+case 5:
+case 6:
+case 7:
+/* upper 32 bits are only available when Config5MI != 0 */
+CP0_CHECK(ctx->mi);
+

[Qemu-devel] [PATCH for 4.2 v2 0/6] target/mips: Misc patches for 4.2

[Aleksandar Markovic]

From: Aleksandar Markovic 

This series includes misc MIPS patches intended to be integrated after
4.1 release.

v1->v2:

  - fixed checkpatch warnings
  - added four new patches on various topics

Aleksandar Markovic (2):
  tests/tcg: target/mips: Add optional printing of more detailed failure
info
  tests/tcg: target/mips: Fix target configurations for MSA tests

Yongbok Kim (4):
  target/mips: Add support for DSPRAM
  target/mips: Extend WatchHi registers
  target/mips: Implement Global Invalidate TLB instruction
  target/mips: Add emulation of CRC32 instructions

 default-configs/mips-softmmu-common.mak|   1 +
 disas/mips.c   |  10 +
 hw/mips/cps.c  |  28 +-
 hw/misc/Makefile.objs  |   1 +
 hw/misc/mips_dspram.c  | 153 
 include/hw/mips/cps.h  |   2 +
 include/hw/misc/mips_dspram.h  |  46 ++
 target/mips/cpu.h  |  11 +-
 target/mips/helper.c   |  24 +-
 target/mips/helper.h   |   7 +
 target/mips/internal.h |   4 +-
 target/mips/machine.c  |   2 +-
 target/mips/op_helper.c| 181 +++-
 target/mips/translate.c| 139 +++-
 target/mips/translate_init.inc.c   |   2 +
 tests/tcg/mips/include/test_utils_128.h|  23 +-
 .../mips/user/ase/msa/test_msa_compile_32r5eb.sh   | 917 +
 .../mips/user/ase/msa/test_msa_compile_32r5el.sh   | 917 +
 .../mips/user/ase/msa/test_msa_compile_32r6eb.sh   | 643 ---
 .../mips/user/ase/msa/test_msa_compile_32r6el.sh   | 643 ---
 tests/tcg/mips/user/ase/msa/test_msa_run_32r5eb.sh | 371 +
 tests/tcg/mips/user/ase/msa/test_msa_run_32r5el.sh | 371 +
 tests/tcg/mips/user/ase/msa/test_msa_run_32r6eb.sh | 371 -
 tests/tcg/mips/user/ase/msa/test_msa_run_32r6el.sh | 371 -
 24 files changed, 3177 insertions(+), 2061 deletions(-)
 create mode 100644 hw/misc/mips_dspram.c
 create mode 100644 include/hw/misc/mips_dspram.h
 create mode 100755 tests/tcg/mips/user/ase/msa/test_msa_compile_32r5eb.sh
 create mode 100755 tests/tcg/mips/user/ase/msa/test_msa_compile_32r5el.sh
 delete mode 100755 tests/tcg/mips/user/ase/msa/test_msa_compile_32r6eb.sh
 delete mode 100755 tests/tcg/mips/user/ase/msa/test_msa_compile_32r6el.sh
 create mode 100755 tests/tcg/mips/user/ase/msa/test_msa_run_32r5eb.sh
 create mode 100755 tests/tcg/mips/user/ase/msa/test_msa_run_32r5el.sh
 delete mode 100644 tests/tcg/mips/user/ase/msa/test_msa_run_32r6eb.sh
 delete mode 100755 tests/tcg/mips/user/ase/msa/test_msa_run_32r6el.sh

-- 
2.7.4

[Qemu-devel] [Bug 1838228] Re: Slirp Broadcast traffic

[Chris Koch]

Thanks. Opened https://gitlab.freedesktop.org/slirp/libslirp/issues/9.

** Bug watch added: gitlab.freedesktop.org/slirp/libslirp/issues #9
   https://gitlab.freedesktop.org/slirp/libslirp/issues/9

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1838228

Title:
  Slirp Broadcast traffic

Status in QEMU:
  New

Bug description:
  Hi all,

  Version: QEMU emulator version 3.1.0 (Debian 1:3.1+dfsg-7+build1)

  I'm running some DHCP traffic to a *custom* DHCP server with user-mode
  networking in QEMU. I'm using port 30067 for the server, so this does
  not conflict with the built-in DHCP server.

  DHCP broadcasts to and from the server, and I'm observing issues with
  both sending and receiving packets.

  Firstly, from the VM, a packet sent to IPv4 x.x.x.2:30067 (gateway)
  makes it to the server, but a packet sent to 255.255.255.255 does not.
  I'd suspect that Slirp has to support sending to the broadcast IP
  address? Or is this something I can turn on with a configuration
  option? (My QEMU version too old?)

  Secondly, the source address in a DHCP IPv4 packet must be 0.0.0.0 (by
  RFC). That means that any return packet will have 0.0.0.0 swapped in
  as its destination address. However, that packet doesn't make it into
  the VM at all. I know that if you deliver this packet to Linux, a raw
  socket will spit it back out. The packets' destination address should
  not prevent the packet from being delivered to the right VM, since
  Slirp (should?) know exactly which VM the session belongs to. (It's a
  proxy, not a router.)

  WDYT? Did I miss some configuration options or use too old a version?

  Thanks,
  Chris

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1838228/+subscriptions

Re: [Qemu-devel] [PATCH RFC 1/2] memory: make MemoryRegion alias migratable

[Dr. David Alan Gilbert]

* Igor Mammedov (imamm...@redhat.com) wrote:
> use qemu_ram_alloc_from_ptr() to create aliased RAMBlock
> to the part of original memory region.
> 
> Signed-off-by: Igor Mammedov 
> ---
>  exec.c   | 7 ---
>  memory.c | 5 +
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 3e78de3b8f..daef0cd54f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2313,7 +2313,7 @@ static void ram_block_add(RAMBlock *new_block, Error 
> **errp, bool shared)
>  new_block->used_length,
>  DIRTY_CLIENTS_ALL);
>  
> -if (new_block->host) {
> +if (new_block->host && !new_block->mr->alias) {
>  qemu_ram_setup_dump(new_block->host, new_block->max_length);
>  qemu_madvise(new_block->host, new_block->max_length, 
> QEMU_MADV_HUGEPAGE);
>  /* MADV_DONTFORK is also needed by KVM in absence of synchronous MMU 
> */
> @@ -2671,7 +2671,8 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool 
> round_offset,
>  
>  rcu_read_lock();
>  block = atomic_rcu_read(_list.mru_block);
> -if (block && block->host && host - block->host < block->max_length) {
> +if (block && !block->mr->alias && block->host &&
> +host - block->host < block->max_length) {
>  goto found;
>  }
>  
> @@ -2680,7 +2681,7 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool 
> round_offset,
>  if (block->host == NULL) {
>  continue;
>  }
> -if (host - block->host < block->max_length) {
> +if (!block->mr->alias && host - block->host < block->max_length) {
>  goto found;
>  }
>  }
> diff --git a/memory.c b/memory.c
> index 5d8c9a9234..d710c17a26 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1678,6 +1678,11 @@ void memory_region_init_alias(MemoryRegion *mr,
>  memory_region_init(mr, owner, name, size);
>  mr->alias = orig;
>  mr->alias_offset = offset;
> +if (orig->ram_block && size) {
> +mr->ram_block = qemu_ram_alloc_from_ptr(size,
> +orig->ram_block->host + 
> offset,
> +mr, _fatal);
> +}
>  }

Doesn't this cause new memory regions to be created in other existing
machines, e.g. x86's mem-smram, or the various PCI vga hacks?

Dave
>  
>  void memory_region_init_rom_nomigrate(MemoryRegion *mr,
> -- 
> 2.18.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions

[Chih-Min Chao]

On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis 
wrote:

> Signed-off-by: Alistair Francis 
> ---
>  hw/riscv/sifive_plic.c | 12 
>  include/hw/riscv/sifive_plic.h |  3 ---
>  2 files changed, 15 deletions(-)
>
> diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
> index 0950e89e15..864a1bed42 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -161,18 +161,6 @@ static void sifive_plic_update(SiFivePLICState *plic)
>  }
>  }
>
> -void sifive_plic_raise_irq(SiFivePLICState *plic, uint32_t irq)
> -{
> -sifive_plic_set_pending(plic, irq, true);
> -sifive_plic_update(plic);
> -}
> -
> -void sifive_plic_lower_irq(SiFivePLICState *plic, uint32_t irq)
> -{
> -sifive_plic_set_pending(plic, irq, false);
> -sifive_plic_update(plic);
> -}
> -
>  static uint32_t sifive_plic_claim(SiFivePLICState *plic, uint32_t addrid)
>  {
>  int i, j;
> diff --git a/include/hw/riscv/sifive_plic.h
> b/include/hw/riscv/sifive_plic.h
> index ce8907f6aa..3b8a623919 100644
> --- a/include/hw/riscv/sifive_plic.h
> +++ b/include/hw/riscv/sifive_plic.h
> @@ -69,9 +69,6 @@ typedef struct SiFivePLICState {
>  uint32_t aperture_size;
>  } SiFivePLICState;
>
> -void sifive_plic_raise_irq(SiFivePLICState *plic, uint32_t irq);
> -void sifive_plic_lower_irq(SiFivePLICState *plic, uint32_t irq);
> -
>  DeviceState *sifive_plic_create(hwaddr addr, char *hart_config,
>  uint32_t num_sources, uint32_t num_priorities,
>  uint32_t priority_base, uint32_t pending_base,
> --
> 2.22.0
>
>
Reviewed-by: Chih-Min Chao 

Re: [Qemu-devel] [PATCH for-4.1] scsi-cd: Fix inserting read-only media in empty drive

[Philippe Mathieu-Daudé]

On 7/29/19 6:42 PM, Kevin Wolf wrote:
> scsi-disks decides whether it has a read-only device by looking at
> whether the BlockBackend specified as drive=... is read-only. In the
> case of an anonymous BlockBackend (with a node name specified in
> drive=...), this is the read-only flag of the attached node. In the case
> of an empty anonymous BlockBackend, it's always read-write because
> nothing prevented it from being read-write.
> 
> This is a problem because scsi-cd would take write permissions on the
> anonymous BlockBackend of an empty drive created without a drive=...
> option. Using blockdev-insert-medium with a read-only node fails then
> with the error message "Block node is read-only".
> 
> Fix scsi_realize() so that scsi-cd devices always take read-only
> permissions on their BlockBackend instead.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1733920
> Signed-off-by: Kevin Wolf 
> ---
>  hw/scsi/scsi-disk.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 8e95e3e38d..af3e622dc5 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2318,6 +2318,7 @@ static void 
> scsi_disk_unit_attention_reported(SCSIDevice *dev)
>  static void scsi_realize(SCSIDevice *dev, Error **errp)
>  {
>  SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
> +bool read_only;
>  
>  if (!s->qdev.conf.blk) {
>  error_setg(errp, "drive property not set");
> @@ -2351,8 +2352,13 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
>  return;
>  }
>  }
> -if (!blkconf_apply_backend_options(>conf,
> -   blk_is_read_only(s->qdev.conf.blk),
> +
> +read_only = blk_is_read_only(s->qdev.conf.blk);
> +if (dev->type == TYPE_ROM) {
> +read_only = true;
> +}
> +
> +if (!blkconf_apply_backend_options(>conf, read_only,
> dev->type == TYPE_DISK, errp)) {
>  return;
>  }
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [Qemu-devel] [PULL 0/1] x86 queue for 4.1

[Peter Maydell]

On Mon, 29 Jul 2019 at 17:10, Eduardo Habkost  wrote:
>
> The following changes since commit 893dc8300c80e3dc32f31e968cf7aa0904da50c3:
>
>   Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into 
> staging (2019-07-29 12:04:53 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/ehabkost/qemu.git tags/x86-next-pull-request
>
> for you to fetch changes up to ff656fcd338a70c4d9783a800733c4ab3806e5b0:
>
>   i386: Fix Snowridge CPU model name and features (2019-07-29 13:08:02 -0300)
>
> 
> x86 queue for 4.1
>
> * Rename and fix SnowRidge CPU model (Paul Lai)
>
> 
>
> Paul Lai (1):
>   i386: Fix Snowridge CPU model name and features
>
>  target/i386/cpu.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> --
> 2.18.0.rc1.1.g3f1ff2140


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1
for any user-visible changes.

-- PMM

Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[Chih-Min Chao]

On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis 
wrote:

> Let's creaate a function that tests if floating point support is
> enabled. We can then protect all floating point operations based on if
> they are enabled.
>
> This patch so far doesn't change anything, it's just preparing for the
> Hypervisor support for floating point operations.
>
> Signed-off-by: Alistair Francis 
> ---
>  target/riscv/cpu.h|  6 +-
>  target/riscv/cpu_helper.c | 10 ++
>  target/riscv/csr.c| 19 ++-
>  3 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0adb307f32..2dc9b17678 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -255,6 +255,7 @@ void riscv_cpu_do_interrupt(CPUState *cpu);
>  int riscv_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>  int riscv_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>  bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
> +bool riscv_cpu_fp_enabled(CPURISCVState *env);
>  int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch);
>  hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> @@ -298,7 +299,10 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState
> *env, target_ulong *pc,
>  #ifdef CONFIG_USER_ONLY
>  *flags = TB_FLAGS_MSTATUS_FS;
>  #else
> -*flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
> +*flags = cpu_mmu_index(env, 0);
> +if (riscv_cpu_fp_enabled(env)) {
> +*flags |= env->mstatus & MSTATUS_FS;
> +}
>  #endif
>  }
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index f027be7f16..225e407cff 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -71,6 +71,16 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
>
>  #if !defined(CONFIG_USER_ONLY)
>
> +/* Return true is floating point support is currently enabled */
> +bool riscv_cpu_fp_enabled(CPURISCVState *env)
> +{
> +if (env->mstatus & MSTATUS_FS) {
> +return true;
> +}
> +
> +return false;
> +}
> +
>  int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint32_t interrupts)
>  {
>  CPURISCVState *env = >env;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index af3b762c8b..7b73b73cf7 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -46,7 +46,7 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations
> *ops)
>  static int fs(CPURISCVState *env, int csrno)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> -if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
> +if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>  return -1;
>  }
>  #endif
> @@ -108,7 +108,7 @@ static int pmp(CPURISCVState *env, int csrno)
>  static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> -if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
> +if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>  return -1;
>  }
>  #endif
> @@ -119,7 +119,7 @@ static int read_fflags(CPURISCVState *env, int csrno,
> target_ulong *val)
>  static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> -if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
> +if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>  return -1;
>  }
>  env->mstatus |= MSTATUS_FS;
> @@ -131,7 +131,7 @@ static int write_fflags(CPURISCVState *env, int csrno,
> target_ulong val)
>  static int read_frm(CPURISCVState *env, int csrno, target_ulong *val)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> -if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
> +if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>  return -1;
>  }
>  #endif
> @@ -142,7 +142,7 @@ static int read_frm(CPURISCVState *env, int csrno,
> target_ulong *val)
>  static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> -if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
> +if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>  return -1;
>  }
>  env->mstatus |= MSTATUS_FS;
> @@ -154,7 +154,7 @@ static int write_frm(CPURISCVState *env, int csrno,
> target_ulong val)
>  static int read_fcsr(CPURISCVState *env, int csrno, target_ulong *val)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> -if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
> +if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>  return -1;
>  }
>  #endif
> @@ -166,7 +166,7 @@ static int read_fcsr(CPURISCVState *env, int csrno,
> target_ulong *val)
>  static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> -if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
> +if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
>  return 

Re: [Qemu-devel] [PATCH v2 2/2] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Cornelia Huck]

On Mon, 29 Jul 2019 17:29:03 +0100
"Dr. David Alan Gilbert (git)"  wrote:

> From: "Dr. David Alan Gilbert" 
> 
> This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
> since that accidentally removes the PCIe capabilities from virtio
> devices because virtio_pci_dc_realize is called before the new 'mode'
> flag is set.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  hw/core/machine.c | 23 +++
>  hw/display/virtio-gpu-pci.c   |  4 +---
>  hw/display/virtio-vga.c   |  4 +---
>  hw/virtio/virtio-crypto-pci.c |  4 +---
>  hw/virtio/virtio-input-pci.c  |  4 +---
>  hw/virtio/virtio-pci.c| 26 ++
>  hw/virtio/virtio-pci.h| 31 ++-
>  7 files changed, 23 insertions(+), 73 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [Qemu-devel] [PATCH v2 1/2] Revert "Revert "globals: Allow global properties to be optional""

[Cornelia Huck]

On Mon, 29 Jul 2019 17:29:02 +0100
"Dr. David Alan Gilbert (git)"  wrote:

> From: "Dr. David Alan Gilbert" 
> 
> This reverts commit 8fa70dbd8bb478d9483c1da3e9976a2d86b3f9a0.
> 
> Because we're about to revert it's neighbour and thus uses an optional
> again.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  include/hw/qdev-core.h | 3 +++
>  qom/object.c   | 3 +++
>  2 files changed, 6 insertions(+)

As discussed on IRC, the 'bugs' mentioned in that commit were related
to the other patch that is being reverted; so I don't really mind it
for 4.1 (IOW, either of the two revert approaches should be fine.)

Reviewed-by: Cornelia Huck 

Re: [Qemu-devel] [For 4.1 PATCH v2 0/2] Reversions to fix PCIe in virtio

[Daniel P . Berrangé]

On Mon, Jul 29, 2019 at 05:43:17PM +0100, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 17:36, Dr. David Alan Gilbert
>  wrote:
> >
> > * Cornelia Huck (coh...@redhat.com) wrote:
> > > On Mon, 29 Jul 2019 17:29:01 +0100
> > > "Dr. David Alan Gilbert (git)"  wrote:
> > >
> > > > From: "Dr. David Alan Gilbert" 
> > > >
> > > > Revert a couple of patches that break PCIe capabilities in virtio
> > > > devices. The 'optional' revert is just reverted to make the main
> > > > reversion trivial.
> > >
> > > Don't want to spoil the party here; but wasn't the optional stuff
> > > removed because it was deemed to be a bad idea?
> >
> > I'm perfectly happy to go either way with this; it maybe a bad idea
> > but it's harmless I think.
> 
> It seems like the original commits were:
>  * patch that does something
>  * patch that removes no-longer used functionality (optional globals)
> 
> so it makes sense to me that if we want to revert the 'patch that
> does something' we should first revert the patch that cleaned
> up unused-functionality (because now we need it again). Is
> that right?
> 
> If optional-globals are a bad idea then we should take another
> run at this for 4.2, but as a "revert stuff for 4.1" strategy
> it seems fine to me.

Functionally both approaches are supposed to be identical, but given
that we already found one last minute problem with the 2nd patch, the
full revert of both feels ever so slightly safer.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-devel] [Bug 1838312] Re: Qemu virt-manager Segmentation fault

[Daniel Berrange]

** Also affects: virt-manager (Ubuntu)
   Importance: Undecided
   Status: New

** Changed in: qemu
   Status: New => Invalid

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1838312

Title:
  Qemu virt-manager Segmentation fault

Status in QEMU:
  Invalid
Status in virt-manager package in Ubuntu:
  New

Bug description:
  Hi!

  I installed all these packages:

  sudo apt install qemu
  sudo apt install ipxe-qemu-256k-compat-efi-roms libspice-server1 libbluetooth3
  sudo apt install libbrlapi0.6 libcacard0 libfdt1 libusbredirparser1 
libvirglrenderer0 libxen-4.9 libxenstore3.0
  sudo apt install cpu-checker ibverbs-providers ipxe-qemu libibverbs1 
libiscsi7 libnl-route-3-200 librados2 librbd1 librdmacm1 msr-tools sharutils
  sudo apt install qemu-block-extra qemu-system-common qemu-system-data 
qemu-system-gui qemu-utils
  sudo apt install --no-install-recommends qemu-kvm qemu-system-x86
  sudo apt install libauparse0 ebtables gir1.2-gtk-vnc-2.0 gir1.2-libosinfo-1.0 
gir1.2-libvirt-glib-1.0 gir1.2-spiceclientglib-2.0 gir1.2-spiceclientgtk-3.0 
libvde0 libvdeplug2 libgovirt-common libgovirt2 libgtk-vnc-2.0-0 libgvnc-1.0-0 
libosinfo-1.0-0 libphodav-2.0-0 libphodav-2.0-common libspice-client-glib-2.0-8 
libspice-client-gtk-3.0-5 libusbredirhost1 libvirt-clients libvirt-daemon 
libvirt-daemon-driver-storage-rbd libvirt-daemon-system libvirt-glib-1.0-0 
libvirt0 osinfo-db python3-libvirt python3-libxml2 
spice-client-glib-usb-acl-helper vde2 vde2-cryptcab virt-viewer virtinst 
virt-manager

  without the i386 packages for Qemu because I want only 64 bit.

  I installed all these packages without error, but when I run

  # virt-manager

  Output: ...shows me:

  Segmentation fault

  
  My hardware is 100% ok.
  Maybee a broken lib?


  How can I fix that?

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1838312/+subscriptions

[Qemu-devel] [Bug 1838312] Re: Qemu virt-manager Segmentation fault

[Hans Peter]

syslog:

kernel: [ 2003.888116] virt-manager[16014]: segfault at 32d0 ip
32d0 sp 7ffeb09ac658 error 14 in python3.7[40+21000]

kernel: [ 2003.888124] Code: Bad RIP value.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1838312

Title:
  Qemu virt-manager Segmentation fault

Status in QEMU:
  Invalid
Status in virt-manager package in Ubuntu:
  New

Bug description:
  Hi!

  I installed all these packages:

  sudo apt install qemu
  sudo apt install ipxe-qemu-256k-compat-efi-roms libspice-server1 libbluetooth3
  sudo apt install libbrlapi0.6 libcacard0 libfdt1 libusbredirparser1 
libvirglrenderer0 libxen-4.9 libxenstore3.0
  sudo apt install cpu-checker ibverbs-providers ipxe-qemu libibverbs1 
libiscsi7 libnl-route-3-200 librados2 librbd1 librdmacm1 msr-tools sharutils
  sudo apt install qemu-block-extra qemu-system-common qemu-system-data 
qemu-system-gui qemu-utils
  sudo apt install --no-install-recommends qemu-kvm qemu-system-x86
  sudo apt install libauparse0 ebtables gir1.2-gtk-vnc-2.0 gir1.2-libosinfo-1.0 
gir1.2-libvirt-glib-1.0 gir1.2-spiceclientglib-2.0 gir1.2-spiceclientgtk-3.0 
libvde0 libvdeplug2 libgovirt-common libgovirt2 libgtk-vnc-2.0-0 libgvnc-1.0-0 
libosinfo-1.0-0 libphodav-2.0-0 libphodav-2.0-common libspice-client-glib-2.0-8 
libspice-client-gtk-3.0-5 libusbredirhost1 libvirt-clients libvirt-daemon 
libvirt-daemon-driver-storage-rbd libvirt-daemon-system libvirt-glib-1.0-0 
libvirt0 osinfo-db python3-libvirt python3-libxml2 
spice-client-glib-usb-acl-helper vde2 vde2-cryptcab virt-viewer virtinst 
virt-manager

  without the i386 packages for Qemu because I want only 64 bit.

  I installed all these packages without error, but when I run

  # virt-manager

  Output: ...shows me:

  Segmentation fault

  
  My hardware is 100% ok.
  Maybee a broken lib?


  How can I fix that?

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1838312/+subscriptions

Re: [Qemu-devel] [For 4.1 PATCH v2 0/2] Reversions to fix PCIe in virtio

[Peter Maydell]

On Mon, 29 Jul 2019 at 17:36, Dr. David Alan Gilbert
 wrote:
>
> * Cornelia Huck (coh...@redhat.com) wrote:
> > On Mon, 29 Jul 2019 17:29:01 +0100
> > "Dr. David Alan Gilbert (git)"  wrote:
> >
> > > From: "Dr. David Alan Gilbert" 
> > >
> > > Revert a couple of patches that break PCIe capabilities in virtio
> > > devices. The 'optional' revert is just reverted to make the main
> > > reversion trivial.
> >
> > Don't want to spoil the party here; but wasn't the optional stuff
> > removed because it was deemed to be a bad idea?
>
> I'm perfectly happy to go either way with this; it maybe a bad idea
> but it's harmless I think.

It seems like the original commits were:
 * patch that does something
 * patch that removes no-longer used functionality (optional globals)

so it makes sense to me that if we want to revert the 'patch that
does something' we should first revert the patch that cleaned
up unused-functionality (because now we need it again). Is
that right?

If optional-globals are a bad idea then we should take another
run at this for 4.2, but as a "revert stuff for 4.1" strategy
it seems fine to me.

thanks
-- PMM

[Qemu-devel] [PATCH for-4.1] scsi-cd: Fix inserting read-only media in empty drive

[Kevin Wolf]

scsi-disks decides whether it has a read-only device by looking at
whether the BlockBackend specified as drive=... is read-only. In the
case of an anonymous BlockBackend (with a node name specified in
drive=...), this is the read-only flag of the attached node. In the case
of an empty anonymous BlockBackend, it's always read-write because
nothing prevented it from being read-write.

This is a problem because scsi-cd would take write permissions on the
anonymous BlockBackend of an empty drive created without a drive=...
option. Using blockdev-insert-medium with a read-only node fails then
with the error message "Block node is read-only".

Fix scsi_realize() so that scsi-cd devices always take read-only
permissions on their BlockBackend instead.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1733920
Signed-off-by: Kevin Wolf 
---
 hw/scsi/scsi-disk.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 8e95e3e38d..af3e622dc5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2318,6 +2318,7 @@ static void scsi_disk_unit_attention_reported(SCSIDevice 
*dev)
 static void scsi_realize(SCSIDevice *dev, Error **errp)
 {
 SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+bool read_only;
 
 if (!s->qdev.conf.blk) {
 error_setg(errp, "drive property not set");
@@ -2351,8 +2352,13 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
 return;
 }
 }
-if (!blkconf_apply_backend_options(>conf,
-   blk_is_read_only(s->qdev.conf.blk),
+
+read_only = blk_is_read_only(s->qdev.conf.blk);
+if (dev->type == TYPE_ROM) {
+read_only = true;
+}
+
+if (!blkconf_apply_backend_options(>conf, read_only,
dev->type == TYPE_DISK, errp)) {
 return;
 }
-- 
2.20.1

Re: [Qemu-devel] [For 4.1 PATCH v2 0/2] Reversions to fix PCIe in virtio

[Dr. David Alan Gilbert]

* Cornelia Huck (coh...@redhat.com) wrote:
> On Mon, 29 Jul 2019 17:29:01 +0100
> "Dr. David Alan Gilbert (git)"  wrote:
> 
> > From: "Dr. David Alan Gilbert" 
> > 
> > Revert a couple of patches that break PCIe capabilities in virtio
> > devices. The 'optional' revert is just reverted to make the main
> > reversion trivial.
> 
> Don't want to spoil the party here; but wasn't the optional stuff
> removed because it was deemed to be a bad idea?

I'm perfectly happy to go either way with this; it maybe a bad idea
but it's harmless I think.

Dave

> > 
> > Symptom:
> >   Loss of PCIe capabilities in virtio devices hung off PCIe bridges
> > 
> > Signed-off-by: Dr. David Alan Gilbert 
> > 
> > 
> > Dr. David Alan Gilbert (2):
> >   Revert "Revert "globals: Allow global properties to be optional""
> >   Revert "hw: report invalid disable-legacy|modern usage for
> > virtio-1-only devs"
> > 
> >  hw/core/machine.c | 23 +++
> >  hw/display/virtio-gpu-pci.c   |  4 +---
> >  hw/display/virtio-vga.c   |  4 +---
> >  hw/virtio/virtio-crypto-pci.c |  4 +---
> >  hw/virtio/virtio-input-pci.c  |  4 +---
> >  hw/virtio/virtio-pci.c| 26 ++
> >  hw/virtio/virtio-pci.h| 31 ++-
> >  include/hw/qdev-core.h|  3 +++
> >  qom/object.c  |  3 +++
> >  9 files changed, 29 insertions(+), 73 deletions(-)
> > 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [Bug 1838312] [NEW] Qemu virt-manager Segmentation fault

[Hans Peter]

Public bug reported:

Hi!

I installed all these packages:

sudo apt install qemu
sudo apt install ipxe-qemu-256k-compat-efi-roms libspice-server1 libbluetooth3
sudo apt install libbrlapi0.6 libcacard0 libfdt1 libusbredirparser1 
libvirglrenderer0 libxen-4.9 libxenstore3.0
sudo apt install cpu-checker ibverbs-providers ipxe-qemu libibverbs1 libiscsi7 
libnl-route-3-200 librados2 librbd1 librdmacm1 msr-tools sharutils
sudo apt install qemu-block-extra qemu-system-common qemu-system-data 
qemu-system-gui qemu-utils
sudo apt install --no-install-recommends qemu-kvm qemu-system-x86
sudo apt install libauparse0 ebtables gir1.2-gtk-vnc-2.0 gir1.2-libosinfo-1.0 
gir1.2-libvirt-glib-1.0 gir1.2-spiceclientglib-2.0 gir1.2-spiceclientgtk-3.0 
libvde0 libvdeplug2 libgovirt-common libgovirt2 libgtk-vnc-2.0-0 libgvnc-1.0-0 
libosinfo-1.0-0 libphodav-2.0-0 libphodav-2.0-common libspice-client-glib-2.0-8 
libspice-client-gtk-3.0-5 libusbredirhost1 libvirt-clients libvirt-daemon 
libvirt-daemon-driver-storage-rbd libvirt-daemon-system libvirt-glib-1.0-0 
libvirt0 osinfo-db python3-libvirt python3-libxml2 
spice-client-glib-usb-acl-helper vde2 vde2-cryptcab virt-viewer virtinst 
virt-manager

without the i386 packages for Qemu because I want only 64 bit.

I installed all these packages without error, but when I run

# virt-manager

Output: ...shows me:

Segmentation fault


My hardware is 100% ok.
Maybee a broken lib?


How can I fix that?

** Affects: qemu
 Importance: Undecided
 Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1838312

Title:
  Qemu virt-manager Segmentation fault

Status in QEMU:
  New

Bug description:
  Hi!

  I installed all these packages:

  sudo apt install qemu
  sudo apt install ipxe-qemu-256k-compat-efi-roms libspice-server1 libbluetooth3
  sudo apt install libbrlapi0.6 libcacard0 libfdt1 libusbredirparser1 
libvirglrenderer0 libxen-4.9 libxenstore3.0
  sudo apt install cpu-checker ibverbs-providers ipxe-qemu libibverbs1 
libiscsi7 libnl-route-3-200 librados2 librbd1 librdmacm1 msr-tools sharutils
  sudo apt install qemu-block-extra qemu-system-common qemu-system-data 
qemu-system-gui qemu-utils
  sudo apt install --no-install-recommends qemu-kvm qemu-system-x86
  sudo apt install libauparse0 ebtables gir1.2-gtk-vnc-2.0 gir1.2-libosinfo-1.0 
gir1.2-libvirt-glib-1.0 gir1.2-spiceclientglib-2.0 gir1.2-spiceclientgtk-3.0 
libvde0 libvdeplug2 libgovirt-common libgovirt2 libgtk-vnc-2.0-0 libgvnc-1.0-0 
libosinfo-1.0-0 libphodav-2.0-0 libphodav-2.0-common libspice-client-glib-2.0-8 
libspice-client-gtk-3.0-5 libusbredirhost1 libvirt-clients libvirt-daemon 
libvirt-daemon-driver-storage-rbd libvirt-daemon-system libvirt-glib-1.0-0 
libvirt0 osinfo-db python3-libvirt python3-libxml2 
spice-client-glib-usb-acl-helper vde2 vde2-cryptcab virt-viewer virtinst 
virt-manager

  without the i386 packages for Qemu because I want only 64 bit.

  I installed all these packages without error, but when I run

  # virt-manager

  Output: ...shows me:

  Segmentation fault

  
  My hardware is 100% ok.
  Maybee a broken lib?


  How can I fix that?

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1838312/+subscriptions

Re: [Qemu-devel] [For 4.1 PATCH v2 0/2] Reversions to fix PCIe in virtio

[Cornelia Huck]

On Mon, 29 Jul 2019 17:29:01 +0100
"Dr. David Alan Gilbert (git)"  wrote:

> From: "Dr. David Alan Gilbert" 
> 
> Revert a couple of patches that break PCIe capabilities in virtio
> devices. The 'optional' revert is just reverted to make the main
> reversion trivial.

Don't want to spoil the party here; but wasn't the optional stuff
removed because it was deemed to be a bad idea?

> 
> Symptom:
>   Loss of PCIe capabilities in virtio devices hung off PCIe bridges
> 
> Signed-off-by: Dr. David Alan Gilbert 
> 
> 
> Dr. David Alan Gilbert (2):
>   Revert "Revert "globals: Allow global properties to be optional""
>   Revert "hw: report invalid disable-legacy|modern usage for
> virtio-1-only devs"
> 
>  hw/core/machine.c | 23 +++
>  hw/display/virtio-gpu-pci.c   |  4 +---
>  hw/display/virtio-vga.c   |  4 +---
>  hw/virtio/virtio-crypto-pci.c |  4 +---
>  hw/virtio/virtio-input-pci.c  |  4 +---
>  hw/virtio/virtio-pci.c| 26 ++
>  hw/virtio/virtio-pci.h| 31 ++-
>  include/hw/qdev-core.h|  3 +++
>  qom/object.c  |  3 +++
>  9 files changed, 29 insertions(+), 73 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 2/2] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Daniel P . Berrangé]

On Mon, Jul 29, 2019 at 05:29:03PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
> since that accidentally removes the PCIe capabilities from virtio
> devices because virtio_pci_dc_realize is called before the new 'mode'
> flag is set.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  hw/core/machine.c | 23 +++
>  hw/display/virtio-gpu-pci.c   |  4 +---
>  hw/display/virtio-vga.c   |  4 +---
>  hw/virtio/virtio-crypto-pci.c |  4 +---
>  hw/virtio/virtio-input-pci.c  |  4 +---
>  hw/virtio/virtio-pci.c| 26 ++
>  hw/virtio/virtio-pci.h| 31 ++-
>  7 files changed, 23 insertions(+), 73 deletions(-)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 1/2] Revert "Revert "globals: Allow global properties to be optional""

[Daniel P . Berrangé]

On Mon, Jul 29, 2019 at 05:29:02PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> This reverts commit 8fa70dbd8bb478d9483c1da3e9976a2d86b3f9a0.
> 
> Because we're about to revert it's neighbour and thus uses an optional
> again.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  include/hw/qdev-core.h | 3 +++
>  qom/object.c   | 3 +++
>  2 files changed, 6 insertions(+)

Reviewed-by: Daniel P. Berrangé 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH v2 2/2] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
since that accidentally removes the PCIe capabilities from virtio
devices because virtio_pci_dc_realize is called before the new 'mode'
flag is set.

Signed-off-by: Dr. David Alan Gilbert 
---
 hw/core/machine.c | 23 +++
 hw/display/virtio-gpu-pci.c   |  4 +---
 hw/display/virtio-vga.c   |  4 +---
 hw/virtio/virtio-crypto-pci.c |  4 +---
 hw/virtio/virtio-input-pci.c  |  4 +---
 hw/virtio/virtio-pci.c| 26 ++
 hw/virtio/virtio-pci.h| 31 ++-
 7 files changed, 23 insertions(+), 73 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c58a8e594e..c4a2ab2282 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -115,26 +115,9 @@ const size_t hw_compat_2_7_len = 
G_N_ELEMENTS(hw_compat_2_7);
 
 GlobalProperty hw_compat_2_6[] = {
 { "virtio-mmio", "format_transport_address", "off" },
-/*
- * don't include devices which are modern-only
- * ie keyboard, mouse, tablet, gpu, vga & crypto
- */
-{ "virtio-9p-pci", "disable-modern", "on" },
-{ "virtio-9p-pci", "disable-legacy", "off" },
-{ "virtio-balloon-pci", "disable-modern", "on" },
-{ "virtio-balloon-pci", "disable-legacy", "off" },
-{ "virtio-blk-pci", "disable-modern", "on" },
-{ "virtio-blk-pci", "disable-legacy", "off" },
-{ "virtio-input-host-pci", "disable-modern", "on" },
-{ "virtio-input-host-pci", "disable-legacy", "off" },
-{ "virtio-net-pci", "disable-modern", "on" },
-{ "virtio-net-pci", "disable-legacy", "off" },
-{ "virtio-rng-pci", "disable-modern", "on" },
-{ "virtio-rng-pci", "disable-legacy", "off" },
-{ "virtio-scsi-pci", "disable-modern", "on" },
-{ "virtio-scsi-pci", "disable-legacy", "off" },
-{ "virtio-serial-pci", "disable-modern", "on" },
-{ "virtio-serial-pci", "disable-legacy", "off" },
+/* Optional because not all virtio-pci devices support legacy mode */
+{ "virtio-pci", "disable-modern", "on",  .optional = true },
+{ "virtio-pci", "disable-legacy", "off", .optional = true },
 };
 const size_t hw_compat_2_6_len = G_N_ELEMENTS(hw_compat_2_6);
 
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index d6f01b4a98..e4c7eb6193 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -33,9 +33,7 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 Error *local_error = NULL;
 
 qdev_set_parent_bus(vdev, BUS(_dev->bus));
-if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-return;
-}
+virtio_pci_force_virtio_1(vpci_dev);
 object_property_set_bool(OBJECT(vdev), true, "realized", _error);
 
 if (local_error) {
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 416e7fec87..79a145e284 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -137,9 +137,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 
 /* init virtio bits */
 qdev_set_parent_bus(DEVICE(g), BUS(_dev->bus));
-if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-return;
-}
+virtio_pci_force_virtio_1(vpci_dev);
 object_property_set_bool(OBJECT(g), true, "realized", );
 if (err) {
 error_propagate(errp, err);
diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c
index c8a2317a10..91d4446080 100644
--- a/hw/virtio/virtio-crypto-pci.c
+++ b/hw/virtio/virtio-crypto-pci.c
@@ -53,9 +53,7 @@ static void virtio_crypto_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 }
 
 qdev_set_parent_bus(vdev, BUS(_dev->bus));
-if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-return;
-}
+virtio_pci_force_virtio_1(vpci_dev);
 object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 object_property_set_link(OBJECT(vcrypto),
  OBJECT(vcrypto->vdev.conf.cryptodev), "cryptodev",
diff --git a/hw/virtio/virtio-input-pci.c b/hw/virtio/virtio-input-pci.c
index 1c40292abc..ad7774e93e 100644
--- a/hw/virtio/virtio-input-pci.c
+++ b/hw/virtio/virtio-input-pci.c
@@ -49,9 +49,7 @@ static void virtio_input_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 DeviceState *vdev = DEVICE(>vdev);
 
 qdev_set_parent_bus(vdev, BUS(_dev->bus));
-if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-return;
-}
+virtio_pci_force_virtio_1(vpci_dev);
 object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce928f2429..f6d2223e78 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1723,22 +1723,16 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
Error **errp)
/* PCI BAR regions must be powers of 2 */
pow2ceil(proxy->notify.offset + 

[Qemu-devel] [For 4.1 PATCH v2 0/2] Reversions to fix PCIe in virtio

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

Revert a couple of patches that break PCIe capabilities in virtio
devices. The 'optional' revert is just reverted to make the main
reversion trivial.

Symptom:
  Loss of PCIe capabilities in virtio devices hung off PCIe bridges

Signed-off-by: Dr. David Alan Gilbert 


Dr. David Alan Gilbert (2):
  Revert "Revert "globals: Allow global properties to be optional""
  Revert "hw: report invalid disable-legacy|modern usage for
virtio-1-only devs"

 hw/core/machine.c | 23 +++
 hw/display/virtio-gpu-pci.c   |  4 +---
 hw/display/virtio-vga.c   |  4 +---
 hw/virtio/virtio-crypto-pci.c |  4 +---
 hw/virtio/virtio-input-pci.c  |  4 +---
 hw/virtio/virtio-pci.c| 26 ++
 hw/virtio/virtio-pci.h| 31 ++-
 include/hw/qdev-core.h|  3 +++
 qom/object.c  |  3 +++
 9 files changed, 29 insertions(+), 73 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v2 1/2] Revert "Revert "globals: Allow global properties to be optional""

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

This reverts commit 8fa70dbd8bb478d9483c1da3e9976a2d86b3f9a0.

Because we're about to revert it's neighbour and thus uses an optional
again.

Signed-off-by: Dr. David Alan Gilbert 
---
 include/hw/qdev-core.h | 3 +++
 qom/object.c   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e157fc4acd..136df7774c 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -252,6 +252,8 @@ struct PropertyInfo {
 /**
  * GlobalProperty:
  * @used: Set to true if property was used when initializing a device.
+ * @optional: If set to true, GlobalProperty will be skipped without errors
+ *if the property doesn't exist.
  *
  * An error is fatal for non-hotplugged devices, when the global is applied.
  */
@@ -260,6 +262,7 @@ typedef struct GlobalProperty {
 const char *property;
 const char *value;
 bool used;
+bool optional;
 } GlobalProperty;
 
 static inline void
diff --git a/qom/object.c b/qom/object.c
index 3966a3d461..147727 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -386,6 +386,9 @@ void object_apply_global_props(Object *obj, const GPtrArray 
*props, Error **errp
 if (object_dynamic_cast(obj, p->driver) == NULL) {
 continue;
 }
+if (p->optional && !object_property_find(obj, p->property, NULL)) {
+continue;
+}
 p->used = true;
 object_property_parse(obj, p->value, p->property, );
 if (err != NULL) {
-- 
2.21.0

Re: [Qemu-devel] [for 4.1 PATCH] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berra...@redhat.com) wrote:
> On Mon, Jul 29, 2019 at 04:59:09PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" 
> > 
> > This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
> > since that accidentally removes the PCIe capabilities from virtio
> > devices because virtio_pci_dc_realize is called before the new 'mode'
> > flag is set.
> > I keep the expanded hw_compat entry because we've lost the ability to
> > do 'optional'.
> 
> That was the commit immediately following. If you revert
> 8fa70dbd8bb478d9483c1da3e9976a2d86b3f9a0 first, then this
> one would likely revert cleanly.

OK, double revert coming up.

Dave

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v5 2/5] hw/block/pflash_cfi01: Use the correct READ_ARRAY value

[Philippe Mathieu-Daudé]

Hi Markus.

On 7/16/19 5:12 PM, Markus Armbruster wrote:
> "Dr. David Alan Gilbert"  writes:
> 
>> * Markus Armbruster (arm...@redhat.com) wrote:
>>> Philippe asked me to have a look at this one, so here goes.

Thanks a lot for your careful analysis.

I got scared the uh-oh you raised would get this series or rework of it
still refused for the release, so I went for a quick-and-dirty bugfix.
Since this bugfix got merged (as commit 3a283507c0347), I can now think
again about how to properly fix this (if it is fixable...).

>>> Philippe Mathieu-Daudé  writes:
>>>
 In the document [*] the "Read Array Flowchart", the READ_ARRAY
 command has a value of 0xff.

 Use the correct value in the pflash model.

 There is no change of behavior in the guest, because:
 - when the guest were sending 0xFF, the reset_flash label
   was setting the command value as 0x00
 - 0x00 was used internally for READ_ARRAY
>>>
>>> *Groan*
>>>
>>> Is this cleanup, or does it fix an observable bug?

Well it depends where you stand.

I have a few patches on top of this adding trace events (4.2 material)
and while debugging it was not making sense with the CFI specs.

1/ The guest writes 0xFF to go in READ_ARRAY mode, the model report a
warning and take the switch default case which calls pflash_reset(),
which happens to set the flash in READ_ARRAY.

2/ Then a later series adds the CFI specs timings (like the CFI02
model), because it would useful to test the UEFI Capsule Update feature
with real-time behavior. For the 'Virt' pflash, the timing is disabled.
Now running a non-Virt pflash, it becomes very slow because each time
the guest goes into READ_ARRAY mode, the reset delay (which is the
longest) occurs.

Talking with Laszlo, I figured for 1/ instead of fixing the model, I can
display 0x00 as 0xFF and ignore the pflash_reset() when the caller is
not system_reset(). Dirty again.

For 2/ it is not that easy, it will depends if there is more interest
from the UEFI community (Intel parallel NOR flashes are used on x86 and
aarch64 UEFI platforms).

If we justify 1/ and 2/ are not important, then it is simply a cleanup.

 To keep migration with older versions behaving correctly, we
 decide to always migrate the READ_ARRAY as 0x00.

 If the CFI open standard decide to assign a new command of value
 0x00, this model is flawed because it uses this value internally.
 If a guest eventually requires this new CFI feature, a different
 model will be required (or this same model but breaking backward
 migration). So it is safe to keep migrating READ_ARRAY as 0x00.
>>>
>>> We could perhaps keep migration working for "benign" device states, with
>>> judicious use of subsections.  We'll cross that bridge when we get to
>>> it.
>>>
 [*] "Common Flash Interface (CFI) and Command Sets"
 (Intel Application Note 646)
 Appendix B "Basic Command Set"

 Reviewed-by: John Snow 
 Reviewed-by: Alistair Francis 
 Regression-tested-by: Laszlo Ersek 
 Signed-off-by: Philippe Mathieu-Daudé 
 ---
 v3: Handle migrating the 'cmd' field.
 v4: Handle migrating to older QEMU (Dave)
 v5: Add a paragraph about why this model is flawed due to
 historically using READ_ARRAY as 0x00 (Dave, Peter).

 Since Laszlo stated he did not test migration [*], I'm keeping his
 test tag, because the change with v2 has no impact in the tests
 he ran.

 Likewise I'm keeping John and Alistair tags, but I'd like an extra
 review for the migration change, thanks!

 [*] https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00679.html
 ---
  hw/block/pflash_cfi01.c | 57 ++---
  1 file changed, 48 insertions(+), 9 deletions(-)

 diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
 index 9e34fd4e82..85bb2132c0 100644
 --- a/hw/block/pflash_cfi01.c
 +++ b/hw/block/pflash_cfi01.c
 @@ -96,6 +96,37 @@ struct PFlashCFI01 {
  bool old_multiple_chip_handling;
  };
  
 +static int pflash_pre_save(void *opaque)
 +{
 +PFlashCFI01 *s = opaque;
 +
 +/*
 + * Previous to QEMU v4.1 an incorrect value of 0x00 was used for the
 + * READ_ARRAY command. To preserve migrating to these older version,
 + * always migrate the READ_ARRAY command as 0x00.
 + */
 +if (s->cmd == 0xff) {
 +s->cmd = 0x00;
 +}
 +
 +return 0;
 +}
 +
 +static int pflash_post_save(void *opaque)
 +{
 +PFlashCFI01 *s = opaque;
 +
 +/*
 + * If migration failed, the guest will continue to run.
 + * Restore the correct READ_ARRAY value.
 + */
 +if (s->cmd == 0x00) {
 +s->cmd = 0xff;
 +}
 +
 +return 0;
 +}
>>>
>>> Uh, this gives me a queasy feeling.  Perhaps David can assuage it.
>>
>> See 

Re: [Qemu-devel] [for 4.1 PATCH] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Cornelia Huck]

On Mon, 29 Jul 2019 16:59:09 +0100
"Dr. David Alan Gilbert (git)"  wrote:

> From: "Dr. David Alan Gilbert" 
> 
> This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
> since that accidentally removes the PCIe capabilities from virtio
> devices because virtio_pci_dc_realize is called before the new 'mode'
> flag is set.

Yes, this is probably too hairy to fix for 4.1; but the change might be
worthwhile to revisit for 4.2.

> I keep the expanded hw_compat entry because we've lost the ability to
> do 'optional'.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  hw/display/virtio-gpu-pci.c   |  4 +---
>  hw/display/virtio-vga.c   |  4 +---
>  hw/virtio/virtio-crypto-pci.c |  4 +---
>  hw/virtio/virtio-input-pci.c  |  4 +---
>  hw/virtio/virtio-pci.c| 26 ++
>  hw/virtio/virtio-pci.h| 31 ++-
>  6 files changed, 20 insertions(+), 53 deletions(-)

Reviewed-by: Cornelia Huck 

[Qemu-devel] [PULL 1/1] i386: Fix Snowridge CPU model name and features

[Eduardo Habkost]

From: Paul Lai 

Changing the name to Snowridge from SnowRidge-Server.
There is no client model of Snowridge, so "-Server" is unnecessary.

Removing CPUID_EXT_VMX from Snowridge cpu feature list.

Signed-off-by: Paul Lai 
Tested-by: Tao3 Xu 
Message-Id: <20190716155808.25010-1-paul.c@intel.com>
Signed-off-by: Eduardo Habkost 
---
 target/i386/cpu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index e3320f5e92..19751e37a7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2472,7 +2472,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
 .model_id = "Intel Xeon Processor (Icelake)",
 },
 {
-.name = "SnowRidge-Server",
+.name = "Snowridge",
 .level = 27,
 .vendor = CPUID_VENDOR_INTEL,
 .family = 6,
@@ -2490,7 +2490,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
 CPUID_FXSR | CPUID_SSE | CPUID_SSE2,
 .features[FEAT_1_ECX] =
 CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_MONITOR |
-CPUID_EXT_VMX |
 CPUID_EXT_SSSE3 |
 CPUID_EXT_CX16 |
 CPUID_EXT_SSE41 |
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PULL 0/1] x86 queue for 4.1

[Eduardo Habkost]

The following changes since commit 893dc8300c80e3dc32f31e968cf7aa0904da50c3:

  Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into 
staging (2019-07-29 12:04:53 +0100)

are available in the Git repository at:

  git://github.com/ehabkost/qemu.git tags/x86-next-pull-request

for you to fetch changes up to ff656fcd338a70c4d9783a800733c4ab3806e5b0:

  i386: Fix Snowridge CPU model name and features (2019-07-29 13:08:02 -0300)


x86 queue for 4.1

* Rename and fix SnowRidge CPU model (Paul Lai)



Paul Lai (1):
  i386: Fix Snowridge CPU model name and features

 target/i386/cpu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [RFC PATCH] tcg: add annotations to dissembler output (RFC, HACK, PoC)

[Alex Bennée]

Sometimes it can be a bit tricky to line up the final host assembly
with the instructions being translated. This is made harder by the
fact disassembly is somewhat grafted into QEMU to support multiple
dissemblers.

Attempt to add more information to the output by:

  - tracking TCGNotes as we generate the code
  - using this to annotate lines of host disassembly

UNRESOLVED ISSUES:

We are duplicating some of the tracking done for INDEX_op_insn_start.
We should probably merge and have a common system that can be used
for:
  a) required stuff (insn bounderies, resolving registers on faults)
  b) nice debug info (which ops are loads/stores/splills)

The hacking in of extra info into the disassembly stream if fugly.

Signed-off-by: Alex Bennée 
---
 accel/tcg/translate-all.c |  5 +++--
 disas.c   | 17 +
 include/disas/disas.h |  4 +++-
 include/exec/log.h|  6 --
 tcg/tcg.c | 39 ---
 tcg/tcg.h | 17 +
 6 files changed, 72 insertions(+), 16 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 0394e989d06..2f7ade495e2 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1775,6 +1775,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 tb->cflags = cflags;
 tb->trace_vcpu_dstate = *cpu->trace_dstate;
 tcg_ctx->tb_cflags = cflags;
+tcg_ctx->notes = g_array_set_size(tcg_ctx->notes, 0);
  tb_overflow:
 
 #ifdef CONFIG_PROFILER
@@ -1896,7 +1897,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 size_t data_size = gen_code_size - code_size;
 size_t i;
 
-log_disas(tb->tc.ptr, code_size);
+log_disas(tb->tc.ptr, code_size, tcg_ctx);
 
 for (i = 0; i < data_size; i += sizeof(tcg_target_ulong)) {
 if (sizeof(tcg_target_ulong) == 8) {
@@ -1910,7 +1911,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 }
 }
 } else {
-log_disas(tb->tc.ptr, gen_code_size);
+log_disas(tb->tc.ptr, gen_code_size, tcg_ctx);
 }
 qemu_log("\n");
 qemu_log_flush();
diff --git a/disas.c b/disas.c
index c0f57284a43..edf4462846f 100644
--- a/disas.c
+++ b/disas.c
@@ -342,7 +342,7 @@ static bool cap_disas_target(disassemble_info *info, 
uint64_t pc, size_t size)
 }
 
 /* Disassemble SIZE bytes at CODE for the host.  */
-static bool cap_disas_host(disassemble_info *info, void *code, size_t size)
+static bool cap_disas_host(disassemble_info *info, void *code, size_t size, 
void *opaque)
 {
 csh handle;
 const uint8_t *cbuf;
@@ -359,6 +359,7 @@ static bool cap_disas_host(disassemble_info *info, void 
*code, size_t size)
 
 while (cs_disasm_iter(handle, , , , insn)) {
cap_dump_insn(info, insn);
+   tcg_disas_annotation(stderr, opaque, pc);
 }
 if (size != 0) {
 (*info->fprintf_func)(info->stream,
@@ -416,7 +417,7 @@ static bool cap_disas_monitor(disassemble_info *info, 
uint64_t pc, int count)
 #endif /* !CONFIG_USER_ONLY */
 #else
 # define cap_disas_target(i, p, s)  false
-# define cap_disas_host(i, p, s)  false
+# define cap_disas_host(i, p, s, o)  false
 # define cap_disas_monitor(i, p, c)  false
 #endif /* CONFIG_CAPSTONE */
 
@@ -462,7 +463,6 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong 
code,
 for (pc = code; size > 0; pc += count, size -= count) {
fprintf(out, "0x" TARGET_FMT_lx ":  ", pc);
count = s.info.print_insn(pc, );
-   fprintf(out, "\n");
if (count < 0)
break;
 if (size < count) {
@@ -477,7 +477,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong 
code,
 
 
 /* Disassemble this for me please... (debugging). */
-void disas(FILE *out, void *code, unsigned long size)
+void disas(FILE *out, void *code, unsigned long size, void *opaque)
 {
 uintptr_t pc;
 int count;
@@ -555,7 +555,7 @@ void disas(FILE *out, void *code, unsigned long size)
 print_insn = print_insn_hppa;
 #endif
 
-if (s.info.cap_arch >= 0 && cap_disas_host(, code, size)) {
+if (s.info.cap_arch >= 0 && cap_disas_host(, code, size, opaque)) {
 return;
 }
 
@@ -565,9 +565,10 @@ void disas(FILE *out, void *code, unsigned long size)
 for (pc = (uintptr_t)code; size > 0; pc += count, size -= count) {
 fprintf(out, "0x%08" PRIxPTR ":  ", pc);
 count = print_insn(pc, );
-   fprintf(out, "\n");
-   if (count < 0)
-   break;
+tcg_disas_annotation(out, opaque, pc);
+fprintf(out, "\n");
+if (count < 0)
+break;
 }
 }
 
diff --git a/include/disas/disas.h b/include/disas/disas.h
index 15da511f49c..c2f75c759f1 100644
--- a/include/disas/disas.h
+++ b/include/disas/disas.h
@@ -5,8 +5,10 @@
 #ifdef NEED_CPU_H
 #include "cpu.h"
 
+void tcg_disas_annotation(FILE *out, void *opaque, uintptr_t pc);
+
 /* Disassemble this for 

Re: [Qemu-devel] Fixing Snowridge CPU model name and features

[Eduardo Habkost]

On Mon, Jul 29, 2019 at 04:46:01PM +0100, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 16:42, Bruce Rogers  wrote:
> >
> > On Tue, 2019-07-16 at 08:58 -0700, Paul Lai wrote:
> > > Changing the name to Snowridge from SnowRidge-Server.
> > > There is no client model of Snowridge, so "-Server" is unnecessary.
> > >
> > > Removing CPUID_EXT_VMX from Snowridge cpu feature list.
> > >
> > > Signed-off-by: Paul Lai 
> > > Tested-by: Tao3 Xu 
> > > ---
> > >  target/i386/cpu.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 63d89276fe..7f56e887ae 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -2688,7 +2688,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
> > >  .model_id = "Intel Xeon Processor (Icelake)",
> > >  },
> > >  {
> > > -.name = "SnowRidge-Server",
> > > +.name = "Snowridge",
> > >  .level = 27,
> > >  .vendor = CPUID_VENDOR_INTEL,
> > >  .family = 6,
> > > @@ -2706,7 +2706,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
> > >  CPUID_FXSR | CPUID_SSE | CPUID_SSE2,
> > >  .features[FEAT_1_ECX] =
> > >  CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_MONITOR
> > > |
> > > -CPUID_EXT_VMX |
> > >  CPUID_EXT_SSSE3 |
> > >  CPUID_EXT_CX16 |
> > >  CPUID_EXT_SSE41 |
> >
> > What is the status of this patch with respect to the v4.1.0 release?
> > It would seem to me that it was targeted for this release, to get the
> > name and features right before codified in a released version, but
> > Intel would know better.
> 
> If nobody picks it up this afternoon then it has (probably) missed the boat.
> Not ccing any of the target/i386 maintainers or putting "for-4.1"
> in the subject line is probably why it got missed. Eduardo/Paolo/Richard ?

I'll pick it and send a pull request ASAP.

-- 
Eduardo

Re: [Qemu-devel] [for 4.1 PATCH] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Daniel P . Berrangé]

On Mon, Jul 29, 2019 at 04:59:09PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
> since that accidentally removes the PCIe capabilities from virtio
> devices because virtio_pci_dc_realize is called before the new 'mode'
> flag is set.
> I keep the expanded hw_compat entry because we've lost the ability to
> do 'optional'.

That was the commit immediately following. If you revert
8fa70dbd8bb478d9483c1da3e9976a2d86b3f9a0 first, then this
one would likely revert cleanly.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-devel] [for 4.1 PATCH] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
since that accidentally removes the PCIe capabilities from virtio
devices because virtio_pci_dc_realize is called before the new 'mode'
flag is set.
I keep the expanded hw_compat entry because we've lost the ability to
do 'optional'.

Signed-off-by: Dr. David Alan Gilbert 
---
 hw/display/virtio-gpu-pci.c   |  4 +---
 hw/display/virtio-vga.c   |  4 +---
 hw/virtio/virtio-crypto-pci.c |  4 +---
 hw/virtio/virtio-input-pci.c  |  4 +---
 hw/virtio/virtio-pci.c| 26 ++
 hw/virtio/virtio-pci.h| 31 ++-
 6 files changed, 20 insertions(+), 53 deletions(-)

diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index d6f01b4a98..e4c7eb6193 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -33,9 +33,7 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 Error *local_error = NULL;
 
 qdev_set_parent_bus(vdev, BUS(_dev->bus));
-if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-return;
-}
+virtio_pci_force_virtio_1(vpci_dev);
 object_property_set_bool(OBJECT(vdev), true, "realized", _error);
 
 if (local_error) {
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 416e7fec87..79a145e284 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -137,9 +137,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 
 /* init virtio bits */
 qdev_set_parent_bus(DEVICE(g), BUS(_dev->bus));
-if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-return;
-}
+virtio_pci_force_virtio_1(vpci_dev);
 object_property_set_bool(OBJECT(g), true, "realized", );
 if (err) {
 error_propagate(errp, err);
diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c
index c8a2317a10..91d4446080 100644
--- a/hw/virtio/virtio-crypto-pci.c
+++ b/hw/virtio/virtio-crypto-pci.c
@@ -53,9 +53,7 @@ static void virtio_crypto_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 }
 
 qdev_set_parent_bus(vdev, BUS(_dev->bus));
-if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-return;
-}
+virtio_pci_force_virtio_1(vpci_dev);
 object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 object_property_set_link(OBJECT(vcrypto),
  OBJECT(vcrypto->vdev.conf.cryptodev), "cryptodev",
diff --git a/hw/virtio/virtio-input-pci.c b/hw/virtio/virtio-input-pci.c
index 1c40292abc..ad7774e93e 100644
--- a/hw/virtio/virtio-input-pci.c
+++ b/hw/virtio/virtio-input-pci.c
@@ -49,9 +49,7 @@ static void virtio_input_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 DeviceState *vdev = DEVICE(>vdev);
 
 qdev_set_parent_bus(vdev, BUS(_dev->bus));
-if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-return;
-}
+virtio_pci_force_virtio_1(vpci_dev);
 object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce928f2429..f6d2223e78 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1723,22 +1723,16 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
Error **errp)
/* PCI BAR regions must be powers of 2 */
pow2ceil(proxy->notify.offset + proxy->notify.size));
 
-if ((proxy->disable_legacy == ON_OFF_AUTO_ON) ||
-((proxy->disable_legacy == ON_OFF_AUTO_AUTO) && pcie_port)) {
-if (proxy->disable_modern) {
-error_setg(errp, "device cannot work as neither modern nor "
-   "legacy mode is enabled");
-error_append_hint(errp, "Set either disable-modern or "
-  "disable-legacy to off\n");
-return;
-}
-proxy->mode = VIRTIO_PCI_MODE_MODERN;
-} else {
-if (proxy->disable_modern) {
-proxy->mode = VIRTIO_PCI_MODE_LEGACY;
-} else {
-proxy->mode = VIRTIO_PCI_MODE_TRANSITIONAL;
-}
+if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
+proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
+}
+
+if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
+error_setg(errp, "device cannot work as neither modern nor legacy mode"
+   " is enabled");
+error_append_hint(errp, "Set either disable-modern or disable-legacy"
+  " to off\n");
+return;
 }
 
 if (pcie_port && pci_is_express(pci_dev)) {
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 619d9098c1..292275acb1 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -15,7 +15,6 @@
 #ifndef QEMU_VIRTIO_PCI_H
 #define QEMU_VIRTIO_PCI_H
 
-#include "qapi/error.h"
 #include "hw/pci/msi.h"
 #include "hw/virtio/virtio-bus.h"
 
@@ -119,12 

[Qemu-devel] [Bug 1838277] Re: qemu-system-aarch64: regression: msr vbar_el2, xN not working in EL2

[Peter Maydell]

If you can provide a repro case for that I'll have a look...

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1838277

Title:
  qemu-system-aarch64: regression: msr vbar_el2, xN not working in EL2

Status in QEMU:
  New

Bug description:
  Affects 3.1.0 (latest stable release) and latest commit
  (893dc8300c80e3dc32f31e968cf7aa0904da50c3) but did *not* affect 2.11
  (qemu from bionic ubuntu LTS).

  With the following code and shell commands:

  test.s:

  .text
  mov x0, #0x6000
  msr vbar_el2, x0
  dsb sy
  isb sy

  $ aarch64-none-elf-as test.s -o test.o
  $ aarch64-none-elf-objcopy -S -O binary test.o test.bin
  $ qemu-system-aarch64 -nographic -machine virt,virtualization=on -cpu 
cortex-a57 -kernel test.bin -s -S

  vbar_el2 is still 0 after the code, instead of being the expected
  0x6000. (see screenshot).

  This regression doesn't seem to happen for vbar_el1 &
  virtualization=off.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1838277/+subscriptions