Re: [PATCH V2 18/21] virito-net: use "qps" instead of "queues" when possible

[Michael S. Tsirkin]

On Mon, Sep 06, 2021 at 11:42:41AM +0800, Jason Wang wrote:
> On Sun, Sep 5, 2021 at 4:42 AM Michael S. Tsirkin  wrote:
> >
> > On Fri, Sep 03, 2021 at 05:10:28PM +0800, Jason Wang wrote:
> > > Most of the time, "queues" really means queue pairs. So this patch
> > > switch to use "qps" to avoid confusion.
> > >
> > > Signed-off-by: Jason Wang 
> >
> > This is far from a standard terminology, except for the people
> > like me, who's mind is permanently warped by close contact with infiniband
> > hardware. Please eschew abbreviation, just say queue_pairs.
> 
> Ok, I will do that in the next version.
> 
> Thanks


Also, s/virito/virtio/
Happens to me too, often enough that I have an abbreviation set up in
vimrc.

> >
> > > ---
> > >  hw/net/vhost_net.c |   6 +-
> > >  hw/net/virtio-net.c| 150 -
> > >  include/hw/virtio/virtio-net.h |   4 +-
> > >  3 files changed, 80 insertions(+), 80 deletions(-)
> > >
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index 7e0b60b4d9..b40fdfa625 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -337,7 +337,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState 
> > > *ncs,
> > >  if (i < data_qps) {
> > >  peer = qemu_get_peer(ncs, i);
> > >  } else { /* Control Virtqueue */
> > > -peer = qemu_get_peer(ncs, n->max_queues);
> > > +peer = qemu_get_peer(ncs, n->max_qps);
> > >  }
> > >
> > >  net = get_vhost_net(peer);
> > > @@ -362,7 +362,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState 
> > > *ncs,
> > >  if (i < data_qps) {
> > >  peer = qemu_get_peer(ncs, i);
> > >  } else {
> > > -peer = qemu_get_peer(ncs, n->max_queues);
> > > +peer = qemu_get_peer(ncs, n->max_qps);
> > >  }
> > >  r = vhost_net_start_one(get_vhost_net(peer), dev);
> > >
> > > @@ -412,7 +412,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState 
> > > *ncs,
> > >  if (i < data_qps) {
> > >  peer = qemu_get_peer(ncs, i);
> > >  } else {
> > > -peer = qemu_get_peer(ncs, n->max_queues);
> > > +peer = qemu_get_peer(ncs, n->max_qps);
> > >  }
> > >  vhost_net_stop_one(get_vhost_net(peer), dev);
> > >  }
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 8fccbaa44c..0a5d9862ec 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -54,7 +54,7 @@
> > >  #define VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE 256
> > >  #define VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE 256
> > >
> > > -/* for now, only allow larger queues; with virtio-1, guest can downsize 
> > > */
> > > +/* for now, only allow larger qps; with virtio-1, guest can downsize */
> > >  #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
> > >  #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
> > >
> > > @@ -131,7 +131,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
> > > uint8_t *config)
> > >  int ret = 0;
> > >  memset(, 0 , sizeof(struct virtio_net_config));
> > >  virtio_stw_p(vdev, , n->status);
> > > -virtio_stw_p(vdev, _virtqueue_pairs, n->max_queues);
> > > +virtio_stw_p(vdev, _virtqueue_pairs, n->max_qps);
> > >  virtio_stw_p(vdev, , n->net_conf.mtu);
> > >  memcpy(netcfg.mac, n->mac, ETH_ALEN);
> > >  virtio_stl_p(vdev, , n->net_conf.speed);
> > > @@ -243,7 +243,7 @@ static void virtio_net_vhost_status(VirtIONet *n, 
> > > uint8_t status)
> > >  {
> > >  VirtIODevice *vdev = VIRTIO_DEVICE(n);
> > >  NetClientState *nc = qemu_get_queue(n->nic);
> > > -int queues = n->multiqueue ? n->max_queues : 1;
> > > +int qps = n->multiqueue ? n->max_qps : 1;
> > >
> > >  if (!get_vhost_net(nc->peer)) {
> > >  return;
> > > @@ -266,7 +266,7 @@ static void virtio_net_vhost_status(VirtIONet *n, 
> > > uint8_t status)
> > >  /* Any packets outstanding? Purge them to avoid touching rings
> > >   * when vhost is running.
> > >   */
> > > -for (i = 0;  i < queues; i++) {
> > > +for (i = 0;  i < qps; i++) {
> > >  NetClientState *qnc = qemu_get_subqueue(n->nic, i);
> > >
> > >  /* Purge both directions: TX and RX. */
> > > @@ -285,14 +285,14 @@ static void virtio_net_vhost_status(VirtIONet *n, 
> > > uint8_t status)
> > >  }
> > >
> > >  n->vhost_started = 1;
> > > -r = vhost_net_start(vdev, n->nic->ncs, queues, 0);
> > > +r = vhost_net_start(vdev, n->nic->ncs, qps, 0);
> > >  if (r < 0) {
> > >  error_report("unable to start vhost net: %d: "
> > >   "falling back on userspace virtio", -r);
> > >  n->vhost_started = 0;
> > >  }
> > >  } else {
> > > -vhost_net_stop(vdev, n->nic->ncs, queues, 0);
> > > +vhost_net_stop(vdev, 

Re: [PATCH v10 02/16] target/riscv: fix clzw implementation to operate on arg1

[Alistair Francis]

On Sun, Sep 5, 2021 at 6:36 AM Philipp Tomsich  wrote:
>
> The refactored gen_clzw() uses ret as its argument, instead of arg1.
> Fix it.
>
> Signed-off-by: Philipp Tomsich 

Reviewed-by: Alistair Francis 

Alistair

> ---
>
> Changes in v10:
> - New patch, fixing regressions discovered with x264_r.
>
>  target/riscv/insn_trans/trans_rvb.c.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvb.c.inc 
> b/target/riscv/insn_trans/trans_rvb.c.inc
> index c0a6e25826..6c85c89f6d 100644
> --- a/target/riscv/insn_trans/trans_rvb.c.inc
> +++ b/target/riscv/insn_trans/trans_rvb.c.inc
> @@ -349,7 +349,7 @@ GEN_TRANS_SHADD(3)
>
>  static void gen_clzw(TCGv ret, TCGv arg1)
>  {
> -tcg_gen_clzi_tl(ret, ret, 64);
> +tcg_gen_clzi_tl(ret, arg1, 64);
>  tcg_gen_subi_tl(ret, ret, 32);
>  }
>
> --
> 2.25.1
>
>

Re: [PATCH v10 01/16] target/riscv: Introduce temporary in gen_add_uw()

[Alistair Francis]

On Sun, Sep 5, 2021 at 6:40 AM Philipp Tomsich  wrote:
>
> Following the recent changes in translate.c, gen_add_uw() causes
> failures on CF3 and SPEC2017 due to the reuse of arg1.  Fix these
> regressions by introducing a temporary.
>
> Signed-off-by: Philipp Tomsich 

Reviewed-by: Alistair Francis 

Alistair

> ---
>
> Changes in v10:
> - new patch
>
>  target/riscv/insn_trans/trans_rvb.c.inc | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvb.c.inc 
> b/target/riscv/insn_trans/trans_rvb.c.inc
> index b72e76255c..c0a6e25826 100644
> --- a/target/riscv/insn_trans/trans_rvb.c.inc
> +++ b/target/riscv/insn_trans/trans_rvb.c.inc
> @@ -624,8 +624,10 @@ GEN_TRANS_SHADD_UW(3)
>
>  static void gen_add_uw(TCGv ret, TCGv arg1, TCGv arg2)
>  {
> -tcg_gen_ext32u_tl(arg1, arg1);
> -tcg_gen_add_tl(ret, arg1, arg2);
> +TCGv t = tcg_temp_new();
> +tcg_gen_ext32u_tl(t, arg1);
> +tcg_gen_add_tl(ret, t, arg2);
> +tcg_temp_free(t);
>  }
>
>  static bool trans_add_uw(DisasContext *ctx, arg_add_uw *a)
> --
> 2.25.1
>
>

Re: [PATCH v2 06/22] target/riscv: Add AIA cpu feature

[Alistair Francis]

On Thu, Sep 2, 2021 at 9:52 PM Anup Patel  wrote:
>
> We define a CPU feature for AIA CSR support in RISC-V CPUs which
> can be set by machine/device emulation. The RISC-V CSR emulation
> will also check this feature for emulating AIA CSRs.
>
> Signed-off-by: Anup Patel 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 6fe1cc67e5..2cde2df7be 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -77,7 +77,8 @@ enum {
>  RISCV_FEATURE_MMU,
>  RISCV_FEATURE_PMP,
>  RISCV_FEATURE_EPMP,
> -RISCV_FEATURE_MISA
> +RISCV_FEATURE_MISA,
> +RISCV_FEATURE_AIA
>  };
>
>  #define PRIV_VERSION_1_10_0 0x00011000
> --
> 2.25.1
>
>

Re: [PATCH v2 05/22] target/riscv: Allow setting CPU feature from machine/device emulation

[Alistair Francis]

On Thu, Sep 2, 2021 at 9:42 PM Anup Patel  wrote:
>
> The machine or device emulation should be able to force set certain
> CPU features because:
> 1) We can have certain CPU features which are in-general optional
>but implemented by RISC-V CPUs on machine.
> 2) We can have devices which require certain CPU feature. For example,
>AIA IMSIC devices expects AIA CSRs implemented by RISC-V CPUs.
>
> Signed-off-by: Anup Patel 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 11 +++
>  target/riscv/cpu.h |  5 +
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 0ade6ad144..9dc9d04923 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -137,11 +137,6 @@ static void set_vext_version(CPURISCVState *env, int 
> vext_ver)
>  env->vext_ver = vext_ver;
>  }
>
> -static void set_feature(CPURISCVState *env, int feature)
> -{
> -env->features |= (1ULL << feature);
> -}
> -
>  static void set_resetvec(CPURISCVState *env, target_ulong resetvec)
>  {
>  #ifndef CONFIG_USER_ONLY
> @@ -423,18 +418,18 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  }
>
>  if (cpu->cfg.mmu) {
> -set_feature(env, RISCV_FEATURE_MMU);
> +riscv_set_feature(env, RISCV_FEATURE_MMU);
>  }
>
>  if (cpu->cfg.pmp) {
> -set_feature(env, RISCV_FEATURE_PMP);
> +riscv_set_feature(env, RISCV_FEATURE_PMP);
>
>  /*
>   * Enhanced PMP should only be available
>   * on harts with PMP support
>   */
>  if (cpu->cfg.epmp) {
> -set_feature(env, RISCV_FEATURE_EPMP);
> +riscv_set_feature(env, RISCV_FEATURE_EPMP);
>  }
>  }
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 59b36f758f..6fe1cc67e5 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -323,6 +323,11 @@ static inline bool riscv_feature(CPURISCVState *env, int 
> feature)
>  return env->features & (1ULL << feature);
>  }
>
> +static inline void riscv_set_feature(CPURISCVState *env, int feature)
> +{
> +env->features |= (1ULL << feature);
> +}
> +
>  #include "cpu_user.h"
>  #include "cpu_bits.h"
>
> --
> 2.25.1
>
>

Re: [PATCH] target/riscv: Fix satp write

[LIU Zhiwei]

On 2021/9/6 上午11:26, Bin Meng wrote:

On Mon, Sep 6, 2021 at 11:23 AM LIU Zhiwei  wrote:


On 2021/9/2 上午10:47, Bin Meng wrote:

On Thu, Sep 2, 2021 at 10:44 AM LIU Zhiwei  wrote:

On 2021/9/2 上午9:59, Bin Meng wrote:

On Thu, Sep 2, 2021 at 9:02 AM LIU Zhiwei  wrote:

On 2021/9/1 下午9:05, Bin Meng wrote:

On Wed, Sep 1, 2021 at 8:51 PM LIU Zhiwei  wrote:

These variables should be target_ulong. If truncated to int,
the bool conditions they indicate will be wrong.

As satp is very important for Linux, this bug almost fails every boot.

Could you please describe which Linux configuration is broken?

I use the image from:

https://gitlab.com/c-sky/buildroot/-/jobs/1251564514/artifacts/browse/output/images/


 I have
a 64-bit 5.10 kernel and it boots fine.

The login is mostly OK for me. But the busybox can't run properly.

Which kernel version is this?

5.10.4

Could you please investigate and
indicate in the commit message?

I just tested current qemu-system-riscv64 can boot to Ubuntu 20.04
distro user space.

Very strange.  This will cause tlb_flush can't be called in this function.


Did your kernel enable asid?

Yes. Is it matter?

Not sure, the tbl_flush is on the ASID path. I suspect the kernel we
(Alistair and me) tested did not enable ASID.
In my opinion, if the ASID is open, we should not flush tlb when ASID 
changes in most cases.

If ASID is not open.

Regards,
Bin

Re: [PATCH] target/riscv: Fix satp write

[Alistair Francis]

On Wed, Sep 1, 2021 at 10:51 PM LIU Zhiwei  wrote:
>
> These variables should be target_ulong. If truncated to int,
> the bool conditions they indicate will be wrong.
>
> As satp is very important for Linux, this bug almost fails every boot.
>
> Signed-off-by: LIU Zhiwei 

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/csr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 50a2c3a3b4..ba9818f6a5 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -986,7 +986,7 @@ static RISCVException read_satp(CPURISCVState *env, int 
> csrno,
>  static RISCVException write_satp(CPURISCVState *env, int csrno,
>   target_ulong val)
>  {
> -int vm, mask, asid;
> +target_ulong vm, mask, asid;
>
>  if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
>  return RISCV_EXCP_NONE;
> --
> 2.25.1
>
>

[PATCH] usb-storage: tag usb_msd_csw as packed struct

[Gerd Hoffmann]

Without this the struct has the wrong size: sizeof() evaluates
to 16 instead of 13.  In most cases the bug is hidden by the
fact that guests submits a buffer which is exactly 13 bytes
long, so the padding added by the compiler is simply ignored.

But sometimes guests submit a larger buffer and expect a short
transfer, which does not work properly with the wrong struct
size.

Cc: vintagepc...@protonmail.com
Signed-off-by: Gerd Hoffmann 
---
 include/hw/usb/msd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
index 7538c54569bf..54e9f38bda46 100644
--- a/include/hw/usb/msd.h
+++ b/include/hw/usb/msd.h
@@ -17,7 +17,7 @@ enum USBMSDMode {
 USB_MSDM_CSW /* Command Status.  */
 };
 
-struct usb_msd_csw {
+struct QEMU_PACKED usb_msd_csw {
 uint32_t sig;
 uint32_t tag;
 uint32_t residue;
-- 
2.31.1

Re: [PATCH V2 0/3] virtio: Add vhost-user-i2c device's support

[Viresh Kumar]

On 04-09-21, 15:44, Michael S. Tsirkin wrote:
> So I'm not sure whether it's appropriate to merge this right now.

This is already merged.

> There are several spec change proposals before the virtio TC
> and I did not investigate whether this code reflects the
> spec before or after these changes.

I believe you are talking about the zero-length stuff, right ?

This patchset doesn't depend on that and won't change with or without
those patches. The backend is implemented separately in Rust.

https://github.com/rust-vmm/vhost-device

> It seems prudent to wait
> until the spec changes are finalized and voted on, in any case.

I will update both the Linux driver and Rust implementation once we
accept the changes for spec.

-- 
viresh

Re: Virtualizing HP PA-RISC unix station other than B160L

[Warner Losh]

Hey Thierry,

I'm answering the following as with my 'Unix history buff' hat on, not due
to direct knowledge of HP PA and porting to it.
I used an HP/Snake workstation for about 9 months back in the day is as
close as I got..

On Sun, Sep 5, 2021 at 6:57 PM  wrote:

> Hi everyone,
>
>
>
> Thank you for your answers.
>
> First of all, I made a mistake : the HP-UX 10.20 kernel (vmunix) is in the
> /stand (not /boot) filesystem.
>
> I tried replacing /stand of the physical machine iso image with the /stand
> filesytem of the emulated B160L because I supposed that drivers are linked
> with the vmunix kernel. Maybe it's false.
>
http://ibgwww.colorado.edu/~lessem/psyc5112/usail/man/hpux/hpux.1.html has
the boot loader manual in it for HP-UX 10.20.
http://www.datadisk.co.uk/html_docs/hp/hpux_cs.htm has a number of
interesting tid-bits that might be useful in reconfiguring.
IIRC, in the early days, you needed to rebuild / reconfigure kernels for
each different kind of system, so you may have
some issues there (though the model numbers are close enough you may not
need to)...

> Furthermore, the /dev tree is important for dealing with the drivers.
>
HP UX 11 introduced devfs, so 10.20 has the mknod entries in the filesystem.

> I will continue to search for which files are hardware related. Maybe
> someone has done this research before ?
>
I haven't done so for HP-UX, but I've done a lot unix history things. 10.20
was released in 1996 so it's late enough
to have things like devfs and loadable kernel modules. However, HP-UX
lagged the cutting edge by a number of
years, despite HP having had a good head start at all things Unix... So
there's some things that are decidedly
retro about it that will be quite unfamiliar unless you (a) lived "the good
old days" or (b) studied the good old
days...

Good luck with this...

Warner


>
>
> Best regards,
>
> Thierry
>
>
>
> -Message d'origine-
> De : Helge Deller 
> Envoyé : dimanche 5 septembre 2021 22:32
> À : Richard Henderson ;
> thierry.br...@gmail.com
> Cc : qemu-devel@nongnu.org Developers ;
> linux-parisc 
> Objet : Re: Virtualizing HP PA-RISC unix station other than B160L
>
>
>
> Hi Thierry,
>
>
>
> On 9/5/21 3:24 PM, Richard Henderson wrote:
>
> > On Sun, 5 Sep 2021, 10:30 ,  > wrote:
>
> > For my company (Nexter Systems, France), I am using qemu-system-hppa
>
> > for virtualizing HP PA-RISC workstations. That works well. You have
>
> > made a very good job !
>
>
>
> Thanks.
>
>
>
> > But my machines are other than B160L (for example B180L), and I have
>
> > to completely reinstall HP-UX on each emulated machine.
>
> > If I do an iso system disk image of my B180L, this iso isn't bootable
>
> > on qemu-system-hppa.
>
> >
>
> > Thus, my questions are :
>
> >
>
> > * Is it planned to emulate other HP unix workstations than B160L (for
>
> > example B180L) ?
>
>
>
> Maybe at some point a 64-bit capable system, e.g. C3000, and maybe an
> older 32-bit system, e.g. 715/64.
>
> For the 64bit system additions to the emulated firmware and additional
> 64-bit qemu support is necessary, and for the 715/64 we need an additional
> NCR710 SCSI driver.
>
> Both are lots of work.
>
>
>
> The B180L is exactly the same as the B160L, with just a faster CPU:
>
> https://www.openpa.net/systems/hp-visualize_b132l_b160l_b180l.html
>
>
>
> > * Or, what changes should I make to my iso image to do it usable ? If
>
> > I replace the /boot /stand filesystem of the B180L image with the B160L
> one,
>
> > I get a kernel panic at boot time.
>
>
>
> I don't know HP-UX so well. I could imagine that your physical machines
> have different SCSI controller cards which are used by HP-UX, and which
> aren't emulated in qemu yet. That's maybe why qemu can't boot your already
> installed images.
>
> If you post the output I maybe can give more info.
>
>
>
> > Helge is the one that did all the hw support, I just did the CPU.
>
> > There are no real plans to do another machine. I'm not familiar with
>
> > the specs between the HP machines to know how much work that would be.
>
>
>
> There is a very good overview of the various HP machines at openPA:
>
> https://www.openpa.net/systems/
>
>
>
> Helge
>

Re: [PATCH V2 17/21] vhost-net: control virtqueue support

[Jason Wang]

On Sun, Sep 5, 2021 at 4:40 AM Michael S. Tsirkin  wrote:
>
> On Fri, Sep 03, 2021 at 05:10:27PM +0800, Jason Wang wrote:
> > We assume there's no cvq in the past, this is not true when we need
> > control virtqueue support for vhost-user backends. So this patch
> > implements the control virtqueue support for vhost-net. As datapath,
> > the control virtqueue is also required to be coupled with the
> > NetClientState. The vhost_net_start/stop() are tweaked to accept the
> > number of datapath queue pairs plus the the number of control
> > virtqueue for us to start and stop the vhost device.
> >
> > Signed-off-by: Jason Wang 
>
>
> Fails build:
>
> FAILED: libcommon.fa.p/hw_net_vhost_net-stub.c.o
> cc -Ilibcommon.fa.p -I. -Iqapi -Itrace -Iui -Iui/shader 
> -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard 
> -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 
> -I/usr/include/nspr4 -I/usr/include/libmount -I/usr/include/blkid 
> -I/usr/include/pixman-1 -I/usr/include/p11-kit-1 -I/usr/include/SDL2 
> -I/usr/include/libpng16 -I/usr/include/virgl -I/usr/include/libusb-1.0 
> -I/usr/include/slirp -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 
> -I/usr/include/harfbuzz -I/usr/include/freetype2 -I/usr/include/fribidi 
> -I/usr/include/libxml2 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 
> -I/usr/include/gio-unix-2.0 -I/usr/include/atk-1.0 
> -I/usr/include/at-spi2-atk/2.0 -I/usr/include/dbus-1.0 
> -I/usr/lib64/dbus-1.0/include -I/usr/include/at-spi-2.0 
> -I/usr/include/vte-2.91 -I/usr/include/capstone -fdiagnostics-color=auto 
> -pipe -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem 
> /scm/qemu/linux-headers -isystem linux-headers -iquote . -iquote /scm/qemu 
> -iquote /scm/qemu/include -iquote /scm/qemu/disas/libvixl -iquote 
> /scm/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -m64 -mcx16 
> -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
> -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes 
> -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration 
> -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k 
> -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels 
> -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs 
> -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIC 
> -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -DNCURSES_WIDECHAR 
> -DSTRUCT_IOVEC_DEFINED -D_REENTRANT -Wno-undef -MD -MQ 
> libcommon.fa.p/hw_net_vhost_net-stub.c.o -MF 
> libcommon.fa.p/hw_net_vhost_net-stub.c.o.d -o 
> libcommon.fa.p/hw_net_vhost_net-stub.c.o -c ../hw/net/vhost_net-stub.c
> ../hw/net/vhost_net-stub.c:34:5: error: conflicting types for 
> ‘vhost_net_start’
>34 | int vhost_net_start(VirtIODevice *dev,
>   | ^~~
> In file included from ../hw/net/vhost_net-stub.c:19:
> /scm/qemu/include/net/vhost_net.h:24:5: note: previous declaration of 
> ‘vhost_net_start’ was here
>24 | int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>   | ^~~
> ../hw/net/vhost_net-stub.c:40:6: error: conflicting types for ‘vhost_net_stop’
>40 | void vhost_net_stop(VirtIODevice *dev,
>   |  ^~
> In file included from ../hw/net/vhost_net-stub.c:19:
> /scm/qemu/include/net/vhost_net.h:26:6: note: previous declaration of 
> ‘vhost_net_stop’ was here
>26 | void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>   |  ^~
> ninja: build stopped: subcommand failed.
> make[1]: *** [Makefile:156: run-ninja] Error 1

Will fix this.

Thanks

>
>
>
> > ---
> >  hw/net/vhost_net.c  | 43 ++---
> >  hw/net/virtio-net.c |  4 ++--
> >  include/net/vhost_net.h |  6 --
> >  3 files changed, 38 insertions(+), 15 deletions(-)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 386ec2eaa2..7e0b60b4d9 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -315,11 +315,14 @@ static void vhost_net_stop_one(struct vhost_net *net,
> >  }
> >
> >  int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> > -int total_queues)
> > +int data_qps, int cvq)
> >  {
> >  BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
> >  VirtioBusState *vbus = VIRTIO_BUS(qbus);
> >  VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> > +int total_notifiers = data_qps * 2 + cvq;
> > +VirtIONet *n = VIRTIO_NET(dev);
> > +int nvhosts = data_qps + cvq;
> >  struct vhost_net *net;
> >  int r, e, i;
> >  NetClientState *peer;
> > @@ -329,9 +332,14 @@ int vhost_net_start(VirtIODevice *dev, NetClientState 
> > *ncs,
> >  return -ENOSYS;
> >  }
> >
> > -for (i = 0; i < total_queues; i++) {
> > +for (i = 0; i < nvhosts; i++) {
> > +
> > +if (i < data_qps) {
> > +peer = qemu_get_peer(ncs, i);
> > +

Re: [PATCH V2 18/21] virito-net: use "qps" instead of "queues" when possible

[Jason Wang]

On Sun, Sep 5, 2021 at 4:42 AM Michael S. Tsirkin  wrote:
>
> On Fri, Sep 03, 2021 at 05:10:28PM +0800, Jason Wang wrote:
> > Most of the time, "queues" really means queue pairs. So this patch
> > switch to use "qps" to avoid confusion.
> >
> > Signed-off-by: Jason Wang 
>
> This is far from a standard terminology, except for the people
> like me, who's mind is permanently warped by close contact with infiniband
> hardware. Please eschew abbreviation, just say queue_pairs.

Ok, I will do that in the next version.

Thanks

>
> > ---
> >  hw/net/vhost_net.c |   6 +-
> >  hw/net/virtio-net.c| 150 -
> >  include/hw/virtio/virtio-net.h |   4 +-
> >  3 files changed, 80 insertions(+), 80 deletions(-)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 7e0b60b4d9..b40fdfa625 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -337,7 +337,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState 
> > *ncs,
> >  if (i < data_qps) {
> >  peer = qemu_get_peer(ncs, i);
> >  } else { /* Control Virtqueue */
> > -peer = qemu_get_peer(ncs, n->max_queues);
> > +peer = qemu_get_peer(ncs, n->max_qps);
> >  }
> >
> >  net = get_vhost_net(peer);
> > @@ -362,7 +362,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState 
> > *ncs,
> >  if (i < data_qps) {
> >  peer = qemu_get_peer(ncs, i);
> >  } else {
> > -peer = qemu_get_peer(ncs, n->max_queues);
> > +peer = qemu_get_peer(ncs, n->max_qps);
> >  }
> >  r = vhost_net_start_one(get_vhost_net(peer), dev);
> >
> > @@ -412,7 +412,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState 
> > *ncs,
> >  if (i < data_qps) {
> >  peer = qemu_get_peer(ncs, i);
> >  } else {
> > -peer = qemu_get_peer(ncs, n->max_queues);
> > +peer = qemu_get_peer(ncs, n->max_qps);
> >  }
> >  vhost_net_stop_one(get_vhost_net(peer), dev);
> >  }
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 8fccbaa44c..0a5d9862ec 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -54,7 +54,7 @@
> >  #define VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE 256
> >  #define VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE 256
> >
> > -/* for now, only allow larger queues; with virtio-1, guest can downsize */
> > +/* for now, only allow larger qps; with virtio-1, guest can downsize */
> >  #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
> >  #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
> >
> > @@ -131,7 +131,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
> > uint8_t *config)
> >  int ret = 0;
> >  memset(, 0 , sizeof(struct virtio_net_config));
> >  virtio_stw_p(vdev, , n->status);
> > -virtio_stw_p(vdev, _virtqueue_pairs, n->max_queues);
> > +virtio_stw_p(vdev, _virtqueue_pairs, n->max_qps);
> >  virtio_stw_p(vdev, , n->net_conf.mtu);
> >  memcpy(netcfg.mac, n->mac, ETH_ALEN);
> >  virtio_stl_p(vdev, , n->net_conf.speed);
> > @@ -243,7 +243,7 @@ static void virtio_net_vhost_status(VirtIONet *n, 
> > uint8_t status)
> >  {
> >  VirtIODevice *vdev = VIRTIO_DEVICE(n);
> >  NetClientState *nc = qemu_get_queue(n->nic);
> > -int queues = n->multiqueue ? n->max_queues : 1;
> > +int qps = n->multiqueue ? n->max_qps : 1;
> >
> >  if (!get_vhost_net(nc->peer)) {
> >  return;
> > @@ -266,7 +266,7 @@ static void virtio_net_vhost_status(VirtIONet *n, 
> > uint8_t status)
> >  /* Any packets outstanding? Purge them to avoid touching rings
> >   * when vhost is running.
> >   */
> > -for (i = 0;  i < queues; i++) {
> > +for (i = 0;  i < qps; i++) {
> >  NetClientState *qnc = qemu_get_subqueue(n->nic, i);
> >
> >  /* Purge both directions: TX and RX. */
> > @@ -285,14 +285,14 @@ static void virtio_net_vhost_status(VirtIONet *n, 
> > uint8_t status)
> >  }
> >
> >  n->vhost_started = 1;
> > -r = vhost_net_start(vdev, n->nic->ncs, queues, 0);
> > +r = vhost_net_start(vdev, n->nic->ncs, qps, 0);
> >  if (r < 0) {
> >  error_report("unable to start vhost net: %d: "
> >   "falling back on userspace virtio", -r);
> >  n->vhost_started = 0;
> >  }
> >  } else {
> > -vhost_net_stop(vdev, n->nic->ncs, queues, 0);
> > +vhost_net_stop(vdev, n->nic->ncs, qps, 0);
> >  n->vhost_started = 0;
> >  }
> >  }
> > @@ -309,11 +309,11 @@ static int 
> > virtio_net_set_vnet_endian_one(VirtIODevice *vdev,
> >  }
> >
> >  static bool virtio_net_set_vnet_endian(VirtIODevice *vdev, NetClientState 
> > *ncs,
> > -   int queues, bool enable)
> > +   int qps, bool enable)

Re: [PATCH] target/riscv: Fix satp write

[Bin Meng]

On Mon, Sep 6, 2021 at 11:23 AM LIU Zhiwei  wrote:
>
>
> On 2021/9/2 上午10:47, Bin Meng wrote:
> > On Thu, Sep 2, 2021 at 10:44 AM LIU Zhiwei  wrote:
> >>
> >> On 2021/9/2 上午9:59, Bin Meng wrote:
> >>> On Thu, Sep 2, 2021 at 9:02 AM LIU Zhiwei  wrote:
>  On 2021/9/1 下午9:05, Bin Meng wrote:
> > On Wed, Sep 1, 2021 at 8:51 PM LIU Zhiwei  wrote:
> >> These variables should be target_ulong. If truncated to int,
> >> the bool conditions they indicate will be wrong.
> >>
> >> As satp is very important for Linux, this bug almost fails every boot.
> > Could you please describe which Linux configuration is broken?
>  I use the image from:
> 
>  https://gitlab.com/c-sky/buildroot/-/jobs/1251564514/artifacts/browse/output/images/
> 
> > I have
> > a 64-bit 5.10 kernel and it boots fine.
>  The login is mostly OK for me. But the busybox can't run properly.
> >>> Which kernel version is this?
> >> 5.10.4
> >>> Could you please investigate and
> >>> indicate in the commit message?
> >>>
> >>> I just tested current qemu-system-riscv64 can boot to Ubuntu 20.04
> >>> distro user space.
> >> Very strange.  This will cause tlb_flush can't be called in this function.
> >>
> > Did your kernel enable asid?
>
> Yes. Is it matter?

Not sure, the tbl_flush is on the ASID path. I suspect the kernel we
(Alistair and me) tested did not enable ASID.

Regards,
Bin

Re: [PATCH] target/riscv: Fix satp write

[LIU Zhiwei]

On 2021/9/2 上午10:47, Bin Meng wrote:

On Thu, Sep 2, 2021 at 10:44 AM LIU Zhiwei  wrote:


On 2021/9/2 上午9:59, Bin Meng wrote:

On Thu, Sep 2, 2021 at 9:02 AM LIU Zhiwei  wrote:

On 2021/9/1 下午9:05, Bin Meng wrote:

On Wed, Sep 1, 2021 at 8:51 PM LIU Zhiwei  wrote:

These variables should be target_ulong. If truncated to int,
the bool conditions they indicate will be wrong.

As satp is very important for Linux, this bug almost fails every boot.

Could you please describe which Linux configuration is broken?

I use the image from:

https://gitlab.com/c-sky/buildroot/-/jobs/1251564514/artifacts/browse/output/images/


I have
a 64-bit 5.10 kernel and it boots fine.

The login is mostly OK for me. But the busybox can't run properly.

Which kernel version is this?

5.10.4

Could you please investigate and
indicate in the commit message?

I just tested current qemu-system-riscv64 can boot to Ubuntu 20.04
distro user space.

Very strange.  This will cause tlb_flush can't be called in this function.


Did your kernel enable asid?


Yes. Is it matter?

Thanks,
Zhiwei



Regards,
Bin

Re: [PATCH v2 02/20] ppc/pnv: Add an assert when calculating the RAM distribution on chips

[David Gibson]

On Thu, Sep 02, 2021 at 03:09:10PM +0200, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater 

Applied to ppc-for-6.2, thanks.

> ---
> 
>  v2: fixed assert value ...
> 
>  hw/ppc/pnv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 03c86508d2f7..71e45515f136 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -723,6 +723,8 @@ static uint64_t pnv_chip_get_ram_size(PnvMachineState 
> *pnv, int chip_id)
>  return QEMU_ALIGN_DOWN(ram_per_chip, 1 * MiB);
>  }
>  
> +assert(pnv->num_chips > 1);
> +
>  ram_per_chip = (machine->ram_size - 1 * GiB) / (pnv->num_chips - 1);
>  return chip_id == 0 ? 1 * GiB : QEMU_ALIGN_DOWN(ram_per_chip, 1 * MiB);
>  }

-- 
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: fix setting of CR flags in bcdcfsq

[David Gibson]

On Mon, Aug 23, 2021 at 12:02:35PM -0300, Luis Pires wrote:
> According to the ISA, CR should be set based on the source value, and
> not on the packed decimal result.
> The way this was implemented would cause GT, LT and EQ to be set
> incorrectly when the source value was too large and the 31 least
> significant digits of the packed decimal result ended up being all zero.
> This would happen for source values of +/-10^31, +/-10^32, etc.
> 
> The new implementation fixes this and also skips the result calculation
> altogether in case of src overflow.
> 
> Signed-off-by: Luis Pires 

Applied to ppc-for-6.2, thanks.

> ---
>  target/ppc/int_helper.c | 61 -
>  1 file changed, 48 insertions(+), 13 deletions(-)
> 
> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
> index efa833ef64..de56056429 100644
> --- a/target/ppc/int_helper.c
> +++ b/target/ppc/int_helper.c
> @@ -2498,10 +2498,26 @@ uint32_t helper_bcdctz(ppc_avr_t *r, ppc_avr_t *b, 
> uint32_t ps)
>  return cr;
>  }
>  
> +/**
> + * Compare 2 128-bit unsigned integers, passed in as unsigned 64-bit pairs
> + *
> + * Returns:
> + * > 0 if ahi|alo > bhi|blo,
> + * 0 if ahi|alo == bhi|blo,
> + * < 0 if ahi|alo < bhi|blo
> + */
> +static inline int ucmp128(uint64_t alo, uint64_t ahi,
> +  uint64_t blo, uint64_t bhi)
> +{
> +return (ahi == bhi) ?
> +(alo > blo ? 1 : (alo == blo ? 0 : -1)) :
> +(ahi > bhi ? 1 : -1);
> +}
> +
>  uint32_t helper_bcdcfsq(ppc_avr_t *r, ppc_avr_t *b, uint32_t ps)
>  {
>  int i;
> -int cr = 0;
> +int cr;
>  uint64_t lo_value;
>  uint64_t hi_value;
>  ppc_avr_t ret = { .u64 = { 0, 0 } };
> @@ -2510,28 +2526,47 @@ uint32_t helper_bcdcfsq(ppc_avr_t *r, ppc_avr_t *b, 
> uint32_t ps)
>  lo_value = -b->VsrSD(1);
>  hi_value = ~b->VsrD(0) + !lo_value;
>  bcd_put_digit(, 0xD, 0);
> +
> +cr = CRF_LT;
>  } else {
>  lo_value = b->VsrD(1);
>  hi_value = b->VsrD(0);
>  bcd_put_digit(, bcd_preferred_sgn(0, ps), 0);
> -}
>  
> -if (divu128(_value, _value, 1000ULL) ||
> -lo_value > ULL) {
> -cr = CRF_SO;
> +if (hi_value == 0 && lo_value == 0) {
> +cr = CRF_EQ;
> +} else {
> +cr = CRF_GT;
> +}
>  }
>  
> -for (i = 1; i < 16; hi_value /= 10, i++) {
> -bcd_put_digit(, hi_value % 10, i);
> -}
> +/*
> + * Check src limits: abs(src) <= 10^31 - 1
> + *
> + * 10^31 - 1 = 0x007e37be2022 c0914b267fff
> + */
> +if (ucmp128(lo_value, hi_value,
> +0xc0914b267fffULL, 0x7e37be2022ULL) > 0) {
> +cr |= CRF_SO;
>  
> -for (; i < 32; lo_value /= 10, i++) {
> -bcd_put_digit(, lo_value % 10, i);
> -}
> +/*
> + * According to the ISA, if src wouldn't fit in the destination
> + * register, the result is undefined.
> + * In that case, we leave r unchanged.
> + */
> +} else {
> +divu128(_value, _value, 1000ULL);
>  
> -cr |= bcd_cmp_zero();
> +for (i = 1; i < 16; hi_value /= 10, i++) {
> +bcd_put_digit(, hi_value % 10, i);
> +}
>  
> -*r = ret;
> +for (; i < 32; lo_value /= 10, i++) {
> +bcd_put_digit(, lo_value % 10, i);
> +}
> +
> +*r = ret;
> +}
>  
>  return cr;
>  }

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


signature.asc
Description: PGP signature

Re: [PATCH v2 01/20] docs/system: ppc: Update the URL for OpenPOWER firmware images

[David Gibson]

On Thu, Sep 02, 2021 at 03:09:09PM +0200, Cédric Le Goater wrote:
> This also fixes a small skiboot/skiroot typo and removes the links to
> the specific POWER8 and POWER9 images since the firmware images can be
> used to run all machines.
> 
> Signed-off-by: Cédric Le Goater 

Applied to ppc-for-6.2, thanks.

> ---
> 
>  v2: fixed URL
> 
>  docs/system/ppc/powernv.rst | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/docs/system/ppc/powernv.rst b/docs/system/ppc/powernv.rst
> index 4c4cdea527e2..86186b7d2cb7 100644
> --- a/docs/system/ppc/powernv.rst
> +++ b/docs/system/ppc/powernv.rst
> @@ -53,8 +53,7 @@ initramfs ``skiroot``. Source code can be found on GitHub:
>  
>https://github.com/open-power.
>  
> -Prebuilt images of ``skiboot`` and ``skiboot`` are made available on the 
> `OpenPOWER `__ 
> site. To boot a POWER9 machine, use the `witherspoon 
> `__
>  images. For POWER8, use
> -the `palmetto 
> `__
>  images.
> +Prebuilt images of ``skiboot`` and ``skiroot`` are made available on the 
> `OpenPOWER `__ site.
>  
>  QEMU includes a prebuilt image of ``skiboot`` which is updated when a
>  more recent version is required by the models.

-- 
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: How does qemu detect the completion of interrupt execution？

[Duo jia]

Thank you for your explanation.

 And finishing the execution of the interrupt routine will automatically
> allow a pending second interrupt to be taken immediately
>

I think this is a hardware feature. But how to achieve it with qemu

Peter Maydell  于2021年9月3日周五 下午7:20写道：

> On Fri, 3 Sept 2021 at 11:54, Duo jia  wrote:
> >
> > I do some support on STM8 arch, the reference manual link is:
> >>
> >>
> https://www.st.com/resource/en/reference_manual/cd00218714-stm8l050j3-stm8l051f3-stm8l052c6-stm8l052r8-mcus-and-stm8l151l152-stm8l162-stm8al31-stm8al3l-lines-stmicroelectronics.pdf
> >
> > I don't kown when to check the "PENDING" because I can't get the when
> interrrupt exec over.
> > Is there a similar implementation in qemu？
>
> Thanks for the link. Looking at that spec, the condition
> for "interrupt execution has finished" is "the guest executes an
> IRET instruction". However, you don't need to do anything special
> at that point (beyond making the IRET instruction do the things
> it should do with restoring registers).
>
> If you look at the diagram, the check you need to make
> is "do the I1:I0 bits for this pending interrupt specify a
> higher priority than the values in CCR.I1 and I0?". That is
> how you determine "interrupt has higher priority than current one"
> to know whether to actually take the interrupt.
>
> On interrupt entry the CCR bits are set from the ITC_SPRx registers,
> which means that taking an interrupt automatically sets the CPU
> priority such that the interrupt cannot be nested. And
> finishing the execution of the interrupt routine will automatically
> allow a pending second interrupt to be taken immediately, because
> the IRET instruction restores the old values of CCR.I1,I0, and
> those old values then mean that the CPU is at a priority that
> permits the pending interrupt to be taken.
>
> thanks
> -- PMM
>

RE: Virtualizing HP PA-RISC unix station other than B160L

[thierry.briot]

Hi everyone,

 

Thank you for your answers.

First of all, I made a mistake : the HP-UX 10.20 kernel (vmunix) is in the 
/stand (not /boot) filesystem.

I tried replacing /stand of the physical machine iso image with the /stand 
filesytem of the emulated B160L because I supposed that drivers are linked with 
the vmunix kernel. Maybe it's false. 

Furthermore, the /dev tree is important for dealing with the drivers.

 

I will continue to search for which files are hardware related. Maybe someone 
has done this research before ?

 

Best regards,

Thierry

 

-Message d'origine-
De : Helge Deller  
Envoyé : dimanche 5 septembre 2021 22:32
À : Richard Henderson ; thierry.br...@gmail.com
Cc : qemu-devel@nongnu.org Developers ; linux-parisc 

Objet : Re: Virtualizing HP PA-RISC unix station other than B160L

 

Hi Thierry,

 

On 9/5/21 3:24 PM, Richard Henderson wrote:

> On Sun, 5 Sep 2021, 10:30 , < 
>  
> thierry.br...@gmail.com > wrote:

> For my company (Nexter Systems, France), I am using qemu-system-hppa 

> for virtualizing HP PA-RISC workstations. That works well. You have 

> made a very good job !

 

Thanks.

 

> But my machines are other than B160L (for example B180L), and I have 

> to completely reinstall HP-UX on each emulated machine.

> If I do an iso system disk image of my B180L, this iso isn't bootable 

> on qemu-system-hppa.

> 

> Thus, my questions are :

> 

> * Is it planned to emulate other HP unix workstations than B160L (for 

> example B180L) ?

 

Maybe at some point a 64-bit capable system, e.g. C3000, and maybe an older 
32-bit system, e.g. 715/64.

For the 64bit system additions to the emulated firmware and additional 64-bit 
qemu support is necessary, and for the 715/64 we need an additional NCR710 SCSI 
driver.

Both are lots of work.

 

The B180L is exactly the same as the B160L, with just a faster CPU:

  
https://www.openpa.net/systems/hp-visualize_b132l_b160l_b180l.html

 

> * Or, what changes should I make to my iso image to do it usable ? If 

> I replace the /boot /stand filesystem of the B180L image with the B160L one, 

> I get a kernel panic at boot time.

 

I don't know HP-UX so well. I could imagine that your physical machines have 
different SCSI controller cards which are used by HP-UX, and which aren't 
emulated in qemu yet. That's maybe why qemu can't boot your already installed 
images.

If you post the output I maybe can give more info.

 

> Helge is the one that did all the hw support, I just did the CPU.

> There are no real plans to do another machine. I'm not familiar with 

> the specs between the HP machines to know how much work that would be.

 

There is a very good overview of the various HP machines at openPA:

  https://www.openpa.net/systems/

 

Helge

Re: [PATCH v3 33/43] bsd-user: Make cpu_model and cpu_type visible to all of main.c

[Warner Losh]

On Sun, Sep 5, 2021 at 12:57 PM Kyle Evans  wrote:

> On Thu, Sep 2, 2021 at 6:53 PM  wrote:
> >
> > From: Warner Losh 
> >
> > cpu_model and cpu_type will be used future commits, so move them from
> > main() scoped to file scoped.
> >
> > Signed-off-by: Warner Losh 
> > Acked-by: Richard Henderson 
> > ---
> >  bsd-user/main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> I think we should reduce this one to just moving cpu_type. cpu_model
> is really only used in main() to derive the appropriate cpu_type,
> which we do use later
>

Fair point. I think I'm going to drop this patch from the series (yea, back
to 42)
and work it out with you on the bsd-user 'blitz' branch and try again in a
future
patch round.

Warner


> >
> > diff --git a/bsd-user/main.c b/bsd-user/main.c
> > index 71fd9d5aba..50c8fdc1e2 100644
> > --- a/bsd-user/main.c
> > +++ b/bsd-user/main.c
> > @@ -54,6 +54,8 @@
> >  int singlestep;
> >  unsigned long mmap_min_addr;
> >  uintptr_t guest_base;
> > +static const char *cpu_model;
> > +static const char *cpu_type;
> >  bool have_guest_base;
> >  unsigned long reserved_va;
> >
> > @@ -201,8 +203,6 @@ static void save_proc_pathname(char *argv0)
> >  int main(int argc, char **argv)
> >  {
> >  const char *filename;
> > -const char *cpu_model;
> > -const char *cpu_type;
> >  const char *log_file = NULL;
> >  const char *log_mask = NULL;
> >  const char *seed_optarg = NULL;
> > --
> > 2.32.0
> >
>

Re: [PATCH v3] hw/arm/aspeed: Add Fuji machine type

[Cédric Le Goater]

On 9/5/21 8:55 PM, p...@fb.com wrote:
> From: Peter Delevoryas 
> 
> This adds a new machine type "fuji-bmc" based on the following device tree:
> 
> https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts
> 
> Most of the i2c devices are not there, they're added here:
> 
> https://github.com/facebook/openbmc/blob/helium/meta-facebook/meta-fuji/recipes-utils/openbmc-utils/files/setup_i2c.sh
> 
> I tested this by building a Fuji image from Facebook's OpenBMC repo,
> booting, and ssh'ing from host-to-guest.
> 
> Signed-off-by: Peter Delevoryas 

There are a few checkpatch warnings that I fixed directly.
Anyhow,

Reviewed-by: Cédric Le Goater 

I will wait a little before resending the PR :)

I have pushed on my tree [1] the fuji machine and a few fixes for
the ADC model. This is still WIP and if you have some ADC tests
that come to mind, they would be welcomed.

Thanks,

C.

[1]  https://github.com/legoater/qemu/commits/aspeed-6.2



> ---
>  hw/arm/aspeed.c | 112 
>  1 file changed, 112 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 7a9459340c..cc2d721ac7 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -159,6 +159,10 @@ struct AspeedMachineState {
>  #define RAINIER_BMC_HW_STRAP1 0x
>  #define RAINIER_BMC_HW_STRAP2 0x
>  
> +/* Fuji hardware value */
> +#define FUJI_BMC_HW_STRAP10x
> +#define FUJI_BMC_HW_STRAP20x
> +
>  /*
>   * The max ram region is for firmwares that scan the address space
>   * with load/store to guess how much RAM the SoC has.
> @@ -772,6 +776,90 @@ static void rainier_bmc_i2c_init(AspeedMachineState *bmc)
>  aspeed_eeprom_init(aspeed_i2c_get_bus(>i2c, 15), 0x50, 64 * KiB);
>  }
>  
> +static void get_pca9548_channels(I2CBus *bus, uint8_t mux_addr, I2CBus 
> **channels) {
> +I2CSlave *mux = i2c_slave_create_simple(bus, "pca9548", mux_addr);
> +for (int i = 0; i < 8; i++) {
> +channels[i] = pca954x_i2c_get_bus(mux, i);
> +}
> +}
> +
> +#define TYPE_LM75 TYPE_TMP105
> +#define TYPE_TMP75 TYPE_TMP105
> +#define TYPE_TMP422 "tmp422"
> +
> +static void fuji_bmc_i2c_init(AspeedMachineState *bmc)
> +{
> +AspeedSoCState *soc = >soc;
> +I2CBus *i2c[144] = {};
> +
> +for (int i = 0; i < 16; i++) {
> +i2c[i] = aspeed_i2c_get_bus(>i2c, i);
> +}
> +I2CBus *i2c180 = i2c[2];
> +I2CBus *i2c480 = i2c[8];
> +I2CBus *i2c600 = i2c[11];
> +
> +get_pca9548_channels(i2c180, 0x70, [16]);
> +get_pca9548_channels(i2c480, 0x70, [24]);
> +// NOTE: The device tree skips [32, 40) in the alias numbering, so we do
> +// the same here.
> +get_pca9548_channels(i2c600, 0x77, [40]);
> +get_pca9548_channels(i2c[24], 0x71, [48]);
> +get_pca9548_channels(i2c[25], 0x72, [56]);
> +get_pca9548_channels(i2c[26], 0x76, [64]);
> +get_pca9548_channels(i2c[27], 0x76, [72]);
> +for (int i = 0; i < 8; i++) {
> +get_pca9548_channels(i2c[40 + i], 0x76, [80 + i * 8]);
> +}
> +
> +i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
> +i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4d);
> +
> +aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB);
> +aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB);
> +aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB);
> +
> +i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x48);
> +i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x49);
> +i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x4a);
> +i2c_slave_create_simple(i2c[3], TYPE_TMP422, 0x4c);
> +
> +aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB);
> +i2c_slave_create_simple(i2c[8], TYPE_LM75, 0x4a);
> +
> +i2c_slave_create_simple(i2c[50], TYPE_LM75, 0x4c);
> +aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB);
> +i2c_slave_create_simple(i2c[51], TYPE_TMP75, 0x48);
> +i2c_slave_create_simple(i2c[52], TYPE_TMP75, 0x49);
> +
> +i2c_slave_create_simple(i2c[59], TYPE_TMP75, 0x48);
> +i2c_slave_create_simple(i2c[60], TYPE_TMP75, 0x49);
> +
> +aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB);
> +i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x49);
> +i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x48);
> +aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB);
> +aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB);
> +aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB);
> +aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB);
> +
> +aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB);
> +i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x49);
> +i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x48);
> +aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB);
> +aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB);
> +aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB);
> +aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB);
> +aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB);
> +
> +for (int i = 0; i < 8; i++) {
> +aspeed_eeprom_init(i2c[81 + i * 8], 0x56, 64 * KiB);
> +

Re: Virtualizing HP PA-RISC unix station other than B160L

[Helge Deller]

Hi Thierry,

On 9/5/21 3:24 PM, Richard Henderson wrote:

On Sun, 5 Sep 2021, 10:30 , mailto:thierry.br...@gmail.com>> wrote:
For my company (Nexter Systems, France), I am using qemu-system-hppa
for virtualizing HP PA-RISC workstations. That works well. You have
made a very good job !


Thanks.


But my machines are other than B160L (for example B180L), and I have
to completely reinstall HP-UX on each emulated machine.
If I do an iso system disk image of my B180L, this iso isn't bootable
on qemu-system-hppa.

Thus, my questions are :

* Is it planned to emulate other HP unix workstations than B160L (for
example B180L) ?


Maybe at some point a 64-bit capable system, e.g. C3000, and maybe
an older 32-bit system, e.g. 715/64.
For the 64bit system additions to the emulated firmware and additional
64-bit qemu support is necessary,
and for the 715/64 we need an additional NCR710 SCSI driver.
Both are lots of work.

The B180L is exactly the same as the B160L, with just a faster CPU:
https://www.openpa.net/systems/hp-visualize_b132l_b160l_b180l.html


* Or, what changes should I make to my iso image to do it usable ? If
I replace the /boot filesystem of the B180L image with the B160L one,
I get a kernel panic at boot time.


I don't know HP-UX so well. I could imagine that your physical machines
have different SCSI controller cards which are used by HP-UX, and which
aren't emulated in qemu yet. That's maybe why qemu can't boot your already
installed images.
If you post the output I maybe can give more info.


Helge is the one that did all the hw support, I just did the CPU.
There are no real plans to do another machine. I'm not familiar with
the specs between the HP machines to know how much work that would
be.


There is a very good overview of the various HP machines at openPA:
https://www.openpa.net/systems/

Helge

Re: [PATCH v3 26/43] bsd-user: *BSD specific siginfo defintions

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:55 PM  wrote:
>
> From: Warner Losh 
>
> Add FreeBSD, NetBSD and OpenBSD values for the various signal info types
> and defines to decode different signals to discover more information
> about the specific signal types.
>
> Signed-off-by: Stacey Son 
> Signed-off-by: Warner Losh 
> Acked-by: Richard Henderson 
> ---
>  bsd-user/freebsd/target_os_siginfo.h | 145 +++
>  bsd-user/freebsd/target_os_signal.h  |  78 ++
>  bsd-user/i386/target_arch_signal.h   |  94 +
>  bsd-user/netbsd/target_os_siginfo.h  |  82 +++
>  bsd-user/netbsd/target_os_signal.h   |  69 +
>  bsd-user/openbsd/target_os_siginfo.h |  82 +++
>  bsd-user/openbsd/target_os_signal.h  |  69 +
>  bsd-user/qemu.h  |   1 +
>  bsd-user/syscall_defs.h  |  10 --
>  bsd-user/x86_64/target_arch_signal.h |  94 +
>  10 files changed, 714 insertions(+), 10 deletions(-)
>  create mode 100644 bsd-user/freebsd/target_os_siginfo.h
>  create mode 100644 bsd-user/freebsd/target_os_signal.h
>  create mode 100644 bsd-user/i386/target_arch_signal.h
>  create mode 100644 bsd-user/netbsd/target_os_siginfo.h
>  create mode 100644 bsd-user/netbsd/target_os_signal.h
>  create mode 100644 bsd-user/openbsd/target_os_siginfo.h
>  create mode 100644 bsd-user/openbsd/target_os_signal.h
>  create mode 100644 bsd-user/x86_64/target_arch_signal.h
>

Reviewed-by: Kyle Evans 

Re: [PATCH V2] block/rbd: implement bdrv_co_block_status

[Ilya Dryomov]

On Thu, Sep 2, 2021 at 4:12 PM Peter Lieven  wrote:
>
> Am 24.08.21 um 22:39 schrieb Ilya Dryomov:
> > On Mon, Aug 23, 2021 at 11:38 AM Peter Lieven  wrote:
> >> Am 22.08.21 um 23:02 schrieb Ilya Dryomov:
> >>> On Tue, Aug 10, 2021 at 3:41 PM Peter Lieven  wrote:
>  the qemu rbd driver currently lacks support for bdrv_co_block_status.
>  This results mainly in incorrect progress during block operations (e.g.
>  qemu-img convert with an rbd image as source).
> 
>  This patch utilizes the rbd_diff_iterate2 call from librbd to detect
>  allocated and unallocated (all zero areas).
> 
>  To avoid querying the ceph OSDs for the answer this is only done if
>  the image has the fast-diff features which depends on the object-map
> >>> Hi Peter,
> >>>
> >>> Nit: "has the fast-diff feature which depends on the object-map and
> >>> exclusive-lock features"
> >>
> >> will reword in V3.
> >>
> >>
>  and exclusive-lock. In this case it is guaranteed that the information
>  is present in memory in the librbd client and thus very fast.
> 
>  If fast-diff is not available all areas are reported to be allocated
>  which is the current behaviour if bdrv_co_block_status is not 
>  implemented.
> 
>  Signed-off-by: Peter Lieven 
>  ---
>  V1->V2:
>  - add commit comment [Stefano]
>  - use failed_post_open [Stefano]
>  - remove redundant assert [Stefano]
>  - add macro+comment for the magic -9000 value [Stefano]
>  - always set *file if its non NULL [Stefano]
> 
> block/rbd.c | 125 
> 1 file changed, 125 insertions(+)
> 
>  diff --git a/block/rbd.c b/block/rbd.c
>  index dcf82b15b8..8692e76f40 100644
>  --- a/block/rbd.c
>  +++ b/block/rbd.c
>  @@ -88,6 +88,7 @@ typedef struct BDRVRBDState {
> char *namespace;
> uint64_t image_size;
> uint64_t object_size;
>  +uint64_t features;
> } BDRVRBDState;
> 
> typedef struct RBDTask {
>  @@ -983,6 +984,13 @@ static int qemu_rbd_open(BlockDriverState *bs, 
>  QDict *options, int flags,
> s->image_size = info.size;
> s->object_size = info.obj_size;
> 
>  +r = rbd_get_features(s->image, >features);
>  +if (r < 0) {
>  +error_setg_errno(errp, -r, "error getting image features from 
>  %s",
>  + s->image_name);
>  +goto failed_post_open;
>  +}
> >>> The object-map and fast-diff features can be enabled/disabled while the
> >>> image is open so this should probably go to qemu_rbd_co_block_status().
> >>>
>  +
> /* If we are using an rbd snapshot, we must be r/o, otherwise
>  * leave as-is */
> if (s->snap != NULL) {
>  @@ -1259,6 +1267,122 @@ static ImageInfoSpecific 
>  *qemu_rbd_get_specific_info(BlockDriverState *bs,
> return spec_info;
> }
> 
>  +typedef struct rbd_diff_req {
>  +uint64_t offs;
>  +uint64_t bytes;
>  +int exists;
>  +} rbd_diff_req;
>  +
>  +/*
>  + * rbd_diff_iterate2 allows to interrupt the exection by returning a 
>  negative
>  + * value in the callback routine. Choose a value that does not conflict 
>  with
>  + * an existing exitcode and return it if we want to prematurely stop the
>  + * execution because we detected a change in the allocation status.
>  + */
>  +#define QEMU_RBD_EXIT_DIFF_ITERATE2 -9000
>  +
>  +static int qemu_rbd_co_block_status_cb(uint64_t offs, size_t len,
>  +   int exists, void *opaque)
>  +{
>  +struct rbd_diff_req *req = opaque;
>  +
>  +assert(req->offs + req->bytes <= offs);
>  +
>  +if (req->exists && offs > req->offs + req->bytes) {
>  +/*
>  + * we started in an allocated area and jumped over an 
>  unallocated area,
>  + * req->bytes contains the length of the allocated area before 
>  the
>  + * unallocated area. stop further processing.
>  + */
>  +return QEMU_RBD_EXIT_DIFF_ITERATE2;
>  +}
>  +if (req->exists && !exists) {
>  +/*
>  + * we started in an allocated area and reached a hole. 
>  req->bytes
>  + * contains the length of the allocated area before the hole.
>  + * stop further processing.
>  + */
>  +return QEMU_RBD_EXIT_DIFF_ITERATE2;
>  +}
>  +if (!req->exists && exists && offs > req->offs) {
>  +/*
>  + * we started in an unallocated area and hit the first allocated
>  + * block. req->bytes must be set to the length of the 
>  unallocated area
>  + * before the allocated area. stop further 

Re: [PATCH v3 30/43] bsd-user: elf cleanup

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:56 PM  wrote:
>
> From: Warner Losh 
>
> Move OS-dependent defines into target_os_elf.h. Move the architectural
> dependent stuff into target_arch_elf.h. Adjust elfload.c to use
> target_create_elf_tables instead of create_elf_tables.
>
> Signed-off-by: Warner Losh 
> Signed-off-by: Stacey Son 
> Signed-off-by: Kyle Evans 
> Signed-off-by: Justin Hibbits 
> Signed-off-by: Alexander Kabaev 
> Acked-by: Richard Henderson 
> ---
>  bsd-user/elfload.c   | 191 ---
>  bsd-user/freebsd/target_os_elf.h | 137 ++
>  bsd-user/netbsd/target_os_elf.h  | 146 +++
>  bsd-user/openbsd/target_os_elf.h | 146 +++
>  bsd-user/qemu.h  |   1 +
>  5 files changed, 454 insertions(+), 167 deletions(-)
>  create mode 100644 bsd-user/freebsd/target_os_elf.h
>  create mode 100644 bsd-user/netbsd/target_os_elf.h
>  create mode 100644 bsd-user/openbsd/target_os_elf.h

Reviewed-by: Kyle Evans 

Re: [PATCH v3 18/43] bsd-user: save the path to the qemu emulator

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:53 PM  wrote:
>
> From: Warner Losh 
>
> Save the path to the qemu emulator. This will be used later when we have
> a more complete implementation of exec.
>
> Signed-off-by: Stacey Son 
> Signed-off-by: Warner Losh 
> Acked-by: Richard Henderson 
> ---
>  bsd-user/main.c | 21 +
>  bsd-user/qemu.h |  1 +
>  2 files changed, 22 insertions(+)
>

Reviewed-by: Kyle Evans 

Re: [PATCH v3 21/43] bsd-user: pull in target_arch_thread.h update target_arch_elf.h

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:55 PM  wrote:
>
> From: Warner Losh 
>
> Update target_arch_elf.h to remove thread_init. Move its contents to
> target_arch_thread.h and rename to target_thread_init(). Update
> elfload.c to call it. Create thread_os_thread.h to hold the os specific
> parts of the thread and threat manipulation routines. Currently, it just

s/threat/thread/

> includes target_arch_thread.h. target_arch_thread.h contains the at the
> moment unused target_thread_set_upcall which will be used in the future
> when creating actual thread (i386 has this stubbed, but other
> architectures in the bsd-user tree have real ones). FreeBSD doesn't do
> AT_HWCAP, so remove that code. Linux does, and this code came from there.
>
> These changes are all interrelated and could be brokend own, but seem to

s/brokend own/broken  down/

> represent a reviewable changeset since most of the change is boiler
> plate.
>
> Signed-off-by: Stacey Son 
> Signed-off-by: Warner Losh 
> ---
>  bsd-user/elfload.c   |  4 ++-
>  bsd-user/freebsd/target_os_thread.h  | 25 +
>  bsd-user/i386/target_arch_elf.h  | 52 ++--
>  bsd-user/i386/target_arch_thread.h   | 47 +
>  bsd-user/netbsd/target_os_thread.h   | 25 +
>  bsd-user/openbsd/target_os_thread.h  | 25 +
>  bsd-user/x86_64/target_arch_elf.h| 38 ++--
>  bsd-user/x86_64/target_arch_thread.h | 40 +
>  8 files changed, 171 insertions(+), 85 deletions(-)
>  create mode 100644 bsd-user/freebsd/target_os_thread.h
>  create mode 100644 bsd-user/i386/target_arch_thread.h
>  create mode 100644 bsd-user/netbsd/target_os_thread.h
>  create mode 100644 bsd-user/openbsd/target_os_thread.h
>  create mode 100644 bsd-user/x86_64/target_arch_thread.h
>

Minor message nits, but otherwise:

Reviewed-by: Kyle Evans 

Re: [PATCH v3 33/43] bsd-user: Make cpu_model and cpu_type visible to all of main.c

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:53 PM  wrote:
>
> From: Warner Losh 
>
> cpu_model and cpu_type will be used future commits, so move them from
> main() scoped to file scoped.
>
> Signed-off-by: Warner Losh 
> Acked-by: Richard Henderson 
> ---
>  bsd-user/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

I think we should reduce this one to just moving cpu_type. cpu_model
is really only used in main() to derive the appropriate cpu_type,
which we do use later.

>
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 71fd9d5aba..50c8fdc1e2 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -54,6 +54,8 @@
>  int singlestep;
>  unsigned long mmap_min_addr;
>  uintptr_t guest_base;
> +static const char *cpu_model;
> +static const char *cpu_type;
>  bool have_guest_base;
>  unsigned long reserved_va;
>
> @@ -201,8 +203,6 @@ static void save_proc_pathname(char *argv0)
>  int main(int argc, char **argv)
>  {
>  const char *filename;
> -const char *cpu_model;
> -const char *cpu_type;
>  const char *log_file = NULL;
>  const char *log_mask = NULL;
>  const char *seed_optarg = NULL;
> --
> 2.32.0
>

Re: [PATCH v3 34/43] bsd-user: update debugging in mmap.c

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:54 PM  wrote:
>
> From: Warner Losh 
>
> Update the debugging code for new features and different targets.
>
> Signed-off-by: Mikaël Urankar 
> Signed-off-by: Sean Bruno 
> Signed-off-by: Kyle Evans 
> Signed-off-by: Warner Losh 
> Acked-by: Richard Henderson 
> ---
>  bsd-user/mmap.c | 55 ++---
>  1 file changed, 38 insertions(+), 17 deletions(-)
>

Reviewed-by: Kyle Evans 

Re: [PATCH v3 32/43] bsd-user: Rewrite target system call definintion glue

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:54 PM  wrote:
>
> From: Warner Losh 
>
> Rewrite target definnitions to interface with the FreeBSD system calls.
> This covers basic types (time_t, iovec, umtx_time, timespec, timeval,
> rusage, rwusage) and basic defines (mmap, rusage). Also included are
> FreeBSD version-specific variations.
>
> Signed-off-by: Stacey Son 
> Signed-off-by: Warner Losh 
> ---
>  bsd-user/bsd-mman.h | 121 
>  bsd-user/mmap.c |   2 -
>  bsd-user/syscall_defs.h | 247 ++--
>  3 files changed, 162 insertions(+), 208 deletions(-)
>  delete mode 100644 bsd-user/bsd-mman.h
>

Reviewed-by: Kyle Evans 

Re: [PATCH v3 39/43] bsd-user: Refactor load_elf_sections and is_target_elf_binary

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:56 PM  wrote:
>
> From: Warner Losh 
>
> Factor out load_elf_sections and is_target_elf_binary out of
> load_elf_interp.
>
> Signed-off-by: Mikaël Urankar 
> Signed-off-by: Stacey Son 
> Signed-off-by: Warner Losh 
> ---
>  bsd-user/elfload.c | 344 +
>  1 file changed, 158 insertions(+), 186 deletions(-)
>

Reviewed-by: Kyle Evans 

Re: [PATCH v3 36/43] bsd-user: Add target_os_user.h to capture the user/kernel structures

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:55 PM  wrote:
>
> From: Warner Losh 
>
> This file evolved over the years to capture the user/kernel interfaces,
> including those that changed over time.
>
> Signed-off-by: Stacey Son 
> Signed-off-by: Michal Meloun 
> Signed-off-by: Warner Losh 
> Acked-by: Richard Henderson 
> ---
>  bsd-user/freebsd/target_os_user.h | 427 ++
>  1 file changed, 427 insertions(+)
>  create mode 100644 bsd-user/freebsd/target_os_user.h
>

Reviewed-by: Kyle Evans 

[PATCH v3] hw/arm/aspeed: Add Fuji machine type

[pdel]

From: Peter Delevoryas 

This adds a new machine type "fuji-bmc" based on the following device tree:

https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts

Most of the i2c devices are not there, they're added here:

https://github.com/facebook/openbmc/blob/helium/meta-facebook/meta-fuji/recipes-utils/openbmc-utils/files/setup_i2c.sh

I tested this by building a Fuji image from Facebook's OpenBMC repo,
booting, and ssh'ing from host-to-guest.

Signed-off-by: Peter Delevoryas 
---
 hw/arm/aspeed.c | 112 
 1 file changed, 112 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 7a9459340c..cc2d721ac7 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -159,6 +159,10 @@ struct AspeedMachineState {
 #define RAINIER_BMC_HW_STRAP1 0x
 #define RAINIER_BMC_HW_STRAP2 0x
 
+/* Fuji hardware value */
+#define FUJI_BMC_HW_STRAP10x
+#define FUJI_BMC_HW_STRAP20x
+
 /*
  * The max ram region is for firmwares that scan the address space
  * with load/store to guess how much RAM the SoC has.
@@ -772,6 +776,90 @@ static void rainier_bmc_i2c_init(AspeedMachineState *bmc)
 aspeed_eeprom_init(aspeed_i2c_get_bus(>i2c, 15), 0x50, 64 * KiB);
 }
 
+static void get_pca9548_channels(I2CBus *bus, uint8_t mux_addr, I2CBus 
**channels) {
+I2CSlave *mux = i2c_slave_create_simple(bus, "pca9548", mux_addr);
+for (int i = 0; i < 8; i++) {
+channels[i] = pca954x_i2c_get_bus(mux, i);
+}
+}
+
+#define TYPE_LM75 TYPE_TMP105
+#define TYPE_TMP75 TYPE_TMP105
+#define TYPE_TMP422 "tmp422"
+
+static void fuji_bmc_i2c_init(AspeedMachineState *bmc)
+{
+AspeedSoCState *soc = >soc;
+I2CBus *i2c[144] = {};
+
+for (int i = 0; i < 16; i++) {
+i2c[i] = aspeed_i2c_get_bus(>i2c, i);
+}
+I2CBus *i2c180 = i2c[2];
+I2CBus *i2c480 = i2c[8];
+I2CBus *i2c600 = i2c[11];
+
+get_pca9548_channels(i2c180, 0x70, [16]);
+get_pca9548_channels(i2c480, 0x70, [24]);
+// NOTE: The device tree skips [32, 40) in the alias numbering, so we do
+// the same here.
+get_pca9548_channels(i2c600, 0x77, [40]);
+get_pca9548_channels(i2c[24], 0x71, [48]);
+get_pca9548_channels(i2c[25], 0x72, [56]);
+get_pca9548_channels(i2c[26], 0x76, [64]);
+get_pca9548_channels(i2c[27], 0x76, [72]);
+for (int i = 0; i < 8; i++) {
+get_pca9548_channels(i2c[40 + i], 0x76, [80 + i * 8]);
+}
+
+i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
+i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4d);
+
+aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB);
+aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB);
+aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB);
+
+i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x48);
+i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x49);
+i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x4a);
+i2c_slave_create_simple(i2c[3], TYPE_TMP422, 0x4c);
+
+aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB);
+i2c_slave_create_simple(i2c[8], TYPE_LM75, 0x4a);
+
+i2c_slave_create_simple(i2c[50], TYPE_LM75, 0x4c);
+aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB);
+i2c_slave_create_simple(i2c[51], TYPE_TMP75, 0x48);
+i2c_slave_create_simple(i2c[52], TYPE_TMP75, 0x49);
+
+i2c_slave_create_simple(i2c[59], TYPE_TMP75, 0x48);
+i2c_slave_create_simple(i2c[60], TYPE_TMP75, 0x49);
+
+aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB);
+i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x49);
+i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x48);
+aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB);
+aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB);
+aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB);
+aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB);
+
+aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB);
+i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x49);
+i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x48);
+aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB);
+aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB);
+aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB);
+aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB);
+aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB);
+
+for (int i = 0; i < 8; i++) {
+aspeed_eeprom_init(i2c[81 + i * 8], 0x56, 64 * KiB);
+i2c_slave_create_simple(i2c[82 + i * 8], TYPE_TMP75, 0x48);
+i2c_slave_create_simple(i2c[83 + i * 8], TYPE_TMP75, 0x4b);
+i2c_slave_create_simple(i2c[84 + i * 8], TYPE_TMP75, 0x4a);
+}
+}
+
 static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
 {
 return ASPEED_MACHINE(obj)->mmio_exec;
@@ -1070,6 +1158,26 @@ static void 
aspeed_machine_rainier_class_init(ObjectClass *oc, void *data)
 aspeed_soc_num_cpus(amc->soc_name);
 };
 
+static void aspeed_machine_fuji_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
+
+mc->desc = "Facebook Fuji BMC 

Re: [PATCH v3 40/43] bsd-user: move qemu_log to later in the file

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:56 PM  wrote:
>
> From: Warner Losh 
>
> Signed-off-by: Warner Losh 
> Acked-by: Richard Henderson 
> ---
>  bsd-user/main.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
>

Subject nit: s/qemu_log/gemu_log/

Reviewed-by: Kyle Evans 

Re: [PATCH v3 43/43] bsd-user: Update mapping to handle reserved and starting conditions

[Kyle Evans]

On Thu, Sep 2, 2021 at 6:56 PM  wrote:
>
> From: Warner Losh 
>
> Update the reserved base based on what platform we're on, as well as the
> start of the mmap range. Update routines that find va ranges to interact
> with the reserved ranges as well as properly align the mapping (this is
> especially important for targets whose page size does not match the
> host's). Loop where appropriate when the initial address space offered
> by mmap does not meet the contraints.
>
> This has 18e80c55bb6 from linux-user folded in to the upstream
> bsd-user code as well.
>
> Signed-off-by: Mikaël Urankar 
> Signed-off-by: Stacey Son 
> Signed-off-by: Warner Losh 
> Acked-by: Richard Henderson 
> ---
>  bsd-user/main.c |  43 -
>  bsd-user/mmap.c | 415 
>  bsd-user/qemu.h |   5 +-
>  3 files changed, 392 insertions(+), 71 deletions(-)
>

Reviewed-by: Kyle Evans 

Re: [PATCH] softmmu/memory: Validate {read, write}_with_attrs before calling

[Peter Maydell]

On Sun, 5 Sept 2021 at 18:07, Bin Meng  wrote:
>
> On Mon, Sep 6, 2021 at 12:54 AM Peter Maydell  
> wrote:
> > I mean that before commit 62a0db942dec leaving the pointers all
> > NULL was not allowed, and after that commit leaving the pointers all
> > NULL was still not allowed. It's been a requirement that you
> > provide at least one function pointer for read and one for
> > write ever since the MemoryRegion APIs were introduced in 2012.
> > You're proposing an API change; it might be a good one, but it
> > isn't a 'Fixes' to anything.
>
> Where is the requirement documented? I don't see anything mentioned in
> docs/devel/memory.rst

It's not documented, but lots of fiddly details of QEMU functions
aren't documented...

> If it's a requirement since 2012, then I agree "Fixes" can be dropped.
> But a doc fix should be made to document the "requirement".

Agreed.

-- PMM

Re: Re: [PATCH v4] virtio/vsock: add two more queues for datagram types

[Jiang Wang .]

On Mon, Aug 9, 2021 at 3:58 AM Stefano Garzarella  wrote:
>
> On Thu, Aug 05, 2021 at 12:07:02PM -0700, Jiang Wang . wrote:
> >On Wed, Aug 4, 2021 at 1:13 AM Stefano Garzarella  
> >wrote:
> >>
> >> On Tue, Aug 03, 2021 at 11:41:32PM +, Jiang Wang wrote:
> >> >Datagram sockets are connectionless and unreliable.
> >> >The sender does not know the capacity of the receiver
> >> >and may send more packets than the receiver can handle.
> >> >
> >> >Add two more dedicate virtqueues for datagram sockets,
> >> >so that it will not unfairly steal resources from
> >> >stream and future connection-oriented sockets.
> >> >
> >> >Signed-off-by: Jiang Wang 
> >> >---
> >> >v1 -> v2: use qemu cmd option to control number of queues,
> >> >   removed configuration settings for dgram.
> >> >v2 -> v3: use ioctl to get features and decide number of
> >> >virt queues, instead of qemu cmd option.
> >> >v3 -> v4: change DGRAM feature bit value to 2. Add an argument
> >> >   in vhost_vsock_common_realize to indicate dgram is supported or 
> >> > not.
> >> >
> >> > hw/virtio/vhost-user-vsock.c  |  2 +-
> >> > hw/virtio/vhost-vsock-common.c| 58 ++-
> >> > hw/virtio/vhost-vsock.c   |  5 +-
> >> > include/hw/virtio/vhost-vsock-common.h|  6 +-
> >> > include/hw/virtio/vhost-vsock.h   |  4 ++
> >> > include/standard-headers/linux/virtio_vsock.h |  1 +
> >> > 6 files changed, 69 insertions(+), 7 deletions(-)
> >> >
> >> >diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> >> >index 6095ed7349..e9ec0e1c00 100644
> >> >--- a/hw/virtio/vhost-user-vsock.c
> >> >+++ b/hw/virtio/vhost-user-vsock.c
> >> >@@ -105,7 +105,7 @@ static void vuv_device_realize(DeviceState *dev, 
> >> >Error **errp)
> >> > return;
> >> > }
> >> >
> >> >-vhost_vsock_common_realize(vdev, "vhost-user-vsock");
> >> >+vhost_vsock_common_realize(vdev, "vhost-user-vsock", false);
> >> >
> >> > vhost_dev_set_config_notifier(>vhost_dev, _ops);
> >> >
> >> >diff --git a/hw/virtio/vhost-vsock-common.c 
> >> >b/hw/virtio/vhost-vsock-common.c
> >> >index 4ad6e234ad..c78536911a 100644
> >> >--- a/hw/virtio/vhost-vsock-common.c
> >> >+++ b/hw/virtio/vhost-vsock-common.c
> >> >@@ -17,6 +17,8 @@
> >> > #include "hw/virtio/vhost-vsock.h"
> >> > #include "qemu/iov.h"
> >> > #include "monitor/monitor.h"
> >> >+#include 
> >> >+#include 
> >> >
> >> > int vhost_vsock_common_start(VirtIODevice *vdev)
> >> > {
> >> >@@ -196,9 +198,39 @@ int vhost_vsock_common_post_load(void *opaque, int 
> >> >version_id)
> >> > return 0;
> >> > }
> >> >
> >> >-void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name)
> >> >+static int vhost_vsock_get_max_qps(bool enable_dgram)
> >> >+{
> >> >+uint64_t features;
> >> >+int ret;
> >> >+int fd = -1;
> >> >+
> >> >+if (!enable_dgram)
> >> >+return MAX_VQS_WITHOUT_DGRAM;
> >> >+
> >> >+fd = qemu_open_old("/dev/vhost-vsock", O_RDONLY);
> >>
> >>
> >> As I said in the previous version, we cannot directly open
> >> /dev/vhost-vsock, for two reasons:
> >>
> >>1. this code is common with vhost-user-vsock which does not use
> >>/dev/vhost-vsock.
> >>
> >>2. the fd may have been passed from the management layer and qemu may
> >>not be able to directly open /dev/vhost-vsock.
> >>
> >> I think is better to move this function in hw/virtio/vhost-vsock.c,
> >> using the `vhostfd`, returning true or false if dgram is supported, then
> >> you can use it for `enable_dgram` param ...
> >>
> >
> >Yes, you are right. Now I remember you said that before but I forgot about 
> >that
> >when I changed the code. I will fix it. Sorry about that.
>
> No problem :-)
>
> >
> >> >+if (fd == -1) {
> >> >+error_report("vhost-vsock: failed to open device. %s", 
> >> >strerror(errno));
> >> >+return -1;
> >> >+}
> >> >+
> >> >+ret = ioctl(fd, VHOST_GET_FEATURES, );
> >> >+if (ret) {
> >> >+error_report("vhost-vsock: failed to read  device. %s", 
> >> >strerror(errno));
> >> >+qemu_close(fd);
> >> >+return ret;
> >> >+}
> >> >+
> >> >+qemu_close(fd);
> >> >+if (features & (1 << VIRTIO_VSOCK_F_DGRAM))
> >> >+return MAX_VQS_WITH_DGRAM;
> >> >+
> >> >+return MAX_VQS_WITHOUT_DGRAM;
> >> >+}
> >> >+
> >> >+void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name, 
> >> >bool enable_dgram)
> >> > {
> >> > VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> >> >+int nvqs = MAX_VQS_WITHOUT_DGRAM;
> >> >
> >> > virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> >> > sizeof(struct virtio_vsock_config));
> >> >@@ -209,12 +241,24 @@ void vhost_vsock_common_realize(VirtIODevice
> >> >*vdev, const char *name)
> >> > vvc->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >> >vhost_vsock_common_handle_output);
> >> >
> >> >+nvqs 

Re: [RFC PATCH v2 04/12] i386/sev: initialize SNP context

[Dov Murik]

On 05/09/2021 16:58, Brijesh Singh wrote:
> Hi Dov,
> 
> On 9/5/21 2:07 AM, Dov Murik wrote:
> ...
>>
>>>  
>>>  uint64_t
>>> @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
>>> **errp)
>>>  uint32_t ebx;
>>>  uint32_t host_cbitpos;
>>>  struct sev_user_data_status status = {};
>>> +void *init_args = NULL;
>>>  
>>>  if (!sev_common) {
>>>  return 0;
>>> @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, 
>>> Error **errp)
>>>  sev_common->api_major = status.api_major;
>>>  sev_common->api_minor = status.api_minor;
>> Not visible here in the context: the code here is using the
>> SEV_PLATFORM_STATUS command to get the build_id, api_major, and api_minor.
>>
>> I see that SNP has a new command SNP_PLATFORM_STATUS, which fills a
>> struct sev_data_snp_platform_status (hmmm, I can't find the struct's
>> definition; I assume it should look like Table 38 in 8.3.2 in SNP FW ABI
>> document).
> 
> The API version can be queries either through the SNP_PLATFORM_STATUS or
> SEV_PLATFORM_STATUS and they both report the same info. As the
> definition of the sev_data_platform_status is concerned it should be
> defined in the kernel include/linux/psp-sev.h.
> 
> 
>> My questions are:
>>
>> 1. Is it OK to call the "legacy" SEV_PLATFORM_STATUS when about to init
>> an SNP guest?
> 
> Yes, the legacy platform status command can be called on the SNP
> initialized host.
> 
> I choose not to new command because we only care about the verison
> string and that is available through either of these commands (SNP or
> SEV platform status).
> 
>> 2. Do we want to save some info like installed TCB version and reported
>> TCB version, and maybe other fields from SNP platform status?
> 
> If we decide to add a new QMP (query-sev-snp) then it makes sense to
> export those fields so that a hypervisor console can give additional
> information; But note that for the guest, all these are available in the
> attestation report.
> 

We have new QMP response for SNP guests (SevSnpGuestProperties, patch 3
in this series).  I think it would make sense to add the
installed+reported TCB versions there (read-only properties), for
debugging/observability purposes.


-Dov

Re: [PATCH] softmmu/memory: Validate {read, write}_with_attrs before calling

[Bin Meng]

On Mon, Sep 6, 2021 at 12:54 AM Peter Maydell  wrote:
>
> On Sun, 5 Sept 2021 at 17:49, Bin Meng  wrote:
> >
> > On Mon, Sep 6, 2021 at 12:29 AM Peter Maydell  
> > wrote:
> > >
> > > On Sun, 5 Sept 2021 at 16:40, Bin Meng  wrote:
> > > >
> > > > {read,write}_with_attrs might be missing, and the codes currently do
> > > > not validate them before calling, which will cause segment fault.
> > > >
> > > > Fixes: 62a0db942dec ("memory: Remove old_mmio accessors")
> > > > Signed-off-by: Bin Meng 
> > >
> > > This 'fixes' tag doesn't seem quite right. Before that
> > > commit 62a0db942dec, we still required that a MemoryRegionOps
> > > provided some form of both read and write.
> >
> > Did you mean before commit 62a0db942dec leaving MemoryRegionOps NULL
> > was not allowed, but things changed so that now it's possible to have
> > NULL MemoryRegionOps? If yes, then the commit id of "Fixes" should
> > probably go to the changes that allowed NULL memory ops. If not, I
> > don't think "Fixes" tag is wrong.
>
> I mean that before commit 62a0db942dec leaving the pointers all
> NULL was not allowed, and after that commit leaving the pointers all
> NULL was still not allowed. It's been a requirement that you
> provide at least one function pointer for read and one for
> write ever since the MemoryRegion APIs were introduced in 2012.
> You're proposing an API change; it might be a good one, but it
> isn't a 'Fixes' to anything.

Where is the requirement documented? I don't see anything mentioned in
docs/devel/memory.rst

If it's a requirement since 2012, then I agree "Fixes" can be dropped.
But a doc fix should be made to document the "requirement".

Regards,
Bin

Re: [RFC PATCH v2 04/12] i386/sev: initialize SNP context

[Dov Murik]

On 05/09/2021 17:05, Brijesh Singh wrote:
> 
> On 9/5/21 4:19 AM, Dov Murik wrote:
>>
>> On 27/08/2021 1:26, Michael Roth wrote:
>>> From: Brijesh Singh 
>>>
>>> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
>>> the platform. The command checks whether SNP is enabled in the KVM, if
>>> enabled then it allocates a new ASID from the SNP pool and calls the
>>> firmware to initialize the all the resources.
>>>
>>
>> From the KVM code ("[PATCH Part2 v5 24/45] KVM: SVM: Add
>> KVM_SEV_SNP_LAUNCH_START command") it seems that KVM_SNP_INIT does *not*
>> allocate the ASID; actually this is done in KVM_SEV_SNP_LAUNCH_START.
> 
> Actually, the KVM_SNP_INIT does allocate the ASID. If you look at the
> driver code then in switch state, the SNP_INIT fallthrough to SEV_INIT
> which will call sev_guest_init(). The sev_guest_init() allocates a new
> ASID.
> https://github.com/AMDESE/linux/blob/bb9ba49cd9b749d5551aae295c091d8757153dd7/arch/x86/kvm/svm/sev.c#L255
> 
> The LAUNCH_START simply binds the ASID to a guest.

OK thank you for clearing this up.  So the kernel is choosing the new
ASID during the KVM_SNP_INIT ioctl, but doesn't "tell" the firmware
about it.  Then later in SNP_LAUNCH_START that integer (saved in the
kernel sev structure) is given to the firmware as an argument of the
SNP_LAUNCH_START (binding?).  Is this description correct?


-Dov

Re: [PATCH] softmmu/memory: Validate {read, write}_with_attrs before calling

[Peter Maydell]

On Sun, 5 Sept 2021 at 17:49, Bin Meng  wrote:
>
> On Mon, Sep 6, 2021 at 12:29 AM Peter Maydell  
> wrote:
> >
> > On Sun, 5 Sept 2021 at 16:40, Bin Meng  wrote:
> > >
> > > {read,write}_with_attrs might be missing, and the codes currently do
> > > not validate them before calling, which will cause segment fault.
> > >
> > > Fixes: 62a0db942dec ("memory: Remove old_mmio accessors")
> > > Signed-off-by: Bin Meng 
> >
> > This 'fixes' tag doesn't seem quite right. Before that
> > commit 62a0db942dec, we still required that a MemoryRegionOps
> > provided some form of both read and write.
>
> Did you mean before commit 62a0db942dec leaving MemoryRegionOps NULL
> was not allowed, but things changed so that now it's possible to have
> NULL MemoryRegionOps? If yes, then the commit id of "Fixes" should
> probably go to the changes that allowed NULL memory ops. If not, I
> don't think "Fixes" tag is wrong.

I mean that before commit 62a0db942dec leaving the pointers all
NULL was not allowed, and after that commit leaving the pointers all
NULL was still not allowed. It's been a requirement that you
provide at least one function pointer for read and one for
write ever since the MemoryRegion APIs were introduced in 2012.
You're proposing an API change; it might be a good one, but it
isn't a 'Fixes' to anything.

-- PMM

Re: [PATCH] softmmu/memory: Validate {read, write}_with_attrs before calling

[Bin Meng]

On Mon, Sep 6, 2021 at 12:29 AM Peter Maydell  wrote:
>
> On Sun, 5 Sept 2021 at 16:40, Bin Meng  wrote:
> >
> > {read,write}_with_attrs might be missing, and the codes currently do
> > not validate them before calling, which will cause segment fault.
> >
> > Fixes: 62a0db942dec ("memory: Remove old_mmio accessors")
> > Signed-off-by: Bin Meng 
>
> This 'fixes' tag doesn't seem quite right. Before that
> commit 62a0db942dec, we still required that a MemoryRegionOps
> provided some form of both read and write.

Did you mean before commit 62a0db942dec leaving MemoryRegionOps NULL
was not allowed, but things changed so that now it's possible to have
NULL MemoryRegionOps? If yes, then the commit id of "Fixes" should
probably go to the changes that allowed NULL memory ops. If not, I
don't think "Fixes" tag is wrong.

> It was never
> valid to leave all of the possible read function pointers NULL.
>
> What are the examples of devices which are deliberately leaving
> these pointers set to NULL?

No in-tree real examples. I just read the codes and found no
protection against the {read,write}_with_attrs ops.

> Last time this came up, we discussed the other option, which
> is to have memory_region_init assert that the MemoryRegionOps
> defines at least one valid read and one valid write pointer,
> so that these bugs get caught quickly rather than only if the
> guest accesses the device in the wrong way. I tend to think
> that that is better, because for any particular device
> it's not necessarily the right thing to return MEMTX_ERROR
> (maybe the right behaviour is "return 0 for reads, ignore
> writes" -- the point is that there is no single default
> behaviour that works for every device and architecture).
> Forcing the device model author to explicitly write the
> code means they have to think about what the behaviour
> they want is.

Yes, either way can prevent the NULL dereferencing. So it's time to
revisit this topic :)

>
> If we do choose to support allowing MemoryRegionOps structs
> to leave all the pointers NULL, we should document that,
> including what the default behaviour is.

Regards,
Bin

Re: [PATCH] softmmu/memory: Validate {read, write}_with_attrs before calling

[Peter Maydell]

On Sun, 5 Sept 2021 at 16:40, Bin Meng  wrote:
>
> {read,write}_with_attrs might be missing, and the codes currently do
> not validate them before calling, which will cause segment fault.
>
> Fixes: 62a0db942dec ("memory: Remove old_mmio accessors")
> Signed-off-by: Bin Meng 

This 'fixes' tag doesn't seem quite right. Before that
commit 62a0db942dec, we still required that a MemoryRegionOps
provided some form of both read and write. It was never
valid to leave all of the possible read function pointers NULL.

What are the examples of devices which are deliberately leaving
these pointers set to NULL?

Last time this came up, we discussed the other option, which
is to have memory_region_init assert that the MemoryRegionOps
defines at least one valid read and one valid write pointer,
so that these bugs get caught quickly rather than only if the
guest accesses the device in the wrong way. I tend to think
that that is better, because for any particular device
it's not necessarily the right thing to return MEMTX_ERROR
(maybe the right behaviour is "return 0 for reads, ignore
writes" -- the point is that there is no single default
behaviour that works for every device and architecture).
Forcing the device model author to explicitly write the
code means they have to think about what the behaviour
they want is.

If we do choose to support allowing MemoryRegionOps structs
to leave all the pointers NULL, we should document that,
including what the default behaviour is.

thanks
-- PMM

Re: [PATCH] softmmu/memory: Validate {read, write}_with_attrs before calling

[Philippe Mathieu-Daudé]

Cc'ing PJP for https://www.mail-archive.com/qemu-devel@nongnu.org/msg730311.html

On Sun, Sep 5, 2021 at 5:41 PM Bin Meng  wrote:
>
> {read,write}_with_attrs might be missing, and the codes currently do
> not validate them before calling, which will cause segment fault.
>
> Fixes: 62a0db942dec ("memory: Remove old_mmio accessors")
> Signed-off-by: Bin Meng 
> ---
>
>  softmmu/memory.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4d..b97ffd4ba7 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1426,12 +1426,14 @@ static MemTxResult 
> memory_region_dispatch_read1(MemoryRegion *mr,
>   mr->ops->impl.max_access_size,
>   memory_region_read_accessor,
>   mr, attrs);
> -} else {
> +} else if (mr->ops->read_with_attrs) {
>  return access_with_adjusted_size(addr, pval, size,
>   mr->ops->impl.min_access_size,
>   mr->ops->impl.max_access_size,
>   
> memory_region_read_with_attrs_accessor,
>   mr, attrs);
> +} else {
> +return MEMTX_ERROR;
>  }
>  }
>
> @@ -1506,13 +1508,15 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
> *mr,
>   mr->ops->impl.max_access_size,
>   memory_region_write_accessor, mr,
>   attrs);
> -} else {
> +} else if (mr->ops->write_with_attrs) {
>  return
>  access_with_adjusted_size(addr, , size,
>mr->ops->impl.min_access_size,
>mr->ops->impl.max_access_size,
>
> memory_region_write_with_attrs_accessor,
>mr, attrs);
> +} else {
> +return MEMTX_ERROR;
>  }
>  }
>
> --
> 2.25.1
>

Re: [PATCH] user: Mark cpu_loop() with noreturn attribute

[Bin Meng]

On Sun, Sep 5, 2021 at 8:18 AM Philippe Mathieu-Daudé  wrote:
>
> cpu_loop() never exits, so mark it with QEMU_NORETURN.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  bsd-user/qemu.h   | 2 +-
>  linux-user/qemu.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng 

Re: [PATCH v2 18/24] target/riscv: Restrict cpu_exec_interrupt() handler to sysemu

[Bin Meng]

On Sun, Sep 5, 2021 at 8:06 AM Philippe Mathieu-Daudé  wrote:
>
> Restrict cpu_exec_interrupt() and its callees to sysemu.
>
> Reviewed-by: Warner Losh 
> Reviewed-by: Richard Henderson 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/riscv/cpu.h| 2 +-
>  target/riscv/cpu.c| 2 +-
>  target/riscv/cpu_helper.c | 5 -
>  3 files changed, 2 insertions(+), 7 deletions(-)
>

Reviewed-by: Bin Meng 

[PATCH] softmmu/memory: Validate {read, write}_with_attrs before calling

[Bin Meng]

{read,write}_with_attrs might be missing, and the codes currently do
not validate them before calling, which will cause segment fault.

Fixes: 62a0db942dec ("memory: Remove old_mmio accessors")
Signed-off-by: Bin Meng 
---

 softmmu/memory.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4d..b97ffd4ba7 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1426,12 +1426,14 @@ static MemTxResult 
memory_region_dispatch_read1(MemoryRegion *mr,
  mr->ops->impl.max_access_size,
  memory_region_read_accessor,
  mr, attrs);
-} else {
+} else if (mr->ops->read_with_attrs) {
 return access_with_adjusted_size(addr, pval, size,
  mr->ops->impl.min_access_size,
  mr->ops->impl.max_access_size,
  
memory_region_read_with_attrs_accessor,
  mr, attrs);
+} else {
+return MEMTX_ERROR;
 }
 }
 
@@ -1506,13 +1508,15 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
*mr,
  mr->ops->impl.max_access_size,
  memory_region_write_accessor, mr,
  attrs);
-} else {
+} else if (mr->ops->write_with_attrs) {
 return
 access_with_adjusted_size(addr, , size,
   mr->ops->impl.min_access_size,
   mr->ops->impl.max_access_size,
   memory_region_write_with_attrs_accessor,
   mr, attrs);
+} else {
+return MEMTX_ERROR;
 }
 }
 
-- 
2.25.1

Re: [PATCH v2 0/5] hw/virtio: Minor housekeeping patches

[Michael S. Tsirkin]

On Thu, Sep 02, 2021 at 06:50:34PM +0200, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series contains few patches I gathered while tooking notes
> trying to understand issues #300-#302.

v1 was includes in my pull request already, pls send
incremental patches on top. Thanks!

> Since v1:
> - Added virtqueue_flush comment (Stefano)
> - Call RCU_READ_LOCK_GUARD in virtqueue_packed_drop_all (Stefano)
> 
> Philippe Mathieu-Daudé (5):
>   hw/virtio: Document virtio_queue_packed_empty_rcu is called within RCU
>   hw/virtio: Comment virtqueue_flush() must be called with RCU read lock
>   hw/virtio: Remove NULL check in virtio_free_region_cache()
>   hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all()
>   hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees
> 
>  include/hw/virtio/virtio.h |  7 +++
>  hw/virtio/virtio.c | 39 ++
>  2 files changed, 25 insertions(+), 21 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PULL 00/13] QAPI patches patches for 2021-09-03

[Peter Maydell]

On Fri, 3 Sept 2021 at 20:32, Markus Armbruster  wrote:
>
> The following changes since commit 8880cc4362fde4ecdac0b2092318893118206fcf:
>
>   Merge remote-tracking branch 'remotes/cschoenebeck/tags/pull-9p-20210902' 
> into staging (2021-09-03 08:27:38 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2021-09-03
>
> for you to fetch changes up to 34f7b25e575a93182b7c0a3558caac34e26227cf:
>
>   qapi: Tweak error messages for unknown / conflicting 'if' keys (2021-09-03 
> 17:09:10 +0200)
>
> 
> QAPI patches patches for 2021-09-03
>


Applied, thanks.

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

-- PMM

Re: arm: Launching EFI-enabled arm32 Linux

[Peter Maydell]

On Sat, 4 Sept 2021 at 20:26, Adam Lackorzynski  wrote:
> while trying to launch an EFI-enabled arm32 Linux binary (zImage) I
> noticed I get an undefined instruction exception on the first
> instruction. Now this is a bit special because Linux uses a nop
> instruction there that also is a PE file signature ('MZ') such that the
> CPU runs over it and the file is still recognized as a PE binary. Linux
> uses 0x13105a4d (tstne r0, #0x4d000) as the instruction (see also
> arch/arm/boot/compressed/head.S and efi-header.S in Linux).
> However, QEMU's instruction decoder will only recognize TST with bits
> 12-15 being 0, which this instruction is not fullfilling, and thus the
> undef exception. I guess other CPU implementations will allow this
> encoding. So while investigating I was doing the following to make Linux
> proceed. I also believe this was working in a previous version of QEMU.
>
> diff --git a/target/arm/a32.decode b/target/arm/a32.decode
> index fcd8cd4f7d..222553750e 100644
> --- a/target/arm/a32.decode
> +++ b/target/arm/a32.decode
> @@ -127,7 +127,7 @@ ADD_rri   001 0100 .      
> @s_rri_rot
>  ADC_rri   001 0101 .      @s_rri_rot
>  SBC_rri   001 0110 .      @s_rri_rot
>  RSC_rri   001 0111 .      @s_rri_rot
> -TST_xri   001 1000 1      @S_xri_rot
> +TST_xri   001 1000 1      @S_xri_rot
>  TEQ_xri   001 1001 1      @S_xri_rot
>  CMP_xri   001 1010 1      @S_xri_rot
>  CMN_xri   001 1011 1      @S_xri_rot
>
>
> Any thoughts on this?

If your guest code is relying on bits [15:12] in the TST (immediate)
Arm encoding being non-zero then it is broken. In the v8A Arm ARM
DDI 0487G.b, section F5.1.262, these bits are noted as "(0)", which
means RES0, should-be-zero. In F1.7.2 this is described as meaning
that if the bit is 1 then the behaviour is CONSTRAINED UNPREDICTABLE,
and can be result in any of:
 * UNDEF (this is what QEMU chooses)
 * NOP
 * executes as if the bit were 0
 * any destination registers become UNKNOWN

This was true also for v7A. Even back as far as ARMv5 these bits
are marked as "SBZ" (should-be-zero).

Since this is all in the UNPREDICTABLE zone, there are presumably
some CPUs that do execute this insn as either a NOP or ignoring
the incorrectly set bits; but I would not be surprised if there
are also some CPUs that behave like QEMU and UNDEF them.

Looking at the code where this is used, I think it probably
needs to abandon the goal of having the insn be a true
or nearly-true NOP. Since this is the first insn the kernel
executes, it doesn't really have to be a NOP, as long as it
doesn't trash the registers where the bootloader passed it
information (r0, r1, r2).

Unless there are other undocumented constraints on this
instruction pattern, you might try
  0xe2255a4d   ; eor r5, r5, 0x4d000

That's not a NOP on its own, but if you use it twice
in a row then it is, and you can make sure the use in
head.S arranges to put two of those and then revert to
more normal-looking NOPs for the rest of its run of NOPs.

(This doesn't work for CONFIG_THUMB2_KERNEL, but neither does
the current insn pattern I think.)

thanks
-- PMM

Re: [PULL 00/14] aspeed queue

[Cédric Le Goater]

On 9/5/21 4:34 PM, Peter Delevoryas wrote:
> 
> 
>> On Sep 5, 2021, at 1:51 AM, Cédric Le Goater  wrote:
>>
>> On 9/5/21 1:03 AM, Philippe Mathieu-Daudé wrote:
 On 9/4/21 7:33 AM, Cédric Le Goater wrote:
 On 9/3/21 10:41 PM, Philippe Mathieu-Daudé wrote:
> Hi Peter,
>
> On 9/3/21 9:40 PM, Cédric Le Goater wrote:
>> The following changes since commit 
>> 8880cc4362fde4ecdac0b2092318893118206fcf:
>>
>>  Merge remote-tracking branch 
>> 'remotes/cschoenebeck/tags/pull-9p-20210902' into staging (2021-09-03 
>> 08:27:38 +0100)
>>
>> are available in the Git repository at:
>>
>>  https://github.com/legoater/qemu/ tags/pull-aspeed-20210903
>>
>> for you to fetch changes up to 907796622b2a6b945c87641d94e254ac898b96ae:
>>
>>  hw/arm/aspeed: Add Fuji machine type (2021-09-03 18:43:16 +0200)
>>
>> 
>> Aspeed patches :
>>
>> * MAC enablement fixes (Guenter)
>> * Watchdog  and pca9552 fixes (Andrew)
>> * GPIO fixes (Joel)
>> * AST2600A3 SoC and DPS310 models (Joel)
>> * New Fuji BMC machine (Peter)
>>
>> 
>
>> Peter Delevoryas (3):
>>  hw/arm/aspeed: Initialize AST2600 UART clock selection registers
>>  hw/arm/aspeed: Allow machine to set UART default
>>  hw/arm/aspeed: Add Fuji machine type
>
> I have a pending question with the last patch, do you mind holding
> this PR until it is resolved with Cédric and the patch author please?
>
> Thanks,
>
> Phil.
>

 I guess we can drop the following from the commit log : 

git clone https://github.com/facebook/openbmc
cd openbmc
./sync_yocto.sh
source openbmc-init-build-env fuji build-fuji
bitbake fuji-image
dd if=/dev/zero of=/tmp/fuji.mtd bs=1M count=128
dd if=./tmp/deploy/images/fuji/flash-fuji of=/tmp/fuji.mtd \
bs=1k conv=notrunc

git clone --branch aspeed-next https://github.com/peterdelevoryas/qemu
cd qemu
./configure --target-list=arm-softmmu --disable-vnc
make -j $(nproc)
./build/arm-softmmu/qemu-system-arm \
-machine fuji-bmc \
-drive file=/tmp/fuji.mtd,format=raw,if=mtd \
-serial stdio \
-nic user,hostfwd=::-:22
sshpass -p 0penBmc ssh root@localhost -p 
>>>
>>> Sounds good. Eventually document that in docs/system/arm/aspeed.rst
>>> in a follow up patch?
>>>
>>> Regards,
>>>
>>> Phil.
>>>
>>
>>
>> Peter D, 
>>
>> Could you please resend the "hw/arm/aspeed: Add Fuji machine type"
>> patch addressing Phil's comment. I will resend a PR with the 
>> update.
>>
>> Thanks,
>>
>> C. 
>>
>>
> 
> Oh! Yes, I can do that, sorry, I wasn’t sure if it was necessary to resend or 
> if it could be fixed inline or something. I’ll send that within the next 24 
> hrs, removing the selected text from the commit description.

I didn't either. 

Thanks, 

C.

Re: [PATCH v3 32/43] bsd-user: Rewrite target system call definintion glue

[Warner Losh]

On Sun, Sep 5, 2021 at 4:33 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 9/3/21 1:47 AM, i...@bsdimp.com wrote:
> > From: Warner Losh
> >
> > Rewrite target definnitions to interface with the FreeBSD system calls.
> > This covers basic types (time_t, iovec, umtx_time, timespec, timeval,
> > rusage, rwusage) and basic defines (mmap, rusage). Also included are
> > FreeBSD version-specific variations.
> >
> > Signed-off-by: Stacey Son
> > Signed-off-by: Warner Losh
> > ---
> >   bsd-user/bsd-mman.h | 121 
> >   bsd-user/mmap.c |   2 -
> >   bsd-user/syscall_defs.h | 247 ++--
> >   3 files changed, 162 insertions(+), 208 deletions(-)
> >   delete mode 100644 bsd-user/bsd-mman.h
>
> I think I gave you an
> Acked-by: Richard Henderson 
>

Ah, so you did. I'm still honing my patch handling skills...

Warner

Re: [PULL 00/14] aspeed queue

[Peter Delevoryas]

> On Sep 5, 2021, at 1:51 AM, Cédric Le Goater  wrote:
> 
> On 9/5/21 1:03 AM, Philippe Mathieu-Daudé wrote:
>>> On 9/4/21 7:33 AM, Cédric Le Goater wrote:
>>> On 9/3/21 10:41 PM, Philippe Mathieu-Daudé wrote:
 Hi Peter,
 
 On 9/3/21 9:40 PM, Cédric Le Goater wrote:
> The following changes since commit 
> 8880cc4362fde4ecdac0b2092318893118206fcf:
> 
>  Merge remote-tracking branch 
> 'remotes/cschoenebeck/tags/pull-9p-20210902' into staging (2021-09-03 
> 08:27:38 +0100)
> 
> are available in the Git repository at:
> 
>  https://github.com/legoater/qemu/ tags/pull-aspeed-20210903
> 
> for you to fetch changes up to 907796622b2a6b945c87641d94e254ac898b96ae:
> 
>  hw/arm/aspeed: Add Fuji machine type (2021-09-03 18:43:16 +0200)
> 
> 
> Aspeed patches :
> 
> * MAC enablement fixes (Guenter)
> * Watchdog  and pca9552 fixes (Andrew)
> * GPIO fixes (Joel)
> * AST2600A3 SoC and DPS310 models (Joel)
> * New Fuji BMC machine (Peter)
> 
> 
 
> Peter Delevoryas (3):
>  hw/arm/aspeed: Initialize AST2600 UART clock selection registers
>  hw/arm/aspeed: Allow machine to set UART default
>  hw/arm/aspeed: Add Fuji machine type
 
 I have a pending question with the last patch, do you mind holding
 this PR until it is resolved with Cédric and the patch author please?
 
 Thanks,
 
 Phil.
 
>>> 
>>> I guess we can drop the following from the commit log : 
>>> 
>>>git clone https://github.com/facebook/openbmc
>>>cd openbmc
>>>./sync_yocto.sh
>>>source openbmc-init-build-env fuji build-fuji
>>>bitbake fuji-image
>>>dd if=/dev/zero of=/tmp/fuji.mtd bs=1M count=128
>>>dd if=./tmp/deploy/images/fuji/flash-fuji of=/tmp/fuji.mtd \
>>>bs=1k conv=notrunc
>>>
>>>git clone --branch aspeed-next https://github.com/peterdelevoryas/qemu
>>>cd qemu
>>>./configure --target-list=arm-softmmu --disable-vnc
>>>make -j $(nproc)
>>>./build/arm-softmmu/qemu-system-arm \
>>>-machine fuji-bmc \
>>>-drive file=/tmp/fuji.mtd,format=raw,if=mtd \
>>>-serial stdio \
>>>-nic user,hostfwd=::-:22
>>>sshpass -p 0penBmc ssh root@localhost -p 
>> 
>> Sounds good. Eventually document that in docs/system/arm/aspeed.rst
>> in a follow up patch?
>> 
>> Regards,
>> 
>> Phil.
>> 
> 
> 
> Peter D, 
> 
> Could you please resend the "hw/arm/aspeed: Add Fuji machine type"
> patch addressing Phil's comment. I will resend a PR with the 
> update.
> 
> Thanks,
> 
> C. 
> 
> 

Oh! Yes, I can do that, sorry, I wasn’t sure if it was necessary to resend or 
if it could be fixed inline or something. I’ll send that within the next 24 
hrs, removing the selected text from the commit description.

Thanks,
Peter

> 
> 
> 

Re: [PATCH v3 27/43] bsd-user: Implement --seed and initialize random state

[Warner Losh]

On Sun, Sep 5, 2021 at 4:33 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 9/3/21 1:47 AM, i...@bsdimp.com wrote:
> > From: Warner Losh 
> >
> > Copy --seed implementation (translated from linux-user's newer command
> > line scheme to the older one bsd-user still uses). Initialize the
> > randomness with the glibc if a specific seed is specified or use the
>
> FWIW, it's glib, not glibc.
>

I make that mistake all the time. Thanks!
And while I'm writing: thanks for all the reviews!

Warner


> Reviewed-by: Richard Henderson 
>
>
> r~
>

Re: [PATCH v2 5/5] hw/char: cadence_uart: Ignore access when unclocked or in reset for uart_{read, write}()

[Bin Meng]

On Thu, Sep 2, 2021 at 2:11 PM Philippe Mathieu-Daudé  wrote:
>
> On 9/2/21 8:09 AM, Philippe Mathieu-Daudé wrote:
> > On 9/1/21 5:27 AM, Bin Meng wrote:
> >> Read or write to uart registers when unclocked or in reset should be
> >> ignored. Add the check there, and as a result of this, the check in
> >> uart_write_tx_fifo() is now unnecessary.
> >>
> >> Signed-off-by: Bin Meng 
> >>
> >> ---
> >>
> >> Changes in v2:
> >> - new patch: hw/char: cadence_uart: Ignore access when unclocked or in 
> >> reset for uart_{read,write}()
> >>
> >>  hw/char/cadence_uart.c | 15 ++-
> >>  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > Reviewed-by: Philippe Mathieu-Daudé 
>
> Just realized it is simpler to implement MemoryRegionOps::accepts().

Is there any guidance on what condition falls into
MemoryRegionOps::accepts() to check?

For example, should we move the register offset check to accepts()?

It looks like only a few codes implemented the MemoryRegionOps::accepts().

Regards,
Bin

Re: [PATCH v2 11/24] target/i386: Move x86_cpu_exec_interrupt() under sysemu/ folder

[Warner Losh]

On Sat, Sep 4, 2021 at 5:56 PM Philippe Mathieu-Daudé 
wrote:

> Following the logic of commit 30493a030ff ("i386: split seg_helper
> into user-only and sysemu parts"), move x86_cpu_exec_interrupt()
> under sysemu/seg_helper.c.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> I prefer to not squash this into the previous patch because the
> ifdef'ry removal (in previous patch) is not trivial IMO.
> ---
>  target/i386/tcg/seg_helper.c| 64 
>  target/i386/tcg/sysemu/seg_helper.c | 65 +
>  2 files changed, 65 insertions(+), 64 deletions(-)
>

Reviewed-By: Warner Losh 



> diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c
> index 13c6e6ee62e..baa905a0cd6 100644
> --- a/target/i386/tcg/seg_helper.c
> +++ b/target/i386/tcg/seg_helper.c
> @@ -1110,70 +1110,6 @@ void do_interrupt_x86_hardirq(CPUX86State *env, int
> intno, int is_hw)
>  do_interrupt_all(env_archcpu(env), intno, 0, 0, 0, is_hw);
>  }
>
> -#ifndef CONFIG_USER_ONLY
> -bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> -{
> -X86CPU *cpu = X86_CPU(cs);
> -CPUX86State *env = >env;
> -int intno;
> -
> -interrupt_request = x86_cpu_pending_interrupt(cs, interrupt_request);
> -if (!interrupt_request) {
> -return false;
> -}
> -
> -/* Don't process multiple interrupt requests in a single call.
> - * This is required to make icount-driven execution deterministic.
> - */
> -switch (interrupt_request) {
> -case CPU_INTERRUPT_POLL:
> -cs->interrupt_request &= ~CPU_INTERRUPT_POLL;
> -apic_poll_irq(cpu->apic_state);
> -break;
> -case CPU_INTERRUPT_SIPI:
> -do_cpu_sipi(cpu);
> -break;
> -case CPU_INTERRUPT_SMI:
> -cpu_svm_check_intercept_param(env, SVM_EXIT_SMI, 0, 0);
> -cs->interrupt_request &= ~CPU_INTERRUPT_SMI;
> -do_smm_enter(cpu);
> -break;
> -case CPU_INTERRUPT_NMI:
> -cpu_svm_check_intercept_param(env, SVM_EXIT_NMI, 0, 0);
> -cs->interrupt_request &= ~CPU_INTERRUPT_NMI;
> -env->hflags2 |= HF2_NMI_MASK;
> -do_interrupt_x86_hardirq(env, EXCP02_NMI, 1);
> -break;
> -case CPU_INTERRUPT_MCE:
> -cs->interrupt_request &= ~CPU_INTERRUPT_MCE;
> -do_interrupt_x86_hardirq(env, EXCP12_MCHK, 0);
> -break;
> -case CPU_INTERRUPT_HARD:
> -cpu_svm_check_intercept_param(env, SVM_EXIT_INTR, 0, 0);
> -cs->interrupt_request &= ~(CPU_INTERRUPT_HARD |
> -   CPU_INTERRUPT_VIRQ);
> -intno = cpu_get_pic_interrupt(env);
> -qemu_log_mask(CPU_LOG_TB_IN_ASM,
> -  "Servicing hardware INT=0x%02x\n", intno);
> -do_interrupt_x86_hardirq(env, intno, 1);
> -break;
> -case CPU_INTERRUPT_VIRQ:
> -/* FIXME: this should respect TPR */
> -cpu_svm_check_intercept_param(env, SVM_EXIT_VINTR, 0, 0);
> -intno = x86_ldl_phys(cs, env->vm_vmcb
> - + offsetof(struct vmcb, control.int_vector));
> -qemu_log_mask(CPU_LOG_TB_IN_ASM,
> -  "Servicing virtual hardware INT=0x%02x\n", intno);
> -do_interrupt_x86_hardirq(env, intno, 1);
> -cs->interrupt_request &= ~CPU_INTERRUPT_VIRQ;
> -break;
> -}
> -
> -/* Ensure that no TB jump will be modified as the program flow was
> changed.  */
> -return true;
> -}
> -#endif /* CONFIG_USER_ONLY */
> -
>  void helper_lldt(CPUX86State *env, int selector)
>  {
>  SegmentCache *dt;
> diff --git a/target/i386/tcg/sysemu/seg_helper.c
> b/target/i386/tcg/sysemu/seg_helper.c
> index 82c0856c417..b425b930f9d 100644
> --- a/target/i386/tcg/sysemu/seg_helper.c
> +++ b/target/i386/tcg/sysemu/seg_helper.c
> @@ -125,6 +125,71 @@ void x86_cpu_do_interrupt(CPUState *cs)
>  }
>  }
>
> +bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> +{
> +X86CPU *cpu = X86_CPU(cs);
> +CPUX86State *env = >env;
> +int intno;
> +
> +interrupt_request = x86_cpu_pending_interrupt(cs, interrupt_request);
> +if (!interrupt_request) {
> +return false;
> +}
> +
> +/*
> + * Don't process multiple interrupt requests in a single call.
> + * This is required to make icount-driven execution deterministic.
> + */
> +switch (interrupt_request) {
> +case CPU_INTERRUPT_POLL:
> +cs->interrupt_request &= ~CPU_INTERRUPT_POLL;
> +apic_poll_irq(cpu->apic_state);
> +break;
> +case CPU_INTERRUPT_SIPI:
> +do_cpu_sipi(cpu);
> +break;
> +case CPU_INTERRUPT_SMI:
> +cpu_svm_check_intercept_param(env, SVM_EXIT_SMI, 0, 0);
> +cs->interrupt_request &= ~CPU_INTERRUPT_SMI;
> +do_smm_enter(cpu);
> +break;
> +case CPU_INTERRUPT_NMI:
> +cpu_svm_check_intercept_param(env, SVM_EXIT_NMI, 0, 0);
> +cs->interrupt_request &= 

Re: [PATCH v2 01/24] target/avr: Remove pointless use of CONFIG_USER_ONLY definition

[Warner Losh]

On Sat, Sep 4, 2021 at 5:55 PM Philippe Mathieu-Daudé 
wrote:

> Commit f1c671f96cb ("target/avr: Introduce basic CPU class object")
> added to target/avr/cpu.h:
>
>   #ifdef CONFIG_USER_ONLY
>   #error "AVR 8-bit does not support user mode"
>   #endif
>
> Remove the CONFIG_USER_ONLY definition introduced by mistake in
> commit 78271684719 ("cpu: tcg_ops: move to tcg-cpu-ops.h, keep a
> pointer in CPUClass").
>
> Reported-by: Richard Henderson 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/avr/cpu.c | 3 ---
>  1 file changed, 3 deletions(-)
>

Reviewed-By: Warner Losh 



> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index ea14175ca55..5d70e34dd54 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -197,10 +197,7 @@ static const struct TCGCPUOps avr_tcg_ops = {
>  .synchronize_from_tb = avr_cpu_synchronize_from_tb,
>  .cpu_exec_interrupt = avr_cpu_exec_interrupt,
>  .tlb_fill = avr_cpu_tlb_fill,
> -
> -#ifndef CONFIG_USER_ONLY
>  .do_interrupt = avr_cpu_do_interrupt,
> -#endif /* !CONFIG_USER_ONLY */
>  };
>
>  static void avr_cpu_class_init(ObjectClass *oc, void *data)
> --
> 2.31.1
>
>

Re: [PATCH] user: Mark cpu_loop() with noreturn attribute

[Warner Losh]

On Sat, Sep 4, 2021 at 6:04 PM Philippe Mathieu-Daudé 
wrote:

> cpu_loop() never exits, so mark it with QEMU_NORETURN.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  bsd-user/qemu.h   | 2 +-
>  linux-user/qemu.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-By: Warner Losh 


diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index c02e8a5ca1a..05bee7aefe5 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -155,7 +155,7 @@ abi_long do_openbsd_syscall(void *cpu_env, int num,
> abi_long arg1,
>  abi_long arg5, abi_long arg6);
>  void gemu_log(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  extern THREAD CPUState *thread_cpu;
> -void cpu_loop(CPUArchState *env);
> +void QEMU_NORETURN cpu_loop(CPUArchState *env);
>  char *target_strerror(int err);
>  int get_osversion(void);
>  void fork_start(void);
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 3b0b6b75fe8..5b2c764ae78 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -236,7 +236,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
> arg1,
>  abi_long arg5, abi_long arg6, abi_long arg7,
>  abi_long arg8);
>  extern __thread CPUState *thread_cpu;
> -void cpu_loop(CPUArchState *env);
> +void QEMU_NORETURN cpu_loop(CPUArchState *env);
>  const char *target_strerror(int err);
>  int get_osversion(void);
>  void init_qemu_uname_release(void);
> --
> 2.31.1
>
>

[PATCH 0/1] Add missing function names to symbol list

[Lukas Jünger]

Hi all,

I have been trying to use the hwprofile and cache plugin on
qemu-system-riscv64. They failed to load with an undefined
symbol error. It looks like some of the plugin API functions
are missing from the symbol list, so I added them. Afterwards
the plugins worked (eventhough the cache plugin is segfaulting
on shutdown, but that is a separate, unrelated issue).

Hope that's okay.

BR,
Lukas

Lukas Jünger (1):
  plugins/: Add missing functions to symbol list

 plugins/qemu-plugins.symbols | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.31.1

[PATCH 1/1] plugins/: Add missing functions to symbol list

[Lukas Jünger]

Some functions of the plugin API were missing in
the symbol list. However, they are all used by
the contributed example plugins. QEMU fails to
load the plugin if the function symbol is not
exported.

Signed-off-by: Lukas Jünger 
---
 plugins/qemu-plugins.symbols | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 67b309ea2a..4834756ba3 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -1,11 +1,14 @@
 {
   qemu_plugin_bool_parse;
   qemu_plugin_get_hwaddr;
+  qemu_plugin_hwaddr_device_name;
   qemu_plugin_hwaddr_is_io;
+  qemu_plugin_hwaddr_phys_addr;
   qemu_plugin_insn_data;
   qemu_plugin_insn_disas;
   qemu_plugin_insn_haddr;
   qemu_plugin_insn_size;
+  qemu_plugin_insn_symbol;
   qemu_plugin_insn_vaddr;
   qemu_plugin_mem_is_big_endian;
   qemu_plugin_mem_is_sign_extended;
-- 
2.31.1

Re: [RFC PATCH v2 04/12] i386/sev: initialize SNP context

[Brijesh Singh]

On 9/5/21 4:19 AM, Dov Murik wrote:
>
> On 27/08/2021 1:26, Michael Roth wrote:
>> From: Brijesh Singh 
>>
>> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
>> the platform. The command checks whether SNP is enabled in the KVM, if
>> enabled then it allocates a new ASID from the SNP pool and calls the
>> firmware to initialize the all the resources.
>>
>
> From the KVM code ("[PATCH Part2 v5 24/45] KVM: SVM: Add
> KVM_SEV_SNP_LAUNCH_START command") it seems that KVM_SNP_INIT does *not*
> allocate the ASID; actually this is done in KVM_SEV_SNP_LAUNCH_START.

Actually, the KVM_SNP_INIT does allocate the ASID. If you look at the
driver code then in switch state, the SNP_INIT fallthrough to SEV_INIT
which will call sev_guest_init(). The sev_guest_init() allocates a new
ASID.
https://github.com/AMDESE/linux/blob/bb9ba49cd9b749d5551aae295c091d8757153dd7/arch/x86/kvm/svm/sev.c#L255

The LAUNCH_START simply binds the ASID to a guest.

thanks

Re: [RFC PATCH v2 04/12] i386/sev: initialize SNP context

[Brijesh Singh]

Hi Dov,

On 9/5/21 2:07 AM, Dov Murik wrote:
...
>
>>  
>>  uint64_t
>> @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
>> **errp)
>>  uint32_t ebx;
>>  uint32_t host_cbitpos;
>>  struct sev_user_data_status status = {};
>> +void *init_args = NULL;
>>  
>>  if (!sev_common) {
>>  return 0;
>> @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
>> **errp)
>>  sev_common->api_major = status.api_major;
>>  sev_common->api_minor = status.api_minor;
> Not visible here in the context: the code here is using the
> SEV_PLATFORM_STATUS command to get the build_id, api_major, and api_minor.
>
> I see that SNP has a new command SNP_PLATFORM_STATUS, which fills a
> struct sev_data_snp_platform_status (hmmm, I can't find the struct's
> definition; I assume it should look like Table 38 in 8.3.2 in SNP FW ABI
> document).

The API version can be queries either through the SNP_PLATFORM_STATUS or
SEV_PLATFORM_STATUS and they both report the same info. As the
definition of the sev_data_platform_status is concerned it should be
defined in the kernel include/linux/psp-sev.h.


> My questions are:
>
> 1. Is it OK to call the "legacy" SEV_PLATFORM_STATUS when about to init
> an SNP guest?

Yes, the legacy platform status command can be called on the SNP
initialized host.

I choose not to new command because we only care about the verison
string and that is available through either of these commands (SNP or
SEV platform status).

> 2. Do we want to save some info like installed TCB version and reported
> TCB version, and maybe other fields from SNP platform status?

If we decide to add a new QMP (query-sev-snp) then it makes sense to
export those fields so that a hypervisor console can give additional
information; But note that for the guest, all these are available in the
attestation report.


> 3. Should we check the state field in the platform status?
>
>
Good point, we could use the SNP platform status. I don't expect the
state to be different between the SNP platform_status and SEV
platform_status.


>>  
>> -if (sev_es_enabled()) {
>> +if (sev_snp_enabled()) {
>> +SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(sev_common);
>> +if (!kvm_kernel_irqchip_allowed()) {
>> +error_report("%s: SEV-SNP guests require in-kernel irqchip 
>> support",
>> + __func__);
> Most errors in this function use error_setg(errp, ...).  This should follow.
>
>
>> +goto err;
>> +}
>> +
>> +cmd = KVM_SEV_SNP_INIT;
>> +init_args = (void *)_snp_guest->kvm_init_conf;
>> +
>> +} else if (sev_es_enabled()) {
>>  if (!kvm_kernel_irqchip_allowed()) {
>>  error_report("%s: SEV-ES guests require in-kernel irqchip 
>> support",
>>   __func__);
> Not part of this patch, but this error_report (and another one in the
> SEV-ES case) should be converted to error_setg similarly.  Maybe add a
> separate patch for fixing this for SEV-ES.
>
>
>
>> @@ -1145,7 +1166,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
>> **errp)
>>  }
>>  
>>  trace_kvm_sev_init();
> Suggestions:
>
> 1. log the guest type (SEV / SEV-ES / SEV-SNP)
> 2. log the SNP init flags value when initializing an SNP guest

Noted.

thanks

virtio "transitional devices"?

[Alexander von Gluck IV]

Could someone explain to me what virtio "transitional devices" are?

https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-1020002

Are "Transitional devices" pre-1.0 specification?

Haiku's virtio driver (PCI) is looking for PCIID Devices 0x1000-0x103F


I've noticed qemu-6.1.0 has begun to offer PCI DeviceID  0x1040-0x107F to
the operating system breaking our virtio drivers.

Expanding our search range to 0x1040+ seems to solve the issues and gives
us a working virtio driver, but I feel like we should be checking for
other differences.

Has something changed in recent qemu's around virtio?  I don't see anything
documented in the release notes.

 -- Alex

Re: Virtualizing HP PA-RISC unix station other than B160L

[Richard Henderson]

On Sun, 5 Sep 2021, 10:30 ,  wrote:

> Hie Richard,
>
>
>
> For my company (Nexter Systems, France), I am using qemu-system-hppa for
> virtualizing HP PA-RISC workstations. That works well. You have made a very
> good job !
>
>
>
> But my machines are other than B160L (for example B180L), and I have to
> completely reinstall HP-UX on each emulated machine.
>
> If I do an iso system disk image of my B180L, this iso isn't bootable on
> qemu-system-hppa.
>
>
>
> Thus, my questions are :
>
>- Is it planned to emulate other HP unix workstations than B160L (for
>example B180L) ?
>- Or, what changes should I make to my iso image to do it usable ? If
>I replace the /boot filesystem of the B180L image with the B160L one, I get
>a kernel panic at boot time.
>
> Helge is the one that did all the hw support, I just did the CPU. There
are no real plans to do another machine. I'm not familiar with the specs
between the HP machines to know how much work that would be.

r~

[PATCH] util/compatfd.c: use libc signalfd wrapper instead of raw syscall

[Kacper Słomiński]

This allows the use of native signalfd instead of the sigtimedwait
based emulation on systems other than Linux.

Signed-off-by: Kacper Słomiński 
---
Apologies if I CC'd the wrong maintaineers, it's my first time
submitting patches to QEMU. According to get_maintainers.pl this
file has no maintainers, and the only system supported upstream
that supports signalfd natively is Linux.

Glibc has had the signalfd wrapper since version 2.8 (2008), and
musl has had it since at least version 0.5.0 (2011), and as such
I think using syscall directly is not necessary here.

Found this while porting QEMU to Managarm
(https://github.com/managarm/managarm).

 meson.build | 7 +++
 util/compatfd.c | 5 ++---
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/meson.build b/meson.build
index bf63784812..bcdfea5492 100644
--- a/meson.build
+++ b/meson.build
@@ -1415,10 +1415,9 @@ config_host_data.set('CONFIG_POSIX_MADVISE', 
cc.links(gnu_source_prefix + '''
   #include 
   int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }'''))
 config_host_data.set('CONFIG_SIGNALFD', cc.links(gnu_source_prefix + '''
-  #include 
-  #include 
-  #include 
-  int main(void) { return syscall(SYS_signalfd, -1, NULL, _NSIG / 8); }'''))
+  #include 
+  #include 
+  int main(void) { return signalfd(-1, NULL, SFD_CLOEXEC); }'''))
 config_host_data.set('CONFIG_SPLICE', cc.links(gnu_source_prefix + '''
   #include 
   #include 
diff --git a/util/compatfd.c b/util/compatfd.c
index a8ec525c6c..ab810c42a9 100644
--- a/util/compatfd.c
+++ b/util/compatfd.c
@@ -17,7 +17,7 @@
 #include "qemu/thread.h"
 
 #if defined(CONFIG_SIGNALFD)
-#include 
+#include 
 #endif
 
 struct sigfd_compat_info {
@@ -96,9 +96,8 @@ int qemu_signalfd(const sigset_t *mask)
 #if defined(CONFIG_SIGNALFD)
 int ret;
 
-ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8);
+ret = signalfd(-1, mask, SFD_CLOEXEC);
 if (ret != -1) {
-qemu_set_cloexec(ret);
 return ret;
 }
 #endif
-- 
2.33.0

Re: [PATCH v2] net/colo: check vnet_hdr_support flag when using virtio-net

[Lukas Straub]

On Thu, 26 Aug 2021 05:49:23 +
"Zhang, Chen"  wrote:

> > -Original Message-
> > From: Lukas Straub 
> > Sent: Sunday, August 22, 2021 12:25 AM
> > To: Xu, Tao3 
> > Cc: Zhang, Chen ; lizhij...@cn.fujitsu.com;
> > jasow...@redhat.com; qemu-devel@nongnu.org
> > Subject: Re: [PATCH v2] net/colo: check vnet_hdr_support flag when using
> > virtio-net
> > 
> > On Thu, 19 Aug 2021 09:27:17 +0800
> > Tao Xu  wrote:
> >   
> > > When COLO use only one vnet_hdr_support parameter between COLO  
> > network  
> > > filter(filter-mirror, filter-redirector or filter-rewriter and
> > > colo-compare, packet will not be parsed correctly. Acquire network
> > > driver related to COLO, if it is nirtio-net, check vnet_hdr_support
> > > flag of COLO network filter and colo-compare.
> > >
> > > Signed-off-by: Tao Xu 
> > > Signed-off-by: Zhang Chen 
> > > ---
> > >
> > > Changelog:
> > > v2:
> > >  Detect virtio-net driver and apply vnet_hdr_support
> > >  automatically. (Jason)
> > > ---
> > >  net/colo-compare.c| 57  
> > +++  
> > >  net/colo.c| 20 +++
> > >  net/colo.h|  4 +++
> > >  net/filter-mirror.c   | 21 
> > >  net/filter-rewriter.c | 10 
> > >  qapi/qom.json |  6 +
> > >  qemu-options.hx   |  6 +++--
> > >  7 files changed, 122 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > b100e7b51f..870bd05a41 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -110,6 +110,7 @@ struct CompareState {
> > >  char *sec_indev;
> > >  char *outdev;
> > >  char *notify_dev;
> > > +char *netdev;
> > >  CharBackend chr_pri_in;
> > >  CharBackend chr_sec_in;
> > >  CharBackend chr_out;
> > > @@ -838,6 +839,28 @@ static int compare_chr_can_read(void *opaque)
> > >  return COMPARE_READ_LEN_MAX;
> > >  }
> > >
> > > +static int colo_set_default_netdev(void *opaque, QemuOpts *opts,
> > > +Error **errp) {
> > > +const char *colo_obj_type, *netdev_from_filter;
> > > +char **netdev = (char **)opaque;
> > > +
> > > +colo_obj_type = qemu_opt_get(opts, "qom-type");
> > > +
> > > +if (colo_obj_type &&
> > > +(strcmp(colo_obj_type, "filter-mirror") == 0 ||
> > > + strcmp(colo_obj_type, "filter-redirector") == 0 ||
> > > + strcmp(colo_obj_type, "filter-rewriter") == 0)) {
> > > +netdev_from_filter = qemu_opt_get(opts, "netdev");
> > > +if (*netdev == NULL) {
> > > +*netdev = g_strdup(netdev_from_filter);
> > > +} else if (strcmp(*netdev, netdev_from_filter) != 0) {
> > > +warn_report("%s is using a different netdev from other COLO "
> > > +"component", colo_obj_type);
> > > +}
> > > +}
> > > +return 0;
> > > +}
> > > +  
> > 
> > Hi,
> > This doesn't properly handle the case where there are multiple network
> > devices and one is virtio-net and the other isn't. This would be a 
> > regression as
> > this worked fine before.  
> 
> No, If have multiple network device this patch just report a warning for it.
> You can still use virtio-net and others at the same time.

As you see it sets netdev to the first filter's netdev. So if
the first netdev is virtio-net and the 2nd is e1000 (for example) it'll
see virtio-net first and set *netdev to it for _both_ devices. Then it
sees the e1000 and prints the warning, but *netdev is still set to
virtio-net. So it'll enable vnet_hdr for e1000 too and segfault when
processing a packet because e1000 doesn't support vnet_hdr.

> >   
> > >  /*
> > >   * Called from the main thread on the primary for packets
> > >   * arriving over the socket from the primary.
> > > @@ -1050,6 +1073,21 @@ static void compare_set_vnet_hdr(Object *obj,
> > >  s->vnet_hdr = value;
> > >  }
> > >
> > > +static char *compare_get_netdev(Object *obj, Error **errp) {
> > > +CompareState *s = COLO_COMPARE(obj);
> > > +
> > > +return g_strdup(s->netdev);
> > > +}
> > > +
> > > +static void compare_set_netdev(Object *obj, const char *value, Error
> > > +**errp) {
> > > +CompareState *s = COLO_COMPARE(obj);
> > > +
> > > +g_free(s->netdev);
> > > +s->netdev = g_strdup(value);
> > > +}
> > > +
> > >  static char *compare_get_notify_dev(Object *obj, Error **errp)  {
> > >  CompareState *s = COLO_COMPARE(obj); @@ -1274,6 +1312,12 @@
> > > static void colo_compare_complete(UserCreatable *uc, Error **errp)
> > >  max_queue_size = MAX_QUEUE_SIZE;
> > >  }
> > >
> > > +if (!s->netdev) {
> > > +/* Set default netdev as the first colo netfilter found */
> > > +qemu_opts_foreach(qemu_find_opts("object"),
> > > +  colo_set_default_netdev, >netdev, NULL);
> > > +}
> > > +
> > >  if (find_and_check_chardev(, s->pri_indev, errp) ||
> > >  !qemu_chr_fe_init(>chr_pri_in, chr, errp)) {

Re: [PATCH] target/ppc: fix setting of CR flags in bcdcfsq

[Richard Henderson]

On 8/23/21 5:02 PM, Luis Pires wrote:

According to the ISA, CR should be set based on the source value, and
not on the packed decimal result.
The way this was implemented would cause GT, LT and EQ to be set
incorrectly when the source value was too large and the 31 least
significant digits of the packed decimal result ended up being all zero.
This would happen for source values of +/-10^31, +/-10^32, etc.

The new implementation fixes this and also skips the result calculation
altogether in case of src overflow.

Signed-off-by: Luis Pires
---
  target/ppc/int_helper.c | 61 -
  1 file changed, 48 insertions(+), 13 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 37/43] bsd-user: add stubbed out core dump support

[Richard Henderson]

On 9/3/21 1:47 AM, i...@bsdimp.com wrote:

From: Warner Losh

Add a stubbed-out version of the bsd-user fork's core dump support. This
allows elfload.c to be almost the same between what's upstream and
what's in qemu-project upstream w/o the burden of reviewing the core
dump support.

Signed-off-by: Stacey Son
Signed-off-by: Warner Losh
---
  bsd-user/elfcore.c | 10 ++
  bsd-user/elfload.c | 22 --
  bsd-user/qemu.h|  6 ++
  3 files changed, 36 insertions(+), 2 deletions(-)
  create mode 100644 bsd-user/elfcore.c


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 32/43] bsd-user: Rewrite target system call definintion glue

[Richard Henderson]

On 9/3/21 1:47 AM, i...@bsdimp.com wrote:

From: Warner Losh

Rewrite target definnitions to interface with the FreeBSD system calls.
This covers basic types (time_t, iovec, umtx_time, timespec, timeval,
rusage, rwusage) and basic defines (mmap, rusage). Also included are
FreeBSD version-specific variations.

Signed-off-by: Stacey Son
Signed-off-by: Warner Losh
---
  bsd-user/bsd-mman.h | 121 
  bsd-user/mmap.c |   2 -
  bsd-user/syscall_defs.h | 247 ++--
  3 files changed, 162 insertions(+), 208 deletions(-)
  delete mode 100644 bsd-user/bsd-mman.h


I think I gave you an
Acked-by: Richard Henderson 

Re: [PATCH v3 27/43] bsd-user: Implement --seed and initialize random state

[Richard Henderson]

On 9/3/21 1:47 AM, i...@bsdimp.com wrote:

From: Warner Losh 

Copy --seed implementation (translated from linux-user's newer command
line scheme to the older one bsd-user still uses). Initialize the
randomness with the glibc if a specific seed is specified or use the


FWIW, it's glib, not glibc.

Reviewed-by: Richard Henderson 


r~

Re: [PATCH v3 22/43] bsd-user: Include more things in qemu.h

[Richard Henderson]

On 9/3/21 1:47 AM, i...@bsdimp.com wrote:

From: Warner Losh

Include more header files to match bsd-user fork.

Signed-off-by: Warner Losh
---
  bsd-user/qemu.h | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 20/43] bsd-user: Move per-cpu code into target_arch_cpu.h

[Richard Henderson]

On 9/3/21 1:47 AM, i...@bsdimp.com wrote:

From: Warner Losh

Move cpu_loop() into target_cpu_loop(), and put that in
target_arch_cpu.h for each architecture.

Signed-off-by: Stacey Son
Signed-off-by: Warner Losh
---
  bsd-user/i386/target_arch_cpu.c   |   1 +
  bsd-user/i386/target_arch_cpu.h   | 209 
  bsd-user/main.c   | 317 ++
  bsd-user/qemu.h   |   1 +
  bsd-user/x86_64/target_arch_cpu.c |   1 +
  bsd-user/x86_64/target_arch_cpu.h | 247 +++
  6 files changed, 473 insertions(+), 303 deletions(-)
  create mode 100644 bsd-user/i386/target_arch_cpu.h
  create mode 100644 bsd-user/x86_64/target_arch_cpu.h


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 10/43] bsd-user: implement path searching

[Richard Henderson]

On 9/3/21 1:46 AM, i...@bsdimp.com wrote:

From: Warner Losh

Use the PATH to find the executable given a bare argument. We need to do
this so we can implement mixing native and emulated binaries (e.g.,
execing a x86 native binary from an emulated arm binary to optimize
parts of the build). By finding the binary, we will know how to exec it.

Signed-off-by: Stacey Son
Signed-off-by: Warner Losh
---
  bsd-user/bsdload.c | 36 +++-
  bsd-user/qemu.h|  3 ++-
  2 files changed, 37 insertions(+), 2 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v4 21/21] scripts: add loongarch64 binfmt config

[Richard Henderson]

On 9/2/21 2:41 PM, Song Gao wrote:

Signed-off-by: Song Gao
Signed-off-by: XiaoJuan Yang
---
  scripts/qemu-binfmt-conf.sh | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v4 20/21] target/loongarch: 'make check-tcg' support

[Richard Henderson]

On 9/2/21 2:41 PM, Song Gao wrote:

This patch support 'make check-tcg' after install cross-tools.

Signed-off-by: Song Gao
Signed-off-by: XiaoJuan Yang
---
  tests/tcg/configure.sh | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v4 19/21] target/loongarch: Add target build suport

[Richard Henderson]

On 9/2/21 2:41 PM, Song Gao wrote:

This patch add build loongarch-linux-user target support.

Signed-off-by: Song Gao
Signed-off-by: XiaoJuan Yang
---
  target/loongarch/meson.build | 18 ++
  target/meson.build   |  1 +
  2 files changed, 19 insertions(+)
  create mode 100644 target/loongarch/meson.build


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v4 17/21] LoongArch Linux User Emulation

[Richard Henderson]

On 9/2/21 2:41 PM, Song Gao wrote:

+#include "qemu/osdep.h"
+#include "qemu.h"
+#include "qemu-common.h"
+#include "cpu_loop-common.h"
+#include "elf.h"
+
+void cpu_loop(CPULoongArchState *env)
+{
+CPUState *cs = env_cpu(env);
+target_siginfo_t info;
+int trapnr;
+abi_long ret;
+
+for (;;) {
+cpu_exec_start(cs);
+trapnr = cpu_exec(cs);
+cpu_exec_end(cs);
+process_queued_cpu_work(cs);
+
+switch (trapnr) {
+case EXCP_INTERRUPT:
+/* just indicate that signals should be handled asap */
+break;
+case EXCP_SYSCALL:
+env->pc += 4;
+ret = do_syscall(env, env->gpr[11],
+ env->gpr[4], env->gpr[5],
+ env->gpr[6], env->gpr[7],
+ env->gpr[8], env->gpr[9],
+ -1, -1);
+if (ret == -TARGET_ERESTARTSYS) {
+env->pc -= 4;
+break;
+}
+if (ret == -TARGET_QEMU_ESIGRETURN) {
+/*
+ * Returning from a successful sigreturn syscall.
+ * Avoid clobbering register state.
+ */
+break;
+}
+env->gpr[4] = ret;
+break;
+case EXCP_ADE:
+info.si_signo = TARGET_SIGSEGV;
+info.si_errno = 0;
+info.si_code = TARGET_SEGV_MAPERR;
+info._sifields._sigfault._addr = env->badaddr;
+queue_signal(env, info.si_signo, QEMU_SI_FAULT, );
+break;
+case EXCP_INE:
+info.si_signo = TARGET_SIGILL;
+info.si_errno = 0;
+info.si_code = 0;
+queue_signal(env, info.si_signo, QEMU_SI_FAULT, );


Missing _addr = pc.


+break;
+case EXCP_FPE:
+info.si_signo = TARGET_SIGFPE;
+info.si_errno = 0;
+info.si_code = TARGET_FPE_FLTUNK;
+if (GET_FP_CAUSE(env->fcsr0) & FP_INVALID) {
+info.si_code = TARGET_FPE_FLTINV;
+} else if (GET_FP_CAUSE(env->fcsr0) & FP_DIV0) {
+info.si_code = TARGET_FPE_FLTDIV;
+} else if (GET_FP_CAUSE(env->fcsr0) & FP_OVERFLOW) {
+info.si_code = TARGET_FPE_FLTOVF;
+} else if (GET_FP_CAUSE(env->fcsr0) & FP_UNDERFLOW) {
+info.si_code = TARGET_FPE_FLTUND;
+} else if (GET_FP_CAUSE(env->fcsr0) & FP_INEXACT) {
+info.si_code = TARGET_FPE_FLTRES;
+}
+queue_signal(env, info.si_signo, QEMU_SI_FAULT, );


Likewise.


+case EXCP_BREAK:
+info.si_signo = TARGET_SIGTRAP;
+info.si_code = TARGET_TRAP_BRKPT;
+queue_signal(env, info.si_signo, QEMU_SI_FAULT, );


Likewise.

Note that there are a set of patches on list that introduce a force_sig_fault(signo, code, 
addr) function, so that none of these parameters get missed.


Missing an entry for EXCP_DEBUG, which should share code with EXCP_BREAK.


+struct sigframe {
+uint32_t sf_ass[4]; /* argument save space for o32 */


Surely there is no "o32" for loongarch?


+uint32_t sf_code[2];/* signal trampoline */


Note that there are patches on-list for moving the signal trampoline off of the 
stack.


diff --git a/linux-user/loongarch64/termbits.h 
b/linux-user/loongarch64/termbits.h
new file mode 100644
index 000..33e74ed
--- /dev/null
+++ b/linux-user/loongarch64/termbits.h
@@ -0,0 +1,229 @@
+#ifndef LINUX_USER_LOONGARCH_TERMBITS_H
+#define LINUX_USER_LOONGARCH_TERMBITS_H
+
+#define TARGET_NCCS 19


Surely you should be using generic/termbits.h?

We will prefer not to merge a linux-user port that is not upstream, because the ABI may 
change in between.  Can you provide a pointer to your kernel port in the meantime?



r~

Re: [RFC PATCH v2 11/12] i386/sev: sev-snp: add support for CPUID validation

[Dov Murik]

Hi Michael,

On 27/08/2021 1:26, Michael Roth wrote:
> SEV-SNP firmware allows a special guest page to be populated with a
> table of guest CPUID values so that they can be validated through
> firmware before being loaded into encrypted guest memory where they can
> be used in place of hypervisor-provided values[1].
> 
> As part of SEV-SNP guest initialization, use this process to validate
> the CPUID entries reported by KVM_GET_CPUID2 prior to initial guest
> start.
> 
> [1]: SEV SNP Firmware ABI Specification, Rev. 0.8, 8.13.2.6
> 
> Signed-off-by: Michael Roth 
> ---
>  target/i386/sev.c | 146 +-
>  1 file changed, 143 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 0009c93d28..72a6146295 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -153,6 +153,36 @@ static const char *const sev_fw_errlist[] = {
>  
>  #define SEV_FW_MAX_ERROR  ARRAY_SIZE(sev_fw_errlist)
>  
> +/*  doesn't expose this, so re-use the max from kvm.c */
> +#define KVM_MAX_CPUID_ENTRIES 100
> +
> +typedef struct KvmCpuidInfo {
> +struct kvm_cpuid2 cpuid;
> +struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
> +} KvmCpuidInfo;
> +
> +#define SNP_CPUID_FUNCTION_MAXCOUNT 64
> +#define SNP_CPUID_FUNCTION_UNKNOWN 0x
> +
> +typedef struct {
> +uint32_t eax_in;
> +uint32_t ecx_in;
> +uint64_t xcr0_in;
> +uint64_t xss_in;
> +uint32_t eax;
> +uint32_t ebx;
> +uint32_t ecx;
> +uint32_t edx;
> +uint64_t reserved;
> +} __attribute__((packed)) SnpCpuidFunc;
> +
> +typedef struct {
> +uint32_t count;
> +uint32_t reserved1;
> +uint64_t reserved2;
> +SnpCpuidFunc entries[SNP_CPUID_FUNCTION_MAXCOUNT];
> +} __attribute__((packed)) SnpCpuidInfo;
> +
>  static int
>  sev_ioctl(int fd, int cmd, void *data, int *error)
>  {
> @@ -1141,6 +1171,117 @@ detect_first_overlap(uint64_t start, uint64_t end, 
> Range *range_list,
>  return overlap;
>  }
>  
> +static int
> +sev_snp_cpuid_info_fill(SnpCpuidInfo *snp_cpuid_info,
> +const KvmCpuidInfo *kvm_cpuid_info)
> +{
> +size_t i;
> +
> +memset(snp_cpuid_info, 0, sizeof(*snp_cpuid_info));
> +
> +for (i = 0; kvm_cpuid_info->entries[i].function != 0x; i++) {

Maybe iterate only while i < kvm_cpuid_info.cpuid.nent ?

> +const struct kvm_cpuid_entry2 *kvm_cpuid_entry;
> +SnpCpuidFunc *snp_cpuid_entry;
> +
> +kvm_cpuid_entry = _cpuid_info->entries[i];
> +snp_cpuid_entry = _cpuid_info->entries[i];

There's no explicit check that i < KVM_MAX_CPUID_ENTRIES and i <
SNP_CPUID_FUNCTION_MAXCOUNT.  The !=0x condition might protect
against this but this is not really clear (the memset to 0xFF is done in
another function).

Since KVM_MAX_CPUID_ENTRIES is 100 and SNP_CPUID_FUNCTION_MAXCOUNT is
64, it seems possible that i will be 65 for example and then
snp_cpuid_info->entries[i] is an out-of-bounds read access.




> +
> +snp_cpuid_entry->eax_in = kvm_cpuid_entry->function;
> +if (kvm_cpuid_entry->flags == KVM_CPUID_FLAG_SIGNIFCANT_INDEX) {
> +snp_cpuid_entry->ecx_in = kvm_cpuid_entry->index;
> +}
> +snp_cpuid_entry->eax = kvm_cpuid_entry->eax;
> +snp_cpuid_entry->ebx = kvm_cpuid_entry->ebx;
> +snp_cpuid_entry->ecx = kvm_cpuid_entry->ecx;
> +snp_cpuid_entry->edx = kvm_cpuid_entry->edx;
> +
> +if (snp_cpuid_entry->eax_in == 0xD &&
> +(snp_cpuid_entry->ecx_in == 0x0 || snp_cpuid_entry->ecx_in == 
> 0x1)) {
> +snp_cpuid_entry->ebx = 0x240;
> +}

Can you please add a comment explaining this special case?




> +}
> +
> +if (i > SNP_CPUID_FUNCTION_MAXCOUNT) {

This can be checked at the top (before the for loop): compare
kvm_cpuid_info.cpuid.nent with SNP_CPUID_FUNCTION_MAXCOUNT.


> +error_report("SEV-SNP: CPUID count '%lu' exceeds max '%u'",
> + i, SNP_CPUID_FUNCTION_MAXCOUNT);
> +return -1;
> +}
> +
> +snp_cpuid_info->count = i;
> +
> +return 0;
> +}
> +
> +static void
> +sev_snp_cpuid_report_mismatches(SnpCpuidInfo *old,
> +SnpCpuidInfo *new)
> +{
> +size_t i;
> +

Add check that new->count == old->count.


> +for (i = 0; i < old->count; i++) {
> +SnpCpuidFunc *old_func, *new_func;
> +
> +old_func = >entries[i];
> +new_func = >entries[i];
> +
> +if (memcmp(old_func, new_func, sizeof(SnpCpuidFunc))) {

Maybe clearer:

if (*old_func != *new_func) ...


> +error_report("SEV-SNP: CPUID validation failed for function %x, 
> index: %x.\n"

Add "0x" prefixes before printing hex values (%x), otherwise we might
have confusing outputs such as "failed for function 13, index: 25" which
is unclear whether it's decimal or hex.


> + "provided: eax:0x%08x, ebx: 0x%08x, ecx: 0x%08x, 
> edx: 0x%08x\n"
> +   

Re: [PATCH v4 15/21] target/loongarch: Add branch instruction translation

[Richard Henderson]

On 9/2/21 2:41 PM, Song Gao wrote:

+static bool gen_rz_bc(DisasContext *ctx, arg_fmt_rjoffs21 *a, TCGCond cond)
+{
+TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
+TCGv src2 = tcg_constant_tl(0);
+
+gen_bc(ctx, src1, src2, (a->offs21 << 2), cond);
+return true;
+}
+static bool gen_cz_bc(DisasContext *ctx, arg_fmt_cjoffs21 *a, TCGCond cond)


Missing space.  Otherwise,
Reviewed-by: Richard Henderson 


r~

Re: [PATCH v4 14/21] target/loongarch: Add floating point load/store instruction translation

[Richard Henderson]

On 9/2/21 2:41 PM, Song Gao wrote:

This patch implement floating point load/store instruction translation.

This includes:
- FLD.{S/D}, FST.{S/D}
- FLDX.{S/D}, FSTX.{S/D}
- FLD{GT/LE}.{S/D}, FST{GT/LE}.{S/D}

Signed-off-by: Song Gao
Signed-off-by: XiaoJuan Yang
---
  target/loongarch/insn_trans/trans_fmemory.c | 143 
  target/loongarch/insns.decode   |  24 +
  target/loongarch/translate.c|   1 +
  3 files changed, 168 insertions(+)
  create mode 100644 target/loongarch/insn_trans/trans_fmemory.c


Reviewed-by: Richard Henderson 

I wonder if you want to add nanboxing for loads, since you did it for the arithmetic?  But 
certainly this is correct per spec as-is.



r~

Re: [PATCH v4 13/21] target/loongarch: Add floating point move instruction translation

[Richard Henderson]

On 9/2/21 2:40 PM, Song Gao wrote:

+TRANS(fmov_s, gen_mov, tcg_gen_mov_tl)


Hmm.  The spec says only the low 32-bits are modified?  But is also says that if the 
source is not in the single-precision format the result is uncertain?


I'm not sure how to reconcile these two statements.

Ideally the language would allow the result to be nanboxed...


r~

Re: [PATCH v4 13/21] target/loongarch: Add floating point move instruction translation

[Richard Henderson]

On 9/2/21 2:40 PM, Song Gao wrote:

+static bool gen_mov(DisasContext *ctx, arg_fmt_fdfj *a,
+void (*func)(TCGv, TCGv))
+{
+TCGv dest = cpu_fpr[a->fd];
+TCGv src = cpu_fpr[a->fj];


Maybe clearer as gen_f2f, to match the others?


+static void gen_movfrh2gr_s(TCGv dest, TCGv src)
+{
+TCGv t0 = tcg_temp_new();
+
+tcg_gen_extract_tl(dest, src, 32, 32);
+
+tcg_temp_free(t0);
+}


t0 is unused.
Use sextract_tl, or sari_tl...


+TRANS(movfr2gr_s, gen_f2r, EXT_NONE, tcg_gen_ext32s_tl)
+TRANS(movfr2gr_d, gen_f2r, EXT_NONE, tcg_gen_mov_tl)
+TRANS(movfrh2gr_s, gen_f2r, EXT_SIGN,  gen_movfrh2gr_s)


... which then means you don't need the EXT_SIGN here, and can drop the 
parameter entirely.


r~

Re: [PATCH v4 12/21] target/loongarch: Add floating point conversion instruction translation

[Richard Henderson]

On 9/2/21 2:40 PM, Song Gao wrote:

This patch implement floating point conversion instruction translation.

This includes:
- FCVT.S.D, FCVT.D.S
- FFINT.{S/D}.{W/L}, FTINT.{W/L}.{S/D}
- FTINT{RM/RP/RZ/RNE}.{W/L}.{S/D}
- FRINT.{S/D}

Signed-off-by: Song Gao
Signed-off-by: XiaoJuan Yang
---
  target/loongarch/fpu_helper.c| 393 +++
  target/loongarch/helper.h|  29 +++
  target/loongarch/insn_trans/trans_fcnv.c |  36 +++
  target/loongarch/insns.decode|  32 +++
  target/loongarch/translate.c |   1 +
  5 files changed, 491 insertions(+)
  create mode 100644 target/loongarch/insn_trans/trans_fcnv.c


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v4 11/21] target/loongarch: Add floating point comparison instruction translation

[Richard Henderson]

On 9/2/21 2:40 PM, Song Gao wrote:

+/* fcmp_cXXX_s */
+uint64_t helper_fcmp_c_s(CPULoongArchState *env, uint64_t fj,
+ uint64_t fk, uint32_t flags)
+{
+uint32_t t0, t1;
+uint64_t cmp = 0;
+t0 = (uint32_t)fj;
+t1 = (uint32_t)fk;
+
+if (flags) {
+if (flags & FCMP_LT) {
+cmp |= float32_lt_quiet(t0, t1, >fp_status);
+}
+if (flags & FCMP_EQ) {
+cmp |= float32_eq_quiet(t0, t1, >fp_status);
+}
+if (flags & FCMP_GT) {
+cmp |= float32_lt_quiet(t1, t0, >fp_status);
+}
+if (flags & FCMP_UN) {
+cmp |= float32_unordered_quiet(t1, t0, >fp_status);
+}
+} else {
+cmp = (float32_unordered_quiet(t1, t0, >fp_status), 0);
+}


This is silly, especially the form of the last.  You want

FloatRelation cmp = float32_unordered_quiet(t1, t0, >fp_status);
bool ret;

switch (cmp) {
case float_relation_less:
ret = (flags & FCMP_LT);
break;
case float_relation_equal:
ret = (flags & FCMP_EQ);
break;
case float_relation_greater:
ret = (flags & FCMP_GT);
break;
case float_relation_unordered:
ret = (flags & FCMP_UN);
break;
default:
g_assert_not_reached();
}
return ret;

And of course the switch can be split out to a common subroutine for use with the other 3 
comparison helpers.



+static bool trans_fcmp_cond_s(DisasContext *ctx, arg_fcmp_cond_s *a)
+{
+TCGv var = tcg_temp_new();
+TCGv_i32 flags = NULL;
+
+switch (a->fcond) {
+/* caf */
+case  0:
+flags = tcg_constant_i32(0);
+gen_helper_fcmp_c_s(var, cpu_env, cpu_fpr[a->fj],
+cpu_fpr[a->fk], flags);
+break;
+/* saf */
+case 1:
+flags = tcg_constant_i32(0);
+gen_helper_fcmp_s_s(var, cpu_env, cpu_fpr[a->fj],
+cpu_fpr[a->fk], flags);
+break;


Hmm.  Perhaps put the integer flags into a table?

fn = (a->fcond & 1 ? gen_helper_fcmp_s_s : gen_helper_fcmp_c_s);
flags = fcond_table[a->fcond >> 1];

fn(var, cpu_env, cpu_fpr[a->fj], cpu_fpr[a->fk],
   tcg_constant_i32(flags));


+static bool trans_fcmp_cond_d(DisasContext *ctx, arg_fcmp_cond_d *a)


You'd get to share the table with this function.


--- a/target/loongarch/translate.h
+++ b/target/loongarch/translate.h
@@ -11,6 +11,11 @@
  
  #include "exec/translator.h"
  
+#define FCMP_LT   0x0001  /* fp0 < fp1 */

+#define FCMP_EQ   0x0010  /* fp0 = fp1 */
+#define FCMP_GT   0x0100  /* fp1 < fp0 */
+#define FCMP_UN   0x1000  /* unordered */


Hmm.  Better in internals.h?  I don't think you want to include translate.h 
into fpu_helper.c.


r~

Re: [RFC PATCH v2 04/12] i386/sev: initialize SNP context

[Dov Murik]

On 27/08/2021 1:26, Michael Roth wrote:
> From: Brijesh Singh 
> 
> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
> the platform. The command checks whether SNP is enabled in the KVM, if
> enabled then it allocates a new ASID from the SNP pool and calls the
> firmware to initialize the all the resources.
> 


>From the KVM code ("[PATCH Part2 v5 24/45] KVM: SVM: Add
KVM_SEV_SNP_LAUNCH_START command") it seems that KVM_SNP_INIT does *not*
allocate the ASID; actually this is done in KVM_SEV_SNP_LAUNCH_START.

If that is indeed the case, I suggest removing this sentence here and
adding it in the appropriate QEMU step (patch 5?).

-Dov



> Signed-off-by: Brijesh Singh 
> Signed-off-by: Michael Roth 
> ---
>  target/i386/sev-stub.c |  6 ++
>  target/i386/sev.c  | 27 ---
>  target/i386/sev_i386.h |  1 +
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 

[...]

Re: [PATCH v4 10/21] target/loongarch: Add floating point arithmetic instruction translation

[Richard Henderson]

On 9/2/21 2:40 PM, Song Gao wrote:

+const FloatRoundMode ieee_rm[4] = {


Make static.


+int ieee_ex_to_loongarch(int xcpt)


This function is only used in this file.  Better to make it static and remove the 
declaration from translate.h, which allows you to remove that include.



+static inline void update_fcsr0(CPULoongArchState *env, uintptr_t pc)


Remove inline.


+{
+int flags = ieee_ex_to_loongarch(get_float_exception_flags(
+  >fp_status));
+
+SET_FP_CAUSE(env->fcsr0, flags);
+if (flags) {


Hmm.  Between the two functions you're testing for zero twice.  Perhaps better 
to rearrange as

{
int flags = get_float_exception_flags(>fp_status);

if (!flags) {
SET_FP_CAUSE(env->fcsr0, 0);
return;
}

set_float_exception_flags(0, >fp_status);
flags = ieee_ex_to_loongarch(flags);
SET_FP_CAUSE(env->fcsr0, flags);

if (GET_FP_ENABLE(env->fcsr0) & flags) {
do_raise_exception(env, EXCP_FPE, pc);
}
UPDATE_FP_FLAGS(env->fcsr0, flags);
}

and remove the explicit zero test in ieee_ex_to_loongarch.


+uint64_t helper_flogb_s(CPULoongArchState *env, uint64_t fj)
+{
+uint64_t fd;
+uint32_t fp;
+float_status *status = >fp_status;
+FloatRoundMode old_mode = get_float_rounding_mode(status);
+
+set_float_exception_flags(0, status);


Unnecessary, since the steady-state is already zero.


+set_float_exception_flags(get_float_exception_flags(status) &
+  (~float_flag_inexact), status);
+update_fcsr0(env, GETPC());


Hmm.  Worth adding a parameter to update_fcsr0, a mask to suppress?  Or a common 
subroutine like


update_fcsr0_mask(env, GETPC(), float_flag_inexact);

static void update_fcsr0(env, ra)
{
update_fcsr0_mask(env, ra, 0);
}


+static bool trans_fcopysign_s(DisasContext *ctx, arg_fmt_fdfjfk *a)
+{
+tcg_gen_deposit_i64(cpu_fpr[a->fd], cpu_fpr[a->fk], cpu_fpr[a->fj], 0, 31);
+return true;
+}
+
+static bool trans_fcopysign_d(DisasContext *ctx, arg_fmt_fdfjfk *a)
+{
+tcg_gen_deposit_i64(cpu_fpr[a->fd], cpu_fpr[a->fk], cpu_fpr[a->fj], 0, 63);
+return true;
+}


For what it's worth, you could treat the abs and neg instructions similarly, 
inline.


r~

Re: [PATCH v10 03/16] target/riscv: clwz must ignore high bits (use shift-left & changed logic)

[Philipp Tomsich]

On Sun 5. Sep 2021 at 11:11, Richard Henderson
 wrote:
>
> On 9/4/21 10:35 PM, Philipp Tomsich wrote:
> > Assume clzw being executed on a register that is not sign-extended, such
> > as for the following sequence that uses (1ULL << 63) | 392 as the operand
> > to clzw:
> >   bseti   a2, zero, 63
> >   addia2, a2, 392
> >   clzwa3, a2
> > The correct result of clzw would be 23, but the current implementation
> > returns -32 (as it performs a 64bit clz, which results in 0 leading zero
> > bits, and then subtracts 32).
> >
> > Fix this by changing the implementation to:
> >   1. shift the original register up by 32
> >   2. performs a target-length (64bit) clz
> >   3. return 32 if no bits are set
> >
> > Signed-off-by: Philipp Tomsich 
> > ---
> >
> > Changes in v10:
> > - New patch, fixing correctnes for clzw called on a register with undefined
> >(as in: not properly sign-extended) upper bits.
>
> But we have
>
>  return gen_unary(ctx, a, EXT_ZERO, gen_clzw);
>
> should *not* be undefined.  Ah, what's missing is
>
>  ctx->w = true;
>
> within trans_clzw to cause the extend to take effect.
>
> There are a few other "w" functions that are missing that set, though they 
> use EXT_NONE so
> there is no visible bug, it would probably be best to set w anyway.


I had originally considered that (it would have to be ctx->w = true;
and EXT_SIGN),
but that has the side-effect of performing an extension on the result
of the clzw as
well (i.e. bot get_gpr and set_gpr are extended).

However, this is not what clzw does: it only ignores the upper bits
and then produces
a result in the value-range [0..32]. An extension on the result of
either a clz or clzw
is never needed (we have been fighting that problem in GCC and had to
use patterns
for the combiner), so I don't want to introduce this inefficiency in qemu.

If you have EXT_SIGN (or _ZERO), we will end up with 2 additional
extensions (one
on the argument, one on the result) in addition to the 2 other tcg
operations that we
need (i.e. clzi/subi for the extending case, shli/clzi for the proposed fix).

So I believe that this commit is the best fix:
1. It doesn't mark this as a w-form (i.e. ignores the high-bits on the
input _and_
extends the output), as it only treats the input as 32bit, but the
output is xlen.
2. It doesn't introduce any unnecessary extensions, but handles the
case in-place.

Philipp.

Re: [PULL 00/14] aspeed queue

[Cédric Le Goater]

On 9/5/21 1:03 AM, Philippe Mathieu-Daudé wrote:
> On 9/4/21 7:33 AM, Cédric Le Goater wrote:
>> On 9/3/21 10:41 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Peter,
>>>
>>> On 9/3/21 9:40 PM, Cédric Le Goater wrote:
 The following changes since commit 
 8880cc4362fde4ecdac0b2092318893118206fcf:

   Merge remote-tracking branch 
 'remotes/cschoenebeck/tags/pull-9p-20210902' into staging (2021-09-03 
 08:27:38 +0100)

 are available in the Git repository at:

   https://github.com/legoater/qemu/ tags/pull-aspeed-20210903

 for you to fetch changes up to 907796622b2a6b945c87641d94e254ac898b96ae:

   hw/arm/aspeed: Add Fuji machine type (2021-09-03 18:43:16 +0200)

 
 Aspeed patches :

 * MAC enablement fixes (Guenter)
 * Watchdog  and pca9552 fixes (Andrew)
 * GPIO fixes (Joel)
 * AST2600A3 SoC and DPS310 models (Joel)
 * New Fuji BMC machine (Peter)

 
>>>
 Peter Delevoryas (3):
   hw/arm/aspeed: Initialize AST2600 UART clock selection registers
   hw/arm/aspeed: Allow machine to set UART default
   hw/arm/aspeed: Add Fuji machine type
>>>
>>> I have a pending question with the last patch, do you mind holding
>>> this PR until it is resolved with Cédric and the patch author please?
>>>
>>> Thanks,
>>>
>>> Phil.
>>>
>>
>> I guess we can drop the following from the commit log : 
>>
>>  git clone https://github.com/facebook/openbmc
>>  cd openbmc
>>  ./sync_yocto.sh
>>  source openbmc-init-build-env fuji build-fuji
>>  bitbake fuji-image
>>  dd if=/dev/zero of=/tmp/fuji.mtd bs=1M count=128
>>  dd if=./tmp/deploy/images/fuji/flash-fuji of=/tmp/fuji.mtd \
>>  bs=1k conv=notrunc
>>  
>>  git clone --branch aspeed-next https://github.com/peterdelevoryas/qemu
>>  cd qemu
>>  ./configure --target-list=arm-softmmu --disable-vnc
>>  make -j $(nproc)
>>  ./build/arm-softmmu/qemu-system-arm \
>>  -machine fuji-bmc \
>>  -drive file=/tmp/fuji.mtd,format=raw,if=mtd \
>>  -serial stdio \
>>  -nic user,hostfwd=::-:22
>>  sshpass -p 0penBmc ssh root@localhost -p 
> 
> Sounds good. Eventually document that in docs/system/arm/aspeed.rst
> in a follow up patch?
> 
> Regards,
> 
> Phil.
> 


Peter D, 

Could you please resend the "hw/arm/aspeed: Add Fuji machine type"
patch addressing Phil's comment. I will resend a PR with the 
update.

Thanks,

C. 

Re: [PATCH v4 09/21] target/loongarch: Add fixed point extra instruction translation

[Richard Henderson]

On 9/2/21 2:40 PM, Song Gao wrote:

This patch implement fixed point extra instruction translation.

This includes:
- CRC[C].W.{B/H/W/D}.W
- SYSCALL
- BREAK
- ASRT{LE/GT}.D
- RDTIME{L/H}.W, RDTIME.D
- CPUCFG

Signed-off-by: Song Gao
Signed-off-by: XiaoJuan Yang
---
  target/loongarch/helper.h |  4 ++
  target/loongarch/insn_trans/trans_extra.c | 88 +++
  target/loongarch/insns.decode | 25 +
  target/loongarch/op_helper.c  | 26 +
  target/loongarch/translate.c  |  1 +
  5 files changed, 144 insertions(+)
  create mode 100644 target/loongarch/insn_trans/trans_extra.c


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2 11/24] target/i386: Move x86_cpu_exec_interrupt() under sysemu/ folder

[Richard Henderson]

On 9/5/21 1:55 AM, Philippe Mathieu-Daudé wrote:

Following the logic of commit 30493a030ff ("i386: split seg_helper
into user-only and sysemu parts"), move x86_cpu_exec_interrupt()
under sysemu/seg_helper.c.

Signed-off-by: Philippe Mathieu-Daudé
---
I prefer to not squash this into the previous patch because the
ifdef'ry removal (in previous patch) is not trivial IMO.
---
  target/i386/tcg/seg_helper.c| 64 
  target/i386/tcg/sysemu/seg_helper.c | 65 +
  2 files changed, 65 insertions(+), 64 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2 01/24] target/avr: Remove pointless use of CONFIG_USER_ONLY definition

[Richard Henderson]

On 9/5/21 1:55 AM, Philippe Mathieu-Daudé wrote:

Commit f1c671f96cb ("target/avr: Introduce basic CPU class object")
added to target/avr/cpu.h:

   #ifdef CONFIG_USER_ONLY
   #error "AVR 8-bit does not support user mode"
   #endif

Remove the CONFIG_USER_ONLY definition introduced by mistake in
commit 78271684719 ("cpu: tcg_ops: move to tcg-cpu-ops.h, keep a
pointer in CPUClass").

Reported-by: Richard Henderson
Signed-off-by: Philippe Mathieu-Daudé
---
  target/avr/cpu.c | 3 ---
  1 file changed, 3 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH] user: Mark cpu_loop() with noreturn attribute

[Richard Henderson]

On 9/5/21 2:04 AM, Philippe Mathieu-Daudé wrote:

cpu_loop() never exits, so mark it with QEMU_NORETURN.

Signed-off-by: Philippe Mathieu-Daudé
---
  bsd-user/qemu.h   | 2 +-
  linux-user/qemu.h | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v10 03/16] target/riscv: clwz must ignore high bits (use shift-left & changed logic)

[Richard Henderson]

On 9/4/21 10:35 PM, Philipp Tomsich wrote:

Assume clzw being executed on a register that is not sign-extended, such
as for the following sequence that uses (1ULL << 63) | 392 as the operand
to clzw:
bseti   a2, zero, 63
addia2, a2, 392
clzwa3, a2
The correct result of clzw would be 23, but the current implementation
returns -32 (as it performs a 64bit clz, which results in 0 leading zero
bits, and then subtracts 32).

Fix this by changing the implementation to:
  1. shift the original register up by 32
  2. performs a target-length (64bit) clz
  3. return 32 if no bits are set

Signed-off-by: Philipp Tomsich 
---

Changes in v10:
- New patch, fixing correctnes for clzw called on a register with undefined
   (as in: not properly sign-extended) upper bits.


But we have

return gen_unary(ctx, a, EXT_ZERO, gen_clzw);

should *not* be undefined.  Ah, what's missing is

ctx->w = true;

within trans_clzw to cause the extend to take effect.

There are a few other "w" functions that are missing that set, though they use EXT_NONE so 
there is no visible bug, it would probably be best to set w anyway.



r~

Re: [PATCH v10 02/16] target/riscv: fix clzw implementation to operate on arg1

[Richard Henderson]

On 9/4/21 10:35 PM, Philipp Tomsich wrote:

The refactored gen_clzw() uses ret as its argument, instead of arg1.
Fix it.

Signed-off-by: Philipp Tomsich
---

Changes in v10:
- New patch, fixing regressions discovered with x264_r.


Fixes: 60903915050 ("target/riscv: Add DisasExtend to gen_unary")
Reviewed-by: Richard Henderson 


r~

Re: [PATCH v10 01/16] target/riscv: Introduce temporary in gen_add_uw()

[Richard Henderson]

On 9/4/21 10:35 PM, Philipp Tomsich wrote:

Following the recent changes in translate.c, gen_add_uw() causes
failures on CF3 and SPEC2017 due to the reuse of arg1.  Fix these
regressions by introducing a temporary.

Signed-off-by: Philipp Tomsich
---

Changes in v10:
- new patch

  target/riscv/insn_trans/trans_rvb.c.inc | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)


Fixes: 191d1dafae9c ("target/riscv: Add DisasExtend to gen_arith*")
Reviewed-by: Richard Henderson 


r~

Re: [PATCH] ide: Cap LBA28 capacity announcement to 2^28-1

[Samuel Thibault]

Ping?

Samuel Thibault, le mar. 24 août 2021 12:43:44 +0200, a ecrit:
> The LBA28 capacity (at offsets 60/61 of identification) is supposed to
> express the maximum size supported by LBA28 commands. If the device is
> larger than this, we have to cap it to 2^28-1.
> 
> At least NetBSD happens to be using this value to determine whether to use
> LBA28 or LBA48 for its commands, using LBA28 for sectors that don't need
> LBA48. This commit thus fixes NetBSD access to disks larger than 128GiB.
> 
> Signed-off-by: Samuel Thibault 
> ---
>  hw/ide/core.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index fd69ca3167..e28f8aad61 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -98,8 +98,12 @@ static void put_le16(uint16_t *p, unsigned int v)
>  static void ide_identify_size(IDEState *s)
>  {
>  uint16_t *p = (uint16_t *)s->identify_data;
> -put_le16(p + 60, s->nb_sectors);
> -put_le16(p + 61, s->nb_sectors >> 16);
> +int64_t nb_sectors_lba28 = s->nb_sectors;
> +if (nb_sectors_lba28 >= 1 << 28) {
> +nb_sectors_lba28 = (1 << 28) - 1;
> +}
> +put_le16(p + 60, nb_sectors_lba28);
> +put_le16(p + 61, nb_sectors_lba28 >> 16);
>  put_le16(p + 100, s->nb_sectors);
>  put_le16(p + 101, s->nb_sectors >> 16);
>  put_le16(p + 102, s->nb_sectors >> 32);
> -- 
> 2.32.0
> 

Re: [RFC PATCH v2 04/12] i386/sev: initialize SNP context

[Dov Murik]

Hi Michael,

On 27/08/2021 1:26, Michael Roth wrote:
> From: Brijesh Singh 
> 
> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
> the platform. The command checks whether SNP is enabled in the KVM, if
> enabled then it allocates a new ASID from the SNP pool and calls the
> firmware to initialize the all the resources.
> 
> Signed-off-by: Brijesh Singh 
> Signed-off-by: Michael Roth 
> ---
>  target/i386/sev-stub.c |  6 ++
>  target/i386/sev.c  | 27 ---
>  target/i386/sev_i386.h |  1 +
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index 0227cb5177..e4fb8e882e 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -81,3 +81,9 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
>  error_setg(errp, "SEV is not available in this QEMU");
>  return NULL;
>  }
> +
> +bool
> +sev_snp_enabled(void)
> +{
> +return false;
> +}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index ba08b7d3ab..b8bd6ed9ea 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -614,12 +614,21 @@ sev_enabled(void)
>  return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON);
>  }
>  
> +bool
> +sev_snp_enabled(void)
> +{
> +ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
> +
> +return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_SNP_GUEST);
> +}
> +
>  bool
>  sev_es_enabled(void)
>  {
>  ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
>  
> -return sev_enabled() && (SEV_GUEST(cgs)->policy & SEV_POLICY_ES);
> +return sev_snp_enabled() ||
> +(sev_enabled() && SEV_GUEST(cgs)->policy & SEV_POLICY_ES);
>  }
>  
>  uint64_t
> @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
> **errp)
>  uint32_t ebx;
>  uint32_t host_cbitpos;
>  struct sev_user_data_status status = {};
> +void *init_args = NULL;
>  
>  if (!sev_common) {
>  return 0;
> @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
> **errp)
>  sev_common->api_major = status.api_major;
>  sev_common->api_minor = status.api_minor;

Not visible here in the context: the code here is using the
SEV_PLATFORM_STATUS command to get the build_id, api_major, and api_minor.

I see that SNP has a new command SNP_PLATFORM_STATUS, which fills a
struct sev_data_snp_platform_status (hmmm, I can't find the struct's
definition; I assume it should look like Table 38 in 8.3.2 in SNP FW ABI
document).

My questions are:

1. Is it OK to call the "legacy" SEV_PLATFORM_STATUS when about to init
an SNP guest?
2. Do we want to save some info like installed TCB version and reported
TCB version, and maybe other fields from SNP platform status?
3. Should we check the state field in the platform status?



>  
> -if (sev_es_enabled()) {
> +if (sev_snp_enabled()) {
> +SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(sev_common);
> +if (!kvm_kernel_irqchip_allowed()) {
> +error_report("%s: SEV-SNP guests require in-kernel irqchip 
> support",
> + __func__);

Most errors in this function use error_setg(errp, ...).  This should follow.


> +goto err;
> +}
> +
> +cmd = KVM_SEV_SNP_INIT;
> +init_args = (void *)_snp_guest->kvm_init_conf;
> +
> +} else if (sev_es_enabled()) {
>  if (!kvm_kernel_irqchip_allowed()) {
>  error_report("%s: SEV-ES guests require in-kernel irqchip 
> support",
>   __func__);

Not part of this patch, but this error_report (and another one in the
SEV-ES case) should be converted to error_setg similarly.  Maybe add a
separate patch for fixing this for SEV-ES.



> @@ -1145,7 +1166,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
> **errp)
>  }
>  
>  trace_kvm_sev_init();

Suggestions:

1. log the guest type (SEV / SEV-ES / SEV-SNP)
2. log the SNP init flags value when initializing an SNP guest


-Dov

> -ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, _error);
> +ret = sev_ioctl(sev_common->sev_fd, cmd, init_args, _error);
>  if (ret) {
>  error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'",
> __func__, ret, fw_error, fw_error_to_str(fw_error));
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index ae6d840478..e0e1a599be 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -29,6 +29,7 @@
>  #define SEV_POLICY_SEV  0x20
>  
>  extern bool sev_es_enabled(void);
> +extern bool sev_snp_enabled(void);
>  extern uint64_t sev_get_me_mask(void);
>  extern SevInfo *sev_get_info(void);
>  extern uint32_t sev_get_cbit_position(void);
>