Re: [PATCH v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller

[Andrew Jones]

On Sat, Nov 16, 2019 at 12:06:42PM +0100, Richard Henderson wrote:
> Coverity reports, in sve_zcr_get_valid_len,
> 
> "Subtract operation overflows on operands
> arm_cpu_vq_map_next_smaller(cpu, start_vq + 1U) and 1U"
> 
> First, the aarch32 stub version of arm_cpu_vq_map_next_smaller,
> returning 0, does exactly what Coverity reports.  Remove it.
> 
> Second, the aarch64 version of arm_cpu_vq_map_next_smaller has
> a set of asserts, but they don't cover the case in question.
> Further, there is a fair amount of extra arithmetic needed to
> convert from the 0-based zcr register, to the 1-base vq form,
> to the 0-based bitmap, and back again.  This can be simplified
> by leaving the value in the 0-based form.
> 
> Finally, use test_bit to simplify the common case, where the
> length in the zcr registers is in fact a supported length.

I don't see test_bit() getting used in the changes below.

> 
> Reported-by: Coverity (CID 1407217)
> Signed-off-by: Richard Henderson 
> ---
> 
> v2: Merge arm_cpu_vq_map_next_smaller into sve_zcr_get_valid_len,
> as suggested by Andrew Jones.
> 
> Use test_bit to make the code even more obvious; the
> current_length + 1 thing to let us find current_length in the
> bitmap seemed unnecessarily clever.
> 
> ---
>  target/arm/cpu.h|  3 ---
>  target/arm/cpu64.c  | 15 ---
>  target/arm/helper.c |  8 ++--
>  3 files changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index e1a66a2d1c..47d24a5375 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -185,12 +185,9 @@ typedef struct {
>  #ifdef TARGET_AARCH64
>  # define ARM_MAX_VQ16
>  void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
> -uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq);
>  #else
>  # define ARM_MAX_VQ1
>  static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
> -static inline uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq)
> -{ return 0; }
>  #endif
>  
>  typedef struct ARMVectorReg {
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 68baf0482f..a39d6fcea3 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -458,21 +458,6 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>  cpu->sve_max_vq = max_vq;
>  }
>  
> -uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq)
> -{
> -uint32_t bitnum;
> -
> -/*
> - * We allow vq == ARM_MAX_VQ + 1 to be input because the caller may want
> - * to find the maximum vq enabled, which may be ARM_MAX_VQ, but this
> - * function always returns the next smaller than the input.
> - */
> -assert(vq && vq <= ARM_MAX_VQ + 1);
> -
> -bitnum = find_last_bit(cpu->sve_vq_map, vq - 1);
> -return bitnum == vq - 1 ? 0 : bitnum + 1;
> -}
> -
>  static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
>  {
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index be67e2c66d..b5ee35a174 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5363,9 +5363,13 @@ int sve_exception_el(CPUARMState *env, int el)
>  
>  static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
>  {
> -uint32_t start_vq = (start_len & 0xf) + 1;
> +uint32_t end_len;
>  
> -return arm_cpu_vq_map_next_smaller(cpu, start_vq + 1) - 1;
> +start_len &= 0xf;
> +end_len = find_last_bit(cpu->sve_vq_map, start_len + 1);
> +
> +assert(end_len <= start_len);
> +return end_len;
>  }
>  
>  /*
> -- 
> 2.17.1
> 
>

Besides the commit message referencing test_bit, but no use of it, this
looks good to me

Reviewed-by: Andrew Jones 

Re: [PATCH] target/arm: Clean up arm_cpu_vq_map_next_smaller asserts

[Andrew Jones]

On Fri, Nov 15, 2019 at 06:45:51PM +0100, Richard Henderson wrote:
> On 11/15/19 5:06 PM, Andrew Jones wrote:
> >>  bitnum = find_last_bit(cpu->sve_vq_map, vq - 1);
> >> -return bitnum == vq - 1 ? 0 : bitnum + 1;
> >> +
> >> +/* We always have vq == 1 present in sve_vq_map.  */
> > 
> > This is true with TCG and 99.% likely to be true with KVM...
> 
> Eh?  It's required by the spec that 128-bit vectors are always supported.

If some vendor messes things up with SVE in a way that makes it impossible
to configure all should-be-supported lengths, then there's a chance KVM
will simply not advertise the lengths that cannot be configured as a
workaround. This may be quite unlikely, but when KVM is in use, IMO, it
should be the sole authority on what lengths are available. Assuming
lengths are available because the spec says so should work, but then
'hardware' is just another way to spell 'errata'...

> 
> 
> > Maybe we should just remove this function and put the
> > find_last_bit() call and all input/output validation directly in
> > sve_zcr_get_valid_len() ?
> 
> But that makes sense all on its own, so we don't do quite so much +1/-1 
> faffing
> about.
>

Thanks,
drew 

Re: [RFC PATCH 02/11] gdbstub: stop passing GDBState * around

[Richard Henderson]

On 11/15/19 6:29 PM, Alex Bennée wrote:
> We only have one GDBState which should be allocated at the time we
> process any commands. This will make further clean-up a bit easier.
> 
> Signed-off-by: Alex Bennée 
> ---
>  gdbstub.c | 307 +++---
>  1 file changed, 177 insertions(+), 130 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index c5b6701825f..2e6ff5f583c 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1399,7 +1399,6 @@ static int cmd_parse_params(const char *data, const 
> char *schema,
>  }
>  
>  typedef struct GdbCmdContext {
> -GDBState *s;
>  GdbCmdVariant *params;
>  int num_params;
>  uint8_t mem_buf[MAX_PACKET_LENGTH];
> @@ -1480,7 +1479,7 @@ static int process_string_cmd(GDBState *s, void 
> *user_ctx, const char *data,
>  return -1;
>  }
>  
> -gdb_ctx.s = s;
> +g_assert(s == gdbserver_state);
>  cmd->handler(_ctx, user_ctx);
>  return 0;
>  }
> @@ -1505,7 +1504,7 @@ static void run_cmd_parser(GDBState *s, const char 
> *data,
>  static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx)
>  {
>  GDBProcess *process;
> -GDBState *s = gdb_ctx->s;
> +GDBState *s = gdbserver_state;
>  uint32_t pid = 1;
>  
>  if (s->multiprocess) {
[...]

Modulo my question about why not use a non-pointer variable,

Reviewed-by: Richard Henderson 


r~

Re: [PATCH 5/5] aspeed: Add support for the tacoma-bmc board

[Cédric Le Goater]

On 18/11/2019 08:18, Joel Stanley wrote:
> On Thu, 14 Nov 2019 at 09:46, Cédric Le Goater  wrote:
>>
>> The Tacoma BMC board is replacement board for the BMC of the OpenPOWER
>> Witherspoon system. It uses a AST2600 SoC instead of a AST2500 and is
>> used for HW bringup.
>>
>> Signed-off-by: Cédric Le Goater 
>> ---
>>  hw/arm/aspeed.c | 52 +
>>  1 file changed, 52 insertions(+)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index e34e6787430b..729f2e79cd79 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -92,6 +92,10 @@ struct AspeedBoardState {
>>  #define AST2600_EVB_HW_STRAP1 0x00C0
>>  #define AST2600_EVB_HW_STRAP2 0x0003
>>
>> +/* Tacoma hardware value */
>> +#define TACOMA_BMC_HW_STRAP1  0x
>> +#define TACOMA_BMC_HW_STRAP2  0x
>> +
>>  /*
>>   * The max ram region is for firmwares that scan the address space
>>   * with load/store to guess how much RAM the SoC has.
>> @@ -167,6 +171,34 @@ static void aspeed_board_init_flashes(AspeedSMCState 
>> *s, const char *flashtype,
>>  }
>>  }
>>
>> +static void tacoma_bmc_i2c_init(AspeedBoardState *bmc)
> 
> This should be identical to witherspoon. Do you want to use the same callback?

You are right. The tacoma board should use the same callback.

Thanks,

C. 

> Either way,
> 
> Reviewed-by: Joel Stanley 
> 
> 
>> +{
>> +AspeedSoCState *soc = >soc;
>> +uint8_t *eeprom_buf = g_malloc0(8 * 1024);
>> +
>> +/* Bus 3: TODO bmp280@77 */
>> +/* Bus 3: TODO max31785@52 */
>> +/* Bus 3: TODO dps310@76 */
>> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 3), "pca9552", 
>> 0x60);
>> +
>> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 4), "tmp423", 
>> 0x4c);
>> +
>> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 5), "tmp423", 
>> 0x4c);
>> +
>> +/* The tacoma expects a TMP275 but a TMP105 is compatible */
>> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 9), TYPE_TMP105,
>> + 0x4a);
>> +
>> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 11), "pca9552",
>> + 0x60);
>> +/* The tacoma expects Epson RX8900 RTC but a ds1338 is compatible */
>> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 11), "ds1338",
>> + 0x32);
>> +smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(>i2c), 11), 0x51,
>> +  eeprom_buf);
>> +/* Bus 11: TODO ucd90160@64 */
>> +}
>> +
>>  static void aspeed_machine_init(MachineState *machine)
>>  {
>>  AspeedBoardState *bmc;
>> @@ -485,6 +517,22 @@ static void 
>> aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
>>  mc->default_ram_size = 1 * GiB;
>>  };
>>
>> +static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data)
>> +{
>> +MachineClass *mc = MACHINE_CLASS(oc);
>> +AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>> +
>> +mc->desc   = "Aspeed AST2600 EVB (Cortex A7)";
>> +amc->soc_name  = "ast2600-a0";
>> +amc->hw_strap1 = TACOMA_BMC_HW_STRAP1;
>> +amc->hw_strap2 = TACOMA_BMC_HW_STRAP2;
>> +amc->fmc_model = "mx66l1g45g";
>> +amc->spi_model = "mx66l1g45g";
>> +amc->num_cs= 2;
>> +amc->i2c_init  = tacoma_bmc_i2c_init;
>> +mc->default_ram_size = 1 * GiB;
>> +};
>> +
>>  static const TypeInfo aspeed_machine_types[] = {
>>  {
>>  .name  = MACHINE_TYPE_NAME("palmetto-bmc"),
>> @@ -510,6 +558,10 @@ static const TypeInfo aspeed_machine_types[] = {
>>  .name  = MACHINE_TYPE_NAME("ast2600-evb"),
>>  .parent= TYPE_ASPEED_MACHINE,
>>  .class_init= aspeed_machine_ast2600_evb_class_init,
>> +}, {
>> +.name  = MACHINE_TYPE_NAME("tacoma-bmc"),
>> +.parent= TYPE_ASPEED_MACHINE,
>> +.class_init= aspeed_machine_tacoma_class_init,
>>  }, {
>>  .name  = TYPE_ASPEED_MACHINE,
>>  .parent= TYPE_MACHINE,
>> --
>> 2.21.0
>>

Re: [RFC PATCH 01/11] gdbstub: move allocation of GDBState to one place

[Richard Henderson]

On 11/15/19 6:29 PM, Alex Bennée wrote:
>  
>  static GDBState *gdbserver_state;
>  
> +static GDBState *gdb_allocate_state(void)
> +{
> +g_assert(!gdbserver_state);
> +gdbserver_state = g_new0(GDBState, 1);
> +return gdbserver_state;
> +}
> +

Actually, if we're only going to have one, why are we allocating it
dynamically?  We might as well allocate it statically and drop the pointer
indirection.


r~

Re: [RFC PATCH 01/11] gdbstub: move allocation of GDBState to one place

[Richard Henderson]

On 11/15/19 6:29 PM, Alex Bennée wrote:
> We use g_new0() as it is the preferred form for such allocations. We
> can also ensure that gdbserver_state is reset in one place.
> 
> Signed-off-by: Alex Bennée 
> ---
>  gdbstub.c | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [PATCH v16 03/14] util/cutils: refactor do_strtosz() to support suffixes list

[Tao Xu]

On 11/15/2019 8:11 PM, Philippe Mathieu-Daudé wrote:

Cc'ing Markus & Stefan.

On 11/15/19 8:53 AM, Tao Xu wrote:

Add do_strtomul() to convert string according to different suffixes.

Reviewed-by: Eduardo Habkost 
Signed-off-by: Tao Xu 
---

No changes in v16.

Changes in v15:
  - Add a new patch to refactor do_strtosz() (Eduardo)
---
   util/cutils.c | 72 ++-
   1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/util/cutils.c b/util/cutils.c
index d94a468954..ffef92338a 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -181,41 +181,37 @@ int fcntl_setfl(int fd, int flag)
   }
   #endif
   
-static int64_t suffix_mul(char suffix, int64_t unit)

+static int64_t suffix_mul(const char *suffixes[], int num_suffix,
+  const char *endptr, int *offset, int64_t unit)
   {
-switch (qemu_toupper(suffix)) {
-case 'B':
-return 1;
-case 'K':
-return unit;
-case 'M':
-return unit * unit;
-case 'G':
-return unit * unit * unit;
-case 'T':
-return unit * unit * unit * unit;
-case 'P':
-return unit * unit * unit * unit * unit;
-case 'E':
-return unit * unit * unit * unit * unit * unit;
+int i, suffix_len;
+int64_t mul = 1;
+
+for (i = 0; i < num_suffix; i++) {
+suffix_len = strlen(suffixes[i]);
+if (g_ascii_strncasecmp(suffixes[i], endptr, suffix_len) == 0) {
+*offset = suffix_len;


So now we can parse "8kB" and "8Kb", and this might be confusing when
parsing bit units.

https://en.wikipedia.org/wiki/Kilobyte#Definitions_and_usage:

IEC 8-13 standard uses the term 'byte' to mean
eight bits (1 B = 8 bit).

At some point we'll need to add the IEC suffix parsing to this function.

https://en.wikipedia.org/wiki/Kibibyte#Definition

Meanwhile, can you keep it to upper case suffix only?


Here I use g_ascii_strncasecmp() because qemu originally
use qemu_toupper(). This will not cause compatibility issue, because 
qemu use B/b for bytes, K/k for KB, M/m for MB, G/g for GB or T/t for TB 
for a long time.


I am wondering if we can add a new do_strtosz_iec() for upper case 
suffix only.



+return mul;
+}
+mul *= unit;
   }
+
   return -1;
   }
   
   /*

- * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
- * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
- * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
- * other error.
+ * Convert string according to different suffixes. End pointer will be returned
+ * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on other 
error.
*/
-static int do_strtosz(const char *nptr, const char **end,
-  const char default_suffix, int64_t unit,
+static int do_strtomul(const char *nptr, const char **end,
+   const char *suffixes[], int num_suffix,
+   const char *default_suffix, int64_t unit,
 uint64_t *result)
   {
   int retval;
   const char *endptr;
-unsigned char c;
   int mul_required = 0;
+int offset = 0;
   long double val, mul, integral, fraction;
   
   retval = qemu_strtold_finite(nptr, , );

@@ -226,12 +222,12 @@ static int do_strtosz(const char *nptr, const char **end,
   if (fraction != 0) {
   mul_required = 1;
   }
-c = *endptr;
-mul = suffix_mul(c, unit);
+
+mul = suffix_mul(suffixes, num_suffix, endptr, , unit);
   if (mul >= 0) {
-endptr++;
+endptr += offset;
   } else {
-mul = suffix_mul(default_suffix, unit);
+mul = suffix_mul(suffixes, num_suffix, default_suffix, , unit);
   assert(mul >= 0);
   }
   if (mul == 1 && mul_required) {
@@ -256,19 +252,35 @@ out:
   return retval;
   }
   
+/*

+ * Convert string to bytes, allowing either B/b for bytes, K/k for KB,


Then also fix here "B/b for bytes".


+ * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned


Shouldn't we refuse m/g/t? (m is the 'milli' suffix)

Thanks,

Phil.


+ * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
+ * other error.
+ */
+static int do_strtosz(const char *nptr, const char **end,
+  const char *default_suffix, int64_t unit,
+  uint64_t *result)
+{
+static const char *suffixes[] = { "B", "K", "M", "G", "T", "P", "E" };
+
+return do_strtomul(nptr, end, suffixes, ARRAY_SIZE(suffixes),
+   default_suffix, unit, result);
+}
+
   int qemu_strtosz(const char *nptr, const char **end, uint64_t *result)
   {
-return do_strtosz(nptr, end, 'B', 1024, result);
+return do_strtosz(nptr, end, "B", 1024, result);
   }
   
   int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result)

   {
-return do_strtosz(nptr, end, 'M', 1024, result);
+return 

Re: [PATCH 3/5] aspeed/smc: Add AST2600 timings registers

[Joel Stanley]

On Thu, 14 Nov 2019 at 09:46, Cédric Le Goater  wrote:
>
> Each CS has its own Read Timing Compensation Register on newer SoCs.
>
> Signed-off-by: Cédric Le Goater 

Reviewed-by: Joel Stanley 

> ---
>  include/hw/ssi/aspeed_smc.h |  1 +
>  hw/ssi/aspeed_smc.c | 17 ++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
> index 684d16e33613..6fbbb238f158 100644
> --- a/include/hw/ssi/aspeed_smc.h
> +++ b/include/hw/ssi/aspeed_smc.h
> @@ -40,6 +40,7 @@ typedef struct AspeedSMCController {
>  uint8_t r_ce_ctrl;
>  uint8_t r_ctrl0;
>  uint8_t r_timings;
> +uint8_t nregs_timings;
>  uint8_t conf_enable_w0;
>  uint8_t max_slaves;
>  const AspeedSegments *segments;
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 86cadbe4cc00..7755eca34976 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -137,7 +137,7 @@
>  /* Checksum Calculation Result */
>  #define R_DMA_CHECKSUM(0x90 / 4)
>
> -/* Misc Control Register #2 */
> +/* Read Timing Compensation Register */
>  #define R_TIMINGS (0x94 / 4)
>
>  /* SPI controller registers and bits (AST2400) */
> @@ -256,6 +256,7 @@ static const AspeedSMCController controllers[] = {
>  .r_ce_ctrl = R_CE_CTRL,
>  .r_ctrl0   = R_CTRL0,
>  .r_timings = R_TIMINGS,
> +.nregs_timings = 1,
>  .conf_enable_w0= CONF_ENABLE_W0,
>  .max_slaves= 5,
>  .segments  = aspeed_segments_legacy,
> @@ -271,6 +272,7 @@ static const AspeedSMCController controllers[] = {
>  .r_ce_ctrl = R_CE_CTRL,
>  .r_ctrl0   = R_CTRL0,
>  .r_timings = R_TIMINGS,
> +.nregs_timings = 1,
>  .conf_enable_w0= CONF_ENABLE_W0,
>  .max_slaves= 5,
>  .segments  = aspeed_segments_fmc,
> @@ -288,6 +290,7 @@ static const AspeedSMCController controllers[] = {
>  .r_ce_ctrl = 0xff,
>  .r_ctrl0   = R_SPI_CTRL0,
>  .r_timings = R_SPI_TIMINGS,
> +.nregs_timings = 1,
>  .conf_enable_w0= SPI_CONF_ENABLE_W0,
>  .max_slaves= 1,
>  .segments  = aspeed_segments_spi,
> @@ -303,6 +306,7 @@ static const AspeedSMCController controllers[] = {
>  .r_ce_ctrl = R_CE_CTRL,
>  .r_ctrl0   = R_CTRL0,
>  .r_timings = R_TIMINGS,
> +.nregs_timings = 1,
>  .conf_enable_w0= CONF_ENABLE_W0,
>  .max_slaves= 3,
>  .segments  = aspeed_segments_ast2500_fmc,
> @@ -320,6 +324,7 @@ static const AspeedSMCController controllers[] = {
>  .r_ce_ctrl = R_CE_CTRL,
>  .r_ctrl0   = R_CTRL0,
>  .r_timings = R_TIMINGS,
> +.nregs_timings = 1,
>  .conf_enable_w0= CONF_ENABLE_W0,
>  .max_slaves= 2,
>  .segments  = aspeed_segments_ast2500_spi1,
> @@ -335,6 +340,7 @@ static const AspeedSMCController controllers[] = {
>  .r_ce_ctrl = R_CE_CTRL,
>  .r_ctrl0   = R_CTRL0,
>  .r_timings = R_TIMINGS,
> +.nregs_timings = 1,
>  .conf_enable_w0= CONF_ENABLE_W0,
>  .max_slaves= 2,
>  .segments  = aspeed_segments_ast2500_spi2,
> @@ -350,6 +356,7 @@ static const AspeedSMCController controllers[] = {
>  .r_ce_ctrl = R_CE_CTRL,
>  .r_ctrl0   = R_CTRL0,
>  .r_timings = R_TIMINGS,
> +.nregs_timings = 1,
>  .conf_enable_w0= CONF_ENABLE_W0,
>  .max_slaves= 3,
>  .segments  = aspeed_segments_ast2600_fmc,
> @@ -365,6 +372,7 @@ static const AspeedSMCController controllers[] = {
>  .r_ce_ctrl = R_CE_CTRL,
>  .r_ctrl0   = R_CTRL0,
>  .r_timings = R_TIMINGS,
> +.nregs_timings = 2,
>  .conf_enable_w0= CONF_ENABLE_W0,
>  .max_slaves= 2,
>  .segments  = aspeed_segments_ast2600_spi1,
> @@ -380,6 +388,7 @@ static const AspeedSMCController controllers[] = {
>  .r_ce_ctrl = R_CE_CTRL,
>  .r_ctrl0   = R_CTRL0,
>  .r_timings = R_TIMINGS,
> +.nregs_timings = 3,
>  .conf_enable_w0= CONF_ENABLE_W0,
>  .max_slaves= 3,
>  .segments  = aspeed_segments_ast2600_spi2,
> @@ -951,7 +960,8 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr 
> addr, unsigned int size)
>  addr >>= 2;
>
>  if (addr == s->r_conf ||
> -addr == s->r_timings ||
> +(addr >= s->r_timings &&
> + addr < s->r_timings + s->ctrl->nregs_timings) ||
>  addr == s->r_ce_ctrl ||
>  addr == R_INTR_CTRL ||
>  addr == 

Re: [PATCH 5/5] aspeed: Add support for the tacoma-bmc board

[Joel Stanley]

On Thu, 14 Nov 2019 at 09:46, Cédric Le Goater  wrote:
>
> The Tacoma BMC board is replacement board for the BMC of the OpenPOWER
> Witherspoon system. It uses a AST2600 SoC instead of a AST2500 and is
> used for HW bringup.
>
> Signed-off-by: Cédric Le Goater 
> ---
>  hw/arm/aspeed.c | 52 +
>  1 file changed, 52 insertions(+)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index e34e6787430b..729f2e79cd79 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -92,6 +92,10 @@ struct AspeedBoardState {
>  #define AST2600_EVB_HW_STRAP1 0x00C0
>  #define AST2600_EVB_HW_STRAP2 0x0003
>
> +/* Tacoma hardware value */
> +#define TACOMA_BMC_HW_STRAP1  0x
> +#define TACOMA_BMC_HW_STRAP2  0x
> +
>  /*
>   * The max ram region is for firmwares that scan the address space
>   * with load/store to guess how much RAM the SoC has.
> @@ -167,6 +171,34 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, 
> const char *flashtype,
>  }
>  }
>
> +static void tacoma_bmc_i2c_init(AspeedBoardState *bmc)

This should be identical to witherspoon. Do you want to use the same callback?

Either way,

Reviewed-by: Joel Stanley 


> +{
> +AspeedSoCState *soc = >soc;
> +uint8_t *eeprom_buf = g_malloc0(8 * 1024);
> +
> +/* Bus 3: TODO bmp280@77 */
> +/* Bus 3: TODO max31785@52 */
> +/* Bus 3: TODO dps310@76 */
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 3), "pca9552", 
> 0x60);
> +
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 4), "tmp423", 
> 0x4c);
> +
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 5), "tmp423", 
> 0x4c);
> +
> +/* The tacoma expects a TMP275 but a TMP105 is compatible */
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 9), TYPE_TMP105,
> + 0x4a);
> +
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 11), "pca9552",
> + 0x60);
> +/* The tacoma expects Epson RX8900 RTC but a ds1338 is compatible */
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 11), "ds1338",
> + 0x32);
> +smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(>i2c), 11), 0x51,
> +  eeprom_buf);
> +/* Bus 11: TODO ucd90160@64 */
> +}
> +
>  static void aspeed_machine_init(MachineState *machine)
>  {
>  AspeedBoardState *bmc;
> @@ -485,6 +517,22 @@ static void 
> aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
>  mc->default_ram_size = 1 * GiB;
>  };
>
> +static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data)
> +{
> +MachineClass *mc = MACHINE_CLASS(oc);
> +AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> +
> +mc->desc   = "Aspeed AST2600 EVB (Cortex A7)";
> +amc->soc_name  = "ast2600-a0";
> +amc->hw_strap1 = TACOMA_BMC_HW_STRAP1;
> +amc->hw_strap2 = TACOMA_BMC_HW_STRAP2;
> +amc->fmc_model = "mx66l1g45g";
> +amc->spi_model = "mx66l1g45g";
> +amc->num_cs= 2;
> +amc->i2c_init  = tacoma_bmc_i2c_init;
> +mc->default_ram_size = 1 * GiB;
> +};
> +
>  static const TypeInfo aspeed_machine_types[] = {
>  {
>  .name  = MACHINE_TYPE_NAME("palmetto-bmc"),
> @@ -510,6 +558,10 @@ static const TypeInfo aspeed_machine_types[] = {
>  .name  = MACHINE_TYPE_NAME("ast2600-evb"),
>  .parent= TYPE_ASPEED_MACHINE,
>  .class_init= aspeed_machine_ast2600_evb_class_init,
> +}, {
> +.name  = MACHINE_TYPE_NAME("tacoma-bmc"),
> +.parent= TYPE_ASPEED_MACHINE,
> +.class_init= aspeed_machine_tacoma_class_init,
>  }, {
>  .name  = TYPE_ASPEED_MACHINE,
>  .parent= TYPE_MACHINE,
> --
> 2.21.0
>

Re: [PATCH 4/5] aspeed: Remove AspeedBoardConfig array and use AspeedMachineClass

[Joel Stanley]

On Thu, 14 Nov 2019 at 09:46, Cédric Le Goater  wrote:
>
> AspeedBoardConfig is a redundant way to define class attributes and it
> complexifies the machine definition and initialization.
>
> Signed-off-by: Cédric Le Goater 

Reviewed-by: Joel Stanley 

A good cleanup.

> ---
>  include/hw/arm/aspeed.h |  24 ++---
>  hw/arm/aspeed.c | 231 ++--
>  2 files changed, 137 insertions(+), 118 deletions(-)
>
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> index f49bc7081e4d..4423cd0cda71 100644
> --- a/include/hw/arm/aspeed.h
> +++ b/include/hw/arm/aspeed.h
> @@ -13,19 +13,6 @@
>
>  typedef struct AspeedBoardState AspeedBoardState;
>
> -typedef struct AspeedBoardConfig {
> -const char *name;
> -const char *desc;
> -const char *soc_name;
> -uint32_t hw_strap1;
> -uint32_t hw_strap2;
> -const char *fmc_model;
> -const char *spi_model;
> -uint32_t num_cs;
> -void (*i2c_init)(AspeedBoardState *bmc);
> -uint32_t ram;
> -} AspeedBoardConfig;
> -
>  #define TYPE_ASPEED_MACHINE   MACHINE_TYPE_NAME("aspeed")
>  #define ASPEED_MACHINE(obj) \
>  OBJECT_CHECK(AspeedMachine, (obj), TYPE_ASPEED_MACHINE)
> @@ -41,7 +28,16 @@ typedef struct AspeedMachine {
>
>  typedef struct AspeedMachineClass {
>  MachineClass parent_obj;
> -const AspeedBoardConfig *board;
> +
> +const char *name;
> +const char *desc;
> +const char *soc_name;
> +uint32_t hw_strap1;
> +uint32_t hw_strap2;
> +const char *fmc_model;
> +const char *spi_model;
> +uint32_t num_cs;
> +void (*i2c_init)(AspeedBoardState *bmc);
>  } AspeedMachineClass;
>
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 028191ff36fc..e34e6787430b 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -167,10 +167,10 @@ static void aspeed_board_init_flashes(AspeedSMCState 
> *s, const char *flashtype,
>  }
>  }
>
> -static void aspeed_board_init(MachineState *machine,
> -  const AspeedBoardConfig *cfg)
> +static void aspeed_machine_init(MachineState *machine)
>  {
>  AspeedBoardState *bmc;
> +AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
>  AspeedSoCClass *sc;
>  DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
>  ram_addr_t max_ram_size;
> @@ -182,18 +182,18 @@ static void aspeed_board_init(MachineState *machine,
> UINT32_MAX);
>
>  object_initialize_child(OBJECT(machine), "soc", >soc,
> -(sizeof(bmc->soc)), cfg->soc_name, _abort,
> +(sizeof(bmc->soc)), amc->soc_name, _abort,
>  NULL);
>
>  sc = ASPEED_SOC_GET_CLASS(>soc);
>
>  object_property_set_uint(OBJECT(>soc), ram_size, "ram-size",
>   _abort);
> -object_property_set_int(OBJECT(>soc), cfg->hw_strap1, "hw-strap1",
> +object_property_set_int(OBJECT(>soc), amc->hw_strap1, "hw-strap1",
>  _abort);
> -object_property_set_int(OBJECT(>soc), cfg->hw_strap2, "hw-strap2",
> +object_property_set_int(OBJECT(>soc), amc->hw_strap2, "hw-strap2",
>  _abort);
> -object_property_set_int(OBJECT(>soc), cfg->num_cs, "num-cs",
> +object_property_set_int(OBJECT(>soc), amc->num_cs, "num-cs",
>  _abort);
>  object_property_set_int(OBJECT(>soc), machine->smp.cpus, "num-cpus",
>  _abort);
> @@ -230,8 +230,8 @@ static void aspeed_board_init(MachineState *machine,
>"max_ram", max_ram_size  - ram_size);
>  memory_region_add_subregion(>ram_container, ram_size, 
> >max_ram);
>
> -aspeed_board_init_flashes(>soc.fmc, cfg->fmc_model, _abort);
> -aspeed_board_init_flashes(>soc.spi[0], cfg->spi_model, 
> _abort);
> +aspeed_board_init_flashes(>soc.fmc, amc->fmc_model, _abort);
> +aspeed_board_init_flashes(>soc.spi[0], amc->spi_model, 
> _abort);
>
>  /* Install first FMC flash content as a boot rom. */
>  if (drive0) {
> @@ -255,8 +255,8 @@ static void aspeed_board_init(MachineState *machine,
>  aspeed_board_binfo.loader_start = sc->memmap[ASPEED_SDRAM];
>  aspeed_board_binfo.nb_cpus = bmc->soc.num_cpus;
>
> -if (cfg->i2c_init) {
> -cfg->i2c_init(bmc);
> +if (amc->i2c_init) {
> +amc->i2c_init(bmc);
>  }
>
>  for (i = 0; i < ARRAY_SIZE(bmc->soc.sdhci.slots); i++) {
> @@ -383,118 +383,141 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState 
> *bmc)
>   0x60);
>  }
>
> -static void aspeed_machine_init(MachineState *machine)
> -{
> -AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
> -
> -aspeed_board_init(machine, amc->board);
> -}
> -
>  static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>  {
>  MachineClass *mc = MACHINE_CLASS(oc);
> -AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> -const 

Re: [PATCH 2/5] aspeed/smc: Do not map disabled segment on the AST2600

[Joel Stanley]

On Thu, 14 Nov 2019 at 09:46, Cédric Le Goater  wrote:
>
> The segments can be disabled on the AST2600 (zero register value).
> CS0 is open by default but not the other CS. This is closing the
> access to the flash device in user mode and forbids scanning.
>
> In the model, check the segment size and disable the associated region
> when the value is zero.
>
> Fixes: bcaa8ddd081c ("aspeed/smc: Add AST2600 support")
> Signed-off-by: Cédric Le Goater 

Reviewed-by: Joel Stanley 

> ---
>  hw/ssi/aspeed_smc.c | 16 +++-
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 955ec21852ac..86cadbe4cc00 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -444,8 +444,13 @@ static void aspeed_2600_smc_reg_to_segment(const 
> AspeedSMCState *s,
>  uint32_t start_offset = (reg << 16) & AST2600_SEG_ADDR_MASK;
>  uint32_t end_offset = reg & AST2600_SEG_ADDR_MASK;
>
> -seg->addr = s->ctrl->flash_window_base + start_offset;
> -seg->size = end_offset + MiB - start_offset;
> +if (reg) {
> +seg->addr = s->ctrl->flash_window_base + start_offset;
> +seg->size = end_offset + MiB - start_offset;
> +} else {
> +seg->addr = s->ctrl->flash_window_base;
> +seg->size = 0;
> +}
>  }
>
>  static bool aspeed_smc_flash_overlap(const AspeedSMCState *s,
> @@ -486,7 +491,7 @@ static void 
> aspeed_smc_flash_set_segment_region(AspeedSMCState *s, int cs,
>  memory_region_transaction_begin();
>  memory_region_set_size(>mmio, seg.size);
>  memory_region_set_address(>mmio, seg.addr - 
> s->ctrl->flash_window_base);
> -memory_region_set_enabled(>mmio, true);
> +memory_region_set_enabled(>mmio, !!seg.size);
>  memory_region_transaction_commit();
>
>  s->regs[R_SEG_ADDR0 + cs] = regval;
> @@ -526,8 +531,9 @@ static void aspeed_smc_flash_set_segment(AspeedSMCState 
> *s, int cs,
>  }
>
>  /* Keep the segment in the overall flash window */
> -if (seg.addr + seg.size <= s->ctrl->flash_window_base ||
> -seg.addr > s->ctrl->flash_window_base + s->ctrl->flash_window_size) {
> +if (seg.size &&
> +(seg.addr + seg.size <= s->ctrl->flash_window_base ||
> + seg.addr > s->ctrl->flash_window_base + 
> s->ctrl->flash_window_size)) {
>  qemu_log_mask(LOG_GUEST_ERROR, "%s: new segment for CS%d is invalid 
> : "
>"[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]\n",
>s->ctrl->name, cs, seg.addr, seg.addr + seg.size);
> --
> 2.21.0
>

Re: [PATCH 1/5] aspeed/smc: Restore default AHB window mapping at reset

[Joel Stanley]

On Thu, 14 Nov 2019 at 09:46, Cédric Le Goater  wrote:
>
> The current model only restores the Segment Register values but leaves
> the previous CS mapping behind. Introduce a helper setting the
> register value and mapping the region at the requested address. Use
> this helper when a Segment register is set and at reset.
>
> Signed-off-by: Cédric Le Goater 

Reviewed-by: Joel Stanley 

> ---
>  hw/ssi/aspeed_smc.c | 32 +---
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index f0c7bbbad302..955ec21852ac 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -475,10 +475,26 @@ static bool aspeed_smc_flash_overlap(const 
> AspeedSMCState *s,
>  return false;
>  }
>
> +static void aspeed_smc_flash_set_segment_region(AspeedSMCState *s, int cs,
> +uint64_t regval)
> +{
> +AspeedSMCFlash *fl = >flashes[cs];
> +AspeedSegments seg;
> +
> +s->ctrl->reg_to_segment(s, regval, );
> +
> +memory_region_transaction_begin();
> +memory_region_set_size(>mmio, seg.size);
> +memory_region_set_address(>mmio, seg.addr - 
> s->ctrl->flash_window_base);
> +memory_region_set_enabled(>mmio, true);
> +memory_region_transaction_commit();
> +
> +s->regs[R_SEG_ADDR0 + cs] = regval;
> +}
> +
>  static void aspeed_smc_flash_set_segment(AspeedSMCState *s, int cs,
>   uint64_t new)
>  {
> -AspeedSMCFlash *fl = >flashes[cs];
>  AspeedSegments seg;
>
>  s->ctrl->reg_to_segment(s, new, );
> @@ -529,13 +545,7 @@ static void aspeed_smc_flash_set_segment(AspeedSMCState 
> *s, int cs,
>  aspeed_smc_flash_overlap(s, , cs);
>
>  /* All should be fine now to move the region */
> -memory_region_transaction_begin();
> -memory_region_set_size(>mmio, seg.size);
> -memory_region_set_address(>mmio, seg.addr - 
> s->ctrl->flash_window_base);
> -memory_region_set_enabled(>mmio, true);
> -memory_region_transaction_commit();
> -
> -s->regs[R_SEG_ADDR0 + cs] = new;
> +aspeed_smc_flash_set_segment_region(s, cs, new);
>  }
>
>  static uint64_t aspeed_smc_flash_default_read(void *opaque, hwaddr addr,
> @@ -897,10 +907,10 @@ static void aspeed_smc_reset(DeviceState *d)
>  qemu_set_irq(s->cs_lines[i], true);
>  }
>
> -/* setup default segment register values for all */
> +/* setup the default segment register values and regions for all */
>  for (i = 0; i < s->ctrl->max_slaves; ++i) {
> -s->regs[R_SEG_ADDR0 + i] =
> -s->ctrl->segment_to_reg(s, >ctrl->segments[i]);
> +aspeed_smc_flash_set_segment_region(s, i,
> +s->ctrl->segment_to_reg(s, >ctrl->segments[i]));
>  }
>
>  /* HW strapping flash type for the AST2600 controllers  */
> --
> 2.21.0
>

Re: [RFC v2 00/14] Add SDEI support for arm64

[Guoheyi]

Hi Peter,

Could you spare some time to review the framework and provide comments 
and advice?


Thanks,

HG


On 2019/11/5 17:10, Heyi Guo wrote:

SDEI is for ARM "Software Delegated Exception Interface". AS ARM64 doesn't have
native non-maskable interrupt (NMI), we rely on higher privileged (larger
exception level) software to change the execution flow of lower privileged
(smaller exception level) software when certain events occur, to emulate NMI
mechanism, and SDEI is the standard interfaces between the two levels of
privileged software. It is based on SMC/HVC calls.

The higher privileged software implements an SDEI dispatcher to handle SDEI
related SMC/HVC calls and trigger SDEI events; the lower privileged software
implements an SDEI client to request SDEI services and handle SDEI events.

Core interfaces provided by SDEI include:

1. interrupt bind: client can request to bind an interrupt to an SDEI event, so
the interrupt will be a non-maskable event and the event number will be returned
to the caller. Only PPI and SPI can be bound to SDEI events.

2. register: client can request to register a handler to an SDEI event, so
dispatcher will change PC of lower privileged software to this handler when
certain event occurs.

3. complete: client notifies dispatcher that it has completed the event
handling, so dispatcher will restore the context of guest when it is
interrupted.

In virtualization situation, guest OS is the lower privileged software and
hypervisor is the higher one.

KVM is supposed to pass SMC/HVC calls to qemu, and qemu will emulate an SDEI
dispatcher to serve the SDEI requests and trigger the events. If an interrupt is
requested to be bound to an event, qemu should not inject the interrupt to guest
any more; instead, it should save the context of VCPU and change the PC to event
handler which is registered by guest, and then return to guest.

To make the conversion of interrupt to SDEI event transparent to other modules
in qemu, we used qemu_irq and qemu_irq_intercept_in() to override the default
irq handler with SDEI event trigger. I saw qemu_irq_intercept_in() should be
only used in qemu MST, but it seemed fit to override interrupt injection with
event trigger after guest requests to bind interrupt to SDEI event.

This patchset is trying to implement the whole SDEI framework in qemu with KVM
enabled, including all SDEI v1.0 interfaces, as well as event trigger conduit
from other qemu devices after interrupt binding.

Key points:
- We propose to only support kvm enabled arm64 virtual machines, for
   non-kvm VMs can emulate EL3 and have Trusted Firmware run on it,
   which has a builtin SDEI dispatcher.
- New kvm capability KVM_CAP_FORWARD_HYPERCALL is added to probe if
   kvm supports forwarding hypercalls, and the capability should be
   enabled explicitly.
- We make the dispatcher as a logical device, to save the states
   during migration or save/restore operation; only one instance is
   allowed in one VM.
- We use qemu_irq as the bridge for other qemu modules to switch from
   irq injection to SDEI event trigger after VM binds the interrupt to
   SDEI event. We use qemu_irq_intercept_in() to override qemu_irq
   handler with SDEI event trigger, and a new interface
   qemu_irq_remove_intercept() is added to restore the handler to
   default one (i.e. ARM GIC).

More details are in the commit message of each patch.

Basic tests are done by emulating a watchdog timer and triggering SDEI
event in every 10s.

Please focus on the interfaces and framework first. We can refine the code for
several rounds after the big things have been determined.

Any comment or suggestion is welcome.

Thanks,

HG

Cc: Peter Maydell 
Cc: Dave Martin 
Cc: Marc Zyngier 
Cc: Mark Rutland 
Cc: James Morse 
Cc: "Michael S. Tsirkin" 
Cc: Cornelia Huck 
Cc: Paolo Bonzini 
Cc: Shannon Zhao 
Cc: Igor Mammedov 

v2:
- Import import linux/arm_sdei.h to standard-headers
- Drop SDEI table definition and add comments
- Some bugfix and code refinement

Heyi Guo (14):
   update-linux-headers.sh: import linux/arm_sdei.h to standard-headers
   standard-headers: import arm_sdei.h
   arm/sdei: add virtual device framework
   arm: add CONFIG_SDEI build flag
   arm/sdei: add support to handle SDEI requests from guest
   arm/sdei: add system reset callback
   arm/sdei: add support to trigger event by GIC interrupt ID
   core/irq: add qemu_irq_remove_intercept interface
   arm/sdei: override qemu_irq handler when binding interrupt
   arm/sdei: add support to register interrupt bind notifier
   linux-headers/kvm.h: add capability to forward hypercall
   arm/sdei: add stub to fix build failure when SDEI is not enabled
   arm/kvm: handle guest exit of hypercall
   virt/acpi: add SDEI table if SDEI is enabled

  default-configs/arm-softmmu.mak   |1 +
  hw/arm/Kconfig|4 +
  hw/arm/virt-acpi-build.c  |   26 +
  hw/core/irq.c |   11 +
  

Re: [RFC v2 14/14] virt/acpi: add SDEI table if SDEI is enabled

[Guoheyi]

On 2019/11/12 22:52, Igor Mammedov wrote:

On Tue, 5 Nov 2019 17:10:56 +0800
Heyi Guo  wrote:


Add SDEI table if SDEI is enabled, so that guest OS can get aware and
utilize the interfaces.

Signed-off-by: Heyi Guo 
Cc: Peter Maydell 
Cc: Dave Martin 
Cc: Marc Zyngier 
Cc: Mark Rutland 
Cc: James Morse 
Cc: Shannon Zhao 
Cc: "Michael S. Tsirkin" 
Cc: Igor Mammedov 
---

Notes:
 v2:
 - Drop SDEI table definition and add comments

  hw/arm/virt-acpi-build.c | 26 ++
  1 file changed, 26 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4cd50175e0..73d3f8cd15 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -32,6 +32,7 @@
  #include "trace.h"
  #include "hw/core/cpu.h"
  #include "target/arm/cpu.h"
+#include "target/arm/sdei.h"
  #include "hw/acpi/acpi-defs.h"
  #include "hw/acpi/acpi.h"
  #include "hw/nvram/fw_cfg.h"
@@ -475,6 +476,26 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
VirtMachineState *vms)
   "IORT", table_data->len - iort_start, 0, NULL, NULL);
  }
  
+/*

+ * ACPI spec 6.2 Software Delegated Exception Interface (SDEI).
+ * (Revision 1.0)
+ * "SDEI" was reserved in ACPI 6.2. See "Links to ACPI-Related Documents"
+ * (http://uefi.org/acpi) under the heading "Software
+ * Delegated Exceptions Interface." The definition is under
+ * "10 Appendix C: ACPI table definitions for SDEI" in the linked document.
+ *
+ * This is a dummy table to expose platform SDEI capbility to OS.
+ */
+static void
+build_sdei(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
+{
+int sdei_start = table_data->len;
+
+(void)acpi_data_push(table_data, sizeof(AcpiTableHeader));
+build_header(linker, table_data, (void *)(table_data->data + sdei_start),
+ "SDEI", table_data->len - sdei_start, 1, NULL, NULL);
+}
+
  static void
  build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
  {
@@ -825,6 +846,11 @@ void virt_acpi_build(VirtMachineState *vms, 
AcpiBuildTables *tables)
  acpi_add_table(table_offsets, tables_blob);
  build_spcr(tables_blob, tables->linker, vms);
  
+if (sdei_enabled) {

globals shouldn't be introduced in new code
OK. For this feature depends on KVM, does it make sense to add a flag to 
struct VirtMachineState?


Thanks,
HG



+acpi_add_table(table_offsets, tables_blob);
+build_sdei(tables_blob, tables->linker, vms);
+}
+
  if (ms->numa_state->num_nodes > 0) {
  acpi_add_table(table_offsets, tables_blob);
  build_srat(tables_blob, tables->linker, vms);


.

[PATCH v2] Implement backend program convention command for vhost-user-blk

[Micky Yun Chan]

From: michan 

Signed-off-by: Micky Yun Chan (michiboo) 
---
 contrib/vhost-user-blk/vhost-user-blk.c | 102 ++--
 1 file changed, 58 insertions(+), 44 deletions(-)

diff --git a/contrib/vhost-user-blk/vhost-user-blk.c 
b/contrib/vhost-user-blk/vhost-user-blk.c
index ae61034656..8759b6a5d0 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -576,70 +576,84 @@ vub_new(char *blk_file)
 return vdev_blk;
 }
 
+static int opt_fdnum = -1;
+static char *opt_socket_path;
+static char *opt_blk_file;
+static gboolean opt_print_caps;
+static gboolean opt_read_only;
+
+
+static GOptionEntry entries[] = {
+{ "print-capabilities", 'c', 0, G_OPTION_ARG_NONE, _print_caps,
+  "Print capabilities", NULL },
+{ "fd", 'f', 0, G_OPTION_ARG_INT, _fdnum,
+  "Use inherited fd socket", "FDNUM" },
+{ "socket-path", 's', 0, G_OPTION_ARG_FILENAME, _socket_path,
+  "Use UNIX socket path", "PATH" },
+{"blk-file", 'b', 0, G_OPTION_ARG_FILENAME, _blk_file,
+ "block device or file path", "PATH"},
+{ "read-only", 'r', 0, G_OPTION_ARG_NONE, _read_only,
+  "Enable read-only", NULL }
+};
+
 int main(int argc, char **argv)
 {
-int opt;
-char *unix_socket = NULL;
-char *blk_file = NULL;
-bool enable_ro = false;
 int lsock = -1, csock = -1;
 VubDev *vdev_blk = NULL;
+GError *error = NULL;
+GOptionContext *context;
 
-while ((opt = getopt(argc, argv, "b:rs:h")) != -1) {
-switch (opt) {
-case 'b':
-blk_file = g_strdup(optarg);
-break;
-case 's':
-unix_socket = g_strdup(optarg);
-break;
-case 'r':
-enable_ro = true;
-break;
-case 'h':
-default:
-printf("Usage: %s [ -b block device or file, -s UNIX domain socket"
-   " | -r Enable read-only ] | [ -h ]\n", argv[0]);
-return 0;
+context = g_option_context_new(NULL);
+g_option_context_add_main_entries(context, entries, NULL);
+if (!g_option_context_parse(context, , , )) {
+g_printerr("Option parsing failed: %s\n", error->message);
+exit(EXIT_FAILURE);
+}
+if (opt_print_caps) {
+g_option_context_get_help(context, true, NULL);
+exit(EXIT_SUCCESS);
+}
+
+if (!opt_blk_file) {
+g_option_context_get_help(context, true, NULL);
+exit(EXIT_FAILURE);
+}
+
+if (opt_socket_path) {
+lsock = unix_sock_new(opt_socket_path);
+if (lsock < 0) {
+   exit(EXIT_FAILURE);
 }
+} else if(opt_fdnum < 0){
+g_option_context_get_help(context, true, NULL);
+} else {
+lsock = opt_fdnum;
 }
 
-if (!unix_socket || !blk_file) {
-printf("Usage: %s [ -b block device or file, -s UNIX domain socket"
-   " | -r Enable read-only ] | [ -h ]\n", argv[0]);
-return -1;
-}
-
-lsock = unix_sock_new(unix_socket);
-if (lsock < 0) {
-goto err;
-}
-
-csock = accept(lsock, (void *)0, (void *)0);
+csock = accept(lsock, NULL, NULL);
 if (csock < 0) {
-fprintf(stderr, "Accept error %s\n", strerror(errno));
-goto err;
+g_printerr("Accept error %s\n", strerror(errno));
+exit(EXIT_FAILURE);
 }
 
-vdev_blk = vub_new(blk_file);
+vdev_blk = vub_new(opt_blk_file);
 if (!vdev_blk) {
-goto err;
+exit(EXIT_FAILURE);
 }
-if (enable_ro) {
+if (opt_read_only) {
 vdev_blk->enable_ro = true;
 }
 
 if (!vug_init(_blk->parent, VHOST_USER_BLK_MAX_QUEUES, csock,
   vub_panic_cb, _iface)) {
-fprintf(stderr, "Failed to initialized libvhost-user-glib\n");
-goto err;
+g_printerr("Failed to initialized libvhost-user-glib\n");
+exit(EXIT_FAILURE);
 }
 
 g_main_loop_run(vdev_blk->loop);
-
+g_main_loop_unref(vdev_blk->loop);
+g_option_context_free(context);
 vug_deinit(_blk->parent);
-
-err:
 vub_free(vdev_blk);
 if (csock >= 0) {
 close(csock);
@@ -647,8 +661,8 @@ err:
 if (lsock >= 0) {
 close(lsock);
 }
-g_free(unix_socket);
-g_free(blk_file);
+g_free(opt_socket_path);
+g_free(opt_blk_file);
 
 return 0;
 }
-- 
2.21.0

[PATCH] misc/pca9552: Add qom set and get

[Joel Stanley]

Following the pattern of the work recently done with the ASPEED GPIO
model, this adds support for inspecting and modifying the PCA9552 LEDs
from the monitor.

 (qemu) qom-set  /machine/unattached/device[17] led0 on
 (qemu) qom-get  /machine/unattached/device[17] led0
 "on"

 (qemu) qom-set  /machine/unattached/device[17] led0 off
 (qemu) qom-get  /machine/unattached/device[17] led0
 "off"

 (qemu) qom-set  /machine/unattached/device[17] led0 pwm0
 (qemu) qom-get  /machine/unattached/device[17] led0
 "pwm0"

 (qemu) qom-set  /machine/unattached/device[17] led0 pwm1
 (qemu) qom-get  /machine/unattached/device[17] led0
 "pwm1"

Signed-off-by: Joel Stanley 
---
The qom device in mainline qemu is a different path. Using the monitor
examine `info qom-tree /machine/unattached/` to discover it.

This can be tested with a Witherspoon image.

$ wget 
https://openpower.xyz/job/openbmc-build/distro=ubuntu,label=builder,target=witherspoon/lastSuccessfulBuild/artifact/deploy/images/witherspoon/obmc-phosphor-image-witherspoon.ubi.mtd

$ qemu-system-arm -M witherspoon-bmc -serial pty -monitor pty -nographic \
 -drive file=obmc-phosphor-image-witherspoon.ubi.mtd,format=raw,if=mtd
char device redirected to /dev/pts/5 (label compat_monitor0)
char device redirected to /dev/pts/10 (label serial0)

$ screen /dev/pts/5
QEMU 4.1.91 monitor - type 'help' for more information
(qemu) qom-get  /machine/unattached/device[17] led0
"off"

$ screen /dev/pts/19
root@witherspoon:~# cd /sys/class/gpio/
root@witherspoon:/sys/class/gpio# echo 248 > export
root@witherspoon:/sys/class/gpio# cat gpio248/value
0

(qemu) qom-set  /machine/unattached/device[17] led0 on

root@witherspoon:/sys/class/gpio# echo out > gpio248/direction
root@witherspoon:/sys/class/gpio# cat gpio248/value
1

(qemu) qom-get  /machine/unattached/device[17] led0
"on"

(qemu) qom-set  /machine/unattached/device[17] led0 off
(qemu) qom-get  /machine/unattached/device[17] led0
"off"

root@witherspoon:/sys/class/gpio# cat gpio248/value
0

Signed-off-by: Joel Stanley 
---
 hw/misc/pca9552.c | 91 +++
 1 file changed, 91 insertions(+)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 73be28d9369c..0362aac8c862 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -15,12 +15,16 @@
 #include "hw/misc/pca9552.h"
 #include "hw/misc/pca9552_regs.h"
 #include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
 
 #define PCA9552_LED_ON   0x0
 #define PCA9552_LED_OFF  0x1
 #define PCA9552_LED_PWM0 0x2
 #define PCA9552_LED_PWM1 0x3
 
+static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
+
 static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
 {
 uint8_t reg   = PCA9552_LS0 + (pin / 4);
@@ -169,6 +173,84 @@ static int pca9552_event(I2CSlave *i2c, enum i2c_event 
event)
 return 0;
 }
 
+static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+PCA9552State *s = PCA9552(obj);
+int led, rc, reg;
+char *str;
+uint8_t state;
+
+rc = sscanf(name, "led%2d", );
+if (rc != 1) {
+error_setg(errp, "%s: error reading %s", __func__, name);
+return;
+}
+if (led < 0 || led > s->nr_leds) {
+error_setg(errp, "%s invalid led %s", __func__, name);
+return;
+}
+/*
+ * Get the LSx register as the qom interface should expose the device
+ * state, not the modeled 'input line' behaviour which would come from
+ * reading the INPUTx reg
+ */
+reg = PCA9552_LS0 + led / 4;
+state = (pca9552_read(s, reg) >> (led % 8)) & 0x3;
+str = g_strdup(led_state[state]);
+visit_type_str(v, name, , errp);
+}
+
+/*
+ * Return an LED selector register value based on an existing one, with
+ * the appropriate 2-bit state value set for the given LED number (0-3).
+ */
+static inline uint8_t pca955x_ledsel(uint8_t oldval, int led_num, int state)
+{
+return (oldval & (~(0x3 << (led_num << 1 |
+((state & 0x3) << (led_num << 1));
+}
+
+static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+PCA9552State *s = PCA9552(obj);
+Error *local_err = NULL;
+int led, rc, reg, val;
+uint8_t state;
+char *state_str;
+
+visit_type_str(v, name, _str, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+rc = sscanf(name, "led%2d", );
+if (rc != 1) {
+error_setg(errp, "%s: error reading %s", __func__, name);
+return;
+}
+if (led < 0 || led > s->nr_leds) {
+error_setg(errp, "%s invalid led %s", __func__, name);
+return;
+}
+
+for (state = 0; state < ARRAY_SIZE(led_state); state++) {
+if (!strcmp(state_str, led_state[state])) {
+break;
+}
+}
+if (state >= ARRAY_SIZE(led_state)) {
+error_setg(errp, "%s invalid 

[PATCH v8 1/3] block: introduce compress filter driver

[Andrey Shinkevich]

Allow writing all the data compressed through the filter driver.
The written data will be aligned by the cluster size.
Based on the QEMU current implementation, that data can be written to
unallocated clusters only. May be used for a backup job.

Suggested-by: Max Reitz 
Signed-off-by: Andrey Shinkevich 
---
 block/Makefile.objs |   1 +
 block/filter-compress.c | 201 
 qapi/block-core.json|  10 ++-
 3 files changed, 208 insertions(+), 4 deletions(-)
 create mode 100644 block/filter-compress.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index e394fe0..330529b 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -43,6 +43,7 @@ block-obj-y += crypto.o
 
 block-obj-y += aio_task.o
 block-obj-y += backup-top.o
+block-obj-y += filter-compress.o
 
 common-obj-y += stream.o
 
diff --git a/block/filter-compress.c b/block/filter-compress.c
new file mode 100644
index 000..522d6c3
--- /dev/null
+++ b/block/filter-compress.c
@@ -0,0 +1,201 @@
+/*
+ * Compress filter block driver
+ *
+ * Copyright (c) 2019 Virtuozzo International GmbH
+ *
+ * Author:
+ *   Andrey Shinkevich 
+ *   (based on block/copy-on-read.c by Max Reitz)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) any later version of the License.
+ *
+ * This program 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "block/block_int.h"
+#include "qemu/module.h"
+
+
+static int compress_open(BlockDriverState *bs, QDict *options, int flags,
+ Error **errp)
+{
+bs->file = bdrv_open_child(NULL, options, "file", bs, _file, false,
+  errp);
+if (!bs->file) {
+return -EINVAL;
+}
+
+bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
+BDRV_REQ_WRITE_COMPRESSED |
+(BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
+
+bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
+((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
+bs->file->bs->supported_zero_flags);
+
+return 0;
+}
+
+
+#define PERM_PASSTHROUGH (BLK_PERM_CONSISTENT_READ \
+  | BLK_PERM_WRITE \
+  | BLK_PERM_RESIZE)
+#define PERM_UNCHANGED (BLK_PERM_ALL & ~PERM_PASSTHROUGH)
+
+static void compress_child_perm(BlockDriverState *bs, BdrvChild *c,
+const BdrvChildRole *role,
+BlockReopenQueue *reopen_queue,
+uint64_t perm, uint64_t shared,
+uint64_t *nperm, uint64_t *nshared)
+{
+*nperm = perm & PERM_PASSTHROUGH;
+*nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED;
+
+/*
+ * We must not request write permissions for an inactive node, the child
+ * cannot provide it.
+ */
+if (!(bs->open_flags & BDRV_O_INACTIVE)) {
+*nperm |= BLK_PERM_WRITE_UNCHANGED;
+}
+}
+
+
+static int64_t compress_getlength(BlockDriverState *bs)
+{
+return bdrv_getlength(bs->file->bs);
+}
+
+
+static int coroutine_fn compress_co_truncate(BlockDriverState *bs,
+ int64_t offset, bool exact,
+ PreallocMode prealloc,
+ Error **errp)
+{
+return bdrv_co_truncate(bs->file, offset, exact, prealloc, errp);
+}
+
+
+static int coroutine_fn compress_co_preadv_part(BlockDriverState *bs,
+uint64_t offset, uint64_t 
bytes,
+QEMUIOVector *qiov,
+size_t qiov_offset,
+int flags)
+{
+return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+   flags);
+}
+
+
+static int coroutine_fn compress_co_pwritev_part(BlockDriverState *bs,
+ uint64_t offset,
+ uint64_t bytes,
+ QEMUIOVector *qiov,
+ size_t qiov_offset, int flags)
+{
+return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
+flags | BDRV_REQ_WRITE_COMPRESSED);
+}
+
+
+static int coroutine_fn 

[PATCH v8 3/3] tests/qemu-iotests: add case to write compressed data of multiple clusters

[Andrey Shinkevich]

Add the case to the iotest #214 that checks possibility of writing
compressed data of more than one cluster size. The test case involves
the compress filter driver showing a sample usage of that.

Signed-off-by: Andrey Shinkevich 
---
 tests/qemu-iotests/214 | 43 +++
 tests/qemu-iotests/214.out | 14 ++
 2 files changed, 57 insertions(+)

diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
index 21ec8a2..5012112 100755
--- a/tests/qemu-iotests/214
+++ b/tests/qemu-iotests/214
@@ -89,6 +89,49 @@ _check_test_img -r all
 $QEMU_IO -c "read  -P 0x11  0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | 
_filter_testdir
 $QEMU_IO -c "read  -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | 
_filter_testdir
 
+echo
+echo "=== Write compressed data of multiple clusters ==="
+echo
+cluster_size=0x1
+_make_test_img 2M -o cluster_size=$cluster_size
+
+echo "Write uncompressed data:"
+let data_size="8 * $cluster_size"
+$QEMU_IO -c "write -P 0xaa 0 $data_size" "$TEST_IMG" \
+ 2>&1 | _filter_qemu_io | _filter_testdir
+sizeA=$($QEMU_IMG info --output=json "$TEST_IMG" |
+sed -n '/"actual-size":/ s/[^0-9]//gp')
+
+_make_test_img 2M -o cluster_size=$cluster_size
+echo "Write compressed data:"
+let data_size="3 * $cluster_size + ($cluster_size / 2)"
+# Set compress on. That will align the written data
+# by the cluster size and will write them compressed.
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
+$QEMU_IO -c "write -P 0xbb 0 $data_size" --image-opts \
+ 
"driver=compress,file.driver=$IMGFMT,file.file.driver=file,file.file.filename=$TEST_IMG"
 \
+ 2>&1 | _filter_qemu_io | _filter_testdir
+
+let offset="4 * $cluster_size"
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
+$QEMU_IO -c "write -P 0xcc $offset $data_size" "json:{\
+'driver': 'compress',
+'file': {'driver': '$IMGFMT',
+ 'file': {'driver': 'file',
+  'filename': '$TEST_IMG'}}}" | \
+  _filter_qemu_io | _filter_testdir
+
+sizeB=$($QEMU_IMG info --output=json "$TEST_IMG" |
+sed -n '/"actual-size":/ s/[^0-9]//gp')
+
+if [ $sizeA -le $sizeB ]
+then
+echo "Compression ERROR"
+fi
+
+$QEMU_IMG check --output=json "$TEST_IMG" |
+  sed -n 's/,$//; /"compressed-clusters":/ s/^ *//p'
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/214.out b/tests/qemu-iotests/214.out
index 0fcd8dc..4a2ec33 100644
--- a/tests/qemu-iotests/214.out
+++ b/tests/qemu-iotests/214.out
@@ -32,4 +32,18 @@ read 4194304/4194304 bytes at offset 0
 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 4194304/4194304 bytes at offset 4194304
 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Write compressed data of multiple clusters ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
+Write uncompressed data:
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
+Write compressed data:
+wrote 229376/229376 bytes at offset 0
+224 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 229376/229376 bytes at offset 262144
+224 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+"compressed-clusters": 8
 *** done
-- 
1.8.3.1

[PATCH v8 2/3] qcow2: Allow writing compressed data of multiple clusters

[Andrey Shinkevich]

QEMU currently supports writing compressed data of the size equal to
one cluster. This patch allows writing QCOW2 compressed data that
exceed one cluster. Now, we split buffered data into separate clusters
and write them compressed using the block/aio_task API.

Suggested-by: Pavel Butsykin 
Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 102 ++
 1 file changed, 75 insertions(+), 27 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7c18721..0e03a1a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4222,10 +4222,8 @@ fail:
 return ret;
 }
 
-/* XXX: put compressed sectors first, then all the cluster aligned
-   tables to avoid losing bytes in alignment */
 static coroutine_fn int
-qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
+qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
  uint64_t offset, uint64_t bytes,
  QEMUIOVector *qiov, size_t qiov_offset)
 {
@@ -4235,32 +4233,11 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
 uint8_t *buf, *out_buf;
 uint64_t cluster_offset;
 
-if (has_data_file(bs)) {
-return -ENOTSUP;
-}
-
-if (bytes == 0) {
-/* align end of file to a sector boundary to ease reading with
-   sector based I/Os */
-int64_t len = bdrv_getlength(bs->file->bs);
-if (len < 0) {
-return len;
-}
-return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL);
-}
-
-if (offset_into_cluster(s, offset)) {
-return -EINVAL;
-}
+assert(bytes == s->cluster_size || (bytes < s->cluster_size &&
+   (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS)));
 
 buf = qemu_blockalign(bs, s->cluster_size);
-if (bytes != s->cluster_size) {
-if (bytes > s->cluster_size ||
-offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS)
-{
-qemu_vfree(buf);
-return -EINVAL;
-}
+if (bytes < s->cluster_size) {
 /* Zero-pad last write if image size is not cluster aligned */
 memset(buf + bytes, 0, s->cluster_size - bytes);
 }
@@ -4309,6 +4286,77 @@ fail:
 return ret;
 }
 
+static coroutine_fn int qcow2_co_pwritev_compressed_task_entry(AioTask *task)
+{
+Qcow2AioTask *t = container_of(task, Qcow2AioTask, task);
+
+assert(!t->cluster_type && !t->l2meta);
+
+return qcow2_co_pwritev_compressed_task(t->bs, t->offset, t->bytes, 
t->qiov,
+t->qiov_offset);
+}
+
+/*
+ * XXX: put compressed sectors first, then all the cluster aligned
+ * tables to avoid losing bytes in alignment
+ */
+static coroutine_fn int
+qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov, size_t qiov_offset)
+{
+BDRVQcow2State *s = bs->opaque;
+AioTaskPool *aio = NULL;
+int ret = 0;
+
+if (has_data_file(bs)) {
+return -ENOTSUP;
+}
+
+if (bytes == 0) {
+/*
+ * align end of file to a sector boundary to ease reading with
+ * sector based I/Os
+ */
+int64_t len = bdrv_getlength(bs->file->bs);
+if (len < 0) {
+return len;
+}
+return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL);
+}
+
+if (offset_into_cluster(s, offset)) {
+return -EINVAL;
+}
+
+while (bytes && aio_task_pool_status(aio) == 0) {
+uint64_t chunk_size = MIN(bytes, s->cluster_size);
+
+if (!aio && chunk_size != bytes) {
+aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
+}
+
+ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
+ 0, 0, offset, chunk_size, qiov, qiov_offset, 
NULL);
+if (ret < 0) {
+break;
+}
+qiov_offset += chunk_size;
+offset += chunk_size;
+bytes -= chunk_size;
+}
+
+if (aio) {
+aio_task_pool_wait_all(aio);
+if (ret == 0) {
+ret = aio_task_pool_status(aio);
+}
+g_free(aio);
+}
+
+return ret;
+}
+
 static int coroutine_fn
 qcow2_co_preadv_compressed(BlockDriverState *bs,
uint64_t file_cluster_offset,
-- 
1.8.3.1

[PATCH v8 0/3] qcow2: advanced compression options

[Andrey Shinkevich]

The compression filter driver is introduced as suggested by Max.
A sample usage of the filter can be found in the test #214.
Now, multiple clusters can be written compressed.
It is useful for the backup job.

v8: The filter child was changed from the 'backing' to the 'file' one.

  Discussed in the email thread with the message ID
  <1573670589-229357-1-git-send-email-andrey.shinkev...@virtuozzo.com>

Andrey Shinkevich (3):
  block: introduce compress filter driver
  qcow2: Allow writing compressed data of multiple clusters
  tests/qemu-iotests: add case to write compressed data of multiple
clusters

 block/Makefile.objs|   1 +
 block/filter-compress.c| 201 +
 block/qcow2.c  | 102 +--
 qapi/block-core.json   |  10 ++-
 tests/qemu-iotests/214 |  43 ++
 tests/qemu-iotests/214.out |  14 
 6 files changed, 340 insertions(+), 31 deletions(-)
 create mode 100644 block/filter-compress.c

-- 
1.8.3.1

[QEMU-DEVEL][PATCH] gpio: fix memory leak in aspeed_gpio_init()

[pannengyuan]

From: PanNengyuan 

Address Sanitizer shows memory leak in hw/gpio/aspeed_gpio.c:875

Reported-by: Euler Robot 
Signed-off-by: PanNengyuan 
---
 hw/gpio/aspeed_gpio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 7acc5fa..41e11ea 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -876,6 +876,7 @@ static void aspeed_gpio_init(Object *obj)
pin_idx % GPIOS_PER_GROUP);
 object_property_add(obj, name, "bool", aspeed_gpio_get_pin,
 aspeed_gpio_set_pin, NULL, NULL, NULL);
+g_free(name);
 }
 }
 
-- 
2.7.2.windows.1

[Qemu-devel][PATCH] ppc/spapr_events: fix potential NULL pointer dereference in rtas_event_log_dequeue

[pannengyuan]

From: PanNengyuan 

source is being dereferenced before it is null checked, hence there is a
potential null pointer dereference.

This fixes:
360
CID 68911917: (NULL_RETURNS)
361. dereference: Dereferencing "source", which is known to be
"NULL".
361if (source->mask & event_mask) {
362break;
363}

Reported-by: Euler Robot 
Signed-off-by: PanNengyuan 
---
 hw/ppc/spapr_events.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 0e4c195..febd2ef 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -358,7 +358,7 @@ static SpaprEventLogEntry 
*rtas_event_log_dequeue(SpaprMachineState *spapr,
 rtas_event_log_to_source(spapr,
  spapr_event_log_entry_type(entry));
 
-if (source->mask & event_mask) {
+if (source && (source->mask & event_mask)) {
 break;
 }
 }
-- 
2.7.2.windows.1

Re: [PATCH 0/2] not use multifd during postcopy

[Wei Yang]

Ping for comments.

On Sat, Oct 26, 2019 at 07:19:58AM +0800, Wei Yang wrote:
>We don't support multifd during postcopy, but user still could enable
>both multifd and postcopy. This leads to migration failure.
>
>Patch 1 does proper cleanup, otherwise we may have data corruption.
>Patch 2 does the main job.
>
>BTW, current multifd synchronization method needs a cleanup. Will send another
>patch set.
>
>Wei Yang (2):
>  migration/multifd: clean pages after filling packet
>  migration/multifd: not use multifd during postcopy
>
> migration/ram.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
>-- 
>2.17.1

-- 
Wei Yang
Help you, Help me

[PATCH for-5.0 4/4] spapr: Abort if XICS interrupt controller cannot be initialized

[Greg Kurz]

Failing to set any of the ICS property should really never happen:
- object_property_add_child() always succeed unless the child object
  already has a parent, which isn't the case here obviously since the
  ICS has just been created with object_new()
- the ICS has an "nr-irqs" property than can be set as long as the ICS
  isn't realized

In both cases, an error indicates there is a bug in QEMU. Propagating the
error, ie. exiting QEMU since spapr_irq_init() is called with _fatal
doesn't make much sense. Abort instead. This is consistent with what is
done with XIVE : both qdev_create() and qdev_prop_set_uint32() abort QEMU
on error.

Signed-off-by: Greg Kurz 
---
 hw/ppc/spapr_irq.c |   13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 487c8ceb35a3..37f65dac9ca8 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -313,20 +313,11 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
**errp)
 Object *obj;
 
 obj = object_new(TYPE_ICS_SPAPR);
-object_property_add_child(OBJECT(spapr), "ics", obj, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
 
+object_property_add_child(OBJECT(spapr), "ics", obj, _abort);
 object_property_set_link(obj, OBJECT(spapr), ICS_PROP_XICS,
  _abort);
-object_property_set_int(obj, smc->nr_xirqs, "nr-irqs", _err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
-
+object_property_set_int(obj, smc->nr_xirqs, "nr-irqs", _abort);
 object_property_set_bool(obj, true, "realized", _err);
 if (local_err) {
 error_propagate(errp, local_err);

[PATCH for-5.0 3/4] xics: Link ICP_PROP_CPU property to ICPState::cs pointer

[Greg Kurz]

The ICP object has both a pointer and an ICP_PROP_CPU property pointing
to the cpu. Confusing bugs could arise if these ever go out of sync.

Change the property definition so that it explicitely sets the pointer.
The property isn't optional : not being able to set the link is a bug
and QEMU should rather abort than exit in this case.

Signed-off-by: Greg Kurz 
---
 hw/intc/xics.c |   21 -
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 35dddb88670e..0b259a09c545 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -305,25 +305,13 @@ void icp_reset(ICPState *icp)
 static void icp_realize(DeviceState *dev, Error **errp)
 {
 ICPState *icp = ICP(dev);
-PowerPCCPU *cpu;
 CPUPPCState *env;
-Object *obj;
 Error *err = NULL;
 
 assert(icp->xics);
+assert(icp->cs);
 
-obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, );
-if (!obj) {
-error_propagate_prepend(errp, err,
-"required link '" ICP_PROP_CPU
-"' not found: ");
-return;
-}
-
-cpu = POWERPC_CPU(obj);
-icp->cs = CPU(obj);
-
-env = >env;
+env = _CPU(icp->cs)->env;
 switch (PPC_INPUT(env)) {
 case PPC_FLAGS_INPUT_POWER7:
 icp->output = env->irq_inputs[POWER7_INPUT_INT];
@@ -363,6 +351,7 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
 static Property icp_properties[] = {
 DEFINE_PROP_LINK(ICP_PROP_XICS, ICPState, xics, TYPE_XICS_FABRIC,
  XICSFabric *),
+DEFINE_PROP_LINK(ICP_PROP_CPU, ICPState, cs, TYPE_CPU, CPUState *),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -397,8 +386,7 @@ Object *icp_create(Object *cpu, const char *type, 
XICSFabric *xi, Error **errp)
 object_property_add_child(cpu, type, obj, _abort);
 object_unref(obj);
 object_property_set_link(obj, OBJECT(xi), ICP_PROP_XICS, _abort);
-object_ref(cpu);
-object_property_add_const_link(obj, ICP_PROP_CPU, cpu, _abort);
+object_property_set_link(obj, cpu, ICP_PROP_CPU, _abort);
 object_property_set_bool(obj, true, "realized", _err);
 if (local_err) {
 object_unparent(obj);
@@ -413,7 +401,6 @@ void icp_destroy(ICPState *icp)
 {
 Object *obj = OBJECT(icp);
 
-object_unref(object_property_get_link(obj, ICP_PROP_CPU, _abort));
 object_unparent(obj);
 }
 

[PATCH for-5.0 2/4] xics: Link ICP_PROP_XICS property to ICPState::xics pointer

[Greg Kurz]

The ICP object has both a pointer and an ICP_PROP_XICS property pointing
to the XICS fabric. Confusing bugs could arise if these ever go out of
sync.

Change the property definition so that it explicitely sets the pointer.
The property isn't optional : not being able to set the link is a bug
and QEMU should rather abort than exit in this case.

Signed-off-by: Greg Kurz 
---
 hw/intc/xics.c |   22 +-
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index f7a454808992..35dddb88670e 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -310,15 +310,7 @@ static void icp_realize(DeviceState *dev, Error **errp)
 Object *obj;
 Error *err = NULL;
 
-obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, );
-if (!obj) {
-error_propagate_prepend(errp, err,
-"required link '" ICP_PROP_XICS
-"' not found: ");
-return;
-}
-
-icp->xics = XICS_FABRIC(obj);
+assert(icp->xics);
 
 obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, );
 if (!obj) {
@@ -368,12 +360,19 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
 vmstate_unregister(NULL, _icp_server, icp);
 }
 
+static Property icp_properties[] = {
+DEFINE_PROP_LINK(ICP_PROP_XICS, ICPState, xics, TYPE_XICS_FABRIC,
+ XICSFabric *),
+DEFINE_PROP_END_OF_LIST(),
+};
+
 static void icp_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->realize = icp_realize;
 dc->unrealize = icp_unrealize;
+dc->props = icp_properties;
 /*
  * Reason: part of XICS interrupt controller, needs to be wired up
  * by icp_create().
@@ -397,9 +396,7 @@ Object *icp_create(Object *cpu, const char *type, 
XICSFabric *xi, Error **errp)
 obj = object_new(type);
 object_property_add_child(cpu, type, obj, _abort);
 object_unref(obj);
-object_ref(OBJECT(xi));
-object_property_add_const_link(obj, ICP_PROP_XICS, OBJECT(xi),
-   _abort);
+object_property_set_link(obj, OBJECT(xi), ICP_PROP_XICS, _abort);
 object_ref(cpu);
 object_property_add_const_link(obj, ICP_PROP_CPU, cpu, _abort);
 object_property_set_bool(obj, true, "realized", _err);
@@ -417,7 +414,6 @@ void icp_destroy(ICPState *icp)
 Object *obj = OBJECT(icp);
 
 object_unref(object_property_get_link(obj, ICP_PROP_CPU, _abort));
-object_unref(object_property_get_link(obj, ICP_PROP_XICS, _abort));
 object_unparent(obj);
 }
 

[PATCH for-5.0 1/4] xics: Link ICS_PROP_XICS property to ICSState::xics pointer

[Greg Kurz]

The ICS object has both a pointer and an ICS_PROP_XICS property pointing
to the XICS fabric. Confusing bugs could arise if these ever go out of
sync.

Change the property definition so that it explicitely sets the pointer.
The property isn't optional : not being able to set the link is a bug
and QEMU should rather abort than exit in this case.

Signed-off-by: Greg Kurz 
---
 hw/intc/xics.c |   13 +++--
 hw/ppc/pnv_psi.c   |3 +--
 hw/ppc/spapr_irq.c |9 ++---
 3 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index e7ac9ba618fa..f7a454808992 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -609,17 +609,8 @@ static void ics_reset_handler(void *dev)
 static void ics_realize(DeviceState *dev, Error **errp)
 {
 ICSState *ics = ICS(dev);
-Error *local_err = NULL;
-Object *obj;
 
-obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, _err);
-if (!obj) {
-error_propagate_prepend(errp, local_err,
-"required link '" ICS_PROP_XICS
-"' not found: ");
-return;
-}
-ics->xics = XICS_FABRIC(obj);
+assert(ics->xics);
 
 if (!ics->nr_irqs) {
 error_setg(errp, "Number of interrupts needs to be greater 0");
@@ -699,6 +690,8 @@ static const VMStateDescription vmstate_ics = {
 
 static Property ics_properties[] = {
 DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0),
+DEFINE_PROP_LINK(ICS_PROP_XICS, ICSState, xics, TYPE_XICS_FABRIC,
+ XICSFabric *),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index a360515a86f8..7e725aaf2bd5 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -497,8 +497,7 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error 
**errp)
 }
 
 /* Create PSI interrupt control source */
-object_property_add_const_link(OBJECT(ics), ICS_PROP_XICS, obj,
-   _abort);
+object_property_set_link(OBJECT(ics), obj, ICS_PROP_XICS, _abort);
 object_property_set_int(OBJECT(ics), PSI_NUM_INTERRUPTS, "nr-irqs", );
 if (err) {
 error_propagate(errp, err);
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 168044be853a..487c8ceb35a3 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -319,13 +319,8 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
 return;
 }
 
-object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
-   _err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
-
+object_property_set_link(obj, OBJECT(spapr), ICS_PROP_XICS,
+ _abort);
 object_property_set_int(obj, smc->nr_xirqs, "nr-irqs", _err);
 if (local_err) {
 error_propagate(errp, local_err);

[PATCH for-5.0 0/4] ppc: Some more QOM cleanup for XICS

[Greg Kurz]

This series consolidates some more QOM links and pointers that point to
the same object. While here also simplify the XICS interrupt controller
init in the machine code.

--
Greg

---

Greg Kurz (4):
  xics: Link ICS_PROP_XICS property to ICSState::xics pointer
  xics: Link ICP_PROP_XICS property to ICPState::xics pointer
  xics: Link ICP_PROP_CPU property to ICPState::cs pointer
  spapr: Abort if XICS interrupt controller cannot be initialized


 hw/intc/xics.c |   54 +++-
 hw/ppc/pnv_psi.c   |3 +--
 hw/ppc/spapr_irq.c |   22 -
 3 files changed, 21 insertions(+), 58 deletions(-)

[PATCH] Modify tests to work with clang

[Taylor Simpson]

Signed-off-by: Taylor Simpson 
---
 tests/tcg/multiarch/float_helpers.c | 2 --
 tests/tcg/multiarch/linux-test.c| 6 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/tcg/multiarch/float_helpers.c 
b/tests/tcg/multiarch/float_helpers.c
index 8ee7903..bc530e5 100644
--- a/tests/tcg/multiarch/float_helpers.c
+++ b/tests/tcg/multiarch/float_helpers.c
@@ -79,11 +79,9 @@ char *fmt_16(uint16_t num)
 
 #ifndef SNANF
 /* Signaling NaN macros, if supported.  */
-# if __GNUC_PREREQ(3, 3)
 #  define SNANF (__builtin_nansf (""))
 #  define SNAN (__builtin_nans (""))
 #  define SNANL (__builtin_nansl (""))
-# endif
 #endif
 
 static float f32_numbers[] = {
diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
index 673d7c8..8a7c15c 100644
--- a/tests/tcg/multiarch/linux-test.c
+++ b/tests/tcg/multiarch/linux-test.c
@@ -485,7 +485,11 @@ static void test_signal(void)
 act.sa_flags = SA_SIGINFO;
 chk_error(sigaction(SIGSEGV, , NULL));
 if (setjmp(jmp_env) == 0) {
-*(uint8_t *)0 = 0;
+/*
+ * clang requires volatile or it will turn this into a
+ * call to abort() instead of forcing a SIGSEGV.
+ */
+*(volatile uint8_t *)0 = 0;
 }
 
 act.sa_handler = SIG_DFL;
-- 
2.7.4

[Bug 1846427] Re: 4.1.0: qcow2 corruption on savevm/quit/loadvm cycle

[Matti Hameister]

The image was fine before upgrading qemu. I rechecked the image after
the first use and it was fine. But after the larger Windows 1903 -> 1909
upgrade done in qemu 4.1.0 the image was damaged. I will try the git
master version of qemu in the coming days and report back.

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

Title:
  4.1.0: qcow2 corruption on savevm/quit/loadvm cycle

Status in QEMU:
  Fix Committed

Bug description:
  I'm seeing massive corruption of qcow2 images with qemu 4.1.0 and git
  master as of 7f21573c822805a8e6be379d9bcf3ad9effef3dc after a few
  savevm/quit/loadvm cycles. I've narrowed it down to the following
  reproducer (further notes below):

  # qemu-img check debian.qcow2
  No errors were found on the image.
  251601/327680 = 76.78% allocated, 1.63% fragmented, 0.00% compressed clusters
  Image end offset: 18340446208
  # bin/qemu/bin/qemu-system-x86_64 -machine pc-q35-4.0.1,accel=kvm -m 4096 
-chardev stdio,id=charmonitor -mon chardev=charmonitor -drive 
file=debian.qcow2,id=d -S
  qemu-system-x86_64: warning: dbind: Couldn't register with accessibility bus: 
Did not receive a reply. Possible causes include: the remote application did 
not send a reply, the message bus security policy blocked the reply, the reply 
timeout expired, or the network connection was broken.
  QEMU 4.1.50 monitor - type 'help' for more information
  (qemu) loadvm foo
  (qemu) c
  (qemu) qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  quit
  [m@nargothrond:~] qemu-img check debian.qcow2
  Leaked cluster 85179 refcount=2 reference=1
  Leaked cluster 85180 refcount=2 reference=1
  ERROR cluster 266150 refcount=0 reference=2
  [...]
  ERROR OFLAG_COPIED data cluster: l2_entry=42284 refcount=1

  9493 errors were found on the image.
  Data may be corrupted, or further writes to the image may corrupt it.

  2 leaked clusters were found on the image.
  This means waste of disk space, but no harm to data.
  259266/327680 = 79.12% allocated, 1.67% fragmented, 0.00% compressed clusters
  Image end offset: 18340446208

  This is on a x86_64 Linux 5.3.1 Gentoo host with qemu-system-x86_64
  and accel=kvm. The compiler is gcc-9.2.0 with the rest of the system
  similarly current.

  Reproduced with qemu-4.1.0 from distribution package as well as
  vanilla git checkout of tag v4.1.0 and commit
  7f21573c822805a8e6be379d9bcf3ad9effef3dc (today's master). Does not
  happen with qemu compiled from vanilla checkout of tag v4.0.0. Build
  sequence:

  ./configure --prefix=$HOME/bin/qemu-bisect --target-list=x86_64-softmmu 
--disable-werror --disable-docs
  [...]
  CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g
  [...] (can provide full configure output if helpful)
  make -j8 install

  The kind of guest OS does not matter: seen with Debian testing 64bit,
  Windows 7 x86/x64 BIOS and Windows 7 x64 EFI.

  The virtual storage controller does not seem to matter: seen with
  VirtIO SCSI, emulated SCSI and emulated SATA AHCI.

  Caching modes (none, directsync, writeback), aio mode (threads,
  native) or discard (ignore, unmap) or detect-zeroes (off, unmap) does
  not influence occurence either.

  Having more RAM in the guest seems to increase odds of corruption:
  With 512MB to the Debian guest problem hardly occurs at all, with 4GB
  RAM it happens almost instantly.

  An automated reproducer works as follows:

  - the guest *does* mount its root fs and swap with option discard and
  my testing leaves me with the impression that file deletion rather
  than reading is causing the issue

  - foo is a snapshot of the running Debian VM which is already running
  command

  # while true ; do dd if=/dev/zero of=foo bs=10240k count=400 ; done

  to produce some I/O to the disk (4GB file with 4GB of RAM).

  - on the host a loop continuously resumes and saves the guest state
  and quits qemu inbetween:

  # while true ; do (echo loadvm foo ; echo c ; sleep 10 ; echo stop ;
  echo savevm foo ; echo quit ) | bin/qemu-bisect/bin/qemu-system-x86_64
  -machine pc-q35-3.1,accel=kvm -m 4096 -chardev stdio,id=charmonitor
  -mon chardev=charmonitor -drive file=debian.qcow2,id=d -S -display
  none ; done

  - quitting qemu inbetween saves and loads seems to be necessary for
  the problem to occur. Just continusouly in one session saving and
  loading guest state does not trigger it.

  - For me, after about 2 to 6 iterations of above loop the image is
  corrupted.

  - corruption manifests with other messages from qemu as well, e.g.:

  (qemu) loadvm foo
  Error: Device 'd' does not have the requested snapshot 'foo'

  Using above reproducer I have to the be best of my ability bisected
  the introduction of the problem to commit
  69f47505ee66afaa513305de0c1895a224e52c45 (block: 

Re: [PATCH v2] mc146818rtc: fix timer interrupt reinjection

[Alex Williamson]

On Sun, 17 Nov 2019 11:12:43 +0100
Paolo Bonzini  wrote:

> On 17/11/19 05:31, Alex Williamson wrote:
> > The 'merge' option gives me a similar error.  The 'delay' option is
> > the only other choice where I can actually start the VM, but this
> > results in the commandline:
> > 
> > -rtc base=localtime
> > 
> > (no driftfix specified)  
> 
> none is the default, so that's okay.
> 
> > This does appear to resolve the issue, but of course compatibility
> > with existing configurations has regressed. Thanks,  
> 
> Yeah, I guess this was just a suggestion to double-check the cause of 
> the regression.
> 
> The problem could be that periodic_timer_update is using old_period == 0 
> for two cases: no period change, and old period was 0 (periodic timer 
> off).
> 
> Something like the following distinguishes the two cases by always using
> s->period (currently it was only used for driftfix=slew) and passing
> s->period instead of 0 when there is no period change.
> 
> More cleanups are possible, but this is the smallest patch that implements
> the idea.  The first patch is big but, indentation changes aside, it's
> moving a single closed brace.
> 
> Alex/Marcelo, can you check if it fixes both of your test cases?

It resolves the majority of the regression, but I think there's still a
performance issue.  Passmark PerformanceTest in the guest shows a 5+%
decrease versus a straight revert.  Thanks,

Alex

> - 8< ---
> From 48dc9d140c636067b8de1ab8e25b819151c83162 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini 
> Date: Sun, 17 Nov 2019 10:07:38 +0100
> Subject: [PATCH 1/2] Revert "mc146818rtc: fix timer interrupt reinjection"
> 
> This reverts commit b429de730174b388ea5760e3debb0d542ea3c261, except
> that the reversal of the outer "if (period)" is left in.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/rtc/mc146818rtc.c | 67 ++--
>  1 file changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index ee6bf82b40..9869dc5031 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -174,7 +174,6 @@ periodic_timer_update(RTCState *s, int64_t current_time, 
> uint32_t old_period)
>  int64_t cur_clock, next_irq_clock, lost_clock = 0;
>  
>  period = rtc_periodic_clock_ticks(s);
> -
>  if (!period) {
>  s->irq_coalesced = 0;
>  timer_del(s->periodic_timer);
> @@ -197,42 +196,42 @@ periodic_timer_update(RTCState *s, int64_t 
> current_time, uint32_t old_period)
>  last_periodic_clock = next_periodic_clock - old_period;
>  lost_clock = cur_clock - last_periodic_clock;
>  assert(lost_clock >= 0);
> +}
>  
> +/*
> + * s->irq_coalesced can change for two reasons:
> + *
> + * a) if one or more periodic timer interrupts have been lost,
> + *lost_clock will be more that a period.
> + *
> + * b) when the period may be reconfigured, we expect the OS to
> + *treat delayed tick as the new period.  So, when switching
> + *from a shorter to a longer period, scale down the missing,
> + *because the OS will treat past delayed ticks as longer
> + *(leftovers are put back into lost_clock).  When switching
> + *to a shorter period, scale up the missing ticks since the
> + *OS handler will treat past delayed ticks as shorter.
> + */
> +if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
> +uint32_t old_irq_coalesced = s->irq_coalesced;
> +
> +s->period = period;
> +lost_clock += old_irq_coalesced * old_period;
> +s->irq_coalesced = lost_clock / s->period;
> +lost_clock %= s->period;
> +if (old_irq_coalesced != s->irq_coalesced ||
> +old_period != s->period) {
> +DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, "
> +  "period scaled from %d to %d\n", old_irq_coalesced,
> +  s->irq_coalesced, old_period, s->period);
> +rtc_coalesced_timer_update(s);
> +}
> +} else {
>  /*
> - * s->irq_coalesced can change for two reasons:
> - *
> - * a) if one or more periodic timer interrupts have been lost,
> - *lost_clock will be more that a period.
> - *
> - * b) when the period may be reconfigured, we expect the OS to
> - *treat delayed tick as the new period.  So, when switching
> - *from a shorter to a longer period, scale down the missing,
> - *because the OS will treat past delayed ticks as longer
> - *(leftovers are put back into lost_clock).  When switching
> - *to a shorter period, scale up the missing ticks since the
> - *OS handler will treat past delayed ticks as shorter.
> + * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
> + * is not used, we should make the time progress 

[PATCH] qom/object: enable setter for uint types

[Felipe Franciosi]

Traditionally, the uint-specific property helpers only offer getters.
When adding object (or class) uint types, one must therefore use the
generic property helper if a setter is needed.

This enhances the uint-specific property helper APIs by adding a
'readonly' field and modifying all users of that API to set this
parameter to true. If 'readonly' is false, though, the helper will add
an automatic setter for the value.

Signed-off-by: Felipe Franciosi 
---
 hw/acpi/ich9.c   |   4 +-
 hw/acpi/pcihp.c  |   6 +--
 hw/acpi/piix4.c  |  12 +++---
 hw/isa/lpc_ich9.c|   4 +-
 hw/ppc/spapr_drc.c   |   2 +-
 include/qom/object.h |  28 
 qom/object.c | 100 ---
 ui/console.c |   3 +-
 8 files changed, 111 insertions(+), 48 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 2034dd749e..94dc5147ce 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs 
*pm, Error **errp)
 pm->s4_val = 2;
 
 object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
-   >pm_io_base, errp);
+   >pm_io_base, true, errp);
 object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
 ich9_pm_get_gpe0_blk,
 NULL, NULL, pm, NULL);
 object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
-   _len, errp);
+   _len, true, errp);
 object_property_add_bool(obj, "memory-hotplug-support",
  ich9_pm_get_memory_hotplug_support,
  ich9_pm_set_memory_hotplug_support,
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 8413348a33..70bc1408e6 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -80,7 +80,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
 
 *bus_bsel = (*bsel_alloc)++;
 object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
-   bus_bsel, _abort);
+   bus_bsel, true, _abort);
 }
 
 return bsel_alloc;
@@ -373,9 +373,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, 
PCIBus *root_bus,
 memory_region_add_subregion(address_space_io, s->io_base, >io);
 
 object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, >io_base,
-   _abort);
+   true, _abort);
 object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, >io_len,
-   _abort);
+   true, _abort);
 }
 
 const VMStateDescription vmstate_acpi_pcihp_pci_status = {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 93aec2dd2c..032ba11e62 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -443,17 +443,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
 static const uint16_t sci_int = 9;
 
 object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
-  _enable_cmd, NULL);
+  _enable_cmd, true, NULL);
 object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
-  _disable_cmd, NULL);
+  _disable_cmd, true, NULL);
 object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
-  _blk, NULL);
+  _blk, true, NULL);
 object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
-  _blk_len, NULL);
+  _blk_len, true, NULL);
 object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
-  _int, NULL);
+  _int, true, NULL);
 object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
-  >io_base, NULL);
+  >io_base, true, NULL);
 }
 
 static void piix4_pm_realize(PCIDevice *dev, Error **errp)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 17c292e306..ce3342 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -645,9 +645,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
 ich9_lpc_get_sci_int,
 NULL, NULL, NULL, NULL);
 object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
-  _enable_cmd, NULL);
+  _enable_cmd, true, NULL);
 object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
-  _disable_cmd, NULL);
+  _disable_cmd, true, NULL);
 
 ich9_pm_add_properties(OBJECT(lpc), >pm, NULL);
 }
diff --git 

Re: [PATCH v2] mc146818rtc: fix timer interrupt reinjection

[Paolo Bonzini]

On 17/11/19 05:31, Alex Williamson wrote:
> The 'merge' option gives me a similar error.  The 'delay' option is
> the only other choice where I can actually start the VM, but this
> results in the commandline:
> 
> -rtc base=localtime
> 
> (no driftfix specified)

none is the default, so that's okay.

> This does appear to resolve the issue, but of course compatibility
> with existing configurations has regressed. Thanks,

Yeah, I guess this was just a suggestion to double-check the cause of 
the regression.

The problem could be that periodic_timer_update is using old_period == 0 
for two cases: no period change, and old period was 0 (periodic timer 
off).

Something like the following distinguishes the two cases by always using
s->period (currently it was only used for driftfix=slew) and passing
s->period instead of 0 when there is no period change.

More cleanups are possible, but this is the smallest patch that implements
the idea.  The first patch is big but, indentation changes aside, it's
moving a single closed brace.

Alex/Marcelo, can you check if it fixes both of your test cases?

- 8< ---
>From 48dc9d140c636067b8de1ab8e25b819151c83162 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini 
Date: Sun, 17 Nov 2019 10:07:38 +0100
Subject: [PATCH 1/2] Revert "mc146818rtc: fix timer interrupt reinjection"

This reverts commit b429de730174b388ea5760e3debb0d542ea3c261, except
that the reversal of the outer "if (period)" is left in.

Signed-off-by: Paolo Bonzini 
---
 hw/rtc/mc146818rtc.c | 67 ++--
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index ee6bf82b40..9869dc5031 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -174,7 +174,6 @@ periodic_timer_update(RTCState *s, int64_t current_time, 
uint32_t old_period)
 int64_t cur_clock, next_irq_clock, lost_clock = 0;
 
 period = rtc_periodic_clock_ticks(s);
-
 if (!period) {
 s->irq_coalesced = 0;
 timer_del(s->periodic_timer);
@@ -197,42 +196,42 @@ periodic_timer_update(RTCState *s, int64_t current_time, 
uint32_t old_period)
 last_periodic_clock = next_periodic_clock - old_period;
 lost_clock = cur_clock - last_periodic_clock;
 assert(lost_clock >= 0);
+}
 
+/*
+ * s->irq_coalesced can change for two reasons:
+ *
+ * a) if one or more periodic timer interrupts have been lost,
+ *lost_clock will be more that a period.
+ *
+ * b) when the period may be reconfigured, we expect the OS to
+ *treat delayed tick as the new period.  So, when switching
+ *from a shorter to a longer period, scale down the missing,
+ *because the OS will treat past delayed ticks as longer
+ *(leftovers are put back into lost_clock).  When switching
+ *to a shorter period, scale up the missing ticks since the
+ *OS handler will treat past delayed ticks as shorter.
+ */
+if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+uint32_t old_irq_coalesced = s->irq_coalesced;
+
+s->period = period;
+lost_clock += old_irq_coalesced * old_period;
+s->irq_coalesced = lost_clock / s->period;
+lost_clock %= s->period;
+if (old_irq_coalesced != s->irq_coalesced ||
+old_period != s->period) {
+DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, "
+  "period scaled from %d to %d\n", old_irq_coalesced,
+  s->irq_coalesced, old_period, s->period);
+rtc_coalesced_timer_update(s);
+}
+} else {
 /*
- * s->irq_coalesced can change for two reasons:
- *
- * a) if one or more periodic timer interrupts have been lost,
- *lost_clock will be more that a period.
- *
- * b) when the period may be reconfigured, we expect the OS to
- *treat delayed tick as the new period.  So, when switching
- *from a shorter to a longer period, scale down the missing,
- *because the OS will treat past delayed ticks as longer
- *(leftovers are put back into lost_clock).  When switching
- *to a shorter period, scale up the missing ticks since the
- *OS handler will treat past delayed ticks as shorter.
+ * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
+ * is not used, we should make the time progress anyway.
  */
-if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
-uint32_t old_irq_coalesced = s->irq_coalesced;
-
-s->period = period;
-lost_clock += old_irq_coalesced * old_period;
-s->irq_coalesced = lost_clock / s->period;
-lost_clock %= s->period;
-if (old_irq_coalesced != s->irq_coalesced ||
-old_period != s->period) {
-DPRINTF_C("cmos: 

[PATCH 2/2] target/arm: Relax r13 restriction for ldrex/strex for v8.0

[Richard Henderson]

Armv8-A removes UNPREDICTABLE for R13 for these cases.

Signed-off-by: Richard Henderson 
---
 target/arm/translate.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index b285b23858..3db8103966 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8931,11 +8931,11 @@ static bool op_strex(DisasContext *s, arg_STREX *a, 
MemOp mop, bool rel)
 /* We UNDEF for these UNPREDICTABLE cases.  */
 if (a->rd == 15 || a->rn == 15 || a->rt == 15
 || a->rd == a->rn || a->rd == a->rt
-|| (s->thumb && (a->rd == 13 || a->rt == 13))
+|| (!ENABLE_ARCH_8 && s->thumb && (a->rd == 13 || a->rt == 13))
 || (mop == MO_64
 && (a->rt2 == 15
 || a->rd == a->rt2
-|| (s->thumb && a->rt2 == 13 {
+|| (!ENABLE_ARCH_8 && s->thumb && a->rt2 == 13 {
 unallocated_encoding(s);
 return true;
 }
@@ -9087,10 +9087,10 @@ static bool op_ldrex(DisasContext *s, arg_LDREX *a, 
MemOp mop, bool acq)
 
 /* We UNDEF for these UNPREDICTABLE cases.  */
 if (a->rn == 15 || a->rt == 15
-|| (s->thumb && a->rt == 13)
+|| (!ENABLE_ARCH_8 && s->thumb && a->rt == 13)
 || (mop == MO_64
 && (a->rt2 == 15 || a->rt == a->rt2
-|| (s->thumb && a->rt2 == 13 {
+|| (!ENABLE_ARCH_8 && s->thumb && a->rt2 == 13 {
 unallocated_encoding(s);
 return true;
 }
-- 
2.17.1

[PATCH 1/2] target/arm: Do not reject rt == rt2 for strexd

[Richard Henderson]

There was too much cut and paste between ldrexd and strexd,
as ldrexd does prohibit two output registers the same.

Fixes: af288228995
Reported-by: Michael Goffioul 
Signed-off-by: Richard Henderson 
---
 target/arm/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 2ea9da7637..b285b23858 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8934,7 +8934,7 @@ static bool op_strex(DisasContext *s, arg_STREX *a, MemOp 
mop, bool rel)
 || (s->thumb && (a->rd == 13 || a->rt == 13))
 || (mop == MO_64
 && (a->rt2 == 15
-|| a->rd == a->rt2 || a->rt == a->rt2
+|| a->rd == a->rt2
 || (s->thumb && a->rt2 == 13 {
 unallocated_encoding(s);
 return true;
-- 
2.17.1

[PATCH for-4.2 0/2] target/arm: two fixes for ldrex/strex

[Richard Henderson]

During this cycle I added checks for UNPREDICTABLE behavior,
but didn't quite get it all right.


r~


Richard Henderson (2):
  target/arm: Do not reject rt == rt2 for strexd
  target/arm: Relax r13 restriction for ldrex/strex for v8.0

 target/arm/translate.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.17.1

Re: Invalid ARM instruction for clang-compiled Android code

[Richard Henderson]

On 11/15/19 12:03 PM, Peter Maydell wrote:
> On Fri, 15 Nov 2019 at 05:03, Michael Goffioul
>  wrote:
>> When running QEMU user mode on some code compiled by clang (dynamic linker 
>> from AOSP-10), the emulator chokes on this instruction:
>>
>>9aa92:   e8c0 2277   strexd  r7, r2, r2, [r0]
> 
> I think that ought to be a valid insn...
> 
>> From debugging, I determined that op_strex() calls unallocated_encoding(), 
>> which I think leads to the SIGILL signal generated.
>>
>> I run the emulator without specifying the ARM cpu type, I think it then 
>> defaults to "any", which should support all instructions, if I'm not 
>> mistaken.
>>
>> Is this instruction really invalid? Or am I doing something wrong?
> 
> Which version of QEMU are you using? (Looking at the code I
> suspect we still have this bug in master, but it's always
> useful to specify what version you're using in a bug report.)
> 
> Richard, I think we're tripping over the check you added
> in commit af2882289951e. Specifically:
> 
> +/* We UNDEF for these UNPREDICTABLE cases.  */
> +if (a->rd == 15 || a->rn == 15 || a->rt == 15
> +|| a->rd == a->rn || a->rd == a->rt
> +|| (s->thumb && (a->rd == 13 || a->rt == 13))
> +|| (mop == MO_64
> +&& (a->rt2 == 15
> +|| a->rd == a->rt2 || a->rt == a->rt2
> +|| (s->thumb && a->rt2 == 13 {
> +unallocated_encoding(s);
> +return true;
> +}
> 
> in the mop == MO_64 subclause we check for
>  a->rt == a->rt2
> so we will UNDEF for rt == rt2, as in this example. But the
> pseudocode in the spec doesn't say that rt == rt2 is
> an UNPREDICTABLE case. (It is an UNDPREDICTABLE
> case for LDREXD, but STREXD lets you write the same
> register twice if you want to.) Or am I misreading this?

You're right.  Too much cut-and-paste between strexd and ldrexd.


r~