Re: Making QEMU easier for management tools and applications

[Markus Armbruster]

John Snow  writes:

> On 1/23/20 2:01 PM, Daniel P. Berrangé wrote:
>> On Thu, Jan 23, 2020 at 12:58:45PM -0500, John Snow wrote:
>>> Yes, I agree: Scrap and start over.
>>>
>>> What SHOULD the syntax look like, though? Clearly the idea of qmp-shell
>>> is that it offers a convenient way to enter the top-level keys of the
>>> arguments dict. This works absolutely fine right up until you need to
>>> start providing nested definitions.
>>>
>>> For the nesting, we say: "Go ahead and use JSON, but you have to take
>>> all the spaces out."
>>>
>>> This... works, charitably, but is hardly what I would call usable.
>>>
>>> For the CLI, we offer a dot syntax notation that resembles nothing in
>>> particular. It often seems the case that it isn't expressive enough to
>>> map losslessly to JSON. I suspect it doesn't handle siblings very well.
>>>
>>> A proper HMP-esque TUI would likely have need of coming up with its own
>>> pet syntax for commands that avoid complicated nested JSON definitions,
>>> but for effort:value ratio, having a QMP shorthand shell that works
>>> arbitrarily with any command might be a better win.
>>>
>>> Do we still have a general-case problem of how to represent QAPI
>>> structures in plaintext? Will this need to be solved for the CLI, too?
>> 
>> I don't know if you've ever looked at how Kubernetes/OpenShift exposes
>> its functionality on the command line ? I think it is interesting to

Yes, superficially.

>> note that they largely don't try to solve this problem of flattening
>> JSON for humans on the CLI using their client.

The fact that their users are happy with this proves it reasonable.

>> Everything in their world is an object described in JSON/YAML, and
>> there are a small set of generic commands that can act on any type
>> of object. These commands primarily input and output JSON or YAML
>> documents directly. As a user you can pick either format since it
>> can do a lossless conversion in both directions server side.

Like Kubernetes/OpenShift, our configuration / control language has an
abstract syntax that permits use of JSON/YAML as concrete syntax.  They
support both, we support just JSON.  We could support YAML, too.

Digression: JSON is a poor choice for configuration files.  YAML is a
complex and confusing beast (it's spec is printed 116 pages, and
yaml-0.2.2/src is ~7kSLOC).  XML is XML, 'nuff said.  TOML is much
simpler than either of the two.

>> So when configuring objects you'll always provide a JSON/YAML doc.
>> They've got some clever stuff for updating objects where you can
>> provide a JSON patch for only the bits which need changing.
>> 
>> When querying/listing objects by default it displays only a small
>> subset of their config information in a human friendly-ish format.
>> If you want to see everything then you ask for it in JSON/YAML
>> format. There's also an expression language that lets you extract
>> particular pieces of information based on requested properties,
>> and you can filter the list of objects based on attributes and so
>> on.
>> 
>> I think it is fair to say the structure of kubernetes object config
>> is on a par with hierarchical complexity of QEMU. The lack of a simple
>> human targetted data input format does not appear to have negatively
>> impacted the adoption of Kubernetes. It is worth questioning why this
>> is the case, while we feel the human CLI syntax for QEMU is so
>> critically important to QEMU's future ?

I consider human CLI syntax for QEMU a mostly solved *design* problem:
dotted keys.  It's an unsolved *implementation* problem: the CLI is a
tangled mess of almost two decades' worth of ideas, and only (some of)
the latest strands actually use dotted keys infrastructure.  The
proposed solution is CLI QAPIfication.  Gives us configuration file(s)
and introspection.

Dotted keys are merely yet another concrete syntax.  They're designed to
satisfy the CLI requirements we have, which include a measure of
compatibility to what's in the tangled mess.  They're reasonably usable
for simple stuff, but complex stuff can be too verbose to be readable.
They can't express all of the abstract syntax.  Tolerable, since they
provide an escape to JSON.  I recommend programs use the JSON escape
always.  Awkward for humans due to shell quoting.

> Well, if the "human CLI syntax" is "Feed it YAML documents", that's
> perfectly peachy for me, too! We need a good, consistent interface.
> Exactly what that interface is isn't really a blocking concern.

Right.

> Configuring a VM is a complicated process and has a lot of moving
> widgets. Feeding it a YAML file is a reasonable thought.

We've grown used to configuring QEMU with gargantuan command lines.
Déformation profesionelle.

> Having JSON and requiring people to type bastardized and differing
> versions of it in 8 places _is_ a concern. We can't document reasonably
> all of the different flavors and why they differ from one place to the
> next.

Yes.

>   We can unify it. If unifying

Re: [PATCH REPOST v3 16/80] arm/kzm: drop RAM size fixup

[Igor Mammedov]

On Thu, 23 Jan 2020 22:23:20 +
"Chubb, Peter (Data61, Kensington NSW)"  wrote:

> Igor> If the user provided too large a RAM size, the code used to
> Igor> complain and trim it to the max size.  Now tht RAM is allocated by
> Igor> generic code, that's no longer possible, so generate an error and
> Igor> exit instead.  
> 
> You can add my 'reviewed-by' to this. There's one really minor typo in
> the comment (tht->that) that you may wish to fix before the final
> commit.

Thanks,
will do it in v4

(considering that there are already fixes to
the current version, I'll rebase and repost)
> 
> Reviewed-by: Peter Chubb 
> 

Re: [PATCH REPOST v3 06/80] alpha:dp264: use memdev for RAM

[Igor Mammedov]

On Thu, 23 Jan 2020 16:07:19 +0100 (CET)
BALATON Zoltan  wrote:

> On Thu, 23 Jan 2020, Igor Mammedov wrote:
> > memory_region_allocate_system_memory() API is going away, so
> > replace it with memdev allocated MemoryRegion. The later is
> > initialized by generic code, so board only needs to opt in
> > to memdev scheme by providing
> >  MachineClass::default_ram_id
> > and using MachineState::ram instead of manually initializing
> > RAM memory region.
> >
> > Signed-off-by: Igor Mammedov 
> > Reviewed-by: Philippe Mathieu-Daudé 
> > Tested-by: Philippe Mathieu-Daudé 
> > Acked-by: Richard Henderson 
> > ---
> > hw/alpha/alpha_sys.h | 2 +-
> > hw/alpha/dp264.c | 3 ++-
> > hw/alpha/typhoon.c   | 8 ++--
> > 3 files changed, 5 insertions(+), 8 deletions(-)  
> 
> This still has a : instead of / in the patch title. Maybe should be 
> alpha/dp264: ... Probably does not worth a respin but you could correct it 
> if there will be another version for some other reason or when applying 
I plan to respin series, so it will be fixed up in v4

Thanks

> it.
> 
> Regrards,
> BALATON Zoltan

Re: [PATCH rc2 12/25] hw/timer: Add limited support for Atmel 16 bit timer peripheral

[Thomas Huth]

On 24/01/2020 01.51, Philippe Mathieu-Daudé wrote:
> From: Michael Rolnik 
> 
> These were designed to facilitate testing but should provide enough
> function to be useful in other contexts.  Only a subset of the functions
> of each peripheral is implemented, mainly due to the lack of a standard
> way to handle electrical connections (like GPIO pins).
> 
> Signed-off-by: Sarah Harris 
> Message-Id: <20200118191416.19934-13-mrol...@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé 
> [rth: Squash info mtree fixes and a file rename from f4bug, which was:]
> Suggested-by: Aleksandar Markovic 
> Signed-off-by: Richard Henderson 
> [PMD: Use qemu_log_mask(LOG_UNIMP), replace goto by return]
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> rc2: Use qemu_log_mask(LOG_UNIMP), replace goto by return (thuth)
> ---
>  include/hw/timer/atmel_timer16.h |  94 +
>  hw/timer/atmel_timer16.c | 605 +++
>  hw/timer/Kconfig |   3 +
>  hw/timer/Makefile.objs   |   2 +
>  4 files changed, 704 insertions(+)
>  create mode 100644 include/hw/timer/atmel_timer16.h
>  create mode 100644 hw/timer/atmel_timer16.c
[...]
> +static void avr_timer16_clksrc_update(AVRTimer16State *t16)
> +{
> +uint16_t divider = 0;
> +switch (CLKSRC(t16)) {
> +case T16_CLKSRC_EXT_FALLING:
> +case T16_CLKSRC_EXT_RISING:
> +qemu_log_mask(LOG_UNIMP, "%s: external clock source unsupported\n",
> +  __func__);
> +break;
> +case T16_CLKSRC_STOPPED:
> +break;
> +case T16_CLKSRC_DIV1:
> +divider = 1;
> +break;
> +case T16_CLKSRC_DIV8:
> +divider = 8;
> +break;
> +case T16_CLKSRC_DIV64:
> +divider = 64;
> +break;
> +case T16_CLKSRC_DIV256:
> +divider = 256;
> +break;
> +case T16_CLKSRC_DIV1024:
> +divider = 1024;
> +break;
> +default:
> +break;
> +}
> +if (divider) {
> +t16->freq_hz = t16->cpu_freq_hz / divider;
> +t16->period_ns = NANOSECONDS_PER_SECOND / t16->freq_hz;
> +DB_PRINT("Timer frequency %" PRIu64 " hz, period %" PRIu64 " ns (%f 
> s)",
> + t16->freq_hz, t16->period_ns, 1 / (double)t16->freq_hz);
> +}
> +return;
> +}

You can remove the "return;" at the end of the function now, too.

 Thomas

[Bug 1860759] [NEW] [REGRESSION] option `-snapshot` ignored with blockdev

[Ildar]

Public bug reported:

After upgrade of qemu 3.1.0 → 4.2.0 I found that running with libvirt doesn't 
honor `-snapshot` option anymore. I.e. disk images get modified.
Using `-hda` option honors `-snapshot`

So I made a test case without libvirt. Testcase using 4.2.0:

> qemu -hda tmp-16G.img -cdrom regular-rescue-latest-x86_64.iso -m 2G

This works fine and tmp-16G.img stays unmodified.

But:
> /usr/bin/qemu-system-x86_64 -name guest=test-linux,debug-threads=on -S 
> -machine pc-i440fx-3.1,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu 
> Broadwell-noTSX,vme=on,ss=on,f16c=on,rdrand=on,hypervisor=on,arat=on,tsc-adjust=on,xsaveopt=on,pdpe1gb=on,abm=on
>  -m 2048 -overcommit mem-lock=off -smp 3,sockets=3,cores=1,threads=1 -uuid 
> d32a9191-f51d-4fae-a419-b73d85b49198 -no-user-config -nodefaults -rtc 
> base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet 
> -no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 
> -boot strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x5.0x7 -device 
> ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x5
>  -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x5.0x1 
> -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x5.0x2 
> -blockdev 
> \{\"driver\":\"file\",\"filename\":\"/tmp/regular-rescue-latest-x86_64.iso\",\"node-name\":\"libvirt-2-storage\",\"auto-read-only\":true,\"discard\":\"unmap\"}
>  -blockdev 
> \{\"node-name\":\"libvirt-2-format\",\"read-only\":true,\"driver\":\"raw\",\"file\":\"libvirt-2-storage\"}
>  -device 
> ide-cd,bus=ide.0,unit=0,drive=libvirt-2-format,id=ide0-0-0,bootindex=1 
> -blockdev 
> \{\"driver\":\"file\",\"filename\":\"/tmp/tmp-2G.img\",\"node-name\":\"libvirt-1-storage\",\"auto-read-only\":true,\"discard\":\"unmap\"}
>  -blockdev 
> \{\"node-name\":\"libvirt-1-format\",\"read-only\":false,\"driver\":\"qcow2\",\"file\":\"libvirt-1-storage\",\"backing\":null}
>  -device 
> virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=libvirt-1-format,id=virtio-disk0
>  -netdev user,id=hostnet0 -device 
> e1000,netdev=hostnet0,id=net0,mac=52:54:00:ab:d8:29,bus=pci.0,addr=0x3 
> -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 
> -device 
> qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2
>  -device intel-hda,id=sound0,bus=pci.0,addr=0x4 -device 
> hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -device 
> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 -snapshot -sandbox 
> on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny -msg 
> timestamp=on

This modifies tmp-16G.img.

** Affects: qemu
 Importance: Undecided
 Status: New

** Bug watch added: Red Hat Bugzilla #508662
   https://bugzilla.redhat.com/show_bug.cgi?id=508662

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

Title:
  [REGRESSION] option `-snapshot` ignored with blockdev

Status in QEMU:
  New

Bug description:
  After upgrade of qemu 3.1.0 → 4.2.0 I found that running with libvirt doesn't 
honor `-snapshot` option anymore. I.e. disk images get modified.
  Using `-hda` option honors `-snapshot`

  So I made a test case without libvirt. Testcase using 4.2.0:

  > qemu -hda tmp-16G.img -cdrom regular-rescue-latest-x86_64.iso -m 2G

  This works fine and tmp-16G.img stays unmodified.

  But:
  > /usr/bin/qemu-system-x86_64 -name guest=test-linux,debug-threads=on -S 
-machine pc-i440fx-3.1,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu 
Broadwell-noTSX,vme=on,ss=on,f16c=on,rdrand=on,hypervisor=on,arat=on,tsc-adjust=on,xsaveopt=on,pdpe1gb=on,abm=on
 -m 2048 -overcommit mem-lock=off -smp 3,sockets=3,cores=1,threads=1 -uuid 
d32a9191-f51d-4fae-a419-b73d85b49198 -no-user-config -nodefaults -rtc 
base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet 
-no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot 
strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x5.0x7 -device 
ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x5 
-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x5.0x1 
-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x5.0x2 
-blockdev 
\{\"driver\":\"file\",\"filename\":\"/tmp/regular-rescue-latest-x86_64.iso\",\"node-name\":\"libvirt-2-storage\",\"auto-read-only\":true,\"discard\":\"unmap\"}
 -blockdev 
\{\"node-name\":\"libvirt-2-format\",\"read-only\":true,\"driver\":\"raw\",\"file\":\"libvirt-2-storage\"}
 -device ide-cd,bus=ide.0,unit=0,drive=libvirt-2-format,id=ide0-0-0,bootindex=1 
-blockdev 
\{\"driver\":\"file\",\"filename\":\"/tmp/tmp-2G.img\",\"node-name\":\"libvirt-1-storage\",\"auto-read-only\":true,\"discard\":\"unmap\"}
 -blockdev 
\{\"node-name\":\"libvirt-1-format\",\"read-only\":false,\"driver\":\"qcow2\",\"file\":\"libvirt-1

[Bug 1860759] Re: [REGRESSION] option `-snapshot` ignored with blockdev

[Ildar]

JFYI, I know that snapshot=on option should be used. But `-snapshot` option 
exists and must work.
Also libvirt doesn't yet support it: 
https://bugzilla.redhat.com/show_bug.cgi?id=508662

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

Title:
  [REGRESSION] option `-snapshot` ignored with blockdev

Status in QEMU:
  New

Bug description:
  After upgrade of qemu 3.1.0 → 4.2.0 I found that running with libvirt doesn't 
honor `-snapshot` option anymore. I.e. disk images get modified.
  Using `-hda` option honors `-snapshot`

  So I made a test case without libvirt. Testcase using 4.2.0:

  > qemu -hda tmp-16G.img -cdrom regular-rescue-latest-x86_64.iso -m 2G

  This works fine and tmp-16G.img stays unmodified.

  But:
  > /usr/bin/qemu-system-x86_64 -name guest=test-linux,debug-threads=on -S 
-machine pc-i440fx-3.1,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu 
Broadwell-noTSX,vme=on,ss=on,f16c=on,rdrand=on,hypervisor=on,arat=on,tsc-adjust=on,xsaveopt=on,pdpe1gb=on,abm=on
 -m 2048 -overcommit mem-lock=off -smp 3,sockets=3,cores=1,threads=1 -uuid 
d32a9191-f51d-4fae-a419-b73d85b49198 -no-user-config -nodefaults -rtc 
base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet 
-no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot 
strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x5.0x7 -device 
ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x5 
-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x5.0x1 
-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x5.0x2 
-blockdev 
\{\"driver\":\"file\",\"filename\":\"/tmp/regular-rescue-latest-x86_64.iso\",\"node-name\":\"libvirt-2-storage\",\"auto-read-only\":true,\"discard\":\"unmap\"}
 -blockdev 
\{\"node-name\":\"libvirt-2-format\",\"read-only\":true,\"driver\":\"raw\",\"file\":\"libvirt-2-storage\"}
 -device ide-cd,bus=ide.0,unit=0,drive=libvirt-2-format,id=ide0-0-0,bootindex=1 
-blockdev 
\{\"driver\":\"file\",\"filename\":\"/tmp/tmp-2G.img\",\"node-name\":\"libvirt-1-storage\",\"auto-read-only\":true,\"discard\":\"unmap\"}
 -blockdev 
\{\"node-name\":\"libvirt-1-format\",\"read-only\":false,\"driver\":\"qcow2\",\"file\":\"libvirt-1-storage\",\"backing\":null}
 -device 
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=libvirt-1-format,id=virtio-disk0
 -netdev user,id=hostnet0 -device 
e1000,netdev=hostnet0,id=net0,mac=52:54:00:ab:d8:29,bus=pci.0,addr=0x3 -chardev 
pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -device 
qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2
 -device intel-hda,id=sound0,bus=pci.0,addr=0x4 -device 
hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 -snapshot -sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny -msg 
timestamp=on

  This modifies tmp-16G.img.

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

Re: [PATCH] hw/s390x: Add a more verbose comment about get_machine_class() and the wrappers

[Thomas Huth]

On 23/01/2020 18.42, Cornelia Huck wrote:
> On Thu, 23 Jan 2020 18:02:56 +0100
> Thomas Huth  wrote:
> 
>> While working on the "Enable adapter interruption suppression again"
>> recently, I had to discover that the meaning of get_machine_class()
>> and the related *_allowed() wrappers is not very obvious. Add a more
>> verbose comment here to clarify how these should be used.
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 15 ---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index e0e28139a2..7fb389f0e5 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -505,6 +505,18 @@ static inline void machine_set_dea_key_wrap(Object 
>> *obj, bool value,
>>  
>>  static S390CcwMachineClass *current_mc;
>>  
>> +/*
>> + * Get the class of the s390-ccw-virtio machine that is currently in use.
>> + * Note: libvirt is using the "none" machine to probe for the features of 
>> the
>> + * host CPU, so in case this is called with the "none" machine, the function
>> + * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the
>> + * various "*_allowed" variables are enabled, so that the *_allowed() 
>> wrappers
>> + * below return the correct default value for the "none" machine.
> 
> Maybe add a blank line here for readability? (Can do so while applying.)

Sure, fine for me!

>> + * Attention! Do *not* add additional new wrappers for CPU features (e.g. 
>> like
>> + * the ri_allowed() wrapper) via this mechanism anymore. CPU features should
>> + * be handled via the CPU models, i.e. checking with cpu_model_allowed() 
>> during
>> + * CPU initialization and s390_has_feat() later should be sufficient.
>> + */
>>  static S390CcwMachineClass *get_machine_class(void)
>>  {
>>  if (unlikely(!current_mc)) {
>> @@ -521,19 +533,16 @@ static S390CcwMachineClass *get_machine_class(void)
>>  
>>  bool ri_allowed(void)
>>  {
>> -/* for "none" machine this results in true */
>>  return get_machine_class()->ri_allowed;
>>  }
>>  
>>  bool cpu_model_allowed(void)
>>  {
>> -/* for "none" machine this results in true */
>>  return get_machine_class()->cpu_model_allowed;
>>  }
>>  
>>  bool hpage_1m_allowed(void)
>>  {
>> -/* for "none" machine this results in true */
>>  return get_machine_class()->hpage_1m_allowed;
>>  }
>>  
> 
> Looks good to me, but will wait for a review or two.
> 

 Thanks,
  Thomas

Re: [PATCH 1/5] target/s390x: Move struct DisasFields definition earlier

[Thomas Huth]

On 24/01/2020 00.22, Richard Henderson wrote:
> We will want to include the struct in DisasContext.
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/s390x/translate.c | 164 ---
>  1 file changed, 83 insertions(+), 81 deletions(-)

Reviewed-by: Thomas Huth 

Re: QEMU for aarch64 with plugins seems to fail basic consistency checks

[Laurent Desnogues]

Hello,

On Fri, Jan 24, 2020 at 1:45 AM Robert Henry  wrote:
>
> I wrote a QEMU plugin for aarch64 where the insn and mem callbacks print out 
> the specifics of the guest instructions as they are "executed".  I expect 
> this trace stream to be well behaved but it is not. By well-behaved, I expect 
> memory insns print out some memory details, non-memory insns don't print 
> anything, and the pc only changes after a control flow instruction.  I don't 
> see that gross correctness about 2% of the time.
>
> I'm using qemu at tag v4.2.0 (or master head; it doesn't matter), running on 
> a x86_64 host.
> I build qemu using   ./configure --disable-sdl --enable-gtk --enable-plugins 
> --enable-debug --target-list=aarch64-softmmu aarch64-linux-user
> I execute qemu from its build area build/aarch64-linux-user/qemu-aarch64, 
> with flags --cpu cortex-a72 and the appropriate args to --plugin ... -d 
> plugin -D .
> I'm emulating a simple C program in linux emulation mode.
> The resulting qemu execution is valgrind clean (eg, I run qemu under 
> valgrind) for my little program save for memory leaks I reported a few days 
> ago.
>
> Below is an example of my trace output (the first int printed is the 
> cpu_index, checked to be always 0). Note that the ldr instruction at 0x41a608 
> sometimes reports a memop, but most of the time it doesn't.  Note that 
> 0x41a608 is seen, by trace, running back to back. Note that (bottom of trace) 
> that the movz instruction reports a memop.  (The executed code comes from 
> glibc _dl_aux_init, executed before main() is called.)
>
> How should this problem be tackled? I can't figure out how to make each tcg 
> block be exactly 1 guest (aarch64) insn, which is where I'd first start out.

To get TBs of a single instruction, you can add -singlestep to the command line.

HTH,

Laurent

> 0 0x0041a784 0x0041a784 0xf1000c3f cmp x1, #3
> 0 0x0041a788 0x0041a788 0x54fff401 b.ne #0xfe80
> 0 0x0041a78c 0x0041a78c 0x52800033 movz w19, #0x1
> 0 0x0041a790 0x0041a790 0xf9400416 ldr x22, [x0, #8] 0 mem  
> {3 0 0 0} 0x004000800618
> 0 0x0041a794 0x0041a794 0x179d b #0xfe74
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!  0 
> mem  {3 0 0 0} 0x004000800620
> 0 0x0041a60c 0x0041a60c 0xb4000221 cbz x1, #0x44
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a60c 0x0041a60c 0xb4000221 cbz x1, #0x44
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a60c 0x0041a60c 0xb4000221 cbz x1, #0x44
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a60c 0x0041a60c 0xb4000221 cbz x1, #0x44
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a60c 0x0041a60c 0xb4000221 cbz x1, #0x44
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a60c 0x0041a60c 0xb4000221 cbz x1, #0x44
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!  0 
> mem  {3 0 0 0} 0x004000800630
> 0 0x0041a60c 0x0041a60c 0xb4000221 cbz x1, #0x44
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a60c 0x0041a60c 0xb4000221 cbz x1, #0x44
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a60c 0x0041a60c 0xb4000221 cbz x1, #0x44
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a608 0x0041a608 0xf8410c01 ldr x1, [x0, #0x10]!
> 0 0x0041a60c 0x0041a60c 0xb4000221 cbz x1, #0x44
> 0 0x0041a7d8 0x0041a7d8 0x52800035 movz w21, #0x1
> 0 0x0041a7dc 0x0041a7dc 0xf9400418 ldr x24, [x0, #8] 0 mem  
> {3 0 0 0} 0x004000800638
> 0 0x0041a7e0 0x0041a7e0 0x178a b #0xfe28
> 0 0x0041a7d8 0x0041a7d8 0x52800035 movz w21, #0x1 0 mem  {3 0 
> 0 0} 0x004000800640
>
>
>
>
>

[PULL v2 58/59] build-sys: clean up flags included in the linker command line

[Paolo Bonzini]

Some of the CFLAGS that are discovered during configure, for example
compiler warnings, are being included on the linker command line because
QEMU_CFLAGS is added to it.  Other flags, such as the -m32, appear twice
because they are included in both QEMU_CFLAGS and LDFLAGS.  All this
leads to confusion with respect to what goes in which Makefile variables
(and we have plenty).

So, introduce QEMU_LDFLAGS for flags discovered by configure, following
the lead of QEMU_CFLAGS, and stop adding to it:

1) options that are already in CFLAGS, for example "-g"

2) duplicate options

At the same time, options that _are_ needed by both compiler and linker
must now be added to both QEMU_CFLAGS and QEMU_LDFLAGS, which is clearer.
This is mostly -fsanitize options.  For now, --extra-cflags has this behavior
(but --extra-cxxflags does not).

Meson will not include CFLAGS on the linker command line, do the same in our
build system as well.

Signed-off-by: Marc-André Lureau 
Signed-off-by: Paolo Bonzini 
---
 .travis.yml |  2 +-
 Makefile|  4 +--
 configure   | 61 +++--
 qga/vss-win32/Makefile.objs |  4 +--
 rules.mak   |  4 +--
 5 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 6c1038a..1ae645e 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -327,7 +327,7 @@ matrix:
 - TEST_CMD=""
   before_script:
 - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
-- ${SRC_DIR}/configure ${CONFIG} --extra-cflags="-g3 -O0 
-Wno-error=stringop-truncation -fsanitize=thread -fuse-ld=gold" || { cat 
config.log && exit 1; }
+- ${SRC_DIR}/configure ${CONFIG} --extra-cflags="-g3 -O0 
-Wno-error=stringop-truncation -fsanitize=thread" 
--extra-ldflags="-fuse-ld=gold" || { cat config.log && exit 1; }
 
 
 # Run check-tcg against linux-user
diff --git a/Makefile b/Makefile
index 6562b0d..c20c6fe 100644
--- a/Makefile
+++ b/Makefile
@@ -490,7 +490,7 @@ DTC_CPPFLAGS=-I$(BUILD_DIR)/dtc -I$(SRC_PATH)/dtc 
-I$(SRC_PATH)/dtc/libfdt
 
 .PHONY: dtc/all
 dtc/all: .git-submodule-status dtc/libfdt dtc/tests
-   $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) 
CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(LDFLAGS)" 
ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) 
libfdt/libfdt.a,)
+   $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) 
CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(QEMU_LDFLAGS)" 
ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) 
libfdt/libfdt.a,)
 
 dtc/%: .git-submodule-status
@mkdir -p $@
@@ -517,7 +517,7 @@ slirp/all: .git-submodule-status
BUILD_DIR="$(BUILD_DIR)/slirp"  \
PKG_CONFIG="$(PKG_CONFIG)"  \
CC="$(CC)" AR="$(AR)"   LD="$(LD)" RANLIB="$(RANLIB)"   \
-   CFLAGS="$(QEMU_CFLAGS) $(CFLAGS)" LDFLAGS="$(LDFLAGS)")
+   CFLAGS="$(QEMU_CFLAGS) $(CFLAGS)" LDFLAGS="$(QEMU_LDFLAGS)")
 
 # Compatibility gunk to keep make working across the rename of targets
 # for recursion, to be removed some time after 4.1.
diff --git a/configure b/configure
index c67a7e7..f7c4d07 100755
--- a/configure
+++ b/configure
@@ -126,7 +126,7 @@ compile_object() {
 compile_prog() {
   local_cflags="$1"
   local_ldflags="$2"
-  do_cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $LDFLAGS $local_ldflags
+  do_cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $QEMU_LDFLAGS $local_ldflags
 }
 
 # symbolically link $1 to $2.  Portable version of "ln -sf".
@@ -523,10 +523,11 @@ for opt do
   --cpu=*) cpu="$optarg"
   ;;
   --extra-cflags=*) QEMU_CFLAGS="$QEMU_CFLAGS $optarg"
+QEMU_LDFLAGS="$QEMU_LDFLAGS $optarg"
   ;;
   --extra-cxxflags=*) QEMU_CXXFLAGS="$QEMU_CXXFLAGS $optarg"
   ;;
-  --extra-ldflags=*) LDFLAGS="$LDFLAGS $optarg"
+  --extra-ldflags=*) QEMU_LDFLAGS="$QEMU_LDFLAGS $optarg"
  EXTRA_LDFLAGS="$optarg"
   ;;
   --enable-debug-info) debug_info="yes"
@@ -599,7 +600,6 @@ QEMU_INCLUDES="-iquote . -iquote \$(SRC_PATH) -iquote 
\$(SRC_PATH)/accel/tcg -iq
 QEMU_INCLUDES="$QEMU_INCLUDES -iquote \$(SRC_PATH)/disas/libvixl"
 if test "$debug_info" = "yes"; then
 CFLAGS="-g $CFLAGS"
-LDFLAGS="-g $LDFLAGS"
 fi
 
 # running configure in the source tree?
@@ -845,12 +845,12 @@ Darwin)
   LDFLAGS_SHARED="-bundle -undefined dynamic_lookup"
   if [ "$cpu" = "x86_64" ] ; then
 QEMU_CFLAGS="-arch x86_64 $QEMU_CFLAGS"
-LDFLAGS="-arch x86_64 $LDFLAGS"
+QEMU_LDFLAGS="-arch x86_64 $QEMU_LDFLAGS"
   fi
   cocoa="yes"
   audio_drv_list="coreaudio try-sdl"
   audio_possible_drivers="coreaudio sdl"
-  LDFLAGS="-framework CoreFoundation -framework IOKit $LDFLAGS"
+  QEMU_LDFLAGS="-framework CoreFoundation -framework IOKit $QEMU_LDFLAGS"
   libs_softmmu="-F/System/Library/Frameworks -framework Cocoa -framework IOKit 
$libs_softmmu"
   # Disable attempts 

[PULL v2 00/59] Misc (x86 and QOM) patches for 2020-01-23

[Paolo Bonzini]

The following changes since commit 3e08b2b9cb64bff2b73fa9128c0e49bfcde0dd40:

  Merge remote-tracking branch 'remotes/philmd-gitlab/tags/edk2-next-20200121' 
into staging (2020-01-21 15:29:25 +)

are available in the git repository at:


  git://github.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to b0993e347e065d2323fbb051fdd5a72c95a6e872:

  tests: fix test-qga on macosx (2020-01-24 10:14:32 +0100)


* Register qdev properties as class properties (Marc-André)
* Cleanups (Philippe)
* virtio-scsi fix (Pan Nengyuan)
* Tweak Skylake-v3 model id (Kashyap)
* x86 UCODE_REV support and nested live migration fix (myself)
* Advisory mode for pvpanic (Zhenwei)


Greg Kurz (2):
  cpu: Introduce cpu_class_set_parent_reset()
  cpu: Use cpu_class_set_parent_reset()

Kashyap Chamarthy (1):
  target/i386: Add the 'model-id' for Skylake -v3 CPU models

Marc-André Lureau (25):
  object: add extra sanity checks
  qdev: remove duplicated qdev_property_add_static() doc
  qdev: remove extraneous error
  qdev: move helper function to monitor/misc
  object: avoid extra class property key duplication
  object: add class property initializer
  object: make object_class_property_add* return property
  qstring: add qstring_free()
  object: add object_property_set_default
  object: do not free class properties
  object: check strong flag with &
  object: rename link "child" to "target"
  object: add direct link flag
  object: express const link with link property
  object: add object_class_property_add_link()
  object: release all props
  object: return self in object_ref()
  qdev: set properties with device_class_set_props()
  qdev: move instance properties to class properties
  qdev: register properties as class properties
  vl: print default value in object help
  qom: introduce object_property_help()
  qapi/qmp: add ObjectPropertyInfo.default-value
  qdev: use object_property_help()
  tests: fix test-qga on macosx

Pan Nengyuan (2):
  virtio-scsi: delete vqs in unrealize to avoid memleaks
  virtio-scsi: convert to new virtio_delete_queue

Paolo Bonzini (6):
  target/i386: kvm: initialize feature MSRs very early
  target/i386: add a ucode-rev property
  target/i386: kvm: initialize microcode revision from KVM
  qdev: rename DeviceClass.props
  qom: simplify qmp_device_list_properties()
  build-sys: clean up flags included in the linker command line

Philippe Mathieu-Daudé (21):
  qom/object: Display more helpful message when an interface is missing
  audio/audio: Add missing fall through comment
  hw/display/tcx: Add missing fall through comments
  hw/timer/aspeed_timer: Add a fall through comment
  hw/net/imx_fec: Rewrite fall through comments
  hw/net/imx_fec: Remove unuseful FALLTHROUGH comments
  hw/pci-host/designware: Remove unuseful FALLTHROUGH comment
  configure: Do not build libfdt if not required
  Makefile: Clarify all the codebase requires qom/ objects
  Makefile: Restrict system emulation and tools objects
  Makefile: Remove unhelpful comment
  hw/core: Restrict reset handlers API to system-mode
  hw/core/Makefile: Group generic objects versus system-mode objects
  hw/ppc/spapr_rtas: Use local MachineState variable
  hw/ppc/spapr_rtas: Access MachineState via SpaprMachineState argument
  hw/ppc/spapr_rtas: Remove local variable
  target/arm/kvm: Use CPUState::kvm_state in kvm_arm_pmu_supported()
  qom/object: Display more helpful message when a parent is missing
  accel: Introduce the current_accel() wrapper
  accel: Replace current_machine->accelerator by current_accel() wrapper
  accel/tcg: Sanitize include path

Zhenwei Pi (2):
  pvpanic: introduce crashloaded for pvpanic
  pvpanic: implement crashloaded event handling

 .travis.yml |   2 +-
 Makefile|   4 +-
 Makefile.objs   |  31 ++--
 accel/accel.c   |   5 +
 accel/kvm/kvm-all.c |   4 +-
 accel/tcg/tcg-all.c |   8 +-
 audio/audio.c   |   1 +
 configure   |  63 
 docs/specs/pvpanic.txt  |  18 ++-
 hw/9pfs/virtio-9p-device.c  |   2 +-
 hw/acpi/generic_event_device.c  |   2 +-
 hw/acpi/piix4.c |   2 +-
 hw/acpi/vmgenid.c   |   2 +-
 hw/arm/armsse.c |   2 +-
 hw/arm/armv7m.c |   4 +-
 hw/arm/aspeed_soc.c |   2 +-
 hw/arm/bcm2836.c|   2 +-
 hw/arm/integratorcp.c   |   2 +-
 hw/arm/msf2-soc.c   |   2 +-
 hw/arm/musicpal.c   |   2 +-
 

Re: [PATCH 2/5] target/s390x: Remove DisasFields argument from callbacks

[Thomas Huth]

On 24/01/2020 00.22, Richard Henderson wrote:
> The DisasFields data is available from DisasContext.
> We do not need to pass a separate argument.
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/s390x/translate.c | 417 ---
>  1 file changed, 210 insertions(+), 207 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH REPOST v3 75/80] exec: drop bogus mem_path from qemu_ram_alloc_from_fd()

[Igor Mammedov]

On Thu, 23 Jan 2020 12:38:40 +0100
Igor Mammedov  wrote:

> Function will report error that will mention global mem_path,
> which was valid the only if legacy -mem-path was used and
> only in case of main RAM.
> 
> However it doesn't work with hostmem backends
> (for example:
> "
>   qemu: -object memory-backend-file,id=ram0,size=128M,mem-path=foo:
> backing store (null) size 0x20 does not match 'size' option 0x800
> ")
> and couldn't possibly work in general FD case the function
> is supposed to handle.
> 
> Taking in account that main RAM was converted into
> memory-backend-foo object, there is no point in printing
> file name (from inappropriate place) as failing path is
> a part of backend's error message.
> 
> Hence drop bogus mem_path usage from qemu_ram_alloc_from_fd(),
> it's a job of its user to add file name to error message if applicable.
> 
> Signed-off-by: Igor Mammedov 

Marc-André,

git blames you as the one who introduced it,
could you take a look at this patch

probably I should add here as well
Fixes: 8d37b030fe ("exec: split file_ram_alloc()")

> ---
> CC: pbonz...@redhat.com
> CC: r...@twiddle.net
> ---
>  exec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 809987c..dc844fd 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2308,9 +2308,9 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, 
> MemoryRegion *mr,
>  size = HOST_PAGE_ALIGN(size);
>  file_size = get_file_size(fd);
>  if (file_size > 0 && file_size < size) {
> -error_setg(errp, "backing store %s size 0x%" PRIx64
> +error_setg(errp, "backing store size 0x%" PRIx64
> " does not match 'size' option 0x" RAM_ADDR_FMT,
> -   mem_path, file_size, size);
> +   file_size, size);
>  return NULL;
>  }
>  

Re: [PATCH v3 1/4] tests/acceptance: avocado_qemu: Introduce the 'accel' test parameter

[Philippe Mathieu-Daudé]

On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:

The test case may need to boot the VM with an accelerator that
isn't actually enabled on the QEMU binary and/or present in the host. In
this case the test behavior is undefined, and the best course of
action is to skip its execution.

This change introduced the 'accel' parameter (and the handler of
tag with same name) used to indicate the test case requires a
given accelerator available. It was implemented a mechanism to
skip the test case if the accelerator is not available. Moreover,
  the QEMU -accel argument is set automatically to any VM
launched if the parameter is present.

Signed-off-by: Wainer dos Santos Moschetta 
---
  docs/devel/testing.rst| 16 
  tests/acceptance/avocado_qemu/__init__.py | 23 +++
  2 files changed, 39 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index ab5be0c729..d17d0e90aa 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -759,6 +759,17 @@ name.  If one is not given explicitly, it will either be 
set to
  ``None``, or, if the test is tagged with one (and only one)
  ``:avocado: tags=machine:VALUE`` tag, it will be set to ``VALUE``.
  
+accel

+~
+The accelerator that will be set to all QEMUMachine instances created
+by the test.
+
+The ``accel`` attribute will be set to the test parameter of the same
+name.  If one is not given explicitly, it will either be set to
+``None``, or, if the test is tagged with one (and only one)
+``:avocado: tags=accel:VALUE`` tag, it will be set to ``VALUE``. Currently
+``VALUE`` should be either ``kvm`` or ``tcg``.
+
  qemu_bin
  
  
@@ -800,6 +811,11 @@ machine

  The machine type that will be set to all QEMUMachine instances created
  by the test.
  
+accel

+~
+The accelerator that will be set to all QEMUMachine instances created
+by the test. In case the accelerator is not available (both QEMU
+binary and the host system are checked) then the test is canceled.
  
  qemu_bin

  
diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 6618ea67c1..c83a75ccbc 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -20,6 +20,7 @@ SRC_ROOT_DIR = os.path.join(os.path.dirname(__file__), '..', 
'..', '..')
  sys.path.append(os.path.join(SRC_ROOT_DIR, 'python'))
  
  from qemu.machine import QEMUMachine

+from qemu.accel import kvm_available, tcg_available
  
  def is_readable_executable_file(path):

  return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
@@ -111,6 +112,8 @@ class Test(avocado.Test):
  
  def setUp(self):

  self._vms = {}
+# VM argumments that are mapped from parameters
+self._param_to_vm_args = []
  
  self.arch = self.params.get('arch',

  default=self._get_unique_tag_val('arch'))
@@ -124,10 +127,30 @@ class Test(avocado.Test):
  if self.qemu_bin is None:
  self.cancel("No QEMU binary defined or found in the source tree")
  
+self.accel = self.params.get('accel',

+ default=self._get_unique_tag_val('accel'))
+if self.accel:
+avail = False
+if self.accel == 'kvm':
+if kvm_available(self.arch, self.qemu_bin):
+avail = True
+elif self.accel == 'tcg':
+if tcg_available(self.qemu_bin):
+avail = True
+else:
+self.cancel("Unknown accelerator: %s" % self.accel)
+
+if avail:
+self._param_to_vm_args.extend(['-accel', self.accel])
+else:
+self.cancel("%s is not available" % self.accel)


Why refuse to test the other accelerators?

Isn't it better to QMP-ask QEMU which accelerator it supports, and SKIP 
if it isn't available?



+
  def _new_vm(self, *args):
  vm = QEMUMachine(self.qemu_bin, sock_dir=tempfile.mkdtemp())
  if args:
  vm.add_args(*args)
+if self._param_to_vm_args:
+vm.add_args(*self._param_to_vm_args)
  return vm
  
  @property

Re: [PATCH 3/5] target/s390x: Pass DisasContext to get_field and have_field

[Thomas Huth]

On 24/01/2020 00.22, Richard Henderson wrote:
> All callers pass s->fields, so we might as well pass s directly.
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/s390x/translate.c| 534 ++--
>  target/s390x/translate_vx.inc.c | 609 
>  2 files changed, 569 insertions(+), 574 deletions(-)

Looks like a pretty automated change (apart from translate_one()).

Reviewed-by: Thomas Huth 

Re: [PATCH v3 01/21] migration-test: Use g_free() instead of free()

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> Signed-off-by: Juan Quintela 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  tests/qtest/migration-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 26e2e77289..b6a74a05ce 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1291,7 +1291,7 @@ static void test_multifd_tcp(void)
>  wait_for_serial("dest_serial");
>  wait_for_migration_complete(from);
>  test_migrate_end(from, to, true);
> -free(uri);
> +g_free(uri);
>  }
>  
>  int main(int argc, char **argv)
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Philippe Mathieu-Daudé]

On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:

Some acceptance tests require KVM or they are skipped. Travis
enables nested virtualization by default with Ubuntu
18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
changed the acceptance builder to run in a Bionic VM. Also
it was needed to ensure the current user has rw permission
to /dev/kvm.

Signed-off-by: Wainer dos Santos Moschetta 
---
  .travis.yml | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 6c1038a0f1..c3edd0a907 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -2,6 +2,7 @@
  # Additional builds with specific requirements for a full VM need to
  # be added as additional matrix: entries later on
  dist: xenial
+sudo: true
  language: c
  compiler:
- gcc
@@ -83,6 +84,9 @@ git:
  
  before_script:

- if command -v ccache ; then ccache --zero-stats ; fi
+  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
+sudo chmod o+rw /dev/kvm ;
+fi
- mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
- ${SRC_DIR}/configure ${BASE_CONFIG} ${CONFIG} || { cat config.log && exit 
1; }
  script:
@@ -272,12 +276,13 @@ matrix:
  - TEST_CMD="make check-acceptance"
after_script:
  - python3 -c 'import json; r = json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) for 
t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
+  dist: bionic
addons:
  apt:
packages:
  - python3-pil
  - python3-pip
-- python3.5-venv
+- python3.6-venv


This line doesn't seem related to the patch.


  - tesseract-ocr
  - tesseract-ocr-eng
  

Re: [PATCH v3 05/21] migration: Create migration_is_running()

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> This function returns if we are in the middle of a migration.
> It is like migration_is_setup_or_active() with CANCELLING and COLO.
> Adapt all calers that are needed.
> 
> Signed-off-by: Juan Quintela 

Yes; the inclusion of cancelling is interesting because it's a state
where you don't want to be running but it still is.

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/migration.c | 29 -
>  migration/migration.h |  1 +
>  migration/savevm.c|  4 +---
>  3 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 990bff00c0..1fb0aab44d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -829,6 +829,27 @@ bool migration_is_setup_or_active(int state)
>  }
>  }
>  
> +bool migration_is_running(int state)
> +{
> +switch (state) {
> +case MIGRATION_STATUS_ACTIVE:
> +case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> +case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +case MIGRATION_STATUS_POSTCOPY_RECOVER:
> +case MIGRATION_STATUS_SETUP:
> +case MIGRATION_STATUS_PRE_SWITCHOVER:
> +case MIGRATION_STATUS_DEVICE:
> +case MIGRATION_STATUS_WAIT_UNPLUG:
> +case MIGRATION_STATUS_CANCELLING:
> +case MIGRATION_STATUS_COLO:
> +return true;
> +
> +default:
> +return false;
> +
> +}
> +}
> +
>  static void populate_time_info(MigrationInfo *info, MigrationState *s)
>  {
>  info->has_status = true;
> @@ -1077,7 +1098,7 @@ void 
> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>  MigrationCapabilityStatusList *cap;
>  bool cap_list[MIGRATION_CAPABILITY__MAX];
>  
> -if (migration_is_setup_or_active(s->state)) {
> +if (migration_is_running(s->state)) {
>  error_setg(errp, QERR_MIGRATION_ACTIVE);
>  return;
>  }
> @@ -1590,7 +1611,7 @@ static void migrate_fd_cancel(MigrationState *s)
>  
>  do {
>  old_state = s->state;
> -if (!migration_is_setup_or_active(old_state)) {
> +if (!migration_is_running(old_state)) {
>  break;
>  }
>  /* If the migration is paused, kick it out of the pause */
> @@ -1888,9 +1909,7 @@ static bool migrate_prepare(MigrationState *s, bool 
> blk, bool blk_inc,
>  return true;
>  }
>  
> -if (migration_is_setup_or_active(s->state) ||
> -s->state == MIGRATION_STATUS_CANCELLING ||
> -s->state == MIGRATION_STATUS_COLO) {
> +if (migration_is_running(s->state)) {
>  error_setg(errp, QERR_MIGRATION_ACTIVE);
>  return false;
>  }
> diff --git a/migration/migration.h b/migration/migration.h
> index aa9ff6f27b..44b1d56929 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -279,6 +279,7 @@ void migrate_fd_error(MigrationState *s, const Error 
> *error);
>  void migrate_fd_connect(MigrationState *s, Error *error_in);
>  
>  bool migration_is_setup_or_active(int state);
> +bool migration_is_running(int state);
>  
>  void migrate_init(MigrationState *s);
>  bool migration_is_blocked(Error **errp);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index adfdca26ac..f19cb9ec7a 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1531,9 +1531,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  MigrationState *ms = migrate_get_current();
>  MigrationStatus status;
>  
> -if (migration_is_setup_or_active(ms->state) ||
> -ms->state == MIGRATION_STATUS_CANCELLING ||
> -ms->state == MIGRATION_STATUS_COLO) {
> +if (migration_is_running(ms->state)) {
>  error_setg(errp, QERR_MIGRATION_ACTIVE);
>  return -EINVAL;
>  }
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 4/5] target/s390x: Move DisasFields into DisasContext

[Thomas Huth]

On 24/01/2020 00.22, Richard Henderson wrote:
> I believe that the separate allocation of DisasFields from DisasContext
> was meant to limit the places from which we could access fields.  But
> that plan did not go unchanged, and since DisasContext contains a pointer
> to fields, the substructure is accessible everywhere.
> 
> By allocating the substructure with DisasContext, we improve the locality
> of the accesses by avoiding one level of pointer chasing.  In addition,
> we avoid a dangling pointer to stack allocated memory, diagnosed by static
> checkers.
> 
> Launchpad: https://bugs.launchpad.net/bugs/1661815
> Signed-off-by: Richard Henderson 
> ---
>  target/s390x/translate.c| 22 +-
>  target/s390x/translate_vx.inc.c | 40 -
>  2 files changed, 30 insertions(+), 32 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v3 06/21] migration: Don't send data if we have stopped

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> If we do a cancel, we got out without one error, but we can't do the
> rest of the output as in a normal situation.
> 
> Signed-off-by: Juan Quintela 

I think it's the sync that's the main problem being avoided here rather
than actually the problem of sending the data.

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/ram.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f95d656c26..3fd7fdffcf 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3524,7 +3524,8 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>  ram_control_after_iterate(f, RAM_CONTROL_ROUND);
>  
>  out:
> -if (ret >= 0) {
> +if (ret >= 0
> +&& migration_is_setup_or_active(migrate_get_current()->state)) {
>  multifd_send_sync_main(rs);
>  qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>  qemu_fflush(f);
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 5/5] target/s390x: Remove DisasFields argument from extract_insn

[Thomas Huth]

On 24/01/2020 00.22, Richard Henderson wrote:
> The separate pointer is now redundant.
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/s390x/translate.c | 19 ---
>  1 file changed, 8 insertions(+), 11 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH 0/5] target/s390x: Do not leak stack address in translate_one

[Thomas Huth]

On 24/01/2020 00.22, Richard Henderson wrote:
> Thomas' patch avoids the leak, but I think we can do a bit more to
> cleaning in this area, and move the structure inline to DisasContext.
>
Sounds like a good idea to me. Cornelia, could you please replace my
patch with Richard's series?

 Thanks,
  Thomas

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Thomas Huth]

On 24/01/2020 10.38, Philippe Mathieu-Daudé wrote:
> On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
>> Some acceptance tests require KVM or they are skipped. Travis
>> enables nested virtualization by default with Ubuntu
>> 18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
>> changed the acceptance builder to run in a Bionic VM. Also
>> it was needed to ensure the current user has rw permission
>> to /dev/kvm.
>>
>> Signed-off-by: Wainer dos Santos Moschetta 
>> ---
>>   .travis.yml | 7 ++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 6c1038a0f1..c3edd0a907 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -2,6 +2,7 @@
>>   # Additional builds with specific requirements for a full VM need to
>>   # be added as additional matrix: entries later on
>>   dist: xenial
>> +sudo: true
>>   language: c
>>   compiler:
>>     - gcc
>> @@ -83,6 +84,9 @@ git:
>>     before_script:
>>     - if command -v ccache ; then ccache --zero-stats ; fi
>> +  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
>> +    sudo chmod o+rw /dev/kvm ;
>> +    fi
>>     - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
>>     - ${SRC_DIR}/configure ${BASE_CONFIG} ${CONFIG} || { cat
>> config.log && exit 1; }
>>   script:
>> @@ -272,12 +276,13 @@ matrix:
>>   - TEST_CMD="make check-acceptance"
>>     after_script:
>>   - python3 -c 'import json; r =
>> json.load(open("tests/results/latest/results.json"));
>> [print(t["logfile"]) for t in r["tests"] if t["status"] not in
>> ("PASS", "SKIP")]' | xargs cat
>> +  dist: bionic
>>     addons:
>>   apt:
>>     packages:
>>   - python3-pil
>>   - python3-pip
>> -    - python3.5-venv
>> +    - python3.6-venv
> 
> This line doesn't seem related to the patch.

"dist:" has been switched from xenial to bionic, so I think it is
required to update to python3.6 here, too?

 Thomas

Re: [PATCH v2 051/109] virtiofsd: add seccomp whitelist

[Florian Weimer]

* David Alan Gilbert:

> +static const int syscall_whitelist[] = {
> +/* TODO ireg sem*() syscalls */
> +SCMP_SYS(brk),
> +SCMP_SYS(capget), /* For CAP_FSETID */
> +SCMP_SYS(capset),
> +SCMP_SYS(clock_gettime),

> +SCMP_SYS(gettimeofday),

Is this to suppose to work on 32-bit architectures?  Then you need to
add the time64 system call variants as well.

Thanks,
Florian

Re: [PATCH] build-sys: clean up flags included in the linker command line

[Philippe Mathieu-Daudé]

On 12/11/19 3:46 PM, Paolo Bonzini wrote:

Some of the CFLAGS that are discovered during configure, for example
compiler warnings, are being included on the linker command line because
QEMU_CFLAGS is added to it.  Other flags, such as the -m32, appear twice
because they are included in both QEMU_CFLAGS and LDFLAGS.  All this
leads to confusion with respect to what goes in which Makefile variables
(and we have plenty).

So, introduce QEMU_LDFLAGS for flags discovered by configure, following
the lead of QEMU_CFLAGS, and stop adding to it:

1) options that are already in CFLAGS, for example "-g"

2) duplicate options

At the same time, options that _are_ needed by both compiler and linker
must now be added to both QEMU_CFLAGS and QEMU_LDFLAGS, which is clearer.
This is mostly -fsanitize options.


So with this patch we can kill configure:CPU_CFLAGS?


Meson will not include CFLAGS on the linker command line, do the same in our
build system as well.

Signed-off-by: Marc-André Lureau 
Signed-off-by: Paolo Bonzini 
---
  .travis.yml |  4 +--
  Makefile|  4 +--
  configure   | 60 ++---
  qga/vss-win32/Makefile.objs |  4 +--
  rules.mak   |  4 +--
  5 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index d3a12ae..186738d 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -193,7 +193,7 @@ matrix:
compiler: clang
before_script:
  - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
-- ${SRC_DIR}/configure ${CONFIG} --extra-cflags="-fsanitize=undefined -Werror" 
|| { cat config.log && exit 1; }
+- ${SRC_DIR}/configure ${CONFIG} --extra-cflags="-fsanitize=undefined -Werror" 
--extra-ldflags="-fsanitize=undefined" || { cat config.log && exit 1; }
  
  
  - env:

@@ -325,7 +325,7 @@ matrix:
  - TEST_CMD=""
before_script:
  - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
-- ${SRC_DIR}/configure ${CONFIG} --extra-cflags="-g3 -O0 
-Wno-error=stringop-truncation -fsanitize=thread -fuse-ld=gold" || { cat config.log 
&& exit 1; }
+- ${SRC_DIR}/configure ${CONFIG} --extra-cflags="-g3 -O0 -Wno-error=stringop-truncation 
-fsanitize=thread" --extra-ldflags="-fsanitize=thread -fuse-ld=gold" || { cat config.log 
&& exit 1; }
  
  
  # Run check-tcg against linux-user

diff --git a/Makefile b/Makefile
index 96e69dd..df92220 100644
--- a/Makefile
+++ b/Makefile
@@ -487,7 +487,7 @@ DTC_CPPFLAGS=-I$(BUILD_DIR)/dtc -I$(SRC_PATH)/dtc 
-I$(SRC_PATH)/dtc/libfdt
  
  .PHONY: dtc/all

  dtc/all: .git-submodule-status dtc/libfdt dtc/tests
-   $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" 
LDFLAGS="$(LDFLAGS)" ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) 
libfdt/libfdt.a,)
+   $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" 
LDFLAGS="$(QEMU_LDFLAGS)" ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) 
libfdt/libfdt.a,)
  
  dtc/%: .git-submodule-status

@mkdir -p $@
@@ -514,7 +514,7 @@ slirp/all: .git-submodule-status
BUILD_DIR="$(BUILD_DIR)/slirp"\
PKG_CONFIG="$(PKG_CONFIG)"\
CC="$(CC)" AR="$(AR)"   LD="$(LD)" RANLIB="$(RANLIB)"   
\
-   CFLAGS="$(QEMU_CFLAGS) $(CFLAGS)" LDFLAGS="$(LDFLAGS)")
+   CFLAGS="$(QEMU_CFLAGS) $(CFLAGS)" LDFLAGS="$(QEMU_LDFLAGS)")
  
  # Compatibility gunk to keep make working across the rename of targets

  # for recursion, to be removed some time after 4.1.
diff --git a/configure b/configure
index 19b209a..7f96279 100755
--- a/configure
+++ b/configure
@@ -126,7 +126,7 @@ compile_object() {
  compile_prog() {
local_cflags="$1"
local_ldflags="$2"
-  do_cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $LDFLAGS $local_ldflags
+  do_cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $QEMU_LDFLAGS $local_ldflags
  }
  
  # symbolically link $1 to $2.  Portable version of "ln -sf".

@@ -526,7 +526,7 @@ for opt do
;;
--extra-cxxflags=*) QEMU_CXXFLAGS="$QEMU_CXXFLAGS $optarg"
;;
-  --extra-ldflags=*) LDFLAGS="$LDFLAGS $optarg"
+  --extra-ldflags=*) QEMU_LDFLAGS="$QEMU_LDFLAGS $optarg"
   EXTRA_LDFLAGS="$optarg"
;;
--enable-debug-info) debug_info="yes"
@@ -599,7 +599,6 @@ QEMU_INCLUDES="-iquote . -iquote \$(SRC_PATH) -iquote 
\$(SRC_PATH)/accel/tcg -iq
  QEMU_INCLUDES="$QEMU_INCLUDES -iquote \$(SRC_PATH)/disas/libvixl"
  if test "$debug_info" = "yes"; then
  CFLAGS="-g $CFLAGS"
-LDFLAGS="-g $LDFLAGS"
  fi
  
  # running configure in the source tree?

@@ -845,12 +844,12 @@ Darwin)
LDFLAGS_SHARED="-bundle -undefined dynamic_lookup"
if [ "$cpu" = "x86_64" ] ; then
  QEMU_CFLAGS="-arch x86_64 $QEMU_CFLAGS"
-LDFLAGS="-arch x86_64 $LDFLAGS"
+QEMU_LDFLAGS="-ar

Re: [PATCH v3 01/21] migration-test: Use g_free() instead of free()

[Philippe Mathieu-Daudé]

On 1/23/20 12:58 PM, Juan Quintela wrote:

Signed-off-by: Juan Quintela 


Nothing changed since v4 (apart it is now v3),
however it misses:

Fixes: b99784ef6c3
Reviewed-by: Thomas Huth 
Reviewed-by: Philippe Mathieu-Daudé 

See:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg672805.html
https://www.mail-archive.com/qemu-devel@nongnu.org/msg672853.html


---
  tests/qtest/migration-test.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 26e2e77289..b6a74a05ce 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1291,7 +1291,7 @@ static void test_multifd_tcp(void)
  wait_for_serial("dest_serial");
  wait_for_migration_complete(from);
  test_migrate_end(from, to, true);
-free(uri);
+g_free(uri);
  }
  
  int main(int argc, char **argv)

Re: [PATCH REPOST v3 77/80] hostmem: introduce "prealloc-threads" property

[Igor Mammedov]

On Thu, 23 Jan 2020 12:38:42 +0100
Igor Mammedov  wrote:

> the property will allow user to specify number of threads to use
> in pre-allocation stage. It also will allow to reduce implicit
> hostmem dependency on current_machine.
> On object creation it will default to 1, but via machine
> compat property it will be updated to MachineState::smp::cpus
> to keep current behavior for hostmem and main RAM (which is
> now also hostmem based).
> 
> Signed-off-by: Igor Mammedov 

Jitendra,

As the one who introduced smp_cpus in backend

(1e356fc14be mem-prealloc: reduce large guest start-up and migration time.)

could you review this patch please?

> ---
> v3:
>   - use object_register_sugar_prop() instead of directly hacking
> compat_props (Paolo Bonzini )
>   - fix TODO description
> 
> CC: pbonz...@redhat.com
> CC: ehabk...@redhat.com
> ---
>  include/sysemu/hostmem.h |  2 ++
>  backends/hostmem.c   | 43 +++
>  vl.c | 14 ++
>  3 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> index 5db0d66..bdf8666 100644
> --- a/include/sysemu/hostmem.h
> +++ b/include/sysemu/hostmem.h
> @@ -61,6 +61,7 @@ struct HostMemoryBackendClass {
>   * @parent: opaque parent object container
>   * @size: amount of memory backend provides
>   * @mr: MemoryRegion representing host memory belonging to backend
> + * @prealloc_threads: number of threads to be used for preallocatining RAM
>   */
>  struct HostMemoryBackend {
>  /* private */
> @@ -70,6 +71,7 @@ struct HostMemoryBackend {
>  uint64_t size;
>  bool merge, dump, use_canonical_path;
>  bool prealloc, force_prealloc, is_mapped, share;
> +uint32_t prealloc_threads;
>  DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
>  HostMemPolicy policy;
>  
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index e773bdf..0988986 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -223,7 +223,6 @@ static void host_memory_backend_set_prealloc(Object *obj, 
> bool value,
>  {
>  Error *local_err = NULL;
>  HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> -MachineState *ms = MACHINE(qdev_get_machine());
>  
>  if (backend->force_prealloc) {
>  if (value) {
> @@ -243,7 +242,7 @@ static void host_memory_backend_set_prealloc(Object *obj, 
> bool value,
>  void *ptr = memory_region_get_ram_ptr(&backend->mr);
>  uint64_t sz = memory_region_size(&backend->mr);
>  
> -os_mem_prealloc(fd, ptr, sz, ms->smp.cpus, &local_err);
> +os_mem_prealloc(fd, ptr, sz, backend->prealloc_threads, &local_err);
>  if (local_err) {
>  error_propagate(errp, local_err);
>  return;
> @@ -252,14 +251,45 @@ static void host_memory_backend_set_prealloc(Object 
> *obj, bool value,
>  }
>  }
>  
> +static void host_memory_backend_get_prealloc_threads(Object *obj, Visitor *v,
> +const char *name, void *opaque, Error **errp)
> +{
> +HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> +visit_type_uint32(v, name, &backend->prealloc_threads, errp);
> +}
> +
> +static void host_memory_backend_set_prealloc_threads(Object *obj, Visitor *v,
> +const char *name, void *opaque, Error **errp)
> +{
> +HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> +Error *local_err = NULL;
> +uint32_t value;
> +
> +visit_type_uint32(v, name, &value, &local_err);
> +if (local_err) {
> +goto out;
> +}
> +if (value <= 0) {
> +error_setg(&local_err,
> +   "property '%s' of %s doesn't take value '%d'",
> +   name, object_get_typename(obj), value);
> +goto out;
> +}
> +backend->prealloc_threads = value;
> +out:
> +error_propagate(errp, local_err);
> +}
> +
>  static void host_memory_backend_init(Object *obj)
>  {
>  HostMemoryBackend *backend = MEMORY_BACKEND(obj);
>  MachineState *machine = MACHINE(qdev_get_machine());
>  
> +/* TODO: convert access to globals to compat properties */
>  backend->merge = machine_mem_merge(machine);
>  backend->dump = machine_dump_guest_core(machine);
>  backend->prealloc = mem_prealloc;
> +backend->prealloc_threads = 1;
>  }
>  
>  static void host_memory_backend_post_init(Object *obj)
> @@ -313,7 +343,6 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
> Error **errp)
>  {
>  HostMemoryBackend *backend = MEMORY_BACKEND(uc);
>  HostMemoryBackendClass *bc = MEMORY_BACKEND_GET_CLASS(uc);
> -MachineState *ms = MACHINE(qdev_get_machine());
>  Error *local_err = NULL;
>  void *ptr;
>  uint64_t sz;
> @@ -378,7 +407,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
> Error **errp)
>   */
>  if (backend->prealloc) {
>  os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz,
> -ms->smp.cpus, &local_err);
> +

Re: Making QEMU easier for management tools and applications

[Daniel P . Berrangé]

On Thu, Jan 23, 2020 at 04:07:09PM -0500, John Snow wrote:
> 
> 
> On 1/23/20 2:01 PM, Daniel P. Berrangé wrote:
> > IOW, the difficulty with configuring QEMU via JSON is not the fault
> > of JSON itself, it is the lack of knowledge amongst users and docs,
> > compounded by our never ending "improvements" to the human syntax.
> > There are other factors too, such as our only partial coverage of
> > config using JSON - some is only possible via the CLI still.
> > 
> 
> I'm fine with getting rid of HMP entirely, I think. It's a weird
> interface with bizarre behavior that's hard to support.
> 
> There's a few commands in there we just don't support at all, but maybe
> it's time to start deprecating one-by-one any of the individual commands
> that are better served by QMP these days, to send the message that HMP's
> days are numbered.
> 
> Bye-bye!
> 
> As for the CLI, well, that's part of the discussion at hand...
> 
> > 
> > I guess my point is that with a scrap & startover view point, we
> > should arguably completely ignore the design question of how to
> > flatten JSON for humans/command line, as it is the wrong problem.
> > Instead focus on the problem of making use of JSON the best way
> > to deal with QEMU both functionally and practically, for humans
> > and machines alike.
> > 
> 
> Well, sure. The context of this email was qmp-shell though, which is
> meant to help facilitate the entry of JSON commands so that you *can*
> indeed just forego the CLI/HMP entirely.
> 
> If you are of the opinion that every user of QEMU should be copy/pasting
> JSON straight into a socket and we should delete qmp-shell, that's
> certainly a fine opinion.

I think part of the pain of qmp-shell comes from the very fact that
it is trying to be an interactive shell. This points people towards
interactively typing in the commands, which is horrific when you get
anywhere near the JSON, or even dot-notation traditional commands.

If it was just a qmp-client that was single shot, we'd encourage
people to create the JSON in a sensible way - vim/emacs/whatever.

Bash/dash/zsh/$whatever is their interactive shell, with massively
more features than qmp-shell. You have command history, autocomplete,
conditional and looping constructs, and everything a normal shell
offers.

The only strong reason for qmp-shell to be interactive would be if
the initial protoocl handshake was too slow. I can't see that being
a problem with QMP. 

> I'm coming from the side that I love qmp-shell; I find it useful, but it
> has some syntax problems. How do I solve them? Is there a way to solve
> them? QAPI is here to stay, and QAPI involves hierarchical data. That
> data is usually best represented by something like json or yaml, but
> those are hard to type in a shell.
> 
> What do we do about that?

Here's one conceptual vision of how a better QEMU might look:

  * qemu-runtime-$TARGET

A binary that contains the implementation for the machine
emulator for $TARGET.

This has no command line arguments except for a UNIX
socket path which is a QMP server


  * qemu-launcher-$TARGET

A binary that is able to launch qemu-runtime-$TARGET
with jailers active.

This has no command line arguments except for a pair
of UNIX socket paths. One is a QMP server, the other
is the path for the QMP of qemu-runtime-$TARGET.

Commands it processes will be in automatically proxied
through to the qemu-runtime-$TARGET QMP, with appropriate
jailer updates being done in between.


  * qemu-client

A binary that speaks QMP, connects, runs a single command,
disconnects.

It is used to talk to either qemu-runtime-$TARGET or
qemu-launcher-$TARGET, depending on whether the mgmt app
or user wants to be making use of the jailer facilities
or not.  


  * qemu-system-$TARGET

The current binaries that exist today.

qemu-system-$TARGET should not be part of our formal
stability promise. We won't gratuitously / knowingly
break without good reason, but we will accept that
breakage can happen. Stability is only offered by
the qemu-{runtime,launcher}-$TARGET.

Several choices for their future in long term:

  - Leave them as-is and basically ignore them
whereever practical going forward, so we
minimally worry about backcompat breakage

  - Plan to re-write them so that they are simply
a shim the forks+execs qemu-runtime-$TARGET
and does syntax translation from CLI/HMP/QMP.

  - Deprecate them with a view to deletion entirely
in $NNN years. For some large-ish value of NNN,
given how well known they are


Example usage:

1. Launch the QEMU runtime for the desired target

 $ qemu-runtime-x86_64 myvm.sock

2. Load the configuration to define the VM

   $ cat myvm.yaml
   commands:
 - machine_declare:
 name: pc-q35-5.0
 ...
 - blockdev_add:
 ...
 - device_add:
 ...
 - blockdev_add:
   

Re: [PATCH v2 051/109] virtiofsd: add seccomp whitelist

[Dr. David Alan Gilbert]

* Florian Weimer (fwei...@redhat.com) wrote:
> * David Alan Gilbert:
> 
> > +static const int syscall_whitelist[] = {
> > +/* TODO ireg sem*() syscalls */
> > +SCMP_SYS(brk),
> > +SCMP_SYS(capget), /* For CAP_FSETID */
> > +SCMP_SYS(capset),
> > +SCMP_SYS(clock_gettime),
> 
> > +SCMP_SYS(gettimeofday),
> 
> Is this to suppose to work on 32-bit architectures?  Then you need to
> add the time64 system call variants as well.

I've build tested on 32 but not tried running it; I'd added time(2) after
hitting it on a static build but didn't know of time64 (not that it has
a manpage!).

I'll do a follow up to fix it.

Dave

> Thanks,
> Florian
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Philippe Mathieu-Daudé]

On 1/24/20 10:44 AM, Thomas Huth wrote:

On 24/01/2020 10.38, Philippe Mathieu-Daudé wrote:

On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:

Some acceptance tests require KVM or they are skipped. Travis
enables nested virtualization by default with Ubuntu
18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
changed the acceptance builder to run in a Bionic VM. Also
it was needed to ensure the current user has rw permission
to /dev/kvm.

Signed-off-by: Wainer dos Santos Moschetta 
---
   .travis.yml | 7 ++-
   1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 6c1038a0f1..c3edd0a907 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -2,6 +2,7 @@
   # Additional builds with specific requirements for a full VM need to
   # be added as additional matrix: entries later on
   dist: xenial
+sudo: true
   language: c
   compiler:
     - gcc
@@ -83,6 +84,9 @@ git:
     before_script:
     - if command -v ccache ; then ccache --zero-stats ; fi
+  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
+    sudo chmod o+rw /dev/kvm ;
+    fi
     - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
     - ${SRC_DIR}/configure ${BASE_CONFIG} ${CONFIG} || { cat
config.log && exit 1; }
   script:
@@ -272,12 +276,13 @@ matrix:
   - TEST_CMD="make check-acceptance"
     after_script:
   - python3 -c 'import json; r =
json.load(open("tests/results/latest/results.json"));
[print(t["logfile"]) for t in r["tests"] if t["status"] not in
("PASS", "SKIP")]' | xargs cat
+  dist: bionic
     addons:
   apt:
     packages:
   - python3-pil
   - python3-pip
-    - python3.5-venv
+    - python3.6-venv


This line doesn't seem related to the patch.


"dist:" has been switched from xenial to bionic, so I think it is
required to update to python3.6 here, too?


OK, I got confused because line 4 is still "dist: xenial".

Wainer can you add a comment about this in the commit description?

I'm still not convinced we should enable "sudo: true" on all our jobs.

Re: qemu-img convert vs writing another copy tool

[Richard W.M. Jones]

On Thu, Jan 23, 2020 at 01:21:28PM -0600, Eric Blake wrote:
> On 1/23/20 12:35 PM, Richard W.M. Jones wrote:
> >  - Hint that the target already contains zeroes.  It's almost always
> >the case that we know this, but we cannot tell qemu.  This was the
> >cause of a big performance regression last year.
> 
> This has just recently been proposed:
> https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg03617.html

Oh indeed, this is good.

> >  - NBD multi-conn.  In my tests this makes a really massive
> >performance difference in certain situations.  Again, virt-v2v has
> >a lot of information that we cannot pass to qemu: we know, for
> >example, exactly if the server supports the feature, how many
> >threads are available, in some situations even have information
> >about the network and backing disks that the data will travel over
> >/ be stored on.
> 
> Multi-conn for reading the source allows better parallelism.
> Multi-conn for writing is a bit trickier - it should be safe if the
> different connections are only touching distinct segments of the
> export (no overlaps), but as qemu does not advertise multiconn in
> such situations, you may still need a command-line switch to force
> multiple writers in spite of the server not advertising it.  Here,
> I'm not aware of anyone with patches underway, but I also think it
> would be a good ground for exploring.

But in the qemu-img convert case specifically, multi-conn should
be safe for writing?

One additional problem with multi-conn is that NBD servers only
advertise that the feature is present, not the best possible degree of
parallelism to use.  (It's possible that the server cannot or doesn't
know this.)

> >  - External block lists.  This is a rather obscure requirement, but
> >it's necessary in the case where we can get the allocated block map
> >from another source (eg. pyvmomi) and then want to use that with an
> >NBD source that does not support extents (eg. nbdkit-ssh-plugin /
> >libssh / sftp).  [Having said that, it may be possible to implement
> >this as an nbdkit filter, so maybe this is not a blocking feature.]
> 
> How are you intending to use this? I'm guessing you have some way of
> feeding in information to qemu-img of which portions of the source
> image you want to copy, and ignore remaining portions.

I should say first that I've nearly finished an nbdkit filter
implementation of this, so feel free to ignore this for qemu.

The background to this feature is that some block device backends do
not have support for determining extents / disk block allocation
status.  The one that is most frequently used is ssh (sftp).  Note
that adding this support to sftp, while possible, doesn't really solve
the problem because the proprietary hypervisors we are pulling from
don't use recent SSH servers.

So copying from SSH is slow because you have no choice except to read
vast amounts of zeroes or deleted data.  (This doesn't affect virt-v2v
because it has another strategy to avoid this, but it does affect
other scenarios such as "warm" conversions and any migration that
doesn't involve using virt-v2v.)

However you can get the extent information by other means.  For VMware
you can use VMOMI to read this.  Or you can ssh in and run commands
like xfs_bmap.

So in theory at least it's possible to assemble the required data
from multiple sources and thus avoid wasteful copying.

With nbdkit you'll be able to do something like:

  # fetch the extents list over VMOMI > extents.txt, then
  nbdkit -U /tmp/sock --filter=extentlist ssh \
   host=server /vmfs/.../file-flat.vmdk \
   extentlist=extents.txt
  qemu-img convert nbd:unix:/tmp/sock ...

> Note that it IS already possible to use qemu's copy-on-read feature
> as a way to copy only a subset of a source file over to a
> destination file. When demonstrating incremental backup, I wrote
> this shell function:
> 
> copyif() {
> if test $# -lt 2 || test $# -gt 3; then
>   echo 'usage: copyif src dst [bitmap]'
>   return 1
> fi
> if test -z "$3"; then
>   map_from="-f raw nbd://localhost:10809/$1"
>   state=true
> else
>   map_from="--image-opts driver=nbd,export=$1,server.type=inet"
>   map_from+=",server.host=localhost,server.port=10809"
>   map_from+=",x-dirty-bitmap=qemu:dirty-bitmap:$3"
>   state=false
> fi
> $qemu_img info -f raw nbd://localhost:10809/$1 || return
> $qemu_img info -f qcow2 $2 || return
> ret=0
> $qemu_img rebase -u -f qcow2 -F raw -b nbd://localhost:10809/$1 $2
> while read line; do
>   [[ $line =~ .*start.:.([0-9]*).*length.:.([0-9]*).*data.:.$state.*
> ]] || continue
>   start=${BASH_REMATCH[1]} len=${BASH_REMATCH[2]}
>   echo
>   echo " $start $len:"
>   qemu-io -C -c "r $start $len" -f qcow2 $2
> done < <($qemu_img map --output=json $map_from)
> $qemu_img rebase -u -f qcow2 -b '' $2
> if test $ret = 0; then echo 'Success!'; fi
> return $ret
> }
> 
> The key lines here are 'qemu-io -C -c "r $start $l

Re: [PATCH REPOST v3 68/80] ppc/virtex_ml507: remove unused arguments

[David Gibson]

On Thu, Jan 23, 2020 at 12:38:33PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov 
> Reviewed-by: Philippe Mathieu-Daudé 

Acked-by: David Gibson 

> ---
> CC: da...@gibson.dropbear.id.au
> CC: qemu-...@nongnu.org
> CC: edgar.igles...@gmail.com
> ---
>  hw/ppc/virtex_ml507.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> index 6862552..651d8db 100644
> --- a/hw/ppc/virtex_ml507.c
> +++ b/hw/ppc/virtex_ml507.c
> @@ -89,10 +89,7 @@ static void mmubooke_create_initial_mapping(CPUPPCState 
> *env,
>  tlb->PID = 0;
>  }
>  
> -static PowerPCCPU *ppc440_init_xilinx(ram_addr_t *ram_size,
> -  int do_init,
> -  const char *cpu_type,
> -  uint32_t sysclk)
> +static PowerPCCPU *ppc440_init_xilinx(const char *cpu_type, uint32_t sysclk)
>  {
>  PowerPCCPU *cpu;
>  CPUPPCState *env;
> @@ -213,7 +210,7 @@ static void virtex_init(MachineState *machine)
>  int i;
>  
>  /* init CPUs */
> -cpu = ppc440_init_xilinx(&ram_size, 1, machine->cpu_type, 4);
> +cpu = ppc440_init_xilinx(machine->cpu_type, 4);
>  env = &cpu->env;
>  
>  if (env->mmu_model != POWERPC_MMU_BOOKE) {

-- 
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


signature.asc
Description: PGP signature

Re: [PATCH v2 051/109] virtiofsd: add seccomp whitelist

[Dr. David Alan Gilbert]

* Florian Weimer (fwei...@redhat.com) wrote:
> * David Alan Gilbert:
> 
> > +static const int syscall_whitelist[] = {
> > +/* TODO ireg sem*() syscalls */
> > +SCMP_SYS(brk),
> > +SCMP_SYS(capget), /* For CAP_FSETID */
> > +SCMP_SYS(capset),
> > +SCMP_SYS(clock_gettime),
> 
> > +SCMP_SYS(gettimeofday),
> 
> Is this to suppose to work on 32-bit architectures?  Then you need to
> add the time64 system call variants as well.

Trying SCMP_SYS(time64) gives me an error for an undefined __NR_time64
on both 64 and 32 bit.

Dave

> Thanks,
> Florian
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PULL 0/3] Ui 20200123 patches

[Peter Maydell]

On Thu, 23 Jan 2020 at 17:41, Gerd Hoffmann  wrote:
>
> The following changes since commit 43d1455cf84283466e5c22a217db5ef4b8197b14:
>
>   qapi: Fix code generation with Python 3.5 (2020-01-20 12:17:38 +)
>
> are available in the Git repository at:
>
>   git://git.kraxel.org/qemu tags/ui-20200123-pull-request
>
> for you to fetch changes up to a1e8853ed2acbda29a52533abc91b035b723952e:
>
>   ui/console: Display the 'none' backend in '-display help' (2020-01-21 
> 07:29:40 +0100)
>
> 
> vnc: fix zlib compression artifacts.
> ui: add "none" to -display help.
>
> 
>
> Cameron Esfahani (1):
>   vnc: prioritize ZRLE compression over ZLIB
>
> Gerd Hoffmann (1):
>   Revert "vnc: allow fall back to RAW encoding"
>
> Philippe Mathieu-Daudé (1):
>   ui/console: Display the 'none' backend in '-display help'


Applied, thanks.

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

-- PMM

[PATCH] s390x: sigp: Fix sense running reporting

[Janosch Frank]

The logic was inversed and reported running if the cpu was stopped.
Let's fix that.

Signed-off-by: Janosch Frank 
---
 target/s390x/sigp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 727875bb4a..286c0d6c9c 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo 
*si)
 }
 
 /* If halted (which includes also STOPPED), it is not running */
-if (CPU(dst_cpu)->halted) {
+if (!CPU(dst_cpu)->halted) {
 si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
 } else {
 set_sigp_status(si, SIGP_STAT_NOT_RUNNING);
-- 
2.20.1

[PATCH v2 2/4] virtio-scsi: default num_queues to -smp N

[Stefan Hajnoczi]

Automatically size the number of request virtqueues to match the number
of vCPUs.  This ensures that completion interrupts are handled on the
same vCPU that submitted the request.  No IPI is necessary to complete
an I/O request and performance is improved.

Signed-off-by: Stefan Hajnoczi 
---
 hw/core/machine.c   |  3 +++
 hw/scsi/vhost-scsi.c|  3 ++-
 hw/scsi/vhost-user-scsi.c   |  3 ++-
 hw/scsi/virtio-scsi.c   |  6 +-
 hw/virtio/vhost-scsi-pci.c  | 10 --
 hw/virtio/vhost-user-scsi-pci.c | 10 --
 hw/virtio/virtio-scsi-pci.c | 10 --
 include/hw/virtio/virtio-scsi.h |  2 ++
 8 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 3e288bfceb..d6e2370c77 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,8 +30,11 @@
 GlobalProperty hw_compat_4_2[] = {
 { "virtio-blk-device", "x-enable-wce-if-config-wce", "off" },
 { "virtio-blk-device", "seg-max-adjust", "off"},
+{ "virtio-scsi-device", "num_queues", "1"},
 { "virtio-scsi-device", "seg_max_adjust", "off"},
 { "vhost-blk-device", "seg_max_adjust", "off"},
+{ "vhost-scsi", "num_queues", "1"},
+{ "vhost-user-scsi", "num_queues", "1"},
 { "usb-host", "suppress-remote-wake", "off" },
 { "usb-redir", "suppress-remote-wake", "off" },
 };
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 26f710d3ec..80fe5d999a 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -272,7 +272,8 @@ static Property vhost_scsi_properties[] = {
 DEFINE_PROP_STRING("vhostfd", VirtIOSCSICommon, conf.vhostfd),
 DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
 DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0),
-DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1),
+DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
+   VIRTIO_SCSI_AUTO_NUM_QUEUES),
 DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size,
128),
 DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSICommon, conf.seg_max_adjust,
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index eb37733bd0..655d300875 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -163,7 +163,8 @@ static void vhost_user_scsi_unrealize(DeviceState *dev, 
Error **errp)
 static Property vhost_user_scsi_properties[] = {
 DEFINE_PROP_CHR("chardev", VirtIOSCSICommon, conf.chardev),
 DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0),
-DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1),
+DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
+   VIRTIO_SCSI_AUTO_NUM_QUEUES),
 DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size,
128),
 DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 224a290498..c9342004ef 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -891,6 +891,9 @@ void virtio_scsi_common_realize(DeviceState *dev,
 virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
 sizeof(VirtIOSCSIConfig));
 
+if (s->conf.num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
+s->conf.num_queues = 1;
+}
 if (s->conf.num_queues == 0 ||
 s->conf.num_queues > VIRTIO_QUEUE_MAX - VIRTIO_SCSI_VQ_NUM_FIXED) {
 error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
@@ -964,7 +967,8 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, 
Error **errp)
 }
 
 static Property virtio_scsi_properties[] = {
-DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 
1),
+DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues,
+   VIRTIO_SCSI_AUTO_NUM_QUEUES),
 DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI,
  parent_obj.conf.virtqueue_size, 128),
 DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI,
diff --git a/hw/virtio/vhost-scsi-pci.c b/hw/virtio/vhost-scsi-pci.c
index e8dfbfc60f..38a8f0c3ef 100644
--- a/hw/virtio/vhost-scsi-pci.c
+++ b/hw/virtio/vhost-scsi-pci.c
@@ -17,6 +17,7 @@
 #include "qemu/osdep.h"
 
 #include "standard-headers/linux/virtio_pci.h"
+#include "hw/boards.h"
 #include "hw/qdev-properties.h"
 #include "hw/virtio/vhost-scsi.h"
 #include "qapi/error.h"
@@ -47,10 +48,15 @@ static void vhost_scsi_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 {
 VHostSCSIPCI *dev = VHOST_SCSI_PCI(vpci_dev);
 DeviceState *vdev = DEVICE(&dev->vdev);
-VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
+VirtIOSCSIConf *conf = &dev->vdev.parent_obj.parent_obj.conf;
+
+/* 1:1 vq to vcpu mapping is ideal because it avoids IPIs */
+if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {

[PATCH v2 1/4] virtio-scsi: introduce a constant for fixed virtqueues

[Stefan Hajnoczi]

The event and control virtqueues are always present, regardless of the
multi-queue configuration.  Define a constant so that virtqueue number
calculations are easier to read.

Signed-off-by: Stefan Hajnoczi 
---
 hw/scsi/vhost-user-scsi.c   | 2 +-
 hw/scsi/virtio-scsi.c   | 7 ---
 include/hw/virtio/virtio-scsi.h | 3 +++
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 23f972df59..eb37733bd0 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -115,7 +115,7 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error 
**errp)
 goto free_virtio;
 }
 
-vsc->dev.nvqs = 2 + vs->conf.num_queues;
+vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
 vsc->dev.vqs = g_new0(struct vhost_virtqueue, vsc->dev.nvqs);
 vsc->dev.vq_index = 0;
 vsc->dev.backend_features = 0;
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index d3af42ef92..224a290498 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -191,7 +191,7 @@ static void virtio_scsi_save_request(QEMUFile *f, 
SCSIRequest *sreq)
 VirtIOSCSIReq *req = sreq->hba_private;
 VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(req->dev);
 VirtIODevice *vdev = VIRTIO_DEVICE(req->dev);
-uint32_t n = virtio_get_queue_index(req->vq) - 2;
+uint32_t n = virtio_get_queue_index(req->vq) - VIRTIO_SCSI_VQ_NUM_FIXED;
 
 assert(n < vs->conf.num_queues);
 qemu_put_be32s(f, &n);
@@ -892,10 +892,11 @@ void virtio_scsi_common_realize(DeviceState *dev,
 sizeof(VirtIOSCSIConfig));
 
 if (s->conf.num_queues == 0 ||
-s->conf.num_queues > VIRTIO_QUEUE_MAX - 2) {
+s->conf.num_queues > VIRTIO_QUEUE_MAX - VIRTIO_SCSI_VQ_NUM_FIXED) {
 error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
  "must be a positive integer less than %d.",
-   s->conf.num_queues, VIRTIO_QUEUE_MAX - 2);
+   s->conf.num_queues,
+   VIRTIO_QUEUE_MAX - VIRTIO_SCSI_VQ_NUM_FIXED);
 virtio_cleanup(vdev);
 return;
 }
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 24e768909d..9f293bcb80 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -36,6 +36,9 @@
 #define VIRTIO_SCSI_MAX_TARGET  255
 #define VIRTIO_SCSI_MAX_LUN 16383
 
+/* Number of virtqueues that are always present */
+#define VIRTIO_SCSI_VQ_NUM_FIXED2
+
 typedef struct virtio_scsi_cmd_req VirtIOSCSICmdReq;
 typedef struct virtio_scsi_cmd_resp VirtIOSCSICmdResp;
 typedef struct virtio_scsi_ctrl_tmf_req VirtIOSCSICtrlTMFReq;
-- 
2.24.1

[PATCH v2 0/4] virtio-pci: enable blk and scsi multi-queue by default

[Stefan Hajnoczi]

v2:
 * Let the virtio-DEVICE-pci device select num-queues because the optimal
   multi-queue configuration may differ between virtio-pci, virtio-mmio, and
   virtio-ccw [Cornelia]

Enabling multi-queue on virtio-pci storage devices improves performance on SMP
guests because the completion interrupt is handled on the vCPU that submitted
the I/O request.  This avoids IPIs inside the guest.

Note that performance is unchanged in these cases:
1. Uniprocessor guests.  They don't have IPIs.
2. Application threads might be scheduled on the sole vCPU that handles
   completion interrupts purely by chance.  (This is one reason why benchmark
   results can vary noticably between runs.)
3. Users may bind the application to the vCPU that handles completion
   interrupts.

Set the number of queues to the number of vCPUs by default.  Older machine
types continue to default to 1 queue for live migration compatibility.

This patch improves IOPS by 1-4% on an Intel Optane SSD with 4 vCPUs, -drive
aio=native, and fio bs=4k direct=1 rw=randread.

Stefan Hajnoczi (4):
  virtio-scsi: introduce a constant for fixed virtqueues
  virtio-scsi: default num_queues to -smp N
  virtio-blk: default num_queues to -smp N
  vhost-user-blk: default num_queues to -smp N

 hw/block/vhost-user-blk.c  |  6 +-
 hw/block/virtio-blk.c  |  6 +-
 hw/core/machine.c  |  5 +
 hw/scsi/vhost-scsi.c   |  3 ++-
 hw/scsi/vhost-user-scsi.c  |  5 +++--
 hw/scsi/virtio-scsi.c  | 13 +
 hw/virtio/vhost-scsi-pci.c | 10 --
 hw/virtio/vhost-user-blk-pci.c |  6 ++
 hw/virtio/vhost-user-scsi-pci.c| 10 --
 hw/virtio/virtio-blk-pci.c |  9 -
 hw/virtio/virtio-scsi-pci.c| 10 --
 include/hw/virtio/vhost-user-blk.h |  2 ++
 include/hw/virtio/virtio-blk.h |  2 ++
 include/hw/virtio/virtio-scsi.h|  5 +
 14 files changed, 76 insertions(+), 16 deletions(-)

-- 
2.24.1

[PATCH v2 3/4] virtio-blk: default num_queues to -smp N

[Stefan Hajnoczi]

Automatically size the number of request virtqueues to match the number
of vCPUs.  This ensures that completion interrupts are handled on the
same vCPU that submitted the request.  No IPI is necessary to complete
an I/O request and performance is improved.

Signed-off-by: Stefan Hajnoczi 
---
 hw/block/virtio-blk.c  | 6 +-
 hw/core/machine.c  | 1 +
 hw/virtio/virtio-blk-pci.c | 9 -
 include/hw/virtio/virtio-blk.h | 2 ++
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9bee514c4e..d3ffaffc93 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1135,6 +1135,9 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)
 error_setg(errp, "Device needs media, but drive is empty");
 return;
 }
+if (conf->num_queues == VIRTIO_BLK_AUTO_NUM_QUEUES) {
+conf->num_queues = 1;
+}
 if (!conf->num_queues) {
 error_setg(errp, "num-queues property must be larger than 0");
 return;
@@ -1271,7 +1274,8 @@ static Property virtio_blk_properties[] = {
 #endif
 DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
 true),
-DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1),
+DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues,
+   VIRTIO_BLK_AUTO_NUM_QUEUES),
 DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
 DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true),
 DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
diff --git a/hw/core/machine.c b/hw/core/machine.c
index d6e2370c77..de6ceaa97f 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,6 +28,7 @@
 #include "hw/mem/nvdimm.h"
 
 GlobalProperty hw_compat_4_2[] = {
+{ "virtio-blk-device", "num-queues", "1"},
 { "virtio-blk-device", "x-enable-wce-if-config-wce", "off" },
 { "virtio-blk-device", "seg-max-adjust", "off"},
 { "virtio-scsi-device", "num_queues", "1"},
diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
index d9b69a5af3..7e6d863963 100644
--- a/hw/virtio/virtio-blk-pci.c
+++ b/hw/virtio/virtio-blk-pci.c
@@ -17,6 +17,7 @@
 
 #include "qemu/osdep.h"
 
+#include "hw/boards.h"
 #include "hw/qdev-properties.h"
 #include "hw/virtio/virtio-blk.h"
 #include "virtio-pci.h"
@@ -50,9 +51,15 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, 
Error **errp)
 {
 VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
 DeviceState *vdev = DEVICE(&dev->vdev);
+VirtIOBlkConf *conf = &dev->vdev.conf;
+
+/* 1:1 vq to vcpu mapping is ideal because it avoids IPIs */
+if (conf->num_queues == VIRTIO_BLK_AUTO_NUM_QUEUES) {
+conf->num_queues = current_machine->smp.cpus;
+}
 
 if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
-vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;
+vpci_dev->nvectors = conf->num_queues + 1;
 }
 
 qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 1e62f869b2..4e5e903f4a 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -30,6 +30,8 @@ struct virtio_blk_inhdr
 unsigned char status;
 };
 
+#define VIRTIO_BLK_AUTO_NUM_QUEUES UINT16_MAX
+
 struct VirtIOBlkConf
 {
 BlockConf conf;
-- 
2.24.1

Re: [PATCH] iscsi: Cap block count from GET LBA STATUS (CVE-2020-1711)

[Philippe Mathieu-Daudé]

On 1/23/20 11:58 PM, Peter Lieven wrote:

Am 23.01.2020 um 22:29 schrieb Felipe Franciosi :

On Jan 23, 2020, at 5:46 PM, Philippe Mathieu-Daudé  wrote:

On 1/23/20 1:44 PM, Felipe Franciosi wrote:
When querying an iSCSI server for the provisioning status of blocks (via
GET LBA STATUS), Qemu only validates that the response descriptor zero's
LBA matches the one requested. Given the SCSI spec allows servers to
respond with the status of blocks beyond the end of the LUN, Qemu may
have its heap corrupted by clearing/setting too many bits at the end of
its allocmap for the LUN.
A malicious guest in control of the iSCSI server could carefully program
Qemu's heap (by selectively setting the bitmap) and then smash it.
This limits the number of bits that iscsi_co_block_status() will try to
update in the allocmap so it can't overflow the bitmap.


Please add:

Fixes: CVE-2020-1711 (title of CVE if possible)


I wasn't sure we had one yet. Kevin: can you do the needful in your branch?


Cc: qemu-sta...@nongnu.org


Yeah, that's there.




Signed-off-by: Felipe Franciosi 
Signed-off-by: Peter Turschmid 
Signed-off-by: Raphael Norwitz 
---
block/iscsi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 2aea7e3f13..cbd57294ab 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -701,7 +701,7 @@ static int coroutine_fn 
iscsi_co_block_status(BlockDriverState *bs,
 struct scsi_get_lba_status *lbas = NULL;
 struct scsi_lba_status_descriptor *lbasd = NULL;
 struct IscsiTask iTask;
-uint64_t lba;
+uint64_t lba, max_bytes;
 int ret;
   iscsi_co_init_iscsitask(iscsilun, &iTask);
@@ -721,6 +721,7 @@ static int coroutine_fn 
iscsi_co_block_status(BlockDriverState *bs,
 }
   lba = offset / iscsilun->block_size;
+max_bytes = (iscsilun->num_blocks - lba) * iscsilun->block_size;
   qemu_mutex_lock(&iscsilun->mutex);
retry:
@@ -764,7 +765,7 @@ retry:
 goto out_unlock;
 }
-*pnum = (int64_t) lbasd->num_blocks * iscsilun->block_size;
+*pnum = MIN((int64_t) lbasd->num_blocks * iscsilun->block_size, max_bytes);
   if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
 lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {


What about this?

-- >8 --
diff --git a/block/iscsi.c b/block/iscsi.c
index 2aea7e3f13..25598accbb 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -506,6 +506,11 @@ iscsi_allocmap_update(IscsiLun *iscsilun, int64_t offset,
/* shrink to touch only completely contained clusters */
cl_num_shrunk = DIV_ROUND_UP(offset, iscsilun->cluster_size);
nb_cls_shrunk = (offset + bytes) / iscsilun->cluster_size - cl_num_shrunk;
+if (nb_cls_expanded >= iscsilun->allocmap_size
+|| nb_cls_shrunk >= iscsilun->allocmap_size) {
+error_report("iSCSI invalid request: ..." /* TODO */);
+return;
+}
if (allocated) {
bitmap_set(iscsilun->allocmap, cl_num_expanded, nb_cls_expanded);
} else {
---


I'm not sure the above is correct because (if I read this right)
nb_cls_* represents the number of clusters, not the last cluster.

Personally, I would have the checks (or "trim"s) closer to where they
were issued (to fail sooner) and assert()s closer to bitmap (as no oob
accesses should be happening at this point). There were also
discussions about using safer (higher level) bitmaps for this. I'm
always in favour of adding all reasonable checks. :)


I would add assertions that cl_num + nb_cls <= allocmap_size before every set 
and clear.


The description starts with "A malicious guest in control of the iSCSI 
server ..." so asserting (and killing the VM) doesn't seem correct... I 
suppose the iSCSI protocol has some error to return for invalid requests.


Also shouldn't we report some warning in case of such invalid request? 
So the management side can look at the 'malicious iSCSI server'?

[PATCH v2 4/4] vhost-user-blk: default num_queues to -smp N

[Stefan Hajnoczi]

Automatically size the number of request virtqueues to match the number
of vCPUs.  This ensures that completion interrupts are handled on the
same vCPU that submitted the request.  No IPI is necessary to complete
an I/O request and performance is improved.

Signed-off-by: Stefan Hajnoczi 
---
 hw/block/vhost-user-blk.c  | 6 +-
 hw/core/machine.c  | 1 +
 hw/virtio/vhost-user-blk-pci.c | 6 ++
 include/hw/virtio/vhost-user-blk.h | 2 ++
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 98b383f90e..2ee26a434c 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -403,6 +403,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, 
Error **errp)
 return;
 }
 
+if (s->num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) {
+s->num_queues = 1;
+}
 if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) {
 error_setg(errp, "vhost-user-blk: invalid number of IO queues");
 return;
@@ -500,7 +503,8 @@ static const VMStateDescription vmstate_vhost_user_blk = {
 
 static Property vhost_user_blk_properties[] = {
 DEFINE_PROP_CHR("chardev", VHostUserBlk, chardev),
-DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues, 1),
+DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues,
+   VHOST_USER_BLK_AUTO_NUM_QUEUES),
 DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
 DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
 DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/core/machine.c b/hw/core/machine.c
index de6ceaa97f..d4c67f4d6e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -35,6 +35,7 @@ GlobalProperty hw_compat_4_2[] = {
 { "virtio-scsi-device", "seg_max_adjust", "off"},
 { "vhost-blk-device", "seg_max_adjust", "off"},
 { "vhost-scsi", "num_queues", "1"},
+{ "vhost-user-blk", "num-queues", "1"},
 { "vhost-user-scsi", "num_queues", "1"},
 { "usb-host", "suppress-remote-wake", "off" },
 { "usb-redir", "suppress-remote-wake", "off" },
diff --git a/hw/virtio/vhost-user-blk-pci.c b/hw/virtio/vhost-user-blk-pci.c
index 1dc834a3ff..cf72b21c16 100644
--- a/hw/virtio/vhost-user-blk-pci.c
+++ b/hw/virtio/vhost-user-blk-pci.c
@@ -19,6 +19,7 @@
 #include "qemu/osdep.h"
 
 #include "standard-headers/linux/virtio_pci.h"
+#include "hw/boards.h"
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/vhost-user-blk.h"
 #include "hw/pci/pci.h"
@@ -54,6 +55,11 @@ static void vhost_user_blk_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 VHostUserBlkPCI *dev = VHOST_USER_BLK_PCI(vpci_dev);
 DeviceState *vdev = DEVICE(&dev->vdev);
 
+/* 1:1 vq to vcpu mapping is ideal because it avoids IPIs */
+if (dev->vdev.num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) {
+dev->vdev.num_queues = current_machine->smp.cpus;
+}
+
 if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
 vpci_dev->nvectors = dev->vdev.num_queues + 1;
 }
diff --git a/include/hw/virtio/vhost-user-blk.h 
b/include/hw/virtio/vhost-user-blk.h
index 108bfadeeb..5a353dc1c6 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -25,6 +25,8 @@
 #define VHOST_USER_BLK(obj) \
 OBJECT_CHECK(VHostUserBlk, (obj), TYPE_VHOST_USER_BLK)
 
+#define VHOST_USER_BLK_AUTO_NUM_QUEUES UINT16_MAX
+
 typedef struct VHostUserBlk {
 VirtIODevice parent_obj;
 CharBackend chardev;
-- 
2.24.1

Re: [PATCH v8 00/11] Multi-phase reset mechanism

[Peter Maydell]

On Thu, 23 Jan 2020 at 13:28, Damien Hedde  wrote:
> v8:
>   + patch 3&5: ResettableState::count type from uint32_t to unsigned
> (Philippe)

We'll have to change that back if we ever want to migrate
the count (migration insists on fixed-sized types), but
I guess we can do that when we get to it...

-- PMM

Re: [PATCH v2 051/109] virtiofsd: add seccomp whitelist

[Florian Weimer]

* David Alan Gilbert:

> * Florian Weimer (fwei...@redhat.com) wrote:
>> * David Alan Gilbert:
>> 
>> > +static const int syscall_whitelist[] = {
>> > +/* TODO ireg sem*() syscalls */
>> > +SCMP_SYS(brk),
>> > +SCMP_SYS(capget), /* For CAP_FSETID */
>> > +SCMP_SYS(capset),
>> > +SCMP_SYS(clock_gettime),
>> 
>> > +SCMP_SYS(gettimeofday),
>> 
>> Is this to suppose to work on 32-bit architectures?  Then you need to
>> add the time64 system call variants as well.
>
> Trying SCMP_SYS(time64) gives me an error for an undefined __NR_time64
> on both 64 and 32 bit.

Sorry, time64 does not exist, Userspace is supposed to use
clock_gettime64 with CLOCK_REALTIME_COARSE.

I actually meant that you'll also need futex_time64, ppoll_time64,
recvmmsg_time64, utimensat_time64.  (Based on cursory checking against
the permit list you posted.)

And for a port to 32-bit RISC-V, I think the 32-bit syscalls need to be
protected by #ifdef because new 32-bit architectures do not have them
anymore.

Thanks,
Florian

Re: [PATCH] s390x: sigp: Fix sense running reporting

[Cornelia Huck]

On Fri, 24 Jan 2020 05:01:37 -0500
Janosch Frank  wrote:

> The logic was inversed and reported running if the cpu was stopped.

s/inversed/inverted/ ?

> Let's fix that.
>

Fixes: d1b468bc8869 ("s390x/tcg: implement SIGP SENSE RUNNING STATUS")

> Signed-off-by: Janosch Frank 
> ---
>  target/s390x/sigp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
> index 727875bb4a..286c0d6c9c 100644
> --- a/target/s390x/sigp.c
> +++ b/target/s390x/sigp.c
> @@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo 
> *si)
>  }
>  
>  /* If halted (which includes also STOPPED), it is not running */
> -if (CPU(dst_cpu)->halted) {
> +if (!CPU(dst_cpu)->halted) {
>  si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>  } else {
>  set_sigp_status(si, SIGP_STAT_NOT_RUNNING);

I'm wondering why nobody noticed this before...

Re: [PULL v2 00/59] Misc (x86 and QOM) patches for 2020-01-23

[Peter Maydell]

On Fri, 24 Jan 2020 at 09:18, Paolo Bonzini  wrote:
>
> The following changes since commit 3e08b2b9cb64bff2b73fa9128c0e49bfcde0dd40:
>
>   Merge remote-tracking branch 
> 'remotes/philmd-gitlab/tags/edk2-next-20200121' into staging (2020-01-21 
> 15:29:25 +)
>
> are available in the git repository at:
>
>
>   git://github.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to b0993e347e065d2323fbb051fdd5a72c95a6e872:
>
>   tests: fix test-qga on macosx (2020-01-24 10:14:32 +0100)
>
> 
> * Register qdev properties as class properties (Marc-André)
> * Cleanups (Philippe)
> * virtio-scsi fix (Pan Nengyuan)
> * Tweak Skylake-v3 model id (Kashyap)
> * x86 UCODE_REV support and nested live migration fix (myself)
> * Advisory mode for pvpanic (Zhenwei

Hi -- this says 'v2', which is the same as the previous one
did, but the commit hash to fetch is different. Presumably
it's a v3? What are the v2->v3 differences, please?

thanks
-- PMM

Re: [PATCH] qemu-img: Add --target-is-zero to convert

[Max Reitz]

On 23.01.20 13:17, David Edmondson wrote:
> On Tuesday, 2020-01-21 at 16:02:16 +01, Max Reitz wrote:
> 
>> Hi,
>>
>> On 17.01.20 11:34, David Edmondson wrote:

[...]

>>> +
>>> +if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
>>> +!s->target_has_backing) {
>>
>> (This will be irrelevant after target_has_backing is gone, but because
>> has_zero_init and target_has_backing are equivalent here, there is no
>> need to check both.)
> 
> I don't understand this comment - I must be missing something.

Just the fact that for some reason I read “target_has_backing” as
“target_is_zero”.  Sorry for the false alarm. O:-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v8 00/11] Multi-phase reset mechanism

[Philippe Mathieu-Daudé]

On 1/24/20 11:05 AM, Peter Maydell wrote:

On Thu, 23 Jan 2020 at 13:28, Damien Hedde  wrote:

v8:
   + patch 3&5: ResettableState::count type from uint32_t to unsigned
 (Philippe)


We'll have to change that back if we ever want to migrate
the count (migration insists on fixed-sized types), but
I guess we can do that when we get to it...


Oh I forgot about migration :( (this was just a suggestion, not a 
requirement).


If you are happy with v7/v8 you can consider to apply v7 instead, the 
only difference is a one-line change in Makefile.objs (which ends no 
modified) and few Tested-by/Reviewed-by tags:


https://patchew.org/QEMU/20200115123620.250132-1-damien.he...@greensocs.com/diff/20200123132823.1117486-1-damien.he...@greensocs.com/

Regards,

Phil.

Re: [PATCH v8 00/11] Multi-phase reset mechanism

[Peter Maydell]

On Fri, 24 Jan 2020 at 10:17, Philippe Mathieu-Daudé  wrote:
>
> On 1/24/20 11:05 AM, Peter Maydell wrote:
> > On Thu, 23 Jan 2020 at 13:28, Damien Hedde  
> > wrote:
> >> v8:
> >>+ patch 3&5: ResettableState::count type from uint32_t to unsigned
> >>  (Philippe)
> >
> > We'll have to change that back if we ever want to migrate
> > the count (migration insists on fixed-sized types), but
> > I guess we can do that when we get to it...
>
> Oh I forgot about migration :( (this was just a suggestion, not a
> requirement).

Migration handling is going to require changes anyway, flipping
the type of the field will just be a minor part of that patch
if/when it arrives. It seems easier to take v8 if it's otherwise OK.

thanks
-- PMM

[Bug 1860759] Re: [REGRESSION] option `-snapshot` ignored with blockdev

[Max Reitz]

Hi,

I don’t know much about libvirt, but I would have thought that any
manual modification of the qemu command line isn’t supported and might
always break.

Anyway, from a QEMU POV, -snapshot only works with -drive (this includes
-hda, etc.).  It doesn’t work with -blockdev.  I can see that this isn’t
documented for -snapshot, but basically whenever -blockdev is used, the
user assumes full responsibility for the block graph (or at least that
particular subgraph).  We cannot enable snapshot functionality then.

So this can’t be fixed in qemu, as -snapshot doesn’t make sense for
-blockdev.  This behavior should be documented, though.

As for libvirt, I don’t know.  I would be surprised if it had a
guarantee for keeping manual qemu command line additions working, and I
can’t imagine that it would scan the XML for “legacy” qemu parameters
and interpret them itself (which it would need to do to keep -snapshot
working for -blockdev).

Max

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

Title:
  [REGRESSION] option `-snapshot` ignored with blockdev

Status in QEMU:
  New

Bug description:
  After upgrade of qemu 3.1.0 → 4.2.0 I found that running with libvirt doesn't 
honor `-snapshot` option anymore. I.e. disk images get modified.
  Using `-hda` option honors `-snapshot`

  So I made a test case without libvirt. Testcase using 4.2.0:

  > qemu -hda tmp-16G.img -cdrom regular-rescue-latest-x86_64.iso -m 2G

  This works fine and tmp-16G.img stays unmodified.

  But:
  > /usr/bin/qemu-system-x86_64 -name guest=test-linux,debug-threads=on -S 
-machine pc-i440fx-3.1,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu 
Broadwell-noTSX,vme=on,ss=on,f16c=on,rdrand=on,hypervisor=on,arat=on,tsc-adjust=on,xsaveopt=on,pdpe1gb=on,abm=on
 -m 2048 -overcommit mem-lock=off -smp 3,sockets=3,cores=1,threads=1 -uuid 
d32a9191-f51d-4fae-a419-b73d85b49198 -no-user-config -nodefaults -rtc 
base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet 
-no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot 
strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x5.0x7 -device 
ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x5 
-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x5.0x1 
-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x5.0x2 
-blockdev 
\{\"driver\":\"file\",\"filename\":\"/tmp/regular-rescue-latest-x86_64.iso\",\"node-name\":\"libvirt-2-storage\",\"auto-read-only\":true,\"discard\":\"unmap\"}
 -blockdev 
\{\"node-name\":\"libvirt-2-format\",\"read-only\":true,\"driver\":\"raw\",\"file\":\"libvirt-2-storage\"}
 -device ide-cd,bus=ide.0,unit=0,drive=libvirt-2-format,id=ide0-0-0,bootindex=1 
-blockdev 
\{\"driver\":\"file\",\"filename\":\"/tmp/tmp-2G.img\",\"node-name\":\"libvirt-1-storage\",\"auto-read-only\":true,\"discard\":\"unmap\"}
 -blockdev 
\{\"node-name\":\"libvirt-1-format\",\"read-only\":false,\"driver\":\"qcow2\",\"file\":\"libvirt-1-storage\",\"backing\":null}
 -device 
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=libvirt-1-format,id=virtio-disk0
 -netdev user,id=hostnet0 -device 
e1000,netdev=hostnet0,id=net0,mac=52:54:00:ab:d8:29,bus=pci.0,addr=0x3 -chardev 
pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -device 
qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2
 -device intel-hda,id=sound0,bus=pci.0,addr=0x4 -device 
hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 -snapshot -sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny -msg 
timestamp=on

  This modifies tmp-16G.img.

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

Re: [PATCH] qemu_set_log_filename: filename argument may be NULL

[Stefan Hajnoczi]

On Thu, Jan 23, 2020 at 08:36:26PM +0100, salva...@qindel.com wrote:
> From: Salvador Fandino 
> 
> NULL is a valid log filename used to indicate we want to use stderr
> but qemu_set_log_filename (which is called by bsd-user/main.c) was not
> handling it correctly.
> 
> That also made redundant a couple of NULL checks in calling code which
> have been removed.
> 
> Signed-off-by: Salvador Fandino 
> ---
>  trace/control.c |  4 +---
>  util/log.c  | 28 
>  vl.c|  5 +
>  3 files changed, 18 insertions(+), 19 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH v8 00/11] Multi-phase reset mechanism

[Damien Hedde]

On 1/24/20 11:17 AM, Philippe Mathieu-Daudé wrote:
> On 1/24/20 11:05 AM, Peter Maydell wrote:
>> On Thu, 23 Jan 2020 at 13:28, Damien Hedde
>>  wrote:
>>> v8:
>>>    + patch 3&5: ResettableState::count type from uint32_t to unsigned
>>>  (Philippe)
>>
>> We'll have to change that back if we ever want to migrate
>> the count (migration insists on fixed-sized types), but
>> I guess we can do that when we get to it...
> 
> Oh I forgot about migration :( (this was just a suggestion, not a
> requirement).

I forgot it too, and I probably put a uint32_t in the first place
because of migration in an earlier version of the patchset...

Damien

Re: [PATCH] qemu_set_log_filename: filename argument may be NULL

[Stefan Hajnoczi]

On Thu, Jan 23, 2020 at 08:36:26PM +0100, salva...@qindel.com wrote:
> From: Salvador Fandino 
> 
> NULL is a valid log filename used to indicate we want to use stderr
> but qemu_set_log_filename (which is called by bsd-user/main.c) was not
> handling it correctly.
> 
> That also made redundant a couple of NULL checks in calling code which
> have been removed.
> 
> Signed-off-by: Salvador Fandino 
> ---
>  trace/control.c |  4 +---
>  util/log.c  | 28 
>  vl.c|  5 +
>  3 files changed, 18 insertions(+), 19 deletions(-)

Thanks, applied to my tracing tree:
https://github.com/stefanha/qemu/commits/tracing

Stefan


signature.asc
Description: PGP signature

Re: [PATCH REPOST v3 74/80] exec: cleanup qemu_minrampagesize()/qemu_maxrampagesize()

[Igor Mammedov]

On Thu, 23 Jan 2020 12:38:39 +0100
Igor Mammedov  wrote:

> Since all RAM is backed by hostmem backends, drop
> global -mem-path invariant and simplify code.

Looks like origin of removed here code is PPC,
could PPC folk review this please?

> Signed-off-by: Igor Mammedov 
> ---
> CC: pbonz...@redhat.com
> CC: r...@twiddle.net
> ---
>  exec.c | 51 +--
>  1 file changed, 5 insertions(+), 46 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 67e520d..809987c 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1667,60 +1667,19 @@ static int find_max_backend_pagesize(Object *obj, 
> void *opaque)
>   */
>  long qemu_minrampagesize(void)
>  {
> -long hpsize = LONG_MAX;
> -long mainrampagesize;
> -Object *memdev_root;
> -MachineState *ms = MACHINE(qdev_get_machine());
> -
> -mainrampagesize = qemu_mempath_getpagesize(mem_path);
> -
> -/* it's possible we have memory-backend objects with
> - * hugepage-backed RAM. these may get mapped into system
> - * address space via -numa parameters or memory hotplug
> - * hooks. we want to take these into account, but we
> - * also want to make sure these supported hugepage
> - * sizes are applicable across the entire range of memory
> - * we may boot from, so we take the min across all
> - * backends, and assume normal pages in cases where a
> - * backend isn't backed by hugepages.
> - */
> -memdev_root = object_resolve_path("/objects", NULL);
> -if (memdev_root) {
> -object_child_foreach(memdev_root, find_min_backend_pagesize, 
> &hpsize);
> -}
> -if (hpsize == LONG_MAX) {
> -/* No additional memory regions found ==> Report main RAM page size 
> */
> -return mainrampagesize;
> -}
> -
> -/* If NUMA is disabled or the NUMA nodes are not backed with a
> - * memory-backend, then there is at least one node using "normal" RAM,
> - * so if its page size is smaller we have got to report that size 
> instead.
> - */
> -if (hpsize > mainrampagesize &&
> -(ms->numa_state == NULL ||
> - ms->numa_state->num_nodes == 0 ||
> - ms->numa_state->nodes[0].node_memdev == NULL)) {
> -static bool warned;
> -if (!warned) {
> -error_report("Huge page support disabled (n/a for main 
> memory).");
> -warned = true;
> -}
> -return mainrampagesize;
> -}
> +long hpsize;
> +Object *memdev_root = object_resolve_path("/objects", NULL);
>  
> +object_child_foreach(memdev_root, find_min_backend_pagesize, &hpsize);
>  return hpsize;
>  }
>  
>  long qemu_maxrampagesize(void)
>  {
> -long pagesize = qemu_mempath_getpagesize(mem_path);
> +long pagesize;
>  Object *memdev_root = object_resolve_path("/objects", NULL);
>  
> -if (memdev_root) {
> -object_child_foreach(memdev_root, find_max_backend_pagesize,
> - &pagesize);
> -}
> +object_child_foreach(memdev_root, find_max_backend_pagesize, &pagesize);
>  return pagesize;
>  }
>  #else

Re: Making QEMU easier for management tools and applications

[Daniel P . Berrangé]

On Fri, Jan 24, 2020 at 08:59:41AM +0100, Markus Armbruster wrote:
> John Snow  writes:
> 
> > On 1/23/20 2:01 PM, Daniel P. Berrangé wrote:
> >> So when configuring objects you'll always provide a JSON/YAML doc.
> >> They've got some clever stuff for updating objects where you can
> >> provide a JSON patch for only the bits which need changing.
> >> 
> >> When querying/listing objects by default it displays only a small
> >> subset of their config information in a human friendly-ish format.
> >> If you want to see everything then you ask for it in JSON/YAML
> >> format. There's also an expression language that lets you extract
> >> particular pieces of information based on requested properties,
> >> and you can filter the list of objects based on attributes and so
> >> on.
> >> 
> >> I think it is fair to say the structure of kubernetes object config
> >> is on a par with hierarchical complexity of QEMU. The lack of a simple
> >> human targetted data input format does not appear to have negatively
> >> impacted the adoption of Kubernetes. It is worth questioning why this
> >> is the case, while we feel the human CLI syntax for QEMU is so
> >> critically important to QEMU's future ?
> 
> I consider human CLI syntax for QEMU a mostly solved *design* problem:
> dotted keys.  It's an unsolved *implementation* problem: the CLI is a
> tangled mess of almost two decades' worth of ideas, and only (some of)
> the latest strands actually use dotted keys infrastructure.  The
> proposed solution is CLI QAPIfication.  Gives us configuration file(s)
> and introspection.
> 
> Dotted keys are merely yet another concrete syntax.  They're designed to
> satisfy the CLI requirements we have, which include a measure of
> compatibility to what's in the tangled mess.  They're reasonably usable
> for simple stuff, but complex stuff can be too verbose to be readable.
> They can't express all of the abstract syntax.  Tolerable, since they
> provide an escape to JSON.  I recommend programs use the JSON escape
> always.  Awkward for humans due to shell quoting.

I agree that the dotted key syntax is our chosen / solved design
for expressing JSON on the CLI. I would also say that, in retrospect,
this was a incorrect design decision that is one of the key things
responsible for QEMU having a bad reputation for complexity.

We should simply never have tried to invent a way to map the full
hiearchy of JSON onto the CLI as the result will always be unpleasant.
The dotted notation is the most verbose way to do this type of
configuration, because of the string repetition it requires for
nested structures.

Lets consider how libvirt uses blockdev for a LUKS volume stored
in iSCSI

  $ qemu-system-x86_64 \
  -object secret,id=libvirt-5-storage-secret0,\
data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\
keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \
  -object secret,id=libvirt-5-format-luks-secret0,\
data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\
keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \
  -blockdev '{"driver":"iscsi","portal":"example.org:6000",\
"target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\
"user":"myname","password-secret":"libvirt-5-storage-secret0",\
"node-name":"libvirt-5-storage","auto-read-only":true,"discard":"unmap"}' \
  -blockdev 
'{"node-name":"libvirt-5-format","read-only":false,"driver":"qcow2",\
"encrypt":{"format":"luks","key-secret":"libvirt-5-format-luks-secret0"},\
"file":"libvirt-5-storage"}' \

We all know JSON is horrible on the CLI, no surprise. So

Lets use human "friendly" dotted syntax instead:

  $ qemu-system-x86_64 \
  -object secret,id=libvirt-5-storage-secret0,\
data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\
keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \
  -object secret,id=libvirt-5-format-luks-secret0,\
data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\
keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \
  -blockdev driver=qcow2,node-name=libvirt-5-format,read-only=false,\
encrypt.format=luks,encrypt.key-secret=libvirt-5-format-luks-secret0,\
file.driver=iscsi,file.portal=example.org:6000,\
file.target=iqn.1992-01.com.example:storage,file.lun=1,file.transport=tcp,\
file.user=myname,file.password-secret=libvirt-6-storage-secret0,\
file.node-name=libvirt-5-storage,file.auto-read-only=true,file.dicard=unmap

I don't think that's much of an improvement, aside from not having
to worry about matching "}".

If we move to JSON in a config file

  $ cat qemu.json
  {
"arguments": [
  {
"arg": "object",
"data": {
  "type": "secret",
  "id":"libvirt-5-storage-secret0",
  "data": 
"9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1",
  "keyid": "masterKey0",
  "iv": "AAECAwQFBgcICQoLDA0ODw==",
 

[Bug 1860553] Re: cmake crashes on qemu-alpha-user with Illegal Instruction

[Laurent Vivier]

It seems halt instruction is not implemented for qemu-user, only for
qemu-system:


1286 #ifndef CONFIG_USER_ONLY
...
1365 static DisasJumpType gen_mtpr(DisasContext *ctx, TCGv vb, int regno)
1366 {
1367 int data;
1368 
1369 switch (regno) {
...
1390 case 252:
1391 /* HALT */
1392 gen_helper_halt(vb);
1393 return DISAS_PC_STALE;
...
1437 }
1438 #endif /* !USER_ONLY*/

...
2673 case 0x1D:
2674 /* HW_MTPR (PALcode) */
2675 #ifndef CONFIG_USER_ONLY
2676 REQUIRE_TB_FLAG(ENV_FLAG_PAL_MODE);
2677 vb = load_gpr(ctx, rb);
2678 ret = gen_mtpr(ctx, vb, insn & 0x);
2679 break;
2680 #else
2681 goto invalid_opc;
2682 #endif

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

Title:
  cmake crashes on qemu-alpha-user with Illegal Instruction

Status in QEMU:
  New

Bug description:
  I tried building cmake on Debian unstable for Alpha today using qemu-
  user and the compiled cmake binary crashed with "Illegal Instruction":

  g++ -Wl,-z,relro -Wl,--as-needed -g -O2 -fdebug-prefix-map=/<>=. 
-Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 
-I/<>/Build/Bootstrap.cmk   -I/<>/Source   
-I/<>/Source/LexerParser   -I/<>/Utilities  
cmAddCustomCommandCommand.o cmAddCustomTargetCommand.o 
cmAddDefinitionsCommand.o cmAddDependenciesCommand.o cmAddExecutableCommand.o 
cmAddLibraryCommand.o cmAddSubDirectoryCommand.o cmAddTestCommand.o 
cmArgumentParser.o cmBreakCommand.o cmBuildCommand.o cmCMakeMinimumRequired.o 
cmCMakePolicyCommand.o cmCPackPropertiesGenerator.o cmCacheManager.o 
cmCommand.o cmCommandArgumentParserHelper.o cmCommands.o 
cmCommonTargetGenerator.o cmComputeComponentGraph.o cmComputeLinkDepends.o 
cmComputeLinkInformation.o cmComputeTargetDepends.o cmConditionEvaluator.o 
cmConfigureFileCommand.o cmContinueCommand.o cmCoreTryCompile.o 
cmCreateTestSourceList.o cmCustomCommand.o cmCustomCommandGenerator.o 
cmDefinePropertyCommand.o cmDefinitions.o cmDepends.o cmDependsC.o 
cmDisallowedCommand.o cmDocumentationFormatter.o cmEnableLanguageCommand.o 
cmEnableTestingCommand.o cmExecProgramCommand.o cmExecuteProcessCommand.o 
cmExpandedCommandArgument.o cmExportBuildFileGenerator.o 
cmExportFileGenerator.o cmExportInstallFileGenerator.o cmExportSet.o 
cmExportSetMap.o cmExportTryCompileFileGenerator.o cmExprParserHelper.o 
cmExternalMakefileProjectGenerator.o cmFileCommand.o cmFileCopier.o 
cmFileInstaller.o cmFileTime.o cmFileTimeCache.o cmFileTimes.o cmFindBase.o 
cmFindCommon.o cmFindFileCommand.o cmFindLibraryCommand.o 
cmFindPackageCommand.o cmFindPathCommand.o cmFindProgramCommand.o 
cmForEachCommand.o cmFunctionCommand.o cmFSPermissions.o 
cmGeneratedFileStream.o cmGeneratorExpression.o cmGeneratorExpressionContext.o 
cmGeneratorExpressionDAGChecker.o cmGeneratorExpressionEvaluationFile.o 
cmGeneratorExpressionEvaluator.o cmGeneratorExpressionLexer.o 
cmGeneratorExpressionNode.o cmGeneratorExpressionParser.o cmGeneratorTarget.o 
cmGetCMakePropertyCommand.o cmGetDirectoryPropertyCommand.o 
cmGetFilenameComponentCommand.o cmGetPipes.o cmGetPropertyCommand.o 
cmGetSourceFilePropertyCommand.o cmGetTargetPropertyCommand.o 
cmGetTestPropertyCommand.o cmGlobalCommonGenerator.o cmGlobalGenerator.o 
cmGlobalUnixMakefileGenerator3.o cmGlobVerificationManager.o 
cmHexFileConverter.o cmIfCommand.o cmIncludeCommand.o cmIncludeGuardCommand.o 
cmIncludeDirectoryCommand.o cmIncludeRegularExpressionCommand.o 
cmInstallCommand.o cmInstallCommandArguments.o cmInstallDirectoryGenerator.o 
cmInstallExportGenerator.o cmInstallFilesCommand.o cmInstallFilesGenerator.o 
cmInstallGenerator.o cmInstallScriptGenerator.o 
cmInstallSubdirectoryGenerator.o cmInstallTargetGenerator.o 
cmInstallTargetsCommand.o cmInstalledFile.o cmLinkDirectoriesCommand.o 
cmLinkItem.o cmLinkLineComputer.o cmLinkLineDeviceComputer.o cmListCommand.o 
cmListFileCache.o cmLocalCommonGenerator.o cmLocalGenerator.o 
cmLocalUnixMakefileGenerator3.o cmMSVC60LinkLineComputer.o cmMacroCommand.o 
cmMakeDirectoryCommand.o cmMakefile.o cmMakefileExecutableTargetGenerator.o 
cmMakefileLibraryTargetGenerator.o cmMakefileTargetGenerator.o 
cmMakefileUtilityTargetGenerator.o cmMarkAsAdvancedCommand.o cmMathCommand.o 
cmMessageCommand.o cmMessenger.o cmNewLineStyle.o cmOSXBundleGenerator.o 
cmOptionCommand.o cmOrderDirectories.o cmOutputConverter.o 
cmParseArgumentsCommand.o cmPathLabel.o cmPolicies.o cmProcessOutput.o 
cmProjectCommand.o cmProperty.o cmPropertyDefinition.o 
cmPropertyDefinitionMap.o cmPropertyMap.o cmReturnCommand.o 
cmRulePlaceholderExpander.o cmScriptGenerator.o cmSearchPath.o 
cmSeparateArgumentsCommand.o cmSetCommand.o cmSetDirectoryPropertiesCommand.o 
cmSetPropertyCommand.o cmSetSourceFilesPropertiesCommand.o 
cmSetTargetPropertiesCommand.o cmSetTestsPropertiesCommand.o 
cmSiteNameCommand.o cmSourceFile.o cmSourceFileLocation.o cmState

Re: [PATCH] tests/boot-serial-test: Allow the HPPA machine to shudown

[Philippe Mathieu-Daudé]

On 1/24/20 8:02 AM, Thomas Huth wrote:

On 23/01/2020 22.37, Philippe Mathieu-Daudé wrote:

On 1/23/20 7:29 PM, Philippe Mathieu-Daudé wrote:

On 1/23/20 5:39 AM, Thomas Huth wrote:

On 23/01/2020 01.36, Philippe Mathieu-Daudé wrote:

The boot-serial test uses SeaBIOS on HPPA, and expects to read the
"SeaBIOS wants SYSTEM HALT" string, see [*]:

   122 void __VISIBLE __noreturn hlt(void)
   123 {
   124 if (pdc_debug)
   125 printf("HALT initiated from %p\n",
__builtin_return_address(0));
   126 printf("SeaBIOS wants SYSTEM HALT.\n\n");
   127 asm volatile("\t.word 0xfffdead0": : :"memory");
   128 while (1);
   129 }

A 'SYSTEM HALT' would really halts the CPU, but SeaBIOS implements
it as an infinite loop.

If SeaBIOS does not use the expected serial port but another device,
we might poll the console indefinitely while the machine is halted.

Allow the HPPA machine to 'shutdown'. When it does, we'll get
a qtest error:

    $ make check-qtest-hppa
  TEST    check-qtest-hppa: tests/qtest/boot-serial-test
    ** (tests/qtest/boot-serial-test:31924): ERROR **: 01:12:37.604:
Failed to find expected string. Please check
'/tmp/qtest-boot-serial-sjxoM6Q'
    ERROR - Bail out! FATAL-ERROR: Failed to find expected string.
Please check '/tmp/qtest-boot-serial-sjxoM6Q'
    make: *** [tests/Makefile.include:628: check-qtest-hppa] Error 1


The tests are run with -no-shutdown. Why does qemu exit in that case?


Because the HPPA firmware called HALT.


Sounds like a bug in another place, and not in the boot-serial-test.


Yes, the bug is elsewhere, but with the bug the boot-serial-test hangs
forever No output on the console, qtest waiting indefinitely.


Richard explained me on IRC what you probably meant, which was not
obvious to me at first.


I think I also did not really understand what you tried to do here ;-)

The -no-shutdown is required, too, otherwise you could get a race
between the test reading the serial output and the termination of QEMU,
see commit 7150d34a1d60851d73d6ab6783b12b1d25e68f86, and I think we need
that for HPPA, too.


Now I found in check_guest_output():

     /* Poll serial output... */
     while (1) {
     ...
     /* Wait at most 360 seconds.  */
     now = time(NULL);
     if (now - start >= 360) {
     break;
     }
     g_usleep(1);
     }

$ QTEST_QEMU_BINARY=hppa-softmmu/qemu-system-hppa \
   time tests/qtest/boot-serial-test -k
/hppa/boot-serial/hppa:
** (tests/qtest/boot-serial-test:18604): ERROR **: 22:33:25.010: Failed
to find expected string. Please check '/tmp/qtest-boot-serial-sZq7ljM'
Command terminated by signal 5
0.31user 0.66system 6:00.07elapsed 0%CPU

Indeed the test fails after 6min, I guess I didn't expect that much
while testing interactively.


Yeah, the huge timeout is ugly, but it is required for very slow hosts,
see commit 627fce617868df87b3757375a2a0318ad2beb381.

So if you want to "fix" your problem, I think you could maybe add a
check for the QEMU events here instead. Or add a check to see whether
the registers of the guest change between iterations to make sure that
the guest is still alive (that way you also handle the case that the
guest crashed and loops forever in a "branch self" instruction). Or
simply continue with the 360s timeout, it's long, but it should trigger
only in case of other bugs, so it's maybe not worth the effort to add
more logic here.


Good idea. With QMP it is easy:

{ "execute": "query-status" }
{
"return": {
"status": "shutdown",
"singlestep": false,
"running": false
}
}

[PATCH v2 0/2] qemu-img: Add --target-is-zero to indicate that a target is blank

[David Edmondson]

qemu-img: Add --target-is-zero to indicate that a target is blank

v2:
- Remove target_is_zero, preferring to set has_zero_init
  directly (Mark Kanda).
- Disallow --target-is-zero in the presence of a backing file (Max
  Reitz).
- Add relevant documentation (Max Reitz).
- @var -> @code for options in qemu-img.texi.


David Edmondson (2):
  qemu-img: Add --target-is-zero to convert
  doc: Use @code rather than @var for options

 qemu-img-cmds.hx |  4 ++--
 qemu-img.c   | 25 ++---
 qemu-img.texi| 20 
 3 files changed, 36 insertions(+), 13 deletions(-)

-- 
2.24.1

[PATCH v2 1/2] qemu-img: Add --target-is-zero to convert

[David Edmondson]

In many cases the target of a convert operation is a newly provisioned
target that the user knows is blank (filled with zeroes). In this
situation there is no requirement for qemu-img to wastefully zero out
the entire device.

Add a new option, --target-is-zero, allowing the user to indicate that
an existing target device is already zero filled.

Signed-off-by: David Edmondson 
---
 qemu-img-cmds.hx |  4 ++--
 qemu-img.c   | 25 ++---
 qemu-img.texi|  4 
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1c93e6d185..6f958a0a48 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -44,9 +44,9 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-"convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] 
[-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B 
backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m 
num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
+"convert [--object objectdef] [--image-opts] [--target-image-opts] 
[--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T 
src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] 
[-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 
[...]] output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] 
[-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T 
@var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o 
@var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m 
@var{num_coroutines}] [-W] [--salvage] @var{filename} [@var{filename2} [...]] 
@var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] 
[--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t 
@var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] 
[-o @var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m 
@var{num_coroutines}] [-W] [--salvage] @var{filename} [@var{filename2} [...]] 
@var{output_filename}
 ETEXI
 
 DEF("create", img_create,
diff --git a/qemu-img.c b/qemu-img.c
index 6233b8ca56..46db72dbe0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -70,6 +70,7 @@ enum {
 OPTION_PREALLOCATION = 265,
 OPTION_SHRINK = 266,
 OPTION_SALVAGE = 267,
+OPTION_TARGET_IS_ZERO = 268,
 };
 
 typedef enum OutputFormat {
@@ -1984,10 +1985,9 @@ static int convert_do_copy(ImgConvertState *s)
 int64_t sector_num = 0;
 
 /* Check whether we have zero initialisation or can get it efficiently */
-if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
+if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
+!s->target_has_backing) {
 s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
-} else {
-s->has_zero_init = false;
 }
 
 if (!s->has_zero_init && !s->target_has_backing &&
@@ -2086,6 +2086,7 @@ static int img_convert(int argc, char **argv)
 {"force-share", no_argument, 0, 'U'},
 {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
 {"salvage", no_argument, 0, OPTION_SALVAGE},
+{"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
 {0, 0, 0, 0}
 };
 c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
@@ -2209,6 +2210,14 @@ static int img_convert(int argc, char **argv)
 case OPTION_TARGET_IMAGE_OPTS:
 tgt_image_opts = true;
 break;
+case OPTION_TARGET_IS_ZERO:
+/*
+ * The user asserting that the target is blank has the
+ * same effect as the target driver supporting zero
+ * initialisation.
+ */
+s.has_zero_init = true;
+break;
 }
 }
 
@@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
 warn_report("This will become an error in future QEMU versions.");
 }
 
+if (s.has_zero_init && !skip_create) {
+error_report("--target-is-zero requires use of -n flag");
+goto fail_getopt;
+}
+
 s.src_num = argc - optind - 1;
 out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
 
@@ -2380,6 +2394,11 @@ static int img_convert(int argc, char **argv)
 }
 s.target_has_backing = (bool) out_baseimg;
 
+if (s.has_zero_init && s.target_has_backing) {
+error_report("Cannot use --target-is-zero with a backing file");
+goto out;
+}
+
 if (s.src_num > 1 && out_baseimg) {
 error_report("Having a backing file for the target makes no sense when 
"
  "concatenating multiple input images");
diff --git a/qemu-img.texi b/qemu-img.texi
index b5156d6316..3b6dfd8682 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -179,6 +179,10 @@ information.
 Try to ignore I/O errors when reading.  Unless in

Re: [PATCH v8 01/11] add device_legacy_reset function to prepare for reset api change

[Philippe Mathieu-Daudé]

On 1/23/20 2:28 PM, Damien Hedde wrote:

Provide a temporary device_legacy_reset function doing what
device_reset does to prepare for the transition with Resettable
API.

All occurrence of device_reset in the code tree are also replaced
by device_legacy_reset.

The new resettable API has different prototype and semantics
(resetting child buses as well as the specified device). Subsequent
commits will make the changeover for each call site individually; once
that is complete device_legacy_reset() will be removed.

Signed-off-by: Damien Hedde 
Reviewed-by: Peter Maydell 
Reviewed-by: Richard Henderson 
Acked-by: David Gibson 
Acked-by: Cornelia Huck 
Tested-by: Philippe Mathieu-Daudé 


Reviewed-by: Philippe Mathieu-Daudé 

[PATCH v2 2/2] doc: Use @code rather than @var for options

[David Edmondson]

Texinfo defines @var for metasyntactic variables and such terms are
shown in upper-case or italics in the output of makeinfo. When
considering an option to a command, such as "-n", upper-casing is
undesirable as it may confuse the reader or be in conflict with the
equivalent upper-case option.

Replace the use of @var for options with @code to avoid this.

Signed-off-by: David Edmondson 
---
 qemu-img.texi | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/qemu-img.texi b/qemu-img.texi
index 3b6dfd8682..6b4a1ac961 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -74,13 +74,13 @@ keys.
 @item --image-opts
 Indicates that the source @var{filename} parameter is to be interpreted as a
 full option string, not a plain filename. This parameter is mutually
-exclusive with the @var{-f} parameter.
+exclusive with the @code{-f} parameter.
 
 @item --target-image-opts
 Indicates that the @var{output_filename} parameter(s) are to be interpreted as
 a full option string, not a plain filename. This parameter is mutually
-exclusive with the @var{-O} parameters. It is currently required to also use
-the @var{-n} parameter to skip image creation. This restriction may be relaxed
+exclusive with the @code{-O} parameters. It is currently required to also use
+the @code{-n} parameter to skip image creation. This restriction may be relaxed
 in a future release.
 
 @item --force-share (-U)
@@ -103,13 +103,13 @@ with or without a command shows help and lists the 
supported formats
 
 @item -p
 display progress bar (compare, convert and rebase commands only).
-If the @var{-p} option is not used for a command that supports it, the
+If the @code{-p} option is not used for a command that supports it, the
 progress is reported when the process receives a @code{SIGUSR1} or
 @code{SIGINFO} signal.
 
 @item -q
 Quiet mode - do not print any output (except errors). There's no progress bar
-in case both @var{-q} and @var{-p} options are used.
+in case both @code{-q} and @code{-p} options are used.
 
 @item -S @var{size}
 indicates the consecutive number of bytes that must contain only zeros
@@ -298,14 +298,14 @@ the top image stays valid).
 Check if two images have the same content. You can compare images with
 different format or settings.
 
-The format is probed unless you specify it by @var{-f} (used for
-@var{filename1}) and/or @var{-F} (used for @var{filename2}) option.
+The format is probed unless you specify it by @code{-f} (used for
+@var{filename1}) and/or @code{-F} (used for @var{filename2}) option.
 
 By default, images with different size are considered identical if the larger
 image contains only unallocated and/or zeroed sectors in the area after the end
 of the other image. In addition, if any sector is not allocated in one image
 and contains only zero bytes in the second one, it is evaluated as equal. You
-can use Strict mode by specifying the @var{-s} option. When compare runs in
+can use Strict mode by specifying the @code{-s} option. When compare runs in
 Strict mode, it fails in case image size differs or a sector is allocated in
 one image and is not allocated in the second one.
 
-- 
2.24.1

Re: [PATCH v3 01/21] migration-test: Use g_free() instead of free()

[Daniel P . Berrangé]

On Thu, Jan 23, 2020 at 12:58:11PM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela 
> ---
>  tests/qtest/migration-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 26e2e77289..b6a74a05ce 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1291,7 +1291,7 @@ static void test_multifd_tcp(void)
>  wait_for_serial("dest_serial");
>  wait_for_migration_complete(from);
>  test_migrate_end(from, to, true);
> -free(uri);
> +g_free(uri);

Not an objection to this patch, just a general FYI.

Our min glib guarantees that g_malloc/g_free are always using the
system allocator. So using free() is not a correctness problem
these days.

In general I'd suggest eliminating both free() and g_free(), and instead
annotating the variable decl for automatic free. eg

  g_autofree char *uri = NULL;


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 v3 10/21] ram_addr: Split RAMBlock definition

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> We need some of the fields without having to poison everything else.
> 
> Signed-off-by: Juan Quintela 

OK.
(I guess the copyright matches the file we're splitting from; would be
good to update the date some time).

I wondered if any of the RAMBlock related function declarations should
move as wlel; but it's not obvious which ones.

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  MAINTAINERS |  1 +
>  include/exec/ram_addr.h | 40 +-
>  include/exec/ramblock.h | 64 +
>  3 files changed, 66 insertions(+), 39 deletions(-)
>  create mode 100644 include/exec/ramblock.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2c768ed3d8..3732f746b3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1965,6 +1965,7 @@ F: ioport.c
>  F: include/exec/memop.h
>  F: include/exec/memory.h
>  F: include/exec/ram_addr.h
> +F: include/exec/ramblock.h
>  F: memory.c
>  F: include/exec/memory-internal.h
>  F: exec.c
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 5adebb0bc7..5e59a3d8d7 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -24,45 +24,7 @@
>  #include "hw/xen/xen.h"
>  #include "sysemu/tcg.h"
>  #include "exec/ramlist.h"
> -
> -struct RAMBlock {
> -struct rcu_head rcu;
> -struct MemoryRegion *mr;
> -uint8_t *host;
> -uint8_t *colo_cache; /* For colo, VM's ram cache */
> -ram_addr_t offset;
> -ram_addr_t used_length;
> -ram_addr_t max_length;
> -void (*resized)(const char*, uint64_t length, void *host);
> -uint32_t flags;
> -/* Protected by iothread lock.  */
> -char idstr[256];
> -/* RCU-enabled, writes protected by the ramlist lock */
> -QLIST_ENTRY(RAMBlock) next;
> -QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
> -int fd;
> -size_t page_size;
> -/* dirty bitmap used during migration */
> -unsigned long *bmap;
> -/* bitmap of already received pages in postcopy */
> -unsigned long *receivedmap;
> -
> -/*
> - * bitmap to track already cleared dirty bitmap.  When the bit is
> - * set, it means the corresponding memory chunk needs a log-clear.
> - * Set this up to non-NULL to enable the capability to postpone
> - * and split clearing of dirty bitmap on the remote node (e.g.,
> - * KVM).  The bitmap will be set only when doing global sync.
> - *
> - * NOTE: this bitmap is different comparing to the other bitmaps
> - * in that one bit can represent multiple guest pages (which is
> - * decided by the `clear_bmap_shift' variable below).  On
> - * destination side, this should always be NULL, and the variable
> - * `clear_bmap_shift' is meaningless.
> - */
> -unsigned long *clear_bmap;
> -uint8_t clear_bmap_shift;
> -};
> +#include "exec/ramblock.h"
>  
>  /**
>   * clear_bmap_size: calculate clear bitmap size
> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
> new file mode 100644
> index 00..07d50864d8
> --- /dev/null
> +++ b/include/exec/ramblock.h
> @@ -0,0 +1,64 @@
> +/*
> + * Declarations for cpu physical memory functions
> + *
> + * Copyright 2011 Red Hat, Inc. and/or its affiliates
> + *
> + * Authors:
> + *  Avi Kivity 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later.  See the COPYING file in the top-level directory.
> + *
> + */
> +
> +/*
> + * This header is for use by exec.c and memory.c ONLY.  Do not include it.
> + * The functions declared here will be removed soon.
> + */
> +
> +#ifndef QEMU_EXEC_RAMBLOCK_H
> +#define QEMU_EXEC_RAMBLOCK_H
> +
> +#ifndef CONFIG_USER_ONLY
> +#include "cpu-common.h"
> +
> +struct RAMBlock {
> +struct rcu_head rcu;
> +struct MemoryRegion *mr;
> +uint8_t *host;
> +uint8_t *colo_cache; /* For colo, VM's ram cache */
> +ram_addr_t offset;
> +ram_addr_t used_length;
> +ram_addr_t max_length;
> +void (*resized)(const char*, uint64_t length, void *host);
> +uint32_t flags;
> +/* Protected by iothread lock.  */
> +char idstr[256];
> +/* RCU-enabled, writes protected by the ramlist lock */
> +QLIST_ENTRY(RAMBlock) next;
> +QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
> +int fd;
> +size_t page_size;
> +/* dirty bitmap used during migration */
> +unsigned long *bmap;
> +/* bitmap of already received pages in postcopy */
> +unsigned long *receivedmap;
> +
> +/*
> + * bitmap to track already cleared dirty bitmap.  When the bit is
> + * set, it means the corresponding memory chunk needs a log-clear.
> + * Set this up to non-NULL to enable the capability to postpone
> + * and split clearing of dirty bitmap on the remote node (e.g.,
> + * KVM).  The bitmap will be set only when doing global sync.
> + *
> + * NOTE: this bitmap is different comparing to the other bitmaps
> + * in that one bit can r

Re: [PATCH rc2 12/25] hw/timer: Add limited support for Atmel 16 bit timer peripheral

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> From: Michael Rolnik 
>
> These were designed to facilitate testing but should provide enough
> function to be useful in other contexts.  Only a subset of the functions
> of each peripheral is implemented, mainly due to the lack of a standard
> way to handle electrical connections (like GPIO pins).
>
> Signed-off-by: Sarah Harris 
> Message-Id: <20200118191416.19934-13-mrol...@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé 
> [rth: Squash info mtree fixes and a file rename from f4bug, which was:]
> Suggested-by: Aleksandar Markovic 
> Signed-off-by: Richard Henderson 
> [PMD: Use qemu_log_mask(LOG_UNIMP), replace goto by return]
> Signed-off-by: Philippe Mathieu-Daudé 
> ---

> --- /dev/null
> +++ b/include/hw/timer/atmel_timer16.h
> @@ -0,0 +1,94 @@
> +/*
> + * Atmel AVR 16 bit timer
> + *
> + * Copyright (c) 2018 University of Kent
> + * Author: Ed Robbins

No sign off from the author here?
> --- /dev/null
> +++ b/hw/timer/atmel_timer16.c
> @@ -0,0 +1,605 @@

> +
> +/* Helper macros */
> +#define VAL16(l, h) ((h << 8) | l)
> +#define DB_PRINT(fmt, args...) /* Nothing */
> +/*#define DB_PRINT(fmt, args...) printf("%s: " fmt "\n", __func__, ##
> args)*/

Format strings are likely to bitrot. Either use a if (GATE) or
tracepoints.



Otherwise:

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH 1/2] qdev: Introduce qdev_get_bus_device

[Igor Mammedov]

On Wed, 15 Jan 2020 23:40:24 +0100
Julia Suvorova  wrote:

I's add () at the end of SUJ so it would be obvious that's a function

> For bus devices, it is useful to be able to handle the parent device.
maybe something like that would be more clear:

Add a wrapper qdev_get_bus_device() to replace dev->parent_bus->parent,
(add why here)

> 
> Signed-off-by: Julia Suvorova 
> ---
>  hw/core/qdev.c  |  5 +
>  hw/pci-bridge/pci_expander_bridge.c |  4 +++-
>  hw/scsi/scsi-bus.c  |  4 +++-
>  hw/usb/bus.c|  4 +++-
>  hw/usb/dev-smartcard-reader.c   | 32 +
>  hw/virtio/virtio-pci.c  | 16 +--
>  include/hw/qdev-core.h  |  2 ++
>  7 files changed, 54 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 9f1753f5cf..ad8226e240 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -114,6 +114,11 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>  }
>  }
>  
> +DeviceState *qdev_get_bus_device(const DeviceState *dev)
> +{
> +return dev->parent_bus ? dev->parent_bus->parent : NULL;

Does any caller expect to get NULL?
If not I'd move asserts you introduce below to this place only
and drop condition.

> +}
> +
>  /* Create a new device.  This only initializes the device state
> structure and allows properties to be set.  The device still needs
> to be realized.  See qdev-core.h.  */
> diff --git a/hw/pci-bridge/pci_expander_bridge.c 
> b/hw/pci-bridge/pci_expander_bridge.c
> index 0592818447..63a6c07406 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -125,9 +125,11 @@ static char *pxb_host_ofw_unit_address(const 
> SysBusDevice *dev)
>  assert(position >= 0);
>  
>  pxb_dev_base = DEVICE(pxb_dev);
> -main_host = PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);
> +main_host = PCI_HOST_BRIDGE(qdev_get_bus_device(pxb_dev_base));
>  main_host_sbd = SYS_BUS_DEVICE(main_host);
>  
> +g_assert(main_host);
> +
>  if (main_host_sbd->num_mmio > 0) {
>  return g_strdup_printf(TARGET_FMT_plx ",%x",
> main_host_sbd->mmio[0].addr, position + 1);
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index ad0e7f6d88..3d9497882b 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1558,10 +1558,12 @@ void scsi_device_purge_requests(SCSIDevice *sdev, 
> SCSISense sense)
>  static char *scsibus_get_dev_path(DeviceState *dev)
>  {
>  SCSIDevice *d = SCSI_DEVICE(dev);
> -DeviceState *hba = dev->parent_bus->parent;
> +DeviceState *hba = qdev_get_bus_device(dev);
>  char *id;
>  char *path;
>  
> +g_assert(hba);
> +
>  id = qdev_get_dev_path(hba);
>  if (id) {
>  path = g_strdup_printf("%s/%d:%d:%d", id, d->channel, d->id, d->lun);
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index a6522f5429..26bf794315 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -587,9 +587,11 @@ static void usb_bus_dev_print(Monitor *mon, DeviceState 
> *qdev, int indent)
>  static char *usb_get_dev_path(DeviceState *qdev)
>  {
>  USBDevice *dev = USB_DEVICE(qdev);
> -DeviceState *hcd = qdev->parent_bus->parent;
> +DeviceState *hcd = qdev_get_bus_device(qdev);
>  char *id = NULL;
>  
> +g_assert(hcd);
> +
>  if (dev->flags & (1 << USB_DEV_FLAG_FULL_PATH)) {
>  id = qdev_get_dev_path(hcd);
>  }
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index 4568db2568..fbb3599ddd 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -1185,10 +1185,12 @@ void ccid_card_send_apdu_to_guest(CCIDCardState *card,
>uint8_t *apdu, uint32_t len)
>  {
>  DeviceState *qdev = DEVICE(card);
> -USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +USBDevice *dev = USB_DEVICE(qdev_get_bus_device(qdev));
>  USBCCIDState *s = USB_CCID_DEV(dev);
>  Answer *answer;
>  
> +g_assert(dev);
> +
>  if (!ccid_has_pending_answers(s)) {
>  DPRINTF(s, 1, "CCID ERROR: got an APDU without pending answers\n");
>  return;
> @@ -1208,9 +1210,11 @@ void ccid_card_send_apdu_to_guest(CCIDCardState *card,
>  void ccid_card_card_removed(CCIDCardState *card)
>  {
>  DeviceState *qdev = DEVICE(card);
> -USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +USBDevice *dev = USB_DEVICE(qdev_get_bus_device(qdev));
>  USBCCIDState *s = USB_CCID_DEV(dev);
>  
> +g_assert(dev);
> +
>  ccid_on_slot_change(s, false);
>  ccid_flush_pending_answers(s);
>  ccid_reset(s);
> @@ -1219,9 +1223,11 @@ void ccid_card_card_removed(CCIDCardState *card)
>  int ccid_card_ccid_attach(CCIDCardState *card)
>  {
>  DeviceState *qdev = DEVICE(card);
> -USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +USBDevice *dev = USB_DEVICE(qde

Re: [PATCH] iscsi: Cap block count from GET LBA STATUS (CVE-2020-1711)

[Felipe Franciosi]

> On Jan 24, 2020, at 10:04 AM, Philippe Mathieu-Daudé  
> wrote:
> 
> On 1/23/20 11:58 PM, Peter Lieven wrote:
>>> Am 23.01.2020 um 22:29 schrieb Felipe Franciosi :
 On Jan 23, 2020, at 5:46 PM, Philippe Mathieu-Daudé  
 wrote:
> On 1/23/20 1:44 PM, Felipe Franciosi wrote:
> When querying an iSCSI server for the provisioning status of blocks (via
> GET LBA STATUS), Qemu only validates that the response descriptor zero's
> LBA matches the one requested. Given the SCSI spec allows servers to
> respond with the status of blocks beyond the end of the LUN, Qemu may
> have its heap corrupted by clearing/setting too many bits at the end of
> its allocmap for the LUN.
> A malicious guest in control of the iSCSI server could carefully program
> Qemu's heap (by selectively setting the bitmap) and then smash it.
> This limits the number of bits that iscsi_co_block_status() will try to
> update in the allocmap so it can't overflow the bitmap.
 
 Please add:
 
 Fixes: CVE-2020-1711 (title of CVE if possible)
>>> 
>>> I wasn't sure we had one yet. Kevin: can you do the needful in your branch?
>>> 
 Cc: qemu-sta...@nongnu.org
>>> 
>>> Yeah, that's there.
>>> 
 
> Signed-off-by: Felipe Franciosi 
> Signed-off-by: Peter Turschmid 
> Signed-off-by: Raphael Norwitz 
> ---
> block/iscsi.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 2aea7e3f13..cbd57294ab 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -701,7 +701,7 @@ static int coroutine_fn 
> iscsi_co_block_status(BlockDriverState *bs,
> struct scsi_get_lba_status *lbas = NULL;
> struct scsi_lba_status_descriptor *lbasd = NULL;
> struct IscsiTask iTask;
> -uint64_t lba;
> +uint64_t lba, max_bytes;
> int ret;
>   iscsi_co_init_iscsitask(iscsilun, &iTask);
> @@ -721,6 +721,7 @@ static int coroutine_fn 
> iscsi_co_block_status(BlockDriverState *bs,
> }
>   lba = offset / iscsilun->block_size;
> +max_bytes = (iscsilun->num_blocks - lba) * iscsilun->block_size;
>   qemu_mutex_lock(&iscsilun->mutex);
> retry:
> @@ -764,7 +765,7 @@ retry:
> goto out_unlock;
> }
> -*pnum = (int64_t) lbasd->num_blocks * iscsilun->block_size;
> +*pnum = MIN((int64_t) lbasd->num_blocks * iscsilun->block_size, 
> max_bytes);
>   if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
> lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {
 
 What about this?
 
 -- >8 --
 diff --git a/block/iscsi.c b/block/iscsi.c
 index 2aea7e3f13..25598accbb 100644
 --- a/block/iscsi.c
 +++ b/block/iscsi.c
 @@ -506,6 +506,11 @@ iscsi_allocmap_update(IscsiLun *iscsilun, int64_t 
 offset,
/* shrink to touch only completely contained clusters */
cl_num_shrunk = DIV_ROUND_UP(offset, iscsilun->cluster_size);
nb_cls_shrunk = (offset + bytes) / iscsilun->cluster_size - 
 cl_num_shrunk;
 +if (nb_cls_expanded >= iscsilun->allocmap_size
 +|| nb_cls_shrunk >= iscsilun->allocmap_size) {
 +error_report("iSCSI invalid request: ..." /* TODO */);
 +return;
 +}
if (allocated) {
bitmap_set(iscsilun->allocmap, cl_num_expanded, nb_cls_expanded);
} else {
 ---
>>> 
>>> I'm not sure the above is correct because (if I read this right)
>>> nb_cls_* represents the number of clusters, not the last cluster.
>>> 
>>> Personally, I would have the checks (or "trim"s) closer to where they
>>> were issued (to fail sooner) and assert()s closer to bitmap (as no oob
>>> accesses should be happening at this point). There were also
>>> discussions about using safer (higher level) bitmaps for this. I'm
>>> always in favour of adding all reasonable checks. :)
>> I would add assertions that cl_num + nb_cls <= allocmap_size before every 
>> set and clear.
> 
> The description starts with "A malicious guest in control of the iSCSI server 
> ..." so asserting (and killing the VM) doesn't seem correct...

Correct. That's why I would have the proper checks (or "trim"s) closer
to where they were issued to fail sooner. What I meant is that if a
guest issues any operation that spans past the end of the drive, then
the operation stops there and an error is returned accordingly.

This means nothing should ever try to touch these bitmaps out of
bounds. Nevertheless, and further to that, assert()s can be used
closer to where the bitmap is touched to catch programming errors.

> I suppose the iSCSI protocol has some error to return for invalid requests.

Which invalid you are referring to? From the initiator or the target?
AFAICT the problem is that the SCSI SPEC doesn't limit a target to
respond provisioning status past the (current) end of the L

Re: [PATCH v4 0/4] Improve default object property_add uint helpers

[Felipe Franciosi]

Hi Marc-Andre and Paolo,

> On Dec 20, 2019, at 3:15 PM, Marc-André Lureau  
> wrote:
> 
> Hi
> 
> On Thu, Dec 19, 2019 at 10:02 PM Felipe Franciosi  wrote:
>> 
>> This improves the family of object_property_add_uintXX_ptr helpers by 
>> enabling
>> a default getter/setter only when desired. To prevent an API behavioural 
>> change
>> (from clients that already used these helpers and did not want a setter), we
>> add a OBJ_PROP_FLAG_READ flag that allow clients to only have a getter. 
>> Patch 1
>> enhances the API and modify current users.
>> 
>> While modifying the clients of the API, a couple of improvement opportunities
>> were observed in ich9. These were added in separate patches (2 and 3).
>> 
>> Patch 3 cleans up a lot of existing code by moving various objects to the
>> enhanced API. Previously, those objects had their own getters/setters that 
>> only
>> updated the values without further checks. Some of them actually lacked a 
>> check
>> for setting overflows, which could have resulted in undesired values being 
>> set.
>> The new default setters include a check for that, not updating the values in
>> case of errors (and propagating them). If they did not provide an error
>> pointer, then that behaviour was maintained.
>> 
>> Felipe Franciosi (4):
>>  qom/object: enable setter for uint types
>>  ich9: fix getter type for sci_int property
>>  ich9: Simplify ich9_lpc_initfn
>>  qom/object: Use common get/set uint helpers
>> 
>> hw/acpi/ich9.c   |  99 ++--
>> hw/acpi/pcihp.c  |   7 +-
>> hw/acpi/piix4.c  |  12 +--
>> hw/isa/lpc_ich9.c|  27 ++
>> hw/misc/edu.c|  13 +--
>> hw/pci-host/q35.c|  14 +--
>> hw/ppc/spapr.c   |  18 +---
>> hw/ppc/spapr_drc.c   |   3 +-
>> include/qom/object.h |  44 +++--
>> memory.c |  15 +--
>> qom/object.c | 216 ++-
>> target/arm/cpu.c |  22 +
>> target/i386/sev.c| 106 ++---
>> ui/console.c |   4 +-
>> 14 files changed, 282 insertions(+), 318 deletions(-)
> 
> It conflicts with some recent changes, so you'll need to send a new
> version, but that one looks good to me:
> Reviewed-by: Marc-André Lureau 
> 
> Paolo, is it going through your queue?

I didn't see any response after this. Did the series get lost?

Cheers,
Felipe

> 
>> 
>> --
>> 2.20.1
>> 
>> Changelog:
>> v1->v2:
>> - Update sci_int directly instead of using stack variable
>> - Defining an enhanced ObjectPropertyFlags instead of just 'readonly'
>> - Erroring out directly (instead of using gotos) on default setters
>> - Retaining lack of errp passing when it wasn't there
>> v2->v3:
>> - Rename flags _RD to _READ and _WR to _WRITE
>> - Add a convenience _READWRITE flag
>> - Drop the usage of UL in the bit flag definitions
>> v3->v4:
>> - Drop changes to hw/vfio/pci-quirks.c
> 
> 
> 
> -- 
> Marc-André Lureau

Re: [Bug 1860759] Re: [REGRESSION] option `-snapshot` ignored with blockdev

[Ildar]

Max, thanks a lot for the explanation.
Do you mean that snapshot-ing isn't possible totally for blockdev? Then I
guess some libvirt users are in trouble :((
Actually I didn't quite caught the reason why a blockdev supports backing
but not {backing to a file on /tmp then promptly deleted} ? What's the
technical difference?

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

Title:
  [REGRESSION] option `-snapshot` ignored with blockdev

Status in QEMU:
  New

Bug description:
  After upgrade of qemu 3.1.0 → 4.2.0 I found that running with libvirt doesn't 
honor `-snapshot` option anymore. I.e. disk images get modified.
  Using `-hda` option honors `-snapshot`

  So I made a test case without libvirt. Testcase using 4.2.0:

  > qemu -hda tmp-16G.img -cdrom regular-rescue-latest-x86_64.iso -m 2G

  This works fine and tmp-16G.img stays unmodified.

  But:
  > /usr/bin/qemu-system-x86_64 -name guest=test-linux,debug-threads=on -S 
-machine pc-i440fx-3.1,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu 
Broadwell-noTSX,vme=on,ss=on,f16c=on,rdrand=on,hypervisor=on,arat=on,tsc-adjust=on,xsaveopt=on,pdpe1gb=on,abm=on
 -m 2048 -overcommit mem-lock=off -smp 3,sockets=3,cores=1,threads=1 -uuid 
d32a9191-f51d-4fae-a419-b73d85b49198 -no-user-config -nodefaults -rtc 
base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet 
-no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot 
strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x5.0x7 -device 
ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x5 
-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x5.0x1 
-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x5.0x2 
-blockdev 
\{\"driver\":\"file\",\"filename\":\"/tmp/regular-rescue-latest-x86_64.iso\",\"node-name\":\"libvirt-2-storage\",\"auto-read-only\":true,\"discard\":\"unmap\"}
 -blockdev 
\{\"node-name\":\"libvirt-2-format\",\"read-only\":true,\"driver\":\"raw\",\"file\":\"libvirt-2-storage\"}
 -device ide-cd,bus=ide.0,unit=0,drive=libvirt-2-format,id=ide0-0-0,bootindex=1 
-blockdev 
\{\"driver\":\"file\",\"filename\":\"/tmp/tmp-2G.img\",\"node-name\":\"libvirt-1-storage\",\"auto-read-only\":true,\"discard\":\"unmap\"}
 -blockdev 
\{\"node-name\":\"libvirt-1-format\",\"read-only\":false,\"driver\":\"qcow2\",\"file\":\"libvirt-1-storage\",\"backing\":null}
 -device 
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=libvirt-1-format,id=virtio-disk0
 -netdev user,id=hostnet0 -device 
e1000,netdev=hostnet0,id=net0,mac=52:54:00:ab:d8:29,bus=pci.0,addr=0x3 -chardev 
pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -device 
qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2
 -device intel-hda,id=sound0,bus=pci.0,addr=0x4 -device 
hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 -snapshot -sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny -msg 
timestamp=on

  This modifies tmp-16G.img.

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

Re: [PATCH rc2 12/25] hw/timer: Add limited support for Atmel 16 bit timer peripheral

[Philippe Mathieu-Daudé]

Hello Sarah,

On 1/24/20 11:42 AM, Alex Bennée wrote:


Philippe Mathieu-Daudé  writes:


From: Michael Rolnik 

These were designed to facilitate testing but should provide enough
function to be useful in other contexts.  Only a subset of the functions
of each peripheral is implemented, mainly due to the lack of a standard
way to handle electrical connections (like GPIO pins).

Signed-off-by: Sarah Harris 
Message-Id: <20200118191416.19934-13-mrol...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
[rth: Squash info mtree fixes and a file rename from f4bug, which was:]
Suggested-by: Aleksandar Markovic 
Signed-off-by: Richard Henderson 
[PMD: Use qemu_log_mask(LOG_UNIMP), replace goto by return]
Signed-off-by: Philippe Mathieu-Daudé 
---



--- /dev/null
+++ b/include/hw/timer/atmel_timer16.h
@@ -0,0 +1,94 @@
+/*
+ * Atmel AVR 16 bit timer
+ *
+ * Copyright (c) 2018 University of Kent
+ * Author: Ed Robbins


No sign off from the author here?


Hmmm I Sarah Harris's one, who is from the University of Kent, isn't it 
enough? (I remember patched from Xilinx with Edgar S-o-b but from other 
authors, Edgar vouched for Xilinx).


Sarah, can you get Ed Signed-off-by?


--- /dev/null
+++ b/hw/timer/atmel_timer16.c
@@ -0,0 +1,605 @@



+
+/* Helper macros */
+#define VAL16(l, h) ((h << 8) | l)
+#define DB_PRINT(fmt, args...) /* Nothing */
+/*#define DB_PRINT(fmt, args...) printf("%s: " fmt "\n", __func__, ##
args)*/


Format strings are likely to bitrot. Either use a if (GATE) or
tracepoints.


Yes...




Otherwise:

Reviewed-by: Alex Bennée 


Thanks!

Re: [PATCH 1/7] migration/block-dirty-bitmap: refactor incoming state to be one struct

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> Move enabled_bitmaps and finish_lock, which are part of incoming state
> to DirtyBitmapLoadState, and make static global variable to store state
> instead of static local one.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  migration/block-dirty-bitmap.c | 77 +++---
>  1 file changed, 43 insertions(+), 34 deletions(-)
>
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 7eafface61..281d20f41d 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -125,6 +125,13 @@ typedef struct DirtyBitmapMigState {
>  BlockDriverState *prev_bs;
>  BdrvDirtyBitmap *prev_bitmap;
>  } DirtyBitmapMigState;
> +static DirtyBitmapMigState dirty_bitmap_mig_state;

Missing new line.

> +
> +typedef struct DirtyBitmapLoadBitmapState {
> +BlockDriverState *bs;
> +BdrvDirtyBitmap *bitmap;
> +bool migrated;
> +} DirtyBitmapLoadBitmapState;
>  
>  typedef struct DirtyBitmapLoadState {
>  uint32_t flags;
> @@ -132,21 +139,15 @@ typedef struct DirtyBitmapLoadState {
>  char bitmap_name[256];
>  BlockDriverState *bs;
>  BdrvDirtyBitmap *bitmap;
> -} DirtyBitmapLoadState;
>  
> -static DirtyBitmapMigState dirty_bitmap_mig_state;
> -
> -typedef struct DirtyBitmapLoadBitmapState {
> -BlockDriverState *bs;
> -BdrvDirtyBitmap *bitmap;
> -bool migrated;
> -} DirtyBitmapLoadBitmapState;
> -static GSList *enabled_bitmaps;
> -QemuMutex finish_lock;
> +GSList *enabled_bitmaps;
> +QemuMutex finish_lock;
> +} DirtyBitmapLoadState;
> +static DirtyBitmapLoadState dbm_load_state;

You move two global variables to an struct (good)
But you create a even bigger global variable (i.e. state that was not
shared before.)

>  /* First occurrence of this bitmap. It should be created if doesn't exist */
> -static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
> +static int dirty_bitmap_load_start(QEMUFile *f)
>  {
> +DirtyBitmapLoadState *s = &dbm_load_state;

You create a local alias.

>  Error *local_err = NULL;
>  uint32_t granularity = qemu_get_be32(f);
>  uint8_t flags = qemu_get_byte(f);
> @@ -482,7 +484,8 @@ static int dirty_bitmap_load_start(QEMUFile *f, 
> DirtyBitmapLoadState *s)
>  b->bs = s->bs;
>  b->bitmap = s->bitmap;
>  b->migrated = false;
> -enabled_bitmaps = g_slist_prepend(enabled_bitmaps, b);
> +dbm_load_state.enabled_bitmaps =
> +g_slist_prepend(dbm_load_state.enabled_bitmaps, b);

And then you access it using the global variable?

> -static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
> +static void dirty_bitmap_load_complete(QEMUFile *f)
>  {
> +DirtyBitmapLoadState *s = &dbm_load_state;

Exactly the same on this function.

I still don't understand why you are removing the pass as parameters of
this function.


> -static DirtyBitmapLoadState s;

Aha, this is why you are moving it as a global.

But, why can't you put this inside dirty_bitmap_mig_state?  Then you:
a- don't have to have "yet" another global variable
b- you can clean it up on save_cleanup handler.

not related to this patch, but to the file in general:

static void dirty_bitmap_save_cleanup(void *opaque)
{
dirty_bitmap_mig_cleanup();
}

We have opaque here, that we can do:

DirtyBitmapMigBitmapState *dbms = opaque;

And then pass dbms to dirty_bitmap_mig_cleanup().

/* Called with iothread lock taken.  */
static void dirty_bitmap_mig_cleanup(void)
{
DirtyBitmapMigBitmapState *dbms;

while ((dbms = QSIMPLEQ_FIRST(&dirty_bitmap_mig_state.dbms_list)) != NULL) {
QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
bdrv_unref(dbms->bs);
g_free(dbms);
}
}


Because here we just use the global variable.

Later, Juan.

Re: [PATCH v3 11/21] multifd: multifd_send_pages only needs the qemufile

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> Signed-off-by: Juan Quintela 

OK, although this is a side effect of multifd_send_state being a global
rather than part of RAMState which might have been cleaner.


Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/ram.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 125c6d0f60..19caf5ed4d 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -929,7 +929,7 @@ struct {
>   * false.
>   */
>  
> -static int multifd_send_pages(RAMState *rs)
> +static int multifd_send_pages(QEMUFile *f)
>  {
>  int i;
>  static int next_channel;
> @@ -965,7 +965,7 @@ static int multifd_send_pages(RAMState *rs)
>  multifd_send_state->pages = p->pages;
>  p->pages = pages;
>  transferred = ((uint64_t) pages->used) * TARGET_PAGE_SIZE + 
> p->packet_len;
> -qemu_file_update_transfer(rs->f, transferred);
> +qemu_file_update_transfer(f, transferred);
>  ram_counters.multifd_bytes += transferred;
>  ram_counters.transferred += transferred;;
>  qemu_mutex_unlock(&p->mutex);
> @@ -993,7 +993,7 @@ static int multifd_queue_page(RAMState *rs, RAMBlock 
> *block, ram_addr_t offset)
>  }
>  }
>  
> -if (multifd_send_pages(rs) < 0) {
> +if (multifd_send_pages(rs->f) < 0) {
>  return -1;
>  }
>  
> @@ -1090,7 +1090,7 @@ static void multifd_send_sync_main(RAMState *rs)
>  return;
>  }
>  if (multifd_send_state->pages->used) {
> -if (multifd_send_pages(rs) < 0) {
> +if (multifd_send_pages(rs->f) < 0) {
>  error_report("%s: multifd_send_pages fail", __func__);
>  return;
>  }
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 2/7] migration/block-dirty-bitmap: rename finish_lock to just lock

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> finish_lock is bad name, as lock used not only on process end.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Juan Quintela 

I still would like the cleanup suggested on the previous patch, but this
one is ok.

Re: Maintainers, please add Message-Id: when merging patches

[Stefan Hajnoczi]

On Thu, Jan 23, 2020 at 06:18:57PM +0100, Kevin Wolf wrote:
> Am 22.01.2020 um 13:28 hat Kevin Wolf geschrieben:
> > Am 22.01.2020 um 13:02 hat Stefan Hajnoczi geschrieben:
> > > Around 66% of qemu.git commits since v4.1.0 include a Message-Id: tag.  
> > > Hooray!
> > > 
> > > Message-Id: references the patch email that a commit was merged from.
> > > This information is helpful to anyone wishing to refer back to email
> > > discussions and patch series.
> > > 
> > > Please use git-am(1) -m/--message-id or set am.messageid in your 
> > > git-config(1).
> > 
> > I've had -m in my scripts for a while (last time someone asked me to
> > make the change, I guess), but it wasn't effective, because my .muttrc
> > has 'set pipe_decode' enabled, which doesn't only decode the output, but
> > also throws away most headers.
> > 
> > I seem to remember that this was necessary at some point because
> > otherwise some mails just wouldn't apply. Maybe 'git am' works better
> > these days and can actually parse the mails that used to give me
> > problems. I'll give it a try and disable pipe_decode.
> 
> Here is the first patch for which it failed for me:
> 
> Message-ID: <20200123124357.124019-1-fel...@nutanix.com>
> 
> The problem seems to be related to line endings because the patch that
> git-apply sees eventually has "\r\n" whereas the file to be patched has
> only "\n".
> 
> If I understand correctly (this is a bit of guesswork after reading man
> pages and trying out a few options), git-mailsplit would normally get
> rid of the "\r". However, this specific patch email is base64 encoded,
> so the encoded "\r" characters survive this stage.
> 
> git-mailinfo later decodes the email, but doesn't seem to do anything
> about "\r" again, so it survives this one as well. This means feeding a
> patch with the wrong line endings to git-apply, which just fails.
> 
> Any suggestion how to fix this? (For this patch, I just enabled
> pipe_decode again, so no Message-Id tag for it.)

This might be a good question for the git mailing list
.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH rc2 22/25] target/avr: Update build system

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> From: Michael Rolnik 
>
> Make AVR support buildable
>
> Signed-off-by: Michael Rolnik 
> Tested-by: Philippe Mathieu-Daudé 
> Reviewed-by: Aleksandar Markovic 
> Message-Id: <20200118191416.19934-19-mrol...@gmail.com>
> Signed-off-by: Richard Henderson 
> ---
>  configure   |  7 +++
>  default-configs/avr-softmmu.mak |  5 +
>  target/avr/Makefile.objs| 34 +
>  3 files changed, 46 insertions(+)
>  create mode 100644 default-configs/avr-softmmu.mak
>  create mode 100644 target/avr/Makefile.objs
>
> diff --git a/configure b/configure
> index 557e4382ea..94e79ca634 100755
> --- a/configure
> +++ b/configure
> @@ -7612,6 +7612,10 @@ case "$target_name" in
>  mttcg="yes"
>  gdb_xml_files="aarch64-core.xml aarch64-fpu.xml arm-core.xml arm-vfp.xml 
> arm-vfp3.xml arm-neon.xml"
>;;
> +  avr)
> +gdb_xml_files="avr-cpu.xml"
> +target_compiler=$cross_cc_avr

I don't think you want this here. target_compiler belongs in the
tests/tcg/configure.sh config.

A docker target with the compiler would also be nice.

> +  ;;
>cris)
>;;
>hppa)
> @@ -7831,6 +7835,9 @@ for i in $ARCH $TARGET_BASE_ARCH ; do
>disas_config "ARM_A64"
>  fi
>;;
> +  avr)
> +disas_config "AVR"
> +  ;;
>cris)
>  disas_config "CRIS"
>;;
> diff --git a/default-configs/avr-softmmu.mak b/default-configs/avr-softmmu.mak
> new file mode 100644
> index 00..80218add98
> --- /dev/null
> +++ b/default-configs/avr-softmmu.mak
> @@ -0,0 +1,5 @@
> +# Default configuration for avr-softmmu
> +
> +# Boards:
> +#
> +CONFIG_ARDUINO=y
> diff --git a/target/avr/Makefile.objs b/target/avr/Makefile.objs
> new file mode 100644
> index 00..7523e0c6e2
> --- /dev/null
> +++ b/target/avr/Makefile.objs
> @@ -0,0 +1,34 @@
> +#
> +#  QEMU AVR CPU
> +#
> +#  Copyright (c) 2019 Michael Rolnik
> +#
> +#  This library is free software; you can redistribute it and/or
> +#  modify it under the terms of the GNU Lesser General Public
> +#  License as published by the Free Software Foundation; either
> +#  version 2.1 of the License, or (at your option) any later version.
> +#
> +#  This library is distributed in the hope that it will be useful,
> +#  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +#  Lesser General Public License for more details.
> +#
> +#  You should have received a copy of the GNU Lesser General Public
> +#  License along with this library; if not, see
> +#  
> +#
> +
> +DECODETREE = $(SRC_PATH)/scripts/decodetree.py
> +decode-y = $(SRC_PATH)/target/avr/insn.decode
> +
> +target/avr/decode_insn.inc.c: $(decode-y) $(DECODETREE)
> + $(call quiet-command, \
> +   $(PYTHON) $(DECODETREE) -o $@ --decode decode_insn --insnwidth 16 $<, 
> \
> +   "GEN", $(TARGET_DIR)$@)
> +
> +target/avr/translate.o: target/avr/decode_insn.inc.c
> +
> +obj-y += translate.o cpu.o helper.o
> +obj-y += gdbstub.o
> +obj-y += disas.o
> +obj-$(CONFIG_SOFTMMU) += machine.o


-- 
Alex Bennée

Re: [PATCH 4/7] migration/block-dirty-bitmap: keep bitmap state for all bitmaps

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> Keep bitmap state for disabled bitmaps too. Keep the state until the
> end of the process. It's needed for the following commit to implement
> bitmap postcopy canceling.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> -
> -b = g_new(DirtyBitmapLoadBitmapState, 1);
> -b->bs = s->bs;
> -b->bitmap = s->bitmap;
> -b->migrated = false;
> -dbm_load_state.enabled_bitmaps =
> -g_slist_prepend(dbm_load_state.enabled_bitmaps, b);
>  }
>  
> +b = g_new(DirtyBitmapLoadBitmapState, 1);
> +*b = (DirtyBitmapLoadBitmapState) {
> +.bs = s->bs,
> +.bitmap = s->bitmap,
> +.migrated = false,
> +.enabled = flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED,
> +};

What is wrong with:
 b->bs = s->bs;
 b->bitmap = s->bitmap;
 b->migrated = false;
 b->enabled = flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED;

???

Later, Juan.

Re: [PULL 000/108] virtiofs queue

[Peter Maydell]

On Thu, 23 Jan 2020 at 19:32, Dr. David Alan Gilbert
 wrote:
>
> * Dr. David Alan Gilbert (git) (dgilb...@redhat.com) wrote:
> > From: "Dr. David Alan Gilbert" 
> >
> > The following changes since commit b7c359c748a2e3ccb97a184b9739feb2cd48de2f:
> >
> >   Merge remote-tracking branch 
> > 'remotes/vivier2/tags/linux-user-for-5.0-pull-request' into staging 
> > (2020-01-23 14:38:43 +)
> >
> > are available in the Git repository at:
> >
> >   g...@gitlab.com:dagrh/qemu.git tags/pull-virtiofs-20200123b
>
> Note the public URI is:
>
>   https://gitlab.com/dagrh/qemu.git



Applied, thanks.

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

-- PMM

Re: [PATCH v3 01/21] migration-test: Use g_free() instead of free()

[Juan Quintela]

Daniel P. Berrangé  wrote:
> On Thu, Jan 23, 2020 at 12:58:11PM +0100, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela 
>> ---
>>  tests/qtest/migration-test.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 26e2e77289..b6a74a05ce 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -1291,7 +1291,7 @@ static void test_multifd_tcp(void)
>>  wait_for_serial("dest_serial");
>>  wait_for_migration_complete(from);
>>  test_migrate_end(from, to, true);
>> -free(uri);
>> +g_free(uri);
>
> Not an objection to this patch, just a general FYI.
>
> Our min glib guarantees that g_malloc/g_free are always using the
> system allocator. So using free() is not a correctness problem
> these days.

Ok.  But the rest of the file uses g_malloc/g_free and friends O:-)

> In general I'd suggest eliminating both free() and g_free(), and instead
> annotating the variable decl for automatic free. eg
>
>   g_autofree char *uri = NULL;

I will investigate this, thanks.

Later, Juan.

[Question] Regarding containers "unattached/peripheral/anonymous" - their relation with hot(un)plug of devices

[Salil Mehta]

Hello,
I am working on vCPU Hotplug feature for ARM64 and I am in mid of understanding 
some aspect of device_add/device_del interface of the QEMU.

Observations:
1. Any object initialised by qmp_device_add() gets into /machine/unattached 
container. I traced the flow to code leg inside  device_set_realized()
2. I could see the reverse qmp_device_del() expects the device to be in  
/machine/peripheral container.
3. I could see any object initially added to unattached container did not had 
their parents until object_add_property_child() was called further in the leg.
which effectively meant a new property was created and property table 
populated and child was parented.
4. Generally, container  /machine/peripheral was being used wherever 
DEVICE(dev)->id was present and non-null.

Question:
1. Wanted to confirm my understanding about the use of having separate 
containers like unattached, peripheral and anonymous.
2. At init time all the vcpus goes under *unattached* container. Now, 
qmp_device_del() cannot be used to unplug them. I am wondering
   if all the hotplug devices need to go under the *peripheral* container while 
they are hotplugged and during object init time as well?
3. I could not see any device being place under *anonymous* container during 
init time. What is the use of this container?

I would be thankful for your valuable insights and answers and help in 
highlighting any gap in my understanding.

Thanks in anticipation!

Best Regards
Salil

Re: [PATCH v3 12/21] multifd: multifd_queue_page only needs the qemufile

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> Signed-off-by: Juan Quintela 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/ram.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 19caf5ed4d..d4c829bc77 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -974,7 +974,7 @@ static int multifd_send_pages(QEMUFile *f)
>  return 1;
>  }
>  
> -static int multifd_queue_page(RAMState *rs, RAMBlock *block, ram_addr_t 
> offset)
> +static int multifd_queue_page(QEMUFile *f, RAMBlock *block, ram_addr_t 
> offset)
>  {
>  MultiFDPages_t *pages = multifd_send_state->pages;
>  
> @@ -993,12 +993,12 @@ static int multifd_queue_page(RAMState *rs, RAMBlock 
> *block, ram_addr_t offset)
>  }
>  }
>  
> -if (multifd_send_pages(rs->f) < 0) {
> +if (multifd_send_pages(f) < 0) {
>  return -1;
>  }
>  
>  if (pages->block != block) {
> -return  multifd_queue_page(rs, block, offset);
> +return  multifd_queue_page(f, block, offset);
>  }
>  
>  return 1;
> @@ -2128,7 +2128,7 @@ static int ram_save_page(RAMState *rs, PageSearchStatus 
> *pss, bool last_stage)
>  static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
>   ram_addr_t offset)
>  {
> -if (multifd_queue_page(rs, block, offset) < 0) {
> +if (multifd_queue_page(rs->f, block, offset) < 0) {
>  return -1;
>  }
>  ram_counters.normal++;
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v3 13/21] multifd: multifd_send_sync_main only needs the qemufile

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> Signed-off-by: Juan Quintela 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/ram.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index d4c829bc77..2783dc60f4 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1082,7 +1082,7 @@ void multifd_save_cleanup(void)
>  multifd_send_state = NULL;
>  }
>  
> -static void multifd_send_sync_main(RAMState *rs)
> +static void multifd_send_sync_main(QEMUFile *f)
>  {
>  int i;
>  
> @@ -1090,7 +1090,7 @@ static void multifd_send_sync_main(RAMState *rs)
>  return;
>  }
>  if (multifd_send_state->pages->used) {
> -if (multifd_send_pages(rs->f) < 0) {
> +if (multifd_send_pages(f) < 0) {
>  error_report("%s: multifd_send_pages fail", __func__);
>  return;
>  }
> @@ -,7 +,7 @@ static void multifd_send_sync_main(RAMState *rs)
>  p->packet_num = multifd_send_state->packet_num++;
>  p->flags |= MULTIFD_FLAG_SYNC;
>  p->pending_job++;
> -qemu_file_update_transfer(rs->f, p->packet_len);
> +qemu_file_update_transfer(f, p->packet_len);
>  ram_counters.multifd_bytes += p->packet_len;
>  ram_counters.transferred += p->packet_len;
>  qemu_mutex_unlock(&p->mutex);
> @@ -3426,7 +3426,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>  ram_control_before_iterate(f, RAM_CONTROL_SETUP);
>  ram_control_after_iterate(f, RAM_CONTROL_SETUP);
>  
> -multifd_send_sync_main(*rsp);
> +multifd_send_sync_main(f);
>  qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>  qemu_fflush(f);
>  
> @@ -3526,7 +3526,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>  out:
>  if (ret >= 0
>  && migration_is_setup_or_active(migrate_get_current()->state)) {
> -multifd_send_sync_main(rs);
> +multifd_send_sync_main(rs->f);
>  qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>  qemu_fflush(f);
>  ram_counters.transferred += 8;
> @@ -3585,7 +3585,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>  }
>  
>  if (ret >= 0) {
> -multifd_send_sync_main(rs);
> +multifd_send_sync_main(rs->f);
>  qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>  qemu_fflush(f);
>  }
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH rc2 00/25] target/avr merger

[Michael Rolnik]

Tested-by: Michael Rolnik 



The only thing I want to change is instead of -kernel put -bios in
qemu-doc.texi file. Should I send a new series?

On Fri, Jan 24, 2020 at 2:51 AM Philippe Mathieu-Daudé 
wrote:

> This is the AVR port from Michael release (merge) candidate 2.
>
> Since v1 [1]:
> - Addressed Thomas comments
> - Fixed a non-critical bug in ATmega (incorrect SRAM base address)
> - Added ELF parsing requested by Aleksandar
> - Dropped default machine (as with the ARM port)
>
> Change since rc1:
>
> $ git backport-diff -u avr-rc1 -r origin/master..
> Key:
> [] : patches are identical
> [] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences,
> respectively
>
> 001/25:[] [--] 'target/avr: Add outward facing interfaces and core CPU
> logic'
> 002/25:[] [--] 'target/avr: Add instruction helpers'
> 003/25:[] [--] 'target/avr: Add instruction translation - Registers
> definition'
> 004/25:[] [--] 'target/avr: Add instruction translation - Arithmetic
> and Logic Instructions'
> 005/25:[] [--] 'target/avr: Add instruction translation - Branch
> Instructions'
> 006/25:[] [--] 'target/avr: Add instruction translation - Data
> Transfer Instructions'
> 007/25:[] [--] 'target/avr: Add instruction translation - Bit and
> Bit-test Instructions'
> 008/25:[] [--] 'target/avr: Add instruction translation - MCU Control
> Instructions'
> 009/25:[] [--] 'target/avr: Add instruction translation - CPU main
> translation function'
> 010/25:[] [--] 'target/avr: Add instruction disassembly function'
> 011/25:[] [--] 'hw/char: Add limited support for Atmel USART
> peripheral'
> 012/25:[0045] [FC] 'hw/timer: Add limited support for Atmel 16 bit timer
> peripheral'
> 013/25:[] [--] 'hw/misc: Add Atmel power device'
> 014/25:[0024] [FC] 'target/avr: Add section about AVR into QEMU
> documentation'
> 015/25:[] [--] 'target/avr: Register AVR support with the rest of QEMU'
> 016/25:[] [--] 'target/avr: Add machine none test'
> 017/25:[0002] [FC] 'target/avr: Update MAINTAINERS file'
> 018/25:[down]  'hw/core/loader: Let load_elf populate the
> processor-specific flags'
> 019/25:[down]  'hw/avr: Add helper to load raw/ELF firmware binaries'
> 020/25:[0015] [FC] 'hw/avr: Add some ATmega microcontrollers'
> 021/25:[0040] [FC] 'hw/avr: Add some Arduino boards'
> 022/25:[] [--] 'target/avr: Update build system'
> 023/25:[] [--] 'tests/boot-serial-test: Test some Arduino boards (AVR
> based)'
> 024/25:[] [--] 'tests/acceptance: Test the Arduino MEGA2560 board'
> 025/25:[] [--] '.travis.yml: Run the AVR acceptance tests'
>
> Repo: https://gitlab.com/philmd/qemu/commits/avr-rc2
>
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg672926.html
> Supersedes: <20200123000307.11541-1-richard.hender...@linaro.org>
>
> Michael Rolnik (20):
>   target/avr: Add outward facing interfaces and core CPU logic
>   target/avr: Add instruction helpers
>   target/avr: Add instruction translation - Registers definition
>   target/avr: Add instruction translation - Arithmetic and Logic
> Instructions
>   target/avr: Add instruction translation - Branch Instructions
>   target/avr: Add instruction translation - Data Transfer Instructions
>   target/avr: Add instruction translation - Bit and Bit-test
> Instructions
>   target/avr: Add instruction translation - MCU Control Instructions
>   target/avr: Add instruction translation - CPU main translation
> function
>   target/avr: Add instruction disassembly function
>   hw/char: Add limited support for Atmel USART peripheral
>   hw/timer: Add limited support for Atmel 16 bit timer peripheral
>   hw/misc: Add Atmel power device
>   target/avr: Add section about AVR into QEMU documentation
>   target/avr: Register AVR support with the rest of QEMU
>   target/avr: Add machine none test
>   target/avr: Update MAINTAINERS file
>   target/avr: Update build system
>   tests/boot-serial-test: Test some Arduino boards (AVR based)
>   tests/acceptance: Test the Arduino MEGA2560 board
>
> Philippe Mathieu-Daudé (5):
>   hw/core/loader: Let load_elf populate the processor-specific flags
>   hw/avr: Add helper to load raw/ELF firmware binaries
>   hw/avr: Add some ATmega microcontrollers
>   hw/avr: Add some Arduino boards
>   .travis.yml: Run the AVR acceptance tests
>
>  qemu-doc.texi|   51 +
>  configure|7 +
>  default-configs/avr-softmmu.mak  |5 +
>  qapi/machine.json|3 +-
>  hw/avr/atmel_atmega.h|   48 +
>  hw/avr/boot.h|   33 +
>  include/disas/dis-asm.h  |   19 +
>  include/elf.h|2 +
>  include/hw/char/atmel_usart.h|   93 +
>  include/hw/elf_ops.h |6 +-
>  include/hw/loader.h  |6 +-
>  include/hw/misc/atmel_power.h|  

Re: [PATCH v3 14/21] multifd: Use qemu_target_page_size()

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> We will make it cpu independent.
> 
> Signed-off-by: Juan Quintela 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/ram.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 2783dc60f4..14b7cbdbc9 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -882,14 +882,14 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams 
> *p, Error **errp)
>  for (i = 0; i < p->pages->used; i++) {
>  uint64_t offset = be64_to_cpu(packet->offset[i]);
>  
> -if (offset > (block->used_length - TARGET_PAGE_SIZE)) {
> +if (offset > (block->used_length - qemu_target_page_size())) {
>  error_setg(errp, "multifd: offset too long %" PRIu64
> " (max " RAM_ADDR_FMT ")",
> offset, block->max_length);
>  return -1;
>  }
>  p->pages->iov[i].iov_base = block->host + offset;
> -p->pages->iov[i].iov_len = TARGET_PAGE_SIZE;
> +p->pages->iov[i].iov_len = qemu_target_page_size();
>  }
>  
>  return 0;
> @@ -964,7 +964,8 @@ static int multifd_send_pages(QEMUFile *f)
>  p->packet_num = multifd_send_state->packet_num++;
>  multifd_send_state->pages = p->pages;
>  p->pages = pages;
> -transferred = ((uint64_t) pages->used) * TARGET_PAGE_SIZE + 
> p->packet_len;
> +transferred = ((uint64_t) pages->used) * qemu_target_page_size()
> ++ p->packet_len;
>  qemu_file_update_transfer(f, transferred);
>  ram_counters.multifd_bytes += transferred;
>  ram_counters.transferred += transferred;;
> @@ -985,7 +986,7 @@ static int multifd_queue_page(QEMUFile *f, RAMBlock 
> *block, ram_addr_t offset)
>  if (pages->block == block) {
>  pages->offset[pages->used] = offset;
>  pages->iov[pages->used].iov_base = block->host + offset;
> -pages->iov[pages->used].iov_len = TARGET_PAGE_SIZE;
> +pages->iov[pages->used].iov_len = qemu_target_page_size();
>  pages->used++;
>  
>  if (pages->used < pages->allocated) {
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH rc2 14/25] target/avr: Add section about AVR into QEMU documentation

[Michael Rolnik]

Hi Thomas.

I will fix it. thanks.

Michael Rolnik

On Fri, Jan 24, 2020 at 9:14 AM Thomas Huth  wrote:

> On 24/01/2020 01.51, Philippe Mathieu-Daudé wrote:
> > From: Michael Rolnik 
> >
> > Signed-off-by: Michael Rolnik 
> > Message-Id: <20200118191416.19934-16-mrol...@gmail.com>
> > Signed-off-by: Richard Henderson 
> > [PMD: Fixed typos]
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> > rc2: Fixed typos, s/sample/Arduino/, removed -serial section (thuth)
> > ---
> >  qemu-doc.texi | 51 +++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/qemu-doc.texi b/qemu-doc.texi
> > index 39f950471f..89df1d325e 100644
> > --- a/qemu-doc.texi
> > +++ b/qemu-doc.texi
> > @@ -1741,6 +1741,7 @@ differences are mentioned in the following
> sections.
> >  * Microblaze System emulator::
> >  * SH4 System emulator::
> >  * Xtensa System emulator::
> > +* AVR System emulator::
> >  @end menu
> >
> >  @node PowerPC System emulator
> > @@ -2514,6 +2515,56 @@ so should only be used with trusted guest OS.
> >
> >  @c man end
> >
> > +@node AVR System emulator
> > +@section AVR System emulator
> > +@cindex system emulation (AVR)
> > +
> > +Use the executable @file{qemu-system-avr} to emulates a AVR 8 bit based
> machine
> > +having one for the following cores: avr1, avr2, avr25, avr3, avr31,
> avr35, avr4,
> > +avr5, avr51, avr6, avrtiny, xmega2, xmega3, xmega4, xmega5, xmega6 and
> xmega7.
> > +
> > +As for now it supports few Arduino boards for educational and testing
> purposes.
> > +These boards use a ATmega controller, which model is limited to USART &
> 16 bit
> > +timer devices, enought to run FreeRTOS based applications (like this
> @url{
> https://github.com/seharris/qemu-avr-tests/blob/master/free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf,,demo
> })
> > +
> > +Following are examples of possible usages, assuming program.elf is
> compiled for
> > +AVR cpu
> > +@itemize
> > +
> > +@item Continuous non interrupted execution
> > +@example
> > +qemu-system-avr -kernel program.elf
> > +@end example
> > +
> > +@item Continuous non interrupted execution with serial output into
> telnet window
> > +@example
> > +qemu-system-avr -kernel program.elf -serial tcp::5678,server,nowait
> -nographic
> > +@end example
> > +and then in another shell
> > +@example
> > +telnet localhost 5678
> > +@end example
> > +
> > +@item Debugging wit GDB debugger
> > +@example
> > +qemu-system-avr -kernel program.elf -s -S
> > +@end example
> > +and then in another shell
> > +@example
> > +avr-gdb program.elf
> > +@end example
> > +and then within GDB shell
> > +@example
> > +target remote :1234
> > +@end example
> > +
> > +@item Print out executed instructions
> > +@example
> > +qemu-system-avr -kernel program.elf -d in_asm
>
> If you don't have a default board anymore, I think you need to list -M
> here, too.
> And didn't you mention that -kernel is not working anyway? Do you need
> to replace it with -bios ?
>
>  Thomas
>
>

-- 
Best Regards,
Michael Rolnik

Re: [PATCH v3 15/21] migration: Make checkpatch happy with comments

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> Signed-off-by: Juan Quintela 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/ram.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 14b7cbdbc9..c24b4cc771 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1312,10 +1312,12 @@ static void multifd_recv_terminate_threads(Error *err)
>  
>  qemu_mutex_lock(&p->mutex);
>  p->quit = true;
> -/* We could arrive here for two reasons:
> -   - normal quit, i.e. everything went fine, just finished
> -   - error quit: We close the channels so the channel threads
> - finish the qio_channel_read_all_eof() */
> +/*
> + * We could arrive here for two reasons:
> + *  - normal quit, i.e. everything went fine, just finished
> + *  - error quit: We close the channels so the channel threads
> + *finish the qio_channel_read_all_eof()
> + */
>  if (p->c) {
>  qio_channel_shutdown(p->c, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
>  }
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH] s390x: sigp: Fix sense running reporting

[David Hildenbrand]

On 24.01.20 11:05, Cornelia Huck wrote:
> On Fri, 24 Jan 2020 05:01:37 -0500
> Janosch Frank  wrote:
> 
>> The logic was inversed and reported running if the cpu was stopped.
> 
> s/inversed/inverted/ ?
> 
>> Let's fix that.
>>
> 
> Fixes: d1b468bc8869 ("s390x/tcg: implement SIGP SENSE RUNNING STATUS")
> 
>> Signed-off-by: Janosch Frank 
>> ---
>>  target/s390x/sigp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
>> index 727875bb4a..286c0d6c9c 100644
>> --- a/target/s390x/sigp.c
>> +++ b/target/s390x/sigp.c
>> @@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, 
>> SigpInfo *si)
>>  }
>>  
>>  /* If halted (which includes also STOPPED), it is not running */
>> -if (CPU(dst_cpu)->halted) {
>> +if (!CPU(dst_cpu)->halted) {
>>  si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>>  } else {
>>  set_sigp_status(si, SIGP_STAT_NOT_RUNNING);
> 
> I'm wondering why nobody noticed this before...

AFAIR, it "SENSE RUNNING" allows you to test if the target CPU is
scheduled by the hypervisor. So it is used for performance optimization,
but correctness of a program barely depends on it.

a) You can always return "not running" and it would be totally fine

b) Return "running" would also most probably always valid (although it
does not make too much sense for STOPPED CPUs).

E.g., in KVM we set CPUSTAT_RUNNING whenever we load the CPU. This can
also happen (AFAIR) when the CPU state is already stopped (e.g.,
currently getting stopped) or still sleeping (e.g., about to wake up, or
in kvm_vcpu_block()).


Long story short: There is no trusting on these values.


But yeah, the heuristic we are using is sub-optimal. :)

Reviewed-by: David Hildenbrand 

-- 
Thanks,

David / dhildenb

Re: [PULL v2 00/11] target/hppa patch queue

[Peter Maydell]

On Thu, 23 Jan 2020 at 22:10, Richard Henderson
 wrote:
>
> Change since v1:
>   * Incorporate Phil's -vga none fix for boot-serial-test (patch 7).
>
>
> r~
>
>
> The following changes since commit 6918ab2570bcf942651e69f7ad975e137679738b:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20200123-4' into staging (2020-01-23 
> 16:36:55 +)
>
> are available in the Git repository at:
>
>   https://github.com/rth7680/qemu.git tags/pull-pa-20200123
>
> for you to fetch changes up to b670f6d717a6a1795358c07823b4e968c0b61a86:
>
>   target/hppa: Allow, but diagnose, LDCW aligned only mod 4 (2020-01-23 
> 10:55:20 -1000)
>
> 
> Improve LASI emulation
> Add Artist graphics
> Fix main memory allocation
> Improve LDCW emulation wrt real hw
>

Hi; I'm afraid this has format-string errors for 32-bit and clang:

OSX:

/Users/pm215/src/qemu-for-merges/hw/display/artist.c:1035:41: error:
format specifies type 'unsigned long' but the argument has type
'hwaddr' (aka 'unsigned long long') [-Werror,-Wformat]
" size=%d\n", __func__, addr, val, size);
^~~~
/Users/pm215/src/qemu-for-merges/include/qemu/log.h:118:30: note:
expanded from macro 'qemu_log_mask'
qemu_log(FMT, ## __VA_ARGS__);  \
 ^~~
/Users/pm215/src/qemu-for-merges/hw/display/artist.c:1035:47: error:
format specifies type 'unsigned long' but the argument has type
'uint64_t' (aka 'unsigned long long') [-Werror,-Wformat]
" size=%d\n", __func__, addr, val, size);
  ^~~
/Users/pm215/src/qemu-for-merges/include/qemu/log.h:118:30: note:
expanded from macro 'qemu_log_mask'
qemu_log(FMT, ## __VA_ARGS__);  \
 ^~~
/Users/pm215/src/qemu-for-merges/hw/display/artist.c:1121:69: error:
format specifies type 'unsigned long' but the argument has type
'hwaddr' (aka 'unsigned long long') [-Werror,-Wformat]
qemu_log("%s: unknown register: %08lx size %d\n", __func__, addr, size);
~   ^~~~
%08llx
3 errors generated.


aarch32 has those 3 and also:

/home/peter.maydell/qemu/hw/net/i82596.c: In function 'i82596_receive':
/home/peter.maydell/qemu/hw/net/i82596.c:531:45: error: format '%lu'
expects argument of type 'long unsigned int', but argument 2 has type
'size_t {aka unsigned int}' [-Werror=format=]
 printf("Received frame too small, %lu vs. %u bytes\n",
   ~~^
   %u


OpenBSD has another 2:

/home/qemu/qemu-test.HtS7yu/src/hw/input/lasips2.c:178:62: warning:
format specifies type 'unsigned long' but the argument has type
'hwaddr' (aka 'unsigned long long') [-Wformat]
qemu_log("%s: unknown register 0x%02lx\n", __func__, addr);
 ~   ^~~~
 %02llx
/home/qemu/qemu-test.HtS7yu/src/hw/input/lasips2.c:239:62: warning:
format specifies type 'unsigned long' but the argument has type
'hwaddr' (aka 'unsigned long long') [-Wformat]
qemu_log("%s: unknown register 0x%02lx\n", __func__, addr);
 ~   ^~~~
 %02llx

thanks
-- PMM

Re: [PATCH rc2 12/25] hw/timer: Add limited support for Atmel 16 bit timer peripheral

[Sarah Harris]

Hi,

Do I understand correctly that you need Ed to email a "Signed-off-by: Ed 
Robbins " himself?
Ed's cc'ed already, but I'll email him directly to make sure he's seen this 
discussion.

Sarah

On Fri, 24 Jan 2020 11:51:13 +0100
Philippe Mathieu-Daudé  wrote:

> Hello Sarah,
> 
> On 1/24/20 11:42 AM, Alex Bennée wrote:
> > 
> > Philippe Mathieu-Daudé  writes:
> > 
> >> From: Michael Rolnik 
> >>
> >> These were designed to facilitate testing but should provide enough
> >> function to be useful in other contexts.  Only a subset of the functions
> >> of each peripheral is implemented, mainly due to the lack of a standard
> >> way to handle electrical connections (like GPIO pins).
> >>
> >> Signed-off-by: Sarah Harris 
> >> Message-Id: <20200118191416.19934-13-mrol...@gmail.com>
> >> Signed-off-by: Philippe Mathieu-Daudé 
> >> [rth: Squash info mtree fixes and a file rename from f4bug, which was:]
> >> Suggested-by: Aleksandar Markovic 
> >> Signed-off-by: Richard Henderson 
> >> [PMD: Use qemu_log_mask(LOG_UNIMP), replace goto by return]
> >> Signed-off-by: Philippe Mathieu-Daudé 
> >> ---
> > 
> >> --- /dev/null
> >> +++ b/include/hw/timer/atmel_timer16.h
> >> @@ -0,0 +1,94 @@
> >> +/*
> >> + * Atmel AVR 16 bit timer
> >> + *
> >> + * Copyright (c) 2018 University of Kent
> >> + * Author: Ed Robbins
> > 
> > No sign off from the author here?
> 
> Hmmm I Sarah Harris's one, who is from the University of Kent, isn't it 
> enough? (I remember patched from Xilinx with Edgar S-o-b but from other 
> authors, Edgar vouched for Xilinx).
> 
> Sarah, can you get Ed Signed-off-by?
> 
> >> --- /dev/null
> >> +++ b/hw/timer/atmel_timer16.c
> >> @@ -0,0 +1,605 @@
> > 
> >> +
> >> +/* Helper macros */
> >> +#define VAL16(l, h) ((h << 8) | l)
> >> +#define DB_PRINT(fmt, args...) /* Nothing */
> >> +/*#define DB_PRINT(fmt, args...) printf("%s: " fmt "\n", __func__, ##
> >> args)*/
> > 
> > Format strings are likely to bitrot. Either use a if (GATE) or
> > tracepoints.
> 
> Yes...
> 
> > 
> > 
> > Otherwise:
> > 
> > Reviewed-by: Alex Bennée 
> 
> Thanks!
> 

Re: [PATCH v3 17/21] multifd: Split multifd code into its own file

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> Signed-off-by: Juan Quintela 

Good, ram.c was getting WAY too big.


Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/Makefile.objs |   1 +
>  migration/migration.c   |   1 +
>  migration/multifd.c | 891 
>  migration/multifd.h | 139 ++
>  migration/ram.c | 980 +---
>  migration/ram.h |   7 -
>  6 files changed, 1033 insertions(+), 986 deletions(-)
>  create mode 100644 migration/multifd.c
>  create mode 100644 migration/multifd.h
> 
> diff --git a/migration/Makefile.objs b/migration/Makefile.objs
> index a4f3bafd86..d3623d5f9b 100644
> --- a/migration/Makefile.objs
> +++ b/migration/Makefile.objs
> @@ -7,6 +7,7 @@ common-obj-y += qemu-file-channel.o
>  common-obj-y += xbzrle.o postcopy-ram.o
>  common-obj-y += qjson.o
>  common-obj-y += block-dirty-bitmap.o
> +common-obj-y += multifd.o
>  
>  common-obj-$(CONFIG_RDMA) += rdma.o
>  
> diff --git a/migration/migration.c b/migration/migration.c
> index ecb56afd50..3501bc3353 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -53,6 +53,7 @@
>  #include "monitor/monitor.h"
>  #include "net/announce.h"
>  #include "qemu/queue.h"
> +#include "multifd.h"
>  
>  #define MAX_THROTTLE  (32 << 20)  /* Migration transfer speed throttling 
> */
>  
> diff --git a/migration/multifd.c b/migration/multifd.c
> new file mode 100644
> index 00..1875bb3aaa
> --- /dev/null
> +++ b/migration/multifd.c
> @@ -0,0 +1,891 @@
> +/*
> + * Multifd common code
> + *
> + * Copyright (c) 2019-2020 Red Hat Inc
> + *
> + * Authors:
> + *  Juan Quintela 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/rcu.h"
> +#include "exec/target_page.h"
> +#include "sysemu/sysemu.h"
> +#include "exec/ramblock.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "ram.h"
> +#include "migration.h"
> +#include "socket.h"
> +#include "qemu-file.h"
> +#include "trace.h"
> +#include "multifd.h"
> +
> +/* Multiple fd's */
> +
> +#define MULTIFD_MAGIC 0x11223344U
> +#define MULTIFD_VERSION 1
> +
> +typedef struct {
> +uint32_t magic;
> +uint32_t version;
> +unsigned char uuid[16]; /* QemuUUID */
> +uint8_t id;
> +uint8_t unused1[7]; /* Reserved for future use */
> +uint64_t unused2[4];/* Reserved for future use */
> +} __attribute__((packed)) MultiFDInit_t;
> +
> +static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
> +{
> +MultiFDInit_t msg = {};
> +int ret;
> +
> +msg.magic = cpu_to_be32(MULTIFD_MAGIC);
> +msg.version = cpu_to_be32(MULTIFD_VERSION);
> +msg.id = p->id;
> +memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid));
> +
> +ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp);
> +if (ret != 0) {
> +return -1;
> +}
> +return 0;
> +}
> +
> +static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
> +{
> +MultiFDInit_t msg;
> +int ret;
> +
> +ret = qio_channel_read_all(c, (char *)&msg, sizeof(msg), errp);
> +if (ret != 0) {
> +return -1;
> +}
> +
> +msg.magic = be32_to_cpu(msg.magic);
> +msg.version = be32_to_cpu(msg.version);
> +
> +if (msg.magic != MULTIFD_MAGIC) {
> +error_setg(errp, "multifd: received packet magic %x "
> +   "expected %x", msg.magic, MULTIFD_MAGIC);
> +return -1;
> +}
> +
> +if (msg.version != MULTIFD_VERSION) {
> +error_setg(errp, "multifd: received packet version %d "
> +   "expected %d", msg.version, MULTIFD_VERSION);
> +return -1;
> +}
> +
> +if (memcmp(msg.uuid, &qemu_uuid, sizeof(qemu_uuid))) {
> +char *uuid = qemu_uuid_unparse_strdup(&qemu_uuid);
> +char *msg_uuid = qemu_uuid_unparse_strdup((const QemuUUID 
> *)msg.uuid);
> +
> +error_setg(errp, "multifd: received uuid '%s' and expected "
> +   "uuid '%s' for channel %hhd", msg_uuid, uuid, msg.id);
> +g_free(uuid);
> +g_free(msg_uuid);
> +return -1;
> +}
> +
> +if (msg.id > migrate_multifd_channels()) {
> +error_setg(errp, "multifd: received channel version %d "
> +   "expected %d", msg.version, MULTIFD_VERSION);
> +return -1;
> +}
> +
> +return msg.id;
> +}
> +
> +static MultiFDPages_t *multifd_pages_init(size_t size)
> +{
> +MultiFDPages_t *pages = g_new0(MultiFDPages_t, 1);
> +
> +pages->allocated = size;
> +pages->iov = g_new0(struct iovec, size);
> +pages->offset = g_new0(ram_addr_t, size);
> +
> +return pages;
> +}
> +
> +static void multifd_pages_clear(MultiFDPages_t *pages)
> +{
> +pages->used = 0;
> +pages->allocated = 0;
> +pages->packet_num = 0;
> +pages->block = NULL;
> +g

Re: [PULL] RISC-V Patches for the 5.0 Soft Freeze, Part 1

[Peter Maydell]

On Thu, 23 Jan 2020 at 18:43, Palmer Dabbelt  wrote:
> On Thu, 23 Jan 2020 06:38:07 PST (-0800), Peter Maydell wrote:
> > Hi. This pull request doesn't seem to be signed with the GPG
> > key that I have on record for you...
>
> When I moved to Google I got a Yubikey and made new subkeys for it.  If I
> understand correctly the new subkeys should be signed by my main key, but 
> maybe
> that didn't make it to your keyring?  I see
>
> $ gpg --list-keys pal...@dabbelt.com
> pub   rsa4096 2017-06-06 [SC] [expires: 2027-11-13]
>   00CE76D1834960DFCE886DF8EF4CA1502CCBAB41
> uid   [ultimate] Palmer Dabbelt 
> uid   [ultimate] Palmer Dabbelt 
> sub   rsa4096 2017-06-06 [E]
> sub   rsa4096 2019-11-26 [S] [expires: 2024-11-24]
> sub   rsa4096 2019-11-26 [A] [expires: 2024-11-24]
> sub   rsa4096 2019-11-26 [E] [expires: 2024-11-24]

Yeah, I have those. I think I must have fumbled something
when I retried the pullreq after doing a refresh of your
gpg key, because I just did a retry now and it's fine.
(I'm just running the pull through my tests now.)

thanks
-- PMM

Re: [PATCH 0/3] migration: add sztd compression

[Juan Quintela]

Denis Plotnikov  wrote:
> zstd date compression algorithm shows better performance on data compression.
> It might be useful to employ the algorithm in VM migration to reduce CPU 
> usage.
> A user will be able to choose between those algorithms, therefor compress-type
> migration parameter is added.
>
> Here are some results of performance comparison zstd vs gzip:

Please, could you comment on the series:

[PATCH v3 00/21] Multifd Migration Compression

That series integrated zstd/zlib compression on top of multifd,
advantages over "old" compression code are:
- We don't have to copy data back and forth
- The unit of compression is 512KB instead of 4kb
- We "conserve" the compression state between packets (this is specially
  interesting for zstd, that uses dictionaries)

> host: i7-4790 8xCPU @ 3.60GHz, 16G RAM
> migration to the same host
> VM: 2xVCPU, 8G RAM total
> 5G RAM used, memory populated with postgreqsl data
> produced by pgbench performance benchmark
>
>
> Threads: 1 compress – 1 decompress
>
> zstd provides slightly less compression ratio with almost the same
> CPU usage but copes with RAM  compression roghly 2 times faster
>
> compression type  zlib   |  zstd
> -
> compression level  1   5 |   1   5
> compression ratio  6.927.05  |   6.696.89
> cpu idle, %82  83|   86  80
> time, sec  49  71|   26  31
> time diff to zlib, sec  -25 -41
>
>
> Threads: 8 compress – 2 decompress
>
> zstd provides the same migration time with less cpu consumption
>
> compression type none  |gzip(zlib)|  zstd
> --
> compression level- |  1  5   9|   1   5   15
> compression ratio- |  6.94   6.997.14 |   6.646.896.93
> time, sec154   |  22 23  27   |   23  23  25
> cpu idle, %  99|  45 30  12   |   70  52  23
> cpu idle diff to zlib  |  |  -25%-22%-11%

I don't have handy results, but it looked for me like you:
- zstd has a way better latency than zlib (i.e. the packet cames sooner)
- And it compress much better

On the migration test (best possible case for a compressor, as we are
writting just one byte of each page, and we write the same value in all
pages):

- zlib: compress 512KB -> 2500 bytes
- zstd: compess 512KB -> 52 bytes (yeap, I tested several times, it
  looked too small)

See that I posted another patch to "delete" the old compression code.
Why?
- I have been unable to modify migration-test to test it and work
  reliablely (only way was to allow a really huge downtime)
- Even with slow networking (1Gigabit) I got really mixed results,
  because as it is so slow, the guest continue dirtying memory, and in
  my tests it was never a winner

Thanks, Juan.

Re: [RFC 0/2] virtio-rng: add a control queue

[Amit Shah]

On Thu, 2020-01-23 at 16:16 +0100, Laurent Vivier wrote:
> The kernel needs sometime to be able to cancel an ongoing command.
> 
> For instance, if the virtio-rng device uses the egd backend
> and this backend doesn't provide data, the buffer provided by the
> kernel is kept as long as it is needed.
> 
> On the kernel side, a read blocks until the buffer returns from QEMU.
> 
> As the read is done with a mutex held, all the hw_random interface
> hangs and we cannot switch to another hw_random backend.
> 
> So this series adds a control queue to the virtio-rng device to allow
> to flush the virtio-rng input queue to release the kernel mutex and
> to allow to switch to another device.
> 
> The kernel side series can be found at:
> 
> https://github.com/vivier/linux/commits/virtio-rng-ctrl

Did you submit the kernel series too?  Can you please CC me to it?

This will need spec changes as well, can you please point me to them
too?

I also recall a previous discussion about this, but my search-fu is
failing to find it...

Amit

Re: [PATCH] migration/multifd: fix nullptr access in multifd_send_terminate_threads

[Juan Quintela]

Zhimin Feng  wrote:
> If the multifd_send_threads is not created when migration is failed,
> multifd_save_cleanup would be called twice. In this senario, the
> multifd_send_state is accessed after it has been released, the result
> is that the source VM is crashing down.
>
> Here is the coredump stack:
> Program received signal SIGSEGV, Segmentation fault.
> 0x5629333a78ef in multifd_send_terminate_threads (err=err@entry=0x0) 
> at migration/ram.c:1012
> 1012MultiFDSendParams *p = &multifd_send_state->params[i];
> #0  0x5629333a78ef in multifd_send_terminate_threads 
> (err=err@entry=0x0) at migration/ram.c:1012
> #1  0x5629333ab8a9 in multifd_save_cleanup () at migration/ram.c:1028
> #2  0x5629333abaea in multifd_new_send_channel_async 
> (task=0x562935450e70, opaque=) at migration/ram.c:1202
> #3  0x56293373a562 in qio_task_complete 
> (task=task@entry=0x562935450e70) at io/task.c:196
> #4  0x56293373a6e0 in qio_task_thread_result (opaque=0x562935450e70) 
> at io/task.c:111
> #5  0x7f475d4d75a7 in g_idle_dispatch () from 
> /usr/lib64/libglib-2.0.so.0
> #6  0x7f475d4da9a9 in g_main_context_dispatch () from 
> /usr/lib64/libglib-2.0.so.0
> #7  0x562933785b33 in glib_pollfds_poll () at util/main-loop.c:219
> #8  os_host_main_loop_wait (timeout=) at 
> util/main-loop.c:242
> #9  main_loop_wait (nonblocking=nonblocking@entry=0) at 
> util/main-loop.c:518
> #10 0x5629334c5acf in main_loop () at vl.c:1810
> #11 0x56293334d7bb in main (argc=, argv= out>, envp=) at vl.c:4471
>
> If the multifd_send_threads is not created when migration is failed.
> In this senario, we don't call multifd_save_cleanup in 
> multifd_new_send_channel_async.
>
> Signed-off-by: Zhimin Feng 

Reviewed-by: Juan Quintela 

Re: [PATCH v3 18/21] migration: Make no compression operations into its own structure

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> It will be used later.
> 
> Signed-off-by: Juan Quintela 
> 
> ---
> Move setup of ->ops helper to proper place (wei)
> Rename s/none/nocomp/ (dave)
> Introduce MULTIFD_FLAG_NOCOMP
> right order of arguments for print
> ---
>  migration/migration.c |   9 +++
>  migration/migration.h |   1 +
>  migration/multifd.c   | 183 --
>  migration/multifd.h   |  22 +
>  migration/ram.c   |   1 +
>  5 files changed, 208 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 3501bc3353..d25fdb3e6b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2245,6 +2245,15 @@ int migrate_multifd_channels(void)
>  return s->parameters.multifd_channels;
>  }
>  
> +int migrate_multifd_method(void)
> +{
> +MigrationState *s;
> +
> +s = migrate_get_current();
> +
> +return s->parameters.multifd_compress;
> +}

Shouldn't that be a MultifdCompress enum returned?

> +
>  int migrate_use_xbzrle(void)
>  {
>  MigrationState *s;
> diff --git a/migration/migration.h b/migration/migration.h
> index 8473ddfc88..3b0b413a93 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -300,6 +300,7 @@ bool migrate_auto_converge(void);
>  bool migrate_use_multifd(void);
>  bool migrate_pause_before_switchover(void);
>  int migrate_multifd_channels(void);
> +int migrate_multifd_method(void);
>  
>  int migrate_use_xbzrle(void);
>  int64_t migrate_xbzrle_cache_size(void);
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 1875bb3aaa..353140cd25 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -38,6 +38,132 @@ typedef struct {
>  uint64_t unused2[4];/* Reserved for future use */
>  } __attribute__((packed)) MultiFDInit_t;
>  
> +/* Multifd without compression */
> +
> +/**
> + * nocomp_send_setup: setup send side
> + *
> + * For no compression this function does nothing.
> + *
> + * Returns 0 for success or -1 for error
> + *
> + * @p: Params for the channel that we are using
> + * @errp: pointer to an error
> + */
> +static int nocomp_send_setup(MultiFDSendParams *p, Error **errp)
> +{
> +return 0;
> +}
> +
> +/**
> + * nocomp_send_cleanup: cleanup send side
> + *
> + * For no compression this function does nothing.
> + *
> + * @p: Params for the channel that we are using
> + */
> +static void nocomp_send_cleanup(MultiFDSendParams *p, Error **errp)
> +{
> +return;
> +}
> +
> +/**
> + * nocomp_send_prepare: prepare date to be able to send
> + *
> + * For no compression we just have to calculate the size of the
> + * packet.
> + *
> + * Returns 0 for success or -1 for error
> + *
> + * @p: Params for the channel that we are using
> + * @used: number of pages used
> + * @errp: pointer to an error
> + */
> +static int nocomp_send_prepare(MultiFDSendParams *p, uint32_t used,
> +   Error **errp)
> +{
> +p->next_packet_size = used * qemu_target_page_size();
> +p->flags |= MULTIFD_FLAG_NOCOMP;
> +return 0;
> +}
> +
> +/**
> + * nocomp_send_write: do the actual write of the data
> + *
> + * For no compression we just have to write the data.
> + *
> + * Returns 0 for success or -1 for error
> + *
> + * @p: Params for the channel that we are using
> + * @used: number of pages used
> + * @errp: pointer to an error
> + */
> +static int nocomp_send_write(MultiFDSendParams *p, uint32_t used, Error 
> **errp)
> +{
> +return qio_channel_writev_all(p->c, p->pages->iov, used, errp);
> +}
> +
> +/**
> + * nocomp_recv_setup: setup receive side
> + *
> + * For no compression this function does nothing.
> + *
> + * Returns 0 for success or -1 for error
> + *
> + * @p: Params for the channel that we are using
> + * @errp: pointer to an error
> + */
> +static int nocomp_recv_setup(MultiFDRecvParams *p, Error **errp)
> +{
> +return 0;
> +}
> +
> +/**
> + * nocomp_recv_cleanup: setup receive side
> + *
> + * For no compression this function does nothing.
> + *
> + * @p: Params for the channel that we are using
> + */
> +static void nocomp_recv_cleanup(MultiFDRecvParams *p)
> +{
> +}
> +
> +/**
> + * nocomp_recv_pages: read the data from the channel into actual pages
> + *
> + * For no compression we just need to read things into the correct place.
> + *
> + * Returns 0 for success or -1 for error
> + *
> + * @p: Params for the channel that we are using
> + * @used: number of pages used
> + * @errp: pointer to an error
> + */
> +static int nocomp_recv_pages(MultiFDRecvParams *p, uint32_t used, Error 
> **errp)
> +{
> +if (p->flags != MULTIFD_FLAG_NOCOMP) {
> +error_setg(errp, "multifd %d: flags received %x flags expected %x",
> +   p->id, p->flags, MULTIFD_FLAG_NOCOMP);
> +return -1;
> +}
> +return qio_channel_readv_all(p->c, p->pages->iov, used, errp);
> +}
> +
> +static MultiFDMethods multifd_nocomp_ops = {
> +.send_setup = nocomp_send_set

Re: [PULL 2/4] q800: add a block backend to the PRAM

[Peter Maydell]

On Tue, 7 Jan 2020 at 14:40, Laurent Vivier  wrote:
>
> This allows to save and restore the content of the PRAM.
> It may be useful if we want to check the configuration or to change it.
>
> The backend is added using mtd interface, for instance:
>
> ... -drive file=pram.img,format=raw,if=mtd ...
>
> where pram.img is the file where the data will be stored, its size must
> be 256 bytes.
>
> Signed-off-by: Laurent Vivier 
> Message-Id: <20191219201439.84804-3-laur...@vivier.eu>
> +static void pram_update(MacVIAState *m)
> +{
> +if (m->blk) {
> +blk_pwrite(m->blk, 0, m->mos6522_via1.PRAM,
> +   sizeof(m->mos6522_via1.PRAM), 0);
> +}
> +}

Hi -- Coverity warns (CID 1412799) that this isn't checking
the return value from blk_pwrite().

thanks
-- PMM

Re: [PATCH rc2 00/25] target/avr merger

[Philippe Mathieu-Daudé]

On 1/24/20 12:41 PM, Michael Rolnik wrote:

Tested-by: Michael Rolnik mailto:mrol...@gmail.com>>


Thanks a lot!

The only thing I want to change is instead of -kernel put -bios in 
qemu-doc.texi file. Should I send a new series?


Please do NOT :)

Richard can do the trivial fixup directly.

On Fri, Jan 24, 2020 at 2:51 AM Philippe Mathieu-Daudé > wrote:


This is the AVR port from Michael release (merge) candidate 2.

Since v1 [1]:
- Addressed Thomas comments
- Fixed a non-critical bug in ATmega (incorrect SRAM base address)
- Added ELF parsing requested by Aleksandar
- Dropped default machine (as with the ARM port)

Change since rc1:

$ git backport-diff -u avr-rc1 -r origin/master..
Key:
[] : patches are identical
[] : number of functional differences between
upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences,
respectively

001/25:[] [--] 'target/avr: Add outward facing interfaces and
core CPU logic'
002/25:[] [--] 'target/avr: Add instruction helpers'
003/25:[] [--] 'target/avr: Add instruction translation -
Registers definition'
004/25:[] [--] 'target/avr: Add instruction translation -
Arithmetic and Logic Instructions'
005/25:[] [--] 'target/avr: Add instruction translation - Branch
Instructions'
006/25:[] [--] 'target/avr: Add instruction translation - Data
Transfer Instructions'
007/25:[] [--] 'target/avr: Add instruction translation - Bit
and Bit-test Instructions'
008/25:[] [--] 'target/avr: Add instruction translation - MCU
Control Instructions'
009/25:[] [--] 'target/avr: Add instruction translation - CPU
main translation function'
010/25:[] [--] 'target/avr: Add instruction disassembly function'
011/25:[] [--] 'hw/char: Add limited support for Atmel USART
peripheral'
012/25:[0045] [FC] 'hw/timer: Add limited support for Atmel 16 bit
timer peripheral'
013/25:[] [--] 'hw/misc: Add Atmel power device'
014/25:[0024] [FC] 'target/avr: Add section about AVR into QEMU
documentation'
015/25:[] [--] 'target/avr: Register AVR support with the rest
of QEMU'
016/25:[] [--] 'target/avr: Add machine none test'
017/25:[0002] [FC] 'target/avr: Update MAINTAINERS file'
018/25:[down]      'hw/core/loader: Let load_elf populate the
processor-specific flags'
019/25:[down]      'hw/avr: Add helper to load raw/ELF firmware
binaries'
020/25:[0015] [FC] 'hw/avr: Add some ATmega microcontrollers'
021/25:[0040] [FC] 'hw/avr: Add some Arduino boards'
022/25:[] [--] 'target/avr: Update build system'
023/25:[] [--] 'tests/boot-serial-test: Test some Arduino boards
(AVR based)'
024/25:[] [--] 'tests/acceptance: Test the Arduino MEGA2560 board'
025/25:[] [--] '.travis.yml: Run the AVR acceptance tests'

Repo: https://gitlab.com/philmd/qemu/commits/avr-rc2

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg672926.html
Supersedes: <20200123000307.11541-1-richard.hender...@linaro.org
>

Michael Rolnik (20):
   target/avr: Add outward facing interfaces and core CPU logic
   target/avr: Add instruction helpers
   target/avr: Add instruction translation - Registers definition
   target/avr: Add instruction translation - Arithmetic and Logic
     Instructions
   target/avr: Add instruction translation - Branch Instructions
   target/avr: Add instruction translation - Data Transfer Instructions
   target/avr: Add instruction translation - Bit and Bit-test
     Instructions
   target/avr: Add instruction translation - MCU Control Instructions
   target/avr: Add instruction translation - CPU main translation
     function
   target/avr: Add instruction disassembly function
   hw/char: Add limited support for Atmel USART peripheral
   hw/timer: Add limited support for Atmel 16 bit timer peripheral
   hw/misc: Add Atmel power device
   target/avr: Add section about AVR into QEMU documentation
   target/avr: Register AVR support with the rest of QEMU
   target/avr: Add machine none test
   target/avr: Update MAINTAINERS file
   target/avr: Update build system
   tests/boot-serial-test: Test some Arduino boards (AVR based)
   tests/acceptance: Test the Arduino MEGA2560 board

Philippe Mathieu-Daudé (5):
   hw/core/loader: Let load_elf populate the processor-specific flags
   hw/avr: Add helper to load raw/ELF firmware binaries
   hw/avr: Add some ATmega microcontrollers
   hw/avr: Add some Arduino boards
   .travis.yml: Run the AVR acceptance tests

  qemu-doc.texi                    |   51 +
  configure                        |    7 +

Re: [PATCH rc2 12/25] hw/timer: Add limited support for Atmel 16 bit timer peripheral

[Philippe Mathieu-Daudé]

On 1/24/20 9:16 AM, Thomas Huth wrote:

On 24/01/2020 01.51, Philippe Mathieu-Daudé wrote:

From: Michael Rolnik 

These were designed to facilitate testing but should provide enough
function to be useful in other contexts.  Only a subset of the functions
of each peripheral is implemented, mainly due to the lack of a standard
way to handle electrical connections (like GPIO pins).

Signed-off-by: Sarah Harris 
Message-Id: <20200118191416.19934-13-mrol...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
[rth: Squash info mtree fixes and a file rename from f4bug, which was:]
Suggested-by: Aleksandar Markovic 
Signed-off-by: Richard Henderson 
[PMD: Use qemu_log_mask(LOG_UNIMP), replace goto by return]
Signed-off-by: Philippe Mathieu-Daudé 
---
rc2: Use qemu_log_mask(LOG_UNIMP), replace goto by return (thuth)
---
  include/hw/timer/atmel_timer16.h |  94 +
  hw/timer/atmel_timer16.c | 605 +++
  hw/timer/Kconfig |   3 +
  hw/timer/Makefile.objs   |   2 +
  4 files changed, 704 insertions(+)
  create mode 100644 include/hw/timer/atmel_timer16.h
  create mode 100644 hw/timer/atmel_timer16.c

[...]

+static void avr_timer16_clksrc_update(AVRTimer16State *t16)
+{
+uint16_t divider = 0;
+switch (CLKSRC(t16)) {
+case T16_CLKSRC_EXT_FALLING:
+case T16_CLKSRC_EXT_RISING:
+qemu_log_mask(LOG_UNIMP, "%s: external clock source unsupported\n",
+  __func__);
+break;
+case T16_CLKSRC_STOPPED:
+break;
+case T16_CLKSRC_DIV1:
+divider = 1;
+break;
+case T16_CLKSRC_DIV8:
+divider = 8;
+break;
+case T16_CLKSRC_DIV64:
+divider = 64;
+break;
+case T16_CLKSRC_DIV256:
+divider = 256;
+break;
+case T16_CLKSRC_DIV1024:
+divider = 1024;
+break;
+default:
+break;
+}
+if (divider) {
+t16->freq_hz = t16->cpu_freq_hz / divider;
+t16->period_ns = NANOSECONDS_PER_SECOND / t16->freq_hz;
+DB_PRINT("Timer frequency %" PRIu64 " hz, period %" PRIu64 " ns (%f 
s)",
+ t16->freq_hz, t16->period_ns, 1 / (double)t16->freq_hz);
+}
+return;
+}


You can remove the "return;" at the end of the function now, too.


Oops I didn't notice.

Richard, do you mind to remove this single line?

Thanks both :)

Re: [PATCH rc2 12/25] hw/timer: Add limited support for Atmel 16 bit timer peripheral

[Philippe Mathieu-Daudé]

On 1/24/20 11:42 AM, Alex Bennée wrote:

Philippe Mathieu-Daudé  writes:


From: Michael Rolnik 

These were designed to facilitate testing but should provide enough
function to be useful in other contexts.  Only a subset of the functions
of each peripheral is implemented, mainly due to the lack of a standard
way to handle electrical connections (like GPIO pins).

Signed-off-by: Sarah Harris 
Message-Id: <20200118191416.19934-13-mrol...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
[rth: Squash info mtree fixes and a file rename from f4bug, which was:]
Suggested-by: Aleksandar Markovic 
Signed-off-by: Richard Henderson 
[PMD: Use qemu_log_mask(LOG_UNIMP), replace goto by return]
Signed-off-by: Philippe Mathieu-Daudé 
---



--- /dev/null
+++ b/hw/timer/atmel_timer16.c
@@ -0,0 +1,605 @@



+
+/* Helper macros */
+#define VAL16(l, h) ((h << 8) | l)
+#define DB_PRINT(fmt, args...) /* Nothing */
+/*#define DB_PRINT(fmt, args...) printf("%s: " fmt "\n", __func__, ##
args)*/


Format strings are likely to bitrot. Either use a if (GATE) or
tracepoints.


To avoid respining, I'll do in a new a patch on top of this series,
and Richard can choose to squash it directly.




Otherwise:

Reviewed-by: Alex Bennée 

Re: [PATCH v3 07/21] migration: Make multifd_save_setup() get an Error parameter

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> Signed-off-by: Juan Quintela 
> 

Reviewed-by: Dr. David Alan Gilbert 

> ---
> 
> We can't trust that error_in is not NULL.  Create a local_error.
> ---
>  migration/migration.c | 4 +++-
>  migration/ram.c   | 2 +-
>  migration/ram.h   | 2 +-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 1fb0aab44d..7140d1e040 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3367,6 +3367,7 @@ static void *migration_thread(void *opaque)
>  
>  void migrate_fd_connect(MigrationState *s, Error *error_in)
>  {
> +Error *local_err = NULL;
>  int64_t rate_limit;
>  bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>  
> @@ -3415,7 +3416,8 @@ void migrate_fd_connect(MigrationState *s, Error 
> *error_in)
>  return;
>  }
>  
> -if (multifd_save_setup() != 0) {
> +if (multifd_save_setup(&local_err) != 0) {
> +error_report_err(local_err);
>  migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>MIGRATION_STATUS_FAILED);
>  migrate_fd_cleanup(s);
> diff --git a/migration/ram.c b/migration/ram.c
> index 3fd7fdffcf..d537264ba5 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1243,7 +1243,7 @@ static void multifd_new_send_channel_async(QIOTask 
> *task, gpointer opaque)
>  }
>  }
>  
> -int multifd_save_setup(void)
> +int multifd_save_setup(Error **errp)
>  {
>  int thread_count;
>  uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size();
> diff --git a/migration/ram.h b/migration/ram.h
> index bd0eee79b6..da22a417ea 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -41,7 +41,7 @@ int xbzrle_cache_resize(int64_t new_size, Error **errp);
>  uint64_t ram_bytes_remaining(void);
>  uint64_t ram_bytes_total(void);
>  
> -int multifd_save_setup(void);
> +int multifd_save_setup(Error **errp);
>  void multifd_save_cleanup(void);
>  int multifd_load_setup(void);
>  int multifd_load_cleanup(Error **errp);
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PULL v2 11/14] semihosting: add qemu_semihosting_console_inc for SYS_READC

[Peter Maydell]

On Thu, 9 Jan 2020 at 14:19, Alex Bennée  wrote:
>
> From: Keith Packard 
>
> Provides a blocking call to read a character from the console using
> semihosting.chardev, if specified. This takes some careful command
> line options to use stdio successfully as the serial ports, monitor
> and semihost all want to use stdio. Here's a sample set of command
> line options which share stdio between semihost, monitor and serial
> ports:

Hi; Coverity has some complaints about this code, and
specifically the use of getchar():

> +/*
> + * For linux-user we can safely block. However as we want to return as
> + * soon as a character is read we need to tweak the termio to disable
> + * line buffering. We restore the old mode afterwards in case the
> + * program is expecting more normal behaviour. This is slow but
> + * nothing using semihosting console reading is expecting to be fast.
> + */
> +target_ulong qemu_semihosting_console_inc(CPUArchState *env)
> +{
> +uint8_t c;
> +struct termios old_tio, new_tio;
> +
> +/* Disable line-buffering and echo */
> +tcgetattr(STDIN_FILENO, &old_tio);
> +new_tio = old_tio;
> +new_tio.c_lflag &= (~ICANON & ~ECHO);
> +tcsetattr(STDIN_FILENO, TCSANOW, &new_tio);
> +
> +c = getchar();

CID 1412794 points out that this assigns the result
of getchar() to a uint8_t, which drops the distinction
between EOF and a legitimate byte.
CID 1412795 is then kind of a run-on error from that,
complaining that the int result from getchar() is
truncated before returning it.

I'm not sure what we should do with EOF, but presumably
we should handle it in some way.

thanks
-- PMM

Re: [PATCH] s390x: sigp: Fix sense running reporting

[Janosch Frank]

On 1/24/20 1:03 PM, David Hildenbrand wrote:
> On 24.01.20 11:05, Cornelia Huck wrote:
>> On Fri, 24 Jan 2020 05:01:37 -0500
>> Janosch Frank  wrote:
>>
>>> The logic was inversed and reported running if the cpu was stopped.
>>
>> s/inversed/inverted/ ?
>>
>>> Let's fix that.
>>>
>>
>> Fixes: d1b468bc8869 ("s390x/tcg: implement SIGP SENSE RUNNING STATUS")
>>
>>> Signed-off-by: Janosch Frank 
>>> ---
>>>  target/s390x/sigp.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
>>> index 727875bb4a..286c0d6c9c 100644
>>> --- a/target/s390x/sigp.c
>>> +++ b/target/s390x/sigp.c
>>> @@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, 
>>> SigpInfo *si)
>>>  }
>>>  
>>>  /* If halted (which includes also STOPPED), it is not running */
>>> -if (CPU(dst_cpu)->halted) {
>>> +if (!CPU(dst_cpu)->halted) {
>>>  si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>>>  } else {
>>>  set_sigp_status(si, SIGP_STAT_NOT_RUNNING);
>>
>> I'm wondering why nobody noticed this before...
> 
> AFAIR, it "SENSE RUNNING" allows you to test if the target CPU is
> scheduled by the hypervisor. So it is used for performance optimization,
> but correctness of a program barely depends on it.
> 
> a) You can always return "not running" and it would be totally fine
> 
> b) Return "running" would also most probably always valid (although it
> does not make too much sense for STOPPED CPUs).
> 
> E.g., in KVM we set CPUSTAT_RUNNING whenever we load the CPU. This can
> also happen (AFAIR) when the CPU state is already stopped (e.g.,
> currently getting stopped) or still sleeping (e.g., about to wake up, or
> in kvm_vcpu_block()).
> 
> 
> Long story short: There is no trusting on these values.

That answer makes me highly uncomfortable...

> 
> 
> But yeah, the heuristic we are using is sub-optimal. :)
> 
> Reviewed-by: David Hildenbrand 

Thanks!




signature.asc
Description: OpenPGP digital signature

Re: [PATCH] s390x: sigp: Fix sense running reporting

[Janosch Frank]

On 1/24/20 11:05 AM, Cornelia Huck wrote:
> On Fri, 24 Jan 2020 05:01:37 -0500
> Janosch Frank  wrote:
> 
>> The logic was inversed and reported running if the cpu was stopped.
> 
> s/inversed/inverted/ ?

Ok

> 
>> Let's fix that.
>>
> 
> Fixes: d1b468bc8869 ("s390x/tcg: implement SIGP SENSE RUNNING STATUS")

Added

> 
>> Signed-off-by: Janosch Frank 
>> ---
>>  target/s390x/sigp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
>> index 727875bb4a..286c0d6c9c 100644
>> --- a/target/s390x/sigp.c
>> +++ b/target/s390x/sigp.c
>> @@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, 
>> SigpInfo *si)
>>  }
>>  
>>  /* If halted (which includes also STOPPED), it is not running */
>> -if (CPU(dst_cpu)->halted) {
>> +if (!CPU(dst_cpu)->halted) {
>>  si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>>  } else {
>>  set_sigp_status(si, SIGP_STAT_NOT_RUNNING);
> 
> I'm wondering why nobody noticed this before...

No idea.
How many people use smp with tcg?




signature.asc
Description: OpenPGP digital signature