Re: [RFC PATCH 10/27] vhost: Allocate shadow vring

[Stefan Hajnoczi]

On Fri, Nov 20, 2020 at 07:50:48PM +0100, Eugenio Pérez wrote:
> Signed-off-by: Eugenio Pérez 
> ---
>  hw/virtio/vhost-sw-lm-ring.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-sw-lm-ring.c b/hw/virtio/vhost-sw-lm-ring.c
> index cbf53965cd..cd7b5ba772 100644
> --- a/hw/virtio/vhost-sw-lm-ring.c
> +++ b/hw/virtio/vhost-sw-lm-ring.c
> @@ -16,8 +16,11 @@
>  #include "qemu/event_notifier.h"
>  
>  typedef struct VhostShadowVirtqueue {
> +struct vring vring;
>  EventNotifier hdev_notifier;
>  VirtQueue *vq;
> +
> +vring_desc_t descs[];
>  } VhostShadowVirtqueue;

VhostShadowVirtqueue is starting to look like VirtQueue. Can the shadow
vq code simply use the VirtIODevice's VirtQueues instead of duplicating
this?

What I mean is:

1. Disable the vhost hdev vq and sync the avail index back to the
   VirtQueue.
2. Move the irq fd to the VirtQueue as its guest notifier.
3. Install the shadow_vq_handler() as the VirtQueue's handle_output
   function.
4. Move the call fd to the VirtQueue as its host notifier.

Now we can process requests from the VirtIODevice's VirtQueue using
virtqueue_pop() and friends. We're also in sync and ready for vmstate
save/load.


signature.asc
Description: PGP signature

[PATCH] virtiofsd: update FUSE_FORGET comment on "lo_inode.nlookup"

[Laszlo Ersek]

Miklos confirms it's *only* the FUSE_FORGET request that the client can
use for decrementing "lo_inode.nlookup".

Cc: "Dr. David Alan Gilbert" 
Cc: Miklos Szeredi 
Cc: Stefan Hajnoczi 
Fixes: 1222f015558fc34cea02aa3a5a92de608c82cec8
Signed-off-by: Laszlo Ersek 
---
 tools/virtiofsd/passthrough_ll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 06543b20dcbb..d3be680e92c3 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -113,7 +113,7 @@ struct lo_inode {
  * This counter keeps the inode alive during the FUSE session.
  * Incremented when the FUSE inode number is sent in a reply
  * (FUSE_LOOKUP, FUSE_READDIRPLUS, etc).  Decremented when an inode is
- * released by requests like FUSE_FORGET, FUSE_RMDIR, FUSE_RENAME, etc.
+ * released by a FUSE_FORGET request.
  *
  * Note that this value is untrusted because the client can manipulate
  * it arbitrarily using FUSE_FORGET requests.
-- 
2.19.1.3.g30247aa5d201

Re: [PATCH for-6.0] accel: Wire accel to /machine

[Roman Bolshakov]

On Mon, Dec 07, 2020 at 05:44:19PM +, Daniel P. Berrangé wrote:
> On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote:
> > An outstanding issue is whether management applications can rely on the
> > value of /machine/accel/type and output of qom-list-types command [2][3]
> > to get current and present accels?
> > 
> > i.e. would it be ok if libvirt assumes that everything up to the first
> > dash in the accel type is the name of the accel (as specified via -M
> > accel=ACCEL flag) when it performs QEMU probing?
> 
> Hmm, I think it is not nice - we shouldn't have to parse the
> accel type names - IMHO typenames should be considered arbitrary
> opaque strings, even if they'll not be expected to change.
> 

Thanks Daniel. I'll then send v2 of ad-hoc QMP/HMP query. By the way, do
we need the HMP query if we get accel in QOM?

Regards,
Roman

Re: [RFC PATCH 09/27] vhost: Route host->guest notification through qemu

[Stefan Hajnoczi]

On Fri, Nov 20, 2020 at 07:50:47PM +0100, Eugenio Pérez wrote:
> Signed-off-by: Eugenio Pérez 
> ---
>  hw/virtio/vhost-sw-lm-ring.c |  3 +++
>  hw/virtio/vhost.c| 20 
>  2 files changed, 23 insertions(+)

I'm not sure I understand what is going here. The guest notifier masking
feature exists to support MSI masking semantics. It looks like this
patch repurposes the notifier to decouple the vhost hdev from the virtio
device's irqfd? But this breaks MSI masking. I think you need to set up
your own eventfd and assign it to the vhost hdev's call fd instead of
using the mask notifier.

> 
> diff --git a/hw/virtio/vhost-sw-lm-ring.c b/hw/virtio/vhost-sw-lm-ring.c
> index 0192e77831..cbf53965cd 100644
> --- a/hw/virtio/vhost-sw-lm-ring.c
> +++ b/hw/virtio/vhost-sw-lm-ring.c
> @@ -50,6 +50,9 @@ VhostShadowVirtqueue *vhost_sw_lm_shadow_vq(struct 
> vhost_dev *dev, int idx)
>  r = dev->vhost_ops->vhost_set_vring_kick(dev, );
>  assert(r == 0);
>  
> +vhost_virtqueue_mask(dev, dev->vdev, idx, true);
> +vhost_virtqueue_pending(dev, idx);

Why is the mask notifier cleared? Could we lose a guest notification
here?

> +
>  return svq;
>  }
>  
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1d55e26d45..9352c56bfa 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -960,12 +960,29 @@ static void handle_sw_lm_vq(VirtIODevice *vdev, 
> VirtQueue *vq)
>  vhost_vring_kick(svq);
>  }
>  
> +static void vhost_handle_call(EventNotifier *n)
> +{
> +struct vhost_virtqueue *hvq = container_of(n,
> +  struct vhost_virtqueue,
> +  masked_notifier);
> +struct vhost_dev *vdev = hvq->dev;
> +int idx = vdev->vq_index + (hvq == >vqs[0] ? 0 : 1);

vhost-net-specific hack

> +VirtQueue *vq = virtio_get_queue(vdev->vdev, idx);
> +
> +if (event_notifier_test_and_clear(n)) {
> +virtio_queue_invalidate_signalled_used(vdev->vdev, idx);
> +virtio_notify_irqfd(vdev->vdev, vq);

/* TODO push used elements into vq? */


signature.asc
Description: PGP signature

Re: [RFC PATCH 08/27] vhost: Add a flag for software assisted Live Migration

[Stefan Hajnoczi]

On Fri, Nov 20, 2020 at 07:50:46PM +0100, Eugenio Pérez wrote:
> @@ -1571,6 +1577,13 @@ void vhost_dev_disable_notifiers(struct vhost_dev 
> *hdev, VirtIODevice *vdev)
>  BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>  int i, r;
>  
> +if (hdev->sw_lm_enabled) {
> +/* We've been called after migration is completed, so no need to
> +   disable it again
> +*/
> +return;
> +}
> +
>  for (i = 0; i < hdev->nvqs; ++i) {
>  r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + 
> i,
>   false);

What is the purpose of this?


signature.asc
Description: PGP signature

Re: [PATCH 7/9] vnc: force initial resize message

[Gerd Hoffmann]

On Fri, Dec 04, 2020 at 03:57:23PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Dec 3, 2020 at 3:12 PM Gerd Hoffmann  wrote:
> 
> > The vnc server should send desktop resize notifications unconditionally
> > on a new client connect, for feature negotiation reasons.  Add a bool
> > flag to vnc_desktop_resize() to force sending the message even in case
> > there is no size change.
> >
> > Signed-off-by: Gerd Hoffmann 
> >
> 
> In principle, this looks harmless. But the spec says:
> 
> "The server should only send a *DesktopSize* pseudo-rectangle when an
> actual change of the framebuffer dimensions has occurred. Some clients
> respond to a *DesktopSize* pseudo-rectangle in a way that could send the
> system into an infinite loop if the server sent out the pseudo-rectangle
> for anything other than an actual change."
> (
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#server-semantics
> )
> 
> I can't say if sending desktop resize during the initial SetEncoding phase
> is really compliant with the specification. Also, it's unclear to me if the
> client is allowed to SetEncoding multiple times (in which there would be no
> dimension change occurring).
> 
> What did you fix with this? Is it worth a clarification in the
> specification?

Well, for ExtendedDesktopResize the spec explicitly asked for this.
But, yes, for DesktopResize this is not needed.  But it also shouldn't
cause much trouble.  It is sent before any actual display updates, so
concerns whenever the client should consider the screen content invalid
or not are moot.

I could squash this into patch #8 and do it for ExtendedDesktopResize
only ...

take care,
  Gerd

[PATCH 1/2] savevm: Remove dead code in save_snapshot()

[Tuguoyi]

The snapshot in each bs is deleted at the beginning, so there is no need
to find the snapshot again.

Signed-off-by: Tuguoyi 
---
 migration/savevm.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 5f937a2..601b514 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2728,7 +2728,7 @@ int qemu_load_device_state(QEMUFile *f)
 int save_snapshot(const char *name, Error **errp)
 {
 BlockDriverState *bs, *bs1;
-QEMUSnapshotInfo sn1, *sn = , old_sn1, *old_sn = _sn1;
+QEMUSnapshotInfo sn1, *sn = 
 int ret = -1, ret2;
 QEMUFile *f;
 int saved_vm_running;
@@ -2797,13 +2797,7 @@ int save_snapshot(const char *name, Error **errp)
 }
 
 if (name) {
-ret = bdrv_snapshot_find(bs, old_sn, name);
-if (ret >= 0) {
-pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
-pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
-} else {
-pstrcpy(sn->name, sizeof(sn->name), name);
-}
+pstrcpy(sn->name, sizeof(sn->name), name);
 } else {
 /* cast below needed for OpenBSD where tv_sec is still 'long' */
 localtime_r((const time_t *)_sec, );
-- 
2.7.4

Re: [PATCH v2] gitlab-ci.yml: Add openSUSE Leap 15.2 for gitlab CI/CD

[Thomas Huth]

On 24/11/2020 10.45, Cho, Yu-Chen wrote:
> v2:
> Drop some package from dockerfile to make docker image more light.
> 
> v1:
> Add build-system-opensuse jobs and opensuse-leap.docker dockerfile.
> Use openSUSE Leap 15.2 container image in the gitlab-CI.
> 
> Signed-off-by: Cho, Yu-Chen 
> ---
>  .gitlab-ci.d/containers.yml   |  5 ++
>  .gitlab-ci.yml| 30 +++
>  tests/docker/dockerfiles/opensuse-leap.docker | 54 +++
>  3 files changed, 89 insertions(+)
>  create mode 100644 tests/docker/dockerfiles/opensuse-leap.docker

 Hi!

While trying to pick up your patch, I noticed that it is failing now in the
gitlab-CI:

 https://gitlab.com/huth/qemu/-/jobs/896384459

Could you please have a look and send a fixed v3?

 Thanks,
  Thomas

[PATCH 2/2] savevm: Delete snapshots just created in case of error

[Tuguoyi]

bdrv_all_create_snapshot() can fails with some snapshots created,
so it's better to delete those snapshots before returns to the caller

Signed-off-by: Tuguoyi 
---
 migration/savevm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 601b514..4a18c9d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2833,6 +2833,7 @@ int save_snapshot(const char *name, Error **errp)
 if (ret < 0) {
 error_setg(errp, "Error while creating snapshot on '%s'",
bdrv_get_device_or_node_name(bs));
+bdrv_all_delete_snapshot(sn->name, , NULL);
 goto the_end;
 }
 
-- 
2.7.4

[PATCH 0/2] savevm: Delete stale snapshots in save_snapshot()

[Tuguoyi]

These two patches just clear dead code and delete stale snapshots
in case of error in save_snapshot()

Tuguoyi (2):
  savevm: Remove dead code in save_snapshot()
  savevm: Delete snapshots just created in case of error

 migration/savevm.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

-- 
2.7.4

[PATCH] virtiofsd: make the debug log timestamp on stderr more human-readable

[Laszlo Ersek]

The current timestamp format doesn't help me visually notice small jumps
in time ("small" as defined on human scale, such as a few seconds or a few
ten seconds). Replace it with a local time format where such differences
stand out.

Before:

> [13316826770337] [ID: 0004] unique: 62, opcode: RELEASEDIR (29), nodeid: 
> 1, insize: 64, pid: 1
> [13316826778175] [ID: 0004]unique: 62, success, outsize: 16
> [13316826781156] [ID: 0004] virtio_send_msg: elem 0: with 1 in desc of 
> length 16
> [15138279317927] [ID: 0001] virtio_loop: Got VU event
> [15138279504884] [ID: 0001] fv_queue_set_started: qidx=1 started=0
> [15138279519034] [ID: 0003] fv_queue_thread: kill event on queue 1 - 
> quitting
> [15138280876463] [ID: 0001] fv_remove_watch: TODO! fd=9
> [15138280897381] [ID: 0001] virtio_loop: Waiting for VU event
> [15138280946834] [ID: 0001] virtio_loop: Got VU event
> [15138281175421] [ID: 0001] virtio_loop: Waiting for VU event
> [15138281182387] [ID: 0001] virtio_loop: Got VU event
> [15138281189474] [ID: 0001] virtio_loop: Waiting for VU event
> [15138309321936] [ID: 0001] virtio_loop: Unexpected poll revents 11
> [15138309434150] [ID: 0001] virtio_loop: Exit

(Notice how you don't (easily) notice the gap in time after
"virtio_send_msg", and especially the amount of time passed is hard to
estimate.)

After:

> [2020-12-08 06:43:22.58+0100] [ID: 0004] unique: 51, opcode: RELEASEDIR 
> (29), nodeid: 1, insize: 64, pid: 1
> [2020-12-08 06:43:22.58+0100] [ID: 0004]unique: 51, success, outsize: 
> 16
> [2020-12-08 06:43:22.58+0100] [ID: 0004] virtio_send_msg: elem 0: with 1 
> in desc of length 16
> [2020-12-08 06:43:29.34+0100] [ID: 0001] virtio_loop: Got VU event
> [2020-12-08 06:43:29.34+0100] [ID: 0001] fv_queue_set_started: qidx=1 
> started=0
> [2020-12-08 06:43:29.34+0100] [ID: 0003] fv_queue_thread: kill event on 
> queue 1 - quitting
> [2020-12-08 06:43:29.34+0100] [ID: 0001] fv_remove_watch: TODO! fd=9
> [2020-12-08 06:43:29.34+0100] [ID: 0001] virtio_loop: Waiting for VU event
> [2020-12-08 06:43:29.34+0100] [ID: 0001] virtio_loop: Got VU event
> [2020-12-08 06:43:29.34+0100] [ID: 0001] virtio_loop: Waiting for VU event
> [2020-12-08 06:43:29.34+0100] [ID: 0001] virtio_loop: Got VU event
> [2020-12-08 06:43:29.34+0100] [ID: 0001] virtio_loop: Waiting for VU event
> [2020-12-08 06:43:29.37+0100] [ID: 0001] virtio_loop: Unexpected poll 
> revents 11
> [2020-12-08 06:43:29.37+0100] [ID: 0001] virtio_loop: Exit

Cc: "Dr. David Alan Gilbert" 
Cc: Stefan Hajnoczi 
Signed-off-by: Laszlo Ersek 
---
 tools/virtiofsd/passthrough_ll.c | 31 +++
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 48a109d3f682..06543b20dcbb 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3311,18 +3311,38 @@ static void setup_nofile_rlimit(unsigned long 
rlimit_nofile)
 static void log_func(enum fuse_log_level level, const char *fmt, va_list ap)
 {
 g_autofree char *localfmt = NULL;
+struct timespec ts;
+struct tm tm;
+char sec_fmt[sizeof "2020-12-07 18:17:54"];
+char zone_fmt[sizeof "+0100"];
 
 if (current_log_level < level) {
 return;
 }
 
 if (current_log_level == FUSE_LOG_DEBUG) {
-if (!use_syslog) {
-localfmt = g_strdup_printf("[%" PRId64 "] [ID: %08ld] %s",
-   get_clock(), syscall(__NR_gettid), fmt);
-} else {
+if (use_syslog) {
+/* no timestamp needed */
 localfmt = g_strdup_printf("[ID: %08ld] %s", syscall(__NR_gettid),
fmt);
+} else {
+/* try formatting a broken-down timestamp */
+if (clock_gettime(CLOCK_REALTIME, ) != -1 &&
+localtime_r(_sec, ) != NULL &&
+strftime(sec_fmt, sizeof sec_fmt, "%Y-%m-%d %H:%M:%S",
+ ) != 0 &&
+strftime(zone_fmt, sizeof zone_fmt, "%z", ) != 0) {
+localfmt = g_strdup_printf("[%s.%02ld%s] [ID: %08ld] %s",
+   sec_fmt,
+   ts.tv_nsec / (10L * 1000 * 1000),
+   zone_fmt, syscall(__NR_gettid),
+   fmt);
+} else {
+/* fall back to a flat timestamp */
+localfmt = g_strdup_printf("[%" PRId64 "] [ID: %08ld] %s",
+   get_clock(), syscall(__NR_gettid),
+   fmt);
+}
 }
 fmt = localfmt;
 }
@@ -3452,6 +3472,9 @@ int main(int argc, char *argv[])
 struct lo_map_elem *reserve_elem;
 int ret = -1;
 
+/* Initialize time 

Re: [PATCH v3] migration: Don't allow migration if vm is in POSTMIGRATE

[Vladimir Sementsov-Ogievskiy]

08.12.2020 04:46, Tuguoyi wrote:

The following steps will cause qemu assertion failure:
- pause vm by executing 'virsh suspend'
- create external snapshot of memory and disk using 'virsh snapshot-create-as'
- doing the above operation again will cause qemu crash

The backtrace looks like:
#0  0x7fbf958c5c37 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x7fbf958c9028 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x7fbf958bebf6 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#3  0x7fbf958beca2 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6
#4  0x55ca8decd39d in bdrv_inactivate_recurse (bs=0x55ca90c80400) at 
/build/qemu-5.0/block.c:5724
#5  0x55ca8dece967 in bdrv_inactivate_all () at 
/build//qemu-5.0/block.c:5792
#6  0x55ca8de5539d in qemu_savevm_state_complete_precopy_non_iterable 
(inactivate_disks=true, in_postcopy=false, f=0x55ca907044b0)
 at /build/qemu-5.0/migration/savevm.c:1401
#7  qemu_savevm_state_complete_precopy (f=0x55ca907044b0, 
iterable_only=iterable_only@entry=false, 
inactivate_disks=inactivate_disks@entry=true)
 at /build/qemu-5.0/migration/savevm.c:1453
#8  0x55ca8de4f581 in migration_completion (s=0x55ca8f64d9f0) at 
/build/qemu-5.0/migration/migration.c:2941
#9  migration_iteration_run (s=0x55ca8f64d9f0) at 
/build/qemu-5.0/migration/migration.c:3295
#10 migration_thread (opaque=opaque@entry=0x55ca8f64d9f0) at 
/build/qemu-5.0/migration/migration.c:3459
#11 0x55ca8dfc6716 in qemu_thread_start (args=) at 
/build/qemu-5.0/util/qemu-thread-posix.c:519
#12 0x7fbf95c5f184 in start_thread () from 
/lib/x86_64-linux-gnu/libpthread.so.0
#13 0x7fbf9598cbed in clone () from /lib/x86_64-linux-gnu/libc.so.6

When the first migration completes, bs->open_flags will set BDRV_O_INACTIVE
flag by bdrv_inactivate_all(), and during the second migration the
bdrv_inactivate_recurse assert that the bs->open_flags is already
BDRV_O_INACTIVE enabled which cause crash.

As Vladimir suggested, this patch makes migrate_prepare check the state of vm 
and
return error if it is in RUN_STATE_POSTMIGRATE state.

Signed-off-by: Tuguoyi


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[Laszlo Ersek]

Hi Vivek,

On 12/07/20 19:30, Vivek Goyal wrote:
> Laszlo is writing a virtiofs client for OVMF and noticed that if he
> sends fuse FLUSH command for directory object, virtiofsd crashes.
> virtiofsd does not expect a FLUSH arriving for a directory object.
> 
> This patch series has one of the patches which fixes that. It also
> has couple of posix lock fixes as a result of lo_flush() related debugging.
> 
> Vivek Goyal (3):
>   virtiofsd: Set up posix_lock hash table for root inode
>   virtiofsd: Disable posix_lock hash table if remote locks are not
> enabled
>   virtiofsd: Check file type in lo_flush()
> 
>  tools/virtiofsd/passthrough_ll.c | 54 +++-
>  1 file changed, 39 insertions(+), 15 deletions(-)
> 

I put back the (wrong) FLUSH for the root dir into my code temporarily, to 
reproduce the crash (it does, with v5.2.0-rc4).

Then I applied your series [*], and retested.

[*] I'm unsure about the email you sent in response to 1/3, namely 
; I 
ignored that when applying the patches.

Indeed now I get a graceful -EBADF:

[13316825985314] [ID: 0004] unique: 60, opcode: FLUSH (25), nodeid: 1, 
insize: 64, pid: 1
[13316825993517] [ID: 0004]unique: 60, error: -9 (Bad file descriptor), 
outsize: 16

For whichever patch in the series my testing is relevant:

Tested-by: Laszlo Ersek 

(I'm having some difficulty figuring out which patch(es) should carry my T-b.

- I think I didn't really test patch#2 with the above, so that one should 
likely not get the T-b

- I think patch#3 is what I really tested.

- But, if that's the case, doesn't patch#3 make the fix in patch#1 untestable, 
in my scenario? I believe the code is no longer reached in lo_flush(), due to 
patch#3, where the change from patch#1 would matter. Patch#1 seems correct, and 
the last paragraph of its commit message relevant, but I think my testing 
currently only covered patch#3.

I'll let you decide where to apply my T-b.)

Thanks!
Laszlo

Re: [PATCH for-6.0] spapr: Allow memory unplug to always succeed

[David Gibson]

On Mon, Dec 07, 2020 at 02:37:04PM +0100, Greg Kurz wrote:
> It is currently impossible to hot-unplug a memory device between
> machine reset and CAS.
> 
> (qemu) device_del dimm1
> Error: Memory hot unplug not supported for this guest
> 
> This limitation was introduced in order to provide an explicit
> error path for older guests that didn't support hot-plug event
> sources (and thus memory hot-unplug).
> 
> The linux kernel has been supporting these since 4.11. All recent
> enough guests are thus capable of handling the removal of a memory
> device at all time, including during early boot.
> 
> Lift the limitation for the latest machine type. This means that
> trying to unplug memory from a guest that doesn't support it will
> likely just do nothing and the memory will only get removed at
> next reboot. Such older guests can still get the existing behavior
> by using an older machine type.
> 
> Signed-off-by: Greg Kurz 

Looks like this conflicts with something I've added to for-6.0
recently.  Can you rebase and resend, please.

> ---
> Based-on: 20201109173928.1001764-1-coh...@redhat.com
> ---
>  include/hw/ppc/spapr.h | 1 +
>  hw/ppc/spapr.c | 6 +-
>  hw/ppc/spapr_events.c  | 3 ++-
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index b7ced9faebf5..7aa630350326 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -139,6 +139,7 @@ struct SpaprMachineClass {
>  hwaddr rma_limit;  /* clamp the RMA to this size */
>  bool pre_5_1_assoc_refpoints;
>  bool pre_5_2_numa_associativity;
> +bool pre_6_0_memory_unplug;
>  
>  bool (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
>uint64_t *buid, hwaddr *pio, 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7e954bc84bed..f0b26b2af30d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4044,7 +4044,8 @@ static void 
> spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>  SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>  
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> +if (!smc->pre_6_0_memory_unplug ||
> +spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
>  spapr_memory_unplug_request(hotplug_dev, dev, errp);
>  } else {
>  /* NOTE: this means there is a window after guest reset, prior to
> @@ -4530,8 +4531,11 @@ DEFINE_SPAPR_MACHINE(6_0, "6.0", true);
>   */
>  static void spapr_machine_5_2_class_options(MachineClass *mc)
>  {
> +SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
>  spapr_machine_6_0_class_options(mc);
>  compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
> +smc->pre_6_0_memory_unplug = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(5_2, "5.2", false);
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 1add53547ec3..c30123177b16 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -659,7 +659,8 @@ static void spapr_hotplug_req_event(uint8_t hp_id, 
> uint8_t hp_action,
>  /* we should not be using count_indexed value unless the guest
>   * supports dedicated hotplug event source
>   */
> -g_assert(spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT));
> +g_assert(!SPAPR_MACHINE_GET_CLASS(spapr)->pre_6_0_memory_unplug ||
> + spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT));
>  hp->drc_id.count_indexed.count =
>  cpu_to_be32(drc_id->count_indexed.count);
>  hp->drc_id.count_indexed.index =

-- 
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: [for-6.0 v5 00/13] Generalize memory encryption models

[David Gibson]

On Fri, Dec 04, 2020 at 02:12:29PM +0100, Cornelia Huck wrote:
> On Fri, 4 Dec 2020 13:07:27 +
> "Dr. David Alan Gilbert"  wrote:
> 
> > * Cornelia Huck (coh...@redhat.com) wrote:
> > > On Fri, 4 Dec 2020 09:06:50 +0100
> > > Christian Borntraeger  wrote:
> > >   
> > > > On 04.12.20 06:44, David Gibson wrote:  
> > > > > A number of hardware platforms are implementing mechanisms whereby the
> > > > > hypervisor does not have unfettered access to guest memory, in order
> > > > > to mitigate the security impact of a compromised hypervisor.
> > > > > 
> > > > > AMD's SEV implements this with in-cpu memory encryption, and Intel has
> > > > > its own memory encryption mechanism.  POWER has an upcoming mechanism
> > > > > to accomplish this in a different way, using a new memory protection
> > > > > level plus a small trusted ultravisor.  s390 also has a protected
> > > > > execution environment.
> > > > > 
> > > > > The current code (committed or draft) for these features has each
> > > > > platform's version configured entirely differently.  That doesn't seem
> > > > > ideal for users, or particularly for management layers.
> > > > > 
> > > > > AMD SEV introduces a notionally generic machine option
> > > > > "machine-encryption", but it doesn't actually cover any cases other
> > > > > than SEV.
> > > > > 
> > > > > This series is a proposal to at least partially unify configuration
> > > > > for these mechanisms, by renaming and generalizing AMD's
> > > > > "memory-encryption" property.  It is replaced by a
> > > > > "securable-guest-memory" property pointing to a platform specific
> > > > 
> > > > Can we do "securable-guest" ?
> > > > s390x also protects registers and integrity. memory is only one piece
> > > > of the puzzle and what we protect might differ from platform to 
> > > > platform.
> > > >   
> > > 
> > > I agree. Even technologies that currently only do memory encryption may
> > > be enhanced with more protections later.  
> > 
> > There's already SEV-ES patches onlist for this on the SEV side.
> > 
> > 
> > 
> > Perhaps 'confidential guest' is actually what we need, since the
> > marketing folks seem to have started labelling this whole idea
> > 'confidential computing'.

That's not a bad idea, much as I usually hate marketing terms.  But it
does seem to be becoming a general term for this style of thing, and
it doesn't overlap too badly with other terms ("secure" and
"protected" are also used for hypervisor-from-guest and
guest-from-guest protection).

> It's more like a 'possibly confidential guest', though.

Hmm.  What about "Confidential Guest Facility" or "Confidential Guest
Mechanism"?  The implication being that the facility is there, whether
or not the guest actually uses it.

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


signature.asc
Description: PGP signature

Re: [for-6.0 v5 00/13] Generalize memory encryption models

[David Gibson]

On Fri, Dec 04, 2020 at 02:02:05PM +0100, Cornelia Huck wrote:
> On Fri, 4 Dec 2020 09:06:50 +0100
> Christian Borntraeger  wrote:
> 
> > On 04.12.20 06:44, David Gibson wrote:
> > > A number of hardware platforms are implementing mechanisms whereby the
> > > hypervisor does not have unfettered access to guest memory, in order
> > > to mitigate the security impact of a compromised hypervisor.
> > > 
> > > AMD's SEV implements this with in-cpu memory encryption, and Intel has
> > > its own memory encryption mechanism.  POWER has an upcoming mechanism
> > > to accomplish this in a different way, using a new memory protection
> > > level plus a small trusted ultravisor.  s390 also has a protected
> > > execution environment.
> > > 
> > > The current code (committed or draft) for these features has each
> > > platform's version configured entirely differently.  That doesn't seem
> > > ideal for users, or particularly for management layers.
> > > 
> > > AMD SEV introduces a notionally generic machine option
> > > "machine-encryption", but it doesn't actually cover any cases other
> > > than SEV.
> > > 
> > > This series is a proposal to at least partially unify configuration
> > > for these mechanisms, by renaming and generalizing AMD's
> > > "memory-encryption" property.  It is replaced by a
> > > "securable-guest-memory" property pointing to a platform specific  
> > 
> > Can we do "securable-guest" ?
> > s390x also protects registers and integrity. memory is only one piece
> > of the puzzle and what we protect might differ from platform to 
> > platform.
> 
> I agree. Even technologies that currently only do memory encryption may
> be enhanced with more protections later.

That's a good point.  I've focused on the memory aspect because that's
what's most immediately relevant to qemu - the fact that we can't
directly access guest memory is something we have to deal with, and
has some uniformity regardless of the details of the protection scheme.

-- 
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 10/15] vl: make qemu_get_machine_opts static

[Daniel Henrique Barboza]

On 12/7/20 1:07 PM, Igor Mammedov wrote:

On Wed,  2 Dec 2020 03:18:49 -0500
Paolo Bonzini  wrote:


Machine options can be retrieved as properties of the machine object.
Encourage that by removing the "easy" accessor to machine options.

Signed-off-by: Paolo Bonzini 
---
  accel/kvm/kvm-all.c | 11 ---
  hw/arm/boot.c   |  2 +-
  hw/microblaze/boot.c|  9 -
  hw/nios2/boot.c |  9 -
  hw/ppc/e500.c   |  5 ++---
  hw/ppc/spapr_nvdimm.c   |  4 ++--
  hw/ppc/virtex_ml507.c   |  2 +-
  hw/riscv/sifive_u.c |  6 ++
  hw/riscv/virt.c |  6 ++
  hw/xtensa/xtfpga.c  |  9 -
  include/sysemu/sysemu.h |  2 --
  softmmu/device_tree.c   |  2 +-
  softmmu/vl.c|  2 +-
  13 files changed, 28 insertions(+), 41 deletions(-)


[...]

diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5e..84715a4d78 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -38,7 +38,6 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, 
NVDIMMDevice *nvdimm,
  {
  const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
  const MachineState *ms = MACHINE(hotplug_dev);
-const char *nvdimm_opt = qemu_opt_get(qemu_get_machine_opts(), "nvdimm");
  g_autofree char *uuidstr = NULL;
  QemuUUID uuid;
  int ret;
@@ -57,10 +56,11 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, 
NVDIMMDevice *nvdimm,
   * ensure that, if the user sets nvdimm=off, we error out
   * regardless of being 5.1 or newer.
   */
-if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
+if (!ms->nvdimms_state->is_enabled && ms->nvdimms_state->has_is_enabled) {
  error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
  return false;
  }
+ms->nvdimms_state->is_enabled = true;


it looks like nvdimm is always enabled since 5.0 and there is no way to disable 
it



I'm not sure I'm following this statement. Testing here with all patches
up to this one applied, in a x86 machine:


$ sudo ./x86_64-softmmu/qemu-system-x86_64 -M pc,nvdimm=off -object 
memory-backend-file,id=mem0,size=1G,mem-path=/tmp/aa -device 
nvdimm,id=dimm0,memdev=mem0
qemu-system-x86_64: -device nvdimm,id=dimm0,memdev=mem0: nvdimm is not enabled: 
missing 'nvdimm' in '-M'
$
$ sudo ./x86_64-softmmu/qemu-system-x86_64 -M pc -object 
memory-backend-file,id=mem0,size=1G,mem-path=/tmp/aa -device 
nvdimm,id=dimm0,memdev=mem0
qemu-system-x86_64: -device nvdimm,id=dimm0,memdev=mem0: nvdimm is not enabled: 
missing 'nvdimm' in '-M'
$

This is the x86 NVDIMM behavior I considered when patching pseries NVDIMM 
support.
As Paolo mentioned in patch 09, pseries has different default values. We screwed
it up when pushing the first version of pseries NVDIMM support and we ended up
enabling it by default, as opposed to disabling it by default like x86. One 
release
later people complained that we weren't honoring 'nvdimm=off' to disable NVDIMM
support. The path of less pain that I chose was to at the very least disable
the support when "nvdimm=off" was specified by the user.






how about dropping 9/15 and just error out is it's not enabled, something like:

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b7e0894019..d63f095bff 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2668,6 +2668,7 @@ static void spapr_machine_init(MachineState *machine)
  char *filename;
  Error *resize_hpt_err = NULL;
+if (mc->nvdimm_supported) { // by default it is always enabled
+ms->nvdimms_state->is_enabled = true;
+}
  msi_nonbroken = true;
  
  QLIST_INIT(>phbs);

diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5e..b11c85dbaa 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -57,7 +57,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, 
NVDIMMDevice *nvdimm,
   * ensure that, if the user sets nvdimm=off, we error out
   * regardless of being 5.1 or newer.
   */
-if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
+if (!ms->nvdimms_state->is_enabled) {
  error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
  return false;
  }

if I didn't miss something, that way we do not abuse nvdimm_opt and still
have nvdimm enabled by default and error out if it turns to disabled for 
whatever reason.



Just tried this out. This doesn't disable the NVDIMM support when passing 
'nvdimm=off'
machine option.


As far pseries NVDIMM support goes, we'll need patch 09 and to consider 
nvdimm_opt
to keep the same behavior we already have today, like Paolo is already doing in
this patch.



Thanks,


DHB





  if (object_property_get_int(OBJECT(nvdimm), NVDIMM_LABEL_SIZE_PROP,
  _abort) == 0) {

[...]

  

Re: [PATCH qemu v11] spapr: Implement Open Firmware client interface

[Alexey Kardashevskiy]

On 07/12/2020 22:48, BALATON Zoltan wrote:


diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 2e89e36cfbdc..048bf49592aa 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -175,6 +175,13 @@ struct SpaprMachineState {
    long kernel_size;
    bool kernel_le;
    uint64_t kernel_addr;
+    bool vof; /* Virtual Open Firmware */
+    uint32_t rtas_base;
+    GArray *claimed; /* array of SpaprOfClaimed */
+    uint64_t claimed_base;
+    GHashTable *of_instances; /* ihandle -> SpaprOfInstance */
+    uint32_t of_instance_last;
+    char *bootargs;


Are these really state for vof so is it better to place them in a 
separate of_state struct instead of adding to the machine state? I'm not 
interested in spapr but interested in using vof as a replacement 
firmware for other machines so clear separation of what is spapr 
specific and what is vof specific would help me (and maybe also other 
reviewers to tell how much impact this really has on spapr which seems 
to be a concern of Greg).


This is a very good point, I'll separate VOF from the rest, may be even 
at QOM level. I was also thinking of adding a pseries-vof machine type 
but this is probably an overkill.


Out of curiosity - how are you going to use this VOF anyway, for what 
machine type?



ps: I'll keep the braces ;)


--
Alexey

Re: [PATCH 10/15] vl: make qemu_get_machine_opts static

[Daniel Henrique Barboza]

On 12/2/20 5:18 AM, Paolo Bonzini wrote:

Machine options can be retrieved as properties of the machine object.
Encourage that by removing the "easy" accessor to machine options.

Signed-off-by: Paolo Bonzini 
---
  accel/kvm/kvm-all.c | 11 ---
  hw/arm/boot.c   |  2 +-
  hw/microblaze/boot.c|  9 -
  hw/nios2/boot.c |  9 -
  hw/ppc/e500.c   |  5 ++---
  hw/ppc/spapr_nvdimm.c   |  4 ++--
  hw/ppc/virtex_ml507.c   |  2 +-
  hw/riscv/sifive_u.c |  6 ++
  hw/riscv/virt.c |  6 ++
  hw/xtensa/xtfpga.c  |  9 -
  include/sysemu/sysemu.h |  2 --
  softmmu/device_tree.c   |  2 +-
  softmmu/vl.c|  2 +-
  13 files changed, 28 insertions(+), 41 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index baaa54249d..666b9ab96c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2013,7 +2013,6 @@ static int kvm_init(MachineState *ms)
  const KVMCapabilityInfo *missing_cap;
  int ret;
  int type = 0;
-const char *kvm_type;
  uint64_t dirty_log_manual_caps;
  
  s = KVM_STATE(ms->accelerator);

@@ -2069,13 +2068,11 @@ static int kvm_init(MachineState *ms)
  }
  s->as = g_new0(struct KVMAs, s->nr_as);
  
-kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");

-if (mc->kvm_type) {
+if (object_property_find(OBJECT(current_machine), "kvm-type")) {
+g_autofree char *kvm_type = 
object_property_get_str(OBJECT(current_machine),
+"kvm-type",
+_abort);
  type = mc->kvm_type(ms, kvm_type);




I'm afraid that this code has unintended consequences for pseries. When 
starting the VM
with '--enable-kvm' in a Power host I'm getting an error:


$ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine 
pseries --enable-kvm
qemu-system-ppc64: Unknown kvm-type specified ''


The reason is that spapr_kvm_type() expects kvm-type to be either NULL (i.e. no 
option
set, in the previous semantic), "HV" or "PR". This was the case for the previous
qemu_opt_get(), but with object_property_get_str() the absence of the option is
returning kvm_type = ''.

This means that, as far as pseries goes, inserting "kvm-type=" (blank string)
has the same effect:


$ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine 
pseries,accel=kvm,kvm-type=
qemu-system-ppc64: Unknown kvm-type specified ''


I investigated object_property_get_str() inner workings a bit and I wasn't able
to find a way for it to return NULL if kvm_type isn't specified.


I see three possible solutions for it:

1) interpret kvm_type='' as if kvm_type=NULL inside spapr_kvm_type():

static int spapr_kvm_type(MachineState *machine, const char *vm_type)
{
if (!vm_type || strcmp(vm_type, "")) {
return 0;
}
(...)


This is kind of ugly because we're validating a "kvm_type=" scenario with it, 
but
it works.


2) find a way to make object_property_get_str() to return kvm_type = NULL if the
'kvm_type' option is absent  and keep spapr code untouched. I don't have the
knowledge to assess how hard this would be.


3) I can change the pseries logic to add an explicit default value for 
kvm_type=NULL
or kvm_type='' (i.e. no user input is given). We're already doing that in a 
sense, but
it's not exposed to the user. I would call it 'auto' and expose it to the user
as default value if no kvm_type is explictly set. This would enhance user 
experience
a bit and avoid having to deal with a scenario where kvm_type=(blank) is
a valid input.


David, if you think (3) is tolerable let me know and I can send a patch. IMO
this change adds a bit of value regardless of Paolo's change.


Thanks,



DHB




-} else if (kvm_type) {
-ret = -EINVAL;
-fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);
-goto err;
  }
  
  do {

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 4d9d47ba1c..e56c42ac22 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1299,7 +1299,7 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, 
struct arm_boot_info *info)
  info->kernel_filename = ms->kernel_filename;
  info->kernel_cmdline = ms->kernel_cmdline;
  info->initrd_filename = ms->initrd_filename;
-info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
+info->dtb_filename = ms->dtb;
  info->dtb_limit = 0;
  
  /* Load the kernel.  */

diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 6715ba2ff9..caaba1aa4c 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -34,6 +34,7 @@
  #include "sysemu/device_tree.h"
  #include "sysemu/reset.h"
  #include "sysemu/sysemu.h"
+#include "hw/boards.h"
  #include "hw/loader.h"
  #include "elf.h"
  #include "qemu/cutils.h"
@@ -116,16 +117,14 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr 
ddr_base,
  

Re: [PATCH] linux-user: add option to chroot before emulation

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20201208001727.17433-1-mcr...@linux.microsoft.com/



Hi,

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

Type: series
Message-id: 20201208001727.17433-1-mcr...@linux.microsoft.com
Subject: [PATCH] linux-user: add option to chroot before emulation

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20201208001727.17433-1-mcr...@linux.microsoft.com 
-> patchew/20201208001727.17433-1-mcr...@linux.microsoft.com
Switched to a new branch 'test'
939b01d linux-user: add option to chroot before emulation

=== OUTPUT BEGIN ===
ERROR: do not use assignment in if condition
#62: FILE: linux-user/main.c:707:
+if ((fp = fopen("/proc/sys/vm/mmap_min_addr", "r")) != NULL) {

ERROR: space prohibited between function name and open parenthesis '('
#88: FILE: linux-user/main.c:733:
+target_argv = calloc(target_argc + 1, sizeof (char *));

total: 2 errors, 0 warnings, 158 lines checked

Commit 939b01d16330 (linux-user: add option to chroot before emulation) has 
style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201208001727.17433-1-mcr...@linux.microsoft.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH] linux-user: add option to chroot before emulation

[Matteo Croce]

From: Matteo Croce 

Add a '-c' option which does a chroot() just before starting the
emulation. This is useful when the static QEMU user binary can't
be copied into the target root filesystem, e.g. if it's readonly.

Move some code which accesses /proc/sys/vm/mmap_min_addr before
the chroot, otherwise it would fail.

Signed-off-by: Matteo Croce 
---
 linux-user/main.c | 128 +++---
 1 file changed, 75 insertions(+), 53 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 24d1eb73ad..4788e4b5bc 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -60,6 +60,7 @@ static const char *seed_optarg;
 unsigned long mmap_min_addr;
 unsigned long guest_base;
 bool have_guest_base;
+static const char *qemu_chroot;
 
 /*
  * Used to implement backwards-compatibility for the `-strace`, and
@@ -304,6 +305,11 @@ static void handle_arg_pagesize(const char *arg)
 }
 }
 
+static void handle_arg_chroot(const char *arg)
+{
+qemu_chroot = arg;
+}
+
 static void handle_arg_seed(const char *arg)
 {
 seed_optarg = arg;
@@ -450,6 +456,8 @@ static const struct qemu_argument arg_table[] = {
  "logfile", "write logs to 'logfile' (default stderr)"},
 {"p",  "QEMU_PAGESIZE",true,  handle_arg_pagesize,
  "pagesize",   "set the host page size to 'pagesize'"},
+{"c",  "QEMU_CHROOT",  true,  handle_arg_chroot,
+ "chroot", "chroot to 'chroot' before starting emulation"},
 {"singlestep", "QEMU_SINGLESTEP",  false, handle_arg_singlestep,
  "",   "run in singlestep mode"},
 {"strace", "QEMU_STRACE",  false, handle_arg_strace,
@@ -688,6 +696,73 @@ int main(int argc, char **argv, char **envp)
 
 init_qemu_uname_release();
 
+/*
+ * Read in mmap_min_addr kernel parameter.  This value is used
+ * When loading the ELF image to determine whether guest_base
+ * is needed.  It is also used in mmap_find_vma.
+ */
+{
+FILE *fp;
+
+if ((fp = fopen("/proc/sys/vm/mmap_min_addr", "r")) != NULL) {
+unsigned long tmp;
+if (fscanf(fp, "%lu", ) == 1 && tmp != 0) {
+mmap_min_addr = tmp;
+qemu_log_mask(CPU_LOG_PAGE, "host mmap_min_addr=0x%lx\n",
+  mmap_min_addr);
+}
+fclose(fp);
+}
+}
+
+/*
+ * We prefer to not make NULL pointers accessible to QEMU.
+ * If we're in a chroot with no /proc, fall back to 1 page.
+ */
+if (mmap_min_addr == 0) {
+mmap_min_addr = qemu_host_page_size;
+qemu_log_mask(CPU_LOG_PAGE,
+  "host mmap_min_addr=0x%lx (fallback)\n",
+  mmap_min_addr);
+}
+
+/*
+ * Prepare copy of argv vector for target.
+ */
+target_argc = argc - optind;
+target_argv = calloc(target_argc + 1, sizeof (char *));
+if (target_argv == NULL) {
+(void) fprintf(stderr, "Unable to allocate memory for target_argv\n");
+exit(EXIT_FAILURE);
+}
+
+/*
+ * If argv0 is specified (using '-0' switch) we replace
+ * argv[0] pointer with the given one.
+ */
+i = 0;
+if (argv0 != NULL) {
+target_argv[i++] = strdup(argv0);
+}
+for (; i < target_argc; i++) {
+target_argv[i] = strdup(argv[optind + i]);
+}
+target_argv[target_argc] = NULL;
+
+/*
+ * Change root if requested wuth '-c'
+ */
+if (qemu_chroot) {
+if (chroot(qemu_chroot) < 0) {
+error_report("chroot failed");
+exit(1);
+}
+if (chdir("/")) {
+error_report("not able to chdir to /: %s", strerror(errno));
+exit(1);
+}
+}
+
 execfd = qemu_getauxval(AT_EXECFD);
 if (execfd == 0) {
 execfd = open(exec_path, O_RDONLY);
@@ -746,59 +821,6 @@ int main(int argc, char **argv, char **envp)
 target_environ = envlist_to_environ(envlist, NULL);
 envlist_free(envlist);
 
-/*
- * Read in mmap_min_addr kernel parameter.  This value is used
- * When loading the ELF image to determine whether guest_base
- * is needed.  It is also used in mmap_find_vma.
- */
-{
-FILE *fp;
-
-if ((fp = fopen("/proc/sys/vm/mmap_min_addr", "r")) != NULL) {
-unsigned long tmp;
-if (fscanf(fp, "%lu", ) == 1 && tmp != 0) {
-mmap_min_addr = tmp;
-qemu_log_mask(CPU_LOG_PAGE, "host mmap_min_addr=0x%lx\n",
-  mmap_min_addr);
-}
-fclose(fp);
-}
-}
-
-/*
- * We prefer to not make NULL pointers accessible to QEMU.
- * If we're in a chroot with no /proc, fall back to 1 page.
- */
-if (mmap_min_addr == 0) {
-mmap_min_addr = qemu_host_page_size;
-qemu_log_mask(CPU_LOG_PAGE,
-  "host mmap_min_addr=0x%lx (fallback)\n",
-  mmap_min_addr);
- 

Re: [for-6.0 v5 12/13] securable guest memory: Alter virtio default properties for protected guests

[David Gibson]

On Fri, Dec 04, 2020 at 03:43:10PM +0100, Halil Pasic wrote:
> On Fri, 4 Dec 2020 09:29:59 +0100
> Christian Borntraeger  wrote:
> 
> > On 04.12.20 09:17, Cornelia Huck wrote:
> > > On Fri, 4 Dec 2020 09:10:36 +0100
> > > Christian Borntraeger  wrote:
> > > 
> > >> On 04.12.20 06:44, David Gibson wrote:
> > >>> The default behaviour for virtio devices is not to use the platforms 
> > >>> normal
> > >>> DMA paths, but instead to use the fact that it's running in a hypervisor
> > >>> to directly access guest memory.  That doesn't work if the guest's 
> > >>> memory
> > >>> is protected from hypervisor access, such as with AMD's SEV or POWER's 
> > >>> PEF.
> > >>>
> > >>> So, if a securable guest memory mechanism is enabled, then apply the
> > >>> iommu_platform=on option so it will go through normal DMA mechanisms.
> > >>> Those will presumably have some way of marking memory as shared with
> > >>> the hypervisor or hardware so that DMA will work.
> > >>>
> > >>> Signed-off-by: David Gibson 
> > >>> Reviewed-by: Dr. David Alan Gilbert 
> > >>> ---
> > >>>  hw/core/machine.c | 13 +
> > >>>  1 file changed, 13 insertions(+)
> > >>>
> > >>> diff --git a/hw/core/machine.c b/hw/core/machine.c
> > >>> index a67a27d03c..d16273d75d 100644
> > >>> --- a/hw/core/machine.c
> > >>> +++ b/hw/core/machine.c
> > >>> @@ -28,6 +28,8 @@
> > >>>  #include "hw/mem/nvdimm.h"
> > >>>  #include "migration/vmstate.h"
> > >>>  #include "exec/securable-guest-memory.h"
> > >>> +#include "hw/virtio/virtio.h"
> > >>> +#include "hw/virtio/virtio-pci.h"
> > >>>  
> > >>>  GlobalProperty hw_compat_5_1[] = {
> > >>>  { "vhost-scsi", "num_queues", "1"},
> > >>> @@ -1169,6 +1171,17 @@ void machine_run_board_init(MachineState 
> > >>> *machine)
> > >>>   * areas.
> > >>>   */
> > >>>  machine_set_mem_merge(OBJECT(machine), false, _abort);
> > >>> +
> > >>> +/*
> > >>> + * Virtio devices can't count on directly accessing guest
> > >>> + * memory, so they need iommu_platform=on to use normal DMA
> > >>> + * mechanisms.  That requires also disabling legacy virtio
> > >>> + * support for those virtio pci devices which allow it.
> > >>> + */
> > >>> +object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy",
> > >>> +   "on", true);
> > >>> +object_register_sugar_prop(TYPE_VIRTIO_DEVICE, 
> > >>> "iommu_platform",
> > >>> +   "on", false);  
> > >>
> > >> I have not followed all the history (sorry). Should we also set 
> > >> iommu_platform
> > >> for virtio-ccw? Halil?
> > >>
> > > 
> > > That line should add iommu_platform for all virtio devices, shouldn't
> > > it?
> > 
> > Yes, sorry. Was misreading that with the line above. 
> > 
> 
> I believe this is the best we can get. In a sense it is still a
> pessimization,

I'm not really clear on what you're getting at here.

> but it is a big usability improvement compared to having
> to set iommu_platform manually. 
> 
> Regards,
> Halil
> 

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


signature.asc
Description: PGP signature

[PATCH v3] migration: Don't allow migration if vm is in POSTMIGRATE

[Tuguoyi]

The following steps will cause qemu assertion failure:
- pause vm by executing 'virsh suspend'
- create external snapshot of memory and disk using 'virsh snapshot-create-as'
- doing the above operation again will cause qemu crash

The backtrace looks like:
#0  0x7fbf958c5c37 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x7fbf958c9028 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x7fbf958bebf6 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#3  0x7fbf958beca2 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6
#4  0x55ca8decd39d in bdrv_inactivate_recurse (bs=0x55ca90c80400) at 
/build/qemu-5.0/block.c:5724
#5  0x55ca8dece967 in bdrv_inactivate_all () at 
/build//qemu-5.0/block.c:5792
#6  0x55ca8de5539d in qemu_savevm_state_complete_precopy_non_iterable 
(inactivate_disks=true, in_postcopy=false, f=0x55ca907044b0)
at /build/qemu-5.0/migration/savevm.c:1401
#7  qemu_savevm_state_complete_precopy (f=0x55ca907044b0, 
iterable_only=iterable_only@entry=false, 
inactivate_disks=inactivate_disks@entry=true)
at /build/qemu-5.0/migration/savevm.c:1453
#8  0x55ca8de4f581 in migration_completion (s=0x55ca8f64d9f0) at 
/build/qemu-5.0/migration/migration.c:2941
#9  migration_iteration_run (s=0x55ca8f64d9f0) at 
/build/qemu-5.0/migration/migration.c:3295
#10 migration_thread (opaque=opaque@entry=0x55ca8f64d9f0) at 
/build/qemu-5.0/migration/migration.c:3459
#11 0x55ca8dfc6716 in qemu_thread_start (args=) at 
/build/qemu-5.0/util/qemu-thread-posix.c:519
#12 0x7fbf95c5f184 in start_thread () from 
/lib/x86_64-linux-gnu/libpthread.so.0
#13 0x7fbf9598cbed in clone () from /lib/x86_64-linux-gnu/libc.so.6

When the first migration completes, bs->open_flags will set BDRV_O_INACTIVE
flag by bdrv_inactivate_all(), and during the second migration the
bdrv_inactivate_recurse assert that the bs->open_flags is already
BDRV_O_INACTIVE enabled which cause crash.

As Vladimir suggested, this patch makes migrate_prepare check the state of vm 
and
return error if it is in RUN_STATE_POSTMIGRATE state.

Signed-off-by: Tuguoyi 
---
 migration/migration.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 87a9b59..5e33962 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2115,6 +2115,12 @@ static bool migrate_prepare(MigrationState *s, bool blk, 
bool blk_inc,
 return false;
 }
 
+if (runstate_check(RUN_STATE_POSTMIGRATE)) {
+error_setg(errp, "Can't migrate the vm that was paused due to "
+   "previous migration");
+return false;
+}
+
 if (migration_is_blocked(errp)) {
 return false;
 }
-- 
2.7.4

[Patch v2]: https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01318.html
[Patch v1]: https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg05950.html

RE: [PATCH v2] migration: Don't allow migration if vm is in POSTMIGRATE state

[Tuguoyi]

On December 07, 2020 6:06 PM Vladimir Sementsov-Ogievskiy wrote:
> 07.12.2020 10:44, Tuguoyi wrote:
> > The following steps will cause qemu assertion failure:
> > - pause vm by executing 'virsh suspend'
> > - create external snapshot of memory and disk using 'virsh
> snapshot-create-as'
> > - doing the above operation again will cause qemu crash
> >
> > The backtrace looks like:
> > #0  0x7fbf958c5c37 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> > #1  0x7fbf958c9028 in abort () from /lib/x86_64-linux-gnu/libc.so.6
> > #2  0x7fbf958bebf6 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
> > #3  0x7fbf958beca2 in __assert_fail () from
> /lib/x86_64-linux-gnu/libc.so.6
> > #4  0x55ca8decd39d in bdrv_inactivate_recurse (bs=0x55ca90c80400)
> at /build/qemu-5.0/block.c:5724
> > #5  0x55ca8dece967 in bdrv_inactivate_all () at
> /build//qemu-5.0/block.c:5792
> > #6  0x55ca8de5539d in
> qemu_savevm_state_complete_precopy_non_iterable (inactivate_disks=true,
> in_postcopy=false, f=0x55ca907044b0)
> >  at /build/qemu-5.0/migration/savevm.c:1401
> > #7  qemu_savevm_state_complete_precopy (f=0x55ca907044b0,
> iterable_only=iterable_only@entry=false,
> inactivate_disks=inactivate_disks@entry=true)
> >  at /build/qemu-5.0/migration/savevm.c:1453
> > #8  0x55ca8de4f581 in migration_completion (s=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:2941
> > #9  migration_iteration_run (s=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:3295
> > #10 migration_thread (opaque=opaque@entry=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:3459
> > #11 0x55ca8dfc6716 in qemu_thread_start (args=) at
> /build/qemu-5.0/util/qemu-thread-posix.c:519
> > #12 0x7fbf95c5f184 in start_thread () from
> /lib/x86_64-linux-gnu/libpthread.so.0
> > #13 0x7fbf9598cbed in clone () from /lib/x86_64-linux-gnu/libc.so.6
> >
> > When the first migration completes, bs->open_flags will set
> BDRV_O_INACTIVE
> > flag by bdrv_inactivate_all(), and during the second migration the
> > bdrv_inactivate_recurse assert that the bs->open_flags is already
> > BDRV_O_INACTIVE enabled which cause crash.
> >
> > As Vladimir suggested, this patch just make migration job error-out with a
> > message in  migrate_fd_connect() if the vm is in
> RUN_STATE_POSTMIGRATE state.
> >
> > Signed-off-by: Tuguoyi 
> > ---
> >   migration/migration.c | 7 +++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 87a9b59..4091678 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -3622,6 +3622,13 @@ void migrate_fd_connect(MigrationState *s,
> Error *error_in)
> >   return;
> >   }
> >
> > +if (runstate_check(RUN_STATE_POSTMIGRATE)) {
> > +error_report("Can't migrate the vm that is in POSTMIGRATE
> state");
> > +migrate_set_state(>state, s->state,
> MIGRATION_STATUS_FAILED);
> > +migrate_fd_cleanup(s);
> > +return;
> > +}
> > +
> >   if (resume) {
> >   /* This is a resumed migration */
> >   rate_limit = s->parameters.max_postcopy_bandwidth /
> >
> 
> 
> I think, correct place for the check migrate_prepare, as it is called for any 
> kind
> of migration, not only fd_. And in it we have already check for wrong state:
> 
>if (runstate_check(RUN_STATE_INMIGRATE)) {
>error_setg(errp, "Guest is waiting for an incoming migration");
>return false;
>}
> 
> and no additional state change and cleanup is needed.

Thank you for your advise, I'll send another patch. 

> --
> Best regards,
> Vladimir

[PATCH 14/17] target/mips: Declare gen_msa/_branch() in 'translate.h'

[Philippe Mathieu-Daudé]

Make gen_msa() and gen_msa_branch() public declarations
so we can keep calling them once extracted from the big
translate.c in the next commit.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.h | 2 ++
 target/mips/translate.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/mips/translate.h b/target/mips/translate.h
index 765018beeea..c26b0d9155d 100644
--- a/target/mips/translate.h
+++ b/target/mips/translate.h
@@ -82,5 +82,7 @@ extern TCGv bcond;
 
 /* MSA */
 void msa_translate_init(void);
+void gen_msa(DisasContext *ctx);
+void gen_msa_branch(DisasContext *ctx, uint32_t op1);
 
 #endif
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 8b1019506fe..d8553c626f3 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -28668,7 +28668,7 @@ static bool gen_msa_BxZ(DisasContext *ctx, int df, int 
wt, int s16, bool if_not)
 return true;
 }
 
-static void gen_msa_branch(DisasContext *ctx, uint32_t op1)
+void gen_msa_branch(DisasContext *ctx, uint32_t op1)
 {
 uint8_t df = (ctx->opcode >> 21) & 0x3;
 uint8_t wt = (ctx->opcode >> 16) & 0x1f;
@@ -30444,7 +30444,7 @@ static void gen_msa_vec(DisasContext *ctx)
 }
 }
 
-static void gen_msa(DisasContext *ctx)
+void gen_msa(DisasContext *ctx)
 {
 uint32_t opcode = ctx->opcode;
 
-- 
2.26.2

[PATCH 16/17] target/mips: Introduce decode tree bindings for MSA opcodes

[Philippe Mathieu-Daudé]

Introduce the 'mod-msa32' decodetree config for the 32-bit MSA ASE.

We decode the branch instructions, and all instructions based
on the MSA opcode.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.h |  1 +
 target/mips/mod-msa32.decode| 24 
 target/mips/mod-msa_translate.c | 31 +++
 target/mips/meson.build |  5 +
 4 files changed, 61 insertions(+)
 create mode 100644 target/mips/mod-msa32.decode

diff --git a/target/mips/translate.h b/target/mips/translate.h
index c26b0d9155d..c4fe18d187e 100644
--- a/target/mips/translate.h
+++ b/target/mips/translate.h
@@ -84,5 +84,6 @@ extern TCGv bcond;
 void msa_translate_init(void);
 void gen_msa(DisasContext *ctx);
 void gen_msa_branch(DisasContext *ctx, uint32_t op1);
+bool decode_msa32(DisasContext *ctx, uint32_t insn);
 
 #endif
diff --git a/target/mips/mod-msa32.decode b/target/mips/mod-msa32.decode
new file mode 100644
index 000..d69675132b8
--- /dev/null
+++ b/target/mips/mod-msa32.decode
@@ -0,0 +1,24 @@
+# MIPS SIMD Architecture Module instruction set
+#
+# Copyright (C) 2020  Philippe Mathieu-Daudé
+#
+# SPDX-License-Identifier: LGPL-2.1-or-later
+#
+# Reference:
+#   MIPS Architecture for Programmers Volume IV-j
+#   The MIPS32 SIMD Architecture Module, Revision 1.12
+#   (Document Number: MD00866-2B-MSA32-AFP-01.12)
+#
+
+_bz df wt s16
+
+@bz .. ... ..   wt:5 s16:16 _bz df=3
+@bz_df  .. ... df:2 wt:5 s16:16 _bz
+
+BZ_V010001 01011  . @bz
+BNZ_V   010001 0  . @bz
+
+BZ_x010001 110 .. . @bz_df
+BNZ_x   010001 111 .. . @bz_df
+
+MSA 00 --
diff --git a/target/mips/mod-msa_translate.c b/target/mips/mod-msa_translate.c
index 55c2a2f1acc..02df39c6b6c 100644
--- a/target/mips/mod-msa_translate.c
+++ b/target/mips/mod-msa_translate.c
@@ -6,6 +6,7 @@
  *  Copyright (c) 2006 Thiemo Seufer (MIPS32R2 support)
  *  Copyright (c) 2009 CodeSourcery (MIPS16 and microMIPS support)
  *  Copyright (c) 2012 Jia Liu & Dongxue Zhang (MIPS ASE DSP support)
+ *  Copyright (c) 2020 Philippe Mathieu-Daudé
  *
  * SPDX-License-Identifier: LGPL-2.1-or-later
  */
@@ -17,6 +18,9 @@
 #include "fpu_helper.h"
 #include "internal.h"
 
+/* Include the auto-generated decoder.  */
+#include "decode-mod-msa32.c.inc"
+
 #define OPC_MSA (0x1E << 26)
 
 #define MASK_MSA_MINOR(op)  (MASK_OP_MAJOR(op) | (op & 0x3F))
@@ -370,6 +374,16 @@ static bool gen_msa_BxZ_V(DisasContext *ctx, int wt, int 
s16, TCGCond cond)
 return true;
 }
 
+static bool trans_BZ_V(DisasContext *ctx, arg_msa_bz *a)
+{
+return gen_msa_BxZ_V(ctx, a->wt, a->s16, TCG_COND_EQ);
+}
+
+static bool trans_BNZ_V(DisasContext *ctx, arg_msa_bz *a)
+{
+return gen_msa_BxZ_V(ctx, a->wt, a->s16, TCG_COND_NE);
+}
+
 static bool gen_msa_BxZ(DisasContext *ctx, int df, int wt, int s16, bool 
if_not)
 {
 check_msa_access(ctx);
@@ -391,6 +405,16 @@ static bool gen_msa_BxZ(DisasContext *ctx, int df, int wt, 
int s16, bool if_not)
 return true;
 }
 
+static bool trans_BZ_x(DisasContext *ctx, arg_msa_bz *a)
+{
+return gen_msa_BxZ(ctx, a->df, a->wt, a->s16, false);
+}
+
+static bool trans_BNZ_x(DisasContext *ctx, arg_msa_bz *a)
+{
+return gen_msa_BxZ(ctx, a->df, a->wt, a->s16, true);
+}
+
 void gen_msa_branch(DisasContext *ctx, uint32_t op1)
 {
 uint8_t df = (ctx->opcode >> 21) & 0x3;
@@ -2264,3 +2288,10 @@ void gen_msa(DisasContext *ctx)
 break;
 }
 }
+
+static bool trans_MSA(DisasContext *ctx, arg_MSA *a)
+{
+gen_msa(ctx);
+
+return true;
+}
diff --git a/target/mips/meson.build b/target/mips/meson.build
index b6697e2fd72..7d0414bbe23 100644
--- a/target/mips/meson.build
+++ b/target/mips/meson.build
@@ -1,4 +1,9 @@
+gen = [
+  decodetree.process('mod-msa32.decode', extra_args: [ '--decode=decode_msa32' 
]),
+]
+
 mips_ss = ss.source_set()
+mips_ss.add(gen)
 mips_ss.add(files(
   'cpu.c',
   'dsp_helper.c',
-- 
2.26.2

[PATCH 12/17] target/mips: Extract MSA helpers from op_helper.c

[Philippe Mathieu-Daudé]

We have ~400 lines of MSA helpers in the generic op_helper.c,
move them with the other helpers in 'mod-msa_helper.c'.

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
Message-Id: <20201123204448.3260804-5-f4...@amsat.org>
---
 target/mips/mod-msa_helper.c | 393 ++
 target/mips/op_helper.c  | 394 ---
 2 files changed, 393 insertions(+), 394 deletions(-)

diff --git a/target/mips/mod-msa_helper.c b/target/mips/mod-msa_helper.c
index f0d728c03f0..1298a1917ce 100644
--- a/target/mips/mod-msa_helper.c
+++ b/target/mips/mod-msa_helper.c
@@ -22,6 +22,7 @@
 #include "internal.h"
 #include "exec/exec-all.h"
 #include "exec/helper-proto.h"
+#include "exec/memop.h"
 #include "fpu/softfloat.h"
 #include "fpu_helper.h"
 
@@ -8202,6 +8203,398 @@ void helper_msa_ffint_u_df(CPUMIPSState *env, uint32_t 
df, uint32_t wd,
 msa_move_v(pwd, pwx);
 }
 
+/* Data format min and max values */
+#define DF_BITS(df) (1 << ((df) + 3))
+
+/* Element-by-element access macros */
+#define DF_ELEMENTS(df) (MSA_WRLEN / DF_BITS(df))
+
+#if !defined(CONFIG_USER_ONLY)
+#define MEMOP_IDX(DF)   \
+TCGMemOpIdx oi = make_memop_idx(MO_TE | DF | MO_UNALN,  \
+cpu_mmu_index(env, false));
+#else
+#define MEMOP_IDX(DF)
+#endif
+
+void helper_msa_ld_b(CPUMIPSState *env, uint32_t wd,
+ target_ulong addr)
+{
+wr_t *pwd = &(env->active_fpu.fpr[wd].wr);
+MEMOP_IDX(DF_BYTE)
+#if !defined(CONFIG_USER_ONLY)
+#if !defined(HOST_WORDS_BIGENDIAN)
+pwd->b[0]  = helper_ret_ldub_mmu(env, addr + (0  << DF_BYTE), oi, GETPC());
+pwd->b[1]  = helper_ret_ldub_mmu(env, addr + (1  << DF_BYTE), oi, GETPC());
+pwd->b[2]  = helper_ret_ldub_mmu(env, addr + (2  << DF_BYTE), oi, GETPC());
+pwd->b[3]  = helper_ret_ldub_mmu(env, addr + (3  << DF_BYTE), oi, GETPC());
+pwd->b[4]  = helper_ret_ldub_mmu(env, addr + (4  << DF_BYTE), oi, GETPC());
+pwd->b[5]  = helper_ret_ldub_mmu(env, addr + (5  << DF_BYTE), oi, GETPC());
+pwd->b[6]  = helper_ret_ldub_mmu(env, addr + (6  << DF_BYTE), oi, GETPC());
+pwd->b[7]  = helper_ret_ldub_mmu(env, addr + (7  << DF_BYTE), oi, GETPC());
+pwd->b[8]  = helper_ret_ldub_mmu(env, addr + (8  << DF_BYTE), oi, GETPC());
+pwd->b[9]  = helper_ret_ldub_mmu(env, addr + (9  << DF_BYTE), oi, GETPC());
+pwd->b[10] = helper_ret_ldub_mmu(env, addr + (10 << DF_BYTE), oi, GETPC());
+pwd->b[11] = helper_ret_ldub_mmu(env, addr + (11 << DF_BYTE), oi, GETPC());
+pwd->b[12] = helper_ret_ldub_mmu(env, addr + (12 << DF_BYTE), oi, GETPC());
+pwd->b[13] = helper_ret_ldub_mmu(env, addr + (13 << DF_BYTE), oi, GETPC());
+pwd->b[14] = helper_ret_ldub_mmu(env, addr + (14 << DF_BYTE), oi, GETPC());
+pwd->b[15] = helper_ret_ldub_mmu(env, addr + (15 << DF_BYTE), oi, GETPC());
+#else
+pwd->b[0]  = helper_ret_ldub_mmu(env, addr + (7  << DF_BYTE), oi, GETPC());
+pwd->b[1]  = helper_ret_ldub_mmu(env, addr + (6  << DF_BYTE), oi, GETPC());
+pwd->b[2]  = helper_ret_ldub_mmu(env, addr + (5  << DF_BYTE), oi, GETPC());
+pwd->b[3]  = helper_ret_ldub_mmu(env, addr + (4  << DF_BYTE), oi, GETPC());
+pwd->b[4]  = helper_ret_ldub_mmu(env, addr + (3  << DF_BYTE), oi, GETPC());
+pwd->b[5]  = helper_ret_ldub_mmu(env, addr + (2  << DF_BYTE), oi, GETPC());
+pwd->b[6]  = helper_ret_ldub_mmu(env, addr + (1  << DF_BYTE), oi, GETPC());
+pwd->b[7]  = helper_ret_ldub_mmu(env, addr + (0  << DF_BYTE), oi, GETPC());
+pwd->b[8]  = helper_ret_ldub_mmu(env, addr + (15 << DF_BYTE), oi, GETPC());
+pwd->b[9]  = helper_ret_ldub_mmu(env, addr + (14 << DF_BYTE), oi, GETPC());
+pwd->b[10] = helper_ret_ldub_mmu(env, addr + (13 << DF_BYTE), oi, GETPC());
+pwd->b[11] = helper_ret_ldub_mmu(env, addr + (12 << DF_BYTE), oi, GETPC());
+pwd->b[12] = helper_ret_ldub_mmu(env, addr + (11 << DF_BYTE), oi, GETPC());
+pwd->b[13] = helper_ret_ldub_mmu(env, addr + (10 << DF_BYTE), oi, GETPC());
+pwd->b[14] = helper_ret_ldub_mmu(env, addr + (9  << DF_BYTE), oi, GETPC());
+pwd->b[15] = helper_ret_ldub_mmu(env, addr + (8  << DF_BYTE), oi, GETPC());
+#endif
+#else
+#if !defined(HOST_WORDS_BIGENDIAN)
+pwd->b[0]  = cpu_ldub_data(env, addr + (0  << DF_BYTE));
+pwd->b[1]  = cpu_ldub_data(env, addr + (1  << DF_BYTE));
+pwd->b[2]  = cpu_ldub_data(env, addr + (2  << DF_BYTE));
+pwd->b[3]  = cpu_ldub_data(env, addr + (3  << DF_BYTE));
+pwd->b[4]  = cpu_ldub_data(env, addr + (4  << DF_BYTE));
+pwd->b[5]  = cpu_ldub_data(env, addr + (5  << DF_BYTE));
+pwd->b[6]  = cpu_ldub_data(env, addr + (6  << DF_BYTE));
+pwd->b[7]  = cpu_ldub_data(env, addr + (7  << DF_BYTE));
+pwd->b[8]  = cpu_ldub_data(env, addr + (8  << DF_BYTE));
+pwd->b[9]  = cpu_ldub_data(env, addr + (9  << DF_BYTE));
+pwd->b[10] = cpu_ldub_data(env, addr + (10 << DF_BYTE));
+pwd->b[11] = cpu_ldub_data(env, addr + 

[PATCH 08/17] target/mips: Remove CPUMIPSState* argument from gen_msa*() methods

[Philippe Mathieu-Daudé]

The gen_msa*() methods don't use the "CPUMIPSState *env"
argument. Remove it to simplify.

Reviewed-by: Jiaxun Yang 
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.c | 57 -
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index bbe06240510..5ed7072f275 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -28623,7 +28623,7 @@ static void gen_check_zero_element(TCGv tresult, 
uint8_t df, uint8_t wt)
 tcg_temp_free_i64(t1);
 }
 
-static void gen_msa_branch(CPUMIPSState *env, DisasContext *ctx, uint32_t op1)
+static void gen_msa_branch(DisasContext *ctx, uint32_t op1)
 {
 uint8_t df = (ctx->opcode >> 21) & 0x3;
 uint8_t wt = (ctx->opcode >> 16) & 0x1f;
@@ -28668,7 +28668,7 @@ static void gen_msa_branch(CPUMIPSState *env, 
DisasContext *ctx, uint32_t op1)
 ctx->hflags |= MIPS_HFLAG_BDS32;
 }
 
-static void gen_msa_i8(CPUMIPSState *env, DisasContext *ctx)
+static void gen_msa_i8(DisasContext *ctx)
 {
 #define MASK_MSA_I8(op)(MASK_MSA_MINOR(op) | (op & (0x03 << 24)))
 uint8_t i8 = (ctx->opcode >> 16) & 0xff;
@@ -28726,7 +28726,7 @@ static void gen_msa_i8(CPUMIPSState *env, DisasContext 
*ctx)
 tcg_temp_free_i32(ti8);
 }
 
-static void gen_msa_i5(CPUMIPSState *env, DisasContext *ctx)
+static void gen_msa_i5(DisasContext *ctx)
 {
 #define MASK_MSA_I5(op)(MASK_MSA_MINOR(op) | (op & (0x7 << 23)))
 uint8_t df = (ctx->opcode >> 21) & 0x3;
@@ -28799,7 +28799,7 @@ static void gen_msa_i5(CPUMIPSState *env, DisasContext 
*ctx)
 tcg_temp_free_i32(timm);
 }
 
-static void gen_msa_bit(CPUMIPSState *env, DisasContext *ctx)
+static void gen_msa_bit(DisasContext *ctx)
 {
 #define MASK_MSA_BIT(op)(MASK_MSA_MINOR(op) | (op & (0x7 << 23)))
 uint8_t dfm = (ctx->opcode >> 16) & 0x7f;
@@ -28883,7 +28883,7 @@ static void gen_msa_bit(CPUMIPSState *env, DisasContext 
*ctx)
 tcg_temp_free_i32(tws);
 }
 
-static void gen_msa_3r(CPUMIPSState *env, DisasContext *ctx)
+static void gen_msa_3r(DisasContext *ctx)
 {
 #define MASK_MSA_3R(op)(MASK_MSA_MINOR(op) | (op & (0x7 << 23)))
 uint8_t df = (ctx->opcode >> 21) & 0x3;
@@ -29865,7 +29865,7 @@ static void gen_msa_3r(CPUMIPSState *env, DisasContext 
*ctx)
 tcg_temp_free_i32(tdf);
 }
 
-static void gen_msa_elm_3e(CPUMIPSState *env, DisasContext *ctx)
+static void gen_msa_elm_3e(DisasContext *ctx)
 {
 #define MASK_MSA_ELM_DF3E(op)   (MASK_MSA_MINOR(op) | (op & (0x3FF << 16)))
 uint8_t source = (ctx->opcode >> 11) & 0x1f;
@@ -29897,8 +29897,7 @@ static void gen_msa_elm_3e(CPUMIPSState *env, 
DisasContext *ctx)
 tcg_temp_free_i32(tsr);
 }
 
-static void gen_msa_elm_df(CPUMIPSState *env, DisasContext *ctx, uint32_t df,
-uint32_t n)
+static void gen_msa_elm_df(DisasContext *ctx, uint32_t df, uint32_t n)
 {
 #define MASK_MSA_ELM(op)(MASK_MSA_MINOR(op) | (op & (0xf << 22)))
 uint8_t ws = (ctx->opcode >> 11) & 0x1f;
@@ -30008,7 +30007,7 @@ static void gen_msa_elm_df(CPUMIPSState *env, 
DisasContext *ctx, uint32_t df,
 tcg_temp_free_i32(tdf);
 }
 
-static void gen_msa_elm(CPUMIPSState *env, DisasContext *ctx)
+static void gen_msa_elm(DisasContext *ctx)
 {
 uint8_t dfn = (ctx->opcode >> 16) & 0x3f;
 uint32_t df = 0, n = 0;
@@ -30027,17 +30026,17 @@ static void gen_msa_elm(CPUMIPSState *env, 
DisasContext *ctx)
 df = DF_DOUBLE;
 } else if (dfn == 0x3E) {
 /* CTCMSA, CFCMSA, MOVE.V */
-gen_msa_elm_3e(env, ctx);
+gen_msa_elm_3e(ctx);
 return;
 } else {
 generate_exception_end(ctx, EXCP_RI);
 return;
 }
 
-gen_msa_elm_df(env, ctx, df, n);
+gen_msa_elm_df(ctx, df, n);
 }
 
-static void gen_msa_3rf(CPUMIPSState *env, DisasContext *ctx)
+static void gen_msa_3rf(DisasContext *ctx)
 {
 #define MASK_MSA_3RF(op)(MASK_MSA_MINOR(op) | (op & (0xf << 22)))
 uint8_t df = (ctx->opcode >> 21) & 0x1;
@@ -30195,7 +30194,7 @@ static void gen_msa_3rf(CPUMIPSState *env, DisasContext 
*ctx)
 tcg_temp_free_i32(tdf);
 }
 
-static void gen_msa_2r(CPUMIPSState *env, DisasContext *ctx)
+static void gen_msa_2r(DisasContext *ctx)
 {
 #define MASK_MSA_2R(op) (MASK_MSA_MINOR(op) | (op & (0x1f << 21)) | \
 (op & (0x7 << 18)))
@@ -30279,7 +30278,7 @@ static void gen_msa_2r(CPUMIPSState *env, DisasContext 
*ctx)
 tcg_temp_free_i32(tdf);
 }
 
-static void gen_msa_2rf(CPUMIPSState *env, DisasContext *ctx)
+static void gen_msa_2rf(DisasContext *ctx)
 {
 #define MASK_MSA_2RF(op)(MASK_MSA_MINOR(op) | (op & (0x1f << 21)) | \
 (op & (0xf << 17)))
@@ -30350,7 +30349,7 @@ static void gen_msa_2rf(CPUMIPSState *env, DisasContext 
*ctx)
 tcg_temp_free_i32(tdf);
 }
 
-static void gen_msa_vec_v(CPUMIPSState *env, DisasContext *ctx)
+static void gen_msa_vec_v(DisasContext *ctx)
 {
 #define MASK_MSA_VEC(op)(MASK_MSA_MINOR(op) 

[PATCH 13/17] target/mips: Extract MSA helper definitions

[Philippe Mathieu-Daudé]

Keep all MSA-related code altogether.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20201120210844.2625602-4-f4...@amsat.org>
---
 target/mips/helper.h | 436 +-
 target/mips/mod-msa_helper.h.inc | 443 +++
 2 files changed, 445 insertions(+), 434 deletions(-)
 create mode 100644 target/mips/mod-msa_helper.h.inc

diff --git a/target/mips/helper.h b/target/mips/helper.h
index e97655dc0eb..80eb675fa64 100644
--- a/target/mips/helper.h
+++ b/target/mips/helper.h
@@ -781,438 +781,6 @@ DEF_HELPER_FLAGS_3(dmthlip, 0, void, tl, tl, env)
 DEF_HELPER_FLAGS_3(wrdsp, 0, void, tl, tl, env)
 DEF_HELPER_FLAGS_2(rddsp, 0, tl, tl, env)
 
-/* MIPS SIMD Architecture */
-
-DEF_HELPER_3(msa_nloc_b, void, env, i32, i32)
-DEF_HELPER_3(msa_nloc_h, void, env, i32, i32)
-DEF_HELPER_3(msa_nloc_w, void, env, i32, i32)
-DEF_HELPER_3(msa_nloc_d, void, env, i32, i32)
-
-DEF_HELPER_3(msa_nlzc_b, void, env, i32, i32)
-DEF_HELPER_3(msa_nlzc_h, void, env, i32, i32)
-DEF_HELPER_3(msa_nlzc_w, void, env, i32, i32)
-DEF_HELPER_3(msa_nlzc_d, void, env, i32, i32)
-
-DEF_HELPER_3(msa_pcnt_b, void, env, i32, i32)
-DEF_HELPER_3(msa_pcnt_h, void, env, i32, i32)
-DEF_HELPER_3(msa_pcnt_w, void, env, i32, i32)
-DEF_HELPER_3(msa_pcnt_d, void, env, i32, i32)
-
-DEF_HELPER_4(msa_binsl_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_binsl_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_binsl_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_binsl_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_binsr_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_binsr_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_binsr_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_binsr_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_bmnz_v, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bmz_v, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bsel_v, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_bclr_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bclr_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bclr_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bclr_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_bneg_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bneg_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bneg_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bneg_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_bset_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bset_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bset_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_bset_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_add_a_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_add_a_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_add_a_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_add_a_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_adds_a_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_adds_a_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_adds_a_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_adds_a_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_adds_s_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_adds_s_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_adds_s_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_adds_s_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_adds_u_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_adds_u_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_adds_u_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_adds_u_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_addv_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_addv_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_addv_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_addv_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_hadd_s_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_hadd_s_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_hadd_s_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_hadd_u_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_hadd_u_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_hadd_u_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_ave_s_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_ave_s_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_ave_s_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_ave_s_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_ave_u_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_ave_u_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_ave_u_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_ave_u_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_aver_s_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_aver_s_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_aver_s_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_aver_s_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_aver_u_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_aver_u_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_aver_u_w, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_aver_u_d, void, env, i32, i32, i32)
-
-DEF_HELPER_4(msa_ceq_b, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_ceq_h, void, env, i32, i32, i32)
-DEF_HELPER_4(msa_ceq_w, void, env, i32, i32, i32)

Re: [PATCH 00/17] target/mips: Convert MSA ASE to decodetree

[Philippe Mathieu-Daudé]

On Tue, Dec 8, 2020 at 1:37 AM Philippe Mathieu-Daudé  wrote:
>
> Finally, we use decodetree with the MIPS target.
>
> Starting easy with the MSA ASE. 2700+ lines extracted
> from helper.h and translate.c, now built as an new
> object: mod-msa_translate.o.
>
> While the diff stat is positive by 86 lines, we actually
> (re)moved code, but added (C) notices.
>
> The most interesting patches are the 2 last ones.
>
> Please review,

I forgot to mention, only 4/17 patches miss review!
- 11, 14, 16, 17

>
> Phil.

[PATCH 17/17] target/mips: Use decode_msa32() generated from decodetree

[Philippe Mathieu-Daudé]

Now that we can decode the MSA ASE opcodes with decode_msa32(),
use it and remove the unreachable code.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/fpu_translate.h | 10 --
 target/mips/translate.h |  2 --
 target/mips/mod-msa_translate.c | 29 +
 target/mips/translate.c | 31 ++-
 4 files changed, 11 insertions(+), 61 deletions(-)

diff --git a/target/mips/fpu_translate.h b/target/mips/fpu_translate.h
index f45314d2ec2..58e5e768c99 100644
--- a/target/mips/fpu_translate.h
+++ b/target/mips/fpu_translate.h
@@ -42,8 +42,6 @@ enum {
 OPC_BC1  = (0x08 << 21) | OPC_CP1, /* bc */
 OPC_BC1ANY2  = (0x09 << 21) | OPC_CP1,
 OPC_BC1ANY4  = (0x0A << 21) | OPC_CP1,
-OPC_BZ_V = (0x0B << 21) | OPC_CP1,
-OPC_BNZ_V= (0x0F << 21) | OPC_CP1,
 OPC_S_FMT= (FMT_S << 21) | OPC_CP1,
 OPC_D_FMT= (FMT_D << 21) | OPC_CP1,
 OPC_E_FMT= (FMT_E << 21) | OPC_CP1,
@@ -53,14 +51,6 @@ enum {
 OPC_PS_FMT   = (FMT_PS << 21) | OPC_CP1,
 OPC_BC1EQZ   = (0x09 << 21) | OPC_CP1,
 OPC_BC1NEZ   = (0x0D << 21) | OPC_CP1,
-OPC_BZ_B = (0x18 << 21) | OPC_CP1,
-OPC_BZ_H = (0x19 << 21) | OPC_CP1,
-OPC_BZ_W = (0x1A << 21) | OPC_CP1,
-OPC_BZ_D = (0x1B << 21) | OPC_CP1,
-OPC_BNZ_B= (0x1C << 21) | OPC_CP1,
-OPC_BNZ_H= (0x1D << 21) | OPC_CP1,
-OPC_BNZ_W= (0x1E << 21) | OPC_CP1,
-OPC_BNZ_D= (0x1F << 21) | OPC_CP1,
 };
 
 #define MASK_CP1_FUNC(op)   (MASK_CP1(op) | (op & 0x3F))
diff --git a/target/mips/translate.h b/target/mips/translate.h
index c4fe18d187e..cba28f49753 100644
--- a/target/mips/translate.h
+++ b/target/mips/translate.h
@@ -82,8 +82,6 @@ extern TCGv bcond;
 
 /* MSA */
 void msa_translate_init(void);
-void gen_msa(DisasContext *ctx);
-void gen_msa_branch(DisasContext *ctx, uint32_t op1);
 bool decode_msa32(DisasContext *ctx, uint32_t insn);
 
 #endif
diff --git a/target/mips/mod-msa_translate.c b/target/mips/mod-msa_translate.c
index 02df39c6b6c..7e7fc0644ff 100644
--- a/target/mips/mod-msa_translate.c
+++ b/target/mips/mod-msa_translate.c
@@ -415,33 +415,6 @@ static bool trans_BNZ_x(DisasContext *ctx, arg_msa_bz *a)
 return gen_msa_BxZ(ctx, a->df, a->wt, a->s16, true);
 }
 
-void gen_msa_branch(DisasContext *ctx, uint32_t op1)
-{
-uint8_t df = (ctx->opcode >> 21) & 0x3;
-uint8_t wt = (ctx->opcode >> 16) & 0x1f;
-int64_t s16 = (int16_t)ctx->opcode;
-
-switch (op1) {
-case OPC_BZ_V:
-case OPC_BNZ_V:
-gen_msa_BxZ_V(ctx, wt, s16, (op1 == OPC_BZ_V) ?
-TCG_COND_EQ : TCG_COND_NE);
-break;
-case OPC_BZ_B:
-case OPC_BZ_H:
-case OPC_BZ_W:
-case OPC_BZ_D:
-gen_msa_BxZ(ctx, df, wt, s16, false);
-break;
-case OPC_BNZ_B:
-case OPC_BNZ_H:
-case OPC_BNZ_W:
-case OPC_BNZ_D:
-gen_msa_BxZ(ctx, df, wt, s16, true);
-break;
-}
-}
-
 static void gen_msa_i8(DisasContext *ctx)
 {
 #define MASK_MSA_I8(op)(MASK_MSA_MINOR(op) | (op & (0x03 << 24)))
@@ -2191,7 +2164,7 @@ static void gen_msa_vec(DisasContext *ctx)
 }
 }
 
-void gen_msa(DisasContext *ctx)
+static void gen_msa(DisasContext *ctx)
 {
 uint32_t opcode = ctx->opcode;
 
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 5cd67459304..41c0b59a473 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -6,6 +6,7 @@
  *  Copyright (c) 2006 Thiemo Seufer (MIPS32R2 support)
  *  Copyright (c) 2009 CodeSourcery (MIPS16 and microMIPS support)
  *  Copyright (c) 2012 Jia Liu & Dongxue Zhang (MIPS ASE DSP support)
+ *  Copyright (c) 2020 Philippe Mathieu-Daudé
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -141,8 +142,6 @@ enum {
 OPC_JIALC= (0x3E << 26),
 /* MDMX ASE specific */
 OPC_MDMX = (0x1E << 26),
-/* MSA ASE, same as MDMX */
-OPC_MSA  = OPC_MDMX,
 /* Cache and prefetch */
 OPC_CACHE= (0x2F << 26),
 OPC_PREF = (0x33 << 26),
@@ -28336,6 +28335,13 @@ static void decode_opc(CPUMIPSState *env, DisasContext 
*ctx)
 gen_set_label(l1);
 }
 
+/* Transition to the auto-generated decoder.  */
+
+/* ISA Extensions */
+if (ase_msa_available(env) && decode_msa32(ctx, ctx->opcode)) {
+return;
+}
+
 op = MASK_OP_MAJOR(ctx->opcode);
 rs = (ctx->opcode >> 21) & 0x1f;
 rt = (ctx->opcode >> 16) & 0x1f;
@@ -28853,20 +28859,6 @@ static void decode_opc(CPUMIPSState *env, DisasContext 
*ctx)
 }
 break;
 }
-case OPC_BZ_V:
-case OPC_BNZ_V:
-case OPC_BZ_B:
-case OPC_BZ_H:
-case OPC_BZ_W:
-case OPC_BZ_D:
-case OPC_BNZ_B:
-case OPC_BNZ_H:
-case OPC_BNZ_W:
-case OPC_BNZ_D:
-if (ase_msa_available(env)) {
-   

[PATCH 06/17] target/mips: Alias MSA vector registers on FPU scalar registers

[Philippe Mathieu-Daudé]

Commits 863f264d10f ("add msa_reset(), global msa register") and
cb269f273fd ("fix multiple TCG registers covering same data")
removed the FPU scalar registers and replaced them by aliases to
the MSA vector registers.
While this might be the case for CPU implementing MSA, this makes
QEMU code incoherent for CPU not implementing it. It is simpler
to inverse the logic and alias the MSA vector registers on the
FPU scalar ones.

Reviewed-by: Jiaxun Yang 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index da0cb98df09..95d07e837c0 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -31561,16 +31561,20 @@ void mips_tcg_init(void)
 offsetof(CPUMIPSState,
  active_tc.gpr[i]),
 regnames[i]);
-
 for (i = 0; i < 32; i++) {
 int off = offsetof(CPUMIPSState, active_fpu.fpr[i].wr.d[0]);
-msa_wr_d[i * 2] =
-tcg_global_mem_new_i64(cpu_env, off, msaregnames[i * 2]);
+
+fpu_f64[i] = tcg_global_mem_new_i64(cpu_env, off, fregnames[i]);
+}
+/* MSA */
+for (i = 0; i < 32; i++) {
+int off = offsetof(CPUMIPSState, active_fpu.fpr[i].wr.d[0]);
+
 /*
- * The scalar floating-point unit (FPU) registers are mapped on
- * the MSA vector registers.
+ * The MSA vector registers are mapped on the
+ * scalar floating-point unit (FPU) registers.
  */
-fpu_f64[i] = msa_wr_d[i * 2];
+msa_wr_d[i * 2] = fpu_f64[i];
 off = offsetof(CPUMIPSState, active_fpu.fpr[i].wr.d[1]);
 msa_wr_d[i * 2 + 1] =
 tcg_global_mem_new_i64(cpu_env, off, msaregnames[i * 2 + 1]);
-- 
2.26.2

[PATCH 11/17] target/mips: Move msa_reset() to mod-msa_helper.c

[Philippe Mathieu-Daudé]

translate_init.c.inc mostly contains CPU definitions.
msa_reset() doesn't belong here, move it with the MSA
helpers.

One comment style is updated to avoid checkpatch.pl warning.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/internal.h   |  2 ++
 target/mips/mod-msa_helper.c | 36 
 target/mips/translate_init.c.inc | 34 --
 3 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/target/mips/internal.h b/target/mips/internal.h
index 3bd41239b1d..7813eb224c9 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -201,6 +201,8 @@ void mips_tcg_init(void);
 void cpu_state_reset(CPUMIPSState *s);
 void cpu_mips_realize_env(CPUMIPSState *env);
 
+void msa_reset(CPUMIPSState *env);
+
 /* cp0_timer.c */
 uint32_t cpu_mips_get_count(CPUMIPSState *env);
 void cpu_mips_store_count(CPUMIPSState *env, uint32_t value);
diff --git a/target/mips/mod-msa_helper.c b/target/mips/mod-msa_helper.c
index b89b4c44902..f0d728c03f0 100644
--- a/target/mips/mod-msa_helper.c
+++ b/target/mips/mod-msa_helper.c
@@ -8201,3 +8201,39 @@ void helper_msa_ffint_u_df(CPUMIPSState *env, uint32_t 
df, uint32_t wd,
 
 msa_move_v(pwd, pwx);
 }
+
+void msa_reset(CPUMIPSState *env)
+{
+if (!ase_msa_available(env)) {
+return;
+}
+
+#ifdef CONFIG_USER_ONLY
+/* MSA access enabled */
+env->CP0_Config5 |= 1 << CP0C5_MSAEn;
+env->CP0_Status |= (1 << CP0St_CU1) | (1 << CP0St_FR);
+#endif
+
+/*
+ * MSA CSR:
+ * - non-signaling floating point exception mode off (NX bit is 0)
+ * - Cause, Enables, and Flags are all 0
+ * - round to nearest / ties to even (RM bits are 0)
+ */
+env->active_tc.msacsr = 0;
+
+restore_msa_fp_status(env);
+
+/* tininess detected after rounding.*/
+set_float_detect_tininess(float_tininess_after_rounding,
+  >active_tc.msa_fp_status);
+
+/* clear float_status exception flags */
+set_float_exception_flags(0, >active_tc.msa_fp_status);
+
+/* clear float_status nan mode */
+set_default_nan_mode(0, >active_tc.msa_fp_status);
+
+/* set proper signanling bit meaning ("1" means "quiet") */
+set_snan_bit_is_one(0, >active_tc.msa_fp_status);
+}
diff --git a/target/mips/translate_init.c.inc b/target/mips/translate_init.c.inc
index f6752d00afe..4856f4c5a4a 100644
--- a/target/mips/translate_init.c.inc
+++ b/target/mips/translate_init.c.inc
@@ -1021,37 +1021,3 @@ static void mvp_init(CPUMIPSState *env)
  (0x0 << CP0MVPC1_PCX) | (0x0 << CP0MVPC1_PCP2) |
  (0x1 << CP0MVPC1_PCP1);
 }
-
-static void msa_reset(CPUMIPSState *env)
-{
-if (!ase_msa_available(env)) {
-return;
-}
-
-#ifdef CONFIG_USER_ONLY
-/* MSA access enabled */
-env->CP0_Config5 |= 1 << CP0C5_MSAEn;
-env->CP0_Status |= (1 << CP0St_CU1) | (1 << CP0St_FR);
-#endif
-
-/* MSA CSR:
-   - non-signaling floating point exception mode off (NX bit is 0)
-   - Cause, Enables, and Flags are all 0
-   - round to nearest / ties to even (RM bits are 0) */
-env->active_tc.msacsr = 0;
-
-restore_msa_fp_status(env);
-
-/* tininess detected after rounding.*/
-set_float_detect_tininess(float_tininess_after_rounding,
-  >active_tc.msa_fp_status);
-
-/* clear float_status exception flags */
-set_float_exception_flags(0, >active_tc.msa_fp_status);
-
-/* clear float_status nan mode */
-set_default_nan_mode(0, >active_tc.msa_fp_status);
-
-/* set proper signanling bit meaning ("1" means "quiet") */
-set_snan_bit_is_one(0, >active_tc.msa_fp_status);
-}
-- 
2.26.2

[PATCH 05/17] target/mips: Remove now unused ASE_MSA definition

[Philippe Mathieu-Daudé]

We don't use ASE_MSA anymore (replaced by ase_msa_available()
checking MSAP bit from CP0_Config3). Remove it.

Reviewed-by: Jiaxun Yang 
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/mips-defs.h  | 1 -
 target/mips/translate_init.c.inc | 8 
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
index ed6a7a9e545..805034b8956 100644
--- a/target/mips/mips-defs.h
+++ b/target/mips/mips-defs.h
@@ -45,7 +45,6 @@
 #define ASE_MT0x4000ULL
 #define ASE_SMARTMIPS 0x8000ULL
 #define ASE_MICROMIPS 0x0001ULL
-#define ASE_MSA   0x0002ULL
 /*
  *   bits 40-51: vendor-specific base instruction sets
  */
diff --git a/target/mips/translate_init.c.inc b/target/mips/translate_init.c.inc
index 3c9ec7e940a..f6752d00afe 100644
--- a/target/mips/translate_init.c.inc
+++ b/target/mips/translate_init.c.inc
@@ -408,7 +408,7 @@ const mips_def_t mips_defs[] =
 .CP1_fcr31_rw_bitmask = 0xFF83,
 .SEGBITS = 32,
 .PABITS = 40,
-.insn_flags = CPU_MIPS32R5 | ASE_MSA,
+.insn_flags = CPU_MIPS32R5,
 .mmu_type = MMU_TYPE_R4000,
 },
 {
@@ -721,7 +721,7 @@ const mips_def_t mips_defs[] =
 .MSAIR = 0x03 << MSAIR_ProcID,
 .SEGBITS = 48,
 .PABITS = 48,
-.insn_flags = CPU_MIPS64R6 | ASE_MSA,
+.insn_flags = CPU_MIPS64R6,
 .mmu_type = MMU_TYPE_R4000,
 },
 {
@@ -761,7 +761,7 @@ const mips_def_t mips_defs[] =
 .MSAIR = 0x03 << MSAIR_ProcID,
 .SEGBITS = 48,
 .PABITS = 48,
-.insn_flags = CPU_MIPS64R6 | ASE_MSA,
+.insn_flags = CPU_MIPS64R6,
 .mmu_type = MMU_TYPE_R4000,
 },
 {
@@ -887,7 +887,7 @@ const mips_def_t mips_defs[] =
 .CP1_fcr31_rw_bitmask = 0xFF83,
 .SEGBITS = 48,
 .PABITS = 48,
-.insn_flags = CPU_LOONGSON3A | ASE_MSA,
+.insn_flags = CPU_LOONGSON3A,
 .mmu_type = MMU_TYPE_R4000,
 },
 {
-- 
2.26.2

[PATCH 10/17] target/mips: Rename msa_helper.c as mod-msa_helper.c

[Philippe Mathieu-Daudé]

MSA means 'MIPS SIMD Architecture' and is defined as a Module by
MIPS.
To keep the directory sorted, we use the 'mod' prefix for MIPS
modules. Rename msa_helper.c as mod-msa_helper.c.

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
Message-Id: <20201123204448.3260804-4-f4...@amsat.org>
---
 target/mips/{msa_helper.c => mod-msa_helper.c} | 0
 target/mips/meson.build| 3 ++-
 2 files changed, 2 insertions(+), 1 deletion(-)
 rename target/mips/{msa_helper.c => mod-msa_helper.c} (100%)

diff --git a/target/mips/msa_helper.c b/target/mips/mod-msa_helper.c
similarity index 100%
rename from target/mips/msa_helper.c
rename to target/mips/mod-msa_helper.c
diff --git a/target/mips/meson.build b/target/mips/meson.build
index 681a5524c0e..35dbbbf6519 100644
--- a/target/mips/meson.build
+++ b/target/mips/meson.build
@@ -6,8 +6,9 @@
   'gdbstub.c',
   'helper.c',
   'lmmi_helper.c',
-  'msa_helper.c',
   'op_helper.c',
+  'mod-msa_helper.c',
+
   'translate.c',
 ))
 mips_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c'))
-- 
2.26.2

[PATCH 09/17] target/mips: Explode gen_msa_branch() as gen_msa_BxZ_V/BxZ()

[Philippe Mathieu-Daudé]

In preparation of using the decodetree script, explode
gen_msa_branch() as following:

- OPC_BZ_V  -> BxZ_V(EQ)
- OPC_BNZ_V -> BxZ_V(NE)
- OPC_BZ_[BHWD] -> BxZ(false)
- OPC_BNZ_[BHWD]-> BxZ(true)

Reviewed-by: Jiaxun Yang 
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.c | 71 -
 1 file changed, 49 insertions(+), 22 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 5ed7072f275..8b1019506fe 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -28623,49 +28623,76 @@ static void gen_check_zero_element(TCGv tresult, 
uint8_t df, uint8_t wt)
 tcg_temp_free_i64(t1);
 }
 
+static bool gen_msa_BxZ_V(DisasContext *ctx, int wt, int s16, TCGCond cond)
+{
+TCGv_i64 t0;
+
+check_msa_access(ctx);
+
+if (ctx->hflags & MIPS_HFLAG_BMASK) {
+generate_exception_end(ctx, EXCP_RI);
+return true;
+}
+t0 = tcg_temp_new_i64();
+tcg_gen_or_i64(t0, msa_wr_d[wt << 1], msa_wr_d[(wt << 1) + 1]);
+tcg_gen_setcondi_i64(cond, t0, t0, 0);
+tcg_gen_trunc_i64_tl(bcond, t0);
+tcg_temp_free_i64(t0);
+
+ctx->btarget = ctx->base.pc_next + (s16 << 2) + 4;
+
+ctx->hflags |= MIPS_HFLAG_BC;
+ctx->hflags |= MIPS_HFLAG_BDS32;
+
+return true;
+}
+
+static bool gen_msa_BxZ(DisasContext *ctx, int df, int wt, int s16, bool 
if_not)
+{
+check_msa_access(ctx);
+
+if (ctx->hflags & MIPS_HFLAG_BMASK) {
+generate_exception_end(ctx, EXCP_RI);
+return true;
+}
+
+gen_check_zero_element(bcond, df, wt);
+if (if_not) {
+tcg_gen_setcondi_tl(TCG_COND_EQ, bcond, bcond, 0);
+}
+
+ctx->btarget = ctx->base.pc_next + (s16 << 2) + 4;
+ctx->hflags |= MIPS_HFLAG_BC;
+ctx->hflags |= MIPS_HFLAG_BDS32;
+
+return true;
+}
+
 static void gen_msa_branch(DisasContext *ctx, uint32_t op1)
 {
 uint8_t df = (ctx->opcode >> 21) & 0x3;
 uint8_t wt = (ctx->opcode >> 16) & 0x1f;
 int64_t s16 = (int16_t)ctx->opcode;
 
-check_msa_access(ctx);
-
-if (ctx->hflags & MIPS_HFLAG_BMASK) {
-generate_exception_end(ctx, EXCP_RI);
-return;
-}
 switch (op1) {
 case OPC_BZ_V:
 case OPC_BNZ_V:
-{
-TCGv_i64 t0 = tcg_temp_new_i64();
-tcg_gen_or_i64(t0, msa_wr_d[wt << 1], msa_wr_d[(wt << 1) + 1]);
-tcg_gen_setcondi_i64((op1 == OPC_BZ_V) ?
-TCG_COND_EQ : TCG_COND_NE, t0, t0, 0);
-tcg_gen_trunc_i64_tl(bcond, t0);
-tcg_temp_free_i64(t0);
-}
+gen_msa_BxZ_V(ctx, wt, s16, (op1 == OPC_BZ_V) ?
+TCG_COND_EQ : TCG_COND_NE);
 break;
 case OPC_BZ_B:
 case OPC_BZ_H:
 case OPC_BZ_W:
 case OPC_BZ_D:
-gen_check_zero_element(bcond, df, wt);
+gen_msa_BxZ(ctx, df, wt, s16, false);
 break;
 case OPC_BNZ_B:
 case OPC_BNZ_H:
 case OPC_BNZ_W:
 case OPC_BNZ_D:
-gen_check_zero_element(bcond, df, wt);
-tcg_gen_setcondi_tl(TCG_COND_EQ, bcond, bcond, 0);
+gen_msa_BxZ(ctx, df, wt, s16, true);
 break;
 }
-
-ctx->btarget = ctx->base.pc_next + (s16 << 2) + 4;
-
-ctx->hflags |= MIPS_HFLAG_BC;
-ctx->hflags |= MIPS_HFLAG_BDS32;
 }
 
 static void gen_msa_i8(DisasContext *ctx)
-- 
2.26.2

[PATCH 04/17] target/mips: Simplify MSA TCG logic

[Philippe Mathieu-Daudé]

Only decode MSA opcodes if MSA is present (implemented).

Now than check_msa_access() will only be called if MSA is
present, the only way to have MIPS_HFLAG_MSA unset is if
MSA is disabled (bit CP0C5_MSAEn cleared, see previous
commit). Therefore we can remove the 'reserved instruction'
exception.

Reviewed-by: Jiaxun Yang 
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index b20cf1b52d9..da0cb98df09 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -28576,13 +28576,8 @@ static inline int check_msa_access(DisasContext *ctx)
 }
 
 if (unlikely(!(ctx->hflags & MIPS_HFLAG_MSA))) {
-if (ctx->insn_flags & ASE_MSA) {
-generate_exception_end(ctx, EXCP_MSADIS);
-return 0;
-} else {
-generate_exception_end(ctx, EXCP_RI);
-return 0;
-}
+generate_exception_end(ctx, EXCP_MSADIS);
+return 0;
 }
 return 1;
 }
@@ -30426,7 +30421,7 @@ static void gen_msa_vec(CPUMIPSState *env, DisasContext 
*ctx)
 static void gen_msa(CPUMIPSState *env, DisasContext *ctx)
 {
 uint32_t opcode = ctx->opcode;
-check_insn(ctx, ASE_MSA);
+
 check_msa_access(ctx);
 
 switch (MASK_MSA_MINOR(opcode)) {
@@ -31073,9 +31068,10 @@ static void decode_opc(CPUMIPSState *env, DisasContext 
*ctx)
 case OPC_BNZ_H:
 case OPC_BNZ_W:
 case OPC_BNZ_D:
-check_insn(ctx, ASE_MSA);
-gen_msa_branch(env, ctx, op1);
-break;
+if (ase_msa_available(env)) {
+gen_msa_branch(env, ctx, op1);
+break;
+}
 default:
 MIPS_INVAL("cp1");
 generate_exception_end(ctx, EXCP_RI);
@@ -31264,7 +31260,9 @@ static void decode_opc(CPUMIPSState *env, DisasContext 
*ctx)
 #endif
 } else {
 /* MDMX: Not implemented. */
-gen_msa(env, ctx);
+if (ase_msa_available(env)) {
+gen_msa(env, ctx);
+}
 }
 break;
 case OPC_PCREL:
-- 
2.26.2

[PATCH 03/17] target/mips: Use CP0_Config3 to set MIPS_HFLAG_MSA

[Philippe Mathieu-Daudé]

MSA presence is expressed by the MSAP bit of CP0_Config3.
We don't need to check anything else.

Reviewed-by: Jiaxun Yang 
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/mips/internal.h b/target/mips/internal.h
index 6b9d1d4b93b..3bd41239b1d 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -379,7 +379,7 @@ static inline void compute_hflags(CPUMIPSState *env)
 env->hflags |= MIPS_HFLAG_COP1X;
 }
 }
-if (env->insn_flags & ASE_MSA) {
+if (ase_msa_available(env)) {
 if (env->CP0_Config5 & (1 << CP0C5_MSAEn)) {
 env->hflags |= MIPS_HFLAG_MSA;
 }
-- 
2.26.2

[PATCH 07/17] target/mips: Extract msa_translate_init() from mips_tcg_init()

[Philippe Mathieu-Daudé]

Extract the logic initialization of the MSA registers from
the generic initialization.

Reviewed-by: Jiaxun Yang 
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.h |  3 +++
 target/mips/translate.c | 33 +++--
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/target/mips/translate.h b/target/mips/translate.h
index dbf7df7ba6d..765018beeea 100644
--- a/target/mips/translate.h
+++ b/target/mips/translate.h
@@ -80,4 +80,7 @@ extern TCGv bcond;
 } \
 } while (0)
 
+/* MSA */
+void msa_translate_init(void);
+
 #endif
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 95d07e837c0..bbe06240510 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -31551,6 +31551,24 @@ void mips_cpu_dump_state(CPUState *cs, FILE *f, int 
flags)
 }
 }
 
+static void msa_translate_init(void)
+{
+int i;
+
+for (i = 0; i < 32; i++) {
+int off = offsetof(CPUMIPSState, active_fpu.fpr[i].wr.d[0]);
+
+/*
+ * The MSA vector registers are mapped on the
+ * scalar floating-point unit (FPU) registers.
+ */
+msa_wr_d[i * 2] = fpu_f64[i];
+off = offsetof(CPUMIPSState, active_fpu.fpr[i].wr.d[1]);
+msa_wr_d[i * 2 + 1] =
+tcg_global_mem_new_i64(cpu_env, off, msaregnames[i * 2 + 1]);
+}
+}
+
 void mips_tcg_init(void)
 {
 int i;
@@ -31566,20 +31584,7 @@ void mips_tcg_init(void)
 
 fpu_f64[i] = tcg_global_mem_new_i64(cpu_env, off, fregnames[i]);
 }
-/* MSA */
-for (i = 0; i < 32; i++) {
-int off = offsetof(CPUMIPSState, active_fpu.fpr[i].wr.d[0]);
-
-/*
- * The MSA vector registers are mapped on the
- * scalar floating-point unit (FPU) registers.
- */
-msa_wr_d[i * 2] = fpu_f64[i];
-off = offsetof(CPUMIPSState, active_fpu.fpr[i].wr.d[1]);
-msa_wr_d[i * 2 + 1] =
-tcg_global_mem_new_i64(cpu_env, off, msaregnames[i * 2 + 1]);
-}
-
+msa_translate_init();
 cpu_PC = tcg_global_mem_new(cpu_env,
 offsetof(CPUMIPSState, active_tc.PC), "PC");
 for (i = 0; i < MIPS_DSP_ACC; i++) {
-- 
2.26.2

[PATCH 02/17] target/mips: Simplify msa_reset()

[Philippe Mathieu-Daudé]

Call msa_reset() unconditionally, but only reset
the MSA registers if MSA is implemented.

Reviewed-by: Jiaxun Yang 
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.c  | 5 +
 target/mips/translate_init.c.inc | 4 
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index cb822e52c4b..b20cf1b52d9 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -31876,10 +31876,7 @@ void cpu_state_reset(CPUMIPSState *env)
 env->hflags |= MIPS_HFLAG_M16;
 }
 
-/* MSA */
-if (ase_msa_available(env)) {
-msa_reset(env);
-}
+msa_reset(env);
 
 compute_hflags(env);
 restore_fp_status(env);
diff --git a/target/mips/translate_init.c.inc b/target/mips/translate_init.c.inc
index cac3d241831..3c9ec7e940a 100644
--- a/target/mips/translate_init.c.inc
+++ b/target/mips/translate_init.c.inc
@@ -1024,6 +1024,10 @@ static void mvp_init(CPUMIPSState *env)
 
 static void msa_reset(CPUMIPSState *env)
 {
+if (!ase_msa_available(env)) {
+return;
+}
+
 #ifdef CONFIG_USER_ONLY
 /* MSA access enabled */
 env->CP0_Config5 |= 1 << CP0C5_MSAEn;
-- 
2.26.2

[PATCH 01/17] target/mips: Introduce ase_msa_available() helper

[Philippe Mathieu-Daudé]

Instead of accessing CP0_Config3 directly and checking
the 'MSA Present' bit, introduce an explicit helper,
making the code easier to read.

Reviewed-by: Jiaxun Yang 
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/cpu.h   |  6 ++
 target/mips/kvm.c   | 12 ++--
 target/mips/translate.c |  8 +++-
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 7b3ff2fd6fb..6d4c8d63930 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1296,6 +1296,12 @@ int cpu_mips_signal_handler(int host_signum, void 
*pinfo, void *puc);
 bool cpu_supports_cps_smp(const char *cpu_type);
 bool cpu_supports_isa(const char *cpu_type, uint64_t isa);
 
+/* Check presence of MSA implementation */
+static inline bool ase_msa_available(CPUMIPSState *env)
+{
+return env->CP0_Config3 & (1 << CP0C3_MSAP);
+}
+
 /* Check presence of multi-threading ASE implementation */
 static inline bool ase_mt_available(CPUMIPSState *env)
 {
diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index 3ca3a0da93f..c511a1303c6 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -82,7 +82,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 }
 }
 
-if (kvm_mips_msa_cap && env->CP0_Config3 & (1 << CP0C3_MSAP)) {
+if (kvm_mips_msa_cap && ase_msa_available(env)) {
 ret = kvm_vcpu_enable_cap(cs, KVM_CAP_MIPS_MSA, 0, 0);
 if (ret < 0) {
 /* mark unsupported so it gets disabled on reset */
@@ -108,7 +108,7 @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu)
 warn_report("KVM does not support FPU, disabling");
 env->CP0_Config1 &= ~(1 << CP0C1_FP);
 }
-if (!kvm_mips_msa_cap && env->CP0_Config3 & (1 << CP0C3_MSAP)) {
+if (!kvm_mips_msa_cap && ase_msa_available(env)) {
 warn_report("KVM does not support MSA, disabling");
 env->CP0_Config3 &= ~(1 << CP0C3_MSAP);
 }
@@ -621,7 +621,7 @@ static int kvm_mips_put_fpu_registers(CPUState *cs, int 
level)
  * FPU register state is a subset of MSA vector state, so don't put FPU
  * registers if we're emulating a CPU with MSA.
  */
-if (!(env->CP0_Config3 & (1 << CP0C3_MSAP))) {
+if (!ase_msa_available(env)) {
 /* Floating point registers */
 for (i = 0; i < 32; ++i) {
 if (env->CP0_Status & (1 << CP0St_FR)) {
@@ -640,7 +640,7 @@ static int kvm_mips_put_fpu_registers(CPUState *cs, int 
level)
 }
 
 /* Only put MSA state if we're emulating a CPU with MSA */
-if (env->CP0_Config3 & (1 << CP0C3_MSAP)) {
+if (ase_msa_available(env)) {
 /* MSA Control Registers */
 if (level == KVM_PUT_FULL_STATE) {
 err = kvm_mips_put_one_reg(cs, KVM_REG_MIPS_MSA_IR,
@@ -701,7 +701,7 @@ static int kvm_mips_get_fpu_registers(CPUState *cs)
  * FPU register state is a subset of MSA vector state, so don't save 
FPU
  * registers if we're emulating a CPU with MSA.
  */
-if (!(env->CP0_Config3 & (1 << CP0C3_MSAP))) {
+if (!ase_msa_available(env)) {
 /* Floating point registers */
 for (i = 0; i < 32; ++i) {
 if (env->CP0_Status & (1 << CP0St_FR)) {
@@ -720,7 +720,7 @@ static int kvm_mips_get_fpu_registers(CPUState *cs)
 }
 
 /* Only get MSA state if we're emulating a CPU with MSA */
-if (env->CP0_Config3 & (1 << CP0C3_MSAP)) {
+if (ase_msa_available(env)) {
 /* MSA Control Registers */
 err = kvm_mips_get_one_reg(cs, KVM_REG_MIPS_MSA_IR,
>msair);
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 80c9c17819f..cb822e52c4b 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -24928,8 +24928,7 @@ static void decode_opc_special(CPUMIPSState *env, 
DisasContext *ctx)
 gen_trap(ctx, op1, rs, rt, -1);
 break;
 case OPC_LSA: /* OPC_PMON */
-if ((ctx->insn_flags & ISA_MIPS32R6) ||
-(env->CP0_Config3 & (1 << CP0C3_MSAP))) {
+if ((ctx->insn_flags & ISA_MIPS32R6) || ase_msa_available(env)) {
 decode_opc_special_r6(env, ctx);
 } else {
 /* Pmon entry point, also R4010 selsl */
@@ -25031,8 +25030,7 @@ static void decode_opc_special(CPUMIPSState *env, 
DisasContext *ctx)
 }
 break;
 case OPC_DLSA:
-if ((ctx->insn_flags & ISA_MIPS32R6) ||
-(env->CP0_Config3 & (1 << CP0C3_MSAP))) {
+if ((ctx->insn_flags & ISA_MIPS32R6) || ase_msa_available(env)) {
 decode_opc_special_r6(env, ctx);
 }
 break;
@@ -31879,7 +31877,7 @@ void cpu_state_reset(CPUMIPSState *env)
 }
 
 /* MSA */
-if (env->CP0_Config3 & (1 << CP0C3_MSAP)) {
+if (ase_msa_available(env)) {
 msa_reset(env);
 }
 
-- 
2.26.2

[PATCH 00/17] target/mips: Convert MSA ASE to decodetree

[Philippe Mathieu-Daudé]

Finally, we use decodetree with the MIPS target.

Starting easy with the MSA ASE. 2700+ lines extracted
from helper.h and translate.c, now built as an new
object: mod-msa_translate.o.

While the diff stat is positive by 86 lines, we actually
(re)moved code, but added (C) notices.

The most interesting patches are the 2 last ones.

Please review,

Phil.

Based-on: <20201207224335.4030582-1-f4...@amsat.org>
(linux-user: Rework get_elf_hwcap() and support MIPS Loongson 2F/3A)
Based-on: <20201207235539.4070364-1-f4...@amsat.org>
(target/mips: Add translate.h and fpu_translate.h headers)

Philippe Mathieu-Daudé (17):
  target/mips: Introduce ase_msa_available() helper
  target/mips: Simplify msa_reset()
  target/mips: Use CP0_Config3 to set MIPS_HFLAG_MSA
  target/mips: Simplify MSA TCG logic
  target/mips: Remove now unused ASE_MSA definition
  target/mips: Alias MSA vector registers on FPU scalar registers
  target/mips: Extract msa_translate_init() from mips_tcg_init()
  target/mips: Remove CPUMIPSState* argument from gen_msa*() methods
  target/mips: Explode gen_msa_branch() as gen_msa_BxZ_V/BxZ()
  target/mips: Rename msa_helper.c as mod-msa_helper.c
  target/mips: Move msa_reset() to mod-msa_helper.c
  target/mips: Extract MSA helpers from op_helper.c
  target/mips: Extract MSA helper definitions
  target/mips: Declare gen_msa/_branch() in 'translate.h'
  target/mips: Extract MSA translation routines
  target/mips: Introduce decode tree bindings for MSA opcodes
  target/mips: Use decode_msa32() generated from decodetree

 target/mips/cpu.h |6 +
 target/mips/fpu_translate.h   |   10 -
 target/mips/helper.h  |  436 +---
 target/mips/internal.h|4 +-
 target/mips/mips-defs.h   |1 -
 target/mips/translate.h   |4 +
 target/mips/mod-msa32.decode  |   24 +
 target/mips/kvm.c |   12 +-
 .../mips/{msa_helper.c => mod-msa_helper.c}   |  429 
 target/mips/mod-msa_translate.c   | 2270 +
 target/mips/op_helper.c   |  394 ---
 target/mips/translate.c   | 2264 +---
 target/mips/meson.build   |9 +-
 target/mips/mod-msa_helper.h.inc  |  443 
 target/mips/translate_init.c.inc  |   38 +-
 15 files changed, 3215 insertions(+), 3129 deletions(-)
 create mode 100644 target/mips/mod-msa32.decode
 rename target/mips/{msa_helper.c => mod-msa_helper.c} (93%)
 create mode 100644 target/mips/mod-msa_translate.c
 create mode 100644 target/mips/mod-msa_helper.h.inc

-- 
2.26.2

Re: [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure

[John Snow]

On 11/16/20 5:12 AM, Markus Armbruster wrote:

John Snow  writes:


This replaces _make_tree with Annotated(). By creating it as a generic
container, we can more accurately describe the exact nature of this
particular value. i.e., each Annotated object is actually an
Annotated, describing its contained value.

This adds stricter typing to Annotated nodes and extra annotated
information.


Inhowfar?



The Generic[T] trick lets us express the type of the annotated node 
itself, which is more specific than Tuple[_something, ...etc...] and 
this type can be preserved when we peel the annotations off.


It's not super crucial, but like you say, the big benefit is the field 
names and strict types for the special-purpose structure.



  It also replaces a check of "isinstance tuple" with the
much more explicit "isinstance Annotated" which is guaranteed not to
break if a tuple is accidentally introduced into the type tree. (Perhaps
as a result of a bad conversion from a list.)


Sure this is worth writing home about?  Such accidents seem quite
unlikely.



We all have our phobias. I find "isinstance(x, 
extremely_common_stdlib_type)" to be extremely fragile and likely to 
frustrate.


Maybe what's unlikely is anyone editing this code ever again. You've 
mentioned wanting to look into changing how the schema information is 
stored in QEMU before, so a lot of this might not matter for too much 
longer, who knows.



For me, the commit's benefit is making the structure of the annotated
tree node more explicit (your first paragraph, I guess).  It's a bit of
a pattern in developing Python code: we start with a Tuple because it's
terse and easy, then things get more complex, terse becomes too terse,
and we're replacing the Tuple with a class.



Yep.


Signed-off-by: John Snow 
---
  scripts/qapi/introspect.py | 97 +++---
  1 file changed, 48 insertions(+), 49 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index a0978cb3adb..a261e402d69 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -13,12 +13,13 @@
  from typing import (
  Any,
  Dict,
+Generic,
+Iterable,
  List,
  Optional,
  Sequence,
-Tuple,
+TypeVar,
  Union,
-cast,
  )
  
  from .common import (

@@ -63,50 +64,48 @@
  _scalar = Union[str, bool, None]
  _nonscalar = Union[Dict[str, _stub], List[_stub]]
  _value = Union[_scalar, _nonscalar]
-TreeValue = Union[_value, 'Annotated']
+TreeValue = Union[_value, 'Annotated[_value]']
  
  # This is just an alias for an object in the structure described above:

  _DObject = Dict[str, object]
  
-# Represents the annotations themselves:

-Annotations = Dict[str, object]
  
-# Represents an annotated node (of some kind).

-Annotated = Tuple[_value, Annotations]
+_AnnoType = TypeVar('_AnnoType', bound=TreeValue)
  
  
-def _make_tree(obj: Union[_DObject, str], ifcond: List[str],

-   comment: Optional[str] = None) -> Annotated:
-extra: Annotations = {
-'if': ifcond,
-'comment': comment,
-}
-return (obj, extra)
+class Annotated(Generic[_AnnoType]):
+"""
+Annotated generally contains a SchemaInfo-like type (as a dict),
+But it also used to wrap comments/ifconds around scalar leaf values,
+for the benefit of features and enums.
+"""
+# Remove after 3.7 adds @dataclass:
+# pylint: disable=too-few-public-methods
+def __init__(self, value: _AnnoType, ifcond: Iterable[str],
+ comment: Optional[str] = None):
+self.value = value
+self.comment: Optional[str] = comment
+self.ifcond: Sequence[str] = tuple(ifcond)
  
  
-def _tree_to_qlit(obj: TreeValue,

-  level: int = 0,
+def _tree_to_qlit(obj: TreeValue, level: int = 0,
suppress_first_indent: bool = False) -> str:
  
  def indent(level: int) -> str:

  return level * 4 * ' '
  
-if isinstance(obj, tuple):

-ifobj, extra = obj
-ifcond = cast(Optional[Sequence[str]], extra.get('if'))
-comment = extra.get('comment')
-
+if isinstance(obj, Annotated):
  msg = "Comments and Conditionals not implemented for dict values"
-assert not (suppress_first_indent and (ifcond or comment)), msg
+assert not (suppress_first_indent and (obj.comment or obj.ifcond)), msg
  
  ret = ''

-if comment:
-ret += indent(level) + '/* %s */\n' % comment
-if ifcond:
-ret += gen_if(ifcond)
-ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
-if ifcond:
-ret += '\n' + gen_endif(ifcond)
+if obj.comment:
+ret += indent(level) + '/* %s */\n' % obj.comment
+if obj.ifcond:
+ret += gen_if(obj.ifcond)
+ret += _tree_to_qlit(obj.value, level, suppress_first_indent)
+if obj.ifcond:
+ret += '\n' + 

Re: [PATCH v2 08/11] qapi/introspect.py: replace 'extra' dict with 'comment' argument

[John Snow]

On 11/16/20 4:55 AM, Markus Armbruster wrote:

John Snow  writes:


This is only used to pass in a dictionary with a comment already set, so
skip the runaround and just accept the comment.

Signed-off-by: John Snow 
---
  scripts/qapi/introspect.py | 17 -
  1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index ef469b6c06e..a0978cb3adb 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -76,12 +76,11 @@
  
  
  def _make_tree(obj: Union[_DObject, str], ifcond: List[str],

-   extra: Optional[Annotations] = None
-   ) -> Annotated:
-if extra is None:
-extra = {}
-if ifcond:
-extra['if'] = ifcond
+   comment: Optional[str] = None) -> Annotated:
+extra: Annotations = {
+'if': ifcond,
+'comment': comment,
+}


Works because _tree_to_qlit() treats 'if': None, 'comment': None exactly
like absent 'if', 'comment'.  Mentioning this in the commit message
wouldn't hurt.



OK.


We create even more dicts now.  Okay.



It's just shuffling around where this dict is made, I don't think we 
create more in total.



  return (obj, extra)
  
  
@@ -228,18 +227,18 @@ def _gen_features(cls,

  def _gen_tree(self, name: str, mtype: str, obj: _DObject,
ifcond: List[str],
features: Optional[List[QAPISchemaFeature]]) -> None:
-extra: Optional[Annotations] = None
+comment: Optional[str] = None
  if mtype not in ('command', 'event', 'builtin', 'array'):
  if not self._unmask:
  # Output a comment to make it easy to map masked names
  # back to the source when reading the generated output.
-extra = {'comment': '"%s" = %s' % (self._name(name), name)}
+comment = f'"{self._name(name)}" = {name}'


Drive-by modernization, fine with me.  Aside: many more opportunities; a
systematic hunt is called for.  Not now.



It happened because of line-length limits, admittedly.


  name = self._name(name)
  obj['name'] = name
  obj['meta-type'] = mtype
  if features:
  obj['features'] = self._gen_features(features)
-self._trees.append(_make_tree(obj, ifcond, extra))
+self._trees.append(_make_tree(obj, ifcond, comment))
  
  def _gen_member(self,

  member: QAPISchemaObjectTypeMember) -> Annotated:

Re: [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree()

[John Snow]

On 11/16/20 4:46 AM, Markus Armbruster wrote:

John Snow  writes:


Returning two different types conditionally can be complicated to
type. Let's always return a tuple for consistency. Prohibit the use of
annotations with dict-values in this circumstance. It can be implemented
later if and when the need for it arises.

Signed-off-by: John Snow 
---
  scripts/qapi/introspect.py | 21 -
  1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 16282f2634b..ef469b6c06e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -77,14 +77,12 @@
  
  def _make_tree(obj: Union[_DObject, str], ifcond: List[str],

 extra: Optional[Annotations] = None
-   ) -> TreeValue:
+   ) -> Annotated:
  if extra is None:
  extra = {}
  if ifcond:
  extra['if'] = ifcond
-if extra:
-return (obj, extra)
-return obj
+return (obj, extra)


Less efficient, but that's okay.



I have bad news about Python :)

  
  
  def _tree_to_qlit(obj: TreeValue,

@@ -98,12 +96,16 @@ def indent(level: int) -> str:
  ifobj, extra = obj
  ifcond = cast(Optional[Sequence[str]], extra.get('if'))
  comment = extra.get('comment')
+
+msg = "Comments and Conditionals not implemented for dict values"
+assert not (suppress_first_indent and (ifcond or comment)), msg


What exactly does this assertion guard?



Something that Eduardo noticed in his review. It's ugly, and I explained 
it poorly.


We don't support annotations on dict *values*, basically. When this 
function is called with suppress_first_indent, we know that we are being 
called to process a dict *value* and not a dict *key*.


What do you do with comments or conditionals on just one half of a key: 
value pair?


Well, break.


+
  ret = ''
  if comment:
  ret += indent(level) + '/* %s */\n' % comment
  if ifcond:
  ret += gen_if(ifcond)
-ret += _tree_to_qlit(ifobj, level)
+ret += _tree_to_qlit(ifobj, level, suppress_first_indent)


Why do you need to pass on @suppress_first_indent now?



We either never should or we always should have. This is just in the 
case that "suppress first indent" is used on an annotated node. Which, 
err, for the annotations we actually support right now (comment, ifcond) 
-- we will reject in this case.


But it felt precarious...


  if ifcond:
  ret += '\n' + gen_endif(ifcond)
  return ret
@@ -152,7 +154,7 @@ def __init__(self, prefix: str, unmask: bool):
  ' * QAPI/QMP schema introspection', __doc__)
  self._unmask = unmask
  self._schema: Optional[QAPISchema] = None
-self._trees: List[TreeValue] = []
+self._trees: List[Annotated] = []
  self._used_types: List[QAPISchemaType] = []
  self._name_map: Dict[str, str] = {}
  self._genc.add(mcgen('''
@@ -219,7 +221,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
  
  @classmethod

  def _gen_features(cls,
-  features: List[QAPISchemaFeature]) -> List[TreeValue]:
+  features: List[QAPISchemaFeature]
+  ) -> List[Annotated]:
  return [_make_tree(f.name, f.ifcond) for f in features]
  
  def _gen_tree(self, name: str, mtype: str, obj: _DObject,

@@ -239,7 +242,7 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
  self._trees.append(_make_tree(obj, ifcond, extra))
  
  def _gen_member(self,

-member: QAPISchemaObjectTypeMember) -> TreeValue:
+member: QAPISchemaObjectTypeMember) -> Annotated:
  obj: _DObject = {
  'name': member.name,
  'type': self._use_type(member.type)
@@ -255,7 +258,7 @@ def _gen_variants(self, tag_name: str,
  return {'tag': tag_name,
  'variants': [self._gen_variant(v) for v in variants]}
  
-def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:

+def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
  obj: _DObject = {
  'case': variant.name,
  'type': self._use_type(variant.type)

[PATCH 7/7] target/mips: Extract FPU specific definitions to fpu_translate.h

[Philippe Mathieu-Daudé]

Extract FPU specific definitions that can be used by
ISA / ASE / extensions to fpu_translate.h header.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/fpu_translate.h | 71 +
 target/mips/translate.c | 70 
 2 files changed, 71 insertions(+), 70 deletions(-)

diff --git a/target/mips/fpu_translate.h b/target/mips/fpu_translate.h
index 430e0b77537..f45314d2ec2 100644
--- a/target/mips/fpu_translate.h
+++ b/target/mips/fpu_translate.h
@@ -12,6 +12,77 @@
 #include "exec/translator.h"
 #include "translate.h"
 
+#define OPC_CP1 (0x11 << 26)
+
+/* Coprocessor 1 (rs field) */
+#define MASK_CP1(op)(MASK_OP_MAJOR(op) | (op & (0x1F << 21)))
+
+/* Values for the fmt field in FP instructions */
+enum {
+/* 0 - 15 are reserved */
+FMT_S = 16,  /* single fp */
+FMT_D = 17,  /* double fp */
+FMT_E = 18,  /* extended fp */
+FMT_Q = 19,  /* quad fp */
+FMT_W = 20,  /* 32-bit fixed */
+FMT_L = 21,  /* 64-bit fixed */
+FMT_PS = 22, /* paired single fp */
+/* 23 - 31 are reserved */
+};
+
+enum {
+OPC_MFC1 = (0x00 << 21) | OPC_CP1,
+OPC_DMFC1= (0x01 << 21) | OPC_CP1,
+OPC_CFC1 = (0x02 << 21) | OPC_CP1,
+OPC_MFHC1= (0x03 << 21) | OPC_CP1,
+OPC_MTC1 = (0x04 << 21) | OPC_CP1,
+OPC_DMTC1= (0x05 << 21) | OPC_CP1,
+OPC_CTC1 = (0x06 << 21) | OPC_CP1,
+OPC_MTHC1= (0x07 << 21) | OPC_CP1,
+OPC_BC1  = (0x08 << 21) | OPC_CP1, /* bc */
+OPC_BC1ANY2  = (0x09 << 21) | OPC_CP1,
+OPC_BC1ANY4  = (0x0A << 21) | OPC_CP1,
+OPC_BZ_V = (0x0B << 21) | OPC_CP1,
+OPC_BNZ_V= (0x0F << 21) | OPC_CP1,
+OPC_S_FMT= (FMT_S << 21) | OPC_CP1,
+OPC_D_FMT= (FMT_D << 21) | OPC_CP1,
+OPC_E_FMT= (FMT_E << 21) | OPC_CP1,
+OPC_Q_FMT= (FMT_Q << 21) | OPC_CP1,
+OPC_W_FMT= (FMT_W << 21) | OPC_CP1,
+OPC_L_FMT= (FMT_L << 21) | OPC_CP1,
+OPC_PS_FMT   = (FMT_PS << 21) | OPC_CP1,
+OPC_BC1EQZ   = (0x09 << 21) | OPC_CP1,
+OPC_BC1NEZ   = (0x0D << 21) | OPC_CP1,
+OPC_BZ_B = (0x18 << 21) | OPC_CP1,
+OPC_BZ_H = (0x19 << 21) | OPC_CP1,
+OPC_BZ_W = (0x1A << 21) | OPC_CP1,
+OPC_BZ_D = (0x1B << 21) | OPC_CP1,
+OPC_BNZ_B= (0x1C << 21) | OPC_CP1,
+OPC_BNZ_H= (0x1D << 21) | OPC_CP1,
+OPC_BNZ_W= (0x1E << 21) | OPC_CP1,
+OPC_BNZ_D= (0x1F << 21) | OPC_CP1,
+};
+
+#define MASK_CP1_FUNC(op)   (MASK_CP1(op) | (op & 0x3F))
+#define MASK_BC1(op)(MASK_CP1(op) | (op & (0x3 << 16)))
+
+enum {
+OPC_BC1F = (0x00 << 16) | OPC_BC1,
+OPC_BC1T = (0x01 << 16) | OPC_BC1,
+OPC_BC1FL= (0x02 << 16) | OPC_BC1,
+OPC_BC1TL= (0x03 << 16) | OPC_BC1,
+};
+
+enum {
+OPC_BC1FANY2 = (0x00 << 16) | OPC_BC1ANY2,
+OPC_BC1TANY2 = (0x01 << 16) | OPC_BC1ANY2,
+};
+
+enum {
+OPC_BC1FANY4 = (0x00 << 16) | OPC_BC1ANY4,
+OPC_BC1TANY4 = (0x01 << 16) | OPC_BC1ANY4,
+};
+
 extern TCGv_i32 fpu_fcr0, fpu_fcr31;
 extern TCGv_i64 fpu_f64[32];
 
diff --git a/target/mips/translate.c b/target/mips/translate.c
index bc54eb58c70..80c9c17819f 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -49,7 +49,6 @@ enum {
 OPC_SPECIAL  = (0x00 << 26),
 OPC_REGIMM   = (0x01 << 26),
 OPC_CP0  = (0x10 << 26),
-OPC_CP1  = (0x11 << 26),
 OPC_CP2  = (0x12 << 26),
 OPC_CP3  = (0x13 << 26),
 OPC_SPECIAL2 = (0x1C << 26),
@@ -1002,75 +1001,6 @@ enum {
 OPC_WAIT = 0x20 | OPC_C0,
 };
 
-/* Coprocessor 1 (rs field) */
-#define MASK_CP1(op)(MASK_OP_MAJOR(op) | (op & (0x1F << 21)))
-
-/* Values for the fmt field in FP instructions */
-enum {
-/* 0 - 15 are reserved */
-FMT_S = 16,  /* single fp */
-FMT_D = 17,  /* double fp */
-FMT_E = 18,  /* extended fp */
-FMT_Q = 19,  /* quad fp */
-FMT_W = 20,  /* 32-bit fixed */
-FMT_L = 21,  /* 64-bit fixed */
-FMT_PS = 22, /* paired single fp */
-/* 23 - 31 are reserved */
-};
-
-enum {
-OPC_MFC1 = (0x00 << 21) | OPC_CP1,
-OPC_DMFC1= (0x01 << 21) | OPC_CP1,
-OPC_CFC1 = (0x02 << 21) | OPC_CP1,
-OPC_MFHC1= (0x03 << 21) | OPC_CP1,
-OPC_MTC1 = (0x04 << 21) | OPC_CP1,
-OPC_DMTC1= (0x05 << 21) | OPC_CP1,
-OPC_CTC1 = (0x06 << 21) | OPC_CP1,
-OPC_MTHC1= (0x07 << 21) | OPC_CP1,
-OPC_BC1  = (0x08 << 21) | OPC_CP1, /* bc */
-OPC_BC1ANY2  = (0x09 << 21) | OPC_CP1,
-OPC_BC1ANY4  = (0x0A << 21) | OPC_CP1,
-OPC_BZ_V = (0x0B << 21) | OPC_CP1,
-OPC_BNZ_V= (0x0F << 21) | OPC_CP1,
-OPC_S_FMT= (FMT_S << 21) | OPC_CP1,
-OPC_D_FMT= (FMT_D << 21) | OPC_CP1,
-OPC_E_FMT= (FMT_E << 21) | OPC_CP1,
-OPC_Q_FMT= (FMT_Q << 21) | OPC_CP1,
-OPC_W_FMT= 

Re: [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper

[John Snow]

On 11/16/20 3:47 AM, Markus Armbruster wrote:

John Snow  writes:


_make_tree might receive a dict or some other type.


Are you talking about @obj?



Yes. It *usually* takes a dict. sometimes it doesn't.


 Adding features
information should arguably be performed by the caller at such a time
when we know the type of the object and don't have to re-interrogate it.


Fair enough.  There are just two such callers anyway.


Signed-off-by: John Snow 
---
  scripts/qapi/introspect.py | 19 ---
  1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 803288a64e7..16282f2634b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -76,16 +76,12 @@
  
  
  def _make_tree(obj: Union[_DObject, str], ifcond: List[str],

-   features: List[QAPISchemaFeature],
 extra: Optional[Annotations] = None
 ) -> TreeValue:
  if extra is None:
  extra = {}
  if ifcond:
  extra['if'] = ifcond
-if features:
-assert isinstance(obj, dict)
-obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
  if extra:
  return (obj, extra)
  return obj
@@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:
  return '[' + self._use_type(typ.element_type) + ']'
  return self._name(typ.name)
  
+@classmethod

+def _gen_features(cls,
+  features: List[QAPISchemaFeature]) -> List[TreeValue]:
+return [_make_tree(f.name, f.ifcond) for f in features]
+


Ignorant question: when to use @classmethod, and when to use
@staticmethod?



Matter of taste. My preference is to just always use @classmethod, 
because they can be extended or referenced by subclasses.


@staticmethod does not take a class argument, @classmethod does. Static 
methods therefore cannot address any other classmethods, but a 
classmethod can.


I just always reach for classmethod by default.


  def _gen_tree(self, name: str, mtype: str, obj: _DObject,
ifcond: List[str],
features: Optional[List[QAPISchemaFeature]]) -> None:
@@ -233,7 +234,9 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
  name = self._name(name)
  obj['name'] = name
  obj['meta-type'] = mtype
-self._trees.append(_make_tree(obj, ifcond, features, extra))
+if features:
+obj['features'] = self._gen_features(features)
+self._trees.append(_make_tree(obj, ifcond, extra))
  
  def _gen_member(self,

  member: QAPISchemaObjectTypeMember) -> TreeValue:


No change when not features.  Else, you change

 obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]

to

 obj['features'] = [_make_tree(f.name, f.ifcond) for f in features]



Yep. I am consolidating the logic for (node, annotation) in so doing.


where

 _make_tree(f.name, f.ifcond)
 = (f.name, {'if': f.ifcond})   if f.ifcond
 = f.name   else

Works, and feels less lazy.  However, the commit message did not prepare
me for this.  If you split this off into its own patch, you can describe
it properly.



OK.


@@ -243,7 +246,9 @@ def _gen_member(self,
  }
  if member.optional:
  obj['default'] = None
-return _make_tree(obj, member.ifcond, member.features)
+if member.features:
+obj['features'] = self._gen_features(member.features)
+return _make_tree(obj, member.ifcond)
  
  def _gen_variants(self, tag_name: str,

variants: List[QAPISchemaVariant]) -> _DObject:
@@ -255,7 +260,7 @@ def _gen_variant(self, variant: QAPISchemaVariant) -> 
TreeValue:
  'case': variant.name,
  'type': self._use_type(variant.type)
  }
-return _make_tree(obj, variant.ifcond, None)
+return _make_tree(obj, variant.ifcond)
  
  def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],

 json_type: str) -> None:

[PATCH 6/7] target/mips: Declare generic FPU functions in 'fpu_translate.h'

[Philippe Mathieu-Daudé]

Some FPU translation functions / registers can be used by
ISA / ASE / extensions out of the big translate.c file.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/fpu_translate.h | 25 +
 target/mips/translate.c | 14 --
 2 files changed, 33 insertions(+), 6 deletions(-)
 create mode 100644 target/mips/fpu_translate.h

diff --git a/target/mips/fpu_translate.h b/target/mips/fpu_translate.h
new file mode 100644
index 000..430e0b77537
--- /dev/null
+++ b/target/mips/fpu_translate.h
@@ -0,0 +1,25 @@
+/*
+ * FPU-related MIPS translation routines.
+ *
+ *  Copyright (C) 2004-2005  Jocelyn Mayer
+ *  Copyright (c) 2006 Marius Groeger (FPU operations)
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+#ifndef TARGET_MIPS_FPU_TRANSLATE_H
+#define TARGET_MIPS_FPU_TRANSLATE_H
+
+#include "exec/translator.h"
+#include "translate.h"
+
+extern TCGv_i32 fpu_fcr0, fpu_fcr31;
+extern TCGv_i64 fpu_f64[32];
+
+void gen_load_fpr64(DisasContext *ctx, TCGv_i64 t, int reg);
+void gen_store_fpr64(DisasContext *ctx, TCGv_i64 t, int reg);
+
+int get_fp_bit(int cc);
+
+void check_cp1_enabled(DisasContext *ctx);
+
+#endif
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 6614512a828..bc54eb58c70 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -40,7 +40,9 @@
 #include "exec/log.h"
 #include "qemu/qemu-print.h"
 #include "fpu_helper.h"
+
 #include "translate.h"
+#include "fpu_translate.h"
 
 enum {
 /* indirect opcode tables */
@@ -2496,8 +2498,8 @@ static TCGv cpu_dspctrl, btarget;
 TCGv bcond;
 static TCGv cpu_lladdr, cpu_llval;
 static TCGv_i32 hflags;
-static TCGv_i32 fpu_fcr0, fpu_fcr31;
-static TCGv_i64 fpu_f64[32];
+TCGv_i32 fpu_fcr0, fpu_fcr31;
+TCGv_i64 fpu_f64[32];
 static TCGv_i64 msa_wr_d[64];
 
 #if defined(TARGET_MIPS64)
@@ -2813,7 +2815,7 @@ static void gen_store_fpr32h(DisasContext *ctx, TCGv_i32 
t, int reg)
 }
 }
 
-static void gen_load_fpr64(DisasContext *ctx, TCGv_i64 t, int reg)
+void gen_load_fpr64(DisasContext *ctx, TCGv_i64 t, int reg)
 {
 if (ctx->hflags & MIPS_HFLAG_F64) {
 tcg_gen_mov_i64(t, fpu_f64[reg]);
@@ -2822,7 +2824,7 @@ static void gen_load_fpr64(DisasContext *ctx, TCGv_i64 t, 
int reg)
 }
 }
 
-static void gen_store_fpr64(DisasContext *ctx, TCGv_i64 t, int reg)
+void gen_store_fpr64(DisasContext *ctx, TCGv_i64 t, int reg)
 {
 if (ctx->hflags & MIPS_HFLAG_F64) {
 tcg_gen_mov_i64(fpu_f64[reg], t);
@@ -2836,7 +2838,7 @@ static void gen_store_fpr64(DisasContext *ctx, TCGv_i64 
t, int reg)
 }
 }
 
-static inline int get_fp_bit(int cc)
+int get_fp_bit(int cc)
 {
 if (cc) {
 return 24 + cc;
@@ -2911,7 +2913,7 @@ static inline void check_cp0_enabled(DisasContext *ctx)
 }
 }
 
-static inline void check_cp1_enabled(DisasContext *ctx)
+void check_cp1_enabled(DisasContext *ctx)
 {
 if (unlikely(!(ctx->hflags & MIPS_HFLAG_FPU))) {
 generate_exception_err(ctx, EXCP_CpU, 1);
-- 
2.26.2

[PATCH 5/7] target/mips/fpu_helper: Remove unused headers

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/fpu_helper.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/target/mips/fpu_helper.c b/target/mips/fpu_helper.c
index 7d949cd8e3a..a3c05160b35 100644
--- a/target/mips/fpu_helper.c
+++ b/target/mips/fpu_helper.c
@@ -21,15 +21,11 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "internal.h"
-#include "qemu/host-utils.h"
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
-#include "exec/memop.h"
-#include "sysemu/kvm.h"
 #include "fpu/softfloat.h"
 #include "fpu_helper.h"
 
-- 
2.26.2

[PATCH 3/7] target/mips: Use FloatRoundMode enum for FCR31 modes conversion

[Philippe Mathieu-Daudé]

Use the FloatRoundMode enum type introduced in commit 3dede407cc6
("softfloat: Name rounding mode enum") instead of 'unsigned int'.

Suggested-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
Message-Id: <20201123204448.3260804-2-f4...@amsat.org>
---
 target/mips/internal.h   | 3 ++-
 target/mips/fpu_helper.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/mips/internal.h b/target/mips/internal.h
index d290c1afe30..5d8a8a1838e 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -226,7 +226,8 @@ bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
 uint32_t float_class_s(uint32_t arg, float_status *fst);
 uint64_t float_class_d(uint64_t arg, float_status *fst);
 
-extern unsigned int ieee_rm[];
+extern const FloatRoundMode ieee_rm[4];
+
 void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask);
 
 static inline void restore_rounding_mode(CPUMIPSState *env)
diff --git a/target/mips/fpu_helper.c b/target/mips/fpu_helper.c
index 020b768e87b..501bd401a16 100644
--- a/target/mips/fpu_helper.c
+++ b/target/mips/fpu_helper.c
@@ -42,7 +42,7 @@
 #define FP_TO_INT64_OVERFLOW 0x7fffULL
 
 /* convert MIPS rounding mode in FCR31 to IEEE library */
-unsigned int ieee_rm[] = {
+const FloatRoundMode ieee_rm[4] = {
 float_round_nearest_even,
 float_round_to_zero,
 float_round_up,
-- 
2.26.2

[PATCH 4/7] target/mips: Extract FPU helpers to 'fpu_helper.h'

[Philippe Mathieu-Daudé]

Extract FPU specific helpers from "internal.h" to "fpu_helper.h".

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20201120210844.2625602-2-f4...@amsat.org>
---
 target/mips/fpu_helper.h   | 59 ++
 target/mips/internal.h | 50 
 linux-user/mips/cpu_loop.c |  1 +
 target/mips/fpu_helper.c   |  1 +
 target/mips/gdbstub.c  |  1 +
 target/mips/kvm.c  |  1 +
 target/mips/machine.c  |  1 +
 target/mips/msa_helper.c   |  1 +
 target/mips/op_helper.c|  1 +
 target/mips/translate.c|  1 +
 10 files changed, 67 insertions(+), 50 deletions(-)
 create mode 100644 target/mips/fpu_helper.h

diff --git a/target/mips/fpu_helper.h b/target/mips/fpu_helper.h
new file mode 100644
index 000..1c2d6d35a71
--- /dev/null
+++ b/target/mips/fpu_helper.h
@@ -0,0 +1,59 @@
+/*
+ * Helpers for emulation of FPU-related MIPS instructions.
+ *
+ *  Copyright (C) 2004-2005  Jocelyn Mayer
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+#include "fpu/softfloat-helpers.h"
+#include "cpu.h"
+
+extern const FloatRoundMode ieee_rm[4];
+
+uint32_t float_class_s(uint32_t arg, float_status *fst);
+uint64_t float_class_d(uint64_t arg, float_status *fst);
+
+static inline void restore_rounding_mode(CPUMIPSState *env)
+{
+set_float_rounding_mode(ieee_rm[env->active_fpu.fcr31 & 3],
+>active_fpu.fp_status);
+}
+
+static inline void restore_flush_mode(CPUMIPSState *env)
+{
+set_flush_to_zero((env->active_fpu.fcr31 & (1 << FCR31_FS)) != 0,
+  >active_fpu.fp_status);
+}
+
+static inline void restore_snan_bit_mode(CPUMIPSState *env)
+{
+set_snan_bit_is_one((env->active_fpu.fcr31 & (1 << FCR31_NAN2008)) == 0,
+>active_fpu.fp_status);
+}
+
+static inline void restore_fp_status(CPUMIPSState *env)
+{
+restore_rounding_mode(env);
+restore_flush_mode(env);
+restore_snan_bit_mode(env);
+}
+
+/* MSA */
+
+enum CPUMIPSMSADataFormat {
+DF_BYTE = 0,
+DF_HALF,
+DF_WORD,
+DF_DOUBLE
+};
+
+static inline void restore_msa_fp_status(CPUMIPSState *env)
+{
+float_status *status = >active_tc.msa_fp_status;
+int rounding_mode = (env->active_tc.msacsr & MSACSR_RM_MASK) >> MSACSR_RM;
+bool flush_to_zero = (env->active_tc.msacsr & MSACSR_FS_MASK) != 0;
+
+set_float_rounding_mode(ieee_rm[rounding_mode], status);
+set_flush_to_zero(flush_to_zero, status);
+set_flush_inputs_to_zero(flush_to_zero, status);
+}
diff --git a/target/mips/internal.h b/target/mips/internal.h
index 5d8a8a1838e..6b9d1d4b93b 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -8,8 +8,6 @@
 #ifndef MIPS_INTERNAL_H
 #define MIPS_INTERNAL_H
 
-#include "fpu/softfloat-helpers.h"
-
 /*
  * MMU types, the first four entries have the same layout as the
  * CP0C0_MT field.
@@ -74,13 +72,6 @@ struct mips_def_t {
 extern const struct mips_def_t mips_defs[];
 extern const int mips_defs_number;
 
-enum CPUMIPSMSADataFormat {
-DF_BYTE = 0,
-DF_HALF,
-DF_WORD,
-DF_DOUBLE
-};
-
 void mips_cpu_do_interrupt(CPUState *cpu);
 bool mips_cpu_exec_interrupt(CPUState *cpu, int int_req);
 void mips_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
@@ -223,49 +214,8 @@ bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
bool probe, uintptr_t retaddr);
 
 /* op_helper.c */
-uint32_t float_class_s(uint32_t arg, float_status *fst);
-uint64_t float_class_d(uint64_t arg, float_status *fst);
-
-extern const FloatRoundMode ieee_rm[4];
-
 void update_pagemask(CPUMIPSState *env, target_ulong arg1, int32_t *pagemask);
 
-static inline void restore_rounding_mode(CPUMIPSState *env)
-{
-set_float_rounding_mode(ieee_rm[env->active_fpu.fcr31 & 3],
->active_fpu.fp_status);
-}
-
-static inline void restore_flush_mode(CPUMIPSState *env)
-{
-set_flush_to_zero((env->active_fpu.fcr31 & (1 << FCR31_FS)) != 0,
-  >active_fpu.fp_status);
-}
-
-static inline void restore_snan_bit_mode(CPUMIPSState *env)
-{
-set_snan_bit_is_one((env->active_fpu.fcr31 & (1 << FCR31_NAN2008)) == 0,
->active_fpu.fp_status);
-}
-
-static inline void restore_fp_status(CPUMIPSState *env)
-{
-restore_rounding_mode(env);
-restore_flush_mode(env);
-restore_snan_bit_mode(env);
-}
-
-static inline void restore_msa_fp_status(CPUMIPSState *env)
-{
-float_status *status = >active_tc.msa_fp_status;
-int rounding_mode = (env->active_tc.msacsr & MSACSR_RM_MASK) >> MSACSR_RM;
-bool flush_to_zero = (env->active_tc.msacsr & MSACSR_FS_MASK) != 0;
-
-set_float_rounding_mode(ieee_rm[rounding_mode], status);
-set_flush_to_zero(flush_to_zero, status);
-set_flush_inputs_to_zero(flush_to_zero, status);
-}
-
 static inline void restore_pamask(CPUMIPSState *env)
 {
 if (env->hflags & MIPS_HFLAG_ELPA) {
diff --git 

[PATCH 2/7] target/mips/translate: Add declarations for generic code

[Philippe Mathieu-Daudé]

Some CPU translation functions / registers / macros and
definitions can be used by ISA / ASE / extensions out of
the big translate.c file. Declare them in "translate.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.h | 33 
 target/mips/translate.c | 42 -
 2 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/target/mips/translate.h b/target/mips/translate.h
index fcda1a99001..dbf7df7ba6d 100644
--- a/target/mips/translate.h
+++ b/target/mips/translate.h
@@ -10,6 +10,8 @@
 
 #include "exec/translator.h"
 
+#define MIPS_DEBUG_DISAS 0
+
 typedef struct DisasContext {
 DisasContextBase base;
 target_ulong saved_pc;
@@ -47,4 +49,35 @@ typedef struct DisasContext {
 int gi;
 } DisasContext;
 
+/* MIPS major opcodes */
+#define MASK_OP_MAJOR(op)   (op & (0x3F << 26))
+
+void generate_exception_end(DisasContext *ctx, int excp);
+void gen_reserved_instruction(DisasContext *ctx);
+void check_insn(DisasContext *ctx, uint64_t flags);
+void gen_base_offset_addr(DisasContext *ctx, TCGv addr, int base, int offset);
+
+void gen_load_gpr(TCGv t, int reg);
+void gen_store_gpr(TCGv t, int reg);
+
+extern TCGv bcond;
+
+#define LOG_DISAS(...)\
+do {  \
+if (MIPS_DEBUG_DISAS) {   \
+qemu_log_mask(CPU_LOG_TB_IN_ASM, ## __VA_ARGS__); \
+} \
+} while (0)
+
+#define MIPS_INVAL(op)\
+do {  \
+if (MIPS_DEBUG_DISAS) {   \
+qemu_log_mask(CPU_LOG_TB_IN_ASM,  \
+  TARGET_FMT_lx ": %08x Invalid %s %03x %03x %03x\n", \
+  ctx->base.pc_next, ctx->opcode, op, \
+  ctx->opcode >> 26, ctx->opcode & 0x3F,  \
+  ((ctx->opcode >> 16) & 0x1F));  \
+} \
+} while (0)
+
 #endif
diff --git a/target/mips/translate.c b/target/mips/translate.c
index d7f5a1e8d84..46aab26b868 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -41,11 +41,6 @@
 #include "qemu/qemu-print.h"
 #include "translate.h"
 
-#define MIPS_DEBUG_DISAS 0
-
-/* MIPS major opcodes */
-#define MASK_OP_MAJOR(op)   (op & (0x3F << 26))
-
 enum {
 /* indirect opcode tables */
 OPC_SPECIAL  = (0x00 << 26),
@@ -2496,7 +2491,8 @@ enum {
 /* global register indices */
 static TCGv cpu_gpr[32], cpu_PC;
 static TCGv cpu_HI[MIPS_DSP_ACC], cpu_LO[MIPS_DSP_ACC];
-static TCGv cpu_dspctrl, btarget, bcond;
+static TCGv cpu_dspctrl, btarget;
+TCGv bcond;
 static TCGv cpu_lladdr, cpu_llval;
 static TCGv_i32 hflags;
 static TCGv_i32 fpu_fcr0, fpu_fcr31;
@@ -2609,26 +2605,8 @@ static const char * const mxuregnames[] = {
 };
 #endif
 
-#define LOG_DISAS(...)\
-do {  \
-if (MIPS_DEBUG_DISAS) {   \
-qemu_log_mask(CPU_LOG_TB_IN_ASM, ## __VA_ARGS__); \
-} \
-} while (0)
-
-#define MIPS_INVAL(op)\
-do {  \
-if (MIPS_DEBUG_DISAS) {   \
-qemu_log_mask(CPU_LOG_TB_IN_ASM,  \
-  TARGET_FMT_lx ": %08x Invalid %s %03x %03x %03x\n", \
-  ctx->base.pc_next, ctx->opcode, op, \
-  ctx->opcode >> 26, ctx->opcode & 0x3F,  \
-  ((ctx->opcode >> 16) & 0x1F));  \
-} \
-} while (0)
-
 /* General purpose registers moves. */
-static inline void gen_load_gpr(TCGv t, int reg)
+void gen_load_gpr(TCGv t, int reg)
 {
 if (reg == 0) {
 tcg_gen_movi_tl(t, 0);
@@ -2637,7 +2615,7 @@ static inline void gen_load_gpr(TCGv t, int reg)
 }
 }
 
-static inline void gen_store_gpr(TCGv t, int reg)
+void gen_store_gpr(TCGv t, int reg)
 {
 if (reg != 0) {
 tcg_gen_mov_tl(cpu_gpr[reg], t);
@@ -2782,11 +2760,16 @@ static inline void generate_exception(DisasContext 
*ctx, int excp)
 gen_helper_0e0i(raise_exception, excp);
 }
 
-static 

[PATCH 1/7] target/mips/translate: Extract DisasContext structure

[Philippe Mathieu-Daudé]

Extract DisasContext to a new 'translate.h' header so
different translation files (ISA, ASE, extensions)
can use it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.h | 50 +
 target/mips/translate.c | 38 +--
 2 files changed, 51 insertions(+), 37 deletions(-)
 create mode 100644 target/mips/translate.h

diff --git a/target/mips/translate.h b/target/mips/translate.h
new file mode 100644
index 000..fcda1a99001
--- /dev/null
+++ b/target/mips/translate.h
@@ -0,0 +1,50 @@
+/*
+ *  MIPS translation routines.
+ *
+ *  Copyright (c) 2004-2005 Jocelyn Mayer
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+#ifndef TARGET_MIPS_TRANSLATE_H
+#define TARGET_MIPS_TRANSLATE_H
+
+#include "exec/translator.h"
+
+typedef struct DisasContext {
+DisasContextBase base;
+target_ulong saved_pc;
+target_ulong page_start;
+uint32_t opcode;
+uint64_t insn_flags;
+int32_t CP0_Config1;
+int32_t CP0_Config2;
+int32_t CP0_Config3;
+int32_t CP0_Config5;
+/* Routine used to access memory */
+int mem_idx;
+MemOp default_tcg_memop_mask;
+uint32_t hflags, saved_hflags;
+target_ulong btarget;
+bool ulri;
+int kscrexist;
+bool rxi;
+int ie;
+bool bi;
+bool bp;
+uint64_t PAMask;
+bool mvh;
+bool eva;
+bool sc;
+int CP0_LLAddr_shift;
+bool ps;
+bool vp;
+bool cmgcr;
+bool mrp;
+bool nan2008;
+bool abs2008;
+bool saar;
+bool mi;
+int gi;
+} DisasContext;
+
+#endif
diff --git a/target/mips/translate.c b/target/mips/translate.c
index ee45dce9a50..d7f5a1e8d84 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -39,6 +39,7 @@
 #include "exec/translator.h"
 #include "exec/log.h"
 #include "qemu/qemu-print.h"
+#include "translate.h"
 
 #define MIPS_DEBUG_DISAS 0
 
@@ -2557,43 +2558,6 @@ static TCGv mxu_CR;
 tcg_temp_free_i32(helper_tmp);\
 } while (0)
 
-typedef struct DisasContext {
-DisasContextBase base;
-target_ulong saved_pc;
-target_ulong page_start;
-uint32_t opcode;
-uint64_t insn_flags;
-int32_t CP0_Config1;
-int32_t CP0_Config2;
-int32_t CP0_Config3;
-int32_t CP0_Config5;
-/* Routine used to access memory */
-int mem_idx;
-MemOp default_tcg_memop_mask;
-uint32_t hflags, saved_hflags;
-target_ulong btarget;
-bool ulri;
-int kscrexist;
-bool rxi;
-int ie;
-bool bi;
-bool bp;
-uint64_t PAMask;
-bool mvh;
-bool eva;
-bool sc;
-int CP0_LLAddr_shift;
-bool ps;
-bool vp;
-bool cmgcr;
-bool mrp;
-bool nan2008;
-bool abs2008;
-bool saar;
-bool mi;
-int gi;
-} DisasContext;
-
 #define DISAS_STOP   DISAS_TARGET_0
 #define DISAS_EXIT   DISAS_TARGET_1
 
-- 
2.26.2

[PATCH 0/7] target/mips: Add translate.h and fpu_translate.h headers

[Philippe Mathieu-Daudé]

As the 'extract MSA' series keep growing, yet another
preliminary series.

Basically we add declarations for everything that will
be reused by code extracted from the big translate.c.

Doing so now, we avoid the intermediate step of using
.c.inc files, and we compile as different objects.
(We would have to do this later anyway).
Slower, as it involve more series, but we can bisect.

This series is common to the other 'extract XYZ from
translate.c' series.

Regards,

Phil.

Based-on: mips-next (https://gitlab.com/philmd/qemu/-/tree/mips-next)

Philippe Mathieu-Daudé (7):
  target/mips/translate: Extract DisasContext structure
  target/mips/translate: Add declarations for generic code
  target/mips: Use FloatRoundMode enum for FCR31 modes conversion
  target/mips: Extract FPU helpers to 'fpu_helper.h'
  target/mips/fpu_helper: Remove unused headers
  target/mips: Declare generic FPU functions in 'fpu_translate.h'
  target/mips: Extract FPU specific definitions to fpu_translate.h

 target/mips/fpu_helper.h|  59 +
 target/mips/fpu_translate.h |  96 +
 target/mips/internal.h  |  49 ---
 target/mips/translate.h |  83 ++
 linux-user/mips/cpu_loop.c  |   1 +
 target/mips/fpu_helper.c|   7 +-
 target/mips/gdbstub.c   |   1 +
 target/mips/kvm.c   |   1 +
 target/mips/machine.c   |   1 +
 target/mips/msa_helper.c|   1 +
 target/mips/op_helper.c |   1 +
 target/mips/translate.c | 163 +---
 12 files changed, 267 insertions(+), 196 deletions(-)
 create mode 100644 target/mips/fpu_helper.h
 create mode 100644 target/mips/fpu_translate.h
 create mode 100644 target/mips/translate.h

-- 
2.26.2

Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations

[John Snow]

On 11/13/20 11:48 AM, Markus Armbruster wrote:

John Snow  writes:


The typing of _make_tree and friends is a bit involved, but it can be
done with some stubbed out types and a bit of elbow grease. The
forthcoming patches attempt to make some simplifications, but having the
type hints in advance may aid in review of subsequent patches.


Some notes on the abstract types used at this point, and what they
represent:

- TreeValue represents any object in the type tree. _make_tree is an
   optional call -- not every node in the final type tree will have been
   passed to _make_tree, so this type encompasses not only what is passed
   to _make_tree (dicts, strings) or returned from it (dicts, strings, a
   2-tuple), but any recursive value for any of the dicts passed to
   _make_tree -- which includes lists, strings, integers, null constants,
   and so on.

- _DObject is a type alias I use to mean "A JSON-style object,
   represented as a Python dict." There is no "JSON" type in Python, they
   are converted natively to recursively nested dicts and lists, with
   leaf values of str, int, float, None, True/False and so on. This type
   structure is not possible to accurately portray in mypy yet, so a
   placeholder is used.

   In this case, _DObject is being used to refer to SchemaInfo-like
   structures as defined in qapi/introspect.json, OR any sub-object
   values they may reference. We don't have strong typing available for
   those, so a generic alternative is used.

- Extra refers explicitly to the dict containing "extra" information
   about a node in the tree. mypy does not offer per-key typing for dicts
   in Python 3.6, so this is the best we can do here.

- Annotated refers to (one of) the return types of _make_tree:
   It represents a 2-tuple of (TreeValue, Extra).


Signed-off-by: Eduardo Habkost 
Signed-off-by: John Snow 
---
  scripts/qapi/introspect.py | 157 -
  scripts/qapi/mypy.ini  |   5 --
  scripts/qapi/schema.py |   2 +-
  3 files changed, 121 insertions(+), 43 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 63f721ebfb6..803288a64e7 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,7 +10,16 @@
  See the COPYING file in the top-level directory.
  """
  
-from typing import Optional, Sequence, cast

+from typing import (
+Any,
+Dict,
+List,
+Optional,
+Sequence,
+Tuple,
+Union,
+cast,
+)
  
  from .common import (

  c_name,
@@ -20,13 +29,56 @@
  )
  from .gen import QAPISchemaMonolithicCVisitor
  from .schema import (
+QAPISchema,
  QAPISchemaArrayType,
  QAPISchemaBuiltinType,
+QAPISchemaEntity,
+QAPISchemaEnumMember,
+QAPISchemaFeature,
+QAPISchemaObjectType,
+QAPISchemaObjectTypeMember,
  QAPISchemaType,
+QAPISchemaVariant,
+QAPISchemaVariants,
  )
+from .source import QAPISourceInfo
  
  
-def _make_tree(obj, ifcond, features, extra=None):

+# This module constructs a tree-like data structure that is used to


"Tree-like" suggests it's not a tree, it just looks like one if you
squint.  Drop "-like"?



Sure. I think I am grammatically predisposed to assume "binary tree" or 
at least some kind of monomorphic tree when I see "tree", hence the 
hedging and weasel-words.


No problem just to drop it.


+# generate the introspection information for QEMU. It behaves similarly
+# to a JSON value.
+#
+# A complexity over JSON is that our values may or may not be annotated.


It's the obvious abstract syntax tree for JSON, hacked up^W^Wextended to
support certain annotations.



Yes.


Let me add a bit of context and history.

The module's job is generating qapi-introspect.[ch] for a QAPISchema.

The purpose of qapi-introspect.[ch] is providing the information
query-qmp-schema needs, i.e. (a suitable C representation of) a JSON
value conforming to [SchemaInfo].  Details of this C representation are
not interesting right now.

We first go from QAPISchema to a suitable Python representation of
[SchemaInfo], then from there to the C source code, neatly separating
concerns.

Stupidest solution Python representation that could possibly work: the
obvious abstract syntax tree for JSON (that's also how Python's json
module works).

Parts corresponding to QAPISchema parts guarded by 'if' conditionals
need to be guarded by #if conditionals.

We want to prefix parts corresponding to certain QAPISchema parts with a
comment.

These two requirements came later, and were hacked into the existing
stupidest solution: any tree node can be a tuple (json, extra), where
json is the "stupidest" node, and extra is a dict of annotations.  In
other words, to annotate an unannotated node N with dict D, replace N by
(N, D).

Possible annotations:

 'comment': str
 'if': Sequence[str]

They say there are just three answers a Marine may give to an officer's
questions: "Yes, sir!", "No, sir!", "No excuse, sir!".  Let me 

Re: [PATCH qemu v11] spapr: Implement Open Firmware client interface

[Alexey Kardashevskiy]

On 08/12/2020 04:47, Cédric Le Goater wrote:

On 12/7/20 6:15 PM, Greg Kurz wrote:

On Mon, 7 Dec 2020 18:33:27 +1100
Alexey Kardashevskiy  wrote:


The PAPR platform which describes an OS environment that's presented by
a combination of a hypervisor and firmware. The features it specifies
require collaboration between the firmware and the hypervisor.

Since the beginning, the runtime component of the firmware (RTAS) has
been implemented as a 20 byte shim which simply forwards it to
a hypercall implemented in qemu. The boot time firmware component is
SLOF - but a build that's specific to qemu, and has always needed to be
updated in sync with it. Even though we've managed to limit the amount
of runtime communication we need between qemu and SLOF, there's some,
and it has become increasingly awkward to handle as we've implemented
new features.

This implements a boot time OF client interface (CI) which is
enabled by a new "x-vof" pseries machine option (stands for "Virtual Open
Firmware). When enabled, QEMU implements the custom H_OF_CLIENT hcall
which implements Open Firmware Client Interface (OF CI). This allows
using a smaller stateless firmware which does not have to manage
the device tree.

The new "vof.bin" firmware image is included with source code under
pc-bios/. It also includes RTAS blob.

This implements a handful of CI methods just to get -kernel/-initrd
working. In particular, this implements the device tree fetching and
simple memory allocator - "claim" (an OF CI memory allocator) and updates
"/memory@0/available" to report the client about available memory.

This implements changing some device tree properties which we know how
to deal with, the rest is ignored. To allow changes, this skips
fdt_pack() when x-vof=on as not packing the blob leaves some room for
appending.

In absence of SLOF, this assigns phandles to device tree nodes to make
device tree traversing work.

When x-vof=on, this adds "/chosen" every time QEMU (re)builds a tree.

This adds basic instances support which are managed by a hash map
ihandle -> [phandle].

Before the guest started, the used memory is:
0..4000 - the initial firmware
1..18 - stack

This OF CI does not implement "interpret".

Unlike SLOF, this does not format uninitialized nvram. Instead, this
includes a disk image with pre-formatted nvram.



[...]


diff --git a/pc-bios/vof/nvram.bin b/pc-bios/vof/nvram.bin
new file mode 100644
index 
..d183901cf980a91d81c4348bb20487c7bb62a2ec
GIT binary patch
literal 16384
zcmeI%Jx;?g6bEpZJ8*)oSZeqZi)sI4{AHlNb4;RW}a70XPHaW57uo=-#R7
zKSLBhJJ0sdixY3IuY@hzo0r$OmE%T;XE9uh@s1k=AOHafKmY;|fB*y_009U<00Izz
z00bZa0SG_<0uX=z1Rwwb2tWV=XCbip6d#B4{{rX#XR%}$Bm^J;0SG|gWP$!?Aq=-I
zcT+0Ix{{?1q>9J8r+eW^JK1tYYZZMWQCUwW%0S*~w^p@wfkX-kxMAEA<~5@*g)@mb%KD5!;O~8c)>8rRQBx55=trhk#+1+T3J_
zaf*G4vZAduqy$qda{``6Gnc2DQg7B^s4f5i

literal 0
HcmV?d1



So this needs an extra drive on the command line, eg:

-drive file=pc-bios/vof/nvram.bin,format=raw,if=pflash

Any chance this can be generated internally if the user
didn't provide one already ?


We can do everything :) The patch is already big and misformatted nvram 
is hardly noticeable.




or simply change the bios filename if x-vof=on ?


It is not bios (pc-bios/vof.bin is), it is NVRAM. You do not usually 
pass a file, instead QEMU creates in-memory blob and SLOF formats it 
when booted the first time. You can use a file to preserve changes made 
in SLOF across QEMU instances.



--
Alexey

Re: [RFC PATCH] python: add __repr__ to ConsoleSocket to aid debugging

[Willian Rampazzo]

Em seg, 7 de dez de 2020 19:14, Philippe Mathieu-Daudé 
escreveu:

> Hi Willian,
>
> On 12/7/20 10:35 PM, Willian Rampazzo wrote:
> > On Mon, Dec 7, 2020 at 5:10 PM Alex Bennée 
> wrote:
> >>
> >> While attempting to debug some console weirdness I thought it would be
> >> worth making it easier to see what it had inside.
> >>
> >> Signed-off-by: Alex Bennée 
> >> ---
> >>  python/qemu/console_socket.py | 8 
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/python/qemu/console_socket.py
> b/python/qemu/console_socket.py
> >> index f060d79e06..77966d1fe9 100644
> >> --- a/python/qemu/console_socket.py
> >> +++ b/python/qemu/console_socket.py
> >> @@ -45,6 +45,14 @@ class ConsoleSocket(socket.socket):
> >>  if drain:
> >>  self._drain_thread = self._thread_start()
> >>
> >> +def __repr__(self):
> >> +s = super(ConsoleSocket, self).__repr__()
> >> +s = s.rstrip(">")
> >> +s += ", logfile=%s" % (self._logfile)
> >> +s += ", drain_thread=%s" % (self._drain_thread)
> >> +s += ">"
> >
> > We could use something more pythonic for this file. Instead of 3
> > string concatenations, my suggestion is to go with string formatting,
> > like:
> >
> > s = "%s,  logfile=%s, drain_thread=%s>" % (s, self._logfile,
> self._drain_thread)
> >
> > As str is immutable in Python, it avoids unnecessary copies.
>
> With this (and John's comment) addressed, are you OK to add
> your R-b tag?
>

Absolutely!

The result will be the same in the end, so

Reviewed-by: Willian Rampazzo 


> >
> >> +return s
> >> +
> >>  def _drain_fn(self) -> None:
> >>  """Drains the socket and runs while the socket is open."""
> >>  while self._open:
> >> --
> >> 2.20.1
> >>
> >>
> >
> >
>
>

Re: [PATCH] MAINTAINERS: che...@lemote.com -> chenhua...@kernel.org

[Philippe Mathieu-Daudé]

On 12/5/20 10:22 AM, Huacai Chen wrote:
> Use @kernel.org address as the main communications end point. Update the
> corresponding M-entries and .mailmap (for git shortlog translation).
> 
> Signed-off-by: Huacai Chen 
> ---
>  .mailmap| 2 ++
>  MAINTAINERS | 8 
>  2 files changed, 6 insertions(+), 4 deletions(-)

Thanks, applied to mips-next.

Re: [PATCH v2 00/28] target/mips: Explode 60% of the 32K-lines translate.c

[Philippe Mathieu-Daudé]

On 11/23/20 9:44 PM, Philippe Mathieu-Daudé wrote:
> Since v1:
> - Addressed Richard review comments
> 
> Patches missing review: 1,3,4,21,22,25
> 
> Hi,
> 
> This series, while boring, helps maintainability.
> 
> I simply exploded 60% of the huge target/mips/translate.c,
> reducing it from 32K lines of code to 13500.
> 
> The small overhead in the diffstat is due to entries added in
> MAINTAINERS and license boilerplate addition:
> 20225 insertions(+), 19987 deletions(-)
> 
> While being a massive diff, it is a no-brain review using
> 'git-diff --color-moved=dimmed-zebra' which highlights very few
> changes: #include and license lines.
> 
> The exploded new layout, which allows more useful filtering
> with the get_maintainer.pl script, is:
> 
> - MIPS ISA, ASE and modules:
> 
>  . isa-micromips_helper.h.inc
>  . isa-nanomips_translate.c.inc
> 
>  . ase-mips16e_translate.c.inc
> 
>  . mod-dsp_helper.c
>  . mod-dsp_helper.h.inc
>  . mod-dsp_translate.c.inc
>  . mod-msa_helper.h.inc
>  . mod-msa_translate.c.inc
>  . mod-msa_helper.c
>  . mod-mt_helper.h.inc
> 
> - MIPS Vendor Specific:
> 
>  . vendor-loong-simd_helper.c
>  . vendor-loong-ext_translate.c.inc
>  . vendor-loong-simd_helper.h.inc
>  . vendor-loong-simd_translate.c.inc
> 
>  . vendor-tx-mmi_translate.c.inc
>  . vendor-tx_translate.c.inc
> 
>  . vendor-vr54xx_helper.c
>  . vendor-vr54xx_helper.h.inc
>  . vendor-vr54xx_translate.c.inc
> 
>  . vendor-mxu_translate.c.inc
> 
> There should be no logical code change (only code movement).
> 
> The series is available at:
> 
>   https://gitlab.com/philmd/qemu.git tags/mips_translate_explode-v2
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (28):
>   target/mips: Use FloatRoundMode enum for FCR31 modes conversion
>   target/mips: Extract FPU helpers to 'fpu_helper.h'

Patches 1 and 2 queued to mips-next.

Re: [PATCH 0/5] mips: Sanitize Multi-Threading ASE

[Philippe Mathieu-Daudé]

On 12/4/20 11:26 PM, Philippe Mathieu-Daudé wrote:
> Reviewing the MIPS code, ASE after ASE.
> Time for MT ASE.
> 
> - Introduce/use ase_mt_available() helper to check
>   if MT ASE is present
> - Avoid setting MT specific registers if MT ASE is absent
> 
> Philippe Mathieu-Daudé (5):
>   target/mips: Remove mips_def_t unused argument from mvp_init()
>   target/mips: Introduce ase_mt_available() helper
>   target/mips: Do not initialize MT registers if MT ASE absent
>   hw/mips/malta: Do not initialize MT registers if MT ASE absent
>   hw/mips/malta: Rewrite CP0_MVPConf0 access using deposit()
> 
>  target/mips/cpu.h|  7 +++
>  hw/mips/cps.c|  3 +--
>  hw/mips/malta.c  | 10 --
>  target/mips/cp0_helper.c |  2 +-
>  target/mips/cpu.c|  2 +-
>  target/mips/helper.c |  2 +-
>  target/mips/translate.c  |  4 ++--
>  target/mips/translate_init.c.inc |  6 +-
>  8 files changed, 26 insertions(+), 10 deletions(-)

Thanks, applied to mips-next.

[PATCH v4 5/6] linux-user/elfload: Update HWCAP bits from linux 5.7

[Philippe Mathieu-Daudé]

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 linux-user/elfload.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 8f943f93ba7..0836e72b5ac 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -986,6 +986,19 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, 
const CPUMIPSState *e
 enum {
 HWCAP_MIPS_R6   = (1 << 0),
 HWCAP_MIPS_MSA  = (1 << 1),
+HWCAP_MIPS_CRC32= (1 << 2),
+HWCAP_MIPS_MIPS16   = (1 << 3),
+HWCAP_MIPS_MDMX = (1 << 4),
+HWCAP_MIPS_MIPS3D   = (1 << 5),
+HWCAP_MIPS_SMARTMIPS= (1 << 6),
+HWCAP_MIPS_DSP  = (1 << 7),
+HWCAP_MIPS_DSP2 = (1 << 8),
+HWCAP_MIPS_DSP3 = (1 << 9),
+HWCAP_MIPS_MIPS16E2 = (1 << 10),
+HWCAP_LOONGSON_MMI  = (1 << 11),
+HWCAP_LOONGSON_EXT  = (1 << 12),
+HWCAP_LOONGSON_EXT2 = (1 << 13),
+HWCAP_LOONGSON_CPUCFG   = (1 << 14),
 };
 
 #define ELF_HWCAP get_elf_hwcap()
-- 
2.26.2

[PATCH v4 2/6] linux-user/elfload: Rename MIPS GET_FEATURE() as GET_FEATURE_INSN()

[Philippe Mathieu-Daudé]

We want to add macros similar to GET_FEATURE().
As this one use the 'insn_flags' field, rename it
GET_FEATURE_INSN().

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 linux-user/elfload.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index aae28fd929d..0e1d7e7677c 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -989,7 +989,7 @@ enum {
 
 #define ELF_HWCAP get_elf_hwcap()
 
-#define GET_FEATURE(_flag, _hwcap) \
+#define GET_FEATURE_INSN(_flag, _hwcap) \
 do { if (cpu->env.insn_flags & (_flag)) { hwcaps |= _hwcap; } } while (0)
 
 static uint32_t get_elf_hwcap(void)
@@ -997,13 +997,13 @@ static uint32_t get_elf_hwcap(void)
 MIPSCPU *cpu = MIPS_CPU(thread_cpu);
 uint32_t hwcaps = 0;
 
-GET_FEATURE(ISA_MIPS32R6 | ISA_MIPS64R6, HWCAP_MIPS_R6);
-GET_FEATURE(ASE_MSA, HWCAP_MIPS_MSA);
+GET_FEATURE_INSN(ISA_MIPS32R6 | ISA_MIPS64R6, HWCAP_MIPS_R6);
+GET_FEATURE_INSN(ASE_MSA, HWCAP_MIPS_MSA);
 
 return hwcaps;
 }
 
-#undef GET_FEATURE
+#undef GET_FEATURE_INSN
 
 #endif /* TARGET_MIPS */
 
-- 
2.26.2

[PATCH v4 6/6] linux-user: Add support for MIPS Loongson 2F/3A

[Philippe Mathieu-Daudé]

Userland ELF binaries using Loongson SIMD instructions have the
HWCAP_LOONGSON_MMI bit set [1].
Binaries compiled for LLoongson 3A [2] have the HWCAP_LOONGSON_EXT
bit set for the LQ / SQ instructions.

[1] commit 8e2d5831e4b ("target/mips: Legalize Loongson insn flags")
[2] commit af868995e1b ("target/mips: Add Loongson-3 CPU definition")

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 linux-user/elfload.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 0836e72b5ac..a64050713f2 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1024,6 +1024,8 @@ static uint32_t get_elf_hwcap(void)
 GET_FEATURE_REG_EQU(CP0_Config0, CP0C0_AR, CP0C0_AR_LENGTH,
 2, HWCAP_MIPS_R6);
 GET_FEATURE_REG_SET(CP0_Config3, 1 << CP0C3_MSAP, HWCAP_MIPS_MSA);
+GET_FEATURE_INSN(ASE_LMMI, HWCAP_LOONGSON_MMI);
+GET_FEATURE_INSN(ASE_LEXT, HWCAP_LOONGSON_EXT);
 
 return hwcaps;
 }
-- 
2.26.2

[PATCH v4 4/6] linux-user/elfload: Introduce MIPS GET_FEATURE_REG_EQU() macro

[Philippe Mathieu-Daudé]

ISA features are usually denoted in read-only bits from
CPU registers. Add the GET_FEATURE_REG_EQU() macro which
checks if a CPU register has bits set to a specific value.

Use the macro to check the 'Architecture Revision' level
of the Config0 register, which is '2' when the Release 6
ISA is implemented.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/cpu.h|  1 +
 linux-user/elfload.c | 12 +++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 23f8c6f96cd..2639b0ea06c 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -844,6 +844,7 @@ struct CPUMIPSState {
 #define CP0C0_MT   7 /*  9..7  */
 #define CP0C0_VI   3
 #define CP0C0_K0   0 /*  2..0  */
+#define CP0C0_AR_LENGTH 3
 int32_t CP0_Config1;
 #define CP0C1_M31
 #define CP0C1_MMU  25/* 30..25 */
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index b7c6d30723a..8f943f93ba7 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -7,6 +7,7 @@
 
 #include "qemu.h"
 #include "disas/disas.h"
+#include "qemu/bitops.h"
 #include "qemu/path.h"
 #include "qemu/queue.h"
 #include "qemu/guest-random.h"
@@ -995,17 +996,26 @@ enum {
 #define GET_FEATURE_REG_SET(_reg, _mask, _hwcap) \
 do { if (cpu->env._reg & (_mask)) { hwcaps |= _hwcap; } } while (0)
 
+#define GET_FEATURE_REG_EQU(_reg, _start, _length, _val, _hwcap) \
+do { \
+if (extract32(cpu->env._reg, (_start), (_length)) == (_val)) { \
+hwcaps |= _hwcap; \
+} \
+} while (0)
+
 static uint32_t get_elf_hwcap(void)
 {
 MIPSCPU *cpu = MIPS_CPU(thread_cpu);
 uint32_t hwcaps = 0;
 
-GET_FEATURE_INSN(ISA_MIPS32R6 | ISA_MIPS64R6, HWCAP_MIPS_R6);
+GET_FEATURE_REG_EQU(CP0_Config0, CP0C0_AR, CP0C0_AR_LENGTH,
+2, HWCAP_MIPS_R6);
 GET_FEATURE_REG_SET(CP0_Config3, 1 << CP0C3_MSAP, HWCAP_MIPS_MSA);
 
 return hwcaps;
 }
 
+#undef GET_FEATURE_REG_EQU
 #undef GET_FEATURE_REG_SET
 #undef GET_FEATURE_INSN
 
-- 
2.26.2

[PATCH v4 3/6] linux-user/elfload: Introduce MIPS GET_FEATURE_REG_SET() macro

[Philippe Mathieu-Daudé]

ISA features are usually denoted in read-only bits from
CPU registers. Add the GET_FEATURE_REG_SET() macro which
checks if a CPU register has bits set.

Use the macro to check for MSA (which sets the MSAP bit of
the Config3 register when the ASE implementation is present).

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 linux-user/elfload.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 0e1d7e7677c..b7c6d30723a 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -992,17 +992,21 @@ enum {
 #define GET_FEATURE_INSN(_flag, _hwcap) \
 do { if (cpu->env.insn_flags & (_flag)) { hwcaps |= _hwcap; } } while (0)
 
+#define GET_FEATURE_REG_SET(_reg, _mask, _hwcap) \
+do { if (cpu->env._reg & (_mask)) { hwcaps |= _hwcap; } } while (0)
+
 static uint32_t get_elf_hwcap(void)
 {
 MIPSCPU *cpu = MIPS_CPU(thread_cpu);
 uint32_t hwcaps = 0;
 
 GET_FEATURE_INSN(ISA_MIPS32R6 | ISA_MIPS64R6, HWCAP_MIPS_R6);
-GET_FEATURE_INSN(ASE_MSA, HWCAP_MIPS_MSA);
+GET_FEATURE_REG_SET(CP0_Config3, 1 << CP0C3_MSAP, HWCAP_MIPS_MSA);
 
 return hwcaps;
 }
 
+#undef GET_FEATURE_REG_SET
 #undef GET_FEATURE_INSN
 
 #endif /* TARGET_MIPS */
-- 
2.26.2

[PATCH v4 1/6] linux-user/elfload: Move GET_FEATURE macro out of get_elf_hwcap() body

[Philippe Mathieu-Daudé]

As we are going to add more macros, keep the function body clear.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 linux-user/elfload.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 0b02a926025..aae28fd929d 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -989,22 +989,22 @@ enum {
 
 #define ELF_HWCAP get_elf_hwcap()
 
+#define GET_FEATURE(_flag, _hwcap) \
+do { if (cpu->env.insn_flags & (_flag)) { hwcaps |= _hwcap; } } while (0)
+
 static uint32_t get_elf_hwcap(void)
 {
 MIPSCPU *cpu = MIPS_CPU(thread_cpu);
 uint32_t hwcaps = 0;
 
-#define GET_FEATURE(flag, hwcap) \
-do { if (cpu->env.insn_flags & (flag)) { hwcaps |= hwcap; } } while (0)
-
 GET_FEATURE(ISA_MIPS32R6 | ISA_MIPS64R6, HWCAP_MIPS_R6);
 GET_FEATURE(ASE_MSA, HWCAP_MIPS_MSA);
 
-#undef GET_FEATURE
-
 return hwcaps;
 }
 
+#undef GET_FEATURE
+
 #endif /* TARGET_MIPS */
 
 #ifdef TARGET_MICROBLAZE
-- 
2.26.2

[PATCH v4 0/6] linux-user: Rework get_elf_hwcap() and support MIPS Loongson 2F/3A

[Philippe Mathieu-Daudé]

Series now fully reviewed.

Since v3:
- Add CP0C0_AR_LENGTH definition (Richard)
- Fixed 3E -> 3A, Longsoon -> Loongson typos (Huacai)

Since v2:
- Use extract32() in GET_FEATURE_REG_EQU (rth)

Introduce the GET_FEATURE_REG_SET() and GET_FEATURE_REG_EQU()
macros to check if an instruction set is supported by a CPU
using CP0 read-only bits (instead of QEMU insn_flags which
is not always coherent - we might remove it soon).

Use these macros to test for MSA ASE and Release 6.

Update the ELF HWCAP bits and set the Loongson instructions
so we can run 2F/3A userland binaries.

Supersedes: <20201201192807.1094919-1-f4...@amsat.org>

Philippe Mathieu-Daudé (6):
  linux-user/elfload: Move GET_FEATURE macro out of get_elf_hwcap() body
  linux-user/elfload: Rename MIPS GET_FEATURE() as GET_FEATURE_INSN()
  linux-user/elfload: Introduce MIPS GET_FEATURE_REG_SET() macro
  linux-user/elfload: Introduce MIPS GET_FEATURE_REG_EQU() macro
  linux-user/elfload: Update HWCAP bits from linux 5.7
  linux-user: Add support for MIPS Loongson 2F/3A

 target/mips/cpu.h|  1 +
 linux-user/elfload.c | 43 ---
 2 files changed, 37 insertions(+), 7 deletions(-)

-- 
2.26.2

Re: [PATCH 0/3] target/mips: Add some CP0/MMU missing definitions

[Philippe Mathieu-Daudé]

On 12/1/20 2:28 PM, Philippe Mathieu-Daudé wrote:
> Add some MIPS3 and R6 definitions to ease code review.
> 
> Philippe Mathieu-Daudé (3):
>   target/mips: Add CP0 Config0 register definitions for MIPS3 ISA
>   target/mips: Replace CP0_Config0 magic values by proper definitions
>   target/mips: Explicit Release 6 MMU types
> 
>  target/mips/cpu.h| 11 +--
>  target/mips/internal.h   |  9 +
>  target/mips/translate_init.c.inc | 14 --
>  3 files changed, 22 insertions(+), 12 deletions(-)

Thanks, applied to mips-next.

Re: [PATCH] target/mips: Allow executing MSA instructions on Loongson-3A4000

[Philippe Mathieu-Daudé]

On 11/30/20 11:22 AM, Philippe Mathieu-Daudé wrote:
> The Loongson-3A4000 is a GS464V-based processor with MIPS MSA ASE:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg763059.html
> 
> Commit af868995e1b correctly set the 'MSA present' bit of Config3
> register, but forgot to allow the MSA instructions decoding in
> insn_flags, so executing them triggers a 'Reserved Instruction'.
> 
> Fix by adding the ASE_MSA mask to insn_flags.
> 
> Fixes: af868995e1b ("target/mips: Add Loongson-3 CPU definition")
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Buggy since 5.1, so probably not a big deal.
> ---
>  target/mips/translate_init.c.inc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, applied to mips-next.

Re: [PATCH 2/2] target/mips/kvm: Assert unreachable code is not used

[Philippe Mathieu-Daudé]

On 11/24/20 12:02 PM, Paolo Bonzini wrote:
> On 24/11/20 11:41, Philippe Mathieu-Daudé wrote:
>> Huacai, ping?
>>
>> On 5/12/20 9:09 AM, Philippe Mathieu-Daudé wrote:
>>> +Paolo
>>>
>>> On 4/29/20 10:29 AM, Philippe Mathieu-Daudé wrote:
 This code must not be used outside of KVM. Abort if it is.

 Signed-off-by: Philippe Mathieu-Daudé 
 ---
    target/mips/kvm.c | 8 ++--
    1 file changed, 2 insertions(+), 6 deletions(-)

 diff --git a/target/mips/kvm.c b/target/mips/kvm.c
 index de3e26ef1f..050bfbd7fa 100644
 --- a/target/mips/kvm.c
 +++ b/target/mips/kvm.c
 @@ -196,9 +196,7 @@ int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq,
 int level)
    CPUState *cs = CPU(cpu);
    struct kvm_mips_interrupt intr;
    -    if (!kvm_enabled()) {
 -    return 0;
 -    }
 +    assert(kvm_enabled());
      intr.cpu = -1;
    @@ -219,9 +217,7 @@ int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int
 irq, int level)
    CPUState *dest_cs = CPU(cpu);
    struct kvm_mips_interrupt intr;
    -    if (!kvm_enabled()) {
 -    return 0;
 -    }
 +    assert(kvm_enabled());
      intr.cpu = dest_cs->cpu_index;
   
>>>
>>
> 
> Acked-by: Paolo Bonzini 
> 
> For kvm_mips_set_ipi_interrupt, however, it would be nicer if
> hw/intc/mips_gic.c always used gic->vps[vp].env->irq[], and the qemu_irq
> handler took care of calling kvm_mips_set_ipi_interrupt.
> 
> Likewise, there is some duplication between kvm_mips_interrupt's caller
> and kvm_arch_pre_run.  I'm not sure if kvm_arch_pre_run is needed at all.

OK, I'll have a look and ask Huacai to test.

Meanwhile, patch applied to mips-next.

Thanks,

Phil.

Re: [RFC PATCH] python: add __repr__ to ConsoleSocket to aid debugging

[Philippe Mathieu-Daudé]

Hi Willian,

On 12/7/20 10:35 PM, Willian Rampazzo wrote:
> On Mon, Dec 7, 2020 at 5:10 PM Alex Bennée  wrote:
>>
>> While attempting to debug some console weirdness I thought it would be
>> worth making it easier to see what it had inside.
>>
>> Signed-off-by: Alex Bennée 
>> ---
>>  python/qemu/console_socket.py | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
>> index f060d79e06..77966d1fe9 100644
>> --- a/python/qemu/console_socket.py
>> +++ b/python/qemu/console_socket.py
>> @@ -45,6 +45,14 @@ class ConsoleSocket(socket.socket):
>>  if drain:
>>  self._drain_thread = self._thread_start()
>>
>> +def __repr__(self):
>> +s = super(ConsoleSocket, self).__repr__()
>> +s = s.rstrip(">")
>> +s += ", logfile=%s" % (self._logfile)
>> +s += ", drain_thread=%s" % (self._drain_thread)
>> +s += ">"
> 
> We could use something more pythonic for this file. Instead of 3
> string concatenations, my suggestion is to go with string formatting,
> like:
> 
> s = "%s,  logfile=%s, drain_thread=%s>" % (s, self._logfile, 
> self._drain_thread)
> 
> As str is immutable in Python, it avoids unnecessary copies.

With this (and John's comment) addressed, are you OK to add
your R-b tag?

> 
>> +return s
>> +
>>  def _drain_fn(self) -> None:
>>  """Drains the socket and runs while the socket is open."""
>>  while self._open:
>> --
>> 2.20.1
>>
>>
> 
> 

[PATCH v2] hw/core: Restrict 'fw-path-provider.c' to system mode emulation

[Philippe Mathieu-Daudé]

fw-path-provider.c is only consumed by qdev-fw.c, which itself
is in softmmu_ss[], so we can restrict fw-path-provider.c to
softmmu too.

Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Fix author email.
---
 hw/core/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/meson.build b/hw/core/meson.build
index 4a744f3b5e7..032576f5717 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -1,7 +1,6 @@
 # core qdev-related obj files, also used by *-user and unit tests
 hwcore_files = files(
   'bus.c',
-  'fw-path-provider.c',
   'hotplug.c',
   'qdev-properties.c',
   'qdev.c',
@@ -25,6 +24,7 @@
 common_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
 
 softmmu_ss.add(files(
+  'fw-path-provider.c',
   'loader.c',
   'machine-hmp-cmds.c',
   'machine.c',
-- 
2.26.2

[PATCH] hw/core: Restrict 'fw-path-provider.c' to system mode emulation

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé 

fw-path-provider.c is only consumed by qdev-fw.c, which itself
is in softmmu_ss[], so we can restrict fw-path-provider.c to
softmmu too.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/core/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/meson.build b/hw/core/meson.build
index 4a744f3b5e7..032576f5717 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -1,7 +1,6 @@
 # core qdev-related obj files, also used by *-user and unit tests
 hwcore_files = files(
   'bus.c',
-  'fw-path-provider.c',
   'hotplug.c',
   'qdev-properties.c',
   'qdev.c',
@@ -25,6 +24,7 @@
 common_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
 
 softmmu_ss.add(files(
+  'fw-path-provider.c',
   'loader.c',
   'machine-hmp-cmds.c',
   'machine.c',
-- 
2.26.2

[PATCH 2/2] target/mips: Introduce cpu_supports_isa() taking CPUMIPSState argument

[Philippe Mathieu-Daudé]

Introduce cpu_supports_isa() which takes a CPUMIPSState
argument, more useful at runtime when the CPU is created
(no need to call the extensive object_class_by_name()).

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/cpu.h | 1 +
 target/mips/cpu.c | 5 +
 2 files changed, 6 insertions(+)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 9c65c87bf99..e8bca75f237 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1287,6 +1287,7 @@ int cpu_mips_signal_handler(int host_signum, void *pinfo, 
void *puc);
 #define CPU_RESOLVING_TYPE TYPE_MIPS_CPU
 
 bool cpu_type_supports_cps_smp(const char *cpu_type);
+bool cpu_supports_isa(const CPUMIPSState *env, uint64_t isa_mask);
 bool cpu_type_supports_isa(const char *cpu_type, uint64_t isa);
 void cpu_set_exception_base(int vp_index, target_ulong address);
 
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 76d50b00b42..687e2680dd1 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -310,3 +310,8 @@ MIPSCPU *mips_cpu_create_with_clock(const char *cpu_type, 
Clock *cpu_refclk)
 
 return MIPS_CPU(cpu);
 }
+
+bool cpu_supports_isa(const CPUMIPSState *env, uint64_t isa_mask)
+{
+return (env->cpu_model->insn_flags & isa_mask) != 0;
+}
-- 
2.26.2

[PATCH 0/2] target/mips: Let cpu_supports_isa() take CPUMIPSState argument

[Philippe Mathieu-Daudé]

Hi Jiaxun,

Here goes the cpu_supports_isa() helper for your bootloader API:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg764582.html

Regards,

Phil.

Philippe Mathieu-Daudé (2):
  target/mips: Rename cpu_supports_FEAT() as cpu_type_supports_FEAT()
  target/mips: Introduce cpu_supports_isa() taking CPUMIPSState argument

 target/mips/cpu.h   | 5 +++--
 hw/mips/boston.c| 4 ++--
 hw/mips/malta.c | 4 ++--
 target/mips/cpu.c   | 5 +
 target/mips/translate.c | 4 ++--
 5 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.26.2

[PATCH 1/2] target/mips: Rename cpu_supports_FEAT() as cpu_type_supports_FEAT()

[Philippe Mathieu-Daudé]

As cpu_supports_isa() / cpu_supports_cps_smp() take a 'cpu_type'
name argument, rename them cpu_type_supports_FEAT().

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/cpu.h   | 4 ++--
 hw/mips/boston.c| 4 ++--
 hw/mips/malta.c | 4 ++--
 target/mips/translate.c | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 23f8c6f96cd..9c65c87bf99 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1286,8 +1286,8 @@ int cpu_mips_signal_handler(int host_signum, void *pinfo, 
void *puc);
 #define MIPS_CPU_TYPE_NAME(model) model MIPS_CPU_TYPE_SUFFIX
 #define CPU_RESOLVING_TYPE TYPE_MIPS_CPU
 
-bool cpu_supports_cps_smp(const char *cpu_type);
-bool cpu_supports_isa(const char *cpu_type, uint64_t isa);
+bool cpu_type_supports_cps_smp(const char *cpu_type);
+bool cpu_type_supports_isa(const char *cpu_type, uint64_t isa);
 void cpu_set_exception_base(int vp_index, target_ulong address);
 
 /* mips_int.c */
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 3d40867dc4c..16467ea4752 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -459,12 +459,12 @@ static void boston_mach_init(MachineState *machine)
 s = BOSTON(dev);
 s->mach = machine;
 
-if (!cpu_supports_cps_smp(machine->cpu_type)) {
+if (!cpu_type_supports_cps_smp(machine->cpu_type)) {
 error_report("Boston requires CPUs which support CPS");
 exit(1);
 }
 
-is_64b = cpu_supports_isa(machine->cpu_type, ISA_MIPS64);
+is_64b = cpu_type_supports_isa(machine->cpu_type, ISA_MIPS64);
 
 object_initialize_child(OBJECT(machine), "cps", >cps, TYPE_MIPS_CPS);
 object_property_set_str(OBJECT(>cps), "cpu-type", machine->cpu_type,
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 9d1a3b50b7a..965a952d934 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1204,7 +1204,7 @@ static void create_cps(MachineState *ms, MaltaState *s,
 static void mips_create_cpu(MachineState *ms, MaltaState *s,
 qemu_irq *cbus_irq, qemu_irq *i8259_irq)
 {
-if ((ms->smp.cpus > 1) && cpu_supports_cps_smp(ms->cpu_type)) {
+if ((ms->smp.cpus > 1) && cpu_type_supports_cps_smp(ms->cpu_type)) {
 create_cps(ms, s, cbus_irq, i8259_irq);
 } else {
 create_cpu_without_cps(ms, s, cbus_irq, i8259_irq);
@@ -1308,7 +1308,7 @@ void mips_malta_init(MachineState *machine)
 loaderparams.initrd_filename = initrd_filename;
 kernel_entry = load_kernel();
 
-if (!cpu_supports_isa(machine->cpu_type, ISA_NANOMIPS32)) {
+if (!cpu_type_supports_isa(machine->cpu_type, ISA_NANOMIPS32)) {
 write_bootloader(memory_region_get_ram_ptr(bios),
  bootloader_run_addr, kernel_entry);
 } else {
diff --git a/target/mips/translate.c b/target/mips/translate.c
index c64a1bc42e1..b8ed16bb779 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -31770,13 +31770,13 @@ void cpu_mips_realize_env(CPUMIPSState *env)
 mvp_init(env, env->cpu_model);
 }
 
-bool cpu_supports_cps_smp(const char *cpu_type)
+bool cpu_type_supports_cps_smp(const char *cpu_type)
 {
 const MIPSCPUClass *mcc = MIPS_CPU_CLASS(object_class_by_name(cpu_type));
 return (mcc->cpu_def->CP0_Config3 & (1 << CP0C3_CMGCR)) != 0;
 }
 
-bool cpu_supports_isa(const char *cpu_type, uint64_t isa)
+bool cpu_type_supports_isa(const char *cpu_type, uint64_t isa)
 {
 const MIPSCPUClass *mcc = MIPS_CPU_CLASS(object_class_by_name(cpu_type));
 return (mcc->cpu_def->insn_flags & isa) != 0;
-- 
2.26.2

Re: [PATCH] target/arm: do not use cc->do_interrupt for KVM directly

[Peter Maydell]

On Mon, 7 Dec 2020 at 21:26, Eduardo Habkost  wrote:
> My understanding is that there's no reason for ARM KVM to use
> another approach, and that CPUClass.do_interrupt is not really
> TCG-specific.
>
> Do we have any case where the CPUClass.do_interrupt
> implementation is really TCG-specific, or it is just a
> coincidence that most other accelerators simply don't to call the
> method?  It looks like the only cases where the
> CPUClass.do_interrupt assignment is conditional on CONFIG_TCG are
> i386 and s390x.

Looking at PPC, its kvm_handle_debug() function does a
direct call to ppc_cpu_do_interrupt(). So the code of
its do_interrupt method must be ok-for-KVM, it's just that
it doesn't use the method pointer. (It's doing the same thing
Arm is -- if a debug event turns out not to be for QEMU itself,
inject a suitable exception into the guest.)

So of our 5 KVM-supporting architectures:

 * i386 and s390x have kernel APIs for "inject suitable
   exception", don't need to call do_interrupt, and make
   the cc->do_interrupt assignment only ifdef CONFIG_TCG,
   so that the code for do_interrupt need not be compiled
   into a KVM-only binary. (In both cases the code for the
   function is in a source file that the meson.build puts
   into the source list only if CONFIG_TCG)
 * ppc and arm both need to use this code even in a KVM
   only binary. Neither of them #ifdef the cc->do_interrupt
   assignment, because there's not much point at the moment
   if you're not going to try to compile out the code.
   ppc happens to do a direct function call, and arm happens
   to go via the cc->do_interrupt pointer, but I don't
   think there's much significance in the choice either way.
   In both cases, the only places making the call are within
   architecture-specific KVM code.
 * mips KVM does neither of these things, probably because it is
   not sufficiently featureful to have run into the cases
   where you might want to re-inject an exception and it's
   not being sufficiently used in production for anybody to
   have looked at minimising the amount of code in a
   KVM-only QEMU binary for it.

So in conclusion we have a basically 50:50 split between
"use the same do_interrupt code as TCG" and "have a kernel
API to make the kernel do the work", plus one arch that
probably hasn't had to make the choice yet.   ¯\_(ツ)_/¯

> Oh, I thought you were arguing that CPUClass.do_interrupt is
> not TCG_specific.

Well, I don't think it really is TCG-specific. But as
a pragmatic thing, if these two lines in the Arm code
are getting in the way of stopping us from having a
useful compile-time check that code that's not supposed
to call this method isn't calling it, I think the balance
maybe leans towards just making the direct function call.
I guess it depends whether you think people are likely to
accidentally make cc->do_interrupt calls in non-target-specific
code that gets used by KVM (which currently would crash if that
code path is exercised on x86 or s390x, but under the
proposed change would become a compile error).

thanks
-- PMM

Re: [RFC PATCH] python: add __repr__ to ConsoleSocket to aid debugging

[Willian Rampazzo]

On Mon, Dec 7, 2020 at 5:10 PM Alex Bennée  wrote:
>
> While attempting to debug some console weirdness I thought it would be
> worth making it easier to see what it had inside.
>
> Signed-off-by: Alex Bennée 
> ---
>  python/qemu/console_socket.py | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
> index f060d79e06..77966d1fe9 100644
> --- a/python/qemu/console_socket.py
> +++ b/python/qemu/console_socket.py
> @@ -45,6 +45,14 @@ class ConsoleSocket(socket.socket):
>  if drain:
>  self._drain_thread = self._thread_start()
>
> +def __repr__(self):
> +s = super(ConsoleSocket, self).__repr__()
> +s = s.rstrip(">")
> +s += ", logfile=%s" % (self._logfile)
> +s += ", drain_thread=%s" % (self._drain_thread)
> +s += ">"

We could use something more pythonic for this file. Instead of 3
string concatenations, my suggestion is to go with string formatting,
like:

s = "%s,  logfile=%s, drain_thread=%s>" % (s, self._logfile, self._drain_thread)

As str is immutable in Python, it avoids unnecessary copies.

> +return s
> +
>  def _drain_fn(self) -> None:
>  """Drains the socket and runs while the socket is open."""
>  while self._open:
> --
> 2.20.1
>
>

Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations

[John Snow]

On 11/6/20 9:12 PM, Cleber Rosa wrote:

On Mon, Oct 26, 2020 at 03:42:45PM -0400, John Snow wrote:

The typing of _make_tree and friends is a bit involved, but it can be
done with some stubbed out types and a bit of elbow grease. The
forthcoming patches attempt to make some simplifications, but having the
type hints in advance may aid in review of subsequent patches.


Some notes on the abstract types used at this point, and what they
represent:

- TreeValue represents any object in the type tree. _make_tree is an
   optional call -- not every node in the final type tree will have been
   passed to _make_tree, so this type encompasses not only what is passed
   to _make_tree (dicts, strings) or returned from it (dicts, strings, a
   2-tuple), but any recursive value for any of the dicts passed to
   _make_tree -- which includes lists, strings, integers, null constants,
   and so on.

- _DObject is a type alias I use to mean "A JSON-style object,
   represented as a Python dict." There is no "JSON" type in Python, they
   are converted natively to recursively nested dicts and lists, with
   leaf values of str, int, float, None, True/False and so on. This type
   structure is not possible to accurately portray in mypy yet, so a
   placeholder is used.

   In this case, _DObject is being used to refer to SchemaInfo-like
   structures as defined in qapi/introspect.json, OR any sub-object
   values they may reference. We don't have strong typing available for
   those, so a generic alternative is used.

- Extra refers explicitly to the dict containing "extra" information
   about a node in the tree. mypy does not offer per-key typing for dicts
   in Python 3.6, so this is the best we can do here.

- Annotated refers to (one of) the return types of _make_tree:
   It represents a 2-tuple of (TreeValue, Extra).


Signed-off-by: Eduardo Habkost 
Signed-off-by: John Snow 
---
  scripts/qapi/introspect.py | 157 -
  scripts/qapi/mypy.ini  |   5 --
  scripts/qapi/schema.py |   2 +-
  3 files changed, 121 insertions(+), 43 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 63f721ebfb6..803288a64e7 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,7 +10,16 @@
  See the COPYING file in the top-level directory.
  """
  
-from typing import Optional, Sequence, cast

+from typing import (
+Any,
+Dict,
+List,
+Optional,
+Sequence,
+Tuple,
+Union,
+cast,
+)
  
  from .common import (

  c_name,
@@ -20,13 +29,56 @@
  )
  from .gen import QAPISchemaMonolithicCVisitor
  from .schema import (
+QAPISchema,
  QAPISchemaArrayType,
  QAPISchemaBuiltinType,
+QAPISchemaEntity,
+QAPISchemaEnumMember,
+QAPISchemaFeature,
+QAPISchemaObjectType,
+QAPISchemaObjectTypeMember,
  QAPISchemaType,
+QAPISchemaVariant,
+QAPISchemaVariants,
  )
+from .source import QAPISourceInfo
  
  
-def _make_tree(obj, ifcond, features, extra=None):

+# This module constructs a tree-like data structure that is used to
+# generate the introspection information for QEMU. It behaves similarly
+# to a JSON value.
+#
+# A complexity over JSON is that our values may or may not be annotated.
+#
+# Un-annotated values may be:
+# Scalar: str, bool, None.
+# Non-scalar: List, Dict
+# _Value = Union[str, bool, None, Dict[str, Value], List[Value]]


Here (and in a few other places) you mention `_Value`, but then define it as
`_value` (lowercase).



This was maybe too subtle: I didn't intend for it to be the "same" as 
_value; this is the "idealized version". And then I went and re-used the 
same exact name of "TreeValue", which muddied the waters.


Let's just err back on the side of synchronicity and say:

# _value = Union[str, bool, None, Dict[str, TreeValue], List[TreeValue]]
# TreeValue = Union[_value, Annotated[_value]] 




+#
+# With optional annotations, the type of all values is:
+# TreeValue = Union[_Value, Annotated[_Value]]
+#
+# Sadly, mypy does not support recursive types, so we must approximate this.
+_stub = Any
+_scalar = Union[str, bool, None]
+_nonscalar = Union[Dict[str, _stub], List[_stub]]


Why not use `Any` here instead of declaring/using `_stub`?



This was my way of calling visual attention to the exact places in the 
type tree we are breaking recursion with a stubbed type.


I wanted to communicate that we didn't WANT the any type, but we were 
forced to accept a "stub" type. (Which we implement with the Any type.)



+_value = Union[_scalar, _nonscalar]
+TreeValue = Union[_value, 'Annotated']
+


Maybe declare `Annotations` first, then `Annotated`, then *use*
`Annotated` proper here?



As long as that doesn't lose clarity and synchronicity with the little 
comment blurb, or cause problems later in the patch. I will see if there 
was a reason I couldn't.


Otherwise, it's not really too important to shuffle around things to 
avoid strings; it 

Re: [PATCH] target/arm: do not use cc->do_interrupt for KVM directly

[Eduardo Habkost]

On Mon, Dec 07, 2020 at 09:12:34PM +, Peter Maydell wrote:
> On Mon, 7 Dec 2020 at 21:06, Claudio Fontana  wrote:
> > As I understand it, for the purpose of code separation,
> > we could:
> >
> > 1) skip do_interrupt move to the separate tcg_ops structure, wait until 
> > KVM/ARM uses another approach (if ever)
> > 2) do the move, and just call arm_cpu_do_interrupt() directly, since for 
> > KVM64 it is the only one that can be assigned to cc->do_interrupt().
> >
> > Which way would you suggest?
> 
> So what's the intention here? To put tcg-only methods in their
> own struct so that on a KVM-only QEMU they're compiled out
> and attempts to use them are a compile error? In that case
> I guess if Arm really is the only user of do_interrupt outside
> TCG then we should do what this patch does and do a direct call.

Oh, I thought you were arguing that CPUClass.do_interrupt is
not TCG_specific.

If you agree with doing what this patch does, the plan sounds
good to me.

-- 
Eduardo

Re: [PATCH] target/arm: do not use cc->do_interrupt for KVM directly

[Eduardo Habkost]

On Mon, Dec 07, 2020 at 10:06:55PM +0100, Claudio Fontana wrote:
> On 12/7/20 9:56 PM, Peter Maydell wrote:
> > On Mon, 7 Dec 2020 at 18:28, Eduardo Habkost  wrote:
> >> All signs seem to indicate that CPUClass.do_interrupt is
> >> TCG-specific, except for those two lines of code in
> >> target/arm/kvm64.c.  The point of this patch would be to allow us
> >> to separate TCG-specific code from accel-independent code later.
> > 
> > So it's TCG-specific except that we call it from KVM.
> > That doesn't sound very TCG-specific :-)
> > 
> >> Maybe those signs are misleading us, and CPUClass.do_interrupt
> >> shouldn't be TCG-specific.  If that's the case, why arm is the
> >> only architecture that uses CPUClass.do_interrupt outside
> >> TCG-specific code?
> > 
> > So, the purpose of the do_interrupt method is "set the guest
> > CPU state up in the way that the architecture specifies
> > happens when an interrupt is taken" (set the program counter,
> > set things like the syndrome register that says what type
> > of exception happens, etc, etc). For TCG we obviously need
> > to do this for every interrupt/exception that happens.
> > For KVM, in most cases the kernel is responsible for
> > delivering an exception to the guest, because it's the
> > kernel that determines that it needs to do this.
> > The two oddball cases[*] in target/arm are for situations
> > where it is userspace code that has identified that it
> > needs to deliver an exception to the guest. The kernel's
> > KVM API doesn't provide a "please deliver an exception to
> > the guest" function, on the grounds that userspace could
> > do the work itself. So we need to do that work (setting the
> > PC, setting syndrome register, etc, etc). In theory we
> > could have a special version of the function for KVM
> > CPUs only, but since in fact the same code works just
> > fine in KVM and TCG we reuse it.
> > 
> > I know that the macOS Hypervisor.Framework APIs are rather
> > lower-level than KVM (they do less work in the kernel and
> > push more of it onto userspace); it's possible that there
> > we might find more situations where userspace needs to do
> > "make the guest CPU take an exception"; I haven't investigated.
> > 
> > [*] The two special cases are:
> >  (1) the vcpu thread got a SIGBUS indicating a memory error,
> >  and we need to deliver a synchronous external abort
> >  exception to the guest to let it know about the error
> >  (2) the kernel told us about a debug exception (breakpoint,
> >  watchpoint, etc) but it turns out not to be for one of
> >  QEMU's own gdbstub breakpoints/watchpoints, so it
> >  must be one the guest itself has set up, and so we need
> >  to deliver it to the guest
> > 
> > These are fairly obscure, and it wouldn't surprise me if
> > other target archs had picked a different separation of
> > concerns between userspace and the kernel such that userspace
> > didn't need to manually deliver an exception.
> > 
> > thanks
> > -- PMM
> > 
> 
> Hello Peter,
> 
> thank you for the explanation, interesting read.
> 
> As I understand it, for the purpose of code separation,
> we could:
> 
> 1) skip do_interrupt move to the separate tcg_ops structure,
> wait until KVM/ARM uses another approach (if ever)

My understanding is that there's no reason for ARM KVM to use
another approach, and that CPUClass.do_interrupt is not really
TCG-specific.

Do we have any case where the CPUClass.do_interrupt
implementation is really TCG-specific, or it is just a
coincidence that most other accelerators simply don't to call the
method?  It looks like the only cases where the
CPUClass.do_interrupt assignment is conditional on CONFIG_TCG are
i386 and s390x.


> 2) do the move, and just call arm_cpu_do_interrupt() directly,
> since for KVM64 it is the only one that can be assigned to
> cc->do_interrupt().
> 
> Which way would you suggest?
> 
> Thanks,
> 
> Claudio
> 
> 
> 
> 
> 

-- 
Eduardo

Plugin Register Accesses

[Aaron Lindsay]

I'm trying to migrate to using the new plugin interface. I see the
following in include/qemu/qemu-plugin.h:

> enum qemu_plugin_cb_flags {
> QEMU_PLUGIN_CB_NO_REGS, /* callback does not access the CPU's regs */
> QEMU_PLUGIN_CB_R_REGS,  /* callback reads the CPU's regs */
> QEMU_PLUGIN_CB_RW_REGS, /* callback reads and writes the CPU's regs */
> };

But I don't see a way to access registers in callbacks. Am I missing
something?

-Aaron

Re: [PATCH] target/arm: do not use cc->do_interrupt for KVM directly

[Claudio Fontana]

On 12/7/20 10:12 PM, Peter Maydell wrote:
> On Mon, 7 Dec 2020 at 21:06, Claudio Fontana  wrote:
>> As I understand it, for the purpose of code separation,
>> we could:
>>
>> 1) skip do_interrupt move to the separate tcg_ops structure, wait until 
>> KVM/ARM uses another approach (if ever)
>> 2) do the move, and just call arm_cpu_do_interrupt() directly, since for 
>> KVM64 it is the only one that can be assigned to cc->do_interrupt().
>>
>> Which way would you suggest?
> 
> So what's the intention here? To put tcg-only methods in their
> own struct so that on a KVM-only QEMU they're compiled out
> and attempts to use them are a compile error?

Yes.

> In that case
> I guess if Arm really is the only user of do_interrupt outside
> TCG then we should do what this patch does and do a direct call.
> 
> thanks
> -- PMM
> 

Thanks, will do!

Claudio

Re: [PATCH] target/arm: do not use cc->do_interrupt for KVM directly

[Peter Maydell]

On Mon, 7 Dec 2020 at 21:06, Claudio Fontana  wrote:
> As I understand it, for the purpose of code separation,
> we could:
>
> 1) skip do_interrupt move to the separate tcg_ops structure, wait until 
> KVM/ARM uses another approach (if ever)
> 2) do the move, and just call arm_cpu_do_interrupt() directly, since for 
> KVM64 it is the only one that can be assigned to cc->do_interrupt().
>
> Which way would you suggest?

So what's the intention here? To put tcg-only methods in their
own struct so that on a KVM-only QEMU they're compiled out
and attempts to use them are a compile error? In that case
I guess if Arm really is the only user of do_interrupt outside
TCG then we should do what this patch does and do a direct call.

thanks
-- PMM

Re: [PATCH] target/arm: do not use cc->do_interrupt for KVM directly

[Claudio Fontana]

On 12/7/20 9:56 PM, Peter Maydell wrote:
> On Mon, 7 Dec 2020 at 18:28, Eduardo Habkost  wrote:
>> All signs seem to indicate that CPUClass.do_interrupt is
>> TCG-specific, except for those two lines of code in
>> target/arm/kvm64.c.  The point of this patch would be to allow us
>> to separate TCG-specific code from accel-independent code later.
> 
> So it's TCG-specific except that we call it from KVM.
> That doesn't sound very TCG-specific :-)
> 
>> Maybe those signs are misleading us, and CPUClass.do_interrupt
>> shouldn't be TCG-specific.  If that's the case, why arm is the
>> only architecture that uses CPUClass.do_interrupt outside
>> TCG-specific code?
> 
> So, the purpose of the do_interrupt method is "set the guest
> CPU state up in the way that the architecture specifies
> happens when an interrupt is taken" (set the program counter,
> set things like the syndrome register that says what type
> of exception happens, etc, etc). For TCG we obviously need
> to do this for every interrupt/exception that happens.
> For KVM, in most cases the kernel is responsible for
> delivering an exception to the guest, because it's the
> kernel that determines that it needs to do this.
> The two oddball cases[*] in target/arm are for situations
> where it is userspace code that has identified that it
> needs to deliver an exception to the guest. The kernel's
> KVM API doesn't provide a "please deliver an exception to
> the guest" function, on the grounds that userspace could
> do the work itself. So we need to do that work (setting the
> PC, setting syndrome register, etc, etc). In theory we
> could have a special version of the function for KVM
> CPUs only, but since in fact the same code works just
> fine in KVM and TCG we reuse it.
> 
> I know that the macOS Hypervisor.Framework APIs are rather
> lower-level than KVM (they do less work in the kernel and
> push more of it onto userspace); it's possible that there
> we might find more situations where userspace needs to do
> "make the guest CPU take an exception"; I haven't investigated.
> 
> [*] The two special cases are:
>  (1) the vcpu thread got a SIGBUS indicating a memory error,
>  and we need to deliver a synchronous external abort
>  exception to the guest to let it know about the error
>  (2) the kernel told us about a debug exception (breakpoint,
>  watchpoint, etc) but it turns out not to be for one of
>  QEMU's own gdbstub breakpoints/watchpoints, so it
>  must be one the guest itself has set up, and so we need
>  to deliver it to the guest
> 
> These are fairly obscure, and it wouldn't surprise me if
> other target archs had picked a different separation of
> concerns between userspace and the kernel such that userspace
> didn't need to manually deliver an exception.
> 
> thanks
> -- PMM
> 

Hello Peter,

thank you for the explanation, interesting read.

As I understand it, for the purpose of code separation,
we could:

1) skip do_interrupt move to the separate tcg_ops structure, wait until KVM/ARM 
uses another approach (if ever)
2) do the move, and just call arm_cpu_do_interrupt() directly, since for KVM64 
it is the only one that can be assigned to cc->do_interrupt().

Which way would you suggest?

Thanks,

Claudio

Re: [RFC PATCH] python: add __repr__ to ConsoleSocket to aid debugging

[John Snow]

On 12/7/20 3:07 PM, Alex Bennée wrote:

While attempting to debug some console weirdness I thought it would be
worth making it easier to see what it had inside.

Signed-off-by: Alex Bennée 
---
  python/qemu/console_socket.py | 8 
  1 file changed, 8 insertions(+)

diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index f060d79e06..77966d1fe9 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -45,6 +45,14 @@ class ConsoleSocket(socket.socket):
  if drain:
  self._drain_thread = self._thread_start()
  
+def __repr__(self):


def __repr__(self) -> str:


+s = super(ConsoleSocket, self).__repr__()


Use python3-style super(): super().__repr__()


+s = s.rstrip(">")
+s += ", logfile=%s" % (self._logfile)
+s += ", drain_thread=%s" % (self._drain_thread)
+s += ">"
+return s
+


Reviewed-by: John Snow 

feel free to take this through your testing tree.

--js

Re: [PATCH] target/arm: do not use cc->do_interrupt for KVM directly

[Peter Maydell]

On Mon, 7 Dec 2020 at 20:56, Peter Maydell  wrote:
> These are fairly obscure, and it wouldn't surprise me if
> other target archs had picked a different separation of
> concerns between userspace and the kernel such that userspace
> didn't need to manually deliver an exception.

...looking at the target/i386 code, it looks like the
approach taken on that architecture is that userspace just
asks the kernel to take the exception using the KVM_SET_VCPU_EVENTS
ioctl. (For Arm we only use that ioctl to handle SError,
because the information that an SError is pending is a piece
of state that's external to the CPU.)

thanks
-- PMM

Re: [PATCH] target/arm: do not use cc->do_interrupt for KVM directly

[Peter Maydell]

On Mon, 7 Dec 2020 at 18:28, Eduardo Habkost  wrote:
> All signs seem to indicate that CPUClass.do_interrupt is
> TCG-specific, except for those two lines of code in
> target/arm/kvm64.c.  The point of this patch would be to allow us
> to separate TCG-specific code from accel-independent code later.

So it's TCG-specific except that we call it from KVM.
That doesn't sound very TCG-specific :-)

> Maybe those signs are misleading us, and CPUClass.do_interrupt
> shouldn't be TCG-specific.  If that's the case, why arm is the
> only architecture that uses CPUClass.do_interrupt outside
> TCG-specific code?

So, the purpose of the do_interrupt method is "set the guest
CPU state up in the way that the architecture specifies
happens when an interrupt is taken" (set the program counter,
set things like the syndrome register that says what type
of exception happens, etc, etc). For TCG we obviously need
to do this for every interrupt/exception that happens.
For KVM, in most cases the kernel is responsible for
delivering an exception to the guest, because it's the
kernel that determines that it needs to do this.
The two oddball cases[*] in target/arm are for situations
where it is userspace code that has identified that it
needs to deliver an exception to the guest. The kernel's
KVM API doesn't provide a "please deliver an exception to
the guest" function, on the grounds that userspace could
do the work itself. So we need to do that work (setting the
PC, setting syndrome register, etc, etc). In theory we
could have a special version of the function for KVM
CPUs only, but since in fact the same code works just
fine in KVM and TCG we reuse it.

I know that the macOS Hypervisor.Framework APIs are rather
lower-level than KVM (they do less work in the kernel and
push more of it onto userspace); it's possible that there
we might find more situations where userspace needs to do
"make the guest CPU take an exception"; I haven't investigated.

[*] The two special cases are:
 (1) the vcpu thread got a SIGBUS indicating a memory error,
 and we need to deliver a synchronous external abort
 exception to the guest to let it know about the error
 (2) the kernel told us about a debug exception (breakpoint,
 watchpoint, etc) but it turns out not to be for one of
 QEMU's own gdbstub breakpoints/watchpoints, so it
 must be one the guest itself has set up, and so we need
 to deliver it to the guest

These are fairly obscure, and it wouldn't surprise me if
other target archs had picked a different separation of
concerns between userspace and the kernel such that userspace
didn't need to manually deliver an exception.

thanks
-- PMM

[Bug 1907137] [NEW] LDTR not properly emulated when MTE tag checks enabled at EL0

[Peter Collingbourne]

Public bug reported:

I am trying to boot Android (just the non-GUI parts for now) under QEMU
with MTE enabled. This can be done by following the instructions here to
build the fvp-eng target with MTE support:

https://cs.android.com/android/platform/superproject/+/master:device/generic/goldfish/fvpbase/

and launching QEMU with the following command:

qemu-system-aarch64 -kernel $ANDROID_PRODUCT_OUT/kernel -initrd
$ANDROID_PRODUCT_OUT/combined-ramdisk.img -machine virt,mte=on -cpu max
-drive driver=raw,file=$ANDROID_PRODUCT_OUT/system-
qemu.img,if=none,id=system -device virtio-blk-device,drive=system
-append "console=ttyAMA0 earlyprintk=ttyAMA0
androidboot.hardware=fvpbase
androidboot.boot_devices=a003e00.virtio_mmio loglevel=9
printk.devkmsg=on buildvariant=eng" -m 512 -nographic -no-reboot

If I do this then QEMU crashes like so:

**
ERROR:../target/arm/mte_helper.c:558:mte_check_fail: code should not be reached
Bail out! ERROR:../target/arm/mte_helper.c:558:mte_check_fail: code should not 
be reached

The error is caused by an MTE tag check fault from an LDTR instruction
in __arch_copy_from_user. At this point TCF=0 and TCF0=2.

I have this patch that gets me past the error but it is unclear whether
this is the correct fix since there may be other confusion between TCF
and TCF0 elsewhere.

diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
index 153bd1e9df..aa5db4eac4 100644
--- a/target/arm/mte_helper.c
+++ b/target/arm/mte_helper.c
@@ -552,10 +552,8 @@ static void mte_check_fail(CPUARMState *env, uint32_t desc,
 case 0:
 /*
  * Tag check fail does not affect the PE.
- * We eliminate this case by not setting MTE_ACTIVE
- * in tb_flags, so that we never make this runtime call.
  */
-g_assert_not_reached();
+break;
 
 case 2:
 /* Tag check fail causes asynchronous flag set.  */

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  LDTR not properly emulated when MTE tag checks enabled at EL0

Status in QEMU:
  New

Bug description:
  I am trying to boot Android (just the non-GUI parts for now) under
  QEMU with MTE enabled. This can be done by following the instructions
  here to build the fvp-eng target with MTE support:

  
https://cs.android.com/android/platform/superproject/+/master:device/generic/goldfish/fvpbase/

  and launching QEMU with the following command:

  qemu-system-aarch64 -kernel $ANDROID_PRODUCT_OUT/kernel -initrd
  $ANDROID_PRODUCT_OUT/combined-ramdisk.img -machine virt,mte=on -cpu
  max -drive driver=raw,file=$ANDROID_PRODUCT_OUT/system-
  qemu.img,if=none,id=system -device virtio-blk-device,drive=system
  -append "console=ttyAMA0 earlyprintk=ttyAMA0
  androidboot.hardware=fvpbase
  androidboot.boot_devices=a003e00.virtio_mmio loglevel=9
  printk.devkmsg=on buildvariant=eng" -m 512 -nographic -no-reboot

  If I do this then QEMU crashes like so:

  **
  ERROR:../target/arm/mte_helper.c:558:mte_check_fail: code should not be 
reached
  Bail out! ERROR:../target/arm/mte_helper.c:558:mte_check_fail: code should 
not be reached

  The error is caused by an MTE tag check fault from an LDTR instruction
  in __arch_copy_from_user. At this point TCF=0 and TCF0=2.

  I have this patch that gets me past the error but it is unclear
  whether this is the correct fix since there may be other confusion
  between TCF and TCF0 elsewhere.

  diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
  index 153bd1e9df..aa5db4eac4 100644
  --- a/target/arm/mte_helper.c
  +++ b/target/arm/mte_helper.c
  @@ -552,10 +552,8 @@ static void mte_check_fail(CPUARMState *env, uint32_t 
desc,
   case 0:
   /*
* Tag check fail does not affect the PE.
  - * We eliminate this case by not setting MTE_ACTIVE
  - * in tb_flags, so that we never make this runtime call.
*/
  -g_assert_not_reached();
  +break;
   
   case 2:
   /* Tag check fail causes asynchronous flag set.  */

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

Re: runaway avocado

[John Snow]

On 10/26/20 8:28 PM, Cleber Rosa wrote:

On Mon, Oct 26, 2020 at 11:43:36PM +0100, Philippe Mathieu-Daudé wrote:

Cc'ing avocado-devel@

On 10/26/20 11:35 PM, Peter Maydell wrote:

So, I somehow ended up with this process still running on my
local machine after a (probably failed) 'make check-acceptance':

petmay01 13710 99.7  3.7 2313448 1235780 pts/16 Sl  16:10 378:00
./qemu-system-aarch64 -display none -vga none -chardev
socket,id=mon,path=/var/tmp/tmp5szft2yi/qemu-13290-monitor.sock -mon
chardev=mon,mode=control -machine virt -chardev
socket,id=console,path=/var/tmp/tmp5szft2yi/qemu-13290-console.sock,server,nowait
-serial chardev:console -icount
shift=7,rr=record,rrfile=/var/tmp/avocado_iv8dehpo/avocado_job_w9efukj5/32-tests_acceptance_reverse_debugging.py_ReverseDebugging_AArch64.test_aarch64_virt/replay.bin,rrsnapshot=init
-net none -drive
file=/var/tmp/avocado_iv8dehpo/avocado_job_w9efukj5/32-tests_acceptance_reverse_debugging.py_ReverseDebugging_AArch64.test_aarch64_virt/disk.qcow2,if=none
-kernel 
/home/petmay01/avocado/data/cache/by_location/a00ac4ae676ef0322126abd2f7d38f50cc9cbc95/vmlinuz
-cpu cortex-a53

and it was continuing to log to a deleted file
/var/tmp/avocado_iv8dehpo/avocado_job_w9efukj5/32-tests_acceptance_reverse_debugging.py_ReverseDebugging_AArch64.test_aarch64_virt/replay.bin

which was steadily eating my disk space and got up to nearly 100GB
in used disk (invisible to du, of course, since it was an unlinked
file) before I finally figured out what was going on and killed it
about six hours later...



Ouch!


Any suggestions for how we might improve the robustness of the
relevant test ?



While this test may be less robust/reliable than others, the core
issue is that the automatic shutdown of the QEMU "vms" can be
improved.  My best guess is that this specific test ended in ERROR,
and (or because?) the tearDown() method failed to end these processes.

All tests can be improved at once by adding a second, even more
forceful round of shutdown.  Currently the process gets, in the worst
case scenario, a SIGKILL.

But, in addition to that, an upper layer above the test could be given
the responsibility to look for and clean up resouces initiated by a
test.  The Avocado job has hooks for running callbacks right before
its own process exits, but, with the new Avocado architecture (AKA "N(ext)
Runner") this should probably be implemented as async cleanup actions
that begin right after a test ends.

I'll give the "second more forceful round of shutdown" approach some
and testing, and in addition to that, open an issue to track the upper
layer resource cleanup on Avocado.



machine.py should have a timeout that it adheres to, unless it was 
disabled explicitly -- then I guess it can't help you.


--js

[RFC PATCH] python: add __repr__ to ConsoleSocket to aid debugging

[Alex Bennée]

While attempting to debug some console weirdness I thought it would be
worth making it easier to see what it had inside.

Signed-off-by: Alex Bennée 
---
 python/qemu/console_socket.py | 8 
 1 file changed, 8 insertions(+)

diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index f060d79e06..77966d1fe9 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -45,6 +45,14 @@ class ConsoleSocket(socket.socket):
 if drain:
 self._drain_thread = self._thread_start()
 
+def __repr__(self):
+s = super(ConsoleSocket, self).__repr__()
+s = s.rstrip(">")
+s += ", logfile=%s" % (self._logfile)
+s += ", drain_thread=%s" % (self._drain_thread)
+s += ">"
+return s
+
 def _drain_fn(self) -> None:
 """Drains the socket and runs while the socket is open."""
 while self._open:
-- 
2.20.1

Re: [PATCH 1/3] virtiofsd: Set up posix_lock hash table for root inode

[Vivek Goyal]

We setup per inode hash table ->posix_lock to support remote posix locks.
But we forgot to initialize this table for root inode.

Laszlo managed to trigger an issue where he sent a FUSE_FLUSH request for
root inode and lo_flush() found inode with inode->posix_lock NULL and
accessing this table crashed virtiofsd.

May be we can get rid of initializing this hash table for directory
objects completely. But that optimization is for another day.

Reported-by: Laszlo Ersek 
Signed-off-by: Vivek Goyal 
---
 tools/virtiofsd/passthrough_ll.c |7 +++
 1 file changed, 7 insertions(+)

Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
===
--- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c 2020-12-07 
14:46:22.198359486 -0500
+++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c  2020-12-07 
14:48:07.873737472 -0500
@@ -3372,6 +3372,9 @@ static void setup_root(struct lo_data *l
 root->key.mnt_id = mnt_id;
 root->nlookup = 2;
 g_atomic_int_set(>refcount, 2);
+pthread_mutex_init(>plock_mutex, NULL);
+root->posix_locks = g_hash_table_new_full(
+g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
 }
 
 static guint lo_key_hash(gconstpointer key)
@@ -3394,6 +3397,10 @@ static void fuse_lo_data_cleanup(struct
 if (lo->inodes) {
 g_hash_table_destroy(lo->inodes);
 }
+
+if (lo->root.posix_locks) {
+g_hash_table_destroy(lo->root.posix_locks);
+}
 lo_map_destroy(>fd_map);
 lo_map_destroy(>dirp_map);
 lo_map_destroy(>ino_map);

Re: [PATCH v2] python, tests: do not use short-form boolean options

[John Snow]

On 11/20/20 10:40 AM, Paolo Bonzini wrote:

They are going to be deprecated, avoid warnings on stdout while the
tests run.

Signed-off-by: Paolo Bonzini 
---
  python/qemu/machine.py   | 2 +-


Acked-by: John Snow 


  tests/qtest/pflash-cfi02-test.c  | 4 ++--
  tests/qtest/test-filter-redirector.c | 8 
  tests/qtest/vhost-user-test.c| 8 
  tests/test-char.c| 8 
  5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6420f01bed..6216530b0d 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -292,7 +292,7 @@ class QEMUMachine:
  for _ in range(self._console_index):
  args.extend(['-serial', 'null'])
  if self._console_set:
-chardev = ('socket,id=console,path=%s,server,nowait' %
+chardev = ('socket,id=console,path=%s,server=on,wait=off' %
 self._console_address)
  args.extend(['-chardev', chardev])
  if self._console_device_type is None:
diff --git a/tests/qtest/pflash-cfi02-test.c b/tests/qtest/pflash-cfi02-test.c
index afb702b565..60db81a3a2 100644
--- a/tests/qtest/pflash-cfi02-test.c
+++ b/tests/qtest/pflash-cfi02-test.c
@@ -261,7 +261,7 @@ static void test_geometry(const void *opaque)
  const FlashConfig *config = opaque;
  QTestState *qtest;
  qtest = qtest_initf("-M musicpal"
-" -drive if=pflash,file=%s,format=raw,copy-on-read"
+" -drive if=pflash,file=%s,format=raw,copy-on-read=on"
  /* Device geometry properties. */
  " -global driver=cfi.pflash02,"
  "property=num-blocks0,value=%d"
@@ -581,7 +581,7 @@ static void test_cfi_in_autoselect(const void *opaque)
  const FlashConfig *config = opaque;
  QTestState *qtest;
  qtest = qtest_initf("-M musicpal"
-" -drive if=pflash,file=%s,format=raw,copy-on-read",
+" -drive if=pflash,file=%s,format=raw,copy-on-read=on",
  image_path);
  FlashConfig explicit_config = expand_config_defaults(config);
  explicit_config.qtest = qtest;
diff --git a/tests/qtest/test-filter-redirector.c 
b/tests/qtest/test-filter-redirector.c
index 829db8c2ea..4269b2cdd9 100644
--- a/tests/qtest/test-filter-redirector.c
+++ b/tests/qtest/test-filter-redirector.c
@@ -95,8 +95,8 @@ static void test_redirector_tx(void)
  qts = qtest_initf(
  "-netdev socket,id=qtest-bn0,fd=%d "
  "-device %s,netdev=qtest-bn0,id=qtest-e0 "
-"-chardev socket,id=redirector0,path=%s,server,nowait "
-"-chardev socket,id=redirector1,path=%s,server,nowait "
+"-chardev socket,id=redirector0,path=%s,server=on,wait=off "
+"-chardev socket,id=redirector1,path=%s,server=on,wait=off "
  "-chardev socket,id=redirector2,path=%s "
  "-object filter-redirector,id=qtest-f0,netdev=qtest-bn0,"
  "queue=tx,outdev=redirector0 "
@@ -165,8 +165,8 @@ static void test_redirector_rx(void)
  qts = qtest_initf(
  "-netdev socket,id=qtest-bn0,fd=%d "
  "-device %s,netdev=qtest-bn0,id=qtest-e0 "
-"-chardev socket,id=redirector0,path=%s,server,nowait "
-"-chardev socket,id=redirector1,path=%s,server,nowait "
+"-chardev socket,id=redirector0,path=%s,server=on,wait=off "
+"-chardev socket,id=redirector1,path=%s,server=on,wait=off "
  "-chardev socket,id=redirector2,path=%s "
  "-object filter-redirector,id=qtest-f0,netdev=qtest-bn0,"
  "queue=rx,indev=redirector0 "
diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 3df5322614..1a5f5313ff 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -537,7 +537,7 @@ static void test_server_create_chr(TestServer *server, 
const gchar *opt)
  
  static void test_server_listen(TestServer *server)

  {
-test_server_create_chr(server, ",server,nowait");
+test_server_create_chr(server, ",server=on,wait=off");
  }
  
  static void test_server_free(TestServer *server)

@@ -846,7 +846,7 @@ static void *vhost_user_test_setup_reconnect(GString 
*cmd_line, void *arg)
  
  g_thread_new("connect", connect_thread, s);

  append_mem_opts(s, cmd_line, 256, TEST_MEMFD_AUTO);
-s->vu_ops->append_opts(s, cmd_line, ",server");
+s->vu_ops->append_opts(s, cmd_line, ",server=on");
  
  g_test_queue_destroy(vhost_user_test_cleanup, s);
  
@@ -883,7 +883,7 @@ static void *vhost_user_test_setup_connect_fail(GString *cmd_line, void *arg)
  
  g_thread_new("connect", connect_thread, s);

  append_mem_opts(s, cmd_line, 256, TEST_MEMFD_AUTO);
-s->vu_ops->append_opts(s, cmd_line, ",server");
+s->vu_ops->append_opts(s, cmd_line, ",server=on");
  
  g_test_queue_destroy(vhost_user_test_cleanup, s);
  
@@ -898,7 

Re: [PATCH] python, tests: do not use short-form boolean options

[John Snow]

On 11/17/20 9:32 AM, Markus Armbruster wrote:

Paolo Bonzini  writes:


On 17/11/20 10:20, Markus Armbruster wrote:

-chardev = ('socket,id=console,path=%s,server,nowait' %
+chardev = ('socket,id=console,path=%s,server=yes,wait=no' %


Let's stick to the canonical 'on' and 'off'.


That was on purpose (for extra coverage and not just because variety is
the spice of life).  But I can use the canonical values as well if you
prefer.


I think the place for extra coverage is tests/test-qemu-opts.c.




Seconded; steer machine.py et al to something like a reference 
implementation for how we want people to drive QEMU.


--js

Re: [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201207183021.22752-1-vgo...@redhat.com/



Hi,

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

Type: series
Message-id: 20201207183021.22752-1-vgo...@redhat.com
Subject: [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20201207183021.22752-1-vgo...@redhat.com -> 
patchew/20201207183021.22752-1-vgo...@redhat.com
Switched to a new branch 'test'
86f3bb1 virtiofsd: Check file type in lo_flush()
92a5c9d virtiofsd: Disable posix_lock hash table if remote locks are not enabled
fbdcaf1 virtiofsd: Set up posix_lock hash table for root inode

=== OUTPUT BEGIN ===
1/3 Checking commit fbdcaf172aed (virtiofsd: Set up posix_lock hash table for 
root inode)
ERROR: suspect code indent for conditional statements (4, 7)
#40: FILE: tools/virtiofsd/passthrough_ll.c:3401:
+if (lo->root.posix_locks)
+   g_hash_table_destroy(lo->root.posix_locks);

ERROR: braces {} are necessary for all arms of this statement
#40: FILE: tools/virtiofsd/passthrough_ll.c:3401:
+if (lo->root.posix_locks)
[...]

total: 2 errors, 0 warnings, 19 lines checked

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

2/3 Checking commit 92a5c9d35c7d (virtiofsd: Disable posix_lock hash table if 
remote locks are not enabled)
3/3 Checking commit 86f3bb1cf5b8 (virtiofsd: Check file type in lo_flush())
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [PATCH 2/8] acpi: cpuhp: introduce 'firmware performs eject' status/control bits

[Ankur Arora]

On 2020-12-07 4:48 a.m., Igor Mammedov wrote:

On Mon, 7 Dec 2020 00:47:13 -0800
Ankur Arora  wrote:


On 2020-12-06 10:31 p.m., Ankur Arora wrote:

On 2020-12-04 9:09 a.m., Igor Mammedov wrote:

Adds bit #4 to status/control field of CPU hotplug MMIO interface.
New bit will be used OSPM to mark CPUs as pending for removal by firmware,
when it calls _EJ0 method on CPU device node. Later on, when firmware
sees this bit set, it will perform CPU eject which will clear bit #4
as well.

Signed-off-by: Igor Mammedov 
---
v1:
    - rearrange status/control bits description (Laszlo)
    - add clear bit #4 on eject
    - drop toggling logic from bit #4, it can be only set by guest
  and clear as part of cpu eject
    - exclude boot CPU from remove request
    - add trace events for new bit
---
   include/hw/acpi/cpu.h   |  1 +
   docs/specs/acpi_cpu_hotplug.txt | 19 ++-
   hw/acpi/cpu.c   |  9 +
   hw/acpi/trace-events    |  2 ++
   4 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 0eeedaa491..d71edde456 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
   uint64_t arch_id;
   bool is_inserting;
   bool is_removing;
+    bool fw_remove;
   uint32_t ost_event;
   uint32_t ost_status;
   } AcpiCpuStatus;
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index 9bb22d1270..9bd59ae0da 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -56,8 +56,11 @@ read access:
     no device check event to OSPM was issued.
     It's valid only when bit 0 is set.
  2: Device remove event, used to distinguish device for which
-  no device eject request to OSPM was issued.
-   3-7: reserved and should be ignored by OSPM
+  no device eject request to OSPM was issued. Firmware must
+  ignore this bit.
+   3: reserved and should be ignored by OSPM
+   4: if set to 1, OSPM requests firmware to perform device eject.
+   5-7: reserved and should be ignored by OSPM
   [0x5-0x7] reserved
   [0x8] Command data: (DWORD access)
     contains 0 unless value last stored in 'Command field' is one of:
@@ -79,10 +82,16 @@ write access:
  selected CPU device
   2: if set to 1 clears device remove event, set by OSPM
  after it has emitted device eject request for the
-   selected CPU device
+   selected CPU device.
   3: if set to 1 initiates device eject, set by OSPM when it
-   triggers CPU device removal and calls _EJ0 method
-    4-7: reserved, OSPM must clear them before writing to register
+   triggers CPU device removal and calls _EJ0 method or by firmware
+   when bit #4 is set. In case bit #4 were set, it's cleared as
+   part of device eject.


So I spent some time testing my OVMF series alongside this.
Just sent out the code on the EDK2 list.

To do the eject, I'm setting bit#3 after queuing up the processor
for removal (via RemoveProcessor()).

This means, however, that the unplug in QEMU would happen before the
real firmware unplug (which'll happen in SmmCpuUpdate()).

Clearly, the right place for eject would be either in the appropriate
APHandler() or in the BSPHandler() after all the APs have been waited
for but from my reading of related code I don't see any infrastructure
for doing this (admittedly, I don't know the EDK2 source well enough
so it's likely I missed something.)

The other possibility might be to do it in the _EJ0 method itself
after we return from the SMI:

@@ -479,9 +515,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, 
CPUHotplugFeatures opts,
   aml_append(method, aml_store(one, fw_ej_evt));
   aml_append(method, aml_store(aml_int(OVMF_CPUHP_SMI_CMD),
  aml_name("%s", opts.smi_path)));
-} else {
-aml_append(method, aml_store(one, ej_evt));
-}
+   }
+aml_append(method, aml_store(one, ej_evt));
   aml_append(method, aml_release(ctrl_lock));

This seems to work but it is possible I'm missing something here.


theoretically this leaves unaccounted CPU on fw side,


On the guest side right? As in the firmware has marked it gone but it's not
really gone.


what if SMM is entered again before CPU is ejected or OS doesn't eject it on 
purpose?


Yeah both of those things would be a mess. I'm not even sure it would enter the
SMM again -- given that the SMM has marked it gone.



I'd prefer firmware to remove CPU before returning from SMM.


That would be ideal though I don't yet see a mechanism for doing that. Laszlo
might have better ideas though.

Another possibility 

Re: [PATCH v5 0/2] MTE support for KVM guest

[Marc Zyngier]

On Mon, 07 Dec 2020 16:34:05 +,
Catalin Marinas  wrote:
> 
> On Mon, Dec 07, 2020 at 04:05:55PM +, Marc Zyngier wrote:
> > What I'd really like to see is a description of how shared memory
> > is, in general, supposed to work with MTE. My gut feeling is that
> > it doesn't, and that you need to turn MTE off when sharing memory
> > (either implicitly or explicitly).
> 
> The allocation tag (in-memory tag) is a property assigned to a physical
> address range and it can be safely shared between different processes as
> long as they access it via pointers with the same allocation tag (bits
> 59:56). The kernel enables such tagged shared memory for user processes
> (anonymous, tmpfs, shmem).

I think that's one case where the shared memory scheme breaks, as we
have two kernels in charge of their own tags, and they obviously can't
be synchronised

> What we don't have in the architecture is a memory type which allows
> access to tags but no tag checking. To access the data when the tags
> aren't known, the tag checking would have to be disabled via either a
> prctl() or by setting the PSTATE.TCO bit.

I guess that's point (3) in Steven's taxonomy. It still a bit ugly to
fit in an existing piece of userspace, specially if it wants to use
MTE for its own benefit.

> The kernel accesses the user memory via the linear map using a match-all
> tag 0xf, so no TCO bit toggling. For user, however, we disabled such
> match-all tag and it cannot be enabled at run-time (at least not easily,
> it's cached in the TLB). However, we already have two modes to disable
> tag checking which Qemu could use when migrating data+tags.

I wonder whether we will have to have something kernel side to
dump/reload tags in a way that matches the patterns used by live
migration.

M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices

[Thomas Huth]

On 07/12/2020 17.34, Thomas Huth wrote:
> On 07/12/2020 17.30, Cornelia Huck wrote:
>> On Mon, 7 Dec 2020 15:28:47 +0100
>> Thomas Huth  wrote:
>>
>>> On 04/12/2020 13.14, Cornelia Huck wrote:
 Hotplug a virtio-net-ccw device, and then hotunplug it again.

 Signed-off-by: Cornelia Huck 
 ---
>
[...]
 +exec_command_and_wait_for_pattern(self, 'ls 
 /sys/bus/ccw/devices/',
 +  '0.0.4711')
 +# and detach it again
 +exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ')  
>>>
>>> If adapt my above change, you could also get rid of this dmesg -c here
>>> (since it's done in the while loop already)
>>
>> I don't think so (there are two CRWs posted, and the loop might have
>> caught the first one only.)
> 
> Oh, you're right. So let's better be safe than sorry and keep this dmesg -c.

Ok, as we had to discover during some testing, it's a bad idea to only wait
for ' ' after the 'dmesg -c' since it matches too early, so that the device
gets added while the dmesg command is still running.

The following code is working for me instead:

 # add another device
 exec_command_and_wait_for_pattern(self,
 'dmesg -c > /dev/null; echo dm-clear\ 1', 'dm-clear 1')
 self.vm.command('device_add', driver='virtio-net-ccw',
 devno='fe.0.4711', id='net_4711')
 exec_command_and_wait_for_pattern(self,
 'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
 'CRW reports')
 exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/',
   '0.0.4711')
 # and detach it again
 exec_command_and_wait_for_pattern(self,
 'dmesg -c > /dev/null; echo dm-clear\ 2', 'dm-clear 2')
 self.vm.command('device_del', id='net_4711')
 self.vm.event_wait(name='DEVICE_DELETED',
match={'data': {'device': 'net_4711'}})
 exec_command_and_wait_for_pattern(self,
 'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
 'CRW reports')
 exec_command_and_wait_for_pattern(self,
   'ls /sys/bus/ccw/devices/0.0.4711',
   'No such file or directory')

 HTH,
  Thomas

[PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[Vivek Goyal]

Laszlo is writing a virtiofs client for OVMF and noticed that if he
sends fuse FLUSH command for directory object, virtiofsd crashes.
virtiofsd does not expect a FLUSH arriving for a directory object.

This patch series has one of the patches which fixes that. It also
has couple of posix lock fixes as a result of lo_flush() related debugging.

Vivek Goyal (3):
  virtiofsd: Set up posix_lock hash table for root inode
  virtiofsd: Disable posix_lock hash table if remote locks are not
enabled
  virtiofsd: Check file type in lo_flush()

 tools/virtiofsd/passthrough_ll.c | 54 +++-
 1 file changed, 39 insertions(+), 15 deletions(-)

-- 
2.25.4

[PATCH 2/3] virtiofsd: Disable posix_lock hash table if remote locks are not enabled

[Vivek Goyal]

If remote posix locks are not enabled (lo->posix_lock == false), then disable
code paths taken to initialize inode->posix_lock hash table and corresponding
destruction and search etc.

lo_getlk() and lo_setlk() have been modified to return ENOSYS if daemon
does not support posix lock but client still sends a lock/unlock request.

Signed-off-by: Vivek Goyal 
---
 tools/virtiofsd/passthrough_ll.c | 51 +---
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 59202a843b..8ba79f503a 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -914,10 +914,11 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t 
parent, const char *name,
 inode->key.ino = e->attr.st_ino;
 inode->key.dev = e->attr.st_dev;
 inode->key.mnt_id = mnt_id;
-pthread_mutex_init(>plock_mutex, NULL);
-inode->posix_locks = g_hash_table_new_full(
-g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
-
+if (lo->posix_lock) {
+pthread_mutex_init(>plock_mutex, NULL);
+inode->posix_locks = g_hash_table_new_full(
+g_direct_hash, g_direct_equal, NULL, 
posix_locks_value_destroy);
+}
 pthread_mutex_lock(>mutex);
 inode->fuse_ino = lo_add_inode_mapping(req, inode);
 g_hash_table_insert(lo->inodes, >key, inode);
@@ -1303,12 +1304,13 @@ static void unref_inode(struct lo_data *lo, struct 
lo_inode *inode, uint64_t n)
 if (!inode->nlookup) {
 lo_map_remove(>ino_map, inode->fuse_ino);
 g_hash_table_remove(lo->inodes, >key);
-if (g_hash_table_size(inode->posix_locks)) {
-fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
+if (lo->posix_lock) {
+if (g_hash_table_size(inode->posix_locks)) {
+fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
+}
+g_hash_table_destroy(inode->posix_locks);
+pthread_mutex_destroy(>plock_mutex);
 }
-g_hash_table_destroy(inode->posix_locks);
-pthread_mutex_destroy(>plock_mutex);
-
 /* Drop our refcount from lo_do_lookup() */
 lo_inode_put(lo, );
 }
@@ -1784,6 +1786,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino, 
struct fuse_file_info *fi,
  ino, fi->flags, fi->lock_owner, lock->l_type, lock->l_start,
  lock->l_len);
 
+if (!lo->posix_lock) {
+fuse_reply_err(req, ENOSYS);
+return;
+}
+
 inode = lo_inode(req, ino);
 if (!inode) {
 fuse_reply_err(req, EBADF);
@@ -1829,6 +1836,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, 
struct fuse_file_info *fi,
  ino, fi->flags, lock->l_type, lock->l_pid, fi->lock_owner, sleep,
  lock->l_whence, lock->l_start, lock->l_len);
 
+if (!lo->posix_lock) {
+fuse_reply_err(req, ENOSYS);
+return;
+}
+
 if (sleep) {
 fuse_reply_err(req, EOPNOTSUPP);
 return;
@@ -1953,6 +1965,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, 
struct fuse_file_info *fi)
 int res;
 (void)ino;
 struct lo_inode *inode;
+struct lo_data *lo = lo_data(req);
 
 inode = lo_inode(req, ino);
 if (!inode) {
@@ -1961,12 +1974,14 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, 
struct fuse_file_info *fi)
 }
 
 /* An fd is going away. Cleanup associated posix locks */
-pthread_mutex_lock(>plock_mutex);
-g_hash_table_remove(inode->posix_locks, GUINT_TO_POINTER(fi->lock_owner));
-pthread_mutex_unlock(>plock_mutex);
-
+if (lo->posix_lock) {
+pthread_mutex_lock(>plock_mutex);
+g_hash_table_remove(inode->posix_locks,
+GUINT_TO_POINTER(fi->lock_owner));
+pthread_mutex_unlock(>plock_mutex);
+}
 res = close(dup(lo_fi_fd(req, fi)));
-lo_inode_put(lo_data(req), );
+lo_inode_put(lo, );
 fuse_reply_err(req, res == -1 ? errno : 0);
 }
 
@@ -3372,9 +3387,11 @@ static void setup_root(struct lo_data *lo, struct 
lo_inode *root)
 root->key.mnt_id = mnt_id;
 root->nlookup = 2;
 g_atomic_int_set(>refcount, 2);
-pthread_mutex_init(>plock_mutex, NULL);
-root->posix_locks = g_hash_table_new_full(
-g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
+if (lo->posix_lock) {
+pthread_mutex_init(>plock_mutex, NULL);
+root->posix_locks = g_hash_table_new_full(
+g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
+}
 }
 
 static guint lo_key_hash(gconstpointer key)
-- 
2.25.4