Re: hw-display-qxl.so: undefined symbol: qemu_qxl_io_log_semaphore

[Gerd Hoffmann]

  Hi,

> FWIW I'm still hitting issues with qemu-5.1.0 GA but maybe it's
> unrelated to that specific fix. Issues reproduce on fedora 33+, not
> fedora 32.

> +Failed to open module:
> /builddir/build/BUILD/qemu-5.1.0-rc3/build-dynamic/x86_64-softmmu/../hw-display-qxl.so:
> undefined symbol: qemu_qxl_client_monitors_config_crc_semaphore

> /builddir/build/BUILD/qemu-5.1.0/build-dynamic/s390x-softmmu/../hw-usb-smartcard.so:
> undefined symbol: ccid_card_send_apdu_to_guest

> So maybe there's a more general problem. FWIW Fedora 33 started using
> LTO by default, but it was disabled for the qemu package.

Hmm, the first looks like a problem.  I'm wondering why it happens on
f33 only, not f32.  LTO could explain that (optimizing away symbols used
by modules but not main qemu), but with that already turned off I have
no clue offhand.

The second is sort-of expected, this comes from s390x not supporting
usb.  I have a pending patch to silence those warnings when qemu tries
to load all available modules (for introspection, to make sure the qom
object list is complete).

take care,
  Gerd

Re: [PATCH] memory: Initialize MemoryRegionOps for RAM memory regions

[Philippe Mathieu-Daudé]

+Prasad

On 8/16/20 8:26 PM, Philippe Mathieu-Daudé wrote:
> There is an issue when using memory_region_dispatch_read() or
> memory_region_dispatch_write() on RAM memory regions.
> 
> RAM memory regions are initialized as:
> 
>   memory_region_init_ram()
>   -> memory_region_init_ram_nomigrate()
>  -> memory_region_init_ram_shared_nomigrate()
> -> memory_region_init()
>-> object_initialize(TYPE_MEMORY_REGION)
>   -> memory_region_initfn()
>  -> mr->ops = _mem_ops;
> 
> Later when accessing the alias, the memory_region_dispatch_read()
> flow is:
> 
>   memory_region_dispatch_read()
>   -> memory_region_dispatch_read1()
>  -> if (mr->ops->read) { ... }
>^^
>NULL deref as unassigned_mem_ops.read is NULL.
> 
>   memory_region_dispatch_write()
>   -> if (mr->ops->write) { ... }
> ^^^
> NULL deref as unassigned_mem_ops.read is NULL.
> 
> Fix by initializing the MemoryRegionOps to ram_device_mem_ops,
> this way the memory accesses are properly dispatched using
> memory_region_ram_device_read() / memory_region_ram_device_write().
> 
> Fixes: 4a2e242bbb ("memory: Don't use memcpy for ram_device regions")
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  softmmu/memory.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index af25987518..2fce3fef2d 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1509,6 +1509,8 @@ void 
> memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>  Error *err = NULL;
>  memory_region_init(mr, owner, name, size);
>  mr->ram = true;
> +mr->ops = _device_mem_ops;
> +mr->opaque = mr;
>  mr->terminates = true;
>  mr->destructor = memory_region_destructor_ram;
>  mr->ram_block = qemu_ram_alloc(size, share, mr, );
> @@ -1533,6 +1535,8 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
>  Error *err = NULL;
>  memory_region_init(mr, owner, name, size);
>  mr->ram = true;
> +mr->ops = _device_mem_ops;
> +mr->opaque = mr;
>  mr->terminates = true;
>  mr->destructor = memory_region_destructor_ram;
>  mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized,
> @@ -1558,6 +1562,8 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>  Error *err = NULL;
>  memory_region_init(mr, owner, name, size);
>  mr->ram = true;
> +mr->ops = _device_mem_ops;
> +mr->opaque = mr;
>  mr->terminates = true;
>  mr->destructor = memory_region_destructor_ram;
>  mr->align = align;
> @@ -1581,6 +1587,8 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>  Error *err = NULL;
>  memory_region_init(mr, owner, name, size);
>  mr->ram = true;
> +mr->ops = _device_mem_ops;
> +mr->opaque = mr;
>  mr->terminates = true;
>  mr->destructor = memory_region_destructor_ram;
>  mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
> @@ -1603,6 +1611,8 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>  {
>  memory_region_init(mr, owner, name, size);
>  mr->ram = true;
> +mr->ops = _device_mem_ops;
> +mr->opaque = mr;
>  mr->terminates = true;
>  mr->destructor = memory_region_destructor_ram;
>  mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
> 

Re: [PATCH v4 0/9] memory: assert and define MemoryRegionOps callbacks

[Philippe Mathieu-Daudé]

On 8/17/20 7:02 AM, P J P wrote:
> +-- On Sun, 16 Aug 2020, Philippe Mathieu-Daudé wrote --+
> | On 8/11/20 1:41 PM, P J P wrote:
> | > From: Prasad J Pandit 
> | > * This series asserts that MemoryRegionOps objects define read/write
> | >   callback methods. Thus avoids potential NULL pointer dereference.
> | >   ex. -> 
> https://git.qemu.org/?p=qemu.git;a=commit;h=bb15013ef34617eb1344f5276292cadd326c21b2
> | > 
> | > * Also adds various undefined MemoryRegionOps read/write functions
> | >   to avoid potential assert failure.
> | 
> | What about read_with_attrs()/write_with_attrs()? It seems they are part of 
> | the same problem.
> 
> * read/write_with_attrs function is called if read/write callback is not 
>   defined
> 
>   ../softmmu/memory.c
> if (mr->ops->write) {
> ... memory_region_write_accessor, mr,
> } else {
> ... memory_region_write_with_attrs_accessor,
> 
>   So, defining read/write methods may also address read/write_with_attrs 
>   issue?
> 
> * $ grep -Eri -A 5 -B 5 '(\.read_with_attrs|\.write_with_attrs)' . | fpaste
> 
>-> https://paste.centos.org/view/386c9597
> 
>   It doesn't show an occurrence where one of the read/write_with_attrs is 
>   missing.
> 
> * Nevertheless, if we need to define read/write_with_attrs routines, because 
>   memory_region_init_io() would assert(3) for them
> 
>   could that be a subsequent patch series please?

Yes no problem, I was just wondering and wasn't sure.

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

[Philippe Mathieu-Daudé]

On 8/17/20 6:47 AM, David Gibson wrote:
> On Wed, Jul 22, 2020 at 11:56:49PM -0300, Thiago Jung Bauermann wrote:
>> The ARM code has a start-powered-off property in ARMCPU, which is a
>> subclass of CPUState. This property causes arm_cpu_reset() to set
>> CPUState::halted to 1, signalling that the CPU should start in a halted
>> state. Other architectures also have code which aim to achieve the same
>> effect, but without using a property.
>>
>> The ppc/spapr version has a bug where QEMU does a KVM_RUN on the vcpu
>> before cs->halted is set to 1, causing the vcpu to run while it's still in
>> an unitialized state (more details in patch 3).
>>
>> Peter Maydell mentioned the ARM start-powered-off property and
>> Eduardo Habkost suggested making it generic, so this patch series does
>> that, for all cases which I was able to find via grep in the code.
>>
>> The only problem is that I was only able to test these changes on a ppc64le
>> pseries KVM guest, so except for patches 2 and 3, all others are only
>> build-tested. Also, my grasp of QOM lifecycle is basically non-existant so
>> please be aware of that when reviewing this series.
>>
>> The last patch may be wrong, as pointed out by Eduardo, so I marked it as
>> RFC. It may make sense to drop it.
>>
>> Applies cleanly on yesterday's master.
> 
> This series appears to break the Travis build for a MIPS target:
> 
> Unexpected error in qdev_prop_set_after_realize() at 
> /home/travis/build/dgibson/qemu/hw/core/qdev-properties.c:30:
> qemu-system-mips64el: Attempt to set property 'start-powered-off' on 
> anonymous device (type 'I6400-mips64-cpu') after it was realized
> Broken pipe
> /home/travis/build/dgibson/qemu/tests/qtest/libqtest.c:175: kill_qemu() 
> detected QEMU death from signal 6 (Aborted) (core dumped)
> Aborted (core dumped)
> ERROR qom-test - too few tests run (expected 8, got 0)
> /home/travis/build/dgibson/qemu/tests/Makefile.include:650: recipe for target 
> 'check-qtest-mips64el' failed

Good catch. hw/mips/cps.c, hw/ppc/e500.c and hw/sparc/sun4m.c are
incorrectly setting the property after the cpu is realized because
the cpu is created with cpu_create(). We need to create them with
object_initialize_child() and realize them manually with qdev_realize().

Re: [PATCH v4 0/9] memory: assert and define MemoryRegionOps callbacks

[P J P]

+-- On Sun, 16 Aug 2020, Philippe Mathieu-Daudé wrote --+
| On 8/11/20 1:41 PM, P J P wrote:
| > From: Prasad J Pandit 
| > * This series asserts that MemoryRegionOps objects define read/write
| >   callback methods. Thus avoids potential NULL pointer dereference.
| >   ex. -> 
https://git.qemu.org/?p=qemu.git;a=commit;h=bb15013ef34617eb1344f5276292cadd326c21b2
| > 
| > * Also adds various undefined MemoryRegionOps read/write functions
| >   to avoid potential assert failure.
| 
| What about read_with_attrs()/write_with_attrs()? It seems they are part of 
| the same problem.

* read/write_with_attrs function is called if read/write callback is not 
  defined

  ../softmmu/memory.c
if (mr->ops->write) {
... memory_region_write_accessor, mr,
} else {
... memory_region_write_with_attrs_accessor,

  So, defining read/write methods may also address read/write_with_attrs 
  issue?

* $ grep -Eri -A 5 -B 5 '(\.read_with_attrs|\.write_with_attrs)' . | fpaste

   -> https://paste.centos.org/view/386c9597

  It doesn't show an occurrence where one of the read/write_with_attrs is 
  missing.

* Nevertheless, if we need to define read/write_with_attrs routines, because 
  memory_region_init_io() would assert(3) for them

  could that be a subsequent patch series please?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

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

[David Gibson]

On Wed, Jul 22, 2020 at 11:56:49PM -0300, Thiago Jung Bauermann wrote:
> The ARM code has a start-powered-off property in ARMCPU, which is a
> subclass of CPUState. This property causes arm_cpu_reset() to set
> CPUState::halted to 1, signalling that the CPU should start in a halted
> state. Other architectures also have code which aim to achieve the same
> effect, but without using a property.
> 
> The ppc/spapr version has a bug where QEMU does a KVM_RUN on the vcpu
> before cs->halted is set to 1, causing the vcpu to run while it's still in
> an unitialized state (more details in patch 3).
> 
> Peter Maydell mentioned the ARM start-powered-off property and
> Eduardo Habkost suggested making it generic, so this patch series does
> that, for all cases which I was able to find via grep in the code.
> 
> The only problem is that I was only able to test these changes on a ppc64le
> pseries KVM guest, so except for patches 2 and 3, all others are only
> build-tested. Also, my grasp of QOM lifecycle is basically non-existant so
> please be aware of that when reviewing this series.
> 
> The last patch may be wrong, as pointed out by Eduardo, so I marked it as
> RFC. It may make sense to drop it.
> 
> Applies cleanly on yesterday's master.

This series appears to break the Travis build for a MIPS target:

Unexpected error in qdev_prop_set_after_realize() at 
/home/travis/build/dgibson/qemu/hw/core/qdev-properties.c:30:
qemu-system-mips64el: Attempt to set property 'start-powered-off' on anonymous 
device (type 'I6400-mips64-cpu') after it was realized
Broken pipe
/home/travis/build/dgibson/qemu/tests/qtest/libqtest.c:175: kill_qemu() 
detected QEMU death from signal 6 (Aborted) (core dumped)
Aborted (core dumped)
ERROR qom-test - too few tests run (expected 8, got 0)
/home/travis/build/dgibson/qemu/tests/Makefile.include:650: recipe for target 
'check-qtest-mips64el' failed

> 
> Changes since v2:
> 
> General:
> - Added Philippe's Reviewed-by to some of the patches.
> 
> Patch "ppc/spapr: Use start-powered-off CPUState property"
> - Set the CPUState::start_powered_off variable directly rather than using
>   object_property_set_bool(). Suggested by Philippe.
> 
> Patch "sparc/sun4m: Remove main_cpu_reset()"
> - New patch. Suggested by Philippe.
> 
> Patch "sparc/sun4m: Use start-powered-off CPUState property"
> - Remove secondary_cpu_reset(). Suggested by Philippe.
> - Remove setting of `cs->halted = 1` from cpu_devinit(). Suggested by 
> Philippe.
> 
> Patch "Don't set CPUState::halted in cpu_devinit()"
> - Squashed into previous patch. Suggested by Philippe.
> 
> Patch "sparc/sun4m: Use one cpu_reset() function for main and secondary CPUs"
> - Dropped.
> 
> Patch "target/s390x: Use start-powered-off CPUState property"
> - Set the CPUState::start_powered_off variable directly rather than using
>   object_property_set_bool(). Suggested by Philippe.
> - Mention in the commit message Eduardo's observation that before this
>   patch, the code didn't set cs->halted on reset.
> 
> Thiago Jung Bauermann (8):
>   target/arm: Move start-powered-off property to generic CPUState
>   target/arm: Move setting of CPU halted state to generic code
>   ppc/spapr: Use start-powered-off CPUState property
>   ppc/e500: Use start-powered-off CPUState property
>   mips/cps: Use start-powered-off CPUState property
>   sparc/sun4m: Remove main_cpu_reset()
>   sparc/sun4m: Use start-powered-off CPUState property
>   target/s390x: Use start-powered-off CPUState property
> 
>  exec.c  |  1 +
>  hw/core/cpu.c   |  2 +-
>  hw/mips/cps.c   |  6 +++---
>  hw/ppc/e500.c   | 10 +++---
>  hw/ppc/spapr_cpu_core.c | 10 +-
>  hw/sparc/sun4m.c| 28 ++--
>  include/hw/core/cpu.h   |  4 
>  target/arm/cpu.c|  4 +---
>  target/arm/cpu.h|  3 ---
>  target/arm/kvm32.c  |  2 +-
>  target/arm/kvm64.c  |  2 +-
>  target/s390x/cpu.c  |  2 +-
>  12 files changed, 27 insertions(+), 47 deletions(-)
> 
> 

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

[Bug 1880507] Re: VMM from Ubuntu 20.04 does not show the memory consumption

[Launchpad Bug Tracker]

[Expired for QEMU because there has been no activity for 60 days.]

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

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

Title:
  VMM from Ubuntu 20.04 does not show the memory consumption

Status in QEMU:
  Expired

Bug description:
  KVM host system: Ubuntu 18.04 and 20.04, guest machines: Windows and
  Ubuntu. Management through Ubuntu 20.04, vmm does not show RAM
  consumption for Windows guest systems (Win7, Win2008R2), for Ubuntu
  values are shown. The error is not observed in Ubuntu 18.04/vmm.

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

Re: [PATCH] ide:do nothing for identify cmd if no any device attached

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200817033803.14014-1-rockcui...@zhaoxin.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TESTcheck-unit: tests/test-char
Unexpected error in object_property_try_add() at 
/tmp/qemu-test/src/qom/object.c:1181:
attempt to add duplicate property 'serial-id' to object (type 'container')
ERROR test-char - too few tests run (expected 38, got 9)
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs
  TESTcheck-qtest-x86_64: tests/qtest/hd-geo-test
  TESTcheck-qtest-x86_64: tests/qtest/boot-order-test
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=8c36fb1727574c069b2f14432c2bddf7', '-u', 
'1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-0cmc1v90/src/docker-src.2020-08-17-00.03.16.18596:/var/tmp/qemu:z,ro',
 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=8c36fb1727574c069b2f14432c2bddf7
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-0cmc1v90/src'
make: *** [docker-run-test-quick@centos7] Error 2

real12m14.785s
user0m8.902s


The full log is available at
http://patchew.org/logs/20200817033803.14014-1-rockcui...@zhaoxin.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH] ide:do nothing for identify cmd if no any device attached

[zhaoxin\RockCuioc]

This patch is for avoiding win7 IDE driver polling 0x1f7 when
no any device attached. During Win7 VM boot procedure, if use virtio for
disk and there is no any device be attached on hda & hdb, the win7 IDE driver
would poll 0x1f7 for a while. This action may be stop windows LOGO atomic for
a while too on a poor performance CPU.

Signed-off-by: zhaoxin\RockCuioc 
---
 hw/ide/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..26d86f4b40 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2073,8 +2073,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
 s = idebus_active_if(bus);
 trace_ide_exec_cmd(bus, s, val);
 
-/* ignore commands to non existent slave */
-if (s != bus->ifs && !s->blk) {
+/* ignore commands if no any device exist or non existent slave */
+if ((!bus->ifs[0].blk && !bus->ifs[1].blk) ||
+(s != bus->ifs && !s->blk)) {
 return;
 }
 
-- 
2.17.1

Re: [PATCH v4] qapi/opts-visitor: Fixed fallthrough compiler warning

[Rohit Shinde]

On Sun, Aug 16, 2020 at 11:26 PM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 8/16/20 5:57 PM, Rohit Shinde wrote:
> > I misread the comment. The comment /* fallthrough */ was meant to stop
> the
> > compiler warning from occurring. I am trying to complete the bite sized
> task
> > mentioned here  under
> > "Compiler driven cleanups". I wanted to take that up to get more
> familiar with
> > the codebase.
>
> Fine, but the current comment matches the compiler regexp, so this instance
> isn't part of that cleanup.
>
But when I was compiling with the -Wimplicit-fallthrough option, the
compiler gave an error at that very line. That's why I came up on that
specific line in that file first.

I am pasting the error that I got below:

home/rohit/Desktop/open-source/qemu/qapi/opts-visitor.c: In function
‘opts_next_list’:
/home/rohit/Desktop/open-source/qemu/qapi/opts-visitor.c:267:23: error:
this statement may fall through [-Werror=implicit-fallthrough=]
  267 | ov->list_mode = LM_IN_PROGRESS;
  | ~~^~~~
/home/rohit/Desktop/open-source/qemu/qapi/opts-visitor.c:270:5: note: here
  270 | case LM_IN_PROGRESS: {
  | ^~~~

Sending it again, because I forgot to reply all.

Thanks,
Rohit.

>
>
> r~
>
>

[Bug 1891829] Re: High bit(s) sometimes set high on rcvd serial bytes when char size < 8 bits

[Michael Slade]

** Summary changed:

- High bits(s) sometimes set high on rcvd serial bytes when char size < 8 bits
+ High bit(s) sometimes set high on rcvd serial bytes when char size < 8 bits

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

Title:
  High bit(s) sometimes set high on rcvd serial bytes when char size < 8
  bits

Status in QEMU:
  New

Bug description:
  I *believe* (not confirmed) that the old standard PC serial ports,
  when configured with a character size of 7 bits or less, should set
  non-data bits to 0 when the CPU reads received chars from the read
  register.  qemu doesn't do this.

  Windows 1.01 will not make use of a serial mouse when bit 7 is 1.  The
  ID byte that the mouse sends on reset is ignored.  I added a temporary
  hack to set bit 7 to 0 on all incoming bytes, and this convinced
  windows 1.01 to use the mouse.

  note 1:  This was using a real serial mouse through a passed-through
  serial port.  The emulated msmouse doesn't work for other reasons.

  note 2:  The USB serial port I am passing through to the guest sets
  non-data bits to 1.  Not sure if this is the USB hardware or linux.

  note 3:  I also needed to add an -icount line to slow down the guest
  CPU, so that certain cpu-sensitive timing code in the guest didn't
  give up too quickly.

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

Re: [PATCH v4] qapi/opts-visitor: Fixed fallthrough compiler warning

[Richard Henderson]

On 8/16/20 5:57 PM, Rohit Shinde wrote:
> I misread the comment. The comment /* fallthrough */ was meant to stop the
> compiler warning from occurring. I am trying to complete the bite sized task
> mentioned here  under
> "Compiler driven cleanups". I wanted to take that up to get more familiar with
> the codebase.

Fine, but the current comment matches the compiler regexp, so this instance
isn't part of that cleanup.


r~

Re: [PATCH v3 00/10] *** A Method for evaluating dirty page rate ***

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/1597634433-18809-1-git-send-email-zhengch...@huawei.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1597634433-18809-1-git-send-email-zhengch...@huawei.com
Subject: [PATCH v3 00/10] *** A Method for evaluating dirty page rate ***

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] 
patchew/1597634433-18809-1-git-send-email-zhengch...@huawei.com -> 
patchew/1597634433-18809-1-git-send-email-zhengch...@huawei.com
Switched to a new branch 'test'
ce6fe0d migration/dirtyrate: Implement 
qmp_cal_dirty_rate()/qmp_get_dirty_rate() function
96ce007 migration/dirtyrate: Implement calculate_dirtyrate() function
42642d4 migration/dirtyrate: Implement get_sample_page_period() and 
block_sample_page_period()
3a4d4a2 migration/dirtyrate: skip sampling ramblock with size below 
MIN_RAMBLOCK_SIZE
31f35b7 migration/dirtyrate: Compare page hash results for recorded sampled page
a3d582d migration/dirtyrate: Record hash results for each sampled page
85c6447 migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h
c7c94cc migration/dirtyrate: Add dirtyrate statistics series functions
a76c0d0 migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info
7f6092c migration/dirtyrate: Add get_dirtyrate_thread() function

=== OUTPUT BEGIN ===
1/10 Checking commit 7f6092c52d5c (migration/dirtyrate: Add 
get_dirtyrate_thread() function)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#26: 
new file mode 100644

total: 0 errors, 1 warnings, 115 lines checked

Patch 1/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/10 Checking commit a76c0d08e93d (migration/dirtyrate: Add RamlockDirtyInfo to 
store sampled page info)
3/10 Checking commit c7c94cc4c17d (migration/dirtyrate: Add dirtyrate 
statistics series functions)
4/10 Checking commit 85c64473ab70 (migration/dirtyrate: move 
RAMBLOCK_FOREACH_MIGRATABLE into ram.h)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#63: FILE: migration/ram.h:42:
+#define RAMBLOCK_FOREACH_NOT_IGNORED(block)\
+INTERNAL_RAMBLOCK_FOREACH(block)   \
+if (ramblock_is_ignored(block)) {} else

ERROR: trailing statements should be on next line
#65: FILE: migration/ram.h:44:
+if (ramblock_is_ignored(block)) {} else

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#67: FILE: migration/ram.h:46:
+#define RAMBLOCK_FOREACH_MIGRATABLE(block) \
+INTERNAL_RAMBLOCK_FOREACH(block)   \
+if (!qemu_ram_is_migratable(block)) {} else

ERROR: trailing statements should be on next line
#69: FILE: migration/ram.h:48:
+if (!qemu_ram_is_migratable(block)) {} else

ERROR: braces {} are necessary for all arms of this statement
#69: FILE: migration/ram.h:48:
+if (!qemu_ram_is_migratable(block)) {} else
[...]
+if (!qemu_ram_is_migratable(block)) {} else
[...]

total: 5 errors, 0 warnings, 45 lines checked

Patch 4/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/10 Checking commit a3d582d88093 (migration/dirtyrate: Record hash results for 
each sampled page)
6/10 Checking commit 31f35b73e761 (migration/dirtyrate: Compare page hash 
results for recorded sampled page)
7/10 Checking commit 3a4d4a207812 (migration/dirtyrate: skip sampling ramblock 
with size below MIN_RAMBLOCK_SIZE)
8/10 Checking commit 42642d45cf29 (migration/dirtyrate: Implement 
get_sample_page_period() and block_sample_page_period())
9/10 Checking commit 96ce00742d23 (migration/dirtyrate: Implement 
calculate_dirtyrate() function)
10/10 Checking commit ce6fe0d124c5 (migration/dirtyrate: Implement 
qmp_cal_dirty_rate()/qmp_get_dirty_rate() function)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/1597634433-18809-1-git-send-email-zhengch...@huawei.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH v3 10/10] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Chuan Zheng]

Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be 
called

Signed-off-by: Chuan Zheng 
---
 migration/dirtyrate.c | 56 +++
 qapi/migration.json   | 42 ++
 2 files changed, 98 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 041d0c6..0e8c9c5 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -67,6 +67,39 @@ static int dirty_rate_set_state(int new_state)
 return 0;
 }
 
+static struct DirtyRateInfo *query_dirty_rate_info(void)
+{
+int64_t dirty_rate = dirty_stat.dirty_rate;
+struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
+
+switch (CalculatingState) {
+case CAL_DIRTY_RATE_INIT:
+info->dirty_rate = -1;
+info->status = g_strdup("Not start measuring");
+break;
+case CAL_DIRTY_RATE_ACTIVE:
+info->dirty_rate = -1;
+info->status = g_strdup("Still measuring");
+break;
+case CAL_DIRTY_RATE_END:
+info->dirty_rate = dirty_rate;
+info->status = g_strdup("Measured");
+break;
+default:
+info->dirty_rate = -1;
+info->status = g_strdup("Unknown status");
+break;
+}
+
+/*
+ * Only support query once for each calculation,
+ * reset as CAL_DIRTY_RATE_INIT after query
+ */
+(void)dirty_rate_set_state(CAL_DIRTY_RATE_INIT);
+
+return info;
+}
+
 static void reset_dirtyrate_stat(void)
 {
 dirty_stat.total_dirty_samples = 0;
@@ -403,3 +436,26 @@ void *get_dirtyrate_thread(void *arg)
 
 return NULL;
 }
+
+void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
+{
+static struct DirtyRateConfig config;
+QemuThread thread;
+
+/*
+ * We don't begin calculating thread only when it's in calculating status.
+ */
+if (CalculatingState == CAL_DIRTY_RATE_ACTIVE) {
+return;
+}
+
+config.sample_period_seconds = get_sample_page_period(calc_time);
+config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
+qemu_thread_create(, "get_dirtyrate", get_dirtyrate_thread,
+   (void *), QEMU_THREAD_DETACHED);
+}
+
+struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
+{
+return query_dirty_rate_info();
+}
diff --git a/qapi/migration.json b/qapi/migration.json
index d500055..ccc7a4e 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1621,3 +1621,45 @@
 ##
 { 'event': 'UNPLUG_PRIMARY',
   'data': { 'device-id': 'str' } }
+
+##
+# @DirtyRateInfo:
+#
+# Information about current dirty page rate of vm.
+#
+# @dirty-rate: @dirtyrate describing the dirty page rate of vm
+#  in units of MB/s.
+#  If this field return '-1', it means querying is not
+#  start or not complete.
+#
+# @status: @status containing dirtyrate query status includes
+#   status with 'not start measuring' or
+#   'Still measuring' or 'measured'(since 5.2)
+##
+{ 'struct': 'DirtyRateInfo',
+  'data': {'dirty-rate': 'int64',
+   'status': 'str'} }
+
+##
+# @calc-dirty-rate:
+#
+# start calculating dirty page rate for vm
+#
+# @calc-time: time in units of second for sample dirty pages
+#
+# Since: 5.2
+#
+# Example:
+#   {"command": "cal-dirty-rate", "data": {"calc-time": 1} }
+#
+##
+{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
+
+##
+# @query-dirty-rate:
+#
+# query dirty page rate in units of MB/s for vm
+#
+# Since: 5.2
+##
+{ 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
-- 
1.8.3.1

[PATCH v3 04/10] migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h

[Chuan Zheng]

RAMBLOCK_FOREACH_MIGRATABLE is need in dirtyrate measure,
move the existing definition up into migration/ram.h

Signed-off-by: Chuan Zheng 
Signed-off-by: YanYing Zhuang 
---
 migration/dirtyrate.c |  1 +
 migration/ram.c   | 11 +--
 migration/ram.h   | 10 ++
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 8708090..c4304ef 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -21,6 +21,7 @@
 #include "qemu/rcu_queue.h"
 #include "qapi/qapi-commands-migration.h"
 #include "migration.h"
+#include "ram.h"
 #include "dirtyrate.h"
 
 CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
diff --git a/migration/ram.c b/migration/ram.c
index 76d4fee..37ef0da 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -158,21 +158,12 @@ out:
 return ret;
 }
 
-static bool ramblock_is_ignored(RAMBlock *block)
+bool ramblock_is_ignored(RAMBlock *block)
 {
 return !qemu_ram_is_migratable(block) ||
(migrate_ignore_shared() && qemu_ram_is_shared(block));
 }
 
-/* Should be holding either ram_list.mutex, or the RCU lock. */
-#define RAMBLOCK_FOREACH_NOT_IGNORED(block)\
-INTERNAL_RAMBLOCK_FOREACH(block)   \
-if (ramblock_is_ignored(block)) {} else
-
-#define RAMBLOCK_FOREACH_MIGRATABLE(block) \
-INTERNAL_RAMBLOCK_FOREACH(block)   \
-if (!qemu_ram_is_migratable(block)) {} else
-
 #undef RAMBLOCK_FOREACH
 
 int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque)
diff --git a/migration/ram.h b/migration/ram.h
index 2eeaacf..011e854 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -37,6 +37,16 @@ extern MigrationStats ram_counters;
 extern XBZRLECacheStats xbzrle_counters;
 extern CompressionStats compression_counters;
 
+bool ramblock_is_ignored(RAMBlock *block);
+/* Should be holding either ram_list.mutex, or the RCU lock. */
+#define RAMBLOCK_FOREACH_NOT_IGNORED(block)\
+INTERNAL_RAMBLOCK_FOREACH(block)   \
+if (ramblock_is_ignored(block)) {} else
+
+#define RAMBLOCK_FOREACH_MIGRATABLE(block) \
+INTERNAL_RAMBLOCK_FOREACH(block)   \
+if (!qemu_ram_is_migratable(block)) {} else
+
 int xbzrle_cache_resize(int64_t new_size, Error **errp);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_total(void);
-- 
1.8.3.1

[PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Chuan Zheng]

Record hash results for each sampled page.

Signed-off-by: Chuan Zheng 
Signed-off-by: YanYing Zhuang 
---
 migration/dirtyrate.c | 144 ++
 migration/dirtyrate.h |   7 +++
 2 files changed, 151 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index c4304ef..62b6f69 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -25,6 +25,7 @@
 #include "dirtyrate.h"
 
 CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
+static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
 static struct DirtyRateStat dirty_stat;
 
 static int dirty_rate_set_state(int new_state)
@@ -71,6 +72,149 @@ static void update_dirtyrate(uint64_t msec)
 dirty_stat.dirty_rate = dirty_rate;
 }
 
+/*
+ * get hash result for the sampled memory with length of 4K byte in ramblock,
+ * which starts from ramblock base address.
+ */
+static int get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
+ unsigned long vfn, uint8_t **md)
+{
+struct iovec iov_array;
+int ret = 0;
+int nkey = 1;
+size_t hash_len = qcrypto_hash_len;
+
+iov_array.iov_base = info->ramblock_addr +
+ vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
+iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
+
+if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_MD5,
+_array, nkey,
+md, _len, NULL) < 0) {
+ret = -1;
+}
+
+return ret;
+}
+
+static int save_ramblock_hash(struct RamblockDirtyInfo *info)
+{
+unsigned int sample_pages_count;
+uint8_t *md = NULL;
+int i;
+int ret = -1;
+GRand *rand = g_rand_new();
+
+sample_pages_count = info->sample_pages_count;
+
+/* ramblock size less than one page, return success to skip this ramblock 
*/
+if (unlikely(info->ramblock_pages == 0 || sample_pages_count == 0)) {
+ret = 0;
+goto out;
+}
+
+info->hash_result = g_try_malloc0_n(sample_pages_count,
+sizeof(uint8_t) * qcrypto_hash_len);
+if (!info->hash_result) {
+ret = -1;
+goto out;
+}
+
+info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
+sizeof(unsigned long));
+if (!info->sample_page_vfn) {
+g_free(info->hash_result);
+ret = -1;
+goto out;
+}
+
+for (i = 0; i < sample_pages_count; i++) {
+md = info->hash_result + i * qcrypto_hash_len;
+info->sample_page_vfn[i] = g_rand_int_range(rand, 0,
+info->ramblock_pages - 1);
+ret = get_ramblock_vfn_hash(info, info->sample_page_vfn[i], );
+if (ret < 0) {
+goto out;
+}
+}
+ret = 0;
+
+out:
+g_rand_free(rand);
+return ret;
+}
+
+static void get_ramblock_dirty_info(RAMBlock *block,
+struct RamblockDirtyInfo *info,
+struct DirtyRateConfig *config)
+{
+uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
+
+/* Right shift 30 bits to calc block size in GB */
+info->sample_pages_count = (qemu_ram_get_used_length(block)
+* sample_pages_per_gigabytes) >> 30;
+
+/* Right shift 12 bits to calc page count in 4KB */
+info->ramblock_pages = qemu_ram_get_used_length(block) >> 12;
+info->ramblock_addr = qemu_ram_get_host_addr(block);
+strcpy(info->idstr, qemu_ram_get_idstr(block));
+}
+
+static struct RamblockDirtyInfo *
+alloc_ramblock_dirty_info(int *block_index,
+  struct RamblockDirtyInfo *block_dinfo)
+{
+struct RamblockDirtyInfo *info = NULL;
+int index = *block_index;
+
+if (!block_dinfo) {
+block_dinfo = g_try_new(struct RamblockDirtyInfo, 1);
+index = 0;
+} else {
+index++;
+block_dinfo = g_try_realloc(block_dinfo, (index + 1) *
+sizeof(struct RamblockDirtyInfo));
+}
+if (!block_dinfo) {
+return NULL;
+}
+
+info = _dinfo[index];
+memset(info, 0, sizeof(struct RamblockDirtyInfo));
+
+*block_index = index;
+return block_dinfo;
+}
+
+static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
+ struct DirtyRateConfig config,
+ int *block_index)
+{
+struct RamblockDirtyInfo *info = NULL;
+struct RamblockDirtyInfo *dinfo = NULL;
+RAMBlock *block = NULL;
+int index = 0;
+
+RAMBLOCK_FOREACH_MIGRATABLE(block) {
+dinfo = alloc_ramblock_dirty_info(, dinfo);
+if (dinfo == NULL) {
+return -1;
+}
+info = [index];
+get_ramblock_dirty_info(block, info, );
+if (save_ramblock_hash(info) < 0) {
+*block_dinfo = dinfo;
+*block_index = index;

[PATCH v3 06/10] migration/dirtyrate: Compare page hash results for recorded sampled page

[Chuan Zheng]

Compare page hash results for recorded sampled page.

Signed-off-by: Chuan Zheng 
Signed-off-by: YanYing Zhuang 
---
 migration/dirtyrate.c | 76 +++
 1 file changed, 76 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 62b6f69..3ce25f5 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -215,6 +215,82 @@ static int record_ramblock_hash_info(struct 
RamblockDirtyInfo **block_dinfo,
 return 0;
 }
 
+static int calc_page_dirty_rate(struct RamblockDirtyInfo *info)
+{
+uint8_t *md = NULL;
+int i;
+int ret = 0;
+
+md = g_try_new0(uint8_t, qcrypto_hash_len);
+if (!md) {
+return -1;
+}
+
+for (i = 0; i < info->sample_pages_count; i++) {
+ret = get_ramblock_vfn_hash(info, info->sample_page_vfn[i], );
+if (ret < 0) {
+goto out;
+}
+
+if (memcmp(md, info->hash_result + i * qcrypto_hash_len,
+   qcrypto_hash_len) != 0) {
+info->sample_dirty_count++;
+}
+}
+
+out:
+g_free(md);
+return ret;
+}
+
+static bool find_page_matched(RAMBlock *block, struct RamblockDirtyInfo *infos,
+  int count, struct RamblockDirtyInfo **matched)
+{
+int i;
+
+for (i = 0; i < count; i++) {
+if (!strcmp(infos[i].idstr, qemu_ram_get_idstr(block))) {
+break;
+}
+}
+
+if (i == count) {
+return false;
+}
+
+if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
+infos[i].ramblock_pages !=
+(qemu_ram_get_used_length(block) >> 12)) {
+return false;
+}
+
+*matched = [i];
+return true;
+}
+
+static int compare_page_hash_info(struct RamblockDirtyInfo *info,
+  int block_index)
+{
+struct RamblockDirtyInfo *block_dinfo = NULL;
+RAMBlock *block = NULL;
+
+RAMBLOCK_FOREACH_MIGRATABLE(block) {
+block_dinfo = NULL;
+if (!find_page_matched(block, info, block_index + 1, _dinfo)) {
+continue;
+}
+if (calc_page_dirty_rate(block_dinfo) < 0) {
+return -1;
+}
+update_dirtyrate_stat(block_dinfo);
+}
+if (!dirty_stat.total_sample_count) {
+return -1;
+}
+
+return 0;
+}
+
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
 /* todo */
-- 
1.8.3.1

[PATCH v3 07/10] migration/dirtyrate: skip sampling ramblock with size below MIN_RAMBLOCK_SIZE

[Chuan Zheng]

In order to sample real RAM, skip ramblock with size below

Signed-off-by: Chuan Zheng 
---
 migration/dirtyrate.c | 24 
 migration/dirtyrate.h |  5 +
 2 files changed, 29 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 3ce25f5..6f30f67 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -186,6 +186,24 @@ alloc_ramblock_dirty_info(int *block_index,
 return block_dinfo;
 }
 
+static int skip_sample_ramblock(RAMBlock *block)
+{
+int64_t ramblock_size;
+
+/* ramblock size in MB */
+ramblock_size = qemu_ram_get_used_length(block) >> 20;
+
+/*
+ * Consider ramblock with size larger than 128M is what we
+ * want to sample.
+ */
+if (ramblock_size < MIN_RAMBLOCK_SIZE) {
+return -1;
+}
+
+return 0;
+}
+
 static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
  struct DirtyRateConfig config,
  int *block_index)
@@ -196,6 +214,9 @@ static int record_ramblock_hash_info(struct 
RamblockDirtyInfo **block_dinfo,
 int index = 0;
 
 RAMBLOCK_FOREACH_MIGRATABLE(block) {
+if (skip_sample_ramblock(block) < 0) {
+continue;
+}
 dinfo = alloc_ramblock_dirty_info(, dinfo);
 if (dinfo == NULL) {
 return -1;
@@ -275,6 +296,9 @@ static int compare_page_hash_info(struct RamblockDirtyInfo 
*info,
 RAMBlock *block = NULL;
 
 RAMBLOCK_FOREACH_MIGRATABLE(block) {
+if (skip_sample_ramblock(block) < 0) {
+continue;
+}
 block_dinfo = NULL;
 if (!find_page_matched(block, info, block_index + 1, _dinfo)) {
 continue;
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 0812b16..fce2e3b 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -31,6 +31,11 @@
 
 #define QCRYPTO_HASH_LEN  16
 
+/*
+ * minimum ramblock size to sampled
+ */
+#define MIN_RAMBLOCK_SIZE128
+
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC  1
 
-- 
1.8.3.1

[PATCH v3 02/10] migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info

[Chuan Zheng]

Add RamlockDirtyInfo to store sampled page info of each ramblock.

Signed-off-by: Chuan Zheng 
Signed-off-by: YanYing Zhuang 
---
 migration/dirtyrate.h | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 914c363..9650566 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -19,6 +19,11 @@
  */
 #define DIRTYRATE_DEFAULT_SAMPLE_PAGES256
 
+/*
+ * Record ramblock idstr
+ */
+#define RAMBLOCK_INFO_MAX_LEN 256
+
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC  1
 
@@ -39,6 +44,19 @@ typedef enum {
 CAL_DIRTY_RATE_END,
 } CalculatingDirtyRateState;
 
+/*
+ * Store dirtypage info for each ramblock.
+ */
+struct RamblockDirtyInfo {
+char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */
+uint8_t *ramblock_addr; /* base address of ramblock we measure */
+size_t ramblock_pages; /* sum of dividation by 4K pages for ramblock */
+size_t *sample_page_vfn; /* relative offset address for sampled page */
+unsigned int sample_pages_count; /* sum of sampled pages */
+unsigned int sample_dirty_count; /* sum of dirty pages we measure */
+uint8_t *hash_result; /* array of hash result for sampled pages */
+};
+
 void *get_dirtyrate_thread(void *arg);
 #endif
 
-- 
1.8.3.1

[PATCH v3 01/10] migration/dirtyrate: Add get_dirtyrate_thread() function

[Chuan Zheng]

Add get_dirtyrate_thread() functions

Signed-off-by: Chuan Zheng 
Signed-off-by: YanYing Zhuang 
---
 migration/Makefile.objs |  1 +
 migration/dirtyrate.c   | 64 +
 migration/dirtyrate.h   | 44 ++
 3 files changed, 109 insertions(+)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

diff --git a/migration/Makefile.objs b/migration/Makefile.objs
index 0fc619e..12ae98c 100644
--- a/migration/Makefile.objs
+++ b/migration/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-y += qemu-file.o global_state.o
 common-obj-y += qemu-file-channel.o
 common-obj-y += xbzrle.o postcopy-ram.o
 common-obj-y += qjson.o
+common-obj-y += dirtyrate.o
 common-obj-y += block-dirty-bitmap.o
 common-obj-y += multifd.o
 common-obj-y += multifd-zlib.o
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
new file mode 100644
index 000..bb0ebe9
--- /dev/null
+++ b/migration/dirtyrate.c
@@ -0,0 +1,64 @@
+/*
+ * Dirtyrate implement code
+ *
+ * Copyright (c) 2017-2020 HUAWEI TECHNOLOGIES CO.,LTD.
+ *
+ * Authors:
+ *  Chuan Zheng 
+ *
+ * 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 "qapi/error.h"
+#include "crypto/hash.h"
+#include "crypto/random.h"
+#include "qemu/config-file.h"
+#include "exec/memory.h"
+#include "exec/ramblock.h"
+#include "exec/target_page.h"
+#include "qemu/rcu_queue.h"
+#include "qapi/qapi-commands-migration.h"
+#include "migration.h"
+#include "dirtyrate.h"
+
+CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
+
+static int dirty_rate_set_state(int new_state)
+{
+int old_state = CalculatingState;
+
+if (new_state == old_state) {
+return -1;
+}
+
+if (atomic_cmpxchg(, old_state, new_state) != old_state) {
+return -1;
+}
+
+return 0;
+}
+
+static void calculate_dirtyrate(struct DirtyRateConfig config)
+{
+/* todo */
+return;
+}
+
+void *get_dirtyrate_thread(void *arg)
+{
+struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
+int ret;
+
+ret = dirty_rate_set_state(CAL_DIRTY_RATE_ACTIVE);
+if (ret == -1) {
+return NULL;
+}
+
+calculate_dirtyrate(config);
+
+ret = dirty_rate_set_state(CAL_DIRTY_RATE_END);
+
+return NULL;
+}
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
new file mode 100644
index 000..914c363
--- /dev/null
+++ b/migration/dirtyrate.h
@@ -0,0 +1,44 @@
+/*
+ *  Dirtyrate common functions
+ *
+ *  Copyright (c) 2020 HUAWEI TECHNOLOGIES CO., LTD.
+ *
+ *  Authors:
+ *  Chuan Zheng 
+ *
+ *  This work is licensed under the terms of the GNU GPL, version 2 or later.
+ *  See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_MIGRATION_DIRTYRATE_H
+#define QEMU_MIGRATION_DIRTYRATE_H
+
+/*
+ * Sample 256 pages per GB as default.
+ * TODO: Make it configurable.
+ */
+#define DIRTYRATE_DEFAULT_SAMPLE_PAGES256
+
+/* Take 1s as default for calculation duration */
+#define DEFAULT_FETCH_DIRTYRATE_TIME_SEC  1
+
+struct DirtyRateConfig {
+uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
+int64_t sample_period_seconds; /* time duration between two sampling */
+};
+
+/*
+ *  To record calculate dirty_rate status:
+ *  0: initial status, calculating thread is not be created here.
+ *  1: calculating thread is created.
+ *  2: calculating thread is end, we can get result.
+ */
+typedef enum {
+CAL_DIRTY_RATE_INIT = 0,
+CAL_DIRTY_RATE_ACTIVE,
+CAL_DIRTY_RATE_END,
+} CalculatingDirtyRateState;
+
+void *get_dirtyrate_thread(void *arg);
+#endif
+
-- 
1.8.3.1

[PATCH v3 08/10] migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period()

[Chuan Zheng]

Implement get_sample_page_period() and set_sample_page_period() to
sleep specific time between sample actions.

Signed-off-by: Chuan Zheng 
Signed-off-by: YanYing Zhuang 
---
 migration/dirtyrate.c | 24 
 migration/dirtyrate.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 6f30f67..4bbfcc3 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -28,6 +28,30 @@ CalculatingDirtyRateState CalculatingState = 
CAL_DIRTY_RATE_INIT;
 static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
 static struct DirtyRateStat dirty_stat;
 
+static int64_t set_sample_page_period(int64_t msec, int64_t initial_time)
+{
+int64_t current_time;
+
+current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+if ((current_time - initial_time) >= msec) {
+msec = current_time - initial_time;
+} else {
+g_usleep((msec + initial_time - current_time) * 1000);
+}
+
+return msec;
+}
+
+static int64_t get_sample_page_period(int64_t sec)
+{
+if (sec <= MIN_FETCH_DIRTYRATE_TIME_SEC ||
+sec > MAX_FETCH_DIRTYRATE_TIME_SEC) {
+sec = DEFAULT_FETCH_DIRTYRATE_TIME_SEC;
+}
+
+return sec;
+}
+
 static int dirty_rate_set_state(int new_state)
 {
 int old_state = CalculatingState;
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index fce2e3b..86d8fa0 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -38,6 +38,8 @@
 
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC  1
+#define MIN_FETCH_DIRTYRATE_TIME_SEC  0
+#define MAX_FETCH_DIRTYRATE_TIME_SEC  60
 
 struct DirtyRateConfig {
 uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
-- 
1.8.3.1

[PATCH v3 00/10] *** A Method for evaluating dirty page rate ***

[Chuan Zheng]

v2 -> v3:
fix size_t compile warning
fix codestyle checked by checkpatch.pl

v1 -> v2:
use g_rand_new() to generate rand_buf
move RAMBLOCK_FOREACH_MIGRATABLE into migration/ram.h
add skip_sample_ramblock to filter sampled ramblock
fix multi-numa vm coredump when query dirtyrate
rename qapi interface and rename some structures and functions
succeed to compile by appling each patch
add test for migrating vm

Sometimes it is neccessary to evaluate dirty page rate before migration.
Users could decide whether to proceed migration based on the evaluation
in case of vm performance loss due to heavy workload.
Unlikey simulating dirtylog sync which could do harm on runnning vm,
we provide a sample-hash method to compare hash results for samping page.
In this way, it would have hardly no impact on vm performance.

Evaluate the dirtypage rate both on running and migration vm.
The VM specifications for migration are as follows:
- VM use 4-K page;
- the number of VCPU is 32;
- the total memory is 32Gigabit;
- use 'mempress' tool to pressurize VM(mempress 4096 1024);
- migration bandwidth is 1GB/s


|  |  running  |  migrating 
   |

| no mempress  |   4MB/s   |  8MB/s  (migrated success) 
   |

| mempress 4096 1024   |  1188MB/s |   536MB/s ~ 1044MB/s (cpu throttle 
triggered) |

| mempress 4096 4096   |  4152MB/s | 608MB/s ~ 4125MB/s (migation failed)   
   |


Test dirtyrate by qmp command like this:
1.  virsh qemu-monitor-command [vmname] '{"execute":"calc-dirty-rate", 
"arguments": {"calc-time": [sleep-time]}}'; 
2.  sleep specific time which is a bit larger than sleep-time
3.  virsh qemu-monitor-command [vmname] '{"execute":"query-dirty-rate"}'

Further test dirtyrate by libvirt api like this:
virsh getdirtyrate [vmname] [sleep-time]

Zheng Chuan (10):
  migration/dirtyrate: Add get_dirtyrate_thread() function
  migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info
  migration/dirtyrate: Add dirtyrate statistics series functions
  migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h
  migration/dirtyrate: Record hash results for each sampled page
  migration/dirtyrate: Compare page hash results for recorded sampled
page
  migration/dirtyrate: skip sampling ramblock with size below
MIN_RAMBLOCK_SIZE
  migration/dirtyrate: Implement get_sample_page_period() and
block_sample_page_period()
  migration/dirtyrate: Implement calculate_dirtyrate() function
  migration/dirtyrate: Implement
qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

 migration/Makefile.objs |   1 +
 migration/dirtyrate.c   | 448 
 migration/dirtyrate.h   |  86 ++
 migration/ram.c |  11 +-
 migration/ram.h |  10 ++
 qapi/migration.json |  42 +
 6 files changed, 588 insertions(+), 10 deletions(-)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

-- 
1.8.3.1

[PATCH v3 09/10] migration/dirtyrate: Implement calculate_dirtyrate() function

[Chuan Zheng]

Implement calculate_dirtyrate() function.

Signed-off-by: Chuan Zheng 
Signed-off-by: YanYing Zhuang 
---
 migration/dirtyrate.c | 46 --
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 4bbfcc3..041d0c6 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -184,6 +184,21 @@ static void get_ramblock_dirty_info(RAMBlock *block,
 strcpy(info->idstr, qemu_ram_get_idstr(block));
 }
 
+static void free_ramblock_dirty_info(struct RamblockDirtyInfo *infos, int 
count)
+{
+int i;
+
+if (!infos) {
+return;
+}
+
+for (i = 0; i < count; i++) {
+g_free(infos[i].sample_page_vfn);
+g_free(infos[i].hash_result);
+}
+g_free(infos);
+}
+
 static struct RamblockDirtyInfo *
 alloc_ramblock_dirty_info(int *block_index,
   struct RamblockDirtyInfo *block_dinfo)
@@ -341,8 +356,35 @@ static int compare_page_hash_info(struct RamblockDirtyInfo 
*info,
 
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
-/* todo */
-return;
+struct RamblockDirtyInfo *block_dinfo = NULL;
+int block_index = 0;
+int64_t msec = 0;
+int64_t initial_time;
+
+rcu_register_thread();
+reset_dirtyrate_stat();
+initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+rcu_read_lock();
+if (record_ramblock_hash_info(_dinfo, config, _index) < 0) {
+goto out;
+}
+rcu_read_unlock();
+
+msec = config.sample_period_seconds * 1000;
+msec = set_sample_page_period(msec, initial_time);
+
+rcu_read_lock();
+if (compare_page_hash_info(block_dinfo, block_index) < 0) {
+goto out;
+}
+
+update_dirtyrate(msec);
+
+out:
+rcu_read_unlock();
+free_ramblock_dirty_info(block_dinfo, block_index + 1);
+rcu_unregister_thread();
+
 }
 
 void *get_dirtyrate_thread(void *arg)
-- 
1.8.3.1

[PATCH v3 03/10] migration/dirtyrate: Add dirtyrate statistics series functions

[Chuan Zheng]

Add dirtyrate statistics to record/update dirtyrate info.

Signed-off-by: Chuan Zheng 
Signed-off-by: YanYing Zhuang 
---
 migration/dirtyrate.c | 30 ++
 migration/dirtyrate.h | 10 ++
 2 files changed, 40 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index bb0ebe9..8708090 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -24,6 +24,7 @@
 #include "dirtyrate.h"
 
 CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
+static struct DirtyRateStat dirty_stat;
 
 static int dirty_rate_set_state(int new_state)
 {
@@ -40,6 +41,35 @@ static int dirty_rate_set_state(int new_state)
 return 0;
 }
 
+static void reset_dirtyrate_stat(void)
+{
+dirty_stat.total_dirty_samples = 0;
+dirty_stat.total_sample_count = 0;
+dirty_stat.total_block_mem_MB = 0;
+dirty_stat.dirty_rate = 0;
+}
+
+static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
+{
+dirty_stat.total_dirty_samples += info->sample_dirty_count;
+dirty_stat.total_sample_count += info->sample_pages_count;
+/* size of 4K pages in MB */
+dirty_stat.total_block_mem_MB += info->ramblock_pages / 256;
+}
+
+static void update_dirtyrate(uint64_t msec)
+{
+uint64_t dirty_rate;
+unsigned int total_dirty_samples = dirty_stat.total_dirty_samples;
+unsigned int total_sample_count = dirty_stat.total_sample_count;
+size_t total_block_mem_MB = dirty_stat.total_block_mem_MB;
+
+dirty_rate = total_dirty_samples * total_block_mem_MB *
+ 1000 / (total_sample_count * msec);
+
+dirty_stat.dirty_rate = dirty_rate;
+}
+
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
 /* todo */
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 9650566..af57c80 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -57,6 +57,16 @@ struct RamblockDirtyInfo {
 uint8_t *hash_result; /* array of hash result for sampled pages */
 };
 
+/*
+ * Store calculate statistics for each measure.
+ */
+struct DirtyRateStat {
+unsigned int total_dirty_samples; /* total dirty pages for this measure */
+unsigned int total_sample_count; /* total sampled pages for this measure */
+size_t total_block_mem_MB; /* size of sampled pages in MB */
+int64_t dirty_rate; /* dirty rate for this measure */
+};
+
 void *get_dirtyrate_thread(void *arg);
 #endif
 
-- 
1.8.3.1

Re: device compatibility interface for live migration with assigned devices

[Yan Zhao]

On Fri, Aug 14, 2020 at 01:30:00PM +0100, Sean Mooney wrote:
> On Fri, 2020-08-14 at 13:16 +0800, Yan Zhao wrote:
> > On Thu, Aug 13, 2020 at 12:24:50PM +0800, Jason Wang wrote:
> > > 
> > > On 2020/8/10 下午3:46, Yan Zhao wrote:
> > > > > driver is it handled by?
> > > > 
> > > > It looks that the devlink is for network device specific, and in
> > > > devlink.h, it says
> > > > include/uapi/linux/devlink.h - Network physical device Netlink
> > > > interface,
> > > 
> > > 
> > > Actually not, I think there used to have some discussion last year and the
> > > conclusion is to remove this comment.
> > > 
> > > It supports IB and probably vDPA in the future.
> > > 
> > 
> > hmm... sorry, I didn't find the referred discussion. only below discussion
> > regarding to why to add devlink.
> > 
> > https://www.mail-archive.com/netdev@vger.kernel.org/msg95801.html
> > >This doesn't seem to be too much related to networking? Why can't 
> > something
> > >like this be in sysfs?
> > 
> > It is related to networking quite bit. There has been couple of
> > iteration of this, including sysfs and configfs implementations. There
> > has been a consensus reached that this should be done by netlink. I
> > believe netlink is really the best for this purpose. Sysfs is not a good
> > idea
> > 
> > https://www.mail-archive.com/netdev@vger.kernel.org/msg96102.html
> > >there is already a way to change eth/ib via
> > >echo 'eth' > /sys/bus/pci/drivers/mlx4_core/:02:00.0/mlx4_port1
> > >
> > >sounds like this is another way to achieve the same?
> > 
> > It is. However the current way is driver-specific, not correct.
> > For mlx5, we need the same, it cannot be done in this way. Do devlink is
> > the correct way to go.
> im not sure i agree with that.
> standardising a filesystem based api that is used across all vendors is also 
> a valid
> option.  that said if devlink is the right choice form a kerenl perspective 
> by all
> means use it but i have not heard a convincing argument for why it actually 
> better.
> with tthat said we have been uing tools like ethtool to manage aspect of nics 
> for decades
> so its not that strange an idea to use a tool and binary protocoal rather 
> then a text
> based interface for this but there are advantages to both approches.
> >
Yes, I agree with you.

> > https://lwn.net/Articles/674867/
> > There a is need for some userspace API that would allow to expose things
> > that are not directly related to any device class like net_device of
> > ib_device, but rather chip-wide/switch-ASIC-wide stuff.
> > 
> > Use cases:
> > 1) get/set of port type (Ethernet/InfiniBand)
> > 2) monitoring of hardware messages to and from chip
> > 3) setting up port splitters - split port into multiple ones and squash 
> > again,
> >enables usage of splitter cable
> > 4) setting up shared buffers - shared among multiple ports within one 
> > chip
> > 
> > 
> > 
> > we actually can also retrieve the same information through sysfs, .e.g
> > 
> > > - [path to device]
> > 
> >   |--- migration
> >   | |--- self
> >   | |   |---device_api
> >   | |   |---mdev_type
> >   | |   |---software_version
> >   | |   |---device_id
> >   | |   |---aggregator
> >   | |--- compatible
> >   | |   |---device_api
> >   | |   |---mdev_type
> >   | |   |---software_version
> >   | |   |---device_id
> >   | |   |---aggregator
> > 
> > 
> > 
> > > 
> > > >   I feel like it's not very appropriate for a GPU driver to use
> > > > this interface. Is that right?
> > > 
> > > 
> > > I think not though most of the users are switch or ethernet devices. It
> > > doesn't prevent you from inventing new abstractions.
> > 
> > so need to patch devlink core and the userspace devlink tool?
> > e.g. devlink migration
> and devlink python libs if openstack was to use it directly.
> we do have caes where we just frok a process and execaute a comannd in a shell
> with or without elevated privladge but we really dont like doing that due to 
> the performacne impacat and security implciations so where we can use python 
> bindign
> over c apis we do. pyroute2 is the only python lib i know off of the top of 
> my head
> that support devlink so we would need to enhacne it to support this new 
> devlink api.
> there may be otherss i have not really looked in the past since we dont need 
> to use
> devlink at all today.
> > 
> > > Note that devlink is based on netlink, netlink has been widely used by
> > > various subsystems other than networking.
> > 
> > the advantage of netlink I see is that it can monitor device status and
> > notify upper layer that migration database needs to get updated.
> > But not sure whether openstack would like to use this capability.
> > As Sean said, it's heavy for openstack. it's heavy for vendor driver
> > as well :)
> > 
> > And devlink monitor now listens the notification and dumps the state
> > changes. If we 

[Bug 1891830] [NEW] msmouse serial mouse emulation broken? No id byte sent on reset

[Michael Slade]

Public bug reported:

I took a shot at getting Windows 1.01 working.  It doesn't support a
PS/2 mouse out-of-the-box but does support MS serial mice.  It doesn't
seem to detect qemu's emulated msmouse.

When I run this command:

> qemu-system-i386 -nodefaults -hda my_windows1_hd.qcow2 -vga std
-serial msmouse -trace enable='serial*'  -icount shift=10,align=on

I get this output (edited):

251908@1597626456.800452:serial_ioport_write write addr 0x04 val 0x01
251908@1597626456.800460:serial_ioport_read read addr 0x00 val 0x00
251908@1597626456.800462:serial_ioport_read read addr 0x00 val 0x00

[snip]

251908@1597626456.961641:serial_ioport_read read addr 0x00 val 0x00
251908@1597626456.961642:serial_ioport_read read addr 0x00 val 0x00
251908@1597626456.961644:serial_ioport_read read addr 0x00 val 0x00
251908@1597626456.961647:serial_ioport_write write addr 0x04 val 0x0b
251908@1597626456.961648:serial_ioport_read read addr 0x05 val 0x60
251908@1597626456.961684:serial_ioport_read read addr 0x05 val 0x60
251908@1597626456.961685:serial_ioport_read read addr 0x05 val 0x60

[snip]

251908@1597626457.045894:serial_ioport_read read addr 0x05 val 0x60
251908@1597626457.045895:serial_ioport_read read addr 0x05 val 0x60
251908@1597626457.045897:serial_ioport_read read addr 0x05 val 0x60
251908@1597626457.045932:serial_ioport_read read addr 0x00 val 0x00

The write of 0x01 and then 0x0b to reg 0x04 is the guest turning the RTS
line off then on.  A real mouse will respond to this by sending 0x4d,
which is how the guest detects the mouse.

Reproducible in current stable-4.2 and 5.0 (debian's build).  I am able
to get the guest to use a real passed-through serial mouse (with a minor
hack, separate bug filed for this)

** Affects: qemu
 Importance: Undecided
 Status: New


** Tags: msmouse

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

Title:
  msmouse serial mouse emulation broken? No id byte sent on reset

Status in QEMU:
  New

Bug description:
  I took a shot at getting Windows 1.01 working.  It doesn't support a
  PS/2 mouse out-of-the-box but does support MS serial mice.  It doesn't
  seem to detect qemu's emulated msmouse.

  When I run this command:

  > qemu-system-i386 -nodefaults -hda my_windows1_hd.qcow2 -vga std
  -serial msmouse -trace enable='serial*'  -icount shift=10,align=on

  I get this output (edited):

  251908@1597626456.800452:serial_ioport_write write addr 0x04 val 0x01
  251908@1597626456.800460:serial_ioport_read read addr 0x00 val 0x00
  251908@1597626456.800462:serial_ioport_read read addr 0x00 val 0x00

  [snip]

  251908@1597626456.961641:serial_ioport_read read addr 0x00 val 0x00
  251908@1597626456.961642:serial_ioport_read read addr 0x00 val 0x00
  251908@1597626456.961644:serial_ioport_read read addr 0x00 val 0x00
  251908@1597626456.961647:serial_ioport_write write addr 0x04 val 0x0b
  251908@1597626456.961648:serial_ioport_read read addr 0x05 val 0x60
  251908@1597626456.961684:serial_ioport_read read addr 0x05 val 0x60
  251908@1597626456.961685:serial_ioport_read read addr 0x05 val 0x60

  [snip]

  251908@1597626457.045894:serial_ioport_read read addr 0x05 val 0x60
  251908@1597626457.045895:serial_ioport_read read addr 0x05 val 0x60
  251908@1597626457.045897:serial_ioport_read read addr 0x05 val 0x60
  251908@1597626457.045932:serial_ioport_read read addr 0x00 val 0x00

  The write of 0x01 and then 0x0b to reg 0x04 is the guest turning the
  RTS line off then on.  A real mouse will respond to this by sending
  0x4d, which is how the guest detects the mouse.

  Reproducible in current stable-4.2 and 5.0 (debian's build).  I am
  able to get the guest to use a real passed-through serial mouse (with
  a minor hack, separate bug filed for this)

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

[Bug 1891829] Re: High bits(s) sometimes set high on rcvd serial bytes when char size < 8 bits

[Michael Slade]

I will hopefully submit a patch for review soon.

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

Title:
  High bits(s) sometimes set high on rcvd serial bytes when char size <
  8 bits

Status in QEMU:
  New

Bug description:
  I *believe* (not confirmed) that the old standard PC serial ports,
  when configured with a character size of 7 bits or less, should set
  non-data bits to 0 when the CPU reads received chars from the read
  register.  qemu doesn't do this.

  Windows 1.01 will not make use of a serial mouse when bit 7 is 1.  The
  ID byte that the mouse sends on reset is ignored.  I added a temporary
  hack to set bit 7 to 0 on all incoming bytes, and this convinced
  windows 1.01 to use the mouse.

  note 1:  This was using a real serial mouse through a passed-through
  serial port.  The emulated msmouse doesn't work for other reasons.

  note 2:  The USB serial port I am passing through to the guest sets
  non-data bits to 1.  Not sure if this is the USB hardware or linux.

  note 3:  I also needed to add an -icount line to slow down the guest
  CPU, so that certain cpu-sensitive timing code in the guest didn't
  give up too quickly.

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

Re: [PATCH v4] qapi/opts-visitor: Fixed fallthrough compiler warning

[Rohit Shinde]

On Sun, Aug 16, 2020 at 6:52 PM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 8/16/20 9:55 AM, Rohit Shinde wrote:
> > Hey Richard,
> >
> >  1. So I should fork off the master again?
>
> Yes.
>
> There need to be special circumstances for not posting a patch set
> relative to
> master, and even then your cover letter would need to detail against what
> base
> the patch set applies.
>
> >  2. I just checked and my version of the code doesn't contain that line,
> so I
> > am unsure on how that line got there. I was trying to fix the
> compiler
> > warnings. Could you please guide me on how I create the next version
> of a
> > patch?
>
> That makes no sense at all.  The line was added at the same time as the
> code
> above it, in d8754f40acb.
>
I misread the comment. The comment /* fallthrough */ was meant to stop the
compiler warning from occurring. I am trying to complete the bite sized
task mentioned here  under
"Compiler driven cleanups". I wanted to take that up to get more familiar
with the codebase.

>
> There should be nothing to fix.
>
>
> r~
>

[Bug 1891829] [NEW] High bits(s) sometimes set high on rcvd serial bytes when char size < 8 bits

[Michael Slade]

Public bug reported:

I *believe* (not confirmed) that the old standard PC serial ports, when
configured with a character size of 7 bits or less, should set non-data
bits to 0 when the CPU reads received chars from the read register.
qemu doesn't do this.

Windows 1.01 will not make use of a serial mouse when bit 7 is 1.  The
ID byte that the mouse sends on reset is ignored.  I added a temporary
hack to set bit 7 to 0 on all incoming bytes, and this convinced windows
1.01 to use the mouse.

note 1:  This was using a real serial mouse through a passed-through
serial port.  The emulated msmouse doesn't work for other reasons.

note 2:  The USB serial port I am passing through to the guest sets non-
data bits to 1.  Not sure if this is the USB hardware or linux.

note 3:  I also needed to add an -icount line to slow down the guest
CPU, so that certain cpu-sensitive timing code in the guest didn't give
up too quickly.

** Affects: qemu
 Importance: Undecided
 Status: New


** Tags: serial

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

Title:
  High bits(s) sometimes set high on rcvd serial bytes when char size <
  8 bits

Status in QEMU:
  New

Bug description:
  I *believe* (not confirmed) that the old standard PC serial ports,
  when configured with a character size of 7 bits or less, should set
  non-data bits to 0 when the CPU reads received chars from the read
  register.  qemu doesn't do this.

  Windows 1.01 will not make use of a serial mouse when bit 7 is 1.  The
  ID byte that the mouse sends on reset is ignored.  I added a temporary
  hack to set bit 7 to 0 on all incoming bytes, and this convinced
  windows 1.01 to use the mouse.

  note 1:  This was using a real serial mouse through a passed-through
  serial port.  The emulated msmouse doesn't work for other reasons.

  note 2:  The USB serial port I am passing through to the guest sets
  non-data bits to 1.  Not sure if this is the USB hardware or linux.

  note 3:  I also needed to add an -icount line to slow down the guest
  CPU, so that certain cpu-sensitive timing code in the guest didn't
  give up too quickly.

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

Re: [PATCH v4] qapi/opts-visitor: Fixed fallthrough compiler warning

[Richard Henderson]

On 8/16/20 9:55 AM, Rohit Shinde wrote:
> Hey Richard,
> 
>  1. So I should fork off the master again?

Yes.

There need to be special circumstances for not posting a patch set relative to
master, and even then your cover letter would need to detail against what base
the patch set applies.

>  2. I just checked and my version of the code doesn't contain that line, so I
> am unsure on how that line got there. I was trying to fix the compiler
> warnings. Could you please guide me on how I create the next version of a
> patch?

That makes no sense at all.  The line was added at the same time as the code
above it, in d8754f40acb.

There should be nothing to fix.


r~

Re: [PATCH] fixed proc myself (linux user) for musl

[Андрей Аладьев]

>From 738f6ec4598b4618acd7ecbd11e5d250a82f28b7 Mon Sep 17 00:00:00 2001
From: Andrew Aladjev 
Date: Mon, 17 Aug 2020 01:20:03 +0300
Subject: [PATCH] fixed proc myself (linux user) for musl

Signed-off-by: Andrew Aladjev 
---
 linux-user/Makefile.objs  |   5 +-
 linux-user/elfload.c  |   7 ++-
 linux-user/exit.c |   7 ++-
 linux-user/main.c |   2 +-
 linux-user/qemu.h |   1 +
 linux-user/syscall.c  |  63 ---
 linux-user/syscall_proc.c | 102 ++
 linux-user/syscall_proc.h |   8 +++
 8 files changed, 146 insertions(+), 49 deletions(-)
 create mode 100644 linux-user/syscall_proc.c
 create mode 100644 linux-user/syscall_proc.h

diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs
index 1940910a73..ad84380738 100644
--- a/linux-user/Makefile.objs
+++ b/linux-user/Makefile.objs
@@ -1,7 +1,8 @@
 obj-y = main.o syscall.o strace.o mmap.o signal.o \
  elfload.o linuxload.o uaccess.o uname.o \
- safe-syscall.o $(TARGET_ABI_DIR)/signal.o \
-$(TARGET_ABI_DIR)/cpu_loop.o exit.o fd-trans.o
+ safe-syscall.o syscall_proc.o \
+ $(TARGET_ABI_DIR)/cpu_loop.o $(TARGET_ABI_DIR)/signal.o \
+ exit.o fd-trans.o

 obj-$(TARGET_HAS_BFLT) += flatload.o
 obj-$(TARGET_I386) += vm86.o
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 619c054cc4..201db61d91 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2319,7 +2319,10 @@ exit_errmsg:
buffer is sufficiently aligned to present no problems to the host
in accessing data at aligned offsets within the buffer.

-   On return: INFO values will be filled in, as necessary or available.  */
+   On return: INFO values will be filled in, as necessary or available.
+
+   WARNING: this function won't close "image_fd".
+*/

 static void load_elf_image(const char *image_name, int image_fd,
struct image_info *info, char **pinterp_name,
@@ -2576,7 +2579,6 @@ static void load_elf_image(const char *image_name,
int image_fd,

 mmap_unlock();

-close(image_fd);
 return;

  exit_read:
@@ -2610,6 +2612,7 @@ static void load_elf_interp(const char *filename,
struct image_info *info,
 }

 load_elf_image(filename, fd, info, NULL, bprm_buf);
+close(fd);
 return;

  exit_perror:
diff --git a/linux-user/exit.c b/linux-user/exit.c
index 1594015444..f0626fc432 100644
--- a/linux-user/exit.c
+++ b/linux-user/exit.c
@@ -28,12 +28,15 @@ extern void __gcov_dump(void);

 void preexit_cleanup(CPUArchState *env, int code)
 {
+close(execfd);
+
 #ifdef CONFIG_GPROF
 _mcleanup();
 #endif
 #ifdef CONFIG_GCOV
 __gcov_dump();
 #endif
-gdb_exit(env, code);
-qemu_plugin_atexit_cb();
+
+gdb_exit(env, code);
+qemu_plugin_atexit_cb();
 }
diff --git a/linux-user/main.c b/linux-user/main.c
index 22578b1633..9cc6c1e6da 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -48,6 +48,7 @@
 #include "crypto/init.h"

 char *exec_path;
+int execfd;

 int singlestep;
 static const char *argv0;
@@ -628,7 +629,6 @@ int main(int argc, char **argv, char **envp)
 int target_argc;
 int i;
 int ret;
-int execfd;
 int log_mask;
 unsigned long max_reserved_va;

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 792c74290f..d822f2b9df 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -156,6 +156,7 @@ typedef struct TaskState {
 } __attribute__((aligned(16))) TaskState;

 extern char *exec_path;
+extern int execfd;
 void init_task_state(TaskState *ts);
 void task_settid(TaskState *);
 void stop_all_tasks(void);
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 05f03919ff..af86385281 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -122,6 +122,7 @@
 #include "qapi/error.h"
 #include "fd-trans.h"
 #include "tcg/tcg.h"
+#include "syscall_proc.h"

 #ifndef CLONE_IO
 #define CLONE_IO0x8000  /* Clone io context */
@@ -7353,38 +7354,6 @@ static int open_self_auxv(void *cpu_env, int fd)
 return 0;
 }

-static int is_proc_myself(const char *filename, const char *entry)
-{
-if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
-filename += strlen("/proc/");
-if (!strncmp(filename, "self/", strlen("self/"))) {
-filename += strlen("self/");
-} else if (*filename >= '1' && *filename <= '9') {
-char myself[80];
-snprintf(myself, sizeof(myself), "%d/", getpid());
-if (!strncmp(filename, myself, strlen(myself))) {
-filename += strlen(myself);
-} else {
-return 0;
-}
-} else {
-return 0;
-}
-if (!strcmp(filename, entry)) {
-return 1;
-}
-}
-return 0;
-}
-
-#if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN) || \
-defined(TARGET_SPARC) || defined(TARGET_M68K)
-static int is_proc(const char *filename, const char *entry)
-{
-  

Guest Physical addresses tracking in KVM mode

[Eltahawy, Mahmoud]

Hi,

I want to ask if it is possible to monitor the Guest Physical addresses inside 
QEMU code while running with KVM mode.
Appreciate your help

Thanks
Mahmoud

Re: [PATCH] fixed proc myself (linux user) for musl

[Philippe Mathieu-Daudé]

Hi Andrew,

Cc'ing the maintainer & Richard.

./scripts/get_maintainer.pl -f linux-user/main.c
Laurent Vivier  (maintainer:Linux user)

On 8/16/20 9:58 PM, Андрей Аладьев wrote:
> From: Andrew Aladjev  >
> Date: Sun, 16 Aug 2020 22:50:13 +0300
> Subject: [PATCH] fixed proc myself (linux user) for musl

Your patch has double subject headers...

This link might be helpful:
https://wiki.qemu.org/Contribute/SubmitAPatch#Use_git_format-patch

> Buglink: https://bugs.gentoo.org/587230
> Signed-off-by: Andrew Aladjev  >

Hmmm did you paste the output of git-format-patch in your
email client?

> ---
>  linux-user/Makefile.objs  |  5 +-
>  linux-user/elfload.c      |  7 ++-
>  linux-user/exit.c         |  7 ++-
>  linux-user/main.c         |  2 +-
>  linux-user/qemu.h         |  1 +
>  linux-user/syscall.c      | 85 +-
>  linux-user/syscall_proc.c | 96 +++
>  linux-user/syscall_proc.h |  8 
>  8 files changed, 150 insertions(+), 61 deletions(-)
>  create mode 100644 linux-user/syscall_proc.c
>  create mode 100644 linux-user/syscall_proc.h
> 
> diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs
> index 1940910a73..ad84380738 100644
> --- a/linux-user/Makefile.objs
> +++ b/linux-user/Makefile.objs
> @@ -1,7 +1,8 @@
>  obj-y = main.o syscall.o strace.o mmap.o signal.o \
>   elfload.o linuxload.o uaccess.o uname.o \
> - safe-syscall.o $(TARGET_ABI_DIR)/signal.o \
> -        $(TARGET_ABI_DIR)/cpu_loop.o exit.o fd-trans.o
> + safe-syscall.o syscall_proc.o \
> + $(TARGET_ABI_DIR)/cpu_loop.o $(TARGET_ABI_DIR)/signal.o \
> + exit.o fd-trans.o
>  
>  obj-$(TARGET_HAS_BFLT) += flatload.o
>  obj-$(TARGET_I386) += vm86.o
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 619c054cc4..201db61d91 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2319,7 +2319,10 @@ exit_errmsg:
>     buffer is sufficiently aligned to present no problems to the host
>     in accessing data at aligned offsets within the buffer.
>  
> -   On return: INFO values will be filled in, as necessary or available.  */
> +   On return: INFO values will be filled in, as necessary or available.
> +
> +   WARNING: this function won't close "image_fd".
> +*/
>  
>  static void load_elf_image(const char *image_name, int image_fd,
>                             struct image_info *info, char **pinterp_name,
> @@ -2576,7 +2579,6 @@ static void load_elf_image(const char *image_name,
> int image_fd,
>  
>      mmap_unlock();
>  
> -    close(image_fd);
>      return;
>  
>   exit_read:
> @@ -2610,6 +2612,7 @@ static void load_elf_interp(const char *filename,
> struct image_info *info,
>      }
>  
>      load_elf_image(filename, fd, info, NULL, bprm_buf);
> +    close(fd);
>      return;
>  
>   exit_perror:
> diff --git a/linux-user/exit.c b/linux-user/exit.c
> index 1594015444..f0626fc432 100644
> --- a/linux-user/exit.c
> +++ b/linux-user/exit.c
> @@ -28,12 +28,15 @@ extern void __gcov_dump(void);
>  
>  void preexit_cleanup(CPUArchState *env, int code)
>  {
> +    close(execfd);
> +
>  #ifdef CONFIG_GPROF
>          _mcleanup();
>  #endif
>  #ifdef CONFIG_GCOV
>          __gcov_dump();
>  #endif
> -        gdb_exit(env, code);
> -        qemu_plugin_atexit_cb();
> +
> +    gdb_exit(env, code);
> +    qemu_plugin_atexit_cb();
>  }
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 22578b1633..9cc6c1e6da 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -48,6 +48,7 @@
>  #include "crypto/init.h"
>  
>  char *exec_path;
> +int execfd;
>  
>  int singlestep;
>  static const char *argv0;
> @@ -628,7 +629,6 @@ int main(int argc, char **argv, char **envp)
>      int target_argc;
>      int i;
>      int ret;
> -    int execfd;
>      int log_mask;
>      unsigned long max_reserved_va;
>  
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 792c74290f..d822f2b9df 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -156,6 +156,7 @@ typedef struct TaskState {
>  } __attribute__((aligned(16))) TaskState;
>  
>  extern char *exec_path;
> +extern int execfd;
>  void init_task_state(TaskState *ts);
>  void task_settid(TaskState *);
>  void stop_all_tasks(void);
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 05f03919ff..483a735c1a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -122,6 +122,7 @@
>  #include "qapi/error.h"
>  #include "fd-trans.h"
>  #include "tcg/tcg.h"
> +#include "syscall_proc.h"
>  
>  #ifndef CLONE_IO
>  #define CLONE_IO                0x8000      /* Clone io context */
> @@ -1104,7 +1105,7 @@ static inline rlim_t target_to_host_rlim(abi_ulong
> target_rlim)
>  {
>      abi_ulong target_rlim_swap;
>      rlim_t result;
> -    
> +
>      target_rlim_swap = tswapal(target_rlim);
>      if (target_rlim_swap == TARGET_RLIM_INFINITY)
>          return RLIM_INFINITY;
> @@ -1112,7 +1113,7 @@ 

[PATCH] fixed proc myself (linux user) for musl

[Андрей Аладьев]

From: Andrew Aladjev 
Date: Sun, 16 Aug 2020 22:50:13 +0300
Subject: [PATCH] fixed proc myself (linux user) for musl
Buglink: https://bugs.gentoo.org/587230
Signed-off-by: Andrew Aladjev 
---
 linux-user/Makefile.objs  |  5 +-
 linux-user/elfload.c  |  7 ++-
 linux-user/exit.c |  7 ++-
 linux-user/main.c |  2 +-
 linux-user/qemu.h |  1 +
 linux-user/syscall.c  | 85 +-
 linux-user/syscall_proc.c | 96 +++
 linux-user/syscall_proc.h |  8 
 8 files changed, 150 insertions(+), 61 deletions(-)
 create mode 100644 linux-user/syscall_proc.c
 create mode 100644 linux-user/syscall_proc.h

diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs
index 1940910a73..ad84380738 100644
--- a/linux-user/Makefile.objs
+++ b/linux-user/Makefile.objs
@@ -1,7 +1,8 @@
 obj-y = main.o syscall.o strace.o mmap.o signal.o \
  elfload.o linuxload.o uaccess.o uname.o \
- safe-syscall.o $(TARGET_ABI_DIR)/signal.o \
-$(TARGET_ABI_DIR)/cpu_loop.o exit.o fd-trans.o
+ safe-syscall.o syscall_proc.o \
+ $(TARGET_ABI_DIR)/cpu_loop.o $(TARGET_ABI_DIR)/signal.o \
+ exit.o fd-trans.o

 obj-$(TARGET_HAS_BFLT) += flatload.o
 obj-$(TARGET_I386) += vm86.o
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 619c054cc4..201db61d91 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2319,7 +2319,10 @@ exit_errmsg:
buffer is sufficiently aligned to present no problems to the host
in accessing data at aligned offsets within the buffer.

-   On return: INFO values will be filled in, as necessary or available.  */
+   On return: INFO values will be filled in, as necessary or available.
+
+   WARNING: this function won't close "image_fd".
+*/

 static void load_elf_image(const char *image_name, int image_fd,
struct image_info *info, char **pinterp_name,
@@ -2576,7 +2579,6 @@ static void load_elf_image(const char *image_name,
int image_fd,

 mmap_unlock();

-close(image_fd);
 return;

  exit_read:
@@ -2610,6 +2612,7 @@ static void load_elf_interp(const char *filename,
struct image_info *info,
 }

 load_elf_image(filename, fd, info, NULL, bprm_buf);
+close(fd);
 return;

  exit_perror:
diff --git a/linux-user/exit.c b/linux-user/exit.c
index 1594015444..f0626fc432 100644
--- a/linux-user/exit.c
+++ b/linux-user/exit.c
@@ -28,12 +28,15 @@ extern void __gcov_dump(void);

 void preexit_cleanup(CPUArchState *env, int code)
 {
+close(execfd);
+
 #ifdef CONFIG_GPROF
 _mcleanup();
 #endif
 #ifdef CONFIG_GCOV
 __gcov_dump();
 #endif
-gdb_exit(env, code);
-qemu_plugin_atexit_cb();
+
+gdb_exit(env, code);
+qemu_plugin_atexit_cb();
 }
diff --git a/linux-user/main.c b/linux-user/main.c
index 22578b1633..9cc6c1e6da 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -48,6 +48,7 @@
 #include "crypto/init.h"

 char *exec_path;
+int execfd;

 int singlestep;
 static const char *argv0;
@@ -628,7 +629,6 @@ int main(int argc, char **argv, char **envp)
 int target_argc;
 int i;
 int ret;
-int execfd;
 int log_mask;
 unsigned long max_reserved_va;

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 792c74290f..d822f2b9df 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -156,6 +156,7 @@ typedef struct TaskState {
 } __attribute__((aligned(16))) TaskState;

 extern char *exec_path;
+extern int execfd;
 void init_task_state(TaskState *ts);
 void task_settid(TaskState *);
 void stop_all_tasks(void);
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 05f03919ff..483a735c1a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -122,6 +122,7 @@
 #include "qapi/error.h"
 #include "fd-trans.h"
 #include "tcg/tcg.h"
+#include "syscall_proc.h"

 #ifndef CLONE_IO
 #define CLONE_IO0x8000  /* Clone io context */
@@ -1104,7 +1105,7 @@ static inline rlim_t target_to_host_rlim(abi_ulong
target_rlim)
 {
 abi_ulong target_rlim_swap;
 rlim_t result;
-
+
 target_rlim_swap = tswapal(target_rlim);
 if (target_rlim_swap == TARGET_RLIM_INFINITY)
 return RLIM_INFINITY;
@@ -1112,7 +1113,7 @@ static inline rlim_t target_to_host_rlim(abi_ulong
target_rlim)
 result = target_rlim_swap;
 if (target_rlim_swap != (rlim_t)result)
 return RLIM_INFINITY;
-
+
 return result;
 }
 #endif
@@ -1122,13 +1123,13 @@ static inline abi_ulong host_to_target_rlim(rlim_t
rlim)
 {
 abi_ulong target_rlim_swap;
 abi_ulong result;
-
+
 if (rlim == RLIM_INFINITY || rlim != (abi_long)rlim)
 target_rlim_swap = TARGET_RLIM_INFINITY;
 else
 target_rlim_swap = rlim;
 result = tswapal(target_rlim_swap);
-
+
 return result;
 }
 #endif
@@ -1615,9 +1616,9 @@ static inline abi_long target_to_host_cmsg(struct
msghdr *msgh,
 abi_ulong target_cmsg_addr;
 struct target_cmsghdr *target_cmsg, *target_cmsg_start;
 

[PATCH] memory: Initialize MemoryRegionOps for RAM memory regions

[Philippe Mathieu-Daudé]

There is an issue when using memory_region_dispatch_read() or
memory_region_dispatch_write() on RAM memory regions.

RAM memory regions are initialized as:

  memory_region_init_ram()
  -> memory_region_init_ram_nomigrate()
 -> memory_region_init_ram_shared_nomigrate()
-> memory_region_init()
   -> object_initialize(TYPE_MEMORY_REGION)
  -> memory_region_initfn()
 -> mr->ops = _mem_ops;

Later when accessing the alias, the memory_region_dispatch_read()
flow is:

  memory_region_dispatch_read()
  -> memory_region_dispatch_read1()
 -> if (mr->ops->read) { ... }
   ^^
   NULL deref as unassigned_mem_ops.read is NULL.

  memory_region_dispatch_write()
  -> if (mr->ops->write) { ... }
^^^
NULL deref as unassigned_mem_ops.read is NULL.

Fix by initializing the MemoryRegionOps to ram_device_mem_ops,
this way the memory accesses are properly dispatched using
memory_region_ram_device_read() / memory_region_ram_device_write().

Fixes: 4a2e242bbb ("memory: Don't use memcpy for ram_device regions")
Signed-off-by: Philippe Mathieu-Daudé 
---
 softmmu/memory.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index af25987518..2fce3fef2d 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1509,6 +1509,8 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion 
*mr,
 Error *err = NULL;
 memory_region_init(mr, owner, name, size);
 mr->ram = true;
+mr->ops = _device_mem_ops;
+mr->opaque = mr;
 mr->terminates = true;
 mr->destructor = memory_region_destructor_ram;
 mr->ram_block = qemu_ram_alloc(size, share, mr, );
@@ -1533,6 +1535,8 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
 Error *err = NULL;
 memory_region_init(mr, owner, name, size);
 mr->ram = true;
+mr->ops = _device_mem_ops;
+mr->opaque = mr;
 mr->terminates = true;
 mr->destructor = memory_region_destructor_ram;
 mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized,
@@ -1558,6 +1562,8 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
 Error *err = NULL;
 memory_region_init(mr, owner, name, size);
 mr->ram = true;
+mr->ops = _device_mem_ops;
+mr->opaque = mr;
 mr->terminates = true;
 mr->destructor = memory_region_destructor_ram;
 mr->align = align;
@@ -1581,6 +1587,8 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
 Error *err = NULL;
 memory_region_init(mr, owner, name, size);
 mr->ram = true;
+mr->ops = _device_mem_ops;
+mr->opaque = mr;
 mr->terminates = true;
 mr->destructor = memory_region_destructor_ram;
 mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
@@ -1603,6 +1611,8 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
 {
 memory_region_init(mr, owner, name, size);
 mr->ram = true;
+mr->ops = _device_mem_ops;
+mr->opaque = mr;
 mr->terminates = true;
 mr->destructor = memory_region_destructor_ram;
 mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
-- 
2.21.3

[PATCH] trace/simple: Enable tracing on startup only if the user specifies a trace option

[duboisj]

From: Josh DuBois 

Tracing can be enabled at the command line or via the
monitor. Command-line trace options are recorded during
trace_opt_parse(), but tracing is not enabled until the various
front-ends later call trace_init_file(). If the user passes a trace
option on the command-line, remember that and enable tracing during
trace_init_file().  Otherwise, trace_init_file() should record the
trace file specified by the frontend and avoid enabling traces
until the user requests them via the monitor.

This fixes 1b7157be3a8c4300fc8044d40f4b2e64a152a1b4 and also
db25d56c014aa1a96319c663e0a60346a223b31e, by allowing the user
to enable traces on the command line and also avoiding
unwanted trace- files when the user has not asked for them.

Fixes: 1b7157be3a8c4300fc8044d40f4b2e64a152a1b4
Signed-off-by: Josh DuBois 
---
 trace/control.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/trace/control.c b/trace/control.c
index 6558b5c906..8f94f09444 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -39,6 +39,7 @@ static TraceEventGroup *event_groups;
 static size_t nevent_groups;
 static uint32_t next_id;
 static uint32_t next_vcpu_id;
+static bool init_trace_on_startup;
 
 QemuOptsList qemu_trace_opts = {
 .name = "trace",
@@ -225,7 +226,9 @@ void trace_init_file(const char *file)
 {
 #ifdef CONFIG_TRACE_SIMPLE
 st_set_trace_file(file);
-st_set_trace_file_enabled(true);
+if (init_trace_on_startup) {
+st_set_trace_file_enabled(true);
+}
 #elif defined CONFIG_TRACE_LOG
 /*
  * If both the simple and the log backends are enabled, "--trace file"
@@ -299,6 +302,7 @@ char *trace_opt_parse(const char *optarg)
 }
 trace_init_events(qemu_opt_get(opts, "events"));
 trace_file = g_strdup(qemu_opt_get(opts, "file"));
+init_trace_on_startup = true;
 qemu_opts_del(opts);
 
 return trace_file;
-- 
2.25.1

[PATCH] memory: Directly dispatch alias accesses on origin memory region

[Philippe Mathieu-Daudé]

There is an issue when accessing an alias memory region via the
memory_region_dispatch_read() / memory_region_dispatch_write()
calls:

The memory_region_init_alias() flow is:

  memory_region_init_alias()
  -> memory_region_init()
 -> object_initialize(TYPE_MEMORY_REGION)
-> memory_region_initfn()
   -> mr->ops = _mem_ops;

Later when accessing the alias, the memory_region_dispatch_read()
flow is:

  memory_region_dispatch_read()
  -> memory_region_access_valid(mr)
 -> mr->ops->valid.accepts()
-> unassigned_mem_accepts()
<- false
 <- false
   <- MEMTX_DECODE_ERROR

The caller gets a MEMTX_DECODE_ERROR while the access is OK.

Fix by directly dispatching aliases accesses to its origin region.

Fixes: 2cdfcf272d ("memory: assign MemoryRegionOps to all regions")
Signed-off-by: Philippe Mathieu-Daudé 
---
 softmmu/memory.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index af25987518..651705b7d1 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1405,6 +1405,10 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
 unsigned size = memop_size(op);
 MemTxResult r;
 
+if (mr->alias) {
+addr += mr->alias_offset;
+mr = mr->alias;
+}
 if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
 *pval = unassigned_mem_read(mr, addr, size);
 return MEMTX_DECODE_ERROR;
@@ -1449,6 +1453,10 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
*mr,
 {
 unsigned size = memop_size(op);
 
+if (mr->alias) {
+addr += mr->alias_offset;
+mr = mr->alias;
+}
 if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
 unassigned_mem_write(mr, addr, data, size);
 return MEMTX_DECODE_ERROR;
-- 
2.21.3

Re: [PATCH v4] qapi/opts-visitor: Fixed fallthrough compiler warning

[Rohit Shinde]

Hey Richard,


   1. So I should fork off the master again? I am a bit unclear on the
   workflow, since this is my first doing patches via format-patch and
   send-email so I am making mistakes.
   2. I just checked and my version of the code doesn't contain that line,
   so I am unsure on how that line got there. I was trying to fix the compiler
   warnings. Could you please guide me on how I create the next version of a
   patch?

Thanks,
Rohit.

On Sun, Aug 16, 2020 at 12:03 PM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 8/15/20 7:31 PM, Rohit Shinde wrote:
> >  /* range has been completed, fall through in order to pop
> option */
> > -__attribute__((fallthrough));
> > +/* fallthrough */
>
> (1) Any patch should not be relative to your own v3.
> (2) The previous line already contains the words "fall through",
> so what is it that you are trying to fix?
>
>
> r~
>

Re: [PATCH 14/18] hw/net: cadence_gem: Add a new 'phy-addr' property

[Philippe Mathieu-Daudé]

On 8/16/20 3:42 PM, Bin Meng wrote:
> On Sun, Aug 16, 2020 at 8:08 PM Nathan Rossi  wrote:
>>
>> On Sun, 16 Aug 2020 at 18:29, Bin Meng  wrote:
>>>
>>> On Sat, Aug 15, 2020 at 5:06 PM Philippe Mathieu-Daudé  
>>> wrote:

 On 8/14/20 6:40 PM, Bin Meng wrote:
> From: Bin Meng 
>
> At present the PHY address of the PHY connected to GEM is hard-coded
> to either 23 (BOARD_PHY_ADDRESS) or 0. This might not be the case for
> all boards. Add a new 'phy-addr' property so that board can specify
> the PHY address for each GEM instance.
>
> Signed-off-by: Bin Meng 
> ---
>
>  hw/net/cadence_gem.c | 7 +--
>  include/hw/net/cadence_gem.h | 2 ++
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index a93b5c0..9fa03de 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -1446,7 +1446,8 @@ static uint64_t gem_read(void *opaque, hwaddr 
> offset, unsigned size)
>  uint32_t phy_addr, reg_num;
>
>  phy_addr = (retval & GEM_PHYMNTNC_ADDR) >> 
> GEM_PHYMNTNC_ADDR_SHFT;
> -if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
> +if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
> +phy_addr == s->phy_addr) {
>  reg_num = (retval & GEM_PHYMNTNC_REG) >> 
> GEM_PHYMNTNC_REG_SHIFT;
>  retval &= 0x;
>  retval |= gem_phy_read(s, reg_num);
> @@ -1569,7 +1570,8 @@ static void gem_write(void *opaque, hwaddr offset, 
> uint64_t val,
>  uint32_t phy_addr, reg_num;
>
>  phy_addr = (val & GEM_PHYMNTNC_ADDR) >> 
> GEM_PHYMNTNC_ADDR_SHFT;
> -if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
> +if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
> +phy_addr == s->phy_addr) {
>  reg_num = (val & GEM_PHYMNTNC_REG) >> 
> GEM_PHYMNTNC_REG_SHIFT;
>  gem_phy_write(s, reg_num, val);
>  }
> @@ -1682,6 +1684,7 @@ static Property gem_properties[] = {
>  DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
>  DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
> GEM_MODID_VALUE),
> +DEFINE_PROP_UINT8("phy-addr", CadenceGEMState, phy_addr, 0),

 This patch would be complete by moving the BOARD_PHY_ADDRESS definition
 to each board using this NIC, and setting the "phy-addr" property to
 this value.
>>>
>>> I actually have no idea which board in QEMU is using this special PHY
>>> address instead of default 0.
>>
>> Given Xilinx's QEMU fork has not used this value for quite some time,
>> I suspect it was only used to match an early chip emulation
>> platform/board.
>>
>> https://github.com/Xilinx/qemu/blame/master/hw/net/cadence_gem.c#L248
>>
>>>
>>> It looks BOARD_PHY_ADDRESS has been there since the initial version of
>>> the cadence_gem model.
>>>
>>> commit e9f186e514a70557d695cadd2c2287ef97737023
>>> Author: Peter A. G. Crosthwaite 
>>> Date:   Mon Mar 5 14:39:12 2012 +1000
>>>
>>> cadence_gem: initial version of device model
>>>
>>> Device model for cadence gem ethernet controller.
>>>
>>> Signed-off-by: Peter A. G. Crosthwaite 
>>> Signed-off-by: John Linn 
>>> Acked-by: Edgar E. Iglesias 
>>> Signed-off-by: Edgar E. Iglesias 
>>>
>>> Later PHY address 0 was added via the following commit:
>>>
>>> commit 553893736885e4f2dda424bff3e2200e1b6482a5
>>> Author: Peter Crosthwaite 
>>> Date:   Thu Apr 3 23:55:19 2014 -0700
>>>
>>> net: cadence_gem: Make phy respond to broadcast
>>>
>>> Phys must respond to address 0 by specification. Implement.
>>>
>>> Signed-off-by: Nathan Rossi 
>>> Signed-off-by: Peter Crosthwaite 
>>> Message-id:
>>> 6f4d53b04ddbfb19895bfb61a595e69f1c08859a.1396594056.git.peter.crosthwa...@xilinx.com
>>> Reviewed-by: Beniamino Galvani 
>>> Signed-off-by: Peter Maydell 
>>>
>>> I doubt the commit message said that PHYs must respond to address 0. I
>>> am not aware of such specs. The issue was probably that whatever board
>>> 2nd commit was tested against did not have a PHY at address
>>> BOARD_PHY_ADDRESS.
>>
>> It is common for phy devices to support it as a broadcast address. It
>> is also commonly written in Xilinx documentation that it is treated as
>> a broadcast address. e.g. the axi ethernet core
>> (https://www.xilinx.com/support/documentation/ip_documentation/axi_ethernet/v7_0/pg138-axi-ethernet.pdf
>> page 45). But 802.3 does not require it, instead address 0 is only
>> reserved.
>>
>> With this commit the "must" refers to the device specification, in
>> that QEMU is emulating a real phy (though more specifically the phy(s)
>> that were being emulated for Zynq boards) that does respond to address
>> 0 

Re: [PATCH v4 0/9] memory: assert and define MemoryRegionOps callbacks

[Philippe Mathieu-Daudé]

On 8/11/20 1:41 PM, P J P wrote:
> From: Prasad J Pandit 
> 
> Hello,
> 
> * This series asserts that MemoryRegionOps objects define read/write
>   callback methods. Thus avoids potential NULL pointer dereference.
>   ex. -> 
> https://git.qemu.org/?p=qemu.git;a=commit;h=bb15013ef34617eb1344f5276292cadd326c21b2
> 
> * Also adds various undefined MemoryRegionOps read/write functions
>   to avoid potential assert failure.

What about read_with_attrs()/write_with_attrs()?
It seems they are part of the same problem.

> 
> Thank you.
> --
> Prasad J Pandit (9):
>   hw/pci-host: add pci-intack write method
>   pci-host: designware: add pcie-msi read method
>   vfio: add quirk device write method
>   prep: add ppc-parity write method
>   nvram: add nrf51_soc flash read method
>   spapr_pci: add spapr msi read method
>   tz-ppc: add dummy read/write methods
>   imx7-ccm: add digprog mmio write method
>   memory: assert MemoryRegionOps callbacks are defined
> 
>  hw/misc/imx7_ccm.c   |  8 
>  hw/misc/tz-ppc.c | 14 ++
>  hw/nvram/nrf51_nvm.c | 10 ++
>  hw/pci-host/designware.c | 19 +++
>  hw/pci-host/prep.c   |  8 
>  hw/ppc/prep_systemio.c   |  8 
>  hw/ppc/spapr_pci.c   | 14 --
>  hw/vfio/pci-quirks.c |  8 
>  softmmu/memory.c | 10 +-
>  9 files changed, 96 insertions(+), 3 deletions(-)
> 
> --
> 2.26.2
> 

Re: [PATCH v4] qapi/opts-visitor: Fixed fallthrough compiler warning

[Richard Henderson]

On 8/15/20 7:31 PM, Rohit Shinde wrote:
>  /* range has been completed, fall through in order to pop option */
> -__attribute__((fallthrough));
> +/* fallthrough */

(1) Any patch should not be relative to your own v3.
(2) The previous line already contains the words "fall through",
so what is it that you are trying to fix?


r~

[PATCH v2] virtio-mem: detach the element from the virtqueue when error occurs

[Li Qiang]

If error occurs while processing the virtio request we should call
'virtqueue_detach_element' to detach the element from the virtqueue
before free the elem.

Signed-off-by: Li Qiang 
---
Change since v1:
Change the subject
Avoid using the goto label

 hw/virtio/virtio-mem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 7740fc613f..e6ffc781b3 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -318,6 +318,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, 
VirtQueue *vq)
 if (iov_to_buf(elem->out_sg, elem->out_num, 0, , len) < len) {
 virtio_error(vdev, "virtio-mem protocol violation: invalid request"
  " size: %d", len);
+virtqueue_detach_element(vq, elem, 0);
 g_free(elem);
 return;
 }
@@ -327,6 +328,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, 
VirtQueue *vq)
 virtio_error(vdev, "virtio-mem protocol violation: not enough 
space"
  " for response: %zu",
  iov_size(elem->in_sg, elem->in_num));
+virtqueue_detach_element(vq, elem, 0);
 g_free(elem);
 return;
 }
@@ -348,6 +350,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, 
VirtQueue *vq)
 default:
 virtio_error(vdev, "virtio-mem protocol violation: unknown request"
  " type: %d", type);
+virtqueue_detach_element(vq, elem, 0);
 g_free(elem);
 return;
 }
-- 
2.17.1

Re: [PATCH 14/18] hw/net: cadence_gem: Add a new 'phy-addr' property

[Bin Meng]

On Sun, Aug 16, 2020 at 8:08 PM Nathan Rossi  wrote:
>
> On Sun, 16 Aug 2020 at 18:29, Bin Meng  wrote:
> >
> > On Sat, Aug 15, 2020 at 5:06 PM Philippe Mathieu-Daudé  
> > wrote:
> > >
> > > On 8/14/20 6:40 PM, Bin Meng wrote:
> > > > From: Bin Meng 
> > > >
> > > > At present the PHY address of the PHY connected to GEM is hard-coded
> > > > to either 23 (BOARD_PHY_ADDRESS) or 0. This might not be the case for
> > > > all boards. Add a new 'phy-addr' property so that board can specify
> > > > the PHY address for each GEM instance.
> > > >
> > > > Signed-off-by: Bin Meng 
> > > > ---
> > > >
> > > >  hw/net/cadence_gem.c | 7 +--
> > > >  include/hw/net/cadence_gem.h | 2 ++
> > > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > > > index a93b5c0..9fa03de 100644
> > > > --- a/hw/net/cadence_gem.c
> > > > +++ b/hw/net/cadence_gem.c
> > > > @@ -1446,7 +1446,8 @@ static uint64_t gem_read(void *opaque, hwaddr 
> > > > offset, unsigned size)
> > > >  uint32_t phy_addr, reg_num;
> > > >
> > > >  phy_addr = (retval & GEM_PHYMNTNC_ADDR) >> 
> > > > GEM_PHYMNTNC_ADDR_SHFT;
> > > > -if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
> > > > +if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
> > > > +phy_addr == s->phy_addr) {
> > > >  reg_num = (retval & GEM_PHYMNTNC_REG) >> 
> > > > GEM_PHYMNTNC_REG_SHIFT;
> > > >  retval &= 0x;
> > > >  retval |= gem_phy_read(s, reg_num);
> > > > @@ -1569,7 +1570,8 @@ static void gem_write(void *opaque, hwaddr 
> > > > offset, uint64_t val,
> > > >  uint32_t phy_addr, reg_num;
> > > >
> > > >  phy_addr = (val & GEM_PHYMNTNC_ADDR) >> 
> > > > GEM_PHYMNTNC_ADDR_SHFT;
> > > > -if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
> > > > +if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
> > > > +phy_addr == s->phy_addr) {
> > > >  reg_num = (val & GEM_PHYMNTNC_REG) >> 
> > > > GEM_PHYMNTNC_REG_SHIFT;
> > > >  gem_phy_write(s, reg_num, val);
> > > >  }
> > > > @@ -1682,6 +1684,7 @@ static Property gem_properties[] = {
> > > >  DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
> > > >  DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
> > > > GEM_MODID_VALUE),
> > > > +DEFINE_PROP_UINT8("phy-addr", CadenceGEMState, phy_addr, 0),
> > >
> > > This patch would be complete by moving the BOARD_PHY_ADDRESS definition
> > > to each board using this NIC, and setting the "phy-addr" property to
> > > this value.
> >
> > I actually have no idea which board in QEMU is using this special PHY
> > address instead of default 0.
>
> Given Xilinx's QEMU fork has not used this value for quite some time,
> I suspect it was only used to match an early chip emulation
> platform/board.
>
> https://github.com/Xilinx/qemu/blame/master/hw/net/cadence_gem.c#L248
>
> >
> > It looks BOARD_PHY_ADDRESS has been there since the initial version of
> > the cadence_gem model.
> >
> > commit e9f186e514a70557d695cadd2c2287ef97737023
> > Author: Peter A. G. Crosthwaite 
> > Date:   Mon Mar 5 14:39:12 2012 +1000
> >
> > cadence_gem: initial version of device model
> >
> > Device model for cadence gem ethernet controller.
> >
> > Signed-off-by: Peter A. G. Crosthwaite 
> > Signed-off-by: John Linn 
> > Acked-by: Edgar E. Iglesias 
> > Signed-off-by: Edgar E. Iglesias 
> >
> > Later PHY address 0 was added via the following commit:
> >
> > commit 553893736885e4f2dda424bff3e2200e1b6482a5
> > Author: Peter Crosthwaite 
> > Date:   Thu Apr 3 23:55:19 2014 -0700
> >
> > net: cadence_gem: Make phy respond to broadcast
> >
> > Phys must respond to address 0 by specification. Implement.
> >
> > Signed-off-by: Nathan Rossi 
> > Signed-off-by: Peter Crosthwaite 
> > Message-id:
> > 6f4d53b04ddbfb19895bfb61a595e69f1c08859a.1396594056.git.peter.crosthwa...@xilinx.com
> > Reviewed-by: Beniamino Galvani 
> > Signed-off-by: Peter Maydell 
> >
> > I doubt the commit message said that PHYs must respond to address 0. I
> > am not aware of such specs. The issue was probably that whatever board
> > 2nd commit was tested against did not have a PHY at address
> > BOARD_PHY_ADDRESS.
>
> It is common for phy devices to support it as a broadcast address. It
> is also commonly written in Xilinx documentation that it is treated as
> a broadcast address. e.g. the axi ethernet core
> (https://www.xilinx.com/support/documentation/ip_documentation/axi_ethernet/v7_0/pg138-axi-ethernet.pdf
> page 45). But 802.3 does not require it, instead address 0 is only
> reserved.
>
> With this commit the "must" refers to the device specification, in
> that QEMU is emulating a real phy (though more specifically the phy(s)
> that were being emulated for 

Re: [PATCH] spapr/xive: Allocate IPIs from the vCPU contexts

[Cédric Le Goater]

On 8/16/20 6:30 AM, David Gibson wrote:
> On Fri, Aug 14, 2020 at 05:08:13PM +0200, Cédric Le Goater wrote:
>>
>> This works as expected with a 128 vCPUs guest with pinned vcpus. The
>> first 64 IPIs are allocated on the first chip and the remaining 64
>> on the second chip.
>>
>> Still, this is more an RFC. We have time before the end of the merge
>> window.
> 
> It looks reasonable to me.  AFAICT it makes things better than they
> were, and even if we can improve it further, that won't break
> migration or other interfaces we need to preserve.

Yeah. What I don't like is this test below. I am not sure that 
machine->smp.cpus is the correct way to test the number of currently
active vCPUs. 

>>> +if (srcno < machine->smp.cpus) {
>>> +return kvmppc_xive_reset_ipi(xive, srcno, errp);
>>> +}
>>> +
>>>  if (xive_source_irq_is_lsi(xsrc, srcno)) {
>>>  state |= KVM_XIVE_LEVEL_SENSITIVE;
>>>  if (xsrc->status[srcno] & XIVE_STATUS_ASSERTED) {

Re: [PATCH] hw: virtio-mem: detach the element fromt the virtqueue when error occurs

[David Hildenbrand]

On 14.08.20 03:01, Li Qiang wrote:
> David Hildenbrand  于2020年8月14日周五 上午1:15写道：
>>
>> On 13.08.20 18:46, Li Qiang wrote:
>>
>> For now we use "virtio-mem:" for the subject, without the "hw: "part.
>>
>>> If error occurs while processing the virtio request we should call
>>> 'virtqueue_detach_element' to detach the element from the virtqueue
>>> before free the elem.
>>
>> What's the effect of this? In all cases we trigger a virtio_error(), so
>> do we really have to bother?
>>
> 
> Though the 'in_use' will be reset to 0 while reseting the virtio device.
> The mapped sglist will not be unammped.
> There maybe some undesired behavior.  CC Paolo to make a confirmation.

Looking at hw/virtio/virtio-crypto.c, this seems to be the right thing
to do.

Can you please respin, avoiding adding the label, only inserting the 3
separate virtqueue_detach_element() calls?

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH 14/18] hw/net: cadence_gem: Add a new 'phy-addr' property

[Nathan Rossi]

On Sun, 16 Aug 2020 at 18:29, Bin Meng  wrote:
>
> On Sat, Aug 15, 2020 at 5:06 PM Philippe Mathieu-Daudé  
> wrote:
> >
> > On 8/14/20 6:40 PM, Bin Meng wrote:
> > > From: Bin Meng 
> > >
> > > At present the PHY address of the PHY connected to GEM is hard-coded
> > > to either 23 (BOARD_PHY_ADDRESS) or 0. This might not be the case for
> > > all boards. Add a new 'phy-addr' property so that board can specify
> > > the PHY address for each GEM instance.
> > >
> > > Signed-off-by: Bin Meng 
> > > ---
> > >
> > >  hw/net/cadence_gem.c | 7 +--
> > >  include/hw/net/cadence_gem.h | 2 ++
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > > index a93b5c0..9fa03de 100644
> > > --- a/hw/net/cadence_gem.c
> > > +++ b/hw/net/cadence_gem.c
> > > @@ -1446,7 +1446,8 @@ static uint64_t gem_read(void *opaque, hwaddr 
> > > offset, unsigned size)
> > >  uint32_t phy_addr, reg_num;
> > >
> > >  phy_addr = (retval & GEM_PHYMNTNC_ADDR) >> 
> > > GEM_PHYMNTNC_ADDR_SHFT;
> > > -if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
> > > +if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
> > > +phy_addr == s->phy_addr) {
> > >  reg_num = (retval & GEM_PHYMNTNC_REG) >> 
> > > GEM_PHYMNTNC_REG_SHIFT;
> > >  retval &= 0x;
> > >  retval |= gem_phy_read(s, reg_num);
> > > @@ -1569,7 +1570,8 @@ static void gem_write(void *opaque, hwaddr offset, 
> > > uint64_t val,
> > >  uint32_t phy_addr, reg_num;
> > >
> > >  phy_addr = (val & GEM_PHYMNTNC_ADDR) >> 
> > > GEM_PHYMNTNC_ADDR_SHFT;
> > > -if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
> > > +if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
> > > +phy_addr == s->phy_addr) {
> > >  reg_num = (val & GEM_PHYMNTNC_REG) >> 
> > > GEM_PHYMNTNC_REG_SHIFT;
> > >  gem_phy_write(s, reg_num, val);
> > >  }
> > > @@ -1682,6 +1684,7 @@ static Property gem_properties[] = {
> > >  DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
> > >  DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
> > > GEM_MODID_VALUE),
> > > +DEFINE_PROP_UINT8("phy-addr", CadenceGEMState, phy_addr, 0),
> >
> > This patch would be complete by moving the BOARD_PHY_ADDRESS definition
> > to each board using this NIC, and setting the "phy-addr" property to
> > this value.
>
> I actually have no idea which board in QEMU is using this special PHY
> address instead of default 0.

Given Xilinx's QEMU fork has not used this value for quite some time,
I suspect it was only used to match an early chip emulation
platform/board.

https://github.com/Xilinx/qemu/blame/master/hw/net/cadence_gem.c#L248

>
> It looks BOARD_PHY_ADDRESS has been there since the initial version of
> the cadence_gem model.
>
> commit e9f186e514a70557d695cadd2c2287ef97737023
> Author: Peter A. G. Crosthwaite 
> Date:   Mon Mar 5 14:39:12 2012 +1000
>
> cadence_gem: initial version of device model
>
> Device model for cadence gem ethernet controller.
>
> Signed-off-by: Peter A. G. Crosthwaite 
> Signed-off-by: John Linn 
> Acked-by: Edgar E. Iglesias 
> Signed-off-by: Edgar E. Iglesias 
>
> Later PHY address 0 was added via the following commit:
>
> commit 553893736885e4f2dda424bff3e2200e1b6482a5
> Author: Peter Crosthwaite 
> Date:   Thu Apr 3 23:55:19 2014 -0700
>
> net: cadence_gem: Make phy respond to broadcast
>
> Phys must respond to address 0 by specification. Implement.
>
> Signed-off-by: Nathan Rossi 
> Signed-off-by: Peter Crosthwaite 
> Message-id:
> 6f4d53b04ddbfb19895bfb61a595e69f1c08859a.1396594056.git.peter.crosthwa...@xilinx.com
> Reviewed-by: Beniamino Galvani 
> Signed-off-by: Peter Maydell 
>
> I doubt the commit message said that PHYs must respond to address 0. I
> am not aware of such specs. The issue was probably that whatever board
> 2nd commit was tested against did not have a PHY at address
> BOARD_PHY_ADDRESS.

It is common for phy devices to support it as a broadcast address. It
is also commonly written in Xilinx documentation that it is treated as
a broadcast address. e.g. the axi ethernet core
(https://www.xilinx.com/support/documentation/ip_documentation/axi_ethernet/v7_0/pg138-axi-ethernet.pdf
page 45). But 802.3 does not require it, instead address 0 is only
reserved.

With this commit the "must" refers to the device specification, in
that QEMU is emulating a real phy (though more specifically the phy(s)
that were being emulated for Zynq boards) that does respond to address
0 as a broadcast. This change was a trade off in order to make QEMU
behave as it would on hardware, such that software using address 0 as
broadcast would work correctly.

Regards,
Nathan


>
> + a couple of Xilinx folks to comment.
>
> >
> > >   

[Bug 1890545] Re: (ARM64) qemu-x86_64+schroot(Debian bullseye) can't run chrome and can't load HTML

[Laurent Vivier]

Could you try attached patch?

** Patch added: "add QEMU_IFLA_INFO_KIND nested type for sit"
   
https://bugs.launchpad.net/qemu/+bug/1890545/+attachment/5401933/+files/0001-linux-user-add-QEMU_IFLA_INFO_KIND-nested-type-for-s.patch

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

Title:
  (ARM64) qemu-x86_64+schroot(Debian bullseye) can't run chrome and
  can't load HTML

Status in QEMU:
  New

Bug description:
  First I creat a file system that is debian(bullseye amd64)on arm64
  machine，then I download google-chrome，however, when I ran Google
  browser, some errors occurred.

  $ google-chrome --no-sandbox
  or 
  $ qemu-x86_64-static google-chrome --no-sandbox

  qemu: uncaught target signal 5 (Trace/breakpoint trap) - core dumped
  qemu: uncaught target signal 5 (Trace/breakpoint trap) - core dumped
  [1661:1661:0806/074307.502638:ERROR:nacl_fork_delegate_linux.cc(323)] Bad 
NaCl helper startup ack (0 bytes)
  [1664:1664:0806/074307.504159:ERROR:nacl_fork_delegate_linux.cc(323)] Bad 
NaCl helper startup ack (0 bytes)
  qemu: uncaught target signal 5 (Trace/breakpoint trap) - core dumped
  qemu: uncaught target signal 5 (Trace/breakpoint trap) - core dumped
  [1637:1678:0806/074308.337567:ERROR:file_path_watcher_linux.cc(315)] 
inotify_init() failed: Function not implemented (38)
  Fontconfig warning: "/etc/fonts/fonts.conf", line 100: unknown element "blank"
  qemu: unknown option 'type=utility'
  [1637:1680:0806/074313.598432:FATAL:gpu_data_manager_impl_private.cc(439)] 
GPU process isn't usable. Goodbye.
  qemu: uncaught target signal 5 (Trace/breakpoint trap) - core dumped
  Trace/breakpoint trap

  Why?
  And then I run firefox,it can be opened, but it can't load any web pages and 
HTML.
  I really need help！
  Thank.

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

Re: [PATCH 14/18] hw/net: cadence_gem: Add a new 'phy-addr' property

[Philippe Mathieu-Daudé]

On 8/16/20 10:29 AM, Bin Meng wrote:
> On Sat, Aug 15, 2020 at 5:06 PM Philippe Mathieu-Daudé  
> wrote:
>>
>> On 8/14/20 6:40 PM, Bin Meng wrote:
>>> From: Bin Meng 
>>>
>>> At present the PHY address of the PHY connected to GEM is hard-coded
>>> to either 23 (BOARD_PHY_ADDRESS) or 0. This might not be the case for
>>> all boards. Add a new 'phy-addr' property so that board can specify
>>> the PHY address for each GEM instance.
>>>
>>> Signed-off-by: Bin Meng 
>>> ---
>>>
>>>  hw/net/cadence_gem.c | 7 +--
>>>  include/hw/net/cadence_gem.h | 2 ++
>>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>>> index a93b5c0..9fa03de 100644
>>> --- a/hw/net/cadence_gem.c
>>> +++ b/hw/net/cadence_gem.c
>>> @@ -1446,7 +1446,8 @@ static uint64_t gem_read(void *opaque, hwaddr offset, 
>>> unsigned size)
>>>  uint32_t phy_addr, reg_num;
>>>
>>>  phy_addr = (retval & GEM_PHYMNTNC_ADDR) >> 
>>> GEM_PHYMNTNC_ADDR_SHFT;
>>> -if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
>>> +if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
>>> +phy_addr == s->phy_addr) {
>>>  reg_num = (retval & GEM_PHYMNTNC_REG) >> 
>>> GEM_PHYMNTNC_REG_SHIFT;
>>>  retval &= 0x;
>>>  retval |= gem_phy_read(s, reg_num);
>>> @@ -1569,7 +1570,8 @@ static void gem_write(void *opaque, hwaddr offset, 
>>> uint64_t val,
>>>  uint32_t phy_addr, reg_num;
>>>
>>>  phy_addr = (val & GEM_PHYMNTNC_ADDR) >> GEM_PHYMNTNC_ADDR_SHFT;
>>> -if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
>>> +if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
>>> +phy_addr == s->phy_addr) {
>>>  reg_num = (val & GEM_PHYMNTNC_REG) >> 
>>> GEM_PHYMNTNC_REG_SHIFT;
>>>  gem_phy_write(s, reg_num, val);
>>>  }
>>> @@ -1682,6 +1684,7 @@ static Property gem_properties[] = {
>>>  DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
>>>  DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
>>> GEM_MODID_VALUE),
>>> +DEFINE_PROP_UINT8("phy-addr", CadenceGEMState, phy_addr, 0),
>>
>> This patch would be complete by moving the BOARD_PHY_ADDRESS definition
>> to each board using this NIC, and setting the "phy-addr" property to
>> this value.
> 
> I actually have no idea which board in QEMU is using this special PHY
> address instead of default 0.

It seems safe to assume all do use address 23.

Anyway you can simply get ride of BOARD_PHY_ADDRESS in the read/write
using:

DEFINE_PROP_UINT8("phy-addr", CadenceGEMState,
  phy_addr, BOARD_PHY_ADDRESS),

> 
> It looks BOARD_PHY_ADDRESS has been there since the initial version of
> the cadence_gem model.
> 
> commit e9f186e514a70557d695cadd2c2287ef97737023
> Author: Peter A. G. Crosthwaite 
> Date:   Mon Mar 5 14:39:12 2012 +1000
> 
> cadence_gem: initial version of device model
> 
> Device model for cadence gem ethernet controller.
> 
> Signed-off-by: Peter A. G. Crosthwaite 
> Signed-off-by: John Linn 
> Acked-by: Edgar E. Iglesias 
> Signed-off-by: Edgar E. Iglesias 
> 
> Later PHY address 0 was added via the following commit:
> 
> commit 553893736885e4f2dda424bff3e2200e1b6482a5
> Author: Peter Crosthwaite 
> Date:   Thu Apr 3 23:55:19 2014 -0700
> 
> net: cadence_gem: Make phy respond to broadcast
> 
> Phys must respond to address 0 by specification. Implement.
> 
> Signed-off-by: Nathan Rossi 
> Signed-off-by: Peter Crosthwaite 
> Message-id:
> 6f4d53b04ddbfb19895bfb61a595e69f1c08859a.1396594056.git.peter.crosthwa...@xilinx.com
> Reviewed-by: Beniamino Galvani 
> Signed-off-by: Peter Maydell 
> 
> I doubt the commit message said that PHYs must respond to address 0. I
> am not aware of such specs. The issue was probably that whatever board
> 2nd commit was tested against did not have a PHY at address
> BOARD_PHY_ADDRESS.
> 
> + a couple of Xilinx folks to comment.
> 
>>
>>>  DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState,
>>>num_priority_queues, 1),
>>>  DEFINE_PROP_UINT8("num-type1-screeners", CadenceGEMState,
>>> diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
>>> index 54e646f..01c6189 100644
>>> --- a/include/hw/net/cadence_gem.h
>>> +++ b/include/hw/net/cadence_gem.h
>>> @@ -73,6 +73,8 @@ typedef struct CadenceGEMState {
>>>  /* Mask of register bits which are write 1 to clear */
>>>  uint32_t regs_w1c[CADENCE_GEM_MAXREG];
>>>
>>> +/* PHY address */
>>> +uint8_t phy_addr;
>>>  /* PHY registers backing store */
>>>  uint16_t phy_regs[32];
> 
> Regards,
> Bin
> 

Re: [PATCH 13/18] hw/riscv: microchip_pfsoc: Connect a DMA controller

[Philippe Mathieu-Daudé]

On 8/16/20 10:57 AM, Bin Meng wrote:
> Hi Philippe,
> 
> On Sat, Aug 15, 2020 at 5:00 PM Philippe Mathieu-Daudé  
> wrote:
>>
>> On 8/14/20 6:40 PM, Bin Meng wrote:
>>> From: Bin Meng 
>>>
>>> Connect a DMA controller to Microchip PolarFire SoC. Note interrupt
>>> has not been connected due to missing information in the manual how
>>> interrupts are routed to PLIC.
>>>
>>> On the Icicle Kit board, the HSS firmware utilizes the on-chip DMA
>>> controller to move the 2nd stage bootloader in the system memory.
>>>
>>> Signed-off-by: Bin Meng 
>>> ---
>>>
>>>  hw/riscv/Kconfig   |  1 +
>>>  hw/riscv/microchip_pfsoc.c | 10 ++
>>>  include/hw/riscv/microchip_pfsoc.h |  3 +++
>>>  3 files changed, 14 insertions(+)
>>>
>>> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
>>> index 7412db9..9323701 100644
>>> --- a/hw/riscv/Kconfig
>>> +++ b/hw/riscv/Kconfig
>>> @@ -55,4 +55,5 @@ config MICROCHIP_PFSOC
>>>  select SIFIVE
>>>  select UNIMP
>>>  select MCHP_PFSOC_MMUART
>>> +select MCHP_PFSOC_DMA
>>>  select CADENCE_SDHCI
>>> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
>>> index 7c09078..1c67cbc 100644
>>> --- a/hw/riscv/microchip_pfsoc.c
>>> +++ b/hw/riscv/microchip_pfsoc.c
>>> @@ -13,6 +13,7 @@
>>>   * 2) eNVM (Embedded Non-Volatile Memory)
>>>   * 3) MMUARTs (Multi-Mode UART)
>>>   * 4) Cadence eMMC/SDHC controller and an SD card connected to it
>>> + * 5) DMA (Direct Memory Access Controller)
>>>   *
>>>   * This board currently generates devicetree dynamically that indicates at 
>>> least
>>>   * two harts and up to five harts.
>>> @@ -71,6 +72,7 @@ static const struct MemmapEntry {
>>>  [MICROCHIP_PFSOC_BUSERR_UNIT4] ={  0x1704000, 0x1000 },
>>>  [MICROCHIP_PFSOC_CLINT] =   {  0x200,0x1 },
>>>  [MICROCHIP_PFSOC_L2CC] ={  0x201, 0x1000 },
>>> +[MICROCHIP_PFSOC_DMA] = {  0x300,   0x10 },
>>>  [MICROCHIP_PFSOC_L2LIM] =   {  0x800,  0x200 },
>>>  [MICROCHIP_PFSOC_PLIC] ={  0xc00,  0x400 },
>>>  [MICROCHIP_PFSOC_MMUART0] = { 0x2000, 0x1000 },
>>> @@ -114,6 +116,9 @@ static void microchip_pfsoc_soc_instance_init(Object 
>>> *obj)
>>>   TYPE_RISCV_CPU_SIFIVE_U54);
>>>  qdev_prop_set_uint64(DEVICE(>u_cpus), "resetvec", RESET_VECTOR);
>>>
>>> +object_initialize_child(obj, "dma-controller", >dma,
>>> +TYPE_MCHP_PFSOC_DMA);
>>> +
>>>  object_initialize_child(obj, "sd-controller", >sdhci,
>>>  TYPE_CADENCE_SDHCI);
>>
>> I haven't looked at the chip specs, but maybe you can add the SD
>> controller after the DMA controller so so you can directly link
>> a DMA address space to it.
>>
> 
> I am not sure I understand what you meant about adding the SD
> controller after the DMA controller. The Cadence SD controller has its
> own built-in DMA and does not depend on this DMA controller.

Ah OK. What I'm concerned about is the SD controller do its DMA
access in a proper DMA address space.

> 
> Regards,
> Bin
> 

Re: [PATCH 09/18] hw/sd: sdhci: Make sdhci_poweron_reset() internal visible

[Philippe Mathieu-Daudé]

On 8/16/20 10:50 AM, Bin Meng wrote:
> Hi Philippe,
> 
> On Sat, Aug 15, 2020 at 3:51 PM Philippe Mathieu-Daudé  
> wrote:
>>
>> On 8/14/20 6:40 PM, Bin Meng wrote:
>>> From: Bin Meng 
>>>
>>> sdhci_poweron_reset() might be needed for any SDHCI compatible
>>> device that is built on top of the generic SDHCI device.
>>
>> NAck. Please use device_legacy_reset() instead.
>>
>> In next patch:
>>
>>   device_legacy_reset(DEVICE(>slot));
>>
> 
> The function comments say this API is deprecated.
> 
> /**
>  * device_legacy_reset:
>  *
>  * Reset a single device (by calling the reset method).
>  * Note: This function is deprecated and will be removed when it becomes 
> unused.
>  * Please use device_cold_reset() now.
>  */
> 
> Should we use device_cold_reset() here?

Oops yes, certainly.

> 
> Regards,
> Bin
> 

[Bug 1891749] Re: CGA Mode 6 is only 100 pixels tall, when it's supposed to be 200

[Laurent Vivier]

** Tags removed: linux-user

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

Title:
  CGA Mode 6 is only 100 pixels tall, when it's supposed to be 200

Status in QEMU:
  New

Bug description:
  I have written a program that used CGA Mode 6 (640x200 black and
  white). However qemu-system-i386 only displays the first 100 pixels,
  effectively limiting the resolution of mode 6 to 640x100. When running
  the same program on a real computer it uses the whole 640x200 pixels.

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

Re: [PATCH 13/18] hw/riscv: microchip_pfsoc: Connect a DMA controller

[Bin Meng]

Hi Philippe,

On Sat, Aug 15, 2020 at 5:00 PM Philippe Mathieu-Daudé  wrote:
>
> On 8/14/20 6:40 PM, Bin Meng wrote:
> > From: Bin Meng 
> >
> > Connect a DMA controller to Microchip PolarFire SoC. Note interrupt
> > has not been connected due to missing information in the manual how
> > interrupts are routed to PLIC.
> >
> > On the Icicle Kit board, the HSS firmware utilizes the on-chip DMA
> > controller to move the 2nd stage bootloader in the system memory.
> >
> > Signed-off-by: Bin Meng 
> > ---
> >
> >  hw/riscv/Kconfig   |  1 +
> >  hw/riscv/microchip_pfsoc.c | 10 ++
> >  include/hw/riscv/microchip_pfsoc.h |  3 +++
> >  3 files changed, 14 insertions(+)
> >
> > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> > index 7412db9..9323701 100644
> > --- a/hw/riscv/Kconfig
> > +++ b/hw/riscv/Kconfig
> > @@ -55,4 +55,5 @@ config MICROCHIP_PFSOC
> >  select SIFIVE
> >  select UNIMP
> >  select MCHP_PFSOC_MMUART
> > +select MCHP_PFSOC_DMA
> >  select CADENCE_SDHCI
> > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> > index 7c09078..1c67cbc 100644
> > --- a/hw/riscv/microchip_pfsoc.c
> > +++ b/hw/riscv/microchip_pfsoc.c
> > @@ -13,6 +13,7 @@
> >   * 2) eNVM (Embedded Non-Volatile Memory)
> >   * 3) MMUARTs (Multi-Mode UART)
> >   * 4) Cadence eMMC/SDHC controller and an SD card connected to it
> > + * 5) DMA (Direct Memory Access Controller)
> >   *
> >   * This board currently generates devicetree dynamically that indicates at 
> > least
> >   * two harts and up to five harts.
> > @@ -71,6 +72,7 @@ static const struct MemmapEntry {
> >  [MICROCHIP_PFSOC_BUSERR_UNIT4] ={  0x1704000, 0x1000 },
> >  [MICROCHIP_PFSOC_CLINT] =   {  0x200,0x1 },
> >  [MICROCHIP_PFSOC_L2CC] ={  0x201, 0x1000 },
> > +[MICROCHIP_PFSOC_DMA] = {  0x300,   0x10 },
> >  [MICROCHIP_PFSOC_L2LIM] =   {  0x800,  0x200 },
> >  [MICROCHIP_PFSOC_PLIC] ={  0xc00,  0x400 },
> >  [MICROCHIP_PFSOC_MMUART0] = { 0x2000, 0x1000 },
> > @@ -114,6 +116,9 @@ static void microchip_pfsoc_soc_instance_init(Object 
> > *obj)
> >   TYPE_RISCV_CPU_SIFIVE_U54);
> >  qdev_prop_set_uint64(DEVICE(>u_cpus), "resetvec", RESET_VECTOR);
> >
> > +object_initialize_child(obj, "dma-controller", >dma,
> > +TYPE_MCHP_PFSOC_DMA);
> > +
> >  object_initialize_child(obj, "sd-controller", >sdhci,
> >  TYPE_CADENCE_SDHCI);
>
> I haven't looked at the chip specs, but maybe you can add the SD
> controller after the DMA controller so so you can directly link
> a DMA address space to it.
>

I am not sure I understand what you meant about adding the SD
controller after the DMA controller. The Cadence SD controller has its
own built-in DMA and does not depend on this DMA controller.

Regards,
Bin

Re: [PATCH 08/18] hw/sd: sd: Correctly set the high capacity bit

[Bin Meng]

Hi Philippe,

On Sat, Aug 15, 2020 at 4:38 PM Philippe Mathieu-Daudé  wrote:
>
> On 8/14/20 6:40 PM, Bin Meng wrote:
> > From: Bin Meng 
> >
> > Per the SD spec, Standard Capacity SD Memory Card (SDSC) supports
> > capacity up to and including 2 GiB.
> >
>
> Fixes: 2d7adea4fe ("hw/sd: Support SDHC size cards")
>
> > Signed-off-by: Bin Meng 
> > ---
> >
> >  hw/sd/sd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index 51f5900..5e7fc3f 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -313,7 +313,7 @@ static void sd_ocr_powerup(void *opaque)
> >  /* card power-up OK */
> >  sd->ocr = FIELD_DP32(sd->ocr, OCR, CARD_POWER_UP, 1);
> >
> > -if (sd->size > 1 * GiB) {
> > +if (sd->size > 2 * GiB) {
>
> But you need to fix sd_set_csd() too, else this is incomplete.
>

Ah yes, I missed that. Will fix in v2. Thanks for the review.

Regards,
Bin

Re: [PATCH 09/18] hw/sd: sdhci: Make sdhci_poweron_reset() internal visible

[Bin Meng]

Hi Philippe,

On Sat, Aug 15, 2020 at 3:51 PM Philippe Mathieu-Daudé  wrote:
>
> On 8/14/20 6:40 PM, Bin Meng wrote:
> > From: Bin Meng 
> >
> > sdhci_poweron_reset() might be needed for any SDHCI compatible
> > device that is built on top of the generic SDHCI device.
>
> NAck. Please use device_legacy_reset() instead.
>
> In next patch:
>
>   device_legacy_reset(DEVICE(>slot));
>

The function comments say this API is deprecated.

/**
 * device_legacy_reset:
 *
 * Reset a single device (by calling the reset method).
 * Note: This function is deprecated and will be removed when it becomes unused.
 * Please use device_cold_reset() now.
 */

Should we use device_cold_reset() here?

Regards,
Bin

Re: [PATCH 3/3] virtio-crypto: don't modify elem->in/out_sg

[Li Qiang]

Stefan Hajnoczi  于2020年8月12日周三 下午6:51写道：

> A number of iov_discard_front/back() operations are made by
> virtio-crypto. The elem->in/out_sg iovec arrays are modified by these
> operations, resulting virtqueue_unmap_sg() calls on different addresses
> than were originally mapped.
>
> This is problematic because dirty memory may not be logged correctly,
> MemoryRegion refcounts may be leaked, and the non-RAM bounce buffer can
> be leaked.
>
> Take a copy of the elem->in/out_sg arrays so that the originals are
> preserved. The iov_discard_undo() API could be used instead (with better
> performance) but requires careful auditing of the code, so do the simple
> thing instead.
>
> Signed-off-by: Stefan Hajnoczi 
>

virtio-net also uses this method.

Reviewed-by: Li Qiang 


> ---
>  hw/virtio/virtio-crypto.c | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index 6da12e315f..54f9bbb789 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -228,6 +228,8 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
>  size_t s;
>
>  for (;;) {
> +g_autofree struct iovec *out_iov_copy = NULL;
> +
>  elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
>  if (!elem) {
>  break;
> @@ -240,9 +242,12 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
>  }
>
>  out_num = elem->out_num;
> -out_iov = elem->out_sg;
> +out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) *
> out_num);
> +out_iov = out_iov_copy;
> +
>  in_num = elem->in_num;
>  in_iov = elem->in_sg;
> +
>  if (unlikely(iov_to_buf(out_iov, out_num, 0, , sizeof(ctrl))
>  != sizeof(ctrl))) {
>  virtio_error(vdev, "virtio-crypto request ctrl_hdr too
> short");
> @@ -582,6 +587,8 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
>  int queue_index =
> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
>  struct virtio_crypto_op_data_req req;
>  int ret;
> +g_autofree struct iovec *in_iov_copy = NULL;
> +g_autofree struct iovec *out_iov_copy = NULL;
>  struct iovec *in_iov;
>  struct iovec *out_iov;
>  unsigned in_num;
> @@ -598,9 +605,13 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
>  }
>
>  out_num = elem->out_num;
> -out_iov = elem->out_sg;
> +out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) * out_num);
> +out_iov = out_iov_copy;
> +
>  in_num = elem->in_num;
> -in_iov = elem->in_sg;
> +in_iov_copy = g_memdup(elem->in_sg, sizeof(in_iov[0]) * in_num);
> +in_iov = in_iov_copy;
> +
>  if (unlikely(iov_to_buf(out_iov, out_num, 0, , sizeof(req))
>  != sizeof(req))) {
>  virtio_error(vdev, "virtio-crypto request outhdr too short");
> --
> 2.26.2
>
>

Re: [PATCH 14/18] hw/net: cadence_gem: Add a new 'phy-addr' property

[Bin Meng]

On Sat, Aug 15, 2020 at 5:06 PM Philippe Mathieu-Daudé  wrote:
>
> On 8/14/20 6:40 PM, Bin Meng wrote:
> > From: Bin Meng 
> >
> > At present the PHY address of the PHY connected to GEM is hard-coded
> > to either 23 (BOARD_PHY_ADDRESS) or 0. This might not be the case for
> > all boards. Add a new 'phy-addr' property so that board can specify
> > the PHY address for each GEM instance.
> >
> > Signed-off-by: Bin Meng 
> > ---
> >
> >  hw/net/cadence_gem.c | 7 +--
> >  include/hw/net/cadence_gem.h | 2 ++
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > index a93b5c0..9fa03de 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -1446,7 +1446,8 @@ static uint64_t gem_read(void *opaque, hwaddr offset, 
> > unsigned size)
> >  uint32_t phy_addr, reg_num;
> >
> >  phy_addr = (retval & GEM_PHYMNTNC_ADDR) >> 
> > GEM_PHYMNTNC_ADDR_SHFT;
> > -if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
> > +if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
> > +phy_addr == s->phy_addr) {
> >  reg_num = (retval & GEM_PHYMNTNC_REG) >> 
> > GEM_PHYMNTNC_REG_SHIFT;
> >  retval &= 0x;
> >  retval |= gem_phy_read(s, reg_num);
> > @@ -1569,7 +1570,8 @@ static void gem_write(void *opaque, hwaddr offset, 
> > uint64_t val,
> >  uint32_t phy_addr, reg_num;
> >
> >  phy_addr = (val & GEM_PHYMNTNC_ADDR) >> GEM_PHYMNTNC_ADDR_SHFT;
> > -if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
> > +if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
> > +phy_addr == s->phy_addr) {
> >  reg_num = (val & GEM_PHYMNTNC_REG) >> 
> > GEM_PHYMNTNC_REG_SHIFT;
> >  gem_phy_write(s, reg_num, val);
> >  }
> > @@ -1682,6 +1684,7 @@ static Property gem_properties[] = {
> >  DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
> >  DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
> > GEM_MODID_VALUE),
> > +DEFINE_PROP_UINT8("phy-addr", CadenceGEMState, phy_addr, 0),
>
> This patch would be complete by moving the BOARD_PHY_ADDRESS definition
> to each board using this NIC, and setting the "phy-addr" property to
> this value.

I actually have no idea which board in QEMU is using this special PHY
address instead of default 0.

It looks BOARD_PHY_ADDRESS has been there since the initial version of
the cadence_gem model.

commit e9f186e514a70557d695cadd2c2287ef97737023
Author: Peter A. G. Crosthwaite 
Date:   Mon Mar 5 14:39:12 2012 +1000

cadence_gem: initial version of device model

Device model for cadence gem ethernet controller.

Signed-off-by: Peter A. G. Crosthwaite 
Signed-off-by: John Linn 
Acked-by: Edgar E. Iglesias 
Signed-off-by: Edgar E. Iglesias 

Later PHY address 0 was added via the following commit:

commit 553893736885e4f2dda424bff3e2200e1b6482a5
Author: Peter Crosthwaite 
Date:   Thu Apr 3 23:55:19 2014 -0700

net: cadence_gem: Make phy respond to broadcast

Phys must respond to address 0 by specification. Implement.

Signed-off-by: Nathan Rossi 
Signed-off-by: Peter Crosthwaite 
Message-id:
6f4d53b04ddbfb19895bfb61a595e69f1c08859a.1396594056.git.peter.crosthwa...@xilinx.com
Reviewed-by: Beniamino Galvani 
Signed-off-by: Peter Maydell 

I doubt the commit message said that PHYs must respond to address 0. I
am not aware of such specs. The issue was probably that whatever board
2nd commit was tested against did not have a PHY at address
BOARD_PHY_ADDRESS.

+ a couple of Xilinx folks to comment.

>
> >  DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState,
> >num_priority_queues, 1),
> >  DEFINE_PROP_UINT8("num-type1-screeners", CadenceGEMState,
> > diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
> > index 54e646f..01c6189 100644
> > --- a/include/hw/net/cadence_gem.h
> > +++ b/include/hw/net/cadence_gem.h
> > @@ -73,6 +73,8 @@ typedef struct CadenceGEMState {
> >  /* Mask of register bits which are write 1 to clear */
> >  uint32_t regs_w1c[CADENCE_GEM_MAXREG];
> >
> > +/* PHY address */
> > +uint8_t phy_addr;
> >  /* PHY registers backing store */
> >  uint16_t phy_regs[32];

Regards,
Bin

Re: [PATCH 1/3] util/iov: add iov_discard_undo()

[Li Qiang]

Stefan Hajnoczi  于2020年8月12日周三 下午6:52写道：

> The iov_discard_front/back() operations are useful for parsing iovecs
> but they modify the array elements. If the original array is needed
> after parsing finishes there is currently no way to restore it.
>
> Although g_memdup() can be used before performing destructive
> iov_discard_front/back() operations, this is inefficient.
>
> Introduce iov_discard_undo() to restore the array to the state prior to
> an iov_discard_front/back() operation.
>
>

Seems there are some errors. See below



> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/qemu/iov.h |  23 +++
>  tests/test-iov.c   | 165 +
>  util/iov.c |  50 --
>  3 files changed, 234 insertions(+), 4 deletions(-)
>
> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> index bffc151282..b6b283a5e5 100644
> --- a/include/qemu/iov.h
> +++ b/include/qemu/iov.h
> @@ -130,6 +130,29 @@ size_t iov_discard_front(struct iovec **iov, unsigned
> int *iov_cnt,
>  size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
>  size_t bytes);
>
> +/* Information needed to undo an iov_discard_*() operation */
> +typedef struct {
> +struct iovec *modified_iov;
> +struct iovec orig;
> +} IOVDiscardUndo;
> +
> +/*
> + * Undo an iov_discard_front_undoable() or iov_discard_back_undoable()
> + * operation. If multiple operations are made then each one needs a
> separate
> + * IOVDiscardUndo and iov_discard_undo() must be called in the reverse
> order
> + * that the operations were made.
> + */
> +void iov_discard_undo(IOVDiscardUndo *undo);
> +
> +/*
> + * Undoable versions of iov_discard_front() and iov_discard_back(). Use
> + * iov_discard_undo() to reset to the state before the discard operations.
> + */
> +size_t iov_discard_front_undoable(struct iovec **iov, unsigned int
> *iov_cnt,
> +  size_t bytes, IOVDiscardUndo *undo);
> +size_t iov_discard_back_undoable(struct iovec *iov, unsigned int *iov_cnt,
> + size_t bytes, IOVDiscardUndo *undo);
> +
>  typedef struct QEMUIOVector {
>  struct iovec *iov;
>  int niov;
> diff --git a/tests/test-iov.c b/tests/test-iov.c
> index 458ca25099..9c415e2f1f 100644
> --- a/tests/test-iov.c
> +++ b/tests/test-iov.c
> @@ -26,6 +26,12 @@ static void iov_free(struct iovec *iov, unsigned niov)
>  g_free(iov);
>  }
>
> +static bool iov_equals(const struct iovec *a, const struct iovec *b,
> +   unsigned niov)
> +{
> +return memcmp(a, b, sizeof(a[0]) * niov) == 0;
> +}
> +
>  static void test_iov_bytes(struct iovec *iov, unsigned niov,
> size_t offset, size_t bytes)
>  {
> @@ -335,6 +341,87 @@ static void test_discard_front(void)
>  iov_free(iov, iov_cnt);
>  }
>
> +static void test_discard_front_undo(void)
> +{
> +IOVDiscardUndo undo;
> +struct iovec *iov;
> +struct iovec *iov_tmp;
> +struct iovec *iov_orig;
> +unsigned int iov_cnt;
> +unsigned int iov_cnt_tmp;
> +size_t size;
> +
> +/* Discard zero bytes */
> +iov_random(, _cnt);
> +iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +iov_tmp = iov;
> +iov_cnt_tmp = iov_cnt;
> +iov_discard_front_undoable(_tmp, _cnt_tmp, 0, );
> +iov_discard_undo();
> +assert(iov_equals(iov, iov_orig, iov_cnt));
> +g_free(iov_orig);
> +iov_free(iov, iov_cnt);
> +
> +/* Discard more bytes than vector size */
> +iov_random(, _cnt);
> +iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +iov_tmp = iov;
> +iov_cnt_tmp = iov_cnt;
> +size = iov_size(iov, iov_cnt);
> +iov_discard_front_undoable(_tmp, _cnt_tmp, size + 1, );
> +iov_discard_undo();
> +assert(iov_equals(iov, iov_orig, iov_cnt));
>

The 'iov_discard_front_undoable' will change the 'iov_tmp' it will not
touch 'iov_orig'.
So the test will be always passed. The actually function will not be tested.

Also, maybe we could abstract a function to do these discard test?


> +g_free(iov_orig);
> +iov_free(iov, iov_cnt);
> +
> +/* Discard entire vector */
> +iov_random(, _cnt);
> +iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +iov_tmp = iov;
> +iov_cnt_tmp = iov_cnt;
> +size = iov_size(iov, iov_cnt);
> +iov_discard_front_undoable(_tmp, _cnt_tmp, size, );
> +iov_discard_undo();
> +assert(iov_equals(iov, iov_orig, iov_cnt));
> +g_free(iov_orig);
> +iov_free(iov, iov_cnt);
> +
> +/* Discard within first element */
> +iov_random(, _cnt);
> +iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +iov_tmp = iov;
> +iov_cnt_tmp = iov_cnt;
> +size = g_test_rand_int_range(1, iov->iov_len);
> +iov_discard_front_undoable(_tmp, _cnt_tmp, size, );
> +iov_discard_undo();
> +assert(iov_equals(iov, iov_orig, iov_cnt));
> +g_free(iov_orig);
> +iov_free(iov, iov_cnt);
> +
> +/* Discard entire 

Re: [PATCH] spapr/xive: Allocate IPIs from the vCPU contexts

[David Gibson]

On Fri, Aug 14, 2020 at 05:08:13PM +0200, Cédric Le Goater wrote:
> 
> This works as expected with a 128 vCPUs guest with pinned vcpus. The
> first 64 IPIs are allocated on the first chip and the remaining 64
> on the second chip.
> 
> Still, this is more an RFC. We have time before the end of the merge
> window.

It looks reasonable to me.  AFAICT it makes things better than they
were, and even if we can improve it further, that won't break
migration or other interfaces we need to preserve.

> 
> Thanks,
> 
> C.  
> 
> 
> On 8/14/20 5:03 PM, Cédric Le Goater wrote:
> > When QEMU switches to the XIVE interrupt mode, it performs a
> > kvmppc_xive_source_reset() which creates all the guest interrupts at
> > the level of the KVM device. These interrupts are backed by real HW
> > interrupts from the IPI interrupt pool of the XIVE controller.
> > 
> > Currently, this is done from the QEMU main thread, which results in
> > allocating all interrupts from the chip on which QEMU is running. IPIs
> > are not distributed across the system and the load is not well
> > balanced across the interrupt controllers.
> > 
> > Change the vCPU IPI allocation to run from the vCPU context in order
> > to allocate the associated XIVE IPI interrupt on the chip on which the
> > vCPU is running. This gives a chance to a better distribution of the
> > IPIs when the guest has a lot of vCPUs. When the vCPUs are pinned, it
> > makes the IPI local to the chip of the vCPU which reduces rerouting
> > between interrupt controllers and gives better performance.
> > 
> > This is only possible for running vCPUs. The IPIs of hot plugable
> > vCPUs will still be allocated in the context of the QEMU main thread.
> > 
> > Device interrupts are treated the same. To improve placement, we would
> > need some information on the chip owning the virtual source or HW
> > source in case of passthrough. This requires changes in PAPR.
> > 
> > Signed-off-by: Cédric Le Goater 
> > ---
> >  hw/intc/spapr_xive_kvm.c | 50 
> >  1 file changed, 50 insertions(+)
> > 
> > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> > index c6958f2da218..553fd7fd8f56 100644
> > --- a/hw/intc/spapr_xive_kvm.c
> > +++ b/hw/intc/spapr_xive_kvm.c
> > @@ -223,6 +223,47 @@ void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t 
> > lisn, Error **errp)
> >NULL, true, errp);
> >  }
> >  
> > +/*
> > + * Allocate the IPIs from the vCPU context. This will allocate the
> > + * XIVE IPI interrupt on the chip on which the vCPU is running. This
> > + * gives a better distribution of IPIs when the guest has a lot of
> > + * vCPUs. When the vCPU are pinned, the IPIs are local which reduces
> > + * rerouting between interrupt controllers and gives better
> > + * performance.
> > + */
> > +typedef struct {
> > +SpaprXive *xive;
> > +int ipi;
> > +Error *err;
> > +int rc;
> > +} XiveInitIPI;
> > +
> > +static void kvmppc_xive_reset_ipi_on_cpu(CPUState *cs, run_on_cpu_data arg)
> > +{
> > +XiveInitIPI *s = arg.host_ptr;
> > +uint64_t state = 0;
> > +
> > +s->rc = kvm_device_access(s->xive->fd, KVM_DEV_XIVE_GRP_SOURCE, s->ipi,
> > +  , true, >err);
> > +}
> > +
> > +static int kvmppc_xive_reset_ipi(SpaprXive *xive, int ipi, Error **errp)
> > +{
> > +PowerPCCPU *cpu = spapr_find_cpu(ipi);
> > +XiveInitIPI s = {
> > +.xive = xive,
> > +.ipi  = ipi,
> > +.err  = NULL,
> > +.rc   = 0,
> > +};
> > +
> > +run_on_cpu(CPU(cpu), kvmppc_xive_reset_ipi_on_cpu, 
> > RUN_ON_CPU_HOST_PTR());
> > +if (s.err) {
> > +error_propagate(errp, s.err);
> > +}
> > +return s.rc;
> > +}
> > +
> >  /*
> >   * At reset, the interrupt sources are simply created and MASKED. We
> >   * only need to inform the KVM XIVE device about their type: LSI or
> > @@ -230,11 +271,20 @@ void kvmppc_xive_sync_source(SpaprXive *xive, 
> > uint32_t lisn, Error **errp)
> >   */
> >  int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp)
> >  {
> > +MachineState *machine = MACHINE(qdev_get_machine());
> >  SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> >  uint64_t state = 0;
> >  
> >  assert(xive->fd != -1);
> >  
> > +/*
> > + * IPIs are special. Allocate the IPIs from the vCPU context for
> > + * those running. Hotplugged CPUs will the QEMU context.
> > + */
> > +if (srcno < machine->smp.cpus) {
> > +return kvmppc_xive_reset_ipi(xive, srcno, errp);
> > +}
> > +
> >  if (xive_source_irq_is_lsi(xsrc, srcno)) {
> >  state |= KVM_XIVE_LEVEL_SENSITIVE;
> >  if (xsrc->status[srcno] & XIVE_STATUS_ASSERTED) {
> > 
> 

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