[Bug 1414466] Re: -net user, hostfwd=... is not working(qemu-system-aarch64)

[Thomas Huth]

So is this now working for everybody with the correct ssh config (maybe
also check your firewall settings)? Could we close this ticket nowadays?
Or is somebody still having trouble?

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

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

Title:
  -net user,hostfwd=... is not working(qemu-system-aarch64)

Status in QEMU:
  Incomplete

Bug description:
  QEMU version: git a46b3aaf6bb038d4f6f192a84df204f10929e75c

   /opt/qemu.git/bin/qemu-system-aarch64 --version
  QEMU emulator version 2.2.50, Copyright (c) 2003-2008 Fabrice Bellard

  Hosts:
  ovs - host machine (Ubuntu 14.04.1, x86_64)
  debian8-arm64 - guest 

  Guest start:
  user@ovs:~$ /opt/qemu.git/bin/qemu-system-aarch64 -machine virt -cpu 
cortex-a57 -nographic -smp 1 -m 512 -kernel vmlinuz-run -initrd initrd-run.img 
-append "root=/dev/sda2 console=ttyAMA0" -global virtio-blk-device.scsi=off 
-device virtio-scsi-device,id=scsi -drive 
file=debian8-arm64.img,id=rootimg,cache=unsafe,if=none -device 
scsi-hd,drive=rootimg -netdev user,id=unet -device 
virtio-net-device,netdev=unet -net user,hostfwd=tcp:127.0.0.1:1122-:22

  root@debian8-arm64:~# netstat -ntplu | grep ssh
  tcp0  0 0.0.0.0:22  0.0.0.0:*   LISTEN
  410/sshd
  tcp6   0  0 :::22   :::*LISTEN
  410/sshd   

  (no firewall in guest vm)

  user@ovs:~$ netstat -ntplu | grep 1122
  tcp0  0 127.0.0.1:1122  0.0.0.0:*   LISTEN
  18722/qemu-system-a

  user@ovs:~$ time ssh user@127.0.0.1 -p 1122
  ssh_exchange_identification: read: Connection reset by peer

  real  1m29.341s
  user  0m0.005s
  sys   0m0.000s

  Inside guest vm sshd works fine:
  root@debian8-arm64:~# ssh user@127.0.0.1 -p 22
  user@127.0.0.1's password: 
  
  user@debian8-arm64:~$ exit
  logout
  Connection to 127.0.0.1 closed.

  root@debian8-arm64:~# ssh user@10.0.2.15 -p 22
  user@10.0.2.15's password: 
  ...
  user@debian8-arm64:~$ exit
  logout
  Connection to 10.0.2.15 closed.

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

Re: [PATCH] scripts/qmp: delete 'qmp' script

[Thomas Huth]

On 19/10/2020 23.04, John Snow wrote:
> This script has not seen a patch that was specifically for this script
> since it was moved to this location in 2013, and I doubt it is used. It
> uses "man qmp" for its help message, which does not exist. It also
> presumes there is a manual page for qmp-XXX, for each defined qmp
> command XXX. I don't think that's true.
> 
> The format it expects arguments in is something like:
> 
> block-dirty-bitmap-add --node=foo --name=bar
> 
> and has no capacity to support nested JSON arguments, either.
> 
> Most developers use either qmp-shell or socat (or pasting JSON directly
> into qmp stdio), so this duplication and additional alternate syntax is
> not helpful.
> 
> Remove it. Leave a breadcrumb script just in case, to be removed next
> release cycle.
> 
> Signed-off-by: John Snow 
> ---
>  scripts/qmp/qmp | 131 +++-
>  1 file changed, 7 insertions(+), 124 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH] intel_iommu: Fix two misuse of "0x%u" prints

[Jason Wang]

On 2020/10/20 上午1:39, Peter Xu wrote:

Dave magically found this.  Fix them with "0x%x".

Reported-by: Dr. David Alan Gilbert 
Signed-off-by: Peter Xu 
---
  hw/i386/intel_iommu.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 749eb6ad63..70ac837733 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2665,7 +2665,7 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, 
unsigned size)
  
  if (addr + size > DMAR_REG_SIZE) {

  error_report_once("%s: MMIO over range: addr=0x%" PRIx64
-  " size=0x%u", __func__, addr, size);
+  " size=0x%x", __func__, addr, size);
  return (uint64_t)-1;
  }
  
@@ -2716,7 +2716,7 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
  
  if (addr + size > DMAR_REG_SIZE) {

  error_report_once("%s: MMIO over range: addr=0x%" PRIx64
-  " size=0x%u", __func__, addr, size);
+  " size=0x%x", __func__, addr, size);
  return;
  }
  



Acked-by: Jason Wang 

Re: [PATCH v2 0/5] qapi: Restrict machine (and migration) specific commands

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> On 10/19/20 6:48 PM, Markus Armbruster wrote:
>> Eduardo Habkost  writes:
>> 
>>> On Mon, Oct 19, 2020 at 09:55:20AM +0200, Markus Armbruster wrote:
 Eduardo Habkost  writes:

> On Mon, Oct 12, 2020 at 02:15:31PM +0200, Philippe Mathieu-Daudé wrote:
>> Reduce the machine code pulled into qemu-storage-daemon.
>>
>> The series is fully Acked, but Markus wants it reviewed
>> by the Machine core maintainers.
>
> I've confirmed that all patches move QAPI schema code without
> introducing any additional changes.
>
> Reviewed-by: Eduardo Habkost 

 I take this as "I agree the things moved to machine.json belong there".
 Holler if I'm mistaken.
>>>
>>> I agree machine.json is better than misc.json for them, yes.
>>>
>>> I miss short descriptions of the purpose of each file, though.
>>> It would help us decide what's appropriate in the future.
>>
>> The QAPI modules are commonly aligned with sub-systems defined in
>> MAINTAINERS.
>>
>> Regardless, file comments would be nice.
>
> I don't understand what you mean/expect by "file comments".
> Example?

A comment explaining the file, at the beginning of the file.

> W.r.t. MAINTAINERS, I can move Xen code to qapi/migration-xen.json;

How much could be moved, and from where?

Sub-modules don't need to mirror MAINTAINERS slavishly.  We want
reasonably-sized modules, and we want useful get_maintainer.pl output.

> 'query-kvm' is used when no KVM built it, so I'll let it in
> machine.json; the others seem to belong in machine.json too,
> with no particular justification.

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Markus Armbruster]

Mark Cave-Ayland  writes:

> One thing I have thought about is being able to mark a link property
> as mandatory so if a value hasn't been set before realize then you

A non-null value, I presume.

> receive a fatal error. This would be for cases like this where 2
> internal devices are connected together without any formal interface,
> i.e. in cases where -device wouldn't work anyway.

Moves the check from code one step closer to data: from the realize
method to the object_property_add_link() call.

I like doing things in data, because data is easier to reason about than
code.

[...]

Re: [PATCH v2 9/9] block: check availablity for preadv/pwritev on mac

[Thomas Huth]

On 20/10/2020 00.20, Joelle van Dyne wrote:
> On Mon, Oct 19, 2020 at 1:27 AM Thomas Huth  wrote:
>>
>> On 19/10/2020 03.39, Joelle van Dyne wrote:
>>> From: osy 
>>
>> That "From:" line looks wrong ... could you please fix the "Author" of your
>> patches / your git config?
> osy wrote the original changes. I joined the UTM project to help bring
> the changes upstream with permission. However, they have agreed that
> if required that we can use my name as the author.

In any way, that "users.noreply.github.com" does not look like a valid
e-mail address and should be replaced.

>>
>>> macOS 11/iOS 14 added preadv/pwritev APIs. Due to weak linking, configure
>>> will succeed with CONFIG_PREADV even when targeting a lower OS version. We
>>> therefore need to check at run time if we can actually use these APIs.
>>
>> That sounds like the wrong approach to me ... could you please try to fix
>> the check in "configure" instead? E.g. by running compile_prog with
>> "-Werror", so that the test fails if there is no valid prototype available?
> It's not that simple. Xcode 11 and below (supporting macOS 10.15 and
> below, iOS 13 and below, etc) does not have preadv/pwritev symbols
> defined and would fail to compile. Xcode 12 (supporting macOS 11 and
> below, iOS 14 and below, etc) have preadv/pwritev weakly defined so if
> it runs on, for example, 10.15, it would abort. There is no way to
> determine at compile time if you can use preadv/pwritev or not when
> building with Xcode 12. The availability checks are Apple's preferred
> way to handle this kind of situation (they discourage directly
> checking if an API exists on a system).

Ok, got it now, thanks for the detailed explanation!

 Thomas

Re: [PATCH v6 04/10] block: allow specifying name of block device for vmstate storage

[Markus Armbruster]

Eric Blake  writes:

> On 10/8/20 10:49 AM, Daniel P. Berrangé wrote:
>> Currently the vmstate will be stored in the first block device that
>> supports snapshots. Historically this would have usually been the
>> root device, but with UEFI it might be the variable store. There
>> needs to be a way to override the choice of block device to store
>> the state in.
>> Signed-off-by: Daniel P. Berrangé 
>> ---
>
>> @@ -83,7 +83,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
>>   (qemu) savevm snap0
>>   Error: Device 'file' is writable but does not support snapshots
>>   (qemu) info snapshots
>> -No block device supports snapshots
>> +no block device can store vmstate for snapshot
>
> We're inconsistent on whether error messages start with a Capital.

Pervasive issue.

Starting with lower case plays more nicely with error_prepend().

> But our split-brain behavior is not made any worse by this patch.
>
> Reviewed-by: Eric Blake 

Re: [PATCH v6 02/10] migration: stop returning errno from load_snapshot()

[Markus Armbruster]

Eric Blake  writes:

> On 10/8/20 10:49 AM, Daniel P. Berrangé wrote:
>> None of the callers care about the errno value since there is a full
>> Error object populated. This gives consistency with save_snapshot()
>> which already just returns -1.
>> Reviewed-by: Dr. David Alan Gilbert 
>> Signed-off-by: Daniel P. Berrangé 
>> ---
>>   migration/savevm.c | 15 +++
>>   1 file changed, 7 insertions(+), 8 deletions(-)
>> 
>
>> @@ -2892,11 +2892,11 @@ int load_snapshot(const char *name, Error **errp)
>>   ret = bdrv_snapshot_find(bs_vm_state, , name);
>>   aio_context_release(aio_context);
>>   if (ret < 0) {
>> -return ret;
>> +return -1;
>>   } else if (sn.vm_state_size == 0) {
>>   error_setg(errp, "This is a disk-only snapshot. Revert to it "
>>  " offline using qemu-img");
>
> While you are here, let's fix the double space in the error message.

The message should be rephrased, because

 * The resulting message should be a single phrase, with no newline or
 * trailing punctuation.

This is from error_setg()'s contract.

Two obvious ways:

1. Use error_append_hint() for the "what you should do" part.

2. Replace '.' by ';' and call it a day.

Re: [PATCH v2 2/9] configure: cross-compiling without cross_prefix

[Thomas Huth]

On 20/10/2020 00.24, Joelle van Dyne wrote:
> Correct me if I'm wrong but wouldn't the following test still fail
> with --cross-prefix=""
> 
> if test -n "$cross_prefix"; then
> ...
> 
> That was my main reason for making this change.

That's why I wrote "still introduce the cross_compile=yes variable" ... that
change is certainly required anyway.

> @@ -456,6 +457,11 @@ for opt do
>optarg=$(expr "x$opt" : 'x[^=]*=\(.*\)')
>case "$opt" in
>--cross-prefix=*) cross_prefix="$optarg"
> +cross_compile="yes"
> +  ;;
> +  --enable-cross-compile) cross_compile="yes"
> +  ;;
> +  --disable-cross-compile) cross_compile="no"

 Can't you simply use --cros-prefix="" instead?
>>>
>>> I mean, still introduce the "cross_compile=yes" variable, just omit the new
>>> options.
>>
>> That seems less intuitive for people trying to find this option. If --help
>> lists --enable-cross-compile I can guess what that means but there's no
>> way I could guess --cros-prefix="" unless I've been told or searched and
>> stumbled upon it. So unless it's a big problem I like the explicit options
>> better. Or is that a convention in other projects to use empty prefix to
>> enable cross compile that I don't know about?

I don't think that --cross-prefix is a "standard" option... Most other
(GNU-tools related) projects use "--build" and "--host" instead... so I
guess we're free to chose here. Let's see whether other people here have an
opionion on this...

 Thomas

[Bug 1889411] Re: RISC-V: Unable to unwind the stack upon signals

[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/1889411

Title:
  RISC-V: Unable to unwind the stack upon signals

Status in QEMU:
  Expired

Bug description:
  Consider the following program:

  ===
  #include 
  #include 

  #define NOINLINE __attribute__ ((noinline))

  void NOINLINE abort_me(void) { abort(); /* trigger SIGABRT */ }

  void NOINLINE level1(void) { abort_me(); }

  void NOINLINE level2(void) { level1(); }

  void NOINLINE level3(void) { level2(); }

  void NOINLINE level4(void) { level3();}

  int main(void) {
level4();
return 0;
  }
  ===

  $ riscv64-linux-gnu-gcc -march=rv64imafdc -O0 -g c.c
  $ qemu-riscv64 -g 31337 ./c &
  $ riscv64-unknown-linux-gnu-gdb -q -ex 'target remote localhost:31337' -ex 'b 
abort_me' -ex c -ex bt ./c
  Reading symbols from c...
  Remote debugging using localhost:31337
  Reading symbols from 
/home/lewurm/riscv/sysroot/lib/ld-linux-riscv64-lp64d.so.1...
  0x004000804f30 in _start () from 
/home/lewurm/riscv/sysroot/lib/ld-linux-riscv64-lp64d.so.1
  Breakpoint 1 at 0x400632: file c.c, line 7.
  Continuing.

  Breakpoint 1, abort_me () at c.c:7
  7   abort(); /* trigger SIGABRT */
  #0  abort_me () at c.c:7
  #1  0x00400642 in level1 () at c.c:11
  #2  0x00400658 in level2 () at c.c:15
  #3  0x0040066e in level3 () at c.c:19
  #4  0x00400684 in level4 () at c.c:23
  #5  0x0040069a in main () at c.c:27
  ===

  So far so good, I get a proper backtrace as expected. If I let the
  signal trigger however, gdb is not able to unwind the stack:

  (gdb) c
  Continuing.

  Program received signal SIGABRT, Aborted.
  0x004000858074 in ?? ()
  (gdb) bt
  #0  0x004000858074 in ?? ()


  I get the same behaviour for SIGSEGV and SIGILL, I didn't try other
  signals. Apparently this scenario works on real hardware (see linked
  gdb issue below), and presumably it would work with system qemu (I
  haven't tested that yet though). So my guess is that qemu does
  something differently around signal handling than the linux kernel.

  
  Full reproducer: 
https://gist.github.com/lewurm/befb9ddf5894bad9628b1df77258598b
  RISC-V GDB issue: https://github.com/riscv/riscv-binutils-gdb/issues/223

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

[PATCH v8 0/2] Add file-backed and write-once features to OTP

[Green Wan]

patch [1/2] - add write function and wrire-once feature
patch [2/2] - add file backend support

Test Steps: (should work even only 1/2 is applied)
 1) Follow instructions to prepare fw_payload - 
https://github.com/riscv/opensbi/blob/master/docs/platform/sifive_fu540.md
a) build 1-round opensbi
   $ cd opensbi
   $ OBJCOPY=riscv64-buildroot-linux-gnu-objcopy \
   LD=riscv64-buildroot-linux-gnu-ld \
   CC=riscv64-buildroot-linux-gnu-gcc \
   make PLATFORM=sifive/fu540
b) build u-boot
   # Make sure the 'CONFIG_SIFIVE_OTP=y' is set
   $ cd u-boot
   $ 
OPENSBI=/xxx/opensbi/build/platform/sifive/fu540/firmware/fw_dynamic.bin \
   ARCH=riscv \
   CROSS_COMPILE=riscv64-buildroot-linux-gnu- \
   make
c) generate fw_payload.elf
   $ cd opensbi
   $ OBJCOPY=riscv64-buildroot-linux-gnu-objcopy \
   LD=riscv64-buildroot-linux-gnu-ld \
   CC=riscv64-buildroot-linux-gnu-gcc \
   make PLATFORM=sifive/fu540 FW_PAYLOAD_PATH=/xxx/u-boot/u-boot.bin
 2) Apply uboot test patch - 
http://patchwork.ozlabs.org/project/uboot/patch/1602657292-82815-1-git-send-email-bmeng...@gmail.com/
Rebuild u-boot and fw_payload.elf
 3) Generate empty otp image. (skip this if only 1/2 is applied.)
$ dd if=/dev/zero of=./otp.img bs=1k count=16
 4) run qemu with fw_payload.elf
$ qemu-system-riscv64 -M sifive_u -m 256M -nographic -bios none \
  -kernel ../opensbi/build/platform/sifive/fu540/firmware/fw_payload.elf \
  -d guest_errors -drive if=none,format=raw,file=otp.img 
 5) (uboot otp driver should do some read/write already) Run read/write in 
u-boot

# dump mem before test
=> md 8020 10
8020: 84ae822a 00061297 7642b283 10529073*.Bvs.R.
80200010: 10401073 031b52c1 13134010 71330153s.@..R...@..S.3q
80200020: 850a0053 28c0b0ef 812a81aa 00062297S..(..*.."..
80200030: 94c2b283 a92f4905 16630862 22970209.I/.b.c"
=> md 8040 10
8040:    
80400010:    
80400020:    
80400030:    

# check read function and see if serial is set
=> misc read  otp@1007 3f0 8040 10
=> md 8040 10
8040: 0001 fffe  
80400010:    
80400020:    
80400030:    

# check write function
=> misc write otp@1007 0 8020 10
=> misc read  otp@1007 0 8040 10
=> md 8040 10
8040: 84ae822a 00061297 7642b283 10529073*.Bvs.R.
80400010:    
80400020:    
80400030:    
=>

Changelogs:
v6 to v7:
 - Rebase to the latest and move debug message patch
   from patch [2/2] to [1/2]
 - Remove RFC tag and add credit

v6 to v7:
 - Fix bug in MACRO, SET_FUSEARRAY_BIT.
 - Add serial initialization in sifive_u_otp_reset().
 - revise write-once error message.

v5 to v6:
 - Rebase to latest. (sifive_u_otp.* are moved to hw/misc)
 - Put the example command to commit message.
 - Refine errp handle when check backend drive.
 - Remove unnecessary debug message.

v4 to v5:
 - Change the patch order
 - Add write operation to update pdin to fuse[] bit by bit 
 - Fix wrong protection for offset 0x0~0x38
 - Add SIFIVE_U_OTP_PWE_EN definition
 - Refine access macro for fuse[] and fuse_wo[]

Summary of Patches 
 - First patch is to add write opertion to update pdin data to fuse[] bit
   by bit. Add 'write-once' feature to block second write to same bit of
   the OTP memory.

 - Second patch is to add file-backed implementation to allow users to use
   '-drive' to assign an OTP raw image file. OTP image file must be bigger
   than 16K.

   For example, '-drive if=none,format=raw,file=otp.img'

Testing
 - Tested on sifive_u for both qemu and u-boot.

Green Wan (2):
  hw/misc/sifive_u_otp: Add write function and write-once protection
  hw/misc/sifive_u_otp: Add backend drive support

 hw/misc/sifive_u_otp.c | 95 +-
 include/hw/misc/sifive_u_otp.h |  5 ++
 2 files changed, 99 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v8 2/2] hw/misc/sifive_u_otp: Add backend drive support

[Green Wan]

Add '-drive' support to OTP device. Allow users to assign a raw file
as OTP image.

test commands for 16k otp.img filled with zero:

$ dd if=/dev/zero of=./otp.img bs=1k count=16
$ ./qemu-system-riscv64 -M sifive_u -m 256M -nographic -bios none \
-kernel ../opensbi/build/platform/sifive/fu540/firmware/fw_payload.elf \
-d guest_errors -drive if=none,format=raw,file=otp.img

Signed-off-by: Green Wan 
Reviewed-by: Bin Meng 
Tested-by: Bin Meng 
---
 hw/misc/sifive_u_otp.c | 65 ++
 include/hw/misc/sifive_u_otp.h |  2 ++
 2 files changed, 67 insertions(+)

diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
index b9238d64cb..60066375ab 100644
--- a/hw/misc/sifive_u_otp.c
+++ b/hw/misc/sifive_u_otp.c
@@ -19,11 +19,14 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "hw/misc/sifive_u_otp.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/block-backend.h"
 
 #define WRITTEN_BIT_ON 0x1
 
@@ -54,6 +57,16 @@ static uint64_t sifive_u_otp_read(void *opaque, hwaddr addr, 
unsigned int size)
 if ((s->pce & SIFIVE_U_OTP_PCE_EN) &&
 (s->pdstb & SIFIVE_U_OTP_PDSTB_EN) &&
 (s->ptrim & SIFIVE_U_OTP_PTRIM_EN)) {
+
+/* read from backend */
+if (s->blk) {
+int32_t buf;
+
+blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, ,
+  SIFIVE_U_OTP_FUSE_WORD);
+return buf;
+}
+
 return s->fuse[s->pa & SIFIVE_U_OTP_PA_MASK];
 } else {
 return 0xff;
@@ -145,6 +158,12 @@ static void sifive_u_otp_write(void *opaque, hwaddr addr,
 /* write bit data */
 SET_FUSEARRAY_BIT(s->fuse, s->pa, s->paio, s->pdin);
 
+/* write to backend */
+if (s->blk) {
+blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD,
+   >fuse[s->pa], SIFIVE_U_OTP_FUSE_WORD, 0);
+}
+
 /* update written bit */
 SET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio, WRITTEN_BIT_ON);
 }
@@ -168,16 +187,48 @@ static const MemoryRegionOps sifive_u_otp_ops = {
 
 static Property sifive_u_otp_properties[] = {
 DEFINE_PROP_UINT32("serial", SiFiveUOTPState, serial, 0),
+DEFINE_PROP_DRIVE("drive", SiFiveUOTPState, blk),
 DEFINE_PROP_END_OF_LIST(),
 };
 
 static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
 {
 SiFiveUOTPState *s = SIFIVE_U_OTP(dev);
+DriveInfo *dinfo;
 
 memory_region_init_io(>mmio, OBJECT(dev), _u_otp_ops, s,
   TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
 sysbus_init_mmio(SYS_BUS_DEVICE(dev), >mmio);
+
+dinfo = drive_get_next(IF_NONE);
+if (dinfo) {
+int ret;
+uint64_t perm;
+int filesize;
+BlockBackend *blk;
+
+blk = blk_by_legacy_dinfo(dinfo);
+filesize = SIFIVE_U_OTP_NUM_FUSES * SIFIVE_U_OTP_FUSE_WORD;
+if (blk_getlength(blk) < filesize) {
+error_setg(errp, "OTP drive size < 16K");
+return;
+}
+
+qdev_prop_set_drive_err(dev, "drive", blk, errp);
+
+if (s->blk) {
+perm = BLK_PERM_CONSISTENT_READ |
+   (blk_is_read_only(s->blk) ? 0 : BLK_PERM_WRITE);
+ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
+if (ret < 0) {
+return;
+}
+
+if (blk_pread(s->blk, 0, s->fuse, filesize) != filesize) {
+error_setg(errp, "failed to read the initial flash content");
+}
+}
+}
 }
 
 static void sifive_u_otp_reset(DeviceState *dev)
@@ -191,6 +242,20 @@ static void sifive_u_otp_reset(DeviceState *dev)
 s->fuse[SIFIVE_U_OTP_SERIAL_ADDR] = s->serial;
 s->fuse[SIFIVE_U_OTP_SERIAL_ADDR + 1] = ~(s->serial);
 
+if (s->blk) {
+/* Put serial number to backend as well*/
+uint32_t serial_data;
+int index = SIFIVE_U_OTP_SERIAL_ADDR;
+
+serial_data = s->serial;
+blk_pwrite(s->blk, index * SIFIVE_U_OTP_FUSE_WORD,
+   _data, SIFIVE_U_OTP_FUSE_WORD, 0);
+
+serial_data = ~(s->serial);
+blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
+   _data, SIFIVE_U_OTP_FUSE_WORD, 0);
+}
+
 /* Initialize write-once map */
 memset(s->fuse_wo, 0x00, sizeof(s->fuse_wo));
 }
diff --git a/include/hw/misc/sifive_u_otp.h b/include/hw/misc/sifive_u_otp.h
index ebffbc1fa5..5d0d7df455 100644
--- a/include/hw/misc/sifive_u_otp.h
+++ b/include/hw/misc/sifive_u_otp.h
@@ -46,6 +46,7 @@
 
 #define SIFIVE_U_OTP_PA_MASK0xfff
 #define SIFIVE_U_OTP_NUM_FUSES  0x1000
+#define SIFIVE_U_OTP_FUSE_WORD  4
 #define SIFIVE_U_OTP_SERIAL_ADDR0xfc
 
 #define SIFIVE_U_OTP_REG_SIZE   0x1000
@@ -80,6 +81,7 @@ struct SiFiveUOTPState {
 

[PATCH v8 1/2] hw/misc/sifive_u_otp: Add write function and write-once protection

[Green Wan]

 - Add write operation to update fuse data bit when PWE bit is on.
 - Add array, fuse_wo, to store the 'written' status for all bits
   of OTP to block the write operation.

Signed-off-by: Green Wan 
Reviewed-by: Alistair Francis 
Reviewed-by: Bin Meng 
Tested-by: Bin Meng 
---
 hw/misc/sifive_u_otp.c | 30 +-
 include/hw/misc/sifive_u_otp.h |  3 +++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
index c2f3c8e129..b9238d64cb 100644
--- a/hw/misc/sifive_u_otp.c
+++ b/hw/misc/sifive_u_otp.c
@@ -25,6 +25,14 @@
 #include "qemu/module.h"
 #include "hw/misc/sifive_u_otp.h"
 
+#define WRITTEN_BIT_ON 0x1
+
+#define SET_FUSEARRAY_BIT(map, i, off, bit)\
+map[i] = bit ? (map[i] | bit << off) : (map[i] & ~(0x1 << off))
+
+#define GET_FUSEARRAY_BIT(map, i, off)\
+((map[i] >> off) & 0x1)
+
 static uint64_t sifive_u_otp_read(void *opaque, hwaddr addr, unsigned int size)
 {
 SiFiveUOTPState *s = opaque;
@@ -123,7 +131,24 @@ static void sifive_u_otp_write(void *opaque, hwaddr addr,
 s->ptrim = val32;
 break;
 case SIFIVE_U_OTP_PWE:
-s->pwe = val32;
+s->pwe = val32 & SIFIVE_U_OTP_PWE_EN;
+
+/* PWE is enabled. Ignore PAS=1 (no redundancy cell) */
+if (s->pwe && !s->pas) {
+if (GET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "write once error: idx<%u>, bit<%u>\n",
+  s->pa, s->paio);
+break;
+}
+
+/* write bit data */
+SET_FUSEARRAY_BIT(s->fuse, s->pa, s->paio, s->pdin);
+
+/* update written bit */
+SET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio, WRITTEN_BIT_ON);
+}
+
 break;
 default:
 qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx
@@ -165,6 +190,9 @@ static void sifive_u_otp_reset(DeviceState *dev)
 /* Make a valid content of serial number */
 s->fuse[SIFIVE_U_OTP_SERIAL_ADDR] = s->serial;
 s->fuse[SIFIVE_U_OTP_SERIAL_ADDR + 1] = ~(s->serial);
+
+/* Initialize write-once map */
+memset(s->fuse_wo, 0x00, sizeof(s->fuse_wo));
 }
 
 static void sifive_u_otp_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/misc/sifive_u_otp.h b/include/hw/misc/sifive_u_otp.h
index 82c9176c8f..ebffbc1fa5 100644
--- a/include/hw/misc/sifive_u_otp.h
+++ b/include/hw/misc/sifive_u_otp.h
@@ -36,6 +36,8 @@
 #define SIFIVE_U_OTP_PTRIM  0x34
 #define SIFIVE_U_OTP_PWE0x38
 
+#define SIFIVE_U_OTP_PWE_EN (1 << 0)
+
 #define SIFIVE_U_OTP_PCE_EN (1 << 0)
 
 #define SIFIVE_U_OTP_PDSTB_EN   (1 << 0)
@@ -75,6 +77,7 @@ struct SiFiveUOTPState {
 uint32_t ptrim;
 uint32_t pwe;
 uint32_t fuse[SIFIVE_U_OTP_NUM_FUSES];
+uint32_t fuse_wo[SIFIVE_U_OTP_NUM_FUSES];
 /* config */
 uint32_t serial;
 };
-- 
2.17.1

Re: [PATCH v3 0/8] Fix some style problems in migration

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/1603163448-27122-1-git-send-email-yubih...@huawei.com/



Hi,

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

Type: series
Message-id: 1603163448-27122-1-git-send-email-yubih...@huawei.com
Subject: [PATCH v3 0/8] Fix some style problems in migration

=== 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/1603163448-27122-1-git-send-email-yubih...@huawei.com -> 
patchew/1603163448-27122-1-git-send-email-yubih...@huawei.com
 - [tag update]  patchew/cover.1602634524.git.alistair.fran...@wdc.com -> 
patchew/cover.1602634524.git.alistair.fran...@wdc.com
Switched to a new branch 'test'
b33edea migration: Delete redundant spaces
277563d migration: Open brace '{' following function declarations go on the 
next line
7dd0ad0 migration: Do not initialise statics and globals to 0 or NULL
5540fd1 migration: Add braces {} for if statement
89c1a12 migration: Open brace '{' following struct go on the same line
23e7f3f migration: Add spaces around operator
cc9ec99 migration: Don't use '#' flag of printf format
122dc68 migration: Do not use C99 // comments

=== OUTPUT BEGIN ===
1/8 Checking commit 122dc68da3df (migration: Do not use C99 // comments)
2/8 Checking commit cc9ec99c5ac0 (migration: Don't use '#' flag of printf 
format)
3/8 Checking commit 23e7f3fb51c2 (migration: Add spaces around operator)
ERROR: spaces required around that '*' (ctx:WxV)
#62: FILE: migration/savevm.c:524:
+.subsections = (const VMStateDescription *[]) {
  ^

total: 1 errors, 0 warnings, 59 lines checked

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

4/8 Checking commit 89c1a1215ef0 (migration: Open brace '{' following struct go 
on the same line)
5/8 Checking commit 5540fd121f19 (migration: Add braces {} for if statement)
6/8 Checking commit 7dd0ad0c5b86 (migration: Do not initialise statics and 
globals to 0 or NULL)
7/8 Checking commit 277563d27c33 (migration: Open brace '{' following function 
declarations go on the next line)
8/8 Checking commit b33edea584b4 (migration: Delete redundant spaces)
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware

[Bin Meng]

On Wed, Oct 14, 2020 at 8:28 AM Alistair Francis
 wrote:
>
> Instead of loading the kernel at a hardcoded start address, let's load
> the kernel at the next alligned address after the end of the firmware.

typo of "aligned"

>
> This should have no impact for current users of OpenSBI, but will
> allow loading a noMMU kernel at the start of memory.
>
> Signed-off-by: Alistair Francis 
> ---
>  include/hw/riscv/boot.h |  3 +++
>  hw/riscv/boot.c | 19 ++-
>  hw/riscv/opentitan.c|  3 ++-
>  hw/riscv/sifive_e.c |  3 ++-
>  hw/riscv/sifive_u.c | 10 --
>  hw/riscv/spike.c| 11 ---
>  hw/riscv/virt.c | 11 ---
>  7 files changed, 45 insertions(+), 15 deletions(-)
>

Reviewed-by: Bin Meng 
Tested-by: Bin Meng 

Re: [PATCH v2 2/4] hw/riscv: Return the end address of the loaded firmware

[Bin Meng]

On Wed, Oct 14, 2020 at 8:28 AM Alistair Francis
 wrote:
>
> Instead of returning the unused entry address from riscv_load_firmware()
> instead return the end address. Also return the end address from
> riscv_find_and_load_firmware().
>
> This tells the caller if a firmware was loaded and how big it is. This
> can be used to determine the load address of the next image (usually the
> kernel).
>
> Signed-off-by: Alistair Francis 
> ---
>  include/hw/riscv/boot.h |  8 
>  hw/riscv/boot.c | 28 +---
>  2 files changed, 21 insertions(+), 15 deletions(-)
>

Reviewed-by: Bin Meng 
Tested-by: Bin Meng 

Re: [PATCH v2 3/4] hw/riscv: Add a riscv_is_32_bit() function

[Bin Meng]

On Wed, Oct 14, 2020 at 8:28 AM Alistair Francis
 wrote:
>
> Signed-off-by: Alistair Francis 
> ---
>  include/hw/riscv/boot.h | 2 ++
>  hw/riscv/boot.c | 9 +
>  2 files changed, 11 insertions(+)
>

Reviewed-by: Bin Meng 
Tested-by: Bin Meng 

[PATCH v3 8/8] migration: Delete redundant spaces

[Bihong Yu]

Signed-off-by: Bihong Yu 
Reviewed-by: Chuan Zheng 
Reviewed-by: Dr. David Alan Gilbert 
---
 migration/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index ca4d315..00eac34 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -855,7 +855,7 @@ static int qemu_rdma_broken_ipv6_kernel(struct ibv_context 
*verbs, Error **errp)
  */
 if (!verbs) {
 int num_devices, x;
-struct ibv_device ** dev_list = ibv_get_device_list(_devices);
+struct ibv_device **dev_list = ibv_get_device_list(_devices);
 bool roce_found = false;
 bool ib_found = false;
 
-- 
1.8.3.1

[PATCH v3 2/8] migration: Don't use '#' flag of printf format

[Bihong Yu]

Signed-off-by: Bihong Yu 
Reviewed-by: Chuan Zheng 
Reviewed-by: Dr. David Alan Gilbert 
---
 migration/block.c | 2 +-
 migration/ram.c   | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 4b8576b..273392b 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -998,7 +998,7 @@ static int block_load(QEMUFile *f, void *opaque, int 
version_id)
(addr == 100) ? '\n' : '\r');
 fflush(stdout);
 } else if (!(flags & BLK_MIG_FLAG_EOS)) {
-fprintf(stderr, "Unknown block migration flags: %#x\n", flags);
+fprintf(stderr, "Unknown block migration flags: 0x%x\n", flags);
 return -EINVAL;
 }
 ret = qemu_file_get_error(f);
diff --git a/migration/ram.c b/migration/ram.c
index 433489d..6ed4f9e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3298,7 +3298,7 @@ static int ram_load_postcopy(QEMUFile *f)
 multifd_recv_sync_main();
 break;
 default:
-error_report("Unknown combination of migration flags: %#x"
+error_report("Unknown combination of migration flags: 0x%x"
  " (postcopy mode)", flags);
 ret = -EINVAL;
 break;
@@ -3576,7 +3576,7 @@ static int ram_load_precopy(QEMUFile *f)
 if (flags & RAM_SAVE_FLAG_HOOK) {
 ram_control_load_hook(f, RAM_CONTROL_HOOK, NULL);
 } else {
-error_report("Unknown combination of migration flags: %#x",
+error_report("Unknown combination of migration flags: 0x%x",
  flags);
 ret = -EINVAL;
 }
-- 
1.8.3.1

Re: [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU

[Bin Meng]

On Wed, Oct 14, 2020 at 8:28 AM Alistair Francis
 wrote:
>
> Allow the user to specify the main application CPU for the sifive_u
> machine.
>
> Signed-off-by: Alistair Francis 
> Reviewed-by: Bin Meng 
> ---
>  include/hw/riscv/sifive_u.h |  1 +
>  hw/riscv/sifive_u.c | 18 +-
>  2 files changed, 14 insertions(+), 5 deletions(-)
>

Tested-by: Bin Meng 

[PATCH v3 5/8] migration: Add braces {} for if statement

[Bihong Yu]

Signed-off-by: Bihong Yu 
Reviewed-by: Chuan Zheng 
Reviewed-by: Dr. David Alan Gilbert 
---
 migration/ram.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 0aea78f..09178cc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -101,14 +101,16 @@ static struct {
 
 static void XBZRLE_cache_lock(void)
 {
-if (migrate_use_xbzrle())
+if (migrate_use_xbzrle()) {
 qemu_mutex_lock();
+}
 }
 
 static void XBZRLE_cache_unlock(void)
 {
-if (migrate_use_xbzrle())
+if (migrate_use_xbzrle()) {
 qemu_mutex_unlock();
+}
 }
 
 /**
-- 
1.8.3.1

[PATCH v3 7/8] migration: Open brace '{' following function declarations go on the next line

[Bihong Yu]

Signed-off-by: Bihong Yu 
Reviewed-by: Chuan Zheng 
Reviewed-by: Dr. David Alan Gilbert 
---
 migration/rdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 0eb42b7..ca4d315 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -273,7 +273,8 @@ static uint64_t htonll(uint64_t v)
 return u.llv;
 }
 
-static uint64_t ntohll(uint64_t v) {
+static uint64_t ntohll(uint64_t v)
+{
 union { uint32_t lv[2]; uint64_t llv; } u;
 u.llv = v;
 return ((uint64_t)ntohl(u.lv[0]) << 32) | (uint64_t) ntohl(u.lv[1]);
-- 
1.8.3.1

[PATCH v3 3/8] migration: Add spaces around operator

[Bihong Yu]

Signed-off-by: Bihong Yu 
Reviewed-by: Chuan Zheng 
Reviewed-by: Dr. David Alan Gilbert 
---
 migration/migration.c|  4 ++--
 migration/postcopy-ram.c |  2 +-
 migration/ram.c  |  2 +-
 migration/savevm.c   |  2 +-
 migration/vmstate.c  | 10 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0575ecb..e050f57 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2478,8 +2478,8 @@ static void migrate_handle_rp_req_pages(MigrationState 
*ms, const char* rbname,
  * Since we currently insist on matching page sizes, just sanity check
  * we're being asked for whole host pages.
  */
-if (start & (our_host_ps-1) ||
-   (len & (our_host_ps-1))) {
+if (start & (our_host_ps - 1) ||
+   (len & (our_host_ps - 1))) {
 error_report("%s: Misaligned page request, start: " RAM_ADDR_FMT
  " len: %zd", __func__, start, len);
 mark_source_rp_bad(ms);
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 0a2f88a8..eea92bb 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -403,7 +403,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState 
*mis)
  strerror(errno));
 goto out;
 }
-g_assert(((size_t)testarea & (pagesize-1)) == 0);
+g_assert(((size_t)testarea & (pagesize - 1)) == 0);
 
 reg_struct.range.start = (uintptr_t)testarea;
 reg_struct.range.len = pagesize;
diff --git a/migration/ram.c b/migration/ram.c
index 6ed4f9e..0aea78f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1563,7 +1563,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t 
start, ram_addr_t len)
 rs->last_req_rb = ramblock;
 }
 trace_ram_save_queue_pages(ramblock->idstr, start, len);
-if (start+len > ramblock->used_length) {
+if (start + len > ramblock->used_length) {
 error_report("%s request overrun start=" RAM_ADDR_FMT " len="
  RAM_ADDR_FMT " blocklen=" RAM_ADDR_FMT,
  __func__, start, len, ramblock->used_length);
diff --git a/migration/savevm.c b/migration/savevm.c
index d2e141f..b21f1c1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -521,7 +521,7 @@ static const VMStateDescription vmstate_configuration = {
 VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
 VMSTATE_END_OF_LIST()
 },
-.subsections = (const VMStateDescription*[]) {
+.subsections = (const VMStateDescription *[]) {
 _target_page_bits,
 _capabilites,
 _uuid,
diff --git a/migration/vmstate.c b/migration/vmstate.c
index bafa890..e9d2aef 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -32,13 +32,13 @@ static int vmstate_n_elems(void *opaque, const VMStateField 
*field)
 if (field->flags & VMS_ARRAY) {
 n_elems = field->num;
 } else if (field->flags & VMS_VARRAY_INT32) {
-n_elems = *(int32_t *)(opaque+field->num_offset);
+n_elems = *(int32_t *)(opaque + field->num_offset);
 } else if (field->flags & VMS_VARRAY_UINT32) {
-n_elems = *(uint32_t *)(opaque+field->num_offset);
+n_elems = *(uint32_t *)(opaque + field->num_offset);
 } else if (field->flags & VMS_VARRAY_UINT16) {
-n_elems = *(uint16_t *)(opaque+field->num_offset);
+n_elems = *(uint16_t *)(opaque + field->num_offset);
 } else if (field->flags & VMS_VARRAY_UINT8) {
-n_elems = *(uint8_t *)(opaque+field->num_offset);
+n_elems = *(uint8_t *)(opaque + field->num_offset);
 }
 
 if (field->flags & VMS_MULTIPLY_ELEMENTS) {
@@ -54,7 +54,7 @@ static int vmstate_size(void *opaque, const VMStateField 
*field)
 int size = field->size;
 
 if (field->flags & VMS_VBUFFER) {
-size = *(int32_t *)(opaque+field->size_offset);
+size = *(int32_t *)(opaque + field->size_offset);
 if (field->flags & VMS_MULTIPLY) {
 size *= field->size;
 }
-- 
1.8.3.1

[PATCH v3 4/8] migration: Open brace '{' following struct go on the same line

[Bihong Yu]

Signed-off-by: Bihong Yu 
Reviewed-by: Chuan Zheng 
Reviewed-by: Dr. David Alan Gilbert 
---
 migration/migration.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index deb411a..99784b4 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -124,8 +124,7 @@ struct MigrationClass {
 DeviceClass parent_class;
 };
 
-struct MigrationState
-{
+struct MigrationState {
 /*< private >*/
 DeviceState parent_obj;
 
-- 
1.8.3.1

[PATCH v3 6/8] migration: Do not initialise statics and globals to 0 or NULL

[Bihong Yu]

Signed-off-by: Bihong Yu 
Reviewed-by: Chuan Zheng 
Reviewed-by: Dr. David Alan Gilbert 
---
 migration/ram.c| 2 +-
 migration/savevm.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 09178cc..2da2b62 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2743,7 +2743,7 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void 
*host)
  */
 static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags)
 {
-static RAMBlock *block = NULL;
+static RAMBlock *block;
 char id[256];
 uint8_t len;
 
diff --git a/migration/savevm.c b/migration/savevm.c
index b21f1c1..57368bd 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -64,7 +64,7 @@
 #include "qemu/bitmap.h"
 #include "net/announce.h"
 
-const unsigned int postcopy_ram_discard_version = 0;
+const unsigned int postcopy_ram_discard_version;
 
 /* Subcommands for QEMU_VM_COMMAND */
 enum qemu_vm_cmd {
-- 
1.8.3.1

[PATCH v3 0/8] Fix some style problems in migration

[Bihong Yu]

Recently I am reading migration related code, find some style problems in
migration directory while using checkpatch.pl to check migration code. Fix the
error style problems.

v2:
- fix Signed-off-by error
- fix printf format error: "%0x" -> "0x%x"

v3:
- change "VMStateDescription * []" to "VMStateDescription *[]"

Bihong Yu (8):
  migration: Do not use C99 // comments
  migration: Don't use '#' flag of printf format
  migration: Add spaces around operator
  migration: Open brace '{' following struct go on the same line
  migration: Add braces {} for if statement
  migration: Do not initialise statics and globals to 0 or NULL
  migration: Open brace '{' following function declarations go on the
next line
  migration: Delete redundant spaces

 migration/block.c|  4 ++--
 migration/migration.c|  4 ++--
 migration/migration.h|  3 +--
 migration/postcopy-ram.c |  2 +-
 migration/ram.c  | 14 --
 migration/rdma.c |  7 ---
 migration/savevm.c   |  4 ++--
 migration/vmstate.c  | 10 +-
 8 files changed, 25 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/8] migration: Do not use C99 // comments

[Bihong Yu]

Signed-off-by: Bihong Yu 
Reviewed-by: Chuan Zheng 
Reviewed-by: Dr. David Alan Gilbert 
---
 migration/block.c | 2 +-
 migration/rdma.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 737b649..4b8576b 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -40,7 +40,7 @@
 #define MAX_IO_BUFFERS 512
 #define MAX_PARALLEL_IO 16
 
-//#define DEBUG_BLK_MIGRATION
+/* #define DEBUG_BLK_MIGRATION */
 
 #ifdef DEBUG_BLK_MIGRATION
 #define DPRINTF(fmt, ...) \
diff --git a/migration/rdma.c b/migration/rdma.c
index 0340841..0eb42b7 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1288,7 +1288,7 @@ const char *print_wrid(int wrid)
  * workload information or LRU information is available, do not attempt to use
  * this feature except for basic testing.
  */
-//#define RDMA_UNREGISTRATION_EXAMPLE
+/* #define RDMA_UNREGISTRATION_EXAMPLE */
 
 /*
  * Perform a non-optimized memory unregistration after every transfer
-- 
1.8.3.1

Re: [PATCH v2] Adding ani's email as an individual contributor

[Ani Sinha]

more ping ...

On Thu, Oct 15, 2020 at 8:22 PM Ani Sinha  wrote:

> Ping ...
>
> On Mon, Oct 12, 2020 at 8:27 PM Ani Sinha  wrote:
> >
> > Request to queue this patch for the next pull.
> >
> > On Wed, Oct 7, 2020 at 23:25 Philippe Mathieu-Daudé 
> wrote:
> >>
> >> On 10/7/20 6:19 PM, Ani Sinha wrote:
> >> > Ani is an individual contributor into qemu project. Adding my email
> into the
> >> > correct file to reflect so.
> >> >
> >>
> >> Reviewed-by: Philippe Mathieu-Daudé 
> >> Thanks!
> >>
> >> > Signed-off-by: Ani Sinha 
> >> > ---
> >> >  contrib/gitdm/group-map-individuals | 1 +
> >> >  1 file changed, 1 insertion(+)
> >> >
> >> > changelog:
> >> > v2: removed accidentally added submodule update into this commit
> >> > v1: initial patch
> >> >
> >> > diff --git a/contrib/gitdm/group-map-individuals
> b/contrib/gitdm/group-map-individuals
> >> > index cf8a2ce367..64cb859193 100644
> >> > --- a/contrib/gitdm/group-map-individuals
> >> > +++ b/contrib/gitdm/group-map-individuals
> >> > @@ -16,3 +16,4 @@ aurel...@aurel32.net
> >> >  bala...@eik.bme.hu
> >> >  e.emanuelegiuse...@gmail.com
> >> >  andrew.smir...@gmail.com
> >> > +a...@anisinha.ca
> >> >
>

Re: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[Ying Fang]

On 10/16/2020 6:07 PM, Andrew Jones wrote:

On Fri, Oct 16, 2020 at 05:40:02PM +0800, Ying Fang wrote:



On 10/15/2020 3:59 PM, Andrew Jones wrote:

On Thu, Oct 15, 2020 at 10:07:16AM +0800, Ying Fang wrote:



On 10/14/2020 2:08 AM, Andrew Jones wrote:

On Tue, Oct 13, 2020 at 12:11:20PM +, Zengtao (B) wrote:

Cc valentin


-Original Message-
From: Qemu-devel
[mailto:qemu-devel-bounces+prime.zeng=hisilicon@nongnu.org]
On Behalf Of Ying Fang
Sent: Thursday, September 17, 2020 11:20 AM
To: qemu-devel@nongnu.org
Cc: peter.mayd...@linaro.org; drjo...@redhat.com; Zhanghailiang;
Chenzhendong (alex); shannon.zha...@gmail.com;
qemu-...@nongnu.org; alistair.fran...@wdc.com; fangying;
imamm...@redhat.com
Subject: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache
topology support

An accurate cpu topology may help improve the cpu scheduler's
decision
making when dealing with multi-core system. So cpu topology
description
is helpful to provide guest with the right view. Cpu cache information
may
also have slight impact on the sched domain, and even userspace
software
may check the cpu cache information to do some optimizations. Thus
this patch
series is posted to provide cpu and cache topology support for arm.

To make the cpu topology consistent with MPIDR, an vcpu ioctl


For aarch64, the cpu topology don't depends on the MPDIR.
See https://patchwork.kernel.org/patch/11744387/



The topology should not be inferred from the MPIDR Aff fields,


MPIDR is abused by ARM OEM manufactures. It is only used as a
identifer for a specific cpu, not representation of the topology.


Right, which is why I stated topology should not be inferred from
it.




but MPIDR is the CPU identifier. When describing a topology
with ACPI or DT the CPU elements in the topology description
must map to actual CPUs. MPIDR is that mapping link. KVM
currently determines what the MPIDR of a VCPU is. If KVM


KVM currently assigns MPIDR with vcpu->vcpu_id which mapped
into affinity levels. See reset_mpidr in sys_regs.c


I know, but how KVM assigns MPIDRs today is not really important
to KVM userspace. KVM userspace shouldn't depend on a KVM
algorithm, as it could change.




userspace is going to determine the VCPU topology, then it
also needs control over the MPIDR values, otherwise it
becomes quite messy trying to get the mapping right.

If we are going to control MPIDR, shall we assign MPIDR with
vcpu_id or map topology hierarchy into affinity levels or any
other link schema ?



We can assign them to whatever we want, as long as they're
unique and as long as Aff0 is assigned per the GIC requirements,
e.g. GICv3 requires that Aff0 be from 0 to 0xf. Also, when
pinning VCPUs to PCPUs we should ensure that MPIDRs with matching
Aff3,Aff2,Aff1 fields should actually be peers with respect to
the GIC.


Still not clear why vCPU's MPIDR need to match pPCPU's GIC affinity.
Maybe I should read spec for GICv3.


Look at how IPIs are efficiently sent to "peers", where the definition
of a peer is that only Aff0 differs in its MPIDR. But, gicv3's
optimizations can only handle 16 peers. If we want pinned VCPUs to
have the same performance as PCPUS, then we should maintain this
Aff0 limit.


Yes I see. I think *virt_cpu_mp_affinity* in qemu has limit
on the clustersz. It groups every 16 vCPUs into a cluster
and then mapped into the first two affinity levels.

Thanks.
Ying.



Thanks,
drew





We shouldn't try to encode topology in the MPIDR in any way,
so we might as well simply increment a counter to assign them,
which could possibly be the same as the VCPU ID.


Hmm, then we can leave it as it is.



Thanks,
drew

.





.

Re: [PATCH v2 3/8] migration: Add spaces around operator

[Bihong Yu]

OK, I will change it to "VMStateDescription *[]". Thank you for your review.

On 2020/10/19 19:59, Markus Armbruster wrote:
> Bihong Yu  writes:
> 
>> Yes, I used to think "const VMStateDescription *[]" was right, but when I 
>> search
>> similar expressions, most of all are "xxx * []". Such as:
>> fsdev/qemu-fsdev.c:54:.opts = (const char * [])
>> hw/intc/s390_flic_kvm.c:567:.subsections = (const VMStateDescription * 
>> [])
>> ...
> 
> All three variations occur in the code: no space, space on both sides,
> space only on the left.
> 
>> So, I keep the same style. Should I change it to "const VMStateDescription 
>> *[]"?
> 
> Dropping the change to savevm.c should be fine.
> 
> Changing it to "VMStateDescription *[]" should be also fine.
> 
> I figure you can keep David's R-by in both cases.
> 
> [...]
> 
> .
> 

Re: [RFC PATCH v7 2/2] hw/misc/sifive_u_otp: Add backend drive support

[Green Wan]

Sorry for replying late. I missed this email. I will revise the patch
today. Thanks,

- Green

On Thu, Oct 15, 2020 at 4:01 PM Bin Meng  wrote:
>
> On Thu, Oct 15, 2020 at 12:15 PM Green Wan  wrote:
> >
> > Add '-drive' support to OTP device. Allow users to assign a raw file
> > as OTP image.
> >
> > test commands for 16k otp.img filled with zero:
> >
> > $ dd if=/dev/zero of=./otp.img bs=1k count=16
> > $ ./qemu-system-riscv64 -M sifive_u -m 256M -nographic -bios none \
> > -kernel ../opensbi/build/platform/sifive/fu540/firmware/fw_payload.elf \
> > -d guest_errors -drive if=none,format=raw,file=otp.img
> >
> > Signed-off-by: Green Wan 
> > ---
> >  hw/misc/sifive_u_otp.c | 67 +-
> >  include/hw/misc/sifive_u_otp.h |  2 +
> >  2 files changed, 68 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
> > index 565eec082f..60066375ab 100644
> > --- a/hw/misc/sifive_u_otp.c
> > +++ b/hw/misc/sifive_u_otp.c
> > @@ -19,11 +19,14 @@
> >   */
> >
> >  #include "qemu/osdep.h"
> > +#include "qapi/error.h"
> >  #include "hw/qdev-properties.h"
> >  #include "hw/sysbus.h"
> >  #include "qemu/log.h"
> >  #include "qemu/module.h"
> >  #include "hw/misc/sifive_u_otp.h"
> > +#include "sysemu/blockdev.h"
> > +#include "sysemu/block-backend.h"
> >
> >  #define WRITTEN_BIT_ON 0x1
> >
> > @@ -54,6 +57,16 @@ static uint64_t sifive_u_otp_read(void *opaque, hwaddr 
> > addr, unsigned int size)
> >  if ((s->pce & SIFIVE_U_OTP_PCE_EN) &&
> >  (s->pdstb & SIFIVE_U_OTP_PDSTB_EN) &&
> >  (s->ptrim & SIFIVE_U_OTP_PTRIM_EN)) {
> > +
> > +/* read from backend */
> > +if (s->blk) {
> > +int32_t buf;
> > +
> > +blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, ,
> > +  SIFIVE_U_OTP_FUSE_WORD);
> > +return buf;
> > +}
> > +
> >  return s->fuse[s->pa & SIFIVE_U_OTP_PA_MASK];
> >  } else {
> >  return 0xff;
> > @@ -137,7 +150,7 @@ static void sifive_u_otp_write(void *opaque, hwaddr 
> > addr,
> >  if (s->pwe && !s->pas) {
> >  if (GET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio)) {
> >  qemu_log_mask(LOG_GUEST_ERROR,
> > -  "Error: write idx<%u>, bit<%u>\n",
> > +  "write once error: idx<%u>, bit<%u>\n",
>
> This should be in the patch 1.
>
> >s->pa, s->paio);
> >  break;
> >  }
> > @@ -145,6 +158,12 @@ static void sifive_u_otp_write(void *opaque, hwaddr 
> > addr,
> >  /* write bit data */
> >  SET_FUSEARRAY_BIT(s->fuse, s->pa, s->paio, s->pdin);
> >
> > +/* write to backend */
> > +if (s->blk) {
> > +blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD,
> > +   >fuse[s->pa], SIFIVE_U_OTP_FUSE_WORD, 0);
> > +}
> > +
> >  /* update written bit */
> >  SET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio, WRITTEN_BIT_ON);
> >  }
> > @@ -168,16 +187,48 @@ static const MemoryRegionOps sifive_u_otp_ops = {
> >
> >  static Property sifive_u_otp_properties[] = {
> >  DEFINE_PROP_UINT32("serial", SiFiveUOTPState, serial, 0),
> > +DEFINE_PROP_DRIVE("drive", SiFiveUOTPState, blk),
> >  DEFINE_PROP_END_OF_LIST(),
> >  };
> >
>
> Otherwise,
> Reviewed-by: Bin Meng 
> Tested-by: Bin Meng 
>
> You can drop the "RFC" tag in the next version.

[PATCH v2 4/6] virtio-gpu: fix incorrect print type

[Zhengui li]

The type of input variable is unsigned int
while the printer type is int. So fix incorrect print type.

Signed-off-by: Zhengui li 
---
 hw/display/virtio-gpu.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 90be4e3..d785d88 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -275,7 +275,7 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
 
 res = virtio_gpu_find_resource(g, c2d.resource_id);
 if (res) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
+qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %u\n",
   __func__, c2d.resource_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
 return;
@@ -291,7 +291,7 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
 pformat = virtio_gpu_get_pixman_format(c2d.format);
 if (!pformat) {
 qemu_log_mask(LOG_GUEST_ERROR,
-  "%s: host couldn't handle guest format %d\n",
+  "%s: host couldn't handle guest format %u\n",
   __func__, c2d.format);
 g_free(res);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
@@ -308,7 +308,7 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
 
 if (!res->image) {
 qemu_log_mask(LOG_GUEST_ERROR,
-  "%s: resource creation failed %d %d %d\n",
+  "%s: resource creation failed %u %u %u\n",
   __func__, c2d.resource_id, c2d.width, c2d.height);
 g_free(res);
 cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
@@ -379,7 +379,7 @@ static void virtio_gpu_resource_unref(VirtIOGPU *g,
 
 res = virtio_gpu_find_resource(g, unref.resource_id);
 if (!res) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal resource specified %d\n",
+qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal resource specified %u\n",
   __func__, unref.resource_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
 return;
@@ -403,7 +403,7 @@ static void virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,
 
 res = virtio_gpu_find_resource(g, t2d.resource_id);
 if (!res || !res->iov) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal resource specified %d\n",
+qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal resource specified %u\n",
   __func__, t2d.resource_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
 return;
@@ -416,7 +416,7 @@ static void virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,
 t2d.r.x + t2d.r.width > res->width ||
 t2d.r.y + t2d.r.height > res->height) {
 qemu_log_mask(LOG_GUEST_ERROR, "%s: transfer bounds outside resource"
-  " bounds for resource %d: %d %d %d %d vs %d %d\n",
+  " bounds for resource %u: %u %u %u %u vs %u %u\n",
   __func__, t2d.resource_id, t2d.r.x, t2d.r.y,
   t2d.r.width, t2d.r.height, res->width, res->height);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
@@ -461,7 +461,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
 
 res = virtio_gpu_find_resource(g, rf.resource_id);
 if (!res) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal resource specified %d\n",
+qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal resource specified %u\n",
   __func__, rf.resource_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
 return;
@@ -474,7 +474,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
 rf.r.x + rf.r.width > res->width ||
 rf.r.y + rf.r.height > res->height) {
 qemu_log_mask(LOG_GUEST_ERROR, "%s: flush bounds outside resource"
-  " bounds for resource %d: %d %d %d %d vs %d %d\n",
+  " bounds for resource %u: %u %u %u %u vs %u %u\n",
   __func__, rf.resource_id, rf.r.x, rf.r.y,
   rf.r.width, rf.r.height, res->width, res->height);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
@@ -533,7 +533,7 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
  ss.r.width, ss.r.height, ss.r.x, ss.r.y);
 
 if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
+qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %u",
   __func__, ss.scanout_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
 return;
@@ -548,7 +548,7 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
 /* create a surface for this scanout */
 res = virtio_gpu_find_resource(g, ss.resource_id);
 if (!res) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal resource 

[PATCH v2 5/6] virtio-iommu: fix incorrect print type

[Zhengui li]

The type of input variable is unsigned int
while the printer type is int. So fix incorrect print type.

Signed-off-by: Zhengui li 
---
 hw/virtio/virtio-iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 21ec63b..bd6ce44 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -632,7 +632,7 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(sid));
 if (!ep) {
 if (!bypass_allowed) {
-error_report_once("%s sid=%d is not known!!", __func__, sid);
+error_report_once("%s sid=%u is not known!!", __func__, sid);
 virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_UNKNOWN,
   VIRTIO_IOMMU_FAULT_F_ADDRESS,
   sid, addr);
@@ -679,7 +679,7 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
(void **)_key,
(void **)_value);
 if (!found) {
-error_report_once("%s no mapping for 0x%"PRIx64" for sid=%d",
+error_report_once("%s no mapping for 0x%"PRIx64" for sid=%u",
   __func__, addr, sid);
 virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
   VIRTIO_IOMMU_FAULT_F_ADDRESS,
@@ -695,7 +695,7 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 flags = read_fault ? VIRTIO_IOMMU_FAULT_F_READ : 0;
 flags |= write_fault ? VIRTIO_IOMMU_FAULT_F_WRITE : 0;
 if (flags) {
-error_report_once("%s permission error on 0x%"PRIx64"(%d): allowed=%d",
+error_report_once("%s permission error on 0x%"PRIx64"(%d): allowed=%u",
   __func__, addr, flag, mapping_value->flags);
 flags |= VIRTIO_IOMMU_FAULT_F_ADDRESS;
 virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
-- 
2.6.4.windows.1

[PATCH v2 6/6] vfio: fix incorrect print type

[Zhengui li]

The type of input variable is unsigned int
while the printer type is int. So fix incorrect print type.

Signed-off-by: Zhengui li 
---
 hw/vfio/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 13471ae..acc3356 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -203,7 +203,7 @@ void vfio_region_write(void *opaque, hwaddr addr,
 buf.qword = cpu_to_le64(data);
 break;
 default:
-hw_error("vfio: unsupported write size, %d bytes", size);
+hw_error("vfio: unsupported write size, %u bytes", size);
 break;
 }
 
@@ -260,7 +260,7 @@ uint64_t vfio_region_read(void *opaque,
 data = le64_to_cpu(buf.qword);
 break;
 default:
-hw_error("vfio: unsupported read size, %d bytes", size);
+hw_error("vfio: unsupported read size, %u bytes", size);
 break;
 }
 
-- 
2.6.4.windows.1

[PATCH v2 3/6] vhost-user: fix incorrect print type

[Zhengui li]

The type of input variable is unsigned int
while the printer type is int. So fix incorrect print type.

Signed-off-by: Zhengui li 
---
 hw/virtio/vhost-user.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 9c5b4f7..db563bd 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -308,7 +308,7 @@ static int vhost_user_read(struct vhost_dev *dev, 
VhostUserMsg *msg)
 /* validate message size is sane */
 if (msg->hdr.size > VHOST_USER_PAYLOAD_SIZE) {
 error_report("Failed to read msg header."
-" Size %d exceeds the maximum %zu.", msg->hdr.size,
+" Size %u exceeds the maximum %zu.", msg->hdr.size,
 VHOST_USER_PAYLOAD_SIZE);
 return -1;
 }
@@ -319,7 +319,7 @@ static int vhost_user_read(struct vhost_dev *dev, 
VhostUserMsg *msg)
 r = qemu_chr_fe_read_all(chr, p, size);
 if (r != size) {
 error_report("Failed to read msg payload."
- " Read %d instead of %d.", r, msg->hdr.size);
+ " Read %d instead of %u.", r, msg->hdr.size);
 return -1;
 }
 }
@@ -740,7 +740,7 @@ static int send_add_regions(struct vhost_dev *dev,
  */
 if (msg_reply.hdr.size != msg->hdr.size) {
 error_report("%s: Unexpected size for postcopy reply "
- "%d vs %d", __func__, msg_reply.hdr.size,
+ "%u vs %u", __func__, msg_reply.hdr.size,
  msg->hdr.size);
 return -1;
 }
@@ -905,7 +905,7 @@ static int vhost_user_set_mem_table_postcopy(struct 
vhost_dev *dev,
  */
 if (msg_reply.hdr.size != msg.hdr.size) {
 error_report("%s: Unexpected size for postcopy reply "
- "%d vs %d", __func__, msg_reply.hdr.size,
+ "%u vs %u", __func__, msg_reply.hdr.size,
  msg.hdr.size);
 return -1;
 }
@@ -1445,7 +1445,7 @@ static void slave_read(void *opaque)
 
 if (hdr.size > VHOST_USER_PAYLOAD_SIZE) {
 error_report("Failed to read msg header."
-" Size %d exceeds the maximum %zu.", hdr.size,
+" Size %u exceeds the maximum %zu.", hdr.size,
 VHOST_USER_PAYLOAD_SIZE);
 goto err;
 }
-- 
2.6.4.windows.1

[PATCH v2 2/6] vhost-user-scsi: fix incorrect print type

[Zhengui li]

The type of input variable is unsigned int
while the printer type is int. So fix incorrect print type.

Signed-off-by: Zhengui li 
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 3c91238..1527ffd 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -182,7 +182,7 @@ static int handle_cmd_sync(struct iscsi_context *ctx,
 task->iovector_in.niov = in_len;
 }
 
-g_debug("Sending iscsi cmd (cdb_len=%d, dir=%d, task=%p)",
+g_debug("Sending iscsi cmd (cdb_len=%d, dir=%u, task=%p)",
  cdb_len, dir, task);
 if (!iscsi_scsi_command_sync(ctx, 0, task, NULL)) {
 g_warning("Error serving SCSI command");
-- 
2.6.4.windows.1

[PATCH v2 1/6] vhost-user-gpu: fix incorrect print type

[Zhengui li]

The type of input variable is unsigned int
while the printer type is int. So fix incorrect print type.

Signed-off-by: Zhengui li 
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 34 -
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c 
b/contrib/vhost-user-gpu/vhost-user-gpu.c
index a019d0a..ee2bf59 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -299,7 +299,7 @@ vg_resource_create_2d(VuGpu *g,
 
 res = virtio_gpu_find_resource(g, c2d.resource_id);
 if (res) {
-g_critical("%s: resource already exists %d", __func__, 
c2d.resource_id);
+g_critical("%s: resource already exists %u", __func__, 
c2d.resource_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
 return;
 }
@@ -312,7 +312,7 @@ vg_resource_create_2d(VuGpu *g,
 
 pformat = virtio_gpu_get_pixman_format(c2d.format);
 if (!pformat) {
-g_critical("%s: host couldn't handle guest format %d",
+g_critical("%s: host couldn't handle guest format %u",
__func__, c2d.format);
 g_free(res);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
@@ -325,7 +325,7 @@ vg_resource_create_2d(VuGpu *g,
   (uint32_t *)res->buffer.mmap,
   res->buffer.stride);
 if (!res->image) {
-g_critical("%s: resource creation failed %d %d %d",
+g_critical("%s: resource creation failed %u %u %u",
__func__, c2d.resource_id, c2d.width, c2d.height);
 g_free(res);
 cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
@@ -395,7 +395,7 @@ vg_resource_unref(VuGpu *g,
 
 res = virtio_gpu_find_resource(g, unref.resource_id);
 if (!res) {
-g_critical("%s: illegal resource specified %d",
+g_critical("%s: illegal resource specified %u",
__func__, unref.resource_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
 return;
@@ -414,7 +414,7 @@ vg_create_mapping_iov(VuGpu *g,
 int i;
 
 if (ab->nr_entries > 16384) {
-g_critical("%s: nr_entries is too big (%d > 16384)",
+g_critical("%s: nr_entries is too big (%u > 16384)",
__func__, ab->nr_entries);
 return -1;
 }
@@ -436,7 +436,7 @@ vg_create_mapping_iov(VuGpu *g,
 (*iov)[i].iov_len = ents[i].length;
 (*iov)[i].iov_base = vu_gpa_to_va(>dev.parent, , ents[i].addr);
 if (!(*iov)[i].iov_base || len != ents[i].length) {
-g_critical("%s: resource %d element %d",
+g_critical("%s: resource %u element %d",
__func__, ab->resource_id, i);
 g_free(*iov);
 g_free(ents);
@@ -461,7 +461,7 @@ vg_resource_attach_backing(VuGpu *g,
 
 res = virtio_gpu_find_resource(g, ab.resource_id);
 if (!res) {
-g_critical("%s: illegal resource specified %d",
+g_critical("%s: illegal resource specified %u",
__func__, ab.resource_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
 return;
@@ -488,7 +488,7 @@ vg_resource_detach_backing(VuGpu *g,
 
 res = virtio_gpu_find_resource(g, detach.resource_id);
 if (!res || !res->iov) {
-g_critical("%s: illegal resource specified %d",
+g_critical("%s: illegal resource specified %u",
__func__, detach.resource_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
 return;
@@ -515,7 +515,7 @@ vg_transfer_to_host_2d(VuGpu *g,
 
 res = virtio_gpu_find_resource(g, t2d.resource_id);
 if (!res || !res->iov) {
-g_critical("%s: illegal resource specified %d",
+g_critical("%s: illegal resource specified %u",
__func__, t2d.resource_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
 return;
@@ -528,7 +528,7 @@ vg_transfer_to_host_2d(VuGpu *g,
 t2d.r.x + t2d.r.width > res->width ||
 t2d.r.y + t2d.r.height > res->height) {
 g_critical("%s: transfer bounds outside resource"
-   " bounds for resource %d: %d %d %d %d vs %d %d",
+   " bounds for resource %u: %u %u %u %u vs %u %u",
__func__, t2d.resource_id, t2d.r.x, t2d.r.y,
t2d.r.width, t2d.r.height, res->width, res->height);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
@@ -571,7 +571,7 @@ vg_set_scanout(VuGpu *g,
 virtio_gpu_bswap_32(, sizeof(ss));
 
 if (ss.scanout_id >= VIRTIO_GPU_MAX_SCANOUTS) {
-g_critical("%s: illegal scanout id specified %d",
+g_critical("%s: illegal scanout id specified %u",
__func__, ss.scanout_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
 return;
@@ -585,7 +585,7 @@ vg_set_scanout(VuGpu *g,
 /* create a 

Re: [PATCH V14 6/8] hw/mips: Add Loongson-3 boot parameter helpers

[Huacai Chen]

Hi, Philippe,

On Fri, Oct 16, 2020 at 10:24 PM Philippe Mathieu-Daudé  wrote:
>
> Hi Huacai,
>
> On 10/16/20 8:51 AM, Huacai Chen wrote:
> > Preparing to add Loongson-3 machine support, add Loongson-3's LEFI (a
> > UEFI-like interface for BIOS-Kernel boot parameters) helpers first.
> >
> > Signed-off-by: Huacai Chen 
> > Co-developed-by: Jiaxun Yang 
>
>  From the kernel documentation [*] on the "Co-developed-by" tag:
>
>A Co-Developed-by: states that the patch was also created
>by another developer along with the original author. This
>is useful at times when multiple people work on a single
>patch. Note, this person also needs to have a Signed-off-by:
>line in the patch as well.
>
> Can Jiaxun Yang add his Signed-off-by tag?
OK, I will add him.

>
> [*]
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
>
> > ---
> >   hw/mips/loongson3_bootp.c | 162 +++
> >   hw/mips/loongson3_bootp.h | 225 ++
> >   hw/mips/meson.build   |   1 +
> >   3 files changed, 388 insertions(+)
> >   create mode 100644 hw/mips/loongson3_bootp.c
> >   create mode 100644 hw/mips/loongson3_bootp.h
>
> Consider using scripts/git.orderfile to avoid reviewer
> scrolling down/up/down/up.
OK, I will do.

>
> >
> > diff --git a/hw/mips/loongson3_bootp.c b/hw/mips/loongson3_bootp.c
> > new file mode 100644
> > index 00..eab6f51a01
> > --- /dev/null
> > +++ b/hw/mips/loongson3_bootp.c
> > @@ -0,0 +1,162 @@
> > +/*
> > + * LEFI (a UEFI-like interface for BIOS-Kernel boot parameters) helpers
> > + *
> > + * Copyright (c) 2017-2020 Huacai Chen (che...@lemote.com)
> > + * Copyright (c) 2017-2020 Jiaxun Yang 
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program. If not, see .
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/units.h"
> > +#include "qemu/cutils.h"
> > +#include "cpu.h"
> > +#include "hw/boards.h"
> > +#include "hw/mips/loongson3_bootp.h"
> > +
> > +static struct efi_cpuinfo_loongson *init_cpu_info(void *g_cpuinfo, 
> > uint64_t cpu_freq)
> > +{
> > +struct efi_cpuinfo_loongson *c = g_cpuinfo;
> > +
> > +stl_le_p(>cputype, Loongson_3A);
> > +stl_le_p(>processor_id, MIPS_CPU(first_cpu)->env.CP0_PRid);
> > +if (cpu_freq > UINT_MAX) {
> > +stl_le_p(>cpu_clock_freq, UINT_MAX);
> > +} else {
> > +stl_le_p(>cpu_clock_freq, cpu_freq);
> > +}
> > +
> > +stw_le_p(>cpu_startup_core_id, 0);
> > +stl_le_p(>nr_cpus, current_machine->smp.cpus);
> > +stl_le_p(>total_node, (current_machine->smp.cpus + 3) / 4);
>
> Please replace this magic values by a definition, such:
>
> #define LOONGSON3_CORE_PER_NODE 4
>
> Then you can use:
>
> stl_le_p(>total_node, DIV_ROUND_UP(current_machine->smp.cpus,
>   LOONGSON3_CORE_PER_NODE));
>
OK, Thank you.

> > +
> > +return c;
> > +}
> > +
> > +static struct efi_memory_map_loongson *init_memory_map(void *g_map, 
> > uint64_t ram_size)
> > +{
> > +struct efi_memory_map_loongson *emap = g_map;
> > +
> > +stl_le_p(>nr_map, 2);
> > +stl_le_p(>mem_freq, 3);
> > +
> > +stl_le_p(>map[0].node_id, 0);
> > +stl_le_p(>map[0].mem_type, 1);
> > +stq_le_p(>map[0].mem_start, 0x0);
> > +stl_le_p(>map[0].mem_size, 240);
> > +
> > +stl_le_p(>map[1].node_id, 0);
> > +stl_le_p(>map[1].mem_type, 2);
> > +stq_le_p(>map[1].mem_start, 0x9000);
> > +stl_le_p(>map[1].mem_size, (ram_size / MiB) - 256);
> > +
> > +return emap;
> > +}
> > +
> > +static struct system_loongson *init_system_loongson(void *g_system)
> > +{
> > +struct system_loongson *s = g_system;
> > +
> > +stl_le_p(>ccnuma_smp, 0);
> > +stl_le_p(>sing_double_channel, 1);
> > +stl_le_p(>nr_uarts, 1);
> > +stl_le_p(>uarts[0].iotype, 2);
> > +stl_le_p(>uarts[0].int_offset, 2);
> > +stl_le_p(>uarts[0].uartclk, 2500); /* Random value */
> > +stq_le_p(>uarts[0].uart_base, virt_memmap[VIRT_UART].base);
> > +
> > +return s;
> > +}
> > +
> > +static struct irq_source_routing_table *init_irq_source(void *g_irq_source)
> > +{
> > +struct irq_source_routing_table *irq_info = g_irq_source;
> > +
> > +stl_le_p(_info->node_id, 0);
> > +stl_le_p(_info->PIC_type, 0);
> > +

Re: [PATCH v2 0/5] memory: Skip assertion in memory_region_unregister_iommu_notifier

[Jason Wang]

On 2020/10/19 下午6:42, Eugenio Pérez wrote:

I am able to hit this assertion when a Red Hat 7 guest virtio_net device
raises an "Invalidation" of all the TLB entries. This happens in the
guest's startup if 'intel_iommu=on' argument is passed to the guest
kernel and right IOMMU/ATS devices are declared in qemu's command line.

Command line:
/home/qemu/x86_64-softmmu/qemu-system-x86_64 -name \
guest=rhel7-test,debug-threads=on -machine \
pc-q35-5.1,accel=kvm,usb=off,dump-guest-core=off,kernel_irqchip=split \
-cpu \
Broadwell,vme=on,ss=on,vmx=on,f16c=on,rdrand=on,hypervisor=on,arat=on,tsc-adjust=on,umip=on,arch-capabilities=on,xsaveopt=on,pdpe1gb=on,abm=on,skip-l1dfl-vmentry=on,rtm=on,hle=on
 \
-m 8096 -realtime mlock=off -smp 2,sockets=2,cores=1,threads=1 -uuid \
d022ecbf-679e-4755-87ce-eb87fc5bbc5d -display none -no-user-config \
-nodefaults -rtc base=utc,driftfix=slew -global \
kvm-pit.lost_tick_policy=delay -no-hpet -no-shutdown -global \
ICH9-LPC.disable_s3=1 -global ICH9-LPC.disable_s4=1 -boot strict=on \
-device intel-iommu,intremap=on,device-iotlb=on -device \
pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x1 
\
-device \
pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \
-device \
pcie-root-port,port=0xa,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x2 \
-device \
pcie-root-port,port=0xb,chassis=4,id=pci.4,bus=pcie.0,addr=0x1.0x3 \
-device \
pcie-root-port,port=0xc,chassis=5,id=pci.5,bus=pcie.0,addr=0x1.0x4 \
-device \
pcie-root-port,port=0xd,chassis=6,id=pci.6,bus=pcie.0,addr=0x1.0x5 \
-device \
pcie-root-port,port=0xe,chassis=7,id=pci.7,bus=pcie.0,addr=0x1.0x6 \
-device qemu-xhci,p2=15,p3=15,id=usb,bus=pci.2,addr=0x0 -device \
virtio-serial-pci,id=virtio-serial0,bus=pci.3,addr=0x0 -drive \
file=/home/virtio-test2.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 \
-device \
virtio-blk-pci,scsi=off,bus=pci.4,addr=0x0,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
 \
-netdev tap,id=hostnet0,vhost=on,vhostforce=on -device \
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:0d:1d:f2,bus=pci.1,addr=0x0,iommu_platform=on,ats=on
 \
-device virtio-balloon-pci,id=balloon0,bus=pci.5,addr=0x0 -object \
rng-random,id=objrng0,filename=/dev/urandom -device \
virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.6,addr=0x0 -s -msg \
timestamp=on

Full backtrace:
  #0  0x7521370f in raise () at /lib64/libc.so.6
  #1  0x751fdb25 in abort () at /lib64/libc.so.6
  #2  0x751fd9f9 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
  #3  0x7520bcc6 in .annobin_assert.c_end () at /lib64/libc.so.6
  #4  0x55888171 in memory_region_notify_one (notifier=0x7ffde0487fa8,
 entry=0x7ffde5dfe200)
   at /home/qemu/memory.c:1918
  #5  0x55888247 in memory_region_notify_iommu (iommu_mr=0x56f6c0b0,
   iommu_idx=0, entry=...)
   at /home/qemu/memory.c:1941
  #6  0x55951c8d in vtd_process_device_iotlb_desc (s=0x57609000,
inv_desc=0x7ffde5dfe2d0)
   at /home/qemu/hw/i386/intel_iommu.c:2468
  #7  0x55951e6a in vtd_process_inv_desc (s=0x57609000)
   at /home/qemu/hw/i386/intel_iommu.c:2531
  #8  0x55951fa5 in vtd_fetch_inv_desc (s=0x57609000)
   at /home/qemu/hw/i386/intel_iommu.c:2563
  #9  0x559520e5 in vtd_handle_iqt_write (s=0x57609000)
   at /home/qemu/hw/i386/intel_iommu.c:2590
  #10 0x55952b45 in vtd_mem_write (opaque=0x57609000, addr=136,
  val=2688, size=4)
   at /home/qemu/hw/i386/intel_iommu.c:2837
  #11 0x55883e17 in memory_region_write_accessor (mr=0x57609330,
 addr=136,
 value=0x7ffde5dfe478,
 size=4,
 shift=0,
 mask=4294967295,
 attrs=...)
  at /home/qemu/memory.c:483
  #12 0x5588401d in access_with_adjusted_size (addr=136,
value=0x7ffde5dfe478,
size=4,
access_size_min=4,
access_size_max=8,
access_fn=0x55883d38 ,
mr=0x57609330,
attrs=...)
at /home/qemu/memory.c:544
  #13 0x55886f37 in memory_region_dispatch_write (mr=0x57609330,
addr=136,
 

Re: [PATCH V14 1/8] target/mips: Fix PageMask with variable page size

[Huacai Chen]

Hi, Philippe,

On Fri, Oct 16, 2020 at 11:15 PM Philippe Mathieu-Daudé  wrote:
>
> On 10/16/20 8:51 AM, Huacai Chen wrote:
> > From: Jiaxun Yang 
> >
> > Our current code assumed the target page size is always 4k
> > when handling PageMask and VPN2, however, variable page size
> > was just added to mips target and that's no longer true.
> >
> > Fixes: ee3863b9d414 ("target/mips: Support variable page size")
> > Signed-off-by: Jiaxun Yang 
> > Signed-off-by: Huacai Chen 
> > ---
> >   target/mips/cp0_helper.c | 36 +---
> >   target/mips/cpu.h|  1 +
> >   2 files changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/mips/cp0_helper.c b/target/mips/cp0_helper.c
> > index de64add038..f3478d826b 100644
> > --- a/target/mips/cp0_helper.c
> > +++ b/target/mips/cp0_helper.c
> > @@ -867,13 +867,35 @@ void helper_mtc0_memorymapid(CPUMIPSState *env, 
> > target_ulong arg1)
> >
> >   void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t 
> > *pagemask)
> >   {
> > -uint64_t mask = arg1 >> (TARGET_PAGE_BITS + 1);
> > -if (!(env->insn_flags & ISA_MIPS32R6) || (arg1 == ~0) ||
> > -(mask == 0x || mask == 0x0003 || mask == 0x000F ||
> > - mask == 0x003F || mask == 0x00FF || mask == 0x03FF ||
> > - mask == 0x0FFF || mask == 0x3FFF || mask == 0x)) {
> > -env->CP0_PageMask = arg1 & (0x1FFF & (TARGET_PAGE_MASK << 1));
> > +unsigned long mask;
> > +int maskbits;
> > +
> > +if (env->insn_flags & ISA_MIPS32R6) {
> > +return;
> > +}
> > +/* Don't care MASKX as we don't support 1KB page */
> > +mask = extract32((uint32_t)arg1, CP0PM_MASK, 16);
> > +maskbits = find_first_zero_bit(, 32);
> > +
> > +/* Ensure no more set bit after first zero */
> > +if (mask >> maskbits) {
> > +goto invalid;
> > +}
> > +/* We don't support VTLB entry smaller than target page */
> > +if ((maskbits + 12) < TARGET_PAGE_BITS) {
> > +goto invalid;
> >   }
> > +env->CP0_PageMask = mask << CP0PM_MASK;
> > +
> > +return;
> > +
> > +invalid:
> > +/*
> > + * When invalid, ensure the value is bigger than or equal to
> > + * the minimal but smaller than or equal to the maxium.
> > + */
> > +maskbits = MIN(16, MAX(maskbits, TARGET_PAGE_BITS - 12));
> > +env->CP0_PageMask = ((1 << (16 + 1)) - 1) << CP0PM_MASK;
> >   }
> >
> >   void helper_mtc0_pagemask(CPUMIPSState *env, target_ulong arg1)
> > @@ -1104,7 +1126,7 @@ void helper_mthc0_saar(CPUMIPSState *env, 
> > target_ulong arg1)
> >   void helper_mtc0_entryhi(CPUMIPSState *env, target_ulong arg1)
> >   {
> >   target_ulong old, val, mask;
> > -mask = (TARGET_PAGE_MASK << 1) | env->CP0_EntryHi_ASID_mask;
> > +mask = ~((1 << 14) - 1) | env->CP0_EntryHi_ASID_mask;
> >   if (((env->CP0_Config4 >> CP0C4_IE) & 0x3) >= 2) {
> >   mask |= 1 << CP0EnHi_EHINV;
> >   }
> > diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> > index 7cf7f5239f..9c8bb23807 100644
> > --- a/target/mips/cpu.h
> > +++ b/target/mips/cpu.h
> > @@ -618,6 +618,7 @@ struct CPUMIPSState {
> >* CP0 Register 5
> >*/
> >   int32_t CP0_PageMask;
> > +#define CP0PM_MASK 13
> >   int32_t CP0_PageGrain_rw_bitmask;
> >   int32_t CP0_PageGrain;
> >   #define CP0PG_RIE 31
> >
>
> Malta test failing:
>
> [0.00] Linux version 4.5.0-2-4kc-malta
> (debian-ker...@lists.debian.org) (gcc version 5.3.1 20160519 (Debian
> 5.3.1-20) ) #1 Debian 4.5.5-1 (2016-05-29)
> [0.00] earlycon: Early serial console at I/O port 0x3f8 (options
> '38400n8')
> [0.00] bootconsole [uart0] enabled
> [0.00] CPU0 revision is: 00019300 (MIPS 24Kc)
> [0.00] FPU revision is: 00739300
> [0.00] MIPS: machine is mti,malta
> [...]
> Freeing unused kernel memory: 412K (80979000 - 809e)
> do_page_fault(): sending SIGSEGV to mount for invalid write access to
> 0018a000
> epc = 77848a54 in libc-2.27.so[7782f000+177000]
> ra  = 779d0618 in ld-2.27.so[779bf000+24000]
> do_page_fault(): sending SIGSEGV to ln for invalid write access to 0018a000
> epc = 778d4a54 in libc-2.27.so[778bb000+177000]
> ra  = 77a5c618 in ld-2.27.so[77a4b000+24000]
> do_page_fault(): sending SIGSEGV to S01logging for invalid write access
> to 0018a000
> epc = 77d08a54 in libc-2.27.so[77cef000+177000]
> ra  = 77e90618 in ld-2.27.so[77e7f000+24000]
> do_page_fault(): sending SIGSEGV to S20urandom for invalid write access
> to 0018a000
> epc = 76ee4a54 in libc-2.27.so[76ecb000+177000]
> ra  = 7706c618 in ld-2.27.so[7705b000+24000]
> do_page_fault(): sending SIGSEGV to ifup for invalid write access to
> 0018a000
> epc = 77974a54 in libc-2.27.so[7795b000+177000]
> ra  = 77afc618 in ld-2.27.so[77aeb000+24000]
> do_page_fault(): sending SIGSEGV to awk for invalid read access from
> 
> epc =  in busybox[40+d8000]
> ra  = 77248110 in libc-2.27.so[770fb000+177000]
> do_page_fault(): sending 

Re: [PATCH v1 0/2] Add timeout mechanism to qmp actions

[Zhenyu Ye]

On 2020/10/19 21:25, Paolo Bonzini wrote:
> On 19/10/20 14:40, Zhenyu Ye wrote:
>> The kernel backtrace for io_submit in GUEST is:
>>
>>  guest# ./offcputime -K -p `pgrep -nx fio`
>>  b'finish_task_switch'
>>  b'__schedule'
>>  b'schedule'
>>  b'io_schedule'
>>  b'blk_mq_get_tag'
>>  b'blk_mq_get_request'
>>  b'blk_mq_make_request'
>>  b'generic_make_request'
>>  b'submit_bio'
>>  b'blkdev_direct_IO'
>>  b'generic_file_read_iter'
>>  b'aio_read'
>>  b'io_submit_one'
>>  b'__x64_sys_io_submit'
>>  b'do_syscall_64'
>>  b'entry_SYSCALL_64_after_hwframe'
>>  -fio (1464)
>>  40031912
>>
>> And Linux io_uring can avoid the latency problem.
> 
> What filesystem are you using?
> 

On host, the VM image and disk images are based on ext4 filesystem.
In guest, the '/' uses xfs filesystem, and the disks are raw devices.

guest# df -hT
Filesystem  Type  Size  Used Avail Use% Mounted on
devtmpfsdevtmpfs   16G 0   16G   0% /dev
tmpfs   tmpfs  16G 0   16G   0% /dev/shm
tmpfs   tmpfs  16G  976K   16G   1% /run
/dev/mapper/fedora-root xfs   8.0G  3.2G  4.9G  40% /
tmpfs   tmpfs  16G 0   16G   0% /tmp
/dev/sda1   xfs  1014M  181M  834M  18% /boot
tmpfs   tmpfs 3.2G 0  3.2G   0% /run/user/0

guest# lsblk
NAMEMAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sda   8:00  10G  0 disk
├─sda18:10   1G  0 part /boot
└─sda28:20   9G  0 part
  ├─fedora-root 253:00   8G  0 lvm  /
  └─fedora-swap 253:10   1G  0 lvm  [SWAP]
vda 252:00  10G  0 disk
vdb 252:16   0  10G  0 disk
vdc 252:32   0  10G  0 disk
vdd 252:48   0  10G  0 disk

Thanks,
Zhenyu

Question on Compression for Raw Image

[Wang, Wei W]

Hi,

Does anyone know the reason why raw-format.c doesn't have compression support 
(but qcow has the supported added)?
For example, raw image backup with compression, "qemu-img convert -c -O raw 
origin.img  dist.img", doesn't work.

Thanks,
Wei

Re: [PATCH v2 7/9] tcg: mirror mapping RWX pages for iOS optional

[Richard Henderson]

On 10/18/20 6:39 PM, Joelle van Dyne wrote:
> From: osy 
> 
> This allows jailbroken devices with entitlements to switch the option off.
> 
> Signed-off-by: Joelle van Dyne 
> ---

I can guess why this performs better: half the page table entries and thus half
the tlb entries required.  Which for any non-trivially sized jit arena is going
to add up. [*]

In line with my comments re patch 6, and making this feature available
everywhere (or at least non-windows), the ifdefs would go away.  I might also
suggest default on for CONFIG_DEBUG_TCG and otherwise default off (when the
host os allows).


r~


* Which makes me wonder how much we should use the "const TranslationBlock *"
version of that structure in the rx mapping, so that we're using a tlb entry
that is more likely to be present, since we've just branched from the code (or
just about to branch to the code) on the same page.

Re: [PATCH v2 6/9] tcg: implement mirror mapped JIT for iOS

[Richard Henderson]

On 10/19/20 3:39 PM, Joelle van Dyne wrote:
>> Explicit cast may not be needed here so this could be a macro if caling it
>> differently helps or why don't you just use tcg_mirror_prr_rw directly
>> everywhere?
> 
> There are quite a bit of code that depends on tcg_insn_unit * type such as
> 
> *tcg_code_ptr_rw(s, code_ptr) = insn;
> 
> and
> 
> (tcg_code_ptr_rw(s, p))[i] = NOP;
> 
> I think it's cleaner to not have to manually cast in every one of 30+
> instances of this. In v1, I used a macro but was told to use an inline
> function instead.

Yep.

>> Is that !defined or are you missing an implementation and #else here?
> No, `flush_dcache_range` is only needed when mirror mapped (after
> writing to the RW mirror). Now there is no iOS compatible compiler for
> any other arch than x86 and ARM. However, in the slim chance that
> Apple decides to change arch again in the future and moves to RISC-V
> or something, then we get a nice compiler error.

*shrug* As opposed to the nice compiler error you get for a missing function
declaration?

That said, I think __builtin___clear_cache() may be the target-independent
runtime function that you need.  Both GCC and LLVM support this, and I'd be
surprised if that doesn't carry through to iOS.

>> Maybe this patch could be split up some more, making the RW offset
>> handling and cache management separate patches even if they don't work
>> separately just to make it easier to review.
> 
> I can probably do that for v3 but imo most of the LOC here is because
> the same change has to be done to every TCG target. No matter how you
> split up the patches, it will look like a lot of changes.

It occurs to me that the majority of the code changes in patches 5 and 6 are
due to your choice that code_gen_buffer points to the RX copy and not the RW 
copy.

Swap the two, and instead have an inline function that produces the executable
pointer from the rw pointer, and suddenly there are very much fewer changes
required.

For the most part, tcg/$cpu/ generates pc-relative code, so it need not
consider the absolute address.  There are a few exceptions including,
obviously, 32-bit x86.  But the number of places that occurs is small.

There's the assignment to tb->tc.ptr of course, and
tcg_ctx.code_gen_prologue/epilogue.

In any case, each of these changes (generic, per tcg backend) can occur before
you finally add a non-zero displacement that actually separates the RX and RW
mappings.

Finally, I'd like to have this implemented on Linux as well, or I'm afraid the
feature will bit-rot.  This can be trivially done by either (1)
MREMAP_DONTUNMAP or (2) mapping from posix shared memory instead of MAP_ANON so
that you can map the same memory twice.  Thus virtually all of the ifdefs
should go away.


r~

Re: [PATCH 7/8] tests/9pfs: add local Tlink test

[Christian Schoenebeck]

On Dienstag, 20. Oktober 2020 01:13:24 CEST Christian Schoenebeck wrote:
> This test case uses a Tlink request to create a hard link to a regular
> file using the 9pfs 'local' fs driver.
> 
> Signed-off-by: Christian Schoenebeck 
> ---
>  tests/qtest/virtio-9p-test.c | 61 
>  1 file changed, 61 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index f7d18f6274..447d8e3344 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
>  id == P9_RMKDIR ? "RMKDIR" :
>  id == P9_RLCREATE ? "RLCREATE" :
>  id == P9_RSYMLINK ? "RSYMLINK" :
> +id == P9_RLINK ? "RLINK" :
>  id == P9_RUNLINKAT ? "RUNLINKAT" :
>  id == P9_RFLUSH ? "RFLUSH" :
>  id == P9_RREADDIR ? "READDIR" :
> @@ -742,6 +743,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
>  v9fs_req_free(req);
>  }
> 
> +/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
> +static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
> + const char *name, uint16_t tag)
> +{

This hard-link test was actually motived by an issue that I recently 
encountered on a machine: it fails to create any hard links with 9p. This 
particular test case succeeds though.

I think the problem is that recent libvirt versions enable qemu's sandbox 
feature by default which filters syscalls. Fact is, any linkat() call fails on 
that machine with EACCES now. I couldn't reproduce it on my development 
machine yet though. I guess it's a difference in white/black-list seccomp 
config or something. Not sure yet if there is some change required on 9p side 
or whether it's really just a seccomp config issue.

P.S. Noisy days from my side, but this is probably the last batch of patches 
from my side in a while, unless I really need to fix something for that hard 
link isssue. We'll see ...

Best regards,
Christian Schoenebeck

Re: [PATCH v2 6/9] tcg: implement mirror mapped JIT for iOS

[BALATON Zoltan via]

On Mon, 19 Oct 2020, Joelle van Dyne wrote:

Explicit cast may not be needed here so this could be a macro if caling it
differently helps or why don't you just use tcg_mirror_prr_rw directly
everywhere?


There are quite a bit of code that depends on tcg_insn_unit * type such as

*tcg_code_ptr_rw(s, code_ptr) = insn;

and

(tcg_code_ptr_rw(s, p))[i] = NOP;


OK that explains it, haven't looked at it at that detail.


I think it's cleaner to not have to manually cast in every one of 30+
instances of this. In v1, I used a macro but was told to use an inline
function instead.


Definitely cleaner to have the cast either in a macro or inline func than 
manually having it everywhere. The static inline in v2 looks better than 
the macro in v1 so unless others disagree it's fine this way, I'm not the 
one who decides, I was just asking if we can avoid having two static 
inlines relying on casting void * but if you also dereference as above 
then returning the right type is needed. Let's see what Richard says who 
suggested the function instead of a macro but it does look more readable 
than the previous macro.



Is that !defined or are you missing an implementation and #else here?

No, `flush_dcache_range` is only needed when mirror mapped (after
writing to the RW mirror). Now there is no iOS compatible compiler for
any other arch than x86 and ARM. However, in the slim chance that


But this was in tcg/arm/tcg-target.h which is ARM but maybe you mean only 
x86 and 64bit ARM which is aarch64 but not 32bit ARM. I've noticed this 
only after sending the question.



Apple decides to change arch again in the future and moves to RISC-V
or something, then we get a nice compiler error.


Maybe this patch could be split up some more, making the RW offset
handling and cache management separate patches even if they don't work
separately just to make it easier to review.


I can probably do that for v3 but imo most of the LOC here is because
the same change has to be done to every TCG target. No matter how you
split up the patches, it will look like a lot of changes.


Sure but it's easier to review if a single patch has only similar changes 
even if it touches a lot of files than if it does independent stuff 
intermixed unless it's a small patch (but even then QEMU tends to prefer a 
lot of smaller patches instead of combining changes in a single patch). 
That's also good for bisectability so that's also something to consider 
when splitting patches. Not sure if in this case this can be split up into 
two working changes because RW mapping may not work without cache flushes 
and cache flushes may not be added before having the RW split but having 
two patches for the review that can be squashed in the final series may 
still help. But lets see if this gets reviewed without further splitting 
or what others say.


Not sure you're aware of this: https://wiki.qemu.org/Planning/5.2 but if 
this series does not get merged this week don't be surprised if your next 
opportunity to pick it up will be in December (when most people who can 
review it will be on holyday so it can be easily take longer). So maybe 
you could try pushing it and do everything to make reviewers' job easier 
if you want it in the next release. Otherwise you'll have time to polish 
it until next year.


Also it may be good to fix checkpatch errors (warnings may be OK but 
errors are not) that are reported even if it's not in your code (it could 
be a separate clean up patch before your changes or for small things in 
the same patch) otherwise automated tests may not run which can also delay 
reviews and merging:


https://patchew.org/QEMU/20201019051953.90107-...@getutm.app/

and I assume you already know this:

https://wiki.qemu.org/Contribute/SubmitAPatch

It might be overwhelming and off putting sometimes to try getting series 
into QEMU but please don't give up.


Regards,
BALATON Zoltan

[PATCH 8/8] tests/9pfs: add local unlinkat hard link test

[Christian Schoenebeck]

This test case uses a Tunlinkat request to remove a previously hard
linked file by using the 9pfs 'local' fs driver.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 447d8e3344..2e50445745 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1378,6 +1378,39 @@ static void fs_hardlink_file(void *obj, void *data, 
QGuestAllocator *t_alloc)
 g_free(real_file);
 }
 
+static void fs_unlinkat_hardlink(void *obj, void *data,
+ QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+P9Req *req;
+uint32_t dfid, fid;
+struct stat st_real, st_link;
+char *real_file = virtio_9p_test_path("08/real_file");
+char *hardlink_file = virtio_9p_test_path("08/hardlink_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "08");
+fid = fs_lcreate(v9p, data, t_alloc, "08", "real_file");
+g_assert(stat(real_file, _real) == 0);
+g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
+
+dfid = fs_walk_fid(v9p, data, t_alloc, "08");
+
+req = v9fs_tlink(v9p, dfid, fid, "hardlink_file", 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlink(req);
+g_assert(stat(hardlink_file, _link) == 0);
+
+fs_unlinkat(v9p, data, t_alloc, "08", "hardlink_file", 0);
+/* symlink should be gone now */
+g_assert(stat(hardlink_file, _link) != 0);
+/* and old file should still exist */
+g_assert(stat(real_file, _real) == 0);
+
+g_free(hardlink_file);
+g_free(real_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
 virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1424,6 +1457,7 @@ static void register_virtio_9p_test(void)
 qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
 qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink, 
);
 qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, );
+qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink, 
);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

[PATCH 7/8] tests/9pfs: add local Tlink test

[Christian Schoenebeck]

This test case uses a Tlink request to create a hard link to a regular
file using the 9pfs 'local' fs driver.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 61 
 1 file changed, 61 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index f7d18f6274..447d8e3344 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
 id == P9_RMKDIR ? "RMKDIR" :
 id == P9_RLCREATE ? "RLCREATE" :
 id == P9_RSYMLINK ? "RSYMLINK" :
+id == P9_RLINK ? "RLINK" :
 id == P9_RUNLINKAT ? "RUNLINKAT" :
 id == P9_RFLUSH ? "RFLUSH" :
 id == P9_RREADDIR ? "READDIR" :
@@ -742,6 +743,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
 v9fs_req_free(req);
 }
 
+/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
+static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
+ const char *name, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TLINK, tag);
+v9fs_uint32_write(req, dfid);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rlink tag[2] */
+static void v9fs_rlink(P9Req *req)
+{
+v9fs_req_recv(req, P9_RLINK);
+v9fs_req_free(req);
+}
+
 /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
 static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
  uint32_t flags, uint16_t tag)
@@ -1318,6 +1346,38 @@ static void fs_unlinkat_symlink(void *obj, void *data,
 g_free(real_file);
 }
 
+static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+P9Req *req;
+uint32_t dfid, fid;
+struct stat st_real, st_link;
+char *real_file = virtio_9p_test_path("07/real_file");
+char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "07");
+fid = fs_lcreate(v9p, data, t_alloc, "07", "real_file");
+g_assert(stat(real_file, _real) == 0);
+g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
+
+dfid = fs_walk_fid(v9p, data, t_alloc, "07");
+
+req = v9fs_tlink(v9p, dfid, fid, "hardlink_file", 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlink(req);
+
+/* check if link exists now ... */
+g_assert(stat(hardlink_file, _link) == 0);
+/* ... and it's a hard link, right? */
+g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
+g_assert(st_link.st_dev == st_real.st_dev);
+g_assert(st_link.st_ino == st_real.st_ino);
+
+g_free(hardlink_file);
+g_free(real_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
 virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1363,6 +1423,7 @@ static void register_virtio_9p_test(void)
 qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
 qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
 qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink, 
);
+qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, );
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

[PATCH 4/8] tests/9pfs: add local unlinkat file test

[Christian Schoenebeck]

This test case uses a Tunlinkat request to remove a regular file using
the 9pfs 'local' fs driver.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 1b133f52bd..06a9f10d34 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1194,6 +1194,28 @@ static void fs_create_file(void *obj, void *data, 
QGuestAllocator *t_alloc)
 g_free(new_file);
 }
 
+static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+struct stat st;
+char *new_file = virtio_9p_test_path("04/doa_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "04");
+fs_lcreate(v9p, data, t_alloc, "04", "doa_file");
+
+/* check if created file exists now ... */
+g_assert(stat(new_file, ) == 0);
+/* ... and is a regular file */
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+fs_unlinkat(v9p, data, t_alloc, "04", "doa_file", 0);
+/* file should be gone now */
+g_assert(stat(new_file, ) != 0);
+
+g_free(new_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
 virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1236,6 +1258,7 @@ static void register_virtio_9p_test(void)
 qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
 qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
 qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
+qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

[PATCH 6/8] tests/9pfs: add local unlinkat symlink test

[Christian Schoenebeck]

This test case uses a Tunlinkat request to remove a symlink using
the 9pfs 'local' fs driver.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 78f4ed7e5f..f7d18f6274 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1293,6 +1293,31 @@ static void fs_symlink_file(void *obj, void *data, 
QGuestAllocator *t_alloc)
 g_free(real_file);
 }
 
+static void fs_unlinkat_symlink(void *obj, void *data,
+QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+struct stat st;
+char *real_file = virtio_9p_test_path("06/real_file");
+char *symlink_file = virtio_9p_test_path("06/symlink_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "06");
+fs_lcreate(v9p, data, t_alloc, "06", "real_file");
+g_assert(stat(real_file, ) == 0);
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+fs_symlink(v9p, data, t_alloc, "06", "symlink_file", "real_file");
+g_assert(stat(symlink_file, ) == 0);
+
+fs_unlinkat(v9p, data, t_alloc, "06", "symlink_file", 0);
+/* symlink should be gone now */
+g_assert(stat(symlink_file, ) != 0);
+
+g_free(symlink_file);
+g_free(real_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
 virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1337,6 +1362,7 @@ static void register_virtio_9p_test(void)
 qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
 qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
 qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
+qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink, 
);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

Re: [PATCH v2 5/9] tcg: add const hints for code pointers

[Richard Henderson]

On 10/19/20 4:36 PM, Joelle van Dyne wrote:
> Seems like I missed a few. Sorry about that. Will fix.

I'll keep looking, Just In Case.  ;-)

>>  s->code_gen_epilogue = tb_ret_addr = s->code_ptr;

In this case, just splitting the chained assignment to two statements is
sufficient to fix.


r~

[PATCH 3/8] tests/9pfs: add local Tlcreate test

[Christian Schoenebeck]

This test case uses a Tlcreate 9p request to create a regular file inside
host's test directory.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 78 
 1 file changed, 78 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 990d074d33..1b133f52bd 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
 id == P9_RLOPEN ? "RLOPEN" :
 id == P9_RWRITE ? "RWRITE" :
 id == P9_RMKDIR ? "RMKDIR" :
+id == P9_RLCREATE ? "RLCREATE" :
 id == P9_RUNLINKAT ? "RUNLINKAT" :
 id == P9_RFLUSH ? "RFLUSH" :
 id == P9_RREADDIR ? "READDIR" :
@@ -669,6 +670,44 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
 v9fs_req_free(req);
 }
 
+/* size[4] Tlcreate tag[2] fid[4] name[s] flags[4] mode[4] gid[4] */
+static P9Req *v9fs_tlcreate(QVirtio9P *v9p, uint32_t fid, const char *name,
+uint32_t flags, uint32_t mode, uint32_t gid,
+uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4 + 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TLCREATE, tag);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, flags);
+v9fs_uint32_write(req, mode);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rlcreate tag[2] qid[13] iounit[4] */
+static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
+{
+v9fs_req_recv(req, P9_RLCREATE);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+if (iounit) {
+v9fs_uint32_read(req, iounit);
+}
+v9fs_req_free(req);
+}
+
 /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
 static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
  uint32_t flags, uint16_t tag)
@@ -1032,6 +1071,26 @@ static void fs_mkdir(void *obj, void *data, 
QGuestAllocator *t_alloc,
 g_free(name);
 }
 
+/* create a regular file with Tlcreate and return file's fid */
+static uint32_t fs_lcreate(void *obj, void *data, QGuestAllocator *t_alloc,
+   const char *path, const char *cname)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char *const name = g_strdup(cname);
+uint32_t fid;
+P9Req *req;
+
+fid = fs_walk_fid(v9p, data, t_alloc, path);
+
+req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlcreate(req, NULL, NULL);
+
+g_free(name);
+return fid;
+}
+
 static void fs_unlinkat(void *obj, void *data, QGuestAllocator *t_alloc,
 const char *atpath, const char *rpath, uint32_t flags)
 {
@@ -1117,6 +1176,24 @@ static void fs_unlinkat_dir(void *obj, void *data, 
QGuestAllocator *t_alloc)
 g_free(root_path);
 }
 
+static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+struct stat st;
+char *new_file = virtio_9p_test_path("03/1st_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "03");
+fs_lcreate(v9p, data, t_alloc, "03", "1st_file");
+
+/* check if created file exists now ... */
+g_assert(stat(new_file, ) == 0);
+/* ... and is a regular file */
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+g_free(new_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
 virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1158,6 +1235,7 @@ static void register_virtio_9p_test(void)
 qos_add_test("local/config", "virtio-9p", pci_config,  );
 qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
 qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
+qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

[PATCH 0/8] 9pfs: more local tests

[Christian Schoenebeck]

Just a bunch of more test case using the 9pfs 'local' fs driver backend,
namely for these 9p requests:

* Tunlinkat, Tlcreate, Tsymlink and Tlink.

Christian Schoenebeck (8):
  tests/9pfs: simplify fs_mkdir()
  tests/9pfs: add local unlinkat directory test
  tests/9pfs: add local Tlcreate test
  tests/9pfs: add local unlinkat file test
  tests/9pfs: add local Tsymlink test
  tests/9pfs: add local unlinkat symlink test
  tests/9pfs: add local Tlink test
  tests/9pfs: add local unlinkat hard link test

 tests/qtest/virtio-9p-test.c | 395 ++-
 1 file changed, 390 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH 5/8] tests/9pfs: add local Tsymlink test

[Christian Schoenebeck]

This test case uses a Tsymlink 9p request to create a symbolic link using
the 9pfs 'local' fs driver.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 78 
 1 file changed, 78 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 06a9f10d34..78f4ed7e5f 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -259,6 +259,7 @@ static const char *rmessage_name(uint8_t id)
 id == P9_RWRITE ? "RWRITE" :
 id == P9_RMKDIR ? "RMKDIR" :
 id == P9_RLCREATE ? "RLCREATE" :
+id == P9_RSYMLINK ? "RSYMLINK" :
 id == P9_RUNLINKAT ? "RUNLINKAT" :
 id == P9_RFLUSH ? "RFLUSH" :
 id == P9_RREADDIR ? "READDIR" :
@@ -708,6 +709,39 @@ static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, 
uint32_t *iounit)
 v9fs_req_free(req);
 }
 
+/* size[4] Tsymlink tag[2] fid[4] name[s] symtgt[s] gid[4] */
+static P9Req *v9fs_tsymlink(QVirtio9P *v9p, uint32_t fid, const char *name,
+const char *symtgt, uint32_t gid, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name) + v9fs_string_size(symtgt);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TSYMLINK, tag);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_string_write(req, symtgt);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rsymlink tag[2] qid[13] */
+static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
+{
+v9fs_req_recv(req, P9_RSYMLINK);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+v9fs_req_free(req);
+}
+
 /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
 static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
  uint32_t flags, uint16_t tag)
@@ -1091,6 +1125,27 @@ static uint32_t fs_lcreate(void *obj, void *data, 
QGuestAllocator *t_alloc,
 return fid;
 }
 
+/* create symlink named @a clink in directory @a path pointing to @a to */
+static void fs_symlink(void *obj, void *data, QGuestAllocator *t_alloc,
+   const char *path, const char *clink, const char *to)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char *const name = g_strdup(clink);
+char *const dst = g_strdup(to);
+uint32_t fid;
+P9Req *req;
+
+fid = fs_walk_fid(v9p, data, t_alloc, path);
+
+req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rsymlink(req, NULL);
+
+g_free(dst);
+g_free(name);
+}
+
 static void fs_unlinkat(void *obj, void *data, QGuestAllocator *t_alloc,
 const char *atpath, const char *rpath, uint32_t flags)
 {
@@ -1216,6 +1271,28 @@ static void fs_unlinkat_file(void *obj, void *data, 
QGuestAllocator *t_alloc)
 g_free(new_file);
 }
 
+static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+struct stat st;
+char *real_file = virtio_9p_test_path("05/real_file");
+char *symlink_file = virtio_9p_test_path("05/symlink_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "05");
+fs_lcreate(v9p, data, t_alloc, "05", "real_file");
+g_assert(stat(real_file, ) == 0);
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+fs_symlink(v9p, data, t_alloc, "05", "symlink_file", "real_file");
+
+/* check if created link exists now */
+g_assert(stat(symlink_file, ) == 0);
+
+g_free(symlink_file);
+g_free(real_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
 virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1259,6 +1336,7 @@ static void register_virtio_9p_test(void)
 qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
 qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
 qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
+qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

[PATCH 1/8] tests/9pfs: simplify fs_mkdir()

[Christian Schoenebeck]

Split out walking a directory path to a separate new utility function
fs_walk_fid() and use that function in fs_mkdir().

The code difference saved this way is not much, but we'll use that new
fs_walk_fid() function in the upcoming patches, so it will avoid quite
some code duplication after all.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 23 ++-
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index c15908f27b..dc724bbb1e 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -967,13 +967,12 @@ static void fs_flush_ignored(void *obj, void *data, 
QGuestAllocator *t_alloc)
 g_free(wnames[0]);
 }
 
-static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
- const char *path, const char *cname)
+/* utility function: walk to requested dir and return fid for that dir */
+static uint32_t fs_walk_fid(void *obj, void *data, QGuestAllocator *t_alloc,
+const char *path)
 {
 QVirtio9P *v9p = obj;
-alloc = t_alloc;
 char **wnames;
-char *const name = g_strdup(cname);
 P9Req *req;
 const uint32_t fid = genfid();
 
@@ -983,12 +982,26 @@ static void fs_mkdir(void *obj, void *data, 
QGuestAllocator *t_alloc,
 v9fs_req_wait_for_reply(req, NULL);
 v9fs_rwalk(req, NULL, NULL);
 
+split_free();
+return fid;
+}
+
+static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
+ const char *path, const char *cname)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char *const name = g_strdup(cname);
+uint32_t fid;
+P9Req *req;
+
+fid = fs_walk_fid(v9p, data, t_alloc, path);
+
 req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
 v9fs_req_wait_for_reply(req, NULL);
 v9fs_rmkdir(req, NULL);
 
 g_free(name);
-split_free();
 }
 
 static void fs_readdir_split_128(void *obj, void *data,
-- 
2.20.1

[PATCH 2/8] tests/9pfs: add local unlinkat directory test

[Christian Schoenebeck]

This test case uses a Tunlinkat 9p request with flag AT_REMOVEDIR
(see 'man 2 unlink') to remove a directory from host's test directory.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 72 
 1 file changed, 72 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index dc724bbb1e..990d074d33 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
 id == P9_RLOPEN ? "RLOPEN" :
 id == P9_RWRITE ? "RWRITE" :
 id == P9_RMKDIR ? "RMKDIR" :
+id == P9_RUNLINKAT ? "RUNLINKAT" :
 id == P9_RFLUSH ? "RFLUSH" :
 id == P9_RREADDIR ? "READDIR" :
 "";
@@ -668,6 +669,33 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
 v9fs_req_free(req);
 }
 
+/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
+static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
+ uint32_t flags, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TUNLINKAT, tag);
+v9fs_uint32_write(req, dirfd);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, flags);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Runlinkat tag[2] */
+static void v9fs_runlinkat(P9Req *req)
+{
+v9fs_req_recv(req, P9_RUNLINKAT);
+v9fs_req_free(req);
+}
+
 /* basic readdir test where reply fits into a single response message */
 static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
 {
@@ -1004,6 +1032,24 @@ static void fs_mkdir(void *obj, void *data, 
QGuestAllocator *t_alloc,
 g_free(name);
 }
 
+static void fs_unlinkat(void *obj, void *data, QGuestAllocator *t_alloc,
+const char *atpath, const char *rpath, uint32_t flags)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char *const name = g_strdup(rpath);
+uint32_t fid;
+P9Req *req;
+
+fid = fs_walk_fid(v9p, data, t_alloc, atpath);
+
+req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_runlinkat(req);
+
+g_free(name);
+}
+
 static void fs_readdir_split_128(void *obj, void *data,
  QGuestAllocator *t_alloc)
 {
@@ -1046,6 +1092,31 @@ static void fs_create_dir(void *obj, void *data, 
QGuestAllocator *t_alloc)
 g_free(root_path);
 }
 
+static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+struct stat st;
+char *root_path = virtio_9p_test_path("");
+char *new_dir = virtio_9p_test_path("02");
+
+g_assert(root_path != NULL);
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "02");
+
+/* check if created directory really exists now ... */
+g_assert(stat(new_dir, ) == 0);
+/* ... and is actually a directory */
+g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+
+fs_unlinkat(v9p, data, t_alloc, "/", "02", AT_REMOVEDIR);
+/* directory should be gone now */
+g_assert(stat(new_dir, ) != 0);
+
+g_free(new_dir);
+g_free(root_path);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
 virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1086,6 +1157,7 @@ static void register_virtio_9p_test(void)
 opts.before = assign_9p_local_driver;
 qos_add_test("local/config", "virtio-9p", pci_config,  );
 qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
+qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

Re: [PATCH v2 5/9] tcg: add const hints for code pointers

[Joelle van Dyne]

Seems like I missed a few. Sorry about that. Will fix.

-j

On Mon, Oct 19, 2020 at 4:27 PM Richard Henderson
 wrote:
>
> On 10/18/20 6:39 PM, Joelle van Dyne wrote:
> > From: osy 
> >
> > We will introduce mirror mapping for JIT segment with separate RX and RW
> > access. Adding 'const' hints will make it easier to identify read-only
> > accesses and allow us to easier catch bugs at compile time in the future.
> >
> > Signed-off-by: Joelle van Dyne 
> > ---
> >  include/tcg/tcg.h|  8 
> >  tcg/tcg.c|  4 ++--
> >  tcg/aarch64/tcg-target.c.inc | 19 +++
> >  tcg/arm/tcg-target.c.inc | 12 +++-
> >  tcg/i386/tcg-target.c.inc| 10 +-
> >  tcg/mips/tcg-target.c.inc| 33 +++--
> >  tcg/ppc/tcg-target.c.inc | 21 +
> >  tcg/riscv/tcg-target.c.inc   | 11 ++-
> >  tcg/s390/tcg-target.c.inc|  9 +
> >  tcg/sparc/tcg-target.c.inc   | 10 +-
> >  tcg/tcg-ldst.c.inc   |  2 +-
> >  tcg/tci/tcg-target.c.inc |  2 +-
> >  12 files changed, 79 insertions(+), 62 deletions(-)
>
> tcg/ppc/tcg-target.c.inc:2349:26: error: assignment discards ‘const’ qualifier
> from pointer target type [-Werror]
>  s->code_gen_epilogue = tb_ret_addr = s->code_ptr;
>
> How many of the targets did you build?
>
>
> r~

Re: [PATCH v2 5/9] tcg: add const hints for code pointers

[Richard Henderson]

On 10/18/20 6:39 PM, Joelle van Dyne wrote:
> From: osy 
> 
> We will introduce mirror mapping for JIT segment with separate RX and RW
> access. Adding 'const' hints will make it easier to identify read-only
> accesses and allow us to easier catch bugs at compile time in the future.
> 
> Signed-off-by: Joelle van Dyne 
> ---
>  include/tcg/tcg.h|  8 
>  tcg/tcg.c|  4 ++--
>  tcg/aarch64/tcg-target.c.inc | 19 +++
>  tcg/arm/tcg-target.c.inc | 12 +++-
>  tcg/i386/tcg-target.c.inc| 10 +-
>  tcg/mips/tcg-target.c.inc| 33 +++--
>  tcg/ppc/tcg-target.c.inc | 21 +
>  tcg/riscv/tcg-target.c.inc   | 11 ++-
>  tcg/s390/tcg-target.c.inc|  9 +
>  tcg/sparc/tcg-target.c.inc   | 10 +-
>  tcg/tcg-ldst.c.inc   |  2 +-
>  tcg/tci/tcg-target.c.inc |  2 +-
>  12 files changed, 79 insertions(+), 62 deletions(-)

tcg/ppc/tcg-target.c.inc:2349:26: error: assignment discards ‘const’ qualifier
from pointer target type [-Werror]
 s->code_gen_epilogue = tb_ret_addr = s->code_ptr;

How many of the targets did you build?


r~

Re: [PATCH v2 5/9] tcg: add const hints for code pointers

[Joelle van Dyne]

You can --author "Joelle van Dyne "

-j

On Mon, Oct 19, 2020 at 4:19 PM Richard Henderson
 wrote:
>
> On 10/18/20 6:39 PM, Joelle van Dyne wrote:
> > From: osy 
> >
> > We will introduce mirror mapping for JIT segment with separate RX and RW
> > access. Adding 'const' hints will make it easier to identify read-only
> > accesses and allow us to easier catch bugs at compile time in the future.
> >
> > Signed-off-by: Joelle van Dyne 
>
> Are you "osy"?  We do need S-o-b with real names from all contributors to the
> patch.
>
> The patch is bigger than I would like, but it all appears to be strongly
> connected, and I don't see where it could be split.  I'm inclined to
> cherry-pick this patch out now to avoid carrying it around.  So if I can 
> either
> fix the --author to you, or we can get a s-o-b from the author, I'll do that.
>
>
> r~

Re: [PATCH v2 5/9] tcg: add const hints for code pointers

[Richard Henderson]

On 10/18/20 6:39 PM, Joelle van Dyne wrote:
> From: osy 
> 
> We will introduce mirror mapping for JIT segment with separate RX and RW
> access. Adding 'const' hints will make it easier to identify read-only
> accesses and allow us to easier catch bugs at compile time in the future.
> 
> Signed-off-by: Joelle van Dyne 

Are you "osy"?  We do need S-o-b with real names from all contributors to the
patch.

The patch is bigger than I would like, but it all appears to be strongly
connected, and I don't see where it could be split.  I'm inclined to
cherry-pick this patch out now to avoid carrying it around.  So if I can either
fix the --author to you, or we can get a s-o-b from the author, I'll do that.


r~

Re: [PATCH v2 3/4] hw/riscv: Add a riscv_is_32_bit() function

[Palmer Dabbelt]

On Tue, 13 Oct 2020 17:17:30 PDT (-0700), Alistair Francis wrote:

Signed-off-by: Alistair Francis 
---
 include/hw/riscv/boot.h | 2 ++
 hw/riscv/boot.c | 9 +
 2 files changed, 11 insertions(+)

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 0acbd8aa6e..2975ed1a31 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -23,6 +23,8 @@
 #include "exec/cpu-defs.h"
 #include "hw/loader.h"

+bool riscv_is_32_bit(MachineState *machine);
+
 target_ulong riscv_find_and_load_firmware(MachineState *machine,
   const char *default_machine_firmware,
   hwaddr firmware_load_addr,
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index fa699308a0..5dea644f47 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -40,6 +40,15 @@
 #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
 #endif

+bool riscv_is_32_bit(MachineState *machine)
+{
+if (!strncmp(machine->cpu_type, "rv32", 4)) {
+return true;
+} else {
+return false;
+}
+}
+
 target_ulong riscv_find_and_load_firmware(MachineState *machine,
   const char *default_machine_firmware,
   hwaddr firmware_load_addr,


Reviewed-by: Palmer Dabbelt 

Re: [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU

[Palmer Dabbelt]

On Tue, 13 Oct 2020 17:17:25 PDT (-0700), Alistair Francis wrote:

Allow the user to specify the main application CPU for the sifive_u
machine.

Signed-off-by: Alistair Francis 
Reviewed-by: Bin Meng 
---
 include/hw/riscv/sifive_u.h |  1 +
 hw/riscv/sifive_u.c | 18 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 22e7e6efa1..a9f7b4a084 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -48,6 +48,7 @@ typedef struct SiFiveUSoCState {
 CadenceGEMState gem;

 uint32_t serial;
+char *cpu_type;
 } SiFiveUSoCState;

 #define TYPE_RISCV_U_MACHINE MACHINE_TYPE_NAME("sifive_u")
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 6ad975d692..5f3ad9bc0f 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -424,6 +424,8 @@ static void sifive_u_machine_init(MachineState *machine)
 object_initialize_child(OBJECT(machine), "soc", >soc, TYPE_RISCV_U_SOC);
 object_property_set_uint(OBJECT(>soc), "serial", s->serial,
  _abort);
+object_property_set_str(OBJECT(>soc), "cpu-type", machine->cpu_type,
+ _abort);
 qdev_realize(DEVICE(>soc), NULL, _abort);

 /* register RAM */
@@ -590,6 +592,11 @@ static void sifive_u_machine_class_init(ObjectClass *oc, 
void *data)
 mc->init = sifive_u_machine_init;
 mc->max_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + SIFIVE_U_COMPUTE_CPU_COUNT;
 mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
+#if defined(TARGET_RISCV32)
+mc->default_cpu_type = TYPE_RISCV_CPU_SIFIVE_U34;
+#elif defined(TARGET_RISCV64)
+mc->default_cpu_type = TYPE_RISCV_CPU_SIFIVE_U54;
+#endif
 mc->default_cpus = mc->min_cpus;

 object_class_property_add_bool(oc, "start-in-flash",
@@ -618,7 +625,6 @@ type_init(sifive_u_machine_init_register_types)

 static void sifive_u_soc_instance_init(Object *obj)
 {
-MachineState *ms = MACHINE(qdev_get_machine());
 SiFiveUSoCState *s = RISCV_U_SOC(obj);

 object_initialize_child(obj, "e-cluster", >e_cluster, TYPE_CPU_CLUSTER);
@@ -636,10 +642,6 @@ static void sifive_u_soc_instance_init(Object *obj)

 object_initialize_child(OBJECT(>u_cluster), "u-cpus", >u_cpus,
 TYPE_RISCV_HART_ARRAY);
-qdev_prop_set_uint32(DEVICE(>u_cpus), "num-harts", ms->smp.cpus - 1);
-qdev_prop_set_uint32(DEVICE(>u_cpus), "hartid-base", 1);
-qdev_prop_set_string(DEVICE(>u_cpus), "cpu-type", SIFIVE_U_CPU);
-qdev_prop_set_uint64(DEVICE(>u_cpus), "resetvec", 0x1004);

 object_initialize_child(obj, "prci", >prci, TYPE_SIFIVE_U_PRCI);
 object_initialize_child(obj, "otp", >otp, TYPE_SIFIVE_U_OTP);
@@ -661,6 +663,11 @@ static void sifive_u_soc_realize(DeviceState *dev, Error 
**errp)
 int i;
 NICInfo *nd = _table[0];

+qdev_prop_set_uint32(DEVICE(>u_cpus), "num-harts", ms->smp.cpus - 1);
+qdev_prop_set_uint32(DEVICE(>u_cpus), "hartid-base", 1);
+qdev_prop_set_string(DEVICE(>u_cpus), "cpu-type", s->cpu_type);
+qdev_prop_set_uint64(DEVICE(>u_cpus), "resetvec", 0x1004);
+
 sysbus_realize(SYS_BUS_DEVICE(>e_cpus), _abort);
 sysbus_realize(SYS_BUS_DEVICE(>u_cpus), _abort);
 /*
@@ -792,6 +799,7 @@ static void sifive_u_soc_realize(DeviceState *dev, Error 
**errp)

 static Property sifive_u_soc_props[] = {
 DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
+DEFINE_PROP_STRING("cpu-type", SiFiveUSoCState, cpu_type),
 DEFINE_PROP_END_OF_LIST()
 };


Reviewed-by: Palmer Dabbelt 

Re: [PATCH v2 2/4] hw/riscv: Return the end address of the loaded firmware

[Palmer Dabbelt]

On Tue, 13 Oct 2020 17:17:28 PDT (-0700), Alistair Francis wrote:

Instead of returning the unused entry address from riscv_load_firmware()
instead return the end address. Also return the end address from
riscv_find_and_load_firmware().

This tells the caller if a firmware was loaded and how big it is. This
can be used to determine the load address of the next image (usually the
kernel).

Signed-off-by: Alistair Francis 
---
 include/hw/riscv/boot.h |  8 
 hw/riscv/boot.c | 28 +---
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 451338780a..0acbd8aa6e 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -23,10 +23,10 @@
 #include "exec/cpu-defs.h"
 #include "hw/loader.h"

-void riscv_find_and_load_firmware(MachineState *machine,
-  const char *default_machine_firmware,
-  hwaddr firmware_load_addr,
-  symbol_fn_t sym_cb);
+target_ulong riscv_find_and_load_firmware(MachineState *machine,
+  const char *default_machine_firmware,
+  hwaddr firmware_load_addr,
+  symbol_fn_t sym_cb);
 char *riscv_find_firmware(const char *firmware_filename);
 target_ulong riscv_load_firmware(const char *firmware_filename,
  hwaddr firmware_load_addr,
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 21adaae56e..fa699308a0 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -40,12 +40,13 @@
 #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
 #endif

-void riscv_find_and_load_firmware(MachineState *machine,
-  const char *default_machine_firmware,
-  hwaddr firmware_load_addr,
-  symbol_fn_t sym_cb)
+target_ulong riscv_find_and_load_firmware(MachineState *machine,
+  const char *default_machine_firmware,
+  hwaddr firmware_load_addr,
+  symbol_fn_t sym_cb)
 {
 char *firmware_filename = NULL;
+target_ulong firmware_end_addr = firmware_load_addr;

 if ((!machine->firmware) || (!strcmp(machine->firmware, "default"))) {
 /*
@@ -60,9 +61,12 @@ void riscv_find_and_load_firmware(MachineState *machine,

 if (firmware_filename) {
 /* If not "none" load the firmware */
-riscv_load_firmware(firmware_filename, firmware_load_addr, sym_cb);
+firmware_end_addr = riscv_load_firmware(firmware_filename,
+firmware_load_addr, sym_cb);
 g_free(firmware_filename);
 }
+
+return firmware_end_addr;
 }

 char *riscv_find_firmware(const char *firmware_filename)
@@ -91,17 +95,19 @@ target_ulong riscv_load_firmware(const char 
*firmware_filename,
  hwaddr firmware_load_addr,
  symbol_fn_t sym_cb)
 {
-uint64_t firmware_entry;
+uint64_t firmware_entry, firmware_size, firmware_end;

 if (load_elf_ram_sym(firmware_filename, NULL, NULL, NULL,
- _entry, NULL, NULL, NULL,
+ _entry, NULL, _end, NULL,
  0, EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
-return firmware_entry;
+return firmware_end;
 }

-if (load_image_targphys_as(firmware_filename, firmware_load_addr,
-   ram_size, NULL) > 0) {
-return firmware_load_addr;
+firmware_size = load_image_targphys_as(firmware_filename,
+   firmware_load_addr, ram_size, NULL);
+
+if (firmware_size > 0) {
+return firmware_load_addr + firmware_size;
 }

 error_report("could not load firmware '%s'", firmware_filename);


Reviewed-by: Palmer Dabbelt 

Re: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware

[Palmer Dabbelt]

On Tue, 13 Oct 2020 17:17:33 PDT (-0700), Alistair Francis wrote:

Instead of loading the kernel at a hardcoded start address, let's load
the kernel at the next alligned address after the end of the firmware.

This should have no impact for current users of OpenSBI, but will
allow loading a noMMU kernel at the start of memory.

Signed-off-by: Alistair Francis 
---
 include/hw/riscv/boot.h |  3 +++
 hw/riscv/boot.c | 19 ++-
 hw/riscv/opentitan.c|  3 ++-
 hw/riscv/sifive_e.c |  3 ++-
 hw/riscv/sifive_u.c | 10 --
 hw/riscv/spike.c| 11 ---
 hw/riscv/virt.c | 11 ---
 7 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 2975ed1a31..0b01988727 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -25,6 +25,8 @@

 bool riscv_is_32_bit(MachineState *machine);

+target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
+  target_ulong firmware_end_addr);
 target_ulong riscv_find_and_load_firmware(MachineState *machine,
   const char *default_machine_firmware,
   hwaddr firmware_load_addr,
@@ -34,6 +36,7 @@ target_ulong riscv_load_firmware(const char 
*firmware_filename,
  hwaddr firmware_load_addr,
  symbol_fn_t sym_cb);
 target_ulong riscv_load_kernel(const char *kernel_filename,
+   target_ulong firmware_end_addr,
symbol_fn_t sym_cb);
 hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
  uint64_t kernel_entry, hwaddr *start);
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 5dea644f47..9b3fe3fb1e 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -33,10 +33,8 @@
 #include 

 #if defined(TARGET_RISCV32)
-# define KERNEL_BOOT_ADDRESS 0x8040
 #define fw_dynamic_info_data(__val) cpu_to_le32(__val)
 #else
-# define KERNEL_BOOT_ADDRESS 0x8020
 #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
 #endif

@@ -49,6 +47,15 @@ bool riscv_is_32_bit(MachineState *machine)
 }
 }

+target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
+  target_ulong firmware_end_addr) {
+if (riscv_is_32_bit(machine)) {
+return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
+} else {
+return QEMU_ALIGN_UP(firmware_end_addr, 2 * MiB);
+}
+}
+
 target_ulong riscv_find_and_load_firmware(MachineState *machine,
   const char *default_machine_firmware,
   hwaddr firmware_load_addr,
@@ -123,7 +130,9 @@ target_ulong riscv_load_firmware(const char 
*firmware_filename,
 exit(1);
 }

-target_ulong riscv_load_kernel(const char *kernel_filename, symbol_fn_t sym_cb)
+target_ulong riscv_load_kernel(const char *kernel_filename,
+   target_ulong kernel_start_addr,
+   symbol_fn_t sym_cb)
 {
 uint64_t kernel_entry;

@@ -138,9 +147,9 @@ target_ulong riscv_load_kernel(const char *kernel_filename, 
symbol_fn_t sym_cb)
 return kernel_entry;
 }

-if (load_image_targphys_as(kernel_filename, KERNEL_BOOT_ADDRESS,
+if (load_image_targphys_as(kernel_filename, kernel_start_addr,
ram_size, NULL) > 0) {
-return KERNEL_BOOT_ADDRESS;
+return kernel_start_addr;
 }

 error_report("could not load kernel '%s'", kernel_filename);
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index 0531bd879b..cc758b78b8 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -75,7 +75,8 @@ static void opentitan_board_init(MachineState *machine)
 }

 if (machine->kernel_filename) {
-riscv_load_kernel(machine->kernel_filename, NULL);
+riscv_load_kernel(machine->kernel_filename,
+  memmap[IBEX_DEV_RAM].base, NULL);
 }
 }

diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index fcfac16816..59bac4cc9a 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState *machine)
   memmap[SIFIVE_E_DEV_MROM].base, 
_space_memory);

 if (machine->kernel_filename) {
-riscv_load_kernel(machine->kernel_filename, NULL);
+riscv_load_kernel(machine->kernel_filename,
+  memmap[SIFIVE_E_DEV_DTIM].base, NULL);
 }
 }

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 5f3ad9bc0f..b2472c6627 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -415,6 +415,7 @@ static void sifive_u_machine_init(MachineState *machine)
 MemoryRegion *main_mem = g_new(MemoryRegion, 1);
 MemoryRegion *flash0 = g_new(MemoryRegion, 1);
 

Re: [PATCH 0/2] tcg: optimize across branches

[Richard Henderson]

Ping.

On 10/13/20 3:23 PM, Richard Henderson wrote:
> In several cases, it's easy to optimize across a non-taken branch
> simply by *not* flushing the relevant tables.  This is true both
> for value propagation and register allocation.
> 
> This comes up in quite a number of cases with arm, most simply in
> how conditional execution is implemented.  But it also came up in
> discussion of how to implement low-overhead looping for v8.1m.
> 
> 
> r~
> 
> 
> Richard Henderson (2):
>   tcg: Do not kill globals at conditional branches
>   tcg/optimize: Flush data at labels not TCG_OPF_BB_END
> 
>  include/tcg/tcg-opc.h |  7 +++---
>  include/tcg/tcg.h |  4 +++-
>  tcg/optimize.c| 35 ++-
>  tcg/tcg.c | 55 +--
>  4 files changed, 78 insertions(+), 23 deletions(-)
> 

Re: [RFC] Don't lookup full CPU state in the indirect branch fast path on AArch64 when running in user mode.

[Richard Henderson]

On 10/19/20 3:44 PM, Owen Anderson wrote:
> My use case is currently using QEMU 4.0, but we will be moving to QEMU
> 4.2 soon.  I do not have --enable-tcg-debug enabled.
> e979972a6a1 does look promising, and like it might deliver increased
> performance for our use case. It looks like the code in 4.0 is doing a
> lot more work gathering the flags values from a variety of places.

Yes, before 4.2, we did a *lot* more work gathering flags, and the overhead you
see roughly corresponds with what I saw.


r~

[PATCH v5 2/6] migration: Introduce migrate_send_rp_message_req_pages()

[Peter Xu]

This is another layer wrapper for sending a page request to the source VM.  The
new migrate_send_rp_message_req_pages() will be used elsewhere in coming
patches.

Reviewed-by: Dr. David Alan Gilbert 
Signed-off-by: Peter Xu 
---
 migration/migration.c | 10 --
 migration/migration.h |  2 ++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index aca7fdcd0b..b2dac6b39c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -316,8 +316,8 @@ error:
  *   Start: Address offset within the RB
  *   Len: Length in bytes required - must be a multiple of pagesize
  */
-int migrate_send_rp_req_pages(MigrationIncomingState *mis, RAMBlock *rb,
-  ram_addr_t start)
+int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
+  RAMBlock *rb, ram_addr_t start)
 {
 uint8_t bufc[12 + 1 + 255]; /* start (8), len (4), rbname up to 256 */
 size_t msglen = 12; /* start + len */
@@ -353,6 +353,12 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, 
RAMBlock *rb,
 return migrate_send_rp_message(mis, msg_type, msglen, bufc);
 }
 
+int migrate_send_rp_req_pages(MigrationIncomingState *mis,
+  RAMBlock *rb, ram_addr_t start)
+{
+return migrate_send_rp_message_req_pages(mis, rb, start);
+}
+
 static bool migration_colo_enabled;
 bool migration_incoming_colo_enabled(void)
 {
diff --git a/migration/migration.h b/migration/migration.h
index deb411aaad..e853ccf8b1 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -333,6 +333,8 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
   uint32_t value);
 int migrate_send_rp_req_pages(MigrationIncomingState *mis, RAMBlock *rb,
   ram_addr_t start);
+int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
+  RAMBlock *rb, ram_addr_t start);
 void migrate_send_rp_recv_bitmap(MigrationIncomingState *mis,
  char *block_name);
 void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
-- 
2.26.2

[PATCH v5 4/6] migration: Sync requested pages after postcopy recovery

[Peter Xu]

We synchronize the requested pages right after a postcopy recovery happens.
This helps to synchronize the prioritized pages on source so that the faulted
threads can be served faster.

Reported-by: Xiaohui Li 
Signed-off-by: Peter Xu 
---
 migration/savevm.c | 57 ++
 migration/trace-events |  1 +
 2 files changed, 58 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index d2e141f7b1..33acbba1a4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2011,6 +2011,49 @@ static int 
loadvm_postcopy_handle_run(MigrationIncomingState *mis)
 return LOADVM_QUIT;
 }
 
+/* We must be with page_request_mutex held */
+static gboolean postcopy_sync_page_req(gpointer key, gpointer value,
+   gpointer data)
+{
+MigrationIncomingState *mis = data;
+void *host_addr = (void *) key;
+ram_addr_t rb_offset;
+RAMBlock *rb;
+int ret;
+
+rb = qemu_ram_block_from_host(host_addr, true, _offset);
+if (!rb) {
+/*
+ * This should _never_ happen.  However be nice for a migrating VM to
+ * not crash/assert.  Post an error (note: intended to not use *_once
+ * because we do want to see all the illegal addresses; and this can
+ * never be triggered by the guest so we're safe) and move on next.
+ */
+error_report("%s: illegal host addr %p", __func__, host_addr);
+/* Try the next entry */
+return FALSE;
+}
+
+ret = migrate_send_rp_message_req_pages(mis, rb, rb_offset);
+if (ret) {
+/* Please refer to above comment. */
+error_report("%s: send rp message failed for addr %p",
+ __func__, host_addr);
+return FALSE;
+}
+
+trace_postcopy_page_req_sync(host_addr);
+
+return FALSE;
+}
+
+static void migrate_send_rp_req_pages_pending(MigrationIncomingState *mis)
+{
+WITH_QEMU_LOCK_GUARD(>page_request_mutex) {
+g_tree_foreach(mis->page_requested, postcopy_sync_page_req, mis);
+}
+}
+
 static int loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
 {
 if (mis->state != MIGRATION_STATUS_POSTCOPY_RECOVER) {
@@ -2033,6 +2076,20 @@ static int 
loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
 /* Tell source that "we are ready" */
 migrate_send_rp_resume_ack(mis, MIGRATION_RESUME_ACK_VALUE);
 
+/*
+ * After a postcopy recovery, the source should have lost the postcopy
+ * queue, or potentially the requested pages could have been lost during
+ * the network down phase.  Let's re-sync with the source VM by re-sending
+ * all the pending pages that we eagerly need, so these threads won't get
+ * blocked too long due to the recovery.
+ *
+ * Without this procedure, the faulted destination VM threads (waiting for
+ * page requests right before the postcopy is interrupted) can keep hanging
+ * until the pages are sent by the source during the background copying of
+ * pages, or another thread faulted on the same address accidentally.
+ */
+migrate_send_rp_req_pages_pending(mis);
+
 return 0;
 }
 
diff --git a/migration/trace-events b/migration/trace-events
index e4d5eb94ca..0fbfd2da60 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -49,6 +49,7 @@ vmstate_save(const char *idstr, const char *vmsd_name) "%s, 
%s"
 vmstate_load(const char *idstr, const char *vmsd_name) "%s, %s"
 postcopy_pause_incoming(void) ""
 postcopy_pause_incoming_continued(void) ""
+postcopy_page_req_sync(void *host_addr) "sync page req %p"
 
 # vmstate.c
 vmstate_load_field_error(const char *field, int ret) "field \"%s\" load 
failed, ret = %d"
-- 
2.26.2

[PATCH v5 6/6] migration-test: Only hide error if !QTEST_LOG

[Peter Xu]

The errors are very useful when debugging qtest failures, especially when
QTEST_LOG=1 is set.  Let's allow override MigrateStart.hide_stderr when
QTEST_LOG=1 is specified, because that means the user wants to be verbose.

Not very nice to introduce the first QTEST_LOG env access in migration-test.c,
however it should be handy.  Without this patch, I was hacking error_report()
when debugging such errors.  Let's make things easier.

Signed-off-by: Peter Xu 
---
 tests/qtest/migration-test.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 00a233cd8c..ff9ed70029 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -461,6 +461,10 @@ static void migrate_postcopy_start(QTestState *from, 
QTestState *to)
 }
 
 typedef struct {
+/*
+ * QTEST_LOG=1 may override this.  When QTEST_LOG=1, we always dump errors
+ * unconditionally, because it means the user would like to be verbose.
+ */
 bool hide_stderr;
 bool use_shmem;
 /* only launch the target process */
@@ -554,7 +558,7 @@ static int test_migrate_start(QTestState **from, QTestState 
**to,
 
 g_free(bootpath);
 
-if (args->hide_stderr) {
+if (!getenv("QTEST_LOG") && args->hide_stderr) {
 ignore_stderr = "2>/dev/null";
 } else {
 ignore_stderr = "";
-- 
2.26.2

[PATCH v5 3/6] migration: Maintain postcopy faulted addresses

[Peter Xu]

Maintain a list of faulted addresses on the destination host for which we're
waiting on.  This is implemented using a GTree rather than a real list to make
sure even there're plenty of vCPUs/threads that are faulting, the lookup will
still be fast with O(log(N)) (because we'll do that after placing each page).
It should bring a slight overhead, but ideally that shouldn't be a big problem
simply because in most cases the requested page list will be short.

Actually we did similar things for postcopy blocktime measurements.  This patch
didn't use that simply because:

  (1) blocktime measurement is towards vcpu threads only, but here we need to
  record all faulted addresses, including main thread and external
  thread (like, DPDK via vhost-user).

  (2) blocktime measurement will require UFFD_FEATURE_THREAD_ID, but here we
  don't want to add that extra dependency on the kernel version since not
  necessary.  E.g., we don't need to know which thread faulted on which
  page, we also don't care about multiple threads faulting on the same
  page.  But we only care about what addresses are faulted so waiting for a
  page copying from src.

  (3) blocktime measurement is not enabled by default.  However we need this by
  default especially for postcopy recover.

Another thing to mention is that this patch introduced a new mutex to serialize
the receivedmap and the page_requested tree, however that serialization does
not cover other procedures like UFFDIO_COPY.

Signed-off-by: Peter Xu 
---
 migration/migration.c| 41 +++-
 migration/migration.h| 19 ++-
 migration/postcopy-ram.c | 17 ++---
 migration/trace-events   |  2 ++
 4 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b2dac6b39c..0b4fcff01f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -143,6 +143,13 @@ static int migration_maybe_pause(MigrationState *s,
  int new_state);
 static void migrate_fd_cancel(MigrationState *s);
 
+static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
+{
+uintptr_t a = (uintptr_t) ap, b = (uintptr_t) bp;
+
+return (a > b) - (a < b);
+}
+
 void migration_object_init(void)
 {
 MachineState *ms = MACHINE(qdev_get_machine());
@@ -165,6 +172,8 @@ void migration_object_init(void)
 qemu_event_init(_incoming->main_thread_load_event, false);
 qemu_sem_init(_incoming->postcopy_pause_sem_dst, 0);
 qemu_sem_init(_incoming->postcopy_pause_sem_fault, 0);
+qemu_mutex_init(_incoming->page_request_mutex);
+current_incoming->page_requested = g_tree_new(page_request_addr_cmp);
 
 if (!migration_object_check(current_migration, )) {
 error_report_err(err);
@@ -240,6 +249,11 @@ void migration_incoming_state_destroy(void)
 
 qemu_event_reset(>main_thread_load_event);
 
+if (mis->page_requested) {
+g_tree_destroy(mis->page_requested);
+mis->page_requested = NULL;
+}
+
 if (mis->socket_address_list) {
 qapi_free_SocketAddressList(mis->socket_address_list);
 mis->socket_address_list = NULL;
@@ -354,8 +368,33 @@ int 
migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
 }
 
 int migrate_send_rp_req_pages(MigrationIncomingState *mis,
-  RAMBlock *rb, ram_addr_t start)
+  RAMBlock *rb, ram_addr_t start, uint64_t haddr)
 {
+void *aligned = (void *)(uintptr_t)(haddr & qemu_real_host_page_mask);
+bool received;
+
+WITH_QEMU_LOCK_GUARD(>page_request_mutex) {
+received = ramblock_recv_bitmap_test_byte_offset(rb, start);
+if (!received && !g_tree_lookup(mis->page_requested, aligned)) {
+/*
+ * The page has not been received, and it's not yet in the page
+ * request list.  Queue it.  Set the value of element to 1, so that
+ * things like g_tree_lookup() will return TRUE (1) when found.
+ */
+g_tree_insert(mis->page_requested, aligned, (gpointer)1);
+mis->page_requested_count++;
+trace_postcopy_page_req_add(aligned, mis->page_requested_count);
+}
+}
+
+/*
+ * If the page is there, skip sending the message.  We don't even need the
+ * lock because as long as the page arrived, it'll be there forever.
+ */
+if (received) {
+return 0;
+}
+
 return migrate_send_rp_message_req_pages(mis, rb, start);
 }
 
diff --git a/migration/migration.h b/migration/migration.h
index e853ccf8b1..8d2d1ce839 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -104,6 +104,23 @@ struct MigrationIncomingState {
 
 /* List of listening socket addresses  */
 SocketAddressList *socket_address_list;
+
+/* A tree of pages that we requested to the source VM */
+GTree *page_requested;
+/* For debugging purpose 

[PATCH v5 5/6] migration/postcopy: Release fd before going into 'postcopy-pause'

[Peter Xu]

Logically below race could trigger with the old code:

  test programmigration thread
  
   wait_until('postcopy-pause')
  postcopy_pause()
set_state('postcopy-pause')
   do_postcopy_recover()
 arm s->to_dst_file with new fd
release s->to_dst_file [1]

Here [1] could have released the just-installed recoverying channel.  Then the
migration could hang without really resuming.

Instead, it should be very safe to release the fd before setting the state into
'postcopy-pause', because there's no reason for any other thread to touch it
during 'postcopy-active'.

Dave reported a very rare postcopy recovery hang that the migration-test
program waited for the migration to complete in migrate_postcopy_complete().
We do suspect it's the same thing that we're gonna fix here.  Hard to tell.
However since we've noticed this, fix this irrelevant of the hang report.

Cc: Dr. David Alan Gilbert 
Cc: Juan Quintela 
Signed-off-by: Peter Xu 
---
 migration/migration.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0b4fcff01f..50df6251b7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3182,9 +3182,6 @@ static MigThrError postcopy_pause(MigrationState *s)
 while (true) {
 QEMUFile *file;
 
-migrate_set_state(>state, s->state,
-  MIGRATION_STATUS_POSTCOPY_PAUSED);
-
 /* Current channel is possibly broken. Release it. */
 assert(s->to_dst_file);
 qemu_mutex_lock(>qemu_file_lock);
@@ -3195,6 +3192,9 @@ static MigThrError postcopy_pause(MigrationState *s)
 qemu_file_shutdown(file);
 qemu_fclose(file);
 
+migrate_set_state(>state, s->state,
+  MIGRATION_STATUS_POSTCOPY_PAUSED);
+
 error_report("Detected IO failure for postcopy. "
  "Migration paused.");
 
-- 
2.26.2

[PATCH v5 1/6] migration: Pass incoming state into qemu_ufd_copy_ioctl()

[Peter Xu]

It'll be used in follow up patches to access more fields out of it.  Meanwhile
fetch the userfaultfd inside the function.

Reviewed-by: Dr. David Alan Gilbert 
Signed-off-by: Peter Xu 
---
 migration/postcopy-ram.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 0a2f88a87d..722034dc01 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1128,10 +1128,12 @@ int postcopy_ram_incoming_setup(MigrationIncomingState 
*mis)
 return 0;
 }
 
-static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
+static int qemu_ufd_copy_ioctl(MigrationIncomingState *mis, void *host_addr,
void *from_addr, uint64_t pagesize, RAMBlock 
*rb)
 {
+int userfault_fd = mis->userfault_fd;
 int ret;
+
 if (from_addr) {
 struct uffdio_copy copy_struct;
 copy_struct.dst = (uint64_t)(uintptr_t)host_addr;
@@ -1185,7 +1187,7 @@ int postcopy_place_page(MigrationIncomingState *mis, void 
*host, void *from,
  * which would be slightly cheaper, but we'd have to be careful
  * of the order of updating our page state.
  */
-if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize, rb)) {
+if (qemu_ufd_copy_ioctl(mis, host, from, pagesize, rb)) {
 int e = errno;
 error_report("%s: %s copy host: %p from: %p (size: %zd)",
  __func__, strerror(e), host, from, pagesize);
@@ -1212,7 +1214,7 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, 
void *host,
  * but it's not available for everything (e.g. hugetlbpages)
  */
 if (qemu_ram_is_uf_zeroable(rb)) {
-if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, NULL, pagesize, rb)) {
+if (qemu_ufd_copy_ioctl(mis, host, NULL, pagesize, rb)) {
 int e = errno;
 error_report("%s: %s zero host: %p",
  __func__, strerror(e), host);
-- 
2.26.2

[PATCH v5 0/6] migration/postcopy: Sync faulted addresses after network recovered

[Peter Xu]

This is v5 of the series.  Probably my first series that got queued/unqueued
twice.

I found a bug in v4 that was about page sizes, however that didn't match with
PeterM's report on big endian hosts.  My manual reproduce on s390x also didn't
reproduce.  However after I ran the tree (with the fix) on travis (thanks
Thomas for suggesting this!) I noticed that s390x passed the test too:

https://travis-ci.com/github/xzpeter/qemu/builds/19103

I still got two tests that got timed out, however I also noticed that the
master branch got similarly two tests that timed out:

https://travis-ci.com/github/xzpeter/qemu/builds/191012879

There's one difference on the failed test, however I really suspect it's
because the uncertainly of the travis host scheduling the tests, or the master
failures should really be a subset of my own branch (while it's not).

So I decided to give it a 3rd shot... PeterM: would it be easy/possible to kick
the CI again against this series for pulls?  I just don't want to break Dave's
pull the 3rd time. :)

I also picked up the other patch [1] that should also fix some rare failures on
postcopy recovery.  However I bet we haven't yet encounter it, at least not 
often.

v5 changelog:
- added one test patch for easier debugging for migration-test
- added one fix patch [1] for another postcopy race
- fixed a bug that could trigger when host/guest page size differs

[1] 
https://lore.kernel.org/qemu-devel/20201007183324.288379-1-pet...@redhat.com/

- v4 cover letter --

v4:
- use "void */ulong" instead of "uint64_t" where proper in patch 3/4 [Dave]

v3:
- fix build on 32bit hosts & rebase
- remove r-bs for the last 2 patches for Dave due to the changes

v2:
- add r-bs for Dave
- add patch "migration: Properly destroy variables on incoming side" as patch 1
- destroy page_request_mutex in migration_incoming_state_destroy() too [Dave]
- use WITH_QEMU_LOCK_GUARD in two places where we can [Dave]

We've seen conditional guest hangs on destination VM after postcopy recovered.
However the hang will resolve itself after a few minutes.

The problem is: after a postcopy recovery, the prioritized postcopy queue on
the source VM is actually missing.  So all the faulted threads before the
postcopy recovery happened will keep halted until (accidentally) the page got
copied by the background precopy migration stream.

The solution is to also refresh this information after postcopy recovery.  To
achieve this, we need to maintain a list of faulted addresses on the
destination node, so that we can resend the list when necessary.  This work is
done via patch 2-5.

With that, the last thing we need to do is to send this extra information to
source VM after recovered.  Very luckily, this synchronization can be
"emulated" by sending a bunch of page requests (although these pages have been
sent previously!) to source VM just like when we've got a page fault.  Even in
the 1st version of the postcopy code we'll handle duplicated pages well.  So
this fix does not even need a new capability bit and it'll work smoothly on old
QEMUs when we migrate from them to the new QEMUs.

Please review, thanks.

Peter Xu (6):
  migration: Pass incoming state into qemu_ufd_copy_ioctl()
  migration: Introduce migrate_send_rp_message_req_pages()
  migration: Maintain postcopy faulted addresses
  migration: Sync requested pages after postcopy recovery
  migration/postcopy: Release fd before going into 'postcopy-pause'
  migration-test: Only hide error if !QTEST_LOG

 migration/migration.c| 55 ++
 migration/migration.h| 21 -
 migration/postcopy-ram.c | 25 
 migration/savevm.c   | 57 
 migration/trace-events   |  3 ++
 tests/qtest/migration-test.c |  6 +++-
 6 files changed, 154 insertions(+), 13 deletions(-)

-- 
2.26.2

Re: [RFC] Don't lookup full CPU state in the indirect branch fast path on AArch64 when running in user mode.

[Owen Anderson]

On Mon, Oct 19, 2020 at 11:22 AM Richard Henderson
 wrote:
>
> (1) What qemu version are you looking at and,
> (2) Do you have --enable-tcg-debug enabled?

My use case is a large automated testing environment for large C++
binaries with heavy use of virtual dispatch.  The binaries are
generally not built at high optimization levels (-O0 or -O1), so it's
not very surprising to me that indirect branches are more dominant in
this as a workload

My use case is currently using QEMU 4.0, but we will be moving to QEMU
4.2 soon.  I do not have --enable-tcg-debug enabled.
e979972a6a1 does look promising, and like it might deliver increased
performance for our use case. It looks like the code in 4.0 is doing a
lot more work gathering the flags values from a variety of places.

--Owen

Re: [PATCH v2 6/9] tcg: implement mirror mapped JIT for iOS

[Joelle van Dyne]

> Explicit cast may not be needed here so this could be a macro if caling it
> differently helps or why don't you just use tcg_mirror_prr_rw directly
> everywhere?

There are quite a bit of code that depends on tcg_insn_unit * type such as

*tcg_code_ptr_rw(s, code_ptr) = insn;

and

(tcg_code_ptr_rw(s, p))[i] = NOP;

I think it's cleaner to not have to manually cast in every one of 30+
instances of this. In v1, I used a macro but was told to use an inline
function instead.

> Is that !defined or are you missing an implementation and #else here?
No, `flush_dcache_range` is only needed when mirror mapped (after
writing to the RW mirror). Now there is no iOS compatible compiler for
any other arch than x86 and ARM. However, in the slim chance that
Apple decides to change arch again in the future and moves to RISC-V
or something, then we get a nice compiler error.

> Maybe this patch could be split up some more, making the RW offset
> handling and cache management separate patches even if they don't work
> separately just to make it easier to review.

I can probably do that for v3 but imo most of the LOC here is because
the same change has to be done to every TCG target. No matter how you
split up the patches, it will look like a lot of changes.

-j

On Mon, Oct 19, 2020 at 4:48 AM BALATON Zoltan  wrote:
>
> On Sun, 18 Oct 2020, Joelle van Dyne wrote:
> > From: osy 
> >
> > On iOS, we cannot allocate RWX pages without special entitlements. As a
> > workaround, we can allocate a RX region and then mirror map it to a separate
> > RX region. Then we can write to one region and execute from the other one.
> >
> > We also define `tcg_mirror_ptr_rw` and `tcg_code_ptr_rw` to return a pointer
> > to RW memory. The difference between the RW and RX regions is stored in the
> > TCG context.
> >
> > To ensure cache coherency, we flush the data cache in the RW mapping and
> > then invalidate the instruction cache in the RX mapping (where applicable).
> > Because data cache flush is OS defined on some architectures, we do not
> > provide implementations for non iOS platforms (ARM/x86).
> >
> > Signed-off-by: Joelle van Dyne 
> > ---
> > docs/devel/ios.rst   | 40 +++
> > configure|  1 +
> > include/exec/exec-all.h  |  8 
> > include/tcg/tcg.h| 17 
> > tcg/aarch64/tcg-target.h | 13 +-
> > tcg/arm/tcg-target.h |  9 -
> > tcg/i386/tcg-target.h| 24 ++-
> > tcg/mips/tcg-target.h|  8 +++-
> > tcg/ppc/tcg-target.h |  8 +++-
> > tcg/riscv/tcg-target.h   |  9 -
> > tcg/s390/tcg-target.h| 13 +-
> > tcg/sparc/tcg-target.h   |  8 +++-
> > tcg/tci/tcg-target.h |  9 -
> > accel/tcg/cpu-exec.c |  7 +++-
> > accel/tcg/translate-all.c| 77 ++--
> > tcg/tcg.c| 56 +-
> > tcg/aarch64/tcg-target.c.inc | 33 ++--
> > tcg/arm/tcg-target.c.inc | 25 ++--
> > tcg/i386/tcg-target.c.inc| 18 -
> > tcg/mips/tcg-target.c.inc| 35 +---
> > tcg/ppc/tcg-target.c.inc | 38 +++---
> > tcg/riscv/tcg-target.c.inc   | 40 +++
> > tcg/s390/tcg-target.c.inc| 16 
> > tcg/sparc/tcg-target.c.inc   | 23 +++
> > tcg/tcg-pool.c.inc   |  9 +++--
> > tcg/tci/tcg-target.c.inc |  6 +--
> > 26 files changed, 416 insertions(+), 134 deletions(-)
> > create mode 100644 docs/devel/ios.rst
> >
> > diff --git a/docs/devel/ios.rst b/docs/devel/ios.rst
> > new file mode 100644
> > index 00..dba9fdd868
> > --- /dev/null
> > +++ b/docs/devel/ios.rst
> > @@ -0,0 +1,40 @@
> > +===
> > +iOS Support
> > +===
> > +
> > +To run qemu on the iOS platform, some modifications were required. Most of 
> > the
> > +modifications are conditioned on the ``CONFIG_IOS`` and ``CONFIG_IOS_JIT``
> > +configuration variables.
> > +
> > +Build support
> > +-
> > +
> > +For the code to compile, certain changes in the block driver and the slirp
> > +driver had to be made. There is no ``system()`` call, so code requiring it 
> > had
> > +to be disabled.
> > +
> > +``ucontext`` support is broken on iOS. The implementation from 
> > ``libucontext``
> > +is used instead.
> > +
> > +Because ``fork()`` is not allowed on iOS apps, the option to build qemu 
> > and the
> > +utilities as shared libraries is added. Note that because qemu does not 
> > perform
> > +resource cleanup in most cases (open files, allocated memory, etc), it is
> > +advisable that the user implements a proxy layer for syscalls so resources 
> > can
> > +be kept track by the app that uses qemu as a shared library.
> > +
> > +JIT support
> > +---
> > +
> > +On iOS, allocating RWX pages require special entitlements not usually 
> > granted to
> > +apps. However, it is possible to use `bulletproof JIT`_ with a development
> > +certificate. This means 

Re: [PATCH v2 2/9] configure: cross-compiling without cross_prefix

[Joelle van Dyne]

Correct me if I'm wrong but wouldn't the following test still fail
with --cross-prefix=""

if test -n "$cross_prefix"; then
...

That was my main reason for making this change.

-j

On Mon, Oct 19, 2020 at 4:24 AM BALATON Zoltan  wrote:
>
> On Mon, 19 Oct 2020, Thomas Huth wrote:
> > On 19/10/2020 10.07, Thomas Huth wrote:
> >> On 19/10/2020 03.39, Joelle van Dyne wrote:
> >>> From: osy 
> >>>
> >>> The iOS toolchain does not use the host prefix naming convention. We add a
> >>> new option `--enable-cross-compile` that forces cross-compile even without
> >>> a cross_prefix.
> >>>
> >>> Signed-off-by: Joelle van Dyne 
> >>> ---
> >>>  configure | 13 -
> >>>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/configure b/configure
> >>> index 3c63879750..46d5db63e8 100755
> >>> --- a/configure
> >>> +++ b/configure
> >>> @@ -234,6 +234,7 @@ cpu=""
> >>>  iasl="iasl"
> >>>  interp_prefix="/usr/gnemul/qemu-%M"
> >>>  static="no"
> >>> +cross_compile="no"
> >>>  cross_prefix=""
> >>>  audio_drv_list=""
> >>>  block_drv_rw_whitelist=""
> >>> @@ -456,6 +457,11 @@ for opt do
> >>>optarg=$(expr "x$opt" : 'x[^=]*=\(.*\)')
> >>>case "$opt" in
> >>>--cross-prefix=*) cross_prefix="$optarg"
> >>> +cross_compile="yes"
> >>> +  ;;
> >>> +  --enable-cross-compile) cross_compile="yes"
> >>> +  ;;
> >>> +  --disable-cross-compile) cross_compile="no"
> >>
> >> Can't you simply use --cros-prefix="" instead?
> >
> > I mean, still introduce the "cross_compile=yes" variable, just omit the new
> > options.
>
> That seems less intuitive for people trying to find this option. If --help
> lists --enable-cross-compile I can guess what that means but there's no
> way I could guess --cros-prefix="" unless I've been told or searched and
> stumbled upon it. So unless it's a big problem I like the explicit options
> better. Or is that a convention in other projects to use empty prefix to
> enable cross compile that I don't know about?
>
> Regards,
> BALATON Zoltan

Re: [PATCH v2 9/9] block: check availablity for preadv/pwritev on mac

[Joelle van Dyne]

On Mon, Oct 19, 2020 at 1:27 AM Thomas Huth  wrote:
>
> On 19/10/2020 03.39, Joelle van Dyne wrote:
> > From: osy 
>
> That "From:" line looks wrong ... could you please fix the "Author" of your
> patches / your git config?
osy wrote the original changes. I joined the UTM project to help bring
the changes upstream with permission. However, they have agreed that
if required that we can use my name as the author.

>
> > macOS 11/iOS 14 added preadv/pwritev APIs. Due to weak linking, configure
> > will succeed with CONFIG_PREADV even when targeting a lower OS version. We
> > therefore need to check at run time if we can actually use these APIs.
>
> That sounds like the wrong approach to me ... could you please try to fix
> the check in "configure" instead? E.g. by running compile_prog with
> "-Werror", so that the test fails if there is no valid prototype available?
It's not that simple. Xcode 11 and below (supporting macOS 10.15 and
below, iOS 13 and below, etc) does not have preadv/pwritev symbols
defined and would fail to compile. Xcode 12 (supporting macOS 11 and
below, iOS 14 and below, etc) have preadv/pwritev weakly defined so if
it runs on, for example, 10.15, it would abort. There is no way to
determine at compile time if you can use preadv/pwritev or not when
building with Xcode 12. The availability checks are Apple's preferred
way to handle this kind of situation (they discourage directly
checking if an API exists on a system).

-j

>
>  Thomas
>

Re: [PATCH 0/6] hw/pci-host/sabre: Report UNIMP/GUEST_ERROR accesses

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:


Notes while trying to understand Mark's patch from yesterday:
"sabre: increase number of PCI bus IRQs from 32 to 64"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg749458.html

Philippe Mathieu-Daudé (6):
   hw/pci-host/sabre: Update documentation link
   hw/pci-host/sabre: Remove superfluous address range check
   hw/pci-host/sabre: Simplify code initializing variable once
   hw/pci-host/sabre: Report unimplemented accesses via UNIMP log_mask
   hw/pci-host/sabre: Report IOMMU address range as unimplemented
   hw/pci-host/sabre: Log reserved address accesses as GUEST_ERROR

  hw/pci-host/sabre.c | 40 ++--
  1 file changed, 22 insertions(+), 18 deletions(-)


Thanks for this - I've applied patches 1-3 to my qemu-sparc branch with some comments 
on patches 4-6. I should be able to look at these later in the week if you're 
currently busy.



ATB,

Mark.

Re: [PATCH 6/6] hw/pci-host/sabre: Log reserved address accesses as GUEST_ERROR

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:


Report accesses to reserved registers using qemu_log_mask(GUEST_ERROR).

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/pci-host/sabre.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 67699ac9058..cc97c266a57 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -189,7 +189,11 @@ static void sabre_config_write(void *opaque, hwaddr addr,
  case 0xa800 ... 0xa80f: /* Interrupt diagnostics */
  case 0xf000 ... 0xf01f: /* FFB config, memory control */
  /* we don't care */
+break;
  default:
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Register 0x%04" HWADDR_PRIX " is reserved\n",
+  __func__, addr);
  break;
  }
  }
@@ -235,7 +239,11 @@ static uint64_t sabre_config_read(void *opaque,
  case 0xa800 ... 0xa80f: /* Interrupt diagnostics */
  case 0xf000 ... 0xf01f: /* FFB config, memory control */
  /* we don't care */
+break;
  default:
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Register 0x%04" HWADDR_PRIX " is reserved\n",
+  __func__, addr);
  break;
  }
  trace_sabre_config_read(addr, val);


As per my comment on patch 4, I think these should be logged at LOG_UNIMP and the 
message changed to "is unimplemented". Other than that:


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH v3 34/81] target/arm: Implement SVE2 WHILERW, WHILEWR

[Richard Henderson]

On 10/12/20 7:33 PM, LIU Zhiwei wrote:
>> +if (a->rw) {
>> +/* WHILERW */
>> +/* diff = abs(op1 - op0), noting that op0/1 are unsigned. */
>> +t1 = tcg_temp_new_i64();
>> +tcg_gen_sub_i64(diff, op0, op1);
>> +tcg_gen_sub_i64(t1, op1, op0);
>> +tcg_gen_movcond_i64(TCG_COND_LTU, diff, op0, op1, diff, t1);
> It should be:
> 
> tcg_gen_movcond_i64(TCG_COND_GTU, diff, op0, op1, diff, t1);

Yep.

> 
>> +tcg_temp_free_i64(t1);
>> +/* If op1 == op0, diff == 0, and the condition is always true. */
>> +tcg_gen_movcond_i64(TCG_COND_EQ, diff, op0, op1, tmax, diff);
>> +} else {
>> +/* WHILEWR */
>> +tcg_gen_sub_i64(diff, op1, op0);
>> +/* If op0 >= op1, diff <= 0, the condition is always true. */
>> +tcg_gen_movcond_i64(TCG_COND_GEU, diff, op0, op1, tmax, diff);
>> +}
>> +
>> +/* Bound to the maximum.  */
>> +tcg_gen_umin_i64(diff, diff, tmax);
>> +tcg_temp_free_i64(tmax);
>> +
>> +/* Since we're bounded, pass as a 32-bit type.  */
>> +t2 = tcg_temp_new_i32();
>> +tcg_gen_extrl_i64_i32(t2, diff);
> We should align count down to (1 << esz),
> 
> tcg_gen_andi_i32(t2，~MAKE_64BIT_MASK(0, esz));

Yep, this corresponds to the "DIV (esize DIV 8)" portion of the psuedo code.
But it needs to go earlier, before we compare diff against 0 in the two movcond
above.

Will fix.  Thanks,


r~

Re: [PATCH 5/6] hw/pci-host/sabre: Report IOMMU address range as unimplemented

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:


Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/pci-host/sabre.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 4412e23131c..67699ac9058 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -122,6 +122,7 @@ static void sabre_config_write(void *opaque, hwaddr addr,
  
  switch (addr) {

  case  0x30 ...  0x4f: /* DMA error registers */
+case 0x200 ... 0x21f: /* IOMMU registers */
  qemu_log_mask(LOG_UNIMP,
"%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
__func__, addr);
@@ -201,6 +202,7 @@ static uint64_t sabre_config_read(void *opaque,
  
  switch (addr) {

  case  0x30 ...  0x4f: /* DMA error registers */
+case 0x200 ... 0x21f: /* IOMMU registers */
  qemu_log_mask(LOG_UNIMP,
"%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
__func__, addr);


In theory this should never happen since a reference to the IOMMU should always be 
set using an object property link (i.e. it is a developer error rather than an 
unimplemented error) and its memory region overlaps this space within the PCI host 
bridge.


Rather than add these logging statemants and/or failing if the property is not set, I 
think now it may be possible to simply embed the IOMMU device within sabre itself 
using the updated QOM APIs. I can take a look to see if this approach will work later 
in the week.



ATB,

Mark.

Re: [PATCH 4/6] hw/pci-host/sabre: Report unimplemented accesses via UNIMP log_mask

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:


Report unimplemented register accesses using qemu_log_mask(UNIMP).

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/pci-host/sabre.c | 12 
  1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 3645bc962cb..4412e23131c 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -121,8 +121,10 @@ static void sabre_config_write(void *opaque, hwaddr addr,
  trace_sabre_config_write(addr, val);
  
  switch (addr) {

-case 0x30 ... 0x4f: /* DMA error registers */
-/* XXX: not implemented yet */
+case  0x30 ...  0x4f: /* DMA error registers */
+qemu_log_mask(LOG_UNIMP,
+  "%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
+  __func__, addr);
  break;
  case 0xc00 ... 0xc3f: /* PCI interrupt control */
  if (addr & 4) {
@@ -198,8 +200,10 @@ static uint64_t sabre_config_read(void *opaque,
  uint32_t val = 0;
  
  switch (addr) {

-case 0x30 ... 0x4f: /* DMA error registers */
-/* XXX: not implemented yet */
+case  0x30 ...  0x4f: /* DMA error registers */
+qemu_log_mask(LOG_UNIMP,
+  "%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
+  __func__, addr);
  break;
  case 0xc00 ... 0xc3f: /* PCI interrupt control */
  if (addr & 4) {


It seems as if there are quite a few other registers that haven't been implemented 
here which aren't mentioned in the comments. My preference would be to rework this 
patch so that the comments for the unimplemented registers are all at the end of the 
switch() with the fallthrough to default, and then update patch 6 to use LOG_UNIMP so 
everything is logged in one place.



ATB,

Mark.

Re: [PATCH v4 4/7] nbd: Update qapi to support exporting multiple bitmaps

[Eric Blake]

On 10/14/20 7:15 AM, Vladimir Sementsov-Ogievskiy wrote:

10.10.2020 00:55, Eric Blake wrote:

Since 'nbd-server-add' is deprecated, and 'block-export-add' is new to
5.2, we can still tweak the interface.  Allowing 'bitmaps':['str'] is
nicer than 'bitmap':'str'.  This wires up the qapi and qemu-nbd
changes to permit passing multiple bitmaps as distinct metadata
contexts that the NBD client may request, but the actual support for
more than one will require a further patch to the server.

Signed-off-by: Eric Blake 
---


[..]


  break;
  case 'B':
-    bitmap = optarg;
+    tmp = g_new(strList, 1);
+    tmp->value = g_strdup(optarg);
+    tmp->next = bitmaps;
+    bitmaps = tmp;


If publish QAPI_LIST_ADD, defined in block.c, it would look like:

     QAPI_LIST_ADD(bitmaps, g_strdup(optarg));


#define QAPI_LIST_ADD(list, element) do { \
typeof(list) _tmp = g_new(typeof(*(list)), 1); \
_tmp->value = (element); \
_tmp->next = (list); \
(list) = _tmp; \
} while (0)


Markus, thoughts on if we should publish this macro, and if so, which 
header would be best?





anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy 



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

Re: [PATCH v10 07/10] memory: Add interface to set iommu page size mask

[Peter Xu]

On Thu, Oct 08, 2020 at 07:15:55PM +0200, Jean-Philippe Brucker wrote:
> From: Bharat Bhushan 
> 
> Allow to set the page size mask supported by an iommu memory region.
> This enables a vIOMMU to communicate the page size granule supported by
> an assigned device, on hosts that use page sizes greater than 4kB.
> 
> Signed-off-by: Bharat Bhushan 
> Signed-off-by: Jean-Philippe Brucker 

Acked-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v6 06/10] migration: control whether snapshots are ovewritten

[Eric Blake]

On 10/8/20 10:49 AM, Daniel P. Berrangé wrote:

The traditional HMP "savevm" command will overwrite an existing snapshot
if it already exists with the requested name. This new flag allows this
to be controlled allowing for safer behaviour with a future QMP command.

Signed-off-by: Daniel P. Berrangé 
---


Reviewed-by: Eric Blake 

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

Re: [PATCH v10 01/10] virtio-iommu: Fix virtio_iommu_mr()

[Peter Xu]

On Thu, Oct 08, 2020 at 07:15:49PM +0200, Jean-Philippe Brucker wrote:
> Due to an invalid mask, virtio_iommu_mr() may return the wrong memory
> region. It hasn't been too problematic so far because the function was
> only used to test existence of an endpoint, but that is about to change.
> 
> Fixes: cfb42188b24d ("virtio-iommu: Implement attach/detach command")
> Signed-off-by: Jean-Philippe Brucker 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v10 09/10] virtio-iommu: Set supported page size mask

[Peter Xu]

On Thu, Oct 08, 2020 at 07:15:57PM +0200, Jean-Philippe Brucker wrote:
> From: Bharat Bhushan 
> 
> The virtio-iommu device can deal with arbitrary page sizes for virtual
> endpoints, but for endpoints assigned with VFIO it must follow the page
> granule used by the host IOMMU driver.
> 
> Implement the interface to set the vIOMMU page size mask, called by VFIO
> for each endpoint. We assume that all host IOMMU drivers use the same
> page granule (the host page granule). Override the page_size_mask field
> in the virtio config space.
> 
> Signed-off-by: Bharat Bhushan 
> Signed-off-by: Jean-Philippe Brucker 
> ---
> v10: Use global page mask, allowing VFIO to override it until boot.
> ---
>  hw/virtio/virtio-iommu.c | 51 
>  1 file changed, 51 insertions(+)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 8823bfc804a..dd0b3093d1b 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -914,6 +914,56 @@ static int 
> virtio_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu_mr,
>  return 0;
>  }
>  
> +static int virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
> +   uint64_t page_size_mask,
> +   Error **errp)
> +{
> +int new_granule, old_granule;
> +IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
> +VirtIOIOMMU *s = sdev->viommu;
> +
> +if (!page_size_mask) {
> +return -1;
> +}
> +
> +new_granule = ctz64(page_size_mask);
> +old_granule = ctz64(s->config.page_size_mask);
> +
> +/*
> + * Modifying the page size after machine initialization isn't supported.
> + * Having a different mask is possible but the guest will use sub-optimal
> + * block sizes, so warn about it.
> + */
> +if (qdev_hotplug) {
> +if (new_granule != old_granule) {
> +error_setg(errp,
> +   "virtio-iommu page mask 0x%"PRIx64
> +   " is incompatible with mask 0x%"PRIx64,
> +   s->config.page_size_mask, page_size_mask);
> +return -1;
> +} else if (page_size_mask != s->config.page_size_mask) {
> +warn_report("virtio-iommu page mask 0x%"PRIx64
> +" does not match 0x%"PRIx64,
> +s->config.page_size_mask, page_size_mask);
> +}
> +return 0;
> +}
> +
> +/*
> + * Disallow shrinking the page size. For example if an endpoint only
> + * supports 64kB pages, we can't globally enable 4kB pages. But that
> + * shouldn't happen, the host is unlikely to setup differing page 
> granules.
> + * The other bits are only hints describing optimal block sizes.
> + */
> +if (new_granule < old_granule) {
> +error_setg(errp, "memory region shrinks the virtio-iommu page 
> granule");
> +return -1;
> +}

My understanding is that shrink is actually allowed, instead we should forbid
growing of the mask?  For example, initially the old_granule will always points
to the guest page size.  Then as long as the host page size (which new_granule
represents) is smaller than the old_granule, then it seems fine... Or am I 
wrong?

Another thing, IIUC this function will be majorly called in vfio code when the
container page mask will be passed into it.  If there're multiple vfio
containers that support different host IOMMU page sizes, then IIUC the order of
the call to virtio_iommu_set_page_size_mask() is undefined.  It's probably
related to which "-device vfio-pci,..." parameter is earlier.

To make this simpler, I'm thinking whether we should just forbid the case where
devices have different iommu page sizes.  So when assigned devices are used, we
make sure all host iommu page sizes are the same, and the value should be
smaller than guest page size.  Otherwise we'll simply fall back to guest psize.

Thanks,

> +
> +s->config.page_size_mask = page_size_mask;
> +return 0;
> +}
> +
>  static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
>  {
>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -1146,6 +1196,7 @@ static void 
> virtio_iommu_memory_region_class_init(ObjectClass *klass,
>  imrc->translate = virtio_iommu_translate;
>  imrc->replay = virtio_iommu_replay;
>  imrc->notify_flag_changed = virtio_iommu_notify_flag_changed;
> +imrc->iommu_set_page_size_mask = virtio_iommu_set_page_size_mask;
>  }
>  
>  static const TypeInfo virtio_iommu_info = {
> -- 
> 2.28.0
> 

-- 
Peter Xu

Re: [PATCH v6 05/10] block: rename and alter bdrv_all_find_snapshot semantics

[Eric Blake]

On 10/8/20 10:49 AM, Daniel P. Berrangé wrote:

Currently bdrv_all_find_snapshot() will return 0 if it finds
a snapshot, -1 if an error occurs, or if it fails to find a
snapshot. New callers to be added want to distinguish between
the error scenario and failing to find a snapshot.

Rename it to bdrv_all_has_snapshot and make it return -1 on
error, 0 if no snapshot is found and 1 if snapshot is found.

Signed-off-by: Daniel P. Berrangé 
---

Reviewed-by: Eric Blake 

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

Re: [PATCH v6 04/10] block: allow specifying name of block device for vmstate storage

[Eric Blake]

On 10/8/20 10:49 AM, Daniel P. Berrangé wrote:

Currently the vmstate will be stored in the first block device that
supports snapshots. Historically this would have usually been the
root device, but with UEFI it might be the variable store. There
needs to be a way to override the choice of block device to store
the state in.

Signed-off-by: Daniel P. Berrangé 
---



@@ -83,7 +83,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
  (qemu) savevm snap0
  Error: Device 'file' is writable but does not support snapshots
  (qemu) info snapshots
-No block device supports snapshots
+no block device can store vmstate for snapshot


We're inconsistent on whether error messages start with a Capital.  But 
our split-brain behavior is not made any worse by this patch.


Reviewed-by: Eric Blake 

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

Re: [PATCH] scripts/qmp: delete 'qmp' script

[Eric Blake]

On 10/19/20 4:04 PM, John Snow wrote:

This script has not seen a patch that was specifically for this script
since it was moved to this location in 2013, and I doubt it is used. It
uses "man qmp" for its help message, which does not exist. It also
presumes there is a manual page for qmp-XXX, for each defined qmp
command XXX. I don't think that's true.

The format it expects arguments in is something like:

block-dirty-bitmap-add --node=foo --name=bar

and has no capacity to support nested JSON arguments, either.

Most developers use either qmp-shell or socat (or pasting JSON directly
into qmp stdio), so this duplication and additional alternate syntax is
not helpful.

Remove it. Leave a breadcrumb script just in case, to be removed next
release cycle.

Signed-off-by: John Snow 
---
  scripts/qmp/qmp | 131 +++-
  1 file changed, 7 insertions(+), 124 deletions(-)

-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp import QEMUMonitorProtocol
+print('''This unmaintained and undocumented script was removed in preference
+for qmp-shell. The assumption is that most users are using either
+qmp-shell, socat, or pasting/piping JSON into stdio. The duplication of
+facilities here is unwanted, and the divergence of syntax harmful.''',
+  file=sys.stderr)
  


Reviewed-by: Eric Blake 

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

Re: [PATCH v6 03/10] block: add ability to specify list of blockdevs during snapshot

[Eric Blake]

On 10/8/20 10:49 AM, Daniel P. Berrangé wrote:

When running snapshot operations, there are various rules for which
blockdevs are included/excluded. While this provides reasonable default
behaviour, there are scenarios that are not well handled by the default
logic. Some of the conditions do not have a single correct answer.

Thus there needs to be a way for the mgmt app to provide an explicit
list of blockdevs to perform snapshots across. This can be achieved
by passing a list of node names that should be used.

Signed-off-by: Daniel P. Berrangé 
---



+static int bdrv_all_get_snapshot_devices(bool has_devices, strList *devices,
+ GList **all_bdrvs,
+ Error **errp)
+{
+g_autoptr(GList) bdrvs = NULL;
+
+if (has_devices) {
+if (!devices) {
+error_setg(errp, "At least one device is required for snapshot");
+return -1;
+}
+
+while (devices) {
+BlockDriverState *bs = bdrv_find_node(devices->value);
+if (!bs) {
+error_setg(errp, "No block device node '%s'", devices->value);
+return -1;
+}
+bdrvs = g_list_append(bdrvs, bs);
+devices = devices->next;
+}


Do we care if the user passes the same device more than once in their 
list?  (If so, a hash table might be better than g_list)


Otherwise, looks good to me.
Reviewed-by: Eric Blake 

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

Re: [PATCH v5] sev: add sev-inject-launch-secret

[Tobin Feldman-Fitzthum]

On 2020-10-19 12:47, Eduardo Habkost wrote:

On Mon, Oct 19, 2020 at 12:46:08PM -0400, Eduardo Habkost wrote:

On Thu, Oct 15, 2020 at 10:37:13AM -0400, to...@linux.ibm.com wrote:
[...]
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index 88e3f39a1e..2d2ee54cc6 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -49,3 +49,8 @@ SevCapability *sev_get_capabilities(Error **errp)
>  error_setg(errp, "SEV is not available in this QEMU");
>  return NULL;
>  }
> +int sev_inject_launch_secret(const char *hdr, const char *secret,
> + uint64_t gpa)
> +{
> +return 1;
> +}

This doesn't match the actual function prototype.  I had to apply the 
following

fixup:

---
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 2d2ee54cc6..62a2587e7b 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -49,8 +49,10 @@ SevCapability *sev_get_capabilities(Error **errp)
 error_setg(errp, "SEV is not available in this QEMU");
 return NULL;
 }
+
 int sev_inject_launch_secret(const char *hdr, const char *secret,
- uint64_t gpa)
+ uint64_t gpa, Error *errp)


Oops. Fixing up the fixup:


Thanks Eduardo.

-Tobin


---
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 62a2587e7b..e4e60d9a7d 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -51,7 +51,7 @@ SevCapability *sev_get_capabilities(Error **errp)
 }

 int sev_inject_launch_secret(const char *hdr, const char *secret,
- uint64_t gpa, Error *errp)
+ uint64_t gpa, Error **errp)
 {
 error_setg(errp, "SEV is not available in this QEMU");
 return 1;

Re: [PATCH v6 02/10] migration: stop returning errno from load_snapshot()

[Eric Blake]

On 10/8/20 10:49 AM, Daniel P. Berrangé wrote:

None of the callers care about the errno value since there is a full
Error object populated. This gives consistency with save_snapshot()
which already just returns -1.

Reviewed-by: Dr. David Alan Gilbert 
Signed-off-by: Daniel P. Berrangé 
---
  migration/savevm.c | 15 +++
  1 file changed, 7 insertions(+), 8 deletions(-)




@@ -2892,11 +2892,11 @@ int load_snapshot(const char *name, Error **errp)
  ret = bdrv_snapshot_find(bs_vm_state, , name);
  aio_context_release(aio_context);
  if (ret < 0) {
-return ret;
+return -1;
  } else if (sn.vm_state_size == 0) {
  error_setg(errp, "This is a disk-only snapshot. Revert to it "
 " offline using qemu-img");


While you are here, let's fix the double space in the error message.

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

[PATCH] scripts/qmp: delete 'qmp' script

[John Snow]

This script has not seen a patch that was specifically for this script
since it was moved to this location in 2013, and I doubt it is used. It
uses "man qmp" for its help message, which does not exist. It also
presumes there is a manual page for qmp-XXX, for each defined qmp
command XXX. I don't think that's true.

The format it expects arguments in is something like:

block-dirty-bitmap-add --node=foo --name=bar

and has no capacity to support nested JSON arguments, either.

Most developers use either qmp-shell or socat (or pasting JSON directly
into qmp stdio), so this duplication and additional alternate syntax is
not helpful.

Remove it. Leave a breadcrumb script just in case, to be removed next
release cycle.

Signed-off-by: John Snow 
---
 scripts/qmp/qmp | 131 +++-
 1 file changed, 7 insertions(+), 124 deletions(-)

diff --git a/scripts/qmp/qmp b/scripts/qmp/qmp
index 8e52e4a54d..0f12307c87 100755
--- a/scripts/qmp/qmp
+++ b/scripts/qmp/qmp
@@ -1,128 +1,11 @@
 #!/usr/bin/env python3
-#
-# QMP command line tool
-#
-# Copyright IBM, Corp. 2011
-#
-# Authors:
-#  Anthony Liguori 
-#
-# This work is licensed under the terms of the GNU GPLv2 or later.
-# See the COPYING file in the top-level directory.
 
-import sys, os
+import sys
 
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp import QEMUMonitorProtocol
+print('''This unmaintained and undocumented script was removed in preference
+for qmp-shell. The assumption is that most users are using either
+qmp-shell, socat, or pasting/piping JSON into stdio. The duplication of
+facilities here is unwanted, and the divergence of syntax harmful.''',
+  file=sys.stderr)
 
-def print_response(rsp, prefix=[]):
-if type(rsp) == list:
-i = 0
-for item in rsp:
-if prefix == []:
-prefix = ['item']
-print_response(item, prefix[:-1] + ['%s[%d]' % (prefix[-1], i)])
-i += 1
-elif type(rsp) == dict:
-for key in rsp.keys():
-print_response(rsp[key], prefix + [key])
-else:
-if len(prefix):
-print('%s: %s' % ('.'.join(prefix), rsp))
-else:
-print('%s' % (rsp))
-
-def main(args):
-path = None
-
-# Use QMP_PATH if it's set
-if 'QMP_PATH' in os.environ:
-path = os.environ['QMP_PATH']
-
-while len(args):
-arg = args[0]
-
-if arg.startswith('--'):
-arg = arg[2:]
-if arg.find('=') == -1:
-value = True
-else:
-arg, value = arg.split('=', 1)
-
-if arg in ['path']:
-if type(value) == str:
-path = value
-elif arg in ['help']:
-os.execlp('man', 'man', 'qmp')
-else:
-print('Unknown argument "%s"' % arg)
-
-args = args[1:]
-else:
-break
-
-if not path:
-print("QMP path isn't set, use --path=qmp-monitor-address or set 
QMP_PATH")
-return 1
-
-if len(args):
-command, args = args[0], args[1:]
-else:
-print('No command found')
-print('Usage: "qmp [--path=qmp-monitor-address] qmp-cmd arguments"')
-return 1
-
-if command in ['help']:
-os.execlp('man', 'man', 'qmp')
-
-srv = QEMUMonitorProtocol(path)
-srv.connect()
-
-def do_command(srv, cmd, **kwds):
-rsp = srv.cmd(cmd, kwds)
-if 'error' in rsp:
-raise Exception(rsp['error']['desc'])
-return rsp['return']
-
-commands = map(lambda x: x['name'], do_command(srv, 'query-commands'))
-
-srv.close()
-
-if command not in commands:
-fullcmd = 'qmp-%s' % command
-try:
-os.environ['QMP_PATH'] = path
-os.execvp(fullcmd, [fullcmd] + args)
-except OSError as exc:
-if exc.errno == 2:
-print('Command "%s" not found.' % (fullcmd))
-return 1
-raise
-return 0
-
-srv = QEMUMonitorProtocol(path)
-srv.connect()
-
-arguments = {}
-for arg in args:
-if not arg.startswith('--'):
-print('Unknown argument "%s"' % arg)
-return 1
-
-arg = arg[2:]
-if arg.find('=') == -1:
-value = True
-else:
-arg, value = arg.split('=', 1)
-
-if arg in ['help']:
-os.execlp('man', 'man', 'qmp-%s' % command)
-return 1
-
-arguments[arg] = value
-
-rsp = do_command(srv, command, **arguments)
-print_response(rsp)
-
-if __name__ == '__main__':
-sys.exit(main(sys.argv[1:]))
+sys.exit(1)
-- 
2.26.2

Re: [PATCH v7 03/11] hw/block/nvme: Add support for Namespace Types

[Klaus Jensen]

On Oct 19 11:17, Dmitry Fomichev wrote:
> From: Niklas Cassel 
> 
> Define the structures and constants required to implement
> Namespace Types support.
> 
> Namespace Types introduce a new command set, "I/O Command Sets",
> that allows the host to retrieve the command sets associated with
> a namespace. Introduce support for the command set and enable
> detection for the NVM Command Set.
> 
> The new workflows for identify commands rely heavily on zero-filled
> identify structs. E.g., certain CNS commands are defined to return
> a zero-filled identify struct when an inactive namespace NSID
> is supplied.
> 
> Add a helper function in order to avoid code duplication when
> reporting zero-filled identify structures.
> 
> Signed-off-by: Niklas Cassel 
> Signed-off-by: Dmitry Fomichev 
> ---
>  hw/block/nvme-ns.c|   2 +
>  hw/block/nvme-ns.h|   1 +
>  hw/block/nvme.c   | 169 +++---
>  hw/block/trace-events |   7 ++
>  include/block/nvme.h  |  65 
>  5 files changed, 202 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> index de735eb9f3..c0362426cc 100644
> --- a/hw/block/nvme-ns.c
> +++ b/hw/block/nvme-ns.c
> @@ -41,6 +41,8 @@ static void nvme_ns_init(NvmeNamespace *ns)
>  
>  id_ns->nsze = cpu_to_le64(nvme_ns_nlbas(ns));
>  
> +ns->csi = NVME_CSI_NVM;
> +
>  /* no thin provisioning */
>  id_ns->ncap = id_ns->nsze;
>  id_ns->nuse = id_ns->ncap;
> diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
> index a38071884a..d795e44bab 100644
> --- a/hw/block/nvme-ns.h
> +++ b/hw/block/nvme-ns.h
> @@ -31,6 +31,7 @@ typedef struct NvmeNamespace {
>  int64_t  size;
>  NvmeIdNs id_ns;
>  const uint32_t *iocs;
> +uint8_t  csi;
>  
>  NvmeNamespaceParams params;
>  } NvmeNamespace;
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 29139d8a17..ca0d0abf5c 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1503,6 +1503,13 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, 
> NvmeRequest *req)
>  return NVME_SUCCESS;
>  }
>  
> +static uint16_t nvme_rpt_empty_id_struct(NvmeCtrl *n, NvmeRequest *req)
> +{
> +uint8_t id[NVME_IDENTIFY_DATA_SIZE] = {};

[-pedantic] empty initializer list

> +
> +return nvme_dma(n, id, sizeof(id), DMA_DIRECTION_FROM_DEVICE, req);
> +}
> +
>  static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeRequest *req)
>  {
>  trace_pci_nvme_identify_ctrl();
> @@ -1511,11 +1518,23 @@ static uint16_t nvme_identify_ctrl(NvmeCtrl *n, 
> NvmeRequest *req)
>  DMA_DIRECTION_FROM_DEVICE, req);
>  }
>  
> +static uint16_t nvme_identify_ctrl_csi(NvmeCtrl *n, NvmeRequest *req)
> +{
> +NvmeIdentify *c = (NvmeIdentify *)>cmd;
> +
> +trace_pci_nvme_identify_ctrl_csi(c->csi);
> +
> +if (c->csi == NVME_CSI_NVM) {
> +return nvme_rpt_empty_id_struct(n, req);
> +}
> +
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
>  static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req)
>  {
>  NvmeNamespace *ns;
>  NvmeIdentify *c = (NvmeIdentify *)>cmd;
> -NvmeIdNs *id_ns, inactive = { 0 };
>  uint32_t nsid = le32_to_cpu(c->nsid);
>  
>  trace_pci_nvme_identify_ns(nsid);
> @@ -1526,23 +1545,46 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, 
> NvmeRequest *req)
>  
>  ns = nvme_ns(n, nsid);
>  if (unlikely(!ns)) {
> -id_ns = 
> -} else {
> -id_ns = >id_ns;
> +return nvme_rpt_empty_id_struct(n, req);
>  }
>  
> -return nvme_dma(n, (uint8_t *)id_ns, sizeof(NvmeIdNs),
> +return nvme_dma(n, (uint8_t *)>id_ns, sizeof(NvmeIdNs),
>  DMA_DIRECTION_FROM_DEVICE, req);
>  }
>  
> +static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req)
> +{
> +NvmeNamespace *ns;
> +NvmeIdentify *c = (NvmeIdentify *)>cmd;
> +uint32_t nsid = le32_to_cpu(c->nsid);
> +
> +trace_pci_nvme_identify_ns_csi(nsid, c->csi);
> +
> +if (!nvme_nsid_valid(n, nsid) || nsid == NVME_NSID_BROADCAST) {
> +return NVME_INVALID_NSID | NVME_DNR;
> +}
> +
> +ns = nvme_ns(n, nsid);
> +if (unlikely(!ns)) {
> +return nvme_rpt_empty_id_struct(n, req);
> +}
> +
> +if (c->csi == NVME_CSI_NVM) {
> +return nvme_rpt_empty_id_struct(n, req);
> +}
> +
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
>  static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
>  {
> +NvmeNamespace *ns;
>  NvmeIdentify *c = (NvmeIdentify *)>cmd;
> -static const int data_len = NVME_IDENTIFY_DATA_SIZE;
>  uint32_t min_nsid = le32_to_cpu(c->nsid);
> -uint32_t *list;
> -uint16_t ret;
> -int j = 0;
> +uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};

[-pedantic] empty initializer list

> +static const int data_len = sizeof(list);
> +uint32_t *list_ptr = (uint32_t *)list;
> +int i, j = 0;
>  
>  trace_pci_nvme_identify_nslist(min_nsid);
>  
> @@ -1556,20 

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

[Cleber Rosa]

On Mon, Oct 19, 2020 at 12:26:10PM +0200, Erik Skultety wrote:
> On Sun, Oct 18, 2020 at 09:50:02PM -0400, Cleber Rosa wrote:
> > +
> > +- name: Checks the availability of official gitlab-runner builds in 
> > the archive
> > +  uri:
> > +url: https://s3.amazonaws.com/gitlab-runner-downloads/v{{ 
> > gitlab_runner_version  }}/binaries/gitlab-runner-linux-386
> > +method: HEAD
> > +status_code:
> > +  - 200
> > +  - 403
> 
> Why is 403 an acceptable success status code?
>

I missed this question in my last reply, sorry.

s3 will throw a 403 if the URI doesn't exist... and we don't want to fail
the playbook because of that, given that we'll attempt the fallback
repo defined in vars.yml{,.template}.

> > +  register: gitlab_runner_available_archive
> > +
> > +- name: Update base url
> > +  set_fact:
> > +gitlab_runner_base_url: 
> > https://s3.amazonaws.com/gitlab-runner-downloads/v{{ gitlab_runner_version  
> > }}/binaries/gitlab-runner-
> > +  when: gitlab_runner_available_archive.status == 200
> > +- debug:
> > +msg: Base gitlab-runner url is {{ gitlab_runner_base_url  }}
> > +
> > +- name: Set OS name (FreeBSD)
> > +  set_fact:
> > +gitlab_runner_os: freebsd
> > +  when: "ansible_facts['system'] == 'FreeBSD'"
> > +
> > +- name: Create a group for the gitlab-runner service
> > +  group:
> > +name: gitlab-runner
> > +
> > +- name: Create a user for the gitlab-runner service
> > +  user:
> > +user: gitlab-runner
> > +group: gitlab-runner
> > +comment: GitLab Runner
> > +home: /home/gitlab-runner
> > +shell: /bin/bash
> > +
> > +- name: Remove the .bash_logout file when on Ubuntu systems
> > +  file:
> > +path: /home/gitlab-runner/.bash_logout
> > +state: absent
> > +  when: "ansible_facts['distribution'] == 'Ubuntu'"
> > +
> > +- name: Downloads the matching gitlab-runner
> > +  get_url:
> > +dest: /usr/local/bin/gitlab-runner
> > +url: "{{ gitlab_runner_base_url }}{{ gitlab_runner_os }}-{{ 
> > gitlab_runner_arch }}"
> > +owner: gitlab-runner
> > +group: gitlab-runner
> > +mode: u=rwx,g=rwx,o=rx
> > +

Here, the actual download is performed, and no 40x is considered OK.
I hope that explains it.

Cheers,
- Cleber.


signature.asc
Description: PGP signature

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

[Cleber Rosa]

On Mon, Oct 19, 2020 at 12:26:10PM +0200, Erik Skultety wrote:
> On Sun, Oct 18, 2020 at 09:50:02PM -0400, Cleber Rosa wrote:
> > To have the jobs dispatched to custom runners, gitlab-runner must
> > be installed, active as a service and properly configured.  The
> > variables file and playbook introduced here should help with those
> > steps.
> > 
> > The playbook introduced here covers a number of different Linux
> > distributions and FreeBSD, and are intended to provide a reproducible
> > environment.
> > 
> > Signed-off-by: Cleber Rosa 
> > Reviewed-by: Daniel P. Berrangé 
> > ---
> 
> In general, there's been put quite some effort into the playbooks - sorry I'm
> late to the game - is there a plan to introduce QEMU as a project to lcitool?

I think it's becoming quite clear that having so much duplication (in
the dockerfiles, tests/vm, this playbook, etc) is costly and error
prone.  I don't know if anyone has invested time in a PoC to
consolidate those (with lcitool), but I can certainly see the upside
to that.  BTW, are you volunteering (wink wink)? :)

> We've taken care of most of the bits in the playbooks that are being 
> introduced
> and for the remaining ones I think it would be that big of an overhaul to do
> the adjustments. One major re-factor though would IMO be to break the
> dependency lcitool has on the machine naming, kind of restricting it to a
> limited set of hosts and corresponding names (e.g. libvirt-fedora-32) which
> makes it inconvenient to prepare physical hosts.
>

Right... I wasn't aware of that depedency.  And, this may be a nice
project to make sure that lcitool doesn't have any other libvirt
specificities.

> More comments inline...
> 
> >  docs/devel/ci.rst  | 63 ++
> >  scripts/ci/setup/.gitignore|  1 +
> >  scripts/ci/setup/gitlab-runner.yml | 72 ++
> >  scripts/ci/setup/vars.yml.template | 13 ++
> >  4 files changed, 149 insertions(+)
> >  create mode 100644 scripts/ci/setup/.gitignore
> >  create mode 100644 scripts/ci/setup/gitlab-runner.yml
> >  create mode 100644 scripts/ci/setup/vars.yml.template
> > 
> > diff --git a/docs/devel/ci.rst b/docs/devel/ci.rst
> > index 208b5e399b..a234a5e24c 100644
> > --- a/docs/devel/ci.rst
> > +++ b/docs/devel/ci.rst
> > @@ -84,3 +84,66 @@ To run the playbook, execute::
> >  
> >cd scripts/ci/setup
> >ansible-playbook -i inventory build-environment.yml
> > +
> > +gitlab-runner setup and registration
> > +
> > +
> > +The gitlab-runner agent needs to be installed on each machine that
> > +will run jobs.  The association between a machine and a GitLab project
> > +happens with a registration token.  To find the registration token for
> > +your repository/project, navigate on GitLab's web UI to:
> > +
> > + * Settings (the gears like icon), then
> > + * CI/CD, then
> > + * Runners, and click on the "Expand" button, then
> > + * Under "Set up a specific Runner manually", look for the value under
> > +   "Use the following registration token during setup"
> > +
> > +Copy the ``scripts/ci/setup/vars.yml.template`` file to
> > +``scripts/ci/setup/vars.yml``.  Then, set the
> > +``gitlab_runner_registration_token`` variable to the value obtained
> > +earlier.
> > +
> > +.. note:: gitlab-runner is not available from the standard location
> > +  for all OS and architectures combinations.  For some systems,
> > +  a custom build may be necessary.  Some builds are avaiable
> > +  at https://cleber.fedorapeople.org/gitlab-runner/ and this
> > +  URI may be used as a value on ``vars.yml``
> 
> Yes, this can be suboptimal...Would it make sense to fall back to build the
> binary of a given version from git as a fallback during this playbook if the
> necessary arch version isn't provided the official way? Just an idea, I'd like
> to avoid the need for you to become the maintainer of the binaries and keep up
> with the releases.
>

Well, building them during the playbook would be a lot more
complex... You can have your own repo with your own builds, and just
tweak your vars.yml.

> > +
> > +To run the playbook, execute::
> > +
> > +  cd scripts/ci/setup
> > +  ansible-playbook -i inventory gitlab-runner.yml
> > +
> > +.. note:: there are currently limitations to gitlab-runner itself when
> > +  setting up a service under FreeBSD systems.  You will need to
> > +  perform steps 4 to 10 manually, as described at
> 
> Which one of them is considered an automation problem? In lcitool we made
> gitlab-runner completely automated on all distros, including FreeBSD:
>

It looks like lcitool went the more practical route.  I was hoping to
not have to treat gitlab-runner in such a special way in any
"supported" OS.  What I mean is, I'd rather write the code within
gitlab-runner (or reespective libraries).  Of course, I did not get to
it, so that's why I just documented the steps here.

I'll 

Re: [PATCH] virtio-gpu: fix incorrect print type

[Eric Blake]

On 10/19/20 9:23 AM, Zhengui li wrote:

The type of input variable is unsigned int
while the printer type is int. So fix incorrect print type.

Signed-off-by: Zhengui li 
---
  hw/display/virtio-gpu.c | 32 
  1 file changed, 16 insertions(+), 16 deletions(-)


This looks like an updated version of an earlier patch.  If so, please 
remember to send with -v2 in the subject line and a summary of the 
differences after the --- comment (even if the differences are just an 
enhanced commit message), to save on reviewer's time.


As an example, 'git send-email -v2 -5' can send a series of 5 patches 
all with -v2 in the subject line, and include a 0/5 cover letter if you 
have git configured correctly.


More patch submission hints at:
https://wiki.qemu.org/Contribute/SubmitAPatch

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

Re: [PATCH 6/30] semihosting: Fix Lesser GPL version number

[Eric Blake]

On 10/16/20 9:42 AM, Chetan Pant wrote:

There is no "version 2" of the "Lesser" General Public License.


Meta-comment: Looking in the archives, I see some odd threading going on:

https://lists.gnu.org/archive/html/qemu-devel/2020-10/threads.html#04020

You sent 1/30 and 2/30 as independent threads, then deeply threaded 
3-8/30, but I don't see a 0/30 cover letter, nor 9-30/30.


When you use deep threading, it gets awkward to read replies in mailers 
that group by thread depth before date:


+ 1/30
 \+ re: 1/30
+ 2/30
 \+ re: 1/30
+ 3/30
| + 4/30
| | + 5/30
| | | + 6/30
| | | | + 7/30
| | | | | + 8/30
| | | | |  \+ re: 8/30
| | | |  \+ re: 7/30
| | |  \+ re: 6/30
| |  \+ re: 5/30
|  \+ re: 4/30
 \+ re: 3/30

instead of the more typical

+ 0/30
| + 1/30
|  \+ re: 1/30
| + 2/30
|  \+ re: 2/30
| + 3/30
|  \+ re: 3/30
| + 4/30
|  \+ re: 4/30
...
| + 30/30
|  \+ re: 30/30
 \+ re: 0/30

Figuring out how to make your mailer thread properly will make it easier 
to interact with your future patches.


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

Re: [PATCH v4 2/4] Jobs based on custom runners: build environment docs and playbook

[Cleber Rosa]

On Mon, Oct 19, 2020 at 12:27:41PM +0200, Erik Skultety wrote:
> ...
> 
> > diff --git a/scripts/ci/setup/inventory b/scripts/ci/setup/inventory
> > new file mode 100644
> > index 00..8bb7ba6b33
> > --- /dev/null
> > +++ b/scripts/ci/setup/inventory
> > @@ -0,0 +1,2 @@
> > +[local]
> 
> Nit pick, is a group for localhost actually needed?
>

You're right, it's not needed... I just thought it gave the
"localhost" entry some "shelter" and "context". :)

And, I think a mostly "ini-like" file without a section triggers an OCD
reaction in me.  I can remove it if it does something similar to you! :)

Thanks!
- Cleber.

> Regards,
> Erik


signature.asc
Description: PGP signature