Re: [PATCH 2/2] gitlab-ci.yml: Add jobs to test CFI flags

[Paolo Bonzini]

On 23/02/21 00:01, Daniele Buono wrote:

+# Set JOBS=1 because this requires LTO and ld consumes a large amount of 
memory.
+# On gitlab runners, default JOBS of 2 sometimes end up calling 2 lds 
concurrently
+# and triggers an Out-Of-Memory error


Does it make sense to test only one target instead?


+# Because of how slirp is used in QEMU, we need to have CFI also on libslirp.
+# System-wide version in fedora is not compiled with CFI so we recompile it 
using
+# -enable-slirp=git


Can you explain what you mean, and perhaps add a check or warning for 
incompatible settings?


Paolo

Re: [PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism

[Paolo Bonzini]

On 23/02/21 00:01, Daniele Buono wrote:
Currently, make parallelism at build time is defined as #cpus+1. Some 
build jobs may need (or benefit from) a different number. An example is 
builds with LTO where, because of the huge demand of memory at link 
time, gitlab runners fails if two linkers are run concurrently This 
patch retains the default value of #cpus+1 but allows setting the "JOBS" 
variable to a different number when applying the template


As I just found out, you can add -Dbackend_max_links=1 to the meson 
command line instead if LTO is enabled.


Paolo

Re: Editing QEMU POWER Platform wiki page

[Mark Cave-Ayland]

On 23/02/2021 07:59, Greg Kurz wrote:


On Tue, 23 Feb 2021 15:51:19 +1100
David Gibson  wrote:


On Mon, Feb 22, 2021 at 06:18:08PM -0300, Leonardo Augusto Guimarães Garcia 
wrote:

On 2/22/21 8:01 AM, Greg Kurz wrote:

On Thu, 18 Feb 2021 10:16:25 -0300
Leonardo Augusto Guimarães Garcia  wrote:


Hi there,

I would like to edit the wiki page at [0] as it contains some outdated
information. Could anyone that has access to the wiki please help me
create a user so that I can edit it?

0. https://wiki.qemu.org/Documentation/Platforms/POWER


Hi Leo,

User creation isn't publicly available to avoid spam : only an existing
user can create a new account.


Yeah, I saw that. That's why I asked here.


The other concerns raised in this thread are valid, but those
notwithstanding, I think it makes sense to let you update the Wiki if
you have the time and inclination.



Sure, but the point is that this incentive to update documentation
would be better used in the main QEMU documentation, i.e. the
docs/system/ppc/pseries.rst file in Cedric's "docs/system: Extend PPC
section" patch.


Certainly in an ideal world it would make sense to have everything held in the 
documentation, but in real life there are circumstances where having the wiki is useful.


I spend a lot of my time with people interested in emulating older machines, and for 
these people who aren't particularly technical, the manual is far too complicated: 
all they want is examples, and to know what works and what doesn't.


There was a recent thread where I think Peter discussed removing the QuickStart 
section from the documentation because it was out of date, and to me the wiki is a 
good replacement here - pretty much all of the content at 
https://wiki.qemu.org/Documentation/Platforms/PowerPC is user-generated. I also like 
the wiki organisation of Documentation/Platforms/FOO since it's easy to point people 
towards tips that will help them get started on IRC.


Another example is the SPARC page that I maintain at 
https://wiki.qemu.org/Documentation/Platforms/SPARC which mainly came about as people 
kept emailing me off-list to ask whether a particular OS will run under QEMU, and 
would then follow up by asking me for examples because the QEMU command line is 
intimidating for new users.



ATB,

Mark.

[PATCH v2] target/riscv: fix TB_FLAGS bits overlapping bug for rvv/rvh

[frank . chang]

From: Frank Chang 

TB_FLAGS mem_idx bits was extended from 2 bits to 3 bits in
commit: c445593, but other TB_FLAGS bits for rvv and rvh were
not shift as well so these bits may overlap with each other when
rvv is enabled.

Signed-off-by: Frank Chang 
---
 target/riscv/cpu.h   | 12 ++--
 target/riscv/translate.c |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 02758ae0eb4..116b16b9362 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -370,7 +370,6 @@ void QEMU_NORETURN riscv_raise_exception(CPURISCVState *env,
 target_ulong riscv_cpu_get_fflags(CPURISCVState *env);
 void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong);
 
-#define TB_FLAGS_MMU_MASK   7
 #define TB_FLAGS_PRIV_MMU_MASK3
 #define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)
 #define TB_FLAGS_MSTATUS_FS MSTATUS_FS
@@ -379,12 +378,13 @@ typedef CPURISCVState CPUArchState;
 typedef RISCVCPU ArchCPU;
 #include "exec/cpu-all.h"
 
-FIELD(TB_FLAGS, VL_EQ_VLMAX, 2, 1)
-FIELD(TB_FLAGS, LMUL, 3, 2)
-FIELD(TB_FLAGS, SEW, 5, 3)
-FIELD(TB_FLAGS, VILL, 8, 1)
+FIELD(TB_FLAGS, MEM_IDX, 0, 3)
+FIELD(TB_FLAGS, VL_EQ_VLMAX, 3, 1)
+FIELD(TB_FLAGS, LMUL, 4, 2)
+FIELD(TB_FLAGS, SEW, 6, 3)
+FIELD(TB_FLAGS, VILL, 9, 1)
 /* Is a Hypervisor instruction load/store allowed? */
-FIELD(TB_FLAGS, HLSX, 9, 1)
+FIELD(TB_FLAGS, HLSX, 10, 1)
 
 bool riscv_cpu_is_32bit(CPURISCVState *env);
 
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 0f28b5f41e4..9b518cdff46 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -802,7 +802,7 @@ static void riscv_tr_init_disas_context(DisasContextBase 
*dcbase, CPUState *cs)
 uint32_t tb_flags = ctx->base.tb->flags;
 
 ctx->pc_succ_insn = ctx->base.pc_first;
-ctx->mem_idx = tb_flags & TB_FLAGS_MMU_MASK;
+ctx->mem_idx = FIELD_EX32(tb_flags, TB_FLAGS, MEM_IDX);
 ctx->mstatus_fs = tb_flags & TB_FLAGS_MSTATUS_FS;
 ctx->priv_ver = env->priv_ver;
 #if !defined(CONFIG_USER_ONLY)
-- 
2.17.1

Re: [PATCH v2 30/42] esp: add 4 byte PDMA read and write transfers

[Mark Cave-Ayland]

On 16/02/2021 07:30, Philippe Mathieu-Daudé wrote:


Are you planning to review any more of this series? I'm keen to put out
a (hopefully final) v3 soon, but I'll hold off for little while if you
want more time to look over the remaining patches.


I talked about this series with Laurent on Sunday, asking him for
review help ;) I don't remember if there is any big comment to
address in patches 1-14. If not I can review the missing ones
there today and you could send directly a pull request for this
first set, then send the rest as v3. Does that help?
For the rest I doubt having time to focus before Friday.


Hi Phil/Laurent,

I know you're both really busy, but gentle ping to ask if anyone is still planning to 
review the second half of this patchset? :)



ATB,

Mark.

Re: [RFC v1 04/38] target/arm: move psci.c into tcg/softmmu/

[Claudio Fontana]

On 2/22/21 6:22 PM, Alex Bennée wrote:
> 
> Claudio Fontana  writes:
> 
>> From: Claudio Fontana 
>>
>> Signed-off-by: Claudio Fontana 
>> ---
>>  target/arm/internals.h  | 23 ++-
>>  target/arm/tcg/helper.c |  2 ++
>>  target/arm/{ => tcg/softmmu}/psci.c |  0
>>  target/arm/tcg/user/psci.c  | 26 ++
>>  target/arm/meson.build  |  1 -
>>  target/arm/tcg/meson.build  |  3 +++
>>  target/arm/tcg/softmmu/meson.build  |  4 
>>  target/arm/tcg/user/meson.build |  4 
>>  8 files changed, 49 insertions(+), 14 deletions(-)
>>  rename target/arm/{ => tcg/softmmu}/psci.c (100%)
>>  create mode 100644 target/arm/tcg/user/psci.c
>>  create mode 100644 target/arm/tcg/softmmu/meson.build
>>  create mode 100644 target/arm/tcg/user/meson.build
>>
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index 05cebc8597..6384461177 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -292,21 +292,18 @@ vaddr arm_adjust_watchpoint_address(CPUState *cs, 
>> vaddr addr, int len);
>>  /* Callback function for when a watchpoint or breakpoint triggers. */
>>  void arm_debug_excp_handler(CPUState *cs);
>>  
>> -#if defined(CONFIG_USER_ONLY) || !defined(CONFIG_TCG)
>> -static inline bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
>> -{
>> -return false;
>> -}
>> -static inline void arm_handle_psci_call(ARMCPU *cpu)
>> -{
>> -g_assert_not_reached();
>> -}
>> -#else
> 
> I'm not sure I'm a fan of pushing these #ifdef tweaks down into the main
> code when the compiler is good an eliding them away. I guess we need
> this because the helper.o wants to be a shared object between both user
> and softmmu/sysemu mode?


Hi Alex,

yes, but will try to clean this up, I agree that this is too much preprocessor 
stuff.

Ciao,

CLaudio

> 
>> -/* Return true if the r0/x0 value indicates that this SMC/HVC is a PSCI 
>> call. */
>> +#ifdef CONFIG_TCG
>> +/*
>> + * Return true only for softmmu, if the r0/x0 value indicates that this
>> + * SMC/HVC is a PSCI call.
>> + */
>>  bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
>> -/* Actually handle a PSCI call */
>> +
>> +#ifndef CONFIG_USER_ONLY
>>  void arm_handle_psci_call(ARMCPU *cpu);
>> -#endif
>> +#endif /* !CONFIG_USER_ONLY */
>> +
>> +#endif /* CONFIG_TCG */
>>  
>>  /**
>>   * arm_clear_exclusive: clear the exclusive monitor
>> diff --git a/target/arm/tcg/helper.c b/target/arm/tcg/helper.c
>> index 0e1a3b9421..beb8a5deed 100644
>> --- a/target/arm/tcg/helper.c
>> +++ b/target/arm/tcg/helper.c
>> @@ -10040,11 +10040,13 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>env->exception.syndrome);
>>  }
>>  
>> +#ifndef CONFIG_USER_ONLY
>>  if (arm_is_psci_call(cpu, cs->exception_index)) {
>>  arm_handle_psci_call(cpu);
>>  qemu_log_mask(CPU_LOG_INT, "...handled as PSCI call\n");
>>  return;
>>  }
>> +#endif /* CONFIG_USER_ONLY */
>>  
>>  /*
>>   * Semihosting semantics depend on the register width of the code
>> diff --git a/target/arm/psci.c b/target/arm/tcg/softmmu/psci.c
>> similarity index 100%
>> rename from target/arm/psci.c
>> rename to target/arm/tcg/softmmu/psci.c
>> diff --git a/target/arm/tcg/user/psci.c b/target/arm/tcg/user/psci.c
>> new file mode 100644
>> index 00..f3e293c516
>> --- /dev/null
>> +++ b/target/arm/tcg/user/psci.c
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Copyright (C) 2014 - Linaro
>> + * Author: Rob Herring 
>> + *
>> + *  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 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  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 "cpu.h"
>> +#include "internals.h"
>> +
>> +bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
>> +{
>> +return false;
>> +}
>> diff --git a/target/arm/meson.build b/target/arm/meson.build
>> index 0172937b40..3d23a6c687 100644
>> --- a/target/arm/meson.build
>> +++ b/target/arm/meson.build
>> @@ -19,7 +19,6 @@ arm_softmmu_ss.add(files(
>>'arm-powerctl.c',
>>'machine.c',
>>'monitor.c',
>> -  'psci.c',
>>  ))
>>  arm_user_ss = ss.source_set()
>>  
>> diff --git a/target/arm/tcg/meson.build b/target/arm/tcg/meson.build
>> index 3b4146d079..4d9ed4b9cf 100644
>> --- a/target/arm/tcg/meson.build
>> +++ b/target/arm/tcg/meson.build
>> @@ -36,3 +36,6 @@ arm_ss.add(when: ['TARGET_AARCH64','C

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Max Reitz]

On 22.02.21 19:15, Daniel P. Berrangé wrote:

On Fri, Feb 19, 2021 at 03:09:43PM +0100, Philippe Mathieu-Daudé wrote:

On 2/19/21 12:07 PM, Max Reitz wrote:

On 13.02.21 22:54, Fam Zheng wrote:

On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:

The null-co driver doesn't zeroize buffer in its default config,
because it is designed for testing and tests want to run fast.
However this confuses security researchers (access to uninit
buffers).


I'm a little surprised.

Is changing default the only way to fix this? I'm not opposed to
changing the default but I'm not convinced this is the easiest way.
block/nvme.c also doesn't touch the memory, but defers to the device
DMA, why doesn't that confuse the security checker?


Generally speaking, there is a balance between security and performance.
We try to provide both, but when we can't, my understanding is security
is more important.

Customers expect a secure product. If they prefer performance and
at the price of security, it is also possible by enabling an option
that is not the default.

I'm not sure why you mention block/nvme here. I have the understanding
the null-co driver is only useful for testing. Are there production
cases where null-co is used?


Do we have any real world figures for the performance of null-co
with & without  zero'ing ?  Before worrying about a tradeoff of
security vs performance, it'd be good to know if there is actually
a real world performance problem in the first place. Personally I'd
go for zero'ing by defualt unless the performance hit was really
bad.


AFAIU, null-co is only used for testing, be it to just create some block 
nodes in the iotests, or perhaps for performance testing where you want 
to get the minimal roundtrip time through the block layer.  So there is 
no "real world performance problem", because there is no real world use 
of null-co or null-aio.  At least there shouldn’t be.


That begs the question of whether read-zeroes=off even makes sense, and 
I think it absolutely does.


In cases where we have a test that just wants a simple block node that 
doesn’t use disk space, the memset() can’t be noticeable.  But it’s just 
a test, so do we even need the memset()?  Strictly speaking, perhaps 
not, but if someone is to run it via Valgrind or something, they may get 
false positives, so just doing the memset() is the right thing to do.


For performance tests, it must be possible to set read-zeroes=off, 
because even though “that memset() isn’t noticeable in a functional 
test”, in a hard-core performance test, it will be.


So we need a switch.  It should default to memset(), because (1) making 
tools like Valgrind happy seems like a reasonable objective to me, and 
(2) in the majority of cases, the memset() cannot have a noticeable impact.


Max

Re: [RFC v1 05/38] target/arm: wrap arm_cpu_exec_interrupt in CONFIG_TCG

[Claudio Fontana]

On 2/22/21 4:54 PM, Richard Henderson wrote:
> On 2/22/21 12:31 AM, Claudio Fontana wrote:
>> actually this is a fix for an error I introduced when adding TCGOps:
>>
>> arm_cpu_exec_interrupt should be wrapped in the ifdef, as it uses tcg_ops, 
>> which is TCG-only.
>> Maybe I should extract this and make it a standalone fix.
>>
>> Currently, there is no real issue because the non-TCG build is not working 
>> for ARM anyway,
>> and that's why the issue went undetected.
> 
> If it doesn't cause a current problem, then let's not bother with a standalone
> patch.  But I wouldn't bother with the ifdef either, but instead move the
> function to an appropriate file.
> 
> 
> r~
> 

Yes, agree.

Ciao,

Claudio

Re: Plugin Address Translations Inconsistent/Incorrect?

[Alex Bennée]

Peter Maydell  writes:

> On Mon, 22 Feb 2021 at 19:53, Alex Bennée  wrote:
>> It certainly is by design. The comment for the helper states:
>>
>>   /*
>>* The following additional queries can be run on the hwaddr structure
>>* to return information about it. For non-IO accesses the device
>>* offset will be into the appropriate block of RAM.
>>*/
>
> That sounds like we're exposing ram_addrs to the plugin. Are we?
> I'm not sure that's a good idea, as they're not a guest-relevant
> construct.

We currently expose qemu_ram_addr_from_host for RAM blocks. Are you
saying we should translate that to the direct physical address mapping?
If we do that for RAM should we be doing the same for IO addresses?

>
> thanks
> -- PMM


-- 
Alex Bennée

Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c

[Claudio Fontana]

On 2/22/21 6:29 PM, Alex Bennée wrote:
> 
> Claudio Fontana  writes:
> 
>> From: Claudio Fontana 
>>
>> Signed-off-by: Claudio Fontana 
>> ---
>>  target/arm/internals.h   |   9 ++-
>>  target/arm/cpu-softmmu.c | 134 +++
>>  target/arm/cpu.c |  95 ---
>>  target/arm/meson.build   |   1 +
>>  4 files changed, 143 insertions(+), 96 deletions(-)
>>  create mode 100644 target/arm/cpu-softmmu.c
>>
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index 6384461177..6589b63ebc 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -1196,4 +1196,11 @@ static inline uint64_t 
>> useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
>>  return ptr;
>>  }
>>  
>> -#endif
>> +#ifndef CONFIG_USER_ONLY
>> +void arm_cpu_set_irq(void *opaque, int irq, int level);
>> +void arm_cpu_kvm_set_irq(void *opaque, int irq, int level);
>> +bool arm_cpu_virtio_is_big_endian(CPUState *cs);
>> +uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri);
>> +#endif /* !CONFIG_USER_ONLY */
>> +
>> +#endif /* TARGET_ARM_INTERNALS_H */
>> diff --git a/target/arm/cpu-softmmu.c b/target/arm/cpu-softmmu.c
>> new file mode 100644
>> index 00..263d1fc588
>> --- /dev/null
>> +++ b/target/arm/cpu-softmmu.c
>> @@ -0,0 +1,134 @@
>> +/*
>> + * QEMU ARM CPU
> 
> I guess apropos the discussion earlier it's really cpu-sysemu.c and we
> could expand the header comment.
> 
>   QEMU ARM CPU - Helpers for system emulation and KVM only
> 
> 
> 
> Otherwise:
> 
> Reviewed-by: Alex Bennée 
> 

Should I rename all *softmmu in the series to "sysemu"?

I wonder if we should take the issue of sysemu/system/softmmu topic into a 
separate series.
Currently basically starting from the build system already, "softmmu", sysemu 
and system are treated as a single thing, and the convention from build system 
and directories seems to be "softmmu",
while from the header files we get "sysemu/".

I agree that this is not a sufficient model to account for the new feature that 
Richard wants to develop,
I just suggest we could also consider tackling this separately, with a pass 
through the whole code, gathering more input in the context of a dedicated 
series.

What do you think? Also Paolo, any comments, since softmmu is all over meson?

Ciao,

Claudio

Re: [PATCH v4 2/5] hw/arm: xlnx-zynqmp: Clean up coding convention issues

[Edgar E. Iglesias]

On Mon, Feb 22, 2021 at 09:05:11PM +0800, Bin Meng wrote:
> From: Xuzhou Cheng 
> 
> There are some coding convention warnings in xlnx-zynqmp.c and
> xlnx-zynqmp.h, as reported by:
> 
>   $ ./scripts/checkpatch.pl include/hw/arm/xlnx-zynqmp.h
>   $ ./scripts/checkpatch.pl hw/arm/xlnx-zynqmp.c
> 
> Let's clean them up.
> 
> Signed-off-by: Xuzhou Cheng 
> Signed-off-by: Bin Meng 

Reviewed-by: Edgar E. Iglesias 



> 
> ---
> 
> Changes in v4:
> - remove one change that is not a checkpatch warning
> 
>  include/hw/arm/xlnx-zynqmp.h | 3 ++-
>  hw/arm/xlnx-zynqmp.c | 9 ++---
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index 6f45387a17..be15cc8814 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -60,7 +60,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPState, XLNX_ZYNQMP)
>  
>  #define XLNX_ZYNQMP_GIC_REGIONS 6
>  
> -/* ZynqMP maps the ARM GIC regions (GICC, GICD ...) at consecutive 64k 
> offsets
> +/*
> + * ZynqMP maps the ARM GIC regions (GICC, GICD ...) at consecutive 64k 
> offsets
>   * and under-decodes the 64k region. This mirrors the 4k regions to every 4k
>   * aligned address in the 64k region. To implement each GIC region needs a
>   * number of memory region aliases.
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 881847255b..49465a2794 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -301,11 +301,13 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
> **errp)
>  
>  ram_size = memory_region_size(s->ddr_ram);
>  
> -/* Create the DDR Memory Regions. User friendly checks should happen at
> +/*
> + * Create the DDR Memory Regions. User friendly checks should happen at
>   * the board level
>   */
>  if (ram_size > XLNX_ZYNQMP_MAX_LOW_RAM_SIZE) {
> -/* The RAM size is above the maximum available for the low DDR.
> +/*
> + * The RAM size is above the maximum available for the low DDR.
>   * Create the high DDR memory region as well.
>   */
>  assert(ram_size <= XLNX_ZYNQMP_MAX_RAM_SIZE);
> @@ -526,7 +528,8 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
> **errp)
>  SysBusDevice *sbd = SYS_BUS_DEVICE(&s->sdhci[i]);
>  Object *sdhci = OBJECT(&s->sdhci[i]);
>  
> -/* Compatible with:
> +/*
> + * Compatible with:
>   * - SD Host Controller Specification Version 3.00
>   * - SDIO Specification Version 3.0
>   * - eMMC Specification Version 4.51
> -- 
> 2.25.1
> 

Re: [PATCH v4 3/5] hw/arm: xlnx-zynqmp: Connect a Xilinx CSU DMA module for QSPI

[Edgar E. Iglesias]

On Mon, Feb 22, 2021 at 09:05:12PM +0800, Bin Meng wrote:
> From: Xuzhou Cheng 
> 
> Add a Xilinx CSU DMA module to ZynqMP SoC, and connent the stream
> link of GQSPI to CSU DMA.
> 
> Signed-off-by: Xuzhou Cheng 
> Signed-off-by: Bin Meng 
> 
> ---
> 
> Changes in v4:
> - Rename "csu_dma" to "qspi_dma"
> 
> Changes in v3:
> - new patch: xlnx-zynqmp: Add XLNX CSU DMA module
> 
>  include/hw/arm/xlnx-zynqmp.h |  2 ++
>  hw/arm/xlnx-zynqmp.c | 14 ++
>  hw/arm/Kconfig   |  1 +
>  3 files changed, 17 insertions(+)
> 
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index be15cc8814..2edeed911c 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -35,6 +35,7 @@
>  #include "target/arm/cpu.h"
>  #include "qom/object.h"
>  #include "net/can_emu.h"
> +#include "hw/dma/xlnx_csu_dma.h"
>  
>  #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
>  OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPState, XLNX_ZYNQMP)
> @@ -108,6 +109,7 @@ struct XlnxZynqMPState {
>  XlnxZynqMPRTC rtc;
>  XlnxZDMA gdma[XLNX_ZYNQMP_NUM_GDMA_CH];
>  XlnxZDMA adma[XLNX_ZYNQMP_NUM_ADMA_CH];
> +XlnxCSUDMA qspi_dma;
>  
>  char *boot_cpu;
>  ARMCPU *boot_cpu_ptr;
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 49465a2794..30f43dfda2 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -50,6 +50,7 @@
>  #define QSPI_ADDR   0xff0f
>  #define LQSPI_ADDR  0xc000
>  #define QSPI_IRQ15
> +#define QSPI_DMA_ADDR   0xff0f0800
>  
>  #define DP_ADDR 0xfd4a
>  #define DP_IRQ  113
> @@ -63,6 +64,8 @@
>  #define RTC_ADDR0xffa6
>  #define RTC_IRQ 26
>  
> +
> +

These blank lines look un-related, if you remove them, this looks good to me:

Reviewed-by: Edgar E. Iglesias 



>  #define SDHCI_CAPABILITIES  0x280737ec6481 /* Datasheet: UG1085 (v1.7) */
>  
>  static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS] = {
> @@ -284,6 +287,8 @@ static void xlnx_zynqmp_init(Object *obj)
>  for (i = 0; i < XLNX_ZYNQMP_NUM_ADMA_CH; i++) {
>  object_initialize_child(obj, "adma[*]", &s->adma[i], TYPE_XLNX_ZDMA);
>  }
> +
> +object_initialize_child(obj, "qspi-dma", &s->qspi_dma, 
> TYPE_XLNX_CSU_DMA);
>  }
>  
>  static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> @@ -643,6 +648,15 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
> **errp)
>  sysbus_connect_irq(SYS_BUS_DEVICE(&s->adma[i]), 0,
> gic_spi[adma_ch_intr[i]]);
>  }
> +
> +if (!sysbus_realize(SYS_BUS_DEVICE(&s->qspi_dma), errp)) {
> +return;
> +}
> +
> +sysbus_mmio_map(SYS_BUS_DEVICE(&s->qspi_dma), 0, QSPI_DMA_ADDR);
> +sysbus_connect_irq(SYS_BUS_DEVICE(&s->qspi_dma), 0, gic_spi[QSPI_IRQ]);
> +object_property_set_link(OBJECT(&s->qspi), "stream-connected-dma",
> + OBJECT(&s->qspi_dma), errp);
>  }
>  
>  static Property xlnx_zynqmp_props[] = {
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 4e6f4ffe90..27ec10f89b 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -353,6 +353,7 @@ config XLNX_ZYNQMP_ARM
>  select SSI_M25P80
>  select XILINX_AXI
>  select XILINX_SPIPS
> +select XLNX_CSU_DMA
>  select XLNX_ZYNQMP
>  select XLNX_ZDMA
>  
> -- 
> 2.25.1
> 

Re: [PATCH v6 09/12] softmmu/physmem: Don't use atomic operations in ram_block_discard_(disable|require)

[David Hildenbrand]

On 22.02.21 18:32, Paolo Bonzini wrote:

On 22/02/21 16:38, David Hildenbrand wrote:

On 22.02.21 15:02, Paolo Bonzini wrote:

On 22/02/21 14:33, David Hildenbrand wrote:

Also, uncoordinated require is unused, and therefore uncoordinated
disable is also never going to block anything.  Does it make sense to
keep it in the API?


Right, "ram_block_discard_require()" is not used yet. I am planning on
using it in virtio-balloon context at some point, but can remove it for
now to simplify.

ram_block_uncoordinated_discard_disable(), however, will block
virtio-balloon already via ram_block_discard_is_disabled(). (yes,
virtio-balloon is ugly)


Oops, I missed that API.

Does it make sense to turn the API inside out, with the
coordinated/uncoordinated choice as an argument and the start/finish
choice in the name?

enum {
   RAM_DISCARD_ALLOW_COORDINATED = 1,
};



Any reason to go with an enum/flags for this case and not "bool
allow_coordinated" ?


I find it slightly easier to remember the meaning of true for "bool
coordinated" than for "bool allow_coordinated".  I don't like the API
below that much, but having both RAM_DISCARD_ALLOW_COORDINATED for
disable/enable and RAM_DISCARD_SUPPORT_COORDINATED for start/finish
would be even uglier...

Paolo


bool ram_discard_disable(int flags, Error **errp);
void ram_discard_enable(int flags);
int ram_discard_start(bool coordinated, Error **errp);
void ram_discard_finish(bool coordinated);




So, the new API I propose is:

int ram_block_discard_disable(bool state)
int ram_block_uncoordinated_discard_disable(bool state)
int ram_block_discard_require(bool state)
int ram_block_coordinated_discard_require(bool state);
bool ram_block_discard_is_disabled(void);
bool ram_block_discard_is_required(void);


Some points (because I thought about this API a bit when I came up with it):

1. I'd really like to keep the functionality of 
ram_block_discard_is_disabled() / ram_block_discard_is_required(). I'd 
assume you just didn't include it in your proposal.


2. I prefer the "require" wording over "start/finish". Start/finish 
sounds like it's a temporary thing like a transaction. For example 
"ram_block_discard_is_started()" sounds misleading to me


3. "ram_discard_enable()" sounds a bit misleading to me as well. We're 
not actually enabling anything, we're not disabling it anymore.


4. I don't think returning an "Error **errp" does make a lot of sense here.


Unless there is real need for a major overhaul I'd like to keep it to 
minor changes.


--
Thanks,

David / dhildenb

Re: [PATCH v4 5/5] hw/ssi: xilinx_spips: Remove DMA related dead codes from zynqmp_spips

[Edgar E. Iglesias]

On Mon, Feb 22, 2021 at 09:05:14PM +0800, Bin Meng wrote:
> From: Xuzhou Cheng 
> 
> Now that the Xilinx CSU DMA model is implemented, the existing
> DMA related dead codes in the ZynqMP QSPI are useless and should
> be removed. The maximum register number is also updated to only
> include the QSPI registers.
> 
> Signed-off-by: Xuzhou Cheng 
> Signed-off-by: Bin Meng 

Reviewed-by: Edgar E. Iglesias 


> 
> ---
> 
> Changes in v4:
> - Modify XLNX_ZYNQMP_SPIPS_R_MAX
> 
> Changes in v3:
> - new patch: xilinx_spips: Remove DMA related code from zynqmp_qspips
> 
>  include/hw/ssi/xilinx_spips.h |  2 +-
>  hw/ssi/xilinx_spips.c | 10 --
>  2 files changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h
> index 3eae73480e..06bfd18312 100644
> --- a/include/hw/ssi/xilinx_spips.h
> +++ b/include/hw/ssi/xilinx_spips.h
> @@ -34,7 +34,7 @@
>  typedef struct XilinxSPIPS XilinxSPIPS;
>  
>  #define XLNX_SPIPS_R_MAX(0x100 / 4)
> -#define XLNX_ZYNQMP_SPIPS_R_MAX (0x830 / 4)
> +#define XLNX_ZYNQMP_SPIPS_R_MAX (0x200 / 4)
>  
>  /* Bite off 4k chunks at a time */
>  #define LQSPI_CACHE_SIZE 1024
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 8a0cc22d42..1e9dba2039 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -195,13 +195,6 @@
>  #define R_GQSPI_MOD_ID(0x1fc / 4)
>  #define R_GQSPI_MOD_ID_RESET  (0x10a)
>  
> -#define R_QSPIDMA_DST_CTRL (0x80c / 4)
> -#define R_QSPIDMA_DST_CTRL_RESET   (0x803ffa00)
> -#define R_QSPIDMA_DST_I_MASK   (0x820 / 4)
> -#define R_QSPIDMA_DST_I_MASK_RESET (0xfe)
> -#define R_QSPIDMA_DST_CTRL2(0x824 / 4)
> -#define R_QSPIDMA_DST_CTRL2_RESET  (0x081bfff8)
> -
>  /* size of TXRX FIFOs */
>  #define RXFF_A  (128)
>  #define TXFF_A  (128)
> @@ -417,9 +410,6 @@ static void xlnx_zynqmp_qspips_reset(DeviceState *d)
>  s->regs[R_GQSPI_GPIO] = 1;
>  s->regs[R_GQSPI_LPBK_DLY_ADJ] = R_GQSPI_LPBK_DLY_ADJ_RESET;
>  s->regs[R_GQSPI_MOD_ID] = R_GQSPI_MOD_ID_RESET;
> -s->regs[R_QSPIDMA_DST_CTRL] = R_QSPIDMA_DST_CTRL_RESET;
> -s->regs[R_QSPIDMA_DST_I_MASK] = R_QSPIDMA_DST_I_MASK_RESET;
> -s->regs[R_QSPIDMA_DST_CTRL2] = R_QSPIDMA_DST_CTRL2_RESET;
>  s->man_start_com_g = false;
>  s->gqspi_irqline = 0;
>  xlnx_zynqmp_qspips_update_ixr(s);
> -- 
> 2.25.1
> 

Re: A brief look at deprecating our JSON extensions over RFC 8259

[Markus Armbruster]

Paolo Bonzini  writes:

> On 22/02/21 16:24, Daniel P. Berrangé wrote:
>> This problem isn't unique to QEMU. Any app using JSON from the
>> shell will have the tedium of quote escaping. JSON is incredibly
>> widespread and no other apps felt it neccessary to introduce single
>> quoting support, because the benefit doesn't outweigh the interop
>> problem it introduces.
>
> The quotes were introduced for C code (and especially qtest), not for
> the shell.  We have something like
>
> response = qmp("{ 'execute': 'qom-get', 'arguments': { 'path': %s, "
>"'property': 'temperature' } }", id);
>
> These are sent to QEMU as double-quoted strings (the single-quoted
> JSON is parsed to get interpolation and printed back; commit
> 563890c7c7, "libqtest: escape strings in QMP commands, fix leak",
> 2014-07-01). However, doing the interpolation requires a parser that
> recognizes the single-quoted strings.

Doing interpolation requires a parser that recognizes %-sequences.
Single quote support isn't *required*, but quite desirable to let us
avoid leaning toothpick syndrome (LTS).

Example: compare the above to

  response = qmp("{ \"execute\": \"qom-get\", \"arguments\": { \"path\": 
%s, "
 "\"property\": \"temperature\" } }", id);

We kept the interpolation extension out of the external interfaces, but
not the single quotes.

> Markus, did you rebuild the qtests after disabling single-quoted
> strings?  "make check-qtest-x86_64" would have rebuilt them, but I'm 
> confused by the results.

I ran "make check" and looked at the failures:

* check-qjson.c

  - escaped_string() covers \'.  Naturally, this fails.

  - escaped_string() and utf8_string() try every string twice, first in
double quotes, then in single quotes.  Naturally, the latter fails.

  - string_with_quotes() tests unquoted single quote in double-quoted
string, and unquoted double quote in single-quoted string.
Naturally, the latter fails.

  - large_dict() and simple_whitespace() use single quotes to avoid LTS.

  This is the test my "The unit test testing the JSON parser is of
  course excused" referred to.

* test-qobject-input-visitor.c
* qtest/qmp-test.c

  More LTS avoidance.

  This is "The remaining qtest and the unit test could perhaps be
  dismissed as atypical use of QEMU from C."

* tests/qemu-iotests/

  Unlike the tests above, these use *external* interfaces.

  In shell, we need to use double quotes to get parameter expansion.  We
  then use single quotes to avoid LTS.

  The Python code has less excuse, I think.

Still confused?

Re: [PATCH qemu v13] spapr: Implement Open Firmware client interface

[BALATON Zoltan]

On Tue, 23 Feb 2021, David Gibson wrote:

On Tue, Feb 23, 2021 at 04:01:00PM +1100, Alexey Kardashevskiy wrote:

On 23/02/2021 14:07, David Gibson wrote:

On Tue, Feb 09, 2021 at 10:02:52PM +1100, Alexey Kardashevskiy wrote:

  #endif /* HW_SPAPR_H */


VOF is pretty much inherently papr specific, so I'm not really seeing
a clear rationale for the distinction between vof_*() things and
spapr_vof_*() things.


spapr_vof_ uses SpaprMachineState, vof_ does not and can be used ... I do
not know... for macs? freescale?

I thought it might be useful for whatever Balaton Zoltran wanted to use it
for.


Hmm, I hadn't thought of that sort of application.  I guess that's a
point.

I feel like the call for vof is less for those platforms, because they
admit a full in-guest firmware implementation, whereas the paravirt
nature of papr means that some components have to go into the
hypervisor.


The immediate plan is to try to use vof for pegasos2 as replacement 
firmware to be able to at least boot guests without needing a 
non-distributable firmware blob (as mentioned and further explained on 
https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2). This seems to be 
a better option than reimplementing its firmware which would then be yet 
another OpenFirmware implementation in QEMU. I was waiting for vof to get 
in before trying to actually implement that to avoid to need too much 
rebasing but in theory it could work. It's true we don't have a hypervisor 
on those platforms but could have one at least while the firmware is 
running to avoid having to do everything in guest firmware. Most guests 
just query the device tree via client interface and vof should be enough 
for that. I'm not sure about RTAS but that's a small problem if this 
allows to boot guests without having to write a whole guest open firmware 
implementation.



Vof is pretty unavoidably tied to spapr, so I don't think you really
need an interface to abstract this.


Balaton Zoltran asked for that, more or less :)


This was my original comment:
https://lists.nongnu.org/archive/html/qemu-ppc/2020-12/msg00063.html

One reason is to keep vof reusable but also to make it clear what are the 
changes to spapr that was previously hard to distinguish from what's part 
of vof itself.


Regards,
BALATON Zoltan

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Claudio Fontana]

On 2/22/21 4:34 PM, Alex Bennée wrote:
> 
> Claudio Fontana  writes:
> 
>> From: Claudio Fontana 
>>
>> KVM has its own cpu->kvm_vtime.
>>
>> Adjust cpu vmstate by putting unused fields instead of the
>> VMSTATE_TIMER_PTR when TCG is not available.
>>
>> Signed-off-by: Claudio Fontana 
>> ---
>>  target/arm/cpu.c | 4 +++-
>>  target/arm/machine.c | 5 +
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 1d81a1e7ac..b929109054 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
>> **errp)
>>  }
>>  }
>>  
>> +#ifdef CONFIG_TCG
>>  {
>>  uint64_t scale;
>>  
>> @@ -1347,7 +1348,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
>> **errp)
>>  cpu->gt_timer[GTIMER_HYPVIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
>>arm_gt_hvtimer_cb, cpu);
>>  }
>> -#endif
>> +#endif /* CONFIG_TCG */
>> +#endif /* !CONFIG_USER_ONLY */
>>  
>>  cpu_exec_realizefn(cs, &local_err);
>>  if (local_err != NULL) {
>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>> index 666ef329ef..13d7c6d930 100644
>> --- a/target/arm/machine.c
>> +++ b/target/arm/machine.c
>> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>>  VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>>  VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>>  VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>> +#ifdef CONFIG_TCG
>>  VMSTATE_TIMER_PTR(gt_timer[GTIMER_PHYS], ARMCPU),
>>  VMSTATE_TIMER_PTR(gt_timer[GTIMER_VIRT], ARMCPU),
>> +#else
>> +VMSTATE_UNUSED(sizeof(QEMUTimer *)),
>> +VMSTATE_UNUSED(sizeof(QEMUTimer *)),
>> +#endif /* CONFIG_TCG */
> 
> I'm not sure this is correct - VMSTATE_TIMER_PTR chases the links to
> just expose expired time but QEMUTimer has more in it than that. Paolo


I am not sure I follow can you state more precisely where the issue could be?

it's not a VMSTATE_TIMER, it's a VMSTATE_TIMER_PTR,
it ends up in VMSTATE_POINTER where a single pointer is assigned;

so if we don't use gt_timer at all (as is the case with !CONFIG_TCG), we just
need to ensure that an unused number is there to assign, migrating from old to 
new version?


> suggested a straight VMSTATE_UNUSED(8) on IRC but I wonder if it would
> be better to have a VMSTATE_UNUSED_TIMER?
> 
> I don't think there is an impact for Xen because I'm fairly certain
> migration isn't a thing we do - but I'll double check.
> 

Thanks Alex, that would be helpful,
if Xen uses gt_timer in any way I would not want to unwillingly break it.

Thanks,

Claudio

Re: [PATCH v3 5/6] hw/pci-host: Add emulation of Marvell MV64361 PPC system controller

[BALATON Zoltan]

On Tue, 23 Feb 2021, David Gibson wrote:

On Mon, Feb 22, 2021 at 04:22:06PM +0100, BALATON Zoltan wrote:

The Marvell Discovery II aka. MV64361 is a PowerPC system controller
chip that is used on the pegasos2 PPC board. This adds emulation of it
that models the device enough to boot guests on this board. The
mv643xx.h header with register definitions is taken from Linux 4.15.10
only fixing end of line white space errors and removing not needed
parts, it's otherwise keeps Linux formatting.

Signed-off-by: BALATON Zoltan 


This needs to go before the previous patch to avoid bisect breakage,
doesn't it?


Yes this is 5/6 and that's 6/6 but sending the patches at once sometimes 
mixes up their order if you sort by receive time.


Regards,
BALATON Zoltan

Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c

[Philippe Mathieu-Daudé]

On 2/23/21 9:55 AM, Claudio Fontana wrote:
> On 2/22/21 6:29 PM, Alex Bennée wrote:
>>
>> Claudio Fontana  writes:
>>
>>> From: Claudio Fontana 
>>>
>>> Signed-off-by: Claudio Fontana 
>>> ---
>>>  target/arm/internals.h   |   9 ++-
>>>  target/arm/cpu-softmmu.c | 134 +++
>>>  target/arm/cpu.c |  95 ---
>>>  target/arm/meson.build   |   1 +
>>>  4 files changed, 143 insertions(+), 96 deletions(-)
>>>  create mode 100644 target/arm/cpu-softmmu.c
>>>
>>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>>> index 6384461177..6589b63ebc 100644
>>> --- a/target/arm/internals.h
>>> +++ b/target/arm/internals.h
>>> @@ -1196,4 +1196,11 @@ static inline uint64_t 
>>> useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
>>>  return ptr;
>>>  }
>>>  
>>> -#endif
>>> +#ifndef CONFIG_USER_ONLY
>>> +void arm_cpu_set_irq(void *opaque, int irq, int level);
>>> +void arm_cpu_kvm_set_irq(void *opaque, int irq, int level);
>>> +bool arm_cpu_virtio_is_big_endian(CPUState *cs);
>>> +uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri);
>>> +#endif /* !CONFIG_USER_ONLY */
>>> +
>>> +#endif /* TARGET_ARM_INTERNALS_H */
>>> diff --git a/target/arm/cpu-softmmu.c b/target/arm/cpu-softmmu.c
>>> new file mode 100644
>>> index 00..263d1fc588
>>> --- /dev/null
>>> +++ b/target/arm/cpu-softmmu.c
>>> @@ -0,0 +1,134 @@
>>> +/*
>>> + * QEMU ARM CPU
>>
>> I guess apropos the discussion earlier it's really cpu-sysemu.c and we
>> could expand the header comment.
>>
>>   QEMU ARM CPU - Helpers for system emulation and KVM only
>>
>> 
>>
>> Otherwise:
>>
>> Reviewed-by: Alex Bennée 
>>
> 
> Should I rename all *softmmu in the series to "sysemu"?
> 
> I wonder if we should take the issue of sysemu/system/softmmu topic into a 
> separate series.
> Currently basically starting from the build system already, "softmmu", sysemu 
> and system are treated as a single thing, and the convention from build 
> system and directories seems to be "softmmu",
> while from the header files we get "sysemu/".
> 
> I agree that this is not a sufficient model to account for the new feature 
> that Richard wants to develop,
> I just suggest we could also consider tackling this separately, with a pass 
> through the whole code, gathering more input in the context of a dedicated 
> series.
> 
> What do you think?

This is a valid reasoning. However I have my doubts "doing
that later" will ever be done/finished (not related to you
Claudio in particular, but with dealing with all subsystems).

Personally I'd rather see this sorted out with the arm target
then once done propose it as an example to the other ones.
You already considered the most complex cases, x86 and arm :)

> Also Paolo, any comments, since softmmu is all over meson?
> 
> Ciao,
> 
> Claudio
> 

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 2/22/21 5:08 PM, Richard Henderson wrote:
> On 2/22/21 12:43 AM, Claudio Fontana wrote:
>> Regarding terminology, I think the mismatch is throughout the code right?
> 
> Yes, e.g. the top-level softmmu/.
> 
> 
>> So many of the existing "softmmu" files and directories should actually be
>> called "system", or "sysemu" to match include/sysemu right?
> Yes, please.  Let's not add new mistakes.
> 
>> Maybe it would be good to have clear documentation about this in devel/ to
>> use as a reference and end-goal, and then we could do a pass of the whole
>> code to fix the discrepancies in the use of the terms?
> I suppose.  Where do you suggest adding those couple of sentences?
> 
> 
> r~
> 

I'd say docs/devel/code-config-macros, or something like that.

I think for a new developer coming into the codebase, the big hurdle is trying 
to figure out why there are so many CONFIG_,

in which files and directories they can and should be used,

which effects they have and the most dangerous pitfalls (f.e. header files and 
mixed common/specific modules), ...

Ciao,

Claudio

Re: [PATCH v3 1/6] vt82c686: Implement control of serial port io ranges via config regs

[BALATON Zoltan]

On Tue, 23 Feb 2021, David Gibson wrote:

On Mon, Feb 22, 2021 at 04:22:06PM +0100, BALATON Zoltan wrote:

In VIA super south bridge the io ranges of superio components
(parallel and serial ports and FDC) can be controlled by superio
config registers to set their base address and enable/disable them.
This is not easy to implement in QEMU because ISA emulation is only
designed to set io base address once on creating the device and io
ranges are registered at creation and cannot easily be disabled or
moved later.

In this patch we hack around that but only for serial ports because
those have a single io range at port base that's relatively easy to
handle and it's what guests actually use and set address different
than the default.

We do not attempt to handle controlling the parallel and FDC regions
because those have multiple io ranges so handling them would be messy
and guests either don't change their deafult or don't care. We could
even get away with disabling and not emulating them, but since they
are already there, this patch leaves them mapped at their default
address just in case this could be useful for a guest in the future.

Signed-off-by: BALATON Zoltan 


The maintainers of the hw/isa/vt82c686.c should probably be CCed on this.


He is (but may not be obvious because the accent in Phil's name is not 
handled well by my mail host so to avoid misspelling his name I've just 
omitted it). And he also said these should go in via some other tree:


https://lists.nongnu.org/archive/html/qemu-devel/2021-02/msg06551.html

He may still review or ack these but I guess he'd need some time.

I'll do the other changes you suggested.

Regards,
BALATON Zoltan


---
 hw/isa/vt82c686.c | 84 +--
 1 file changed, 82 insertions(+), 2 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 5db9b1706c..98bd57a074 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -252,8 +252,24 @@ static const TypeInfo vt8231_pm_info = {
 typedef struct SuperIOConfig {
 uint8_t regs[0x100];
 MemoryRegion io;
+ISASuperIODevice *superio;
+MemoryRegion *serial_io[SUPERIO_MAX_SERIAL_PORTS];
 } SuperIOConfig;

+static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
+int offs)
+{
+MemoryRegion *subregion, *mr = NULL;
+
+QTAILQ_FOREACH(subregion, &parent->subregions, subregions_link) {
+if (subregion->addr == offs) {
+mr = subregion;
+break;
+}
+}
+return mr;
+}
+
 static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
   unsigned size)
 {
@@ -279,7 +295,53 @@ static void superio_cfg_write(void *opaque, hwaddr addr, 
uint64_t data,
 case 0xfd ... 0xff:
 /* ignore write to read only registers */
 return;
-/* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
+case 0xe2:
+{
+data &= 0x1f;
+if (data & BIT(2)) { /* Serial port 1 enable */
+ISADevice *dev = sc->superio->serial[0];
+if (!memory_region_is_mapped(sc->serial_io[0])) {
+memory_region_add_subregion(isa_address_space_io(dev),
+dev->ioport_id, sc->serial_io[0]);
+}
+} else {
+MemoryRegion *io = isa_address_space_io(sc->superio->serial[0]);
+if (memory_region_is_mapped(sc->serial_io[0])) {
+memory_region_del_subregion(io, sc->serial_io[0]);
+}
+}
+if (data & BIT(3)) { /* Serial port 2 enable */
+ISADevice *dev = sc->superio->serial[1];
+if (!memory_region_is_mapped(sc->serial_io[1])) {
+memory_region_add_subregion(isa_address_space_io(dev),
+dev->ioport_id, sc->serial_io[1]);
+}
+} else {
+MemoryRegion *io = isa_address_space_io(sc->superio->serial[1]);
+if (memory_region_is_mapped(sc->serial_io[1])) {
+memory_region_del_subregion(io, sc->serial_io[1]);
+}
+}
+break;
+}
+case 0xe7: /* Serial port 1 io base address */
+{
+data &= 0xfe;
+sc->superio->serial[0]->ioport_id = data << 2;
+if (memory_region_is_mapped(sc->serial_io[0])) {
+memory_region_set_address(sc->serial_io[0], data << 2);
+}
+break;
+}
+case 0xe8: /* Serial port 2 io base address */
+{
+data &= 0xfe;
+sc->superio->serial[1]->ioport_id = data << 2;
+if (memory_region_is_mapped(sc->serial_io[1])) {
+memory_region_set_address(sc->serial_io[1], data << 2);
+}
+break;
+}
 default:
 qemu_log_mask(LOG_UNIMP,
   "via_superio_cfg: unimplemented register 0x%x\n", idx);
@@ -385,6 +447,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 2/22/21 8:00 PM, Alex Bennée wrote:
> 
> Claudio Fontana  writes:
> 
>> Hi all,
>>
>> this is an experiment, a cleanup based on and requiring the series
>> "i386 cleanup PART 2":
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05935.html
>>
>> The goal is to split the code between TCG-only and non-TCG code,
>> fixing the KVM-only build (configure --disable-tcg),
>>
>> and laying the ground for further cleanups and the use of the
>> new accel objects in the hierarchy to specialize the cpu
>> according to the accelerator.
>>
>> This is known to be an early state, with probably a lot of work
>> still needed.
> 
> Well early work is looking pretty good:
> 
>   18:59:22 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh 
> qemu-system-aarch64
>   -rwxr-xr-x 1 alex alex 107M Feb 22 18:08 qemu-system-aarch64*
>   18:59:29 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh 
> ../disable.tcg/qemu-system-aarch64
>   -rwxr-xr-x 1 alex alex 76M Feb 22 17:47 ../disable.tcg/qemu-system-aarch64*
> 
> and I've tested the KVM side works well enough with a basic image.

Thanks for giving it a spin :-)

Now needs a cleanup pass for sure..

Ciao,

Claudio

> 
>>
>> I thought it could be useful to share early, especially in light
>> of the combination of this with Philippe's work on building
>> only the machines and devices compatible with KVM for arm.
>>
>> Comments welcome, thanks,
>>
>> Claudio
>>
>>
>> Claudio Fontana (38):
>>   target/arm: move translate modules to tcg/
>>   target/arm: move helpers to tcg/
>>   arm: tcg: only build under CONFIG_TCG
>>   target/arm: move psci.c into tcg/softmmu/
>>   target/arm: wrap arm_cpu_exec_interrupt in CONFIG_TCG
>>   target/arm: split off cpu-softmmu.c
>>   target/arm: split off softmmu/helper.c
>>   target/arm/tcg: split softmmu parts of v8_cp_reginfo and
>> el2_cp_reginfo
>>   target/arm/tcg: split softmmu parts of vhe_cp_reginfo
>>   target/arm/tcg: move v8_user_idregs to user-only subdir
>>   target/arm/tcg: move id_v8_user_midr_cp_reginfo to user-only subdir
>>   target/arm/tcg: move mpidr_user_cp_reginfo to user-only subdir
>>   target/arm/tcg: split vapa_cp_reginfo softmmu part
>>   target/arm: move vec_internal.h to tcg/
>>   target/arm: move aarch64_sync_32_to_64 and vv to cpu code
>>   target/arm: move arm_sctlr away from tcg helpers
>>   target/arm: move switch_mode and cpsr_read/write to cpu
>>   target/arm: split vfp state setting from tcg helpers
>>   target/arm: move register read/write to common_cpu
>>   target/arm: move arm_hcr_el2_eff to common_cpu
>>   target/arm: move cp regs definition functions to common_cpu
>>   target/arm: move arm_cpu_list to common_cpu
>>   target/arm: move arm_mmu_idx_el to common-cpu
>>   target/arm: move aa64_va_parameter_tbi,tbid,tcma and
>> arm_rebuild_hflags
>>   target/arm: move fp_exception_el outside of tcg helpers
>>   target/arm: move sve_exception_el to cpu
>>   target/arm: move sve_zcr_len_for_el to common_cpu
>>   target/arm: make arm_pmu_timer_cb TCG-only, starting tcg-stub
>>   target/arm/tcg: add write_v7m_exception to stubs
>>   target/arm: make hw_watchpoint and hw_breakpoint stuff tcg-only
>>   target/arm: move cp register write-ignore and read-as-zero to cpu
>>   target/arm: cpu: do not initialize TCG PMU for KVM
>>   target/arm: cpu: do not initialize TCG view of cpregs
>>   target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG
>>   target/arm: get-phys-addr: new module
>>   target/arm: move qmp_query_cpu_definitions to common_cpu
>>   target/arm: move arm_mmu_idx from tcg to get-phys-addr.c
>>   target/arm: move arm_cpu_do_interrupt from tcg to common code
>>
>>  meson.build   |2 +
>>  target/arm/cpu.h  |5 +
>>  target/arm/internals.h|   84 +-
>>  target/arm/tcg/helper-tcg.h   |   50 +
>>  target/arm/tcg/softmmu/trace.h|1 +
>>  target/arm/tcg/trace.h|1 +
>>  target/arm/{ => tcg}/translate-a64.h  |0
>>  target/arm/{ => tcg}/translate.h  |0
>>  target/arm/{ => tcg}/vec_internal.h   |0
>>  target/arm/{ => tcg}/a32-uncond.decode|0
>>  target/arm/{ => tcg}/a32.decode   |0
>>  target/arm/{ => tcg}/m-nocp.decode|0
>>  target/arm/{ => tcg}/neon-dp.decode   |0
>>  target/arm/{ => tcg}/neon-ls.decode   |0
>>  target/arm/{ => tcg}/neon-shared.decode   |0
>>  target/arm/{ => tcg}/sve.decode   |0
>>  target/arm/{ => tcg}/t16.decode   |0
>>  target/arm/{ => tcg}/t32.decode   |0
>>  target/arm/{ => tcg}/vfp-uncond.decode|0
>>  target/arm/{ => tcg}/vfp.decode   |0
>>  target/arm/cpu-common.c   | 1388 +
>>  target/arm/cpu-softmmu.c  | 1228 
>>  target/arm/cpu-user.c |   77 +
>>  target/arm/cpu-vfp.c  |  232 +
>>  target/arm/cpu.c   

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Philippe Mathieu-Daudé]

On 2/22/21 8:00 PM, Alex Bennée wrote:
> 
> Claudio Fontana  writes:
> 
>> Hi all,
>>
>> this is an experiment, a cleanup based on and requiring the series
>> "i386 cleanup PART 2":
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05935.html
>>
>> The goal is to split the code between TCG-only and non-TCG code,
>> fixing the KVM-only build (configure --disable-tcg),
>>
>> and laying the ground for further cleanups and the use of the
>> new accel objects in the hierarchy to specialize the cpu
>> according to the accelerator.
>>
>> This is known to be an early state, with probably a lot of work
>> still needed.
> 
> Well early work is looking pretty good:
> 
>   18:59:22 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh 
> qemu-system-aarch64
>   -rwxr-xr-x 1 alex alex 107M Feb 22 18:08 qemu-system-aarch64*
>   18:59:29 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh 
> ../disable.tcg/qemu-system-aarch64
>   -rwxr-xr-x 1 alex alex 76M Feb 22 17:47 ../disable.tcg/qemu-system-aarch64*

:~)

> 
> and I've tested the KVM side works well enough with a basic image.

Re: [PATCH v4 3/5] hw/arm: xlnx-zynqmp: Connect a Xilinx CSU DMA module for QSPI

[Bin Meng]

Hi Edgar,

On Tue, Feb 23, 2021 at 5:01 PM Edgar E. Iglesias
 wrote:
>
> On Mon, Feb 22, 2021 at 09:05:12PM +0800, Bin Meng wrote:
> > From: Xuzhou Cheng 
> >
> > Add a Xilinx CSU DMA module to ZynqMP SoC, and connent the stream
> > link of GQSPI to CSU DMA.
> >
> > Signed-off-by: Xuzhou Cheng 
> > Signed-off-by: Bin Meng 
> >
> > ---
> >
> > Changes in v4:
> > - Rename "csu_dma" to "qspi_dma"
> >
> > Changes in v3:
> > - new patch: xlnx-zynqmp: Add XLNX CSU DMA module
> >
> >  include/hw/arm/xlnx-zynqmp.h |  2 ++
> >  hw/arm/xlnx-zynqmp.c | 14 ++
> >  hw/arm/Kconfig   |  1 +
> >  3 files changed, 17 insertions(+)
> >
> > diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> > index be15cc8814..2edeed911c 100644
> > --- a/include/hw/arm/xlnx-zynqmp.h
> > +++ b/include/hw/arm/xlnx-zynqmp.h
> > @@ -35,6 +35,7 @@
> >  #include "target/arm/cpu.h"
> >  #include "qom/object.h"
> >  #include "net/can_emu.h"
> > +#include "hw/dma/xlnx_csu_dma.h"
> >
> >  #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
> >  OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPState, XLNX_ZYNQMP)
> > @@ -108,6 +109,7 @@ struct XlnxZynqMPState {
> >  XlnxZynqMPRTC rtc;
> >  XlnxZDMA gdma[XLNX_ZYNQMP_NUM_GDMA_CH];
> >  XlnxZDMA adma[XLNX_ZYNQMP_NUM_ADMA_CH];
> > +XlnxCSUDMA qspi_dma;
> >
> >  char *boot_cpu;
> >  ARMCPU *boot_cpu_ptr;
> > diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> > index 49465a2794..30f43dfda2 100644
> > --- a/hw/arm/xlnx-zynqmp.c
> > +++ b/hw/arm/xlnx-zynqmp.c
> > @@ -50,6 +50,7 @@
> >  #define QSPI_ADDR   0xff0f
> >  #define LQSPI_ADDR  0xc000
> >  #define QSPI_IRQ15
> > +#define QSPI_DMA_ADDR   0xff0f0800
> >
> >  #define DP_ADDR 0xfd4a
> >  #define DP_IRQ  113
> > @@ -63,6 +64,8 @@
> >  #define RTC_ADDR0xffa6
> >  #define RTC_IRQ 26
> >
> > +
> > +
>
> These blank lines look un-related, if you remove them, this looks good to me:
>
> Reviewed-by: Edgar E. Iglesias 

Thanks for the review.

A RESEND version was already sent out to the ML before. Sorry for the
inconvenience.
http://patchwork.ozlabs.org/project/qemu-devel/patch/20210222131502.3098-4-bmeng...@gmail.com/

Regards,
Bin

Re: [PATCH v4 1/5] hw/dma: xlnx_csu_dma: Implement a Xilinx CSU DMA model

[Edgar E. Iglesias]

On Mon, Feb 22, 2021 at 09:05:10PM +0800, Bin Meng wrote:
> From: Xuzhou Cheng 
> 
> ZynqMP QSPI supports SPI transfer using DMA mode, but currently this
> is unimplemented. When QSPI is programmed to use DMA mode, QEMU will
> crash. This is observed when testing VxWorks 7.
> 
> This adds a Xilinx CSU DMA model and the implementation is based on
> https://github.com/Xilinx/qemu/blob/master/hw/dma/csu_stream_dma.c.
> The DST part of the model is verified along with ZynqMP GQSPI model.
> 
> Signed-off-by: Xuzhou Cheng 
> Signed-off-by: Bin Meng 
> 
> ---
> 
> Changes in v4:
> - Add complete CSU DMA model based on Edgar's branch
> - Differences with Edgar's branch:
>   1. Match the registers' FIELD to UG1807.
>   2. Remove "byte-align" property. Per UG1807, SIZE and ADDR registers
>  must be word aligned.

The relaxation of alignment is a new feature, not included on the ZynqMP but
it will be included in future versions. Would be nice to keep it but we can
also add it later since it's not really related to QSPI.

>   3. Make the values of int_enable and int_disable mutually exclusive
>  otherwise IRQ cannot be delivered.

This doesn't sound right. The enable and disable regs are stateless.
They both indirectly modify the MASK register.

I.e, setting a bit in the enable register will clear the correspoding bit in the
mask register, atomically, without the need for read-modify-write of MASK.

The disable register does the opposite.

>   4. Clear int_status after int_disable is set.

This doesn't sound right either. Status is a w1c register, i.e bits get set
when the interrupt event happens in the DMA and bits only get cleared when
SW writes a 1 to the STATUS reg to clear bits (write one to clear, w1c).

Other than the interrupt issues, I think this looks good.

Cheers,
Edgar

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Fam Zheng]

On 2021-02-22 18:55, Philippe Mathieu-Daudé wrote:
> On 2/22/21 6:35 PM, Fam Zheng wrote:
> > On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote:
> >> On 2/19/21 12:07 PM, Max Reitz wrote:
> >>> On 13.02.21 22:54, Fam Zheng wrote:
>  On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
> > The null-co driver doesn't zeroize buffer in its default config,
> > because it is designed for testing and tests want to run fast.
> > However this confuses security researchers (access to uninit
> > buffers).
> 
>  I'm a little surprised.
> 
>  Is changing default the only way to fix this? I'm not opposed to
>  changing the default but I'm not convinced this is the easiest way.
>  block/nvme.c also doesn't touch the memory, but defers to the device
>  DMA, why doesn't that confuse the security checker?
> >>
> >> Generally speaking, there is a balance between security and performance.
> >> We try to provide both, but when we can't, my understanding is security
> >> is more important.
> > 
> > Why is hiding the code path behind a non-default more secure? What is
> > not secure now?
> 
> Se we are back to the problem of having default values.
> 
> I'd like to remove the default and have the option explicit,
> but qemu_opt_get_bool() expects a 'default' value.
> 
> Should we rename qemu_opt_get_bool() -> qemu_opt_get_bool_with_default()
> and add a simpler qemu_opt_get_bool()?

My point is I still don't get the full context of the problem this
series is trying to solve. If the problem is tools are confused, I would
like to understand why. What is the thing that matters here, exactly?

But there's always been nullblk.ko in kernel that doesn't lie in the
name. If we change the default we are not very honest any more about
what the driver actually does.

Even if null-co:// and null-aio:// is a bad idea, then let's add
zero-co://co and zero-aio://, and deprecate null-*://.

Fam

Re: [RESEND PATCH v4 3/5] hw/arm: xlnx-zynqmp: Connect a Xilinx CSU DMA module for QSPI

[Edgar E. Iglesias]

On Mon, Feb 22, 2021 at 09:15:00PM +0800, Bin Meng wrote:
> From: Xuzhou Cheng 
> 
> Add a Xilinx CSU DMA module to ZynqMP SoC, and connent the stream
> link of GQSPI to CSU DMA.
> 
> Signed-off-by: Xuzhou Cheng 
> Signed-off-by: Bin Meng 

Reviewed-by: Edgar E. Iglesias 



> 
> ---
> RESEND this patch to remove 2 blank lines that were accidently added
> 
> Changes in v4:
> - Rename "csu_dma" to "qspi_dma"
> 
> Changes in v3:
> - new patch: xlnx-zynqmp: Add XLNX CSU DMA module
> 
>  include/hw/arm/xlnx-zynqmp.h |  2 ++
>  hw/arm/xlnx-zynqmp.c | 12 
>  hw/arm/Kconfig   |  1 +
>  3 files changed, 15 insertions(+)
> 
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index be15cc8814..2edeed911c 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -35,6 +35,7 @@
>  #include "target/arm/cpu.h"
>  #include "qom/object.h"
>  #include "net/can_emu.h"
> +#include "hw/dma/xlnx_csu_dma.h"
>  
>  #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
>  OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPState, XLNX_ZYNQMP)
> @@ -108,6 +109,7 @@ struct XlnxZynqMPState {
>  XlnxZynqMPRTC rtc;
>  XlnxZDMA gdma[XLNX_ZYNQMP_NUM_GDMA_CH];
>  XlnxZDMA adma[XLNX_ZYNQMP_NUM_ADMA_CH];
> +XlnxCSUDMA qspi_dma;
>  
>  char *boot_cpu;
>  ARMCPU *boot_cpu_ptr;
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 49465a2794..76cc3b5e78 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -50,6 +50,7 @@
>  #define QSPI_ADDR   0xff0f
>  #define LQSPI_ADDR  0xc000
>  #define QSPI_IRQ15
> +#define QSPI_DMA_ADDR   0xff0f0800
>  
>  #define DP_ADDR 0xfd4a
>  #define DP_IRQ  113
> @@ -284,6 +285,8 @@ static void xlnx_zynqmp_init(Object *obj)
>  for (i = 0; i < XLNX_ZYNQMP_NUM_ADMA_CH; i++) {
>  object_initialize_child(obj, "adma[*]", &s->adma[i], TYPE_XLNX_ZDMA);
>  }
> +
> +object_initialize_child(obj, "qspi-dma", &s->qspi_dma, 
> TYPE_XLNX_CSU_DMA);
>  }
>  
>  static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> @@ -643,6 +646,15 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
> **errp)
>  sysbus_connect_irq(SYS_BUS_DEVICE(&s->adma[i]), 0,
> gic_spi[adma_ch_intr[i]]);
>  }
> +
> +if (!sysbus_realize(SYS_BUS_DEVICE(&s->qspi_dma), errp)) {
> +return;
> +}
> +
> +sysbus_mmio_map(SYS_BUS_DEVICE(&s->qspi_dma), 0, QSPI_DMA_ADDR);
> +sysbus_connect_irq(SYS_BUS_DEVICE(&s->qspi_dma), 0, gic_spi[QSPI_IRQ]);
> +object_property_set_link(OBJECT(&s->qspi), "stream-connected-dma",
> + OBJECT(&s->qspi_dma), errp);
>  }
>  
>  static Property xlnx_zynqmp_props[] = {
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 4e6f4ffe90..27ec10f89b 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -353,6 +353,7 @@ config XLNX_ZYNQMP_ARM
>  select SSI_M25P80
>  select XILINX_AXI
>  select XILINX_SPIPS
> +select XLNX_CSU_DMA
>  select XLNX_ZYNQMP
>  select XLNX_ZDMA
>  
> -- 
> 2.25.1
> 

Re: [PATCH v4 1/5] hw/dma: xlnx_csu_dma: Implement a Xilinx CSU DMA model

[Bin Meng]

Hi Edgar,

On Tue, Feb 23, 2021 at 5:21 PM Edgar E. Iglesias
 wrote:
>
> On Mon, Feb 22, 2021 at 09:05:10PM +0800, Bin Meng wrote:
> > From: Xuzhou Cheng 
> >
> > ZynqMP QSPI supports SPI transfer using DMA mode, but currently this
> > is unimplemented. When QSPI is programmed to use DMA mode, QEMU will
> > crash. This is observed when testing VxWorks 7.
> >
> > This adds a Xilinx CSU DMA model and the implementation is based on
> > https://github.com/Xilinx/qemu/blob/master/hw/dma/csu_stream_dma.c.
> > The DST part of the model is verified along with ZynqMP GQSPI model.
> >
> > Signed-off-by: Xuzhou Cheng 
> > Signed-off-by: Bin Meng 
> >
> > ---
> >
> > Changes in v4:
> > - Add complete CSU DMA model based on Edgar's branch
> > - Differences with Edgar's branch:
> >   1. Match the registers' FIELD to UG1807.
> >   2. Remove "byte-align" property. Per UG1807, SIZE and ADDR registers
> >  must be word aligned.
>
> The relaxation of alignment is a new feature, not included on the ZynqMP but
> it will be included in future versions. Would be nice to keep it but we can
> also add it later since it's not really related to QSPI.

I think Xilinx folks can add the "byte-align" property in the future
patches. Is this a new feature for Versal?

>
> >   3. Make the values of int_enable and int_disable mutually exclusive
> >  otherwise IRQ cannot be delivered.
>
> This doesn't sound right. The enable and disable regs are stateless.
> They both indirectly modify the MASK register.
>
> I.e, setting a bit in the enable register will clear the correspoding bit in 
> the
> mask register, atomically, without the need for read-modify-write of MASK.
>
> The disable register does the opposite.
>
> >   4. Clear int_status after int_disable is set.
>
> This doesn't sound right either. Status is a w1c register, i.e bits get set
> when the interrupt event happens in the DMA and bits only get cleared when
> SW writes a 1 to the STATUS reg to clear bits (write one to clear, w1c).
>
> Other than the interrupt issues, I think this looks good.

Without these interrupt fixes, our tests cannot pass. We will have a
further look at your comments.

Regards,
Bin

Re: [PATCH v4 3/5] hw/arm: xlnx-zynqmp: Connect a Xilinx CSU DMA module for QSPI

[Edgar E. Iglesias]

On Tue, Feb 23, 2021 at 05:20:36PM +0800, Bin Meng wrote:
> Hi Edgar,
> 
> On Tue, Feb 23, 2021 at 5:01 PM Edgar E. Iglesias
>  wrote:
> >
> > On Mon, Feb 22, 2021 at 09:05:12PM +0800, Bin Meng wrote:
> > > From: Xuzhou Cheng 
> > >
> > > Add a Xilinx CSU DMA module to ZynqMP SoC, and connent the stream
> > > link of GQSPI to CSU DMA.
> > >
> > > Signed-off-by: Xuzhou Cheng 
> > > Signed-off-by: Bin Meng 
> > >
> > > ---
> > >
> > > Changes in v4:
> > > - Rename "csu_dma" to "qspi_dma"
> > >
> > > Changes in v3:
> > > - new patch: xlnx-zynqmp: Add XLNX CSU DMA module
> > >
> > >  include/hw/arm/xlnx-zynqmp.h |  2 ++
> > >  hw/arm/xlnx-zynqmp.c | 14 ++
> > >  hw/arm/Kconfig   |  1 +
> > >  3 files changed, 17 insertions(+)
> > >
> > > diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> > > index be15cc8814..2edeed911c 100644
> > > --- a/include/hw/arm/xlnx-zynqmp.h
> > > +++ b/include/hw/arm/xlnx-zynqmp.h
> > > @@ -35,6 +35,7 @@
> > >  #include "target/arm/cpu.h"
> > >  #include "qom/object.h"
> > >  #include "net/can_emu.h"
> > > +#include "hw/dma/xlnx_csu_dma.h"
> > >
> > >  #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
> > >  OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPState, XLNX_ZYNQMP)
> > > @@ -108,6 +109,7 @@ struct XlnxZynqMPState {
> > >  XlnxZynqMPRTC rtc;
> > >  XlnxZDMA gdma[XLNX_ZYNQMP_NUM_GDMA_CH];
> > >  XlnxZDMA adma[XLNX_ZYNQMP_NUM_ADMA_CH];
> > > +XlnxCSUDMA qspi_dma;
> > >
> > >  char *boot_cpu;
> > >  ARMCPU *boot_cpu_ptr;
> > > diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> > > index 49465a2794..30f43dfda2 100644
> > > --- a/hw/arm/xlnx-zynqmp.c
> > > +++ b/hw/arm/xlnx-zynqmp.c
> > > @@ -50,6 +50,7 @@
> > >  #define QSPI_ADDR   0xff0f
> > >  #define LQSPI_ADDR  0xc000
> > >  #define QSPI_IRQ15
> > > +#define QSPI_DMA_ADDR   0xff0f0800
> > >
> > >  #define DP_ADDR 0xfd4a
> > >  #define DP_IRQ  113
> > > @@ -63,6 +64,8 @@
> > >  #define RTC_ADDR0xffa6
> > >  #define RTC_IRQ 26
> > >
> > > +
> > > +
> >
> > These blank lines look un-related, if you remove them, this looks good to 
> > me:
> >
> > Reviewed-by: Edgar E. Iglesias 
> 
> Thanks for the review.
> 
> A RESEND version was already sent out to the ML before. Sorry for the
> inconvenience.
> http://patchwork.ozlabs.org/project/qemu-devel/patch/20210222131502.3098-4-bmeng...@gmail.com/
> 

Ah I see, no worries!

Cheers,
Edgar

Re: [PATCH v3 1/6] vt82c686: Implement control of serial port io ranges via config regs

[Philippe Mathieu-Daudé]

On 2/23/21 10:18 AM, BALATON Zoltan wrote:
> On Tue, 23 Feb 2021, David Gibson wrote:
>> On Mon, Feb 22, 2021 at 04:22:06PM +0100, BALATON Zoltan wrote:
>>> In VIA super south bridge the io ranges of superio components
>>> (parallel and serial ports and FDC) can be controlled by superio
>>> config registers to set their base address and enable/disable them.
>>> This is not easy to implement in QEMU because ISA emulation is only
>>> designed to set io base address once on creating the device and io
>>> ranges are registered at creation and cannot easily be disabled or
>>> moved later.
>>>
>>> In this patch we hack around that but only for serial ports because
>>> those have a single io range at port base that's relatively easy to
>>> handle and it's what guests actually use and set address different
>>> than the default.
>>>
>>> We do not attempt to handle controlling the parallel and FDC regions
>>> because those have multiple io ranges so handling them would be messy
>>> and guests either don't change their deafult or don't care. We could
>>> even get away with disabling and not emulating them, but since they
>>> are already there, this patch leaves them mapped at their default
>>> address just in case this could be useful for a guest in the future.
>>>
>>> Signed-off-by: BALATON Zoltan 
>>
>> The maintainers of the hw/isa/vt82c686.c should probably be CCed on this.
> 
> He is (but may not be obvious because the accent in Phil's name is not
> handled well by my mail host so to avoid misspelling his name I've just
> omitted it).

We are 3 (other now Cc'ed):

$ ./scripts/get_maintainer.pl -f hw/isa/vt82c686.c
Huacai Chen  (odd fixer:Fuloong 2E)
"Philippe Mathieu-Daudé"  (odd fixer:Fuloong 2E)
Jiaxun Yang  (reviewer:Fuloong 2E)

> And he also said these should go in via some other tree:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2021-02/msg06551.html
> 
> He may still review or ack these but I guess he'd need some time.

I took a bunch of Zoltan's patches:

$ git log --oneline e551455f1e7..00d8ba9e0d6 hw/isa/vt82c686.c
cc2b4550115 vt82c686: Fix superio_cfg_{read,write}() functions
2c4c556e061 vt82c686: Log superio_cfg unimplemented accesses
b7741b77425 vt82c686: Simplify by returning earlier
2b98dca9571 vt82c686: Reduce indentation by returning early
c953bf71182 vt82c686: Remove index field of SuperIOConfig
3dc31cb8490 vt82c686: Move creation of ISA devices to the ISA bridge
9859ad1c4b6 vt82c686: Simplify vt82c686b_realize()
e1a69736e59 vt82c686: Make vt82c686b-pm an abstract base class and add
vt8231-pm based on it
084bf4b41d4 vt82c686: Set user_creatable=false for VT82C686B_PM
3ab1eea6bce vt82c686: Fix up power management io base and config
9af8e529b91 vt82c686: Correctly reset all registers to default values on
reset
40a0bba1e3f vt82c686: Correct vt82c686-pm I/O size
35e360ed674 vt82c686: Make vt82c686-pm an I/O tracing region
911629e6d37 vt82c686: Fix SMBus IO base and configuration registers
94349bffda0 vt82c686: Reorganise code
6be6e4bc769 vt82c686: Move superio memory region to SuperIOConfig struct
7886a674f13 vt82c686: Rename superio config related parts
007b3103a39 vt82c686: Use shorter name for local variable holding object
state
9b0fbae2cbf vt82c686: Remove unneeded includes and defines
ff413a1f7f6 vt82c686: Convert debug printf to trace points
dc66439542c vt82c686: Remove legacy vt82c686b_pm_init() function
0bfda9a225b vt82c686: Remove legacy vt82c686b_isa_init() function
657fae258f9 vt82c686: Split off via-[am]c97 into separate file in hw/audio
07c6832cb2c vt82c686: Remove vt82c686b_[am]c97_init() functions
0f79846147f vt82c686: Rename VT82C686B to VT82C686B_ISA
e6340505441 vt82c686: Remove unnecessary _DEVICE suffix from type macros
5a4856ed78e vt82c686: Rename AC97/MC97 parts from VT82C686B to VIA

But with this one I raised a question for find_subregion() ISA use,
and deferred to Paolo for this particular question, and to PPC for
the new VT8231 device model.

Regards,

Phil.

Re: [PATCH v3 0/6] Pegasos2 emulation

[BALATON Zoltan]

On Tue, 23 Feb 2021, David Gibson wrote:

On Mon, Feb 22, 2021 at 04:22:06PM +0100, BALATON Zoltan wrote:

Hello,

This is adding a new PPC board called pegasos2. More info on it can be
found at:

https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2

Currently it needs a firmware ROM image that I cannot include due to
original copyright holder (bPlan) did not release it under a free
licence but I have plans to write a replacement in the future. With
the original board firmware it can boot MorphOS now as:

qemu-system-ppc -M pegasos2 -cdrom morphos.iso -device ati-vga,romfile="" 
-serial stdio

then enter "boot cd boot.img" at the firmware "ok" prompt as described
in the MorphOS.readme. To boot Linux use same command line with e.g.
-cdrom debian-8.11.0-powerpc-netinst.iso then enter
"boot cd install/pegasos"

The last patch adds the actual board code after previous patches
adding VT8231 and MV64361 system controller chip emulation. The
mv643xx.h header file is taken from Linux and produces a bunch of
checkpatch warnings due to different formatting rules it follows, I'm
not sure we want to adopt it and change formatting or keep it as it
is.


A couple of overall comments:

* Adding yourself to MAINTAINERS for the new files would be a good
  idea
* At least some rudimentary tests would be good, though I guess that
  might be tricky with non-free firmware


I've described here what could be a test:

https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg01553.html

but I'm not sure how to implement that in qtest so if somebody helped with 
that that would be greatly appreciated.


Regards,
BALATON Zoltan

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Daniel P . Berrangé]

On Tue, Feb 23, 2021 at 09:44:51AM +0100, Max Reitz wrote:
> On 22.02.21 19:15, Daniel P. Berrangé wrote:
> > On Fri, Feb 19, 2021 at 03:09:43PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 2/19/21 12:07 PM, Max Reitz wrote:
> > > > On 13.02.21 22:54, Fam Zheng wrote:
> > > > > On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote:
> > > > > > The null-co driver doesn't zeroize buffer in its default config,
> > > > > > because it is designed for testing and tests want to run fast.
> > > > > > However this confuses security researchers (access to uninit
> > > > > > buffers).
> > > > > 
> > > > > I'm a little surprised.
> > > > > 
> > > > > Is changing default the only way to fix this? I'm not opposed to
> > > > > changing the default but I'm not convinced this is the easiest way.
> > > > > block/nvme.c also doesn't touch the memory, but defers to the device
> > > > > DMA, why doesn't that confuse the security checker?
> > > 
> > > Generally speaking, there is a balance between security and performance.
> > > We try to provide both, but when we can't, my understanding is security
> > > is more important.
> > > 
> > > Customers expect a secure product. If they prefer performance and
> > > at the price of security, it is also possible by enabling an option
> > > that is not the default.
> > > 
> > > I'm not sure why you mention block/nvme here. I have the understanding
> > > the null-co driver is only useful for testing. Are there production
> > > cases where null-co is used?
> > 
> > Do we have any real world figures for the performance of null-co
> > with & without  zero'ing ?  Before worrying about a tradeoff of
> > security vs performance, it'd be good to know if there is actually
> > a real world performance problem in the first place. Personally I'd
> > go for zero'ing by defualt unless the performance hit was really
> > bad.
> 
> AFAIU, null-co is only used for testing, be it to just create some block
> nodes in the iotests, or perhaps for performance testing where you want to
> get the minimal roundtrip time through the block layer.  So there is no
> "real world performance problem", because there is no real world use of
> null-co or null-aio.  At least there shouldn’t be.
> 
> That begs the question of whether read-zeroes=off even makes sense, and I
> think it absolutely does.
> 
> In cases where we have a test that just wants a simple block node that
> doesn’t use disk space, the memset() can’t be noticeable.  But it’s just a
> test, so do we even need the memset()?  Strictly speaking, perhaps not, but
> if someone is to run it via Valgrind or something, they may get false
> positives, so just doing the memset() is the right thing to do.
> 
> For performance tests, it must be possible to set read-zeroes=off, because
> even though “that memset() isn’t noticeable in a functional test”, in a
> hard-core performance test, it will be.
> 
> So we need a switch.  It should default to memset(), because (1) making
> tools like Valgrind happy seems like a reasonable objective to me, and (2)
> in the majority of cases, the memset() cannot have a noticeable impact.

Yes, that all makes sense to me.


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

Re: [PATCH v4 1/5] hw/dma: xlnx_csu_dma: Implement a Xilinx CSU DMA model

[Edgar E. Iglesias]

On Tue, Feb 23, 2021 at 05:23:43PM +0800, Bin Meng wrote:
> Hi Edgar,
> 
> On Tue, Feb 23, 2021 at 5:21 PM Edgar E. Iglesias
>  wrote:
> >
> > On Mon, Feb 22, 2021 at 09:05:10PM +0800, Bin Meng wrote:
> > > From: Xuzhou Cheng 
> > >
> > > ZynqMP QSPI supports SPI transfer using DMA mode, but currently this
> > > is unimplemented. When QSPI is programmed to use DMA mode, QEMU will
> > > crash. This is observed when testing VxWorks 7.
> > >
> > > This adds a Xilinx CSU DMA model and the implementation is based on
> > > https://github.com/Xilinx/qemu/blob/master/hw/dma/csu_stream_dma.c.
> > > The DST part of the model is verified along with ZynqMP GQSPI model.
> > >
> > > Signed-off-by: Xuzhou Cheng 
> > > Signed-off-by: Bin Meng 
> > >
> > > ---
> > >
> > > Changes in v4:
> > > - Add complete CSU DMA model based on Edgar's branch
> > > - Differences with Edgar's branch:
> > >   1. Match the registers' FIELD to UG1807.
> > >   2. Remove "byte-align" property. Per UG1807, SIZE and ADDR registers
> > >  must be word aligned.
> >
> > The relaxation of alignment is a new feature, not included on the ZynqMP but
> > it will be included in future versions. Would be nice to keep it but we can
> > also add it later since it's not really related to QSPI.
> 
> I think Xilinx folks can add the "byte-align" property in the future
> patches. Is this a new feature for Versal?

It's not in silicon yet, yeah, we can add it later.


> 
> >
> > >   3. Make the values of int_enable and int_disable mutually exclusive
> > >  otherwise IRQ cannot be delivered.
> >
> > This doesn't sound right. The enable and disable regs are stateless.
> > They both indirectly modify the MASK register.
> >
> > I.e, setting a bit in the enable register will clear the correspoding bit 
> > in the
> > mask register, atomically, without the need for read-modify-write of MASK.
> >
> > The disable register does the opposite.
> >
> > >   4. Clear int_status after int_disable is set.
> >
> > This doesn't sound right either. Status is a w1c register, i.e bits get set
> > when the interrupt event happens in the DMA and bits only get cleared when
> > SW writes a 1 to the STATUS reg to clear bits (write one to clear, w1c).
> >
> > Other than the interrupt issues, I think this looks good.
> 
> Without these interrupt fixes, our tests cannot pass. We will have a
> further look at your comments.

This is a common interrupt handling pattern in most Xilinx devices.
You can look at the xlnx-zdma.c IMR, ISR, IEN, IDS regs as an example.

Cheers,
Edgar

Re: A brief look at deprecating our JSON extensions over RFC 8259

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Mon, Feb 22, 2021 at 06:47:30PM +0100, Paolo Bonzini wrote:
>> On 22/02/21 16:24, Daniel P. Berrangé wrote:
>> > This problem isn't unique to QEMU. Any app using JSON from the
>> > shell will have the tedium of quote escaping. JSON is incredibly
>> > widespread and no other apps felt it neccessary to introduce single
>> > quoting support, because the benefit doesn't outweigh the interop
>> > problem it introduces.
>> 
>> The quotes were introduced for C code (and especially qtest), not for the
>> shell.  We have something like
>> 
>> response = qmp("{ 'execute': 'qom-get', 'arguments': { 'path': %s, "
>>"'property': 'temperature' } }", id);
>> 
>> These are sent to QEMU as double-quoted strings (the single-quoted JSON is
>> parsed to get interpolation and printed back; commit 563890c7c7, "libqtest:
>> escape strings in QMP commands, fix leak", 2014-07-01). However, doing the
>> interpolation requires a parser that recognizes the single-quoted strings.
>
> IMHO this is the wrong solution to the problem.  Consider the equivalent
> libvirt code that uses a standard JSON library underneath and has a high
> level API to serialize args into the command
>
>   qemuMonitorJSONMakeCommand("qom-get",
>  "s:path", id,
>"s:property", "temperature");
>
> Of course this example is reasonably easy since it is a flat set of
> arguments. Nested args get slightly more complicated, but still always
> preferrable to doing string interpolation IMHO.

Misunderstanding: our JSON interpolation feature is *not* string
interpolation!  It interpolates *objects* into the QObject built by the
parser.

Best explained with an example.  The qmp() call above internally builds
the following QObject:

QDict mapping
key "execute" to a QString for "qom-get"
key "arguments" to a QDict mapping
key "path" to a QString for @id (interpolation!)
key "property" to a QString for "temperature"

Unlike string interpolation, this is safe for any valid C string @id.

If you think like a hacker, you might try shenanigans like

"{'execute': '%s'}"

You will then find that somebody has thought like a hacker before
you[*].

The %-sequences are cleverly chosen to get some protection against
common mistakes from the compiler.

This interpolation has worked quite well for us, and I have no plans to
replace it.  Doesn't mean I'm against replacing it, only that I want to
see benefits exceeding the costs.

A bigger stumbling block for replacement is our need for a streaming
interface: we feed the parser characters, and expect to be called back
when an expression is complete.


[*] Commit 16a4859921 "json: Improve safety of
qobject_from_jsonf_nofail() & friends", fixed up in commit bbc0586ced
"json: Fix % handling when not interpolating".

Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c

[Claudio Fontana]

On 2/23/21 10:16 AM, Philippe Mathieu-Daudé wrote:
> On 2/23/21 9:55 AM, Claudio Fontana wrote:
>> On 2/22/21 6:29 PM, Alex Bennée wrote:
>>>
>>> Claudio Fontana  writes:
>>>
 From: Claudio Fontana 

 Signed-off-by: Claudio Fontana 
 ---
  target/arm/internals.h   |   9 ++-
  target/arm/cpu-softmmu.c | 134 +++
  target/arm/cpu.c |  95 ---
  target/arm/meson.build   |   1 +
  4 files changed, 143 insertions(+), 96 deletions(-)
  create mode 100644 target/arm/cpu-softmmu.c

 diff --git a/target/arm/internals.h b/target/arm/internals.h
 index 6384461177..6589b63ebc 100644
 --- a/target/arm/internals.h
 +++ b/target/arm/internals.h
 @@ -1196,4 +1196,11 @@ static inline uint64_t 
 useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
  return ptr;
  }
  
 -#endif
 +#ifndef CONFIG_USER_ONLY
 +void arm_cpu_set_irq(void *opaque, int irq, int level);
 +void arm_cpu_kvm_set_irq(void *opaque, int irq, int level);
 +bool arm_cpu_virtio_is_big_endian(CPUState *cs);
 +uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri);
 +#endif /* !CONFIG_USER_ONLY */
 +
 +#endif /* TARGET_ARM_INTERNALS_H */
 diff --git a/target/arm/cpu-softmmu.c b/target/arm/cpu-softmmu.c
 new file mode 100644
 index 00..263d1fc588
 --- /dev/null
 +++ b/target/arm/cpu-softmmu.c
 @@ -0,0 +1,134 @@
 +/*
 + * QEMU ARM CPU
>>>
>>> I guess apropos the discussion earlier it's really cpu-sysemu.c and we
>>> could expand the header comment.
>>>
>>>   QEMU ARM CPU - Helpers for system emulation and KVM only
>>>
>>> 
>>>
>>> Otherwise:
>>>
>>> Reviewed-by: Alex Bennée 
>>>
>>
>> Should I rename all *softmmu in the series to "sysemu"?
>>
>> I wonder if we should take the issue of sysemu/system/softmmu topic into a 
>> separate series.
>> Currently basically starting from the build system already, "softmmu", 
>> sysemu and system are treated as a single thing, and the convention from 
>> build system and directories seems to be "softmmu",
>> while from the header files we get "sysemu/".
>>
>> I agree that this is not a sufficient model to account for the new feature 
>> that Richard wants to develop,
>> I just suggest we could also consider tackling this separately, with a pass 
>> through the whole code, gathering more input in the context of a dedicated 
>> series.
>>
>> What do you think?
> 
> This is a valid reasoning. However I have my doubts "doing
> that later" will ever be done/finished (not related to you
> Claudio in particular, but with dealing with all subsystems).
> 
> Personally I'd rather see this sorted out with the arm target
> then once done propose it as an example to the other ones.
> You already considered the most complex cases, x86 and arm :)


Ok, if there are no other comments I would go with "sysemu", just because 
"system" is a bit too much of a loaded word,
and we have the precedent of include/sysemu/ .


> 
>> Also Paolo, any comments, since softmmu is all over meson?
>>

And Peter, any comments, preference?

Ciao,

Claudio

Re: romfile resize

[Philippe Mathieu-Daudé]

Cc'ing qemu-devel@

On 2/23/21 1:45 AM, Jiatong Shen wrote:
> Hello,
> 
>   we are faced with an issue where a changed sized romfile
> (efi-virtio.rom) fails live migration. Do qemu load this rom from its
> current host only? If yes, why cannot sync this from source vm?
> 
> thank you.
> 
> -- 
> 
> Best Regards,
> 
> Jiatong Shen

Re: A brief look at deprecating our JSON extensions over RFC 8259

[Markus Armbruster]

Markus Armbruster  writes:

[...]
> A bigger stumbling block for replacement is our need for a streaming
> interface: we feed the parser characters, and expect to be called back
> when an expression is complete.

Another stumbling block: check-qjson.c test case "/literals/string/utf8"
and "/literals/string/escaped".  Off-the-shelf parsers flunking them
would surprise me about as much as the sun going up in the morning.

[...]

Re: [PATCH v5 0/9] block: Add retry for werror=/rerror= mechanism

[Stefan Hajnoczi]

On Fri, Feb 05, 2021 at 06:13:06PM +0800, Jiahui Cen wrote:
> This patch series propose to extend the werror=/rerror= mechanism to add
> a 'retry' feature. It can automatically retry failed I/O requests on error
> without sending error back to guest, and guest can get back running smoothly
> when I/O is recovred.

This patch series implements a retry followed by werror/rerror=report
after a timeout. This mechanism could be made more generic (and the code
could be simplified) by removing the new werror/rerror=retry action and
instead implementing the retry/timeout followed by *any* werror=/rerror=
policy chosen by the user.

In other words, if the retry interval is non-zero, retry the request and
check for timeouts. When the timeout is reached, obey the
werror=/rerror= action.

This is more flexible than hard-coding werror=retry to mean retry
timeout followed by werror=report.

For example:

  werror=stop,write-retry-interval=1000,write-retry-timeout=15000,
  rerror=report,read-retry-interval=1000,read-retry-timeout=15000

Failed write requests will be retried once a second for 15 seconds.
If the timeout is reached the guest is stopped.

Failed read requests will be retried once a second for 15 seconds. If
the timeout is reached the error is reported to the guest.

Stefan


signature.asc
Description: PGP signature

Re: romfile resize

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> Cc'ing qemu-devel@
> 
> On 2/23/21 1:45 AM, Jiatong Shen wrote:
> > Hello,
> > 
> >   we are faced with an issue where a changed sized romfile
> > (efi-virtio.rom) fails live migration. Do qemu load this rom from its
> > current host only? If yes, why cannot sync this from source vm?

Hi,
  For migration to work the ROM has to be the same size on the source
and destination.

  The problem is that whne the destination starts up it allocates the
size of the ROM based on the size of the file;  but then the migration
comes along and tries to copy the data from the source machine into that
allocation; and isn't sure what should happen when it doesn't quite fit.

  There is some variation allowed (I think the allocated size gets
rounded up, maybe to the next power of 2); but you still hit problems
wehn the ROM size crosses certain thresholds.

  In the latest qemu, a 'romsize' property was added (see git commit
08b1df8ff463e72b0875538fb991d5393047606c ); that lets you specifiy a
size that's big enough to hold some space for future expansion - e.g.
lets say your ROM is currently 300k, you might specify romsize=512k
and then it doesn't matter what size the actual file is, we'll always
allocate 512k, and as long as the file is less than 512k migration will
work.

  The more manual way to do that, is to arrange for your files to be
padded to a larger boundary so that you leave room for growth.
Some distros have done that for a while.

Dave
  
> > thank you.
> > 
> > -- 
> > 
> > Best Regards,
> > 
> > Jiatong Shen
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: who's using the ozlabs patchwork install for QEMU patches ?

[Greg Kurz]

On Tue, 23 Feb 2021 14:57:21 +0800
Bin Meng  wrote:

> On Mon, Feb 22, 2021 at 10:43 PM Greg Kurz  wrote:
> >
> > On Mon, 22 Feb 2021 13:59:34 +
> > Peter Maydell  wrote:
> >
> > > On Mon, 22 Feb 2021 at 07:21, Greg Kurz  wrote:
> > > >
> > > > On Fri, 19 Feb 2021 17:51:02 +0100
> > > > Thomas Huth  wrote:
> > > >
> > > > > On 19/02/2021 17.26, Peter Maydell wrote:
> > > > > > Does anybody use the ozlabs patchwork install for QEMU patches,
> > > > > > either occasionally or on a regular basis ?
> > > > > > http://patchwork.ozlabs.org/project/qemu-devel/list/
> > > > > > The admins for that system are trying to identify which of
> > > > > > the various projects are really using their patchwork instances,
> > > > > > so I figured I'd do a quick survey here. We don't use it
> > > > > > as an official project tool but it's certainly possible to
> > > > > > use it as an individual developer in one way or another.
> > > > >
> > > > > I think it might be used by some of the ppc hackers ... so CC:-ing to
> > > > > qemu-pcc ...
> > > > >
> > > >
> > > > I do on a very regular basis.
> > >
> > > Thanks for the reports. Do you use the features like assigning
> > > patches to people and changing patch status, or do you mostly
> > > just use it as a read-only archive-of-patches ?
> > >
> >
> > Only the latter but mostly because I don't have the permissions
> > to change status, e.g. when trying to change status of this
> > recent patch from Cedric to rearrange the PowerPC docs:
> >
> > You don't have permissions to edit patch 'docs/system: Extend PPC section'
> >
> > My understanding is that users must be "maintainer" to edit other's
> > patches. Only three 'maintainers' are currently listed at ozlabs for
> > QEMU:
> 
> I can update my patch status in the QEMU project. I am not sure if
> this is due to I am a maintainer of another project hosted on
> ozlabs.org.
> 

Yeah everyone can update its own patches but you need to
be maintainer of a project to update the status of other's
patch for this project IIUC.

> >
> > https://patchwork.ozlabs.org/api/1.0/projects/14/
> >
> > We had a discussion about that a few months back with Christian Schoenebeck
> > (9pfs maintainer, Cc'd) who also uses patchworks. It turned out we didn't
> > quite know how to go further because of lack of documentation, but I'd be
> > glad to experiment the full patchwork experience if someone knows how to
> > do it :-)
> 
> I personally found patchwork is really helpful for mainatiner's work.
> But it looks the maintainers from the QEMU community do not use it.
> 
> Regards,
> Bin

[PATCH] docs: move CODING_STYLE into the developer documentation

[Alex Bennée]

There is no particular reason to keep this on it's own in the root of
the tree. Move it into the rest of the fine developer manual and fixup
any links to it. The only tweak I've made is to fix the code-block
annotations to mention the language C.

Signed-off-by: Alex Bennée 
---
 docs/devel/index.rst | 1 +
 CODING_STYLE.rst => docs/devel/style.rst | 6 +++---
 README.rst   | 4 +++-
 scripts/fix-multiline-comments.sh| 2 +-
 4 files changed, 8 insertions(+), 5 deletions(-)
 rename CODING_STYLE.rst => docs/devel/style.rst (99%)

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 22854e334d..ae664da00c 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -14,6 +14,7 @@ Contents:
:maxdepth: 2
 
build-system
+   style
kconfig
testing
fuzzing
diff --git a/CODING_STYLE.rst b/docs/devel/style.rst
similarity index 99%
rename from CODING_STYLE.rst
rename to docs/devel/style.rst
index 7bf4e39d48..8b0bdb3570 100644
--- a/CODING_STYLE.rst
+++ b/docs/devel/style.rst
@@ -641,7 +641,7 @@ trace-events style
 
 In trace-events files, use a '0x' prefix to specify hex numbers, as in:
 
-.. code-block::
+.. code-block:: c
 
 some_trace(unsigned x, uint64_t y) "x 0x%x y 0x" PRIx64
 
@@ -649,14 +649,14 @@ An exception is made for groups of numbers that are 
hexadecimal by
 convention and separated by the symbols '.', '/', ':', or ' ' (such as
 PCI bus id):
 
-.. code-block::
+.. code-block:: c
 
 another_trace(int cssid, int ssid, int dev_num) "bus id: %x.%x.%04x"
 
 However, you can use '0x' for such groups if you want. Anyway, be sure that
 it is obvious that numbers are in hex, ex.:
 
-.. code-block::
+.. code-block:: c
 
 data_dump(uint8_t c1, uint8_t c2, uint8_t c3) "bytes (in hex): %02x %02x 
%02x"
 
diff --git a/README.rst b/README.rst
index ce39d89077..f5d41e59b1 100644
--- a/README.rst
+++ b/README.rst
@@ -66,7 +66,9 @@ When submitting patches, one common approach is to use 'git
 format-patch' and/or 'git send-email' to format & send the mail to the
 qemu-devel@nongnu.org mailing list. All patches submitted must contain
 a 'Signed-off-by' line from the author. Patches should follow the
-guidelines set out in the CODING_STYLE.rst file.
+guidelines set out in the `style section
+` of
+the Developers Guide.
 
 Additional information on submitting patches can be found online via
 the QEMU website
diff --git a/scripts/fix-multiline-comments.sh 
b/scripts/fix-multiline-comments.sh
index 93f9b10669..c15a041272 100755
--- a/scripts/fix-multiline-comments.sh
+++ b/scripts/fix-multiline-comments.sh
@@ -1,6 +1,6 @@
 #! /bin/sh
 #
-# Fix multiline comments to match CODING_STYLE
+# Fix multiline comments to match docs/devel/style.rst
 #
 # Copyright (C) 2018 Red Hat, Inc.
 #
-- 
2.20.1

Re: [RFC v1 24/38] target/arm: move aa64_va_parameter_tbi,tbid,tcma and arm_rebuild_hflags

[Claudio Fontana]

On 2/22/21 7:02 AM, Richard Henderson wrote:
> On 2/21/21 1:24 AM, Claudio Fontana wrote:
>> From: Claudio Fontana 
>>
>> they are needed for KVM too, move them out of TCG helpers.
>>
>> Signed-off-by: Claudio Fontana 
>> ---
>>  target/arm/internals.h  |  37 +
>>  target/arm/tcg/helper-tcg.h |  32 -
>>  target/arm/cpu-common.c | 252 ++
>>  target/arm/tcg/helper.c | 264 +---
>>  4 files changed, 293 insertions(+), 292 deletions(-)
>>
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index 6589b63ebc..9eb5d7fd79 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -1196,6 +1196,43 @@ static inline uint64_t 
>> useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
>>  return ptr;
>>  }
>>  
>> +/*
>> + * Convert a possible stage1+2 MMU index into the appropriate
>> + * stage 1 MMU index
>> + */
>> +static inline ARMMMUIdx stage_1_mmu_idx(ARMMMUIdx mmu_idx)
>> +{
>> +switch (mmu_idx) {
>> +case ARMMMUIdx_SE10_0:
>> +return ARMMMUIdx_Stage1_SE0;
>> +case ARMMMUIdx_SE10_1:
>> +return ARMMMUIdx_Stage1_SE1;
>> +case ARMMMUIdx_SE10_1_PAN:
>> +return ARMMMUIdx_Stage1_SE1_PAN;
>> +case ARMMMUIdx_E10_0:
>> +return ARMMMUIdx_Stage1_E0;
>> +case ARMMMUIdx_E10_1:
>> +return ARMMMUIdx_Stage1_E1;
>> +case ARMMMUIdx_E10_1_PAN:
>> +return ARMMMUIdx_Stage1_E1_PAN;
>> +default:
>> +return mmu_idx;
>> +}
>> +}
>> +
>> +int aa64_va_parameter_tbi(uint64_t tcr, ARMMMUIdx mmu_idx);
>> +int aa64_va_parameter_tbid(uint64_t tcr, ARMMMUIdx mmu_idx);
> 
> I can see these being needed for get-phys-addr -- and that probably answers my
> arm_mmu_idx_el question earlier too.
> 
> 
>> +uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
>> +ARMMMUIdx mmu_idx);
>> +uint32_t rebuild_hflags_a32(CPUARMState *env, int fp_el, ARMMMUIdx mmu_idx);
>> +uint32_t rebuild_hflags_m32(CPUARMState *env, int fp_el, ARMMMUIdx mmu_idx);
> 
> However these really really shouldn't be used for !tcg.  I would even wrap
> CPUARMState::hflags in #ifdef CONFIG_TCG to enforce that.
> 
> I think maybe the best option is
> 
> if (tcg_enabled()) {
> rebuild_hflags();
> }
> 
> so that we don't spend the time on the rebuild for a regular build that has
> both tcg and kvm enabled, and the symbol gets
> compiled out when tcg is disabled.

is the code elimination for "if (0)" a guarantee, ie, we won't encounter 
compiler or compiler-options differences,
for the compilers we support?

This is a doubt that is bothering me since some time, since I remember I 
encountered problems like this before,
with my compiler behaving differently than Paolo's in particular.

Is there some way to force the compilers to not even look at what is in the if 
(0) block?
That should work also with --enable-debug?

This way we could avoid a lot of boilerplate/stubs...

Ciao,

Claudio


> 
> 
> r~
> 

Re: [PATCH v5 0/9] block: Add retry for werror=/rerror= mechanism

[Jiahui Cen]

Hi Stefan,

On 2021/2/23 17:40, Stefan Hajnoczi wrote:
> On Fri, Feb 05, 2021 at 06:13:06PM +0800, Jiahui Cen wrote:
>> This patch series propose to extend the werror=/rerror= mechanism to add
>> a 'retry' feature. It can automatically retry failed I/O requests on error
>> without sending error back to guest, and guest can get back running smoothly
>> when I/O is recovred.
> 
> This patch series implements a retry followed by werror/rerror=report
> after a timeout. This mechanism could be made more generic (and the code
> could be simplified) by removing the new werror/rerror=retry action and
> instead implementing the retry/timeout followed by *any* werror=/rerror=
> policy chosen by the user.
> 
> In other words, if the retry interval is non-zero, retry the request and
> check for timeouts. When the timeout is reached, obey the
> werror=/rerror= action.
> 
> This is more flexible than hard-coding werror=retry to mean retry
> timeout followed by werror=report.
> 
> For example:
> 
>   werror=stop,write-retry-interval=1000,write-retry-timeout=15000,
>   rerror=report,read-retry-interval=1000,read-retry-timeout=15000
> 
> Failed write requests will be retried once a second for 15 seconds.
> If the timeout is reached the guest is stopped.
> 
> Failed read requests will be retried once a second for 15 seconds. If
> the timeout is reached the error is reported to the guest.

Sounds like a better way for me. I'll refactor this patch series under
your suggestion.

Also thanks for your review.

Thanks,
Jiahui

Re: romfile resize

[Dr. David Alan Gilbert]

* Jiatong Shen (yshxxsjt...@gmail.com) wrote:
> Hi,
> 
>   Thank you very much for the answer. so if romfile on destination got a
> larger size than source, why romfile check still does not
> pass? because dest got enough space to hold romfile.

Right.

Dave

> thank you.
> 
> Jiatong Shen
> 
> On Tue, Feb 23, 2021 at 5:46 PM Dr. David Alan Gilbert 
> wrote:
> 
> > * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> > > Cc'ing qemu-devel@
> > >
> > > On 2/23/21 1:45 AM, Jiatong Shen wrote:
> > > > Hello,
> > > >
> > > >   we are faced with an issue where a changed sized romfile
> > > > (efi-virtio.rom) fails live migration. Do qemu load this rom from its
> > > > current host only? If yes, why cannot sync this from source vm?
> >
> > Hi,
> >   For migration to work the ROM has to be the same size on the source
> > and destination.
> >
> >   The problem is that whne the destination starts up it allocates the
> > size of the ROM based on the size of the file;  but then the migration
> > comes along and tries to copy the data from the source machine into that
> > allocation; and isn't sure what should happen when it doesn't quite fit.
> >
> >   There is some variation allowed (I think the allocated size gets
> > rounded up, maybe to the next power of 2); but you still hit problems
> > wehn the ROM size crosses certain thresholds.
> >
> >   In the latest qemu, a 'romsize' property was added (see git commit
> > 08b1df8ff463e72b0875538fb991d5393047606c ); that lets you specifiy a
> > size that's big enough to hold some space for future expansion - e.g.
> > lets say your ROM is currently 300k, you might specify romsize=512k
> > and then it doesn't matter what size the actual file is, we'll always
> > allocate 512k, and as long as the file is less than 512k migration will
> > work.
> >
> >   The more manual way to do that, is to arrange for your files to be
> > padded to a larger boundary so that you leave room for growth.
> > Some distros have done that for a while.
> >
> > Dave
> >
> > > > thank you.
> > > >
> > > > --
> > > >
> > > > Best Regards,
> > > >
> > > > Jiatong Shen
> > >
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> >
> >
> 
> -- 
> 
> Best Regards,
> 
> Jiatong Shen
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Rust-VMM] vhost reply_ack negotiation (a.k.a differences in vhost-user behaviour with libvhost-user and vhost-user-backend.rs)

[Jiang Liu]

I just found this thread in my email junk box:(
I do have found some bugs in the vhost_rs crate, related to handle the 
need_reply flag.
But that bug only affects virtio-fs fs_map operations.
Please refer to:
https://github.com/rust-vmm/vhost/pull/19
https://github.com/rust-vmm/vhost/pull/19/commits/a2c5a4f50e45ae1ab78622dda9a3f861bd43a17e

Thanks,
Gerry

> On Feb 22, 2021, at 9:27 PM, Dr. David Alan Gilbert  
> wrote:
> 
> * Alex Bennée (alex.ben...@linaro.org) wrote:
>> 
>> Dr. David Alan Gilbert  writes:
>> 
>>> * Alex Bennée (alex.ben...@linaro.org) wrote:
 Hi,
 
 I finally got a chance to get down into the guts of vhost-user while
 attempting to port my original C RPMB daemon to Rust using the
 vhost-user-backend and related crates. I ended up with this hang during
 negotiation:
 
 startup
 
 vhost_user_write req:1 flags:0x1
 vhost_user_read_start
 vhost_user_read req:1 flags:0x5
 vhost_user_backend_init: we got 17000
>> 
>> GET_FEATURES
>> 
 vhost_user_write req:15 flags:0x1
 vhost_user_read_start
 vhost_user_read req:15 flags:0x5
 vhost_user_set_protocol_features: 2008
 vhost_user_write req:16 flags:0x1
 vhost_user_write req:3 flags:0x1
 vhost_user_write req:1 flags:0x1
 vhost_user_read_start
 vhost_user_read req:1 flags:0x5
 vhost_user_write req:13 flags:0x1
 
 kernel initialises device
 
 virtio_rpmb virtio1: init done!
 vhost_user_write req:13 flags:0x1
 vhost_dev_set_features: 13000
 vhost_user_set_features: 13000
>> 
>> SET_FEATURES
>> 
 vhost_user_write req:2 flags:0x1
 vhost_user_write req:5 flags:0x9
 vhost_user_read_start
 
>> 
 
 - Should QEMU have preserved VhostUserVirtioFeatures::PROTOCOL_FEATURES
  when doing the eventual VHOST_USER_SET_FEATURES reply?
 
 - Is vhost.rs being to strict or libvhost-user too lax in interpreting
  the negotiated features before processing the ``need_reply`` [Bit 3]
  field of the messages?
>>> 
>>> I think vhost.rs is being correctly strict - but there would be no harm
>>> in it flagging that you'd hit an inconsistency if it finds a need_reply
>>> without the feature.
>> 
>> But the feature should have been negotiated. So unless the slave can
>> assume it is enabled because it asked I think QEMU is in the wrong by
>> not preserving the feature bits in it's SET_FEATURES reply. We just gets
>> away with it with libvhostuser being willing to reply anyway.
> 
> Oh I wasn't trying to reply to that bit; I can never remember how the
> vhost/virtio feature bit negotiation works...
> 
> Dave
> 
>>> 
 - are VHOST_USER_SET_MEM_TABLE to VHOST_USER_SET_INFLIGHT_FD included
  in the "list of the ones that do" require replies or do they only
  reply when REPLY_ACK has been negotiated as the ambiguous "seealso::"
  box out seems to imply?
>>> 
>>> set_mem_table gives a reply when postcopy is enabled (and then qemu
>>> replies to the reply!) but otherwise doesn't.
>>> (Note there's an issue opened for .rs to support ADD_MEM_REGION
>>> since it's a lot better than SET_MEM_TABLE which has a fixed size table
>>> that's small).
>> 
>> Thanks for the heads up.
>> 
>>> 
>>> Dave
>>> 
 Currently I have some hacks in:
 
 https://github.com/stsquad/vhost/tree/my-hacks
 
 which gets my daemon booting up to the point we actually need to do a
 transaction. However I won't submit a PR until I've worked out exactly
 where the problems are.
 
 -- 
 Alex Bennée
 
>> 
>> 
>> -- 
>> Alex Bennée
>> 
> -- 
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> 
> 
> ___
> Rust-vmm mailing list
> rust-...@lists.opendev.org
> http://lists.opendev.org/cgi-bin/mailman/listinfo/rust-vmm

Re: romfile resize

[Paolo Bonzini]

On 23/02/21 10:57, Jiatong Shen wrote:


   Thank you very much for the answer. so if romfile on destination got 
a larger size than source, why romfile check still does not

pass? because dest got enough space to hold romfile.


Because QEMU checks that memory areas have the same size on the source 
and destination.  You're right that it's overly strict, but it's a case 
that has never been an issue before; probably because the ROM size 
should be fixed for each QEMU "machine type", and it's better to have a 
consistent set of ROM files on all your hosts.


You can create a dummy file with the right size, or copy it from the source.

Paolo

Re: [PATCH v2 00/15] tests/tcg: Add TriCore tests

[Bastian Koppelmann]

Hi Thomas,

On Mon, Feb 22, 2021 at 10:23:23AM +0100, Thomas Huth wrote:
> On 04/06/2020 10.54, Bastian Koppelmann wrote:
> > Hi Alex,
> > 
> > I managed to update the series to successfully run make check-tcg. This 
> > required
> > some changes to the makefiles. I tried running the riscv64 and arm tests 
> > and so
> > far I didn't break anything.
> > 
> > You can find the full tree here:
> > https://github.com/bkoppelmann/qemu/tree/tricore-tcg-tests
> > 
> > Cheers,
> > Bastian
> > 
> > Bastian Koppelmann (15):
> >docker/tricore: Use stretch-slim as a base image
> >tests/tcg: Add docker_as and docker_ld cmds
> >tests/tcg: Run timeout cmds using --foreground
> >hw/tricore: Add testdevice for tests in tests/tcg/
> >tests/tcg/tricore: Add build infrastructure
> >tests/tcg/tricore: Add macros to create tests and first test 'abs'
> >tests/tcg/tricore: Add bmerge test
> >tests/tcg/tricore: Add clz test
> >tests/tcg/tricore: Add dvstep test
> >tests/tcg/tricore: Add fadd test
> >tests/tcg/tricore: Add fmul test
> >tests/tcg/tricore: Add ftoi test
> >tests/tcg/tricore: Add madd test
> >tests/tcg/tricore: Add msub test
> >tests/tcg/tricore: Add muls test
> 
>  Hi Bastian,
> 
> I'm currently looking at the containers that we build in the gitlab-CI, and
> it seems that the "debian-tricore-cross" container is currently always built
> though it is never used.
> So I'm wondering: Do you still plan to finish this series here and get it
> merged, or could we remove the "debian-tricore-cross" container from the
> gitlab-CI again?

yes, I'm still working on it. However, right now I have limited time. I plan
work on it again in the beginning of March.

Cheers,
Bastian

Re: who's using the ozlabs patchwork install for QEMU patches ?

[Bin Meng]

On Tue, Feb 23, 2021 at 5:55 PM Greg Kurz  wrote:
>
> On Tue, 23 Feb 2021 14:57:21 +0800
> Bin Meng  wrote:
>
> > On Mon, Feb 22, 2021 at 10:43 PM Greg Kurz  wrote:
> > >
> > > On Mon, 22 Feb 2021 13:59:34 +
> > > Peter Maydell  wrote:
> > >
> > > > On Mon, 22 Feb 2021 at 07:21, Greg Kurz  wrote:
> > > > >
> > > > > On Fri, 19 Feb 2021 17:51:02 +0100
> > > > > Thomas Huth  wrote:
> > > > >
> > > > > > On 19/02/2021 17.26, Peter Maydell wrote:
> > > > > > > Does anybody use the ozlabs patchwork install for QEMU patches,
> > > > > > > either occasionally or on a regular basis ?
> > > > > > > http://patchwork.ozlabs.org/project/qemu-devel/list/
> > > > > > > The admins for that system are trying to identify which of
> > > > > > > the various projects are really using their patchwork instances,
> > > > > > > so I figured I'd do a quick survey here. We don't use it
> > > > > > > as an official project tool but it's certainly possible to
> > > > > > > use it as an individual developer in one way or another.
> > > > > >
> > > > > > I think it might be used by some of the ppc hackers ... so CC:-ing 
> > > > > > to
> > > > > > qemu-pcc ...
> > > > > >
> > > > >
> > > > > I do on a very regular basis.
> > > >
> > > > Thanks for the reports. Do you use the features like assigning
> > > > patches to people and changing patch status, or do you mostly
> > > > just use it as a read-only archive-of-patches ?
> > > >
> > >
> > > Only the latter but mostly because I don't have the permissions
> > > to change status, e.g. when trying to change status of this
> > > recent patch from Cedric to rearrange the PowerPC docs:
> > >
> > > You don't have permissions to edit patch 'docs/system: Extend PPC section'
> > >
> > > My understanding is that users must be "maintainer" to edit other's
> > > patches. Only three 'maintainers' are currently listed at ozlabs for
> > > QEMU:
> >
> > I can update my patch status in the QEMU project. I am not sure if
> > this is due to I am a maintainer of another project hosted on
> > ozlabs.org.
> >
>
> Yeah everyone can update its own patches but you need to
> be maintainer of a project to update the status of other's
> patch for this project IIUC.
>

Ah, I see, thanks. So the question is whether QEMU maintainers want to
try the practice of using patchwork to help their maintenance work ...

> > >
> > > https://patchwork.ozlabs.org/api/1.0/projects/14/
> > >
> > > We had a discussion about that a few months back with Christian 
> > > Schoenebeck
> > > (9pfs maintainer, Cc'd) who also uses patchworks. It turned out we didn't
> > > quite know how to go further because of lack of documentation, but I'd be
> > > glad to experiment the full patchwork experience if someone knows how to
> > > do it :-)
> >
> > I personally found patchwork is really helpful for mainatiner's work.
> > But it looks the maintainers from the QEMU community do not use it.

Regards,
Bin

[Bug 1916394] Re: [git] Cannot build qemu: FAILED: target/hexagon/semantics_generated.pyinc

[Frederic Bezies]

Sorry for the late reply. Tried patch... Here is the output:

Option b_staticpic is: false [default: false]
Found ninja-1.10.2 at /usr/bin/ninja
[658/9072] Generating iset.py with a custom command
FAILED: target/hexagon/iset.py 
@INPUT@ target/hexagon/iset.py
/bin/sh: line 1: @INPUT@: command not found
[663/9072] Compiling C object tests/qtest/libqos/libqos.fa.p/.._libqtest.c.o
ninja: build stopped: subcommand failed.

Still busted. Nothing changed.

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

Title:
  [git] Cannot build qemu: FAILED:
  target/hexagon/semantics_generated.pyinc

Status in QEMU:
  New

Bug description:
  Hello.

  I'm using Archlinux and I maintain qemu-git AUR package.

  I tried to build Qemu at commit
  4115aec9af2a3de5fa89a0b1daa12debcd7741ff but it stops with this error
  message:

  Found ninja-1.10.2 at /usr/bin/ninja
  [632/9068] Generating semantics_generated.pyinc with a custom command
  FAILED: target/hexagon/semantics_generated.pyinc
  @INPUT@ target/hexagon/semantics_generated.pyinc
  /bin/sh: line 1: @INPUT@: command not found
  [637/9068] Compiling C object 
fsdev/vi...proxy-helper.p/virtfs-proxy-helper.c.o
  ninja: build stopped: subcommand failed.

  ninja version: 1.10.2
  meson version: 0.57.1

  Downgrading meson doesn't change anything.

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

Re: What prevents discarding a cluster during rewrite?

[Kevin Wolf]

Am 22.02.2021 um 22:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 23.02.2021 00:30, Vladimir Sementsov-Ogievskiy wrote:
> > Thinking of how to prevent dereferencing to zero (and discard) of
> > host cluster during flush of compressed cache (which I'm working on
> > now), I have a question.. What prevents it for normal writes?
> 
> I have no idea about why didn't it fail for years.. May be, I'm
> missing something?

Ouch. I suppose the reason why we never ran into it is that afaik Linux
drains the queue before sending discard requests.

Of course, we still need to protect against this in QEMU. We can't just
unref a cluster that is still in use.

> I have idea of fixing: increase the refcount of host cluster before
> write to data_file (it's important to increase refacount in same
> s->lock critical section where we get host_offset) and dereference it
> after write.. It should help. Any thoughts?

It would cause metadata updates for rewrites. I don't know whether the
intermediate value would ever be written to disk, but at least we'd
rewrite the same refcounts as before. I don't think we want that.

Discard requests might be rare enough that not considering the cluster
at all could work. We could then take a reader CoRwlock during most
operations, and a writer lock for discard.

Actually, maybe s->lock should be this CoRwlock, and instead of dropping
it temporarily like we do now we would just upgrade and downgrade it as
needed. Maybe this would allow finer grained locking in other places,
too.

Kevin

Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[Cornelia Huck]

On Tue, 23 Feb 2021 10:37:01 +1100
David Gibson  wrote:

> On Tue, Feb 23, 2021 at 10:33:55AM +1100, David Gibson wrote:
> > On Mon, Feb 22, 2021 at 06:50:44PM +0100, Cornelia Huck wrote:  
> > > On Mon, 22 Feb 2021 18:41:07 +0100
> > > Philippe Mathieu-Daudé  wrote:
> > >   
> > > > On 2/22/21 6:24 PM, Cornelia Huck wrote:  
> > > > > On Fri, 19 Feb 2021 18:38:37 +0100
> > > > > Philippe Mathieu-Daudé  wrote:
> > > > > 
> > > > >> MachineClass::kvm_type() can return -1 on failure.
> > > > >> Document it, and add a check in kvm_init().
> > > > >>
> > > > >> Signed-off-by: Philippe Mathieu-Daudé 
> > > > >> ---
> > > > >>  include/hw/boards.h | 3 ++-
> > > > >>  accel/kvm/kvm-all.c | 6 ++
> > > > >>  2 files changed, 8 insertions(+), 1 deletion(-)
> > > > >>
> > > > >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > >> index a46dfe5d1a6..68d3d10f6b0 100644
> > > > >> --- a/include/hw/boards.h
> > > > >> +++ b/include/hw/boards.h
> > > > >> @@ -127,7 +127,8 @@ typedef struct {
> > > > >>   *implement and a stub device is required.
> > > > >>   * @kvm_type:
> > > > >>   *Return the type of KVM corresponding to the kvm-type string 
> > > > >> option or
> > > > >> - *computed based on other criteria such as the host kernel 
> > > > >> capabilities.
> > > > >> + *computed based on other criteria such as the host kernel 
> > > > >> capabilities
> > > > >> + *(which can't be negative), or -1 on error.
> > > > >>   * @numa_mem_supported:
> > > > >>   *true if '--numa node.mem' option is supported and false 
> > > > >> otherwise
> > > > >>   * @smp_parse:
> > > > >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > > > >> index 84c943fcdb2..b069938d881 100644
> > > > >> --- a/accel/kvm/kvm-all.c
> > > > >> +++ b/accel/kvm/kvm-all.c
> > > > >> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
> > > > >>  
> > > > >> "kvm-type",
> > > > >>  
> > > > >> &error_abort);
> > > > >>  type = mc->kvm_type(ms, kvm_type);
> > > > >> +if (type < 0) {
> > > > >> +ret = -EINVAL;
> > > > >> +fprintf(stderr, "Failed to detect kvm-type for machine 
> > > > >> '%s'\n",
> > > > >> +mc->name);
> > > > >> +goto err;
> > > > >> +}
> > > > >>  }
> > > > >>  
> > > > >>  do {
> > > > > 
> > > > > No objection to this patch; but I'm wondering why some non-pseries
> > > > > machines implement the kvm_type callback, when I see the kvm-type
> > > > > property only for pseries? Am I holding my git grep wrong?
> > > > 
> > > > Can it be what David commented here?
> > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html
> > > >   
> > > 
> > > Ok, I might be confused about the other ppc machines; but I'm wondering
> > > about the kvm_type callback for mips and arm/virt. Maybe I'm just
> > > confused by the whole mechanism?  
> > 
> > For ppc at least, not sure about in general, pseries is the only
> > machine type that can possibly work under more than one KVM flavour
> > (HV or PR).  So, it's the only one where it's actually useful to be
> > able to configure this.  
> 
> Wait... I'm not sure that's true.  At least theoretically, some of the
> Book3E platforms could work with either PR or the Book3E specific
> KVM.  Not sure if KVM PR supports all the BookE instructions it would
> need to in practice.
> 
> Possibly pseries is just the platform where there's been enough people
> interested in setting the KVM flavour so far.

If I'm not utterly confused by the code, it seems the pseries machines
are the only ones where you can actually get to an invocation of
->kvm_type(): You need to have a 'kvm-type' machine property, and
AFAICS only the pseries machine has that.

(Or is something hiding behind some macro magic?)


pgpBB6MqDKqZs.pgp
Description: OpenPGP digital signature

Re: [PATCH v6 01/12] memory: Introduce RamDiscardMgr for RAM memory regions

[David Hildenbrand]

On 22.02.21 20:43, David Hildenbrand wrote:

The main motivation is to let listener decide how it wants to handle the
memory region. For example, for vhost, vdpa, kvm, ... I only want a
single region, not separate ones for each and every populated range,
punching out discarded ranges. Note that there are cases (i.e.,
anonymous memory), where it's even valid for the guest to read discarded
memory.


Yes, I agree with that.  You would still have the same
region-add/region_nop/region_del callbacks for KVM and friends; on top
of that you would have region_populate/region_discard callbacks for VFIO.


I think instead of region_populate/region_discard we would want
individual region_add/region_del when populating/discarding for all
MemoryListeners that opt-in somehow (e.g., VFIO, dump-guest-memory,
...). Similarly, we would want to call log_sync()/log_clear() then only
for these parts.

But what happens when I populate/discard some memory? I don't want to
trigger an address space transaction (begin()...region_nop()...commit())
- whenever I populate/discard memory (e.g., in 2 MB granularity).
Especially not, if nothing might have changed for most other
MemoryListeners.


Right, that was the reason why I was suggesting different callbacks.
For the VFIO listener, which doesn't have begin or commit callbacks, I
think you could just rename region_add to region_populate, and point
both region_del and region_discard to the existing region_del commit.

Calling log_sync/log_clear only for populated parts also makes sense.
log_sync and log_clear do not have to be within begin/commit, so you can
change the semantics to call them more than once.


So I looked at the simplest of all cases (discard) and I am not convinced yet
that this is the right approach. I can understand why it looks like this fits
into the MemoryListener, but I am not sure if gives us any real benefits or
makes the code any clearer (I'd even say it's the contrary).


+void memory_region_notify_discard(MemoryRegion *mr, hwaddr offset,
+  hwaddr size)
+{
+hwaddr mr_start, mr_end;
+MemoryRegionSection mrs;
+MemoryListener *listener;
+AddressSpace *as;
+FlatView *view;
+FlatRange *fr;
+
+QTAILQ_FOREACH(listener, &memory_listeners, link) {
+if (!listener->region_discard) {
+continue;
+}
+as = listener->address_space;
+view = address_space_get_flatview(as);
+FOR_EACH_FLAT_RANGE(fr, view) {
+if (fr->mr != mr) {
+continue;
+}
+
+mrs = section_from_flat_range(fr, view);
+
+mr_start = MAX(mrs.offset_within_region, offset);
+mr_end = MIN(offset + size,
+  mrs.offset_within_region + int128_get64(mrs.size));
+mr_end = MIN(mr_end, offset + size);
+
+if (mr_start >= mr_end) {
+continue;
+}
+
+mrs.offset_within_address_space += mr_start -
+   mrs.offset_within_region;
+mrs.offset_within_region = mr_start;
+mrs.size = int128_make64(mr_end - mr_start);
+listener->region_discard(listener, &mrs);
+}
+flatview_unref(view);
+}
+}

Maybe I am missing something important. This looks highly inefficient.

1. Although we know the memory region we have to walk over the whole address
 space ... over and over again for each potential listener.

2. Even without any applicable listeners (=> ! VFIO) we loop over all listeners.
 There are ways around that but it doesn't make the code nicer IMHO.

3. In the future I am planning on sending populate/discard events
 without the BQL (in my approach, synchronizing internally against
 register/unregister/populate/discard ...). I don't see an easy way
 to achieve that here. I think we are required to hold the BQL on any
 updates.

memory_region_notify_populate() gets quite ugly when we realize halfway that
we have to revert what we already did by notifying about already populated
pieces ...



So, the more I look into it the more I doubt this should go into the 
MemoryListener.


The RamDiscardManager is specific to RAM memory regions - similarly the 
IOMMU notifier is specific to IOMMU regions.


In the near future we will have two "clients" (vfio, 
tpm/dump-guest-memory), whereby only vfio will actually has to register 
for updates at runtime.


I really want to have a dedicated registration/notifier mechanism, for 
reasons already mentioned in my last mail, but also to later reuse that 
mechanism in other context as noted in the cover letter:


"Note: At some point, we might want to let RAMBlock users (esp. vfio 
used for nvme://) consume this interface as well. We'll need RAMBlock 
notifier calls when a RAMBlock is getting mapped/unmapped (via the 
corresponding memory region), so we can properly register a listener 
there as well."



However, I d

[RFC PATCH] docs/devel: re-organise the developers guide into sections

[Alex Bennée]

The list of sub-sections was getting a bit long and sporadically
organised. Let's try and impose some order on this hairball of
documentation.

[AJB: RFC because I wonder if we should make a more concerted effort
to move bits of the wiki into a canonical maintained document. There
is also probably a need for a quickbuild or tldr section of the
build-system for users who just want to build something.]

Based-on: 20210223095931.16908-1-alex.ben...@linaro.org
Signed-off-by: Alex Bennée 
---
 docs/devel/index.rst | 32 ++--
 docs/devel/multi-thread-tcg.rst  |  5 +++--
 docs/devel/section-apis.rst  | 16 ++
 docs/devel/section-building.rst  | 13 +++
 docs/devel/section-concepts.rst  | 21 ++
 docs/devel/section-process.rst   | 11 ++
 docs/devel/section-tcg-emulation.rst | 19 +
 docs/devel/section-testing.rst   | 20 +
 docs/devel/tcg-icount.rst|  6 +++---
 docs/devel/testing.rst   |  6 +++---
 10 files changed, 115 insertions(+), 34 deletions(-)
 create mode 100644 docs/devel/section-apis.rst
 create mode 100644 docs/devel/section-building.rst
 create mode 100644 docs/devel/section-concepts.rst
 create mode 100644 docs/devel/section-process.rst
 create mode 100644 docs/devel/section-tcg-emulation.rst
 create mode 100644 docs/devel/section-testing.rst

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index ae664da00c..2af7bf8736 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -13,29 +13,9 @@ Contents:
 .. toctree::
:maxdepth: 2
 
-   build-system
-   style
-   kconfig
-   testing
-   fuzzing
-   control-flow-integrity
-   loads-stores
-   memory
-   migration
-   atomics
-   stable-process
-   qtest
-   decodetree
-   secure-coding-practices
-   tcg
-   tcg-icount
-   tracing
-   multi-thread-tcg
-   tcg-plugins
-   bitops
-   reset
-   s390-dasd-ipl
-   clocks
-   qom
-   block-coroutine-wrapper
-   multi-process
+   section-building
+   section-testing
+   section-concepts
+   section-apis
+   section-tcg-emulation
+   section-process
diff --git a/docs/devel/multi-thread-tcg.rst b/docs/devel/multi-thread-tcg.rst
index 92a9eba13c..5b446ee08b 100644
--- a/docs/devel/multi-thread-tcg.rst
+++ b/docs/devel/multi-thread-tcg.rst
@@ -4,8 +4,9 @@
   This work is licensed under the terms of the GNU GPL, version 2 or
   later. See the COPYING file in the top-level directory.
 
-Introduction
-
+==
+Multi-threaded TCG
+==
 
 This document outlines the design for multi-threaded TCG (a.k.a MTTCG)
 system-mode emulation. user-mode emulation has always mirrored the
diff --git a/docs/devel/section-apis.rst b/docs/devel/section-apis.rst
new file mode 100644
index 00..ab1f6bed60
--- /dev/null
+++ b/docs/devel/section-apis.rst
@@ -0,0 +1,16 @@
+*
+QEMU APIs
+*
+
+There are a number of APIs in QEMU and the following sections document
+some of the most important ones. For tose that aren't documented here
+you can also find notes on usage in the header definitions.
+
+.. toctree::
+   :maxdepth: 2
+
+   atomics
+   bitops
+   loads-stores
+   memory
+   tracing
diff --git a/docs/devel/section-building.rst b/docs/devel/section-building.rst
new file mode 100644
index 00..e6e9fa1d6a
--- /dev/null
+++ b/docs/devel/section-building.rst
@@ -0,0 +1,13 @@
+*
+Building QEMU
+*
+
+The following sections deal with how the build system works, how it is
+configured and some basic guidance on how code should be written.
+
+.. toctree::
+   :maxdepth: 2
+
+   build-system
+   kconfig
+   style
diff --git a/docs/devel/section-concepts.rst b/docs/devel/section-concepts.rst
new file mode 100644
index 00..566c52e90a
--- /dev/null
+++ b/docs/devel/section-concepts.rst
@@ -0,0 +1,21 @@
+*
+QEMU Concepts
+*
+
+There are a number of high level concepts that are useful to
+understand when working with the code base. Perhaps the most pervasive
+is the QEMU Object Model (QOM) which underpins much of the flexibility
+and configurable of the project. The following sections document that
+as well as diving into other concepts that are useful to know if
+working on some areas of the code.
+
+.. toctree::
+   :maxdepth: 2
+
+   qom
+   clocks
+   reset
+   block-coroutine-wrapper
+   migration
+   multi-process
+   s390-dasd-ipl
diff --git a/docs/devel/section-process.rst b/docs/devel/section-process.rst
new file mode 100644
index 00..3b0ae4f19b
--- /dev/null
+++ b/docs/devel/section-process.rst
@@ -0,0 +1,11 @@
+===
+Development Process
+===
+
+The following sections detail aspects of the development process.
+
+.. toctree::
+   :maxdepth: 2
+
+   secure-coding-practices
+   stable-process
diff --git a/docs/devel/section-tcg-emulation.rst 
b/docs/devel/section-tcg-emulation.rst
new file mode 100644
index 0

Re: [PATCH] vhost: simplify vhost_dev_init() fail_busyloop label

[Stefano Garzarella]

On Mon, Feb 22, 2021 at 11:49:31AM +, Stefan Hajnoczi wrote:

Requiring a conditional for every goto is tedious:

 if (busyloop_timeout) {
 goto fail_busyloop;
 } else {
 goto fail;
 }

Move the conditional to into the fail_busyloop label so that it's safe
to jump to this label unconditionally.

This change makes the migrate_add_blocker() error case more consistent.
It jumped to fail_busyloop unconditionally whereas the memslots limits
error case was conditional.

Signed-off-by: Stefan Hajnoczi 
---
hw/virtio/vhost.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)


Reviewed-by: Stefano Garzarella 



diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6e17d631f7..2a01662b08 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1388,18 +1388,16 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
error_report("vhost backend memory slots limit is less"
" than current number of present memory slots");
r = -1;
-if (busyloop_timeout) {
-goto fail_busyloop;
-} else {
-goto fail;
-}
+goto fail_busyloop;
}

return 0;

fail_busyloop:
-while (--i >= 0) {
-vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
+if (busyloop_timeout) {
+while (--i >= 0) {
+vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
+}
}
fail:
hdev->nvqs = n_initialized_vqs;
--
2.29.2

Re: [PATCH 2/2] hw/nvme: move device-scoped functions

[Minwoo Im]

On 21-02-09 12:08:26, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Move a bunch of functions that are internal to a device out of the
> shared header.
> 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/nvme/nvme.h | 110 +
>  hw/nvme/ctrl.c |  90 +++-
>  hw/nvme/ns.c   |   7 +++-
>  3 files changed, 97 insertions(+), 110 deletions(-)
> 
> diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> index 452a64499b1b..929c6c553ca2 100644
> --- a/hw/nvme/nvme.h
> +++ b/hw/nvme/nvme.h
> @@ -96,36 +96,13 @@ static inline uint32_t nvme_nsid(NvmeNamespace *ns)
>  return -1;
>  }
>  
> -static inline bool nvme_ns_shared(NvmeNamespace *ns)
> -{
> -return !!ns->subsys;
> -}

Re-raising this up.

Something like this helper is related to the data structure usages
itself like, if ns->subsys is not NULL, it "can" mean that this
namespace is shared among controllers.  This helper represents that the
'subsys' member itself is indicating some meaning, not only just for the
subsystem itself.

That's why I've been hesitating to simply ack to this patch ;)

But, I am not strongly against to this so please make a decision with
Keith and go ahead!

Thanks!

>  static inline uint8_t nvme_ns_lbads(NvmeNamespace *ns)
>  {
> -return nvme_ns_lbaf(ns)->ds;
> -}
> +NvmeLBAF lbaf = ns->id_ns.lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)];
>  
> -/* calculate the number of LBAs that the namespace can accomodate */
> -static inline uint64_t nvme_ns_nlbas(NvmeNamespace *ns)
> -{
> -return ns->size >> nvme_ns_lbads(ns);
> +return lbaf.ds;
>  }
>  
> -/* convert an LBA to the equivalent in bytes */
> -static inline size_t nvme_l2b(NvmeNamespace *ns, uint64_t lba)
> -{
> -return lba << nvme_ns_lbads(ns);
> -}
> -
> -typedef struct NvmeCtrl NvmeCtrl;
> -
>  static inline NvmeZoneState nvme_get_zone_state(NvmeZone *zone)
>  {
>  return zone->d.zs >> 4;
> @@ -136,31 +113,6 @@ static inline void nvme_set_zone_state(NvmeZone *zone, 
> NvmeZoneState state)
>  zone->d.zs = state << 4;
>  }
>  
> -static inline uint64_t nvme_zone_rd_boundary(NvmeNamespace *ns, NvmeZone 
> *zone)
> -{
> -return zone->d.zslba + ns->zone_size;
> -}
> -
> -static inline uint64_t nvme_zone_wr_boundary(NvmeZone *zone)
> -{
> -return zone->d.zslba + zone->d.zcap;
> -}
> -
> -static inline bool nvme_wp_is_valid(NvmeZone *zone)
> -{
> -uint8_t st = nvme_get_zone_state(zone);
> -
> -return st != NVME_ZONE_STATE_FULL &&
> -   st != NVME_ZONE_STATE_READ_ONLY &&
> -   st != NVME_ZONE_STATE_OFFLINE;
> -}
> -
> -static inline uint8_t *nvme_get_zd_extension(NvmeNamespace *ns,
> - uint32_t zone_idx)
> -{
> -return &ns->zd_extensions[zone_idx * ns->params.zd_extension_size];
> -}
> -
>  static inline void nvme_aor_inc_open(NvmeNamespace *ns)
>  {
>  assert(ns->nr_open_zones >= 0);
> @@ -203,7 +155,6 @@ void nvme_ns_drain(NvmeNamespace *ns);
>  void nvme_ns_shutdown(NvmeNamespace *ns);
>  void nvme_ns_cleanup(NvmeNamespace *ns);
>  
> -
>  typedef struct NvmeParams {
>  char *serial;
>  uint32_t num_queues; /* deprecated since 5.1 */
> @@ -237,40 +188,6 @@ typedef struct NvmeRequest {
>  QTAILQ_ENTRY(NvmeRequest)entry;
>  } NvmeRequest;
>  
> -static inline const char *nvme_adm_opc_str(uint8_t opc)
> -{
> -switch (opc) {
> -case NVME_ADM_CMD_DELETE_SQ:return "NVME_ADM_CMD_DELETE_SQ";
> -case NVME_ADM_CMD_CREATE_SQ:return "NVME_ADM_CMD_CREATE_SQ";
> -case NVME_ADM_CMD_GET_LOG_PAGE: return "NVME_ADM_CMD_GET_LOG_PAGE";
> -case NVME_ADM_CMD_DELETE_CQ:return "NVME_ADM_CMD_DELETE_CQ";
> -case NVME_ADM_CMD_CREATE_CQ:return "NVME_ADM_CMD_CREATE_CQ";
> -case NVME_ADM_CMD_IDENTIFY: return "NVME_ADM_CMD_IDENTIFY";
> -case NVME_ADM_CMD_ABORT:return "NVME_ADM_CMD_ABORT";
> -case NVME_ADM_CMD_SET_FEATURES: return "NVME_ADM_CMD_SET_FEATURES";
> -case NVME_ADM_CMD_GET_FEATURES: return "NVME_ADM_CMD_GET_FEATURES";
> -case NVME_ADM_CMD_ASYNC_EV_REQ: return "NVME_ADM_CMD_ASYNC_EV_REQ";
> -default:return "NVME_ADM_CMD_UNKNOWN";
> -}
> -}
> -
> -static inline const char *nvme_io_opc_str(uint8_t opc)
> -{
> -switch (opc) {
> -case NVME_CMD_FLUSH:return "NVME_NVM_CMD_FLUSH";
> -case NVME_CMD_WRITE:return "NVME_NVM_CMD_WRITE";
> -case NVME_CMD_READ: return "NVME_NVM_CMD_READ";
> -case NVME_CMD_COMPARE:  return "NVME_NVM_CMD_COMPARE";
> -case NVME_CMD_WRITE_ZEROES: return "NVME_NVM_CMD_WRITE_ZEROES";
> -case NVME_CMD_DSM:  return "NVME_NVM_CMD_DSM";
> -case NVME_CMD_COPY: return "NVME_NVM_CMD_COPY";
> -case NVME_CMD_ZONE_MGMT_SEND:   return "NVME_ZONED_CMD_MGMT_SEND";
> -case NVME_CMD_ZONE_MGMT_RECV:   return "NVME_ZONED_CMD_MGMT_RECV";
> -case NVME_C

Re: Plugin Address Translations Inconsistent/Incorrect?

[Peter Maydell]

On Tue, 23 Feb 2021 at 08:55, Alex Bennée  wrote:
>
>
> Peter Maydell  writes:
>
> > On Mon, 22 Feb 2021 at 19:53, Alex Bennée  wrote:
> >> It certainly is by design. The comment for the helper states:
> >>
> >>   /*
> >>* The following additional queries can be run on the hwaddr structure
> >>* to return information about it. For non-IO accesses the device
> >>* offset will be into the appropriate block of RAM.
> >>*/
> >
> > That sounds like we're exposing ram_addrs to the plugin. Are we?
> > I'm not sure that's a good idea, as they're not a guest-relevant
> > construct.
>
> We currently expose qemu_ram_addr_from_host for RAM blocks. Are you
> saying we should translate that to the direct physical address mapping?
> If we do that for RAM should we be doing the same for IO addresses

This is not a fully-thought-through position, but as a general
thing I feel that the plugin API should be exposing as far as
possible concepts that relate to the guest program, not concepts
that relate to QEMU's internal implementation. One of the issues
with the -d logging is that it's very much "provide information
that's easy for QEMU to provide", and this confuses people who
just want to debug their guest. We can reasonably expect users to
understand concepts like "physical address", but it's less nice
to need them to understand "QEMU models guest RAM as a bunch of
blocks with a 'ram address' which is an offset into the stack
of blocks, and this isn't the same thing as any guest address".
It also constrains our ability to change the QEMU implementation
later if we let implementation details leak out via the plugin API.

thanks
-- PMM

Re: A brief look at deprecating our JSON extensions over RFC 8259

[Paolo Bonzini]

On 23/02/21 10:06, Markus Armbruster wrote:

Markus, did you rebuild the qtests after disabling single-quoted
strings?  "make check-qtest-x86_64" would have rebuilt them, but I'm
confused by the results.

I ran "make check" and looked at the failures:

Still confused?


Yes.  What's the patch that you used to disable the single quotes, and 
why didn't the patched parser choke on


response = qmp("{ 'execute': 'qom-get', 'arguments': { 'path': %s, "
   "'property': 'temperature' } }", id);

?

Paolo

Re: A brief look at deprecating our JSON extensions over RFC 8259

[Peter Maydell]

On Tue, 23 Feb 2021 at 09:33, Markus Armbruster  wrote:
> Misunderstanding: our JSON interpolation feature is *not* string
> interpolation!  It interpolates *objects* into the QObject built by the
> parser.

Given that it's basically undocumented except in a scattered
handful of comments inside the qjson parser implementation, it's
not too surprising that people misunderstand it :-) (One surprising
feature: the parser takes ownership of the object that you pass it
via the '%p' interpolation, and will qobject_unref() it.)

-- PMM

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Alex Bennée]

Claudio Fontana  writes:

> On 2/22/21 4:34 PM, Alex Bennée wrote:
>> 
>> Claudio Fontana  writes:
>> 
>>> From: Claudio Fontana 
>>>
>>> KVM has its own cpu->kvm_vtime.
>>>
>>> Adjust cpu vmstate by putting unused fields instead of the
>>> VMSTATE_TIMER_PTR when TCG is not available.
>>>
>>> Signed-off-by: Claudio Fontana 
>>> ---
>>>  target/arm/cpu.c | 4 +++-
>>>  target/arm/machine.c | 5 +
>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 1d81a1e7ac..b929109054 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
>>> **errp)
>>>  }
>>>  }
>>>  
>>> +#ifdef CONFIG_TCG
>>>  {
>>>  uint64_t scale;
>>>  
>>> @@ -1347,7 +1348,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
>>> **errp)
>>>  cpu->gt_timer[GTIMER_HYPVIRT] = timer_new(QEMU_CLOCK_VIRTUAL, 
>>> scale,
>>>arm_gt_hvtimer_cb, cpu);
>>>  }
>>> -#endif
>>> +#endif /* CONFIG_TCG */
>>> +#endif /* !CONFIG_USER_ONLY */
>>>  
>>>  cpu_exec_realizefn(cs, &local_err);
>>>  if (local_err != NULL) {
>>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>>> index 666ef329ef..13d7c6d930 100644
>>> --- a/target/arm/machine.c
>>> +++ b/target/arm/machine.c
>>> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>>>  VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>>>  VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>>>  VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>>> +#ifdef CONFIG_TCG
>>>  VMSTATE_TIMER_PTR(gt_timer[GTIMER_PHYS], ARMCPU),
>>>  VMSTATE_TIMER_PTR(gt_timer[GTIMER_VIRT], ARMCPU),
>>> +#else
>>> +VMSTATE_UNUSED(sizeof(QEMUTimer *)),
>>> +VMSTATE_UNUSED(sizeof(QEMUTimer *)),
>>> +#endif /* CONFIG_TCG */
>> 
>> I'm not sure this is correct - VMSTATE_TIMER_PTR chases the links to
>> just expose expired time but QEMUTimer has more in it than that. Paolo
>
>
> I am not sure I follow can you state more precisely where the issue could be?
>
> it's not a VMSTATE_TIMER, it's a VMSTATE_TIMER_PTR,
> it ends up in VMSTATE_POINTER where a single pointer is assigned;

Does it? I thought it ended up with the .expire_time (int64_t) which
will be bigger than sizeof(QemuTimer *) on a 32 bit system.

>
> so if we don't use gt_timer at all (as is the case with !CONFIG_TCG), we just
> need to ensure that an unused number is there to assign, migrating from old 
> to new version?
>
>
>> suggested a straight VMSTATE_UNUSED(8) on IRC but I wonder if it would
>> be better to have a VMSTATE_UNUSED_TIMER?
>> 
>> I don't think there is an impact for Xen because I'm fairly certain
>> migration isn't a thing we do - but I'll double check.
>> 
>
> Thanks Alex, that would be helpful,
> if Xen uses gt_timer in any way I would not want to unwillingly break
> it.

Not for ARM no, currently there is no ARM specific machine emulated by
QEMU for Xen. All ARM guests are PV guests.

>
> Thanks,
>
> Claudio


-- 
Alex Bennée

Re: [PATCH] docs: move CODING_STYLE into the developer documentation

[Claudio Fontana]

On 2/23/21 10:59 AM, Alex Bennée wrote:
> There is no particular reason to keep this on it's own in the root of
> the tree. Move it into the rest of the fine developer manual and fixup
> any links to it. The only tweak I've made is to fix the code-block
> annotations to mention the language C.
> 
> Signed-off-by: Alex Bennée 

Reviewed-by: Claudio Fontana 

Are there pointers in wiki.qemu.org that need updating?


> ---
>  docs/devel/index.rst | 1 +
>  CODING_STYLE.rst => docs/devel/style.rst | 6 +++---
>  README.rst   | 4 +++-
>  scripts/fix-multiline-comments.sh| 2 +-
>  4 files changed, 8 insertions(+), 5 deletions(-)
>  rename CODING_STYLE.rst => docs/devel/style.rst (99%)
> 
> diff --git a/docs/devel/index.rst b/docs/devel/index.rst
> index 22854e334d..ae664da00c 100644
> --- a/docs/devel/index.rst
> +++ b/docs/devel/index.rst
> @@ -14,6 +14,7 @@ Contents:
> :maxdepth: 2
>  
> build-system
> +   style
> kconfig
> testing
> fuzzing
> diff --git a/CODING_STYLE.rst b/docs/devel/style.rst
> similarity index 99%
> rename from CODING_STYLE.rst
> rename to docs/devel/style.rst
> index 7bf4e39d48..8b0bdb3570 100644
> --- a/CODING_STYLE.rst
> +++ b/docs/devel/style.rst
> @@ -641,7 +641,7 @@ trace-events style
>  
>  In trace-events files, use a '0x' prefix to specify hex numbers, as in:
>  
> -.. code-block::
> +.. code-block:: c
>  
>  some_trace(unsigned x, uint64_t y) "x 0x%x y 0x" PRIx64
>  
> @@ -649,14 +649,14 @@ An exception is made for groups of numbers that are 
> hexadecimal by
>  convention and separated by the symbols '.', '/', ':', or ' ' (such as
>  PCI bus id):
>  
> -.. code-block::
> +.. code-block:: c
>  
>  another_trace(int cssid, int ssid, int dev_num) "bus id: %x.%x.%04x"
>  
>  However, you can use '0x' for such groups if you want. Anyway, be sure that
>  it is obvious that numbers are in hex, ex.:
>  
> -.. code-block::
> +.. code-block:: c
>  
>  data_dump(uint8_t c1, uint8_t c2, uint8_t c3) "bytes (in hex): %02x %02x 
> %02x"
>  
> diff --git a/README.rst b/README.rst
> index ce39d89077..f5d41e59b1 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -66,7 +66,9 @@ When submitting patches, one common approach is to use 'git
>  format-patch' and/or 'git send-email' to format & send the mail to the
>  qemu-devel@nongnu.org mailing list. All patches submitted must contain
>  a 'Signed-off-by' line from the author. Patches should follow the
> -guidelines set out in the CODING_STYLE.rst file.
> +guidelines set out in the `style section
> +` of
> +the Developers Guide.
>  
>  Additional information on submitting patches can be found online via
>  the QEMU website
> diff --git a/scripts/fix-multiline-comments.sh 
> b/scripts/fix-multiline-comments.sh
> index 93f9b10669..c15a041272 100755
> --- a/scripts/fix-multiline-comments.sh
> +++ b/scripts/fix-multiline-comments.sh
> @@ -1,6 +1,6 @@
>  #! /bin/sh
>  #
> -# Fix multiline comments to match CODING_STYLE
> +# Fix multiline comments to match docs/devel/style.rst
>  #
>  # Copyright (C) 2018 Red Hat, Inc.
>  #
> 

Re: [PATCH v2 00/12] block/export: vhost-user-blk server tests and input validation

[Philippe Mathieu-Daudé]

On 2/19/21 11:38 PM, Peter Maydell wrote:
> On Mon, 15 Feb 2021 at 10:41, Kevin Wolf  wrote:
>>
>> Am 07.12.2020 um 18:20 hat Stefan Hajnoczi geschrieben:
>>> v2:
>>>  * Add abrt handler that terminates qemu-storage-daemon to
>>>vhost-user-blk-test. No more orphaned processes on test failure. [Peter]
>>>  * Fix sector number calculation in vhost-user-blk-server.c
>>>  * Introduce VIRTIO_BLK_SECTOR_BITS/SIZE to make code clearer [Max]
>>>  * Fix vhost-user-blk-server.c blk_size double byteswap
>>>  * Fix vhost-user-blk blkcfg->num_queues endianness [Peter]
>>>  * Squashed cleanups into Coiby vhost-user-blk-test commit so the code is
>>>easier to review
>>>
>>> The vhost-user-blk server test was already in Michael Tsirkin's recent vhost
>>> pull request, but was dropped because it exposed vhost-user regressions
>>> (b7c1bd9d7848 and the Based-on tag below). Now that the vhost-user 
>>> regressions
>>> are fixed we can re-introduce the test case.
>>>
>>> This series adds missing input validation that led to a Coverity report. The
>>> virtio-blk read, write, discard, and write zeroes commands need to check
>>> sector/byte ranges and other inputs. This solves the issue Peter Maydell 
>>> raised
>>> in "[PATCH for-5.2] block/export/vhost-user-blk-server.c: Avoid potential
>>> integer overflow".
>>>
>>> Merging just the input validation patches would be possible too, but I 
>>> prefer
>>> to merge the corresponding tests so the code is exercised by the CI.
>>
>> Is this series still open? I don't see it in master.
> 
> The Coverity issue is still unfixed, at any rate...

Copying Coverity report here:

CID 1435956 Unintentional integer overflow

In vu_blk_discard_write_zeroes: An integer overflow occurs, with the
result converted to a wider integer type (CWE-190)

 61 static int coroutine_fn
 62 vu_blk_discard_write_zeroes(BlockBackend *blk, struct iovec *iov,
 63 uint32_t iovcnt, uint32_t type)
 64 {
 65 struct virtio_blk_discard_write_zeroes desc;
 66 ssize_t size = iov_to_buf(iov, iovcnt, 0, &desc, sizeof(desc));
 67 if (unlikely(size != sizeof(desc))) {
 68 error_report("Invalid size %zd, expect %zu", size,
sizeof(desc));
 69 return -EINVAL;
 70 }
 71
 72 uint64_t range[2] = { le64_to_cpu(desc.sector) << 9,

CID 1435956 (#1 of 1): Unintentional integer overflow
(OVERFLOW_BEFORE_WIDEN)
overflow_before_widen: Potentially overflowing expression
le32_to_cpu(desc.num_sectors) << 9 with type uint32_t (32 bits,
unsigned) is evaluated using 32-bit arithmetic, and then used in a
context that expects an expression of type uint64_t (64 bits, unsigned).

 73   le32_to_cpu(desc.num_sectors) << 9 };

[PATCH] ui/cocoa: Do not rely on the first argument

[Akihiko Odaki]

The first argument of the executable was used to get its path, but it is
not reliable because the executer can specify any arbitrary string. Use the
interfaces provided by QEMU and the platform to get those paths.

This change also changes the icon shown in the "about" window to QEMU's
cute one.

Signed-off-by: Akihiko Odaki 
---
 ui/cocoa.m | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 0ef5fdf3b7a..b3e7833e7fa 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -39,6 +39,7 @@
 #include "qapi/qapi-commands-misc.h"
 #include "sysemu/blockdev.h"
 #include "qemu-version.h"
+#include "qemu/cutils.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include 
@@ -1401,18 +1402,13 @@ - (void)make_about_window
 y = about_height - picture_height - 10;
 NSRect picture_rect = NSMakeRect(x, y, picture_width, picture_height);
 
-/* Get the path to the QEMU binary */
-NSString *binary_name = [NSString stringWithCString: gArgv[0]
-  encoding: NSASCIIStringEncoding];
-binary_name = [binary_name lastPathComponent];
-NSString *program_path = [[NSString alloc] initWithFormat: @"%@/%@",
-[[NSBundle mainBundle] bundlePath], binary_name];
-
 /* Make the picture of QEMU */
 NSImageView *picture_view = [[NSImageView alloc] initWithFrame:
  picture_rect];
-NSImage *qemu_image = [[NSWorkspace sharedWorkspace] iconForFile:
- program_path];
+char *qemu_image_path_c = get_relocated_path(CONFIG_QEMU_ICONDIR 
"/hicolor/512x512/apps/qemu.png");
+NSString *qemu_image_path = [NSString 
stringWithUTF8String:qemu_image_path_c];
+g_free(qemu_image_path_c);
+NSImage *qemu_image = [[NSImage alloc] 
initWithContentsOfFile:qemu_image_path];
 [picture_view setImage: qemu_image];
 [picture_view setImageScaling: NSImageScaleProportionallyUpOrDown];
 [superView addSubview: picture_view];
@@ -1427,9 +1423,7 @@ - (void)make_about_window
 [name_label setBezeled: NO];
 [name_label setDrawsBackground: NO];
 [name_label setAlignment: NSTextAlignmentCenter];
-NSString *qemu_name = [[NSString alloc] initWithCString: gArgv[0]
-encoding: NSASCIIStringEncoding];
-qemu_name = [qemu_name lastPathComponent];
+NSString *qemu_name = [[[NSBundle mainBundle] executablePath] 
lastPathComponent];
 [name_label setStringValue: qemu_name];
 [superView addSubview: name_label];
 
-- 
2.24.3 (Apple Git-128)

Re: [PATCH] docs: move CODING_STYLE into the developer documentation

[Peter Maydell]

On Tue, 23 Feb 2021 at 10:02, Alex Bennée  wrote:
>
> There is no particular reason to keep this on it's own in the root of
> the tree. Move it into the rest of the fine developer manual and fixup
> any links to it. The only tweak I've made is to fix the code-block
> annotations to mention the language C.
>
> Signed-off-by: Alex Bennée 
> ---
> diff --git a/README.rst b/README.rst
> index ce39d89077..f5d41e59b1 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -66,7 +66,9 @@ When submitting patches, one common approach is to use 'git
>  format-patch' and/or 'git send-email' to format & send the mail to the
>  qemu-devel@nongnu.org mailing list. All patches submitted must contain
>  a 'Signed-off-by' line from the author. Patches should follow the
> -guidelines set out in the CODING_STYLE.rst file.
> +guidelines set out in the `style section
> +` of
> +the Developers Guide.

This is the first instance of a qemu.readthedocs.io URL in the
tree. Do we really want to have our references to our documentation
be to a third party website ?

thanks
-- PMM

Re: who's using the ozlabs patchwork install for QEMU patches ?

[Peter Maydell]

On Mon, 22 Feb 2021 at 14:43, Greg Kurz  wrote:
> My understanding is that users must be "maintainer" to edit other's
> patches. Only three 'maintainers' are currently listed at ozlabs for
> QEMU:
>
> https://patchwork.ozlabs.org/api/1.0/projects/14/
>
> We had a discussion about that a few months back with Christian Schoenebeck
> (9pfs maintainer, Cc'd) who also uses patchworks. It turned out we didn't
> quite know how to go further because of lack of documentation, but I'd be
> glad to experiment the full patchwork experience if someone knows how to
> do it :-)

If people want to try that kind of thing out I'm happy to try
to tweak their permissions on the patchwork instance.

-- PMM

[PATCH] target/hexagon/opcodes: Add missing varargs cleanup

[Philippe Mathieu-Daudé]

Fix a trivial incorrect usage of variable argument macros detected
by Coverity (missing_va_end: va_end was not called for ap).

Fixes: Coverity CID 1446720 (VARARGS)
Fixes: e3c00c2ed75 ("Hexagon (target/hexagon) opcode data structures")
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/hexagon/opcodes.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/hexagon/opcodes.c b/target/hexagon/opcodes.c
index 4eef5fc40f6..35d790cdd5b 100644
--- a/target/hexagon/opcodes.c
+++ b/target/hexagon/opcodes.c
@@ -82,6 +82,7 @@ static void init_attribs(int tag, ...)
 while ((attr = va_arg(ap, int)) != 0) {
 set_bit(attr, opcode_attribs[tag]);
 }
+va_end(ap);
 }
 
 const OpcodeEncoding opcode_encodings[] = {
-- 
2.26.2

Re: [RFC PATCH] docs/devel: re-organise the developers guide into sections

[Peter Maydell]

On Tue, 23 Feb 2021 at 10:51, Alex Bennée  wrote:
>
> The list of sub-sections was getting a bit long and sporadically
> organised. Let's try and impose some order on this hairball of
> documentation.

Yeah, the 'devel' section has always been just a grab-bag
of whatever docs we had to hand. On the other hand, to my
mind it is the area of the documentation that is least in
need of much effort, because it's only for developers, not
for the much wider group of end-users.

Anyways, a bit more structure certainly doesn't hurt.

> [AJB: RFC because I wonder if we should make a more concerted effort
> to move bits of the wiki into a canonical maintained document. There
> is also probably a need for a quickbuild or tldr section of the
> build-system for users who just want to build something.]

'How to build' is in README.rst. We don't want that in the devel/
section, because we describe 'devel' as "You only need to read it
if you are interested in reading or modifying QEMU’s source code".

> diff --git a/docs/devel/section-apis.rst b/docs/devel/section-apis.rst
> new file mode 100644
> index 00..ab1f6bed60
> --- /dev/null
> +++ b/docs/devel/section-apis.rst
> @@ -0,0 +1,16 @@
> +*
> +QEMU APIs
> +*
> +
> +There are a number of APIs in QEMU and the following sections document
> +some of the most important ones. For tose that aren't documented here
> +you can also find notes on usage in the header definitions.

"those"

-- PMM

Re: [PATCH] vhost: simplify vhost_dev_init() fail_busyloop label

[Philippe Mathieu-Daudé]

On 2/22/21 12:49 PM, Stefan Hajnoczi wrote:
> Requiring a conditional for every goto is tedious:
> 
>   if (busyloop_timeout) {
>   goto fail_busyloop;
>   } else {
>   goto fail;
>   }
> 
> Move the conditional to into the fail_busyloop label so that it's safe
> to jump to this label unconditionally.
> 
> This change makes the migrate_add_blocker() error case more consistent.
> It jumped to fail_busyloop unconditionally whereas the memslots limits
> error case was conditional.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  hw/virtio/vhost.c | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v5 1/4] Jobs based on custom runners: documentation and configuration placeholder

[Alex Bennée]

Cleber Rosa  writes:

> As described in the included documentation, the "custom runner" jobs
> extend the GitLab CI jobs already in place.  One of their primary
> goals of catching and preventing regressions on a wider number of host
> systems than the ones provided by GitLab's shared runners.
>
> This sets the stage in which other community members can add their own
> machine configuration documentation/scripts, and accompanying job
> definitions.  As a general rule, those newly added contributed jobs
> should run as "non-gating", until their reliability is verified (AKA
> "allow_failure: true").
>
> Signed-off-by: Cleber Rosa 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[Philippe Mathieu-Daudé]

On 2/23/21 11:36 AM, Cornelia Huck wrote:
> On Tue, 23 Feb 2021 10:37:01 +1100
> David Gibson  wrote:
> 
>> On Tue, Feb 23, 2021 at 10:33:55AM +1100, David Gibson wrote:
>>> On Mon, Feb 22, 2021 at 06:50:44PM +0100, Cornelia Huck wrote:  
 On Mon, 22 Feb 2021 18:41:07 +0100
 Philippe Mathieu-Daudé  wrote:
   
> On 2/22/21 6:24 PM, Cornelia Huck wrote:  
>> On Fri, 19 Feb 2021 18:38:37 +0100
>> Philippe Mathieu-Daudé  wrote:
>> 
>>> MachineClass::kvm_type() can return -1 on failure.
>>> Document it, and add a check in kvm_init().
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé 
>>> ---
>>>  include/hw/boards.h | 3 ++-
>>>  accel/kvm/kvm-all.c | 6 ++
>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>> index a46dfe5d1a6..68d3d10f6b0 100644
>>> --- a/include/hw/boards.h
>>> +++ b/include/hw/boards.h
>>> @@ -127,7 +127,8 @@ typedef struct {
>>>   *implement and a stub device is required.
>>>   * @kvm_type:
>>>   *Return the type of KVM corresponding to the kvm-type string 
>>> option or
>>> - *computed based on other criteria such as the host kernel 
>>> capabilities.
>>> + *computed based on other criteria such as the host kernel 
>>> capabilities
>>> + *(which can't be negative), or -1 on error.
>>>   * @numa_mem_supported:
>>>   *true if '--numa node.mem' option is supported and false otherwise
>>>   * @smp_parse:
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index 84c943fcdb2..b069938d881 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
>>>  "kvm-type",
>>>  
>>> &error_abort);
>>>  type = mc->kvm_type(ms, kvm_type);
>>> +if (type < 0) {
>>> +ret = -EINVAL;
>>> +fprintf(stderr, "Failed to detect kvm-type for machine 
>>> '%s'\n",
>>> +mc->name);
>>> +goto err;
>>> +}
>>>  }
>>>  
>>>  do {
>>
>> No objection to this patch; but I'm wondering why some non-pseries
>> machines implement the kvm_type callback, when I see the kvm-type
>> property only for pseries? Am I holding my git grep wrong?
>
> Can it be what David commented here?
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html
>   

 Ok, I might be confused about the other ppc machines; but I'm wondering
 about the kvm_type callback for mips and arm/virt. Maybe I'm just
 confused by the whole mechanism?  
>>>
>>> For ppc at least, not sure about in general, pseries is the only
>>> machine type that can possibly work under more than one KVM flavour
>>> (HV or PR).  So, it's the only one where it's actually useful to be
>>> able to configure this.  
>>
>> Wait... I'm not sure that's true.  At least theoretically, some of the
>> Book3E platforms could work with either PR or the Book3E specific
>> KVM.  Not sure if KVM PR supports all the BookE instructions it would
>> need to in practice.
>>
>> Possibly pseries is just the platform where there's been enough people
>> interested in setting the KVM flavour so far.
> 
> If I'm not utterly confused by the code, it seems the pseries machines
> are the only ones where you can actually get to an invocation of
> ->kvm_type(): You need to have a 'kvm-type' machine property, and
> AFAICS only the pseries machine has that.

OMG you are right... This changed in commit f2ce39b4f06
("vl: make qemu_get_machine_opts static"):

@@ -2069,13 +2068,11 @@ static int kvm_init(MachineState *ms)
 }
 s->as = g_new0(struct KVMAs, s->nr_as);

-kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
-if (mc->kvm_type) {
+if (object_property_find(OBJECT(current_machine), "kvm-type")) {
+g_autofree char *kvm_type =
object_property_get_str(OBJECT(current_machine),
+"kvm-type",
+&error_abort);
 type = mc->kvm_type(ms, kvm_type);
-} else if (kvm_type) {
-ret = -EINVAL;
-fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);
-goto err;
 }

Paolo, is that expected?

So these callbacks are dead code:
hw/arm/virt.c:2585:mc->kvm_type = virt_kvm_type;
hw/mips/loongson3_virt.c:625:mc->kvm_type = mips_kvm_type;
hw/ppc/mac_newworld.c:598:mc->kvm_type = core99_kvm_type;
hw/ppc/mac_oldworld.c:447:mc->kvm_type = heathrow_kvm_type;

> 
> (Or is something hiding behind some macro magic?)
> 

Re: [PATCH v2] virtio-blk: Respect discard granularity

[Stefano Garzarella]

On Tue, Feb 23, 2021 at 02:36:16PM +0900, Akihiko Odaki wrote:

Signed-off-by: Akihiko Odaki 
---
hw/block/virtio-blk.c  | 8 +++-
hw/core/machine.c  | 9 -
include/hw/virtio/virtio-blk.h | 1 +
3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index bac2d6fa2b2..f4378e61182 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -962,10 +962,14 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
blkcfg.wce = blk_enable_write_cache(s->blk);
virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_DISCARD)) {
+uint32_t discard_granularity = conf->discard_granularity;
+if (discard_granularity == -1 || !s->conf.report_discard_granularity) {
+discard_granularity = blk_size;
+}
virtio_stl_p(vdev, &blkcfg.max_discard_sectors,
 s->conf.max_discard_sectors);
virtio_stl_p(vdev, &blkcfg.discard_sector_alignment,
- blk_size >> BDRV_SECTOR_BITS);
+ discard_granularity >> BDRV_SECTOR_BITS);
/*
 * We support only one segment per request since multiple segments
 * are not widely used and there are no userspace APIs that allow
@@ -1299,6 +1303,8 @@ static Property virtio_blk_properties[] = {
 IOThread *),
DEFINE_PROP_BIT64("discard", VirtIOBlock, host_features,
  VIRTIO_BLK_F_DISCARD, true),
+DEFINE_PROP_BOOL("report-discard-granularity", VirtIOBlock,
+ conf.report_discard_granularity, true),
DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
  VIRTIO_BLK_F_WRITE_ZEROES, true),
DEFINE_PROP_UINT32("max-discard-sectors", VirtIOBlock,
diff --git a/hw/core/machine.c b/hw/core/machine.c
index de3b8f1b318..3ba976e5bbc 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -33,7 +33,9 @@
#include "migration/global_state.h"
#include "migration/vmstate.h"

-GlobalProperty hw_compat_5_2[] = {};
+GlobalProperty hw_compat_5_2[] = {
+{ "virtio-blk-device", "report-discard-granularity", "off" },


IIUC older machines inherit the properties set for newer machines, so I 
think only this one is enough.


Thanks,
Stefano


+};
const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);

GlobalProperty hw_compat_5_1[] = {
@@ -41,6 +43,7 @@ GlobalProperty hw_compat_5_1[] = {
{ "vhost-user-blk", "num-queues", "1"},
{ "vhost-user-scsi", "num_queues", "1"},
{ "virtio-blk-device", "num-queues", "1"},
+{ "virtio-blk-device", "report-discard-granularity", "off" },
{ "virtio-scsi-device", "num_queues", "1"},
{ "nvme", "use-intel-id", "on"},
{ "pvpanic", "events", "1"}, /* PVPANIC_PANICKED */
@@ -50,6 +53,7 @@ const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1);
GlobalProperty hw_compat_5_0[] = {
{ "pci-host-bridge", "x-config-reg-migration-enabled", "off" },
{ "virtio-balloon-device", "page-poison", "false" },
+{ "virtio-blk-device", "report-discard-granularity", "off" },
{ "vmport", "x-read-set-eax", "off" },
{ "vmport", "x-signal-unsupported-cmd", "off" },
{ "vmport", "x-report-vmx-type", "off" },
@@ -59,6 +63,7 @@ GlobalProperty hw_compat_5_0[] = {
const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);

GlobalProperty hw_compat_4_2[] = {
+{ "virtio-blk-device", "report-discard-granularity", "off" },
{ "virtio-blk-device", "queue-size", "128"},
{ "virtio-scsi-device", "virtqueue_size", "128"},
{ "virtio-blk-device", "x-enable-wce-if-config-wce", "off" },
@@ -74,6 +79,7 @@ GlobalProperty hw_compat_4_2[] = {
const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);

GlobalProperty hw_compat_4_1[] = {
+{ "virtio-blk-device", "report-discard-granularity", "off" },
{ "virtio-pci", "x-pcie-flr-init", "off" },
{ "virtio-device", "use-disabled-flag", "false" },
};
@@ -83,6 +89,7 @@ GlobalProperty hw_compat_4_0[] = {
{ "VGA","edid", "false" },
{ "secondary-vga",  "edid", "false" },
{ "bochs-display",  "edid", "false" },
+{ "virtio-blk-device", "report-discard-granularity", "off" },
{ "virtio-vga", "edid", "false" },
{ "virtio-gpu-device", "edid", "false" },
{ "virtio-device", "use-started", "false" },
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 214ab748229..29655a406dd 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -41,6 +41,7 @@ struct VirtIOBlkConf
uint16_t num_queues;
uint16_t queue_size;
bool seg_max_adjust;
+bool report_discard_granularity;
uint32_t max_discard_sectors;
uint32_t max_write_zeroes_sectors;
bool x_enable_wce_if_config_wce;
--
2.24.3 (Apple Git-128)

Re: [PATCH v5 1/4] Jobs based on custom runners: documentation and configuration placeholder

[Thomas Huth]

On 19/02/2021 22.58, Cleber Rosa wrote:

As described in the included documentation, the "custom runner" jobs
extend the GitLab CI jobs already in place.  One of their primary
goals of catching and preventing regressions on a wider number of host
systems than the ones provided by GitLab's shared runners.

This sets the stage in which other community members can add their own
machine configuration documentation/scripts, and accompanying job
definitions.  As a general rule, those newly added contributed jobs
should run as "non-gating", until their reliability is verified (AKA
"allow_failure: true").

Signed-off-by: Cleber Rosa 
---
  .gitlab-ci.d/custom-runners.yml | 14 ++
  .gitlab-ci.yml  |  1 +
  docs/devel/ci.rst   | 28 
  docs/devel/index.rst|  1 +
  4 files changed, 44 insertions(+)
  create mode 100644 .gitlab-ci.d/custom-runners.yml
  create mode 100644 docs/devel/ci.rst

diff --git a/.gitlab-ci.d/custom-runners.yml b/.gitlab-ci.d/custom-runners.yml
new file mode 100644
index 00..3004da2bda
--- /dev/null
+++ b/.gitlab-ci.d/custom-runners.yml
@@ -0,0 +1,14 @@
+# The CI jobs defined here require GitLab runners installed and
+# registered on machines that match their operating system names,
+# versions and architectures.  This is in contrast to the other CI
+# jobs that are intended to run on GitLab's "shared" runners.
+
+# Different than the default approach on "shared" runners, based on
+# containers, the custom runners have no such *requirement*, as those
+# jobs should be capable of running on operating systems with no
+# compatible container implementation, or no support from
+# gitlab-runner.  To avoid problems that gitlab-runner can cause while
+# reusing the GIT repository, let's enable the recursive submodule
+# strategy.
+variables:
+  GIT_SUBMODULE_STRATEGY: recursive


Is it really necessary? I thought our configure script would take care of 
the submodules?


Apart from that:
Acked-by: Thomas Huth 

Re: [PATCH] docs: move CODING_STYLE into the developer documentation

[Philippe Mathieu-Daudé]

On 2/23/21 12:07 PM, Peter Maydell wrote:
> On Tue, 23 Feb 2021 at 10:02, Alex Bennée  wrote:
>>
>> There is no particular reason to keep this on it's own in the root of
>> the tree. Move it into the rest of the fine developer manual and fixup
>> any links to it. The only tweak I've made is to fix the code-block
>> annotations to mention the language C.
>>
>> Signed-off-by: Alex Bennée 
>> ---
>> diff --git a/README.rst b/README.rst
>> index ce39d89077..f5d41e59b1 100644
>> --- a/README.rst
>> +++ b/README.rst
>> @@ -66,7 +66,9 @@ When submitting patches, one common approach is to use 'git
>>  format-patch' and/or 'git send-email' to format & send the mail to the
>>  qemu-devel@nongnu.org mailing list. All patches submitted must contain
>>  a 'Signed-off-by' line from the author. Patches should follow the
>> -guidelines set out in the CODING_STYLE.rst file.
>> +guidelines set out in the `style section
>> +` of
>> +the Developers Guide.
> 
> This is the first instance of a qemu.readthedocs.io URL in the
> tree. Do we really want to have our references to our documentation
> be to a third party website ?

We can use https://www.qemu.org/docs/master/devel/style.html:

$ curl https://www.qemu.org/docs/master/devel/style.html


302 Found

Found
The document has moved https://qemu.readthedocs.io/en/latest/devel/style.html";>here.

Re: [PATCH] docs: move CODING_STYLE into the developer documentation

[Philippe Mathieu-Daudé]

On 2/23/21 12:29 PM, Philippe Mathieu-Daudé wrote:
> On 2/23/21 12:07 PM, Peter Maydell wrote:
>> On Tue, 23 Feb 2021 at 10:02, Alex Bennée  wrote:
>>>
>>> There is no particular reason to keep this on it's own in the root of
>>> the tree. Move it into the rest of the fine developer manual and fixup
>>> any links to it. The only tweak I've made is to fix the code-block
>>> annotations to mention the language C.
>>>
>>> Signed-off-by: Alex Bennée 
>>> ---
>>> diff --git a/README.rst b/README.rst
>>> index ce39d89077..f5d41e59b1 100644
>>> --- a/README.rst
>>> +++ b/README.rst
>>> @@ -66,7 +66,9 @@ When submitting patches, one common approach is to use 
>>> 'git
>>>  format-patch' and/or 'git send-email' to format & send the mail to the
>>>  qemu-devel@nongnu.org mailing list. All patches submitted must contain
>>>  a 'Signed-off-by' line from the author. Patches should follow the
>>> -guidelines set out in the CODING_STYLE.rst file.
>>> +guidelines set out in the `style section
>>> +` of
>>> +the Developers Guide.
>>
>> This is the first instance of a qemu.readthedocs.io URL in the
>> tree. Do we really want to have our references to our documentation
>> be to a third party website ?
> 
> We can use https://www.qemu.org/docs/master/devel/style.html:
> 
> $ curl https://www.qemu.org/docs/master/devel/style.html
> 
> 
> 302 Found
> 
> Found
> The document has moved  href="https://qemu.readthedocs.io/en/latest/devel/style.html";>here.
> 

Or even better since we have a job pushing to Gitlab pages
accessible on https://qemu-project.gitlab.io/qemu/:

https://qemu-project.gitlab.io/qemu/devel/style.html

Maybe the https://www.qemu.org/docs/ redirect should
go to gitlab page now?

Phil.

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Claudio Fontana]

On 2/23/21 12:01 PM, Alex Bennée wrote:
> 
> Claudio Fontana  writes:
> 
>> On 2/22/21 4:34 PM, Alex Bennée wrote:
>>>
>>> Claudio Fontana  writes:
>>>
 From: Claudio Fontana 

 KVM has its own cpu->kvm_vtime.

 Adjust cpu vmstate by putting unused fields instead of the
 VMSTATE_TIMER_PTR when TCG is not available.

 Signed-off-by: Claudio Fontana 
 ---
  target/arm/cpu.c | 4 +++-
  target/arm/machine.c | 5 +
  2 files changed, 8 insertions(+), 1 deletion(-)

 diff --git a/target/arm/cpu.c b/target/arm/cpu.c
 index 1d81a1e7ac..b929109054 100644
 --- a/target/arm/cpu.c
 +++ b/target/arm/cpu.c
 @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, 
 Error **errp)
  }
  }
  
 +#ifdef CONFIG_TCG
  {
  uint64_t scale;
  
 @@ -1347,7 +1348,8 @@ static void arm_cpu_realizefn(DeviceState *dev, 
 Error **errp)
  cpu->gt_timer[GTIMER_HYPVIRT] = timer_new(QEMU_CLOCK_VIRTUAL, 
 scale,
arm_gt_hvtimer_cb, cpu);
  }
 -#endif
 +#endif /* CONFIG_TCG */
 +#endif /* !CONFIG_USER_ONLY */
  
  cpu_exec_realizefn(cs, &local_err);
  if (local_err != NULL) {
 diff --git a/target/arm/machine.c b/target/arm/machine.c
 index 666ef329ef..13d7c6d930 100644
 --- a/target/arm/machine.c
 +++ b/target/arm/machine.c
 @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
  VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
  VMSTATE_UINT32(env.exception.fsr, ARMCPU),
  VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
 +#ifdef CONFIG_TCG
  VMSTATE_TIMER_PTR(gt_timer[GTIMER_PHYS], ARMCPU),
  VMSTATE_TIMER_PTR(gt_timer[GTIMER_VIRT], ARMCPU),
 +#else
 +VMSTATE_UNUSED(sizeof(QEMUTimer *)),
 +VMSTATE_UNUSED(sizeof(QEMUTimer *)),
 +#endif /* CONFIG_TCG */
>>>
>>> I'm not sure this is correct - VMSTATE_TIMER_PTR chases the links to
>>> just expose expired time but QEMUTimer has more in it than that. Paolo
>>
>>
>> I am not sure I follow can you state more precisely where the issue could be?
>>
>> it's not a VMSTATE_TIMER, it's a VMSTATE_TIMER_PTR,
>> it ends up in VMSTATE_POINTER where a single pointer is assigned;
> 
> Does it? I thought it ended up with the .expire_time (int64_t) which
> will be bigger than sizeof(QemuTimer *) on a 32 bit system.

Ok I understand what you mean. Lets see:

Looking at vmstate.h,

#define VMSTATE_TIMER_PTR(_f, _s) \
VMSTATE_TIMER_PTR_V(_f, _s, 0)

#define VMSTATE_TIMER_PTR_V(_f, _s, _v)   \
VMSTATE_POINTER(_f, _s, _v, vmstate_info_timer, QEMUTimer *)

#define VMSTATE_POINTER(_field, _state, _version, _info, _type) {\
.name   = (stringify(_field)),   \
.version_id = (_version),\
.info   = &(_info),  \
.size   = sizeof(_type), \
.flags  = VMS_SINGLE|VMS_POINTER,\
.offset = vmstate_offset_value(_state, _field, _type),   \
}

so here we get the vmstate field definition.

.size is fine, as it is sizeof(QEMUTimer *).

.info, is &vmstate_info_timer, migration/savevm.c:

const VMStateInfo vmstate_info_timer = {
.name = "timer",
.get  = get_timer,
.put  = put_timer,
};

void timer_put(QEMUFile *f, QEMUTimer *ts)
{
uint64_t expire_time;

expire_time = timer_expire_time_ns(ts);
qemu_put_be64(f, expire_time);
}

void timer_get(QEMUFile *f, QEMUTimer *ts)
{
uint64_t expire_time;

expire_time = qemu_get_be64(f);
if (expire_time != -1) {
timer_mod_ns(ts, expire_time);
} else {
timer_del(ts);
}
}

---

And the migration code does: (migration/vmstate.c):

int vmstate_save_state_v() {
  ...
  ret = field->info->put(f, curr_elem, size, field, vmdesc_loop);
  ...
}

which puts a BE64 in the QEMUFile *f (see timer_put above).

The load code in the same file does:

int vmstate_load_state() {
  ...
  ret = field->info->get(f, curr_elem, size, field);
  ...
}

which reads a BE64 from the QEMUFile *f (see timer_get above).

Would be "fine" from the field sizes perspective (the .size of the field type, 
and the value of the BE64),

but it's the calculations done in timer_get and timer_put which are scary, as 
they dereference the timer pointer.


Should we actually have a check for null pointer in vmstate.c?

We _do_ have one in vmstate_save_state_v and vmstate_load_state, but it is 
actually active only for VMS_ARRAY_OF_POINTER.
Why? Why not also do the same (write the null pointer and not following it) for 
normal VMS_POINTER ?

int vmstate_save_state_v() {
 ...
if (!curr_elem &

Re: who's using the ozlabs patchwork install for QEMU patches ?

[Greg Kurz]

On Tue, 23 Feb 2021 11:09:05 +
Peter Maydell  wrote:

> On Mon, 22 Feb 2021 at 14:43, Greg Kurz  wrote:
> > My understanding is that users must be "maintainer" to edit other's
> > patches. Only three 'maintainers' are currently listed at ozlabs for
> > QEMU:
> >
> > https://patchwork.ozlabs.org/api/1.0/projects/14/
> >
> > We had a discussion about that a few months back with Christian Schoenebeck
> > (9pfs maintainer, Cc'd) who also uses patchworks. It turned out we didn't
> > quite know how to go further because of lack of documentation, but I'd be
> > glad to experiment the full patchwork experience if someone knows how to
> > do it :-)
> 
> If people want to try that kind of thing out I'm happy to try
> to tweak their permissions on the patchwork instance.
> 

Please do for me then. My name is groug :)

Cheers,

--
Greg

> -- PMM

Re: QEMU hosting

[Philippe Mathieu-Daudé]

Updating this thread for the list, I have been told that
Cleber is looking at all these options for mainstream CI,
and is already in contact with fosshost.org.

I'll let him follow these ideas.

Regards,

Phil.

On 2/22/21 5:11 PM, Philippe Mathieu-Daudé wrote:
> On 2/22/21 5:07 PM, Philippe Mathieu-Daudé wrote:
>> On 2/22/21 4:56 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Thomas and Stefan,
>>>
>>> On 1/5/21 7:55 PM, Fosshost wrote:
 Hi Stefan

 Thank you for your email.

  1. We do not offer MacOS hosting
  2. We can provide virtual machines with full KVM virt on x86
 architecture and soon arm64 v8
> 
> And from https://fosshost.org/about:
> "We support most operating systems including CentOS, Debian, Ubuntu,
> ArchLinux and FreeBSD and custom OS."
> 
> Eventually we could add a pair of FreeBSD runners to our Gitlab CI?
> https://docs.gitlab.com/runner/install/freebsd.html
>
  3. We do not provide dedicated servers.
>>>
>>> Would it be possible to have a dedicated VM for a git LFS server [*]?
>>
>> Just noticed the Mirrors-as-a-Service option "(available for package
>> mirrors, operating systems, repositories, documentation, static assets,
>> etc)":
>>
>> https://docs.fosshost.org/en/home/getting-started#production-services
>> https://docs.fosshost.org/en/home/mirrors-as-a-service
>>
>>> If so, what storage is usable? Are there network traffic limits?
>>>
>>> Thanks,
>>>
>>> Phil.
>>>
>>> [*] https://docs.gitlab.com/ee/topics/git/lfs/
>>>
 
 *From:* Stefan Hajnoczi 
 *Sent:* 05 January 2021 14:21
 *To:* Fosshost 
 *Cc:* qemu-devel 
 *Subject:* QEMU hosting
  
 Hi Thomas,
 In November you emailed qemu-devel asking if the QEMU project was
 interested in exploring hosting with Fosshost.org. I think my reply
 may have gotten lost so I wanted to check if you have time to discuss
 this again.

 The main hosting need that QEMU has is for continuous integration
 system runners. We are particularly interested in non-x86/non-Linux
 build machines and a dedicated server for reproducible performance
 tests. Just today there was discussion on #qemu IRC about how to go
 about adding a macOS build machine, for example.

 It would be great to find out more about Fosshost.org and whether we
 can work together.

 Thanks,
 Stefan
>>>
>>
> 

Re: vhost reply_ack negotiation (a.k.a differences in vhost-user behaviour with libvhost-user and vhost-user-backend.rs)

[Michael S. Tsirkin]

Cc: Raphael

On Fri, Feb 19, 2021 at 04:04:34PM +, Alex Bennée wrote:
> Hi,
> 
> I finally got a chance to get down into the guts of vhost-user while
> attempting to port my original C RPMB daemon to Rust using the
> vhost-user-backend and related crates. I ended up with this hang during
> negotiation:
> 
>   startup
> 
>   vhost_user_write req:1 flags:0x1
>   vhost_user_read_start
>   vhost_user_read req:1 flags:0x5
>   vhost_user_backend_init: we got 17000
>   vhost_user_write req:15 flags:0x1
>   vhost_user_read_start
>   vhost_user_read req:15 flags:0x5
>   vhost_user_set_protocol_features: 2008
>   vhost_user_write req:16 flags:0x1
>   vhost_user_write req:3 flags:0x1
>   vhost_user_write req:1 flags:0x1
>   vhost_user_read_start
>   vhost_user_read req:1 flags:0x5
>   vhost_user_write req:13 flags:0x1
> 
>   kernel initialises device
> 
>   virtio_rpmb virtio1: init done!
>   vhost_user_write req:13 flags:0x1
>   vhost_dev_set_features: 13000
>   vhost_user_set_features: 13000
>   vhost_user_write req:2 flags:0x1
>   vhost_user_write req:5 flags:0x9
>   vhost_user_read_start
> 
> The proximate cause is the vhost crate handling:
> 
>   MasterReq::SET_MEM_TABLE => {
>   let res = self.set_mem_table(&hdr, size, &buf, rfds);
>   self.send_ack_message(&hdr, res)?;
>   }
> 
> which gates on the replay_ack_enabled flag:
> 
> fn send_ack_message(
> &mut self,
> req: &VhostUserMsgHeader,
> res: Result<()>,
> ) -> Result<()> {
> if dbg!(self.reply_ack_enabled) {
> let hdr = self.new_reply_header::(req, 0)?;
> let val = match res {
> Ok(_) => 0,
> Err(_) => 1,
> };
> let msg = VhostUserU64::new(val);
> self.main_sock.send_message(&hdr, &msg, None)?;
> }
> Ok(())
> }
> 
> which is only set when we have all the appropriate acknowledged flags:
> 
> fn update_reply_ack_flag(&mut self) {
> let vflag = VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits();
> let pflag = VhostUserProtocolFeatures::REPLY_ACK;
> if (self.virtio_features & vflag) != 0
> && (self.acked_virtio_features & vflag) != 0
> && self.protocol_features.contains(pflag)
> && (self.acked_protocol_features & pflag.bits()) != 0
> {
> self.reply_ack_enabled = true;
> } else {
> self.reply_ack_enabled = false;
> }
> }
> 
> which from above you can see QEMU helpfully dropped those bits in the
> reply. It does however work in the C/libvhost version:
> 
>   virtio_rpmb virtio1: init done!
>   vhost_user_write req:13 flags:0x1
>   vhost_dev_set_features: 13000
>   vhost_user_set_features: 13000
>   vhost_user_write req:2 flags:0x1
>   vhost_user_write req:37 flags:0x9
>   vhost_user_read_start
>   vhost_user_read req:37 flags:0x5
>   vhost_user_write req:8 flags:0x1
>   vhost_user_write req:10 flags:0x1
>   vhost_user_write req:9 flags:0x1
>   vhost_user_write req:12 flags:0x1
>   vhost_user_write req:13 flags:0x1
> 
> albeit with a slightly different message sequence
> (VHOST_USER_ADD_MEM_REG instead of VHOST_USER_SET_MEM_TABLE). Reading
> the C code you can see why:
> 
> need_reply = vmsg.flags & VHOST_USER_NEED_REPLY_MASK;
> 
> reply_requested = vu_process_message(dev, &vmsg);
> if (!reply_requested && need_reply) {
> vmsg_set_reply_u64(&vmsg, 0);
> reply_requested = 1;
> }
> 
> So regardless of what may have been negotiated it will always reply with
> something if the master requested it do so. This points us at the
> specification which reads:
> 
>   - Bit 3 is the need_reply flag - see :ref:`REPLY_ACK ` for
> details.
> 
> which says in VHOST_USER_PROTOCOL_F_REPLY_ACK that this bit should only
> be honoured when the feature has been negotiated. Which brings us to a
> series of questions:
> 
>  - Should QEMU have preserved VhostUserVirtioFeatures::PROTOCOL_FEATURES
>when doing the eventual VHOST_USER_SET_FEATURES reply?

Hmm looks like a bug indeed ... Anyone wants to look
into fixing that? Marc-André?


>  - Is vhost.rs being to strict or libvhost-user too lax in interpreting
>the negotiated features before processing the ``need_reply`` [Bit 3]
>field of the messages?
> 
>  - are VHOST_USER_SET_MEM_TABLE to VHOST_USER_SET_INFLIGHT_FD included
>in the "list of the ones that do" require replies or do they only
>reply when REPLY_ACK has been negotiated as the ambiguous "seealso::"
>box out seems to imply?
> 
> Currently I have some hacks in:
> 
>   https://github.com/stsquad/vhost/tree/my-hacks
> 
> which gets my daemon booting up to the point we actually need to do a
> transaction. However I won't submit a PR until I've worked out exactly
> where the problems are.
> 
> -- 
> Alex Bennée

Re: who's using the ozlabs patchwork install for QEMU patches ?

[Peter Maydell]

On Tue, 23 Feb 2021 at 11:39, Greg Kurz  wrote:
>
> On Tue, 23 Feb 2021 11:09:05 +
> Peter Maydell  wrote:
>
> > On Mon, 22 Feb 2021 at 14:43, Greg Kurz  wrote:
> > > My understanding is that users must be "maintainer" to edit other's
> > > patches. Only three 'maintainers' are currently listed at ozlabs for
> > > QEMU:
> > >
> > > https://patchwork.ozlabs.org/api/1.0/projects/14/
> > >
> > > We had a discussion about that a few months back with Christian 
> > > Schoenebeck
> > > (9pfs maintainer, Cc'd) who also uses patchworks. It turned out we didn't
> > > quite know how to go further because of lack of documentation, but I'd be
> > > glad to experiment the full patchwork experience if someone knows how to
> > > do it :-)
> >
> > If people want to try that kind of thing out I'm happy to try
> > to tweak their permissions on the patchwork instance.
> >
>
> Please do for me then. My name is groug :)

Hmm. Having looked through the UI and docs I can't figure
out how to do that (or even if 'maintainer' permission is
sufficient to add other maintainers; maybe one has to ask the
patchwork instance admins to do that?). If you can tell me what
I need to do to add you to the maintainer list for QEMU I'll do it :-)

-- PMM

Re: [PATCH v6 01/11] hvf: Add hypervisor entitlement to output binaries

[Akihiko Odaki]

Hi,

I use your patches when running QEMU on M1 MacBook Air.

I noticed that the installation process corrupts the code signature 
because meson modifies the file to fix dynamic shared library install 
names. Also, stripping apparently does not work because the signed file 
is not considered as "executable" by meson. Here is some change I wrote 
for my own use, just for reference:

https://github.com/akihikodaki/qemu/commit/6a9b5d7e4ea03b1e757be1eedf256871bb6a5bdd

Also, the patch series do no longer apply to master. Here is my merge 
with conflict resolution (It is not a rebase and was done for my own 
purpose. Just for reference.):

https://github.com/akihikodaki/qemu/commit/b7885e4370a2fe426e80d32afe6eb5d01a71640d

Regards,
Akihiko Odaki

On 2021/01/21 7:44, Alexander Graf wrote:

In macOS 11, QEMU only gets access to Hypervisor.framework if it has the
respective entitlement. Add an entitlement template and automatically self
sign and apply the entitlement in the build.

Signed-off-by: Alexander Graf 
Reviewed-by: Roman Bolshakov 
Tested-by: Roman Bolshakov 

---

v1 -> v2:

   - Make safe to ctrl-C

v3 -> v4:

   - Remove unused exe_full variable
   - Reuse exe_name variable
---
  accel/hvf/entitlements.plist |  8 
  meson.build  | 29 +
  scripts/entitlement.sh   | 13 +
  3 files changed, 46 insertions(+), 4 deletions(-)
  create mode 100644 accel/hvf/entitlements.plist
  create mode 100755 scripts/entitlement.sh

diff --git a/accel/hvf/entitlements.plist b/accel/hvf/entitlements.plist
new file mode 100644
index 00..154f3308ef
--- /dev/null
+++ b/accel/hvf/entitlements.plist
@@ -0,0 +1,8 @@
+
+http://www.apple.com/DTDs/PropertyList-1.0.dtd";>
+
+
+com.apple.security.hypervisor
+
+
+
diff --git a/meson.build b/meson.build
index 3d889857a0..c667d64498 100644
--- a/meson.build
+++ b/meson.build
@@ -2146,9 +2146,14 @@ foreach target : target_dirs
  }]
endif
foreach exe: execs
-emulators += {exe['name']:
- executable(exe['name'], exe['sources'],
-   install: true,
+exe_name = exe['name']
+exe_sign = 'CONFIG_HVF' in config_target
+if exe_sign
+  exe_name += '-unsigned'
+endif
+
+emulator = executable(exe_name, exe['sources'],
+   install: not exe_sign,
 c_args: c_args,
 dependencies: arch_deps + deps + exe['dependencies'],
 objects: lib.extract_all_objects(recursive: true),
@@ -2156,7 +2161,23 @@ foreach target : target_dirs
 link_depends: [block_syms, qemu_syms] + 
exe.get('link_depends', []),
 link_args: link_args,
 gui_app: exe['gui'])
-}
+
+if exe_sign
+  emulators += {exe['name'] : custom_target(exe['name'],
+   install: true,
+   install_dir: get_option('bindir'),
+   depends: emulator,
+   output: exe['name'],
+   command: [
+ meson.current_source_dir() / 'scripts/entitlement.sh',
+ meson.current_build_dir() / exe_name,
+ meson.current_build_dir() / exe['name'],
+ meson.current_source_dir() / 
'accel/hvf/entitlements.plist'
+   ])
+  }
+else
+  emulators += {exe['name']: emulator}
+endif
  
  if 'CONFIG_TRACE_SYSTEMTAP' in config_host

foreach stp: [
diff --git a/scripts/entitlement.sh b/scripts/entitlement.sh
new file mode 100755
index 00..c540fa6435
--- /dev/null
+++ b/scripts/entitlement.sh
@@ -0,0 +1,13 @@
+#!/bin/sh -e
+#
+# Helper script for the build process to apply entitlements
+
+SRC="$1"
+DST="$2"
+ENTITLEMENT="$3"
+
+trap 'rm "$DST.tmp"' exit
+cp -af "$SRC" "$DST.tmp"
+codesign --entitlements "$ENTITLEMENT" --force -s - "$DST.tmp"
+mv "$DST.tmp" "$DST"
+trap '' exit

Re: [PATCH] docs: move CODING_STYLE into the developer documentation

[Alex Bennée]

Peter Maydell  writes:

> On Tue, 23 Feb 2021 at 10:02, Alex Bennée  wrote:
>>
>> There is no particular reason to keep this on it's own in the root of
>> the tree. Move it into the rest of the fine developer manual and fixup
>> any links to it. The only tweak I've made is to fix the code-block
>> annotations to mention the language C.
>>
>> Signed-off-by: Alex Bennée 
>> ---
>> diff --git a/README.rst b/README.rst
>> index ce39d89077..f5d41e59b1 100644
>> --- a/README.rst
>> +++ b/README.rst
>> @@ -66,7 +66,9 @@ When submitting patches, one common approach is to use 'git
>>  format-patch' and/or 'git send-email' to format & send the mail to the
>>  qemu-devel@nongnu.org mailing list. All patches submitted must contain
>>  a 'Signed-off-by' line from the author. Patches should follow the
>> -guidelines set out in the CODING_STYLE.rst file.
>> +guidelines set out in the `style section
>> +` of
>> +the Developers Guide.
>
> This is the first instance of a qemu.readthedocs.io URL in the
> tree. Do we really want to have our references to our documentation
> be to a third party website ?

Well I browsed to:

  https://www.qemu.org/docs/master/

which re-directed me to the readthedocs URL. However it looks like:

  https://www.qemu.org/docs/master/devel/index.html

DTRT so I can fix that.

>
> thanks
> -- PMM


-- 
Alex Bennée

[PATCH v3] virtio-blk: Respect discard granularity

[Akihiko Odaki]

Signed-off-by: Akihiko Odaki 
---
 hw/block/virtio-blk.c  | 8 +++-
 hw/core/machine.c  | 4 +++-
 include/hw/virtio/virtio-blk.h | 1 +
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index bac2d6fa2b2..f4378e61182 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -962,10 +962,14 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
 blkcfg.wce = blk_enable_write_cache(s->blk);
 virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
 if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_DISCARD)) {
+uint32_t discard_granularity = conf->discard_granularity;
+if (discard_granularity == -1 || !s->conf.report_discard_granularity) {
+discard_granularity = blk_size;
+}
 virtio_stl_p(vdev, &blkcfg.max_discard_sectors,
  s->conf.max_discard_sectors);
 virtio_stl_p(vdev, &blkcfg.discard_sector_alignment,
- blk_size >> BDRV_SECTOR_BITS);
+ discard_granularity >> BDRV_SECTOR_BITS);
 /*
  * We support only one segment per request since multiple segments
  * are not widely used and there are no userspace APIs that allow
@@ -1299,6 +1303,8 @@ static Property virtio_blk_properties[] = {
  IOThread *),
 DEFINE_PROP_BIT64("discard", VirtIOBlock, host_features,
   VIRTIO_BLK_F_DISCARD, true),
+DEFINE_PROP_BOOL("report-discard-granularity", VirtIOBlock,
+ conf.report_discard_granularity, true),
 DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
   VIRTIO_BLK_F_WRITE_ZEROES, true),
 DEFINE_PROP_UINT32("max-discard-sectors", VirtIOBlock,
diff --git a/hw/core/machine.c b/hw/core/machine.c
index de3b8f1b318..e4df5797e72 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -33,7 +33,9 @@
 #include "migration/global_state.h"
 #include "migration/vmstate.h"
 
-GlobalProperty hw_compat_5_2[] = {};
+GlobalProperty hw_compat_5_2[] = {
+{ "virtio-blk-device", "report-discard-granularity", "off" },
+};
 const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
 
 GlobalProperty hw_compat_5_1[] = {
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 214ab748229..29655a406dd 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -41,6 +41,7 @@ struct VirtIOBlkConf
 uint16_t num_queues;
 uint16_t queue_size;
 bool seg_max_adjust;
+bool report_discard_granularity;
 uint32_t max_discard_sectors;
 uint32_t max_write_zeroes_sectors;
 bool x_enable_wce_if_config_wce;
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v2 1/1] hw/s390x: modularize virtio-gpu-ccw

[Halil Pasic]

On Mon, 22 Feb 2021 17:30:50 +
Daniel P. Berrangé  wrote:

> > Paolo, Daniel,
> > I am in general (s390 unrelated) a bit puzzled about the scenario of QEMU
> > being modularized.
> > Libvirt probes QEMU executables for their capabilities and creates a
> > capabilities cache of the probed QEMU binary. There are a few triggers that
> > invalidate the cache. One is the QEMU binary changing.
> > Is there one for QEMU modules being installed or uninstalled?
> > How is that supposed to work?  
> 
> Libvirt doesn't check the modules specifically, but it does look at the
> mtime on the directory containing modules, and that should be touched
> when a moduled is added/removed.  This is since libvirt 6.8.0 or later.

It seems we are covered, at least upstream.
,
Regards,
Halil

Re: [PATCH qemu v13] spapr: Implement Open Firmware client interface

[Alexey Kardashevskiy]

On 23/02/2021 14:07, David Gibson wrote:

On Tue, Feb 09, 2021 at 10:02:52PM +1100, Alexey Kardashevskiy wrote:

The PAPR platform which describes an OS environment that's presented by
a combination of a hypervisor and firmware. The features it specifies
require collaboration between the firmware and the hypervisor.



[...]


+target_ulong spapr_h_vof_client(PowerPCCPU *cpu, SpaprMachineState *spapr,
+target_ulong opcode, target_ulong *args)
+{
+target_ulong of_client_args = ppc64_phys_to_real(args[0]);
+struct prom_args pargs = { 0 };
+char service[64];
+unsigned nargs, nret, i;
+
+cpu_physical_memory_read(of_client_args, &pargs, sizeof(pargs));


Need to check for read errors in case an out of bounds address is passed.



cpu_physical_memory_read() returns void and so does 
cpu_physical_memory_rw() but eventually called address_space_rw() 
returns an error code, should I switch to it?






+nargs = be32_to_cpu(pargs.nargs);
+if (nargs >= ARRAY_SIZE(pargs.args)) {
+return H_PARAMETER;
+}
+
+cpu_physical_memory_read(be32_to_cpu(pargs.service), service,
+ sizeof(service));
+if (strnlen(service, sizeof(service)) == sizeof(service)) {
+/* Too long service name */
+return H_PARAMETER;
+}
+
+for (i = 0; i < nargs; ++i) {
+pargs.args[i] = be32_to_cpu(pargs.args[i]);


In general I dislike in-place endian conversion of structs, since I
think it's less confusing to think of the endianness as a property of
the type.


The type is uint32_t and there is no be32 in QEMU. I can have 2 copies 
of pargs if this makes reviewing easier, should I?





--
Alexey

Re: who's using the ozlabs patchwork install for QEMU patches ?

[Christian Schoenebeck]

On Dienstag, 23. Februar 2021 12:54:47 CET Peter Maydell wrote:
> On Tue, 23 Feb 2021 at 11:39, Greg Kurz  wrote:
> > On Tue, 23 Feb 2021 11:09:05 +
> > 
> > Peter Maydell  wrote:
> > > On Mon, 22 Feb 2021 at 14:43, Greg Kurz  wrote:
> > > > My understanding is that users must be "maintainer" to edit other's
> > > > patches. Only three 'maintainers' are currently listed at ozlabs for
> > > > QEMU:
> > > > 
> > > > https://patchwork.ozlabs.org/api/1.0/projects/14/
> > > > 
> > > > We had a discussion about that a few months back with Christian
> > > > Schoenebeck
> > > > (9pfs maintainer, Cc'd) who also uses patchworks. It turned out we
> > > > didn't
> > > > quite know how to go further because of lack of documentation, but I'd
> > > > be
> > > > glad to experiment the full patchwork experience if someone knows how
> > > > to
> > > > do it :-)
> > > 
> > > If people want to try that kind of thing out I'm happy to try
> > > to tweak their permissions on the patchwork instance.
> > 
> > Please do for me then. My name is groug :)
> 
> Hmm. Having looked through the UI and docs I can't figure
> out how to do that (or even if 'maintainer' permission is
> sufficient to add other maintainers; maybe one has to ask the
> patchwork instance admins to do that?). If you can tell me what
> I need to do to add you to the maintainer list for QEMU I'll do it :-)
> 
> -- PMM

We were looking into this last year, and from my (poor) understanding this is 
how it works:

https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg02864.html

If somebody knows a more convenient/appropriate way, that would be 
appreciated.

Best regards,
Christian Schoenebeck

Re: A brief look at deprecating our JSON extensions over RFC 8259

[Markus Armbruster]

Paolo Bonzini  writes:

> On 23/02/21 10:06, Markus Armbruster wrote:
>>> Markus, did you rebuild the qtests after disabling single-quoted
>>> strings?  "make check-qtest-x86_64" would have rebuilt them, but I'm
>>> confused by the results.
>> I ran "make check" and looked at the failures:
>> Still confused?
>
> Yes.  What's the patch that you used to disable the single quotes, and
> why didn't the patched parser choke on
>
> response = qmp("{ 'execute': 'qom-get', 'arguments': { 'path': %s, "
>"'property': 'temperature' } }", id);
>
> ?

My bad!  I neglected to mention that I tied "disable single-quoted
strings" to "interpolation is off" for my experiment.  This is a lazy,
half-assed approximation of "internal interface".

Here's the experimental patch.


commit 57138b9d4188dd8ce1814237fe53f7131bbb3f45
Author: Markus Armbruster 
Date:   Mon Feb 22 17:04:10 2021 +0100

qobject: Tie single quote extension to interpolation WIP

This makes several tests fail or hang.  Some are hacked up.

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 008b326fb8..c1ddc65d96 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -150,9 +150,6 @@ static QString *parse_string(JSONParserContext *ctxt, 
JSONToken *token)
 case '"':
 g_string_append_c(str, '"');
 break;
-case '\'':
-g_string_append_c(str, '\'');
-break;
 case '\\':
 g_string_append_c(str, '\\');
 break;
@@ -201,6 +198,12 @@ static QString *parse_string(JSONParserContext *ctxt, 
JSONToken *token)
 }
 g_string_append(str, utf8_buf);
 break;
+case '\'':
+if (ctxt->ap) {
+g_string_append_c(str, '\'');
+break;
+}
+/* fall through */
 default:
 parse_error(ctxt, token, "invalid escape sequence in string");
 goto out;
diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index b93d97b995..3d4d3b484e 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -49,6 +49,11 @@ void json_message_process_token(JSONLexer *lexer, GString 
*input,
 case JSON_RSQUARE:
 parser->bracket_count--;
 break;
+case JSON_STRING:
+if (input->str[0] == '\"' || parser->ap) {
+break;
+}
+/* fall through */
 case JSON_ERROR:
 error_setg(&err, "JSON parse error, stray '%s'", input->str);
 goto out_emit;

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Claudio Fontana]

On 2/23/21 12:36 PM, Claudio Fontana wrote:
> On 2/23/21 12:01 PM, Alex Bennée wrote:
>>
>> Claudio Fontana  writes:
>>
>>> On 2/22/21 4:34 PM, Alex Bennée wrote:

 Claudio Fontana  writes:

> From: Claudio Fontana 
>
> KVM has its own cpu->kvm_vtime.
>
> Adjust cpu vmstate by putting unused fields instead of the
> VMSTATE_TIMER_PTR when TCG is not available.
>
> Signed-off-by: Claudio Fontana 
> ---
>  target/arm/cpu.c | 4 +++-
>  target/arm/machine.c | 5 +
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 1d81a1e7ac..b929109054 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, 
> Error **errp)
>  }
>  }
>  
> +#ifdef CONFIG_TCG
>  {
>  uint64_t scale;
>  
> @@ -1347,7 +1348,8 @@ static void arm_cpu_realizefn(DeviceState *dev, 
> Error **errp)
>  cpu->gt_timer[GTIMER_HYPVIRT] = timer_new(QEMU_CLOCK_VIRTUAL, 
> scale,
>arm_gt_hvtimer_cb, 
> cpu);
>  }
> -#endif
> +#endif /* CONFIG_TCG */
> +#endif /* !CONFIG_USER_ONLY */
>  
>  cpu_exec_realizefn(cs, &local_err);
>  if (local_err != NULL) {
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 666ef329ef..13d7c6d930 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>  VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>  VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>  VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
> +#ifdef CONFIG_TCG
>  VMSTATE_TIMER_PTR(gt_timer[GTIMER_PHYS], ARMCPU),
>  VMSTATE_TIMER_PTR(gt_timer[GTIMER_VIRT], ARMCPU),
> +#else
> +VMSTATE_UNUSED(sizeof(QEMUTimer *)),
> +VMSTATE_UNUSED(sizeof(QEMUTimer *)),
> +#endif /* CONFIG_TCG */

 I'm not sure this is correct - VMSTATE_TIMER_PTR chases the links to
 just expose expired time but QEMUTimer has more in it than that. Paolo
>>>
>>>
>>> I am not sure I follow can you state more precisely where the issue could 
>>> be?
>>>
>>> it's not a VMSTATE_TIMER, it's a VMSTATE_TIMER_PTR,
>>> it ends up in VMSTATE_POINTER where a single pointer is assigned;
>>
>> Does it? I thought it ended up with the .expire_time (int64_t) which
>> will be bigger than sizeof(QemuTimer *) on a 32 bit system.
> 
> Ok I understand what you mean. Lets see:
> 
> Looking at vmstate.h,
> 
> #define VMSTATE_TIMER_PTR(_f, _s) \
> VMSTATE_TIMER_PTR_V(_f, _s, 0)
> 
> #define VMSTATE_TIMER_PTR_V(_f, _s, _v)   \
> VMSTATE_POINTER(_f, _s, _v, vmstate_info_timer, QEMUTimer *)
> 
> #define VMSTATE_POINTER(_field, _state, _version, _info, _type) {\
> .name   = (stringify(_field)),   \
> .version_id = (_version),\
> .info   = &(_info),  \
> .size   = sizeof(_type), \
> .flags  = VMS_SINGLE|VMS_POINTER,\
> .offset = vmstate_offset_value(_state, _field, _type),   \
> }
> 
> so here we get the vmstate field definition.
> 
> .size is fine, as it is sizeof(QEMUTimer *).
> 
> .info, is &vmstate_info_timer, migration/savevm.c:
> 
> const VMStateInfo vmstate_info_timer = {
> .name = "timer",
> .get  = get_timer,
> .put  = put_timer,
> };
> 
> void timer_put(QEMUFile *f, QEMUTimer *ts)
> {
> uint64_t expire_time;
> 
> expire_time = timer_expire_time_ns(ts);
> qemu_put_be64(f, expire_time);
> }
> 
> void timer_get(QEMUFile *f, QEMUTimer *ts)
> {
> uint64_t expire_time;
> 
> expire_time = qemu_get_be64(f);
> if (expire_time != -1) {
> timer_mod_ns(ts, expire_time);
> } else {
> timer_del(ts);
> }
> }
> 
> ---
> 
> And the migration code does: (migration/vmstate.c):
> 
> int vmstate_save_state_v() {
>   ...
>   ret = field->info->put(f, curr_elem, size, field, vmdesc_loop);
>   ...
> }
> 
> which puts a BE64 in the QEMUFile *f (see timer_put above).
> 
> The load code in the same file does:
> 
> int vmstate_load_state() {
>   ...
>   ret = field->info->get(f, curr_elem, size, field);
>   ...
> }
> 
> which reads a BE64 from the QEMUFile *f (see timer_get above).
> 
> Would be "fine" from the field sizes perspective (the .size of the field 
> type, and the value of the BE64),
> 
> but it's the calculations done in timer_get and timer_put which are scary, as 
> they dereference the timer pointer.
> 
> 
> Should we actually have a check for null pointer in vmstate.c?
> 

Re: who's using the ozlabs patchwork install for QEMU patches ?

[Alexey Kardashevskiy]

On 23/02/2021 23:24, Christian Schoenebeck wrote:

On Dienstag, 23. Februar 2021 12:54:47 CET Peter Maydell wrote:

On Tue, 23 Feb 2021 at 11:39, Greg Kurz  wrote:

On Tue, 23 Feb 2021 11:09:05 +

Peter Maydell  wrote:

On Mon, 22 Feb 2021 at 14:43, Greg Kurz  wrote:

My understanding is that users must be "maintainer" to edit other's
patches. Only three 'maintainers' are currently listed at ozlabs for
QEMU:

https://patchwork.ozlabs.org/api/1.0/projects/14/

We had a discussion about that a few months back with Christian
Schoenebeck
(9pfs maintainer, Cc'd) who also uses patchworks. It turned out we
didn't
quite know how to go further because of lack of documentation, but I'd
be
glad to experiment the full patchwork experience if someone knows how
to
do it :-)


If people want to try that kind of thing out I'm happy to try
to tweak their permissions on the patchwork instance.


Please do for me then. My name is groug :)


Hmm. Having looked through the UI and docs I can't figure
out how to do that (or even if 'maintainer' permission is
sufficient to add other maintainers; maybe one has to ask the
patchwork instance admins to do that?). If you can tell me what
I need to do to add you to the maintainer list for QEMU I'll do it :-)

-- PMM


We were looking into this last year, and from my (poor) understanding this is
how it works:

https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg02864.html

If somebody knows a more convenient/appropriate way, that would be
appreciated.



Jeremy or Stephen (cc-ing) do definitely know if there is a better way.


--
Alexey

Re: A brief look at deprecating our JSON extensions over RFC 8259

[Markus Armbruster]

Peter Maydell  writes:

> On Tue, 23 Feb 2021 at 09:33, Markus Armbruster  wrote:
>> Misunderstanding: our JSON interpolation feature is *not* string
>> interpolation!  It interpolates *objects* into the QObject built by the
>> parser.
>
> Given that it's basically undocumented except in a scattered
> handful of comments inside the qjson parser implementation, it's
> not too surprising that people misunderstand it :-)

Yes, that's fair.

I added a fair amount of commentary, but it's heavily geared towards
maintainers, not users.

> (One surprising
> feature: the parser takes ownership of the object that you pass it
> via the '%p' interpolation, and will qobject_unref() it.)

Yes, %p takes over the reference.

[PATCH] ui/cocoa: Replace fprintf with error_report

[Akihiko Odaki]

Signed-off-by: Akihiko Odaki 
---
 ui/cocoa.m | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 0ef5fdf3b7a..900bc984733 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -270,7 +270,7 @@ static bool bool_with_iothread_lock(BoolCodeBlock block)
 static int cocoa_keycode_to_qemu(int keycode)
 {
 if (ARRAY_SIZE(mac_to_qkeycode_map) <= keycode) {
-fprintf(stderr, "(cocoa) warning unknown keycode 0x%x\n", keycode);
+error_report("(cocoa) warning unknown keycode 0x%x", keycode);
 return 0;
 }
 return mac_to_qkeycode_map[keycode];
@@ -1071,7 +1071,7 @@ - (id) init
 // create a view and add it to the window
 cocoaView = [[QemuCocoaView alloc] initWithFrame:NSMakeRect(0.0, 0.0, 
640.0, 480.0)];
 if(!cocoaView) {
-fprintf(stderr, "(cocoa) can't create a view\n");
+error_report("(cocoa) can't create a view");
 exit(1);
 }
 
@@ -1080,7 +1080,7 @@ - (id) init
 
styleMask:NSWindowStyleMaskTitled|NSWindowStyleMaskMiniaturizable|NSWindowStyleMaskClosable
 backing:NSBackingStoreBuffered defer:NO];
 if(!normalWindow) {
-fprintf(stderr, "(cocoa) can't create window\n");
+error_report("(cocoa) can't create window");
 exit(1);
 }
 [normalWindow setAcceptsMouseMovedEvents:YES];
-- 
2.24.3 (Apple Git-128)

[PATCH] blockjob: report a better error message

[Stefano Garzarella]

When a block job fails, we report 'strerror(-job->job.ret)' error
message, also if the job set an error object.
Let's report a better error message using 'error_get_pretty(job->job.err)'.

If an error object was not set, strerror(-job->ret) is used as fallback,
as explained in include/qemu/job.h:

typedef struct Job {
...
/**
 * Error object for a failed job.
 * If job->ret is nonzero and an error object was not set, it will be set
 * to strerror(-job->ret) during job_completed.
 */
Error *err;
}

Suggested-by: Kevin Wolf 
Signed-off-by: Stefano Garzarella 
---
 blockjob.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index f2feff051d..a696f3408d 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -319,7 +319,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
 info->auto_finalize = job->job.auto_finalize;
 info->auto_dismiss  = job->job.auto_dismiss;
 info->has_error = job->job.ret != 0;
-info->error = job->job.ret ? g_strdup(strerror(-job->job.ret)) : NULL;
+info->error = job->job.ret ?
+g_strdup(error_get_pretty(job->job.err)) : NULL;
 return info;
 }
 
@@ -356,7 +357,7 @@ static void block_job_event_completed(Notifier *n, void 
*opaque)
 }
 
 if (job->job.ret < 0) {
-msg = strerror(-job->job.ret);
+msg = error_get_pretty(job->job.err);
 }
 
 qapi_event_send_block_job_completed(job_type(&job->job),
-- 
2.29.2

Re: [PATCH] ui/cocoa: Replace fprintf with error_report

[Philippe Mathieu-Daudé]

On 2/23/21 2:11 PM, Akihiko Odaki wrote:
> Signed-off-by: Akihiko Odaki 
> ---
>  ui/cocoa.m | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 0ef5fdf3b7a..900bc984733 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -270,7 +270,7 @@ static bool bool_with_iothread_lock(BoolCodeBlock block)
>  static int cocoa_keycode_to_qemu(int keycode)
>  {
>  if (ARRAY_SIZE(mac_to_qkeycode_map) <= keycode) {
> -fprintf(stderr, "(cocoa) warning unknown keycode 0x%x\n", keycode);
> +error_report("(cocoa) warning unknown keycode 0x%x", keycode);
>  return 0;
>  }
>  return mac_to_qkeycode_map[keycode];
> @@ -1071,7 +1071,7 @@ - (id) init
>  // create a view and add it to the window
>  cocoaView = [[QemuCocoaView alloc] initWithFrame:NSMakeRect(0.0, 
> 0.0, 640.0, 480.0)];
>  if(!cocoaView) {
> -fprintf(stderr, "(cocoa) can't create a view\n");
> +error_report("(cocoa) can't create a view");
>  exit(1);
>  }
>  
> @@ -1080,7 +1080,7 @@ - (id) init
>  
> styleMask:NSWindowStyleMaskTitled|NSWindowStyleMaskMiniaturizable|NSWindowStyleMaskClosable
>  backing:NSBackingStoreBuffered defer:NO];
>  if(!normalWindow) {
> -fprintf(stderr, "(cocoa) can't create window\n");
> +error_report("(cocoa) can't create window");
>  exit(1);
>  }
>  [normalWindow setAcceptsMouseMovedEvents:YES];
> 

Alistair did this 3 years ago:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg484556.html
but it was never merged... so:
Reviewed-by: Philippe Mathieu-Daudé 

Re: Can not set high msize with virtio-9p (Was: Re: virtiofs vs 9p performance)

[Christian Schoenebeck]

On Montag, 22. Februar 2021 18:11:59 CET Greg Kurz wrote:
> On Mon, 22 Feb 2021 16:08:04 +0100
> Christian Schoenebeck  wrote:
> 
> [...]
> 
> > I did not ever have a kernel crash when I boot a Linux guest with a 9pfs
> > root fs and 100 MiB msize.
> 
> Interesting.
> 
> > Should we ask virtio or 9p Linux client maintainers if
> > they can add some info what this is about?
> 
> Probably worth to try that first, even if I'm not sure anyone has a
> answer for that since all the people who worked on virtio-9p at
> the time have somehow deserted the project.

Michael, Dominique,

we are wondering here about the message size limitation of just 5 kiB in the 
9p Linux client (using virtio transport) which imposes a performance 
bottleneck, introduced by this kernel commit:

commit b49d8b5d7007a673796f3f99688b46931293873e
Author: Aneesh Kumar K.V 
Date:   Wed Aug 17 16:56:04 2011 +

net/9p: Fix kernel crash with msize 512K

With msize equal to 512K (PAGE_SIZE * VIRTQUEUE_NUM), we hit multiple
crashes. This patch fix those.

Signed-off-by: Aneesh Kumar K.V 
Signed-off-by: Eric Van Hensbergen 

Is this a fundamental maximum message size that cannot be exceeded with virtio 
in general or is there another reason for this limit that still applies?

Full discussion:
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg06343.html

> > > > As the kernel code sais trans_mod->maxsize, maybe its something in
> > > > virtio
> > > > on qemu side that does an automatic step back for some reason. I don't
> > > > see something in the 9pfs virtio transport driver
> > > > (hw/9pfs/virtio-9p-device.c on QEMU side) that would do this, so I
> > > > would
> > > > also need to dig deeper.
> > > > 
> > > > Do you have some RAM limitation in your setup somewhere?
> > > > 
> > > > For comparison, this is how I started the VM:
> > > > 
> > > > ~/git/qemu/build/qemu-system-x86_64 \
> > > > -machine pc,accel=kvm,usb=off,dump-guest-core=off -m 2048 \
> > > > -smp 4,sockets=4,cores=1,threads=1 -rtc base=utc \
> > > > -boot strict=on -kernel
> > > > /home/bee/vm/stretch/boot/vmlinuz-4.9.0-13-amd64 \
> > > > -initrd /home/bee/vm/stretch/boot/initrd.img-4.9.0-13-amd64 \
> > > > -append 'root=svnRoot rw rootfstype=9p
> > > > rootflags=trans=virtio,version=9p2000.L,msize=104857600,cache=mmap
> > > > console=ttyS0' \
> > > 
> > > First obvious difference I see between your setup and mine is that
> > > you're mounting the 9pfs as root from the kernel command line. For
> > > some reason, maybe this has an impact on the check in p9_client_create()
> > > ?
> > > 
> > > Can you reproduce with a scenario like Vivek's one ?
> > 
> > Yep, confirmed. If I boot a guest from an image file first and then try to
> > manually mount a 9pfs share after guest booted, then I get indeed that
> > msize capping of just 512 kiB as well. That's far too small. :/
> 
> Maybe worth digging :
> - why no capping happens in your scenario ?

Because I was wrong.

I just figured even in the 9p rootfs scenario it does indeed cap msize to 5kiB 
as well. The output of /etc/mtab on guest side was fooling me. I debugged this 
on 9p server side and the Linux 9p client always connects with a max. msize of 
5 kiB, no matter what you do.

> - is capping really needed ?
> 
> Cheers,

That's a good question and probably depends on whether there is a limitation 
on virtio side, which I don't have an answer for. Maybe Michael or Dominique 
can answer this.

Best regards,
Christian Schoenebeck

Re: [PATCH v5 0/9] block: Add retry for werror=/rerror= mechanism

[Eric Blake]

On 2/23/21 3:40 AM, Stefan Hajnoczi wrote:
> On Fri, Feb 05, 2021 at 06:13:06PM +0800, Jiahui Cen wrote:
>> This patch series propose to extend the werror=/rerror= mechanism to add
>> a 'retry' feature. It can automatically retry failed I/O requests on error
>> without sending error back to guest, and guest can get back running smoothly
>> when I/O is recovred.
> 
> This patch series implements a retry followed by werror/rerror=report
> after a timeout. This mechanism could be made more generic (and the code
> could be simplified) by removing the new werror/rerror=retry action and
> instead implementing the retry/timeout followed by *any* werror=/rerror=
> policy chosen by the user.
> 
> In other words, if the retry interval is non-zero, retry the request and
> check for timeouts. When the timeout is reached, obey the
> werror=/rerror= action.
> 
> This is more flexible than hard-coding werror=retry to mean retry
> timeout followed by werror=report.
> 
> For example:
> 
>   werror=stop,write-retry-interval=1000,write-retry-timeout=15000,
>   rerror=report,read-retry-interval=1000,read-retry-timeout=15000
> 
> Failed write requests will be retried once a second for 15 seconds.
> If the timeout is reached the guest is stopped.
> 
> Failed read requests will be retried once a second for 15 seconds. If
> the timeout is reached the error is reported to the guest.

You may also want to look at what the NBD block device already
implements for retries, and see if making retry generic to the block
layer in general can do everything already possible in the NBD code, at
which point the NBD code can be simplified.  Vladimir (added in cc) is
the best point of contact there.

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

Re: [PATCH v5 3/4] Jobs based on custom runners: docs and gitlab-runner setup playbook

[Philippe Mathieu-Daudé]

On 2/19/21 10:58 PM, Cleber Rosa wrote:
> To have the jobs dispatched to custom runners, gitlab-runner must
> be installed, active as a service and properly configured.  The
> variables file and playbook introduced here should help with those
> steps.
> 
> The playbook introduced here covers a number of different Linux
> distributions and FreeBSD, and are intended to provide a reproducible
> environment.
> 
> Signed-off-by: Cleber Rosa 
> Reviewed-by: Daniel P. Berrangé 
> ---
>  docs/devel/ci.rst  | 58 ++
>  scripts/ci/setup/.gitignore|  1 +
>  scripts/ci/setup/gitlab-runner.yml | 65 ++
>  scripts/ci/setup/vars.yml.template | 13 ++
>  4 files changed, 137 insertions(+)
>  create mode 100644 scripts/ci/setup/.gitignore
>  create mode 100644 scripts/ci/setup/gitlab-runner.yml
>  create mode 100644 scripts/ci/setup/vars.yml.template
...

> +- name: Remove the .bash_logout file when on Ubuntu systems
> +  file:
> +path: /home/gitlab-runner/.bash_logout
> +state: absent
> +  when: "ansible_facts['distribution'] == 'Ubuntu'"

Is this only a problem with Ubuntu and not Debian?

> +- name: Downloads the matching gitlab-runner
> +  get_url:
> +dest: /usr/local/bin/gitlab-runner
> +url: "{{ gitlab_runner_base_url }}{{ gitlab_runner_os }}-{{ 
> gitlab_runner_arch }}"

Can we move the dash at the end of gitlab_runner_base_url here before
gitlab_runner_os?

...
> diff --git a/scripts/ci/setup/vars.yml.template 
> b/scripts/ci/setup/vars.yml.template
> new file mode 100644
> index 00..621435d030
> --- /dev/null
> +++ b/scripts/ci/setup/vars.yml.template
> @@ -0,0 +1,13 @@
> +# The version of the gitlab-runner to use
> +gitlab_runner_version: 13.1.1
> +# The base location of gitlab-runner binaries, this will be suffixed by 
> $OS-$ARCH
> +gitlab_runner_base_url: 
> https://gitlab-runner-downloads.s3.amazonaws.com/latest/binaries/gitlab-runner-

Are we using a specific feature from the official builds,
or can we use any runner?

Re: [PATCH] docs: move CODING_STYLE into the developer documentation

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> On 2/23/21 12:29 PM, Philippe Mathieu-Daudé wrote:
>> On 2/23/21 12:07 PM, Peter Maydell wrote:
>>> On Tue, 23 Feb 2021 at 10:02, Alex Bennée  wrote:

 There is no particular reason to keep this on it's own in the root of
 the tree. Move it into the rest of the fine developer manual and fixup
 any links to it. The only tweak I've made is to fix the code-block
 annotations to mention the language C.

 Signed-off-by: Alex Bennée 
 ---
 diff --git a/README.rst b/README.rst
 index ce39d89077..f5d41e59b1 100644
 --- a/README.rst
 +++ b/README.rst
 @@ -66,7 +66,9 @@ When submitting patches, one common approach is to use 
 'git
  format-patch' and/or 'git send-email' to format & send the mail to the
  qemu-devel@nongnu.org mailing list. All patches submitted must contain
  a 'Signed-off-by' line from the author. Patches should follow the
 -guidelines set out in the CODING_STYLE.rst file.
 +guidelines set out in the `style section
 +` of
 +the Developers Guide.
>>>
>>> This is the first instance of a qemu.readthedocs.io URL in the
>>> tree. Do we really want to have our references to our documentation
>>> be to a third party website ?
>> 
>> We can use https://www.qemu.org/docs/master/devel/style.html:
>> 
>> $ curl https://www.qemu.org/docs/master/devel/style.html
>> 
>> 
>> 302 Found
>> 
>> Found
>> The document has moved > href="https://qemu.readthedocs.io/en/latest/devel/style.html";>here.
>> 

I think if we treat the qemu.org domain as being the canonical URL and
then let it redirect where it wants. 

> Or even better since we have a job pushing to Gitlab pages
> accessible on https://qemu-project.gitlab.io/qemu/:
>
> https://qemu-project.gitlab.io/qemu/devel/style.html
>
> Maybe the https://www.qemu.org/docs/ redirect should
> go to gitlab page now?

It could do either, I think the result is exactly the same.

>
> Phil.


-- 
Alex Bennée

Re: [PATCH v5 3/4] Jobs based on custom runners: docs and gitlab-runner setup playbook

[Philippe Mathieu-Daudé]

On 2/19/21 10:58 PM, Cleber Rosa wrote:
> To have the jobs dispatched to custom runners, gitlab-runner must
> be installed, active as a service and properly configured.  The
> variables file and playbook introduced here should help with those
> steps.
> 
> The playbook introduced here covers a number of different Linux
> distributions and FreeBSD, and are intended to provide a reproducible
> environment.
> 
> Signed-off-by: Cleber Rosa 
> Reviewed-by: Daniel P. Berrangé 
> ---
>  docs/devel/ci.rst  | 58 ++
>  scripts/ci/setup/.gitignore|  1 +
>  scripts/ci/setup/gitlab-runner.yml | 65 ++
>  scripts/ci/setup/vars.yml.template | 13 ++
>  4 files changed, 137 insertions(+)
>  create mode 100644 scripts/ci/setup/.gitignore
>  create mode 100644 scripts/ci/setup/gitlab-runner.yml
>  create mode 100644 scripts/ci/setup/vars.yml.template

> +- name: Create a user for the gitlab-runner service
> +  user:
> +user: gitlab-runner
> +group: gitlab-runner
> +comment: GitLab Runner
> +home: /home/gitlab-runner
> +shell: /bin/bash
> +
> +- name: Remove the .bash_logout file when on Ubuntu systems
> +  file:
> +path: /home/gitlab-runner/.bash_logout
> +state: absent
> +  when: "ansible_facts['distribution'] == 'Ubuntu'"

Can we have a {{gitlab_runner_homedir}} in vars.yml?

Re: Can not set high msize with virtio-9p (Was: Re: virtiofs vs 9p performance)

[Michael S. Tsirkin]

On Tue, Feb 23, 2021 at 02:39:48PM +0100, Christian Schoenebeck wrote:
> On Montag, 22. Februar 2021 18:11:59 CET Greg Kurz wrote:
> > On Mon, 22 Feb 2021 16:08:04 +0100
> > Christian Schoenebeck  wrote:
> > 
> > [...]
> > 
> > > I did not ever have a kernel crash when I boot a Linux guest with a 9pfs
> > > root fs and 100 MiB msize.
> > 
> > Interesting.
> > 
> > > Should we ask virtio or 9p Linux client maintainers if
> > > they can add some info what this is about?
> > 
> > Probably worth to try that first, even if I'm not sure anyone has a
> > answer for that since all the people who worked on virtio-9p at
> > the time have somehow deserted the project.
> 
> Michael, Dominique,
> 
> we are wondering here about the message size limitation of just 5 kiB in the 
> 9p Linux client (using virtio transport) which imposes a performance 
> bottleneck, introduced by this kernel commit:
> 
> commit b49d8b5d7007a673796f3f99688b46931293873e
> Author: Aneesh Kumar K.V 
> Date:   Wed Aug 17 16:56:04 2011 +
> 
> net/9p: Fix kernel crash with msize 512K
> 
> With msize equal to 512K (PAGE_SIZE * VIRTQUEUE_NUM), we hit multiple
> crashes. This patch fix those.
> 
> Signed-off-by: Aneesh Kumar K.V 
> Signed-off-by: Eric Van Hensbergen 

Well the change I see is:

-   .maxsize = PAGE_SIZE*VIRTQUEUE_NUM,
+   .maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 3),


so how come you say it changes 512K to 5K?
Looks more like 500K to me.

> Is this a fundamental maximum message size that cannot be exceeded with 
> virtio 
> in general or is there another reason for this limit that still applies?
> 
> Full discussion:
> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg06343.html
> 
> > > > > As the kernel code sais trans_mod->maxsize, maybe its something in
> > > > > virtio
> > > > > on qemu side that does an automatic step back for some reason. I don't
> > > > > see something in the 9pfs virtio transport driver
> > > > > (hw/9pfs/virtio-9p-device.c on QEMU side) that would do this, so I
> > > > > would
> > > > > also need to dig deeper.
> > > > > 
> > > > > Do you have some RAM limitation in your setup somewhere?
> > > > > 
> > > > > For comparison, this is how I started the VM:
> > > > > 
> > > > > ~/git/qemu/build/qemu-system-x86_64 \
> > > > > -machine pc,accel=kvm,usb=off,dump-guest-core=off -m 2048 \
> > > > > -smp 4,sockets=4,cores=1,threads=1 -rtc base=utc \
> > > > > -boot strict=on -kernel
> > > > > /home/bee/vm/stretch/boot/vmlinuz-4.9.0-13-amd64 \
> > > > > -initrd /home/bee/vm/stretch/boot/initrd.img-4.9.0-13-amd64 \
> > > > > -append 'root=svnRoot rw rootfstype=9p
> > > > > rootflags=trans=virtio,version=9p2000.L,msize=104857600,cache=mmap
> > > > > console=ttyS0' \
> > > > 
> > > > First obvious difference I see between your setup and mine is that
> > > > you're mounting the 9pfs as root from the kernel command line. For
> > > > some reason, maybe this has an impact on the check in p9_client_create()
> > > > ?
> > > > 
> > > > Can you reproduce with a scenario like Vivek's one ?
> > > 
> > > Yep, confirmed. If I boot a guest from an image file first and then try to
> > > manually mount a 9pfs share after guest booted, then I get indeed that
> > > msize capping of just 512 kiB as well. That's far too small. :/
> > 
> > Maybe worth digging :
> > - why no capping happens in your scenario ?
> 
> Because I was wrong.
> 
> I just figured even in the 9p rootfs scenario it does indeed cap msize to 
> 5kiB 
> as well. The output of /etc/mtab on guest side was fooling me. I debugged 
> this 
> on 9p server side and the Linux 9p client always connects with a max. msize 
> of 
> 5 kiB, no matter what you do.
> 
> > - is capping really needed ?
> > 
> > Cheers,
> 
> That's a good question and probably depends on whether there is a limitation 
> on virtio side, which I don't have an answer for. Maybe Michael or Dominique 
> can answer this.
> 
> Best regards,
> Christian Schoenebeck
> 

Re: [PATCH v5 3/4] Jobs based on custom runners: docs and gitlab-runner setup playbook

[Philippe Mathieu-Daudé]

On 2/19/21 10:58 PM, Cleber Rosa wrote:
> To have the jobs dispatched to custom runners, gitlab-runner must
> be installed, active as a service and properly configured.  The
> variables file and playbook introduced here should help with those
> steps.
> 
> The playbook introduced here covers a number of different Linux
> distributions and FreeBSD, and are intended to provide a reproducible
> environment.
> 
> Signed-off-by: Cleber Rosa 
> Reviewed-by: Daniel P. Berrangé 
> ---
>  docs/devel/ci.rst  | 58 ++
>  scripts/ci/setup/.gitignore|  1 +
>  scripts/ci/setup/gitlab-runner.yml | 65 ++
>  scripts/ci/setup/vars.yml.template | 13 ++
>  4 files changed, 137 insertions(+)
>  create mode 100644 scripts/ci/setup/.gitignore
>  create mode 100644 scripts/ci/setup/gitlab-runner.yml
>  create mode 100644 scripts/ci/setup/vars.yml.template

> +- name: Register the gitlab-runner
> +  command: "/usr/local/bin/gitlab-runner register --non-interactive 
> --url {{ gitlab_runner_server_url }} --registration-token {{ 
> gitlab_runner_registration_token }} --executor shell  --description '{{ 
> ansible_facts[\"distribution\"] }} {{ ansible_facts[\"distribution_version\"] 
> }} {{ ansible_facts[\"architecture\"] }} ({{ ansible_facts[\"os_family\"] 
> }})'"

Hmm maybe we want to register them with --run-untagged=false or
explicitly add tags like {{ ansible_facts[\"architecture\"] }}.

Also, maybe have --cache-shared by default?

And set a reasonable limits values...
 --maximum-timeout 10800 # 3h
 --output-limit 8192 # 8MiB

No CPU/memory limits yet.

Re: [RFC PATCH 0/6] vhost-user: Shutdown/Flush slave channel properly

[Michael S. Tsirkin]

On Mon, Jan 25, 2021 at 01:01:09PM -0500, Vivek Goyal wrote:
> Hi,
> 
> We are working on DAX support in virtiofs and have some patches out of
> the tree hosted here.
> 
> https://gitlab.com/virtio-fs/qemu/-/commits/virtio-fs-dev

any plans to post a non RFC version?

> These patches have not been proposed for merge yet, becasue David
> Gilbert noticed that we can run into a deadlock during an emergency
> reboot of guest kernel. (echo b > /proc/sysrq-trigger).
> 
> I have provided details of deadlock in 4th path of the series with
> subject "qemu, vhost-user: Extend protocol to start/stop/flush slave
> channel".
> 
> Basic problem seems to be that we don't have a proper mechanism to
> shutdown slave channel when vhost-user device is stopping. This means
> there might be pending messages in slave channel and slave is blocked
> and waiting for response.
> 
> This is an RFC patch series to enhance vhost-user protocol to 
> properly shutdown/flush slave channel and avoid the deadlock. Though
> we faced the issue in the context of virtiofs, any vhost-user
> device using slave channel can potentially run into issues and
> can benefit from these patches.
> 
> Any feedback is welcome. Currently patches are based on out of
> tree code but after I get some feedback, I can only take pieces
> which are relevant to upstream and post separately.
> 
> Thanks
> Vivek
> 
> Vivek Goyal (6):
>   virtiofsd: Drop ->vu_dispatch_rwlock while waiting for thread to exit
>   libvhost-user: Use slave_mutex in all slave messages
>   vhost-user: Return error code from slave_read()
>   qemu, vhost-user: Extend protocol to start/stop/flush slave channel
>   libvhost-user: Add support to start/stop/flush slave channel
>   virtiofsd: Opt in for slave start/stop/shutdown functionality
> 
>  hw/virtio/vhost-user.c| 151 +-
>  subprojects/libvhost-user/libvhost-user.c | 147 +
>  subprojects/libvhost-user/libvhost-user.h |   8 +-
>  tools/virtiofsd/fuse_virtio.c |  20 +++
>  4 files changed, 294 insertions(+), 32 deletions(-)
> 
> -- 
> 2.25.4

Re: [PATCH v3] virtio-blk: Respect discard granularity

[Michael S. Tsirkin]

On Tue, Feb 23, 2021 at 09:09:40PM +0900, Akihiko Odaki wrote:
> Signed-off-by: Akihiko Odaki 


Acked-by: Michael S. Tsirkin 


whoever knows more about the detail here, feel free to merge.

> ---
>  hw/block/virtio-blk.c  | 8 +++-
>  hw/core/machine.c  | 4 +++-
>  include/hw/virtio/virtio-blk.h | 1 +
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index bac2d6fa2b2..f4378e61182 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -962,10 +962,14 @@ static void virtio_blk_update_config(VirtIODevice 
> *vdev, uint8_t *config)
>  blkcfg.wce = blk_enable_write_cache(s->blk);
>  virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
>  if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_DISCARD)) {
> +uint32_t discard_granularity = conf->discard_granularity;
> +if (discard_granularity == -1 || 
> !s->conf.report_discard_granularity) {
> +discard_granularity = blk_size;
> +}
>  virtio_stl_p(vdev, &blkcfg.max_discard_sectors,
>   s->conf.max_discard_sectors);
>  virtio_stl_p(vdev, &blkcfg.discard_sector_alignment,
> - blk_size >> BDRV_SECTOR_BITS);
> + discard_granularity >> BDRV_SECTOR_BITS);
>  /*
>   * We support only one segment per request since multiple segments
>   * are not widely used and there are no userspace APIs that allow
> @@ -1299,6 +1303,8 @@ static Property virtio_blk_properties[] = {
>   IOThread *),
>  DEFINE_PROP_BIT64("discard", VirtIOBlock, host_features,
>VIRTIO_BLK_F_DISCARD, true),
> +DEFINE_PROP_BOOL("report-discard-granularity", VirtIOBlock,
> + conf.report_discard_granularity, true),
>  DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
>VIRTIO_BLK_F_WRITE_ZEROES, true),
>  DEFINE_PROP_UINT32("max-discard-sectors", VirtIOBlock,
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index de3b8f1b318..e4df5797e72 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -33,7 +33,9 @@
>  #include "migration/global_state.h"
>  #include "migration/vmstate.h"
>  
> -GlobalProperty hw_compat_5_2[] = {};
> +GlobalProperty hw_compat_5_2[] = {
> +{ "virtio-blk-device", "report-discard-granularity", "off" },
> +};
>  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
>  
>  GlobalProperty hw_compat_5_1[] = {
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 214ab748229..29655a406dd 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -41,6 +41,7 @@ struct VirtIOBlkConf
>  uint16_t num_queues;
>  uint16_t queue_size;
>  bool seg_max_adjust;
> +bool report_discard_granularity;
>  uint32_t max_discard_sectors;
>  uint32_t max_write_zeroes_sectors;
>  bool x_enable_wce_if_config_wce;
> -- 
> 2.24.3 (Apple Git-128)