Re: [PATCH] pci_dma_rw: return correct value instead of 0

[Stefano Garzarella]

On Thu, Jul 30, 2020 at 12:17:32AM +0200, Emanuele Giuseppe Esposito wrote:
> pci_dma_rw currently always returns 0, regardless
> of the result of dma_memory_rw. Adjusted to return
> the correct value.
> 
> Signed-off-by: Emanuele Giuseppe Esposito 
> ---
>  include/hw/pci/pci.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index c1bf7d5356..41c4ab5932 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -787,8 +787,7 @@ static inline AddressSpace 
> *pci_get_address_space(PCIDevice *dev)
>  static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
>   void *buf, dma_addr_t len, DMADirection dir)
>  {
> -dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> -return 0;
> +return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
>  }

I think it's a left over from when we used "void cpu_physical_memory_rw()".

I agree that it is better to return the dma_memory_rw() return value, but
at first look, no one seems to check the return value of pci_dma_rw(),
pci_dma_read(), andpci_dma_write().

Should we make them void?


Anyway, for this patch:

Reviewed-by: Stefano Garzarella 

Thanks,
Stefano

Re: [PATCH] virtio-mem: Work around format specifier mismatch for RISC-V

[Stefano Garzarella]

On Wed, Jul 29, 2020 at 06:54:38PM -0600, Bruce Rogers wrote:
> This likely affects other, less popular host architectures as well.
> Less common host architectures under linux get QEMU_VMALLOC_ALIGN (from
> which VIRTIO_MEM_MIN_BLOCK_SIZE is derived) define to a variable of
> type uintptr, which isn't compatible with the format specifier used to
> print a user message. Since this particular usage of the underlying data
> seems unique, the simple fix is to just cast it to the corresponding
> format specifier.
> 
> Signed-off-by: Bruce Rogers 
> ---
>  hw/virtio/virtio-mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index c12e9f79b0..fd01ffd83e 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -754,7 +754,7 @@ static void virtio_mem_set_block_size(Object *obj, 
> Visitor *v, const char *name,
>  
>  if (value < VIRTIO_MEM_MIN_BLOCK_SIZE) {
>  error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, name,
> -   VIRTIO_MEM_MIN_BLOCK_SIZE);
> +   (unsigned int)VIRTIO_MEM_MIN_BLOCK_SIZE);

Since we use PRIx32, could be better to cast VIRTIO_MEM_MIN_BLOCK_SIZE
to uint32_t?

Thanks,
Stefano

>  return;
>  } else if (!is_power_of_2(value)) {
>  error_setg(errp, "'%s' property has to be a power of two", name);
> -- 
> 2.27.0
> 
> 

Re: [PATCH] virtio-mem: Work around format specifier mismatch for RISC-V

[David Hildenbrand]

On 30.07.20 09:49, Stefano Garzarella wrote:
> On Wed, Jul 29, 2020 at 06:54:38PM -0600, Bruce Rogers wrote:
>> This likely affects other, less popular host architectures as well.
>> Less common host architectures under linux get QEMU_VMALLOC_ALIGN (from
>> which VIRTIO_MEM_MIN_BLOCK_SIZE is derived) define to a variable of
>> type uintptr, which isn't compatible with the format specifier used to
>> print a user message. Since this particular usage of the underlying data
>> seems unique, the simple fix is to just cast it to the corresponding
>> format specifier.
>>
>> Signed-off-by: Bruce Rogers 
>> ---
>>  hw/virtio/virtio-mem.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
>> index c12e9f79b0..fd01ffd83e 100644
>> --- a/hw/virtio/virtio-mem.c
>> +++ b/hw/virtio/virtio-mem.c
>> @@ -754,7 +754,7 @@ static void virtio_mem_set_block_size(Object *obj, 
>> Visitor *v, const char *name,
>>  
>>  if (value < VIRTIO_MEM_MIN_BLOCK_SIZE) {
>>  error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, 
>> name,
>> -   VIRTIO_MEM_MIN_BLOCK_SIZE);
>> +   (unsigned int)VIRTIO_MEM_MIN_BLOCK_SIZE);
> 
> Since we use PRIx32, could be better to cast VIRTIO_MEM_MIN_BLOCK_SIZE
> to uint32_t?

Yeah, I guess something like

-#define VIRTIO_MEM_MIN_BLOCK_SIZE QEMU_VMALLOC_ALIGN
+#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)

would be cleaner

-- 
Thanks,

David / dhildenb

Re: [PATCH] virtio-mem: Work around format specifier mismatch for RISC-V

[Stefano Garzarella]

On Thu, Jul 30, 2020 at 09:51:19AM +0200, David Hildenbrand wrote:
> On 30.07.20 09:49, Stefano Garzarella wrote:
> > On Wed, Jul 29, 2020 at 06:54:38PM -0600, Bruce Rogers wrote:
> >> This likely affects other, less popular host architectures as well.
> >> Less common host architectures under linux get QEMU_VMALLOC_ALIGN (from
> >> which VIRTIO_MEM_MIN_BLOCK_SIZE is derived) define to a variable of
> >> type uintptr, which isn't compatible with the format specifier used to
> >> print a user message. Since this particular usage of the underlying data
> >> seems unique, the simple fix is to just cast it to the corresponding
> >> format specifier.
> >>
> >> Signed-off-by: Bruce Rogers 
> >> ---
> >>  hw/virtio/virtio-mem.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> >> index c12e9f79b0..fd01ffd83e 100644
> >> --- a/hw/virtio/virtio-mem.c
> >> +++ b/hw/virtio/virtio-mem.c
> >> @@ -754,7 +754,7 @@ static void virtio_mem_set_block_size(Object *obj, 
> >> Visitor *v, const char *name,
> >>  
> >>  if (value < VIRTIO_MEM_MIN_BLOCK_SIZE) {
> >>  error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, 
> >> name,
> >> -   VIRTIO_MEM_MIN_BLOCK_SIZE);
> >> +   (unsigned int)VIRTIO_MEM_MIN_BLOCK_SIZE);
> > 
> > Since we use PRIx32, could be better to cast VIRTIO_MEM_MIN_BLOCK_SIZE
> > to uint32_t?
> 
> Yeah, I guess something like
> 
> -#define VIRTIO_MEM_MIN_BLOCK_SIZE QEMU_VMALLOC_ALIGN
> +#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)
> 
> would be cleaner

Yeah, it is cleaner.

Otherwise we could use PRIxPTR in the error message.

Thanks,
Stefano

Re: [RFC v2 01/76] target/riscv: drop vector 0.7.1 support

[Frank Chang]

On Tue, Jul 28, 2020 at 4:05 AM Alistair Francis 
wrote:

> On Mon, Jul 27, 2020 at 12:54 PM Palmer Dabbelt 
> wrote:
> >
> > On Wed, 22 Jul 2020 02:15:24 PDT (-0700), frank.ch...@sifive.com wrote:
> > > From: Frank Chang 
> > >
> > > Signed-off-by: Frank Chang 
> > > ---
> > >  target/riscv/cpu.c | 24 ++--
> > >  target/riscv/cpu.h |  2 --
> > >  2 files changed, 6 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 228b9bdb5d..2800953e6c 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -106,11 +106,6 @@ static void set_priv_version(CPURISCVState *env,
> int priv_ver)
> > >  env->priv_ver = priv_ver;
> > >  }
> > >
> > > -static void set_vext_version(CPURISCVState *env, int vext_ver)
> > > -{
> > > -env->vext_ver = vext_ver;
> > > -}
> > > -
> > >  static void set_feature(CPURISCVState *env, int feature)
> > >  {
> > >  env->features |= (1ULL << feature);
> > > @@ -339,7 +334,6 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
> > >  CPURISCVState *env = &cpu->env;
> > >  RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> > >  int priv_version = PRIV_VERSION_1_11_0;
> > > -int vext_version = VEXT_VERSION_0_07_1;
> > >  target_ulong target_misa = 0;
> > >  Error *local_err = NULL;
> > >
> > > @@ -363,7 +357,6 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
> > >  }
> > >
> > >  set_priv_version(env, priv_version);
> > > -set_vext_version(env, vext_version);
> > >
> > >  if (cpu->cfg.mmu) {
> > >  set_feature(env, RISCV_FEATURE_MMU);
> > > @@ -455,19 +448,14 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
> > >  return;
> > >  }
> > >  if (cpu->cfg.vext_spec) {
> > > -if (!g_strcmp0(cpu->cfg.vext_spec, "v0.7.1")) {
> > > -vext_version = VEXT_VERSION_0_07_1;
> > > -} else {
> > > -error_setg(errp,
> > > -   "Unsupported vector spec version '%s'",
> > > -   cpu->cfg.vext_spec);
> > > -return;
> > > -}
> > > +error_setg(errp,
> > > +   "Unsupported vector spec version '%s'",
> > > +   cpu->cfg.vext_spec);
> > > +return;
> > >  } else {
> > > -qemu_log("vector verison is not specified, "
> > > -"use the default value v0.7.1\n");
> > > +qemu_log("vector version is not specified\n");
> > > +return;
> > >  }
> > > -set_vext_version(env, vext_version);
> > >  }
> > >
> > >  set_misa(env, RVXLEN | target_misa);
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index eef20ca6e5..6766dcd914 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -79,8 +79,6 @@ enum {
> > >  #define PRIV_VERSION_1_10_0 0x00011000
> > >  #define PRIV_VERSION_1_11_0 0x00011100
> > >
> > > -#define VEXT_VERSION_0_07_1 0x0701
> > > -
> > >  #define TRANSLATE_PMP_FAIL 2
> > >  #define TRANSLATE_FAIL 1
> > >  #define TRANSLATE_SUCCESS 0
> >
> > If I'm reading things correctly, 5.0 did not have the V extension.  This
> means
> > that we can technically drop 0.7.1 from QEMU, as it's never been
> released.
>
> There is no intention of this making it into 5.1. We are currently in
> hard freeze.
>
> The idea is that QEMU 5.1 will support v0.7.1 and then hopefully 5.2
> will support v0.9.
>
> > That said, I'd still prefer to avoid dropping 0.7.1 so late in the
> release
> > cycle (it's already soft freeze, right?).  Given the extended length of
> the V
> > extension development process it seems likely that 0.7.1 is going to end
> up in
> > some silicon, which means it would be quite useful to have it in QEMU.
>
> Agreed!
>
> >
> > I understand it's a lot more work to maintain multiple vector
> extensions, but
> > it was very useful to have multiple privileged extensions supported in
> QEMU
> > while that was all getting sorted out and as the vector drafts has
> massive
> > differences it'll probably be even more useful.
>
> Hopefully a release will be enough for this as managing both will be a
> huge maintenance burden.
>
> Alistair
>
> >




Although RVV v1.0 spec is not ratified yet.
However, the differences between RVV v0.9 by far are minor:
1. SLEN=VLEN layout mandatory.
>> Luckily we haven't defined *slen* in our RVV implementation by far, so
we can just ignore the change.
2. Support ELEN > VLEN for LMUL > 1
3. Defined vector FP exception behavior.
4. Added reciprocal and reciprocal square-root estimate instructions.
>> *vfrece7.v* and *vfrsqrte7.v* instructions are added, but the
behaviors are still undefined.
5. Defined hint behavior on whole register moves and load/stores to enable
microarchitectures 

Re: [PATCH] virtio-mem: Work around format specifier mismatch for RISC-V

[David Hildenbrand]

On 30.07.20 09:58, Stefano Garzarella wrote:
> On Thu, Jul 30, 2020 at 09:51:19AM +0200, David Hildenbrand wrote:
>> On 30.07.20 09:49, Stefano Garzarella wrote:
>>> On Wed, Jul 29, 2020 at 06:54:38PM -0600, Bruce Rogers wrote:
 This likely affects other, less popular host architectures as well.
 Less common host architectures under linux get QEMU_VMALLOC_ALIGN (from
 which VIRTIO_MEM_MIN_BLOCK_SIZE is derived) define to a variable of
 type uintptr, which isn't compatible with the format specifier used to
 print a user message. Since this particular usage of the underlying data
 seems unique, the simple fix is to just cast it to the corresponding
 format specifier.

 Signed-off-by: Bruce Rogers 
 ---
  hw/virtio/virtio-mem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
 index c12e9f79b0..fd01ffd83e 100644
 --- a/hw/virtio/virtio-mem.c
 +++ b/hw/virtio/virtio-mem.c
 @@ -754,7 +754,7 @@ static void virtio_mem_set_block_size(Object *obj, 
 Visitor *v, const char *name,
  
  if (value < VIRTIO_MEM_MIN_BLOCK_SIZE) {
  error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, 
 name,
 -   VIRTIO_MEM_MIN_BLOCK_SIZE);
 +   (unsigned int)VIRTIO_MEM_MIN_BLOCK_SIZE);
>>>
>>> Since we use PRIx32, could be better to cast VIRTIO_MEM_MIN_BLOCK_SIZE
>>> to uint32_t?
>>
>> Yeah, I guess something like
>>
>> -#define VIRTIO_MEM_MIN_BLOCK_SIZE QEMU_VMALLOC_ALIGN
>> +#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)
>>
>> would be cleaner
> 
> Yeah, it is cleaner.

Bruce, can you respin if you agree? Thanks!


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/3] trace: Add support for recorder back-end

[Markus Armbruster]

Christophe de Dinechin  writes:

> On 2020-07-29 at 13:53 CEST, Markus Armbruster wrote...
>> Christophe de Dinechin  writes:
>>
>>> On 2020-07-27 at 10:23 CEST, Markus Armbruster wrote...
 Christophe de Dinechin  writes:

> On 2020-07-23 at 16:06 CEST, Markus Armbruster wrote...
>> Christophe de Dinechin  writes:
>> [...]
>>> I'm certainly not against adding a command-line option to activate 
>>> recorder
>>> options specifically, but as I understand, the option -trace already 
>>> exists,
>>> and its semantics is sufficiently different from the one in recorder
>>> patterns that I decided to not connect the two for now. For example, to
>>> disable trace foo, you'd pass "-foo" to the -trace option, but "foo=0" 
>>> to
>>> RECORDER_TRACES. The parsing of graphing options and other related
>>> recorder-specific stuff is a bit difficult to integrate with -trace too.
>>
>> We need proper integration with the existing trace UI.
>
> I agree, but I don't think this belongs to this particular patch series.
> See below why.
>
>>
>> In particular, the ability to enable and disable trace events
>> dynamically provided by QMP commands trace-event-get-state,
>> trace-event-set-state, and HMP command trace-event is really useful.
>
> That ability exists, but given the many differences between the
> recorder and other tracing mechanisms, I found it useful to add a specific
> "recorder" command.

 Precedence for commands specific to a trace backend: trace-file.

 Name yours trace-recorder?
>>>
>>> But then "recorder dump" does not fit with any "trace" concept.
>>
>> Once you make the recorder a trace backend, whatever the recorder does
>> becomes a trace concept :)
>
> I understand your point, but I want to make a distinction between recorder
> tracing and other recorder features. Does that make sense?

Maybe :)  My thoughts haven't settled, yet.

> For example, assuming I built with both recorder and log trace backends,
> from the monitor, I can type:
>
>   trace-event kvm_run_exit on
>
> What I get then is something like that:
>
>   2091091@1595518935.441273:kvm_run_exit cpu_index 0, reason 2
>   2091091@1595518935.441292:kvm_run_exit cpu_index 0, reason 2
>   2091091@1595518935.441301:kvm_run_exit cpu_index 0, reason 2
>   2091091@1595518935.441309:kvm_run_exit cpu_index 0, reason 2
>   2091091@1595518935.441254:kvm_run_exit cpu_index 0, reason 2
>
> It would not be very useful to activate recorder traces as well when that
> happens, which would have the undesired side effect of purging any
>> corresponding entry from a following recorder dump.

 I'm afraid I don't understand, because the gaps in my understanding of
 what the recorder can do are just too big.
>>>
>>> There is a video at the top of https://github.com/c3d/recorder, or direct
>>> link https://www.youtube.com/watch?v=kEnQY1zFa0Y. Hope this helps.
>>>
>>>

 From your cover letter:

 1. Flight recorder: Dump information on recent events in case of crash

 Define "recent events", please.  Is it all trace events (except for the
 ones disabled at build time, of course)?
>>>
>>> For event categories only known through qemu trace definitions, by default,
>>
>> Right now, there are no others, aren't there?
>
> There is one in the second patch, the example. That was actually the whole
> point of that second patch, which is not otherwise particularly exciting.
>
>>
>>> it's the last 8 events. If you manually declare a recorder, then you can
>>> specify any size.
>>>
>>> (The difference between this manual recorder usage and the recorder backend
>>> generated code is similar to the difference between the log backend and
>>> "DPRINTF")
>>
>> I'm not sure I get the parenthesis.
>
> I took that example because it's mentioned in docs/devel/tracing.txt:
>
> === Log ===
>
> The "log" backend sends trace events directly to standard error.  This
> effectively turns trace events into debug printfs.
>
> This is the simplest backend and can be used together with existing code 
> that
> uses DPRINTF().
>
> Just the same way, the recorder back-end can both use existing trace points,
> using the recorder back-end, but you can also add explicit record()
> statements much like you can add DPRINTF() statements.

Got it.

> Whether this is a good idea or not is debatable. I would argue that in some
> cases, it's a good idea, or at least, probably a better idea than DPRINTF ;-)

No need to convince me a low-overhead flight recorder can be useful.
I've improvised special-purpose flight recorders many times when
printf-like tracing interfered enough with the timing to make the
interesting behavior go away.

I doubt having both trace points and record() makes sense in QEMU except
for ad hoc debugging.

 "Flight recorder" hints at rec

Re: [PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Markus Armbruster]

Paolo Bonzini  writes:

> On 29/07/20 09:39, Markus Armbruster wrote:
>> Taking a step back, I disagree with the notion that assertions should be
>> avoided just because we have an Error **.  A programming error doesn't
>> become less wrong, and continuing when the program is in disorder
>> doesn't become any safer when you add an Error ** parameter to a
>> function.
>
> I don't think it is actually unsafe to continue after passing a bus-less
> device with a bus_type to qdev_realize.  It will fail, but orderly.
>
> So even though it's a programming error, it should not be a big deal to
> avoid the assertion here: either the caller will pass &error_abort, or
> it will print a nice error message and let the user go on with their
> business.

An error message the user can do nothing about other than report as a
bug is never nice.

We can try to phrase the message in a way that makes "this is a bug,
please report" clear, but doing that case-by-case can only result in
inconsistency and confusion.  Having a common way to do it gets us to:

>> If you're calling for recovering from programming errors where that can
>> be done safely, we can talk about creating the necessary infrastructure.
>> Handling them as if they were errors the user can do something about can
>> only lead to confusion.

Moreover, we want programming errors reported even when we recover, so
they get fixed.  If we treat them like any other error, that is not
assured: errors may be handled silently.

> I'm not particularly attached to the change, but it seemed inconsistent
> to use error_setg(&error_abort).

>From error.h:

 * Please don't error_setg(&error_fatal, ...), use error_report() and
 * exit(), because that's more obvious.
 * Likewise, don't error_setg(&error_abort, ...), use assert().

Re: [PATCH 2/2] target/arm: Fix compile error.

[Peter Maydell]

On Thu, 30 Jul 2020 at 02:56, Kaige Li  wrote:
>
> When I compile qemu with such as:
>
> git clone https://git.qemu.org/git/qemu.git
> cd qemu
> git submodule init
> git submodule update --recursive
> ./configure
> make
>
> There is error log:
>
> /home/LiKaige/qemu/target/arm/translate-a64.c: In function ‘disas_ldst’:
> /home/LiKaige/qemu/target/arm/translate-a64.c:3392:5: error: ‘fn’ may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
>  fn(cpu_reg(s, rt), clean_addr, tcg_rs, get_mem_index(s),
>  ^
> /home/LiKaige/qemu/target/arm/translate-a64.c:3318:22: note: ‘fn’ was 
> declared here
>  AtomicThreeOpFn *fn;
>   ^
> cc1: all warnings being treated as errors
>
> So, add an initiallization value for fn to fix this.
>
> Signed-off-by: Kaige Li 

What compiler version is this ?

> ---
>  target/arm/translate-a64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 8c07649..910a91f 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -3315,7 +3315,7 @@ static void disas_ldst_atomic(DisasContext *s, uint32_t 
> insn,
>  bool r = extract32(insn, 22, 1);
>  bool a = extract32(insn, 23, 1);
>  TCGv_i64 tcg_rs, clean_addr;
> -AtomicThreeOpFn *fn;
> +AtomicThreeOpFn *fn = tcg_gen_atomic_fetch_add_i64;

NULL would be a better choice for a "this is never actually used"
initialiser.

thanks
-- PMM

Re: [PATCH] pci_dma_rw: return correct value instead of 0

[Peter Maydell]

On Wed, 29 Jul 2020 at 23:19, Emanuele Giuseppe Esposito
 wrote:
>
> pci_dma_rw currently always returns 0, regardless
> of the result of dma_memory_rw. Adjusted to return
> the correct value.
>
> Signed-off-by: Emanuele Giuseppe Esposito 

We also have the equivalent patch from Klaus Jensen back in 2019
which got reviewed at the time but seems to have gotten lost along the way:
https://patchwork.kernel.org/patch/11184911/

thanks
-- PMM

Re: [PATCH] pci_dma_rw: return correct value instead of 0

[Emanuele Giuseppe Esposito]

On 30/07/2020 09:41, Stefano Garzarella wrote:

On Thu, Jul 30, 2020 at 12:17:32AM +0200, Emanuele Giuseppe Esposito wrote:

pci_dma_rw currently always returns 0, regardless
of the result of dma_memory_rw. Adjusted to return
the correct value.

Signed-off-by: Emanuele Giuseppe Esposito 
---
  include/hw/pci/pci.h | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index c1bf7d5356..41c4ab5932 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -787,8 +787,7 @@ static inline AddressSpace *pci_get_address_space(PCIDevice 
*dev)
  static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
   void *buf, dma_addr_t len, DMADirection dir)
  {
-dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
-return 0;
+return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
  }


I think it's a left over from when we used "void cpu_physical_memory_rw()".

I agree that it is better to return the dma_memory_rw() return value, but
at first look, no one seems to check the return value of pci_dma_rw(),
pci_dma_read(), andpci_dma_write().

Should we make them void?


I noticed that nobody checks the return of those functions, but I think 
checking for possible error is always useful. I am using the edu device 
and clearly doing something wrong since with this fix I discovered that 
the pci_dma_read call returns nonzero.


Keeping the function as it is or void would make it harder to spot such 
errors in future.


Thank you,
Emanuele




Anyway, for this patch:

Reviewed-by: Stefano Garzarella 

Thanks,
Stefano

Re: [PATCH v3 08/18] hw/block/nvme: add support for the asynchronous event request command

[Maxim Levitsky]

On Wed, 2020-07-29 at 22:08 +0200, Klaus Jensen wrote:
> On Jul 29 21:45, Maxim Levitsky wrote:
> > On Wed, 2020-07-29 at 15:37 +0200, Klaus Jensen wrote:
> > > On Jul 29 13:43, Maxim Levitsky wrote:
> > > > On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> > > > > +DEFINE_PROP_UINT8("aerl", NvmeCtrl, params.aerl, 3),
> > > > So this is number of AERs that we allow the user to be outstanding
> > > 
> > > Yeah, and per the spec, 0's based.
> > > 
> > > > > +DEFINE_PROP_UINT32("aer_max_queued", NvmeCtrl, 
> > > > > params.aer_max_queued, 64),
> > > > And this is the number of AERs that we keep in our internal AER queue 
> > > > untill user posts and AER so that we
> > > > can complete it.
> > > > 
> > > 
> > > Correct.
> > 
> > Yep - this is what I understood after examining all of the patch, but from 
> > the names itself it is hard to understand this.
> > Maybe a comment next to property to at least make it easier for advanced 
> > user (e.g user that reads code)
> > to understand?
> > 
> > (I often end up reading source to understand various qemu device 
> > parameters).
> > 
> 
> I should add this in docs/specs/nvme.txt (which shows up in one of my
> next series when I add a new PCI id for the device). For now, I will add
> it to the top of the file like the rest of the parameters.
This is a good idea!
> 
> Subsequent series contains a lot more additions of new parameters that
> is directly from the spec and to me it really only makes sense that they
> share the names if they can.
> 
> We could consider having them under a "spec namespace"? So, say, we do
> DEFINE_PROP_UINT("spec.aerl", ...)?
I personally tend to think that it won't make it much more readable.

Best regards,
Maxim Levitsky
> 

Re: [PATCH] pci_dma_rw: return correct value instead of 0

[Peter Maydell]

On Thu, 30 Jul 2020 at 08:42, Stefano Garzarella  wrote:
> I agree that it is better to return the dma_memory_rw() return value, but
> at first look, no one seems to check the return value of pci_dma_rw(),
> pci_dma_read(), andpci_dma_write().
>
> Should we make them void?

In general code (eg device models) that issues memory transactions
need to have a mechanism for finding out whether the transaction
succeeds. Traditionally QEMU didn't have the concept of a
transaction failing, but we have added it, starting with the
APIs at the bottom level (the address_space_* ones). We haven't
always plumbed the error-handling (or the memory-transaction
input, for that matter) through to some of these other APIs.
I think for consistency we should do that, and ideally we
should make all these APIs look the same as the base-level
address_space* ones, which would mean returning a MemTxError
rather than a bool.

We should also figure out why the dma_* functions exist at all:
they include some calls to dma_barrier(), but not all devices
do DMA with the dma_* functions, so we have an inconsistency
that should be sorted out...

thanks
-- PMM

Re: [PATCH 2/5] virtiofsd: create lock/pid file in per user cache dir

[Daniel P . Berrangé]

On Wed, Jul 29, 2020 at 06:14:07PM -0400, Vivek Goyal wrote:
> Right now we create lock/pid file in /usr/local/var/... and unprivliged
> user does not have access to create files there.
> 
> So create this file in per user cache dir as queried as specified
> by environment variable XDG_RUNTIME_DIR.
> 
> Note: "su $USER" does not update XDG_RUNTIME_DIR and it still points to
> root user's director. So for now I create a directory /tmp/$UID to save
> lock/pid file. Dan pointed out that it can be a problem if a malicious
> app already has /tmp/$UID created. So we probably need to get rid of this.

IMHO use of "su $USER" is simply user error and we don't need to
care about workarounds. They will see the startup fail due to
EPERM on /run/user/0 directory, and then they'll have to fix
their command to use "su - $USER" to setup a clean environment.


> +/*
> + * Unpriviliged users don't have access to /usr/local/var. Hence
> + * store lock/pid file in per user directory. Use environment
> + * variable XDG_RUNTIME_DIR.
> + * If one logs into the system as root and then does "su" then
> + * XDG_RUNTIME_DIR still points to root user directory. In that
> + * case create a directory for user in /tmp/$UID
> + */
> +if (unprivileged) {
> +gchar *user_dir = NULL;
> +gboolean create_dir = false;
> +user_dir = g_strdup(g_get_user_runtime_dir());
> +if (!user_dir || g_str_has_suffix(user_dir, "/0")) {
> +user_dir = g_strdup_printf("/tmp/%d", geteuid());
> +create_dir = true;
> +}

As above, I don't think we need to have this fallback code to deal
with something that is just user error.

Also, g_get_user_runtime_dir() is guaranteed to return non-NULL.

> +
> +if (create_dir && g_mkdir_with_parents(user_dir, S_IRWXU) < 0) {
> +fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
> + __func__, user_dir, strerror(errno));
> +g_free(user_dir);
> +return false;
> +}
> +dir = g_strdup(user_dir);

Don't we also want to be appending "virtiofsd" to this directory path
like we do in the privileged case ?


> +g_free(user_dir);
> +} else {
> +dir = qemu_get_local_state_pathname("run/virtiofsd");
> +if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {
> +fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
> + __func__, dir, strerror(errno));
> +return false;
> +}
>  }
>  
>  sk_name = g_strdup(se->vu_socket_path);
> -- 
> 2.25.4
> 

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] schemas: Add vim modeline

[Markus Armbruster]

Andrea Bolognani  writes:

> The various schemas included in QEMU use a JSON-based format which
> is, however, strictly speaking not valid JSON.
>
> As a consequence, when vim tries to apply syntax highlight rules
> for JSON (as guessed from the file name), the result is an unreadable
> mess which mostly consist of red markers pointing out supposed errors
> in, well, pretty much everything.
>
> Using Python syntax highlighting produces much better results, and
> in fact these files already start with specially-formatted comments
> that instruct Emacs to process them as if they were Python files.
>
> This commit adds the equivalent special comments for vim.
>
> Signed-off-by: Andrea Bolognani 

Naming QAPI schema files .json even though their contents isn't was a
mistake.  Correcting it would be a pain.  If we correct it, then the
sooner the better.

Renaming them to .py gives decent editor support out of the box.  Their
contents isn't quite Python, though: true vs. True, false vs. False.  Do
we care?  Only a few dozen occurences; they could be adjusted.

Renaming them to .qapi would perhaps be less confusing, for the price of
"out of the box".

Thoughts?

[PATCH] qapi: Delete unwanted indentation of top-level expressions

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 qapi/block-core.json | 24 
 qapi/ui.json |  4 ++--
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab7bf3c612..bdcc8e5f9f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1847,8 +1847,8 @@
 #
 # Since: 4.0
 ##
-  { 'enum': 'BlockPermission',
-'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize',
+{ 'enum': 'BlockPermission',
+  'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize',
   'graph-mod' ] }
 ##
 # @XDbgBlockGraphEdge:
@@ -2155,8 +2155,8 @@
 # <- { "return": {} }
 #
 ##
-  { 'command': 'block-dirty-bitmap-enable',
-'data': 'BlockDirtyBitmap' }
+{ 'command': 'block-dirty-bitmap-enable',
+  'data': 'BlockDirtyBitmap' }
 
 ##
 # @block-dirty-bitmap-disable:
@@ -2176,8 +2176,8 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'block-dirty-bitmap-disable',
-  'data': 'BlockDirtyBitmap' }
+{ 'command': 'block-dirty-bitmap-disable',
+  'data': 'BlockDirtyBitmap' }
 
 ##
 # @block-dirty-bitmap-merge:
@@ -2208,8 +2208,8 @@
 # <- { "return": {} }
 #
 ##
-  { 'command': 'block-dirty-bitmap-merge',
-'data': 'BlockDirtyBitmapMerge' }
+{ 'command': 'block-dirty-bitmap-merge',
+  'data': 'BlockDirtyBitmapMerge' }
 
 ##
 # @BlockDirtyBitmapSha256:
@@ -2220,8 +2220,8 @@
 #
 # Since: 2.10
 ##
-  { 'struct': 'BlockDirtyBitmapSha256',
-'data': {'sha256': 'str'} }
+{ 'struct': 'BlockDirtyBitmapSha256',
+  'data': {'sha256': 'str'} }
 
 ##
 # @x-debug-block-dirty-bitmap-sha256:
@@ -2235,8 +2235,8 @@
 #
 # Since: 2.10
 ##
-  { 'command': 'x-debug-block-dirty-bitmap-sha256',
-'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' }
+{ 'command': 'x-debug-block-dirty-bitmap-sha256',
+  'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' }
 
 ##
 # @blockdev-mirror:
diff --git a/qapi/ui.json b/qapi/ui.json
index e16e98a060..1568cfeaad 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1081,8 +1081,8 @@
  # Since: 3.0
  #
  ##
- { 'enum': 'DisplayGLMode',
-   'data': [ 'off', 'on', 'core', 'es' ] }
+{ 'enum': 'DisplayGLMode',
+  'data': [ 'off', 'on', 'core', 'es' ] }
 
 ##
 # @DisplayCurses:
-- 
2.26.2

Re: [PATCH for-5.1] qapi/machine.json: Fix missing newline in doc comment

[Markus Armbruster]

Peter Maydell  writes:

> In commit 176d2cda0dee9f4 we added the @die-id field
> to the CpuInstanceProperties struct, but in the process
> accidentally removed the newline between the doc-comment
> lines for @core-id and @thread-id.
>
> Put the newline back in; this fixes a misformatting in the
> generated HTML QMP reference manual.
>
> Signed-off-by: Peter Maydell 
> ---
> Not very important but I've suggested for-5.1 as it's a safe
> docs fix. You can see the misrendered doc at
> https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#index-CpuInstanceProperties

Makes sense.

Queued, thanks!

Re: [PATCH] schemas: Add vim modeline

[Daniel P . Berrangé]

On Thu, Jul 30, 2020 at 11:07:26AM +0200, Markus Armbruster wrote:
> Andrea Bolognani  writes:
> 
> > The various schemas included in QEMU use a JSON-based format which
> > is, however, strictly speaking not valid JSON.
> >
> > As a consequence, when vim tries to apply syntax highlight rules
> > for JSON (as guessed from the file name), the result is an unreadable
> > mess which mostly consist of red markers pointing out supposed errors
> > in, well, pretty much everything.
> >
> > Using Python syntax highlighting produces much better results, and
> > in fact these files already start with specially-formatted comments
> > that instruct Emacs to process them as if they were Python files.
> >
> > This commit adds the equivalent special comments for vim.
> >
> > Signed-off-by: Andrea Bolognani 

Given that we already have emacs mode-lines, I see no reason to
not also have vim mode-lines, so regardless of the deeper discussion
I think this is patch is fine to merge in the short term

  Reviewed-by: Daniel P. Berrangé 


> Naming QAPI schema files .json even though their contents isn't was a
> mistake.  Correcting it would be a pain.  If we correct it, then the
> sooner the better.
> 
> Renaming them to .py gives decent editor support out of the box.  Their
> contents isn't quite Python, though: true vs. True, false vs. False.  Do
> we care?  Only a few dozen occurences; they could be adjusted.
> 
> Renaming them to .qapi would perhaps be less confusing, for the price of
> "out of the box".

IMHO, the critical rule is that if you a pick a particular file extension
associated with an existing language, you absolutely MUST BE compliant
with that language.

We fail at compliance with both JSON and Python because we're actually
using our own DSL (domain specific language).

IOW if we're going to stick with our current file format, then we should
be naming them .qapi. We can still use an editor mode line if we want to
claim we're approximately equiv to another language, but we can't be
surprised if editors get upset.


The bigger question is whether having our own DSL is justified ?

I'm *really* sceptical that it is.


We can't use JSON because it lacks comments. So we invented our own
psuedo-JSON parser that supported comments, and used ' instead of "
for some reason. We also needed to be able to parse a sequence of
multiple JSON documents in one file. We should have just picked a 
different language because JSON clearly didn't do what we eneeded.

You suggest naming them .py. If we do that, we must declare that they
are literally Python code and modify them so that we can load the 
files straight into the python intepretor as code, and not parse 
them as data. I feel unhappy about treating data as code though.


While JSON doesn't do what we need, its second-cousin YAML is a more
flexible format. Taking one example

---
##
# @ImageInfoSpecificQCow2:
#
# @compat: compatibility level
#
# ...snip...
#
# Since: 1.7
##
struct: ImageInfoSpecificQCow2
data:
  compat: str
  "*data-file": str
  "*data-file-raw": bool
  "*lazy-refcounts": bool
  "*corrupt": bool
  refcount-bits: int
  "*encrypt": ImageInfoSpecificQCow2Encryption
  "*bitmaps":
- Qcow2BitmapInfo
  compression-type: Qcow2CompressionType


Then we could use a regular off the shelf YAML parser in python.

The uglyiness with quotes is due to the use of "*". Slightly less ugly
if we simply declare that quotes are always used, even where they're
not strictly required.

struct: ImageInfoSpecificQCow2
data:
  "compat": "str"
  "*data-file": "str"
  "*data-file-raw": "bool"
  "*lazy-refcounts": "bool"
  "*corrupt": "bool"
  "refcount-bits": "int"
  "*encrypt": "ImageInfoSpecificQCow2Encryption"
  "*bitmaps":
- "Qcow2BitmapInfo"
  "compression-type": "Qcow2CompressionType"

With the use of "---" to denote the start of document, we have no trouble 
parsing our files which would actually be a concatenation of multiple 
documents. The python YAML library provides the easy yaml.load_all()
method.


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: [RFC PATCH v3 8/8] target/s390x: Use start-powered-off CPUState property

[Cornelia Huck]

On Tue, 28 Jul 2020 21:51:33 -0300
Thiago Jung Bauermann  wrote:

> Hi,
> 
> Cornelia Huck  writes:
> 
> > On Wed, 22 Jul 2020 23:56:57 -0300
> > Thiago Jung Bauermann  wrote:
> >  
> >> Instead of setting CPUState::halted to 1 in s390_cpu_initfn(), use the
> >> start-powered-off property which makes cpu_common_reset() initialize it
> >> to 1 in common code.
> >> 
> >> Note that this changes behavior by setting cs->halted to 1 on reset, which
> >> didn't happen before.  
> >
> > I think that should be fine, as we change the cpu state to STOPPED in
> > the reset function, which sets halted to 1.  
> 
> Nice, thanks for checking.
> 
> >> 
> >> Signed-off-by: Thiago Jung Bauermann 
> >> ---
> >>  target/s390x/cpu.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> NB: I was only able to test that this patch builds. I wasn't able to
> >> run it.  
> >
> > No noticeable difference under kvm, but running under tcg seems a bit
> > more sluggish than usual, and I saw some pausing on reboot (after the
> > bios handover to the kernel). Not sure if it were just flukes on my
> > laptop, would appreciate if someone else could give it a go.  

Experimented a bit with it again. There's a pause when switching from
the bios to the kernel (after the load reset normal has been done, I
guess), which is always there, but seems to get more noticeable with
this patch (varying wildly, but seems longer on average.) Hard to pin
down, and I don't really see a reason why that should happen, as we
should end up with halted == 1 in any case. Might still be a fluke,
even though I see it both on my laptop and on an LPAR (when running
under tcg; not seen under kvm, which is much faster anyway.)

> 
> I tried today setting up a TCG guest, but didn't have success yet.
> Will try some more tomorrow.
> 

I'm also looking a bit at the other s390 folks :)

[PATCH 2/3] softfloat: add APIs to handle alternative sNaN propagation

[Chih-Min Chao]

For "fmax/fmin ft0, ft1, ft2" and if one of the inputs is sNaN,
  The original logic
return NaN and set invalid flag if ft1 == sNaN || ft2 == sNan

  The alternative path
set invalid flag if ft1 == sNaN || ft2 == sNaN
return NaN if ft1 == sNaN && ft2 == sNaN

   The ieee754 spec allows both implementation and some architecture such
   as riscv choose differenct defintion in two spec versions.
   (riscv-spec-v2.2 use original version, riscv-spec-20191213 changes to
alternative)

Signed-off-by: Chih-Min Chao 
---
 fpu/softfloat.c | 75 +++--
 include/fpu/softfloat.h |  6 
 2 files changed, 54 insertions(+), 27 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 79be4f5..4466ece 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -870,11 +870,16 @@ static FloatParts return_nan(FloatParts a, float_status 
*s)
 return a;
 }
 
-static FloatParts pick_nan(FloatParts a, FloatParts b, float_status *s)
+static void set_snan_flag(FloatParts a, FloatParts b, float_status *s)
 {
 if (is_snan(a.cls) || is_snan(b.cls)) {
 s->float_exception_flags |= float_flag_invalid;
 }
+}
+
+static FloatParts pick_nan(FloatParts a, FloatParts b, float_status *s)
+{
+set_snan_flag(a, b, s);
 
 if (s->default_nan_mode) {
 return parts_default_nan(s);
@@ -2743,23 +2748,32 @@ float64 uint16_to_float64(uint16_t a, float_status 
*status)
  * and minNumMag() from the IEEE-754 2008.
  */
 static FloatParts minmax_floats(FloatParts a, FloatParts b, bool ismin,
-bool ieee, bool ismag, float_status *s)
+bool ieee, bool ismag, bool issnan_prop,
+float_status *s)
 {
 if (unlikely(is_nan(a.cls) || is_nan(b.cls))) {
 if (ieee) {
 /* Takes two floating-point values `a' and `b', one of
  * which is a NaN, and returns the appropriate NaN
  * result. If either `a' or `b' is a signaling NaN,
- * the invalid exception is raised.
+ * the invalid exception is raised but the NaN
+ * propagation is 'shall'.
  */
 if (is_snan(a.cls) || is_snan(b.cls)) {
-return pick_nan(a, b, s);
-} else if (is_nan(a.cls) && !is_nan(b.cls)) {
+if (issnan_prop) {
+return pick_nan(a, b, s);
+} else {
+set_snan_flag(a, b, s);
+}
+}
+
+if (is_nan(a.cls) && !is_nan(b.cls)) {
 return b;
 } else if (is_nan(b.cls) && !is_nan(a.cls)) {
 return a;
 }
 }
+
 return pick_nan(a, b, s);
 } else {
 int a_exp, b_exp;
@@ -2813,37 +2827,44 @@ static FloatParts minmax_floats(FloatParts a, 
FloatParts b, bool ismin,
 }
 }
 
-#define MINMAX(sz, name, ismin, isiee, ismag)   \
+#define MINMAX(sz, name, ismin, isiee, ismag, issnan_prop)  \
 float ## sz float ## sz ## _ ## name(float ## sz a, float ## sz b,  \
  float_status *s)   \
 {   \
 FloatParts pa = float ## sz ## _unpack_canonical(a, s); \
 FloatParts pb = float ## sz ## _unpack_canonical(b, s); \
-FloatParts pr = minmax_floats(pa, pb, ismin, isiee, ismag, s);  \
+FloatParts pr = minmax_floats(pa, pb, ismin, isiee, ismag,  \
+  issnan_prop, s);  \
 \
 return float ## sz ## _round_pack_canonical(pr, s); \
 }
 
-MINMAX(16, min, true, false, false)
-MINMAX(16, minnum, true, true, false)
-MINMAX(16, minnummag, true, true, true)
-MINMAX(16, max, false, false, false)
-MINMAX(16, maxnum, false, true, false)
-MINMAX(16, maxnummag, false, true, true)
-
-MINMAX(32, min, true, false, false)
-MINMAX(32, minnum, true, true, false)
-MINMAX(32, minnummag, true, true, true)
-MINMAX(32, max, false, false, false)
-MINMAX(32, maxnum, false, true, false)
-MINMAX(32, maxnummag, false, true, true)
-
-MINMAX(64, min, true, false, false)
-MINMAX(64, minnum, true, true, false)
-MINMAX(64, minnummag, true, true, true)
-MINMAX(64, max, false, false, false)
-MINMAX(64, maxnum, false, true, false)
-MINMAX(64, maxnummag, false, true, true)
+MINMAX(16, min, true, false, false, true)
+MINMAX(16, minnum, true, true, false, true)
+MINMAX(16, minnum_noprop, true, true, false, false)
+MINMAX(16, minnummag, true, true, true, true)
+MINMAX(16, max, false, false, false, true)
+MINMAX(16, maxnum, false, true, false, true)
+MINMAX(16, maxnum_noprop, false, true, false, false)
+MINMAX(16, maxnummag, false, true, true, true)
+
+MINMAX(32, min, true, false, false, true)
+MINMAX(32, minnum, tru

[PATCH 0/3] float16 APIs and alternative sNaN handling

[Chih-Min Chao]

These patches are separated from riscv vector extension 0.9 patchset.
The set includes 
   1. alternative NaN handlding
   2. float16 comparision APIs.
   3. float16 to int8/uint8 conversion APIs

Chih-Min Chao (1):
  softfloat: add APIs to handle alternative sNaN propagation

Frank Chang (1):
  softfloat: add fp16 and uint8/int8 interconvert functions

Kito Cheng (1):
  softfloat: target/riscv: implement full set fp16 comparision

 fpu/softfloat.c  | 109 ---
 include/fpu/softfloat.h  |  55 ++
 target/riscv/vector_helper.c |  25 --
 3 files changed, 137 insertions(+), 52 deletions(-)

-- 
2.7.4

[PATCH 1/3] softfloat: target/riscv: implement full set fp16 comparision

[Chih-Min Chao]

From: Kito Cheng 

Implement them in softfloat and remove local version in riscv

Signed-off-by: Kito Cheng 
Signed-off-by: Chih-Min Chao 
Acked-by: Alex Bennée 
---
 include/fpu/softfloat.h  | 41 +
 target/riscv/vector_helper.c | 25 -
 2 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 659218b..573fce9 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -285,6 +285,47 @@ static inline float16 float16_set_sign(float16 a, int sign)
 return make_float16((float16_val(a) & 0x7fff) | (sign << 15));
 }
 
+static inline bool float16_eq(float16 a, float16 b, float_status *s)
+{
+return float16_compare(a, b, s) == float_relation_equal;
+}
+
+static inline bool float16_le(float16 a, float16 b, float_status *s)
+{
+return float16_compare(a, b, s) <= float_relation_equal;
+}
+
+static inline bool float16_lt(float16 a, float16 b, float_status *s)
+{
+return float16_compare(a, b, s) < float_relation_equal;
+}
+
+static inline bool float16_unordered(float16 a, float16 b, float_status *s)
+{
+return float16_compare(a, b, s) == float_relation_unordered;
+}
+
+static inline bool float16_eq_quiet(float16 a, float16 b, float_status *s)
+{
+return float16_compare_quiet(a, b, s) == float_relation_equal;
+}
+
+static inline bool float16_le_quiet(float16 a, float16 b, float_status *s)
+{
+return float16_compare_quiet(a, b, s) <= float_relation_equal;
+}
+
+static inline bool float16_lt_quiet(float16 a, float16 b, float_status *s)
+{
+return float16_compare_quiet(a, b, s) < float_relation_equal;
+}
+
+static inline bool float16_unordered_quiet(float16 a, float16 b,
+   float_status *s)
+{
+return float16_compare_quiet(a, b, s) == float_relation_unordered;
+}
+
 #define float16_zero make_float16(0)
 #define float16_half make_float16(0x3800)
 #define float16_one make_float16(0x3c00)
diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index 39f44d1..c68e6c4 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -3955,12 +3955,6 @@ void HELPER(NAME)(void *vd, void *v0, void *vs1, void 
*vs2,   \
 } \
 }
 
-static bool float16_eq_quiet(uint16_t a, uint16_t b, float_status *s)
-{
-FloatRelation compare = float16_compare_quiet(a, b, s);
-return compare == float_relation_equal;
-}
-
 GEN_VEXT_CMP_VV_ENV(vmfeq_vv_h, uint16_t, H2, float16_eq_quiet)
 GEN_VEXT_CMP_VV_ENV(vmfeq_vv_w, uint32_t, H4, float32_eq_quiet)
 GEN_VEXT_CMP_VV_ENV(vmfeq_vv_d, uint64_t, H8, float64_eq_quiet)
@@ -4017,12 +4011,6 @@ GEN_VEXT_CMP_VF(vmfne_vf_h, uint16_t, H2, vmfne16)
 GEN_VEXT_CMP_VF(vmfne_vf_w, uint32_t, H4, vmfne32)
 GEN_VEXT_CMP_VF(vmfne_vf_d, uint64_t, H8, vmfne64)
 
-static bool float16_lt(uint16_t a, uint16_t b, float_status *s)
-{
-FloatRelation compare = float16_compare(a, b, s);
-return compare == float_relation_less;
-}
-
 GEN_VEXT_CMP_VV_ENV(vmflt_vv_h, uint16_t, H2, float16_lt)
 GEN_VEXT_CMP_VV_ENV(vmflt_vv_w, uint32_t, H4, float32_lt)
 GEN_VEXT_CMP_VV_ENV(vmflt_vv_d, uint64_t, H8, float64_lt)
@@ -4030,13 +4018,6 @@ GEN_VEXT_CMP_VF(vmflt_vf_h, uint16_t, H2, float16_lt)
 GEN_VEXT_CMP_VF(vmflt_vf_w, uint32_t, H4, float32_lt)
 GEN_VEXT_CMP_VF(vmflt_vf_d, uint64_t, H8, float64_lt)
 
-static bool float16_le(uint16_t a, uint16_t b, float_status *s)
-{
-FloatRelation compare = float16_compare(a, b, s);
-return compare == float_relation_less ||
-   compare == float_relation_equal;
-}
-
 GEN_VEXT_CMP_VV_ENV(vmfle_vv_h, uint16_t, H2, float16_le)
 GEN_VEXT_CMP_VV_ENV(vmfle_vv_w, uint32_t, H4, float32_le)
 GEN_VEXT_CMP_VV_ENV(vmfle_vv_d, uint64_t, H8, float64_le)
@@ -4091,12 +4072,6 @@ GEN_VEXT_CMP_VF(vmfge_vf_h, uint16_t, H2, vmfge16)
 GEN_VEXT_CMP_VF(vmfge_vf_w, uint32_t, H4, vmfge32)
 GEN_VEXT_CMP_VF(vmfge_vf_d, uint64_t, H8, vmfge64)
 
-static bool float16_unordered_quiet(uint16_t a, uint16_t b, float_status *s)
-{
-FloatRelation compare = float16_compare_quiet(a, b, s);
-return compare == float_relation_unordered;
-}
-
 GEN_VEXT_CMP_VV_ENV(vmford_vv_h, uint16_t, H2, !float16_unordered_quiet)
 GEN_VEXT_CMP_VV_ENV(vmford_vv_w, uint32_t, H4, !float32_unordered_quiet)
 GEN_VEXT_CMP_VV_ENV(vmford_vv_d, uint64_t, H8, !float64_unordered_quiet)
-- 
2.7.4

[PATCH 3/3] softfloat: add fp16 and uint8/int8 interconvert functions

[Chih-Min Chao]

From: Frank Chang 

Signed-off-by: Frank Chang 
Reviewed-by: Alex Bennée 
---
 fpu/softfloat.c | 34 ++
 include/fpu/softfloat.h |  8 
 2 files changed, 42 insertions(+)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 4466ece..c700a39 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -2114,6 +2114,13 @@ static int64_t round_to_int_and_pack(FloatParts in, 
FloatRoundMode rmode,
 }
 }
 
+int8_t float16_to_int8_scalbn(float16 a, FloatRoundMode rmode, int scale,
+  float_status *s)
+{
+return round_to_int_and_pack(float16_unpack_canonical(a, s),
+ rmode, scale, INT8_MIN, INT8_MAX, s);
+}
+
 int16_t float16_to_int16_scalbn(float16 a, FloatRoundMode rmode, int scale,
 float_status *s)
 {
@@ -2177,6 +2184,11 @@ int64_t float64_to_int64_scalbn(float64 a, 
FloatRoundMode rmode, int scale,
  rmode, scale, INT64_MIN, INT64_MAX, s);
 }
 
+int8_t float16_to_int8(float16 a, float_status *s)
+{
+return float16_to_int8_scalbn(a, s->float_rounding_mode, 0, s);
+}
+
 int16_t float16_to_int16(float16 a, float_status *s)
 {
 return float16_to_int16_scalbn(a, s->float_rounding_mode, 0, s);
@@ -2327,6 +2339,13 @@ static uint64_t round_to_uint_and_pack(FloatParts in, 
FloatRoundMode rmode,
 }
 }
 
+uint8_t float16_to_uint8_scalbn(float16 a, FloatRoundMode rmode, int scale,
+float_status *s)
+{
+return round_to_uint_and_pack(float16_unpack_canonical(a, s),
+  rmode, scale, UINT8_MAX, s);
+}
+
 uint16_t float16_to_uint16_scalbn(float16 a, FloatRoundMode rmode, int scale,
   float_status *s)
 {
@@ -2390,6 +2409,11 @@ uint64_t float64_to_uint64_scalbn(float64 a, 
FloatRoundMode rmode, int scale,
   rmode, scale, UINT64_MAX, s);
 }
 
+uint8_t float16_to_uint8(float16 a, float_status *s)
+{
+return float16_to_uint8_scalbn(a, s->float_rounding_mode, 0, s);
+}
+
 uint16_t float16_to_uint16(float16 a, float_status *s)
 {
 return float16_to_uint16_scalbn(a, s->float_rounding_mode, 0, s);
@@ -2544,6 +2568,11 @@ float16 int16_to_float16(int16_t a, float_status *status)
 return int64_to_float16_scalbn(a, 0, status);
 }
 
+float16 int8_to_float16(int8_t a, float_status *status)
+{
+return int64_to_float16_scalbn(a, 0, status);
+}
+
 float32 int64_to_float32_scalbn(int64_t a, int scale, float_status *status)
 {
 FloatParts pa = int_to_float(a, scale, status);
@@ -2669,6 +2698,11 @@ float16 uint16_to_float16(uint16_t a, float_status 
*status)
 return uint64_to_float16_scalbn(a, 0, status);
 }
 
+float16 uint8_to_float16(uint8_t a, float_status *status)
+{
+return uint64_to_float16_scalbn(a, 0, status);
+}
+
 float32 uint64_to_float32_scalbn(uint64_t a, int scale, float_status *status)
 {
 FloatParts pa = uint_to_float(a, scale, status);
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 65842b0..fec670a 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -136,9 +136,11 @@ float16 uint16_to_float16_scalbn(uint16_t a, int, 
float_status *status);
 float16 uint32_to_float16_scalbn(uint32_t a, int, float_status *status);
 float16 uint64_to_float16_scalbn(uint64_t a, int, float_status *status);
 
+float16 int8_to_float16(int8_t a, float_status *status);
 float16 int16_to_float16(int16_t a, float_status *status);
 float16 int32_to_float16(int32_t a, float_status *status);
 float16 int64_to_float16(int64_t a, float_status *status);
+float16 uint8_to_float16(uint8_t a, float_status *status);
 float16 uint16_to_float16(uint16_t a, float_status *status);
 float16 uint32_to_float16(uint32_t a, float_status *status);
 float16 uint64_to_float16(uint64_t a, float_status *status);
@@ -187,10 +189,13 @@ float32 float16_to_float32(float16, bool ieee, 
float_status *status);
 float16 float64_to_float16(float64 a, bool ieee, float_status *status);
 float64 float16_to_float64(float16 a, bool ieee, float_status *status);
 
+int8_t  float16_to_int8_scalbn(float16, FloatRoundMode, int,
+   float_status *status);
 int16_t float16_to_int16_scalbn(float16, FloatRoundMode, int, float_status *);
 int32_t float16_to_int32_scalbn(float16, FloatRoundMode, int, float_status *);
 int64_t float16_to_int64_scalbn(float16, FloatRoundMode, int, float_status *);
 
+int8_t  float16_to_int8(float16, float_status *status);
 int16_t float16_to_int16(float16, float_status *status);
 int32_t float16_to_int32(float16, float_status *status);
 int64_t float16_to_int64(float16, float_status *status);
@@ -199,6 +204,8 @@ int16_t float16_to_int16_round_to_zero(float16, 
float_status *status);
 int32_t float16_to_int32_round_to_zero(float16, float_status *status);
 int64_t float16_to_int64_round_to_zero(float16, float_status *status);
 
+uint8_t float16_to_uint8_scalbn(floa

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Paolo Bonzini  writes:

> On 29/07/20 15:18, Markus Armbruster wrote:
>>> Even code riddled by backwards-compatibility special cases, such as
>>> -accel and -machine, can share code between themselves and -object to
>>> some extent; this is thanks to functions such as object_property_parse,
>>> whose parsing is deferred to visitors and hence to QAPI.
>>
>> QOM relies on QAPI visitors to access properties.  There is no
>> integration with the QAPI schema.
>
> Indeed it doesn't use _all_ of the QAPI goodies.  It does use visitors
> and it's a major feature of QOM.

No argument.

>> Going through a visitor enables property access from QMP, HMP and CLI.
>> 
>> Access from C *also* goes through a visitor.  We typically go from C
>> type to QObject and back.  Comically inefficient (which hardly matters),
>> verbose to use and somewhat hard to understand (which does).
>
> It's verbose in the getters/setters, but we have wrappers such as
> object_property_set_str, object_property_set_bool etc. that do not make
> it too hard to understand.

qdev C layer:

frob->prop = 42;

Least cognitive load.

QOM has no C layer.

qdev property layer works even when @frob has incomplete type:

qdev_prop_set_int32(DEVICE(frob), "prop", 42);

This used to map property name to struct offset & copy the value.
Simple, stupid.

Nowadays, it is the same as

object_property_set_int(OBJECT(frob), "frob", 42, &error_abort);

which first converts the int to a QObject, then uses a QObject input
visitor with a virtual walk to convert it back to int and store it in
@frob.  It's quite a sight in the debugger.

qdev "text" layer is really a QemuOpts layer (because that's what we had
back then).  If we have prop=42 in a QemuOpts, it calls

set_property("prop", "42", frob, &err);

Nowadays, this is a thin wrapper around object_property_parse(),
basically

object_property_parse(frob, "prop", 42, &err);

Fine print: except set_property() does nothing for @prop "driver" and
"bus", which look just like properties in -device / device-add, but
aren't.

object_property_parse() uses the string input visitor, which I loathe.

>> Compare to what QOM replaced: qdev.  Properties are a layer on top of
>> ordinary C.  From C, you can either use the C layer (struct members,
>> basically), or the property layer for C (functions taking C types, no
>> conversion to string and back under the hood), or the "text" layer
>> (parse from text / format to text).
>> 
>> My point is not that qdev was great and QOM is terrible.  There are
>> reasons we replaced qdev with QOM.  My point is QOM doesn't *have* to be
>> the way it is.  It is the way it is because we made it so.
>
> QOM didn't only replace qdev: it also removed the need to have a command
> line option du jour for any new concept, e.g. all the TLS stuff, RNG
> backends, RAM backends, etc.

Yes.  There are good reasons for QOM.

> It didn't succeed (at all) in deprecating chardev/netdev/device etc.,
> but this is a very underappreciated part of QOM, and this is why I think
> it's appropriate to say QOM is "C with classes and CLI/RPC
> serialization", as opposed for example to "C with classes and multi
> programming language interface" that is GObject.

That's fair.

>> I've long had the nagging feeling that if we had special-cased
>> containers, children and links, we could have made a QOM that was easier
>> to reason about, and much easier to integrate with a QAPI schema.
>
> That's at least plausible.  But I have a nagging feeling that it would
> only cover 99% of what we're doing with QOM. :)

The question is whether that 1% really should be done the way it is done
:)

Re: [PATCH v8 4/7] 9pfs: add new function v9fs_co_readdir_many()

[Christian Schoenebeck]

On Mittwoch, 29. Juli 2020 10:12:33 CEST Christian Schoenebeck wrote:
> The newly added function v9fs_co_readdir_many() retrieves multiple
> directory entries with a single fs driver request. It is intended to
> replace uses of v9fs_co_readdir(), the latter only retrives a single
> directory entry per fs driver request instead.
> 
> The reason for this planned replacement is that for every fs driver
> request the coroutine is dispatched from main I/O thread to a
> background I/O thread and eventually dispatched back to main I/O
> thread. Hopping between threads adds latency. So if a 9pfs Treaddir
> request reads a large amount of directory entries, this currently
> sums up to huge latencies of several hundred ms or even more. So
> using v9fs_co_readdir_many() instead of v9fs_co_readdir() will
> provide significant performance improvements.
> 
> Signed-off-by: Christian Schoenebeck 
> ---
>  hw/9pfs/9p.h|  22 +++
>  hw/9pfs/codir.c | 165 
>  hw/9pfs/coth.h  |   3 +
>  3 files changed, 190 insertions(+)
> 
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 561774e843..93b7030edf 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -215,6 +215,28 @@ static inline void v9fs_readdir_init(V9fsDir *dir)
>  qemu_co_mutex_init(&dir->readdir_mutex);
>  }
> 
> +/**
> + * Type for 9p fs drivers' (a.k.a. 9p backends) result of readdir requests,
> + * which is a chained list of directory entries.
> + */
> +typedef struct V9fsDirEnt {
> +/* mandatory (must not be NULL) information for all readdir requests */
> +struct dirent *dent;
> +/*
> + * optional (may be NULL): A full stat of each directory entry is just
> + * done if explicitly told to fs driver.
> + */
> +struct stat *st;
> +/*
> + * instead of an array, directory entries are always returned as
> + * chained list, that's because the amount of entries retrieved by fs
> + * drivers is dependent on the individual entries' name (since response
> + * messages are size limited), so the final amount cannot be estimated
> + * before hand
> + */
> +struct V9fsDirEnt *next;
> +} V9fsDirEnt;
> +
>  /*
>   * Filled by fs driver on open and other
>   * calls.
> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> index ff57fb8619..8a8128ea13 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -38,6 +38,10 @@ static int do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
> struct dirent **dent) return err;
>  }
> 
> +/*
> + * TODO: This will be removed for performance reasons.
> + * Use v9fs_co_readdir_many() instead.
> + */
>  int coroutine_fn v9fs_co_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>   struct dirent **dent)
>  {
> @@ -52,6 +56,167 @@ int coroutine_fn v9fs_co_readdir(V9fsPDU *pdu,
> V9fsFidState *fidp, return err;
>  }
> 
> +/*
> + * This is solely executed on a background IO thread.
> + *
> + * See v9fs_co_readdir_many() (as its only user) below for details.
> + */
> +static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
> +   struct V9fsDirEnt **entries, off_t offset,
> +   int32_t maxsize, bool dostat)
> +{
> +V9fsState *s = pdu->s;
> +V9fsString name;
> +int len, err = 0;
> +int32_t size = 0;
> +off_t saved_dir_pos;
> +struct dirent *dent;
> +struct V9fsDirEnt *e = NULL;
> +V9fsPath path;
> +struct stat stbuf;
> +
> +*entries = NULL;
> +v9fs_path_init(&path);
> +
> +/*
> + * TODO: Here should be a warn_report_once() if lock failed.
> + *
> + * With a good 9p client we should not get into concurrency here,
> + * because a good client would not use the same fid for concurrent
> + * requests. We do the lock here for safety reasons though. However
> + * the client would then suffer performance issues, so better log that
> + * issue here.
> + */
> +v9fs_readdir_lock(&fidp->fs.dir);
> +
> +/* seek directory to requested initial position */
> +if (offset == 0) {
> +s->ops->rewinddir(&s->ctx, &fidp->fs);
> +} else {
> +s->ops->seekdir(&s->ctx, &fidp->fs, offset);
> +}
> +
> +/* save the directory position */
> +saved_dir_pos = s->ops->telldir(&s->ctx, &fidp->fs);
> +if (saved_dir_pos < 0) {
> +err = saved_dir_pos;
> +goto out;
> +}
> +
> +while (true) {

To address your concern about aborting a readdir request, one could add here:

if (v9fs_request_cancelled(pdu)) {
err = -EINTR;
break;
}

That's a minimal invasive change, so it would be Ok to add it to this patch 
already, or at any time later as a separate patch in future, as you like.

> +/* get directory entry from fs driver */
> +err = do_readdir(pdu, fidp, &dent);
> +if (err || !dent) {
> +break;
> +}
> +
> +/*
> + * stop this loop as soon as it would exceed the all

Re: [PATCH v2 01/16] hw/block/nvme: memset preallocated requests structures

[Minwoo Im]

On Thu, Jul 30, 2020 at 7:06 AM Klaus Jensen  wrote:
>
> From: Klaus Jensen 
>
> This is preparatory to subsequent patches that change how QSGs/IOVs are
> handled. It is important that the qsg and iov members of the NvmeRequest
> are initially zeroed.
>
> Signed-off-by: Klaus Jensen 
> Reviewed-by: Maxim Levitsky 

Reviewed-by: Minwoo Im 

Re: [PATCH v2 04/16] hw/block/nvme: remove redundant has_sg member

[Minwoo Im]

On Thu, Jul 30, 2020 at 7:06 AM Klaus Jensen  wrote:
>
> From: Klaus Jensen 
>
> Remove the has_sg member from NvmeRequest since it's redundant.
>
> Signed-off-by: Klaus Jensen 

Looks better than the previous one to me.

Reviewed-by: Minwoo Im 

Re: [PATCH v2 05/16] hw/block/nvme: destroy request iov before reuse

[Minwoo Im]

On Thu, Jul 30, 2020 at 7:06 AM Klaus Jensen  wrote:
>
> From: Klaus Jensen 
>
> Make sure the request iov is destroyed before reuse; fixing a memory
> leak.
>
> Signed-off-by: Klaus Jensen 

Looks good to me and Thanks for splitting this up.

Reviewed-by: Minwoo Im 

Re: [PATCH v2 07/16] hw/block/nvme: add tracing to nvme_map_prp

[Minwoo Im]

On Thu, Jul 30, 2020 at 7:06 AM Klaus Jensen  wrote:
>
> From: Klaus Jensen 
>
> Add tracing to nvme_map_prp.
>
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c   | 2 ++
>  hw/block/trace-events | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 571635ebe9f9..952afbb05175 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -274,6 +274,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, 
> QEMUIOVector *iov, uint64_t prp1,
>  int num_prps = (len >> n->page_bits) + 1;
>  uint16_t status;
>
> +trace_pci_nvme_map_prp(trans_len, len, prp1, prp2, num_prps);

Hmm.. Okay with this.  But once QEMUSGList and QEMUIOVector instances are coming
into the NvmeRequest, we just can pass the NvmeRequest instance here
and print the cid as well
later :)

Reviewed-by: Minwoo Im 

Thanks!

> +
>  if (unlikely(!prp1)) {
>  trace_pci_nvme_err_invalid_prp();
>  return NVME_INVALID_FIELD | NVME_DNR;
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index f3b2d004e078..f20c59a4b542 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -35,6 +35,7 @@ pci_nvme_irq_masked(void) "IRQ is masked"
>  pci_nvme_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64" 
> prp2=0x%"PRIx64""
>  pci_nvme_map_addr(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len 
> %"PRIu64""
>  pci_nvme_map_addr_cmb(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len 
> %"PRIu64""
> +pci_nvme_map_prp(uint64_t trans_len, uint32_t len, uint64_t prp1, uint64_t 
> prp2, int num_prps) "trans_len %"PRIu64" len %"PRIu32" prp1 0x%"PRIx64" prp2 
> 0x%"PRIx64" num_prps %d"
>  pci_nvme_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode) 
> "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
>  pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t opcode) "cid 
> %"PRIu16" sqid %"PRIu16" opc 0x%"PRIx8""
>  pci_nvme_rw(const char *verb, uint32_t blk_count, uint64_t byte_count, 
> uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
> --
> 2.27.0
>

Re: [PATCH v2 08/16] hw/block/nvme: add request mapping helper

[Minwoo Im]

On Thu, Jul 30, 2020 at 7:06 AM Klaus Jensen  wrote:
>
> From: Klaus Jensen 
>
> Introduce the nvme_map helper to remove some noise in the main nvme_rw
> function.
>
> Signed-off-by: Klaus Jensen 
> Reviewed-by: Maxim Levitsky 
> ---
>  hw/block/nvme.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 952afbb05175..198a26890e0c 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -415,6 +415,15 @@ static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, 
> uint32_t len,
>  return status;
>  }
>
> +static uint16_t nvme_map_dptr(NvmeCtrl *n, NvmeCmd *cmd, size_t len,
> + NvmeRequest *req)
> +{
> +uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
> +uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
> +
> +return nvme_map_prp(&req->qsg, &req->iov, prp1, prp2, len, n);
> +}

Let's do something for MPTR laster also when we are right in front of that.

Looks good to me.

Reviewed-by: Minwoo Im 

> +
>  static void nvme_post_cqes(void *opaque)
>  {
>  NvmeCQueue *cq = opaque;
> @@ -602,8 +611,6 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, 
> NvmeCmd *cmd,
>  NvmeRwCmd *rw = (NvmeRwCmd *)cmd;
>  uint32_t nlb  = le32_to_cpu(rw->nlb) + 1;
>  uint64_t slba = le64_to_cpu(rw->slba);
> -uint64_t prp1 = le64_to_cpu(rw->dptr.prp1);
> -uint64_t prp2 = le64_to_cpu(rw->dptr.prp2);
>
>  uint8_t lba_index  = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
>  uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
> @@ -620,7 +627,7 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, 
> NvmeCmd *cmd,
>  return NVME_LBA_RANGE | NVME_DNR;
>  }
>
> -if (nvme_map_prp(&req->qsg, &req->iov, prp1, prp2, data_size, n)) {
> +if (nvme_map_dptr(n, cmd, data_size, req)) {
>  block_acct_invalid(blk_get_stats(n->conf.blk), acct);
>  return NVME_INVALID_FIELD | NVME_DNR;
>  }
> --
> 2.27.0
>

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Daniel P . Berrangé]

On Wed, Jul 29, 2020 at 03:02:22PM +0200, Halil Pasic wrote:
> As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1)
> reads one past of the end of ms->loadparm, so g_memdup() can not be used
> here.
> 
> Let's use malloc and memcpy instead!
> 
> Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter")
> Fixes: Coverity CID 1431058
> Reported-by: Peter Maydell 
> Signed-off-by: Halil Pasic 
> ---
>  hw/s390x/s390-virtio-ccw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 403d30e13b..8b7bac0392 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -704,8 +704,8 @@ static char *machine_get_loadparm(Object *obj, Error 
> **errp)
>  char *loadparm_str;
>  
>  /* make a NUL-terminated string */
> -loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
> -loadparm_str[sizeof(ms->loadparm)] = 0;
> +loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1);
> +memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm));

I feel like  g_strndup(ms->loadparm, sizeof(ms->loadparm))
is what should have been used here.

It copies N characters, but allocates N+1 adding a trailing NUL
which are the semantic we wanted here.

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 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Cornelia Huck]

On Wed, 29 Jul 2020 15:02:22 +0200
Halil Pasic  wrote:

> As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1)
> reads one past of the end of ms->loadparm, so g_memdup() can not be used
> here.
> 
> Let's use malloc and memcpy instead!

Hm, an alternative would be to use g_strndup(). What do you think?

> 
> Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter")
> Fixes: Coverity CID 1431058
> Reported-by: Peter Maydell 
> Signed-off-by: Halil Pasic 
> ---
>  hw/s390x/s390-virtio-ccw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 403d30e13b..8b7bac0392 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -704,8 +704,8 @@ static char *machine_get_loadparm(Object *obj, Error 
> **errp)
>  char *loadparm_str;
>  
>  /* make a NUL-terminated string */
> -loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
> -loadparm_str[sizeof(ms->loadparm)] = 0;
> +loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1);
> +memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm));
>  return loadparm_str;
>  }
>  
> 
> base-commit: 5772f2b1fc5d00e7e04e01fa28e9081d6550440a

Re: [PATCH v2 2/3] trace: Add support for recorder back-end

[Christophe de Dinechin]

On 2020-07-30 at 10:13 CEST, Markus Armbruster wrote...
> Christophe de Dinechin  writes:
>
>> On 2020-07-29 at 13:53 CEST, Markus Armbruster wrote...
>>> Christophe de Dinechin  writes:
>>>
 On 2020-07-27 at 10:23 CEST, Markus Armbruster wrote...
> Christophe de Dinechin  writes:
>
>> On 2020-07-23 at 16:06 CEST, Markus Armbruster wrote...
>>> Christophe de Dinechin  writes:
>>> [...]
 I'm certainly not against adding a command-line option to activate 
 recorder
 options specifically, but as I understand, the option -trace already 
 exists,
 and its semantics is sufficiently different from the one in recorder
 patterns that I decided to not connect the two for now. For example, to
 disable trace foo, you'd pass "-foo" to the -trace option, but "foo=0" 
 to
 RECORDER_TRACES. The parsing of graphing options and other related
 recorder-specific stuff is a bit difficult to integrate with -trace 
 too.
>>>
>>> We need proper integration with the existing trace UI.
>>
>> I agree, but I don't think this belongs to this particular patch series.
>> See below why.
>>
>>>
>>> In particular, the ability to enable and disable trace events
>>> dynamically provided by QMP commands trace-event-get-state,
>>> trace-event-set-state, and HMP command trace-event is really useful.
>>
>> That ability exists, but given the many differences between the
>> recorder and other tracing mechanisms, I found it useful to add a 
>> specific
>> "recorder" command.
>
> Precedence for commands specific to a trace backend: trace-file.
>
> Name yours trace-recorder?

 But then "recorder dump" does not fit with any "trace" concept.
>>>
>>> Once you make the recorder a trace backend, whatever the recorder does
>>> becomes a trace concept :)
>>
>> I understand your point, but I want to make a distinction between recorder
>> tracing and other recorder features. Does that make sense?
>
> Maybe :)  My thoughts haven't settled, yet.
>
>> For example, assuming I built with both recorder and log trace backends,
>> from the monitor, I can type:
>>
>>   trace-event kvm_run_exit on
>>
>> What I get then is something like that:
>>
>>   2091091@1595518935.441273:kvm_run_exit cpu_index 0, reason 2
>>   2091091@1595518935.441292:kvm_run_exit cpu_index 0, reason 2
>>   2091091@1595518935.441301:kvm_run_exit cpu_index 0, reason 2
>>   2091091@1595518935.441309:kvm_run_exit cpu_index 0, reason 2
>>   2091091@1595518935.441254:kvm_run_exit cpu_index 0, reason 2
>>
>> It would not be very useful to activate recorder traces as well when that
>> happens, which would have the undesired side effect of purging any
>>> corresponding entry from a following recorder dump.
>
> I'm afraid I don't understand, because the gaps in my understanding of
> what the recorder can do are just too big.

 There is a video at the top of https://github.com/c3d/recorder, or direct
 link https://www.youtube.com/watch?v=kEnQY1zFa0Y. Hope this helps.


>
> From your cover letter:
>
> 1. Flight recorder: Dump information on recent events in case of crash
>
> Define "recent events", please.  Is it all trace events (except for the
> ones disabled at build time, of course)?

 For event categories only known through qemu trace definitions, by default,
>>>
>>> Right now, there are no others, aren't there?
>>
>> There is one in the second patch, the example. That was actually the whole
>> point of that second patch, which is not otherwise particularly exciting.
>>
>>>
 it's the last 8 events. If you manually declare a recorder, then you can
 specify any size.

 (The difference between this manual recorder usage and the recorder backend
 generated code is similar to the difference between the log backend and
 "DPRINTF")
>>>
>>> I'm not sure I get the parenthesis.
>>
>> I took that example because it's mentioned in docs/devel/tracing.txt:
>>
>> === Log ===
>>
>> The "log" backend sends trace events directly to standard error.  This
>> effectively turns trace events into debug printfs.
>>
>> This is the simplest backend and can be used together with existing code 
>> that
>> uses DPRINTF().
>>
>> Just the same way, the recorder back-end can both use existing trace points,
>> using the recorder back-end, but you can also add explicit record()
>> statements much like you can add DPRINTF() statements.
>
> Got it.
>
>> Whether this is a good idea or not is debatable. I would argue that in some
>> cases, it's a good idea, or at least, probably a better idea than DPRINTF ;-)
>
> No need to convince me a low-overhead flight recorder can be useful.
> I've improvised special-purpose flight recorders many times when
> printf-like tracing inte

Re: [PATCH v2 15/16] hw/block/nvme: use preallocated qsg/iov in nvme_dma_prp

[Minwoo Im]

On 20-07-30 00:06:37, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Since clean up of the request qsg/iov is now always done post-use, there
> is no need to use a stack-allocated qsg/iov in nvme_dma_prp.
> 
> Signed-off-by: Klaus Jensen 
> Acked-by: Keith Busch 
> Reviewed-by: Maxim Levitsky 

Reviewed-by: Minwoo Im 

Re: [PATCH v2 16/16] hw/block/nvme: remove explicit qsg/iov parameters

[Minwoo Im]

On 20-07-30 00:06:38, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Since nvme_map_prp always operate on the request-scoped qsg/iovs, just
> pass a single pointer to the NvmeRequest instead of two for each of the
> qsg and iov.
> 
> Suggested-by: Minwoo Im 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 55b1a68ced8c..aea8a8b6946c 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -284,8 +284,8 @@ static uint16_t nvme_map_addr(NvmeCtrl *n, QEMUSGList 
> *qsg, QEMUIOVector *iov,
>  return NVME_SUCCESS;
>  }
>  
> -static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t 
> prp1,
> - uint64_t prp2, uint32_t len, NvmeCtrl *n)
> +static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, uint64_t prp2,
> + uint32_t len, NvmeRequest *req)
>  {
>  hwaddr trans_len = n->page_size - (prp1 % n->page_size);
>  trans_len = MIN(len, trans_len);
> @@ -293,6 +293,9 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, 
> QEMUIOVector *iov, uint64_t prp1,
>  uint16_t status;
>  bool prp_list_in_cmb = false;
>  
> +QEMUSGList *qsg = &req->qsg;
> +QEMUIOVector *iov = &req->iov;
> +
>  trace_pci_nvme_map_prp(trans_len, len, prp1, prp2, num_prps);
>  
>  if (unlikely(!prp1)) {
> @@ -386,7 +389,7 @@ static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, 
> uint32_t len,
>  {
>  uint16_t status = NVME_SUCCESS;
>  
> -status = nvme_map_prp(&req->qsg, &req->iov, prp1, prp2, len, n);
> +status = nvme_map_prp(n, prp1, prp2, len, req);
>  if (status) {
>  return status;
>  }
> @@ -431,7 +434,7 @@ static uint16_t nvme_map_dptr(NvmeCtrl *n, size_t len, 
> NvmeRequest *req)
>  uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
>  uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
>  
> -return nvme_map_prp(&req->qsg, &req->iov, prp1, prp2, len, n);
> +return nvme_map_prp(n, prp1, prp2, len, req);
>  }
>  
>  static void nvme_post_cqes(void *opaque)


This looks better to have qsg and iov in the NvmeRequest.

Reviewed-by: Minwoo Im 

Re: [PATCH v2 14/16] hw/block/nvme: consolidate qsg/iov clearing

[Minwoo Im]

On 20-07-30 00:06:36, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Always destroy the request qsg/iov at the end of request use.
> 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 52 -
>  1 file changed, 21 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 3d7275eae369..045dd55376a5 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -217,6 +217,17 @@ static void nvme_req_clear(NvmeRequest *req)
>  memset(&req->cqe, 0x0, sizeof(req->cqe));
>  }
>  
> +static void nvme_req_exit(NvmeRequest *req)
> +{
> +if (req->qsg.sg) {
> +qemu_sglist_destroy(&req->qsg);
> +}
> +
> +if (req->iov.iov) {
> +qemu_iovec_destroy(&req->iov);
> +}
> +}
> +

Klaus,

What is differences between 'clear' and 'exit' from the request
perspective?

Thanks,

Re: [PATCH] pci_dma_rw: return correct value instead of 0

[Stefano Garzarella]

On Thu, Jul 30, 2020 at 10:50:43AM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 30/07/2020 09:41, Stefano Garzarella wrote:
> > On Thu, Jul 30, 2020 at 12:17:32AM +0200, Emanuele Giuseppe Esposito wrote:
> > > pci_dma_rw currently always returns 0, regardless
> > > of the result of dma_memory_rw. Adjusted to return
> > > the correct value.
> > > 
> > > Signed-off-by: Emanuele Giuseppe Esposito 
> > > ---
> > >   include/hw/pci/pci.h | 3 +--
> > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index c1bf7d5356..41c4ab5932 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -787,8 +787,7 @@ static inline AddressSpace 
> > > *pci_get_address_space(PCIDevice *dev)
> > >   static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
> > >void *buf, dma_addr_t len, DMADirection 
> > > dir)
> > >   {
> > > -dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> > > -return 0;
> > > +return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, 
> > > dir);
> > >   }
> > 
> > I think it's a left over from when we used "void cpu_physical_memory_rw()".
> > 
> > I agree that it is better to return the dma_memory_rw() return value, but
> > at first look, no one seems to check the return value of pci_dma_rw(),
> > pci_dma_read(), andpci_dma_write().
> > 
> > Should we make them void?
> 
> I noticed that nobody checks the return of those functions, but I think
> checking for possible error is always useful. I am using the edu device and
> clearly doing something wrong since with this fix I discovered that the
> pci_dma_read call returns nonzero.
> 
> Keeping the function as it is or void would make it harder to spot such
> errors in future.

I agree, I was just worried that no one checks the return value.

Thanks,
Stefano

Re: [PATCH v2 04/16] hw/block/nvme: remove redundant has_sg member

[Maxim Levitsky]

On Thu, 2020-07-30 at 00:06 +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Remove the has_sg member from NvmeRequest since it's redundant.
> 
> Signed-off-by: Klaus Jensen 


Reviewed-by: Maxim Levitsky 

Best regards,
Maxim Levitsky

> ---
>  hw/block/nvme.c | 7 ++-
>  hw/block/nvme.h | 1 -
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index d60b19e1840f..a9d9a2912655 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -550,7 +550,8 @@ static void nvme_rw_cb(void *opaque, int ret)
>  block_acct_failed(blk_get_stats(n->conf.blk), &req->acct);
>  req->status = NVME_INTERNAL_DEV_ERROR;
>  }
> -if (req->has_sg) {
> +
> +if (req->qsg.nalloc) {
>  qemu_sglist_destroy(&req->qsg);
>  }
>  nvme_enqueue_req_completion(cq, req);
> @@ -559,7 +560,6 @@ static void nvme_rw_cb(void *opaque, int ret)
>  static uint16_t nvme_flush(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
>  NvmeRequest *req)
>  {
> -req->has_sg = false;
>  block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0,
>   BLOCK_ACCT_FLUSH);
>  req->aiocb = blk_aio_flush(n->conf.blk, nvme_rw_cb, req);
> @@ -585,7 +585,6 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, 
> NvmeNamespace *ns, NvmeCmd *cmd,
>  return NVME_LBA_RANGE | NVME_DNR;
>  }
>  
> -req->has_sg = false;
>  block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0,
>   BLOCK_ACCT_WRITE);
>  req->aiocb = blk_aio_pwrite_zeroes(n->conf.blk, offset, count,
> @@ -623,7 +622,6 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, 
> NvmeCmd *cmd,
>  }
>  
>  if (req->qsg.nsg > 0) {
> -req->has_sg = true;
>  block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 
> req->qsg.size,
>   acct);
>  req->aiocb = is_write ?
> @@ -632,7 +630,6 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, 
> NvmeCmd *cmd,
>  dma_blk_read(n->conf.blk, &req->qsg, data_offset, 
> BDRV_SECTOR_SIZE,
>   nvme_rw_cb, req);
>  } else {
> -req->has_sg = false;
>  block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 
> req->iov.size,
>   acct);
>  req->aiocb = is_write ?
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 0b6a8ae66559..5519b5cc7686 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -22,7 +22,6 @@ typedef struct NvmeRequest {
>  struct NvmeSQueue   *sq;
>  BlockAIOCB  *aiocb;
>  uint16_tstatus;
> -boolhas_sg;
>  NvmeCqe cqe;
>  BlockAcctCookie acct;
>  QEMUSGList  qsg;

Re: [PATCH v2 05/16] hw/block/nvme: destroy request iov before reuse

[Maxim Levitsky]

On Thu, 2020-07-30 at 00:06 +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Make sure the request iov is destroyed before reuse; fixing a memory
> leak.
> 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index a9d9a2912655..8f8257e06eed 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -554,6 +554,10 @@ static void nvme_rw_cb(void *opaque, int ret)
>  if (req->qsg.nalloc) {
>  qemu_sglist_destroy(&req->qsg);
>  }
> +if (req->iov.nalloc) {
> +qemu_iovec_destroy(&req->iov);
> +}
> +
>  nvme_enqueue_req_completion(cq, req);
>  }
>  

Reviewed-by: Maxim Levitsky 

Best regards,
Maxim Levitsky

Re: [PATCH v2 07/16] hw/block/nvme: add tracing to nvme_map_prp

[Maxim Levitsky]

On Thu, 2020-07-30 at 00:06 +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Add tracing to nvme_map_prp.
> 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c   | 2 ++
>  hw/block/trace-events | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 571635ebe9f9..952afbb05175 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -274,6 +274,8 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, 
> QEMUIOVector *iov, uint64_t prp1,
>  int num_prps = (len >> n->page_bits) + 1;
>  uint16_t status;
>  
> +trace_pci_nvme_map_prp(trans_len, len, prp1, prp2, num_prps);
> +
>  if (unlikely(!prp1)) {
>  trace_pci_nvme_err_invalid_prp();
>  return NVME_INVALID_FIELD | NVME_DNR;
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index f3b2d004e078..f20c59a4b542 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -35,6 +35,7 @@ pci_nvme_irq_masked(void) "IRQ is masked"
>  pci_nvme_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64" 
> prp2=0x%"PRIx64""
>  pci_nvme_map_addr(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len 
> %"PRIu64""
>  pci_nvme_map_addr_cmb(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len 
> %"PRIu64""
> +pci_nvme_map_prp(uint64_t trans_len, uint32_t len, uint64_t prp1, uint64_t 
> prp2, int num_prps) "trans_len %"PRIu64" len %"PRIu32" prp1 0x%"PRIx64" prp2 
> 0x%"PRIx64" num_prps %d"
>  pci_nvme_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode) 
> "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
>  pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t opcode) "cid 
> %"PRIu16" sqid %"PRIu16" opc 0x%"PRIx8""
>  pci_nvme_rw(const char *verb, uint32_t blk_count, uint64_t byte_count, 
> uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""

Reviewed-by: Maxim Levitsky 

Best regards,
Maxim Levitsky

Re: [PATCH] pci_dma_rw: return correct value instead of 0

[Stefano Garzarella]

On Thu, Jul 30, 2020 at 09:58:21AM +0100, Peter Maydell wrote:
> On Thu, 30 Jul 2020 at 08:42, Stefano Garzarella  wrote:
> > I agree that it is better to return the dma_memory_rw() return value, but
> > at first look, no one seems to check the return value of pci_dma_rw(),
> > pci_dma_read(), andpci_dma_write().
> >
> > Should we make them void?
> 
> In general code (eg device models) that issues memory transactions
> need to have a mechanism for finding out whether the transaction
> succeeds. Traditionally QEMU didn't have the concept of a
> transaction failing, but we have added it, starting with the
> APIs at the bottom level (the address_space_* ones). We haven't
> always plumbed the error-handling (or the memory-transaction
> input, for that matter) through to some of these other APIs.
> I think for consistency we should do that, and ideally we
> should make all these APIs look the same as the base-level
> address_space* ones, which would mean returning a MemTxError
> rather than a bool.

Yeah, that makes a lot of sense to me.

> 
> We should also figure out why the dma_* functions exist at all:
> they include some calls to dma_barrier(), but not all devices
> do DMA with the dma_* functions, so we have an inconsistency
> that should be sorted out...
> 

I've never looked in detail, but I agree we should have more consistency.

Thanks for the details!
Stefano

Re: [PATCH] target/arm: Fix AddPAC error indication

[Peter Maydell]

On Tue, 28 Jul 2020 at 20:57, Richard Henderson
 wrote:
>
> The definition of top_bit used in this function is one higher
> than that used in the Arm ARM psuedo-code, which put the error
> indication at top_bit - 1 at the wrong place, which meant that
> it wasn't visible to Auth.
>
> Fixing the definition of top_bit requires more changes, because
> its most common use is for the count of bits in top_bit:bot_bit,
> which would then need to be computed as top_bit - bot_bit + 1.
>
> For now, prefer the minimal fix to the error indication alone.
>
> Fixes: 63ff0ca94cb
> Reported-by: Derrick McKee 
> Signed-off-by: Richard Henderson 

This seems like it might confuse us in future so I've added
a comment inside the if():

/*
 * Note that our top_bit is one greater than the pseudocode's
 * version, hence "- 2" here.
 */

Otherwise
Reviewed-by: Peter Maydell 

and added to target-arm.next.

thanks
-- PMM

Re: [PATCH v2 14/16] hw/block/nvme: consolidate qsg/iov clearing

[Maxim Levitsky]

On Thu, 2020-07-30 at 19:31 +0900, Minwoo Im wrote:
> On 20-07-30 00:06:36, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Always destroy the request qsg/iov at the end of request use.
> > 
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c | 52 -
> >  1 file changed, 21 insertions(+), 31 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 3d7275eae369..045dd55376a5 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -217,6 +217,17 @@ static void nvme_req_clear(NvmeRequest *req)
> >  memset(&req->cqe, 0x0, sizeof(req->cqe));
> >  }
> >  
> > +static void nvme_req_exit(NvmeRequest *req)
> > +{
> > +if (req->qsg.sg) {
> > +qemu_sglist_destroy(&req->qsg);
> > +}
> > +
> > +if (req->iov.iov) {
> > +qemu_iovec_destroy(&req->iov);
> > +}
> > +}
> > +
> 
> Klaus,
> 
> What is differences between 'clear' and 'exit' from the request
> perspective?
> 
> Thanks,
> 
In my personal opinion, I don't think the name matters that much here.

Reviewed-by: Maxim Levitsky 

Best regards,
Maxim Levitsky

Re: [PATCH v2 16/16] hw/block/nvme: remove explicit qsg/iov parameters

[Maxim Levitsky]

On Thu, 2020-07-30 at 00:06 +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Since nvme_map_prp always operate on the request-scoped qsg/iovs, just
> pass a single pointer to the NvmeRequest instead of two for each of the
> qsg and iov.
> 
> Suggested-by: Minwoo Im 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 55b1a68ced8c..aea8a8b6946c 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -284,8 +284,8 @@ static uint16_t nvme_map_addr(NvmeCtrl *n, QEMUSGList 
> *qsg, QEMUIOVector *iov,
>  return NVME_SUCCESS;
>  }
>  
> -static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t 
> prp1,
> - uint64_t prp2, uint32_t len, NvmeCtrl *n)
> +static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, uint64_t prp2,
> + uint32_t len, NvmeRequest *req)
>  {
>  hwaddr trans_len = n->page_size - (prp1 % n->page_size);
>  trans_len = MIN(len, trans_len);
> @@ -293,6 +293,9 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, 
> QEMUIOVector *iov, uint64_t prp1,
>  uint16_t status;
>  bool prp_list_in_cmb = false;
>  
> +QEMUSGList *qsg = &req->qsg;
> +QEMUIOVector *iov = &req->iov;
> +
>  trace_pci_nvme_map_prp(trans_len, len, prp1, prp2, num_prps);
>  
>  if (unlikely(!prp1)) {
> @@ -386,7 +389,7 @@ static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, 
> uint32_t len,
>  {
>  uint16_t status = NVME_SUCCESS;
>  
> -status = nvme_map_prp(&req->qsg, &req->iov, prp1, prp2, len, n);
> +status = nvme_map_prp(n, prp1, prp2, len, req);
>  if (status) {
>  return status;
>  }
> @@ -431,7 +434,7 @@ static uint16_t nvme_map_dptr(NvmeCtrl *n, size_t len, 
> NvmeRequest *req)
>  uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
>  uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
>  
> -return nvme_map_prp(&req->qsg, &req->iov, prp1, prp2, len, n);
> +return nvme_map_prp(n, prp1, prp2, len, req);
>  }
>  
>  static void nvme_post_cqes(void *opaque)
Reviewed-by: Maxim Levitsky 

Best regards,
Maxim Levitsky

Re: [PATCH v3 0/8] Generalize start-powered-off property from ARM

[Philippe Mathieu-Daudé]

Le jeu. 30 juil. 2020 03:00, David Gibson  a
écrit :

> On Tue, Jul 28, 2020 at 09:56:36PM -0300, Thiago Jung Bauermann wrote:
> >
> > Thiago Jung Bauermann  writes:
> >
> > > The ARM code has a start-powered-off property in ARMCPU, which is a
> > > subclass of CPUState. This property causes arm_cpu_reset() to set
> > > CPUState::halted to 1, signalling that the CPU should start in a halted
> > > state. Other architectures also have code which aim to achieve the same
> > > effect, but without using a property.
> > >
> > > The ppc/spapr version has a bug where QEMU does a KVM_RUN on the vcpu
> > > before cs->halted is set to 1, causing the vcpu to run while it's
> still in
> > > an unitialized state (more details in patch 3).
> >
> > Since this series fixes a bug is it eligible for 5.1, at least the
> > patches that were already approved by the appropriate maintainers?
>
> Ok by me.
>

Maybe just the arm generalization and ppc fix for 5.1, delaying all not
bugfix to 5.2?


> --
> David Gibson| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>

Re: [PATCH v2 14/16] hw/block/nvme: consolidate qsg/iov clearing

[Klaus Jensen]

On Jul 30 19:31, Minwoo Im wrote:
> On 20-07-30 00:06:36, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Always destroy the request qsg/iov at the end of request use.
> > 
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c | 52 -
> >  1 file changed, 21 insertions(+), 31 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 3d7275eae369..045dd55376a5 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -217,6 +217,17 @@ static void nvme_req_clear(NvmeRequest *req)
> >  memset(&req->cqe, 0x0, sizeof(req->cqe));
> >  }
> >  
> > +static void nvme_req_exit(NvmeRequest *req)
> > +{
> > +if (req->qsg.sg) {
> > +qemu_sglist_destroy(&req->qsg);
> > +}
> > +
> > +if (req->iov.iov) {
> > +qemu_iovec_destroy(&req->iov);
> > +}
> > +}
> > +
> 
> Klaus,
> 
> What is differences between 'clear' and 'exit' from the request
> perspective?
> 

Hi Minwoo,

The is that on 'exit' we release request resources (like the qsg and
iov). On 'clear' we initialize the request by clearing the struct. I
guess I could call it nvme_req_init instead maybe, but really - it is
clearing it.

Re: sysbus_create_simple Vs qdev_create

[Paolo Bonzini]

On 30/07/20 12:03, Markus Armbruster wrote:
> qdev C layer:
> 
> frob->prop = 42;
> 
> Least cognitive load.
> 
> QOM has no C layer.

Not really, a QOM object is totally free to do frob->prop = 42.  And
just like we didn't do that outside device implementation in qdev as our
tithe to the Church of Information Hiding; the same applies to QOM.

> qdev property layer works even when @frob has incomplete type:
> 
> qdev_prop_set_int32(DEVICE(frob), "prop", 42);
> 
> This used to map property name to struct offset & copy the value.
> Simple, stupid.
> 
> Nowadays, it is the same as
> 
> object_property_set_int(OBJECT(frob), "frob", 42, &error_abort);
> 
> which first converts the int to a QObject, then uses a QObject input
> visitor with a virtual walk to convert it back to int and store it in
> @frob.  It's quite a sight in the debugger.

Yes, but thatt's just because we never bothered to create single-type
visitors.  For a good reason though: I don't think the extra QAPI code
is worth (not even that much) nicer backtraces when we already have a
QObject as a battle-tested variant type.

> qdev "text" layer is really a QemuOpts layer (because that's what we had
> back then).  If we have prop=42 in a QemuOpts, it calls
> 
> set_property("prop", "42", frob, &err);
> 
> Nowadays, this is a thin wrapper around object_property_parse(),
> basically
> 
> object_property_parse(frob, "prop", 42, &err);
> 
> Fine print: except set_property() does nothing for @prop "driver" and
> "bus", which look just like properties in -device / device-add, but
> aren't.

Ugly indeed.  They should be special cased up in the caller, probably,
or use the long-discussed "remainder" feature of the QAPI schema.

> object_property_parse() uses the string input visitor, which I loathe.

Apart from the list syntax, the string input visitor is decent I think.

>>> I've long had the nagging feeling that if we had special-cased
>>> containers, children and links, we could have made a QOM that was easier
>>> to reason about, and much easier to integrate with a QAPI schema.
>>
>> That's at least plausible.  But I have a nagging feeling that it would
>> only cover 99% of what we're doing with QOM. :)
> 
> The question is whether that 1% really should be done the way it is done
> :)

And that's a very fair question, but it implies non-trivial design work,
so the smiley changes to a frown. :(

Paolo

[Bug 1886343] Re: configure has non-posix bash syntax

[Peter Maydell]

Fixed in commit b418d2656112174c; this will be in QEMU 5.1.


** Changed in: qemu
   Status: Confirmed => Fix Committed

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

Title:
  configure has non-posix bash syntax

Status in QEMU:
  Fix Committed

Bug description:
  which gives an error when run on a system that uses dash for /bin/sh.

  The problem is at line 6464 which has
  if test "$have_keyring" == "yes"
  the double equal sign is non-posix bash syntax that isn't accepted by posix 
shells like dash.  This was added 2020-05-25 according to git blame so looks 
like a recent problem.

  On an Ubuntu 20.04 system with top of tree sources I get
  gondor:2027$ ../qemu/configure 
--prefix=/home/wilson/FOSS/qemu/install-qemu-tmp 
--target-list=riscv64-linux-user,riscv64-softmmu,riscv32-linux-user,riscv32-softmmu
  ../qemu/configure: 6464: test: yes: unexpected operator
  ...

  configure completes OK, so this is a minor problem.  It is just one
  configure test that is failing to work properly.

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

Re: [PATCH 2/2] target/arm: Fix compile error.

[Kaige Li]

On 07/30/2020 04:44 PM, Peter Maydell wrote:


On Thu, 30 Jul 2020 at 02:56, Kaige Li  wrote:

When I compile qemu with such as:

git clone https://git.qemu.org/git/qemu.git
cd qemu
git submodule init
git submodule update --recursive
./configure
make

There is error log:

/home/LiKaige/qemu/target/arm/translate-a64.c: In function ‘disas_ldst’:
/home/LiKaige/qemu/target/arm/translate-a64.c:3392:5: error: ‘fn’ may be used 
uninitialized in this function [-Werror=maybe-uninitialized]
  fn(cpu_reg(s, rt), clean_addr, tcg_rs, get_mem_index(s),
  ^
/home/LiKaige/qemu/target/arm/translate-a64.c:3318:22: note: ‘fn’ was declared 
here
  AtomicThreeOpFn *fn;
   ^
cc1: all warnings being treated as errors

So, add an initiallization value for fn to fix this.

Signed-off-by: Kaige Li 

What compiler version is this ?

It's the latest version: v5.1.0-rc2, but VERSION shows that is 5.0.92.
Commit id is 5772f2b1fc5d00e7e04e01fa28e9081d6550440a



---
  target/arm/translate-a64.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 8c07649..910a91f 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -3315,7 +3315,7 @@ static void disas_ldst_atomic(DisasContext *s, uint32_t 
insn,
  bool r = extract32(insn, 22, 1);
  bool a = extract32(insn, 23, 1);
  TCGv_i64 tcg_rs, clean_addr;
-AtomicThreeOpFn *fn;
+AtomicThreeOpFn *fn = tcg_gen_atomic_fetch_add_i64;

NULL would be a better choice for a "this is never actually used"
initialiser.

Ok, I will have a try and submit it in v2.

Thank you.
Kaige.


thanks
-- PMM

Re: [PATCH 2/2] target/arm: Fix compile error.

[Peter Maydell]

On Thu, 30 Jul 2020 at 12:19, Kaige Li  wrote:
>
> On 07/30/2020 04:44 PM, Peter Maydell wrote:
>
> > On Thu, 30 Jul 2020 at 02:56, Kaige Li  wrote:
> >> When I compile qemu with such as:
> >>
> >> git clone https://git.qemu.org/git/qemu.git
> >> cd qemu
> >> git submodule init
> >> git submodule update --recursive
> >> ./configure
> >> make
> >>
> >> There is error log:
> >>
> >> /home/LiKaige/qemu/target/arm/translate-a64.c: In function ‘disas_ldst’:
> >> /home/LiKaige/qemu/target/arm/translate-a64.c:3392:5: error: ‘fn’ may be 
> >> used uninitialized in this function [-Werror=maybe-uninitialized]
> >>   fn(cpu_reg(s, rt), clean_addr, tcg_rs, get_mem_index(s),
> >>   ^
> >> /home/LiKaige/qemu/target/arm/translate-a64.c:3318:22: note: ‘fn’ was 
> >> declared here
> >>   AtomicThreeOpFn *fn;
> >>^
> >> cc1: all warnings being treated as errors
> >>
> >> So, add an initiallization value for fn to fix this.
> >>
> >> Signed-off-by: Kaige Li 
> > What compiler version is this ?
> It's the latest version: v5.1.0-rc2, but VERSION shows that is 5.0.92.
> Commit id is 5772f2b1fc5d00e7e04e01fa28e9081d6550440a

I asked for the compiler version, not the QEMU version :-)
Clang, gcc, OSX clang, something else, and which version number?

-- PMM

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Halil Pasic]

On Thu, 30 Jul 2020 12:26:56 +0200
Cornelia Huck  wrote:

> On Wed, 29 Jul 2020 15:02:22 +0200
> Halil Pasic  wrote:
> 
> > As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1)
> > reads one past of the end of ms->loadparm, so g_memdup() can not be used
> > here.
> > 
> > Let's use malloc and memcpy instead!
> 
> Hm, an alternative would be to use g_strndup(). What do you think?

Sure. It is more concise and does exactly what we want. I'm not too
familiar with the string utility funcitons of glib, so it didn't jup
at me.

Shall I spin a v2?

Halil

> 
> > 
> > Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter")
> > Fixes: Coverity CID 1431058
> > Reported-by: Peter Maydell 
> > Signed-off-by: Halil Pasic 
> > ---
> >  hw/s390x/s390-virtio-ccw.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 403d30e13b..8b7bac0392 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -704,8 +704,8 @@ static char *machine_get_loadparm(Object *obj, Error 
> > **errp)
> >  char *loadparm_str;
> >  
> >  /* make a NUL-terminated string */
> > -loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
> > -loadparm_str[sizeof(ms->loadparm)] = 0;
> > +loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1);
> > +memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm));
> >  return loadparm_str;
> >  }
> >  
> > 
> > base-commit: 5772f2b1fc5d00e7e04e01fa28e9081d6550440a
> 
> 

[Bug 1884728] Re: facing build error for qemu-4.0.0 on SUSE11 OS

[Peter Maydell]

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

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

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

Title:
  facing build error for qemu-4.0.0 on SUSE11 OS

Status in QEMU:
  Invalid

Bug description:
  I am trying to compile qemu-4.0.0 on suse11 OS and facing the following error 
on the console:
  ERROR: sizeof(size_t) doesn't match GLIB_SIZEOF_SIZE_T.
 You probably need to set PKG_CONFIG_LIBDIR
 to point to the right pkg-config files for your
 build target

  Looking into the config.log file following is the error that is
  listed:

  config-temp/qemu-conf.c:12:11: error: 'WACS_DEGREE' undeclared (first use in 
this function)
 add_wch(WACS_DEGREE);
 ^
  config-temp/qemu-conf.c:12:11: note: each undeclared identifier is reported 
only once for each function it appears in

  ld: skipping incompatible /usr/lib//libc.so when searching for -lc
  ld: skipping incompatible /usr/lib//libc.a when searching for -lc
  /tmp/ccmme6E4.o: In function `main':
  qemu-conf.c:(.text+0x2b): undefined reference to `resize_term'
  qemu-conf.c:(.text+0x32): undefined reference to `stdscr'
  qemu-conf.c:(.text+0x49): undefined reference to `waddnwstr'
  qemu-conf.c:(.text+0x50): undefined reference to `stdscr'
  qemu-conf.c:(.text+0x67): undefined reference to `waddnwstr'
  qemu-conf.c:(.text+0x6e): undefined reference to `_nc_wacs'
  qemu-conf.c:(.text+0x7f): undefined reference to `stdscr'
  qemu-conf.c:(.text+0x8d): undefined reference to `wadd_wch'
  collect2: error: ld returned 1 exit status

  Following are the details of the tools versions:
  OS version = SUSE Linux Enterprise Server 11 (x86_64)
  python = v2.7.10
  glib = v2.56.1
  gcc = v4.8.3
  sdl2 = v2.0.12

  Can someone help me understand the cause of this error?

  regards,
  Harshit

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

Re: [PATCH v4 1/2] nvme: indicate CMB support through controller capabilities register

[Maxim Levitsky]

On Tue, 2020-07-07 at 21:15 +0200, Klaus Jensen wrote:
> On Jul  7 19:27, Maxim Levitsky wrote:
> > On Wed, 2020-07-01 at 14:48 -0700, Andrzej Jakowski wrote:
> > > This patch sets CMBS bit in controller capabilities register when user
> > > configures NVMe driver with CMB support, so capabilites are correctly
> > > reported to guest OS.
> > > 
> > > Signed-off-by: Andrzej Jakowski 
> > > Reviewed-by: Klaus Jensen 
> > > ---
> > >  hw/block/nvme.c  | 2 +-
> > >  include/block/nvme.h | 6 +-
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 1aee042d4c..9f11f3e9da 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -1582,6 +1582,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
> > > *pci_dev)
> > >  NVME_CAP_SET_TO(n->bar.cap, 0xf);
> > >  NVME_CAP_SET_CSS(n->bar.cap, 1);
> > >  NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
> > > +NVME_CAP_SET_CMBS(n->bar.cap, n->params.cmb_size_mb ? 1 : 0);
> > >  
> > >  n->bar.vs = 0x00010200;
> > >  n->bar.intmc = n->bar.intms = 0;
> > > @@ -1591,7 +1592,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> > > **errp)
> > >  {
> > >  NvmeCtrl *n = NVME(pci_dev);
> > >  Error *local_err = NULL;
> > > -
> > >  int i;
> > >  
> > >  nvme_check_constraints(n, &local_err);
> > > diff --git a/include/block/nvme.h b/include/block/nvme.h
> > > index 1720ee1d51..14cf398dfa 100644
> > > --- a/include/block/nvme.h
> > > +++ b/include/block/nvme.h
> > > @@ -35,6 +35,7 @@ enum NvmeCapShift {
> > >  CAP_MPSMIN_SHIFT   = 48,
> > >  CAP_MPSMAX_SHIFT   = 52,
> > >  CAP_PMR_SHIFT  = 56,
> > > +CAP_CMB_SHIFT  = 57,
> > >  };
> > >  
> > >  enum NvmeCapMask {
> > > @@ -48,6 +49,7 @@ enum NvmeCapMask {
> > >  CAP_MPSMIN_MASK= 0xf,
> > >  CAP_MPSMAX_MASK= 0xf,
> > >  CAP_PMR_MASK   = 0x1,
> > > +CAP_CMB_MASK   = 0x1,
> > >  };
> > >  
> > >  #define NVME_CAP_MQES(cap)  (((cap) >> CAP_MQES_SHIFT)   & CAP_MQES_MASK)
> > > @@ -78,8 +80,10 @@ enum NvmeCapMask {
> > > << 
> > > CAP_MPSMIN_SHIFT)
> > >  #define NVME_CAP_SET_MPSMAX(cap, val) (cap |= (uint64_t)(val & 
> > > CAP_MPSMAX_MASK)\
> > >  << 
> > > CAP_MPSMAX_SHIFT)
> > > -#define NVME_CAP_SET_PMRS(cap, val) (cap |= (uint64_t)(val & 
> > > CAP_PMR_MASK)\
> > > +#define NVME_CAP_SET_PMRS(cap, val)   (cap |= (uint64_t)(val & 
> > > CAP_PMR_MASK)   \
> > >  << 
> > > CAP_PMR_SHIFT)
> > > +#define NVME_CAP_SET_CMBS(cap, val)   (cap |= (uint64_t)(val & 
> > > CAP_CMB_MASK)   \
> > > +   << 
> > > CAP_CMB_SHIFT)
> > >  
> > >  enum NvmeCcShift {
> > >  CC_EN_SHIFT = 0,
> > 
> > I wonder how this could have beeing forgotten. Hmm.
> > I see that Linux kernel uses CMBSZ != for that.
> > I guess this explains it.
> > 
> > Reviewed-by: Maxim Levitsky 
> > 
> 
> It is a v1.4 field. The CMB support was added when NVMe was at v1.2.
> And the Linux kernel is also basically adhering to v1.3 wrt. CMB
> support. In v1.4 the host actually needs to specifically enable the CMB
> - and that is not something the kernel does currently IIRC.
> 
Ah, makes sense!
I by now have specs for each NVME revision, but I am getting lazy sometimes to 
cross-check
them.

Best regards,
Maxim Levitsky

[Bug 1881552] Re: potential AArch64 ABI bug wrt handling of 128-bit bit-fields

[Peter Maydell]

The warnings aren't a problem for QEMU because we don't expose these
functions as public ABI, so the whole compile will be consistently built
with the same compiler version. So we added -Wno-psabi in commit
bac8d222a19f4a30d to silence the compiler here.


** Changed in: qemu
   Status: New => Fix Committed

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

Title:
  potential AArch64 ABI bug wrt handling of 128-bit bit-fields

Status in QEMU:
  Fix Committed

Bug description:
  After upgrading to Ubuntu 20.04 LTS, GCC 9.3 displays a lot of notes:

  hw/block/pflash_cfi01.c: In function ‘pflash_mem_read_with_attrs’:
  hw/block/pflash_cfi01.c:663:20: note: parameter passing for argument of type 
‘MemTxAttrs’ {aka ‘struct MemTxAttrs’} changed in GCC 9.1
663 | static MemTxResult pflash_mem_read_with_attrs(void *opaque, hwaddr 
addr, uint64_t *value,
|^~
  hw/block/pflash_cfi01.c: In function ‘pflash_mem_write_with_attrs’:
  hw/block/pflash_cfi01.c:677:20: note: parameter passing for argument of type 
‘MemTxAttrs’ {aka ‘struct MemTxAttrs’} changed in GCC 9.1
677 | static MemTxResult pflash_mem_write_with_attrs(void *opaque, hwaddr 
addr, uint64_t value,
|^~~
  hw/nvram/fw_cfg.c: In function ‘fw_cfg_dma_mem_valid’:
  hw/nvram/fw_cfg.c:475:13: note: parameter passing for argument of type 
‘MemTxAttrs’ {aka ‘struct MemTxAttrs’} changed in GCC 9.1
475 | static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr,
| ^~~~
  hw/nvram/fw_cfg.c: In function ‘fw_cfg_data_mem_valid’:
  hw/nvram/fw_cfg.c:483:13: note: parameter passing for argument of type 
‘MemTxAttrs’ {aka ‘struct MemTxAttrs’} changed in GCC 9.1
483 | static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
| ^
  hw/nvram/fw_cfg.c: In function ‘fw_cfg_ctl_mem_valid’:
  hw/nvram/fw_cfg.c:501:13: note: parameter passing for argument of type 
‘MemTxAttrs’ {aka ‘struct MemTxAttrs’} changed in GCC 9.1
501 | static bool fw_cfg_ctl_mem_valid(void *opaque, hwaddr addr,
| ^~~~
  hw/nvram/fw_cfg.c: In function ‘fw_cfg_comb_valid’:
  hw/nvram/fw_cfg.c:521:13: note: parameter passing for argument of type 
‘MemTxAttrs’ {aka ‘struct MemTxAttrs’} changed in GCC 9.1
521 | static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
| ^
  hw/intc/arm_gic.c: In function ‘gic_do_hyp_read’:
  hw/intc/arm_gic.c:1996:20: note: parameter passing for argument of type 
‘MemTxAttrs’ {aka ‘struct MemTxAttrs’} changed in GCC 9.1
   1996 | static MemTxResult gic_do_hyp_read(void *opaque, hwaddr addr, 
uint64_t *data,
|^~~
  hw/intc/arm_gic.c: In function ‘gic_thiscpu_hyp_read’:
  hw/intc/arm_gic.c:1979:20: note: parameter passing for argument of type 
‘MemTxAttrs’ {aka ‘struct MemTxAttrs’} changed in GCC 9.1
   1979 | static MemTxResult gic_thiscpu_hyp_read(void *opaque, hwaddr addr, 
uint64_t *data,
|^~~~
  hw/intc/arm_gic.c: In function ‘gic_get_current_pending_irq’:
  hw/intc/arm_gic.c:419:17: note: parameter passing for argument of type 
‘MemTxAttrs’ {aka ‘struct MemTxAttrs’} changed in GCC 9.1
419 | static uint16_t gic_get_current_pending_irq(GICState *s, int cpu,
| ^~~

  This seems related to:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88469
  https://gcc.gnu.org/git/?p=gcc.git&a=commit;h=c590597c45

This is pretty unlikely in real code, but similar to Arm, the AArch64
ABI has a bug with the handling of 128-bit bit-fields, where if the
bit-field dominates the overall alignment the back-end code may end up
passing the argument correctly.  This is a regression that started in
gcc-6 when the ABI support code was updated to support overaligned
types.  The fix is very similar in concept to the Arm fix.  128-bit
bit-fields are fortunately extremely rare, so I'd be very surprised if
anyone has been bitten by this.

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

[Bug 1879587] Re: Register number in ESR is incorrect for certain banked registers when switching from AA32 to AA64

[Peter Maydell]

** Tags added: arm

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

Title:
  Register number in ESR is incorrect for certain banked registers when
  switching from AA32 to AA64

Status in QEMU:
  New

Bug description:
  I am running into a situation where I have:
  - A hypervisor running in EL2, AA64
  - A guest running in EL1, AA32

  We trap certain accesses to special registers such as DACR (via
  HCR.TVM). One instruction that is trapped is:

  ee03ef10  ->mcr 15, 0, lr, cr3, cr0, {0}

  The guest is running in SVC mode. So, LR should refer to LR_svc there.
  LR_svc is mapped to X18 in AA64. So, ESR should reflect that. However,
  the actual ESR value is: 0xfe00dc0

  If we decode the 'rt':
  >>> (0xfe00dc0 >> 5) & 0x1f
  14

  My understanding is that 14 is incorrect in the context of AA64. rt
  should be set to 18. The current mode being SVC, LR refers to LR_svc
  not LR_usr. In other words, the mapping between registers in AA64 and
  AA32 doesn't seem to be accounted for. I've tested this with Qemu
  5.0.0

  Let me know if that makes sense and if you would like more info. I am also 
happy to test patches.
  Thanks for all the great work on Qemu!

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

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Halil Pasic]

On Thu, 30 Jul 2020 11:25:06 +0100
Daniel P. Berrangé  wrote:

> >  /* make a NUL-terminated string */
> > -loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
> > -loadparm_str[sizeof(ms->loadparm)] = 0;
> > +loadparm_str = g_malloc0(sizeof(ms->loadparm) + 1);
> > +memcpy(loadparm_str, ms->loadparm, sizeof(ms->loadparm));  
> 
> I feel like  g_strndup(ms->loadparm, sizeof(ms->loadparm))
> is what should have been used here.
> 
> It copies N characters, but allocates N+1 adding a trailing NUL
> which are the semantic we wanted here.

I agree. Thanks for pointing this out. I'm not very familiar with the
string utilities of glib.

Regards,
Halil

Re: [PATCH v2 1/3] hw/i386: Initialize topo_ids from CpuInstanceProperties

[Igor Mammedov]

On Wed, 29 Jul 2020 16:22:32 -0500
Babu Moger  wrote:

> Igor,
> Sorry. Few more questions.
> 
> > -Original Message-
> > From: Igor Mammedov 
> > Sent: Wednesday, July 29, 2020 9:12 AM
> > To: Moger, Babu 
> > Cc: pbonz...@redhat.com; r...@twiddle.net; qemu-devel@nongnu.org;
> > ehabk...@redhat.com
> > Subject: Re: [PATCH v2 1/3] hw/i386: Initialize topo_ids from
> > CpuInstanceProperties
> > 
> > On Mon, 27 Jul 2020 18:59:42 -0500
> > Babu Moger  wrote:
> >   
> > > > -Original Message-
> > > > From: Igor Mammedov 
> > > > Sent: Monday, July 27, 2020 12:14 PM
> > > > To: Moger, Babu 
> > > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com;  
> > ehabk...@redhat.com;  
> > > > r...@twiddle.net
> > > > Subject: Re: [PATCH v2 1/3] hw/i386: Initialize topo_ids from
> > > > CpuInstanceProperties
> > > >
> > > > On Mon, 27 Jul 2020 10:49:08 -0500
> > > > Babu Moger  wrote:
> > > >  
> > > > > > -Original Message-
> > > > > > From: Igor Mammedov 
> > > > > > Sent: Friday, July 24, 2020 12:05 PM
> > > > > > To: Moger, Babu 
> > > > > > Cc: qemu-devel@nongnu.org; pbonz...@redhat.com;  
> > > > ehabk...@redhat.com;  
> > > > > > r...@twiddle.net
> > > > > > Subject: Re: [PATCH v2 1/3] hw/i386: Initialize topo_ids from
> > > > > > CpuInstanceProperties
> > > > > >
> > > > > > On Mon, 13 Jul 2020 14:30:29 -0500 Babu Moger
> > > > > >  wrote:
> > > > > >  
> > > > > > > > -Original Message-
> > > > > > > > From: Igor Mammedov 
> > > > > > > > Sent: Monday, July 13, 2020 12:32 PM
> > > > > > > > To: Moger, Babu 
> > > > > > > > Cc: pbonz...@redhat.com; r...@twiddle.net;
> > > > > > > > ehabk...@redhat.com;
> > > > > > > > qemu- de...@nongnu.org
> > > > > > > > Subject: Re: [PATCH v2 1/3] hw/i386: Initialize topo_ids
> > > > > > > > from CpuInstanceProperties
> > > > > > > >
> > > > > > > > On Mon, 13 Jul 2020 11:43:33 -0500 Babu Moger
> > > > > > > >  wrote:
> > > > > > > >  
> > > > > > > > > On 7/13/20 11:17 AM, Igor Mammedov wrote:  
> > > > > > > > > > On Mon, 13 Jul 2020 10:02:22 -0500 Babu Moger
> > > > > > > > > >  wrote:
> > > > > > > > > >  
> > > > > > > > > >>> -Original Message-
> > > > > > > > > >>> From: Igor Mammedov 
> > > > > > > > > >>> Sent: Monday, July 13, 2020 4:08 AM
> > > > > > > > > >>> To: Moger, Babu 
> > > > > > > > > >>> Cc: pbonz...@redhat.com; r...@twiddle.net;
> > > > > > > > > >>> ehabk...@redhat.com;
> > > > > > > > > >>> qemu- de...@nongnu.org
> > > > > > > > > >>> Subject: Re: [PATCH v2 1/3] hw/i386: Initialize
> > > > > > > > > >>> topo_ids from CpuInstanceProperties  
[...]

> > > > > There will be complications when user configures with both die_id
> > > > > and numa_id. It will complicate things further. I will have to
> > > > > look closely at the code if it is feasible.  
> > > >
> > > > it's worth a try.
> > > > conseptionally die_id in intel/amd is the same. Most likely intel
> > > > has a dedicated memory controller on each die so it still should
> > > > form a NUMA node. But that aspect probably was ignored while
> > > > implementing it in QEMU so ping of configuring QEMU right is on
> > > > user's shoulders (there is no checks whatsoever if cpu belonging to 
> > > > specific  
> > die is on right NUMA node).  
> > >
> > > So you are suggesting to use die_id to build the topology for EPYC.
> > > Also initialize die_id based on the numa information. Re-arrange the
> > > numa code to make sure we have all the information before we build the
> > > topology. And then remove the node_id inside X86CPUTopoIDs. Is that the  
> > plan?
> > reusing die_id might simplify logic and at very least we won't have 2 very 
> > similar
> > fields to deal with. With die_id it should be conditional on EPYC.  
> 
> Not convinced if the using the die_id will solve the problem here. But
> going to investigate this little bit.
it allows  us to drop nodes_per_pkg calculation with its dependency on numa,
since it's provided by user with -smp dies=

We might need a sanity check that user provided value is valid in case on EPYC 
though.

> > > Regardless of using die_id, we should  
> > 
> > (1) error out if tolopolgy will require more than 1 numa node and no numa
> > config was provided.  
> 
> We already have node_id check in numa_cpu_pre_plug. Do you want me to
> bring this check in pc_cpu_pre_plug?

There are several checks there and that includes validating per CPU node-id
values and workarounds for broken libvirt.

Where I'm talking more about number of numa nodes required f(-smp dies,-cpu 
EPYC),
like:
  if (dies>1 && epyc && nb_numa_nodes != sockets * dies)
 error_steg("chosen cpu model ... and -smp ... parameters require X numa 
nodes being configured")
 error_append_hint("use -numa options to create requred number of numa 
nodes")

I'm not sure where put it in for now, we can try to put it into x86_cpus_init()
for starters and later see if there is more sutable place for it.


> > (2) for 1 NUMA node use autonuma to create 1 node implicitly, tha

Re: [PATCH 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Cornelia Huck]

On Thu, 30 Jul 2020 13:25:21 +0200
Halil Pasic  wrote:

> On Thu, 30 Jul 2020 12:26:56 +0200
> Cornelia Huck  wrote:
> 
> > On Wed, 29 Jul 2020 15:02:22 +0200
> > Halil Pasic  wrote:
> >   
> > > As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1)
> > > reads one past of the end of ms->loadparm, so g_memdup() can not be used
> > > here.
> > > 
> > > Let's use malloc and memcpy instead!  
> > 
> > Hm, an alternative would be to use g_strndup(). What do you think?  
> 
> Sure. It is more concise and does exactly what we want. I'm not too
> familiar with the string utility funcitons of glib, so it didn't jup
> at me.
> 
> Shall I spin a v2?

Sounds good.

Re: [PATCH 2/2] target/arm: Fix compile error.

[Kaige Li]

On 07/30/2020 07:21 PM, Peter Maydell wrote:


On Thu, 30 Jul 2020 at 12:19, Kaige Li  wrote:

On 07/30/2020 04:44 PM, Peter Maydell wrote:


On Thu, 30 Jul 2020 at 02:56, Kaige Li  wrote:

When I compile qemu with such as:

git clone https://git.qemu.org/git/qemu.git
cd qemu
git submodule init
git submodule update --recursive
./configure
make

There is error log:

/home/LiKaige/qemu/target/arm/translate-a64.c: In function ‘disas_ldst’:
/home/LiKaige/qemu/target/arm/translate-a64.c:3392:5: error: ‘fn’ may be used 
uninitialized in this function [-Werror=maybe-uninitialized]
   fn(cpu_reg(s, rt), clean_addr, tcg_rs, get_mem_index(s),
   ^
/home/LiKaige/qemu/target/arm/translate-a64.c:3318:22: note: ‘fn’ was declared 
here
   AtomicThreeOpFn *fn;
^
cc1: all warnings being treated as errors

So, add an initiallization value for fn to fix this.

Signed-off-by: Kaige Li 

What compiler version is this ?

It's the latest version: v5.1.0-rc2, but VERSION shows that is 5.0.92.
Commit id is 5772f2b1fc5d00e7e04e01fa28e9081d6550440a

I asked for the compiler version, not the QEMU version :-)
Clang, gcc, OSX clang, something else, and which version number?

Sorry for that. Gcc version is 4.9.4.

-- PMM

Re: [PATCH 2/2] target/arm: Fix compile error.

[Peter Maydell]

On Thu, 30 Jul 2020 at 12:28, Kaige Li  wrote:
>
> On 07/30/2020 07:21 PM, Peter Maydell wrote:
> > Clang, gcc, OSX clang, something else, and which version number?
> Sorry for that. Gcc version is 4.9.4.

Ah, that makes sense. That's a pretty old version of gcc (it's
almost the oldest we still support), and it had more false-positives
for maybe-used-uninitialized warnings than more recent gcc versions.
Adding in the = NULL initializer should help it.

thanks
-- PMM

Re: [PATCH-for-5.2] default-configs: Remove ACPI_CPU_HOTPLUG from MIPS machines

[Igor Mammedov]

On Fri, 24 Jul 2020 18:52:59 +0200
Philippe Mathieu-Daudé  wrote:

> No MIPS machine uses the ACPI cpu-hotplug feature
> (QEMU implementation is X86 specific).

if I recall correctly we were building it to satisfy symbol dependencies
due to hw/acpi/piix4.c being shared between x86 and mips.

It no longer the case?


> Fixes: 135a67a692 ("ACPI: split CONFIG_ACPI into 4 pieces")
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  default-configs/mips-softmmu-common.mak | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/default-configs/mips-softmmu-common.mak 
> b/default-configs/mips-softmmu-common.mak
> index da29c6c0b2..e9c208da3d 100644
> --- a/default-configs/mips-softmmu-common.mak
> +++ b/default-configs/mips-softmmu-common.mak
> @@ -21,7 +21,6 @@ CONFIG_ACPI=y
>  CONFIG_ACPI_X86=y
>  CONFIG_ACPI_MEMORY_HOTPLUG=y
>  CONFIG_ACPI_NVDIMM=y
> -CONFIG_ACPI_CPU_HOTPLUG=y
>  CONFIG_APM=y
>  CONFIG_I8257=y
>  CONFIG_PIIX4=y

Re: [PATCH 1/1] pci/pcie: refuse another hotplug/unplug event if attention button is pending

[Igor Mammedov]

On Wed, 29 Jul 2020 08:09:37 +0300
Maxim Levitsky  wrote:

> On Wed, 2020-07-22 at 19:19 +0300, Maxim Levitsky wrote:
> > On Wed, 2020-07-22 at 19:17 +0300, Maxim Levitsky wrote:  
> > > Curently it is possible to hotplug a device and then immediatly
> > > hotunplug it before the OS notices, and that will result
> > > in missed unplug event since we can only send one attention button event.
> > > 
> > > Moreover the device will stuck in unplugging state forever.
> > > 
> > > Error out in such cases and rely on the caller (e.g libvirt) to retry
> > > the unplug a bit later
> > > 
> > > Signed-off-by: Maxim Levitsky 
> > > ---
> > >  hw/pci/pcie.c | 11 +++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 5b48bae0f6..9e836cf2f4 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -402,6 +402,17 @@ static void pcie_cap_slot_plug_common(PCIDevice 
> > > *hotplug_dev, DeviceState *dev,
> > >   */
> > >  error_setg_errno(errp, EBUSY, "slot is electromechanically 
> > > locked");
> > >  }
> > > +
> > > +if (sltsta & PCI_EXP_SLTSTA_ABP) {
> > > +/*
> > > + * Attention button is pressed, thus we can't send another
> > > + * hotpplug event  
> > Typo here, forgot to refresh the commit.  
> > > + */
> > > +error_setg_errno(errp, EBUSY,
> > > + "attention button is already pressed, can't "
> > > + "send another hotplug event");
> > > +}
> > > +
> > >  }
> > >  
> > >  void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState 
> > > *dev,  
> ping.

CCing Julia since she was looking into PCI hotplug/unplug code recently.

> 
> Best regards,
>   Maxim Levitsky
> 
> 

Re: [PATCH] schemas: Add vim modeline

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Thu, Jul 30, 2020 at 11:07:26AM +0200, Markus Armbruster wrote:
>> Andrea Bolognani  writes:
>> 
>> > The various schemas included in QEMU use a JSON-based format which
>> > is, however, strictly speaking not valid JSON.
>> >
>> > As a consequence, when vim tries to apply syntax highlight rules
>> > for JSON (as guessed from the file name), the result is an unreadable
>> > mess which mostly consist of red markers pointing out supposed errors
>> > in, well, pretty much everything.
>> >
>> > Using Python syntax highlighting produces much better results, and
>> > in fact these files already start with specially-formatted comments
>> > that instruct Emacs to process them as if they were Python files.
>> >
>> > This commit adds the equivalent special comments for vim.
>> >
>> > Signed-off-by: Andrea Bolognani 
>
> Given that we already have emacs mode-lines, I see no reason to
> not also have vim mode-lines, so regardless of the deeper discussion
> I think this is patch is fine to merge in the short term
>
>   Reviewed-by: Daniel P. Berrangé 
>
>
>> Naming QAPI schema files .json even though their contents isn't was a
>> mistake.  Correcting it would be a pain.  If we correct it, then the
>> sooner the better.
>> 
>> Renaming them to .py gives decent editor support out of the box.  Their
>> contents isn't quite Python, though: true vs. True, false vs. False.  Do
>> we care?  Only a few dozen occurences; they could be adjusted.
>> 
>> Renaming them to .qapi would perhaps be less confusing, for the price of
>> "out of the box".
>
> IMHO, the critical rule is that if you a pick a particular file extension
> associated with an existing language, you absolutely MUST BE compliant
> with that language.

Can't argue with that :)

> We fail at compliance with both JSON and Python because we're actually
> using our own DSL (domain specific language).
>
> IOW if we're going to stick with our current file format, then we should
> be naming them .qapi. We can still use an editor mode line if we want to
> claim we're approximately equiv to another language, but we can't be
> surprised if editors get upset.
>
>
> The bigger question is whether having our own DSL is justified ?
>
> I'm *really* sceptical that it is.

To be precise: our own DSL *syntax*.  Semantics is a separate question
to be skeptical about.

> We can't use JSON because it lacks comments. So we invented our own
> psuedo-JSON parser that supported comments, and used ' instead of "
> for some reason. We also needed to be able to parse a sequence of
> multiple JSON documents in one file. We should have just picked a 
> different language because JSON clearly didn't do what we eneeded.

JSON is a exceptionally poor choice for a DSL, or even a configuration
language.

Correcting our mistake involves a flag day and a re-learn.  We need to
weigh costs against benefits.

The QAPI schema language has two layers:

* JSON, with a lexical and a syntactical sub-layer (both in parser.py)

* QAPI, with a context-free and a context-dependend sub-layer (in
  expr.py and schema.py, respectively)

Replacing the JSON layer is possible as long as the replacement is
sufficiently expressive (not a tall order).

> You suggest naming them .py. If we do that, we must declare that they
> are literally Python code

Agree.

>   modify them so that we can load the 
> files straight into the python intepretor as code, and not parse 
> them as data. I feel unhappy about treating data as code though.

Stress on *can* load.  Doesn't mean we should.

Ancient prior art: Lisp programs routinely use s-expressions as
configuration file syntax.  They don't load them as code, they read them
as data.

With Python, it's ast.parse(), I think.

> While JSON doesn't do what we need, its second-cousin YAML is a more
> flexible format. Taking one example
>
> ---
> ##
> # @ImageInfoSpecificQCow2:
> #
> # @compat: compatibility level
> #
> # ...snip...
> #
> # Since: 1.7
> ##
> struct: ImageInfoSpecificQCow2
> data:
>   compat: str
>   "*data-file": str
>   "*data-file-raw": bool
>   "*lazy-refcounts": bool
>   "*corrupt": bool
>   refcount-bits: int
>   "*encrypt": ImageInfoSpecificQCow2Encryption
>   "*bitmaps":
> - Qcow2BitmapInfo
>   compression-type: Qcow2CompressionType
>
>
> Then we could use a regular off the shelf YAML parser in python.
>
> The uglyiness with quotes is due to the use of "*". Slightly less ugly
> if we simply declare that quotes are always used, even where they're
> not strictly required.

StrictYAML insists on quotes.

I hate having to quote identifiers.  There's a reason we don't write

'int'
'main'('int', 'argc', 'char' *'argv'[])
{
'printf'("hello world\n");
return 0;
}

> struct: ImageInfoSpecificQCow2
> data:
>   "compat": "str"
>   "*data-file": "str"
>   "*data-file-raw": "bool"
>   "*lazy-refcounts": "bool"
>   "*corrupt": "bool"
>   "refcount-bits": "int"
>   "*encrypt": "Image

[PATCH v2 2/2] target/arm: Fix compile error.

[Kaige Li]

When I compile qemu with such as:

git clone https://git.qemu.org/git/qemu.git
cd qemu
git submodule init
git submodule update --recursive
./configure
make

There is error log:

/home/LiKaige/qemu/target/arm/translate-a64.c: In function ‘disas_ldst’:
/home/LiKaige/qemu/target/arm/translate-a64.c:3392:5: error: ‘fn’ may be used 
uninitialized in this function [-Werror=maybe-uninitialized]
 fn(cpu_reg(s, rt), clean_addr, tcg_rs, get_mem_index(s),
 ^
/home/LiKaige/qemu/target/arm/translate-a64.c:3318:22: note: ‘fn’ was declared 
here
 AtomicThreeOpFn *fn;
  ^
cc1: all warnings being treated as errors

So, add an initiallization value NULL for fn to fix this.

Signed-off-by: Kaige Li 
---
 target/arm/translate-a64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 8c07649..c98dfb1 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -3315,7 +3315,7 @@ static void disas_ldst_atomic(DisasContext *s, uint32_t 
insn,
 bool r = extract32(insn, 22, 1);
 bool a = extract32(insn, 23, 1);
 TCGv_i64 tcg_rs, clean_addr;
-AtomicThreeOpFn *fn;
+AtomicThreeOpFn *fn = NULL;
 
 if (is_vector || !dc_isar_feature(aa64_atomics, s)) {
 unallocated_encoding(s);
-- 
2.1.0

[PATCH v2 1/2] virtio-mem: Change PRIx32 to PRIXPTR to fix compile error.

[Kaige Li]

When I compile qemu with such as:

git clone https://git.qemu.org/git/qemu.git
cd qemu
git submodule init
git submodule update --recursive
./configure
make

There is error log:

/home/LiKaige/qemu/hw/virtio/virtio-mem.c: In function 
‘virtio_mem_set_block_size’:
/home/LiKaige/qemu/hw/virtio/virtio-mem.c:756:9: error: format ‘%x’ expects 
argument of type ‘unsigned int’, but argument 7 has type ‘uintptr_t’ 
[-Werror=format=]
 error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, name,
 ^
cc1: all warnings being treated as errors
/home/LiKaige/qemu/rules.mak:69: recipe for target 'hw/virtio/virtio-mem.o' 
failed

So, change PRIx32 to PRIXPTR to fix this.

Signed-off-by: Kaige Li 
---
 hw/virtio/virtio-mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index c12e9f7..3dcaf9a 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -753,7 +753,7 @@ static void virtio_mem_set_block_size(Object *obj, Visitor 
*v, const char *name,
 }
 
 if (value < VIRTIO_MEM_MIN_BLOCK_SIZE) {
-error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, name,
+error_setg(errp, "'%s' property has to be at least 0x%" PRIXPTR "\n", 
name,
VIRTIO_MEM_MIN_BLOCK_SIZE);
 return;
 } else if (!is_power_of_2(value)) {
-- 
2.1.0

[PATCH 1/2] qcow2: Release read-only bitmaps when inactivated

[Max Reitz]

During migration, we release all bitmaps after storing them on disk, as
long as they are (1) stored on disk, (2) not read-only, and (3)
consistent.

(2) seems arbitrary, though.  The reason we do not release them is
because we do not write them, as there is no need to; and then we just
forget about all bitmaps that we have not written to the file.  However,
read-only persistent bitmaps are still in the file and in sync with
their in-memory representation, so we may as well release them just like
any R/W bitmap that we have updated.

It leads to actual problems, too: After migration, letting the source
continue may result in an error if there were any bitmaps on read-only
nodes (such as backing images), because those have not been released by
bdrv_inactive_all(), but bdrv_invalidate_cache_all() attempts to reload
them (which fails, because they are still present in memory).

Signed-off-by: Max Reitz 
---
 block/qcow2-bitmap.c | 23 +++
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 1f38806ca6..8c34b2aef7 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1562,11 +1562,22 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
 Qcow2Bitmap *bm;
 
 if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
-bdrv_dirty_bitmap_readonly(bitmap) ||
 bdrv_dirty_bitmap_inconsistent(bitmap)) {
 continue;
 }
 
+if (bdrv_dirty_bitmap_readonly(bitmap)) {
+/*
+ * Store the bitmap in the associated Qcow2Bitmap so it
+ * can be released later
+ */
+bm = find_bitmap_by_name(bm_list, name);
+if (bm) {
+bm->dirty_bitmap = bitmap;
+}
+continue;
+}
+
 need_write = true;
 
 if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) {
@@ -1618,7 +1629,9 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
 
 /* allocate clusters and store bitmaps */
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-if (bm->dirty_bitmap == NULL) {
+BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
+
+if (bitmap == NULL || bdrv_dirty_bitmap_readonly(bitmap)) {
 continue;
 }
 
@@ -1641,6 +1654,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
 g_free(tb);
 }
 
+success:
 if (release_stored) {
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
 if (bm->dirty_bitmap == NULL) {
@@ -1651,13 +1665,14 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
 }
 }
 
-success:
 bitmap_list_free(bm_list);
 return;
 
 fail:
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-if (bm->dirty_bitmap == NULL || bm->table.offset == 0) {
+if (bm->dirty_bitmap == NULL || bm->table.offset == 0 ||
+bdrv_dirty_bitmap_readonly(bm->dirty_bitmap))
+{
 continue;
 }
 
-- 
2.26.2

[PATCH 0/2] qcow2: Release read-only bitmaps when inactivated

[Max Reitz]

Hi,

When beginning migration, the qcow2 driver syncs all persistent bitmaps
to disk and then releases them.  If the user decides to continue on the
source after migration, those bitmaps are re-loaded from the qcow2
image.

However, we only do this for bitmaps that were actively synced, i.e. R/W
bitmaps.  RO bitmaps (those on backing images) are not written and thus
not released.  However, we still try to re-load them when continuing,
and that will then fail.

To fix this problem, I think we should just consider RO bitmaps to be in
sync from the beginning, so we can release them just like bitmaps that
we have actively written back to the image.  This is done by patch 1.

However, there’s a catch: Peter Krempa noted that it isn’t in libvirt’s
interest for the bitmaps to be released before migration at all, because
this makes them disappear from query-named-block-node’s dirty-bitmaps
list, but libvirt needs the bitmaps to be there:

https://bugzilla.redhat.com/show_bug.cgi?id=1858739#c3

If it’s really not feasible to keep the bitmaps around, then I suppose
what might work for libvirt is to query
image/format-specific/data/bitmaps in addition to dirty-bitmaps (every
bitmap that we released before migration must be a persistent bitmap).

What are your thoughts on this?


Max Reitz (2):
  qcow2: Release read-only bitmaps when inactivated
  iotests/169: Test source cont with backing bmap

 block/qcow2-bitmap.c   | 23 +++---
 tests/qemu-iotests/169 | 64 +-
 tests/qemu-iotests/169.out |  4 +--
 3 files changed, 84 insertions(+), 7 deletions(-)

-- 
2.26.2

Re: [PATCH v2 1/2] virtio-mem: Change PRIx32 to PRIXPTR to fix compile error.

[David Hildenbrand]

On 30.07.20 13:57, Kaige Li wrote:
> When I compile qemu with such as:
> 
> git clone https://git.qemu.org/git/qemu.git
> cd qemu
> git submodule init
> git submodule update --recursive
> ./configure
> make
> 
> There is error log:
> 
> /home/LiKaige/qemu/hw/virtio/virtio-mem.c: In function 
> ‘virtio_mem_set_block_size’:
> /home/LiKaige/qemu/hw/virtio/virtio-mem.c:756:9: error: format ‘%x’ expects 
> argument of type ‘unsigned int’, but argument 7 has type ‘uintptr_t’ 
> [-Werror=format=]
>  error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, name,
>  ^
> cc1: all warnings being treated as errors
> /home/LiKaige/qemu/rules.mak:69: recipe for target 'hw/virtio/virtio-mem.o' 
> failed
> 
> So, change PRIx32 to PRIXPTR to fix this.
> 
> Signed-off-by: Kaige Li 
> ---
>  hw/virtio/virtio-mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index c12e9f7..3dcaf9a 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -753,7 +753,7 @@ static void virtio_mem_set_block_size(Object *obj, 
> Visitor *v, const char *name,
>  }
>  
>  if (value < VIRTIO_MEM_MIN_BLOCK_SIZE) {
> -error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, name,
> +error_setg(errp, "'%s' property has to be at least 0x%" PRIXPTR 
> "\n", name,
> VIRTIO_MEM_MIN_BLOCK_SIZE);
>  return;
>  } else if (!is_power_of_2(value)) {
> 

That's not what I suggested ... and you should mention the compiler/host
architecture used.

-- 
Thanks,

David / dhildenb

[PATCH 2/2] iotests/169: Test source cont with backing bmap

[Max Reitz]

Test migrating from a VM with a persistent bitmap in the backing chain,
and then continuing that VM after the migration

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/169 | 64 +-
 tests/qemu-iotests/169.out |  4 +--
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index 2c5a132aa3..40afb15299 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -24,11 +24,12 @@ import time
 import itertools
 import operator
 import re
-from iotests import qemu_img
+from iotests import qemu_img, qemu_img_create, Timeout
 
 
 disk_a = os.path.join(iotests.test_dir, 'disk_a')
 disk_b = os.path.join(iotests.test_dir, 'disk_b')
+base_a = os.path.join(iotests.test_dir, 'base_a')
 size = '1M'
 mig_file = os.path.join(iotests.test_dir, 'mig_file')
 mig_cmd = 'exec: cat > ' + mig_file
@@ -234,6 +235,67 @@ for cmb in list(itertools.product((True, False), 
repeat=2)):
 inject_test_case(TestDirtyBitmapMigration, name,
  'do_test_migration_resume_source', *list(cmb))
 
+
+class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
+def setUp(self):
+qemu_img_create('-f', iotests.imgfmt, base_a, size)
+qemu_img_create('-f', iotests.imgfmt, '-F', iotests.imgfmt,
+'-b', base_a, disk_a, size)
+
+for f in (disk_a, base_a):
+qemu_img('bitmap', '--add', f, 'bmap0')
+
+blockdev = {
+'node-name': 'node0',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': disk_a
+},
+'backing': {
+'node-name': 'node0-base',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': base_a
+}
+}
+}
+
+self.vm = iotests.VM()
+self.vm.launch()
+
+result = self.vm.qmp('blockdev-add', **blockdev)
+self.assert_qmp(result, 'return', {})
+
+# Check that the bitmaps are there
+for node in self.vm.qmp('query-named-block-nodes', 
flat=True)['return']:
+if 'node0' in node['node-name']:
+self.assert_qmp(node, 'dirty-bitmaps[0]/name', 'bmap0')
+
+caps = [{'capability': 'events', 'state': True}]
+result = self.vm.qmp('migrate-set-capabilities', capabilities=caps)
+self.assert_qmp(result, 'return', {})
+
+def tearDown(self):
+self.vm.shutdown()
+for f in (disk_a, base_a):
+os.remove(f)
+
+def test_cont_on_source(self):
+"""
+Continue the source after migration.
+"""
+result = self.vm.qmp('migrate', uri=f'exec: cat > /dev/null')
+self.assert_qmp(result, 'return', {})
+
+with Timeout(10, 'Migration timeout'):
+self.vm.wait_migration('postmigrate')
+
+result = self.vm.qmp('cont')
+self.assert_qmp(result, 'return', {})
+
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2'],
  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/169.out b/tests/qemu-iotests/169.out
index 5c26d15c0d..cafb8161f7 100644
--- a/tests/qemu-iotests/169.out
+++ b/tests/qemu-iotests/169.out
@@ -1,5 +1,5 @@
-
+.
 --
-Ran 36 tests
+Ran 37 tests
 
 OK
-- 
2.26.2

Re: [PATCH v2 14/16] hw/block/nvme: consolidate qsg/iov clearing

[Minwoo Im]

> Hi Minwoo,
>
> The is that on 'exit' we release request resources (like the qsg and
> iov). On 'clear' we initialize the request by clearing the struct. I
> guess I could call it nvme_req_init instead maybe, but really - it is
> clearing it.

Yeah, I just wanted to make it clear to understand myself here.

Reviewed-by: Minwoo Im 

Re: [RFC v2 01/76] target/riscv: drop vector 0.7.1 support

[Richard Henderson]

On 7/30/20 1:07 AM, Frank Chang wrote:
> So, if it's okay, we can skip RVV v0.9 and bump to RVV v1.0 directly in our
> next version of patchset.
> Which I think may relieve the burden for the community to maintain
> multi-version of vector drafts.

That would be my preference.  Thanks,


r~

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Paolo Bonzini  writes:

> On 30/07/20 12:03, Markus Armbruster wrote:
>> qdev C layer:
>> 
>> frob->prop = 42;
>> 
>> Least cognitive load.
>> 
>> QOM has no C layer.
>
> Not really, a QOM object is totally free to do frob->prop = 42.  And
> just like we didn't do that outside device implementation in qdev as our
> tithe to the Church of Information Hiding; the same applies to QOM.

I screwed up the part of my argument that actually has a hope to be
valid, let me try again.

With qdev, you can always do frob->prop = 42, because properties are
always backed by a struct member.

With QOM, properties are built around visitor-based getters and setters.
This means you can always do (but never actually would do) something
like

fortytwo = qnum_from_int(42);
v = qobject_input_visitor_new(fortytwo);
set_prop(OBJECT(frob), v, "prop", cookie, &err);
visit_free(v);
qobject_unref(fortytwo);

where set_prop() is the setter you passed to object_property_add(), and
@cookie is the opaque value you passed along with it.

*Maybe* set_prop() wraps around a simpler setter you can call directly,
or even a struct member you can set directy.  QOM does not care.

And that's my point: QOM does not care for the C layer.

>> qdev property layer works even when @frob has incomplete type:
>> 
>> qdev_prop_set_int32(DEVICE(frob), "prop", 42);
>> 
>> This used to map property name to struct offset & copy the value.
>> Simple, stupid.
>> 
>> Nowadays, it is the same as
>> 
>> object_property_set_int(OBJECT(frob), "frob", 42, &error_abort);
>> 
>> which first converts the int to a QObject, then uses a QObject input
>> visitor with a virtual walk to convert it back to int and store it in
>> @frob.  It's quite a sight in the debugger.
>
> Yes, but thatt's just because we never bothered to create single-type
> visitors.  For a good reason though: I don't think the extra QAPI code
> is worth (not even that much) nicer backtraces when we already have a
> QObject as a battle-tested variant type.
>
>> qdev "text" layer is really a QemuOpts layer (because that's what we had
>> back then).  If we have prop=42 in a QemuOpts, it calls
>> 
>> set_property("prop", "42", frob, &err);
>> 
>> Nowadays, this is a thin wrapper around object_property_parse(),
>> basically
>> 
>> object_property_parse(frob, "prop", 42, &err);
>> 
>> Fine print: except set_property() does nothing for @prop "driver" and
>> "bus", which look just like properties in -device / device-add, but
>> aren't.
>
> Ugly indeed.  They should be special cased up in the caller, probably,
> or use the long-discussed "remainder" feature of the QAPI schema.

qdev_device_add() is still stuck in the QemuOpts age.

>> object_property_parse() uses the string input visitor, which I loathe.
>
> Apart from the list syntax, the string input visitor is decent I think.

It's a death trap:

/*
 * The string input visitor does not implement support for visiting
 * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
 * of integers (except type "size") are supported.
 */

"Does not implement support for visiting" is polite language for
"crashes when you dare to visit".

 I've long had the nagging feeling that if we had special-cased
 containers, children and links, we could have made a QOM that was easier
 to reason about, and much easier to integrate with a QAPI schema.
>>>
>>> That's at least plausible.  But I have a nagging feeling that it would
>>> only cover 99% of what we're doing with QOM. :)
>> 
>> The question is whether that 1% really should be done the way it is done
>> :)
>
> And that's a very fair question, but it implies non-trivial design work,
> so the smiley changes to a frown. :(

True!

Re: [RFC v2 28/76] target/riscv: rvv-0.9: update vext_max_elems() for load/store insns

[Richard Henderson]

On 7/22/20 2:15 AM, frank.ch...@sifive.com wrote:
> -static inline uint32_t vext_maxsz(uint32_t desc)
> +static inline uint32_t vext_max_elems(uint32_t desc, uint32_t esz, bool 
> is_ldst)
>  {
> -return simd_maxsz(desc) << vext_lmul(desc);
> +/*
> + * As simd_desc support at most 256, the max vlen is 512 bits,
> + * so vlen in bytes (vlenb) is encoded as maxsz.
> + */
> +uint32_t vlenb = simd_maxsz(desc);
> +
> +if (is_ldst) {
> +/*
> + * Vector load/store instructions have the EEW encoded
> + * directly in the instructions. The maximum vector size is
> + * calculated with EMUL rather than LMUL.
> + */
> +uint32_t eew = esz << 3;
> +uint32_t sew = vext_sew(desc);
> +float flmul = vext_vflmul(desc);
> +float emul = (float)eew / sew * flmul;
> +uint32_t emul_r = emul < 1 ? 1 : emul;
> +return vlenb * emul_r / esz;
> +} else {
> +/* Return VLMAX */
> +return vlenb * vext_vflmul(desc) / esz;
> +}
>  }

We do not want to be doing all of this arithmetic at runtime.  We want to be
doing it at translation time and pass the result to the helper.

If we must do any arithmetic at runtime, we would very much prefer to pass
log2(esz) so that we can use shifts instead of full integer division.

We really really want to avoid a bunch of floating-point conversions and
operations.

If you need to adjust the vext descriptor to make this happen, do so.  Do not
feel that load/store needs to pass the *same* descriptor to the helpers as
everything else.


r~

Re: [PATCH] virtio-mem: Work around format specifier mismatch for RISC-V

[Bruce Rogers]

On Thu, 2020-07-30 at 10:12 +0200, David Hildenbrand wrote:
> On 30.07.20 09:58, Stefano Garzarella wrote:
> > On Thu, Jul 30, 2020 at 09:51:19AM +0200, David Hildenbrand wrote:
> > > On 30.07.20 09:49, Stefano Garzarella wrote:
> > > > On Wed, Jul 29, 2020 at 06:54:38PM -0600, Bruce Rogers wrote:
> > > > > This likely affects other, less popular host architectures as
> > > > > well.
> > > > > Less common host architectures under linux get
> > > > > QEMU_VMALLOC_ALIGN (from
> > > > > which VIRTIO_MEM_MIN_BLOCK_SIZE is derived) define to a
> > > > > variable of
> > > > > type uintptr, which isn't compatible with the format
> > > > > specifier used to
> > > > > print a user message. Since this particular usage of the
> > > > > underlying data
> > > > > seems unique, the simple fix is to just cast it to the
> > > > > corresponding
> > > > > format specifier.
> > > > > 
> > > > > Signed-off-by: Bruce Rogers 
> > > > > ---
> > > > >  hw/virtio/virtio-mem.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> > > > > index c12e9f79b0..fd01ffd83e 100644
> > > > > --- a/hw/virtio/virtio-mem.c
> > > > > +++ b/hw/virtio/virtio-mem.c
> > > > > @@ -754,7 +754,7 @@ static void
> > > > > virtio_mem_set_block_size(Object *obj, Visitor *v, const char
> > > > > *name,
> > > > >  
> > > > >  if (value < VIRTIO_MEM_MIN_BLOCK_SIZE) {
> > > > >  error_setg(errp, "'%s' property has to be at least
> > > > > 0x%" PRIx32, name,
> > > > > -   VIRTIO_MEM_MIN_BLOCK_SIZE);
> > > > > +   (unsigned int)VIRTIO_MEM_MIN_BLOCK_SIZE);
> > > > 
> > > > Since we use PRIx32, could be better to cast
> > > > VIRTIO_MEM_MIN_BLOCK_SIZE
> > > > to uint32_t?
> > > 
> > > Yeah, I guess something like
> > > 
> > > -#define VIRTIO_MEM_MIN_BLOCK_SIZE QEMU_VMALLOC_ALIGN
> > > +#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)
> > > 
> > > would be cleaner
> > 
> > Yeah, it is cleaner.
> 
> Bruce, can you respin if you agree? Thanks!
> 
Agreed.

- Bruce

Re: [RFC v2 29/76] target/riscv: rvv-0.9: take fractional LMUL into vector max elements calculation

[Richard Henderson]

On 7/22/20 2:15 AM, frank.ch...@sifive.com wrote:
> -/*
> - * A simplification for VLMAX
> - * = (1 << LMUL) * VLEN / (8 * (1 << SEW))
> - * = (VLEN << LMUL) / (8 << SEW)
> - * = (VLEN << LMUL) >> (SEW + 3)
> - * = VLEN >> (SEW + 3 - LMUL)
> - */
>  static inline uint32_t vext_get_vlmax(RISCVCPU *cpu, target_ulong vtype)
>  {
>  uint8_t sew, lmul;
> -
>  sew = FIELD_EX64(vtype, VTYPE, VSEW);
> -lmul = FIELD_EX64(vtype, VTYPE, VLMUL);
> -return cpu->cfg.vlen >> (sew + 3 - lmul);
> +lmul = (FIELD_EX64(vtype, VTYPE, VFLMUL) << 2)
> +| FIELD_EX64(vtype, VTYPE, VLMUL);
> +float flmul = flmul_table[lmul];
> +return cpu->cfg.vlen * flmul / (1 << (sew + 3));
>  }

I think if you encode lmul differently, the original formulation can still work.

E.g. LMUL = 1   -> lmul = 0
 LMUL = 2   -> lmul = 1
 LMUL = 1/2 -> lmul = -1

so that, for SEW=8 and LMUL=1/2 we get

  cfg.vlen >> (0 + 3 - (-1))
= cfg.vlen >> (0 + 3 + 1)
= cfg.vlen >> 4

Which neatly avoids the floating-point calculation that I don't like.


r~

Re: [PATCH v2 1/2] virtio-mem: Change PRIx32 to PRIXPTR to fix compile error.

[David Hildenbrand]

On 30.07.20 14:03, David Hildenbrand wrote:
> On 30.07.20 13:57, Kaige Li wrote:
>> When I compile qemu with such as:
>>
>> git clone https://git.qemu.org/git/qemu.git
>> cd qemu
>> git submodule init
>> git submodule update --recursive
>> ./configure
>> make
>>
>> There is error log:
>>
>> /home/LiKaige/qemu/hw/virtio/virtio-mem.c: In function 
>> ‘virtio_mem_set_block_size’:
>> /home/LiKaige/qemu/hw/virtio/virtio-mem.c:756:9: error: format ‘%x’ expects 
>> argument of type ‘unsigned int’, but argument 7 has type ‘uintptr_t’ 
>> [-Werror=format=]
>>  error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, 
>> name,
>>  ^
>> cc1: all warnings being treated as errors
>> /home/LiKaige/qemu/rules.mak:69: recipe for target 'hw/virtio/virtio-mem.o' 
>> failed
>>
>> So, change PRIx32 to PRIXPTR to fix this.
>>
>> Signed-off-by: Kaige Li 
>> ---
>>  hw/virtio/virtio-mem.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
>> index c12e9f7..3dcaf9a 100644
>> --- a/hw/virtio/virtio-mem.c
>> +++ b/hw/virtio/virtio-mem.c
>> @@ -753,7 +753,7 @@ static void virtio_mem_set_block_size(Object *obj, 
>> Visitor *v, const char *name,
>>  }
>>  
>>  if (value < VIRTIO_MEM_MIN_BLOCK_SIZE) {
>> -error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, 
>> name,
>> +error_setg(errp, "'%s' property has to be at least 0x%" PRIXPTR 
>> "\n", name,
>> VIRTIO_MEM_MIN_BLOCK_SIZE);
>>  return;
>>  } else if (!is_power_of_2(value)) {
>>
> 
> That's not what I suggested ... and you should mention the compiler/host
> architecture used.
> 

Sorry, I thought this was a resend from Bruce:

https://lkml.kernel.org/r/1b00c0e25ec65e113d4c7fa98b1466689f05a986.ca...@suse.com

-- 
Thanks,

David / dhildenb

Any reason VIRTQUEUE_MAX_SIZE is 1024? Can we increase this limit?

[Yajun Wu]

Hi all,

I'm doing iperf test on VIRTIO net through vhost-user(HW VDPA).
Find maximal acceptable tx_queue_size/rx_queue_size is 1024.
Basically increase queue size can get better RX rate for my case.

Can we increase the limit(VIRTQUEUE_MAX_SIZE) to 8192 to possibly gain better 
performance?

Thanks,
Yajun

[PATCH v2 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Halil Pasic]

As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1)
reads one past of the end of ms->loadparm, so g_memdup() can not be used
here.

Let's use g_strndup instead!

Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter")
Fixes: Coverity CID 1431058
Reported-by: Peter Maydell 
Signed-off-by: Halil Pasic 
---
 hw/s390x/s390-virtio-ccw.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 403d30e13b..e72c61d2ea 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -701,12 +701,9 @@ bool hpage_1m_allowed(void)
 static char *machine_get_loadparm(Object *obj, Error **errp)
 {
 S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
-char *loadparm_str;
 
 /* make a NUL-terminated string */
-loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
-loadparm_str[sizeof(ms->loadparm)] = 0;
-return loadparm_str;
+return g_strndup((char *) ms->loadparm, sizeof(ms->loadparm));
 }
 
 static void machine_set_loadparm(Object *obj, const char *val, Error **errp)

base-commit: 5772f2b1fc5d00e7e04e01fa28e9081d6550440a
-- 
2.17.1

Re: [RFC v2 30/76] target/riscv: rvv-0.9: floating-point square-root instruction

[Richard Henderson]

On 7/22/20 2:15 AM, frank.ch...@sifive.com wrote:
> From: Frank Chang 
> 
> Signed-off-by: Frank Chang 
> ---
>  target/riscv/insn32.decode | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

Re: [RFC v2 31/76] target/riscv: rvv-0.9: floating-point classify instructions

[Richard Henderson]

On 7/22/20 2:15 AM, frank.ch...@sifive.com wrote:
> From: Frank Chang 
> 
> Signed-off-by: Frank Chang 
> ---
>  target/riscv/insn32.decode | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

Re: [RFC v2 32/76] target/riscv: rvv-0.9: mask population count instruction

[Richard Henderson]

On 7/22/20 2:15 AM, frank.ch...@sifive.com wrote:
> From: Frank Chang 
> 
> Signed-off-by: Frank Chang 
> ---
>  target/riscv/helper.h   | 2 +-
>  target/riscv/insn32.decode  | 2 +-
>  target/riscv/insn_trans/trans_rvv.inc.c | 7 ---
>  target/riscv/vector_helper.c| 6 +++---
>  4 files changed, 9 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson 

r~

[PATCH] virtio-mem: Correct format specifier mismatch for RISC-V

[Bruce Rogers]

This likely affects other, less popular host architectures as well.
Less common host architectures under linux get QEMU_VMALLOC_ALIGN (from
which VIRTIO_MEM_MIN_BLOCK_SIZE is derived) define to a variable of
type uintptr, which isn't compatible with the format specifier used to
print a user message. Since this particular usage of the underlying data
seems unique to this file, the simple fix is to just cast
QEMU_VMALLOC_ALIGN to uint32_t, which corresponds to the format specifier
used.

Signed-off-by: Bruce Rogers 
---
 hw/virtio/virtio-mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index c12e9f79b0..7740fc613f 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -36,7 +36,7 @@
  * Use QEMU_VMALLOC_ALIGN, so no THP will have to be split when unplugging
  * memory (e.g., 2MB on x86_64).
  */
-#define VIRTIO_MEM_MIN_BLOCK_SIZE QEMU_VMALLOC_ALIGN
+#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)
 /*
  * Size the usable region bigger than the requested size if possible. Esp.
  * Linux guests will only add (aligned) memory blocks in case they fully
-- 
2.27.0

Re: [PATCH v2 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[Peter Maydell]

On Thu, 30 Jul 2020 at 14:02, Halil Pasic  wrote:
>
> As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1)
> reads one past of the end of ms->loadparm, so g_memdup() can not be used
> here.
>
> Let's use g_strndup instead!
>
> Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter")
> Fixes: Coverity CID 1431058
> Reported-by: Peter Maydell 
> Signed-off-by: Halil Pasic 
> ---
>  hw/s390x/s390-virtio-ccw.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 403d30e13b..e72c61d2ea 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -701,12 +701,9 @@ bool hpage_1m_allowed(void)
>  static char *machine_get_loadparm(Object *obj, Error **errp)
>  {
>  S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> -char *loadparm_str;
>
>  /* make a NUL-terminated string */
> -loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
> -loadparm_str[sizeof(ms->loadparm)] = 0;
> -return loadparm_str;
> +return g_strndup((char *) ms->loadparm, sizeof(ms->loadparm));
>  }

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH v2 1/1] s390x/s390-virtio-ccw: fix off-by-one in loadparm getter

[David Hildenbrand]

On 30.07.20 15:01, Halil Pasic wrote:
> As pointed out by Peter, g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1)
> reads one past of the end of ms->loadparm, so g_memdup() can not be used
> here.
> 
> Let's use g_strndup instead!
> 
> Fixes: d664548328 ("s390x/s390-virtio-ccw: fix loadparm property getter")
> Fixes: Coverity CID 1431058
> Reported-by: Peter Maydell 
> Signed-off-by: Halil Pasic 
> ---
>  hw/s390x/s390-virtio-ccw.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 403d30e13b..e72c61d2ea 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -701,12 +701,9 @@ bool hpage_1m_allowed(void)
>  static char *machine_get_loadparm(Object *obj, Error **errp)
>  {
>  S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> -char *loadparm_str;
>  
>  /* make a NUL-terminated string */
> -loadparm_str = g_memdup(ms->loadparm, sizeof(ms->loadparm) + 1);
> -loadparm_str[sizeof(ms->loadparm)] = 0;
> -return loadparm_str;
> +return g_strndup((char *) ms->loadparm, sizeof(ms->loadparm));
>  }
>  
>  static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
> 
> base-commit: 5772f2b1fc5d00e7e04e01fa28e9081d6550440a
> 

Reviewed-by: David Hildenbrand 

-- 
Thanks,

David / dhildenb

Re: [PATCH] qapi: Delete unwanted indentation of top-level expressions

[Max Reitz]

On 30.07.20 11:16, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/block-core.json | 24 
>  qapi/ui.json |  4 ++--
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ab7bf3c612..bdcc8e5f9f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1847,8 +1847,8 @@
>  #
>  # Since: 4.0
>  ##
> -  { 'enum': 'BlockPermission',
> -'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize',
> +{ 'enum': 'BlockPermission',
> +  'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize',
>'graph-mod' ] }

Do we want to keep the alignment at the opening parenthesis here?

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] schemas: Add vim modeline

[Markus Armbruster]

Andrea Bolognani  writes:

> The various schemas included in QEMU use a JSON-based format which
> is, however, strictly speaking not valid JSON.
>
> As a consequence, when vim tries to apply syntax highlight rules
> for JSON (as guessed from the file name), the result is an unreadable
> mess which mostly consist of red markers pointing out supposed errors
> in, well, pretty much everything.
>
> Using Python syntax highlighting produces much better results, and
> in fact these files already start with specially-formatted comments
> that instruct Emacs to process them as if they were Python files.
>
> This commit adds the equivalent special comments for vim.
>
> Signed-off-by: Andrea Bolognani 

Fixing the real mistake would be better, but until then, mitigating it
helps, like the existing Emacs modelines do.

Queued, thanks!

Re: [RFC v2 33/76] target/riscv: rvv-0.9: find-first-set mask bit instruction

[Richard Henderson]

On 7/22/20 2:15 AM, frank.ch...@sifive.com wrote:
> From: Frank Chang 
> 
> Signed-off-by: Frank Chang 
> ---
>  target/riscv/helper.h   | 2 +-
>  target/riscv/insn32.decode  | 2 +-
>  target/riscv/insn_trans/trans_rvv.inc.c | 4 ++--
>  target/riscv/vector_helper.c| 6 +++---
>  4 files changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [RFC v2 2/3] libvhost-user: handle endianness as mandated by the spec

[Marc Hartmayer]

On Tue, Jul 21, 2020 at 01:48 PM +0100, Stefan Hajnoczi  
wrote:
> On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:
>> Since virtio existed even before it got standardized, the virtio
>> standard defines the following types of virtio devices:
>> 
>>  + legacy device (pre-virtio 1.0)
>>  + non-legacy or VIRTIO 1.0 device
>>  + transitional device (which can act both as legacy and non-legacy)
>> 
>> Virtio 1.0 defines the fields of the virtqueues as little endian,
>> while legacy uses guest's native endian [1]. Currently libvhost-user
>> does not handle virtio endianness at all, i.e. it works only if the
>> native endianness matches with whatever is actually needed. That means
>> things break spectacularly on big-endian targets. Let us handle virtio
>> endianness for non-legacy as required by the virtio specification
>> [1]. We will fence non-legacy virtio devices with the upcoming patch.
>> 
>> [1] 
>> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003
>> 
>> Signed-off-by: Marc Hartmayer 
>> 
>> ---
>> Note: As we don't support legacy virtio devices there is dead code in
>> libvhost-access.h that could be removed. But for the sake of
>> completeness, I left it in the code.
>
> Please remove the dead code. It is unlikely that legacy device support
> will be required in the future and it will just confuse people reading
> the code.

Done.

>
>> ---
>>  contrib/libvhost-user/libvhost-access.h |  61 
>>  contrib/libvhost-user/libvhost-user.c   | 119 
>>  2 files changed, 121 insertions(+), 59 deletions(-)
>>  create mode 100644 contrib/libvhost-user/libvhost-access.h
>> 
>> diff --git a/contrib/libvhost-user/libvhost-access.h 
>> b/contrib/libvhost-user/libvhost-access.h
>> new file mode 100644
>> index ..868ba3e7bfb8
>> --- /dev/null
>> +++ b/contrib/libvhost-user/libvhost-access.h
>> @@ -0,0 +1,61 @@
>> +#ifndef LIBVHOST_ACCESS_H
>
> License/copyright header?

With the dead code removed there is no reason for having
libvhost-access.h so I’ve removed it.

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v2 1/2] virtio-mem: Change PRIx32 to PRIXPTR to fix compile error.

[Philippe Mathieu-Daudé]

On 7/30/20 1:57 PM, Kaige Li wrote:
> When I compile qemu with such as:
> 
> git clone https://git.qemu.org/git/qemu.git
> cd qemu
> git submodule init
> git submodule update --recursive
> ./configure
> make

^ this timeless description is pointless (think at a developer
who read this in 2 weeks, 3 months, 1 year).

> 
> There is error log:
> 
> /home/LiKaige/qemu/hw/virtio/virtio-mem.c: In function 
> ‘virtio_mem_set_block_size’:
> /home/LiKaige/qemu/hw/virtio/virtio-mem.c:756:9: error: format ‘%x’ expects 
> argument of type ‘unsigned int’, but argument 7 has type ‘uintptr_t’ 
> [-Werror=format=]

What compiler are you using? That is the relevant information to
include.

>  error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, name,
>  ^
> cc1: all warnings being treated as errors
> /home/LiKaige/qemu/rules.mak:69: recipe for target 'hw/virtio/virtio-mem.o' 
> failed
> 
> So, change PRIx32 to PRIXPTR to fix this.
> 
> Signed-off-by: Kaige Li 
> ---
>  hw/virtio/virtio-mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index c12e9f7..3dcaf9a 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -753,7 +753,7 @@ static void virtio_mem_set_block_size(Object *obj, 
> Visitor *v, const char *name,
>  }
>  
>  if (value < VIRTIO_MEM_MIN_BLOCK_SIZE) {
> -error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, name,
> +error_setg(errp, "'%s' property has to be at least 0x%" PRIXPTR 
> "\n", name,
> VIRTIO_MEM_MIN_BLOCK_SIZE);
>  return;
>  } else if (!is_power_of_2(value)) {
> 

Re: device compatibility interface for live migration with assigned devices

[Sean Mooney]

On Thu, 2020-07-30 at 09:56 +0800, Yan Zhao wrote:
> On Wed, Jul 29, 2020 at 12:28:46PM +0100, Sean Mooney wrote:
> > On Wed, 2020-07-29 at 16:05 +0800, Yan Zhao wrote:
> > > On Mon, Jul 27, 2020 at 04:23:21PM -0600, Alex Williamson wrote:
> > > > On Mon, 27 Jul 2020 15:24:40 +0800
> > > > Yan Zhao  wrote:
> > > > 
> > > > > > > As you indicate, the vendor driver is responsible for checking 
> > > > > > > version
> > > > > > > information embedded within the migration stream.  Therefore a
> > > > > > > migration should fail early if the devices are incompatible.  Is 
> > > > > > > it  
> > > > > > 
> > > > > > but as I know, currently in VFIO migration protocol, we have no way 
> > > > > > to
> > > > > > get vendor specific compatibility checking string in migration 
> > > > > > setup stage
> > > > > > (i.e. .save_setup stage) before the device is set to _SAVING state.
> > > > > > In this way, for devices who does not save device data in precopy 
> > > > > > stage,
> > > > > > the migration compatibility checking is as late as in stop-and-copy
> > > > > > stage, which is too late.
> > > > > > do you think we need to add the getting/checking of vendor specific
> > > > > > compatibility string early in save_setup stage?
> > > > > >  
> > > > > 
> > > > > hi Alex,
> > > > > after an offline discussion with Kevin, I realized that it may not be 
> > > > > a
> > > > > problem if migration compatibility check in vendor driver occurs late 
> > > > > in
> > > > > stop-and-copy phase for some devices, because if we report device
> > > > > compatibility attributes clearly in an interface, the chances for
> > > > > libvirt/openstack to make a wrong decision is little.
> > > > 
> > > > I think it would be wise for a vendor driver to implement a pre-copy
> > > > phase, even if only to send version information and verify it at the
> > > > target.  Deciding you have no device state to send during pre-copy does
> > > > not mean your vendor driver needs to opt-out of the pre-copy phase
> > > > entirely.  Please also note that pre-copy is at the user's discretion,
> > > > we've defined that we can enter stop-and-copy at any point, including
> > > > without a pre-copy phase, so I would recommend that vendor drivers
> > > > validate compatibility at the start of both the pre-copy and the
> > > > stop-and-copy phases.
> > > > 
> > > 
> > > ok. got it!
> > > 
> > > > > so, do you think we are now arriving at an agreement that we'll give 
> > > > > up
> > > > > the read-and-test scheme and start to defining one interface (perhaps 
> > > > > in
> > > > > json format), from which libvirt/openstack is able to parse and find 
> > > > > out
> > > > > compatibility list of a source mdev/physical device?
> > > > 
> > > > Based on the feedback we've received, the previously proposed interface
> > > > is not viable.  I think there's agreement that the user needs to be
> > > > able to parse and interpret the version information.  Using json seems
> > > > viable, but I don't know if it's the best option.  Is there any
> > > > precedent of markup strings returned via sysfs we could follow?
> > > 
> > > I found some examples of using formatted string under /sys, mostly under
> > > tracing. maybe we can do a similar implementation.
> > > 
> > > #cat /sys/kernel/debug/tracing/events/kvm/kvm_mmio/format
> > > 
> > > name: kvm_mmio
> > > ID: 32
> > > format:
> > > field:unsigned short common_type;   offset:0;   size:2; 
> > > signed:0;
> > > field:unsigned char common_flags;   offset:2;   size:1; 
> > > signed:0;
> > > field:unsigned char common_preempt_count;   offset:3;   
> > > size:1; signed:0;
> > > field:int common_pid;   offset:4;   size:4; signed:1;
> > > 
> > > field:u32 type; offset:8;   size:4; signed:0;
> > > field:u32 len;  offset:12;  size:4; signed:0;
> > > field:u64 gpa;  offset:16;  size:8; signed:0;
> > > field:u64 val;  offset:24;  size:8; signed:0;
> > > 
> > > print fmt: "mmio %s len %u gpa 0x%llx val 0x%llx", 
> > > __print_symbolic(REC->type, { 0, "unsatisfied-read" }, { 1,
> > > "read"
> > > }, { 2, "write" }), REC->len, REC->gpa, REC->val
> > > 
> > 
> > this is not json fromat and its not supper frendly to parse.
> 
> yes, it's just an example. It's exported to be used by userspace perf &
> trace_cmd.
> 
> > > 
> > > #cat /sys/devices/pci:00/:00:02.0/uevent
> > > DRIVER=vfio-pci
> > > PCI_CLASS=3
> > > PCI_ID=8086:591D
> > > PCI_SUBSYS_ID=8086:2212
> > > PCI_SLOT_NAME=:00:02.0
> > > MODALIAS=pci:v8086d591Dsv8086sd2212bc03sc00i00
> > > 
> > 
> > this is ini format or conf formant 
> > this is pretty simple to parse whichi would be fine.
> > that said you could also have a version or capablitiy directory with a file
> > for each key and a singel value.
> > 
> 
> if this is easy for openstack, maybe we can organize the data like below way?
>  
>  |- [device]
> |- migration
>

Re: [PATCH v2 2/2] target/arm: Fix compile error.

[Philippe Mathieu-Daudé]

On 7/30/20 1:57 PM, Kaige Li wrote:
> When I compile qemu with such as:
> 
> git clone https://git.qemu.org/git/qemu.git
> cd qemu
> git submodule init
> git submodule update --recursive
> ./configure
> make

Again, timeless information is not useful.

> 
> There is error log:
> 
> /home/LiKaige/qemu/target/arm/translate-a64.c: In function ‘disas_ldst’:
> /home/LiKaige/qemu/target/arm/translate-a64.c:3392:5: error: ‘fn’ may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
>  fn(cpu_reg(s, rt), clean_addr, tcg_rs, get_mem_index(s),
>  ^
> /home/LiKaige/qemu/target/arm/translate-a64.c:3318:22: note: ‘fn’ was 
> declared here
>  AtomicThreeOpFn *fn;
>   ^
> cc1: all warnings being treated as errors

Again, what compiler / version are you using? My guess is you are
using an old GCC, and I wonder if it is still supported.

> 
> So, add an initiallization value NULL for fn to fix this.
> 
> Signed-off-by: Kaige Li 
> ---
>  target/arm/translate-a64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 8c07649..c98dfb1 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -3315,7 +3315,7 @@ static void disas_ldst_atomic(DisasContext *s, uint32_t 
> insn,
>  bool r = extract32(insn, 22, 1);
>  bool a = extract32(insn, 23, 1);
>  TCGv_i64 tcg_rs, clean_addr;
> -AtomicThreeOpFn *fn;
> +AtomicThreeOpFn *fn = NULL;
>  
>  if (is_vector || !dc_isar_feature(aa64_atomics, s)) {
>  unallocated_encoding(s);
> 

Re: device compatibility interface for live migration with assigned devices

[Sean Mooney]

On Thu, 2020-07-30 at 11:41 +0800, Yan Zhao wrote:
> > > >interface_version=3
> > 
> > Not much granularity here, I prefer Sean's previous
> > .[.bugfix] scheme.
> > 
> 
> yes, .[.bugfix] scheme may be better, but I'm not sure if
> it works for a complicated scenario.
> e.g for pv_mode,
> (1) initially,  pv_mode is not supported, so it's pv_mode=none, it's 0.0.0,
> (2) then, pv_mode=ppgtt is supported, pv_mode="none+ppgtt", it's 0.1.0,
> indicating pv_mode=none can migrate to pv_mode="none+ppgtt", but not vice 
> versa.
> (3) later, pv_mode=context is also supported,
> pv_mode="none+ppgtt+context", so it's 0.2.0.
> 
> But if later, pv_mode=ppgtt is removed. pv_mode="none+context", how to
> name its version?
it would become 1.0.0
addtion of a feature is a minor version bump as its backwards compatiable.
if you dont request the new feature you dont need to use it and it can continue 
to behave like
a 0.0.0 device evne if its capably of acting as a 0.1.0 device.
when you remove a feature that is backward incompatable as any isnstance that 
was prevously not
using it would nolonger work so you have to bump the major version.
>  "none+ppgtt" (0.1.0) is not compatible to
> "none+context", but "none+ppgtt+context" (0.2.0) is compatible to
> "none+context".
> 
> Maintain such scheme is painful to vendor driver.
not really its how most software libs are version today. some use other schemes
but semantic versioning is don right is a concies and easy to consume set of 
rules
https://semver.org/ however you are right that it forcnes vendor to think about 
backwards
and forwards compatiablty with each change which for the most part is a good 
thing.
it goes hand in hand with have stable abi and api definitons to ensuring 
firmware updates and driver chagnes
dont break userspace that depend on the kernel interfaces they expose.

Re: [PATCH] virtio-mem: Correct format specifier mismatch for RISC-V

[David Hildenbrand]

On 30.07.20 15:05, Bruce Rogers wrote:
> This likely affects other, less popular host architectures as well.
> Less common host architectures under linux get QEMU_VMALLOC_ALIGN (from
> which VIRTIO_MEM_MIN_BLOCK_SIZE is derived) define to a variable of
> type uintptr, which isn't compatible with the format specifier used to
> print a user message. Since this particular usage of the underlying data
> seems unique to this file, the simple fix is to just cast
> QEMU_VMALLOC_ALIGN to uint32_t, which corresponds to the format specifier
> used.
> 
> Signed-off-by: Bruce Rogers 
> ---
>  hw/virtio/virtio-mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index c12e9f79b0..7740fc613f 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -36,7 +36,7 @@
>   * Use QEMU_VMALLOC_ALIGN, so no THP will have to be split when unplugging
>   * memory (e.g., 2MB on x86_64).
>   */
> -#define VIRTIO_MEM_MIN_BLOCK_SIZE QEMU_VMALLOC_ALIGN
> +#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)
>  /*
>   * Size the usable region bigger than the requested size if possible. Esp.
>   * Linux guests will only add (aligned) memory blocks in case they fully
> 

We should probably force that to always at least 1MB or so ... other
discussion :)

Thanks (compile-tested on x86-64)!

Acked-by: David Hildenbrand 

-- 
Thanks,

David / dhildenb

Re: [PATCH] schemas: Add vim modeline

[Daniel P . Berrangé]

On Thu, Jul 30, 2020 at 01:51:10PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> >   modify them so that we can load the 
> > files straight into the python intepretor as code, and not parse 
> > them as data. I feel unhappy about treating data as code though.
> 
> Stress on *can* load.  Doesn't mean we should.
> 
> Ancient prior art: Lisp programs routinely use s-expressions as
> configuration file syntax.  They don't load them as code, they read them
> as data.
> 
> With Python, it's ast.parse(), I think.

Yes, that could work


> > struct: ImageInfoSpecificQCow2
> > data:
> >   compat: str
> >   "*data-file": str
> >   "*data-file-raw": bool
> >   "*lazy-refcounts": bool
> >   "*corrupt": bool
> >   refcount-bits: int
> >   "*encrypt": ImageInfoSpecificQCow2Encryption
> >   "*bitmaps":
> > - Qcow2BitmapInfo
> >   compression-type: Qcow2CompressionType
> >
> >
> > Then we could use a regular off the shelf YAML parser in python.
> >
> > The uglyiness with quotes is due to the use of "*". Slightly less ugly
> > if we simply declare that quotes are always used, even where they're
> > not strictly required.
> 
> StrictYAML insists on quotes.

I wouldn't suggest StrictYAML, just normal YAML is what pretty much
everyone uses.

If we came up with a different way to mark a field as optional
instead of using the magic "*" then we wouldn't need to quote
anything

> I hate having to quote identifiers.  There's a reason we don't write
> 
> 'int'
> 'main'('int', 'argc', 'char' *'argv'[])
> {
> 'printf'("hello world\n");
> return 0;
> }
> 
> > struct: ImageInfoSpecificQCow2
> > data:
> >   "compat": "str"
> >   "*data-file": "str"
> >   "*data-file-raw": "bool"
> >   "*lazy-refcounts": "bool"
> >   "*corrupt": "bool"
> >   "refcount-bits": "int"
> >   "*encrypt": "ImageInfoSpecificQCow2Encryption"
> >   "*bitmaps":
> > - "Qcow2BitmapInfo"
> >   "compression-type": "Qcow2CompressionType"
> >
> > With the use of "---" to denote the start of document, we have no trouble 
> > parsing our files which would actually be a concatenation of multiple 
> > documents. The python YAML library provides the easy yaml.load_all()
> > method.
> 
> Required reading on YAML:
> https://www.arp242.net/yaml-config.html

I don't think this is especially helpful to our evaluation. You can write
such blog posts about pretty much any thing if you want to pick holes in a
proposal. Certainly there's plenty of awful stuff you can write about
JSON, and Python.

> Some of the criticism there doesn't matter for our use case.

Yeah, what matters is whether it can do the job we need in a way that is
better than what we have today, and whether there are any further options
to consider that might be viable alternatives.

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: [RFC v2 34/76] target/riscv: rvv-0.9: set-X-first mask bit instructions

[Richard Henderson]

On 7/22/20 2:15 AM, frank.ch...@sifive.com wrote:
> From: Frank Chang 
> 
> Signed-off-by: Frank Chang 
> ---
>  target/riscv/insn32.decode  | 6 +++---
>  target/riscv/insn_trans/trans_rvv.inc.c | 5 -
>  target/riscv/vector_helper.c| 4 
>  3 files changed, 7 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH] virtio-mem: Correct format specifier mismatch for RISC-V

[Stefano Garzarella]

On Thu, Jul 30, 2020 at 07:05:19AM -0600, Bruce Rogers wrote:
> This likely affects other, less popular host architectures as well.
> Less common host architectures under linux get QEMU_VMALLOC_ALIGN (from
> which VIRTIO_MEM_MIN_BLOCK_SIZE is derived) define to a variable of
> type uintptr, which isn't compatible with the format specifier used to
> print a user message. Since this particular usage of the underlying data
> seems unique to this file, the simple fix is to just cast
> QEMU_VMALLOC_ALIGN to uint32_t, which corresponds to the format specifier
> used.
> 
> Signed-off-by: Bruce Rogers 
> ---
>  hw/virtio/virtio-mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index c12e9f79b0..7740fc613f 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -36,7 +36,7 @@
>   * Use QEMU_VMALLOC_ALIGN, so no THP will have to be split when unplugging
>   * memory (e.g., 2MB on x86_64).
>   */
> -#define VIRTIO_MEM_MIN_BLOCK_SIZE QEMU_VMALLOC_ALIGN
> +#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)
>  /*
>   * Size the usable region bigger than the requested size if possible. Esp.
>   * Linux guests will only add (aligned) memory blocks in case they fully
> -- 
> 2.27.0
> 

Reviewed-by: Stefano Garzarella 

Thanks,
Stefano

Re: [RFC v2 35/76] target/riscv: rvv-0.9: iota instruction

[Richard Henderson]

On 7/22/20 2:15 AM, frank.ch...@sifive.com wrote:
> From: Frank Chang 
> 
> Signed-off-by: Frank Chang 
> ---
>  target/riscv/insn32.decode   | 2 +-
>  target/riscv/vector_helper.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [RFC v2 36/76] target/riscv: rvv-0.9: element index instruction

[Richard Henderson]

On 7/22/20 2:15 AM, frank.ch...@sifive.com wrote:
> From: Frank Chang 
> 
> Signed-off-by: Frank Chang 
> ---
>  target/riscv/insn32.decode | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

Re: sysbus_create_simple Vs qdev_create

[Paolo Bonzini]

On 30/07/20 14:36, Markus Armbruster wrote:
> Paolo Bonzini  writes:
> 
>> On 30/07/20 12:03, Markus Armbruster wrote:
>>> qdev C layer:
>>>
>>> frob->prop = 42;
>>>
>>> Least cognitive load.
>>>
>>> QOM has no C layer.
>>
>> Not really, a QOM object is totally free to do frob->prop = 42.  And
>> just like we didn't do that outside device implementation in qdev as our
>> tithe to the Church of Information Hiding; the same applies to QOM.
> 
> I screwed up the part of my argument that actually has a hope to be
> valid, let me try again.
> 
> With qdev, you can always do frob->prop = 42, because properties are
> always backed by a struct member.
> 
> With QOM, properties are built around visitor-based getters and setters.
> This means you can always do (but never actually would do) something
> like
> 
> fortytwo = qnum_from_int(42);
> v = qobject_input_visitor_new(fortytwo);
> set_prop(OBJECT(frob), v, "prop", cookie, &err);
> visit_free(v);
> qobject_unref(fortytwo);
> 
> where set_prop() is the setter you passed to object_property_add(), and
> @cookie is the opaque value you passed along with it.
> 
> *Maybe* set_prop() wraps around a simpler setter you can call directly,
> or even a struct member you can set directy.  QOM does not care.
> 
> And that's my point: QOM does not care for the C layer.

Ok, I think I got it.  In practice it depends on how much you want to
hide the implementation of the properties and (especially for
set-only-before-realize properties) the answer will be "not at all
inside the device implementation".  So inside the implementation you can
always break the information hiding layer if needed (usually for
performance or as you point out sanity), and I don't think there is a
substantial difference between qdev and QOM.

But yeah, I'm willing to concede that QOM the QAPI-- layer of QOM comes
first and the C layer comes second. :)

Paolo

Re: [PATCH for-5.2 v4 05/11] hw/arm/smmu-common: Manage IOTLB block entries

[Peter Maydell]

On Tue, 28 Jul 2020 at 16:09, Eric Auger  wrote:
>
> At the moment each entry in the IOTLB corresponds to a page sized
> mapping (4K, 16K or 64K), even if the page belongs to a mapped
> block. In case of block mapping this unefficiently consumes IOTLB
> entries.
>
> Change the value of the entry so that it reflects the actual
> mapping it belongs to (block or page start address and size).
>
> Also the level/tg of the entry is encoded in the key. In subsequent
> patches we will enable range invalidation. This latter is able
> to provide the level/tg of the entry.
>
> Encoding the level/tg directly in the key will allow to invalidate
> using g_hash_table_remove() when num_pages equals to 1.
>
> Signed-off-by: Eric Auger 
>
> ---
> v3 -> v4:
> - also use the tg field when computing the hash
> - in the key equal function, compare the fields instead of
>   using memcmp()
> - fixed a couple of indents
> - added Peter's R-b

You say that, but you didn't :-)

Reviewed-by: Peter Maydell 

thanks
-- PMM