Re: [PATCH v5 3/3] ppc: Enable 2nd DAWR support on p10

[David Gibson]

On Wed, Apr 21, 2021 at 11:50:40AM +0530, Ravi Bangoria wrote:
> Hi David,
> 
> On 4/19/21 10:23 AM, David Gibson wrote:
> > On Mon, Apr 12, 2021 at 05:14:33PM +0530, Ravi Bangoria wrote:
> > > As per the PAPR, bit 0 of byte 64 in pa-features property indicates
> > > availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
> > > DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
> > > find whether kvm supports 2nd DAWR or not. If it's supported, allow
> > > user to set the pa-feature bit in guest DT using cap-dawr1 machine
> > > capability. Though, watchpoint on powerpc TCG guest is not supported
> > > and thus 2nd DAWR is not enabled for TCG mode.
> > > 
> > > Signed-off-by: Ravi Bangoria 
> > > Reviewed-by: Greg Kurz 
> > 
> > So, I'm actually not sure if using an spapr capability is what we want
> > to do here.  The problem is that presumably the idea is to at some
> > point make the DAWR1 capability default to on (on POWER10, at least).
> > But at that point you'll no longer to be able to start TCG guests
> > without explicitly disabling it.  That's technically correct, since we
> > don't implement DAWR1 in TCG, but then we also don't implement DAWR0
> > and we let that slide... which I think is probably going to cause less
> > irritation on balance.
> 
> Ok. Probably something like this is what you want?
> 
> Power10 behavior:
>   - KVM does not support DAWR1: Boot the guest without DAWR1
> support (No warnings). Error out only if user tries with
> cap-dawr1=on.
>   - KVM supports DAWR1: Boot the guest with DAWR1 support, unless
> user specifies cap-dawr1=off.
>   - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
> DAWR0 (Should be fixed in future while adding PowerPC watch-
> point support in TCG mode)
> 
> Power10 predecessor behavior:
>   - KVM guest: Boot the guest without DAWR1 support. Error out
> if user tries with cap-dawr1=on.
>   - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
> DAWR0 (Should be fixed in future while adding PowerPC watch-
> point support in TCG mode)

Sorry I've neglected this thread so long.  I'm afraid the logic above
won't work.  As a general rule we never want to change guest-visible
details of the platform based on KVM capabilities, because it makes a
total mess of migration across clusters.

So, if we'd introduced this along with initial POWER10 support, I
don't think we'd need a capability at all: we just present DAWR1 on
POWER10, don't otherwise.  The fact it doesn't work in TCG we just
treat as a TCG bug we'll probably never get around to fixing, just
like TCG not supporting DAWR0 is a TCG bug right now.

Since we have released versions with POWER10 support, but no DAWR1, in
theory we need a capability so new qemu with old machine types don't
gain guest visible features that the same machine types on older qemus
had.

Except.. there's a loophole we might use to sidestep that.  The
current POWER10 CPU modelled in qemu is a DD1 - which I strongly
suspect will never appear outside of IBM.  I'm pretty sure we want to
replace that with a DD2.

While the modelled CPU is DD1, I think it's pretty reasonable to say
our POWER10 support hasn't yet stabilized, and it would therefore be
ok to simply add DAWR1 on POWER10 unconditionally, as long as we do it
before we switch over to DD2.

> > I'm wondering if we're actually just better off setting the pa feature
> > just based on the guest CPU model.  TCG will be broken if you try to
> > use it, but then, it already is.  AFAIK there's no inherent reason we
> > couldn't implement DAWR support in TCG, it's just never been worth the
> > trouble.
> 
> Correct. Probably there is no practical usecase for DAWR in TCG mode.
> 
> Thanks,
> Ravi
> 

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


signature.asc
Description: PGP signature

Re: [PATCH] vfio/pci: Revert nvlink removal uAPI breakage

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig 

[Bug 1905356] Re: No check for unaligned data access in ARM32 instructions

[Thomas Huth]

Richard's patches have been merged (see
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=4d753eb5fb03ee7bc71ec
and the following ones), so I'm setting the state to "Fix committed"
now.

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

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

Title:
  No check for unaligned data access in ARM32 instructions

Status in QEMU:
  Fix Committed

Bug description:
  hi

  According to the ARM documentation, there are alignment requirements
  of load/store instructions.  Alignment fault should be raised if the
  alignment check is failed. However, it seems that QEMU doesn't
  implement this, which is against the documentation of ARM. For
  example, the instruction LDRD/STRD/LDREX/STREX must check the address
  is word alignment no matter what value the SCTLR.A is.

  I attached a testcase, which contains an instruction at VA 0x10240:
  ldrd r0,[pc.#1] in the main function. QEMU can successfully load the
  data in the unaligned address. The test is done in QEMU 5.1.0. I can
  provide more testcases for the other instructions if you need. Many
  thanks.

  To patch this, we need a check while we translate the instruction to
  tcg. If the address is unaligned, a signal number (i.e., SIGBUS)
  should be raised.

  Regards
  Muhui

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

[Bug 1868116] Re: QEMU monitor no longer works

[Christian Ehrhardt ]

@Thomas - there is a leftover task here and I've filed [1] for it in the new 
tracker.
What is the right state to move this bug here into now?

[1]: https://gitlab.com/qemu-project/qemu/-/issues/137

** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #137
   https://gitlab.com/qemu-project/qemu/-/issues/137

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

Title:
  QEMU monitor no longer works

Status in QEMU:
  Incomplete
Status in qemu package in Ubuntu:
  Invalid
Status in vte2.91 package in Ubuntu:
  Fix Released
Status in qemu package in Debian:
  Fix Released

Bug description:
  Repro:
  VTE
  $ meson _build && ninja -C _build && ninja -C _build install

  qemu:
  $ ../configure --python=/usr/bin/python3 --disable-werror --disable-user 
--disable-linux-user --disable-docs --disable-guest-agent --disable-sdl 
--enable-gtk --disable-vnc --disable-xen --disable-brlapi --disable-fdt 
--disable-hax --disable-vde --disable-netmap --disable-rbd --disable-libiscsi 
--disable-libnfs --disable-smartcard --disable-libusb --disable-usb-redir 
--disable-seccomp --disable-glusterfs --disable-tpm --disable-numa 
--disable-opengl --disable-virglrenderer --disable-xfsctl --disable-vxhs 
--disable-slirp --disable-blobs --target-list=x86_64-softmmu --disable-rdma 
--disable-pvrdma --disable-attr --disable-vhost-net --disable-vhost-vsock 
--disable-vhost-scsi --disable-vhost-crypto --disable-vhost-user 
--disable-spice --disable-qom-cast-debug --disable-vxhs --disable-bochs 
--disable-cloop --disable-dmg --disable-qcow1 --disable-vdi --disable-vvfat 
--disable-qed --disable-parallels --disable-sheepdog --disable-avx2 
--disable-nettle --disable-gnutls --disable-capstone --disable-tools 
--disable-libpmem --disable-iconv --disable-cap-ng
  $ make

  Test:
  $ LD_LIBRARY_PATH=/usr/local/lib/x86_64-linux-gnu/:$LD_LIBRARY_PATH 
./build/x86_64-softmmu/qemu-system-x86_64 -enable-kvm --drive 
media=cdrom,file=http://archive.ubuntu.com/ubuntu/dists/bionic/main/installer-amd64/current/images/netboot/mini.iso
  - switch to monitor with CTRL+ALT+2
  - try to enter something

  Affects head of both usptream git repos.

  
  --- original bug ---

  It was observed that the QEMU console (normally accessible using
  Ctrl+Alt+2) accepts no input, so it can't be used. This is being
  problematic because there are cases where it's required to send
  commands to the guest, or key combinations that the host would grab
  (as Ctrl-Alt-F1 or Alt-F4).

  ProblemType: Bug
  DistroRelease: Ubuntu 20.04
  Package: qemu 1:4.2-3ubuntu2
  Uname: Linux 5.6.0-rc6+ x86_64
  ApportVersion: 2.20.11-0ubuntu20
  Architecture: amd64
  CurrentDesktop: XFCE
  Date: Thu Mar 19 12:16:31 2020
  Dependencies:

  InstallationDate: Installed on 2017-06-13 (1009 days ago)
  InstallationMedia: Xubuntu 17.04 "Zesty Zapus" - Release amd64 (20170412)
  KvmCmdLine:
   COMMAND STAT  EUID  RUID PIDPPID %CPU COMMAND
   qemu-system-x86 Sl+   1000  1000   34275   25235 29.2 qemu-system-x86_64 -m 
4G -cpu Skylake-Client -device virtio-vga,virgl=true,xres=1280,yres=720 -accel 
kvm -device nec-usb-xhci -serial vc -serial stdio -hda 
/home/usuario/Sistemas/androidx86.img -display gtk,gl=on -device usb-audio
   kvm-nx-lpage-re S0 0   34284   2  0.0 [kvm-nx-lpage-re]
   kvm-pit/34275   S0 0   34286   2  0.0 [kvm-pit/34275]
  MachineType: LENOVO 80UG
  ProcKernelCmdLine: BOOT_IMAGE=/boot/vmlinuz-5.6.0-rc6+ 
root=UUID=6b4ae5c0-c78c-49a6-a1ba-029192618a7a ro quiet ro kvm.ignore_msrs=1 
kvm.report_ignored_msrs=0 kvm.halt_poll_ns=0 kvm.halt_poll_ns_grow=0 
i915.enable_gvt=1 i915.fastboot=1 cgroup_enable=memory swapaccount=1 
zswap.enabled=1 zswap.zpool=z3fold 
resume=UUID=a82e38a0-8d20-49dd-9cbd-de7216b589fc log_buf_len=16M 
usbhid.quirks=0x0079:0x0006:0x10 config_scsi_mq_default=y 
scsi_mod.use_blk_mq=1 mtrr_gran_size=64M mtrr_chunk_size=64M nbd.nbds_max=2 
nbd.max_part=63
  SourcePackage: qemu
  UpgradeStatus: Upgraded to focal on 2019-12-22 (87 days ago)
  dmi.bios.date: 08/09/2018
  dmi.bios.vendor: LENOVO
  dmi.bios.version: 0XCN45WW
  dmi.board.asset.tag: NO Asset Tag
  dmi.board.name: Toronto 4A2
  dmi.board.vendor: LENOVO
  dmi.board.version: SDK0J40679 WIN
  dmi.chassis.asset.tag: NO Asset Tag
  dmi.chassis.type: 10
  dmi.chassis.vendor: LENOVO
  dmi.chassis.version: Lenovo ideapad 310-14ISK
  dmi.modalias: 
dmi:bvnLENOVO:bvr0XCN45WW:bd08/09/2018:svnLENOVO:pn80UG:pvrLenovoideapad310-14ISK:rvnLENOVO:rnToronto4A2:rvrSDK0J40679WIN:cvnLENOVO:ct10:cvrLenovoideapad310-14ISK:
  dmi.product.family: IDEAPAD
  dmi.product.name: 80UG
  dmi.product.sku: LENOVO_MT_80UG_BU_idea_FM_Lenovo ideapad 310-14ISK
  dmi.product.version: Lenovo ideapad 310-14ISK
  dmi.sys.vendor: LENOVO
  mtime.conffile..etc.apport.crashdb.conf: 2019-08-29T08:39:36.787240

To manage notifications about this bug go to:

[Bug 1759522] Re: windows qemu-img create vpc/vhdx error

[Thomas Huth]

This is an automated cleanup. This bug report has been moved to QEMU's
new bug tracker on gitlab.com and thus gets marked as 'expired' now.
Please continue with the discussion here:

 https://gitlab.com/qemu-project/qemu/-/issues/136


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

** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #136
   https://gitlab.com/qemu-project/qemu/-/issues/136

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

Title:
  windows qemu-img create vpc/vhdx error

Status in QEMU:
  Expired

Bug description:
  On windows, using qemu-img (version 2.11.90) to create vpc/vhdx
  virtual disk tends to fail. Here's the way to reproduce:

  1. Install qemu-w64-setup-20180321.exe

  2. Use `qemu-img create -f vhdx -o subformat=fixed disk.vhdx 512M` to create 
a vhdx:
 Formatting 'disk.vhdx', fmt=vhdx size=536870912 log_size=1048576 
block_size=0 subformat=fixed

  3. Execute `qemu-img info disk.vhdx` gives the result, (note the `disk size` 
is incorrect):
 image: disk.vhdx
 file format: vhdx
 virtual size: 512M (536870912 bytes)
 disk size: 1.4M
 cluster_size: 8388608

  4. On Windows 10 (V1709), double click disk.vhdx gives an error:
 Make sure the file is in an NTFS volume and isn't in a compressed folder 
or volume.

 Using Disk Management -> Action -> Attach VHD gives an error:
 The requested operation could not be completed due to a virtual disk 
system limitation. Virtual hard disk files must be uncompressed and uneccrypted 
and must not be sparse.

  Comparison with Windows 10 created VHDX:

  1. Using Disk Management -> Action -> Create VHD:
 File name: win.vhdx
 Virtual hard disk size: 512MB
 Virtual hard disk format: VHDX
 Virtual hard disk type: Fixed size

  2. Detach VHDX

  3. Execute `qemu-img info win.vhdx` gives the result:
 image: win.vhdx
 file format: vhdx
 virtual size: 512M (536870912 bytes)
 disk size: 516M
 cluster_size: 33554432

  Comparison with qemu-img under Ubuntu:

  1. Version: qemu-img version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.16),
  Copyright (c) 2004-2008 Fabrice Bellard

  2. qemu-img create -f vhdx -o subformat=fixed lin.vhdx 512M
 Formatting 'lin.vhdx', fmt=vhdx size=536870912 log_size=1048576 
block_size=0 subformat=fixed

  3. qemu-img info lin.vhdx
 image: lin.vhdx
 file format: vhdx
 virtual size: 512M (536870912 bytes)
 disk size: 520M
 cluster_size: 8388608

  4. Load lin.vhdx under Windows 10 is ok

  The same thing happens on `vpc` format with or without
  `oformat=fixed`, it seems that windows version of qemu-img has some
  incorrect operation? My guess is that windows version of qemu-img
  doesn't handle the description field of vpc/vhdx, which leads to an
  incorrect `disk size` field.

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

[PATCH 0/7] vhost-user-gpu: fix several security issues

[Li Qiang]

These security issue is low severity and is similar with the
virtio-vga/virtio-gpu device. All of them can be triggered by
the guest user.

Li Qiang (7):
  vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
  vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
  vhost-user-gpu: fix memory leak in vg_resource_attach_backing
  vhost-user-gpu: fix memory link while calling 'vg_resource_unref'
  vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
  vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
  vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'

 contrib/vhost-user-gpu/vhost-user-gpu.c |  7 +++
 contrib/vhost-user-gpu/virgl.c  | 17 -
 2 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH 4/7] vhost-user-gpu: fix memory link while calling 'vg_resource_unref'

[Li Qiang]

If the guest trigger following sequences, the attach_backing will be leaked:

vg_resource_create_2d
vg_resource_attach_backing
vg_resource_unref

This patch fix this by freeing 'res->iov' in vg_resource_destroy.

Signed-off-by: Li Qiang 
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c 
b/contrib/vhost-user-gpu/vhost-user-gpu.c
index 0437e52b64..770dfad529 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -400,6 +400,7 @@ vg_resource_destroy(VuGpu *g,
 }
 
 vugbm_buffer_destroy(>buffer);
+g_free(res->iov);
 pixman_image_unref(res->image);
 QTAILQ_REMOVE(>reslist, res, next);
 g_free(res);
-- 
2.25.1

[PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'

[Li Qiang]

If 'virgl_renderer_resource_attach_iov' failed, the 'res_iovs' will
be leaked.

Signed-off-by: Li Qiang 
---
 contrib/vhost-user-gpu/virgl.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index c669d73a1d..a16a311d80 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
 return;
 }
 
-virgl_renderer_resource_attach_iov(att_rb.resource_id,
+ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
res_iovs, att_rb.nr_entries);
+if (ret != 0) {
+g_free(res_iovs);
+}
 }
 
 static void
-- 
2.25.1

[PATCH 7/7] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'

[Li Qiang]

If 'virgl_cmd_get_capset' set 'max_size' to 0,
the 'virgl_renderer_fill_caps' will write the data after the 'resp'.
This patch avoid this by checking the returned 'max_size'.

Signed-off-by: Li Qiang 
---
 contrib/vhost-user-gpu/virgl.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index a16a311d80..7172104b19 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -177,6 +177,10 @@ virgl_cmd_get_capset(VuGpu *g,
 
 virgl_renderer_get_cap_set(gc.capset_id, _ver,
_size);
+if (!max_size) {
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+return;
+}
 resp = g_malloc0(sizeof(*resp) + max_size);
 
 resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET;
-- 
2.25.1

[PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing

[Li Qiang]

Check whether the 'res' has already been attach_backing to avoid
memory leak.

Signed-off-by: Li Qiang 
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c 
b/contrib/vhost-user-gpu/vhost-user-gpu.c
index b5e153d0d6..0437e52b64 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -489,6 +489,11 @@ vg_resource_attach_backing(VuGpu *g,
 return;
 }
 
+if (res->iov) {
+cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+return;
+}
+
 ret = vg_create_mapping_iov(g, , cmd, >iov);
 if (ret != 0) {
 cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-- 
2.25.1

[PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'

[Li Qiang]

The 'res->iov' will be leaked if the guest trigger following sequences:

virgl_cmd_create_resource_2d
virgl_resource_attach_backing
virgl_cmd_resource_unref

This patch fixes this.

Signed-off-by: Li Qiang 
---
 contrib/vhost-user-gpu/virgl.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index 6a332d601f..c669d73a1d 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -108,9 +108,16 @@ virgl_cmd_resource_unref(VuGpu *g,
  struct virtio_gpu_ctrl_command *cmd)
 {
 struct virtio_gpu_resource_unref unref;
+struct iovec *res_iovs = NULL;
+int num_iovs = 0;
 
 VUGPU_FILL_CMD(unref);
 
+virgl_renderer_resource_detach_iov(unref.resource_id,
+   _iovs,
+   _iovs);
+g_free(res_iovs);
+
 virgl_renderer_resource_unref(unref.resource_id);
 }
 
-- 
2.25.1

[PATCH 2/7] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'

[Li Qiang]

Call 'vugbm_buffer_destroy' in error path to avoid resource leak.

Signed-off-by: Li Qiang 
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c 
b/contrib/vhost-user-gpu/vhost-user-gpu.c
index f73f292c9f..b5e153d0d6 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -349,6 +349,7 @@ vg_resource_create_2d(VuGpu *g,
 g_critical("%s: resource creation failed %d %d %d",
__func__, c2d.resource_id, c2d.width, c2d.height);
 g_free(res);
+vugbm_buffer_destroy(>buffer);
 cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
 return;
 }
-- 
2.25.1

[PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info

[Li Qiang]

Otherwise some of the 'resp' will be leaked to guest.

Signed-off-by: Li Qiang 
---
 contrib/vhost-user-gpu/virgl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index 9e6660c7ab..6a332d601f 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -128,6 +128,7 @@ virgl_cmd_get_capset_info(VuGpu *g,
 
 VUGPU_FILL_CMD(info);
 
+memset(, 0, sizeof(resp));
 if (info.capset_index == 0) {
 resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
 virgl_renderer_get_cap_set(resp.capset_id,
-- 
2.25.1

Re: [PATCH] target/ppc: Do not check for LPCR[HAIL] on power10_v1.0 CPUs

[David Gibson]

On Tue, May 04, 2021 at 01:54:39PM +0200, Cédric Le Goater wrote:
> On 5/4/21 12:49 PM, Nicholas Piggin wrote:
> > Excerpts from Cédric Le Goater's message of May 4, 2021 7:59 pm:
> >> The LPCR[HAIL] bit only applies to POWER10 DD2 CPUs. On POWER10 DD1,
> >> the ail value should be extracted using the LPCR_AIL mask like on P9.
> >>
> >> Cc: Nicholas Piggin 
> >> Signed-off-by: Cédric Le Goater 
> > 
> > Thanks for this, my oversight for not realising the P10 CPU is DD1 
> > (which doesn't have HAIL).
> > 
> > I wonder if it could just use the POWER9 excp_model?
> 
> Yes. Why not. It does bring up another problem which is how to define
> (cleanly) different characteristics for CPUs of the same POWER family.
> 
> Currently, all P10s are under POWERPC_FAMILY(POWER10). This is a base 
> abstract class and definitions can not depend on the PVR. See below
> what needs to be done to add a custom LPCR mask for DD2 :/
> 
> We could also simply switch P10 to DD2. I would favor that instead of 
> adding complexity.

Definitely.  I'm guessing DD1 POWER10s will never be seen outside IBM,
so I don't think we want support for them in upstream qemu at all.

> 
> David, what is your opinion on this ? 
> 
> Thank,
> 
> C. 
> 
> 
> Signed-off-by: Cédric Le Goater 
> ---
>  target/ppc/cpu-models.c |   13 +++--
>  target/ppc/cpu-models.h |1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> Index: qemu-powernv-6.1.git/target/ppc/cpu-models.c
> ===
> --- qemu-powernv-6.1.git.orig/target/ppc/cpu-models.c
> +++ qemu-powernv-6.1.git/target/ppc/cpu-models.c
> @@ -32,7 +32,7 @@
>  /* PowerPC CPU definitions */
>  #define POWERPC_DEF_PREFIX(pvr, svr, type)  \
>  glue(glue(glue(glue(pvr, _), svr), _), type)
> -#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type)\
> +#define __POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type, _lpcr)   \
>  static void \
>  glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init)\
>  (ObjectClass *oc, void *data)   \
> @@ -40,6 +40,7 @@
>  DeviceClass *dc = DEVICE_CLASS(oc); \
>  PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);   \
>  \
> +pcc->lpcr_mask|= _lpcr; \
>  pcc->pvr  = _pvr;   \
>  pcc->svr  = _svr;   \
>  dc->desc  = _desc;  \
> @@ -63,6 +64,12 @@
>  type_init(  \
>  glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_register_types))
>  
> +#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type)\
> +__POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type, 0)
> +
> +#define POWERPC_DEF_LPCR(_name, _pvr, _type, _desc, _lpcr)  \
> +__POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type, _lpcr)
> +
>  #define POWERPC_DEF(_name, _pvr, _type, _desc)  \
>  POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
>  
> @@ -776,6 +783,8 @@
>  "POWER9 v2.0")
>  POWERPC_DEF("power10_v1.0",  CPU_POWERPC_POWER10_DD1,POWER10,
>  "POWER10 v1.0")
> +POWERPC_DEF_LPCR("power10_v2.0",  CPU_POWERPC_POWER10_DD20,  POWER10,
> + "POWER10 v2.0", LPCR_HAIL)
>  #endif /* defined (TARGET_PPC64) */
>  
>  /***/
> @@ -952,7 +961,7 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>  { "power8", "power8_v2.0" },
>  { "power8nvl", "power8nvl_v1.0" },
>  { "power9", "power9_v2.0" },
> -{ "power10", "power10_v1.0" },
> +{ "power10", "power10_v2.0" },
>  #endif
>  
>  /* Generic PowerPCs */
> Index: qemu-powernv-6.1.git/target/ppc/cpu-models.h
> ===
> --- qemu-powernv-6.1.git.orig/target/ppc/cpu-models.h
> +++ qemu-powernv-6.1.git/target/ppc/cpu-models.h
> @@ -375,6 +375,7 @@ enum {
>  CPU_POWERPC_POWER9_DD20= 0x004E1200,
>  CPU_POWERPC_POWER10_BASE   = 0x0080,
>  CPU_POWERPC_POWER10_DD1= 0x00800100,
> +CPU_POWERPC_POWER10_DD20   = 0x00800200,
>  CPU_POWERPC_970_v22= 0x00390202,
>  CPU_POWERPC_970FX_v10  = 0x00391100,
>  CPU_POWERPC_970FX_v20  = 0x003C0200,
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  

Re: [RFC PATCH v2 0/2] hw/ppc: code motion to compile without TCG

[David Gibson]

On Mon, May 03, 2021 at 07:21:11PM -0300, Fabiano Rosas wrote:
> "Lucas Mateus Castro (alqotel)"  writes:
> 
> > After the feedback from v1 I reworked the patch with suggested ideas and
> > this version has less duplicated code and is overall simpler.
> >
> > This patch series is still a WIP, there are still 2 main problems I am
> > trying to solve, I'll mention them in their respective patches.
> >
> > The aim of these patches is to progress toward enabling disable-tcg on
> > PPC by solving errors in hw/ppc with that option.
> >
> > As a WIP comments are welcome.
> >
> > Lucas Mateus Castro (alqotel) (2):
> >   target/ppc: Moved functions out of mmu-hash64
> >   hw/ppc: Moved TCG code to spapr_hcall_tcg
> >
> >  hw/ppc/meson.build   |   3 +
> >  hw/ppc/spapr.c   |   1 +
> >  hw/ppc/spapr_caps.c  |   1 +
> >  hw/ppc/spapr_cpu_core.c  |   1 +
> >  hw/ppc/spapr_hcall.c | 301 ++
> >  hw/ppc/spapr_hcall_tcg.c | 343 +++
> >  hw/ppc/spapr_rtas.c  |   1 +
> >  target/ppc/meson.build   |   1 +
> >  target/ppc/mmu-hash64.c  |  81 +
> >  target/ppc/mmu-hash64.h  |   6 -
> >  target/ppc/mmu-misc.c|  86 ++
> >  target/ppc/mmu-misc.h|  22 +++
> >  12 files changed, 478 insertions(+), 369 deletions(-)
> >  create mode 100644 hw/ppc/spapr_hcall_tcg.c
> >  create mode 100644 target/ppc/mmu-misc.c
> >  create mode 100644 target/ppc/mmu-misc.h
> 
> This is the list of hypercalls registered with spapr_register_hypercall
> and whether they are implemented by KVM HV, KVM PR or none. I also list
> whether the KVM hcall uses the QEMU implementation as a fallback. Maybe
> it will be helpful to this discussion.
> 
> (This is from just looking at the code, so take it with a grain of salt)
> 
> H_ADD_LOGICAL_LAN_BUFFER  - not impl. by KVM
> H_CHANGE_LOGICAL_LAN_MAC  - not impl. by KVM
> H_ENABLE_CRQ  - not impl. by KVM
> H_FREE_CRQ- not impl. by KVM
> H_FREE_LOGICAL_LAN- not impl. by KVM
> H_GET_CPU_CHARACTERISTICS - not impl. by KVM
> H_GET_TERM_CHAR   - not impl. by KVM
> H_HOME_NODE_ASSOCIATIVITY - not impl. by KVM
> H_INT_ESB - not impl. by KVM
> H_INT_GET_QUEUE_INFO  - not impl. by KVM
> H_INT_GET_SOURCE_CONFIG   - not impl. by KVM
> H_INT_GET_SOURCE_INFO - not impl. by KVM
> H_INT_RESET   - not impl. by KVM
> H_INT_SET_QUEUE_CONFIG- not impl. by KVM
> H_INT_SET_SOURCE_CONFIG   - not impl. by KVM
> H_INT_SYNC- not impl. by KVM
> H_JOIN- not impl. by KVM
> H_LOGICAL_CACHE_LOAD  - not impl. by KVM
> H_LOGICAL_CACHE_STORE - not impl. by KVM
> H_LOGICAL_DCBF- not impl. by KVM
> H_LOGICAL_ICBI- not impl. by KVM
> H_MULTICAST_CTRL  - not impl. by KVM
> H_PUT_TERM_CHAR   - not impl. by KVM
> H_REGISTER_LOGICAL_LAN- not impl. by KVM
> H_REGISTER_PROC_TBL   - not impl. by KVM
> H_REG_CRQ - not impl. by KVM
> H_RESIZE_HPT_COMMIT   - not impl. by KVM
> H_RESIZE_HPT_PREPARE  - not impl. by KVM
> H_SCM_BIND_MEM- not impl. by KVM
> H_SCM_READ_METADATA   - not impl. by KVM
> H_SCM_UNBIND_ALL  - not impl. by KVM
> H_SCM_WRITE_METADATA  - not impl. by KVM
> H_SEND_CRQ- not impl. by KVM
> H_SEND_LOGICAL_LAN- not impl. by KVM
> H_SET_SPRG0   - not impl. by KVM
> H_SIGNAL_SYS_RESET- not impl. by KVM
> H_VIO_SIGNAL  - not impl. by KVM
> 
> H_CAS - not impl. by KVM | called by SLOF only
> H_LOGICAL_MEMOP   - not impl. by KVM | called by SLOF only
> H_TPM_COMM- not impl. by KVM | called by UV only
> H_UPDATE_DT   - not impl. by KVM | called by SLOF only
> 
> H_INT_GET_OS_REPORTING_LINE - not impl. by KVM | not called by linux/SLOF/UV
> H_INT_GET_QUEUE_CONFIG  - not impl. by KVM | not called by linux/SLOF/UV
> H_INT_SET_OS_REPORTING_LINE - not impl. by KVM | not called by linux/SLOF/UV
> H_SCM_UNBIND_MEM- not impl. by KVM | not called by linux/SLOF/UV
> 
> H_GET_TCE  - HV | not impl. by PR | QEMU fallback
> H_SET_MODE - HV | not impl. by PR | QEMU fallback
> H_CONFER   - HV | not impl. by PR
> H_PAGE_INIT- HV | not impl. by PR
> H_PROD - HV | not impl. by PR
> H_RANDOM   - HV | not impl. by PR
> H_READ - HV | not impl. by PR
> H_REGISTER_VPA - HV | not impl. by PR
> H_SET_DABR - HV | not impl. by PR
> H_SET_XDABR- HV | not impl. by PR
> 
> H_CPPR - HV | PR | QEMU fallback
> H_EOI  - HV | PR | QEMU fallback
> H_IPI  - HV | PR | QEMU fallback
> H_IPOLL- HV | PR | QEMU fallback
> H_LOGICAL_CI_LOAD  - HV | PR | QEMU fallback
> H_LOGICAL_CI_STORE - HV | PR | QEMU fallback
> H_PUT_TCE  - HV | PR | QEMU fallback
> H_PUT_TCE_INDIRECT - HV | PR | QEMU fallback
> H_RTAS - HV | PR | QEMU 

Re: [RFC PATCH v2 2/2] hw/ppc: Moved TCG code to spapr_hcall_tcg

[David Gibson]

On Tue, May 04, 2021 at 03:14:17PM -0300, Lucas Mateus Martins Araujo e Castro 
wrote:
> 
> On 03/05/2021 01:34, David Gibson wrote:
> > On Fri, Apr 30, 2021 at 03:40:47PM -0300, Lucas Mateus Castro (alqotel) 
> > wrote:
> > > Moved h_enter, remove_hpte, h_remove, h_bulk_remove,h_protect and
> > > h_read to spapr_hcall_tcg.c, added h_tcg_only to be used in a !TCG
> > > environment in spapr_hcall.c and changed build options to only build
> > > spapr_hcall_tcg.c when CONFIG_TCG is enabled.
> > This looks good.  I'd suggest the name 'spapr_softmmu.c' instead,
> > which more specifically describes what's special about these
> > functions.
> > 
> > h_resize_hpt_prepare(), h_resize_hpt_commit() and the functions they
> > depend on belong in the softmmu set as well.
> 
> Moved these hcalls to spapr_softmmu.c as well, along with the most
> functions they depend on, but 1 function (push_sregs_to_kvm_pr) is
> also used by hcalls in spapr_hcall.c, so for now I'm just leaving it in
> spapr_hcall.c and exporting it to be used in spapr_softmmu.c.

Right.  That one's an ugly workaround for some weird KVM PR behaviour,
and we will need it in the common code.

> On a related note, from what I've seen h_resize_hpt_prepare and
> h_resize_hpt_commit are not implementede in KVM, so they're only
> called when there's softmmu so that's why they can be moved to
> spapr_softmmu.c?

Ah, sorry, I forgot how these worked.  The bulk of the logic to
implement these *is* in KVM, but KVM doesn't directly intercept them
as hcalls.  Instead the hcall is caught by qemu, but it just forwards
them back to KVM by calling an ioctl().  See the calls to
kvmppc_resize_hpt_prepare() and kvmppc_resize_hpt_commit() in
spapr_hcall.c.

[Aside: the reason for this is that they're not latency critical, and
 it's useful for qemu to be aware of and possibly filter HPT resize
 requests, but because KVM manages the HPT, qemu can't implement these
 directly for the KVM case]

So what we'll need to do for those is keep the first part if
h_resize_hpt_{prepare,commit}() in common code - that's basically just
parameter validation - up to the call to
kvmppc_resize_hpt_{prepare,commit}().

if the kvmppc_() call fails with -ENOSYS then we need to do
if (kvm_enabled()) {
return H_HARDWARE;
}
softmmu_resize_hpt_{prepare,commit}()

Where that softmmu_..() function is the remainder of the logic that's
in the current implementation.  That bit can move to the softmmu file
and can be stubbed with an assert-not-reached for the !TCG case.

In practice we should never actually hit that H_HARDWARE - we should
have failed at machine setup if we tried to enable the resize HPT
feature on a KVM that doesn't implement it.

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


signature.asc
Description: PGP signature

Re: [PATCH] target/ppc/spapr: Update H_GET_CPU_CHARACTERISTICS bits

[David Gibson]

On Tue, May 04, 2021 at 06:50:54PM +1000, Nicholas Piggin wrote:
> Excerpts from David Gibson's message of May 4, 2021 10:41 am:
> > On Mon, May 03, 2021 at 10:58:33PM +1000, Nicholas Piggin wrote:
> >> There are several new bits added to the hcall which reflect new issues
> >> found and new hardware mitigations.
> >> 
> >> This adds the link stack flush behaviour, link stack flush accelerated
> >> instruction capability, and several L1D flush type behaviours (which are
> >> now being specified as negative in order to simplify patched kernel
> >> compatibility with older firmware).
> > 
> > So, to clarify here, the bits your adding aren't advertising any new
> > behaviour on qemu/KVM's part, they're just new ways of advertising the
> > same behaviour?
> 
> I... think so. "Behaviour" is in context of the hcall that advertises
> how the processor behaves (or what the guest must do for security).

Heh.  Don't get me started on how the difference between
"characteristics" and "behaviours" in the fields is totally
non-obvious.

> The new NO_ bits added are for processors that don't require a particular
> flush. The FLUSH_LINK_STACK was basically always required but I think
> Linux just keyed off the count cache flush and did both at once.
> 
> The new LINK_FLUSH_ASSIST is a new processor feature the guest will use
> to implement link stack flushing, so maybe that does need a cap?

Yeah, sounds like it will.

> 
> > 
> >> 
> >> Signed-off-by: Nicholas Piggin 
> >> ---
> >>  hw/ppc/spapr_hcall.c   | 5 +
> >>  include/hw/ppc/spapr.h | 6 ++
> >>  2 files changed, 11 insertions(+)
> >> 
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index 7275d0bba1..f656620232 100644
> >> --- a/hw/ppc/spapr_hcall.c
> >> +++ b/hw/ppc/spapr_hcall.c
> >> @@ -1878,6 +1878,9 @@ static target_ulong 
> >> h_get_cpu_characteristics(PowerPCCPU *cpu,
> >>  behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR;
> >>  break;
> >>  case SPAPR_CAP_FIXED:
> >> +behaviour |= H_CPU_BEHAV_NO_L1D_FLUSH_ENTRY;
> >> +behaviour |= H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS;
> >> +behaviour |= H_CPU_BEHAV_NO_STF_BARRIER;
> >>  break;
> >>  default: /* broken */
> >>  assert(safe_cache == SPAPR_CAP_BROKEN);
> >> @@ -1909,9 +1912,11 @@ static target_ulong 
> >> h_get_cpu_characteristics(PowerPCCPU *cpu,
> >>  break;
> >>  case SPAPR_CAP_WORKAROUND:
> >>  behaviour |= H_CPU_BEHAV_FLUSH_COUNT_CACHE;
> >> +behaviour |= H_CPU_BEHAV_FLUSH_LINK_STACK;
> >>  if (count_cache_flush_assist) {
> >>  characteristics |= H_CPU_CHAR_BCCTR_FLUSH_ASSIST;
> >>  }
> >> +/* Should have a way to enable BCCTR_LINK_FLUSH_ASSIST */
> > 
> > Do we need a new spapr capability for this link flush thing?
> 
> It is independent of the FLUSH_COUNT_CACHE capability, so it seems like
> it should I think? Should that be added as a following patch?

No, it will have to go in first or at the same time.  Otherwise we'll
be errneously advertising things.

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


signature.asc
Description: PGP signature

Re: [PATCH v4 1/5] target/ppc: Fold gen_*_xer into their callers

[David Gibson]

On Tue, May 04, 2021 at 11:01:53AM -0300, Bruno Larsen (billionai) wrote:
> folded gen_{read,write}_xer into their only callers, spr_{read,write}_xer
> 
> Signed-off-by: Bruno Larsen (billionai) 
> Reviewed-by: Richard Henderson 

Good cleanup on its own.  Applied to ppc-for-6.1.

> ---
>  target/ppc/translate.c  | 37 -
>  target/ppc/translate_init.c.inc | 33 +++--
>  2 files changed, 31 insertions(+), 39 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index b319d409c6..2f10aa2fea 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4175,43 +4175,6 @@ static void gen_tdi(DisasContext *ctx)
>  
>  /***  Processor control
> ***/
>  
> -static void gen_read_xer(DisasContext *ctx, TCGv dst)
> -{
> -TCGv t0 = tcg_temp_new();
> -TCGv t1 = tcg_temp_new();
> -TCGv t2 = tcg_temp_new();
> -tcg_gen_mov_tl(dst, cpu_xer);
> -tcg_gen_shli_tl(t0, cpu_so, XER_SO);
> -tcg_gen_shli_tl(t1, cpu_ov, XER_OV);
> -tcg_gen_shli_tl(t2, cpu_ca, XER_CA);
> -tcg_gen_or_tl(t0, t0, t1);
> -tcg_gen_or_tl(dst, dst, t2);
> -tcg_gen_or_tl(dst, dst, t0);
> -if (is_isa300(ctx)) {
> -tcg_gen_shli_tl(t0, cpu_ov32, XER_OV32);
> -tcg_gen_or_tl(dst, dst, t0);
> -tcg_gen_shli_tl(t0, cpu_ca32, XER_CA32);
> -tcg_gen_or_tl(dst, dst, t0);
> -}
> -tcg_temp_free(t0);
> -tcg_temp_free(t1);
> -tcg_temp_free(t2);
> -}
> -
> -static void gen_write_xer(TCGv src)
> -{
> -/* Write all flags, while reading back check for isa300 */
> -tcg_gen_andi_tl(cpu_xer, src,
> -~((1u << XER_SO) |
> -  (1u << XER_OV) | (1u << XER_OV32) |
> -  (1u << XER_CA) | (1u << XER_CA32)));
> -tcg_gen_extract_tl(cpu_ov32, src, XER_OV32, 1);
> -tcg_gen_extract_tl(cpu_ca32, src, XER_CA32, 1);
> -tcg_gen_extract_tl(cpu_so, src, XER_SO, 1);
> -tcg_gen_extract_tl(cpu_ov, src, XER_OV, 1);
> -tcg_gen_extract_tl(cpu_ca, src, XER_CA, 1);
> -}
> -
>  /* mcrxr */
>  static void gen_mcrxr(DisasContext *ctx)
>  {
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index d10d7e5bf6..d5527c149f 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -116,12 +116,41 @@ static void spr_access_nop(DisasContext *ctx, int sprn, 
> int gprn)
>  /* XER */
>  static void spr_read_xer(DisasContext *ctx, int gprn, int sprn)
>  {
> -gen_read_xer(ctx, cpu_gpr[gprn]);
> +TCGv dst = cpu_gpr[gprn];
> +TCGv t0 = tcg_temp_new();
> +TCGv t1 = tcg_temp_new();
> +TCGv t2 = tcg_temp_new();
> +tcg_gen_mov_tl(dst, cpu_xer);
> +tcg_gen_shli_tl(t0, cpu_so, XER_SO);
> +tcg_gen_shli_tl(t1, cpu_ov, XER_OV);
> +tcg_gen_shli_tl(t2, cpu_ca, XER_CA);
> +tcg_gen_or_tl(t0, t0, t1);
> +tcg_gen_or_tl(dst, dst, t2);
> +tcg_gen_or_tl(dst, dst, t0);
> +if (is_isa300(ctx)) {
> +tcg_gen_shli_tl(t0, cpu_ov32, XER_OV32);
> +tcg_gen_or_tl(dst, dst, t0);
> +tcg_gen_shli_tl(t0, cpu_ca32, XER_CA32);
> +tcg_gen_or_tl(dst, dst, t0);
> +}
> +tcg_temp_free(t0);
> +tcg_temp_free(t1);
> +tcg_temp_free(t2);
>  }
>  
>  static void spr_write_xer(DisasContext *ctx, int sprn, int gprn)
>  {
> -gen_write_xer(cpu_gpr[gprn]);
> +TCGv src = cpu_gpr[gprn];
> +/* Write all flags, while reading back check for isa300 */
> +tcg_gen_andi_tl(cpu_xer, src,
> +~((1u << XER_SO) |
> +  (1u << XER_OV) | (1u << XER_OV32) |
> +  (1u << XER_CA) | (1u << XER_CA32)));
> +tcg_gen_extract_tl(cpu_ov32, src, XER_OV32, 1);
> +tcg_gen_extract_tl(cpu_ca32, src, XER_CA32, 1);
> +tcg_gen_extract_tl(cpu_so, src, XER_SO, 1);
> +tcg_gen_extract_tl(cpu_ov, src, XER_OV, 1);
> +tcg_gen_extract_tl(cpu_ca, src, XER_CA, 1);
>  }
>  
>  /* LR */

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


signature.asc
Description: PGP signature

Re: [PATCH v4 3/5] target/ppc: move SPR R/W callbacks to translate.c

[David Gibson]

On Tue, May 04, 2021 at 11:01:55AM -0300, Bruno Larsen (billionai) wrote:
> Moved all read and write callbacks for SPRs away from
> translate_init.c.inc and into translate.c; these functions are
> TCG only, so this motion is required to enable building with
> the flag disable-tcg
> 
> Signed-off-by: Bruno Larsen (billionai) 
> ---
>  target/ppc/translate.c  | 1037 ++-
>  target/ppc/translate_init.c.inc | 1011 --
>  2 files changed, 1028 insertions(+), 1020 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 2f10aa2fea..e48fdc6cdf 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -370,6 +370,1034 @@ static inline void gen_sync_exception(DisasContext 
> *ctx)
>  }
>  #endif
>  
> +/*/
> +/* SPR READ/RITE CALLBACKS */

s/RITE/WRITE/

Otherwise, LGTM.

> +
> +static void spr_noaccess(DisasContext *ctx, int gprn, int sprn)
> +{
> +#if 0
> +sprn = ((sprn >> 5) & 0x1F) | ((sprn & 0x1F) << 5);
> +printf("ERROR: try to access SPR %d !\n", sprn);
> +#endif
> +}
> +#define SPR_NOACCESS (_noaccess)
> +
> +/* #define PPC_DUMP_SPR_ACCESSES */
> +
> +/*
> + * Generic callbacks:
> + * do nothing but store/retrieve spr value
> + */
> +static void spr_load_dump_spr(int sprn)
> +{
> +#ifdef PPC_DUMP_SPR_ACCESSES
> +TCGv_i32 t0 = tcg_const_i32(sprn);
> +gen_helper_load_dump_spr(cpu_env, t0);
> +tcg_temp_free_i32(t0);
> +#endif
> +}
> +
> +static void spr_read_generic(DisasContext *ctx, int gprn, int sprn)
> +{
> +gen_load_spr(cpu_gpr[gprn], sprn);
> +spr_load_dump_spr(sprn);
> +}
> +
> +static void spr_store_dump_spr(int sprn)
> +{
> +#ifdef PPC_DUMP_SPR_ACCESSES
> +TCGv_i32 t0 = tcg_const_i32(sprn);
> +gen_helper_store_dump_spr(cpu_env, t0);
> +tcg_temp_free_i32(t0);
> +#endif
> +}
> +
> +static void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
> +{
> +gen_store_spr(sprn, cpu_gpr[gprn]);
> +spr_store_dump_spr(sprn);
> +}
> +
> +#if !defined(CONFIG_USER_ONLY)
> +static void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
> +{
> +#ifdef TARGET_PPC64
> +TCGv t0 = tcg_temp_new();
> +tcg_gen_ext32u_tl(t0, cpu_gpr[gprn]);
> +gen_store_spr(sprn, t0);
> +tcg_temp_free(t0);
> +spr_store_dump_spr(sprn);
> +#else
> +spr_write_generic(ctx, sprn, gprn);
> +#endif
> +}
> +
> +static void spr_write_clear(DisasContext *ctx, int sprn, int gprn)
> +{
> +TCGv t0 = tcg_temp_new();
> +TCGv t1 = tcg_temp_new();
> +gen_load_spr(t0, sprn);
> +tcg_gen_neg_tl(t1, cpu_gpr[gprn]);
> +tcg_gen_and_tl(t0, t0, t1);
> +gen_store_spr(sprn, t0);
> +tcg_temp_free(t0);
> +tcg_temp_free(t1);
> +}
> +
> +static void spr_access_nop(DisasContext *ctx, int sprn, int gprn)
> +{
> +}
> +
> +#endif
> +
> +/* SPR common to all PowerPC */
> +/* XER */
> +static void spr_read_xer(DisasContext *ctx, int gprn, int sprn)
> +{
> +TCGv dst = cpu_gpr[gprn];
> +TCGv t0 = tcg_temp_new();
> +TCGv t1 = tcg_temp_new();
> +TCGv t2 = tcg_temp_new();
> +tcg_gen_mov_tl(dst, cpu_xer);
> +tcg_gen_shli_tl(t0, cpu_so, XER_SO);
> +tcg_gen_shli_tl(t1, cpu_ov, XER_OV);
> +tcg_gen_shli_tl(t2, cpu_ca, XER_CA);
> +tcg_gen_or_tl(t0, t0, t1);
> +tcg_gen_or_tl(dst, dst, t2);
> +tcg_gen_or_tl(dst, dst, t0);
> +if (is_isa300(ctx)) {
> +tcg_gen_shli_tl(t0, cpu_ov32, XER_OV32);
> +tcg_gen_or_tl(dst, dst, t0);
> +tcg_gen_shli_tl(t0, cpu_ca32, XER_CA32);
> +tcg_gen_or_tl(dst, dst, t0);
> +}
> +tcg_temp_free(t0);
> +tcg_temp_free(t1);
> +tcg_temp_free(t2);
> +}
> +
> +static void spr_write_xer(DisasContext *ctx, int sprn, int gprn)
> +{
> +TCGv src = cpu_gpr[gprn];
> +/* Write all flags, while reading back check for isa300 */
> +tcg_gen_andi_tl(cpu_xer, src,
> +~((1u << XER_SO) |
> +  (1u << XER_OV) | (1u << XER_OV32) |
> +  (1u << XER_CA) | (1u << XER_CA32)));
> +tcg_gen_extract_tl(cpu_ov32, src, XER_OV32, 1);
> +tcg_gen_extract_tl(cpu_ca32, src, XER_CA32, 1);
> +tcg_gen_extract_tl(cpu_so, src, XER_SO, 1);
> +tcg_gen_extract_tl(cpu_ov, src, XER_OV, 1);
> +tcg_gen_extract_tl(cpu_ca, src, XER_CA, 1);
> +}
> +
> +/* LR */
> +static void spr_read_lr(DisasContext *ctx, int gprn, int sprn)
> +{
> +tcg_gen_mov_tl(cpu_gpr[gprn], cpu_lr);
> +}
> +
> +static void spr_write_lr(DisasContext *ctx, int sprn, int gprn)
> +{
> +tcg_gen_mov_tl(cpu_lr, cpu_gpr[gprn]);
> +}
> +
> +/* CFAR */
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +static void spr_read_cfar(DisasContext *ctx, int gprn, int sprn)
> +{
> +tcg_gen_mov_tl(cpu_gpr[gprn], cpu_cfar);
> +}
> +
> +static void spr_write_cfar(DisasContext *ctx, int sprn, int gprn)
> +{
> +tcg_gen_mov_tl(cpu_cfar, cpu_gpr[gprn]);
> +}
> +#endif 

Re: [PATCH v4 2/5] target/ppc: renamed SPR registration functions

[David Gibson]

On Tue, May 04, 2021 at 11:01:54AM -0300, Bruno Larsen (billionai) wrote:
> Renamed all gen_spr_* and gen_* functions specifically related to
> registering SPRs to register_*_sprs and register_*, to avoid future
> confusion with other TCG related code.
> 
> Signed-off-by: Bruno Larsen (billionai)
> 

Sorry, this doesn't apply cleanly to the ppc-for-6.1 branch.  Can you
rebase please.

> ---
>  target/ppc/translate_init.c.inc | 860 
>  1 file changed, 430 insertions(+), 430 deletions(-)
> 
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index d5527c149f..4fac8a9950 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -842,7 +842,7 @@ static void _spr_register(CPUPPCState *env, int num, 
> const char *name,
>   oea_read, oea_write, 0, ival)
>  
>  /* Generic PowerPC SPRs */
> -static void gen_spr_generic(CPUPPCState *env)
> +static void register_generic_sprs(CPUPPCState *env)
>  {
>  /* Integer processing */
>  spr_register(env, SPR_XER, "XER",
> @@ -887,7 +887,7 @@ static void gen_spr_generic(CPUPPCState *env)
>  }
>  
>  /* SPR common to all non-embedded PowerPC, including 601 */
> -static void gen_spr_ne_601(CPUPPCState *env)
> +static void register_ne_601_sprs(CPUPPCState *env)
>  {
>  /* Exception processing */
>  spr_register_kvm(env, SPR_DSISR, "DSISR",
> @@ -906,7 +906,7 @@ static void gen_spr_ne_601(CPUPPCState *env)
>  }
>  
>  /* Storage Description Register 1 */
> -static void gen_spr_sdr1(CPUPPCState *env)
> +static void register_sdr1_sprs(CPUPPCState *env)
>  {
>  #ifndef CONFIG_USER_ONLY
>  if (env->has_hv_mode) {
> @@ -929,7 +929,7 @@ static void gen_spr_sdr1(CPUPPCState *env)
>  }
>  
>  /* BATs 0-3 */
> -static void gen_low_BATs(CPUPPCState *env)
> +static void register_low_BATs(CPUPPCState *env)
>  {
>  #if !defined(CONFIG_USER_ONLY)
>  spr_register(env, SPR_IBAT0U, "IBAT0U",
> @@ -1001,7 +1001,7 @@ static void gen_low_BATs(CPUPPCState *env)
>  }
>  
>  /* BATs 4-7 */
> -static void gen_high_BATs(CPUPPCState *env)
> +static void register_high_BATs(CPUPPCState *env)
>  {
>  #if !defined(CONFIG_USER_ONLY)
>  spr_register(env, SPR_IBAT4U, "IBAT4U",
> @@ -1073,7 +1073,7 @@ static void gen_high_BATs(CPUPPCState *env)
>  }
>  
>  /* Generic PowerPC time base */
> -static void gen_tbl(CPUPPCState *env)
> +static void register_tbl(CPUPPCState *env)
>  {
>  spr_register(env, SPR_VTBL,  "TBL",
>   _read_tbl, SPR_NOACCESS,
> @@ -1094,7 +1094,7 @@ static void gen_tbl(CPUPPCState *env)
>  }
>  
>  /* Softare table search registers */
> -static void gen_6xx_7xx_soft_tlb(CPUPPCState *env, int nb_tlbs, int nb_ways)
> +static void register_6xx_7xx_soft_tlb(CPUPPCState *env, int nb_tlbs, int 
> nb_ways)
>  {
>  #if !defined(CONFIG_USER_ONLY)
>  env->nb_tlb = nb_tlbs;
> @@ -1133,7 +1133,7 @@ static void gen_6xx_7xx_soft_tlb(CPUPPCState *env, int 
> nb_tlbs, int nb_ways)
>  }
>  
>  /* SPR common to MPC755 and G2 */
> -static void gen_spr_G2_755(CPUPPCState *env)
> +static void register_G2_755_sprs(CPUPPCState *env)
>  {
>  /* SGPRs */
>  spr_register(env, SPR_SPRG4, "SPRG4",
> @@ -1155,7 +1155,7 @@ static void gen_spr_G2_755(CPUPPCState *env)
>  }
>  
>  /* SPR common to all 7xx PowerPC implementations */
> -static void gen_spr_7xx(CPUPPCState *env)
> +static void register_7xx_sprs(CPUPPCState *env)
>  {
>  /* Breakpoints */
>  /* XXX : not implemented */
> @@ -1353,7 +1353,7 @@ static void spr_write_iamr(DisasContext *ctx, int sprn, 
> int gprn)
>  }
>  #endif /* CONFIG_USER_ONLY */
>  
> -static void gen_spr_amr(CPUPPCState *env)
> +static void register_amr_sprs(CPUPPCState *env)
>  {
>  #ifndef CONFIG_USER_ONLY
>  /*
> @@ -1385,7 +1385,7 @@ static void gen_spr_amr(CPUPPCState *env)
>  #endif /* !CONFIG_USER_ONLY */
>  }
>  
> -static void gen_spr_iamr(CPUPPCState *env)
> +static void register_iamr_sprs(CPUPPCState *env)
>  {
>  #ifndef CONFIG_USER_ONLY
>  spr_register_kvm_hv(env, SPR_IAMR, "IAMR",
> @@ -1406,7 +1406,7 @@ static void spr_read_thrm(DisasContext *ctx, int gprn, 
> int sprn)
>  }
>  #endif /* !CONFIG_USER_ONLY */
>  
> -static void gen_spr_thrm(CPUPPCState *env)
> +static void register_thrm_sprs(CPUPPCState *env)
>  {
>  /* Thermal management */
>  /* XXX : not implemented */
> @@ -1427,7 +1427,7 @@ static void gen_spr_thrm(CPUPPCState *env)
>  }
>  
>  /* SPR specific to PowerPC 604 implementation */
> -static void gen_spr_604(CPUPPCState *env)
> +static void register_604_sprs(CPUPPCState *env)
>  {
>  /* Processor identification */
>  spr_register(env, SPR_PIR, "PIR",
> @@ -1480,7 +1480,7 @@ static void gen_spr_604(CPUPPCState *env)
>  }
>  
>  /* SPR specific to PowerPC 603 implementation */
> -static void gen_spr_603(CPUPPCState *env)
> +static void register_603_sprs(CPUPPCState *env)
>  {
>  /* External access control */
>  /* XXX : not implemented */
> @@ -1498,7 

Re: [PATCH 0/2] Fix for compat mode in P9 < 2.2

[David Gibson]

On Tue, May 04, 2021 at 09:11:28PM -0300, Fabiano Rosas wrote:
> We dropped support in KVM for mixing MMU modes in machines that have
> old POWER9 processors which don't support mixing. As usual I forgot
> that compat mode exists:
> 
>   $ lscpu | grep pvr
>   Model:   2.1 (pvr 004e 1201)
> 
>   $ ~/qemu-system-ppc64 -machine pseries,accel=kvm,max-cpu-compat=power8 ...
>   error: kvm run failed Invalid argument
>   NIP 0100   LR  CTR  XER 
>  CPU#0
>   MSR 80001000 HID0   HF 8000 iidx 3 didx 
> 3
>   TB   DECR 0
>   GPR00    7ff0
>   GPR04    
>   GPR08    
>   GPR12    
>   GPR16    
>   GPR20    
>   GPR24    
>   GPR28    
>   CR   [ -  -  -  -  -  -  -  -  ] RES 
>SRR0   SRR1 PVR 004e1201 
> VRSAVE 
>   SPRG0  SPRG1   SPRG2   
> SPRG3 
>   SPRG4  SPRG5   SPRG6   
> SPRG7 
>   HSRR0  HSRR1 
>CFAR 
>LPCR 0004f01f
>PTCR    DAR   DSISR 
> 
> This series reuses some code we already have to abort and print a
> proper message if the chosen MMU mode is not supported by the host.

Applied to ppc-for-6.1, thanks.

> 
> Fabiano Rosas (2):
>   hw/ppc/spapr.c: Extract MMU mode error reporting into a function
>   hw/ppc/spapr.c: Make sure the host supports the selected MMU mode
> 
>  hw/ppc/spapr.c | 17 +
>  hw/ppc/spapr_hcall.c   | 14 ++
>  include/hw/ppc/spapr.h |  1 +
>  3 files changed, 20 insertions(+), 12 deletions(-)
> 

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


signature.asc
Description: PGP signature

[PATCH v8 4/4] target/arm: set ID_AA64ISAR0.TLB to 2 for max AARCH64 CPU type

[Rebecca Cran]

Indicate support for FEAT_TLBIOS and FEAT_TLBIRANGE by setting
ID_AA64ISAR0.TLB to 2 for the max AARCH64 CPU type.

Signed-off-by: Rebecca Cran 
Reviewed-by: Richard Henderson 
---
 target/arm/cpu64.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index f0a9e968c9c1..f42803ecaf1d 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -651,6 +651,7 @@ static void aarch64_max_initfn(Object *obj)
 t = FIELD_DP64(t, ID_AA64ISAR0, DP, 1);
 t = FIELD_DP64(t, ID_AA64ISAR0, FHM, 1);
 t = FIELD_DP64(t, ID_AA64ISAR0, TS, 2); /* v8.5-CondM */
+t = FIELD_DP64(t, ID_AA64ISAR0, TLB, 2); /* FEAT_TLBIRANGE */
 t = FIELD_DP64(t, ID_AA64ISAR0, RNDR, 1);
 cpu->isar.id_aa64isar0 = t;
 
-- 
2.26.2

[PATCH v8 1/4] accel/tcg: Add TLB invalidation support for ranges of addresses

[Rebecca Cran]

Add functions to support the FEAT_TLBIRANGE ARMv8.4 feature that adds
TLB invalidation instructions to invalidate ranges of addresses.

Signed-off-by: Rebecca Cran 
---
 accel/tcg/cputlb.c  | 128 +++-
 include/exec/exec-all.h |  46 +++
 2 files changed, 171 insertions(+), 3 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 8a7b779270a4..9381745f2528 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -709,7 +709,7 @@ void tlb_flush_page_all_cpus_synced(CPUState *src, 
target_ulong addr)
 tlb_flush_page_by_mmuidx_all_cpus_synced(src, addr, ALL_MMUIDX_BITS);
 }
 
-static void tlb_flush_page_bits_locked(CPUArchState *env, int midx,
+static bool tlb_flush_page_bits_locked(CPUArchState *env, int midx,
target_ulong page, unsigned bits)
 {
 CPUTLBDesc *d = _tlb(env)->d[midx];
@@ -729,7 +729,7 @@ static void tlb_flush_page_bits_locked(CPUArchState *env, 
int midx,
   TARGET_FMT_lx "/" TARGET_FMT_lx ")\n",
   midx, page, mask);
 tlb_flush_one_mmuidx_locked(env, midx, get_clock_realtime());
-return;
+return true;
 }
 
 /* Check if we need to flush due to large pages.  */
@@ -738,13 +738,14 @@ static void tlb_flush_page_bits_locked(CPUArchState *env, 
int midx,
   TARGET_FMT_lx "/" TARGET_FMT_lx ")\n",
   midx, d->large_page_addr, d->large_page_mask);
 tlb_flush_one_mmuidx_locked(env, midx, get_clock_realtime());
-return;
+return true;
 }
 
 if (tlb_flush_entry_mask_locked(tlb_entry(env, midx, page), page, mask)) {
 tlb_n_used_entries_dec(env, midx);
 }
 tlb_flush_vtlb_page_mask_locked(env, midx, page, mask);
+return false;
 }
 
 typedef struct {
@@ -943,6 +944,127 @@ void 
tlb_flush_page_bits_by_mmuidx_all_cpus_synced(CPUState *src_cpu,
 }
 }
 
+typedef struct {
+target_ulong addr;
+target_ulong length;
+uint16_t idxmap;
+uint16_t bits;
+}  TLBFlushPageRangeBitsByMMUIdxData;
+
+static void
+tlb_flush_page_range_bits_by_mmuidx_async_0(CPUState *cpu,
+target_ulong addr,
+target_ulong length,
+uint16_t idxmap,
+unsigned bits)
+{
+CPUArchState *env = cpu->env_ptr;
+bool full_flush;
+int mmu_idx;
+target_ulong page;
+
+assert_cpu_is_self(cpu);
+
+tlb_debug("page addr:" TARGET_FMT_lx "/%u len: " TARGET_FMT_lx
+  " mmu_map:0x%x\n",
+  addr, bits, length, idxmap);
+
+qemu_spin_lock(_tlb(env)->c.lock);
+for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) {
+if ((idxmap >> mmu_idx) & 1) {
+for (page = addr; page < (addr + length); page += 
TARGET_PAGE_SIZE) {
+full_flush = tlb_flush_page_bits_locked(env, mmu_idx,
+page, bits);
+if (full_flush) {
+break;
+}
+}
+}
+}
+qemu_spin_unlock(_tlb(env)->c.lock);
+
+for (page = addr; page < (addr + length); page += TARGET_PAGE_SIZE) {
+tb_flush_jmp_cache(cpu, page);
+}
+}
+
+static void
+tlb_flush_page_range_bits_by_mmuidx_async_1(CPUState *cpu,
+run_on_cpu_data data)
+{
+TLBFlushPageRangeBitsByMMUIdxData *d = data.host_ptr;
+
+tlb_flush_page_range_bits_by_mmuidx_async_0(cpu, d->addr, d->length,
+d->idxmap, d->bits);
+
+g_free(d);
+}
+
+void tlb_flush_page_range_bits_by_mmuidx(CPUState *cpu,
+ target_ulong addr,
+ target_ulong length,
+ uint16_t idxmap,
+ unsigned bits)
+{
+TLBFlushPageRangeBitsByMMUIdxData d;
+TLBFlushPageRangeBitsByMMUIdxData *p;
+
+/* This should already be page aligned */
+addr &= TARGET_PAGE_BITS;
+
+d.addr = addr & TARGET_PAGE_MASK;
+d.idxmap = idxmap;
+d.bits = bits;
+d.length = length;
+
+if (qemu_cpu_is_self(cpu)) {
+tlb_flush_page_range_bits_by_mmuidx_async_0(cpu, addr, length,
+idxmap, bits);
+} else {
+p = g_new(TLBFlushPageRangeBitsByMMUIdxData, 1);
+
+/* Allocate a structure, freed by the worker.  */
+*p = d;
+async_run_on_cpu(cpu, tlb_flush_page_range_bits_by_mmuidx_async_1,
+ RUN_ON_CPU_HOST_PTR(p));
+}
+}
+
+void tlb_flush_page_range_bits_by_mmuidx_all_cpus_synced(CPUState *src_cpu,
+ target_ulong addr,
+ target_ulong length,
+  

[PATCH v8 3/4] target/arm: Add support for FEAT_TLBIOS

[Rebecca Cran]

ARMv8.4 adds the mandatory FEAT_TLBIOS. It provides TLBI
maintenance instructions that extend to the Outer Shareable domain.

Signed-off-by: Rebecca Cran 
---
 target/arm/cpu.h|  5 ++
 target/arm/helper.c | 75 
 2 files changed, 80 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5802798c3069..7986a217acdd 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -4076,6 +4076,11 @@ static inline bool isar_feature_aa64_tlbirange(const 
ARMISARegisters *id)
 return FIELD_EX64(id->id_aa64isar0, ID_AA64ISAR0, TLB) == 2;
 }
 
+static inline bool isar_feature_aa64_tlbios(const ARMISARegisters *id)
+{
+return FIELD_EX64(id->id_aa64isar0, ID_AA64ISAR0, TLB) != 0;
+}
+
 static inline bool isar_feature_aa64_sb(const ARMISARegisters *id)
 {
 return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, SB) != 0;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index cb10851efda8..04c4d766adb9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7213,6 +7213,78 @@ static const ARMCPRegInfo tlbirange_reginfo[] = {
 REGINFO_SENTINEL
 };
 
+static const ARMCPRegInfo tlbios_reginfo[] = {
+{ .name = "TLBI_VMALLE1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 0, .crn = 8, .crm = 1, .opc2 = 0,
+  .access = PL1_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_vmalle1is_write },
+{ .name = "TLBI_ASIDE1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 0, .crn = 8, .crm = 1, .opc2 = 2,
+  .access = PL1_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_vmalle1is_write },
+{ .name = "TLBI_RVAE1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 0, .crn = 8, .crm = 5, .opc2 = 1,
+  .access = PL1_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_rvae1is_write },
+{ .name = "TLBI_RVAAE1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 0, .crn = 8, .crm = 5, .opc2 = 3,
+  .access = PL1_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_rvae1is_write },
+   { .name = "TLBI_RVALE1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 0, .crn = 8, .crm = 5, .opc2 = 5,
+  .access = PL1_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_rvae1is_write },
+{ .name = "TLBI_RVAALE1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 0, .crn = 8, .crm = 5, .opc2 = 7,
+  .access = PL1_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_rvae1is_write },
+{ .name = "TLBI_ALLE2OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 1, .opc2 = 0,
+  .access = PL2_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_alle2is_write },
+   { .name = "TLBI_ALLE1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 1, .opc2 = 4,
+  .access = PL2_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_alle1is_write },
+{ .name = "TLBI_VMALLS12E1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 1, .opc2 = 6,
+  .access = PL2_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_alle1is_write },
+{ .name = "TLBI_IPAS2E1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 4, .opc2 = 0,
+  .access = PL2_W, .type = ARM_CP_NOP },
+{ .name = "TLBI_RIPAS2E1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 4, .opc2 = 3,
+  .access = PL2_W, .type = ARM_CP_NOP },
+{ .name = "TLBI_IPAS2LE1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 4, .opc2 = 4,
+  .access = PL2_W, .type = ARM_CP_NOP },
+{ .name = "TLBI_RIPAS2LE1OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 4, .opc2 = 7,
+  .access = PL2_W, .type = ARM_CP_NOP },
+   { .name = "TLBI_RVAE2OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 5, .opc2 = 1,
+  .access = PL2_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_rvae2is_write },
+   { .name = "TLBI_RVALE2OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 5, .opc2 = 5,
+  .access = PL2_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_rvae2is_write },
+{ .name = "TLBI_ALLE3OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 6, .crn = 8, .crm = 1, .opc2 = 0,
+  .access = PL3_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_alle3is_write },
+   { .name = "TLBI_RVAE3OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 6, .crn = 8, .crm = 5, .opc2 = 1,
+  .access = PL3_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_rvae3is_write },
+   { .name = "TLBI_RVALE3OS", .state = ARM_CP_STATE_AA64,
+  .opc0 = 1, .opc1 = 6, .crn = 8, .crm = 5, .opc2 = 5,
+  .access = PL3_W, .type = ARM_CP_NO_RAW,
+  .writefn = tlbi_aa64_rvae3is_write },
+REGINFO_SENTINEL
+};
+
 static uint64_t rndr_readfn(CPUARMState *env, const ARMCPRegInfo *ri)
 {
 Error *err = NULL;
@@ -8585,6 +8657,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 if 

[PATCH v8 0/4] aarch64: add support for FEAT_TLBIRANGE and FEAT_TLBIOS

[Rebecca Cran]

Improved readability and fixed a bug in
tlb_flush_page_range_bits_by_mmuidx_async_0.

Rebecca Cran (4):
  accel/tcg: Add TLB invalidation support for ranges of addresses
  target/arm: Add support for FEAT_TLBIRANGE
  target/arm: Add support for FEAT_TLBIOS
  target/arm: set ID_AA64ISAR0.TLB to 2 for max AARCH64 CPU type

 accel/tcg/cputlb.c  | 128 ++-
 include/exec/exec-all.h |  46 +++
 target/arm/cpu.h|  10 +
 target/arm/cpu64.c  |   1 +
 target/arm/helper.c | 371 
 5 files changed, 553 insertions(+), 3 deletions(-)

-- 
2.26.2

[PATCH v8 2/4] target/arm: Add support for FEAT_TLBIRANGE

[Rebecca Cran]

ARMv8.4 adds the mandatory FEAT_TLBIRANGE. It provides TLBI
maintenance instructions that apply to a range of input addresses.

Signed-off-by: Rebecca Cran 
---
 target/arm/cpu.h|   5 +
 target/arm/helper.c | 296 
 2 files changed, 301 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 616b39325347..5802798c3069 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -4071,6 +4071,11 @@ static inline bool isar_feature_aa64_pauth_arch(const 
ARMISARegisters *id)
 return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, APA) != 0;
 }
 
+static inline bool isar_feature_aa64_tlbirange(const ARMISARegisters *id)
+{
+return FIELD_EX64(id->id_aa64isar0, ID_AA64ISAR0, TLB) == 2;
+}
+
 static inline bool isar_feature_aa64_sb(const ARMISARegisters *id)
 {
 return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, SB) != 0;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 9b1b98705f91..cb10851efda8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4759,6 +4759,219 @@ static void tlbi_aa64_vae3is_write(CPUARMState *env, 
const ARMCPRegInfo *ri,
   ARMMMUIdxBit_SE3, bits);
 }
 
+#ifdef TARGET_AARCH64
+static uint64_t tlbi_aa64_range_get_length(CPUARMState *env,
+   uint64_t value)
+{
+unsigned int page_shift;
+unsigned int page_size_granule;
+uint64_t num;
+uint64_t scale;
+uint64_t exponent;
+uint64_t length;
+
+num = extract64(value, 39, 4);
+scale = extract64(value, 44, 2);
+page_size_granule = extract64(value, 46, 2);
+
+page_shift = page_size_granule * 2 + 10;
+
+if (page_size_granule == 0) {
+qemu_log_mask(LOG_GUEST_ERROR, "Invalid page size granule %d\n",
+  page_size_granule);
+return 0;
+}
+
+exponent = (5 * scale) + 1;
+length = (num + 1) << (exponent + page_shift);
+
+return length;
+}
+
+static void tlbi_aa64_rvae1_write(CPUARMState *env, const ARMCPRegInfo *ri,
+  uint64_t value)
+{
+/*
+ * Invalidate by VA range, EL1&0.
+ * Currently handles all of RVAE1, RVAAE1, RVAALE1 and RVALE1,
+ * since we don't support flush-for-specific-ASID-only or
+ * flush-last-level-only.
+ */
+ARMMMUIdx mmu_idx;
+int mask;
+int bits;
+uint64_t pageaddr;
+uint64_t length;
+
+CPUState *cs = env_cpu(env);
+mask = vae1_tlbmask(env);
+mmu_idx = ARM_MMU_IDX_A | ctz32(mask);
+if (regime_has_2_ranges(mmu_idx)) {
+pageaddr = sextract64(value, 0, 37) << TARGET_PAGE_BITS;
+} else {
+pageaddr = extract64(value, 0, 37) << TARGET_PAGE_BITS;
+}
+length = tlbi_aa64_range_get_length(env, value);
+bits = tlbbits_for_regime(env, mmu_idx, pageaddr);
+
+if (tlb_force_broadcast(env)) {
+tlb_flush_page_range_bits_by_mmuidx_all_cpus_synced(cs, pageaddr,
+length, mask,
+bits);
+} else {
+tlb_flush_page_range_bits_by_mmuidx(cs, pageaddr, length, mask,
+bits);
+}
+}
+
+static void tlbi_aa64_rvae1is_write(CPUARMState *env, const ARMCPRegInfo *ri,
+uint64_t value)
+{
+/*
+ * Invalidate by VA range, Inner/Outer Shareable EL1&0.
+ * Currently handles all of RVAE1IS, RVAE1OS, RVAAE1IS, RVAAE1OS,
+ * RVAALE1IS, RVAALE1OS, RVALE1IS and RVALE1OS, since we don't support
+ * flush-for-specific-ASID-only, flush-last-level-only or inner/outer
+ * shareable specific flushes.
+ */
+ARMMMUIdx mmu_idx;
+int mask;
+int bits;
+uint64_t pageaddr;
+uint64_t length;
+
+CPUState *cs = env_cpu(env);
+mask = vae1_tlbmask(env);
+mmu_idx = ARM_MMU_IDX_A | ctz32(mask);
+if (regime_has_2_ranges(mmu_idx)) {
+pageaddr = sextract64(value, 0, 37) << TARGET_PAGE_BITS;
+} else {
+pageaddr = extract64(value, 0, 37) << TARGET_PAGE_BITS;
+}
+length = tlbi_aa64_range_get_length(env, value);
+bits = tlbbits_for_regime(env, mmu_idx, pageaddr);
+
+tlb_flush_page_range_bits_by_mmuidx_all_cpus_synced(cs, pageaddr,
+length, mask,
+bits);
+}
+
+static void tlbi_aa64_rvae2_write(CPUARMState *env, const ARMCPRegInfo *ri,
+  uint64_t value)
+{
+/*
+ * Invalidate by VA range, EL2.
+ * Currently handles all of RVAE2 and RVALE2,
+ * since we don't support flush-for-specific-ASID-only or
+ * flush-last-level-only.
+ */
+bool secure;
+int mask;
+int bits;
+uint64_t pageaddr;
+uint64_t length;
+
+CPUState *cs = env_cpu(env);
+secure = arm_is_secure_below_el3(env);
+pageaddr = extract64(value, 0, 37) << TARGET_PAGE_BITS;

Re: [PATCH 1/3] hw/i2c: add support for PMBus

[Joel Stanley]

On Tue, 4 May 2021 at 16:30, Titus Rwantare  wrote:
>
> QEMU has support for SMBus devices, and PMBus is a more specific
> implementation of SMBus. The additions made in this commit makes it easier to
> add new PMBus devices to QEMU.
>
> https://pmbus.org/specification-archives/

I'm not a pmbus expert, but I am happy that someone has created a
framework to model it. I've given them a read and some minor comments
below.

> Reviewed-by: Hao Wu 

Did this review happen on the mailing list? if not, I recommend doing
your review on the public lists, so we can see what comments Hao made.

> Signed-off-by: Titus Rwantare 

> +++ b/hw/i2c/pmbus_device.c
> @@ -0,0 +1,1611 @@
> +/*
> + * PMBus wrapper over SMBus
> + *
> + * Copyright 2021 Google LLC
> + *
> + * 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 can replace these two paragraphs with a SDPX line:

 SPDX-License-Identifier: GPL-2.0-or-later

In addition, add this to your git config to put the .h files on top :

git config diff.orderFile = /some/path/qemu.git/scripts/git.orderfile

It's easier for review.

I'd also suggest putting your tests in a separate patch, following the
addition of the model, again for easier review.

> + */
> +#include "qemu/osdep.h"
> +#include 
> +#include 
> +#include "hw/i2c/pmbus_device.h"
> +#include "qemu/module.h"
> +#include "qemu/log.h"
> +
> +uint16_t pmbus_data2direct_mode(PMBusCoefficients c, uint32_t value)
> +{
> +/* R is usually negative to fit large readings into 16 bits */
> +uint16_t y = (c.m * value + c.b) * pow(10, c.R);
> +return y;
> +}
> +
> +uint32_t pmbus_direct_mode2data(PMBusCoefficients c, uint16_t value)
> +{
> +/* X = (Y * 10^-R - b) / m */
> +uint32_t x = (value / pow(10, c.R) - c.b) / c.m;
> +return x;
> +}
> +
> +static void pmbus_send(PMBusDevice *pmdev, const uint8_t *data, uint16_t len)
> +{
> +if (pmdev->out_buf_len + len > SMBUS_DATA_MAX_LEN) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "PMBus device tried to send too much data");
> +len = 0;
> +}
> +
> +for (int i = len - 1; i >= 0; i--) {
> +pmdev->out_buf[i + pmdev->out_buf_len] = data[len - i - 1];
> +}
> +pmdev->out_buf_len += len;
> +}
> +
> +/* Internal only, convert unsigned ints to the little endian bus */
> +static void pmbus_send_uint(PMBusDevice *pmdev, uint64_t data, uint8_t size)
> +{
> +uint8_t bytes[8];

Do you need to assert that size is less than the array size? Probably
not as all the callers are local.

> +for (int i = 0; i < size; i++) {
> +bytes[i] = data & 0xFF;
> +data = data >> 8;
> +}
> +pmbus_send(pmdev, bytes, size);
> +}
> +
> +void pmbus_send_block(PMBusDevice *pmdev, PMBusBlock block)
> +{
> +pmbus_send(pmdev, block.buf, block.len);
> +}
> +
> +void pmbus_send8(PMBusDevice *pmdev, uint8_t data)
> +{
> +pmbus_send_uint(pmdev, data, 1);
> +}
> +
> +void pmbus_send16(PMBusDevice *pmdev, uint16_t data)
> +{
> +pmbus_send_uint(pmdev, data, 2);
> +}
> +
> +void pmbus_send32(PMBusDevice *pmdev, uint32_t data)
> +{
> +pmbus_send_uint(pmdev, data, 4);
> +}
> +
> +void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
> +{
> +pmbus_send_uint(pmdev, data, 8);
> +}
> +
> +void pmbus_send_string(PMBusDevice *pmdev, const char *data)
> +{
> +size_t len = strlen(data);

Do you need to assert that len is > 0?

> +g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
> +pmdev->out_buf[len + pmdev->out_buf_len] = len;
> +
> +for (int i = len - 1; i >= 0; i--) {
> +pmdev->out_buf[i + pmdev->out_buf_len] = data[len - 1 - i];
> +}
> +pmdev->out_buf_len += len + 1;
> +}
> +
> +
> +static uint64_t pmbus_receive_uint(const uint8_t *buf, uint8_t len)
> +{
> +uint64_t ret = 0;
> +
> +/* Exclude command code from return value */
> +buf++;
> +len--;
> +
> +for (int i = len - 1; i >= 0; i--) {
> +ret = ret << 8 | buf[i];
> +}
> +return ret;
> +}
> +
> +uint8_t pmbus_receive8(PMBusDevice *pmdev)
> +{
> +if (pmdev->in_buf_len - 1 != 1) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "%s: length mismatch. Expected 1 byte, got %d bytes\n",
> +  __func__, pmdev->in_buf_len - 1);
> +}
> +return (uint8_t) pmbus_receive_uint(pmdev->in_buf, pmdev->in_buf_len);

The casts are implicit in C, so you do not need to explicitly cast the
return types in this and the functions below.

> +}
> +
> +uint16_t pmbus_receive16(PMBusDevice 

[PATCH 0/2] Fix for compat mode in P9 < 2.2

[Fabiano Rosas]

We dropped support in KVM for mixing MMU modes in machines that have
old POWER9 processors which don't support mixing. As usual I forgot
that compat mode exists:

  $ lscpu | grep pvr
  Model:   2.1 (pvr 004e 1201)

  $ ~/qemu-system-ppc64 -machine pseries,accel=kvm,max-cpu-compat=power8 ...
  error: kvm run failed Invalid argument
  NIP 0100   LR  CTR  XER 
 CPU#0
  MSR 80001000 HID0   HF 8000 iidx 3 didx 3
  TB   DECR 0
  GPR00    7ff0
  GPR04    
  GPR08    
  GPR12    
  GPR16    
  GPR20    
  GPR24    
  GPR28    
  CR   [ -  -  -  -  -  -  -  -  ] RES 
   SRR0   SRR1 PVR 004e1201 VRSAVE 

  SPRG0  SPRG1   SPRG2   SPRG3 

  SPRG4  SPRG5   SPRG6   SPRG7 

  HSRR0  HSRR1 
   CFAR 
   LPCR 0004f01f
   PTCR    DAR   DSISR 

This series reuses some code we already have to abort and print a
proper message if the chosen MMU mode is not supported by the host.

Fabiano Rosas (2):
  hw/ppc/spapr.c: Extract MMU mode error reporting into a function
  hw/ppc/spapr.c: Make sure the host supports the selected MMU mode

 hw/ppc/spapr.c | 17 +
 hw/ppc/spapr_hcall.c   | 14 ++
 include/hw/ppc/spapr.h |  1 +
 3 files changed, 20 insertions(+), 12 deletions(-)

--
2.29.2

Re: [PATCH v4 0/3] nvdimm: Enable sync-dax property for nvdimm

[Dan Williams]

On Tue, May 4, 2021 at 2:03 AM Aneesh Kumar K.V
 wrote:
>
> On 5/4/21 11:13 AM, Pankaj Gupta wrote:
> 
>
> >>
> >> What this patch series did was to express that property via a device
> >> tree node and guest driver enables a hypercall based flush mechanism to
> >> ensure persistence.
> >
> > Would VIRTIO (entirely asynchronous, no trap at host side) based
> > mechanism is better
> > than hyper-call based? Registering memory can be done any way. We
> > implemented virtio-pmem
> > flush mechanisms with below considerations:
> >
> > - Proper semantic for guest flush requests.
> > - Efficient mechanism for performance pov.
> >
>
> sure, virio-pmem can be used as an alternative.
>
> > I am just asking myself if we have platform agnostic mechanism already
> > there, maybe
> > we can extend it to suit our needs? Maybe I am missing some points here.
> >
>
> What is being attempted in this series is to indicate to the guest OS
> that the backing device/file used for emulated nvdimm device cannot
> guarantee the persistence via cpu cache flush instructions.

Right, the detail I am arguing is that it should be a device
description, not a backend file property. In other words this proposal
is advocating this:

-object memory-backend-file,id=mem1,share,sync-dax=$sync-dax,mem-path=$path
-device nvdimm,memdev=mem1

...and I am advocating for reusing or duplicating the virtio-pmem
model like this:

-object memory-backend-file,id=mem1,share,mem-path=$path
-device spapr-hcall,memdev=mem1

...because the interface to the guest is the device, not the
memory-backend-file.

[PATCH 2/2] hw/ppc/spapr.c: Make sure the host supports the selected MMU mode

[Fabiano Rosas]

Starting with Linux kernel v5.12 we dropped support[1] in KVM for
hosts that can't have their threads running in different MMU modes
(POWER9 < DD2.2). In these hosts, KVM will no longer report the
KVM_CAP_PPC_MMU_HASH_V3 capability[2] when the host is running Radix.

For guests that support both MMU modes, the negotiation during CAS
will make sure it selects the correct one.

For guests that only support Hash, such as P8 compat mode guests, the
following error is currently thrown:

  $ ~/qemu-system-ppc64 -machine pseries,accel=kvm,max-cpu-compat=power8 ...
  error: kvm run failed Invalid argument
  NIP 0100   LR  CTR  XER 
 CPU#0
  MSR 80001000 HID0   HF 8000 iidx 3 didx 3
  TB   DECR 0
  GPR00    7ff0
  GPR04    
  GPR08    
  GPR12    
  GPR16    
  GPR20    
  GPR24    
  GPR28    
  CR   [ -  -  -  -  -  -  -  -  ] RES 
   SRR0   SRR1 PVR 004e1201 VRSAVE 

  SPRG0  SPRG1   SPRG2   SPRG3 

  SPRG4  SPRG5   SPRG6   SPRG7 

  HSRR0  HSRR1 
   CFAR 
   LPCR 0004f01f
   PTCR    DAR   DSISR 

This patch adds a verification during the writing of the platform
support vector so that we error out as soon as we determine this guest
only supports Hash and the host doesn't.

  ~/qemu-system-ppc64 -machine pseries,accel=kvm,max-cpu-compat=power8 ...
  qemu-system-ppc64: Guest requested unavailable MMU mode (hash).

1- https://git.kernel.org/torvalds/p/b1b1697ae0cc8
2- https://git.kernel.org/torvalds/p/a722076e94702

Signed-off-by: Fabiano Rosas 
---
 hw/ppc/spapr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4fd226b15b..69201399eb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -981,6 +981,7 @@ static void spapr_dt_ov5_platform_support(SpaprMachineState 
*spapr, void *fdt,
  */
 val[1] = SPAPR_OV5_XIVE_LEGACY; /* XICS */
 val[3] = 0x00; /* Hash */
+spapr_check_mmu_mode(false);
 } else if (kvm_enabled()) {
 if (kvmppc_has_cap_mmu_radix() && kvmppc_has_cap_mmu_hash_v3()) {
 val[3] = 0x80; /* OV5_MMU_BOTH */
-- 
2.29.2

[PATCH 1/2] hw/ppc/spapr.c: Extract MMU mode error reporting into a function

[Fabiano Rosas]

A following patch will make use of it.

Signed-off-by: Fabiano Rosas 
---
 hw/ppc/spapr.c | 16 
 hw/ppc/spapr_hcall.c   | 14 ++
 include/hw/ppc/spapr.h |  1 +
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 529ff056dd..4fd226b15b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1558,6 +1558,22 @@ void spapr_setup_hpt(SpaprMachineState *spapr)
 }
 }
 
+void spapr_check_mmu_mode(bool guest_radix)
+{
+if (guest_radix) {
+if (kvm_enabled() && !kvmppc_has_cap_mmu_radix()) {
+error_report("Guest requested unavailable MMU mode (radix).");
+exit(EXIT_FAILURE);
+}
+} else {
+if (kvm_enabled() && kvmppc_has_cap_mmu_radix()
+&& !kvmppc_has_cap_mmu_hash_v3()) {
+error_report("Guest requested unavailable MMU mode (hash).");
+exit(EXIT_FAILURE);
+}
+}
+}
+
 static void spapr_machine_reset(MachineState *machine)
 {
 SpaprMachineState *spapr = SPAPR_MACHINE(machine);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 7b5cd3553c..32b7100125 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1755,18 +1755,8 @@ target_ulong do_client_architecture_support(PowerPCCPU 
*cpu,
 spapr_ovec_intersect(spapr->ov5_cas, spapr->ov5, ov5_guest);
 spapr_ovec_cleanup(ov5_guest);
 
-if (guest_radix) {
-if (kvm_enabled() && !kvmppc_has_cap_mmu_radix()) {
-error_report("Guest requested unavailable MMU mode (radix).");
-exit(EXIT_FAILURE);
-}
-} else {
-if (kvm_enabled() && kvmppc_has_cap_mmu_radix()
-&& !kvmppc_has_cap_mmu_hash_v3()) {
-error_report("Guest requested unavailable MMU mode (hash).");
-exit(EXIT_FAILURE);
-}
-}
+spapr_check_mmu_mode(guest_radix);
+
 spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
 spapr_ovec_cleanup(ov1_guest);
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index bf7cab7a2c..52b68b2d92 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -820,6 +820,7 @@ void spapr_dt_events(SpaprMachineState *sm, void *fdt);
 void close_htab_fd(SpaprMachineState *spapr);
 void spapr_setup_hpt(SpaprMachineState *spapr);
 void spapr_free_hpt(SpaprMachineState *spapr);
+void spapr_check_mmu_mode(bool guest_radix);
 SpaprTceTable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
 void spapr_tce_table_enable(SpaprTceTable *tcet,
 uint32_t page_shift, uint64_t bus_offset,
-- 
2.29.2

[PATCH v3 1/2] ui/cocoa: capture all keys and combos when mouse is grabbed

[gustavo]

From: Gustavo Noronha Silva 

Applications such as Gnome may use Alt-Tab and Super-Tab for different
purposes, some use Ctrl-arrows so we want to allow qemu to handle
everything when it captures the mouse/keyboard.

However, Mac OS handles some combos like Command-Tab and Ctrl-arrows
at an earlier part of the event handling chain, not letting qemu see it.

We add a global Event Tap that allows qemu to see all events when the
mouse is grabbed. Note that this requires additional permissions.

See:

https://developer.apple.com/documentation/coregraphics/1454426-cgeventtapcreate?language=objc#discussion
https://support.apple.com/en-in/guide/mac-help/mh32356/mac

Acked-by: Markus Armbruster 
Signed-off-by: Gustavo Noronha Silva 
---
 qapi/ui.json| 16 +++
 qemu-options.hx |  3 +++
 ui/cocoa.m  | 71 -
 3 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 1052ca9c38..4ccfae4bbb 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1088,6 +1088,21 @@
 { 'struct'  : 'DisplayCurses',
   'data': { '*charset'   : 'str' } }
 
+##
+# @DisplayCocoa:
+#
+# Cocoa display options.
+#
+# @full-grab: Capture all key presses, including system combos. This
+# requires accessibility permissions, since it performs
+# a global grab on key events. (default: off)
+# See https://support.apple.com/en-in/guide/mac-help/mh32356/mac
+#
+# Since: 6.1
+##
+{ 'struct'  : 'DisplayCocoa',
+  'data': { '*full-grab' : 'bool' } }
+
 ##
 # @DisplayType:
 #
@@ -1153,6 +1168,7 @@
 '*gl': 'DisplayGLMode' },
   'discriminator' : 'type',
   'data': { 'gtk': 'DisplayGTK',
+'cocoa'  : 'DisplayCocoa',
 'curses' : 'DisplayCurses',
 'egl-headless'   : 'DisplayEGLHeadless'} }
 
diff --git a/qemu-options.hx b/qemu-options.hx
index fd21002bd6..a77505241f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1783,6 +1783,9 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
 #if defined(CONFIG_CURSES)
 "-display curses[,charset=]\n"
 #endif
+#if defined(CONFIG_COCOA)
+"-display cocoa[,full_grab=on|off]\n"
+#endif
 #if defined(CONFIG_OPENGL)
 "-display egl-headless[,rendernode=]\n"
 #endif
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 37e1fb52eb..0a45ab30e6 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -72,6 +72,7 @@
 typedef struct {
 int width;
 int height;
+bool full_grab;
 } QEMUScreen;
 
 static void cocoa_update(DisplayChangeListener *dcl,
@@ -304,11 +305,13 @@ @interface QemuCocoaView : NSView
 BOOL isMouseGrabbed;
 BOOL isFullscreen;
 BOOL isAbsoluteEnabled;
+CFMachPortRef eventsTap;
 }
 - (void) switchSurface:(pixman_image_t *)image;
 - (void) grabMouse;
 - (void) ungrabMouse;
 - (void) toggleFullScreen:(id)sender;
+- (void) setFullGrab:(id)sender;
 - (void) handleMonitorInput:(NSEvent *)event;
 - (bool) handleEvent:(NSEvent *)event;
 - (bool) handleEventLocked:(NSEvent *)event;
@@ -323,6 +326,7 @@ - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled;
  */
 - (BOOL) isMouseGrabbed;
 - (BOOL) isAbsoluteEnabled;
+- (BOOL) isFullGrabEnabled;
 - (float) cdx;
 - (float) cdy;
 - (QEMUScreen) gscreen;
@@ -331,6 +335,19 @@ - (void) raiseAllKeys;
 
 QemuCocoaView *cocoaView;
 
+static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, 
CGEventRef cgEvent, void *userInfo)
+{
+QemuCocoaView *cocoaView = (QemuCocoaView*) userInfo;
+NSEvent* event = [NSEvent eventWithCGEvent:cgEvent];
+if ([cocoaView isFullGrabEnabled] && [cocoaView isMouseGrabbed] && 
[cocoaView handleEvent:event]) {
+COCOA_DEBUG("Global events tap: qemu handled the event, capturing!\n");
+return NULL;
+}
+COCOA_DEBUG("Global events tap: qemu did not handle the event, letting it 
through...\n");
+
+return cgEvent;
+}
+
 @implementation QemuCocoaView
 - (id)initWithFrame:(NSRect)frameRect
 {
@@ -344,6 +361,32 @@ - (id)initWithFrame:(NSRect)frameRect
 kbd = qkbd_state_init(dcl.con);
 
 }
+
+CGEventMask mask = CGEventMaskBit(kCGEventKeyDown) | 
CGEventMaskBit(kCGEventKeyUp) | CGEventMaskBit(kCGEventFlagsChanged);
+eventsTap = CGEventTapCreate(kCGHIDEventTap, kCGHeadInsertEventTap, 
kCGEventTapOptionDefault,
+ mask, handleTapEvent, self);
+if (!eventsTap) {
+warn_report("Could not create event tap, system key combos will not be 
captured.\n");
+return self;
+} else {
+COCOA_DEBUG("Global events tap created! Will capture system key 
combos.\n");
+}
+
+CFRunLoopRef runLoop = CFRunLoopGetCurrent();
+if (!runLoop) {
+warn_report("Could not obtain current CF RunLoop, system key combos 
will not be captured.\n");
+return self;
+}
+
+CFRunLoopSourceRef tapEventsSrc = 
CFMachPortCreateRunLoopSource(kCFAllocatorDefault, eventsTap, 0);
+if 

[PATCH v3 0/2] cocoa: keyboard quality of life

[gustavo]

From: Gustavo Noronha Silva 

v3 removes a rogue ; that made its way into v2 and makes the
swap-option-command parameter off by default, so existing
behaviour is maintained, as suggested by BALATON Zoltan.

-

This series adds two new options to the cocoa display:

 - full-grab causes it to use a global tap to steal system combos
   away from Mac OS X, so they can be handled by the VM

 - swap-option-command does what it says on the tin; while that is
   something you can do at the Mac OS X level or even supported by
   some keyboards, it is much more convenient to have qemu put
   Meta/Super and Alt where they belong if you are running a
   non-Mac VM

Both are off by default. For full-grab in particular, it is off also
 because unfortunately it needs accessibility permissions for input
grabbing, so it requires more deliberate action by the user anyway.


Gustavo Noronha Silva (2):
  ui/cocoa: capture all keys and combos when mouse is grabbed
  ui/cocoa: add option to swap Option and Command, enable by default

 qapi/ui.json|  22 
 qemu-options.hx |   4 ++
 ui/cocoa.m  | 135 
 3 files changed, 152 insertions(+), 9 deletions(-)

-- 
2.24.3 (Apple Git-128)

[PATCH v3 2/2] ui/cocoa: add option to swap Option and Command, enable by default

[gustavo]

From: Gustavo Noronha Silva 

On Mac OS X the Option key maps to Alt and Command to Super/Meta. This change
swaps them around so that Alt is the key closer to the space bar and Meta/Super
is between Control and Alt, like on non-Mac keyboards.

It is a cocoa display option, disabled by default.

Acked-by: Markus Armbruster 
Signed-off-by: Gustavo Noronha Silva 
---
 qapi/ui.json|  8 ++-
 qemu-options.hx |  3 ++-
 ui/cocoa.m  | 64 ++---
 3 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 4ccfae4bbb..ee6fde46d5 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1098,10 +1098,16 @@
 # a global grab on key events. (default: off)
 # See https://support.apple.com/en-in/guide/mac-help/mh32356/mac
 #
+# @swap-option-command: Swap the Option and Command keys so that their key
+#   codes match their position on non-Mac keyboards and
+#   you can use Meta/Super and Alt where you expect them.
+#   (default: off)
+#
 # Since: 6.1
 ##
 { 'struct'  : 'DisplayCocoa',
-  'data': { '*full-grab' : 'bool' } }
+  'data': { '*full-grab'   : 'bool',
+'*swap-option-command' : 'bool' } }
 
 ##
 # @DisplayType:
diff --git a/qemu-options.hx b/qemu-options.hx
index a77505241f..6fcb0b6aaa 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1784,7 +1784,8 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
 "-display curses[,charset=]\n"
 #endif
 #if defined(CONFIG_COCOA)
-"-display cocoa[,full_grab=on|off]\n"
+"-display cocoa[,full-grab=on|off]\n"
+"  [,swap-option-command=on|off]\n"
 #endif
 #if defined(CONFIG_OPENGL)
 "-display egl-headless[,rendernode=]\n"
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 0a45ab30e6..b8fe64e252 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -73,6 +73,7 @@
 int width;
 int height;
 bool full_grab;
+bool swap_option_command;
 } QEMUScreen;
 
 static void cocoa_update(DisplayChangeListener *dcl,
@@ -327,6 +328,7 @@ - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled;
 - (BOOL) isMouseGrabbed;
 - (BOOL) isAbsoluteEnabled;
 - (BOOL) isFullGrabEnabled;
+- (BOOL) isSwapOptionCommandEnabled;
 - (float) cdx;
 - (float) cdy;
 - (QEMUScreen) gscreen;
@@ -648,6 +650,13 @@ - (void) setFullGrab:(id)sender
 screen.full_grab = true;
 }
 
+- (void) setSwapOptionCommand:(id)sender
+{
+COCOA_DEBUG("QemuCocoaView: setSwapOptionCommand\n");
+
+screen.swap_option_command = true;
+}
+
 - (void) toggleKey: (int)keycode {
 qkbd_state_key_event(kbd, keycode, !qkbd_state_key_get(kbd, keycode));
 }
@@ -797,12 +806,22 @@ - (bool) handleEventLocked:(NSEvent *)event
 qkbd_state_key_event(kbd, Q_KEY_CODE_CTRL_R, false);
 }
 if (!(modifiers & NSEventModifierFlagOption)) {
-qkbd_state_key_event(kbd, Q_KEY_CODE_ALT, false);
-qkbd_state_key_event(kbd, Q_KEY_CODE_ALT_R, false);
+if ([self isSwapOptionCommandEnabled]) {
+qkbd_state_key_event(kbd, Q_KEY_CODE_META_L, false);
+qkbd_state_key_event(kbd, Q_KEY_CODE_META_R, false);
+} else {
+qkbd_state_key_event(kbd, Q_KEY_CODE_ALT, false);
+qkbd_state_key_event(kbd, Q_KEY_CODE_ALT_R, false);
+}
 }
 if (!(modifiers & NSEventModifierFlagCommand)) {
-qkbd_state_key_event(kbd, Q_KEY_CODE_META_L, false);
-qkbd_state_key_event(kbd, Q_KEY_CODE_META_R, false);
+if ([self isSwapOptionCommandEnabled]) {
+qkbd_state_key_event(kbd, Q_KEY_CODE_ALT, false);
+qkbd_state_key_event(kbd, Q_KEY_CODE_ALT_R, false);
+} else {
+qkbd_state_key_event(kbd, Q_KEY_CODE_META_L, false);
+qkbd_state_key_event(kbd, Q_KEY_CODE_META_R, false);
+}
 }
 
 switch ([event type]) {
@@ -834,13 +853,21 @@ - (bool) handleEventLocked:(NSEvent *)event
 
 case kVK_Option:
 if (!!(modifiers & NSEventModifierFlagOption)) {
-[self toggleKey:Q_KEY_CODE_ALT];
+if ([self isSwapOptionCommandEnabled]) {
+[self toggleKey:Q_KEY_CODE_META_L];
+} else {
+[self toggleKey:Q_KEY_CODE_ALT];
+}
 }
 break;
 
 case kVK_RightOption:
 if (!!(modifiers & NSEventModifierFlagOption)) {
-[self toggleKey:Q_KEY_CODE_ALT_R];
+if ([self isSwapOptionCommandEnabled]) {
+[self toggleKey:Q_KEY_CODE_META_R];
+} else {
+[self toggleKey:Q_KEY_CODE_ALT_R];
+}
 }
 break;
 
@@ -848,14 +875,22 @@ - (bool) handleEventLocked:(NSEvent *)event

Re: [PATCH 1/3] hw/i2c: add support for PMBus

[Titus Rwantare]

Hi Phil,

It looks big but the bulk of it is simple switch statements and
definitions. Unless you mean you'd like it split despite that.

Regards,
Titus


On Tue, 4 May 2021 at 16:49, Philippe Mathieu-Daudé  wrote:
>
> Hi Titus,
>
> On 5/4/21 6:28 PM, Titus Rwantare wrote:
> > QEMU has support for SMBus devices, and PMBus is a more specific
> > implementation of SMBus. The additions made in this commit makes it easier 
> > to
> > add new PMBus devices to QEMU.
> >
> > https://pmbus.org/specification-archives/
> >
> > Reviewed-by: Hao Wu 
> > Signed-off-by: Titus Rwantare 
> > ---
> >  hw/arm/Kconfig|1 +
> >  hw/i2c/Kconfig|4 +
> >  hw/i2c/meson.build|1 +
> >  hw/i2c/pmbus_device.c | 1611 +
> >  include/hw/i2c/pmbus_device.h |  520 +++
> >  5 files changed, 2137 insertions(+)
>
> TBH this is quite a big patch to digest.
>
> Any chance you could split it?
>
> Thanks,
>
> Phil.

Re: Gitlab Issue Tracker - Proposed Workflow

[John Snow]

On 5/4/21 4:24 PM, Peter Maydell wrote:

On Tue, 4 May 2021 at 19:34, John Snow  wrote:

# Some concerns on the above scheme:

- "In Progress" is not likely to be used faithfully and will fall out of
sync often. It's not clear if there should be a difference between a bug
having an assignee and a bug labeled "In Progress". I don't want to get
in the business of managing people's time and forcing them to track
their work.

At the same time, for bugs being actively fixed by a contributor on-list
who is not [known to be] present on gitlab, it's still possibly a nice
courtesy to mark a bug as not 'free for the taking'.

Meanwhile, there are several bugs I "own" but I am not actually actively
working on. These are the sorts of things that I wouldn't mind someone
taking from me if they wanted to, so the "In Progress" label provides
some useful distinction there if someone wished to use it.

I think I am inclined to keep it for now, at least until gitlab adoption
rate is higher and bugs can be assigned more faithfully.


I like "In Progress" because I use it largely to track "I wrote a
patch for this and submitted it to the list, but it hasn't gone in
yet". This means that later on when a release is more imminent I
can easily scan through a list of "in progress" bug reports to find
the "we just forgot to update the bug state when the patch was
committed" ones and the "somebody submitted a patch but it fell
through the cracks" ones. That's a lot harder if you have to look
through the whole pile of "new" bugs.



Yeah, I think that's something that winds up being nice. As an 
informational/non-exhaustive label, you can at least quickly search them 
to see if you can close any of those bugs if we missed 'em.


Good argument for keeping that state for now.


- Gitlab will automatically close issues that reference the gitlab issue
tracker from the commit message, but it won't apply the "Merged" label,
so it's at risk of falling out of sync.

Closed issues retain their "Workflow::" labels, but won't show up in the
issue search by default unless you ask to include closed issues as well.

I think we can likely just drop this label, and have bugs go directly
from whatever state they're in to "Closed". The issue board will look
cleaner and there's less custodial work for maintainers.



- Similarly to the above concern, "Released" is extra paperwork for us
to maintain. I recommend instead we drop the label and begin using
"Milestones" for each release, and attaching issues we have fixed or
want to fix to specific Milestones. This will give us a way to track
pending issues for release candidates as well as a convenient list, in
the end, of which bugs were fixed in which specific release, which is
far more useful than a "Released" tag anyway.


I guess so. I dunno how much our old fixed/released distinction
was helping users anyway. I do think that using Milestones both
for "I want to fix this bug for release X" and "this bug is fixed
in release X" is going to mean that it's not reliable for the latter
because we're going to have bugs where somebody optimistically set
the milestone and then the patches didn't land in time for the release.
But unless we care enough to write a bot that auto-updates milestones
we'll just have to live with that.



Not a huge problem to punt "I want to fix" issues off to the next 
release during the rc0 phase. Writing a script to do this should be 
pretty easy:


Just look for any issues in Milestone 6.1.0 that are still Open and not 
marked 'Blocker/Regression/In Progress' (as examples) and either punt 
them to 6.2.0 or simply delete their milestone association and allow 
someone who cares to re-assess.



-- PMM



Thanks!

--js

Re: Gitlab Issue Tracker - Proposed Workflow

[John Snow]

On 5/4/21 2:52 PM, Daniel P. Berrangé wrote:

On Tue, May 04, 2021 at 02:33:58PM -0400, John Snow wrote:

I'm seeking feedback on our Gitlab issue handling workflow.
(There's a TLDR at the bottom of the mail.)



- In Progress: For issues that a developer is actively working on. The
intent was to be able to mark bugs in such a way that contributors would not
assume they are available to be hacked on. For instance, if a non-gitlab
contributor submits patches for an issue, we might mark it as In Progress so
that it does not appear to be going unaddressed.


If someone is actively working on it, they could just stick a comment
on the issue, possibly with a mailing list posting.

With a simple "in progress" flag you can end up marking things as if
someone is working on it, but then they go off and do other things.

If you just have a mailing list posting/comment, you can at least
see how up2date that comment was and judge whether the person is
still caring about the bug.



I can see this critique. OTOH, and as Peter has written, it's also a 
fine way to find a list of candidate bugs that we may have forgotten about.


Ideally people still post a link to the patch if they move it to "in 
progress" so we get the benefits of both methods. I usually try to link 
to a mailing list thread whenever I update a BZ/LP like this. I can keep 
that habit alive here.


It might be fine as simply an informational state that we just don't 
treat as authoritative/exhaustive.



- Similarly to the above concern, "Released" is extra paperwork for us to
maintain. I recommend instead we drop the label and begin using "Milestones"
for each release, and attaching issues we have fixed or want to fix to
specific Milestones. This will give us a way to track pending issues for
release candidates as well as a convenient list, in the end, of which bugs
were fixed in which specific release, which is far more useful than a
"Released" tag anyway.


Yeah, if we really wnat to record release info against bugs, then the
milestones looks more useful, but

Generating a list of bugs in the release is only useful if that list
is reasonably complete. That means we need someone who really cares
about this because most people will never bother to set a milestone
and so some janitor will have to cleanup for each release. I'm loathe
to define a system that creates busy-work for someone unless they
definitely want that work.

IOW, in abence of a janitor volunteering, I'd just keep life simple
and let bugs be marked closed as they get merged via the commit message.

We can query bugs linked to commits in this way, and I feel we'll have
more luck getting maintainers to reliably add bug links in commits
than playing with milestones.



Yeah, I don't expect us to do all of our release planning in Gitlab. 
This is merely a convenient way to associate an issue with a release.


I'm personally willing to do SOME of the janitoring here; in that I can 
associate closed issues with a release milestone and -- when preparing 
for rc0 freeze -- punt off any bugs that aren't blockers/in-progress to 
the next milestone.


I don't expect it would become an *exhaustive* list of changes, just 
merely a way to associate the things that DID make it into the issue 
tracker with a release.


It may allow us to publish a nice list in the changelog with links to 
the gitlab issue tracker each release, which might be nice because issue 
tracker bugs are often ones that end-users filed, so it's a way to pay 
very visible attention to that feedback.



- Let's add "Workflow::Confirmed" to help distinguish lightly-triaged
   bugs from serious, actionable bugs.


I wonder if instead of that we could just have some prioritization
tags eg

   - "Regression" - something that previously worked and we broke

   - "Release blocker" - we decided we must have this fixed in the
 next relaase - currently we paste such bugs into the planning
 page wiki for a release

   - "Important" - something that's high priority to address


This confer more useful meaning that a "Confirmed" tag I think.



Definitely nothing stopping us from adding those labels outside of the 
Workflow scope.


"Regression" might be nice for highlighting issues for inclusion into 
the next stable release.


"Release blocker" could be just as well served by inclusion in the 
upcoming Milestone. (Granted: there would be no difference between stuff 
that's puntable or not-puntable, so there may indeed be some use to a 
"Blocker" label.)


"Important" might be best served by the "weight" fields, and might be a 
bit too subjective.



Anything else without those tags can be considered a "normal" bug
that may or may not be dealt.


So I wonder if it just suffices to have standalone "workflow:triaged" and
"workflow:needinfo" flags, possibly as prefixed labels, not scoped labels.



I think I am rather fond of having the scoped labels for the sake of 
making the "Board" view interesting to look at.


Open --> 

Re: [PATCH v2 0/4] hw/sparc: Kconfig fixes to build with/without the leon3 machine

[Mark Cave-Ayland]

On 28/04/2021 15:16, Philippe Mathieu-Daudé wrote:


Missing review: 2-4

Since v1:
- move cpu_check_irqs() to target/sparc/ (rth)

This series fixes link failure when building either the leon3
machine or the sun4m ones.

The problem is we have hardware specific code in the architectural
translation code. Move this code to hw/sparc/.

The link failures can be reproduced doing:

   $ echo CONFIG_LEON3=y > default-configs/devices/sparc-softmmu.mak
   $ configure --without-default-devices
   $ ninja qemu-system-sparc
   $ ./qemu-system-sparc -M leon3 -S

or:

   $ echo CONFIG_SUN4M=y > default-configs/devices/sparc-softmmu.mak

Philippe Mathieu-Daudé (4):
   hw/sparc: Allow building without the leon3 machine
   hw/sparc64: Remove unused "hw/char/serial.h" header
   hw/sparc64: Fix code style for checkpatch.pl
   hw/sparc*: Move cpu_check_irqs() to target/sparc/

  target/sparc/cpu.h  |  6 
  hw/sparc/leon3.c| 37 +++-
  hw/sparc/sun4m.c| 32 -
  hw/sparc64/sparc64.c| 63 -
  target/sparc/int32_helper.c | 70 +
  target/sparc/int64_helper.c | 66 ++
  hw/sparc/trace-events   |  4 +--
  hw/sparc64/trace-events |  4 ---
  target/sparc/trace-events   | 12 ---
  9 files changed, 145 insertions(+), 149 deletions(-)


Thanks. I've queued this to my qemu-sparc branch.


ATB,

Mark.

Re: [PATCH v3 0/6] hw/sparc/sun4m: Introduce Sun4mMachineClass to access sun4m_hwdefs

[Mark Cave-Ayland]

On 03/05/2021 18:12, Philippe Mathieu-Daudé wrote:


Missing review: 6

Hi Mark,

This series QOM'ify the sun4m machines.
I need it for a further memory maxsize check.
It is mostly code movement (and the diff-stat is good).

Since v2:
- use static const variable for hwdef (Richard)
- added Richard's R-b tag

Since v1:
- Full rewrite after Mark review

Philippe Mathieu-Daudé (6):
   hw/sparc/sun4m: Have sun4m machines inherit new TYPE_SUN4M_MACHINE
   hw/sparc/sun4m: Introduce Sun4mMachineClass
   hw/sparc/sun4m: Factor out sun4m_machine_class_init()
   hw/sparc/sun4m: Register machine types in sun4m_machine_types[]
   hw/sparc/sun4m: Fix code style for checkpatch.pl
   hw/sparc/sun4m: Move each sun4m_hwdef definition in its class_init

  hw/sparc/sun4m.c | 459 +++
  1 file changed, 186 insertions(+), 273 deletions(-)


Looks good to me:

Reviewed-by: Mark Cave-Ayland 

I've also queued this to my qemu-sparc branch.


ATB,

Mark.

Re: [RFC PATCH 07/27] virtio-snd: Add properties for class init

[Shreyansh Chouhan]

On Wed, 5 May 2021 at 02:00, Laurent Vivier  wrote:

> Hi Shreyansh,
>
> First of all, thank you for your work, I was expecting a virtio sound
> device for some time...
>
> You're welcome :)

> Le 04/05/2021 à 21:35, Shreyansh Chouhan a écrit :
> > On Tue, 4 May 2021 at 19:02, Laurent Vivier  laur...@vivier.eu>> wrote:
> >
> > There is nothing specific to PCI in that code, why do you prevent
> the use of virtio-snd as a MMIO
> > device?
> >
> > I am sorry I do not understand your question completely. If by
> preventing the use of virtio-snd, you
> > mean
> > why did I add the PCI dependencies to the Kconfig file, then I think I
> must have been a bit confused
> > while writing it. VIRTIO_PCI already includes those dependencies, I will
> change the dependency to
> > VIRTIO. (Which is what it is for other virtio devices too.)
> >
> > However if you mean why did I not add an MMIO binding for this device,
> then there is no
> > specific reason. I simply followed what QEMU had been doing for the
> other virtio devices.
> > Will there be any advantages to implementing the device as a MMIO
> device?
>
> No, the question was only about the dependencies, generally a a virtio
> device is binded to a virtio
> bus, and virtio PCI is a PCI card providing a virtio bus with the virtio
> device attached to it.
>
> For instance, for virtio-net-pci:
>
> HOST
>
>   -> PCI Host controller
>
> -> PCI virtio net device (TYPE_VIRTIO_NET_PCI)
>
>   -> virtio Bus (TYPE_VIRTIO_BUS)
>
> -> virtio net device (TYPE_VIRTIO_NET)
>
> TYPE_VIRTIO_NET_PCI is created by hw/virtio/virtio-net-pci.c and
> TYPE_VIRTIO_NET by hw/net/vrtio-net.c:
>
> hw/virtio/meson.build:
>
> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_NET', if_true:
> files('virtio-net-pci.c'))
> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
>
> hw/net/meson.build
> specific_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-net.c'))
>
> hw/net/Kconfig:
>
> config VIRTIO_NET
> bool
> default y
> depends on VIRTIO
>
> So:
>
> the virtio-net device is built when VIRTIO_NET is set,
> the virtio-net-pci device is build when VIRTIO_NET and VIRTIO_PCI are set.
>
> So what I expect for virtio-snd:
>
> hw/virtio/meson.build:
>
> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SND', if_true:
> files('virtio-snd-pci.c'))
>
> hw/audio/meson.build:
>
> softmmu_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: files('virtio-snd.c'))
>
> hw/audio/Kconfig
>
> config VIRTIO_SND
> bool
> default y
> depends on VIRTIO
>
> With that kind of config, a machine without PCI bus will be able to create
> a virtio bus to add your
> virtio device (like s390x with virtio-ccw of any other MMIO machine like
> the virt machines).
>
> Thanks a lot for the detailed explanation. It clarifies everything
regarding the separate
pci and device source files and how they are built, which was confusing me
a little bit.
I will fix the meson.build and Kconfig files in the upcoming versions of
these patches.

In short: update your hw/audio/config, and all will be fine.


>
Thanks,
> Laurent
>
--
Thanks
Shreyansh

Re: [PATCH 1/3] hw/i2c: add support for PMBus

[Philippe Mathieu-Daudé]

Hi Titus,

On 5/4/21 6:28 PM, Titus Rwantare wrote:
> QEMU has support for SMBus devices, and PMBus is a more specific
> implementation of SMBus. The additions made in this commit makes it easier to
> add new PMBus devices to QEMU.
> 
> https://pmbus.org/specification-archives/
> 
> Reviewed-by: Hao Wu 
> Signed-off-by: Titus Rwantare 
> ---
>  hw/arm/Kconfig|1 +
>  hw/i2c/Kconfig|4 +
>  hw/i2c/meson.build|1 +
>  hw/i2c/pmbus_device.c | 1611 +
>  include/hw/i2c/pmbus_device.h |  520 +++
>  5 files changed, 2137 insertions(+)

TBH this is quite a big patch to digest.

Any chance you could split it?

Thanks,

Phil.

Re: [PATCH v4 0/5] target/ppc: Untangle CPU init from translation

[Fabiano Rosas]

"Bruno Larsen (billionai)"  writes:

> Based-on: ppc-for-6.1 tree
>
> This patch series aims to remove the logic of initializing CPU from
> the file related to TCG translation. To achieve this, we have to make
> it so registering SPRs isn't directly tied to TCG, and move code only
> related to translation out of translate_init.c.inc and into translate.c.
> This is in preparation to compile this target without TCG.
>
> Changes for v4:
>  * reordered patches, to make partially applying simpler
>  * removed patches that were already applied
>  * undone creation of spt_tcg.c.inc, now waiting for further cleanup
>  * moved SPR_NOACCESS motion to last patch, and to spr_tcg.h
>
> Changes for v3:
>  * fixed the parameters of _spr_register
>  * remove some redundant #include statements
>  * removed some functions that were mentioned in v2 as unnecessary
>  * added copyright header to relevant files
>  * removed first patch, that was already applied
>  * removed a changed that would add a regression
>
>  Changes for v2:
>  * split and reordered patches, to make it easier to review
>  * improved commit messages 
>  * Undid creation of spr_common, as it was unnecessary
>  * kept more functions as static
>  * ensured that the project builds after every commit
>
> Bruno Larsen (billionai) (5):
>   target/ppc: Fold gen_*_xer into their callers
>   target/ppc: renamed SPR registration functions
>   target/ppc: move SPR R/W callbacks to translate.c
>   target/ppc: turned SPR R/W callbacks not static
>   target/ppc: isolated cpu init from translation logic
>
>  .../ppc/{translate_init.c.inc => cpu_init.c}  | 1848 -
>  target/ppc/meson.build|1 +
>  target/ppc/spr_tcg.h  |  136 ++
>  target/ppc/translate.c| 1072 +-
>  4 files changed, 1598 insertions(+), 1459 deletions(-)
>  rename target/ppc/{translate_init.c.inc => cpu_init.c} (89%)
>  create mode 100644 target/ppc/spr_tcg.h

We're still missing some changes:

- some files (hw/ppc/pnv.c, hw/ppc/spapr.c) use oea_read to check if an
SPR exists. This needs to be changed to something that is present in
both configs (I believe Bruno is working on this).

- The commit 6113563982 ("target/ppc: Clean up _spr_register et al")
from the ppc-for-6.1 branch missed some TCG-specific code in
gen_spr_BookE206:

$ ../configure --target-list=ppc64-softmmu --disable-tcg
$ make
(...)
[193/264] Compiling C object libqemu-ppc64-softmmu.fa.p/target_ppc_cpu_init.c.o
FAILED: libqemu-ppc64-softmmu.fa.p/target_ppc_cpu_init.c.o 
(...)
../target/ppc/cpu_init.c: In function ‘register_BookE206_sprs’:
../target/ppc/cpu_init.c:1207:16: error: variable ‘uea_write’ set but not used 
[-Werror=unused-but-set-variable]
 void (*uea_write)(DisasContext *ctx, int sprn, int gprn) =
^
cc1: all warnings being treated as errors

We need something like:

--- a/target/ppc/translate_init.c.inc   2021-05-04 16:24:53.549556292 -0400
+++ b/target/ppc/translate_init.c.inc   2021-05-04 16:26:41.005280971 -0400
@@ -2025,11 +2025,13 @@
 /* TLB assist registers */
 /* XXX : not implemented */
 for (i = 0; i < 8; i++) {
+#ifdef CONFIG_TCG
 void (*uea_write)(DisasContext *ctx, int sprn, int gprn) =
 _write_generic32;
 if (i == 2 && (mas_mask & (1 << i)) && (env->insns_flags & PPC_64B)) {
 uea_write = _write_generic;
 }
+#endif
 if (mas_mask & (1 << i)) {
 spr_register(env, mas_sprn[i], mas_names[i],
  SPR_NOACCESS, SPR_NOACCESS,
---

Re: [RFC PATCH 07/27] virtio-snd: Add properties for class init

[Laurent Vivier]

Hi Shreyansh,

First of all, thank you for your work, I was expecting a virtio sound device 
for some time...

Le 04/05/2021 à 21:35, Shreyansh Chouhan a écrit :
> On Tue, 4 May 2021 at 19:02, Laurent Vivier  > wrote:
> 
> There is nothing specific to PCI in that code, why do you prevent the use 
> of virtio-snd as a MMIO
> device?
> 
> I am sorry I do not understand your question completely. If by preventing the 
> use of virtio-snd, you
> mean
> why did I add the PCI dependencies to the Kconfig file, then I think I must 
> have been a bit confused
> while writing it. VIRTIO_PCI already includes those dependencies, I will 
> change the dependency to
> VIRTIO. (Which is what it is for other virtio devices too.)
> 
> However if you mean why did I not add an MMIO binding for this device, then 
> there is no
> specific reason. I simply followed what QEMU had been doing for the other 
> virtio devices.
> Will there be any advantages to implementing the device as a MMIO device? 

No, the question was only about the dependencies, generally a a virtio device 
is binded to a virtio
bus, and virtio PCI is a PCI card providing a virtio bus with the virtio device 
attached to it.

For instance, for virtio-net-pci:

HOST

  -> PCI Host controller

-> PCI virtio net device (TYPE_VIRTIO_NET_PCI)

  -> virtio Bus (TYPE_VIRTIO_BUS)

-> virtio net device (TYPE_VIRTIO_NET)

TYPE_VIRTIO_NET_PCI is created by hw/virtio/virtio-net-pci.c and 
TYPE_VIRTIO_NET by hw/net/vrtio-net.c:

hw/virtio/meson.build:

virtio_pci_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-net-pci.c'))
virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)

hw/net/meson.build
specific_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-net.c'))

hw/net/Kconfig:

config VIRTIO_NET
bool
default y
depends on VIRTIO

So:

the virtio-net device is built when VIRTIO_NET is set,
the virtio-net-pci device is build when VIRTIO_NET and VIRTIO_PCI are set.

So what I expect for virtio-snd:

hw/virtio/meson.build:

virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: files('virtio-snd-pci.c'))

hw/audio/meson.build:

softmmu_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: files('virtio-snd.c'))

hw/audio/Kconfig

config VIRTIO_SND
bool
default y
depends on VIRTIO

With that kind of config, a machine without PCI bus will be able to create a 
virtio bus to add your
virtio device (like s390x with virtio-ccw of any other MMIO machine like the 
virt machines).

In short: update your hw/audio/config, and all will be fine.

Thanks,
Laurent

Re: Gitlab Issue Tracker - Proposed Workflow

[Peter Maydell]

On Tue, 4 May 2021 at 19:34, John Snow  wrote:
> # Some concerns on the above scheme:
>
> - "In Progress" is not likely to be used faithfully and will fall out of
> sync often. It's not clear if there should be a difference between a bug
> having an assignee and a bug labeled "In Progress". I don't want to get
> in the business of managing people's time and forcing them to track
> their work.
>
> At the same time, for bugs being actively fixed by a contributor on-list
> who is not [known to be] present on gitlab, it's still possibly a nice
> courtesy to mark a bug as not 'free for the taking'.
>
> Meanwhile, there are several bugs I "own" but I am not actually actively
> working on. These are the sorts of things that I wouldn't mind someone
> taking from me if they wanted to, so the "In Progress" label provides
> some useful distinction there if someone wished to use it.
>
> I think I am inclined to keep it for now, at least until gitlab adoption
> rate is higher and bugs can be assigned more faithfully.

I like "In Progress" because I use it largely to track "I wrote a
patch for this and submitted it to the list, but it hasn't gone in
yet". This means that later on when a release is more imminent I
can easily scan through a list of "in progress" bug reports to find
the "we just forgot to update the bug state when the patch was
committed" ones and the "somebody submitted a patch but it fell
through the cracks" ones. That's a lot harder if you have to look
through the whole pile of "new" bugs.

> - Gitlab will automatically close issues that reference the gitlab issue
> tracker from the commit message, but it won't apply the "Merged" label,
> so it's at risk of falling out of sync.
>
> Closed issues retain their "Workflow::" labels, but won't show up in the
> issue search by default unless you ask to include closed issues as well.
>
> I think we can likely just drop this label, and have bugs go directly
> from whatever state they're in to "Closed". The issue board will look
> cleaner and there's less custodial work for maintainers.

> - Similarly to the above concern, "Released" is extra paperwork for us
> to maintain. I recommend instead we drop the label and begin using
> "Milestones" for each release, and attaching issues we have fixed or
> want to fix to specific Milestones. This will give us a way to track
> pending issues for release candidates as well as a convenient list, in
> the end, of which bugs were fixed in which specific release, which is
> far more useful than a "Released" tag anyway.

I guess so. I dunno how much our old fixed/released distinction
was helping users anyway. I do think that using Milestones both
for "I want to fix this bug for release X" and "this bug is fixed
in release X" is going to mean that it's not reliable for the latter
because we're going to have bugs where somebody optimistically set
the milestone and then the patches didn't land in time for the release.
But unless we care enough to write a bot that auto-updates milestones
we'll just have to live with that.

-- PMM

Re: [PATCH 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved

[Eric Blake]

On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> Split checking for reserved bits out of aligned offset check.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2.h  |  1 +
>  block/qcow2-refcount.c | 10 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits

[Eric Blake]

On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2.h  | 1 +
>  block/qcow2-refcount.c | 6 ++
>  2 files changed, 7 insertions(+)
> 
Reviewed-by: Eric Blake 

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

Re: [PATCH 08/10] qcow2-refcount: improve style of check_refcounts_l1()

[Eric Blake]

On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>  - use g_autofree for l1_table
>  - better name for size in bytes variable
>  - reduce code blocks nesting
>  - whitespaces, braces, newlines
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2-refcount.c | 97 +-
>  1 file changed, 49 insertions(+), 48 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 44fc0dd5dc..eb6de3dabd 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1864,71 +1864,72 @@ static int check_refcounts_l1(BlockDriverState *bs,
>int flags, BdrvCheckMode fix, bool active)
>  {
>  BDRVQcow2State *s = bs->opaque;
> -uint64_t *l1_table = NULL, l2_offset, l1_size2;
> +size_t l1_size_bytes = l1_size * L1E_SIZE;
> +g_autofree uint64_t *l1_table = g_try_malloc(l1_size_bytes);

Note that this now happens...

> +uint64_t l2_offset;
>  int i, ret;
>  
> -l1_size2 = l1_size * L1E_SIZE;
> +if (!l1_size) {
> +return 0;

...before you validate whether l1_size is non-zero, which can result in
g_try_malloc(0).  Probably harmless, but it might be better if you declare
 g_autofree uint64_t *l1_table = NULL;
and then initialize it via malloc only after the sanity check.

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

[Bug 1892081] Re: Performance improvement when using "QEMU_FLATTEN" with softfloat type conversions

[Thomas Huth]

This is an automated cleanup. This bug report has been moved to QEMU's
new bug tracker on gitlab.com and thus gets marked as 'expired' now.
Please continue with the discussion here:

 https://gitlab.com/qemu-project/qemu/-/issues/134


** Changed in: qemu
   Status: In Progress => Expired

** Changed in: qemu
 Assignee: Richard Henderson (rth) => (unassigned)

** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #134
   https://gitlab.com/qemu-project/qemu/-/issues/134

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

Title:
  Performance improvement when using "QEMU_FLATTEN" with softfloat type
  conversions

Status in QEMU:
  Expired

Bug description:
  Attached below is a matrix multiplication program for double data
  types. The program performs the casting operation "(double)rand()"
  when generating random numbers.

  This operation calls the integer to float softfloat conversion
  function "int32_to_float_64".

  Adding the "QEMU_FLATTEN" attribute to the function definition
  decreases the instructions per call of the function by about 63%.

  Attached are before and after performance screenshots from
  KCachegrind.

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

Re: [PATCH 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits

[Eric Blake]

On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2.h  |  1 +
>  block/qcow2-refcount.c | 12 +++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake 

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

[Bug 1828608] Re: Chardev websocket might not support pasting more than a few chars

[Thomas Huth]

This is an automated cleanup. This bug report has been moved to QEMU's
new bug tracker on gitlab.com and thus gets marked as 'expired' now.
Please continue with the discussion here:

 https://gitlab.com/qemu-project/qemu/-/issues/133


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

** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #133
   https://gitlab.com/qemu-project/qemu/-/issues/133

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

Title:
  Chardev websocket might not support pasting more than a few chars

Status in QEMU:
  Expired

Bug description:
  When sending more than 4-5 characters on the websocket serial console
  (with pasting for example), the guest might not receive all of them,
  or worse interpret the input as Magic SysRq keys.

  This might be due to the io loop not checking the backend readiness
  before calling the read function.

  Attached patched fixes the problem on my system. I'm not sure it's the
  proper approach, this is just to start discussion.

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

Re: [PATCH 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap

[Eric Blake]

On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> Check subcluster bitmap of the l2 entry for different types of
> clusters:
> 
>  - for compressed it must be zero
>  - for allocated check consistency of two parts of the bitmap
>  - for unallocated all subclusters should be unallocated
>(or zero-plain)
> 
> For unallocated clusters we can safely fix the entry by making it
> zero-plain.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2-refcount.c | 30 +-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake 

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

Re: [PATCH 05/10] qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap

[Eric Blake]

On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> We'll reuse the function to fix wrong L2 entry bitmap. Support it now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2-refcount.c | 18 +++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index f1e771d742..62d59eb2e9 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1588,7 +1588,8 @@ enum {
>  };
>  
>  /*
> - * Fix L2 entry by making it QCOW2_CLUSTER_ZERO_PLAIN.
> + * Fix L2 entry by making it QCOW2_CLUSTER_ZERO_PLAIN (or maing all its 
> present

making

> + * subclusters QCOW2_SUBCLUSTER_ZERO_PLAIN).
>   *
>   * Function do res->corruptions-- on success, so caller is responsible to do
>   * corresponding res->corruptions++ prior to the call.
> @@ -1605,9 +1606,20 @@ static int fix_l2_entry_by_zero(BlockDriverState *bs, 
> BdrvCheckResult *res,
>  int idx = l2_index * (l2_entry_size(s) / sizeof(uint64_t));
>  uint64_t l2e_offset = l2_offset + (uint64_t)l2_index * l2_entry_size(s);
>  int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2;
> -uint64_t l2_entry = has_subclusters(s) ? 0 : QCOW_OFLAG_ZERO;
>  
> -set_l2_entry(s, l2_table, l2_index, l2_entry);
> +if (has_subclusters(s)) {
> +uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, l2_index);
> +
> +/* Allocated subclusters becomes zero */

become

> +l2_bitmap |= l2_bitmap << 32;
> +l2_bitmap &= QCOW_L2_BITMAP_ALL_ZEROES;
> +
> +set_l2_bitmap(s, l2_table, l2_index, l2_bitmap);
> +set_l2_entry(s, l2_table, l2_index, 0);
> +} else {
> +set_l2_entry(s, l2_table, l2_index, QCOW_OFLAG_ZERO);
> +}
> +
>  ret = qcow2_pre_write_overlap_check(bs, ign, l2e_offset, 
> l2_entry_size(s),
>  false);
>  if (metadata_overlap) {
> 

Reviewed-by: Eric Blake 

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

[Bug 1861404] Re: AVX instruction VMOVDQU implementation error for YMM registers

[Thomas Huth]

This is an automated cleanup. This bug report has been moved to QEMU's
new bug tracker on gitlab.com and thus gets marked as 'expired' now.
Please continue with the discussion here:

 https://gitlab.com/qemu-project/qemu/-/issues/132


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

** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #132
   https://gitlab.com/qemu-project/qemu/-/issues/132

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

Title:
  AVX instruction VMOVDQU implementation error for YMM registers

Status in QEMU:
  Expired

Bug description:
  Hi,

  Tested with Qemu 4.2.0, and with git version
  bddff6f6787c916b0e9d63ef9e4d442114257739.

  The x86 AVX instruction VMOVDQU doesn't work properly with YMM registers (32 
bytes).
  It works with XMM registers (16 bytes) though.

  See the attached test case `ymm.c`: when copying from memory-to-ymm0
  and then back from ymm0-to-memory using VMOVDQU, Qemu only copies the
  first 16 of the total 32 bytes.

  ```
  user@ubuntu ~/Qemu % gcc -o ymm ymm.c -Wall -Wextra -Werror

  user@ubuntu ~/Qemu % ./ymm
  00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F 10 11 12 13 14 15 16 17 18 19 
1A 1B 1C 1D 1E 1F

  user@ubuntu ~/Qemu % ./x86_64-linux-user/qemu-x86_64 -cpu max ymm
  00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00
  ```

  This seems to be because in `translate.c > gen_sse()`, the case
  handling the VMOVDQU instruction calls `gen_ldo_env_A0` which always
  performs a 16 bytes copy using two 8 bytes load and store operations
  (with `tcg_gen_qemu_ld_i64` and `tcg_gen_st_i64`).

  Instead, the `gen_ldo_env_A0` function should generate a copy with a
  size corresponding to the used register.

  
  ```
  static void gen_sse(CPUX86State *env, DisasContext *s, int b,
  target_ulong pc_start, int rex_r)
  {
  [...]
  case 0x26f: /* movdqu xmm, ea */
  if (mod != 3) {
  gen_lea_modrm(env, s, modrm);
  gen_ldo_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]));
  } else { 
  [...]
  ```

  ```
  static inline void gen_ldo_env_A0(DisasContext *s, int offset)
  {
  int mem_index = s->mem_index;
  tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0, mem_index, MO_LEQ);
  tcg_gen_st_i64(s->tmp1_i64, cpu_env, offset + offsetof(ZMMReg, ZMM_Q(0)));
  tcg_gen_addi_tl(s->tmp0, s->A0, 8);
  tcg_gen_qemu_ld_i64(s->tmp1_i64, s->tmp0, mem_index, MO_LEQ);
  tcg_gen_st_i64(s->tmp1_i64, cpu_env, offset + offsetof(ZMMReg, ZMM_Q(1)));
  }
  ```

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

Re: [RFC PATCH 07/27] virtio-snd: Add properties for class init

[Shreyansh Chouhan]

On Tue, 4 May 2021 at 19:02, Laurent Vivier  wrote:

> There is nothing specific to PCI in that code, why do you prevent the use
> of virtio-snd as a MMIO
> device?
>
> I am sorry I do not understand your question completely. If by preventing
the use of virtio-snd, you mean
why did I add the PCI dependencies to the Kconfig file, then I think I must
have been a bit confused
while writing it. VIRTIO_PCI already includes those dependencies, I will
change the dependency to
VIRTIO. (Which is what it is for other virtio devices too.)

However if you mean why did I not add an MMIO binding for this device, then
there is no
specific reason. I simply followed what QEMU had been doing for the other
virtio devices.
Will there be any advantages to implementing the device as a MMIO device?

> Thanks,
>
> Laurent
>
> --
Regards
Shreyansh

Re: [PATCH 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero()

[Eric Blake]

On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> Split fix_l2_entry_by_zero() out of check_refcounts_l2() to be
> reused in further patch.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2-refcount.c | 87 +-
>  1 file changed, 60 insertions(+), 27 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 66cbb94ef9..f1e771d742 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1587,6 +1587,54 @@ enum {
>  CHECK_FRAG_INFO = 0x2,  /* update BlockFragInfo counters */
>  };
>  
> +/*
> + * Fix L2 entry by making it QCOW2_CLUSTER_ZERO_PLAIN.
> + *
> + * Function do res->corruptions-- on success, so caller is responsible to do
> + * corresponding res->corruptions++ prior to the call.

This function decrements res->corruptions on success, so the caller is
responsible to increment res->corruptions prior to the call.

> + *
> + * On failure in-memory @l2_table may be modified.
> + */
> +static int fix_l2_entry_by_zero(BlockDriverState *bs, BdrvCheckResult *res,
> +uint64_t l2_offset,
> +uint64_t *l2_table, int l2_index, bool 
> active,
> +bool *metadata_overlap)

Otherwise seems okay.
Reviewed-by: Eric Blake 

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

[Bug 1658141] Re: QEMU's default msrs handling causes Windows 10 64 bit to crash

[Thomas Huth]

This is an automated cleanup. This bug report has been moved to QEMU's
new bug tracker on gitlab.com and thus gets marked as 'expired' now.
Please continue with the discussion here:

 https://gitlab.com/qemu-project/qemu/-/issues/131


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

** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #131
   https://gitlab.com/qemu-project/qemu/-/issues/131

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

Title:
  QEMU's default msrs handling causes Windows 10 64 bit to crash

Status in QEMU:
  Expired

Bug description:
  Wine uses QEMU to run its conformance test suite on Windows virtual
  machines. Wine's conformance tests check the behavior of various
  Windows APIs and verify that they behave as expected.

  One such test checks handling of exceptions down. When run on Windows 10 64 
bit in QEMU it triggers a "KMOD_EXCEPTION_NOT_HANDLED" BSOD in the VM. See:
  https://bugs.winehq.org/show_bug.cgi?id=40240

  
  To reproduce this bug:
  * Pick a Windows 10 64 bit VM on an Intel host.

  * Start the VM. I'm pretty sure any qemu command will do but here's what I 
used:
qemu-system-x86_64 -machine pc-i440fx-2.1,accel=kvm -cpu core2duo,+nx -m 
2048 -hda /var/lib/libvirt/images/wtbw1064.qcow2

  * Grab the attached source code. The tar file is a bit big at 85KB
  because I had to include some Wine headers. However the source file
  proper, exception.c, is only 85 lines, including the LGPL header.

  * Compile the source code with MinGW by typing 'make'. This produces a
  32 bit exception.exe executable. I'll attach it for good measure.

  * Put exception.exe on the VM and run it.

  
  After investigation it turns out this happens:
   * Only for Windows 10 64 bit guests. Windows 10 32 bit and older Windows 
versions are unaffected.

   * Only on Intel hosts. At least both my Xeon E3-1226 v3 and i7-4790K
  hosts are impacted but not my Opteron 6128 one.

   * It does not seem to depend on the emulated CPU type: on the Intel hosts 
this happened with both 
  core2duo,nx and 'copy the host configuration' and did not depend on the 
number of emulated cpus/cores.

   * This happened with both QEMU 2.1 and 2.7, and both the 3.16.0 and
  4.8.11 Linux kernels, both on Debian 8.6 and Debian Testing.

  
  After searching for quite some time I discovered that the kvm kernel module 
was sneaking the following messages into /var/log/syslog precisely when the 
BSOD happens:

  Dec 16 13:43:48 vm3 kernel: [  191.624802] kvm [2064]: vcpu0, guest rIP: 
0xf803cb3c0bf3 kvm_set_msr_common: MSR_IA32_DEBUGCTLMSR 0x1, nop
  Dec 16 13:43:48 vm3 kernel: [  191.624835] kvm [2064]: vcpu0, guest rIP: 
0xf803cb3c0c5c unhandled rdmsr: 0x1c9

  A search on the Internet turned up a post suggesting to change kvm's
  ignore_msrs setting:

 echo 1 >/sys/module/kvm/parameters/ignore_msrs

  
https://www.reddit.com/r/VFIO/comments/42dj7n/some_games_crash_to_biosboot_on_launch/

  This does actually work and provides a workaround at least.

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

Re: [PATCH 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper

[Eric Blake]

On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add helper to parse compressed l2_entry and use it everywhere instead
> of opencoding.

open-coding

> 
> Note, that in most places we move to precise coffset/csize instead of
> sector-aligned. Still it should work good enough for updating
> refcounts.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2.h  |  3 ++-
>  block/qcow2-cluster.c  | 15 +++
>  block/qcow2-refcount.c | 36 +---
>  block/qcow2.c  |  9 ++---
>  4 files changed, 36 insertions(+), 27 deletions(-)
> 

Reviewed-by: Eric Blake 

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

[Bug 1868116] Re: QEMU monitor no longer works

[Thomas Huth]

Looking at the comments, it seems to me that this was an issue in VTE
that got fixed. Is there still anything left to do for upstream QEMU
here?

** Changed in: qemu
   Status: New => 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/1868116

Title:
  QEMU monitor no longer works

Status in QEMU:
  Incomplete
Status in qemu package in Ubuntu:
  Invalid
Status in vte2.91 package in Ubuntu:
  Fix Released
Status in qemu package in Debian:
  Fix Released

Bug description:
  Repro:
  VTE
  $ meson _build && ninja -C _build && ninja -C _build install

  qemu:
  $ ../configure --python=/usr/bin/python3 --disable-werror --disable-user 
--disable-linux-user --disable-docs --disable-guest-agent --disable-sdl 
--enable-gtk --disable-vnc --disable-xen --disable-brlapi --disable-fdt 
--disable-hax --disable-vde --disable-netmap --disable-rbd --disable-libiscsi 
--disable-libnfs --disable-smartcard --disable-libusb --disable-usb-redir 
--disable-seccomp --disable-glusterfs --disable-tpm --disable-numa 
--disable-opengl --disable-virglrenderer --disable-xfsctl --disable-vxhs 
--disable-slirp --disable-blobs --target-list=x86_64-softmmu --disable-rdma 
--disable-pvrdma --disable-attr --disable-vhost-net --disable-vhost-vsock 
--disable-vhost-scsi --disable-vhost-crypto --disable-vhost-user 
--disable-spice --disable-qom-cast-debug --disable-vxhs --disable-bochs 
--disable-cloop --disable-dmg --disable-qcow1 --disable-vdi --disable-vvfat 
--disable-qed --disable-parallels --disable-sheepdog --disable-avx2 
--disable-nettle --disable-gnutls --disable-capstone --disable-tools 
--disable-libpmem --disable-iconv --disable-cap-ng
  $ make

  Test:
  $ LD_LIBRARY_PATH=/usr/local/lib/x86_64-linux-gnu/:$LD_LIBRARY_PATH 
./build/x86_64-softmmu/qemu-system-x86_64 -enable-kvm --drive 
media=cdrom,file=http://archive.ubuntu.com/ubuntu/dists/bionic/main/installer-amd64/current/images/netboot/mini.iso
  - switch to monitor with CTRL+ALT+2
  - try to enter something

  Affects head of both usptream git repos.

  
  --- original bug ---

  It was observed that the QEMU console (normally accessible using
  Ctrl+Alt+2) accepts no input, so it can't be used. This is being
  problematic because there are cases where it's required to send
  commands to the guest, or key combinations that the host would grab
  (as Ctrl-Alt-F1 or Alt-F4).

  ProblemType: Bug
  DistroRelease: Ubuntu 20.04
  Package: qemu 1:4.2-3ubuntu2
  Uname: Linux 5.6.0-rc6+ x86_64
  ApportVersion: 2.20.11-0ubuntu20
  Architecture: amd64
  CurrentDesktop: XFCE
  Date: Thu Mar 19 12:16:31 2020
  Dependencies:

  InstallationDate: Installed on 2017-06-13 (1009 days ago)
  InstallationMedia: Xubuntu 17.04 "Zesty Zapus" - Release amd64 (20170412)
  KvmCmdLine:
   COMMAND STAT  EUID  RUID PIDPPID %CPU COMMAND
   qemu-system-x86 Sl+   1000  1000   34275   25235 29.2 qemu-system-x86_64 -m 
4G -cpu Skylake-Client -device virtio-vga,virgl=true,xres=1280,yres=720 -accel 
kvm -device nec-usb-xhci -serial vc -serial stdio -hda 
/home/usuario/Sistemas/androidx86.img -display gtk,gl=on -device usb-audio
   kvm-nx-lpage-re S0 0   34284   2  0.0 [kvm-nx-lpage-re]
   kvm-pit/34275   S0 0   34286   2  0.0 [kvm-pit/34275]
  MachineType: LENOVO 80UG
  ProcKernelCmdLine: BOOT_IMAGE=/boot/vmlinuz-5.6.0-rc6+ 
root=UUID=6b4ae5c0-c78c-49a6-a1ba-029192618a7a ro quiet ro kvm.ignore_msrs=1 
kvm.report_ignored_msrs=0 kvm.halt_poll_ns=0 kvm.halt_poll_ns_grow=0 
i915.enable_gvt=1 i915.fastboot=1 cgroup_enable=memory swapaccount=1 
zswap.enabled=1 zswap.zpool=z3fold 
resume=UUID=a82e38a0-8d20-49dd-9cbd-de7216b589fc log_buf_len=16M 
usbhid.quirks=0x0079:0x0006:0x10 config_scsi_mq_default=y 
scsi_mod.use_blk_mq=1 mtrr_gran_size=64M mtrr_chunk_size=64M nbd.nbds_max=2 
nbd.max_part=63
  SourcePackage: qemu
  UpgradeStatus: Upgraded to focal on 2019-12-22 (87 days ago)
  dmi.bios.date: 08/09/2018
  dmi.bios.vendor: LENOVO
  dmi.bios.version: 0XCN45WW
  dmi.board.asset.tag: NO Asset Tag
  dmi.board.name: Toronto 4A2
  dmi.board.vendor: LENOVO
  dmi.board.version: SDK0J40679 WIN
  dmi.chassis.asset.tag: NO Asset Tag
  dmi.chassis.type: 10
  dmi.chassis.vendor: LENOVO
  dmi.chassis.version: Lenovo ideapad 310-14ISK
  dmi.modalias: 
dmi:bvnLENOVO:bvr0XCN45WW:bd08/09/2018:svnLENOVO:pn80UG:pvrLenovoideapad310-14ISK:rvnLENOVO:rnToronto4A2:rvrSDK0J40679WIN:cvnLENOVO:ct10:cvrLenovoideapad310-14ISK:
  dmi.product.family: IDEAPAD
  dmi.product.name: 80UG
  dmi.product.sku: LENOVO_MT_80UG_BU_idea_FM_Lenovo ideapad 310-14ISK
  dmi.product.version: Lenovo ideapad 310-14ISK
  dmi.sys.vendor: LENOVO
  mtime.conffile..etc.apport.crashdb.conf: 2019-08-29T08:39:36.787240

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

Re: [PATCH 02/10] qcow2: compressed read: simplify cluster descriptor passing

[Eric Blake]

On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> Let's pass the whole L2 entry and not bother with
> L2E_COMPRESSED_OFFSET_SIZE_MASK.
> 
> It also helps further refactoring that adds generic
> qcow2_parse_compressed_l2_entry() helper.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2.h |  1 -
>  block/qcow2-cluster.c |  5 ++---
>  block/qcow2.c | 12 +++-
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 

Reviewed-by: Eric Blake 

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

Re: [PATCH 01/10] qcow2-refcount: improve style of check_refcounts_l2()

[Eric Blake]

On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>  - don't use same name for size in bytes and in entries
>  - use g_autofree for l2_table
>  - add whitespace
>  - fix block comment style
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2-refcount.c | 47 +-
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 

Reviewed-by: Eric Blake 

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

Re: [PATCH] migration: do not restart VM after successful snapshot-load

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonz...@redhat.com) wrote:
> The HMP loadvm code is calling load_snapshot rather than
> qmp_snapshot_load, in order to bypass the job infrastructure.  The code
> around it is almost the same, with one difference: hmp_loadvm is
> restarting the VM if load_snapshot fails, qmp_snapshot_load is doing so
> if load_snapshot succeeds.
> 
> Fix the bug in QMP by moving the common code to load_snapshot.

So I agree it's nice to have them consistent, but hmm.

> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Paolo Bonzini 
> ---
>  migration/savevm.c | 16 
>  monitor/hmp-cmds.c |  7 +--
>  2 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 52e2d72e4b..a899191cbf 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2992,6 +2992,7 @@ bool load_snapshot(const char *name, const char 
> *vmstate,
>  int ret;
>  AioContext *aio_context;
>  MigrationIncomingState *mis = migration_incoming_get_current();
> +int saved_vm_running  = runstate_is_running();
>  
>  if (!bdrv_all_can_snapshot(has_devices, devices, errp)) {
>  return false;
> @@ -3024,6 +3025,8 @@ bool load_snapshot(const char *name, const char 
> *vmstate,
>  return false;
>  }
>  
> +vm_stop(RUN_STATE_RESTORE_VM);
> +
>  /*
>   * Flush the record/replay queue. Now the VM state is going
>   * to change. Therefore we don't need to preserve its consistency
> @@ -3061,13 +3064,17 @@ bool load_snapshot(const char *name, const char 
> *vmstate,
>  
>  if (ret < 0) {
>  error_setg(errp, "Error %d while loading VM state", ret);
> -return false;
> +goto err_restart;
>  }

I don't think this is safe.
If qemu_loadvm_state(f) fails, depending on the point that it fails,
the state of the VM is in a part loaded, indeterminate state - it
doesn't seem right to auto-restart it.

Note, that's a destinct failure from the earlier failures, e.g. trying
to find the snapshot and noticing it doesn't exist - that's OK to
restart the VM.

Then there's the question of what to do if the load_snapshot succeeds;
qmp's behaviour of running the loaded-VM doesn't seem wrong to me;
although you'd think that would be based on the loaded state not the
original; but that's probably a different question.

Dave

>  return true;
>  
>  err_drain:
>  bdrv_drain_all_end();
> +err_restart:
> +if (saved_vm_running) {
> +vm_start();
> +}
>  return false;
>  }
>  
> @@ -3135,17 +3142,10 @@ static void snapshot_load_job_bh(void *opaque)
>  {
>  Job *job = opaque;
>  SnapshotJob *s = container_of(job, SnapshotJob, common);
> -int orig_vm_running;
>  
>  job_progress_set_remaining(>common, 1);
>  
> -orig_vm_running = runstate_is_running();
> -vm_stop(RUN_STATE_RESTORE_VM);
> -
>  s->ret = load_snapshot(s->tag, s->vmstate, true, s->devices, s->errp);
> -if (s->ret && orig_vm_running) {
> -vm_start();
> -}
>  
>  job_progress_update(>common, 1);
>  
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 0ad5b77477..a39436c8cb 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1127,15 +1127,10 @@ void hmp_balloon(Monitor *mon, const QDict *qdict)
>  
>  void hmp_loadvm(Monitor *mon, const QDict *qdict)
>  {
> -int saved_vm_running  = runstate_is_running();
>  const char *name = qdict_get_str(qdict, "name");
>  Error *err = NULL;
>  
> -vm_stop(RUN_STATE_RESTORE_VM);
> -
> -if (!load_snapshot(name, NULL, false, NULL, ) && saved_vm_running) {
> -vm_start();
> -}
> +load_snapshot(name, NULL, false, NULL, );
>  hmp_handle_error(mon, err);
>  }
>  
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: Gitlab Issue Tracker - Proposed Workflow

[Daniel P . Berrangé]

On Tue, May 04, 2021 at 02:33:58PM -0400, John Snow wrote:
> I'm seeking feedback on our Gitlab issue handling workflow.
> (There's a TLDR at the bottom of the mail.)
> 
> 
> # Background:
> 
> Last KVM Forum, I started experimenting with the Gitlab issue tracker:
> https://gitlab.com/qemu-project/qemu/-/issues
> 
> At the time, I hastily added some labels to roughly approximate our
> Launchpad issue lifecycle.
> 
> Gitlab offers "Scoped" labels that allow us to set a 'category' of labels
> where only one label from that category at a time may be applied.
> 
> I created the "Workflow::" scope to create labels for our issue lifecycle.
> You can see how this looks like by visiting our issue board:
> https://gitlab.com/qemu-project/qemu/-/boards
> 
> Workflow labels can be changed by manually setting or removing tags, OR by
> clicking and dragging issues from one column of the board to another.
> 
> 
> # The Workflow:: states we have currently:
> 
> https://gitlab.com/qemu-project/qemu/-/labels?utf8=%E2%9C%93==workflow
> 
> - [Open] - For newly created issues. These show up in the far-left panel on
> the boards view and allow us to quickly see which bugs have not been
> assigned a Workflow:: label. The intent is to keep this list as empty as
> possible, aggressively moving things into "Needs Info" or "Triaged" lists.
> 
> - Needs Info: For issues that either cannot be triaged or cannot be verified
> because they lack information. The intent is to allow the CI Janitor to
> close such issues after 60 days of inactivity.
> 
> - Triaged: For issues that have been classified (Bug/Feature), and sorted
> into their appropriate topic label(s) -- e.g. Storage, Audio, accel: TCG,
> etc. The intent is that subsystem maintainers will subscribe to relevant
> topics and either re-triage, kick it back to Needs Info, assign someone,
> etc.
> 
> - In Progress: For issues that a developer is actively working on. The
> intent was to be able to mark bugs in such a way that contributors would not
> assume they are available to be hacked on. For instance, if a non-gitlab
> contributor submits patches for an issue, we might mark it as In Progress so
> that it does not appear to be going unaddressed.

If someone is actively working on it, they could just stick a comment
on the issue, possibly with a mailing list posting.

With a simple "in progress" flag you can end up marking things as if
someone is working on it, but then they go off and do other things.

If you just have a mailing list posting/comment, you can at least
see how up2date that comment was and judge whether the person is
still caring about the bug.

> 
> - Merged: Intended for issues that have had code that is merged, but not yet
> released. This was done to mimic our Launchpad workflow.
> 
> - Released: Intended for issues that have had been fixed and packaged in a
> release. This was also added to mimic Launchpad.
> 
> - [Closed] - For issues that are resolved in one way or another.
> 
> 
> # Some concerns on the above scheme:
> 
> - "In Progress" is not likely to be used faithfully and will fall out of
> sync often. It's not clear if there should be a difference between a bug
> having an assignee and a bug labeled "In Progress". I don't want to get in
> the business of managing people's time and forcing them to track their work.
> 
> At the same time, for bugs being actively fixed by a contributor on-list who
> is not [known to be] present on gitlab, it's still possibly a nice courtesy
> to mark a bug as not 'free for the taking'.
> 
> Meanwhile, there are several bugs I "own" but I am not actually actively
> working on. These are the sorts of things that I wouldn't mind someone
> taking from me if they wanted to, so the "In Progress" label provides some
> useful distinction there if someone wished to use it.
> 
> I think I am inclined to keep it for now, at least until gitlab adoption
> rate is higher and bugs can be assigned more faithfully.
> 
> 
> - Gitlab will automatically close issues that reference the gitlab issue
> tracker from the commit message, but it won't apply the "Merged" label, so
> it's at risk of falling out of sync.
> 
> Closed issues retain their "Workflow::" labels, but won't show up in the
> issue search by default unless you ask to include closed issues as well.
> 
> I think we can likely just drop this label, and have bugs go directly from
> whatever state they're in to "Closed". The issue board will look cleaner and
> there's less custodial work for maintainers.
> 
> 
> - Similarly to the above concern, "Released" is extra paperwork for us to
> maintain. I recommend instead we drop the label and begin using "Milestones"
> for each release, and attaching issues we have fixed or want to fix to
> specific Milestones. This will give us a way to track pending issues for
> release candidates as well as a convenient list, in the end, of which bugs
> were fixed in which specific release, which is far more useful than a
> "Released" tag anyway.

Gitlab Issue Tracker - Proposed Workflow

[John Snow]

I'm seeking feedback on our Gitlab issue handling workflow.
(There's a TLDR at the bottom of the mail.)


# Background:

Last KVM Forum, I started experimenting with the Gitlab issue tracker:
https://gitlab.com/qemu-project/qemu/-/issues

At the time, I hastily added some labels to roughly approximate our 
Launchpad issue lifecycle.


Gitlab offers "Scoped" labels that allow us to set a 'category' of 
labels where only one label from that category at a time may be applied.


I created the "Workflow::" scope to create labels for our issue 
lifecycle. You can see how this looks like by visiting our issue board:

https://gitlab.com/qemu-project/qemu/-/boards

Workflow labels can be changed by manually setting or removing tags, OR 
by clicking and dragging issues from one column of the board to another.



# The Workflow:: states we have currently:

https://gitlab.com/qemu-project/qemu/-/labels?utf8=%E2%9C%93==workflow

- [Open] - For newly created issues. These show up in the far-left panel 
on the boards view and allow us to quickly see which bugs have not been 
assigned a Workflow:: label. The intent is to keep this list as empty as 
possible, aggressively moving things into "Needs Info" or "Triaged" lists.


- Needs Info: For issues that either cannot be triaged or cannot be 
verified because they lack information. The intent is to allow the CI 
Janitor to close such issues after 60 days of inactivity.


- Triaged: For issues that have been classified (Bug/Feature), and 
sorted into their appropriate topic label(s) -- e.g. Storage, Audio, 
accel: TCG, etc. The intent is that subsystem maintainers will subscribe 
to relevant topics and either re-triage, kick it back to Needs Info, 
assign someone, etc.


- In Progress: For issues that a developer is actively working on. The 
intent was to be able to mark bugs in such a way that contributors would 
not assume they are available to be hacked on. For instance, if a 
non-gitlab contributor submits patches for an issue, we might mark it as 
In Progress so that it does not appear to be going unaddressed.


- Merged: Intended for issues that have had code that is merged, but not 
yet released. This was done to mimic our Launchpad workflow.


- Released: Intended for issues that have had been fixed and packaged in 
a release. This was also added to mimic Launchpad.


- [Closed] - For issues that are resolved in one way or another.


# Some concerns on the above scheme:

- "In Progress" is not likely to be used faithfully and will fall out of 
sync often. It's not clear if there should be a difference between a bug 
having an assignee and a bug labeled "In Progress". I don't want to get 
in the business of managing people's time and forcing them to track 
their work.


At the same time, for bugs being actively fixed by a contributor on-list 
who is not [known to be] present on gitlab, it's still possibly a nice 
courtesy to mark a bug as not 'free for the taking'.


Meanwhile, there are several bugs I "own" but I am not actually actively 
working on. These are the sorts of things that I wouldn't mind someone 
taking from me if they wanted to, so the "In Progress" label provides 
some useful distinction there if someone wished to use it.


I think I am inclined to keep it for now, at least until gitlab adoption 
rate is higher and bugs can be assigned more faithfully.



- Gitlab will automatically close issues that reference the gitlab issue 
tracker from the commit message, but it won't apply the "Merged" label, 
so it's at risk of falling out of sync.


Closed issues retain their "Workflow::" labels, but won't show up in the 
issue search by default unless you ask to include closed issues as well.


I think we can likely just drop this label, and have bugs go directly 
from whatever state they're in to "Closed". The issue board will look 
cleaner and there's less custodial work for maintainers.



- Similarly to the above concern, "Released" is extra paperwork for us 
to maintain. I recommend instead we drop the label and begin using 
"Milestones" for each release, and attaching issues we have fixed or 
want to fix to specific Milestones. This will give us a way to track 
pending issues for release candidates as well as a convenient list, in 
the end, of which bugs were fixed in which specific release, which is 
far more useful than a "Released" tag anyway.



- "Triaged" is a very lightweight label that I created only to mean that 
the bug has been seen and tagged, which will hopefully deliver it to 
someone's queue that they are paying attention to. It does not 
necessarily mean "Tested and Confirmed". It's possible we may want a new 
label "Confirmed" to help us sort through reports in a way that's a 
little more meaningful than just "triaged".



# TLDR:

- Let's keep "Workflow::In Progress", but acknowledge it as informative
  instead of exhaustive.
- Let's drop "Workflow::Merged" and just rely on the auto-close feature.
- Let's drop "Workflow::Released" 

Re: [RFC PATCH v2 2/2] hw/ppc: Moved TCG code to spapr_hcall_tcg

[Lucas Mateus Martins Araujo e Castro]

On 03/05/2021 01:34, David Gibson wrote:

On Fri, Apr 30, 2021 at 03:40:47PM -0300, Lucas Mateus Castro (alqotel) wrote:

Moved h_enter, remove_hpte, h_remove, h_bulk_remove,h_protect and
h_read to spapr_hcall_tcg.c, added h_tcg_only to be used in a !TCG
environment in spapr_hcall.c and changed build options to only build
spapr_hcall_tcg.c when CONFIG_TCG is enabled.

This looks good.  I'd suggest the name 'spapr_softmmu.c' instead,
which more specifically describes what's special about these
functions.

h_resize_hpt_prepare(), h_resize_hpt_commit() and the functions they
depend on belong in the softmmu set as well.


Moved these hcalls to spapr_softmmu.c as well, along with the most
functions they depend on, but 1 function (push_sregs_to_kvm_pr) is
also used by hcalls in spapr_hcall.c, so for now I'm just leaving it in
spapr_hcall.c and exporting it to be used in spapr_softmmu.c.

On a related note, from what I've seen h_resize_hpt_prepare and
h_resize_hpt_commit are not implementede in KVM, so they're only
called when there's softmmu so that's why they can be moved to
spapr_softmmu.c?

Added the function h_tcg_only to be used for hypercalls that should be
called only in TCG environment but have been called in a TCG-less
one.

Again, 'h_softmmu' would be a better name.

Ok, I will use that name.

Right now, a #ifndef is used to know if there is a need of a h_tcg_only
function to be implemented and used as hypercalls, I initially thought
of always having that option turned on and having spapr_hcall_tcg.c
overwrite those hypercalls when TCG is enabled, but
spapr_register_hypercalls checks if a hypercall already exists for that
opcode so that wouldn't work, so if anyone has any suggestions I'm
interested.

The ifdef is fine.  We don't want to litter the code with them, but a
few is fine.  Especially in this context where it's pretty clearly
just excluding some things from a simple list of calls.


Also spapr_hcall_tcg.c only has 2 duplicated functions (valid_ptex and
is_ram_address), what is the advised way to deal with these
duplications?

valid_ptex() is only used by softmmu functions that are moving, so it
should travel with them, no need for duplication.  is_ram_address() is
also used by h_page_init() which is also needed in !TCG code.  So,
leave it in spapr_hcall.c and just export it for use in the TCG only
code.


On both is_ram_address and push_sregs_to_kvm_pr I exported them

by adding their prototypes to spapr.h and made them non-static.


Signed-off-by: Lucas Mateus Castro (alqotel)
---
  hw/ppc/meson.build   |   3 +
  hw/ppc/spapr_hcall.c | 300 ++
  hw/ppc/spapr_hcall_tcg.c | 343 +++
  3 files changed, 363 insertions(+), 283 deletions(-)
  create mode 100644 hw/ppc/spapr_hcall_tcg.c

diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index 218631c883..3c7f2f08b7 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -29,6 +29,9 @@ ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files(
'spapr_numa.c',
'pef.c',
  ))
+ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_TCG'], if_true: files(
+  'spapr_hcall_tcg.c'
+))
  ppc_ss.add(when: 'CONFIG_SPAPR_RNG', if_true: files('spapr_rng.c'))
  ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_LINUX'], if_true: files(
'spapr_pci_vfio.c',
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 4b0ba69841..b37fbdc32e 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -22,6 +22,15 @@
  #include "mmu-book3s-v3.h"
  #include "hw/mem/memory-device.h"
  
+#ifndef CONFIG_TCG

+static target_ulong h_tcg_only(PowerPCCPU *cpu, SpaprMachineState *spapr,
+target_ulong opcode, target_ulong *args)
+{
+g_assert_not_reached();
+return 0;
+}
+#endif /* !CONFIG_TCG */
+
  static bool has_spr(PowerPCCPU *cpu, int spr)
  {
  /* We can test whether the SPR is defined by checking for a valid name */
@@ -55,284 +64,6 @@ static bool is_ram_address(SpaprMachineState *spapr, hwaddr 
addr)
  return false;
  }
  
-static target_ulong h_enter(PowerPCCPU *cpu, SpaprMachineState *spapr,

-target_ulong opcode, target_ulong *args)
-{
-target_ulong flags = args[0];
-target_ulong ptex = args[1];
-target_ulong pteh = args[2];
-target_ulong ptel = args[3];
-unsigned apshift;
-target_ulong raddr;
-target_ulong slot;
-const ppc_hash_pte64_t *hptes;
-
-apshift = ppc_hash64_hpte_page_shift_noslb(cpu, pteh, ptel);
-if (!apshift) {
-/* Bad page size encoding */
-return H_PARAMETER;
-}
-
-raddr = (ptel & HPTE64_R_RPN) & ~((1ULL << apshift) - 1);
-
-if (is_ram_address(spapr, raddr)) {
-/* Regular RAM - should have WIMG=0010 */
-if ((ptel & HPTE64_R_WIMG) != HPTE64_R_M) {
-return H_PARAMETER;
-}
-} else {
-target_ulong wimg_flags;
-/* Looks like an IO address */
-/* FIXME: What WIMG combinations could be 

Re: [PATCH v11 5/6] KVM: arm64: ioctl to fetch/store tags in a guest

[Catalin Marinas]

On Thu, Apr 29, 2021 at 05:06:07PM +0100, Steven Price wrote:
> On 27/04/2021 18:58, Catalin Marinas wrote:
> > On Fri, Apr 16, 2021 at 04:43:08PM +0100, Steven Price wrote:
> > > diff --git a/arch/arm64/include/uapi/asm/kvm.h 
> > > b/arch/arm64/include/uapi/asm/kvm.h
> > > index 24223adae150..2b85a047c37d 100644
> > > --- a/arch/arm64/include/uapi/asm/kvm.h
> > > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > > @@ -184,6 +184,20 @@ struct kvm_vcpu_events {
> > >   __u32 reserved[12];
> > >   };
> > > +struct kvm_arm_copy_mte_tags {
> > > + __u64 guest_ipa;
> > > + __u64 length;
> > > + union {
> > > + void __user *addr;
> > > + __u64 padding;
> > > + };
> > > + __u64 flags;
> > > + __u64 reserved[2];
> > > +};
[...]
> > Maybe add the two reserved
> > values to the union in case we want to store something else in the
> > future.
> 
> I'm not sure what you mean here. What would the reserved fields be unioned
> with? And surely they are no longer reserved in that case?

In case you want to keep the structure size the same for future
expansion and the expansion only happens via the union, you'd add some
padding in there just in case. We do this for struct siginfo with an
_si_pad[] array in the union.

-- 
Catalin

Re: [PATCH v11 2/6] arm64: kvm: Introduce MTE VM feature

[Catalin Marinas]

On Thu, Apr 29, 2021 at 05:06:41PM +0100, Steven Price wrote:
> On 28/04/2021 18:07, Catalin Marinas wrote:
> > I probably asked already but is the only way to map a standard RAM page
> > (not device) in stage 2 via the fault handler? One case I had in mind
> > was something like get_user_pages() but it looks like that one doesn't
> > call set_pte_at_notify(). There are a few other places where
> > set_pte_at_notify() is called and these may happen before we got a
> > chance to fault on stage 2, effectively populating the entry (IIUC). If
> > that's an issue, we could move the above loop and check closer to the
> > actual pte setting like kvm_pgtable_stage2_map().
> 
> The only call sites of kvm_pgtable_stage2_map() are in mmu.c:
> 
>  * kvm_phys_addr_ioremap() - maps as device in stage 2
> 
>  * user_mem_abort() - handled above
> 
>  * kvm_set_spte_handler() - ultimately called from the .change_pte()
> callback of the MMU notifier
> 
> So the last one is potentially a problem. It's called via the MMU notifiers
> in the case of set_pte_at_notify(). The users of that are:
> 
>  * uprobe_write_opcode(): Allocates a new page and performs a
> copy_highpage() to copy the data to the new page (which with MTE includes
> the tags and will copy across the PG_mte_tagged flag).
> 
>  * write_protect_page() (KSM): Changes the permissions on the PTE but it's
> still the same page, so nothing to do regarding MTE.

My concern here is that the VMM had a stage 1 pte but we haven't yet
faulted in at stage 2 via user_mem_abort(), so we don't have any stage 2
pte set. write_protect_page() comes in and sets the new stage 2 pte via
the callback. I couldn't find any check in kvm_pgtable_stage2_map() for
the old pte, so it will set the new stage 2 pte regardless. A subsequent
guest read would no longer fault at stage 2.

>  * replace_page() (KSM): If the page has MTE tags then the MTE version of
> memcmp_pages() will return false, so the only caller
> (try_to_merge_one_page()) will never call this on a page with tags.
> 
>  * wp_page_copy(): This one is more interesting - if we go down the
> cow_user_page() path with an old page then everything is safe (tags are
> copied over). The is_zero_pfn() case worries me a bit - a new page is
> allocated, but I can't instantly see anything to zero out the tags (and set
> PG_mte_tagged).

True, I think tag zeroing happens only if we map it as PROT_MTE in the
VMM.

>  * migrate_vma_insert_page(): I think migration should be safe as the tags
> should be copied.
> 
> So wp_page_copy() looks suspicious.
> 
> kvm_pgtable_stage2_map() looks like it could be a good place for the checks,
> it looks like it should work and is probably a more obvious place for the
> checks.

That would be my preference. It also matches the stage 1 set_pte_at().

> > While the set_pte_at() race on the page flags is somewhat clearer, we
> > may still have a race here with the VMM's set_pte_at() if the page is
> > mapped as tagged. KVM has its own mmu_lock but it wouldn't be held when
> > handling the VMM page tables (well, not always, see below).
> > 
> > gfn_to_pfn_prot() ends up calling get_user_pages*(). At least the slow
> > path (hva_to_pfn_slow()) ends up with FOLL_TOUCH in gup and the VMM pte
> > would be set, tags cleared (if PROT_MTE) before the stage 2 pte. I'm not
> > sure whether get_user_page_fast_only() does the same.
> > 
> > The race with an mprotect(PROT_MTE) in the VMM is fine I think as the
> > KVM mmu notifier is invoked before set_pte_at() and racing with another
> > user_mem_abort() is serialised by the KVM mmu_lock. The subsequent
> > set_pte_at() would see the PG_mte_tagged set either by the current CPU
> > or by the one it was racing with.
> 
> Given the changes to set_pte_at() which means that tags are restored from
> swap even if !PROT_MTE, the only race I can see remaining is the creation of
> new PROT_MTE mappings. As you mention an attempt to change mappings in the
> VMM memory space should involve a mmu notifier call which I think serialises
> this. So the remaining issue is doing this in a separate address space.
> 
> So I guess the potential problem is:
> 
>  * allocate memory MAP_SHARED but !PROT_MTE
>  * fork()
>  * VM causes a fault in parent address space
>  * child does a mprotect(PROT_MTE)
> 
> With the last two potentially racing. Sadly I can't see a good way of
> handling that.

Ah, the mmap lock doesn't help as they are different processes
(mprotect() acquires it as a writer).

I wonder whether this is racy even in the absence of KVM. If both parent
and child do an mprotect(PROT_MTE), one of them may be reading stale
tags for a brief period.

Maybe we should revisit whether shared MTE pages are of any use, though
it's an ABI change (not bad if no-one is relying on this). However...

Thinking about this, we have a similar problem with the PG_dcache_clean
and two processes doing mprotect(PROT_EXEC). One of them could see the
flag set and skip the I-cache maintenance while the 

Re: [PATCH] migration: do not restart VM after successful snapshot-load

[Daniel P . Berrangé]

On Tue, May 04, 2021 at 12:58:26PM -0400, Paolo Bonzini wrote:
> The HMP loadvm code is calling load_snapshot rather than
> qmp_snapshot_load, in order to bypass the job infrastructure.  The code
> around it is almost the same, with one difference: hmp_loadvm is
> restarting the VM if load_snapshot fails, qmp_snapshot_load is doing so
> if load_snapshot succeeds.
> 
> Fix the bug in QMP by moving the common code to load_snapshot.

I'm wondering how did you discover this bug ?

> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Paolo Bonzini 
> ---
>  migration/savevm.c | 16 
>  monitor/hmp-cmds.c |  7 +--
>  2 files changed, 9 insertions(+), 14 deletions(-)

We ought to assert this behaviour in some test cases

We have  qemu-iotests/068  for HMP  and 

qemu-iotests/tests/internal-snapshots-qapi   for QMP...

doh, i just realize we never got the latter merged.

> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 52e2d72e4b..a899191cbf 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2992,6 +2992,7 @@ bool load_snapshot(const char *name, const char 
> *vmstate,
>  int ret;
>  AioContext *aio_context;
>  MigrationIncomingState *mis = migration_incoming_get_current();
> +int saved_vm_running  = runstate_is_running();
>  
>  if (!bdrv_all_can_snapshot(has_devices, devices, errp)) {
>  return false;
> @@ -3024,6 +3025,8 @@ bool load_snapshot(const char *name, const char 
> *vmstate,
>  return false;
>  }
>  
> +vm_stop(RUN_STATE_RESTORE_VM);
> +
>  /*
>   * Flush the record/replay queue. Now the VM state is going
>   * to change. Therefore we don't need to preserve its consistency
> @@ -3061,13 +3064,17 @@ bool load_snapshot(const char *name, const char 
> *vmstate,
>  
>  if (ret < 0) {
>  error_setg(errp, "Error %d while loading VM state", ret);
> -return false;
> +goto err_restart;
>  }
>  
>  return true;
>  
>  err_drain:
>  bdrv_drain_all_end();
> +err_restart:
> +if (saved_vm_running) {
> +vm_start();
> +}
>  return false;
>  }
>  
> @@ -3135,17 +3142,10 @@ static void snapshot_load_job_bh(void *opaque)
>  {
>  Job *job = opaque;
>  SnapshotJob *s = container_of(job, SnapshotJob, common);
> -int orig_vm_running;
>  
>  job_progress_set_remaining(>common, 1);
>  
> -orig_vm_running = runstate_is_running();
> -vm_stop(RUN_STATE_RESTORE_VM);
> -
>  s->ret = load_snapshot(s->tag, s->vmstate, true, s->devices, s->errp);
> -if (s->ret && orig_vm_running) {
> -vm_start();
> -}
>  
>  job_progress_update(>common, 1);
>  
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 0ad5b77477..a39436c8cb 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1127,15 +1127,10 @@ void hmp_balloon(Monitor *mon, const QDict *qdict)
>  
>  void hmp_loadvm(Monitor *mon, const QDict *qdict)
>  {
> -int saved_vm_running  = runstate_is_running();
>  const char *name = qdict_get_str(qdict, "name");
>  Error *err = NULL;
>  
> -vm_stop(RUN_STATE_RESTORE_VM);
> -
> -if (!load_snapshot(name, NULL, false, NULL, ) && saved_vm_running) {
> -vm_start();
> -}
> +load_snapshot(name, NULL, false, NULL, );
>  hmp_handle_error(mon, err);
>  }
>  
> -- 
> 2.26.2
> 
> 

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

[Bug 1857226] Re: 'set_link net0 off' not working with e1000e driver

[Jannik]

Still reproducible on Fedora 34 with QEMU 5.2.0.

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

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

Title:
  'set_link net0 off' not working with e1000e driver

Status in QEMU:
  New

Bug description:
  I'm encountering a bug with the e1000e network driver, that appears to
  got previously reported at rhbz. Steps to reproduce are provided in
  detail there:

  https://bugzilla.redhat.com/show_bug.cgi?id=1707646

  It is about switching off network link state (set_link net0 off)
  having no effect when using the e1000e driver.

  Version details:
  QEMU emulator version 4.1.1 (qemu-4.1.1-1.fc31)
  Fedora 31

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

Re: [PATCH] migration: do not restart VM after successful snapshot-load

[Eric Blake]

On 5/4/21 11:58 AM, Paolo Bonzini wrote:
> The HMP loadvm code is calling load_snapshot rather than
> qmp_snapshot_load, in order to bypass the job infrastructure.  The code
> around it is almost the same, with one difference: hmp_loadvm is
> restarting the VM if load_snapshot fails, qmp_snapshot_load is doing so
> if load_snapshot succeeds.
> 
> Fix the bug in QMP by moving the common code to load_snapshot.
> 
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Paolo Bonzini 
> ---
>  migration/savevm.c | 16 
>  monitor/hmp-cmds.c |  7 +--
>  2 files changed, 9 insertions(+), 14 deletions(-)

Reviewed-by: Eric Blake 

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

[PATCH] migration: do not restart VM after successful snapshot-load

[Paolo Bonzini]

The HMP loadvm code is calling load_snapshot rather than
qmp_snapshot_load, in order to bypass the job infrastructure.  The code
around it is almost the same, with one difference: hmp_loadvm is
restarting the VM if load_snapshot fails, qmp_snapshot_load is doing so
if load_snapshot succeeds.

Fix the bug in QMP by moving the common code to load_snapshot.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Paolo Bonzini 
---
 migration/savevm.c | 16 
 monitor/hmp-cmds.c |  7 +--
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 52e2d72e4b..a899191cbf 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2992,6 +2992,7 @@ bool load_snapshot(const char *name, const char *vmstate,
 int ret;
 AioContext *aio_context;
 MigrationIncomingState *mis = migration_incoming_get_current();
+int saved_vm_running  = runstate_is_running();
 
 if (!bdrv_all_can_snapshot(has_devices, devices, errp)) {
 return false;
@@ -3024,6 +3025,8 @@ bool load_snapshot(const char *name, const char *vmstate,
 return false;
 }
 
+vm_stop(RUN_STATE_RESTORE_VM);
+
 /*
  * Flush the record/replay queue. Now the VM state is going
  * to change. Therefore we don't need to preserve its consistency
@@ -3061,13 +3064,17 @@ bool load_snapshot(const char *name, const char 
*vmstate,
 
 if (ret < 0) {
 error_setg(errp, "Error %d while loading VM state", ret);
-return false;
+goto err_restart;
 }
 
 return true;
 
 err_drain:
 bdrv_drain_all_end();
+err_restart:
+if (saved_vm_running) {
+vm_start();
+}
 return false;
 }
 
@@ -3135,17 +3142,10 @@ static void snapshot_load_job_bh(void *opaque)
 {
 Job *job = opaque;
 SnapshotJob *s = container_of(job, SnapshotJob, common);
-int orig_vm_running;
 
 job_progress_set_remaining(>common, 1);
 
-orig_vm_running = runstate_is_running();
-vm_stop(RUN_STATE_RESTORE_VM);
-
 s->ret = load_snapshot(s->tag, s->vmstate, true, s->devices, s->errp);
-if (s->ret && orig_vm_running) {
-vm_start();
-}
 
 job_progress_update(>common, 1);
 
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 0ad5b77477..a39436c8cb 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1127,15 +1127,10 @@ void hmp_balloon(Monitor *mon, const QDict *qdict)
 
 void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
-int saved_vm_running  = runstate_is_running();
 const char *name = qdict_get_str(qdict, "name");
 Error *err = NULL;
 
-vm_stop(RUN_STATE_RESTORE_VM);
-
-if (!load_snapshot(name, NULL, false, NULL, ) && saved_vm_running) {
-vm_start();
-}
+load_snapshot(name, NULL, false, NULL, );
 hmp_handle_error(mon, err);
 }
 
-- 
2.26.2

Re: [PATCH] qcow2: set bdi->is_dirty

[Kirill Tkhai]

On 04.05.2021 19:06, Vladimir Sementsov-Ogievskiy wrote:
> Set bdi->is_dirty, so that qemu-img info could show dirty flag.
> 
> After this commit the following check will show '"dirty-flag": true':
> 
> ./build/qemu-img create -f qcow2 -o lazy_refcounts=on x 1M
> ./build/qemu-io x
> qemu-io> write 0 1M
> 
>  After "write" command success, kill the qemu-io process:
> 
> kill -9 
> 
> ./build/qemu-img info --output=json x
> 
> This will show '"dirty-flag": true' among other things. (before this
> commit it shows '"dirty-flag": false')
> 
> Note, that qcow2's dirty-bit is not a "dirty bit for the image". It
> only protects qcow2 lazy refcounts feature. So, there are a lot of
> conditions when qcow2 session may be not closed correctly, but bit is
> 0. Still, when bit is set, the last session is definitely not finished
> correctly and it's better to report it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Works for me. Thanks,

Tested-by: Kirill Tkhai 

> ---
>  block/qcow2.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 9727ae8fe3..39b91ef940 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5089,6 +5089,7 @@ static int qcow2_get_info(BlockDriverState *bs, 
> BlockDriverInfo *bdi)
>  BDRVQcow2State *s = bs->opaque;
>  bdi->cluster_size = s->cluster_size;
>  bdi->vm_state_offset = qcow2_vm_state_offset(s);
> +bdi->is_dirty = s->incompatible_features & QCOW2_INCOMPAT_DIRTY;
>  return 0;
>  }
>  
> 

[PATCH 1/2] Consistent function names for sifive uart read and write function

[Lukas Jünger]

Signed-off-by: Lukas Jünger 
---
 hw/char/sifive_uart.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
index 3a00ba7f00..cb70374ead 100644
--- a/hw/char/sifive_uart.c
+++ b/hw/char/sifive_uart.c
@@ -65,7 +65,7 @@ static void update_irq(SiFiveUARTState *s)
 }
 
 static uint64_t
-uart_read(void *opaque, hwaddr addr, unsigned int size)
+sifive_uart_read(void *opaque, hwaddr addr, unsigned int size)
 {
 SiFiveUARTState *s = opaque;
 unsigned char r;
@@ -101,8 +101,8 @@ uart_read(void *opaque, hwaddr addr, unsigned int size)
 }
 
 static void
-uart_write(void *opaque, hwaddr addr,
-   uint64_t val64, unsigned int size)
+sifive_uart_write(void *opaque, hwaddr addr,
+  uint64_t val64, unsigned int size)
 {
 SiFiveUARTState *s = opaque;
 uint32_t value = val64;
@@ -131,9 +131,9 @@ uart_write(void *opaque, hwaddr addr,
   __func__, (int)addr, (int)value);
 }
 
-static const MemoryRegionOps uart_ops = {
-.read = uart_read,
-.write = uart_write,
+static const MemoryRegionOps sifive_uart_ops = {
+.read = sifive_uart_read,
+.write = sifive_uart_write,
 .endianness = DEVICE_NATIVE_ENDIAN,
 .valid = {
 .min_access_size = 4,
@@ -187,7 +187,7 @@ SiFiveUARTState *sifive_uart_create(MemoryRegion 
*address_space, hwaddr base,
 qemu_chr_fe_init(>chr, chr, _abort);
 qemu_chr_fe_set_handlers(>chr, uart_can_rx, uart_rx, uart_event,
 uart_be_change, s, NULL, true);
-memory_region_init_io(>mmio, NULL, _ops, s,
+memory_region_init_io(>mmio, NULL, _uart_ops, s,
   TYPE_SIFIVE_UART, SIFIVE_UART_MAX);
 memory_region_add_subregion(address_space, base, >mmio);
 return s;
-- 
2.30.2

Re: [PATCH v4 4/5] target/ppc: turned SPR R/W callbacks not static

[Richard Henderson]

On 5/4/21 7:01 AM, Bruno Larsen (billionai) wrote:

To be able to compile translate_init.c.inc as a standalone file,
we have to make the callbacks accessible outside of translate.c;
This patch does exactly that

Signed-off-by: Bruno Larsen (billionai)
---
  target/ppc/spr_tcg.h   | 134 ++
  target/ppc/translate.c | 210 -
  2 files changed, 237 insertions(+), 107 deletions(-)
  create mode 100644 target/ppc/spr_tcg.h


Reviewed-by: Richard Henderson 

r~

Re: [RESEND PATCH 05/32] vl: Add "sgx-epc" option to expose SGX EPC sections to guest

[Paolo Bonzini]

On 04/05/21 18:20, Sean Christopherson wrote:

If it's just CPUID, one possibility could be to mark the EPC sections
specially in KVM_SET_USER_MEMORY_REGION and synthesize the leaves within
KVM; or even look inside the VMA structs and detect EPC regions that way.


I experimented with those options, and a few others, and they all lack the
flexibility of making EPC just another memory backend.

For synthesizing CPUID within KVM:
   - Requires a vendor specific memory region flag for all architectures to work
 around a quirk of one userspace VMM.
   - Pushes a lot of complexity into KVM, e.g. KVM needs to update CPUID in
 response to memslot changes, and needs to query memslots in response to
 CPUID changes.
   - Does KVM or userspace define the section attributes, e.g. confidentiality,
 integrity, etc...?  If KVM, are they hardcoded to match the host?  What
 happens if a future Intel platform supports multiple EPC sections with
 different attributes?  If userspace, how does userspace communicate the
 attributes?
   - How does userspace know what KVM enumerated to the guest?  See the whole
 KVM_GET_CPUID2 fiasco...
   - Prevents userspace from enumerating EPC without a memslot, e.g. to trap on
 the first EPC access for tracking purposes.
  
For probing VMAs:

   - In addition to the above issues, requires MMU notifier integration to 
update
 CPUID in response to a VMA change.
   - Requires SGX subsystem to provide a helper to identify EPC VMAs.

In short, I feel very strongly that this is QEMU's problem to solve.



Makes sense, thanks.  (Of course this or other design comments could 
have been in the commit message too; but in all fairness it's never 
obvious which misguided ideas the reviewers could come up with).


Paolo

[PATCH 0/2] QOMify Sifive UART model

[Lukas Jünger]

Hello,

This patch QOMifies the Sifive UART model.
It is split into two commits.
The first commit makes the sifive uart read and write function names
more consistent.
The second commit QOMifies the model.

Looking forward to hear from you,
Lukas

Lukas Jünger (2):
  Consistent function names for sifive uart read and write function
  QOMify sifive_uart model

 include/hw/char/sifive_uart.h |  6 +--
 hw/char/sifive_uart.c | 84 ---
 2 files changed, 71 insertions(+), 19 deletions(-)

-- 
2.30.2

[PATCH 2/3] hw/misc: add ADM1272 device

[Titus Rwantare]

The ADM1272 is a PMBus compliant Hot Swap Controller and Digital Power
Monitor by Analog Devices.

This commit adds support for interfacing with it, and support for
setting and monitoring sensor limits.

Datasheet: 
https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1272.pdf

Reviewed-by: Hao Wu 
Signed-off-by: Titus Rwantare 
---
 hw/arm/Kconfig |   1 +
 hw/misc/Kconfig|   4 +
 hw/misc/adm1272.c  | 551 +
 hw/misc/meson.build|   1 +
 tests/qtest/adm1272-test.c | 453 ++
 tests/qtest/meson.build|   1 +
 6 files changed, 1011 insertions(+)
 create mode 100644 hw/misc/adm1272.c
 create mode 100644 tests/qtest/adm1272-test.c

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 8c9ae17efd..41e8c573a2 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -370,6 +370,7 @@ config XLNX_VERSAL
 config NPCM7XX
 bool
 select A9MPCORE
+select ADM1272
 select ARM_GIC
 select AT24C  # EEPROM
 select PL310  # cache controller
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index c71ed25820..0774c1f70c 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -14,6 +14,10 @@ config ARMSSE_CPU_PWRCTRL
 config MAX111X
 bool
 
+config ADM1272
+bool
+depends on I2C
+
 config TMP105
 bool
 depends on I2C
diff --git a/hw/misc/adm1272.c b/hw/misc/adm1272.c
new file mode 100644
index 00..8c9b139eda
--- /dev/null
+++ b/hw/misc/adm1272.c
@@ -0,0 +1,551 @@
+/*
+ * Analog Devices ADM1272 High Voltage Positive Hot Swap Controller and Digital
+ * Power Monitor with PMBus
+ *
+ * Copyright 2021 Google LLC
+ *
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include 
+#include "hw/i2c/pmbus_device.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "trace.h"
+
+#define TYPE_ADM1272 "adm1272"
+#define ADM1272(obj) OBJECT_CHECK(ADM1272State, (obj), TYPE_ADM1272)
+
+#define ADM1272_RESTART_TIME0xCC
+#define ADM1272_MFR_PEAK_IOUT   0xD0
+#define ADM1272_MFR_PEAK_VIN0xD1
+#define ADM1272_MFR_PEAK_VOUT   0xD2
+#define ADM1272_MFR_PMON_CONTROL0xD3
+#define ADM1272_MFR_PMON_CONFIG 0xD4
+#define ADM1272_MFR_ALERT1_CONFIG   0xD5
+#define ADM1272_MFR_ALERT2_CONFIG   0xD6
+#define ADM1272_MFR_PEAK_TEMPERATURE0xD7
+#define ADM1272_MFR_DEVICE_CONFIG   0xD8
+#define ADM1272_MFR_POWER_CYCLE 0xD9
+#define ADM1272_MFR_PEAK_PIN0xDA
+#define ADM1272_MFR_READ_PIN_EXT0xDB
+#define ADM1272_MFR_READ_EIN_EXT0xDC
+
+#define ADM1272_HYSTERESIS_LOW  0xF2
+#define ADM1272_HYSTERESIS_HIGH 0xF3
+#define ADM1272_STATUS_HYSTERESIS   0xF4
+#define ADM1272_STATUS_GPIO 0xF5
+#define ADM1272_STRT_UP_IOUT_LIM0xF6
+
+/* Defaults */
+#define ADM1272_OPERATION_DEFAULT   0x80
+#define ADM1272_CAPABILITY_DEFAULT  0xB0
+#define ADM1272_CAPABILITY_NO_PEC   0x30
+#define ADM1272_DIRECT_MODE 0x40
+#define ADM1272_HIGH_LIMIT_DEFAULT  0x0FFF
+#define ADM1272_PIN_OP_DEFAULT  0x7FFF
+#define ADM1272_PMBUS_REVISION_DEFAULT  0x22
+#define ADM1272_MFR_ID_DEFAULT  "ADI"
+#define ADM1272_MODEL_DEFAULT   "ADM1272-A1"
+#define ADM1272_MFR_DEFAULT_REVISION"25"
+#define ADM1272_DEFAULT_DATE"160301"
+#define ADM1272_RESTART_TIME_DEFAULT0x64
+#define ADM1272_PMON_CONTROL_DEFAULT0x1
+#define ADM1272_PMON_CONFIG_DEFAULT 0x3F35
+#define ADM1272_DEVICE_CONFIG_DEFAULT   0x8
+#define ADM1272_HYSTERESIS_HIGH_DEFAULT 0x
+#define ADM1272_STRT_UP_IOUT_LIM_DEFAULT0x000F
+#define ADM1272_VOLT_DEFAULT12000
+#define ADM1272_IOUT_DEFAULT25000
+#define ADM1272_PWR_DEFAULT 300  /* 12V 25A */
+#define ADM1272_SHUNT   300 /* micro-ohms */
+#define ADM1272_VOLTAGE_COEFF_DEFAULT   1
+#define ADM1272_CURRENT_COEFF_DEFAULT   3
+#define ADM1272_PWR_COEFF_DEFAULT   7
+#define ADM1272_IOUT_OFFSET 0x5000
+#define ADM1272_IOUT_OFFSET 0x5000
+
+
+typedef struct ADM1272State {
+PMBusDevice parent;
+
+uint64_t ein_ext;
+uint32_t pin_ext;
+uint8_t restart_time;
+
+uint16_t peak_vin;
+uint16_t peak_vout;
+uint16_t peak_iout;
+uint16_t peak_temperature;
+uint16_t peak_pin;
+
+uint8_t pmon_control;
+uint16_t 

[PATCH 2/2] QOMify sifive_uart model

[Lukas Jünger]

Signed-off-by: Lukas Jünger 
---
 include/hw/char/sifive_uart.h |  6 +--
 hw/char/sifive_uart.c | 72 ++-
 2 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/include/hw/char/sifive_uart.h b/include/hw/char/sifive_uart.h
index 3e962be659..45d66b1db5 100644
--- a/include/hw/char/sifive_uart.h
+++ b/include/hw/char/sifive_uart.h
@@ -21,6 +21,7 @@
 #define HW_SIFIVE_UART_H
 
 #include "chardev/char-fe.h"
+#include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
 #include "qom/object.h"
 
@@ -51,10 +52,7 @@ enum {
 #define SIFIVE_UART_GET_RXCNT(rxctrl)   ((rxctrl >> 16) & 0x7)
 
 #define TYPE_SIFIVE_UART "riscv.sifive.uart"
-
-typedef struct SiFiveUARTState SiFiveUARTState;
-DECLARE_INSTANCE_CHECKER(SiFiveUARTState, SIFIVE_UART,
- TYPE_SIFIVE_UART)
+OBJECT_DECLARE_SIMPLE_TYPE(SiFiveUARTState, SIFIVE_UART)
 
 struct SiFiveUARTState {
 /*< private >*/
diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
index cb70374ead..0307568d0a 100644
--- a/hw/char/sifive_uart.c
+++ b/hw/char/sifive_uart.c
@@ -25,6 +25,7 @@
 #include "hw/hw.h"
 #include "hw/irq.h"
 #include "hw/char/sifive_uart.h"
+#include "hw/qdev-properties-system.h"
 
 /*
  * Not yet implemented:
@@ -176,19 +177,72 @@ static int uart_be_change(void *opaque)
 return 0;
 }
 
+static Property sifive_uart_properties[] = {
+DEFINE_PROP_CHR("chardev", SiFiveUARTState, chr),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void sifive_uart_init(Object *obj)
+{
+SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+SiFiveUARTState *s = SIFIVE_UART(obj);
+
+memory_region_init_io(>mmio, OBJECT(s), _uart_ops, s,
+  TYPE_SIFIVE_UART, SIFIVE_UART_MAX);
+sysbus_init_mmio(sbd, >mmio);
+sysbus_init_irq(sbd, >irq);
+}
+
+static void sifive_uart_realize(DeviceState *dev, Error **errp)
+{
+SiFiveUARTState *s = SIFIVE_UART(dev);
+
+qemu_chr_fe_set_handlers(>chr, uart_can_rx, uart_rx, uart_event,
+ uart_be_change, s, NULL, true);
+
+}
+
+static void sifive_uart_class_init(ObjectClass *oc, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(oc);
+
+dc->realize = sifive_uart_realize;
+device_class_set_props(dc, sifive_uart_properties);
+}
+
+static const TypeInfo sifive_uart_info = {
+.name  = TYPE_SIFIVE_UART,
+.parent= TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(SiFiveUARTState),
+.instance_init = sifive_uart_init,
+.class_init= sifive_uart_class_init,
+};
+
+static void sifive_uart_register_types(void)
+{
+type_register_static(_uart_info);
+}
+
+type_init(sifive_uart_register_types)
+
 /*
  * Create UART device.
  */
 SiFiveUARTState *sifive_uart_create(MemoryRegion *address_space, hwaddr base,
 Chardev *chr, qemu_irq irq)
 {
-SiFiveUARTState *s = g_malloc0(sizeof(SiFiveUARTState));
-s->irq = irq;
-qemu_chr_fe_init(>chr, chr, _abort);
-qemu_chr_fe_set_handlers(>chr, uart_can_rx, uart_rx, uart_event,
-uart_be_change, s, NULL, true);
-memory_region_init_io(>mmio, NULL, _uart_ops, s,
-  TYPE_SIFIVE_UART, SIFIVE_UART_MAX);
-memory_region_add_subregion(address_space, base, >mmio);
-return s;
+DeviceState *dev;
+SysBusDevice *s;
+SiFiveUARTState *r;
+
+dev = qdev_new("riscv.sifive.uart");
+s = SYS_BUS_DEVICE(dev);
+qdev_prop_set_chr(dev, "chardev", chr);
+sysbus_realize_and_unref(s, _fatal);
+memory_region_add_subregion(address_space, base,
+sysbus_mmio_get_region(s, 0));
+sysbus_connect_irq(s, 0, irq);
+
+r = SIFIVE_UART(dev);
+return r;
 }
-- 
2.30.2

[PATCH 3/3] hw/misc: add MAX34451 device

[Titus Rwantare]

The MAX34451 is a Maxim power-supply system manager that can monitor up to 16 
voltage rails or currents. It also contains a temperature sensor and supports 
up to four external temperature sensors.

This commit adds support for interfacing with it, and setting limits on the 
supported sensors.

Reviewed-by: Hao Wu 
Signed-off-by: Titus Rwantare 
---
 hw/arm/Kconfig  |   1 +
 hw/misc/Kconfig |   4 +
 hw/misc/max34451.c  | 727 
 hw/misc/meson.build |   1 +
 tests/qtest/max34451-test.c | 344 +
 tests/qtest/meson.build |   1 +
 6 files changed, 1078 insertions(+)
 create mode 100644 hw/misc/max34451.c
 create mode 100644 tests/qtest/max34451-test.c

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 41e8c573a2..128228f878 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -373,6 +373,7 @@ config NPCM7XX
 select ADM1272
 select ARM_GIC
 select AT24C  # EEPROM
+select MAX34451
 select PL310  # cache controller
 select PMBUS
 select SERIAL
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 0774c1f70c..1747667984 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -30,6 +30,10 @@ config EMC141X
 bool
 depends on I2C
 
+config MAX34451
+bool
+depends on I2C
+
 config ISA_DEBUG
 bool
 depends on ISA_BUS
diff --git a/hw/misc/max34451.c b/hw/misc/max34451.c
new file mode 100644
index 00..16366fad6d
--- /dev/null
+++ b/hw/misc/max34451.c
@@ -0,0 +1,727 @@
+/*
+ * Maxim MAX34451 PMBus 16-Channel V/I monitor and 12-Channel 
Sequencer/Marginer
+ *
+ * Copyright 2021 Google LLC
+ *
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/i2c/pmbus_device.h"
+#include "hw/irq.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+
+#define TYPE_MAX34451 "max34451"
+#define MAX34451(obj) OBJECT_CHECK(MAX34451State, (obj), TYPE_MAX34451)
+
+#define MAX34451_MFR_MODE   0xD1
+#define MAX34451_MFR_PSEN_CONFIG0xD2
+#define MAX34451_MFR_VOUT_PEAK  0xD4
+#define MAX34451_MFR_IOUT_PEAK  0xD5
+#define MAX34451_MFR_TEMPERATURE_PEAK   0xD6
+#define MAX34451_MFR_VOUT_MIN   0xD7
+#define MAX34451_MFR_NV_LOG_CONFIG  0xD8
+#define MAX34451_MFR_FAULT_RESPONSE 0xD9
+#define MAX34451_MFR_FAULT_RETRY0xDA
+#define MAX34451_MFR_NV_FAULT_LOG   0xDC
+#define MAX34451_MFR_TIME_COUNT 0xDD
+#define MAX34451_MFR_MARGIN_CONFIG  0xDF
+#define MAX34451_MFR_FW_SERIAL  0xE0
+#define MAX34451_MFR_IOUT_AVG   0xE2
+#define MAX34451_MFR_CHANNEL_CONFIG 0xE4
+#define MAX34451_MFR_TON_SEQ_MAX0xE6
+#define MAX34451_MFR_PWM_CONFIG 0xE7
+#define MAX34451_MFR_SEQ_CONFIG 0xE8
+#define MAX34451_MFR_STORE_ALL  0xEE
+#define MAX34451_MFR_RESTORE_ALL0xEF
+#define MAX34451_MFR_TEMP_SENSOR_CONFIG 0xF0
+#define MAX34451_MFR_STORE_SINGLE   0xFC
+#define MAX34451_MFR_CRC0xFE
+
+#define MAX34451_NUM_MARGINED_PSU   12
+#define MAX34451_NUM_PWR_DEVICES16
+#define MAX34451_NUM_TEMP_DEVICES   5
+#define MAX34451_NUM_PAGES  21
+
+#define DEFAULT_OP_ON   0x80
+#define DEFAULT_CAPABILITY  0x20
+#define DEFAULT_ON_OFF_CONFIG   0x1a
+#define DEFAULT_VOUT_MODE   0x40
+#define DEFAULT_TEMPERATURE 2500
+#define DEFAULT_SCALE   0x7FFF
+#define DEFAULT_OV_LIMIT0x7FFF
+#define DEFAULT_OC_LIMIT0x7FFF
+#define DEFAULT_OT_LIMIT0x7FFF
+#define DEFAULT_VMIN0x7FFF
+#define DEFAULT_TON_FAULT_LIMIT 0x
+#define DEFAULT_CHANNEL_CONFIG  0x20
+#define DEFAULT_TEXT0x3130313031303130
+
+/**
+ * MAX34451State:
+ * @code: The command code received
+ * @page: Each page corresponds to a device monitored by the Max 34451
+ * The page register determines the available commands depending on device
+  ___
+ |   0   |  Power supply monitored by RS0, controlled by PSEN0, and  |
+ |   |  margined with PWM0.  |
+ |___|___|
+ |   1   |  Power supply monitored by RS1, controlled by PSEN1, and  |
+ |   |  margined with PWM1.

Re: remove the nvlink2 pci_vfio subdriver v2

[Jason Gunthorpe]

On Tue, May 04, 2021 at 04:23:40PM +0200, Daniel Vetter wrote:

> Just my 2cents from drm (where we deprecate old gunk uapi quite often):
> Imo it's best to keep the uapi headers as-is, but exchange the
> documentation with a big "this is removed, never use again" warning:

We in RDMA have been doing the opposite, the uapi headers are supposed
to reflect the current kernel. This helps make the kernel
understandable.

When userspace needs backwards compat to ABI that the current kernel
doesn't support then userspace has distinct copies of that information
in some compat location. It has happened a few times over the last 15
years.

We keep full copies of the current kernel headers in the userspace
source tree, when the kernel headers change in a compile incompatible
way we fix everything while updating to the new kernel headers.

> - it's good to know which uapi numbers (like parameter extensions or
>   whatever they are in this case) are defacto reserved, because there are
>   binaries (qemu in this) that have code acting on them out there.

Numbers and things get marked reserved or the like

> Anyway feel free to ignore since this might be different than drivers/gpu.

AFAIK drives/gpu has a lot wider userspace, rdma manages this OK
because we only have one library package that provides the user/kernel
interface.
 
Jason

Re: remove the nvlink2 pci_vfio subdriver v2

[Daniel Vetter]

On Tue, May 04, 2021 at 12:53:27PM -0300, Jason Gunthorpe wrote:
> On Tue, May 04, 2021 at 04:23:40PM +0200, Daniel Vetter wrote:
> 
> > Just my 2cents from drm (where we deprecate old gunk uapi quite often):
> > Imo it's best to keep the uapi headers as-is, but exchange the
> > documentation with a big "this is removed, never use again" warning:
> 
> We in RDMA have been doing the opposite, the uapi headers are supposed
> to reflect the current kernel. This helps make the kernel
> understandable.
> 
> When userspace needs backwards compat to ABI that the current kernel
> doesn't support then userspace has distinct copies of that information
> in some compat location. It has happened a few times over the last 15
> years.
> 
> We keep full copies of the current kernel headers in the userspace
> source tree, when the kernel headers change in a compile incompatible
> way we fix everything while updating to the new kernel headers.

Yeah we do the same since forever (it's either from libdrm package, or
directly in the corresponding userspace header). So largely include/uapi
is for documentation

> > - it's good to know which uapi numbers (like parameter extensions or
> >   whatever they are in this case) are defacto reserved, because there are
> >   binaries (qemu in this) that have code acting on them out there.
> 
> Numbers and things get marked reserved or the like
> 
> > Anyway feel free to ignore since this might be different than drivers/gpu.
> 
> AFAIK drives/gpu has a lot wider userspace, rdma manages this OK
> because we only have one library package that provides the user/kernel
> interface.

But since we have some many projects we've started asking all the userspace
projects to directly take the kernel ones (after the make step to filter
them) so that there's only one source of truth. And also to make sure they
don't merge stuff before the kernel side is reviewed Which also
means we can't ditch anything userspace might still need on older trees
and stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 1/3] hw/i2c: add support for PMBus

[Titus Rwantare]

QEMU has support for SMBus devices, and PMBus is a more specific
implementation of SMBus. The additions made in this commit makes it easier to
add new PMBus devices to QEMU.

https://pmbus.org/specification-archives/

Reviewed-by: Hao Wu 
Signed-off-by: Titus Rwantare 
---
 hw/arm/Kconfig|1 +
 hw/i2c/Kconfig|4 +
 hw/i2c/meson.build|1 +
 hw/i2c/pmbus_device.c | 1611 +
 include/hw/i2c/pmbus_device.h |  520 +++
 5 files changed, 2137 insertions(+)
 create mode 100644 hw/i2c/pmbus_device.c
 create mode 100644 include/hw/i2c/pmbus_device.h

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 8c37cf00da..8c9ae17efd 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -373,6 +373,7 @@ config NPCM7XX
 select ARM_GIC
 select AT24C  # EEPROM
 select PL310  # cache controller
+select PMBUS
 select SERIAL
 select SSI
 select UNIMP
diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
index 09642a6dcb..87f7004cea 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -28,3 +28,7 @@ config IMX_I2C
 config MPC_I2C
 bool
 select I2C
+
+config PMBUS
+bool
+select SMBUS
diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
index cdcd694a7f..f7e2d482b0 100644
--- a/hw/i2c/meson.build
+++ b/hw/i2c/meson.build
@@ -14,4 +14,5 @@ i2c_ss.add(when: 'CONFIG_SMBUS_EEPROM', if_true: 
files('smbus_eeprom.c'))
 i2c_ss.add(when: 'CONFIG_VERSATILE_I2C', if_true: files('versatile_i2c.c'))
 i2c_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_i2c.c'))
 i2c_ss.add(when: 'CONFIG_PPC4XX', if_true: files('ppc4xx_i2c.c'))
+i2c_ss.add(when: 'CONFIG_PMBUS', if_true: files('pmbus_device.c'))
 softmmu_ss.add_all(when: 'CONFIG_I2C', if_true: i2c_ss)
diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
new file mode 100644
index 00..73b944e983
--- /dev/null
+++ b/hw/i2c/pmbus_device.c
@@ -0,0 +1,1611 @@
+/*
+ * PMBus wrapper over SMBus
+ *
+ * Copyright 2021 Google LLC
+ *
+ * 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.
+ */
+#include "qemu/osdep.h"
+#include 
+#include 
+#include "hw/i2c/pmbus_device.h"
+#include "qemu/module.h"
+#include "qemu/log.h"
+
+uint16_t pmbus_data2direct_mode(PMBusCoefficients c, uint32_t value)
+{
+/* R is usually negative to fit large readings into 16 bits */
+uint16_t y = (c.m * value + c.b) * pow(10, c.R);
+return y;
+}
+
+uint32_t pmbus_direct_mode2data(PMBusCoefficients c, uint16_t value)
+{
+/* X = (Y * 10^-R - b) / m */
+uint32_t x = (value / pow(10, c.R) - c.b) / c.m;
+return x;
+}
+
+static void pmbus_send(PMBusDevice *pmdev, const uint8_t *data, uint16_t len)
+{
+if (pmdev->out_buf_len + len > SMBUS_DATA_MAX_LEN) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "PMBus device tried to send too much data");
+len = 0;
+}
+
+for (int i = len - 1; i >= 0; i--) {
+pmdev->out_buf[i + pmdev->out_buf_len] = data[len - i - 1];
+}
+pmdev->out_buf_len += len;
+}
+
+/* Internal only, convert unsigned ints to the little endian bus */
+static void pmbus_send_uint(PMBusDevice *pmdev, uint64_t data, uint8_t size)
+{
+uint8_t bytes[8];
+for (int i = 0; i < size; i++) {
+bytes[i] = data & 0xFF;
+data = data >> 8;
+}
+pmbus_send(pmdev, bytes, size);
+}
+
+void pmbus_send_block(PMBusDevice *pmdev, PMBusBlock block)
+{
+pmbus_send(pmdev, block.buf, block.len);
+}
+
+void pmbus_send8(PMBusDevice *pmdev, uint8_t data)
+{
+pmbus_send_uint(pmdev, data, 1);
+}
+
+void pmbus_send16(PMBusDevice *pmdev, uint16_t data)
+{
+pmbus_send_uint(pmdev, data, 2);
+}
+
+void pmbus_send32(PMBusDevice *pmdev, uint32_t data)
+{
+pmbus_send_uint(pmdev, data, 4);
+}
+
+void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
+{
+pmbus_send_uint(pmdev, data, 8);
+}
+
+void pmbus_send_string(PMBusDevice *pmdev, const char *data)
+{
+size_t len = strlen(data);
+g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
+pmdev->out_buf[len + pmdev->out_buf_len] = len;
+
+for (int i = len - 1; i >= 0; i--) {
+pmdev->out_buf[i + pmdev->out_buf_len] = data[len - 1 - i];
+}
+pmdev->out_buf_len += len + 1;
+}
+
+
+static uint64_t pmbus_receive_uint(const uint8_t *buf, uint8_t len)
+{
+uint64_t ret = 0;
+
+/* Exclude command code from return value */
+buf++;
+len--;
+
+for (int i = len - 1; i >= 0; i--) {
+ret = ret << 8 | buf[i];
+}
+return ret;
+}
+
+uint8_t 

[PATCH 0/3] Add support for PMBus in QEMU

[Titus Rwantare]

Hello,

This patch series adds an interface to start supporting PMBus devices in QEMU.
I’ve included two PMBus devices: MAX34451 and ADM1272.

PMBus is a variant of SMBus meant for digital management of power supplies.
PMBus adds to the SMBus standard by defining a number of constants and commands
used by compliant devices. The specification for PMBus can be found at:

https://pmbus.org/specification-archives/

Currently, the goal for these devices is to emulate basic functionality by
reading and writing registers. Timing, and some logical operation is not
implemented. This implementation supports nearly all available registers for
PMBus including:
   - Voltage inputs and outputs
   - Current inputs and outputs
   - Temperature sensors

Unimplimented registers get passed through to the device model, and device
models can opt out of using the standard registers with flags. The included
devices make use of these fields and illustrate how to interface with the pmbus
class.

Datasheets for sensors:

https://datasheets.maximintegrated.com/en/ds/MAX34451.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1272.pdf

Thanks for reviewing,

Titus Rwantare

Titus Rwantare (3):
  hw/i2c: add support for PMBus
  hw/misc: add ADM1272 device
  hw/misc: add MAX34451 device

 hw/arm/Kconfig|3 +
 hw/i2c/Kconfig|4 +
 hw/i2c/meson.build|1 +
 hw/i2c/pmbus_device.c | 1611 +
 hw/misc/Kconfig   |8 +
 hw/misc/adm1272.c |  551 +++
 hw/misc/max34451.c|  727 +++
 hw/misc/meson.build   |2 +
 include/hw/i2c/pmbus_device.h |  520 +++
 tests/qtest/adm1272-test.c|  453 +
 tests/qtest/max34451-test.c   |  344 +++
 tests/qtest/meson.build   |2 +
 12 files changed, 4226 insertions(+)
 create mode 100644 hw/i2c/pmbus_device.c
 create mode 100644 hw/misc/adm1272.c
 create mode 100644 hw/misc/max34451.c
 create mode 100644 include/hw/i2c/pmbus_device.h
 create mode 100644 tests/qtest/adm1272-test.c
 create mode 100644 tests/qtest/max34451-test.c

-- 
2.31.1.527.g47e6f16901-goog

Re: [PATCH] qcow2: set bdi->is_dirty

[Eric Blake]

On 5/4/21 11:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> Set bdi->is_dirty, so that qemu-img info could show dirty flag.
> 
> After this commit the following check will show '"dirty-flag": true':
> 
> ./build/qemu-img create -f qcow2 -o lazy_refcounts=on x 1M
> ./build/qemu-io x
> qemu-io> write 0 1M
> 
>  After "write" command success, kill the qemu-io process:
> 
> kill -9 
> 
> ./build/qemu-img info --output=json x
> 
> This will show '"dirty-flag": true' among other things. (before this
> commit it shows '"dirty-flag": false')
> 
> Note, that qcow2's dirty-bit is not a "dirty bit for the image". It
> only protects qcow2 lazy refcounts feature. So, there are a lot of
> conditions when qcow2 session may be not closed correctly, but bit is
> 0. Still, when bit is set, the last session is definitely not finished
> correctly and it's better to report it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake 


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

Re: [RESEND PATCH 05/32] vl: Add "sgx-epc" option to expose SGX EPC sections to guest

[Sean Christopherson]

On Tue, May 04, 2021, Paolo Bonzini wrote:
> On 04/05/21 02:09, Sean Christopherson wrote:
> > Is there a way to process "-device sgx-epc..." before vCPUs are realized?  
> > The
> > ordering problem was the only reason I added a dedicated option.
> 
> If it's just CPUID, one possibility could be to mark the EPC sections
> specially in KVM_SET_USER_MEMORY_REGION and synthesize the leaves within
> KVM; or even look inside the VMA structs and detect EPC regions that way.

I experimented with those options, and a few others, and they all lack the
flexibility of making EPC just another memory backend.

For synthesizing CPUID within KVM:
  - Requires a vendor specific memory region flag for all architectures to work
around a quirk of one userspace VMM.
  - Pushes a lot of complexity into KVM, e.g. KVM needs to update CPUID in
response to memslot changes, and needs to query memslots in response to
CPUID changes.
  - Does KVM or userspace define the section attributes, e.g. confidentiality,
integrity, etc...?  If KVM, are they hardcoded to match the host?  What
happens if a future Intel platform supports multiple EPC sections with
different attributes?  If userspace, how does userspace communicate the
attributes?
  - How does userspace know what KVM enumerated to the guest?  See the whole
KVM_GET_CPUID2 fiasco...
  - Prevents userspace from enumerating EPC without a memslot, e.g. to trap on
the first EPC access for tracking purposes.
 
For probing VMAs:
  - In addition to the above issues, requires MMU notifier integration to update
CPUID in response to a VMA change.
  - Requires SGX subsystem to provide a helper to identify EPC VMAs.

In short, I feel very strongly that this is QEMU's problem to solve.

> Otherwise, the -M solution would work.
> 
> Paolo
> 
> >  From the changelog:
> > 
> >Because SGX EPC is enumerated through CPUID, EPC "devices" need to be
> >realized prior to realizing the vCPUs themselves, i.e. long before
> >generic devices are parsed and realized.
> > 
> >So even though EPC sections could be realized through the generic
> >-devices command, they need to be created much earlier for them to
> >actually be usable by the guest.
> 

Re: [PATCH v4 3/5] target/ppc: move SPR R/W callbacks to translate.c

[Richard Henderson]

On 5/4/21 7:01 AM, Bruno Larsen (billionai) wrote:

Moved all read and write callbacks for SPRs away from
translate_init.c.inc and into translate.c; these functions are
TCG only, so this motion is required to enable building with
the flag disable-tcg

Signed-off-by: Bruno Larsen (billionai) 


Reviewed-by: Richard Henderson 


+/*/
+/* SPR READ/RITE CALLBACKS */


WRITE.


r~

Re: [PATCH v6 11/12] qtest/bios-tables-test: Make test build-independent from accelerator

[Philippe Mathieu-Daudé]

On 5/3/21 11:19 PM, Eric Blake wrote:
> On 5/3/21 4:10 PM, Philippe Mathieu-Daudé wrote:
>> Now than we can probe if the TCG accelerator is available
> 
> that

Oops. I'll wait for Igor review before respinning.

> 
>> at runtime with a QMP command, do it once at the beginning
>> and only register the tests we can run.
>> We can then replace the #ifdef'ry by an assertion.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>> v5 had:
>> Reviewed-by: Eric Blake 
>> Reviewed-by: Alex Bennée 
>>
>> v6 is simplified and keeps the same logic, however since
>> it is different, I'm not keeping the R-b tags.
>> ---
>>  tests/qtest/bios-tables-test.c | 14 ++
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
> 
> Reviewed-by: Eric Blake 

Thanks!

Phil.

[PATCH] qcow2: set bdi->is_dirty

[Vladimir Sementsov-Ogievskiy]

Set bdi->is_dirty, so that qemu-img info could show dirty flag.

After this commit the following check will show '"dirty-flag": true':

./build/qemu-img create -f qcow2 -o lazy_refcounts=on x 1M
./build/qemu-io x
qemu-io> write 0 1M

 After "write" command success, kill the qemu-io process:

kill -9 

./build/qemu-img info --output=json x

This will show '"dirty-flag": true' among other things. (before this
commit it shows '"dirty-flag": false')

Note, that qcow2's dirty-bit is not a "dirty bit for the image". It
only protects qcow2 lazy refcounts feature. So, there are a lot of
conditions when qcow2 session may be not closed correctly, but bit is
0. Still, when bit is set, the last session is definitely not finished
correctly and it's better to report it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 9727ae8fe3..39b91ef940 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5089,6 +5089,7 @@ static int qcow2_get_info(BlockDriverState *bs, 
BlockDriverInfo *bdi)
 BDRVQcow2State *s = bs->opaque;
 bdi->cluster_size = s->cluster_size;
 bdi->vm_state_offset = qcow2_vm_state_offset(s);
+bdi->is_dirty = s->incompatible_features & QCOW2_INCOMPAT_DIRTY;
 return 0;
 }
 
-- 
2.29.2

Re: [PATCH] vfio/pci: Revert nvlink removal uAPI breakage

[Greg Kurz]

On Tue, 04 May 2021 09:52:02 -0600
Alex Williamson  wrote:

> Revert the uAPI changes from the below commit with notice that these
> regions and capabilities are no longer provided.
> 
> Fixes: b392a1989170 ("vfio/pci: remove vfio_pci_nvlink2")
> Reported-by: Greg Kurz 
> Signed-off-by: Alex Williamson 
> ---
> 
> Greg (Kurz), please double check this resolves the issue.  Thanks!
> 

It does. Feel free to add:

Reviewed-by: Greg Kurz 

and

Tested-by: Greg Kurz 

Thanks for the quick fix.

Cheers,

--
Greg

>  include/uapi/linux/vfio.h |   46 
> +
>  1 file changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 34b1f53a3901..ef33ea002b0b 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -333,10 +333,21 @@ struct vfio_region_info_cap_type {
>  #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG(3)
>  
>  /* 10de vendor PCI sub-types */
> -/* subtype 1 was VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM, don't use */
> +/*
> + * NVIDIA GPU NVlink2 RAM is coherent RAM mapped onto the host address space.
> + *
> + * Deprecated, region no longer provided
> + */
> +#define VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM   (1)
>  
>  /* 1014 vendor PCI sub-types */
> -/* subtype 1 was VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD, don't use */
> +/*
> + * IBM NPU NVlink2 ATSD (Address Translation Shootdown) register of NPU
> + * to do TLB invalidation on a GPU.
> + *
> + * Deprecated, region no longer provided
> + */
> +#define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1)
>  
>  /* sub-types for VFIO_REGION_TYPE_GFX */
>  #define VFIO_REGION_SUBTYPE_GFX_EDID(1)
> @@ -630,9 +641,36 @@ struct vfio_device_migration_info {
>   */
>  #define VFIO_REGION_INFO_CAP_MSIX_MAPPABLE   3
>  
> -/* subtype 4 was VFIO_REGION_INFO_CAP_NVLINK2_SSATGT, don't use */
> +/*
> + * Capability with compressed real address (aka SSA - small system address)
> + * where GPU RAM is mapped on a system bus. Used by a GPU for DMA routing
> + * and by the userspace to associate a NVLink bridge with a GPU.
> + *
> + * Deprecated, capability no longer provided
> + */
> +#define VFIO_REGION_INFO_CAP_NVLINK2_SSATGT  4
> +
> +struct vfio_region_info_cap_nvlink2_ssatgt {
> + struct vfio_info_cap_header header;
> + __u64 tgt;
> +};
>  
> -/* subtype 5 was VFIO_REGION_INFO_CAP_NVLINK2_LNKSPD, don't use */
> +/*
> + * Capability with an NVLink link speed. The value is read by
> + * the NVlink2 bridge driver from the bridge's "ibm,nvlink-speed"
> + * property in the device tree. The value is fixed in the hardware
> + * and failing to provide the correct value results in the link
> + * not working with no indication from the driver why.
> + *
> + * Deprecated, capability no longer provided
> + */
> +#define VFIO_REGION_INFO_CAP_NVLINK2_LNKSPD  5
> +
> +struct vfio_region_info_cap_nvlink2_lnkspd {
> + struct vfio_info_cap_header header;
> + __u32 link_speed;
> + __u32 __pad;
> +};
>  
>  /**
>   * VFIO_DEVICE_GET_IRQ_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 9,
> 
> 

Re: [PATCH] vfio/pci: Revert nvlink removal uAPI breakage

[Cornelia Huck]

On Tue, 04 May 2021 09:52:02 -0600
Alex Williamson  wrote:

> Revert the uAPI changes from the below commit with notice that these
> regions and capabilities are no longer provided.
> 
> Fixes: b392a1989170 ("vfio/pci: remove vfio_pci_nvlink2")
> Reported-by: Greg Kurz 
> Signed-off-by: Alex Williamson 
> ---
> 
> Greg (Kurz), please double check this resolves the issue.  Thanks!
> 
>  include/uapi/linux/vfio.h |   46 
> +
>  1 file changed, 42 insertions(+), 4 deletions(-)

I had already hacked up a QEMU patch that moved the definitions into
local headers, but this one is less of a hassle. (Code compiles fine
after doing a headers update.)

Reviewed-by: Cornelia Huck 

Re: [PULL 0/5] bsd-user: minor cleanup patches

[Peter Maydell]

On Fri, 30 Apr 2021 at 18:46, Warner Losh  wrote:
>
> The following changes since commit ffa090bc56e73e287a63261e70ac02c0970be61a:
>
>   target/s390x: fix s390_probe_access to check PAGE_WRITE_ORG for 
> writeability (2021-04-23 14:10:56 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/bsdimp/qemu.git tags/pull-bsd-user-20210430
>
> for you to fetch changes up to 58b3beb483d08066548d84eccd007b9d8bd24a2b:
>
>   bsd-user: style tweak: Put {} around all if/else/for statements (2021-04-30 
> 09:14:06 -0600)
>
> 
> bsd-user: start to cleanup the mess
>
> A number of small cleanups to get started. All the checkpatch.pl warnings for
> bsdload.c have been fixed, as well as a warning from qemu.h (though more 
> remain
> and this patch series fails the format check still). I've also fixed a
> compile-time warning about a missing break.
>
> 
> Warner Losh (5):
>   bsd-user: whitespace changes
>   bsd-user: style tweak: keyword space (
>   bsd-user: style tweak: return is not a function, eliminate ()
>   bsd-user: put back a break; that had gone missing...
>   bsd-user: style tweak: Put {} around all if/else/for statements


Applied, thanks.

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

-- PMM

Re: [PATCH 4/7] tests/acceptance: Sun4uMachine: Remove dependency to LinuxKernelTest

[Philippe Mathieu-Daudé]

Hi Wainer,

On 5/4/21 12:43 AM, Wainer dos Santos Moschetta wrote:
> The Sun4uMachine class inherit from LinuxKernelTest to effectively only use
> the KERNEL_COMMON_COMMAND_LINE attribute. This change remove that unneeded
> dependency, making Sun4uMachine self-content.

It is odd because the test boots a Linux kernel...

Once you added ConsoleMixIn, LinuxKernelTest is left with
2 methods related to archive extraction. Not particularly
specific to Linux kernel. Beside, shouldn't these methods
be provided by Avocado directly, by avocado.utils.archive
and avocado.utils.software_manager.backends?

> I took the occasion to delint the code: the unused os import was
> removed, imports were reordered, and the module has a docstring now.
> 
> Signed-off-by: Wainer dos Santos Moschetta 
> ---
>  tests/acceptance/machine_sparc64_sun4u.py | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)

Re: [PATCH v4 2/5] target/ppc: renamed SPR registration functions

[Richard Henderson]

On 5/4/21 7:01 AM, Bruno Larsen (billionai) wrote:

Renamed all gen_spr_* and gen_* functions specifically related to
registering SPRs to register_*_sprs and register_*, to avoid future
confusion with other TCG related code.

Signed-off-by: Bruno Larsen (billionai)
---
  target/ppc/translate_init.c.inc | 860 
  1 file changed, 430 insertions(+), 430 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [RFC PATCH v2 0/2] hw/ppc: code motion to compile without TCG

[Lucas Mateus Martins Araujo e Castro]

Thanks, it will be quite helpful.

Also, I agree with Bruno including this information somewhere would be quite 
good in my opinion.

From: Fabiano Rosas 
Sent: Monday, May 3, 2021 7:21 PM
To: Lucas Mateus Martins Araujo e Castro ; 
qemu-devel@nongnu.org ; qemu-...@nongnu.org 

Cc: Bruno Piazera Larsen ; Lucas Mateus Martins 
Araujo e Castro ; da...@gibson.dropbear.id.au 

Subject: Re: [RFC PATCH v2 0/2] hw/ppc: code motion to compile without TCG

"Lucas Mateus Castro (alqotel)"  writes:

> After the feedback from v1 I reworked the patch with suggested ideas and
> this version has less duplicated code and is overall simpler.
>
> This patch series is still a WIP, there are still 2 main problems I am
> trying to solve, I'll mention them in their respective patches.
>
> The aim of these patches is to progress toward enabling disable-tcg on
> PPC by solving errors in hw/ppc with that option.
>
> As a WIP comments are welcome.
>
> Lucas Mateus Castro (alqotel) (2):
>   target/ppc: Moved functions out of mmu-hash64
>   hw/ppc: Moved TCG code to spapr_hcall_tcg
>
>  hw/ppc/meson.build   |   3 +
>  hw/ppc/spapr.c   |   1 +
>  hw/ppc/spapr_caps.c  |   1 +
>  hw/ppc/spapr_cpu_core.c  |   1 +
>  hw/ppc/spapr_hcall.c | 301 ++
>  hw/ppc/spapr_hcall_tcg.c | 343 +++
>  hw/ppc/spapr_rtas.c  |   1 +
>  target/ppc/meson.build   |   1 +
>  target/ppc/mmu-hash64.c  |  81 +
>  target/ppc/mmu-hash64.h  |   6 -
>  target/ppc/mmu-misc.c|  86 ++
>  target/ppc/mmu-misc.h|  22 +++
>  12 files changed, 478 insertions(+), 369 deletions(-)
>  create mode 100644 hw/ppc/spapr_hcall_tcg.c
>  create mode 100644 target/ppc/mmu-misc.c
>  create mode 100644 target/ppc/mmu-misc.h

This is the list of hypercalls registered with spapr_register_hypercall
and whether they are implemented by KVM HV, KVM PR or none. I also list
whether the KVM hcall uses the QEMU implementation as a fallback. Maybe
it will be helpful to this discussion.

(This is from just looking at the code, so take it with a grain of salt)

H_ADD_LOGICAL_LAN_BUFFER  - not impl. by KVM
H_CHANGE_LOGICAL_LAN_MAC  - not impl. by KVM
H_ENABLE_CRQ  - not impl. by KVM
H_FREE_CRQ- not impl. by KVM
H_FREE_LOGICAL_LAN- not impl. by KVM
H_GET_CPU_CHARACTERISTICS - not impl. by KVM
H_GET_TERM_CHAR   - not impl. by KVM
H_HOME_NODE_ASSOCIATIVITY - not impl. by KVM
H_INT_ESB - not impl. by KVM
H_INT_GET_QUEUE_INFO  - not impl. by KVM
H_INT_GET_SOURCE_CONFIG   - not impl. by KVM
H_INT_GET_SOURCE_INFO - not impl. by KVM
H_INT_RESET   - not impl. by KVM
H_INT_SET_QUEUE_CONFIG- not impl. by KVM
H_INT_SET_SOURCE_CONFIG   - not impl. by KVM
H_INT_SYNC- not impl. by KVM
H_JOIN- not impl. by KVM
H_LOGICAL_CACHE_LOAD  - not impl. by KVM
H_LOGICAL_CACHE_STORE - not impl. by KVM
H_LOGICAL_DCBF- not impl. by KVM
H_LOGICAL_ICBI- not impl. by KVM
H_MULTICAST_CTRL  - not impl. by KVM
H_PUT_TERM_CHAR   - not impl. by KVM
H_REGISTER_LOGICAL_LAN- not impl. by KVM
H_REGISTER_PROC_TBL   - not impl. by KVM
H_REG_CRQ - not impl. by KVM
H_RESIZE_HPT_COMMIT   - not impl. by KVM
H_RESIZE_HPT_PREPARE  - not impl. by KVM
H_SCM_BIND_MEM- not impl. by KVM
H_SCM_READ_METADATA   - not impl. by KVM
H_SCM_UNBIND_ALL  - not impl. by KVM
H_SCM_WRITE_METADATA  - not impl. by KVM
H_SEND_CRQ- not impl. by KVM
H_SEND_LOGICAL_LAN- not impl. by KVM
H_SET_SPRG0   - not impl. by KVM
H_SIGNAL_SYS_RESET- not impl. by KVM
H_VIO_SIGNAL  - not impl. by KVM

H_CAS - not impl. by KVM | called by SLOF only
H_LOGICAL_MEMOP   - not impl. by KVM | called by SLOF only
H_TPM_COMM- not impl. by KVM | called by UV only
H_UPDATE_DT   - not impl. by KVM | called by SLOF only

H_INT_GET_OS_REPORTING_LINE - not impl. by KVM | not called by linux/SLOF/UV
H_INT_GET_QUEUE_CONFIG  - not impl. by KVM | not called by linux/SLOF/UV
H_INT_SET_OS_REPORTING_LINE - not impl. by KVM | not called by linux/SLOF/UV
H_SCM_UNBIND_MEM- not impl. by KVM | not called by linux/SLOF/UV

H_GET_TCE  - HV | not impl. by PR | QEMU fallback
H_SET_MODE - HV | not impl. by PR | QEMU fallback
H_CONFER   - HV | not impl. by PR
H_PAGE_INIT- HV | not impl. by PR
H_PROD - HV | not impl. by PR
H_RANDOM   - HV | not impl. by PR
H_READ - HV | not impl. by PR
H_REGISTER_VPA - HV | not impl. by PR
H_SET_DABR - HV | not impl. by PR
H_SET_XDABR- HV | not impl. by PR

H_CPPR - HV | PR | QEMU fallback
H_EOI  - HV | PR | QEMU fallback
H_IPI  - HV | PR | QEMU fallback
H_IPOLL- HV | PR | QEMU fallback

[Bug 1828867] Re: QEmu translation is incorrect when using REX in combination with LAHF/SAHF

[Thomas Huth]

This is an automated cleanup. This bug report has been moved to QEMU's
new bug tracker on gitlab.com and thus gets marked as 'expired' now.
Please continue with the discussion here:

 https://gitlab.com/qemu-project/qemu/-/issues/130


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

** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #130
   https://gitlab.com/qemu-project/qemu/-/issues/130

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

Title:
  QEmu translation is incorrect when using REX in combination with
  LAHF/SAHF

Status in QEMU:
  Expired

Bug description:
  When translating code that is using LAHF and SAHF in combination with the REX 
prefix then qemu translates incorrectly.
  These two instructions only ever use the AH register. Contrary to other 
instructions where if you use REX + high bit offsets then it'll pull in rsp and 
a few other registers.
  On hardware the REX prefix doesn't effect behaviour of these instructions at 
all.
  QEMU incorrectly selects RSP as the register of choice here due to this 
combination of REX + AH register usage.

  I've attached a patch that is super terrible just so I can work around
  the issue locally and to sort of show off how it is to be "fixed"

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

[PATCH] vfio/pci: Revert nvlink removal uAPI breakage

[Alex Williamson]

Revert the uAPI changes from the below commit with notice that these
regions and capabilities are no longer provided.

Fixes: b392a1989170 ("vfio/pci: remove vfio_pci_nvlink2")
Reported-by: Greg Kurz 
Signed-off-by: Alex Williamson 
---

Greg (Kurz), please double check this resolves the issue.  Thanks!

 include/uapi/linux/vfio.h |   46 +
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 34b1f53a3901..ef33ea002b0b 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -333,10 +333,21 @@ struct vfio_region_info_cap_type {
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG  (3)
 
 /* 10de vendor PCI sub-types */
-/* subtype 1 was VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM, don't use */
+/*
+ * NVIDIA GPU NVlink2 RAM is coherent RAM mapped onto the host address space.
+ *
+ * Deprecated, region no longer provided
+ */
+#define VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM (1)
 
 /* 1014 vendor PCI sub-types */
-/* subtype 1 was VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD, don't use */
+/*
+ * IBM NPU NVlink2 ATSD (Address Translation Shootdown) register of NPU
+ * to do TLB invalidation on a GPU.
+ *
+ * Deprecated, region no longer provided
+ */
+#define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD   (1)
 
 /* sub-types for VFIO_REGION_TYPE_GFX */
 #define VFIO_REGION_SUBTYPE_GFX_EDID(1)
@@ -630,9 +641,36 @@ struct vfio_device_migration_info {
  */
 #define VFIO_REGION_INFO_CAP_MSIX_MAPPABLE 3
 
-/* subtype 4 was VFIO_REGION_INFO_CAP_NVLINK2_SSATGT, don't use */
+/*
+ * Capability with compressed real address (aka SSA - small system address)
+ * where GPU RAM is mapped on a system bus. Used by a GPU for DMA routing
+ * and by the userspace to associate a NVLink bridge with a GPU.
+ *
+ * Deprecated, capability no longer provided
+ */
+#define VFIO_REGION_INFO_CAP_NVLINK2_SSATGT4
+
+struct vfio_region_info_cap_nvlink2_ssatgt {
+   struct vfio_info_cap_header header;
+   __u64 tgt;
+};
 
-/* subtype 5 was VFIO_REGION_INFO_CAP_NVLINK2_LNKSPD, don't use */
+/*
+ * Capability with an NVLink link speed. The value is read by
+ * the NVlink2 bridge driver from the bridge's "ibm,nvlink-speed"
+ * property in the device tree. The value is fixed in the hardware
+ * and failing to provide the correct value results in the link
+ * not working with no indication from the driver why.
+ *
+ * Deprecated, capability no longer provided
+ */
+#define VFIO_REGION_INFO_CAP_NVLINK2_LNKSPD5
+
+struct vfio_region_info_cap_nvlink2_lnkspd {
+   struct vfio_info_cap_header header;
+   __u32 link_speed;
+   __u32 __pad;
+};
 
 /**
  * VFIO_DEVICE_GET_IRQ_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 9,

Re: [PATCH v11 1/6] arm64: mte: Sync tags for pages where PTE is untagged

[Catalin Marinas]

On Thu, Apr 29, 2021 at 05:06:05PM +0100, Steven Price wrote:
> On 27/04/2021 18:43, Catalin Marinas wrote:
> > On Fri, Apr 16, 2021 at 04:43:04PM +0100, Steven Price wrote:
> > > diff --git a/arch/arm64/include/asm/pgtable.h 
> > > b/arch/arm64/include/asm/pgtable.h
> > > index e17b96d0e4b5..cf4b52a33b3c 100644
> > > --- a/arch/arm64/include/asm/pgtable.h
> > > +++ b/arch/arm64/include/asm/pgtable.h
> > > @@ -312,7 +312,7 @@ static inline void set_pte_at(struct mm_struct *mm, 
> > > unsigned long addr,
> > >   __sync_icache_dcache(pte);
> > >   if (system_supports_mte() &&
> > > - pte_present(pte) && pte_tagged(pte) && !pte_special(pte))
> > > + pte_present(pte) && (pte_val(pte) & PTE_USER) && !pte_special(pte))
> > 
> > I would add a pte_user() macro here or, if we restore the tags only when
> > the page is readable, use pte_access_permitted(pte, false). Also add a
> > comment why we do this.
> 
> pte_access_permitted() looks like it describes what we want (user space can
> access the memory). I'll add the following comment:
> 
>  /*
>   * If the PTE would provide user space will access to the tags

I think drop "will".

>   * associated with it then ensure that the MTE tags are synchronised.
>   * Exec-only mappings don't expose tags (instruction fetches don't
>   * check tags).
>   */

Sounds fine.

> > There's also the pte_user_exec() case which may not have the PTE_USER
> > set (exec-only permission) but I don't think it matters. We don't do tag
> > checking on instruction fetches, so if the user adds a PROT_READ to it,
> > it would go through set_pte_at() again. I'm not sure KVM does anything
> > special with exec-only mappings at stage 2, I suspect they won't be
> > accessible by the guest (but needs checking).
> 
> It comes down to the behaviour of get_user_pages(). AFAICT that will fail if
> the memory is exec-only, so no stage 2 mapping will be created. Which of
> course means the guest can't do anything with that memory. That certainly
> seems like the only sane behaviour even without MTE.

That's my understanding as well. The get_user_pages_fast() path uses
pte_access_permitted() and should return false. The slower
get_user_pages() relies on checking the vma flags and it checks for
VM_READ.

-- 
Catalin

Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property

[Eduardo Habkost]

On Tue, May 04, 2021 at 03:32:55PM +0100, Stefan Hajnoczi wrote:
> On Thu, Apr 29, 2021 at 02:03:52PM -0400, Eduardo Habkost wrote:
> > On Thu, Apr 29, 2021 at 04:52:21PM +0100, Stefan Hajnoczi wrote:
> > > Live migrating old guests from an old QEMU with the SCSI feature bit
> > > enabled will fail with "Features 0x... unsupported. Allowed features:
> > > 0x...". We've followed the QEMU deprecation policy so users have been
> > > warned...
> > > 
> > 
> > Were they really warned, though?  People running
> > "-machine pc-i440fx-2.4" might be completely unaware that it was
> > silently enabling a deprecated feature.
> > 
> > Can we have this documented in a more explicit way?  Maybe just a
> > comment at hw_compat_2_4 would be enough, to warn people doing
> > backports and rebases downstream.
> > 
> > Can we make QEMU refuse to start if using pc-2.4 + virtio-blk
> > together, just to be sure?
> 
> On second thought, do we really want to break pc-2.4 user's QEMU
> command-lines if they have a virtio-blk device?

It depends which command line you are talking about.

I believe we _must_ break the following:
"-machine pc-i440fx-2.4 -device virtio-blk", and
"-machine pc-i440fx-2.4 -device virtio-blk,scsi=on".
Your patch breaks only the latter.

Your patch also breaks the following:
"-machine pc-i440fx-2.4 -device virtio-blk,scsi=off",
which I don't think we should break.

> 
> BTW Peter mentioned libvirt avoids the unnecessary scsi=off:
> https://gitlab.com/libvirt/libvirt/-/commit/ec69f0190be731d12faeac08dbf63325836509a9
> 
> Stefan



-- 
Eduardo

Re: [PATCH v3 03/26] DAX: vhost-user: Rework slave return values

[Stefan Hajnoczi]

On Wed, Apr 28, 2021 at 12:00:37PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> All the current slave handlers on the qemu side generate an 'int'
> return value that's squashed down to a bool (!!ret) and stuffed into
> a uint64_t (field of a union) to be returned.
> 
> Move the uint64_t type back up through the individual handlers so
> that we can make one actually return a full uint64_t.
> 
> Note that the definition in the interop spec says most of these
> cases are defined as returning 0 on success and non-0 for failure,
> so it's OK to change from a bool to another non-0.
> 
> Vivek:
> This is needed because upcoming patches in series will add new functions
> which want to return full error code. Existing functions continue to
> return true/false so, it should not lead to change of behavior for
> existing users.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> Reviewed-by: Greg Kurz 
> ---
>  hw/virtio/vhost-backend.c |  6 +++---
>  hw/virtio/vhost-user.c| 31 ---
>  include/hw/virtio/vhost-backend.h |  2 +-
>  3 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 31b33bde37..1686c94767 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -401,8 +401,8 @@ int vhost_backend_invalidate_device_iotlb(struct 
> vhost_dev *dev,
>  return -ENODEV;
>  }
>  
> -int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
> -  struct vhost_iotlb_msg *imsg)
> +uint64_t vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
> +struct vhost_iotlb_msg *imsg)
>  {
>  int ret = 0;

This patch is messy. We want uint64_t but these functions use int ret
internally. The actual return values are true/false instead of int 0 and
1.

Please use uint64_t everywhere: return 0 for success and 1 for failure
instead of bool literals and change the type of the local ret variables
to uint64_t.


signature.asc
Description: PGP signature

Re: [PATCH v3 04/26] DAX: libvhost-user: Route slave message payload

[Stefan Hajnoczi]

On Wed, Apr 28, 2021 at 12:00:38PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Route the uint64 payload from message replies on the slave back up
> through vu_process_message_reply and to the callers.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> Reviewed-by: Stefan Hajnoczi 
> ---
>  subprojects/libvhost-user/libvhost-user.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: remove the nvlink2 pci_vfio subdriver v2

[Alex Williamson]

On Tue, 4 May 2021 16:11:31 +0200
Greg Kurz  wrote:

> On Tue, 4 May 2021 15:30:15 +0200
> Greg Kroah-Hartman  wrote:
> 
> > On Tue, May 04, 2021 at 03:20:34PM +0200, Greg Kurz wrote:  
> > > On Tue, 4 May 2021 14:59:07 +0200
> > > Greg Kroah-Hartman  wrote:
> > >   
> > > > On Tue, May 04, 2021 at 02:22:36PM +0200, Greg Kurz wrote:  
> > > > > On Fri, 26 Mar 2021 07:13:09 +0100
> > > > > Christoph Hellwig  wrote:
> > > > >   
> > > > > > Hi all,
> > > > > > 
> > > > > > the nvlink2 vfio subdriver is a weird beast.  It supports a hardware
> > > > > > feature without any open source component - what would normally be
> > > > > > the normal open source userspace that we require for kernel drivers,
> > > > > > although in this particular case user space could of course be a
> > > > > > kernel driver in a VM.  It also happens to be a complete mess that
> > > > > > does not properly bind to PCI IDs, is hacked into the vfio_pci 
> > > > > > driver
> > > > > > and also pulles in over 1000 lines of code always build into powerpc
> > > > > > kernels that have Power NV support enabled.  Because of all these
> > > > > > issues and the lack of breaking userspace when it is removed I think
> > > > > > the best idea is to simply kill.
> > > > > > 
> > > > > > Changes since v1:
> > > > > >  - document the removed subtypes as reserved
> > > > > >  - add the ACK from Greg
> > > > > > 
> > > > > > Diffstat:
> > > > > >  arch/powerpc/platforms/powernv/npu-dma.c |  705 
> > > > > > ---
> > > > > >  b/arch/powerpc/include/asm/opal.h|3 
> > > > > >  b/arch/powerpc/include/asm/pci-bridge.h  |1 
> > > > > >  b/arch/powerpc/include/asm/pci.h |7 
> > > > > >  b/arch/powerpc/platforms/powernv/Makefile|2 
> > > > > >  b/arch/powerpc/platforms/powernv/opal-call.c |2 
> > > > > >  b/arch/powerpc/platforms/powernv/pci-ioda.c  |  185 ---
> > > > > >  b/arch/powerpc/platforms/powernv/pci.c   |   11 
> > > > > >  b/arch/powerpc/platforms/powernv/pci.h   |   17 
> > > > > >  b/arch/powerpc/platforms/pseries/pci.c   |   23 
> > > > > >  b/drivers/vfio/pci/Kconfig   |6 
> > > > > >  b/drivers/vfio/pci/Makefile  |1 
> > > > > >  b/drivers/vfio/pci/vfio_pci.c|   18 
> > > > > >  b/drivers/vfio/pci/vfio_pci_private.h|   14 
> > > > > >  b/include/uapi/linux/vfio.h  |   38 -  
> > > > > 
> > > > > 
> > > > > Hi Christoph,
> > > > > 
> > > > > FYI, these uapi changes break build of QEMU.  
> > > > 
> > > > What uapi changes?
> > > >   
> > > 
> > > All macros and structure definitions that are being removed
> > > from include/uapi/linux/vfio.h by patch 1.
> > >   
> > > > What exactly breaks?
> > > >   
> > > 
> > > These macros and types are used by the current QEMU code base.
> > > Next time the QEMU source tree updates its copy of the kernel
> > > headers, the compilation of affected code will fail.  
> > 
> > So does QEMU use this api that is being removed, or does it just have
> > some odd build artifacts of the uapi things?
> >   
> 
> These are region subtypes definition and associated capabilities.
> QEMU basically gets information on VFIO regions from the kernel
> driver and for those regions with a nvlink2 subtype, it tries
> to extract some more nvlink2 related info.


Urgh, let's put the uapi header back in place with a deprecation
notice.  Userspace should never have a dependency on the existence of a
given region, but clearly will have code to parse the data structure
describing that region.  I'll post a patch.  Thanks,

Alex