date:20230220


On 21/2/23 00:26, Richard Henderson wrote:

We will need 2 bits to represent ARMSecurityState.

Do not attempt to replace or widen secure, even though it
logically overlaps the new field -- there are uses within
e.g. hw/block/pflash_cfi01.c, which don't know anything
specific about ARM.

Reviewed-by: Peter Maydell 
Signed-off-by: Richard Henderson 
---
  include/exec/memattrs.h | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 9fb98bc1ef..d04170aa27 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -29,10 +29,17 @@ typedef struct MemTxAttrs {
   * "didn't specify" if necessary.
   */
  unsigned int unspecified:1;
-/* ARM/AMBA: TrustZone Secure access
+/*
+ * ARM/AMBA: TrustZone Secure access
   * x86: System Management Mode access
   */
  unsigned int secure:1;
+/*
+ * ARM: ArmSecuritySpace.  This partially overlaps secure, but it is
+ * easier to have both fields to assist code that does not understand
+ * ARMv9 RME, or no specific knowledge of ARM at all (e.g. pflash).
+ */
+unsigned int space:2;
  /* Memory access is usermode (unprivileged) */
  unsigned int user:1;


'secure' & 'user' seem mutually exclusive. If we get short in bits,
they could be shared.

Re: [PATCH 2/2] qga/win32: Use rundll for VSS installation

2023-02-20 Thread Yan Vugenfirer

Reviewed-by: Yan Vugenfirer 


On Mon, Feb 20, 2023 at 7:41 PM Konstantin Kostiuk  wrote:
>
> Add specific an entry points for rundll which is
> just a wrapper for COMRegister/COMUnregister functions.
>
> resolves: rhbz#2167436
> fixes: CVE-2023-0664
>
> Signed-off-by: Konstantin Kostiuk 
> ---
>  qga/installer/qemu-ga.wxs | 10 +-
>  qga/vss-win32/install.cpp |  9 +
>  qga/vss-win32/qga-vss.def |  2 ++
>  3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
> index feb629ec47..46ae9e7a13 100644
> --- a/qga/installer/qemu-ga.wxs
> +++ b/qga/installer/qemu-ga.wxs
> @@ -127,22 +127,22 @@
>
>  
>
> -
> +
>  
>
>  
>   -  ExeCommand='/c "[qemu_ga_directory]qemu-ga.exe" -s vss-install'
> +  ExeCommand='"[qemu_ga_directory]qga-vss.dll",DLLCOMRegister'
>Execute="deferred"
> -  Property="cmd"
> +  Property="rundll"
>Impersonate="no"
>Return="check"
>>
>  
>   -  ExeCommand='/c "[qemu_ga_directory]qemu-ga.exe" -s 
> vss-uninstall'
> +  ExeCommand='"[qemu_ga_directory]qga-vss.dll",DLLCOMUnregister'
>Execute="deferred"
> -  Property="cmd"
> +  Property="rundll"
>Impersonate="no"
>Return="check"
>>
> diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> index b57508fbe0..68662a6dfc 100644
> --- a/qga/vss-win32/install.cpp
> +++ b/qga/vss-win32/install.cpp
> @@ -357,6 +357,15 @@ out:
>  return hr;
>  }
>
> +STDAPI_(void) CALLBACK DLLCOMRegister(HWND, HINSTANCE, LPSTR, int)
> +{
> +COMRegister();
> +}
> +
> +STDAPI_(void) CALLBACK DLLCOMUnregister(HWND, HINSTANCE, LPSTR, int)
> +{
> +COMUnregister();
> +}
>
>  static BOOL CreateRegistryKey(LPCTSTR key, LPCTSTR value, LPCTSTR data)
>  {
> diff --git a/qga/vss-win32/qga-vss.def b/qga/vss-win32/qga-vss.def
> index 927782c31b..ee97a81427 100644
> --- a/qga/vss-win32/qga-vss.def
> +++ b/qga/vss-win32/qga-vss.def
> @@ -1,6 +1,8 @@
>  LIBRARY  "QGA-PROVIDER.DLL"
>
>  EXPORTS
> +   DLLCOMRegister
> +   DLLCOMUnregister
> COMRegister PRIVATE
> COMUnregister   PRIVATE
> DllCanUnloadNow PRIVATE
> --
> 2.25.1
>

Re: [PATCH 1/2] qga/win32: Remove change action from MSI installer

2023-02-20 Thread Yan Vugenfirer

Reviewed-by: Yan Vugenfirer 

On Mon, Feb 20, 2023 at 7:41 PM Konstantin Kostiuk  wrote:
>
> resolves: rhbz#2167436
> fixes: CVE-2023-0664
>
> Signed-off-by: Konstantin Kostiuk 
> ---
>  qga/installer/qemu-ga.wxs | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
> index 51340f7ecc..feb629ec47 100644
> --- a/qga/installer/qemu-ga.wxs
> +++ b/qga/installer/qemu-ga.wxs
> @@ -31,6 +31,7 @@
>/>
>   EmbedCab="yes" />
>  1
> +
>  DowngradeErrorMessage="Error: A newer version of QEMU guest agent is 
> already installed."
>/>
> --
> 2.25.1
>

Re: [PATCH v3 2/6] configure: Add courtesy hint to Python version failure message


On 21/2/23 02:24, John Snow wrote:

If we begin requiring Python 3.7+, a few platforms are going to need to
install an additional Python interpreter package.

As a courtesy to the user, suggest the optional package they might need
to install. This will hopefully minimize any downtime caused by the
change in Python dependency.

Signed-off-by: John Snow 
---
  configure | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v2 14/14] target/arm: Implement gdbstub m-profile systemreg and secext


On 21/2/23 03:19, Richard Henderson wrote:

The upstream gdb xml only implements {MSP,PSP}{,_NS,S}, but
go ahead and implement the other system registers as well.

Since there is significant overlap between the two, implement
them with common code.  The only exception is the systemreg
view of CONTROL, which merges the banked bits as per MRS.

Signed-off-by: David Reiss 
[rth: Substatial rewrite using enumerator and shared code.]
Signed-off-by: Richard Henderson 
---
  target/arm/cpu.h |   2 +
  target/arm/gdbstub.c | 194 +++
  2 files changed, 196 insertions(+)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v2 13/14] target/arm: Export arm_v7m_get_sp_ptr


On 21/2/23 03:19, Richard Henderson wrote:

From: David Reiss 

Allow the function to be used outside of m_helper.c.
Move to be outside of ifndef CONFIG_USER_ONLY block.
Rename from get_v7m_sp_ptr.

Reviewed-by: Peter Maydell 
Signed-off-by: David Reiss 
[rth: Split out of a larger patch]
Signed-off-by: Richard Henderson 
---
  target/arm/internals.h | 10 +
  target/arm/m_helper.c  | 84 +-
  2 files changed, 51 insertions(+), 43 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v2 12/14] target/arm: Export arm_v7m_mrs_control


On 21/2/23 03:19, Richard Henderson wrote:

From: David Reiss 

Allow the function to be used outside of m_helper.c.
Rename with an "arm_" prefix.

Reviewed-by: Peter Maydell 
Signed-off-by: David Reiss 
[rth: Split out of a larger patch]
Signed-off-by: Richard Henderson 
---
  target/arm/internals.h | 3 +++
  target/arm/m_helper.c  | 6 +++---
  2 files changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v2 08/14] target/arm: Add name argument to output_vector_union_type


On 21/2/23 03:19, Richard Henderson wrote:

This will make the function usable between SVE and SME.

Reviewed-by: Peter Maydell 
Signed-off-by: Richard Henderson 
---
  target/arm/gdbstub64.c | 28 ++--
  1 file changed, 14 insertions(+), 14 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v2 06/14] target/arm: Hoist pred_width in arm_gen_dynamic_svereg_xml


On 21/2/23 03:19, Richard Henderson wrote:

Reviewed-by: Fabiano Rosas 
Signed-off-by: Richard Henderson 
---
  target/arm/gdbstub64.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v2 05/14] target/arm: Simplify register counting in arm_gen_dynamic_svereg_xml


On 21/2/23 03:19, Richard Henderson wrote:

Rather than increment base_reg and num, compute num
from the change to base_reg at the end.  Clean up some
nearby comments.

Signed-off-by: Richard Henderson 
---
  target/arm/gdbstub64.c | 27 ---
  1 file changed, 16 insertions(+), 11 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v11 0/9] support subsets of code size reduction extension

2023-02-20 Thread liweiwei


Hi,  any  new comments for this patchset?

Regards,

Weiwei Li

On 2023/2/9 12:13, Weiwei Li wrote:

This patchset implements RISC-V Zc* extension v1.0.2-1 version instructions.

Specification:
https://github.com/riscv/riscv-code-size-reduction/tree/main/Zc-specification

The port is available here:
https://github.com/plctlab/plct-qemu/tree/plct-zce-upstream-v11

To test Zc* implementation, specify cpu argument with 'x-zca=true,x-zcb=true,x-zcf=true,f=true" and 
"x-zcd=true,d=true" (or "x-zcmp=true,x-zcmt=true" with c or d=false) to enable 
Zca/Zcb/Zcf and Zcd(or Zcmp,Zcmt) extensions support.


This implementation can pass the basic zc tests from 
https://github.com/yulong-plct/zc-test

v11
* update format and field name based on the latest spec in patch 5, 6, 7 
(without other functional changes)
* rebase on riscv-to-apply.next

v10:
* rebase on Daniel's series(riscv-to-apply.next) and adjust riscv-tests to test 
on sifive related CPUs

v9:
* rebase on riscv-to-apply.next

v8:
* improve disas support in Patch 9

v7:
* Fix description for Zca

v6：
* fix base address for jump table in Patch 7
* rebase on riscv-to-apply.next

v5:
* fix exception unwind problem for cpu_ld*_code in helper of cm_jalt

v4:
* improve Zcmp suggested by Richard
* fix stateen related check for Zcmt

v3:
* update the solution for Zcf to the way of Zcd
* update Zcb to reuse gen_load/store
* use trans function instead of helper for push/pop

v2:
* add check for relationship between Zca/Zcf/Zcd with C/F/D based on related 
discussion in review of Zc* spec
* separate c.fld{sp}/fsd{sp} with fld{sp}/fsd{sp} before support of zcmp/zcmt

Weiwei Li (9):
   target/riscv: add cfg properties for Zc* extension
   target/riscv: add support for Zca extension
   target/riscv: add support for Zcf extension
   target/riscv: add support for Zcd extension
   target/riscv: add support for Zcb extension
   target/riscv: add support for Zcmp extension
   target/riscv: add support for Zcmt extension
   target/riscv: expose properties for Zc* extension
   disas/riscv.c: add disasm support for Zc*

  disas/riscv.c | 228 +++-
  target/riscv/cpu.c|  56 
  target/riscv/cpu.h|  10 +
  target/riscv/cpu_bits.h   |   7 +
  target/riscv/csr.c|  38 ++-
  target/riscv/helper.h |   3 +
  target/riscv/insn16.decode|  62 -
  target/riscv/insn_trans/trans_rvd.c.inc   |  18 ++
  target/riscv/insn_trans/trans_rvf.c.inc   |  18 ++
  target/riscv/insn_trans/trans_rvi.c.inc   |   4 +-
  target/riscv/insn_trans/trans_rvzce.c.inc | 313 ++
  target/riscv/machine.c|  19 ++
  target/riscv/meson.build  |   3 +-
  target/riscv/translate.c  |  15 +-
  target/riscv/zce_helper.c |  55 
  15 files changed, 833 insertions(+), 16 deletions(-)
  create mode 100644 target/riscv/insn_trans/trans_rvzce.c.inc
  create mode 100644 target/riscv/zce_helper.c

Re: [PATCH v2 02/14] target/arm: Unexport arm_gen_dynamic_sysreg_xml


On 21/2/23 03:19, Richard Henderson wrote:

This function is not used outside gdbstub.c.

Reviewed-by: Fabiano Rosas 
Signed-off-by: Richard Henderson 
---
  target/arm/cpu.h | 1 -
  target/arm/gdbstub.c | 2 +-
  2 files changed, 1 insertion(+), 2 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v2 03/14] target/arm: Move arm_gen_dynamic_svereg_xml to gdbstub64.c


On 21/2/23 03:19, Richard Henderson wrote:

The function is only used for aarch64, so move it to the
file that has the other aarch64 gdbstub stuff.  Move the
declaration to internals.h.

Reviewed-by: Fabiano Rosas 
Signed-off-by: Richard Henderson 
---
  target/arm/cpu.h   |   6 ---
  target/arm/internals.h |   1 +
  target/arm/gdbstub.c   | 120 -
  target/arm/gdbstub64.c | 118 
  4 files changed, 119 insertions(+), 126 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v2 01/14] target/arm: Normalize aarch64 gdbstub get/set function names


On 21/2/23 03:19, Richard Henderson wrote:

Make the form of the function names between fp and sve the same:
   - arm_gdb_*_svereg -> aarch64_gdb_*_sve_reg.
   - aarch64_fpu_gdb_*_reg -> aarch64_gdb_*_fpu_reg.

Reviewed-by: Fabiano Rosas 
Signed-off-by: Richard Henderson 
---
  target/arm/internals.h | 8 
  target/arm/gdbstub.c   | 9 +
  target/arm/gdbstub64.c | 8 
  3 files changed, 13 insertions(+), 12 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v3 6/6] Python: Drop support for Python 3.6

John Snow  writes:

> Python 3.6 was EOL 2021-12-31. Newer versions of upstream libraries have
> been dropping support for this version and it is becoming more
> cumbersome to support. Avocado-framework and qemu.qmp each have their
> own reasons for wanting to drop Python 3.6, but won't until QEMU does.

I'd prefer more thorough rationale.  I can dig it out of v2's review if
you like.  Might be best to wait for the rebase on Paolo's work, so we
know the context.

> Versions of Python available in our supported build platforms as of today,
> with optional versions available in parentheses:
>
> openSUSE Leap 15.4: 3.6.15 (3.9.10, 3.10.2)
> CentOS Stream 8:3.6.8  (3.8.13, 3.9.16)
> CentOS Stream 9:3.9.13
> Fedora 36:  3.10
> Fedora 37:  3.11
> Debian 11:  3.9.2
> Alpine 3.14, 3.15:  3.9.16
> Alpine 3.16, 3.17:  3.10.10
> Ubuntu 20.04 LTS:   3.8.10
> Ubuntu 22.04 LTS:   3.10.4
> NetBSD 9.3: 3.9.13*
> FreeBSD 12.4:   3.9.16
> FreeBSD 13.1:   3.9.16
> OpenBSD 7.2:3.9.16
>
> Note: Our VM tests install 3.7 specifically for freebsd and netbsd; the
> default for "python" or "python3" in FreeBSD is 3.9.16. NetBSD does not
> appear to have a default meta-package, but offers several options, the
> lowest of which is 3.7.15. "python39" appears to be a pre-requisite to
> one of the other packages we request in tests/vm/netbsd.
>
> Since it is safe under our supported platform policy, bump our minimum
> supported version of Python to 3.7.
>
> Signed-off-by: John Snow

Re: [PATCH v2 04/13] vdpa: move vhost reset after get vring base

2023-02-20 Thread Eugenio Perez Martin

On Tue, Feb 21, 2023 at 6:36 AM Jason Wang  wrote:
>
>
> 在 2023/2/8 17:42, Eugenio Pérez 写道:
> > The function vhost.c:vhost_dev_stop calls vhost operation
> > vhost_dev_start(false). In the case of vdpa it totally reset and wipes
> > the device, making the fetching of the vring base (virtqueue state) totally
> > useless.
> >
> > The kernel backend does not use vhost_dev_start vhost op callback, but
> > vhost-user do. A patch to make vhost_user_dev_start more similar to vdpa
> > is desirable, but it can be added on top.
> >
> > Signed-off-by: Eugenio Pérez 
> > ---
> >   include/hw/virtio/vhost-backend.h |  4 
> >   hw/virtio/vhost-vdpa.c| 22 --
> >   hw/virtio/vhost.c |  3 +++
> >   3 files changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/hw/virtio/vhost-backend.h 
> > b/include/hw/virtio/vhost-backend.h
> > index c5ab49051e..ec3fbae58d 100644
> > --- a/include/hw/virtio/vhost-backend.h
> > +++ b/include/hw/virtio/vhost-backend.h
> > @@ -130,6 +130,9 @@ typedef bool (*vhost_force_iommu_op)(struct vhost_dev 
> > *dev);
> >
> >   typedef int (*vhost_set_config_call_op)(struct vhost_dev *dev,
> >  int fd);
> > +
> > +typedef void (*vhost_reset_status_op)(struct vhost_dev *dev);
> > +
> >   typedef struct VhostOps {
> >   VhostBackendType backend_type;
> >   vhost_backend_init vhost_backend_init;
> > @@ -177,6 +180,7 @@ typedef struct VhostOps {
> >   vhost_get_device_id_op vhost_get_device_id;
> >   vhost_force_iommu_op vhost_force_iommu;
> >   vhost_set_config_call_op vhost_set_config_call;
> > +vhost_reset_status_op vhost_reset_status;
> >   } VhostOps;
> >
> >   int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index cbbe92ffe8..26e38a6aab 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -1152,14 +1152,23 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
> > *dev, bool started)
> >   if (started) {
> >   memory_listener_register(&v->listener, &address_space_memory);
> >   return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > -} else {
> > -vhost_vdpa_reset_device(dev);
> > -vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > -   VIRTIO_CONFIG_S_DRIVER);
> > -memory_listener_unregister(&v->listener);
> > +}
> >
> > -return 0;
> > +return 0;
> > +}
> > +
> > +static void vhost_vdpa_reset_status(struct vhost_dev *dev)
> > +{
> > +struct vhost_vdpa *v = dev->opaque;
> > +
> > +if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
> > +return;
> >   }
> > +
> > +vhost_vdpa_reset_device(dev);
> > +vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > +VIRTIO_CONFIG_S_DRIVER);
> > +memory_listener_unregister(&v->listener);
> >   }
> >
> >   static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
> > @@ -1346,4 +1355,5 @@ const VhostOps vdpa_ops = {
> >   .vhost_vq_get_addr = vhost_vdpa_vq_get_addr,
> >   .vhost_force_iommu = vhost_vdpa_force_iommu,
> >   .vhost_set_config_call = vhost_vdpa_set_config_call,
> > +.vhost_reset_status = vhost_vdpa_reset_status,
> >   };
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index eb8c4c378c..a266396576 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -2049,6 +2049,9 @@ void vhost_dev_stop(struct vhost_dev *hdev, 
> > VirtIODevice *vdev, bool vrings)
> >hdev->vqs + i,
> >hdev->vq_index + i);
> >   }
> > +if (hdev->vhost_ops->vhost_reset_status) {
> > +hdev->vhost_ops->vhost_reset_status(hdev);
> > +}
>
>
> This looks racy, if we don't suspend/reset the device, device can move
> last_avail_idx even after get_vring_base()?
>
> Instead of doing things like this, should we fallback to
> virtio_queue_restore_last_avail_idx() in this case?
>

Right, we can track if the device is suspended / SVQ and then return
an error in vring_get_base if it is not. Would that work?

Thanks!

> Thanks
>
>
> >
> >   if (vhost_dev_has_iommu(hdev)) {
> >   if (hdev->vhost_ops->vhost_set_iotlb_callback) {
>

Re: [PATCH v2 03/13] vdpa: add vhost_vdpa_suspend

2023-02-20 Thread Eugenio Perez Martin

On Tue, Feb 21, 2023 at 6:33 AM Jason Wang  wrote:
>
>
> 在 2023/2/21 13:27, Jason Wang 写道:
> >
> > 在 2023/2/8 17:42, Eugenio Pérez 写道:
> >> The function vhost.c:vhost_dev_stop fetches the vring base so the vq
> >> state can be migrated to other devices.  However, this is unreliable in
> >> vdpa, since we didn't signal the device to suspend the queues, making
> >> the value fetched useless.
> >>
> >> Suspend the device if possible before fetching first and subsequent
> >> vring bases.
> >>
> >> Moreover, vdpa totally reset and wipes the device at the last device
> >> before fetch its vrings base, making that operation useless in the last
> >> device. This will be fixed in later patches of this series.
> >
> >
> > It would be better not introduce a bug first and fix it in the
> > following patch.
> >
> >
> >>
> >> Signed-off-by: Eugenio Pérez 
> >> ---
> >>   hw/virtio/vhost-vdpa.c | 19 +++
> >>   hw/virtio/trace-events |  1 +
> >>   2 files changed, 20 insertions(+)
> >>
> >> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >> index 2e79fbe4b2..cbbe92ffe8 100644
> >> --- a/hw/virtio/vhost-vdpa.c
> >> +++ b/hw/virtio/vhost-vdpa.c
> >> @@ -1108,6 +1108,24 @@ static void vhost_vdpa_svqs_stop(struct
> >> vhost_dev *dev)
> >>   }
> >>   }
> >>   +static void vhost_vdpa_suspend(struct vhost_dev *dev)
> >> +{
> >> +struct vhost_vdpa *v = dev->opaque;
> >> +int r;
> >> +
> >> +if (!vhost_vdpa_first_dev(dev) ||
> >
> >
> > Any reason we need to use vhost_vdpa_first_dev() instead of replacing the
> >
> > if (started) {
> > } else {
> > vhost_vdpa_reset_device(dev);
> > 
> > }

I can also move the check to vhost_vdpa_dev_start, for sure.

>
> Ok, I think I kind of understand, so I think we need re-order the
> patches, at least patch 4 should come before this patch?
>

I think it is doable, yes. I'll check and come back to you.

Thanks!

> Thanks
>
>
> >
> >
> > We check
> >
> > if (dev->vq_index + dev->nvqs != dev->vq_index_end) in
> > vhost_vdpa_dev_start() but vhost_vdpa_first_dev() inside
> > vhost_vdpa_suspend(). This will result code that is hard to maintain.
> >
> > Thanks
> >
> >
> >> +!(dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) {
> >> +return;
> >> +}
> >> +
> >> +trace_vhost_vdpa_suspend(dev);
> >> +r = ioctl(v->device_fd, VHOST_VDPA_SUSPEND);
> >> +if (unlikely(r)) {
> >> +error_report("Cannot suspend: %s(%d)", g_strerror(errno),
> >> errno);
> >> +/* Not aborting since we're called from stop context */
> >> +}
> >> +}
> >> +
> >>   static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> >>   {
> >>   struct vhost_vdpa *v = dev->opaque;
> >> @@ -1122,6 +1140,7 @@ static int vhost_vdpa_dev_start(struct
> >> vhost_dev *dev, bool started)
> >>   }
> >>   vhost_vdpa_set_vring_ready(dev);
> >>   } else {
> >> +vhost_vdpa_suspend(dev);
> >>   vhost_vdpa_svqs_stop(dev);
> >>   vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
> >>   }
> >> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> >> index a87c5f39a2..8f8d05cf9b 100644
> >> --- a/hw/virtio/trace-events
> >> +++ b/hw/virtio/trace-events
> >> @@ -50,6 +50,7 @@ vhost_vdpa_set_vring_ready(void *dev) "dev: %p"
> >>   vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
> >>   vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size,
> >> uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags:
> >> 0x%"PRIx32
> >>   vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len)
> >> "dev: %p config: %p config_len: %"PRIu32
> >> +vhost_vdpa_suspend(void *dev) "dev: %p"
> >>   vhost_vdpa_dev_start(void *dev, bool started) "dev: %p started: %d"
> >>   vhost_vdpa_set_log_base(void *dev, uint64_t base, unsigned long
> >> long size, int refcnt, int fd, void *log) "dev: %p base: 0x%"PRIx64"
> >> size: %llu refcnt: %d fd: %d log: %p"
> >>   vhost_vdpa_set_vring_addr(void *dev, unsigned int index, unsigned
> >> int flags, uint64_t desc_user_addr, uint64_t used_user_addr, uint64_t
> >> avail_user_addr, uint64_t log_guest_addr) "dev: %p index: %u flags:
> >> 0x%x desc_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64"
> >> avail_user_addr: 0x%"PRIx64" log_guest_addr: 0x%"PRIx64
>

Re: [PATCH v3 0/6] Python: Drop support for Python 3.6

John Snow  writes:

> CI: https://gitlab.com/jsnow/qemu/-/pipelines/783612696
> [Updated for v3, still all green.]
> GL: https://gitlab.com/jsnow/qemu/-/commits/python-require-37
>
> Hi, discussion about this series is ongoing. This series (v3) is not
> meant to address all of that discussion, but rather is an updated
> baseline for what we are capable of right now, today, without much
> additional engineering. It's meant to serve as a reference for further
> discussion.

Misses the RFC tag then :)

> To my knowledge, the inconveniences caused by this patchset as currently
> written are:
>
> (1) Users of CentOS 8 and OpenSUSE 15.4 would need to install an
> additional python package that will exist side-by-side with their
> base platform's Python 3.6 package.
>
> "zypper install python39" or "dnf install python38" is enough;
> configure will do the rest of the work.
>
> It's my understanding that this is largely a non-issue.
>
> (2) Due to our Sphinx plugin that imports QAPI code from the tree,

I can read this as "Due to our Sphinx plugin (which by the way imports
some QAPI code)" or as "Due to out Sphinx plugin importing QAPI code".
The former is more accurate.  We need a newer Sphinx because we use a
plugin, the plugin is written in Python, so our new Python requirement
applies.  Fine print: the code the plugin imports from QAPI is going to
break first.

> distro-provided versions of Sphinx that are installed and tied to
> Python 3.6 will no longer be suitable. Users may forego building
> docs or install a suitable sphinx using "pip".
>
> It's my understanding that this one is "kind of a bummer".
>
> I feel that the inconvenience caused by (1) is minimized as is possible;
> the inconvenience caused by (2) is slightly worse and I concede the
> workaround has some complexities that I would otherwise seek to avoid.
>
> As far as I am aware, the way forward is to work with Paolo to implement
> a proper venv solution for the build tree that will help mitigate the
> fallout from (2) by automating the use of a pip-provided Sphinx in the
> cases where the distro-provided version is insufficient.

So, your current plan is to rebase this series less its DO-NOT-MERGE
parts, on top of Paolo's.  Correct?

> OK, seeya later!

Re: [PATCH v3 5/6] meson: prefer 'sphinx-build' to 'sphinx-build-3'

John Snow  writes:

> Once upon a time, "sphinx-build" on certain RPM platforms invoked
> specifically a Python 2.x version, while "sphinx-build-3" was a distro
> shim for the Python 3.x version.
>
> These days, none of our supported platforms utilize a 2.x version, so it
> should be safe to search for 'sphinx-build' prior to 'sphinx-build-3',
> which will prefer pip/venv installed versions of sphinx if they're
> available.
>
> This adds an extremely convenient ability to test document building
> ability in QEMU across multiple versions of Sphinx for the purposes of
> compatibility testing.
>
> Signed-off-by: John Snow 
> ---
>  docs/meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/docs/meson.build b/docs/meson.build
> index 9136fed3b73..906034f9a87 100644
> --- a/docs/meson.build
> +++ b/docs/meson.build
> @@ -1,5 +1,5 @@
>  if get_option('sphinx_build') == ''
> -  sphinx_build = find_program(['sphinx-build-3', 'sphinx-build'],
> +  sphinx_build = find_program(['sphinx-build', 'sphinx-build-3'],
>required: get_option('docs'))
>  else
>sphinx_build = find_program(get_option('sphinx_build'),

Do we still need to check for sphinx-build-3?  Or asked differently, is
there any supported build host that provides only sphinx-build-3?

Re: [PATCH v4 0/6] qapi: static typing conversion, pt5c

John Snow  writes:

> On Wed, Feb 15, 2023 at 8:39 AM Markus Armbruster  wrote:
>>
>> I had a few suggestions, but none of them requires a respin.  Let's
>> discuss them, and then I merge.
>
> Hiya, I lost track of things a little due to the other Python
> discussion. Who is waiting for whom?

Just two questions remain:

* PATCH 3: Dumb down check_keys() argument all the way to List[str]?

* PATCH 4: Suggested commit message addition okay?

We settle them, and then I'll take it from there.

Re: online blockdev-backup, a clarification (was: Summary on new backup interfaces in QEMU)

2023-02-20 Thread Vladimir Sementsov-Ogievskiy

On 20.02.23 18:18, John Maline wrote:

As a qemu newcomer I had a related question and confusion from reading existing
docs. Searching qemu-block, this seemed related to my question so I’ll ask…

On Mar 15, 2022, at 12:57 PM, Vladimir Sementsov-Ogievskiy
wrote:

Hi all!

Here I want to summarize new interfaces and use cases for backup in QEMU.

TODO for me: convert this into good rst documentation in docs/.

The existing docs I found at
https://qemu.readthedocs.io/en/latest/interop/live-block-operations.html#live-disk-backup-blockdev-backup-and-the-deprecated-drive-backup
are confusing me. This, if I’m understanding, seem clearer.

OK, let's begin.

First, note that drive-backup qmp command is deprecated.

Next, some terminology:

push backup: the whole process is inside QEMU process, also may be called "internal
backup"

pull backup: QEMU only exports a kind of snapshot (for example by NBD), and third party
software reads this export and stores it somehow, also called "external backup"

copy-before-write operations: We usually do backup of active disk, guest is
running and may write to the disk during the process of backup. When guest
wants to rewrite data region which is not backed up yet, we must stop this
guest write, and copy original data to somewhere before continuing guest write.
That's a copy-before-write operation.

image-fleecing: the technique that allows to export a "snapshotted" state of the active disk with help of
copy-before-write operations. We create a temporary image - target for copy-before-write operations, and provide an
interface to the user to read the "snapshotted" state. And for read, we do read from temporary image the data
which is already changed in original active disk, and we read unchanged data directly from active disk. The temporary
image itself is also called "reverse delta" or "reversed delta".

== Simple push backup ==

Just use blockdev-backup, nothing new here. I just note some technical details,
that are relatively new:

1. First, backup job inserts copy-before-write filter above source disk, to do
copy-before-write operation.
2. Created copy-before-write filter shares internal block-copy state with
backup job, so they work in collaboration, to not copy same things twice.

The simple case I’m aiming for matches a push backup. I’m OK w/ a snapshot.

Environment - macos 12.6 on arm processor, guest is aarch64 centos linux using
hvf accelerator. Qemu 7.2.

I assume what you describe w/ copy-before-write is behavior in qemu 7.2. I’m
fine if the Linux client needs to do a bit of log replay if I revert to a
backup.

In the docs I link above it talks as if a VM shutdown is recommended after the
job completes. Seems to ruin the whole point of an online backup. I tried
instead finishing w/ a blockdev-del and I see the backup file closed by qemu.
I’m guessing that’s an appropriate way to flush/complete the backup. In an
experiment, it seemed the generated backup worked as expected.

Yes, shutdown is unrelated. Also, block-jobs do flush target on finish, so it's
really synced after block-job completion event. Still, blockdev-del(target) is
right thing to do.

I’m hoping for confirmation or correction on my approach.

Specifically I’m doing the following QMP commands.

{"execute": "qmp_capabilities"}

{"execute":"blockdev-add",
"arguments":{"node-name":"backup-node", "driver":"qcow2", "file":{"driver":"file",
"filename":"backups/backup1.img"}}
}

{"execute":"blockdev-backup",
"arguments":{"device":"drive0", "job-id":"job0", "target":"backup-node",
"sync":"full"}
}

... watch many job state change events ...

The last one should be BLOCK_JOB_COMPLETED, you wait for it, and check "error"
field - if it exist the job is failed.

You also can poll with query-block-jobs command.

{"execute":"blockdev-del",
"arguments": {"node-name":"backup-node"}
}

Yes your approach is correct. Note that ideally, you also should do fs-freeze /
fs-thaw in guest around blockdev-backup command call, to be sure that the
moment in time when we start the backup (the final target image will correspond
to this moment in time) is consistent and we'll be able to boot from the backup
image later.

--
Best regards,
Vladimir

[PATCH] hw/riscv: Skip re-generating DT nodes for a given DTB

2023-02-20 Thread Bin Meng

Lanuch qemu-system-riscv64 with a given dtb for 'sifive_u' and 'virt'
machines, QEMU complains:

  qemu_fdt_add_subnode: Failed to create subnode /soc: FDT_ERR_EXISTS

The whole DT generation logic should be skipped when a given DTB is
present.

Fixes: b1f19f238cae ("hw/riscv: write bootargs 'chosen' FDT after 
riscv_load_kernel()")
Signed-off-by: Bin Meng 
---

 hw/riscv/sifive_u.c | 1 +
 hw/riscv/virt.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index ad3bb35b34..76db5ed3dd 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -118,6 +118,7 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry 
*memmap,
 error_report("load_device_tree() failed");
 exit(1);
 }
+return;
 } else {
 fdt = ms->fdt = create_device_tree(&fdt_size);
 if (!fdt) {
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 86c4adc0c9..0c7b4a1e46 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1014,6 +1014,7 @@ static void create_fdt(RISCVVirtState *s, const 
MemMapEntry *memmap)
 error_report("load_device_tree() failed");
 exit(1);
 }
+return;
 } else {
 ms->fdt = create_device_tree(&s->fdt_size);
 if (!ms->fdt) {
-- 
2.25.1

Re: [PATCH v2 05/13] vdpa: rewind at get_base, not set_base




在 2023/2/8 17:42, Eugenio Pérez 写道:

At this moment it is only possible to migrate to a vdpa device running
with x-svq=on. As a protective measure, the rewind of the inflight
descriptors was done at the destination. That way if the source sent a
virtqueue with inuse descriptors they are always discarded.

Since this series allows to migrate also to passthrough devices with no
SVQ, the right thing to do is to rewind at the source so the base of
vrings are correct.

Support for inflight descriptors may be added in the future.

Signed-off-by: Eugenio Pérez 



Acked-by: Jason Wang 

Thanks



---
  hw/virtio/vhost-vdpa.c | 24 +---
  1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 26e38a6aab..d99db0bd03 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1211,18 +1211,7 @@ static int vhost_vdpa_set_vring_base(struct vhost_dev 
*dev,
 struct vhost_vring_state *ring)
  {
  struct vhost_vdpa *v = dev->opaque;
-VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
  
-/*

- * vhost-vdpa devices does not support in-flight requests. Set all of them
- * as available.
- *
- * TODO: This is ok for networking, but other kinds of devices might
- * have problems with these retransmissions.
- */
-while (virtqueue_rewind(vq, 1)) {
-continue;
-}
  if (v->shadow_vqs_enabled) {
  /*
   * Device vring base was set at device start. SVQ base is handled by
@@ -1241,6 +1230,19 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev 
*dev,
  int ret;
  
  if (v->shadow_vqs_enabled) {

+VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
+
+/*
+ * vhost-vdpa devices does not support in-flight requests. Set all of
+ * them as available.
+ *
+ * TODO: This is ok for networking, but other kinds of devices might
+ * have problems with these retransmissions.
+ */
+while (virtqueue_rewind(vq, 1)) {
+continue;
+}
+
  ring->num = virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
  return 0;
  }

Re: [PATCH v2 04/13] vdpa: move vhost reset after get vring base




在 2023/2/8 17:42, Eugenio Pérez 写道:

The function vhost.c:vhost_dev_stop calls vhost operation
vhost_dev_start(false). In the case of vdpa it totally reset and wipes
the device, making the fetching of the vring base (virtqueue state) totally
useless.

The kernel backend does not use vhost_dev_start vhost op callback, but
vhost-user do. A patch to make vhost_user_dev_start more similar to vdpa
is desirable, but it can be added on top.

Signed-off-by: Eugenio Pérez 
---
  include/hw/virtio/vhost-backend.h |  4 
  hw/virtio/vhost-vdpa.c| 22 --
  hw/virtio/vhost.c |  3 +++
  3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/include/hw/virtio/vhost-backend.h 
b/include/hw/virtio/vhost-backend.h
index c5ab49051e..ec3fbae58d 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -130,6 +130,9 @@ typedef bool (*vhost_force_iommu_op)(struct vhost_dev *dev);
  
  typedef int (*vhost_set_config_call_op)(struct vhost_dev *dev,

 int fd);
+
+typedef void (*vhost_reset_status_op)(struct vhost_dev *dev);
+
  typedef struct VhostOps {
  VhostBackendType backend_type;
  vhost_backend_init vhost_backend_init;
@@ -177,6 +180,7 @@ typedef struct VhostOps {
  vhost_get_device_id_op vhost_get_device_id;
  vhost_force_iommu_op vhost_force_iommu;
  vhost_set_config_call_op vhost_set_config_call;
+vhost_reset_status_op vhost_reset_status;
  } VhostOps;
  
  int vhost_backend_update_device_iotlb(struct vhost_dev *dev,

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index cbbe92ffe8..26e38a6aab 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1152,14 +1152,23 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, 
bool started)
  if (started) {
  memory_listener_register(&v->listener, &address_space_memory);
  return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
-} else {
-vhost_vdpa_reset_device(dev);
-vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
-   VIRTIO_CONFIG_S_DRIVER);
-memory_listener_unregister(&v->listener);
+}
  
-return 0;

+return 0;
+}
+
+static void vhost_vdpa_reset_status(struct vhost_dev *dev)
+{
+struct vhost_vdpa *v = dev->opaque;
+
+if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
+return;
  }
+
+vhost_vdpa_reset_device(dev);
+vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
+VIRTIO_CONFIG_S_DRIVER);
+memory_listener_unregister(&v->listener);
  }
  
  static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,

@@ -1346,4 +1355,5 @@ const VhostOps vdpa_ops = {
  .vhost_vq_get_addr = vhost_vdpa_vq_get_addr,
  .vhost_force_iommu = vhost_vdpa_force_iommu,
  .vhost_set_config_call = vhost_vdpa_set_config_call,
+.vhost_reset_status = vhost_vdpa_reset_status,
  };
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index eb8c4c378c..a266396576 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -2049,6 +2049,9 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice 
*vdev, bool vrings)
   hdev->vqs + i,
   hdev->vq_index + i);
  }
+if (hdev->vhost_ops->vhost_reset_status) {
+hdev->vhost_ops->vhost_reset_status(hdev);
+}



This looks racy, if we don't suspend/reset the device, device can move 
last_avail_idx even after get_vring_base()?


Instead of doing things like this, should we fallback to 
virtio_queue_restore_last_avail_idx() in this case?


Thanks


  
  if (vhost_dev_has_iommu(hdev)) {

  if (hdev->vhost_ops->vhost_set_iotlb_callback) {

Re: [PATCH v2 03/13] vdpa: add vhost_vdpa_suspend




在 2023/2/21 13:27, Jason Wang 写道:


在 2023/2/8 17:42, Eugenio Pérez 写道:

The function vhost.c:vhost_dev_stop fetches the vring base so the vq
state can be migrated to other devices.  However, this is unreliable in
vdpa, since we didn't signal the device to suspend the queues, making
the value fetched useless.

Suspend the device if possible before fetching first and subsequent
vring bases.

Moreover, vdpa totally reset and wipes the device at the last device
before fetch its vrings base, making that operation useless in the last
device. This will be fixed in later patches of this series.



It would be better not introduce a bug first and fix it in the 
following patch.





Signed-off-by: Eugenio Pérez 
---
  hw/virtio/vhost-vdpa.c | 19 +++
  hw/virtio/trace-events |  1 +
  2 files changed, 20 insertions(+)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 2e79fbe4b2..cbbe92ffe8 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1108,6 +1108,24 @@ static void vhost_vdpa_svqs_stop(struct 
vhost_dev *dev)

  }
  }
  +static void vhost_vdpa_suspend(struct vhost_dev *dev)
+{
+    struct vhost_vdpa *v = dev->opaque;
+    int r;
+
+    if (!vhost_vdpa_first_dev(dev) ||



Any reason we need to use vhost_vdpa_first_dev() instead of replacing the

if (started) {
} else {
    vhost_vdpa_reset_device(dev);
    
}



Ok, I think I kind of understand, so I think we need re-order the 
patches, at least patch 4 should come before this patch?


Thanks





We check

if (dev->vq_index + dev->nvqs != dev->vq_index_end) in 
vhost_vdpa_dev_start() but vhost_vdpa_first_dev() inside 
vhost_vdpa_suspend(). This will result code that is hard to maintain.


Thanks



+    !(dev->backend_cap & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) {
+    return;
+    }
+
+    trace_vhost_vdpa_suspend(dev);
+    r = ioctl(v->device_fd, VHOST_VDPA_SUSPEND);
+    if (unlikely(r)) {
+    error_report("Cannot suspend: %s(%d)", g_strerror(errno), 
errno);

+    /* Not aborting since we're called from stop context */
+    }
+}
+
  static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
  {
  struct vhost_vdpa *v = dev->opaque;
@@ -1122,6 +1140,7 @@ static int vhost_vdpa_dev_start(struct 
vhost_dev *dev, bool started)

  }
  vhost_vdpa_set_vring_ready(dev);
  } else {
+    vhost_vdpa_suspend(dev);
  vhost_vdpa_svqs_stop(dev);
  vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
  }
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index a87c5f39a2..8f8d05cf9b 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -50,6 +50,7 @@ vhost_vdpa_set_vring_ready(void *dev) "dev: %p"
  vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
  vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, 
uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 
0x%"PRIx32
  vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) 
"dev: %p config: %p config_len: %"PRIu32

+vhost_vdpa_suspend(void *dev) "dev: %p"
  vhost_vdpa_dev_start(void *dev, bool started) "dev: %p started: %d"
  vhost_vdpa_set_log_base(void *dev, uint64_t base, unsigned long 
long size, int refcnt, int fd, void *log) "dev: %p base: 0x%"PRIx64" 
size: %llu refcnt: %d fd: %d log: %p"
  vhost_vdpa_set_vring_addr(void *dev, unsigned int index, unsigned 
int flags, uint64_t desc_user_addr, uint64_t used_user_addr, uint64_t 
avail_user_addr, uint64_t log_guest_addr) "dev: %p index: %u flags: 
0x%x desc_user_addr: 0x%"PRIx64" used_user_addr: 0x%"PRIx64" 
avail_user_addr: 0x%"PRIx64" log_guest_addr: 0x%"PRIx64

Re: [PATCH v2 03/13] vdpa: add vhost_vdpa_suspend