Re: [PATCH v2] vhost-vdpa: Do not send empty IOTLB update batches

[Jason Wang]

On Thu, Aug 12, 2021 at 2:43 PM Eugenio Perez Martin
 wrote:
>
> On Thu, Aug 12, 2021 at 8:16 AM Jason Wang  wrote:
> >
> > On Thu, Aug 12, 2021 at 12:32 AM Eugenio Pérez  wrote:
> > >
> > > With the introduction of the batch hinting, meaningless batches can be
> > > created with no IOTLB updates if the memory region was skipped by
> > > vhost_vdpa_listener_skipped_section. This is the case of host notifiers
> > > memory regions, device un/realize, and others. This causes the vdpa
> > > device to receive dma mapping settings with no changes, a possibly
> > > expensive operation for nothing.
> > >
> > > To avoid that, VHOST_IOTLB_BATCH_BEGIN hint is delayed until we have a
> > > meaningful (not skipped section) mapping or unmapping operation, and
> > > VHOST_IOTLB_BATCH_END is not written unless at least one of _UPDATE /
> > > _INVALIDATE has been issued.
> > >
> > > Signed-off-by: Eugenio Pérez 
> > > ---
> > >  include/hw/virtio/vhost-vdpa.h |  1 +
> > >  hw/virtio/vhost-vdpa.c | 50 ++
> > >  2 files changed, 39 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/include/hw/virtio/vhost-vdpa.h 
> > > b/include/hw/virtio/vhost-vdpa.h
> > > index e98e327f12..6538572a6f 100644
> > > --- a/include/hw/virtio/vhost-vdpa.h
> > > +++ b/include/hw/virtio/vhost-vdpa.h
> > > @@ -23,6 +23,7 @@ typedef struct vhost_vdpa {
> > >  int device_fd;
> > >  int index;
> > >  uint32_t msg_type;
> > > +size_t n_mr_updated;
> > >  MemoryListener listener;
> > >  struct vhost_dev *dev;
> > >  VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > index 6ce94a1f4d..512fa18d68 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -89,19 +89,13 @@ static int vhost_vdpa_dma_unmap(struct vhost_vdpa *v, 
> > > hwaddr iova,
> > >  return ret;
> > >  }
> > >
> > > -static void vhost_vdpa_listener_begin(MemoryListener *listener)
> > > +static void vhost_vdpa_listener_begin_batch(struct vhost_vdpa *v)
> > >  {
> > > -struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, 
> > > listener);
> > > -struct vhost_dev *dev = v->dev;
> > > -struct vhost_msg_v2 msg = {};
> > >  int fd = v->device_fd;
> > > -
> > > -if (!(dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH))) {
> > > -return;
> > > -}
> > > -
> > > -msg.type = v->msg_type;
> > > -msg.iotlb.type = VHOST_IOTLB_BATCH_BEGIN;
> > > +struct vhost_msg_v2 msg = {
> > > +.type = v->msg_type,
> > > +.iotlb.type = VHOST_IOTLB_BATCH_BEGIN,
> > > +};
> > >
> > >  if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) {
> > >  error_report("failed to write, fd=%d, errno=%d (%s)",
> > > @@ -109,6 +103,25 @@ static void vhost_vdpa_listener_begin(MemoryListener 
> > > *listener)
> > >  }
> > >  }
> > >
> > > +static bool vhost_vdpa_iotlb_batch_is_started(const struct vhost_vdpa *v)
> > > +{
> > > +return v->n_mr_updated != 0;
> > > +}
> > > +
> > > +static void vhost_vdpa_iotlb_batch_begin_once(struct vhost_vdpa *v)
> > > +{
> > > +if (!vhost_vdpa_iotlb_batch_is_started(v)) {
> > > +vhost_vdpa_listener_begin_batch(v);
> > > +}
> > > +
> > > +v->n_mr_updated++;
> > > +}
> > > +
> > > +static void vhost_vdpa_iotlb_batch_reset(struct vhost_vdpa *v)
> > > +{
> > > +v->n_mr_updated = 0;
> > > +}
> > > +
> > >  static void vhost_vdpa_listener_commit(MemoryListener *listener)
> > >  {
> > >  struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, 
> > > listener);
> > > @@ -120,6 +133,10 @@ static void 
> > > vhost_vdpa_listener_commit(MemoryListener *listener)
> > >  return;
> > >  }
> > >
> > > +if (vhost_vdpa_iotlb_batch_is_started(v)) {
> > > +return;
> > > +}
> > > +
> > >  msg.type = v->msg_type;
> > >  msg.iotlb.type = VHOST_IOTLB_BATCH_END;
> > >
> > > @@ -127,6 +144,8 @@ static void vhost_vdpa_listener_commit(MemoryListener 
> > > *listener)
> > >  error_report("failed to write, fd=%d, errno=%d (%s)",
> > >   fd, errno, strerror(errno));
> > >  }
> > > +
> > > +vhost_vdpa_iotlb_batch_reset(v);
> > >  }
> > >
> > >  static void vhost_vdpa_listener_region_add(MemoryListener *listener,
> > > @@ -170,6 +189,10 @@ static void 
> > > vhost_vdpa_listener_region_add(MemoryListener *listener,
> > >
> > >  llsize = int128_sub(llend, int128_make64(iova));
> > >
> > > +if (v->dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH)) {
> >
> > Let's move this in to vhost_vdpa_iotlb_batch_begin_once()?
> >
>
> Sure
>
> > > +vhost_vdpa_iotlb_batch_begin_once(v);
> > > +}
> > > +
> > >  ret = vhost_vdpa_dma_map(v, iova, int128_get64(llsize),
> > >   vaddr, section->readonly);
> > >  if (ret) {
> > > @@ -221,6 +244,10 @@ static void 
> > > vhost_vdpa_listener_region_del(MemoryListene

Re: [PATCH v2] vhost-vdpa: Do not send empty IOTLB update batches

[Eugenio Perez Martin]

On Thu, Aug 12, 2021 at 8:16 AM Jason Wang  wrote:
>
> On Thu, Aug 12, 2021 at 12:32 AM Eugenio Pérez  wrote:
> >
> > With the introduction of the batch hinting, meaningless batches can be
> > created with no IOTLB updates if the memory region was skipped by
> > vhost_vdpa_listener_skipped_section. This is the case of host notifiers
> > memory regions, device un/realize, and others. This causes the vdpa
> > device to receive dma mapping settings with no changes, a possibly
> > expensive operation for nothing.
> >
> > To avoid that, VHOST_IOTLB_BATCH_BEGIN hint is delayed until we have a
> > meaningful (not skipped section) mapping or unmapping operation, and
> > VHOST_IOTLB_BATCH_END is not written unless at least one of _UPDATE /
> > _INVALIDATE has been issued.
> >
> > Signed-off-by: Eugenio Pérez 
> > ---
> >  include/hw/virtio/vhost-vdpa.h |  1 +
> >  hw/virtio/vhost-vdpa.c | 50 ++
> >  2 files changed, 39 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > index e98e327f12..6538572a6f 100644
> > --- a/include/hw/virtio/vhost-vdpa.h
> > +++ b/include/hw/virtio/vhost-vdpa.h
> > @@ -23,6 +23,7 @@ typedef struct vhost_vdpa {
> >  int device_fd;
> >  int index;
> >  uint32_t msg_type;
> > +size_t n_mr_updated;
> >  MemoryListener listener;
> >  struct vhost_dev *dev;
> >  VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 6ce94a1f4d..512fa18d68 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -89,19 +89,13 @@ static int vhost_vdpa_dma_unmap(struct vhost_vdpa *v, 
> > hwaddr iova,
> >  return ret;
> >  }
> >
> > -static void vhost_vdpa_listener_begin(MemoryListener *listener)
> > +static void vhost_vdpa_listener_begin_batch(struct vhost_vdpa *v)
> >  {
> > -struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, 
> > listener);
> > -struct vhost_dev *dev = v->dev;
> > -struct vhost_msg_v2 msg = {};
> >  int fd = v->device_fd;
> > -
> > -if (!(dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH))) {
> > -return;
> > -}
> > -
> > -msg.type = v->msg_type;
> > -msg.iotlb.type = VHOST_IOTLB_BATCH_BEGIN;
> > +struct vhost_msg_v2 msg = {
> > +.type = v->msg_type,
> > +.iotlb.type = VHOST_IOTLB_BATCH_BEGIN,
> > +};
> >
> >  if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) {
> >  error_report("failed to write, fd=%d, errno=%d (%s)",
> > @@ -109,6 +103,25 @@ static void vhost_vdpa_listener_begin(MemoryListener 
> > *listener)
> >  }
> >  }
> >
> > +static bool vhost_vdpa_iotlb_batch_is_started(const struct vhost_vdpa *v)
> > +{
> > +return v->n_mr_updated != 0;
> > +}
> > +
> > +static void vhost_vdpa_iotlb_batch_begin_once(struct vhost_vdpa *v)
> > +{
> > +if (!vhost_vdpa_iotlb_batch_is_started(v)) {
> > +vhost_vdpa_listener_begin_batch(v);
> > +}
> > +
> > +v->n_mr_updated++;
> > +}
> > +
> > +static void vhost_vdpa_iotlb_batch_reset(struct vhost_vdpa *v)
> > +{
> > +v->n_mr_updated = 0;
> > +}
> > +
> >  static void vhost_vdpa_listener_commit(MemoryListener *listener)
> >  {
> >  struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, 
> > listener);
> > @@ -120,6 +133,10 @@ static void vhost_vdpa_listener_commit(MemoryListener 
> > *listener)
> >  return;
> >  }
> >
> > +if (vhost_vdpa_iotlb_batch_is_started(v)) {
> > +return;
> > +}
> > +
> >  msg.type = v->msg_type;
> >  msg.iotlb.type = VHOST_IOTLB_BATCH_END;
> >
> > @@ -127,6 +144,8 @@ static void vhost_vdpa_listener_commit(MemoryListener 
> > *listener)
> >  error_report("failed to write, fd=%d, errno=%d (%s)",
> >   fd, errno, strerror(errno));
> >  }
> > +
> > +vhost_vdpa_iotlb_batch_reset(v);
> >  }
> >
> >  static void vhost_vdpa_listener_region_add(MemoryListener *listener,
> > @@ -170,6 +189,10 @@ static void 
> > vhost_vdpa_listener_region_add(MemoryListener *listener,
> >
> >  llsize = int128_sub(llend, int128_make64(iova));
> >
> > +if (v->dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH)) {
>
> Let's move this in to vhost_vdpa_iotlb_batch_begin_once()?
>

Sure

> > +vhost_vdpa_iotlb_batch_begin_once(v);
> > +}
> > +
> >  ret = vhost_vdpa_dma_map(v, iova, int128_get64(llsize),
> >   vaddr, section->readonly);
> >  if (ret) {
> > @@ -221,6 +244,10 @@ static void 
> > vhost_vdpa_listener_region_del(MemoryListener *listener,
> >
> >  llsize = int128_sub(llend, int128_make64(iova));
> >
> > +if (v->dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH)) {
> > +vhost_vdpa_iotlb_batch_begin_once(v);
> > +}
> > +
>
> Do we need to check vhost_vdpa_iotlb_batch_is_started() in the .commit?
>

I don't follow you here. It's that

[PATCH v4 2/3] hw/arm/virt: target-arm: Add A64FX processor support to virt machine

[Shuuichirou Ishii]

Add -cpu a64fx to use A64FX processor when -machine virt option is specified.
In addition, add a64fx to the Supported guest CPU types in the virt.rst 
document.

Signed-off-by: Shuuichirou Ishii 
---
 docs/system/arm/virt.rst | 1 +
 hw/arm/virt.c| 1 +
 2 files changed, 2 insertions(+)

diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
index 59acf0eeaf..850787495b 100644
--- a/docs/system/arm/virt.rst
+++ b/docs/system/arm/virt.rst
@@ -55,6 +55,7 @@ Supported guest CPU types:
 - ``cortex-a53`` (64-bit)
 - ``cortex-a57`` (64-bit)
 - ``cortex-a72`` (64-bit)
+- ``a64fx`` (64-bit)
 - ``host`` (with KVM only)
 - ``max`` (same as ``host`` for KVM; best possible emulation with TCG)
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 81eda46b0b..10286d3fd6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -200,6 +200,7 @@ static const char *valid_cpus[] = {
 ARM_CPU_TYPE_NAME("cortex-a53"),
 ARM_CPU_TYPE_NAME("cortex-a57"),
 ARM_CPU_TYPE_NAME("cortex-a72"),
+ARM_CPU_TYPE_NAME("a64fx"),
 ARM_CPU_TYPE_NAME("host"),
 ARM_CPU_TYPE_NAME("max"),
 };
-- 
2.27.0

[PATCH v4 1/3] target-arm: Add support for Fujitsu A64FX

[Shuuichirou Ishii]

Add a definition for the Fujitsu A64FX processor.

The A64FX processor does not implement the AArch32 Execution state,
so there are no associated AArch32 Identification registers.

For SVE, the A64FX processor supports only 128,256 and 512bit vector lengths.

Signed-off-by: Shuuichirou Ishii 
---
 target/arm/cpu.c   | 27 +++
 target/arm/cpu.h   |  1 +
 target/arm/cpu64.c | 42 ++
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 2866dd7658..162e46afc3 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1345,15 +1345,34 @@ static void arm_cpu_finalizefn(Object *obj)
 #endif
 }
 
+static void a64fx_cpu_set_sve(ARMCPU *cpu)
+{
+/* Suppport of A64FX's vector length are 128,256 and 512bit only */
+bitmap_zero(cpu->sve_vq_map, ARM_MAX_VQ);
+bitmap_zero(cpu->sve_vq_init, ARM_MAX_VQ);
+set_bit(0, cpu->sve_vq_map); /* 128bit */
+set_bit(0, cpu->sve_vq_init);
+set_bit(1, cpu->sve_vq_map); /* 256bit */
+set_bit(1, cpu->sve_vq_init);
+set_bit(3, cpu->sve_vq_map); /* 512bit */
+set_bit(3, cpu->sve_vq_init);
+
+cpu->sve_max_vq = find_last_bit(cpu->sve_vq_map, ARM_MAX_VQ) + 1;
+}
+
 void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
 {
 Error *local_err = NULL;
 
 if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
-arm_cpu_sve_finalize(cpu, &local_err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-return;
+if (arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
+a64fx_cpu_set_sve(cpu);
+} else {
+arm_cpu_sve_finalize(cpu, &local_err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+return;
+}
 }
 
 /*
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9f0a5f84d5..84ebca731a 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2145,6 +2145,7 @@ enum arm_features {
 ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
 ARM_FEATURE_M_MAIN, /* M profile Main Extension */
 ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
+ARM_FEATURE_A64FX, /* Fujitsu A64FX processor */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index c690318a9b..5e7e885f9d 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -847,10 +847,52 @@ static void aarch64_max_initfn(Object *obj)
 cpu_max_set_sve_max_vq, NULL, NULL);
 }
 
+static void aarch64_a64fx_initfn(Object *obj)
+{
+ARMCPU *cpu = ARM_CPU(obj);
+
+cpu->dtb_compatible = "arm,a64fx";
+set_feature(&cpu->env, ARM_FEATURE_A64FX);
+set_feature(&cpu->env, ARM_FEATURE_V8);
+set_feature(&cpu->env, ARM_FEATURE_NEON);
+set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+set_feature(&cpu->env, ARM_FEATURE_AARCH64);
+set_feature(&cpu->env, ARM_FEATURE_EL2);
+set_feature(&cpu->env, ARM_FEATURE_EL3);
+set_feature(&cpu->env, ARM_FEATURE_PMU);
+cpu->midr = 0x461f0010;
+cpu->revidr = 0x;
+cpu->ctr = 0x86668006;
+cpu->reset_sctlr = 0x3180;
+cpu->isar.id_aa64pfr0 =   0x00010111; /* No RAS Extensions */
+cpu->isar.id_aa64pfr1 = 0x;
+cpu->isar.id_aa64dfr0 = 0x10305408;
+cpu->isar.id_aa64dfr1 = 0x;
+cpu->id_aa64afr0 = 0x;
+cpu->id_aa64afr1 = 0x;
+cpu->isar.id_aa64mmfr0 = 0x1122;
+cpu->isar.id_aa64mmfr1 = 0x11212100;
+cpu->isar.id_aa64mmfr2 = 0x1011;
+cpu->isar.id_aa64isar0 = 0x10211120;
+cpu->isar.id_aa64isar1 = 0x00010001;
+cpu->isar.id_aa64zfr0 = 0x;
+cpu->clidr = 0x8023;
+cpu->ccsidr[0] = 0x7007e01c; /* 64KB L1 dcache */
+cpu->ccsidr[1] = 0x2007e01c; /* 64KB L1 icache */
+cpu->ccsidr[2] = 0x70ffe07c; /* 8MB L2 cache */
+cpu->dcz_blocksize = 6; /* 256 bytes */
+cpu->gic_num_lrs = 4;
+cpu->gic_vpribits = 5;
+cpu->gic_vprebits = 5;
+
+/* TODO:  Add A64FX specific HPC extension registers */
+}
+
 static const ARMCPUInfo aarch64_cpus[] = {
 { .name = "cortex-a57", .initfn = aarch64_a57_initfn },
 { .name = "cortex-a53", .initfn = aarch64_a53_initfn },
 { .name = "cortex-a72", .initfn = aarch64_a72_initfn },
+{ .name = "a64fx",  .initfn = aarch64_a64fx_initfn },
 { .name = "max",.initfn = aarch64_max_initfn },
 };
 
-- 
2.27.0

[PATCH v4 0/3] Add support for Fujitsu A64FX processor

[Shuuichirou Ishii]

This is the v4 patch series.

v4:
The following changes have been made to match the SVE specification of
the A64FX processor.
- Implemented internally only the vector lengths of 128, 256, and 512 bit
  supported by the A64FX processor.
- Removed sve and sve-max-vq properties due to the above changes, and
  fixed them so that no explicit options can be specified.

v3:
When we created the v2 patch series, we based it on the v1 patch series
that had not been merged into the upstream, so we created the v3 patch
series as a patch series that can be applied independently.

v2:
No features have been added or removed from the v1 patch series. Removal
of unused definitions that were added in excess, and consolidation of
patches for the purpose of functional consistency.

For patch 1, Implemented Identification registers for A64FX processor.
HPC extension registers will be implemented in the future.
For SVE, the A64FX processor supports only 128,256 and 512bit vector lengths.

For patch 2, A64FX processor can now be used by specifying the -cpu
a64fx option when the -macine virt option is specified.

For patch 3, added A64FX processor related tests.

Shuuichirou Ishii (3):
  target-arm: Add support for Fujitsu A64FX
  hw/arm/virt: target-arm: Add A64FX processor support to virt machine
  tests/arm-cpu-features: Add A64FX processor related tests

 docs/system/arm/virt.rst   |  1 +
 hw/arm/virt.c  |  1 +
 target/arm/cpu.c   | 27 ++
 target/arm/cpu.h   |  1 +
 target/arm/cpu64.c | 42 ++
 tests/qtest/arm-cpu-features.c |  2 ++
 6 files changed, 70 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH v4 3/3] tests/arm-cpu-features: Add A64FX processor related tests

[Shuuichirou Ishii]

Signed-off-by: Shuuichirou Ishii 
---
 tests/qtest/arm-cpu-features.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 8252b85bb8..6d704bc947 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -472,6 +472,8 @@ static void test_query_cpu_model_expansion(const void *data)
 assert_has_feature_enabled(qts, "max", "sve128");
 assert_has_feature_enabled(qts, "cortex-a57", "pmu");
 assert_has_feature_enabled(qts, "cortex-a57", "aarch64");
+assert_has_feature_enabled(qts, "a64fx", "pmu");
+assert_has_feature_enabled(qts, "a64fx", "aarch64");
 
 sve_tests_default(qts, "max");
 pauth_tests_default(qts, "max");
-- 
2.27.0

Re: [PATCH v2] vhost-vdpa: Do not send empty IOTLB update batches

[Jason Wang]

On Thu, Aug 12, 2021 at 12:32 AM Eugenio Pérez  wrote:
>
> With the introduction of the batch hinting, meaningless batches can be
> created with no IOTLB updates if the memory region was skipped by
> vhost_vdpa_listener_skipped_section. This is the case of host notifiers
> memory regions, device un/realize, and others. This causes the vdpa
> device to receive dma mapping settings with no changes, a possibly
> expensive operation for nothing.
>
> To avoid that, VHOST_IOTLB_BATCH_BEGIN hint is delayed until we have a
> meaningful (not skipped section) mapping or unmapping operation, and
> VHOST_IOTLB_BATCH_END is not written unless at least one of _UPDATE /
> _INVALIDATE has been issued.
>
> Signed-off-by: Eugenio Pérez 
> ---
>  include/hw/virtio/vhost-vdpa.h |  1 +
>  hw/virtio/vhost-vdpa.c | 50 ++
>  2 files changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> index e98e327f12..6538572a6f 100644
> --- a/include/hw/virtio/vhost-vdpa.h
> +++ b/include/hw/virtio/vhost-vdpa.h
> @@ -23,6 +23,7 @@ typedef struct vhost_vdpa {
>  int device_fd;
>  int index;
>  uint32_t msg_type;
> +size_t n_mr_updated;
>  MemoryListener listener;
>  struct vhost_dev *dev;
>  VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 6ce94a1f4d..512fa18d68 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -89,19 +89,13 @@ static int vhost_vdpa_dma_unmap(struct vhost_vdpa *v, 
> hwaddr iova,
>  return ret;
>  }
>
> -static void vhost_vdpa_listener_begin(MemoryListener *listener)
> +static void vhost_vdpa_listener_begin_batch(struct vhost_vdpa *v)
>  {
> -struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, 
> listener);
> -struct vhost_dev *dev = v->dev;
> -struct vhost_msg_v2 msg = {};
>  int fd = v->device_fd;
> -
> -if (!(dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH))) {
> -return;
> -}
> -
> -msg.type = v->msg_type;
> -msg.iotlb.type = VHOST_IOTLB_BATCH_BEGIN;
> +struct vhost_msg_v2 msg = {
> +.type = v->msg_type,
> +.iotlb.type = VHOST_IOTLB_BATCH_BEGIN,
> +};
>
>  if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) {
>  error_report("failed to write, fd=%d, errno=%d (%s)",
> @@ -109,6 +103,25 @@ static void vhost_vdpa_listener_begin(MemoryListener 
> *listener)
>  }
>  }
>
> +static bool vhost_vdpa_iotlb_batch_is_started(const struct vhost_vdpa *v)
> +{
> +return v->n_mr_updated != 0;
> +}
> +
> +static void vhost_vdpa_iotlb_batch_begin_once(struct vhost_vdpa *v)
> +{
> +if (!vhost_vdpa_iotlb_batch_is_started(v)) {
> +vhost_vdpa_listener_begin_batch(v);
> +}
> +
> +v->n_mr_updated++;
> +}
> +
> +static void vhost_vdpa_iotlb_batch_reset(struct vhost_vdpa *v)
> +{
> +v->n_mr_updated = 0;
> +}
> +
>  static void vhost_vdpa_listener_commit(MemoryListener *listener)
>  {
>  struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, 
> listener);
> @@ -120,6 +133,10 @@ static void vhost_vdpa_listener_commit(MemoryListener 
> *listener)
>  return;
>  }
>
> +if (vhost_vdpa_iotlb_batch_is_started(v)) {
> +return;
> +}
> +
>  msg.type = v->msg_type;
>  msg.iotlb.type = VHOST_IOTLB_BATCH_END;
>
> @@ -127,6 +144,8 @@ static void vhost_vdpa_listener_commit(MemoryListener 
> *listener)
>  error_report("failed to write, fd=%d, errno=%d (%s)",
>   fd, errno, strerror(errno));
>  }
> +
> +vhost_vdpa_iotlb_batch_reset(v);
>  }
>
>  static void vhost_vdpa_listener_region_add(MemoryListener *listener,
> @@ -170,6 +189,10 @@ static void 
> vhost_vdpa_listener_region_add(MemoryListener *listener,
>
>  llsize = int128_sub(llend, int128_make64(iova));
>
> +if (v->dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH)) {

Let's move this in to vhost_vdpa_iotlb_batch_begin_once()?

> +vhost_vdpa_iotlb_batch_begin_once(v);
> +}
> +
>  ret = vhost_vdpa_dma_map(v, iova, int128_get64(llsize),
>   vaddr, section->readonly);
>  if (ret) {
> @@ -221,6 +244,10 @@ static void 
> vhost_vdpa_listener_region_del(MemoryListener *listener,
>
>  llsize = int128_sub(llend, int128_make64(iova));
>
> +if (v->dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH)) {
> +vhost_vdpa_iotlb_batch_begin_once(v);
> +}
> +

Do we need to check vhost_vdpa_iotlb_batch_is_started() in the .commit?

Others look good.

Thanks

>  ret = vhost_vdpa_dma_unmap(v, iova, int128_get64(llsize));
>  if (ret) {
>  error_report("vhost_vdpa dma unmap error!");
> @@ -234,7 +261,6 @@ static void vhost_vdpa_listener_region_del(MemoryListener 
> *listener,
>   * depends on the addnop().
>   */
>  static const MemoryListener vhost_vdpa_memory_listener = {
> -.be

Re: [RFC PATCH 02/13] target/riscv: Support UXL32 for branch instructions

[Richard Henderson]

On 8/11/21 7:03 PM, LIU Zhiwei wrote:


On 2021/8/12 下午12:42, Richard Henderson wrote:

On 8/11/21 12:40 PM, LIU Zhiwei wrote:
If the software doesn't use the high part, who cares the really value in high part? Do 
you know the benefit?  Thanks again.


I do not.

I simply presume that they already have the hardware, in the form of the addw 
instruction, etc.


The mistake, I think, was changing the definition of "add" in the first place, which 
required the addition of a different opcode "addw", which is then undefined for RV32. 


Sorry, I don't get "the mistake" here. Do you think the specification is not 
right.


I was critiquing the development of the risc-v specification, in that there are 
complications in the current specification that could have been foreseen and avoided with 
different choices years ago.



They should simply have had "addw" and "addq" as different opcodes that didn't change 
behaviour.  Etc.


I don't get  this statement. Is it related to UXL32?


No.  I was just musing.  It's not important.


r~

Re: [PATCH 0/7] Add vmnet.framework based network backend

[Roman Bolshakov]

On Thu, Jun 17, 2021 at 05:32:39PM +0300, Vladislav Yaroshchuk wrote:
> macOS provides networking API for VMs called vmnet.framework.
> I tried to add it as a network backend. All three modes are supported:
> 
> -shared:
>   allows the guest to comminicate with other guests in shared mode and
>   also with external network (Internet) via NAT
> 
> -host:
>   allows the guest to communicate with other guests in host mode
> 
> -bridged:
>   bridges the guest with a physical network interface
> 
> Separate netdev for each vmnet mode was created because they use quite
> different settings, especially since macOS 11.0 when vmnet.framework
> gets a lot of updates.
> 
> Not sure that I use qemu_mutex_lock_iothread() and
> qemu_mutex_unlock_iothread() in correct way while sending packet
> from vmnet interface to QEMU. I'll be happy to receive
> recomendations how to make this thing better if I done sth wrong.
> 
> Also vmnet.framework requires com.apple.vm.networking entitlement to
> run without root priveledges. Ad-hoc signing does not fit there,
> so I didn't touch anything related to signing. As a result we should
> run qemu-system by a priviledged user:
> `$ sudo qemu-system-x86_64 -nic vmnet-shared`
> otherwise vmnet fails with 'general failure'.
> 
> But in any way it seems working now,
> I tested it within qemu-system-x86-64 on macOS 10.15.7 host, with nic
> models:
> - e1000-82545em
> - virtio-net-pci
> 
> and having such guests:
> - macOS 10.15.7
> - Ubuntu Bionic (server cloudimg) 
> 

Hi Vladislav,

I appreciate the efforts and I'm sorry I didn't look into it yet, lack
of time :(

To all: earlier this year another series was sent by Phillip Tennen to
add vmnet.framework and some comments were provided:
https://mail.gnu.org/archive/html/qemu-devel/2021-02/msg05874.html

I'm not sure how to proceed with arbitration which of the series is
preferred. FIFO or LIFO?

Regards,
Roman

> Vladislav Yaroshchuk (7):
>   net/vmnet: dependencies setup, initial preparations
>   net/vmnet: add new netdevs to qapi/net
>   net/vmnet: create common netdev state structure
>   net/vmnet: implement shared mode (vmnet-shared)
>   net/vmnet: implement host mode (vmnet-host)
>   net/vmnet: implement bridged mode (vmnet-bridged)
>   net/vmnet: update qemu-options.hx
> 
>  configure   |  31 +
>  meson.build |   5 +
>  net/clients.h   |  11 ++
>  net/meson.build |   7 ++
>  net/net.c   |  10 ++
>  net/vmnet-bridged.m | 123 ++
>  net/vmnet-common.m  | 294 
>  net/vmnet-host.c|  93 ++
>  net/vmnet-shared.c  |  94 ++
>  net/vmnet_int.h |  48 
>  qapi/net.json   |  99 ++-
>  qemu-options.hx |  17 +++
>  12 files changed, 830 insertions(+), 2 deletions(-)
>  create mode 100644 net/vmnet-bridged.m
>  create mode 100644 net/vmnet-common.m
>  create mode 100644 net/vmnet-host.c
>  create mode 100644 net/vmnet-shared.c
>  create mode 100644 net/vmnet_int.h
> 
> -- 
> 2.23.0
> 

Re: [RFC PATCH 02/13] target/riscv: Support UXL32 for branch instructions

[LIU Zhiwei]

On 2021/8/12 下午12:42, Richard Henderson wrote:

On 8/11/21 12:40 PM, LIU Zhiwei wrote:
If the software doesn't use the high part, who cares the really value 
in high part? Do you know the benefit?  Thanks again.


I do not.

I simply presume that they already have the hardware, in the form of 
the addw instruction, etc.


The mistake, I think, was changing the definition of "add" in the 
first place, which required the addition of a different opcode "addw", 
which is then undefined for RV32. 


Sorry, I don't get "the mistake" here. Do you think the specification is 
not right.

Or the QEMU implementation of this patch set is not right?
Currently I don't know there is  a 64-bit hardware which has done with 
UXL32.


They should simply have had "addw" and "addq" as different opcodes 
that didn't change behaviour.  Etc.


I don't get  this statement. Is it related to UXL32?

Best Regards,
Zhiwei



But what's done is done.


r~

Re: [PATCH 12/19] target/ppc/pmu_book3s_helper.c: enable PMC1 counter negative EBB

[Richard Henderson]

On 8/11/21 6:45 PM, Richard Henderson wrote:

On 8/11/21 5:39 PM, David Gibson wrote:

I mean, nothing is stopping us from calculating cycles using time, but in the
end we would do the same thing we're already doing today.


Oh.. ok.  I had assumed that icount worked by instrumenting the
generate TCG code to actually count instructions, rather than working
off the time.


David, you're right, icount instruments the generated tcg code.
You also have to add -icount to the command-line.


Oh, and btw, icount disables multi-threaded tcg, so you're going to be running that guest 
in round-robin mode.


Icount affects so many aspects of qemu that I really do not think it is the best option 
for a PMU.


If you want to count instructions retired, then just do that.  Stuff values into 
tcg_gen_insn_start so they're available for exception unwind, and otherwise decrement the 
counter at the end of a TB.


If you really must interrupt exactly at 0 (and not simply at some point past underflow), 
then we can adjust the tb lookup logic to let you reduce tb->cflags.CF_COUNT_MASK in 
cpu_get_tb_cpu_state.



r~

Re: [PATCH 12/19] target/ppc/pmu_book3s_helper.c: enable PMC1 counter negative EBB

[Richard Henderson]

On 8/11/21 5:39 PM, David Gibson wrote:

I mean, nothing is stopping us from calculating cycles using time, but in the
end we would do the same thing we're already doing today.


Oh.. ok.  I had assumed that icount worked by instrumenting the
generate TCG code to actually count instructions, rather than working
off the time.


David, you're right, icount instruments the generated tcg code.
You also have to add -icount to the command-line.


r~

Re: [RFC PATCH 02/13] target/riscv: Support UXL32 for branch instructions

[Richard Henderson]

On 8/11/21 12:40 PM, LIU Zhiwei wrote:
If the software doesn't use the high part, who cares the really value in high part? Do you 
know the benefit?  Thanks again.


I do not.

I simply presume that they already have the hardware, in the form of the addw instruction, 
etc.


The mistake, I think, was changing the definition of "add" in the first place, which 
required the addition of a different opcode "addw", which is then undefined for RV32. 
They should simply have had "addw" and "addq" as different opcodes that didn't change 
behaviour.  Etc.


But what's done is done.


r~

Re: [PATCH 12/19] target/ppc/pmu_book3s_helper.c: enable PMC1 counter negative EBB

[David Gibson]

On Wed, Aug 11, 2021 at 08:18:29AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/11/21 12:40 AM, David Gibson wrote:
> > On Tue, Aug 10, 2021 at 05:26:09PM -0300, Daniel Henrique Barboza wrote:
> > > 
> > > 
> > > On 8/10/21 1:01 AM, David Gibson wrote:
> > > > On Mon, Aug 09, 2021 at 10:10:50AM -0300, Daniel Henrique Barboza wrote:
> > > > > This patch starts the counter negative EBB support by enabling PMC1
> > > > > counter negative condition.
> > > > > 
> > > > > A counter negative condition happens when a performance monitor 
> > > > > counter
> > > > > reaches the value 0x8000. When that happens, if a counter negative
> > > > > condition is enabled in that counter, a performance monitor alert is
> > > > > triggered. For PMC1, this condition is enabled by MMCR0_PMC1CE.
> > > > > 
> > > > > An icount-based logic is used to predict when we need to wake up the 
> > > > > timer
> > > > > to trigger the alert in both PM_INST_CMPL (0x2) and PM_CYC (0x1E) 
> > > > > events.
> > > > > The timer callback will then trigger a PPC_INTERRUPT_PMC which will 
> > > > > become a
> > > > > event-based exception later.
> > > > > 
> > > > > Some EBB powerpc kernel selftests are passing after this patch, but a
> > > > > substancial amount of them relies on other PMCs to be enabled and 
> > > > > events
> > > > > that we don't support at this moment. We'll address that in the next
> > > > > patches.
> > > > > 
> > > > > Signed-off-by: Daniel Henrique Barboza 
> > > > > ---
> > > > >target/ppc/cpu.h   |   1 +
> > > > >target/ppc/pmu_book3s_helper.c | 127 
> > > > > +++--
> > > > >2 files changed, 92 insertions(+), 36 deletions(-)
> > > > > 
> > > > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > > > index 1d38b8cf7a..5c81d459f4 100644
> > > > > --- a/target/ppc/cpu.h
> > > > > +++ b/target/ppc/cpu.h
> > > > > @@ -350,6 +350,7 @@ typedef struct ppc_v3_pate_t {
> > > > >#define MMCR0_EBE   PPC_BIT(43) /* Perf Monitor EBB Enable 
> > > > > */
> > > > >#define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or 
> > > > > Event */
> > > > >#define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
> > > > > +#define MMCR0_PMC1CE PPC_BIT(48)
> > > > >#define MMCR1_PMC1SEL_SHIFT (63 - 39)
> > > > >#define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
> > > > > diff --git a/target/ppc/pmu_book3s_helper.c 
> > > > > b/target/ppc/pmu_book3s_helper.c
> > > > > index 43cc0eb722..58ae65e22b 100644
> > > > > --- a/target/ppc/pmu_book3s_helper.c
> > > > > +++ b/target/ppc/pmu_book3s_helper.c
> > > > > @@ -25,6 +25,7 @@
> > > > > * and SPAPR code.
> > > > > */
> > > > >#define PPC_CPU_FREQ 10
> > > > > +#define COUNTER_NEGATIVE_VAL 0x8000
> > > > >static uint64_t get_cycles(uint64_t icount_delta)
> > > > >{
> > > > > @@ -32,22 +33,9 @@ static uint64_t get_cycles(uint64_t icount_delta)
> > > > >NANOSECONDS_PER_SECOND);
> > > > >}
> > > > > -static void update_PMC_PM_INST_CMPL(CPUPPCState *env, int sprn,
> > > > > -uint64_t icount_delta)
> > > > > -{
> > > > > -env->spr[sprn] += icount_delta;
> > > > > -}
> > > > > -
> > > > > -static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> > > > > -  uint64_t icount_delta)
> > > > > -{
> > > > > -env->spr[sprn] += get_cycles(icount_delta);
> > > > > -}
> > > > > -
> > > > > -static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> > > > > -uint64_t icount_delta)
> > > > > +static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
> > > > >{
> > > > > -int event;
> > > > > +int event = 0x0;
> > > > >switch (sprn) {
> > > > >case SPR_POWER_PMC1:
> > > > > @@ -65,11 +53,35 @@ static void 
> > > > > update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> > > > >case SPR_POWER_PMC4:
> > > > >event = MMCR1_PMC4SEL & env->spr[SPR_POWER_MMCR1];
> > > > >break;
> > > > > +case SPR_POWER_PMC5:
> > > > > +event = 0x2;
> > > > > +break;
> > > > > +case SPR_POWER_PMC6:
> > > > > +event = 0x1E;
> > > > > +break;
> > > > 
> > > > This looks like a nice cleanup that would be better folded into an
> > > > earlier patch.
> > > > 
> > > > >default:
> > > > > -return;
> > > > > +break;
> > > > >}
> > > > > -switch (event) {
> > > > > +return event;
> > > > > +}
> > > > > +
> > > > > +static void update_PMC_PM_INST_CMPL(CPUPPCState *env, int sprn,
> > > > > +uint64_t icount_delta)
> > > > > +{
> > > > > +env->spr[sprn] += icount_delta;
> > > > > +}
> > > > > +
> > > > > +static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> > > > > +  uint64_t icount_delta)
> > > > > +{
> > > > > +env->spr[sprn] += get_cycles(icount_delta);
> > 

[PATCH] hw/arm/Kconfig: no need to enable ACPI_MEMORY_HOTPLUG explicitly

[Ani Sinha]

ACPI_MEMORY_HOTPLUG is implicitly turned on when ACPI_HW_REDUCED is selected.
ACPI_HW_REDUCED is already enabled. No need to turn on ACPI_MEMORY_HOTPLUG
explicitly. This is a minor cleanup.

Signed-off-by: Ani Sinha 
---
 hw/arm/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 4ba0aca067..38cf9f44e2 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -25,7 +25,6 @@ config ARM_VIRT
 select ACPI_PCI
 select MEM_DEVICE
 select DIMM
-select ACPI_MEMORY_HOTPLUG
 select ACPI_HW_REDUCED
 select ACPI_NVDIMM
 select ACPI_APEI
-- 
2.25.1

[PATCH] hw/virtio: Fix leak of host-notifier memory-region

[Yajun Wu via]

If call virtio_queue_set_host_notifier_mr fails, should free
host-notifier memory-region.

Signed-off-by: Yajun Wu 
---
 hw/virtio/vhost-user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index aec6cc1..3ae5297 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1474,6 +1474,7 @@ static int 
vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
 g_free(name);
 
 if (virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true)) {
+object_unparent(OBJECT(&n->mr));
 munmap(addr, page_size);
 return -1;
 }
-- 
1.8.3.1

Re: [PATCH 06/19] target/ppc/pmu_book3s_helper: enable PMC1-PMC4 events

[David Gibson]

On Tue, Aug 10, 2021 at 08:08:04PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/10/21 12:03 PM, Daniel Henrique Barboza wrote:
> > 
> > 
> > On 8/10/21 12:42 AM, David Gibson wrote:
> > > On Mon, Aug 09, 2021 at 10:10:44AM -0300, Daniel Henrique Barboza wrote:
> > > > So far the PMU logic was using PMC5 for instruction counting (linux
> > > > kernel PM_INST_CMPL) and PMC6 to count cycles (PM_CYC). We aren't using
> > > > PMCs 1-4.
> > > > 
> > > > Let's enable all PMCs to count these 2 events we already provide. The
> > > > logic used to calculate PMC5 is now being provided by
> > > > update_PMC_PM_INST_CMPL() and PMC6 logic is now implemented in
> > > > update_PMC_PM_CYC().
> > > > 
> > > > The enablement of these 2 events for all PMUs are done by using the
> > > > Linux kernel definition of those events: 0x02 for PM_INST_CMPL and 0x1e
> > > > for PM_CYC,
> > > 
> > > I'm confused by this.  Surely the specific values here should be
> > > defined by the hardware, not by Linux.
> > 
> > The hardware/PowerISA defines these events as follows for all counters:
> > 
> > 00 Disable events. (No events occur.)
> > 01-BF Implementation-dependent
> > C0-DF Reserved
> > 
> > And then hardware events defined by the ISA goes from E0 to FF. Each counter
> > has a different set of hardware defined events in this range.
> > 
> > The Linux perf driver defines some events in the 'Implementation-dependent'
> > area, allowing for events codes such as '0x02' to count instructions
> > completed in PMC1 even though this event is not defined in the ISA for this
> > PMC. I am assuming that the real hardware - at least the ones that IBM
> > produces - does this mapping internally. I'll ask around to see if I find
> > out whether it's the HW or some part of the Perf subsystem that I don't
> > comprehend that are doing it.
> 
> The kernel guys confirmed that the HW is aware of the 
> implementantion-dependent
> Perf event codes that the Linux kernel uses. Also, by reading the kernel code 
> it
> is safe to say that this is true since Power7 at least.

Ok.  I'm pretty sure POWER6 and POWER5 have totally different PMUs
though.  So best to be explicit that this is the POWER7 (and later
compatible chips) PMU model you're building here.

> What we can do here to play nicer with other non-IBM PowerPC chips, that might
> not have the same implementation-dependent Perf events in the hardware, is to 
> make
> this mapping only for emulation of IBM processors.

Well.. I'm not sure there are any other chips claiming to implement
the architecture (at least recent versions).  But for the broad swath
of things that might be considered book3s it's much worse than that:
some of the POWER generations have completely different PMU designs.
It's not just different event values, but different numbers of PMCs,
different controls in the MMCRs different all sorts of things.

> That way we can emulate these
> events that IBM PMU implements while not making any assumptions for every 
> other
> PowerPC chip that implements Book3s.
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> > 
> > 
> > I am not particularly happy about having to rely on 
> > 'implementation-dependent'
> > fields that are defined by the Perf subsystem of Linux in the emulator
> > code that should be OS-agnostic. Unfortunately, I didn't find any 
> > alternative
> > to make the kernel operate this PMU implementation other than baking these
> > event codes into the PMU logic.
> > 
> > 
> > Thanks,
> > 
> > 
> > Daniel
> > 
> > 
> > > 
> > > > all of those defined by specific bits in MMCR1 for each PMC.
> > > > PMCs 1-4 relies on the correct event to be defined in MMCR1. PMC5 and
> > > > PMC6 will count PM_INST_CMPL and PMC_CYC, respectively, regardless of
> > > > MMCR1 setup.
> > > > 
> > > > Signed-off-by: Daniel Henrique Barboza 
> > > > ---
> > > >   target/ppc/cpu.h   |  8 +
> > > >   target/ppc/pmu_book3s_helper.c | 60 --
> > > >   2 files changed, 65 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > > index 8cea8f2aca..afd9cd402b 100644
> > > > --- a/target/ppc/cpu.h
> > > > +++ b/target/ppc/cpu.h
> > > > @@ -350,6 +350,14 @@ typedef struct ppc_v3_pate_t {
> > > >   #define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or 
> > > > Event */
> > > >   #define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
> > > > +#define MMCR1_PMC1SEL_SHIFT (63 - 39)
> > > > +#define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
> > > > +#define MMCR1_PMC2SEL_SHIFT (63 - 47)
> > > > +#define MMCR1_PMC2SEL PPC_BITMASK(40, 47)
> > > > +#define MMCR1_PMC3SEL_SHIFT (63 - 55)
> > > > +#define MMCR1_PMC3SEL PPC_BITMASK(48, 55)
> > > > +#define MMCR1_PMC4SEL PPC_BITMASK(56, 63)
> > > > +
> > > >   /* LPCR bits */
> > > >   #define LPCR_VPM0 PPC_BIT(0)
> > > >   #define LPCR_VPM1 PPC_BIT(1)
> > > > diff --git a/target/ppc/pmu_book3s_helper.c 
> > > > b/target/ppc/pmu_book3s_helper.c
> > > > index 0994

Re: [PATCH 15/19] target/ppc/pmu_book3s_helper: enable counter negative for all PMCs

[David Gibson]

On Tue, Aug 10, 2021 at 06:02:41PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/10/21 1:11 AM, David Gibson wrote:
> > On Mon, Aug 09, 2021 at 10:10:53AM -0300, Daniel Henrique Barboza wrote:
> > > All performance monitor counters can trigger a counter negative
> > > condition if the proper MMCR0 bits are set. This patch does that by
> > > doing the following:
> > > 
> > > - pmc_counter_negative_enabled() will check whether a given PMC is
> > > eligible to trigger the counter negative alert;
> > > 
> > > - get_counter_neg_timeout() will return the timeout for the counter
> > > negative condition for a given PMC, or -1 if the PMC is not able to
> > > trigger this alert;
> > > 
> > > - the existing counter_negative_cond_enabled() now must consider the
> > > counter negative bit for PMCs 2-6, MMCR0_PMCjCE;
> > > 
> > > - set_PMU_excp_timer() will now search all existing PMCs for the
> > > shortest counter negative timeout. The shortest timeout will be used to
> > > set the PMC interrupt timer.
> > > 
> > > This change makes most EBB powepc kernel tests pass, validating that the
> > > existing EBB logic is consistent. There are a few tests that aren't 
> > > passing
> > > due to additional PMU bits and perf events that aren't covered yet.
> > > We'll attempt to cover some of those in the next patches.
> > > 
> > > Signed-off-by: Daniel Henrique Barboza 
> > > ---
> > >   target/ppc/cpu.h   |  1 +
> > >   target/ppc/pmu_book3s_helper.c | 96 ++
> > >   2 files changed, 87 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > index 5c81d459f4..1aa1fd42af 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -351,6 +351,7 @@ typedef struct ppc_v3_pate_t {
> > >   #define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event 
> > > */
> > >   #define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
> > >   #define MMCR0_PMC1CE PPC_BIT(48)
> > > +#define MMCR0_PMCjCE PPC_BIT(49)
> > >   #define MMCR1_PMC1SEL_SHIFT (63 - 39)
> > >   #define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
> > > diff --git a/target/ppc/pmu_book3s_helper.c 
> > > b/target/ppc/pmu_book3s_helper.c
> > > index 7126e9b3d5..c5c5ab38c9 100644
> > > --- a/target/ppc/pmu_book3s_helper.c
> > > +++ b/target/ppc/pmu_book3s_helper.c
> > > @@ -143,22 +143,98 @@ static int64_t get_CYC_timeout(CPUPPCState *env, 
> > > int sprn)
> > >   return muldiv64(remaining_cyc, NANOSECONDS_PER_SECOND, 
> > > PPC_CPU_FREQ);
> > >   }
> > > -static void set_PMU_excp_timer(CPUPPCState *env)
> > > +static bool pmc_counter_negative_enabled(CPUPPCState *env, int sprn)
> > >   {
> > > -uint64_t timeout, now;
> > > +switch (sprn) {
> > > +case SPR_POWER_PMC1:
> > > +return env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE;
> > > -if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE)) {
> > > -return;
> > > +case SPR_POWER_PMC2:
> > > +case SPR_POWER_PMC3:
> > > +case SPR_POWER_PMC4:
> > > +case SPR_POWER_PMC5:
> > > +case SPR_POWER_PMC6:
> > > +return env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE;
> > > +
> > > +default:
> > > +break;
> > >   }
> > > -switch (get_PMC_event(env, SPR_POWER_PMC1)) {
> > > -case 0x2:
> > > -timeout = get_INST_CMPL_timeout(env, SPR_POWER_PMC1);
> > > +return false;
> > > +}
> > > +
> > > +static int64_t get_counter_neg_timeout(CPUPPCState *env, int sprn)
> > > +{
> > > +int64_t timeout = -1;
> > > +
> > > +if (!pmc_counter_negative_enabled(env, sprn)) {
> > > +return -1;
> > > +}
> > > +
> > > +if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL) {
> > > +return 0;
> > > +}
> > > +
> > > +switch (sprn) {
> > > +case SPR_POWER_PMC1:
> > > +case SPR_POWER_PMC2:
> > > +case SPR_POWER_PMC3:
> > > +case SPR_POWER_PMC4:
> > > +switch (get_PMC_event(env, sprn)) {
> > > +case 0x2:
> > > +timeout = get_INST_CMPL_timeout(env, sprn);
> > > +break;
> > > +case 0x1E:
> > > +timeout = get_CYC_timeout(env, sprn);
> > > +break;
> > > +}
> > > +
> > >   break;
> > > -case 0x1e:
> > > -timeout = get_CYC_timeout(env, SPR_POWER_PMC1);
> > > +case SPR_POWER_PMC5:
> > > +timeout = get_INST_CMPL_timeout(env, sprn);
> > > +break;
> > > +case SPR_POWER_PMC6:
> > > +timeout = get_CYC_timeout(env, sprn);
> > >   break;
> > >   default:
> > > +break;
> > > +}
> > > +
> > > +return timeout;
> > > +}
> > > +
> > > +static void set_PMU_excp_timer(CPUPPCState *env)
> > > +{
> > > +int64_t timeout = -1;
> > > +uint64_t now;
> > > +int i;
> > > +
> > > +/*
> > > + * Scroll through all PMCs and check which one is closer to a
> > > + * counter negative timeout.
> > 
> > I'm wondering if it would be simpler to use a separate timer for each
> > PMC: after all the co

Re: [PATCH v2] target/riscv: Don't wrongly override isa version

[Bin Meng]

On Wed, Aug 11, 2021 at 10:46 PM LIU Zhiwei  wrote:
>
> For some cpu, the isa version has already been set in cpu init function.
> Thus only override the isa version when isa version is not set, or
> users set different isa version explicitly by cpu parameters.
>
> Signed-off-by: LIU Zhiwei 
> ---
>  target/riscv/cpu.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)

Please include a changelog in the newer version in the future.

>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 991a6bb760..1a2b03d579 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -392,9 +392,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  RISCVCPU *cpu = RISCV_CPU(dev);
>  CPURISCVState *env = &cpu->env;
>  RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> -int priv_version = PRIV_VERSION_1_11_0;
> -int bext_version = BEXT_VERSION_0_93_0;
> -int vext_version = VEXT_VERSION_0_07_1;
> +int priv_version = 0;
>  target_ulong target_misa = env->misa;
>  Error *local_err = NULL;
>
> @@ -417,9 +415,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  }
>  }
>
> -set_priv_version(env, priv_version);
> -set_bext_version(env, bext_version);
> -set_vext_version(env, vext_version);
> +if (priv_version) {
> +set_priv_version(env, priv_version);
> +} else if (!env->priv_ver) {
> +set_priv_version(env, PRIV_VERSION_1_11_0);
> +}
>
>  if (cpu->cfg.mmu) {
>  set_feature(env, RISCV_FEATURE_MMU);
> @@ -497,6 +497,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  target_misa |= RVH;
>  }
>  if (cpu->cfg.ext_b) {
> +int bext_version = BEXT_VERSION_0_93_0;
>  target_misa |= RVB;
>
>  if (cpu->cfg.bext_spec) {
> @@ -515,6 +516,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  set_bext_version(env, bext_version);
>  }
>  if (cpu->cfg.ext_v) {
> +int vext_version = VEXT_VERSION_0_07_1;
>  target_misa |= RVV;
>  if (!is_power_of_2(cpu->cfg.vlen)) {
>  error_setg(errp,
> --

Reviewed-by: Bin Meng 

Re: [PATCH] tests/acceptance: Test powernv machines

[David Gibson]

On Wed, Aug 11, 2021 at 11:07:31AM +0200, Thomas Huth wrote:
> On 10/08/2021 11.09, Cédric Le Goater wrote:
> > On 8/10/21 10:36 AM, Joel Stanley wrote:
> > > On Tue, 10 Aug 2021 at 08:34, Cédric Le Goater  wrote:
> > > > 
> > > > Fetch the OpenPOWER images to boot the powernv8 and powernv9 machines
> > > > with a simple PCI layout.
> > > > 
> > > > Cc: Cleber Rosa 
> > > > Cc: Wainer dos Santos Moschetta 
> > > > Signed-off-by: Cédric Le Goater 
> > > > ---
> > > >   tests/acceptance/boot_linux_console.py | 42 ++
> > > >   1 file changed, 42 insertions(+)
> > > > 
> > > > diff --git a/tests/acceptance/boot_linux_console.py 
> > > > b/tests/acceptance/boot_linux_console.py
> > > > index 5248c8097df9..da93a475ca87 100644
> > > > --- a/tests/acceptance/boot_linux_console.py
> > > > +++ b/tests/acceptance/boot_linux_console.py
> > > > @@ -1176,6 +1176,48 @@ def test_ppc64_e500(self):
> > > >   tar_hash = '6951d86d644b302898da2fd701739c9406527fe1'
> > > >   self.do_test_advcal_2018('19', tar_hash, 'uImage')
> > > > 
> > > > +def do_test_ppc64_powernv(self, proc):
> > > > +
> > > > +images_url = 
> > > > ('https://github.com/open-power/op-build/releases/download/v2.7/')
> > > > +
> > > > +skiboot_url = images_url + 'skiboot.lid'
> > > > +skiboot_hash = 
> > > > 'a9ffcddbf238f86cda4b2cae2882d6bd13cff8489109758a4980efaf154f4a29'
> > > > +skiboot_path = self.fetch_asset(skiboot_url, 
> > > > asset_hash=skiboot_hash,
> > > > +   algorithm='sha256')
> > > 
> > > What's the thought that led you to using this instead of the one that
> > > gets packaged with qemu?
> > 
> > Good question.
> > 
> > I considered that the skiboot.lid shipped with QEMU was somewhat a default
> > to make things work. The official released versions are the ones used by
> > the outside world on real systems and were a better target for tests.
> > 
> > That said, using the default version might be enough. Maintainers, please
> > advise !
> 
> IMHO:
> 
> - We want to test the things that *we* ship
> 
> - We want to download as few things as possible, since downloads
>   often slow down the tests and break CI runs if the network to
>   the download site is not available
> 
>  ==> I'd prefer to use the internal skiboot.lid unless there is
>  really a compelling reason to use the external one.
> 
> Just my 0.02 € though.

I agree 100%.  If the internal skiboot isn't the "real" one, then we
should update it to something that is.

-- 
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: [RFC PATCH v0 1/2] spapr: Add H_REG_SNS hcall

[David Gibson]

On Wed, Aug 11, 2021 at 02:56:13PM +0530, Bharata B Rao wrote:
> On Mon, Aug 09, 2021 at 01:49:54PM +1000, David Gibson wrote:
> > On Thu, Aug 05, 2021 at 01:02:27PM +0530, Bharata B Rao wrote:
[snip]
> > > diff --git a/linux-headers/asm-powerpc/kvm.h 
> > > b/linux-headers/asm-powerpc/kvm.h
> > > index 9f18fa090f..d72739126a 100644
> > > --- a/linux-headers/asm-powerpc/kvm.h
> > > +++ b/linux-headers/asm-powerpc/kvm.h
> > > @@ -470,6 +470,12 @@ struct kvm_ppc_cpu_char {
> > >  #define KVM_PPC_CPU_BEHAV_BNDS_CHK_SPEC_BAR  (1ULL << 61)
> > >  #define KVM_PPC_CPU_BEHAV_FLUSH_COUNT_CACHE  (1ull << 58)
> > >  
> > > +/* For KVM_PPC_SET_SNS */
> > > +struct kvm_ppc_sns_reg {
> > > + __u64 addr;
> > > + __u64 len;
> > > +};
> > > +
> > 
> > Updates to linux-headers/ should be done as a separate preliminary
> > patch, listing the specific kernel commit that you're updating too.
> 
> Yes, I am aware of it. Since the kernel patches are still in RFC
> state, I noted this as a TODO in patch description :-)

Sorry, I missed that.  In general, even for draft posts, I'd suggest
doing the linux-headers/ updates as a separate patch (but you can
construct that ad-hoc).

-- 
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

Using loadvm with snapshot

[Gabriel Southern]

Hi,

Are there plans to support using -loadvm with -snapshot?

I saw some past discussion on mailing list including bug that was closed 
earlier this year but no recent activity:

https://lore.kernel.org/qemu-devel/162424905685.11837.7303570061857383641.mal...@loganberry.canonical.com/

Testing with latest qemu it looks like -loadvm still does not work when 
combined with -snapshot.

I'm curious how complex it would be to implement this feature and if it may 
show up on QEMU roadmap in the future. Or if there is alterative command that 
can be used to save the state of a running VM and then use the same qcow to run 
multiple QEMU instances loading this VM state running in snapshot mode.

Thanks,

-Gabriel

Re: [PATCH 06/19] target/ppc/pmu_book3s_helper: enable PMC1-PMC4 events

[Daniel Henrique Barboza]

On 8/10/21 8:08 PM, Daniel Henrique Barboza wrote:



On 8/10/21 12:03 PM, Daniel Henrique Barboza wrote:



On 8/10/21 12:42 AM, David Gibson wrote:

On Mon, Aug 09, 2021 at 10:10:44AM -0300, Daniel Henrique Barboza wrote:

So far the PMU logic was using PMC5 for instruction counting (linux
kernel PM_INST_CMPL) and PMC6 to count cycles (PM_CYC). We aren't using
PMCs 1-4.

Let's enable all PMCs to count these 2 events we already provide. The
logic used to calculate PMC5 is now being provided by
update_PMC_PM_INST_CMPL() and PMC6 logic is now implemented in
update_PMC_PM_CYC().

The enablement of these 2 events for all PMUs are done by using the
Linux kernel definition of those events: 0x02 for PM_INST_CMPL and 0x1e
for PM_CYC,


I'm confused by this.  Surely the specific values here should be
defined by the hardware, not by Linux.


The hardware/PowerISA defines these events as follows for all counters:

00 Disable events. (No events occur.)
01-BF Implementation-dependent
C0-DF Reserved

And then hardware events defined by the ISA goes from E0 to FF. Each counter
has a different set of hardware defined events in this range.

The Linux perf driver defines some events in the 'Implementation-dependent'
area, allowing for events codes such as '0x02' to count instructions
completed in PMC1 even though this event is not defined in the ISA for this
PMC. I am assuming that the real hardware - at least the ones that IBM
produces - does this mapping internally. I'll ask around to see if I find
out whether it's the HW or some part of the Perf subsystem that I don't
comprehend that are doing it.


The kernel guys confirmed that the HW is aware of the implementantion-dependent
Perf event codes that the Linux kernel uses. Also, by reading the kernel code it
is safe to say that this is true since Power7 at least.

What we can do here to play nicer with other non-IBM PowerPC chips, that might
not have the same implementation-dependent Perf events in the hardware, is to 
make
this mapping only for emulation of IBM processors. That way we can emulate these
events that IBM PMU implements while not making any assumptions for every other
PowerPC chip that implements Book3s.



Scratch that. I got told by the kernel folks that, starting in v5.14, the
generic-compat-pmu events are being calculated by using the architected ISA 
events
only. They did that to not rely on implementation-dependent events in the Perf
subsystem.

What I'll attempt here is implement some architected events (cycles, 
instructions
and a small variant of those 2 that uses the run latch) and see how the PMU
behaves in the selftests.



Daniel






Thanks,


Daniel





I am not particularly happy about having to rely on 'implementation-dependent'
fields that are defined by the Perf subsystem of Linux in the emulator
code that should be OS-agnostic. Unfortunately, I didn't find any alternative
to make the kernel operate this PMU implementation other than baking these
event codes into the PMU logic.


Thanks,


Daniel





all of those defined by specific bits in MMCR1 for each PMC.
PMCs 1-4 relies on the correct event to be defined in MMCR1. PMC5 and
PMC6 will count PM_INST_CMPL and PMC_CYC, respectively, regardless of
MMCR1 setup.

Signed-off-by: Daniel Henrique Barboza 
---
  target/ppc/cpu.h   |  8 +
  target/ppc/pmu_book3s_helper.c | 60 --
  2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 8cea8f2aca..afd9cd402b 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -350,6 +350,14 @@ typedef struct ppc_v3_pate_t {
  #define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
  #define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
+#define MMCR1_PMC1SEL_SHIFT (63 - 39)
+#define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
+#define MMCR1_PMC2SEL_SHIFT (63 - 47)
+#define MMCR1_PMC2SEL PPC_BITMASK(40, 47)
+#define MMCR1_PMC3SEL_SHIFT (63 - 55)
+#define MMCR1_PMC3SEL PPC_BITMASK(48, 55)
+#define MMCR1_PMC4SEL PPC_BITMASK(56, 63)
+
  /* LPCR bits */
  #define LPCR_VPM0 PPC_BIT(0)
  #define LPCR_VPM1 PPC_BIT(1)
diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
index 0994531f65..99e62bd37b 100644
--- a/target/ppc/pmu_book3s_helper.c
+++ b/target/ppc/pmu_book3s_helper.c
@@ -28,6 +28,56 @@ static uint64_t get_cycles(uint64_t insns)
  return insns * 4;
  }
+static void update_PMC_PM_INST_CMPL(CPUPPCState *env, int sprn,
+    uint64_t curr_icount)
+{
+    env->spr[sprn] += curr_icount - env->pmu_base_icount;
+}
+
+static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
+  uint64_t curr_icount)
+{
+    uint64_t insns = curr_icount - env->pmu_base_icount;
+    env->spr[sprn] += get_cycles(insns);
+}
+
+static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
+    uint64_t curr_icount)
+{
+    int ev

Re: [RFC PATCH 02/13] target/riscv: Support UXL32 for branch instructions

[LIU Zhiwei]

On 2021/8/12 上午1:56, Richard Henderson wrote:

On 8/11/21 4:57 AM, LIU Zhiwei wrote:
I  still don't know why the value written sign-extended.  If that's 
the the rule of final specification, I will try to obey it although 
our Linux will not depend on the high part.


The text that I'm looking at is

https://github.com/riscv/riscv-isa-manual/releases/download/Ratified-IMFDQC-and-Priv-v1.11/riscv-privileged-20190608.pdf 



3.1.6.2 Base ISA Control in mstatus Register

In the fifth paragraph, the requirement for sign-extension is detailed.


Thanks. I have already seen this rule.

/"Whenever XLEN in any mode is set to a value less than the widest 
supported XLEN, all operationsmust ignore source operand register bits above the configured XLEN, and 
must sign-extend resultsto fill the entire widest supported XLEN in the destination register.//" /


I still don't know why the specification has this constraint. It just 
requires that fill hardware registers with defined sign-extension value.

But it doesn't give the real benefit of this constraint.

If the software doesn't use the high part, who cares the really value in 
high part? Do you know the benefit?  Thanks again.


Best Regards,
Zhiwei//



r~

[PATCH] accel/tcg: remove redundant TCG_KICK_PERIOD define

[Luc Michel]

The TCG_KICK_PERIOD macro is already defined in tcg-accel-ops-rr.h.
Remove it from tcg-accel-ops-rr.c.

Signed-off-by: Luc Michel 
---
 accel/tcg/tcg-accel-ops-rr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
index c02c061ecb..a5fd26190e 100644
--- a/accel/tcg/tcg-accel-ops-rr.c
+++ b/accel/tcg/tcg-accel-ops-rr.c
@@ -58,12 +58,10 @@ void rr_kick_vcpu_thread(CPUState *unused)
  */
 
 static QEMUTimer *rr_kick_vcpu_timer;
 static CPUState *rr_current_cpu;
 
-#define TCG_KICK_PERIOD (NANOSECONDS_PER_SECOND / 10)
-
 static inline int64_t rr_next_kick_time(void)
 {
 return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + TCG_KICK_PERIOD;
 }
 
-- 
2.17.1

Re: [PATCH] target/i386: Remove split lock detect in Snowridge CPU model

[Eduardo Habkost]

On Wed, Jun 30, 2021 at 09:20:53AM +0800, Chenyi Qiang wrote:
> At present, there's no mechanism intelligent enough to virtualize split
> lock detection correctly. Remove it in Snowridge CPU model to avoid the
> feature exposure.
> 
> Signed-off-by: Chenyi Qiang 

Sorry I have missed this before 6.1 soft freeze.  I'm queueing
this for 6.2.  Thanks!

-- 
Eduardo

Re: [PATCH] accel/tcg: remove redundant TCG_KICK_PERIOD define

[Richard Henderson]

On 8/11/21 4:12 AM, Luc Michel wrote:

The TCG_KICK_PERIOD macro is already defined in tcg-accel-ops-rr.h.
Remove it from tcg-accel-ops-rr.c.

Signed-off-by: Luc Michel
---
  accel/tcg/tcg-accel-ops-rr.c | 2 --
  1 file changed, 2 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [RFC PATCH 02/13] target/riscv: Support UXL32 for branch instructions

[Richard Henderson]

On 8/11/21 4:57 AM, LIU Zhiwei wrote:
I  still don't know why the value written sign-extended.  If that's the the rule of final 
specification, I will try to obey it although our Linux will not depend on the high part.


The text that I'm looking at is

https://github.com/riscv/riscv-isa-manual/releases/download/Ratified-IMFDQC-and-Priv-v1.11/riscv-privileged-20190608.pdf

3.1.6.2 Base ISA Control in mstatus Register

In the fifth paragraph, the requirement for sign-extension is detailed.


r~

Re: [RFC PATCH] vhost-vdpa: Do not send empty IOTLB update batches

[Eugenio Perez Martin]

On Thu, Aug 5, 2021 at 9:10 AM Jason Wang  wrote:
>
>
> 在 2021/8/5 下午3:04, Eugenio Perez Martin 写道:
> > On Thu, Aug 5, 2021 at 8:16 AM Jason Wang  wrote:
> >> On Wed, Aug 4, 2021 at 10:44 PM Eugenio Pérez  wrote:
> >>> With the introduction of the batch hinting, meaningless batches can be
> >>> created with no IOTLB updates if the memory region was skipped by
> >>> vhost_vdpa_listener_skipped_section. This is the case of host notifiers
> >>> memory regions, but others could fall on this category. This caused the
> >>> vdpa device to receive dma mapping settings with no changes, a possibly
> >>> expensive operation for nothing.
> >>>
> >>> To avoid that, VHOST_IOTLB_BATCH_BEGIN hint is delayed until we have a
> >>> meaningful (not skipped section) mapping or unmapping operation, and
> >>> VHOST_IOTLB_BATCH_END is not written unless at least one of _UPDATE /
> >>> _INVALIDATE has been issued.
> >> Hi Eugeni:
> >>
> >> This should work but it looks to me it's better to optimize the kernel.
> >>
> >> E.g to make sure we don't send set_map() if there is no real updating
> >> between batch start and end.
> >>
> > Hi Jason,
> >
> > I think we should do both in parallel anyway.
>
>
> Ok, I'm fine with this.
>
>
> >   We also obtain an
> > (unmeasured at this moment) decrease in startup time for qemu with
> > vdpa this way, for example. I consider that this particular RFC has
> > room to improve or change totally of course.
> >
> > I've made these changes in the kernel too, just counting the number of
> > memory updates and not calling set_map if no actual changes have been
> > made.
>
>
> Right, that is what we want to have.
>
>
> >
> >> This makes sure that it can work for all kinds of userspace (not only for 
> >> Qemu).
> >>
> >> Another optimization is to batch the memory region transaction by adding:
> >>
> >> memory_region_transaction_begin() and memory_region_transaction_end() in
> >>
> >> both vhost_vdpa_host_notifiers_init() and 
> >> vhost_vdpa_host_notifiers_uninit().
> >>
> >> This can make sure only at least one memory transactions when
> >> adding/removing host notifier regions.
> >>
> > That solves the updates about memory regions, but it does not solve
> > the case where updating memory regions are not interesting to the
> > device.
>
>
> Kind of, I guess with this we only get one more set_map().
>
>
> > In other words, where all the changes meet
> > vhost_vdpa_listener_skipped_section() == true. I did not spend a lot
> > of time trying to raise these though, maybe it happens when
> > hot-plugging a device, for example?
>
>
> Yes, so transaction is per device optimization that can't help in this case.
>

I've left it out, since we already obtain 0 IOTLB update commits with
this approach. Let me know if you think it should be included.

>
> >
> > We could abstract these changes in memory_region_transaction_begin() /
> > memory_region_transaction_end() wrappers though.
> >
> > Thanks!
> >
> >> Thanks
> >>
> >>> Signed-off-by: Eugenio Pérez 
> >>> ---
> >>>   include/hw/virtio/vhost-vdpa.h |  1 +
> >>>   hw/virtio/vhost-vdpa.c | 38 +++---
> >>>   2 files changed, 27 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/include/hw/virtio/vhost-vdpa.h 
> >>> b/include/hw/virtio/vhost-vdpa.h
> >>> index e98e327f12..0a7edbe4eb 100644
> >>> --- a/include/hw/virtio/vhost-vdpa.h
> >>> +++ b/include/hw/virtio/vhost-vdpa.h
> >>> @@ -23,6 +23,7 @@ typedef struct vhost_vdpa {
> >>>   int device_fd;
> >>>   int index;
> >>>   uint32_t msg_type;
> >>> +size_t n_iotlb_sent_batch;
>
>
> Not a native speaker but we probably need a better name, e.g "n_mr_updated?"
>

I totally agree.

>
> >>>   MemoryListener listener;
> >>>   struct vhost_dev *dev;
> >>>   VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >>> index 6ce94a1f4d..2517fc6103 100644
> >>> --- a/hw/virtio/vhost-vdpa.c
> >>> +++ b/hw/virtio/vhost-vdpa.c
> >>> @@ -89,19 +89,13 @@ static int vhost_vdpa_dma_unmap(struct vhost_vdpa *v, 
> >>> hwaddr iova,
> >>>   return ret;
> >>>   }
> >>>
> >>> -static void vhost_vdpa_listener_begin(MemoryListener *listener)
> >>> +static void vhost_vdpa_listener_begin_batch(struct vhost_vdpa *v)
> >>>   {
> >>> -struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, 
> >>> listener);
> >>> -struct vhost_dev *dev = v->dev;
> >>> -struct vhost_msg_v2 msg = {};
> >>>   int fd = v->device_fd;
> >>> -
> >>> -if (!(dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH))) {
> >>> -return;
> >>> -}
> >>> -
> >>> -msg.type = v->msg_type;
> >>> -msg.iotlb.type = VHOST_IOTLB_BATCH_BEGIN;
> >>> +struct vhost_msg_v2 msg = {
> >>> +.type = v->msg_type,
> >>> +.iotlb.type = VHOST_IOTLB_BATCH_BEGIN,
> >>> +};
> >>>
> >>>   if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) {
> >>>   error_report("failed to write, fd=%

[PATCH v2] vhost-vdpa: Do not send empty IOTLB update batches

[Eugenio Pérez]

With the introduction of the batch hinting, meaningless batches can be
created with no IOTLB updates if the memory region was skipped by
vhost_vdpa_listener_skipped_section. This is the case of host notifiers
memory regions, device un/realize, and others. This causes the vdpa
device to receive dma mapping settings with no changes, a possibly
expensive operation for nothing.

To avoid that, VHOST_IOTLB_BATCH_BEGIN hint is delayed until we have a
meaningful (not skipped section) mapping or unmapping operation, and
VHOST_IOTLB_BATCH_END is not written unless at least one of _UPDATE /
_INVALIDATE has been issued.

Signed-off-by: Eugenio Pérez 
---
 include/hw/virtio/vhost-vdpa.h |  1 +
 hw/virtio/vhost-vdpa.c | 50 ++
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index e98e327f12..6538572a6f 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -23,6 +23,7 @@ typedef struct vhost_vdpa {
 int device_fd;
 int index;
 uint32_t msg_type;
+size_t n_mr_updated;
 MemoryListener listener;
 struct vhost_dev *dev;
 VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 6ce94a1f4d..512fa18d68 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -89,19 +89,13 @@ static int vhost_vdpa_dma_unmap(struct vhost_vdpa *v, 
hwaddr iova,
 return ret;
 }
 
-static void vhost_vdpa_listener_begin(MemoryListener *listener)
+static void vhost_vdpa_listener_begin_batch(struct vhost_vdpa *v)
 {
-struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener);
-struct vhost_dev *dev = v->dev;
-struct vhost_msg_v2 msg = {};
 int fd = v->device_fd;
-
-if (!(dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH))) {
-return;
-}
-
-msg.type = v->msg_type;
-msg.iotlb.type = VHOST_IOTLB_BATCH_BEGIN;
+struct vhost_msg_v2 msg = {
+.type = v->msg_type,
+.iotlb.type = VHOST_IOTLB_BATCH_BEGIN,
+};
 
 if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) {
 error_report("failed to write, fd=%d, errno=%d (%s)",
@@ -109,6 +103,25 @@ static void vhost_vdpa_listener_begin(MemoryListener 
*listener)
 }
 }
 
+static bool vhost_vdpa_iotlb_batch_is_started(const struct vhost_vdpa *v)
+{
+return v->n_mr_updated != 0;
+}
+
+static void vhost_vdpa_iotlb_batch_begin_once(struct vhost_vdpa *v)
+{
+if (!vhost_vdpa_iotlb_batch_is_started(v)) {
+vhost_vdpa_listener_begin_batch(v);
+}
+
+v->n_mr_updated++;
+}
+
+static void vhost_vdpa_iotlb_batch_reset(struct vhost_vdpa *v)
+{
+v->n_mr_updated = 0;
+}
+
 static void vhost_vdpa_listener_commit(MemoryListener *listener)
 {
 struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener);
@@ -120,6 +133,10 @@ static void vhost_vdpa_listener_commit(MemoryListener 
*listener)
 return;
 }
 
+if (vhost_vdpa_iotlb_batch_is_started(v)) {
+return;
+}
+
 msg.type = v->msg_type;
 msg.iotlb.type = VHOST_IOTLB_BATCH_END;
 
@@ -127,6 +144,8 @@ static void vhost_vdpa_listener_commit(MemoryListener 
*listener)
 error_report("failed to write, fd=%d, errno=%d (%s)",
  fd, errno, strerror(errno));
 }
+
+vhost_vdpa_iotlb_batch_reset(v);
 }
 
 static void vhost_vdpa_listener_region_add(MemoryListener *listener,
@@ -170,6 +189,10 @@ static void vhost_vdpa_listener_region_add(MemoryListener 
*listener,
 
 llsize = int128_sub(llend, int128_make64(iova));
 
+if (v->dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH)) {
+vhost_vdpa_iotlb_batch_begin_once(v);
+}
+
 ret = vhost_vdpa_dma_map(v, iova, int128_get64(llsize),
  vaddr, section->readonly);
 if (ret) {
@@ -221,6 +244,10 @@ static void vhost_vdpa_listener_region_del(MemoryListener 
*listener,
 
 llsize = int128_sub(llend, int128_make64(iova));
 
+if (v->dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH)) {
+vhost_vdpa_iotlb_batch_begin_once(v);
+}
+
 ret = vhost_vdpa_dma_unmap(v, iova, int128_get64(llsize));
 if (ret) {
 error_report("vhost_vdpa dma unmap error!");
@@ -234,7 +261,6 @@ static void vhost_vdpa_listener_region_del(MemoryListener 
*listener,
  * depends on the addnop().
  */
 static const MemoryListener vhost_vdpa_memory_listener = {
-.begin = vhost_vdpa_listener_begin,
 .commit = vhost_vdpa_listener_commit,
 .region_add = vhost_vdpa_listener_region_add,
 .region_del = vhost_vdpa_listener_region_del,
-- 
2.27.0

Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host

[Ani Sinha]

On Fri, 6 Aug 2021, Philippe Mathieu-Daudé wrote:

> On 8/6/21 12:52 PM, Ani Sinha wrote:
> > On Fri, 6 Aug 2021, Igor Mammedov wrote:
> >> On Thu, 5 Aug 2021 19:42:35 +0530 (IST)
> >> Ani Sinha  wrote:
> >>> On Thu, 5 Aug 2021, Ani Sinha wrote:
>  On Thu, 5 Aug 2021, Ani Sinha wrote:
> > On Thu, 5 Aug 2021, Igor Mammedov wrote:
> >> On Mon, 26 Jul 2021 22:27:43 +0530
> >> Ani Sinha  wrote:
> >>
> >>> All existing code using acpi_get_i386_pci_host() checks for a non-null
> >>> return value from this function call. Instead of returning early when 
> >>> the value
> >>> returned is NULL, assert instead. Since there are only two possible 
> >>> host buses
> >>> for i386 - q35 and i440fx, a null value return from the function does 
> >>> not make
> >>> sense in most cases and is likely an error situation.
> >>>
> >>> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> >>>
> >>> Signed-off-by: Ani Sinha 
> >>> ---
> >>>  hw/acpi/pcihp.c  |  8 
> >>>  hw/i386/acpi-build.c | 15 ++-
> >>>  2 files changed, 14 insertions(+), 9 deletions(-)
> >>>
> >>> changelog:
> >>> v1: initial patch
> >>> v2: removed comment addition - that can be sent as a separate patch.
> >>> v3: added assertion for null host values for all cases except one.
> >>>
> >>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> >>> index f4d706e47d..054ee8cbc5 100644
> >>> --- a/hw/acpi/pcihp.c
> >>> +++ b/hw/acpi/pcihp.c
> >>> @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> >>>  bsel_is_set = true;
> >>>
> >>>  if (!host) {
> >>> +/*
> >>> + * This function can be eventually called from
> >>> + * qemu_devices_reset() -> acpi_pcihp_reset() even
> >>> + * for architectures other than i386. Hence, we need
> >>> + * to ignore null values for host here.
> >>> + */
> >>>  return;
> >>>  }
> >>
> >> I suspect it's a MIPS target that call this code unnecessarily.
> >> It would be better to get rid of this condition altogether.
> >> Frr that I can suggest to make acpi_pcihp_reset() stub and
> >> replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
> >> for MIPS.
> >>
> >> then a bunch of asserts/ifs won't be necessary,
> >> just one in acpi_get_i386_pci_host() will be sufficient.
> >>
> >
> > OK this is a good idea.
> > I can see that mips-softmmu-config-devices.h has
> > CONFIG_ACPI_X86 turned on for mips. This does not seem right.
> >
> > The issue here is:
> >
> > $ grep -R CONFIG_ACPI_X86 *
> > devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
> >
> > So after
> >
> > -CONFIG_ACPI_X86=y
> > -CONFIG_PIIX4=y
> >
> > (the second one is needed because after removing first one we get:
> >
> > /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function 
> > `piix4_create':
> > /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
> > reference to `piix4_pm_init'
> >
> > This is because in hw/acpi/meson.build, piix4.c is conditional on
> > CONFIG_ACPI_X86. )
> >
> > /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
> > function `gt64120_pci_set_irq':
> > /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
> > undefined reference to `piix4_dev'
> > /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
> > `mips_malta_init':
> > /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
> > reference to `piix4_create'
> >
> > So should mips be doing piix stuff anyway? Is Piix4 etc not x86 
> > specific?
>
> PIIX, PIIX3 and PIIX4 are generic chipsets, not X86-specific.
>
> QEMU's PIIX3 is a Frankenstein to support virtualization to a chipset
> not designed for it.
> If you look at it, the X86 machine use a PIIX3 but the PIIX3 doesn't
> even provide an ACPI function. It appeared in the PIIX4. The kludge is
> to instanciate the PIIX4.acpi from the PIIX3 and X86 ppl are happy with
> it, but it makes it ugly for the other architectures.
>
>  Apparently this is by design:
>  https://qemu.readthedocs.io/en/stable/system/target-mips.html
>
> What do you mean "by design"? The Malta uses a PIIX4 chipset for its
> southbridge indeed.

I meant it was intentional and not by accident.

>
>  which means mips malta will continue to use the x86 specific functions
>  like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
>  will result in a double symbol definition because CONFIG_PC is off for
>  mips.
> 
> >>>
> >>> Also to be noted that there is a stub for acpi_get_i386_pci_host() which
> >>> simply returns NULL. This activates when CONFIG_PC is disabled. It is this
> >>> stub that 

Re: [RFC PATCH 02/13] target/riscv: Support UXL32 for branch instructions

[LIU Zhiwei]

On 2021/8/10 上午3:34, Richard Henderson wrote:

On 8/8/21 3:45 PM, LIU Zhiwei wrote:


On 2021/8/6 上午3:06, Richard Henderson wrote:

On 8/4/21 4:53 PM, LIU Zhiwei wrote:

+static TCGv gpr_src_u(DisasContext *ctx, int reg_num)
+{
+    if (reg_num == 0) {
+    return ctx->zero;
+    }
+    if (ctx->uxl32) {
+    tcg_gen_ext32u_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]);
+    }
+    return cpu_gpr[reg_num];
+}
+
+static TCGv gpr_src_s(DisasContext *ctx, int reg_num)
+{
+    if (reg_num == 0) {
+    return ctx->zero;
+    }
+    if (ctx->uxl32) {
+    tcg_gen_ext32s_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]);
+    }
+    return cpu_gpr[reg_num];
+}


This is bad: you cannot modify the source registers like this.


In my opinion, when uxl32, the only meaningful part is the low 32 
bits, and it doesn't matter to modify the high parts.


Then why does the architecture manual specify that when registers are 
modified the value written sign-extended?  This effect should be 
visible...



Hi Richard,

I  still don't know why the value written sign-extended.  If that's the 
the rule of final specification, I will try to obey it although our 
Linux will not depend on the high part.


Thanks,
Zhiwei





These incorrect modifications will be visible to the kernel on 
transition back to S-mode.


When transition back to S-mode, I think the kernel will save the 
U-mode registers to memory.


... here.  Once we're in S-mode, we have SXLEN, and if SXLEN > UXLEN, 
the high part of the register will be visible.  It really must be 
either (1) sign-extended because U-mode wrote to the register or (2) 
unmodified from the last time S-mode wrote to the register.



r~

Re: [PATCH] tests/acceptance: Test powernv machines

[Cédric Le Goater]

On 8/11/21 11:07 AM, Thomas Huth wrote:
> On 10/08/2021 11.09, Cédric Le Goater wrote:
>> On 8/10/21 10:36 AM, Joel Stanley wrote:
>>> On Tue, 10 Aug 2021 at 08:34, Cédric Le Goater  wrote:

 Fetch the OpenPOWER images to boot the powernv8 and powernv9 machines
 with a simple PCI layout.

 Cc: Cleber Rosa 
 Cc: Wainer dos Santos Moschetta 
 Signed-off-by: Cédric Le Goater 
 ---
   tests/acceptance/boot_linux_console.py | 42 ++
   1 file changed, 42 insertions(+)

 diff --git a/tests/acceptance/boot_linux_console.py 
 b/tests/acceptance/boot_linux_console.py
 index 5248c8097df9..da93a475ca87 100644
 --- a/tests/acceptance/boot_linux_console.py
 +++ b/tests/acceptance/boot_linux_console.py
 @@ -1176,6 +1176,48 @@ def test_ppc64_e500(self):
   tar_hash = '6951d86d644b302898da2fd701739c9406527fe1'
   self.do_test_advcal_2018('19', tar_hash, 'uImage')

 +    def do_test_ppc64_powernv(self, proc):
 +
 +    images_url = 
 ('https://github.com/open-power/op-build/releases/download/v2.7/')
 +
 +    skiboot_url = images_url + 'skiboot.lid'
 +    skiboot_hash = 
 'a9ffcddbf238f86cda4b2cae2882d6bd13cff8489109758a4980efaf154f4a29'
 +    skiboot_path = self.fetch_asset(skiboot_url, 
 asset_hash=skiboot_hash,
 +   algorithm='sha256')
>>>
>>> What's the thought that led you to using this instead of the one that
>>> gets packaged with qemu?
>>
>> Good question.
>>
>> I considered that the skiboot.lid shipped with QEMU was somewhat a default
>> to make things work. The official released versions are the ones used by
>> the outside world on real systems and were a better target for tests.
>>
>> That said, using the default version might be enough. Maintainers, please
>> advise !
> 
> IMHO:
> 
> - We want to test the things that *we* ship
> 
> - We want to download as few things as possible, since downloads
>   often slow down the tests and break CI runs if the network to
>   the download site is not available
> 
>  ==> I'd prefer to use the internal skiboot.lid unless there is
>  really a compelling reason to use the external one.

OK. I changed the test to use the internal skiboot.lid.

Thanks,

C.

[PATCH v2] target/riscv: Don't wrongly override isa version

[LIU Zhiwei]

For some cpu, the isa version has already been set in cpu init function.
Thus only override the isa version when isa version is not set, or
users set different isa version explicitly by cpu parameters.

Signed-off-by: LIU Zhiwei 
---
 target/riscv/cpu.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 991a6bb760..1a2b03d579 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -392,9 +392,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
 RISCVCPU *cpu = RISCV_CPU(dev);
 CPURISCVState *env = &cpu->env;
 RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
-int priv_version = PRIV_VERSION_1_11_0;
-int bext_version = BEXT_VERSION_0_93_0;
-int vext_version = VEXT_VERSION_0_07_1;
+int priv_version = 0;
 target_ulong target_misa = env->misa;
 Error *local_err = NULL;
 
@@ -417,9 +415,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
 }
 }
 
-set_priv_version(env, priv_version);
-set_bext_version(env, bext_version);
-set_vext_version(env, vext_version);
+if (priv_version) {
+set_priv_version(env, priv_version);
+} else if (!env->priv_ver) {
+set_priv_version(env, PRIV_VERSION_1_11_0);
+}
 
 if (cpu->cfg.mmu) {
 set_feature(env, RISCV_FEATURE_MMU);
@@ -497,6 +497,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
 target_misa |= RVH;
 }
 if (cpu->cfg.ext_b) {
+int bext_version = BEXT_VERSION_0_93_0;
 target_misa |= RVB;
 
 if (cpu->cfg.bext_spec) {
@@ -515,6 +516,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
 set_bext_version(env, bext_version);
 }
 if (cpu->cfg.ext_v) {
+int vext_version = VEXT_VERSION_0_07_1;
 target_misa |= RVV;
 if (!is_power_of_2(cpu->cfg.vlen)) {
 error_setg(errp,
-- 
2.25.1

Re: [PATCH] target/riscv: Don't wrongly overide isa version

[Bin Meng]

On Wed, Aug 11, 2021 at 10:07 PM LIU Zhiwei  wrote:
>
>
> On 2021/8/11 下午5:26, Bin Meng wrote:
> > On Tue, Aug 10, 2021 at 11:35 AM LIU Zhiwei  wrote:
> >> For some cpu, the isa version has already been set in cpu init function.
> >> Thus only overide the isa version when isa version is not set, or
> > typo: override, please fix the commit title as well
> OK
> >
> >> users set different isa version explicitly by cpu parameters.
> >>
> >> Signed-off-by: LIU Zhiwei 
> >> ---
> >>   target/riscv/cpu.c | 14 --
> >>   1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 991a6bb760..425efba0c8 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -392,9 +392,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> >> **errp)
> >>   RISCVCPU *cpu = RISCV_CPU(dev);
> >>   CPURISCVState *env = &cpu->env;
> >>   RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> >> -int priv_version = PRIV_VERSION_1_11_0;
> >> -int bext_version = BEXT_VERSION_0_93_0;
> >> -int vext_version = VEXT_VERSION_0_07_1;
> >> +int priv_version = env->priv_ver;
> >>   target_ulong target_misa = env->misa;
> >>   Error *local_err = NULL;
> >>
> >> @@ -417,9 +415,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> >> **errp)
> >>   }
> >>   }
> >>
> >> -set_priv_version(env, priv_version);
> >> -set_bext_version(env, bext_version);
> >> -set_vext_version(env, vext_version);
> >> +if (!env->priv_ver) {
> >> +set_priv_version(env, PRIV_VERSION_1_11_0);
> >> +} else if (env->priv_ver != priv_version) {
> >> +set_priv_version(env, priv_version);
> >> +}
> > This logic seems incorrect to me. So if cpu init function does not set
> > the priv, and cfg set it to v1.10, v1.11 will be set in the new logic.
>
> Yes,  it's also here.
>

But the previous logic makes sure the cfg value overrides the cpu init
value which seems to be intended. So in that case, it should still be
v1.10 not v1.11.

Regards,
Bin

Re: [PATCH] target/riscv: Don't wrongly overide isa version

[LIU Zhiwei]

On 2021/8/11 下午5:26, Bin Meng wrote:

On Tue, Aug 10, 2021 at 11:35 AM LIU Zhiwei  wrote:

For some cpu, the isa version has already been set in cpu init function.
Thus only overide the isa version when isa version is not set, or

typo: override, please fix the commit title as well

OK



users set different isa version explicitly by cpu parameters.

Signed-off-by: LIU Zhiwei 
---
  target/riscv/cpu.c | 14 --
  1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 991a6bb760..425efba0c8 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -392,9 +392,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
  RISCVCPU *cpu = RISCV_CPU(dev);
  CPURISCVState *env = &cpu->env;
  RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
-int priv_version = PRIV_VERSION_1_11_0;
-int bext_version = BEXT_VERSION_0_93_0;
-int vext_version = VEXT_VERSION_0_07_1;
+int priv_version = env->priv_ver;
  target_ulong target_misa = env->misa;
  Error *local_err = NULL;

@@ -417,9 +415,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
  }
  }

-set_priv_version(env, priv_version);
-set_bext_version(env, bext_version);
-set_vext_version(env, vext_version);
+if (!env->priv_ver) {
+set_priv_version(env, PRIV_VERSION_1_11_0);
+} else if (env->priv_ver != priv_version) {
+set_priv_version(env, priv_version);
+}

This logic seems incorrect to me. So if cpu init function does not set
the priv, and cfg set it to v1.10, v1.11 will be set in the new logic.


Yes,  it's also here.

Thanks,
Zhiwei


The previous logic makes sure the cfg value overrides the cpu init
value which seems to be intended.


  if (cpu->cfg.mmu) {
  set_feature(env, RISCV_FEATURE_MMU);
@@ -497,6 +497,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
  target_misa |= RVH;
  }
  if (cpu->cfg.ext_b) {
+int bext_version = BEXT_VERSION_0_93_0;
  target_misa |= RVB;

  if (cpu->cfg.bext_spec) {
@@ -515,6 +516,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
  set_bext_version(env, bext_version);
  }
  if (cpu->cfg.ext_v) {
+int vext_version = VEXT_VERSION_0_07_1;
  target_misa |= RVV;
  if (!is_power_of_2(cpu->cfg.vlen)) {
  error_setg(errp,
--

Regards,
Bin

Re: [PATCH v12] qapi: introduce 'query-x86-cpuid' QMP command.

[Eduardo Habkost]

On Wed, Aug 11, 2021 at 9:44 AM Thomas Huth  wrote:
>
> On 11/08/2021 15.40, Eduardo Habkost wrote:
> > On Wed, Aug 11, 2021 at 2:10 AM Thomas Huth  wrote:
> >>
> >> On 10/08/2021 20.56, Eduardo Habkost wrote:
> >>> On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
>  Is this intended to be a stable interface?  Interfaces intended just for
>  debugging usually aren't.
> >>>
> >>> I don't think we need to make it a stable interface, but I won't
> >>> mind if we declare it stable.
> >>
> >> If we don't feel 100% certain yet, it's maybe better to introduce this with
> >> a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's clear
> >> that this is only experimental/debugging/not-stable yet. Just my 0.02 €.
> >
> > That would be my expectation. Is this a documented policy?
> >
>
> According to docs/interop/qmp-spec.txt :
>
>   Any command or member name beginning with "x-" is deemed
>   experimental, and may be withdrawn or changed in an incompatible
>   manner in a future release.

Thanks! I had looked at other QMP docs, but not qmp-spec.txt.

In my reply above, please read "make it a stable interface" as
"declare it as supported by not using the 'x-' prefix".

I don't think we have to make it stable, but I won't argue against it
if the current proposal is deemed acceptable by other maintainers.

Personally, I'm still frustrated by the complexity of the current
proposal, but I don't want to block it just because of my frustration.

-- 
Eduardo

Re: [PATCH v12] qapi: introduce 'query-x86-cpuid' QMP command.

[Thomas Huth]

On 11/08/2021 15.40, Eduardo Habkost wrote:

On Wed, Aug 11, 2021 at 2:10 AM Thomas Huth  wrote:


On 10/08/2021 20.56, Eduardo Habkost wrote:

On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:

Is this intended to be a stable interface?  Interfaces intended just for
debugging usually aren't.


I don't think we need to make it a stable interface, but I won't
mind if we declare it stable.


If we don't feel 100% certain yet, it's maybe better to introduce this with
a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's clear
that this is only experimental/debugging/not-stable yet. Just my 0.02 €.


That would be my expectation. Is this a documented policy?



According to docs/interop/qmp-spec.txt :

 Any command or member name beginning with "x-" is deemed
 experimental, and may be withdrawn or changed in an incompatible
 manner in a future release.

 Thomas

Re: [PATCH v12] qapi: introduce 'query-x86-cpuid' QMP command.

[Eduardo Habkost]

On Wed, Aug 11, 2021 at 2:10 AM Thomas Huth  wrote:
>
> On 10/08/2021 20.56, Eduardo Habkost wrote:
> > On Sat, Aug 07, 2021 at 04:22:42PM +0200, Markus Armbruster wrote:
> >> Is this intended to be a stable interface?  Interfaces intended just for
> >> debugging usually aren't.
> >
> > I don't think we need to make it a stable interface, but I won't
> > mind if we declare it stable.
>
> If we don't feel 100% certain yet, it's maybe better to introduce this with
> a "x-" prefix first, isn't it? I.e. "x-query-x86-cpuid" ... then it's clear
> that this is only experimental/debugging/not-stable yet. Just my 0.02 €.

That would be my expectation. Is this a documented policy?

-- 
Eduardo

Re: [PULL 0/7] Maintainers 20210811 patches

[Peter Maydell]

On Wed, 11 Aug 2021 at 07:44, Gerd Hoffmann  wrote:
>
> The following changes since commit 703e8cd6189cf699c8d5c094bc68b5f3afa6ad71:
>
>   Update version for v6.1.0-rc3 release (2021-08-10 19:08:09 +0100)
>
> are available in the Git repository at:
>
>   git://git.kraxel.org/qemu tags/maintainers-20210811-pull-request
>
> for you to fetch changes up to a4de5e8a0667e3ee43ca9953ec7fd11ff19f2c92:
>
>   MAINTAINERS: update virtio-gpu entry. (2021-08-11 08:39:16 +0200)
>
> 
> MAINTAINERS: update kraxel's entries.
>
> 

I'll keep this in my list in case we need to roll an rc4, but
if we are able to release 6.1 without needing another rc then
I'll just hold this over until we reopen trunk for 6.2.

thanks
-- PMM

Re: [PATCH 0/2] Set user creatable for flag ibex uart and plic

[Damien Hedde]

On 8/11/21 1:15 PM, Peter Maydell wrote:
> On Wed, 11 Aug 2021 at 10:43, Damien Hedde  wrote:
>> This small series only consist in setting the user_creatable flag
>> of ibex_uart and ibex_plic devices. These two devices are already
>> using properties to configure themselves so nothing else is required.
>>
>> Note that this change alone will not allow creation of these devices
>> using -device cli option or device_add qmp command as they are sysbus
>> devices.
>>
>> We do that because we are currently working on adding the possibily
>> to configure/build a machine from qmp commands (see this rfc:
>> https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg03706.html).
>> We are using these simple devices in order to test our additions.
>>
>> We prefer to send these 2 patches on a separate series as they are not
>> really related to the main topic. We will send a following series
>> for the additions.
> 
> No, these patches should go in with your other series that
> requires them, please. As standalone patches they are definitely
> wrong, because (as you note) you cannot usefully user-create a
> sysbus device like these from the command line.
> 
> Even there I'm not convinced that just marking the devices
> user-creatable is the right thing -- if we support creating
> a complete machine from QMP commands we probably want to think
> about whether that means we need to have separate categories
> of "only creatable from C code", "only creatable as part of
> QMP machine creation", "creatable on commandline but only
> for cold-plug", and "hotpluggable".
> 
> -- PMM
> 

I understand, we will put them in the main series where
we will discuss all of this.

Thanks,
--
Damien

qcow2 perfomance: read-only IO on the guest generates high write IO on the host

[Christopher Pereira]

Hi,

I'm reading a directory with 5.000.000 files (2,4 GB) inside a guest 
using "find | grep -c".


On the host I saw high write IO (40 MB/s !) during over 1 hour using 
virt-top.


I later repeated the read-only operation inside the guest and no 
additional data was written on the host. The operation took only some 
seconds.


I believe QEMU was creating some kind of cache or metadata map the first 
time I accessed the inodes.


But I wonder why the cache or metadata map wasn't available the first 
time and why QEMU had to recreate it?


The VM has "compressed base <- snap 1" and base was converted without 
prealloc.


Is it because we created the base using convert without metadata 
prealloc and so the metadata map got lost?


I will do some experiments soon using convert + metadata prealloc and 
probably find out myself, but I will happy to read your comments and 
gain some additional insights.

If it the problem persists, I would try again without compression.

Additional info:

 * Guest fs is xfs.
 * (I believe the snapshot didn't significantly increase in size, but I
   would need to double check)
 * This is a production host with old QEMU emulator version 2.3.0
   (qemu-kvm-ev-2.3.0-31.el7_2.10.1)

Re: [PATCH v2] docs: make sphinx-build be quiet by default

[Daniel P . Berrangé]

On Wed, Aug 11, 2021 at 03:22:43PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 10, 2021 at 3:32 PM Daniel P. Berrangé 
> wrote:
> 
> > The sphinx-build is fairly verbose spitting out pages of output to the
> > console, which causes errors from other build commands to be scrolled
> > off the top of the terminal. This can leave the mistaken impression that
> > the build passed, when in fact there was a failure.
> >
> > Signed-off-by: Daniel P. Berrangé 
> >
> 
> Without this patch, I miss the warnings 99% of the time!!
> 
> You are missing one -q though:
> 
> diff --git a/docs/meson.build b/docs/meson.build
> index 42d7555bc4..51fa902cd9 100644
> --- a/docs/meson.build
> +++ b/docs/meson.build
> @@ -78,7 +78,7 @@ if build_docs
>  input: files('conf.py'),
>  depfile: 'docs.d',
>  depend_files: [ sphinx_extn_depends, sphinx_template_files
> ],
> -command: [SPHINX_ARGS, '-Ddepfile=@DEPFILE@',
> +command: [SPHINX_ARGS, '-q', '-Ddepfile=@DEPFILE@',
>'-Ddepfile_stamp=@OUTPUT0@',
>'-b', 'html', '-d', private_dir,
>input_dir, output_dir])
> 
> Why not update SPHINX_ARGS instead?

🤦 that would be a better idea.


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

Re: [PATCH v2] docs: make sphinx-build be quiet by default

[Marc-André Lureau]

Hi

On Tue, Aug 10, 2021 at 3:32 PM Daniel P. Berrangé 
wrote:

> The sphinx-build is fairly verbose spitting out pages of output to the
> console, which causes errors from other build commands to be scrolled
> off the top of the terminal. This can leave the mistaken impression that
> the build passed, when in fact there was a failure.
>
> Signed-off-by: Daniel P. Berrangé 
>

Without this patch, I miss the warnings 99% of the time!!

You are missing one -q though:

diff --git a/docs/meson.build b/docs/meson.build
index 42d7555bc4..51fa902cd9 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -78,7 +78,7 @@ if build_docs
 input: files('conf.py'),
 depfile: 'docs.d',
 depend_files: [ sphinx_extn_depends, sphinx_template_files
],
-command: [SPHINX_ARGS, '-Ddepfile=@DEPFILE@',
+command: [SPHINX_ARGS, '-q', '-Ddepfile=@DEPFILE@',
   '-Ddepfile_stamp=@OUTPUT0@',
   '-b', 'html', '-d', private_dir,
   input_dir, output_dir])

Why not update SPHINX_ARGS instead?

---

>
> In v2:
>
>  - This time with the extra trailing ',' actually committed
>
>  docs/meson.build | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/docs/meson.build b/docs/meson.build
> index 300b134329..42d7555bc4 100644
> --- a/docs/meson.build
> +++ b/docs/meson.build
> @@ -21,7 +21,7 @@ if sphinx_build.found()
>run_command('mkdir', ['-p', tmpdir / 'sphinx'])
>run_command('touch', [tmpdir / 'sphinx/index.rst'])
>sphinx_build_test_out = run_command(SPHINX_ARGS + [
> -'-c', meson.current_source_dir(),
> +'-q', '-c', meson.current_source_dir(),
>  '-b', 'html', tmpdir / 'sphinx',
>  tmpdir / 'sphinx/out'])
>build_docs = (sphinx_build_test_out.returncode() == 0)
> @@ -98,8 +98,9 @@ if build_docs
>input: this_manual,
>install: build_docs,
>install_dir: install_dirs,
> -  command: [SPHINX_ARGS, '-b', 'man', '-d',
> private_dir,
> -input_dir,
> meson.current_build_dir()])
> +  command: [SPHINX_ARGS, '-q', '-b', 'man',
> +'-d', private_dir, input_dir,
> +meson.current_build_dir()])
>
>alias_target('sphinxdocs', sphinxdocs)
>alias_target('html', sphinxdocs)
> --
> 2.31.1
>
>
>

-- 
Marc-André Lureau

Re: [PATCH 12/19] target/ppc/pmu_book3s_helper.c: enable PMC1 counter negative EBB

[Daniel Henrique Barboza]

On 8/11/21 12:40 AM, David Gibson wrote:

On Tue, Aug 10, 2021 at 05:26:09PM -0300, Daniel Henrique Barboza wrote:



On 8/10/21 1:01 AM, David Gibson wrote:

On Mon, Aug 09, 2021 at 10:10:50AM -0300, Daniel Henrique Barboza wrote:

This patch starts the counter negative EBB support by enabling PMC1
counter negative condition.

A counter negative condition happens when a performance monitor counter
reaches the value 0x8000. When that happens, if a counter negative
condition is enabled in that counter, a performance monitor alert is
triggered. For PMC1, this condition is enabled by MMCR0_PMC1CE.

An icount-based logic is used to predict when we need to wake up the timer
to trigger the alert in both PM_INST_CMPL (0x2) and PM_CYC (0x1E) events.
The timer callback will then trigger a PPC_INTERRUPT_PMC which will become a
event-based exception later.

Some EBB powerpc kernel selftests are passing after this patch, but a
substancial amount of them relies on other PMCs to be enabled and events
that we don't support at this moment. We'll address that in the next
patches.

Signed-off-by: Daniel Henrique Barboza 
---
   target/ppc/cpu.h   |   1 +
   target/ppc/pmu_book3s_helper.c | 127 +++--
   2 files changed, 92 insertions(+), 36 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 1d38b8cf7a..5c81d459f4 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -350,6 +350,7 @@ typedef struct ppc_v3_pate_t {
   #define MMCR0_EBE   PPC_BIT(43) /* Perf Monitor EBB Enable */
   #define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
   #define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
+#define MMCR0_PMC1CE PPC_BIT(48)
   #define MMCR1_PMC1SEL_SHIFT (63 - 39)
   #define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
index 43cc0eb722..58ae65e22b 100644
--- a/target/ppc/pmu_book3s_helper.c
+++ b/target/ppc/pmu_book3s_helper.c
@@ -25,6 +25,7 @@
* and SPAPR code.
*/
   #define PPC_CPU_FREQ 10
+#define COUNTER_NEGATIVE_VAL 0x8000
   static uint64_t get_cycles(uint64_t icount_delta)
   {
@@ -32,22 +33,9 @@ static uint64_t get_cycles(uint64_t icount_delta)
   NANOSECONDS_PER_SECOND);
   }
-static void update_PMC_PM_INST_CMPL(CPUPPCState *env, int sprn,
-uint64_t icount_delta)
-{
-env->spr[sprn] += icount_delta;
-}
-
-static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
-  uint64_t icount_delta)
-{
-env->spr[sprn] += get_cycles(icount_delta);
-}
-
-static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
-uint64_t icount_delta)
+static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
   {
-int event;
+int event = 0x0;
   switch (sprn) {
   case SPR_POWER_PMC1:
@@ -65,11 +53,35 @@ static void update_programmable_PMC_reg(CPUPPCState *env, 
int sprn,
   case SPR_POWER_PMC4:
   event = MMCR1_PMC4SEL & env->spr[SPR_POWER_MMCR1];
   break;
+case SPR_POWER_PMC5:
+event = 0x2;
+break;
+case SPR_POWER_PMC6:
+event = 0x1E;
+break;


This looks like a nice cleanup that would be better folded into an
earlier patch.


   default:
-return;
+break;
   }
-switch (event) {
+return event;
+}
+
+static void update_PMC_PM_INST_CMPL(CPUPPCState *env, int sprn,
+uint64_t icount_delta)
+{
+env->spr[sprn] += icount_delta;
+}
+
+static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
+  uint64_t icount_delta)
+{
+env->spr[sprn] += get_cycles(icount_delta);
+}
+
+static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
+uint64_t icount_delta)
+{
+switch (get_PMC_event(env, sprn)) {
   case 0x2:
   update_PMC_PM_INST_CMPL(env, sprn, icount_delta);
   break;
@@ -99,30 +111,57 @@ static void update_PMCs(CPUPPCState *env, uint64_t 
icount_delta)
   update_PMC_PM_CYC(env, SPR_POWER_PMC6, icount_delta);
   }
+static void set_PMU_excp_timer(CPUPPCState *env)
+{
+uint64_t timeout, now, remaining_val;
+
+if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE)) {
+return;
+}
+
+remaining_val = COUNTER_NEGATIVE_VAL - env->spr[SPR_POWER_PMC1];
+
+switch (get_PMC_event(env, SPR_POWER_PMC1)) {
+case 0x2:
+timeout = icount_to_ns(remaining_val);
+break;
+case 0x1e:
+timeout = muldiv64(remaining_val, NANOSECONDS_PER_SECOND,
+   PPC_CPU_FREQ);


So.. this appears to be simulating to the guest that cycles are
occurring at a constant rate, consistent with the advertised CPU
frequency.  Which sounds right, except... it's not clear to me that
you're using the same logic to generate the values you read from the
cycles PM

Re: [PATCH 0/2] Set user creatable for flag ibex uart and plic

[Peter Maydell]

On Wed, 11 Aug 2021 at 10:43, Damien Hedde  wrote:
> This small series only consist in setting the user_creatable flag
> of ibex_uart and ibex_plic devices. These two devices are already
> using properties to configure themselves so nothing else is required.
>
> Note that this change alone will not allow creation of these devices
> using -device cli option or device_add qmp command as they are sysbus
> devices.
>
> We do that because we are currently working on adding the possibily
> to configure/build a machine from qmp commands (see this rfc:
> https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg03706.html).
> We are using these simple devices in order to test our additions.
>
> We prefer to send these 2 patches on a separate series as they are not
> really related to the main topic. We will send a following series
> for the additions.

No, these patches should go in with your other series that
requires them, please. As standalone patches they are definitely
wrong, because (as you note) you cannot usefully user-create a
sysbus device like these from the command line.

Even there I'm not convinced that just marking the devices
user-creatable is the right thing -- if we support creating
a complete machine from QMP commands we probably want to think
about whether that means we need to have separate categories
of "only creatable from C code", "only creatable as part of
QMP machine creation", "creatable on commandline but only
for cold-plug", and "hotpluggable".

-- PMM

Re: [PATCH] tests/qtest/vhost-user-blk-test: Check whether qemu-storage-daemon is available

[Peter Maydell]

On Wed, 11 Aug 2021 at 11:00, Thomas Huth  wrote:
>
> The vhost-user-blk-test currently hangs if QTEST_QEMU_STORAGE_DAEMON_BINARY
> points to a non-existing binary. Let's improve this situation by checking
> for the availability of the binary first, so we can fail gracefully if
> it is not accessible.
>
> Signed-off-by: Thomas Huth 
> ---
>  tests/qtest/vhost-user-blk-test.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/tests/qtest/vhost-user-blk-test.c 
> b/tests/qtest/vhost-user-blk-test.c
> index 8796c74ca4..6f108a1b62 100644
> --- a/tests/qtest/vhost-user-blk-test.c
> +++ b/tests/qtest/vhost-user-blk-test.c
> @@ -789,6 +789,14 @@ static const char *qtest_qemu_storage_daemon_binary(void)
>  exit(0);
>  }
>
> +/* If we've got a path to the binary, check whether we can access it */
> +if (strchr(qemu_storage_daemon_bin, '/') &&
> +access(qemu_storage_daemon_bin, X_OK) != 0) {
> +fprintf(stderr, "ERROR: '%s' is not accessible\n",
> +qemu_storage_daemon_bin);
> +exit(1);
> +}

It makes sense not to bother starting the test if the binary isn't
even present, but why does the test hang? Shouldn't QEMU cleanly
exit rather than hanging if it turns out that it can't contact
the daemon ?

-- PMM

Re: [PATCH] storage-daemon: Add missing build dependency to the vhost-user-blk-test

[Peter Maydell]

On Wed, 11 Aug 2021 at 10:47, Thomas Huth  wrote:
>
> vhost-user-blk-test needs the qemu-storage-deamon, otherwise it

typo: 'daemon'

> currently hangs. So make sure that we build the daemon before running
> the tests.

-- PMM

Re: [PATCH] Use EGL device extension in display initialization.

[Marc-André Lureau]

Hi

On Wed, Aug 11, 2021 at 11:58 AM Eugene Huang  wrote:

> Hi,
>
>
>
> I have had some hard time to set up git send-email. I am not even sure if
> it is doable here. I read that attachments can be used a last resort for
> first timers. Here are the attachments. Hope it works.
>
>
>

Unfortunately, the patches still fail to apply.
https://patchew.org/QEMU/byapr12mb3192ec20271bf35444ce2ff3d9...@byapr12mb3192.namprd12.prod.outlook.com/

Thanks,
>
> Eugene
>
>
>
> *From:* Marc-André Lureau 
> *Sent:* Friday, August 6, 2021 12:25 AM
> *To:* Eugene Huang 
> *Cc:* qemu-devel@nongnu.org
> *Subject:* Re: [PATCH] Use EGL device extension in display initialization.
>
>
>
> *External email: Use caution opening links or attachments*
>
>
>
> Hi
>
>
>
> On Fri, Aug 6, 2021 at 2:28 AM Eugene Huang  wrote:
>
> This patch enables running generic EGL devices such as Nvidia’s in
> headless mode. It assumes single device. More work is needed to support
> multiple devices.
>
>
>
> Signed-off-by: Eugene Huang 
>
>
>
> Thanks for the patch. It isn't correctly formatted and git apply fails  (
> https://patchew.org/QEMU/byapr12mb319275649a1403c254a9ea43d9...@byapr12mb3192.namprd12.prod.outlook.com/
> ).
> Please use git send-email.
>
>
>
> ---
>
> ui/egl-helpers.c | 41 +
>
> 1 file changed, 37 insertions(+), 4 deletions(-)
>
>
>
> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
>
> index 7c530c2825..c11610c083 100644
>
> --- a/ui/egl-helpers.c
>
> +++ b/ui/egl-helpers.c
>
> @@ -1,6 +1,8 @@
>
> /*
>
>   * Copyright (C) 2015-2016 Gerd Hoffmann 
>
>   *
>
> + * Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights
> reserved.
>
> + *
>
>   * This library is free software; you can redistribute it and/or
>
>   * modify it under the terms of the GNU Lesser General Public
>
>   * License as published by the Free Software Foundation; either
>
> @@ -349,11 +351,26 @@ static EGLDisplay
> qemu_egl_get_display(EGLNativeDisplayType native,
>
>  EGLDisplay dpy = EGL_NO_DISPLAY;
>
>  /* In practise any EGL 1.5 implementation would support the EXT
> extension */
>
> -if (epoxy_has_egl_extension(NULL, "EGL_EXT_platform_base")) {
>
> +if (epoxy_has_egl_extension(NULL, "EGL_EXT_platform_base")
>
> +&& epoxy_has_egl_extension(NULL, "EGL_EXT_platform_device")
>
> +&& (epoxy_has_egl_extension(NULL, "EGL_EXT_device_base")
>
> +|| epoxy_has_egl_extension(NULL, "EGL_EXT_device_enumeration"))) {
>
>  PFNEGLGETPLATFORMDISPLAYEXTPROC getPlatformDisplayEXT =
>
>  (void *) eglGetProcAddress("eglGetPlatformDisplayEXT");
>
>  if (getPlatformDisplayEXT && platform != 0) {
>
> -dpy = getPlatformDisplayEXT(platform, native, NULL);
>
> +if (platform == EGL_PLATFORM_DEVICE_EXT) {
>
> +static const int MAX_DEVICES = 4;
>
> +EGLDeviceEXT eglDevs[MAX_DEVICES];
>
> +EGLint numDevices;
>
> +
>
> +PFNEGLQUERYDEVICESEXTPROC eglQueryDevicesEXT =
>
> +(PFNEGLQUERYDEVICESEXTPROC)
>
> +eglGetProcAddress("eglQueryDevicesEXT");
>
> +eglQueryDevicesEXT(MAX_DEVICES, eglDevs, &numDevices);
>
> +dpy = getPlatformDisplayEXT(platform, eglDevs[0], 0);
>
>
>
> Given that the function has a lengthy comment to explain it, and this is
> quite archaic stuff, I think you should update the comments with your
> additions.
>
>
>
> +} else {
>
> +dpy = getPlatformDisplayEXT(platform, native, NULL);
>
> +}
>
>  }
>
>  }
>
> @@ -386,6 +403,17 @@ static int qemu_egl_init_dpy(EGLNativeDisplayType dpy,
>
>  EGL_ALPHA_SIZE, 0,
>
>  EGL_NONE,
>
>  };
>
> +
>
> +static const EGLint conf_att_pbuffer[] = {
>
> +EGL_SURFACE_TYPE, EGL_PBUFFER_BIT,
>
> +EGL_RED_SIZE, 8,
>
> +EGL_GREEN_SIZE, 8,
>
> +EGL_BLUE_SIZE, 8,
>
> +EGL_DEPTH_SIZE, 1,
>
> +EGL_RENDERABLE_TYPE, EGL_OPENGL_BIT,
>
> +EGL_NONE
>
> +};
>
> +
>
>  EGLint major, minor;
>
>  EGLBoolean b;
>
>  EGLint n;
>
> @@ -411,8 +439,8 @@ static int qemu_egl_init_dpy(EGLNativeDisplayType dpy,
>
>  }
>
>  b = eglChooseConfig(qemu_egl_display,
>
> -gles ? conf_att_gles : conf_att_core,
>
> -&qemu_egl_config, 1, &n);
>
> +gles ? conf_att_gles : (platform == EGL_PLATFORM_DEVICE_EXT ?
> conf_att_pbuffer : conf_att_core),

Re: [PATCH] tests/qtest/vhost-user-blk-test: Check whether qemu-storage-daemon is available

[Alexander Bulekov]

On 210811 1159, Thomas Huth wrote:
> The vhost-user-blk-test currently hangs if QTEST_QEMU_STORAGE_DAEMON_BINARY
> points to a non-existing binary. Let's improve this situation by checking
> for the availability of the binary first, so we can fail gracefully if
> it is not accessible.
> 
> Signed-off-by: Thomas Huth 

I manually removed ./storage-daemon/qemu-storage-daemon and re-ran
qos-test. The test errored-out without hanging.

Reviewed-by: Alexander Bulekov 
Tested-by: Alexander Bulekov 

Re: [PATCH] storage-daemon: Add missing build dependency to the vhost-user-blk-test

[Alexander Bulekov]

On 210811 1147, Thomas Huth wrote:
> vhost-user-blk-test needs the qemu-storage-deamon, otherwise it
> currently hangs. So make sure that we build the daemon before running
> the tests.
> 
> Signed-off-by: Thomas Huth 
> ---

Tested-by: Alexander Bulekov 

[PATCH for 6.1] plugins: do not limit exported symbols if modules are active

[Paolo Bonzini]

On Mac --enable-modules and --enable-plugins are currently incompatible, 
because the
Apple -Wl,-exported_symbols_list command line options prevents the export of any
symbols needed by the modules.  On x86 -Wl,--dynamic-list does not have this 
effect,
but only because the -Wl,--export-dynamic option provided by gmodule-2.0.pc 
overrides
it.  On Apple there is no -Wl,--export-dynamic, because it is the default, and 
thus
no override.

Either way, when modules are active there is no reason to include the 
plugin_ldflags.
While at it, avoid the useless -Wl,--export-dynamic when --enable-plugins is
specified but --enable-modules is not; this way, the GNU and Apple 
configurations
are more similar.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/516
Cc: Alex Bennée 
Signed-off-by: Paolo Bonzini 
---
 configure   |  5 ++---
 plugins/meson.build | 14 --
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/configure b/configure
index 9a79a004d7..a8721601ea 100755
--- a/configure
+++ b/configure
@@ -3187,9 +3187,8 @@ glib_req_ver=2.56
 glib_modules=gthread-2.0
 if test "$modules" = yes; then
 glib_modules="$glib_modules gmodule-export-2.0"
-fi
-if test "$plugins" = "yes"; then
-glib_modules="$glib_modules gmodule-2.0"
+elif test "$plugins" = "yes"; then
+glib_modules="$glib_modules gmodule-noexport-2.0"
 fi
 
 for i in $glib_modules; do
diff --git a/plugins/meson.build b/plugins/meson.build
index e77723010e..bfd5c9822a 100644
--- a/plugins/meson.build
+++ b/plugins/meson.build
@@ -1,9 +1,11 @@
-if 'CONFIG_HAS_LD_DYNAMIC_LIST' in config_host
-  plugin_ldflags = ['-Wl,--dynamic-list=' + (meson.build_root() / 
'qemu-plugins-ld.symbols')]
-elif 'CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST' in config_host
-  plugin_ldflags = ['-Wl,-exported_symbols_list,' + (meson.build_root() / 
'qemu-plugins-ld64.symbols')]
-else
-  plugin_ldflags = []
+plugin_ldflags = []
+# Modules need more symbols than just those in plugins/qemu-plugins.symbols
+if not enable_modules
+  if 'CONFIG_HAS_LD_DYNAMIC_LIST' in config_host
+plugin_ldflags = ['-Wl,--dynamic-list=' + (meson.build_root() / 
'qemu-plugins-ld.symbols')]
+  elif 'CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST' in config_host
+plugin_ldflags = ['-Wl,-exported_symbols_list,' + (meson.build_root() / 
'qemu-plugins-ld64.symbols')]
+  endif
 endif
 
 specific_ss.add(when: 'CONFIG_PLUGIN', if_true: [files(
-- 
2.31.1

[PATCH for 6.1] plugins: do not limit exported symbols if modules are active

[Paolo Bonzini]

On Mac --enable-modules and --enable-plugins are currently incompatible, 
because the
Apple -Wl,-exported_symbols_list command line options prevents the export of any
symbols needed by the modules.  On x86 -Wl,--dynamic-list does not have this 
effect,
but only because the -Wl,--export-dynamic option provided by gmodule-2.0.pc 
overrides
it.  On Apple there is no -Wl,--export-dynamic, because it is the default, and 
thus
no override.

Either way, when modules are active there is no reason to include the 
plugin_ldflags.
While at it, avoid the useless -Wl,--export-dynamic when --enable-plugins is
specified but --enable-modules is not; this way, the GNU and Apple 
configurations
are more similar.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/516
Cc: Alex Bennée 
Signed-off-by: Paolo Bonzini 
---
 configure   |  5 ++---
 plugins/meson.build | 14 --
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/configure b/configure
index 9a79a004d7..a8721601ea 100755
--- a/configure
+++ b/configure
@@ -3187,9 +3187,8 @@ glib_req_ver=2.56
 glib_modules=gthread-2.0
 if test "$modules" = yes; then
 glib_modules="$glib_modules gmodule-export-2.0"
-fi
-if test "$plugins" = "yes"; then
-glib_modules="$glib_modules gmodule-2.0"
+elif test "$plugins" = "yes"; then
+glib_modules="$glib_modules gmodule-noexport-2.0"
 fi
 
 for i in $glib_modules; do
diff --git a/plugins/meson.build b/plugins/meson.build
index e77723010e..bfd5c9822a 100644
--- a/plugins/meson.build
+++ b/plugins/meson.build
@@ -1,9 +1,11 @@
-if 'CONFIG_HAS_LD_DYNAMIC_LIST' in config_host
-  plugin_ldflags = ['-Wl,--dynamic-list=' + (meson.build_root() / 
'qemu-plugins-ld.symbols')]
-elif 'CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST' in config_host
-  plugin_ldflags = ['-Wl,-exported_symbols_list,' + (meson.build_root() / 
'qemu-plugins-ld64.symbols')]
-else
-  plugin_ldflags = []
+plugin_ldflags = []
+# Modules need more symbols than just those in plugins/qemu-plugins.symbols
+if not enable_modules
+  if 'CONFIG_HAS_LD_DYNAMIC_LIST' in config_host
+plugin_ldflags = ['-Wl,--dynamic-list=' + (meson.build_root() / 
'qemu-plugins-ld.symbols')]
+  elif 'CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST' in config_host
+plugin_ldflags = ['-Wl,-exported_symbols_list,' + (meson.build_root() / 
'qemu-plugins-ld64.symbols')]
+  endif
 endif
 
 specific_ss.add(when: 'CONFIG_PLUGIN', if_true: [files(
-- 
2.31.1

[PATCH] tests/qtest/vhost-user-blk-test: Check whether qemu-storage-daemon is available

[Thomas Huth]

The vhost-user-blk-test currently hangs if QTEST_QEMU_STORAGE_DAEMON_BINARY
points to a non-existing binary. Let's improve this situation by checking
for the availability of the binary first, so we can fail gracefully if
it is not accessible.

Signed-off-by: Thomas Huth 
---
 tests/qtest/vhost-user-blk-test.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/tests/qtest/vhost-user-blk-test.c 
b/tests/qtest/vhost-user-blk-test.c
index 8796c74ca4..6f108a1b62 100644
--- a/tests/qtest/vhost-user-blk-test.c
+++ b/tests/qtest/vhost-user-blk-test.c
@@ -789,6 +789,14 @@ static const char *qtest_qemu_storage_daemon_binary(void)
 exit(0);
 }
 
+/* If we've got a path to the binary, check whether we can access it */
+if (strchr(qemu_storage_daemon_bin, '/') &&
+access(qemu_storage_daemon_bin, X_OK) != 0) {
+fprintf(stderr, "ERROR: '%s' is not accessible\n",
+qemu_storage_daemon_bin);
+exit(1);
+}
+
 return qemu_storage_daemon_bin;
 }
 
-- 
2.27.0

Re: [PATCH v4] failover: unregister ROM on unplug

[Laurent Vivier]

On 11/08/2021 08:50, Michael S. Tsirkin wrote:
> On Wed, Jul 21, 2021 at 06:09:05PM +0200, Laurent Vivier wrote:
>> The intend of failover is to allow to migrate a VM with a VFIO
>> networking card without disrupting the network operation by switching
>> to a virtio-net device during the migration.
>>
>> This simple change allows to test failover with a simulated device
>> like e1000e rather than a vfio device, even if it's useless in real
>> life it can help to debug failover.
>>
>> This is interesting to developers that want to test failover on
>> a system with no vfio device. Moreover it simplifies host networking
>> configuration as we can use the same bridge for virtio-net and
>> the other failover networking device.
>>
>> Without this change the migration of a system configured with failover
>> fails with:
>>
>>   ...
>>   -device virtio-net-pci,id=virtionet0,failover=on,...  \
>>   -device e1000,failover_pair_id=virtionet0,... \
>>   ...
>>
>>   (qemu) migrate ...
>>
>>   Unknown ramblock ":00:01.1:00.0/e1000e.rom", cannot accept migration
>>   error while loading state for instance 0x0 of device 'ram'
>>   load of migration failed: Invalid argument
>>
>> This happens because QEMU correctly unregisters the interface vmstate but
>> not the ROM one. This patch fixes that.
>>
>> Signed-off-by: Laurent Vivier 
> 
> I began to wonder about this. Why are we sending the option ROM at all?
> I think there's no need to do it for the primary ...

I think there is no way to check the ROM is the same on source and destination, 
and it has
to be.

By sending the ROM:
- we can check the size is the same, otherwise the migration fails,
- after the migration, even if a different ROM was provided on the destination 
side, the
guest executes the ROM provided by the source.

Thanks,
Laurent

[PATCH] storage-daemon: Add missing build dependency to the vhost-user-blk-test

[Thomas Huth]

vhost-user-blk-test needs the qemu-storage-deamon, otherwise it
currently hangs. So make sure that we build the daemon before running
the tests.

Signed-off-by: Thomas Huth 
---
 storage-daemon/meson.build | 8 
 tests/qtest/meson.build| 7 +--
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/storage-daemon/meson.build b/storage-daemon/meson.build
index 68852f3d25..49c9d2eac9 100644
--- a/storage-daemon/meson.build
+++ b/storage-daemon/meson.build
@@ -6,8 +6,8 @@ subdir('qapi')
 
 if have_tools
   qsd_ss = qsd_ss.apply(config_host, strict: false)
-  executable('qemu-storage-daemon',
- qsd_ss.sources(),
- dependencies: qsd_ss.dependencies(),
- install: true)
+  qsd = executable('qemu-storage-daemon',
+   qsd_ss.sources(),
+   dependencies: qsd_ss.dependencies(),
+   install: true)
 endif
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e22a0792c5..2bc3efd49f 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -276,8 +276,11 @@ foreach dir : target_dirs
   endif
   qtest_env.set('G_TEST_DBUS_DAEMON', meson.source_root() / 
'tests/dbus-vmstate-daemon.sh')
   qtest_env.set('QTEST_QEMU_BINARY', './qemu-system-' + target_base)
-  qtest_env.set('QTEST_QEMU_STORAGE_DAEMON_BINARY', 
'./storage-daemon/qemu-storage-daemon')
-  
+  if have_tools and have_vhost_user_blk_server
+qtest_env.set('QTEST_QEMU_STORAGE_DAEMON_BINARY', 
'./storage-daemon/qemu-storage-daemon')
+test_deps += [qsd]
+  endif
+
   foreach test : target_qtests
 # Executables are shared across targets, declare them only the first time 
we
 # encounter them
-- 
2.27.0

[PATCH 1/2] hw/char/ibex_uart: set user-creatable

[Damien Hedde]

The ibex_uart meets the criteria to be user_creatable: all
parameters are set from properties.
Note that this patch, alone, does not allow to create an instance
with -device cli option or device_add qmp command. Since it is
a sysbus device, additional authorization must be done by the
machine.

Signed-off-by: Damien Hedde 
---
 hw/char/ibex_uart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
index 9b0a817713..b1646422c0 100644
--- a/hw/char/ibex_uart.c
+++ b/hw/char/ibex_uart.c
@@ -546,6 +546,7 @@ static void ibex_uart_class_init(ObjectClass *klass, void 
*data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 
+dc->user_creatable = true;
 dc->reset = ibex_uart_reset;
 dc->realize = ibex_uart_realize;
 dc->vmsd = &vmstate_ibex_uart;
-- 
2.32.0

[PATCH 0/2] Set user creatable for flag ibex uart and plic

[Damien Hedde]

Hi,

This small series only consist in setting the user_creatable flag
of ibex_uart and ibex_plic devices. These two devices are already
using properties to configure themselves so nothing else is required.

Note that this change alone will not allow creation of these devices
using -device cli option or device_add qmp command as they are sysbus
devices.

We do that because we are currently working on adding the possibily
to configure/build a machine from qmp commands (see this rfc:
https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg03706.html). 
We are using these simple devices in order to test our additions.

We prefer to send these 2 patches on a separate series as they are not
really related to the main topic. We will send a following series
for the additions.

Thanks,
Damien

Damien Hedde (2):
  hw/char/ibex_uart: set user-creatable
  hw/char/ibex_plic: set user-creatable

 hw/char/ibex_uart.c | 1 +
 hw/intc/ibex_plic.c | 1 +
 2 files changed, 2 insertions(+)

-- 
2.32.0

[PATCH 2/2] hw/char/ibex_plic: set user-creatable

[Damien Hedde]

The ibex_plic meets the criteria to be user_creatable: all
parameters are set from properties.
Note that this patch, alone, does not allow to create an instance
with -device cli option or device_add qmp command. Since it is
a sysbus device, additional authorization must be done by the
machine.

Signed-off-by: Damien Hedde 
---
 hw/intc/ibex_plic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/intc/ibex_plic.c b/hw/intc/ibex_plic.c
index edf76e4f61..8abd5ee613 100644
--- a/hw/intc/ibex_plic.c
+++ b/hw/intc/ibex_plic.c
@@ -291,6 +291,7 @@ static void ibex_plic_class_init(ObjectClass *klass, void 
*data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 
+dc->user_creatable = true;
 dc->reset = ibex_plic_reset;
 device_class_set_props(dc, ibex_plic_properties);
 dc->realize = ibex_plic_realize;
-- 
2.32.0

Re: [PATCH] target/riscv: Don't wrongly overide isa version

[Bin Meng]

On Tue, Aug 10, 2021 at 11:35 AM LIU Zhiwei  wrote:
>
> For some cpu, the isa version has already been set in cpu init function.
> Thus only overide the isa version when isa version is not set, or

typo: override, please fix the commit title as well

> users set different isa version explicitly by cpu parameters.
>
> Signed-off-by: LIU Zhiwei 
> ---
>  target/riscv/cpu.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 991a6bb760..425efba0c8 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -392,9 +392,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  RISCVCPU *cpu = RISCV_CPU(dev);
>  CPURISCVState *env = &cpu->env;
>  RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> -int priv_version = PRIV_VERSION_1_11_0;
> -int bext_version = BEXT_VERSION_0_93_0;
> -int vext_version = VEXT_VERSION_0_07_1;
> +int priv_version = env->priv_ver;
>  target_ulong target_misa = env->misa;
>  Error *local_err = NULL;
>
> @@ -417,9 +415,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  }
>  }
>
> -set_priv_version(env, priv_version);
> -set_bext_version(env, bext_version);
> -set_vext_version(env, vext_version);
> +if (!env->priv_ver) {
> +set_priv_version(env, PRIV_VERSION_1_11_0);
> +} else if (env->priv_ver != priv_version) {
> +set_priv_version(env, priv_version);
> +}

This logic seems incorrect to me. So if cpu init function does not set
the priv, and cfg set it to v1.10, v1.11 will be set in the new logic.

The previous logic makes sure the cfg value overrides the cpu init
value which seems to be intended.

>
>  if (cpu->cfg.mmu) {
>  set_feature(env, RISCV_FEATURE_MMU);
> @@ -497,6 +497,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  target_misa |= RVH;
>  }
>  if (cpu->cfg.ext_b) {
> +int bext_version = BEXT_VERSION_0_93_0;
>  target_misa |= RVB;
>
>  if (cpu->cfg.bext_spec) {
> @@ -515,6 +516,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  set_bext_version(env, bext_version);
>  }
>  if (cpu->cfg.ext_v) {
> +int vext_version = VEXT_VERSION_0_07_1;
>  target_misa |= RVV;
>  if (!is_power_of_2(cpu->cfg.vlen)) {
>  error_setg(errp,
> --

Regards,
Bin

Re: [RFC PATCH v0 1/2] spapr: Add H_REG_SNS hcall

[Bharata B Rao]

On Mon, Aug 09, 2021 at 01:49:54PM +1000, David Gibson wrote:
> On Thu, Aug 05, 2021 at 01:02:27PM +0530, Bharata B Rao wrote:
> > Add support for H_REG_SNS hcall so that asynchronous page
> > fault mechanism can be supported on PowerKVM guests.
> > 
> > This hcall essentially issues KVM_PPC_SET_SNS to let the
> > host map and pin the memory containing the Subvention
> > Notification Structure. It also claims SPAPR_IRQ_SNS to
> > be used as subvention notification interrupt.
> > 
> > Note: Updates to linux-headers/linux/kvm.h are temporary
> > pending headers update.
> > 
> > Signed-off-by: Bharata B Rao 
> > ---
> >  hw/ppc/spapr.c  |  3 ++
> >  hw/ppc/spapr_hcall.c| 56 +
> >  include/hw/ppc/spapr.h  |  3 ++
> >  include/hw/ppc/spapr_irq.h  |  1 +
> >  linux-headers/asm-powerpc/kvm.h |  6 
> >  linux-headers/linux/kvm.h   |  1 +
> >  target/ppc/kvm.c| 14 +
> >  target/ppc/kvm_ppc.h| 10 ++
> >  8 files changed, 94 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 81699d4f8b..5f1f75826d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2830,6 +2830,9 @@ static void spapr_machine_init(MachineState *machine)
> >  
> >  /* Enable H_PAGE_INIT */
> >  kvmppc_enable_h_page_init();
> > +
> > +/* Enable H_REG_SNS */
> > +kvmppc_enable_h_reg_sns();
> >  }
> >  
> >  /* map RAM */
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 0e9a5b2e40..957edecb13 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1405,6 +1405,59 @@ static target_ulong h_update_dt(PowerPCCPU *cpu, 
> > SpaprMachineState *spapr,
> >  return H_SUCCESS;
> >  }
> >  
> > +static target_ulong deregister_sns(PowerPCCPU *cpu, SpaprMachineState 
> > *spapr)
> > +{
> > +spapr->sns_addr = -1;
> > +spapr->sns_len = 0;
> > +spapr_irq_free(spapr, SPAPR_IRQ_SNS, 1);
> > +
> > +return H_SUCCESS;
> > +}
> > +
> > +static target_ulong h_reg_sns(PowerPCCPU *cpu, SpaprMachineState *spapr,
> > +  target_ulong opcode, target_ulong *args)
> > +{
> > +target_ulong addr = args[0];
> > +target_ulong len = args[1];
> > +
> > +if (addr == -1) {
> > +return deregister_sns(cpu, spapr);
> > +}
> > +
> > +/*
> > + * If SNS area is already registered, can't register again before
> > + * deregistering it first.
> > + */
> > +if (spapr->sns_addr == -1) {
> 
> As Fabiano says, it looks like the logic is reversed here.

Correct.

> 
> > +return H_PARAMETER;
> 
> Also, H_PARAMETER doesn't seem like the right error for this case.
> H_BUSY, maybe?

Yes we may return H_BUSY.

> 
> > +}
> > +
> > +if (!QEMU_IS_ALIGNED(addr, 4096)) {
> 
> What's the significance of 4096 here?  Should this be one of the page
> size defines instead?

PAPR specifies this alignment.
"If the Address parameter is not 4K aligned in the valid logical address
space of the caller, then return H_Parameter."

> 
> > +return H_PARAMETER;
> > +}
> > +
> > +if (len < 256) {
> 
> A defined constant (SPAPR_MIN_SNS_SIZE?) would be worthwhile here, I think.

Yes.

> 
> > +return H_P2;
> > +}
> > +
> > +/* TODO: SNS area is not allowed to cross a page boundary */
> > +
> > +/* KVM_PPC_SET_SNS ioctl */
> > +if (kvmppc_set_sns_reg(addr, len)) {
> 
> What will happen if you attempt this on a TCG system?

We should have a variant of kvmppc_set_sns_reg() for TCG that
returns error, so that this hcall can fail.

> 
> > +return H_PARAMETER;
> > +}
> > +
> > +/* Record SNS addr and len */
> > +spapr->sns_addr = addr;
> > +spapr->sns_len = len;
> > +
> > +/* Register irq source for sending ESN notification */
> > +spapr_irq_claim(spapr, SPAPR_IRQ_SNS, false, &error_fatal);
> 
> I don't think &error_fatal can be right here.  AFAICT this must be one
> of two cases:
>1) This should never fail, no matter what the guest does.  If it
>   does fail, that indicates a qemu bug.  In that case &error_abort is
>   more appropriate
>2) This could fail for certain sequences of guest actions.  If
>   that's the case, we shouldn't kill qemu, but should return
>   H_HARDWARE (or something) instead.

I think we could just check for error and return failure so that
the guest wouldn't go ahead and enable async-pf feature.

> 
> > +args[1] = SPAPR_IRQ_SNS; /* irq no in R5 */
> > +
> > +return H_SUCCESS;
> > +}
> > +
> >  static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
> >  static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - 
> > KVMPPC_HCALL_BASE + 1];
> >  static spapr_hcall_fn svm_hypercall_table[(SVM_HCALL_MAX - SVM_HCALL_BASE) 
> > / 4 + 1];
> > @@ -1545,6 +1598,9 @@ static void hypercall_register_types(void)
> >  spapr_register_hypercall(KVMPPC_H_CAS, h_c

Re: [PATCH] tests/acceptance: Test powernv machines

[Thomas Huth]

On 10/08/2021 11.09, Cédric Le Goater wrote:

On 8/10/21 10:36 AM, Joel Stanley wrote:

On Tue, 10 Aug 2021 at 08:34, Cédric Le Goater  wrote:


Fetch the OpenPOWER images to boot the powernv8 and powernv9 machines
with a simple PCI layout.

Cc: Cleber Rosa 
Cc: Wainer dos Santos Moschetta 
Signed-off-by: Cédric Le Goater 
---
  tests/acceptance/boot_linux_console.py | 42 ++
  1 file changed, 42 insertions(+)

diff --git a/tests/acceptance/boot_linux_console.py 
b/tests/acceptance/boot_linux_console.py
index 5248c8097df9..da93a475ca87 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -1176,6 +1176,48 @@ def test_ppc64_e500(self):
  tar_hash = '6951d86d644b302898da2fd701739c9406527fe1'
  self.do_test_advcal_2018('19', tar_hash, 'uImage')

+def do_test_ppc64_powernv(self, proc):
+
+images_url = 
('https://github.com/open-power/op-build/releases/download/v2.7/')
+
+skiboot_url = images_url + 'skiboot.lid'
+skiboot_hash = 
'a9ffcddbf238f86cda4b2cae2882d6bd13cff8489109758a4980efaf154f4a29'
+skiboot_path = self.fetch_asset(skiboot_url, asset_hash=skiboot_hash,
+   algorithm='sha256')


What's the thought that led you to using this instead of the one that
gets packaged with qemu?


Good question.

I considered that the skiboot.lid shipped with QEMU was somewhat a default
to make things work. The official released versions are the ones used by
the outside world on real systems and were a better target for tests.

That said, using the default version might be enough. Maintainers, please
advise !


IMHO:

- We want to test the things that *we* ship

- We want to download as few things as possible, since downloads
  often slow down the tests and break CI runs if the network to
  the download site is not available

 ==> I'd prefer to use the internal skiboot.lid unless there is
 really a compelling reason to use the external one.

Just my 0.02 € though.

 Thomas

[PATCH v2 09/13] hw/s390x/s390-skeys: use memory mapping to detect which storage keys to migrate

[David Hildenbrand]

Let's use the guest_phys_blocks API to get physical memory regions
that are well defined inside our physical address space and migrate the
storage keys of these.

This is a preparation for having memory besides initial ram defined in
the guest physical address space, for example, via memory devices. We
get rid of the ms->ram_size dependency.

Please note that we will usually have very little (--> 1) physical
ranges. With virtio-mem might have significantly more ranges in the
future. If that turns out to be a problem (e.g., total memory
footprint of the list), we could look into a memory mapping
API that avoids creation of a list and instead triggers a callback for
each range.

Reviewed-by: Thomas Huth 
Signed-off-by: David Hildenbrand 
---
 hw/s390x/s390-skeys.c | 70 ++-
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 9a8d60d1d9..250685a95a 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -17,6 +17,7 @@
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu/error-report.h"
+#include "sysemu/memory_mapping.h"
 #include "sysemu/kvm.h"
 #include "migration/qemu-file-types.h"
 #include "migration/register.h"
@@ -257,10 +258,9 @@ static void s390_storage_keys_save(QEMUFile *f, void 
*opaque)
 {
 S390SKeysState *ss = S390_SKEYS(opaque);
 S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
-MachineState *ms = MACHINE(qdev_get_machine());
-uint64_t pages_left = ms->ram_size / TARGET_PAGE_SIZE;
-uint64_t read_count, eos = S390_SKEYS_SAVE_FLAG_EOS;
-vaddr cur_gfn = 0;
+GuestPhysBlockList guest_phys_blocks;
+GuestPhysBlock *block;
+uint64_t pages, gfn;
 int error = 0;
 uint8_t *buf;
 
@@ -274,36 +274,52 @@ static void s390_storage_keys_save(QEMUFile *f, void 
*opaque)
 goto end_stream;
 }
 
-/* We only support initial memory. Standby memory is not handled yet. */
-qemu_put_be64(f, (cur_gfn * TARGET_PAGE_SIZE) | 
S390_SKEYS_SAVE_FLAG_SKEYS);
-qemu_put_be64(f, pages_left);
-
-while (pages_left) {
-read_count = MIN(pages_left, S390_SKEYS_BUFFER_SIZE);
-
-if (!error) {
-error = skeyclass->get_skeys(ss, cur_gfn, read_count, buf);
-if (error) {
-/*
- * If error: we want to fill the stream with valid data instead
- * of stopping early so we pad the stream with 0x00 values and
- * use S390_SKEYS_SAVE_FLAG_ERROR to indicate failure to the
- * reading side.
- */
-error_report("S390_GET_KEYS error %d", error);
-memset(buf, 0, S390_SKEYS_BUFFER_SIZE);
-eos = S390_SKEYS_SAVE_FLAG_ERROR;
+guest_phys_blocks_init(&guest_phys_blocks);
+guest_phys_blocks_append(&guest_phys_blocks);
+
+/* Send each contiguous physical memory range separately. */
+QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
+assert(QEMU_IS_ALIGNED(block->target_start, TARGET_PAGE_SIZE));
+assert(QEMU_IS_ALIGNED(block->target_end, TARGET_PAGE_SIZE));
+
+gfn = block->target_start / TARGET_PAGE_SIZE;
+pages = (block->target_end - block->target_start) / TARGET_PAGE_SIZE;
+qemu_put_be64(f, block->target_start | S390_SKEYS_SAVE_FLAG_SKEYS);
+qemu_put_be64(f, pages);
+
+while (pages) {
+const uint64_t cur_pages = MIN(pages, S390_SKEYS_BUFFER_SIZE);
+
+if (!error) {
+error = skeyclass->get_skeys(ss, gfn, cur_pages, buf);
+if (error) {
+/*
+ * Create a valid stream with all 0x00 and indicate
+ * S390_SKEYS_SAVE_FLAG_ERROR to the destination.
+ */
+error_report("S390_GET_KEYS error %d", error);
+memset(buf, 0, S390_SKEYS_BUFFER_SIZE);
+}
 }
+
+qemu_put_buffer(f, buf, cur_pages);
+gfn += cur_pages;
+pages -= cur_pages;
 }
 
-qemu_put_buffer(f, buf, read_count);
-cur_gfn += read_count;
-pages_left -= read_count;
+if (error) {
+break;
+}
 }
 
+guest_phys_blocks_free(&guest_phys_blocks);
 g_free(buf);
 end_stream:
-qemu_put_be64(f, eos);
+if (error) {
+qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_ERROR);
+} else {
+qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);
+}
 }
 
 static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
-- 
2.31.1

[PATCH v2 06/13] s390x/mmu_helper: fixup mmu_translate() documentation

[David Hildenbrand]

Looks like we forgot to adjust documentation of one parameter.

Reviewed-by: Thomas Huth 
Signed-off-by: David Hildenbrand 
---
 target/s390x/mmu_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 167f1b1455..ca25dadb5b 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -374,7 +374,8 @@ static void mmu_handle_skey(target_ulong addr, int rw, int 
*flags)
  * @param ascaddress space control (one of the PSW_ASC_* modes)
  * @param raddr  the translated address is stored to this pointer
  * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this pointer
- * @param exctrue = inject a program check if a fault occurred
+ * @param tecthe translation exception code if stored to this pointer if
+ *   there is an exception to raise
  * @return   0 = success, != 0, the exception to raise
  */
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
-- 
2.31.1

[PATCH v2 05/13] s390x/mmu_helper: no need to pass access type to mmu_translate_asce()

[David Hildenbrand]

The access type is unused since commit 81d7e3bc45 ("s390x/mmu: Inject
DAT exceptions from a single place").

Reviewed-by: Thomas Huth 
Signed-off-by: David Hildenbrand 
---
 target/s390x/mmu_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 0620b1803e..167f1b1455 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -125,7 +125,7 @@ static inline bool read_table_entry(CPUS390XState *env, 
hwaddr gaddr,
 
 static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
   uint64_t asc, uint64_t asce, target_ulong *raddr,
-  int *flags, int rw)
+  int *flags)
 {
 const bool edat1 = (env->cregs[0] & CR0_EDAT) &&
s390_has_feat(S390_FEAT_EDAT);
@@ -428,7 +428,7 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, 
int rw, uint64_t asc,
 }
 
 /* perform the DAT translation */
-r = mmu_translate_asce(env, vaddr, asc, asce, raddr, flags, rw);
+r = mmu_translate_asce(env, vaddr, asc, asce, raddr, flags);
 if (unlikely(r)) {
 return r;
 }
-- 
2.31.1

[PATCH v2 13/13] hw/s390x/s390-skeys: lazy storage key enablement under TCG

[David Hildenbrand]

Let's enable storage keys lazily under TCG, just as we do under KVM.
Only fairly old Linux versions actually make use of storage keys, so it
can be kind of wasteful to allocate quite some memory and track
changes and references if nobody cares.

We have to make sure to flush the TLB when enabling storage keys after
the VM was already running: otherwise it might happen that we don't
catch references or modifications afterwards.

Add proper documentation to all callbacks.

The kvm-unit-tests skey tests keeps on working with this change.

Signed-off-by: David Hildenbrand 
---
 hw/s390x/s390-skeys.c   | 65 ++---
 include/hw/s390x/storage-keys.h | 63 
 target/s390x/mmu_helper.c   |  8 
 target/s390x/tcg/mem_helper.c   |  9 +
 4 files changed, 131 insertions(+), 14 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 9e994a5582..5024faf411 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -191,18 +191,45 @@ out:
 fclose(f);
 }
 
-static void qemu_s390_skeys_init(Object *obj)
+static bool qemu_s390_skeys_are_enabled(S390SKeysState *ss)
 {
-QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
-MachineState *machine = MACHINE(qdev_get_machine());
+QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
 
-skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
-skeys->keydata = g_malloc0(skeys->key_count);
+/* Lockless check is sufficient. */
+return !!skeys->keydata;
 }
 
-static bool qemu_s390_skeys_are_enabled(S390SKeysState *ss)
+static bool qemu_s390_enable_skeys(S390SKeysState *ss)
 {
-return true;
+QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
+static gsize initialized;
+
+if (likely(skeys->keydata)) {
+return true;
+}
+
+/*
+ * TODO: Modern Linux doesn't use storage keys unless running KVM guests
+ *   that use storage keys. Therefore, we keep it simple for now.
+ *
+ * 1) We should initialize to "referenced+changed" for an initial
+ *over-indication. Let's avoid touching megabytes of data for now and
+ *assume that any sane user will issue a storage key instruction before
+ *actually relying on this data.
+ * 2) Relying on ram_size and allocating a big array is ugly. We should
+ *allocate and manage storage key data per RAMBlock or optimally using
+ *some sparse data structure.
+ * 3) We only ever have a single S390SKeysState, so relying on
+ *g_once_init_enter() is good enough.
+ */
+if (g_once_init_enter(&initialized)) {
+MachineState *machine = MACHINE(qdev_get_machine());
+
+skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
+skeys->keydata = g_malloc0(skeys->key_count);
+g_once_init_leave(&initialized, 1);
+}
+return false;
 }
 
 static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
@@ -212,9 +239,10 @@ static int qemu_s390_skeys_set(S390SKeysState *ss, 
uint64_t start_gfn,
 int i;
 
 /* Check for uint64 overflow and access beyond end of key data */
-if (start_gfn + count > skeydev->key_count || start_gfn + count < count) {
-error_report("Error: Setting storage keys for page beyond the end "
- "of memory: gfn=%" PRIx64 " count=%" PRId64,
+if (unlikely(!skeydev->keydata || start_gfn + count > skeydev->key_count ||
+  start_gfn + count < count)) {
+error_report("Error: Setting storage keys for pages with unallocated "
+ "storage key memory: gfn=%" PRIx64 " count=%" PRId64,
  start_gfn, count);
 return -EINVAL;
 }
@@ -232,9 +260,10 @@ static int qemu_s390_skeys_get(S390SKeysState *ss, 
uint64_t start_gfn,
 int i;
 
 /* Check for uint64 overflow and access beyond end of key data */
-if (start_gfn + count > skeydev->key_count || start_gfn + count < count) {
-error_report("Error: Getting storage keys for page beyond the end "
- "of memory: gfn=%" PRIx64 " count=%" PRId64,
+if (unlikely(!skeydev->keydata || start_gfn + count > skeydev->key_count ||
+  start_gfn + count < count)) {
+error_report("Error: Getting storage keys for pages with unallocated "
+ "storage key memory: gfn=%" PRIx64 " count=%" PRId64,
  start_gfn, count);
 return -EINVAL;
 }
@@ -251,6 +280,7 @@ static void qemu_s390_skeys_class_init(ObjectClass *oc, 
void *data)
 DeviceClass *dc = DEVICE_CLASS(oc);
 
 skeyclass->skeys_are_enabled = qemu_s390_skeys_are_enabled;
+skeyclass->enable_skeys = qemu_s390_enable_skeys;
 skeyclass->get_skeys = qemu_s390_skeys_get;
 skeyclass->set_skeys = qemu_s390_skeys_set;
 
@@ -261,7 +291,6 @@ static void qemu_s390_skeys_class_init(ObjectClass *oc, 
void *data)
 static const TypeInfo qemu_s390_skeys_info = {
 .name  = TYPE

[PATCH v2 12/13] hw/s390x/s390-skeys: rename skeys_enabled to skeys_are_enabled

[David Hildenbrand]

... and make it return a bool instead.

Signed-off-by: David Hildenbrand 
---
 hw/s390x/s390-skeys-kvm.c   |  4 ++--
 hw/s390x/s390-skeys.c   | 12 ++--
 include/hw/s390x/storage-keys.h |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c
index 1c4d805ad8..3ff9d94b80 100644
--- a/hw/s390x/s390-skeys-kvm.c
+++ b/hw/s390x/s390-skeys-kvm.c
@@ -15,7 +15,7 @@
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 
-static int kvm_s390_skeys_enabled(S390SKeysState *ss)
+static bool kvm_s390_skeys_are_enabled(S390SKeysState *ss)
 {
 S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
 uint8_t single_key;
@@ -57,7 +57,7 @@ static void kvm_s390_skeys_class_init(ObjectClass *oc, void 
*data)
 S390SKeysClass *skeyclass = S390_SKEYS_CLASS(oc);
 DeviceClass *dc = DEVICE_CLASS(oc);
 
-skeyclass->skeys_enabled = kvm_s390_skeys_enabled;
+skeyclass->skeys_are_enabled = kvm_s390_skeys_are_enabled;
 skeyclass->get_skeys = kvm_s390_skeys_get;
 skeyclass->set_skeys = kvm_s390_skeys_set;
 
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index db73e9091d..9e994a5582 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -82,7 +82,7 @@ void hmp_info_skeys(Monitor *mon, const QDict *qdict)
 int r;
 
 /* Quick check to see if guest is using storage keys*/
-if (!skeyclass->skeys_enabled(ss)) {
+if (!skeyclass->skeys_are_enabled(ss)) {
 monitor_printf(mon, "Error: This guest is not using storage keys\n");
 return;
 }
@@ -128,7 +128,7 @@ void qmp_dump_skeys(const char *filename, Error **errp)
 FILE *f;
 
 /* Quick check to see if guest is using storage keys*/
-if (!skeyclass->skeys_enabled(ss)) {
+if (!skeyclass->skeys_are_enabled(ss)) {
 error_setg(errp, "This guest is not using storage keys - "
  "nothing to dump");
 return;
@@ -200,9 +200,9 @@ static void qemu_s390_skeys_init(Object *obj)
 skeys->keydata = g_malloc0(skeys->key_count);
 }
 
-static int qemu_s390_skeys_enabled(S390SKeysState *ss)
+static bool qemu_s390_skeys_are_enabled(S390SKeysState *ss)
 {
-return 1;
+return true;
 }
 
 static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
@@ -250,7 +250,7 @@ static void qemu_s390_skeys_class_init(ObjectClass *oc, 
void *data)
 S390SKeysClass *skeyclass = S390_SKEYS_CLASS(oc);
 DeviceClass *dc = DEVICE_CLASS(oc);
 
-skeyclass->skeys_enabled = qemu_s390_skeys_enabled;
+skeyclass->skeys_are_enabled = qemu_s390_skeys_are_enabled;
 skeyclass->get_skeys = qemu_s390_skeys_get;
 skeyclass->set_skeys = qemu_s390_skeys_set;
 
@@ -277,7 +277,7 @@ static void s390_storage_keys_save(QEMUFile *f, void 
*opaque)
 int error = 0;
 uint8_t *buf;
 
-if (!skeyclass->skeys_enabled(ss)) {
+if (!skeyclass->skeys_are_enabled(ss)) {
 goto end_stream;
 }
 
diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
index 2888d42d0b..eb091842c8 100644
--- a/include/hw/s390x/storage-keys.h
+++ b/include/hw/s390x/storage-keys.h
@@ -28,7 +28,7 @@ struct S390SKeysState {
 
 struct S390SKeysClass {
 DeviceClass parent_class;
-int (*skeys_enabled)(S390SKeysState *ks);
+bool (*skeys_are_enabled)(S390SKeysState *ks);
 int (*get_skeys)(S390SKeysState *ks, uint64_t start_gfn, uint64_t count,
  uint8_t *keys);
 int (*set_skeys)(S390SKeysState *ks, uint64_t start_gfn, uint64_t count,
-- 
2.31.1

[PATCH v2 08/13] s390x/mmu_helper: avoid setting the storage key if nothing changed

[David Hildenbrand]

Avoid setting the key if nothing changed.

Reviewed-by: Thomas Huth 
Signed-off-by: David Hildenbrand 
---
 target/s390x/mmu_helper.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index de6df928d2..e2b372efd9 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -301,7 +301,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int 
*flags)
 {
 static S390SKeysClass *skeyclass;
 static S390SKeysState *ss;
-uint8_t key;
+uint8_t key, old_key;
 int rc;
 
 /*
@@ -337,6 +337,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int 
*flags)
 trace_get_skeys_nonzero(rc);
 return;
 }
+old_key = key;
 
 switch (rw) {
 case MMU_DATA_LOAD:
@@ -360,9 +361,11 @@ static void mmu_handle_skey(target_ulong addr, int rw, int 
*flags)
 /* Any store/fetch sets the reference bit */
 key |= SK_R;
 
-rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
-if (rc) {
-trace_set_skeys_nonzero(rc);
+if (key != old_key) {
+rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+if (rc) {
+trace_set_skeys_nonzero(rc);
+}
 }
 }
 
-- 
2.31.1

[PATCH v2 04/13] s390x/tcg: check for addressing exceptions for RRBE, SSKE and ISKE

[David Hildenbrand]

Let's replace the ram_size check by a proper physical address space
check (for example, to prepare for memory hotplug), trigger addressing
exceptions and trace the return value of the storage key getter/setter.

Provide an helper mmu_absolute_addr_valid() to be used in other context
soon. Always test for "read" instead of "write" as we are not actually
modifying the page itself.

Signed-off-by: David Hildenbrand 
---
 target/s390x/helper.h |  6 +++---
 target/s390x/mmu_helper.c |  8 
 target/s390x/s390x-internal.h |  1 +
 target/s390x/tcg/mem_helper.c | 36 ++-
 4 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 6215ca00bc..271b081e8c 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -336,9 +336,9 @@ DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, 
i64, i32)
 DEF_HELPER_FLAGS_4(stctg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_2(testblock, TCG_CALL_NO_WG, i32, env, i64)
 DEF_HELPER_FLAGS_3(tprot, TCG_CALL_NO_WG, i32, env, i64, i64)
-DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64)
-DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64)
-DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64)
+DEF_HELPER_2(iske, i64, env, i64)
+DEF_HELPER_3(sske, void, env, i64, i64)
+DEF_HELPER_2(rrbe, i32, env, i64)
 DEF_HELPER_4(mvcs, i32, env, i64, i64, i64)
 DEF_HELPER_4(mvcp, i32, env, i64, i64, i64)
 DEF_HELPER_4(sigp, i32, env, i64, i32, i32)
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index d779a9fc51..0620b1803e 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -94,6 +94,14 @@ target_ulong mmu_real2abs(CPUS390XState *env, target_ulong 
raddr)
 return raddr;
 }
 
+bool mmu_absolute_addr_valid(target_ulong addr, bool is_write)
+{
+return address_space_access_valid(&address_space_memory,
+  addr & TARGET_PAGE_MASK,
+  TARGET_PAGE_SIZE, is_write,
+  MEMTXATTRS_UNSPECIFIED);
+}
+
 static inline bool read_table_entry(CPUS390XState *env, hwaddr gaddr,
 uint64_t *entry)
 {
diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 5506f185e8..d246d26b04 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -373,6 +373,7 @@ void probe_write_access(CPUS390XState *env, uint64_t addr, 
uint64_t len,
 
 
 /* mmu_helper.c */
+bool mmu_absolute_addr_valid(target_ulong addr, bool is_write);
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
   target_ulong *raddr, int *flags, uint64_t *tec);
 int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index dd506d8d17..90ac82fdcc 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -28,6 +28,7 @@
 #include "qemu/int128.h"
 #include "qemu/atomic128.h"
 #include "tcg/tcg.h"
+#include "trace.h"
 
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/s390x/storage-keys.h"
@@ -2171,15 +2172,15 @@ uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, 
uint64_t a2)
 /* insert storage key extended */
 uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
 {
-MachineState *ms = MACHINE(qdev_get_machine());
 static S390SKeysState *ss;
 static S390SKeysClass *skeyclass;
 uint64_t addr = wrap_address(env, r2);
 uint8_t key;
+int rc;
 
 addr = mmu_real2abs(env, addr);
-if (addr > ms->ram_size) {
-return 0;
+if (!mmu_absolute_addr_valid(addr, false)) {
+trigger_pgm_exception(env, PGM_ADDRESSING);
 }
 
 if (unlikely(!ss)) {
@@ -2187,7 +2188,9 @@ uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
 skeyclass = S390_SKEYS_GET_CLASS(ss);
 }
 
-if (skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
+rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+if (rc) {
+trace_get_skeys_nonzero(rc);
 return 0;
 }
 return key;
@@ -2196,15 +2199,15 @@ uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
 /* set storage key extended */
 void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
 {
-MachineState *ms = MACHINE(qdev_get_machine());
 static S390SKeysState *ss;
 static S390SKeysClass *skeyclass;
 uint64_t addr = wrap_address(env, r2);
 uint8_t key;
+int rc;
 
 addr = mmu_real2abs(env, addr);
-if (addr > ms->ram_size) {
-return;
+if (!mmu_absolute_addr_valid(addr, false)) {
+trigger_pgm_exception(env, PGM_ADDRESSING);
 }
 
 if (unlikely(!ss)) {
@@ -2213,7 +2216,10 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, 
uint64_t r2)
 }
 
 key = r1 & 0xfe;
-skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+ 

[PATCH qemu v2] docs: how to use gdb with unix sockets (v2)

[~archi42]

From: Sebastian Meyer 

This includes the changes suggested by Philippe.
I kept the `-S` in the command line. The user shall
use that instead of `wait=on`.

Signed-off-by: Sebastian Meyer 
---
 docs/system/gdb.rst | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/docs/system/gdb.rst b/docs/system/gdb.rst
index 144d083df3..f7411cb8ed 100644
--- a/docs/system/gdb.rst
+++ b/docs/system/gdb.rst
@@ -15,7 +15,8 @@ The ``-s`` option will make QEMU listen for an incoming 
connection
 from gdb on TCP port 1234, and ``-S`` will make QEMU not start the
 guest until you tell it to from gdb. (If you want to specify which
 TCP port to use or to use something other than TCP for the gdbstub
-connection, use the ``-gdb dev`` option instead of ``-s``.)
+connection, use the ``-gdb dev`` option instead of ``-s``. See
+`Using unix sockets`_ for an example.)
 
 .. parsed-literal::
 
@@ -168,3 +169,24 @@ The memory mode can be checked by sending the following 
command:
 
 ``maintenance packet Qqemu.PhyMemMode:0``
 This will change it back to normal memory mode.
+
+Using unix sockets
+^^
+
+An alternate method for connecting gdb to the QEMU gdbstub are unix
+sockets (if supported by your operating system). This is useful when
+running several tests in parallel and/or you do not have a free TCP
+port a priori (e.g. when running automated tests).
+First create a chardev with the appropriate options, then
+instruct the gdbserver to use that device::
+
+.. parsed-literal::
+
+   |qemu_system| -chardev 
socket,path=/tmp/gdb-socket,server=on,wait=off,id=gdb0 -gdb chardev:gdb0 -S ...
+
+Start gdb as before, but this time connect using the path to
+the socket::
+
+   (gdb) target remote /tmp/gdb-socket
+
+Note gdb version 9.0 or newer is required.
-- 
2.32.0

[PATCH v2 10/13] hw/s390x/s390-skeys: use memory mapping to detect which storage keys to dump

[David Hildenbrand]

Handle it similar to migration. Assert that we're holding the BQL, to
make sure we don't see concurrent modifications.

Reviewed-by: Thomas Huth 
Signed-off-by: David Hildenbrand 
---
 hw/s390x/s390-skeys.c | 50 ++-
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 250685a95a..56a47fe180 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -110,11 +110,10 @@ void qmp_dump_skeys(const char *filename, Error **errp)
 {
 S390SKeysState *ss = s390_get_skeys_device();
 S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
-MachineState *ms = MACHINE(qdev_get_machine());
-const uint64_t total_count = ms->ram_size / TARGET_PAGE_SIZE;
-uint64_t handled_count = 0, cur_count;
+GuestPhysBlockList guest_phys_blocks;
+GuestPhysBlock *block;
+uint64_t pages, gfn;
 Error *lerr = NULL;
-vaddr cur_gfn = 0;
 uint8_t *buf;
 int ret;
 int fd;
@@ -145,28 +144,39 @@ void qmp_dump_skeys(const char *filename, Error **errp)
 goto out;
 }
 
-/* we'll only dump initial memory for now */
-while (handled_count < total_count) {
-/* Calculate how many keys to ask for & handle overflow case */
-cur_count = MIN(total_count - handled_count, S390_SKEYS_BUFFER_SIZE);
+assert(qemu_mutex_iothread_locked());
+guest_phys_blocks_init(&guest_phys_blocks);
+guest_phys_blocks_append(&guest_phys_blocks);
 
-ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf);
-if (ret < 0) {
-error_setg(errp, "get_keys error %d", ret);
-goto out_free;
-}
+QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
+assert(QEMU_IS_ALIGNED(block->target_start, TARGET_PAGE_SIZE));
+assert(QEMU_IS_ALIGNED(block->target_end, TARGET_PAGE_SIZE));
 
-/* write keys to stream */
-write_keys(f, buf, cur_gfn, cur_count, &lerr);
-if (lerr) {
-goto out_free;
-}
+gfn = block->target_start / TARGET_PAGE_SIZE;
+pages = (block->target_end - block->target_start) / TARGET_PAGE_SIZE;
 
-cur_gfn += cur_count;
-handled_count += cur_count;
+while (pages) {
+const uint64_t cur_pages = MIN(pages, S390_SKEYS_BUFFER_SIZE);
+
+ret = skeyclass->get_skeys(ss, gfn, cur_pages, buf);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "get_keys error");
+goto out_free;
+}
+
+/* write keys to stream */
+write_keys(f, buf, gfn, cur_pages, &lerr);
+if (lerr) {
+goto out_free;
+}
+
+gfn += cur_pages;
+pages -= cur_pages;
+}
 }
 
 out_free:
+guest_phys_blocks_free(&guest_phys_blocks);
 error_propagate(errp, lerr);
 g_free(buf);
 out:
-- 
2.31.1

[PATCH v2 03/13] s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE

[David Hildenbrand]

For RRBE, SSKE, and ISKE, we're dealing with real addresses, so we have to
convert to an absolute address first.

In the future, when adding EDAT1 support, we'll have to pay attention to
SSKE handling, as we'll be dealing with absolute addresses when the
multiple-block control is one.

Signed-off-by: David Hildenbrand 
---
 target/s390x/tcg/mem_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 3c0820dd74..dd506d8d17 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2177,6 +2177,7 @@ uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
 uint64_t addr = wrap_address(env, r2);
 uint8_t key;
 
+addr = mmu_real2abs(env, addr);
 if (addr > ms->ram_size) {
 return 0;
 }
@@ -2201,6 +2202,7 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, 
uint64_t r2)
 uint64_t addr = wrap_address(env, r2);
 uint8_t key;
 
+addr = mmu_real2abs(env, addr);
 if (addr > ms->ram_size) {
 return;
 }
@@ -2228,6 +2230,7 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
 static S390SKeysClass *skeyclass;
 uint8_t re, key;
 
+addr = mmu_real2abs(env, addr);
 if (addr > ms->ram_size) {
 return 0;
 }
-- 
2.31.1

[PATCH v2 02/13] s390x/tcg: fix ignoring bit 63 when setting the storage key in SSKE

[David Hildenbrand]

Right now we could set an 8-bit storage key via SSKE and retrieve it
again via ISKE, which is against the architecture description:

SSKE:
"
The new seven-bit storage-key value, or selected bits
thereof, is obtained from bit positions 56-62 of gen-
eral register R 1 . The contents of bit positions 0-55
and 63 of the register are ignored.
"

ISKE:
"
The seven-bit storage key is inserted in bit positions
56-62 of general register R 1 , and bit 63 is set to zero.
"

Let's properly ignore bit 63 to create the correct seven-bit storage key.

Signed-off-by: David Hildenbrand 
---
 target/s390x/tcg/mem_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index e0befd0f03..3c0820dd74 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2210,7 +2210,7 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, 
uint64_t r2)
 skeyclass = S390_SKEYS_GET_CLASS(ss);
 }
 
-key = (uint8_t) r1;
+key = r1 & 0xfe;
 skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
/*
 * As we can only flush by virtual address and not all the entries
-- 
2.31.1

[PATCH v2 11/13] hw/s390x/s390-skeys: check if an address is valid before dumping the key

[David Hildenbrand]

Let's validate the given address and report a proper error in case it's
not. All call paths now properly check the validity of the given GFN.
Remove the TODO.

The errors inside the getter and setter should only trigger if something
really goes wrong now, for example, with a broken migration stream. Or
when we forget to update the storage key allocation with memory hotplug.

Signed-off-by: David Hildenbrand 
---
 hw/s390x/s390-skeys.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 56a47fe180..db73e9091d 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -18,6 +18,7 @@
 #include "qapi/qmp/qdict.h"
 #include "qemu/error-report.h"
 #include "sysemu/memory_mapping.h"
+#include "exec/address-spaces.h"
 #include "sysemu/kvm.h"
 #include "migration/qemu-file-types.h"
 #include "migration/register.h"
@@ -86,6 +87,13 @@ void hmp_info_skeys(Monitor *mon, const QDict *qdict)
 return;
 }
 
+if (!address_space_access_valid(&address_space_memory,
+addr & TARGET_PAGE_MASK, TARGET_PAGE_SIZE,
+false, MEMTXATTRS_UNSPECIFIED)) {
+monitor_printf(mon, "Error: The given address is not valid\n");
+return;
+}
+
 r = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
 if (r < 0) {
 monitor_printf(mon, "Error: %s\n", strerror(-r));
@@ -197,11 +205,6 @@ static int qemu_s390_skeys_enabled(S390SKeysState *ss)
 return 1;
 }
 
-/*
- * TODO: for memory hotplug support qemu_s390_skeys_set and qemu_s390_skeys_get
- * will have to make sure that the given gfn belongs to a memory region and not
- * a memory hole.
- */
 static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
   uint64_t count, uint8_t *keys)
 {
-- 
2.31.1

[PATCH v2 01/13] s390x/tcg: wrap address for RRBE

[David Hildenbrand]

Let's wrap the address just like for SSKE and ISKE.

Reviewed-by: Thomas Huth 
Signed-off-by: David Hildenbrand 
---
 target/s390x/tcg/mem_helper.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 21a4de4067..e0befd0f03 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2223,11 +2223,12 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, 
uint64_t r2)
 uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
 {
 MachineState *ms = MACHINE(qdev_get_machine());
+uint64_t addr = wrap_address(env, r2);
 static S390SKeysState *ss;
 static S390SKeysClass *skeyclass;
 uint8_t re, key;
 
-if (r2 > ms->ram_size) {
+if (addr > ms->ram_size) {
 return 0;
 }
 
@@ -2236,14 +2237,14 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
 skeyclass = S390_SKEYS_GET_CLASS(ss);
 }
 
-if (skeyclass->get_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) {
+if (skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
 return 0;
 }
 
 re = key & (SK_R | SK_C);
 key &= ~SK_R;
 
-if (skeyclass->set_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) {
+if (skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
 return 0;
 }
/*
-- 
2.31.1

[PATCH v2 07/13] s390x/mmu_helper: move address validation into mmu_translate*()

[David Hildenbrand]

Let's move address validation into mmu_translate() and
mmu_translate_real(). This allows for checking whether an absolute
address is valid before looking up the storage key. We can now get rid of
the ram_size check.

Interestingly, we're already handling LOAD REAL ADDRESS wrong, because
a) We're not supposed to touch storage keys
b) We're not supposed to convert to an absolute address

Let's use a fake, negative MMUAccessType to teach mmu_translate() to
fix that handling and to not perform address validation.

Reviewed-by: Thomas Huth 
Signed-off-by: David Hildenbrand 
---
 target/s390x/mmu_helper.c  | 36 --
 target/s390x/s390x-internal.h  |  2 ++
 target/s390x/tcg/excp_helper.c | 13 
 target/s390x/tcg/mem_helper.c  |  2 +-
 4 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index ca25dadb5b..de6df928d2 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -301,14 +301,13 @@ static void mmu_handle_skey(target_ulong addr, int rw, 
int *flags)
 {
 static S390SKeysClass *skeyclass;
 static S390SKeysState *ss;
-MachineState *ms = MACHINE(qdev_get_machine());
 uint8_t key;
 int rc;
 
-if (unlikely(addr >= ms->ram_size)) {
-return;
-}
-
+/*
+ * We expect to be called with an absolute address that has already been
+ * validated, such that we can reliably use it to lookup the storage key.
+ */
 if (unlikely(!ss)) {
 ss = s390_get_skeys_device();
 skeyclass = S390_SKEYS_GET_CLASS(ss);
@@ -370,7 +369,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int 
*flags)
 /**
  * Translate a virtual (logical) address into a physical (absolute) address.
  * @param vaddr  the virtual address
- * @param rw 0 = read, 1 = write, 2 = code fetch
+ * @param rw 0 = read, 1 = write, 2 = code fetch, < 0 = load real address
  * @param ascaddress space control (one of the PSW_ASC_* modes)
  * @param raddr  the translated address is stored to this pointer
  * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this pointer
@@ -449,10 +448,17 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, 
int rw, uint64_t asc,
 }
 
 nodat:
-/* Convert real address -> absolute address */
-*raddr = mmu_real2abs(env, *raddr);
+if (rw >= 0) {
+/* Convert real address -> absolute address */
+*raddr = mmu_real2abs(env, *raddr);
 
-mmu_handle_skey(*raddr, rw, flags);
+if (!mmu_absolute_addr_valid(*raddr, rw == MMU_DATA_STORE)) {
+*tec = 0; /* unused */
+return PGM_ADDRESSING;
+}
+
+mmu_handle_skey(*raddr, rw, flags);
+}
 return 0;
 }
 
@@ -473,12 +479,6 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int 
nr_pages,
 if (ret) {
 return ret;
 }
-if (!address_space_access_valid(&address_space_memory, pages[i],
-TARGET_PAGE_SIZE, is_write,
-MEMTXATTRS_UNSPECIFIED)) {
-*tec = 0; /* unused */
-return PGM_ADDRESSING;
-}
 addr += TARGET_PAGE_SIZE;
 }
 
@@ -588,6 +588,12 @@ int mmu_translate_real(CPUS390XState *env, target_ulong 
raddr, int rw,
 
 *addr = mmu_real2abs(env, raddr & TARGET_PAGE_MASK);
 
+if (!mmu_absolute_addr_valid(*addr, rw == MMU_DATA_STORE)) {
+/* unused */
+*tec = 0;
+return PGM_ADDRESSING;
+}
+
 mmu_handle_skey(*addr, rw, flags);
 return 0;
 }
diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index d246d26b04..7a6aa4dacc 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -374,6 +374,8 @@ void probe_write_access(CPUS390XState *env, uint64_t addr, 
uint64_t len,
 
 /* mmu_helper.c */
 bool mmu_absolute_addr_valid(target_ulong addr, bool is_write);
+/* Special access mode only valid for mmu_translate() */
+#define MMU_S390_LRA-1
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
   target_ulong *raddr, int *flags, uint64_t *tec);
 int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
index a61917d04f..3d6662a53c 100644
--- a/target/s390x/tcg/excp_helper.c
+++ b/target/s390x/tcg/excp_helper.c
@@ -150,19 +150,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
 g_assert_not_reached();
 }
 
-/* check out of RAM access */
-if (!excp &&
-!address_space_access_valid(&address_space_memory, raddr,
-TARGET_PAGE_SIZE, access_type,
-MEMTXATTRS_UNSPECIFIED)) {
-MachineState *ms = MACHINE(qdev_get_machine());
-qemu_log_mask(CPU_LOG_MMU,
-  "%s: raddr %" PRIx64 " > ram_size

[PATCH v2 00/13] s390x: skey related fixes, cleanups, and memory device preparations

[David Hildenbrand]

This series fixes multiple TCG issues related to storage key instructions,
fixes some TCG issues related to LOAD REAL ADDRESS and skeys, implements
lazy skey enablement under TCG, and prepares the whole skey infrastructure
for dealing with addresses beyond initial RAM (e.g., memory devices like
virtio-mem). Along, some cleanups.

To prepare for memory devices / memory hotplug, my goal was to get rid of
as many ms->ram_size users as possible. Unfortunately, I stumbled over
many other things on the way. The remaining s390x users of ms->ram_size
are:

a) hw/s390x/ipl.c: loading the FW. Won't need adjustment.
b) hw/s390x/s390-skeys.c: allocating the array for storage keys. Will need
   adjustment for memory devices.
c) hw/s390x/s390-stattrib-kvm.c: will need adjustments in the future when
   using memory devices with CMM.
d) hw/s390x/s390-virtio-ccw.c: fixing up / handling initital ram. Won't
   need adjustment.
e) hw/s390x/sclp.c: same as d)

Especially patch 9-12 also affect KVM. The remaining ones mostly only
affect TCG.

v1 -> v2:
- "s390x/tcg: fix ignoring bit 63 when setting the storage key in SSKE"
-- Extended description
- "s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE"
-- Extended description
- "s390x/tcg: check for addressing exceptions for RRBE, SSKE and ISKE"
-- Fix compilation issue
- "s390x/mmu_helper: no need to pass access type to mmu_translate_asce()"
-- Mention commit id in description
- "s390x/mmu_helper: move address validation into mmu_translate*()"
-- Introduce and use MMU_S390_LRA
- "hw/s390x/s390-skeys: check if an address is valid before dumping the
   key"
-- Add missing return
- "hw/s390x/s390-skeys: rename skeys_enabled to skeys_are_enabled"
-- Added
- "hw/s390x/s390-skeys: lazy storage key enablement under TCG"
-- Adjust error cases/message in TCG get_skeys() and set_skeys()
-- Use "enable_skeys" and return a bool
-- Adjust function documentation

Cc: Richard Henderson 
Cc: Cornelia Huck 
Cc: Halil Pasic 
Cc: Christian Borntraeger 
Cc: Thomas Huth 
Cc: Janosch Frank 
Cc: Claudio Imbrenda 
Cc: Jason J. Herne 
Cc: qemu-s3...@nongnu.org

David Hildenbrand (13):
  s390x/tcg: wrap address for RRBE
  s390x/tcg: fix ignoring bit 63 when setting the storage key in SSKE
  s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE
  s390x/tcg: check for addressing exceptions for RRBE, SSKE and ISKE
  s390x/mmu_helper: no need to pass access type to mmu_translate_asce()
  s390x/mmu_helper: fixup mmu_translate() documentation
  s390x/mmu_helper: move address validation into mmu_translate*()
  s390x/mmu_helper: avoid setting the storage key if nothing changed
  hw/s390x/s390-skeys: use memory mapping to detect which storage keys
to migrate
  hw/s390x/s390-skeys: use memory mapping to detect which storage keys
to dump
  hw/s390x/s390-skeys: check if an address is valid before dumping the
key
  hw/s390x/s390-skeys: rename skeys_enabled to skeys_are_enabled
  hw/s390x/s390-skeys: lazy storage key enablement under TCG

 hw/s390x/s390-skeys-kvm.c   |   4 +-
 hw/s390x/s390-skeys.c   | 206 +---
 include/hw/s390x/storage-keys.h |  65 +-
 target/s390x/helper.h   |   6 +-
 target/s390x/mmu_helper.c   |  70 +++
 target/s390x/s390x-internal.h   |   3 +
 target/s390x/tcg/excp_helper.c  |  13 --
 target/s390x/tcg/mem_helper.c   |  53 +---
 8 files changed, 294 insertions(+), 126 deletions(-)

-- 
2.31.1

[PATCH 1/1] edid: Added support for 4k@60 Hz monitor

[Singh, Satyeshwar]

Previously, the large modes (>1080p) that were generated by Qemu in its EDID
were all 50 Hz. If we provide them to a Guest OS and the user selects
one of these modes, then the OS by default only gets 50 FPS. This is
especially true for Windows OS. With this patch, we are now exposing a
3840x2160@60 Hz which will allow the guest OS to get 60 FPS.

Cc: Gerd Hoffmann kra...@redhat.com
Signed-off-by: Satyeshwar Singh 
satyeshwar.si...@intel.com
---
hw/display/edid-generate.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/hw/display/edid-generate.c b/hw/display/edid-generate.c
index f2b874d5e3..397af0bd63 100644
--- a/hw/display/edid-generate.c
+++ b/hw/display/edid-generate.c
@@ -24,6 +24,9 @@ static const struct edid_mode {
 { .xres = 2048,   .yres = 1152 },
 { .xres = 1920,   .yres = 1080,   .dta =  31 },
+/* dea/dta extension timings (all @ 60 Hz) */
+{ .xres = 3840,   .yres = 2160,   .dta =  97 },
+
 /* additional standard timings 3 (all @ 60Hz) */
 { .xres = 1920,   .yres = 1200,   .xtra3 = 10,   .bit = 0 },
 { .xres = 1600,   .yres = 1200,   .xtra3 =  9,   .bit = 2 },
--
2.21.3

[PATCH 2/2] hw/i386: Rename default_bus_bypass_iommu

[Jean-Philippe Brucker]

Since commit d8fb7d0969d5 ("vl: switch -M parsing to keyval"), machine
parameter definitions cannot use underscores, because keyval_dashify()
transforms them to dashes and the parser doesn't find the parameter.

This affects option default_bus_bypass_iommu which was introduced in the
same release:

$ qemu-system-x86_64 -M q35,default_bus_bypass_iommu=on
qemu-system-x86_64: Property 'pc-q35-6.1-machine.default-bus-bypass-iommu' not 
found

Rename the parameter to "default-bus-bypass-iommu". Passing
"default_bus_bypass_iommu" is still valid since the underscore are
transformed automatically.

Fixes: c9e96b04fc19 ("hw/i386: Add a default_bus_bypass_iommu pc machine 
option")
Signed-off-by: Jean-Philippe Brucker 
---
 hw/i386/pc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fb24f000e7..ce4756ad59 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1779,7 +1779,7 @@ static void pc_machine_class_init(ObjectClass *oc, void 
*data)
 object_class_property_add_bool(oc, "hpet",
 pc_machine_get_hpet, pc_machine_set_hpet);
 
-object_class_property_add_bool(oc, "default_bus_bypass_iommu",
+object_class_property_add_bool(oc, "default-bus-bypass-iommu",
 pc_machine_get_default_bus_bypass_iommu,
 pc_machine_set_default_bus_bypass_iommu);
 
-- 
2.32.0

[PATCH 1/2] hw/arm/virt: Rename default_bus_bypass_iommu

[Jean-Philippe Brucker]

Since commit d8fb7d0969d5 ("vl: switch -M parsing to keyval"), machine
parameter definitions cannot use underscores, because keyval_dashify()
transforms them to dashes and the parser doesn't find the parameter.

This affects option default_bus_bypass_iommu which was introduced in the
same release:

$ qemu-system-aarch64 -M virt,default_bus_bypass_iommu=on
qemu-system-aarch64: Property 'virt-6.1-machine.default-bus-bypass-iommu' not 
found

Rename the parameter to "default-bus-bypass-iommu". Passing
"default_bus_bypass_iommu" is still valid since the underscore are
transformed automatically.

Fixes: 6d7a85483a06 ("hw/arm/virt: Add default_bus_bypass_iommu machine option")
Signed-off-by: Jean-Philippe Brucker 
---
 hw/arm/virt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b4598d3fe6..7075cdc15e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2671,10 +2671,10 @@ static void virt_machine_class_init(ObjectClass *oc, 
void *data)
   "Set the IOMMU type. "
   "Valid values are none and smmuv3");
 
-object_class_property_add_bool(oc, "default_bus_bypass_iommu",
+object_class_property_add_bool(oc, "default-bus-bypass-iommu",
virt_get_default_bus_bypass_iommu,
virt_set_default_bus_bypass_iommu);
-object_class_property_set_description(oc, "default_bus_bypass_iommu",
+object_class_property_set_description(oc, "default-bus-bypass-iommu",
   "Set on/off to enable/disable "
   "bypass_iommu for default root bus");
 
-- 
2.32.0

Re: [PATCH 2/2] gitlab: skip many more targets in windows cross builds

[Daniel P . Berrangé]

On Wed, Aug 11, 2021 at 08:20:37AM +0200, Philippe Mathieu-Daudé wrote:
> On 8/10/21 4:06 PM, Daniel P. Berrangé wrote:
> > The windows cross builds still take way too long in gitlab CI, so need
> > more targets to be skipped. We don't want to hurt coverage of other
> > cross builds more though, so we let jobs fine tune with a new env
> > variale $CROSS_SKIP_TARGETS.
> > 
> > We take the set of targets that are considered relatively niche or
> > very old architectures, and skip approx half of them in win32 builds
> > and the other half of them in win64.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  .gitlab-ci.d/crossbuild-template.yml | 2 +-
> >  .gitlab-ci.d/crossbuilds.yml | 2 ++
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/.gitlab-ci.d/crossbuild-template.yml 
> > b/.gitlab-ci.d/crossbuild-template.yml
> > index cfb576b54c..10d22dcf6c 100644
> > --- a/.gitlab-ci.d/crossbuild-template.yml
> > +++ b/.gitlab-ci.d/crossbuild-template.yml
> > @@ -10,7 +10,7 @@
> >  --disable-user --target-list-exclude="arm-softmmu cris-softmmu
> >i386-softmmu microblaze-softmmu mips-softmmu mipsel-softmmu
> >mips64-softmmu ppc-softmmu riscv32-softmmu sh4-softmmu
> > -  sparc-softmmu xtensa-softmmu"
> > +  sparc-softmmu xtensa-softmmu $CROSS_SKIP_TARGETS"
> >  - make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS
> >  - if grep -q "EXESUF=.exe" config-host.mak;
> >then make installer;
> > diff --git a/.gitlab-ci.d/crossbuilds.yml b/.gitlab-ci.d/crossbuilds.yml
> > index 4ff3aa3cfc..a5f9dbcbeb 100644
> > --- a/.gitlab-ci.d/crossbuilds.yml
> > +++ b/.gitlab-ci.d/crossbuilds.yml
> > @@ -160,6 +160,7 @@ cross-win32-system:
> >  job: win32-fedora-cross-container
> >variables:
> >  IMAGE: fedora-win32-cross
> > +CROSS_SKIP_TARGETS: or1k-softmmu rx-softmmu sh4eb-softmmu 
> > sparc64-softmmu tricore-softmmu xtensaeb-softmmu
> >artifacts:
> >  paths:
> >- build/qemu-setup*.exe
> > @@ -170,6 +171,7 @@ cross-win64-system:
> >  job: win64-fedora-cross-container
> >variables:
> >  IMAGE: fedora-win64-cross
> > +CROSS_SKIP_TARGETS: alpha-softmmu avr-softmmu hppa-softmmu 
> > m68k-softmmu microblazeel-softmmu mips64el-softmmu nios2-softmmu
> 
> It looks you inverted the lists. We expect more Win64 users, and these
> are the targets of interest. I'd keep here (skipping in Win32):

I wouldn't say inverted, because the split was entirely arbitrary. While
32-bit vs 64-bit could conceivably show a difference, in practice those
differences are likely to already be caught by non-windows builds. These
jobs are most important for catching UNIX vs Windows differences, so in
that view whether a target is built for win32 vs win64 doesn't matter.

> 
> alpha-softmmu avr-softmmu hppa-softmmu m68k-softmmu microblazeel-softmmu
> mips64el-softmmu sparc64-softmmu
> 
> And skip (keep them in win32):
> 
> or1k-softmmu rx-softmmu sh4eb-softmmu nios2-softmmu tricore-softmmu
> xtensaeb-softmmu

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

[PATCH v2 4/6] docs/about/removed-features: Document removed HMP commands from QEMU v2.12

[Thomas Huth]

These HMP commands had been removed/replaced in QEMU v2.12. Still, some
people might want to update from older versions to the recent QEMU version,
so we should give some recommendations for the replacements in our
documentation.

Signed-off-by: Thomas Huth 
---
 docs/about/removed-features.rst | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 40d2cc4ffa..8bf3ebecab 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -356,6 +356,17 @@ Specify the properties for the object as top-level 
arguments instead.
 Human Monitor Protocol (HMP) commands
 -
 
+``usb_add`` and ``usb_remove`` (removed in 2.12)
+
+
+Replaced by ``device_add`` and ``device_del`` (use ``device_add help`` for a
+list of available devices).
+
+``host_net_add`` and ``host_net_remove`` (removed in 2.12)
+''
+
+Replaced by ``netdev_add`` and ``netdev_del``.
+
 The ``hub_id`` parameter of ``hostfwd_add`` / ``hostfwd_remove`` (removed in 
5.0)
 
'
 
-- 
2.27.0

Re: [PATCH v1 12/12] hw/s390x/s390-skeys: lazy storage key enablement under TCG

[David Hildenbrand]

Shouldn't there be some modifications to qemu_s390_skeys_get() in that case?
Or does "fail" mean that it crashes?


qemu_s390_skeys_get() should simply return -EINVAL because
skeydev->key_count == 0. So don't think we need any modifications.

Makes sense?


Ah, right, make sense, indeed.



But I'll still make the clearer and also fixup the error messages that 
are getting printed. Thanks!



   Thomas




--
Thanks,

David / dhildenb

[PATCH v2 5/6] docs/about/removed-features: Document removed devices from older QEMU versions

[Thomas Huth]

These devices had been removed/replaced in QEMU v2.12 and v4.0.

Signed-off-by: Thomas Huth 
---
 docs/about/removed-features.rst | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 8bf3ebecab..0c860be62d 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -513,6 +513,17 @@ running the old binaries, you can use older versions of 
QEMU.
 System emulator devices
 ---
 
+``spapr-pci-vfio-host-bridge`` (removed in 2.12)
+'
+
+The ``spapr-pci-vfio-host-bridge`` device type has been replaced by the
+``spapr-pci-host-bridge`` device type.
+
+``ivshmem`` (removed in 4.0)
+
+
+Replaced by either the ``ivshmem-plain`` or ``ivshmem-doorbell``.
+
 ``ide-drive`` (removed in 6.0)
 ''
 
-- 
2.27.0

[PATCH v2 6/6] docs/about/removed-features: Document removed machines from older QEMU versions

[Thomas Huth]

These machines had been removed in the QEMU v2.6 up to 4.0 time frame.

Signed-off-by: Thomas Huth 
---
 docs/about/removed-features.rst | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 0c860be62d..cbfa1a8e31 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -473,6 +473,22 @@ Removed without replacement.
 System emulator machines
 
 
+``s390-virtio`` (removed in 2.6)
+
+
+Use the ``s390-ccw-virtio`` machine instead.
+
+The m68k ``dummy`` machine (removed in 2.9)
+'''
+
+Use the ``none`` machine with the ``loader`` device instead.
+
+``xlnx-ep108`` (removed in 3.0)
+'''
+
+The EP108 was an early access development board that is no longer used.
+Use the ``xlnx-zcu102`` machine instead.
+
 ``spike_v1.9.1`` and ``spike_v1.10`` (removed in 5.1)
 '
 
@@ -491,8 +507,8 @@ mips ``fulong2e`` machine alias (removed in 6.0)
 
 This machine has been renamed ``fuloong2e``.
 
-``pc-1.0``, ``pc-1.1``, ``pc-1.2`` and ``pc-1.3`` (removed in 6.0)
-''
+``pc-0.10`` up to ``pc-1.3`` (removed in 4.0 up to 6.0)
+'''
 
 These machine types were very old and likely could not be used for live
 migration from old QEMU versions anymore. Use a newer machine type instead.
-- 
2.27.0

[PATCH v2 1/6] docs/about/removed-features: Document removed CLI options from QEMU v2.12

[Thomas Huth]

These CLI options had been removed/replaced in QEMU v2.12. Still, some
people might want to update from older versions to the recent QEMU version,
so we should give some recommendations for the replacements in our
documentation.

Signed-off-by: Thomas Huth 
---
 docs/about/removed-features.rst | 39 +++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 07d597847c..c18af3c76f 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -9,8 +9,43 @@ trouble after a recent upgrade.
 System emulator command line arguments
 --
 
-``-net ...,name=``\ *name* (removed in 5.1)
-'''
+``-hdachs`` (removed in 2.12)
+'
+
+The geometry defined by ``-hdachs c,h,s,t`` should now be specified via
+``-device ide-hd,drive=dr,cyls=c,heads=h,secs=s,bios-chs-trans=t``
+(together with ``-drive if=none,id=dr,...``).
+
+``-net channel`` (removed in 2.12)
+''
+
+This option has been replaced by ``-net user,guestfwd=...``.
+
+``-net dump`` (removed in 2.12)
+'''
+
+``-net dump[,vlan=n][,file=filename][,len=maxlen]`` has been replaced by
+``-object filter-dump,id=id,netdev=dev[,file=filename][,maxlen=maxlen]``.
+Note that the new syntax works with netdev IDs instead of the old "vlan" hubs.
+
+``-no-kvm-pit`` (removed in 2.12)
+'
+
+This was just a dummy option that has been ignored, since the in-kernel PIT
+cannot be disabled separately from the irqchip anymore. A similar effect
+(which also disables the KVM IOAPIC) can be obtained with
+``-M kernel_irqchip=split``.
+
+``-tdf`` (removed in 2.12)
+''
+
+There is no replacement, the ``-tdf`` option has just been ignored since the
+behaviour that could be changed by this option in qemu-kvm is now the default
+when using the KVM PIT. It still can be requested explicitly using
+``-global kvm-pit.lost_tick_policy=delay``.
+
+``-net ...,name=...`` (removed in 5.1)
+''
 
 The ``name`` parameter of the ``-net`` option was a synonym
 for the ``id`` parameter, which should now be used instead.
-- 
2.27.0

[PATCH v2 0/6] Document removed features from older QEMU versions

[Thomas Huth]

Before we started to gather the information about removed features
in the git repo, we collected the information in the Wiki here:

 https://wiki.qemu.org/Features/RemovedFeatures

Since it's rather confusing for the users to have two sources where to
look up the related information, let's add the features from the Wiki
into the documentation in our git repo instead (and then remove the
Wiki page once these patches here have been merged).

v2:
 - Update the descriptions of `-no-kvm-pit`, `-tdf` and `-balloon`
   according to the suggestions of Paolo

Thomas Huth (6):
  docs/about/removed-features: Document removed CLI options from QEMU
v2.12
  docs/about/removed-features: Document removed CLI options from QEMU
v3.0
  docs/about/removed-features: Document removed CLI options from QEMU
v3.1
  docs/about/removed-features: Document removed HMP commands from QEMU
v2.12
  docs/about/removed-features: Document removed devices from older QEMU
versions
  docs/about/removed-features: Document removed machines from older QEMU
versions

 docs/about/removed-features.rst | 183 +++-
 1 file changed, 179 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH v2 3/6] docs/about/removed-features: Document removed CLI options from QEMU v3.1

[Thomas Huth]

These CLI options had been removed/replaced in QEMU v3.1. Still, some
people might want to update from older versions to the recent QEMU version,
so we should give some recommendations for the replacements in our
documentation.

Signed-off-by: Thomas Huth 
---
 docs/about/removed-features.rst | 67 +
 1 file changed, 67 insertions(+)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index c4b702968e..40d2cc4ffa 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -79,6 +79,73 @@ Use ``-machine kernel_irqchip=off`` instead.
 
 Use ``-global kvm-pit.lost_tick_policy=discard`` instead.
 
+``-balloon`` (removed in 3.1)
+'
+
+The ``-balloon virtio`` option has been replaced by ``-device virtio-balloon``.
+The ``-balloon none`` option was a no-op and has no replacement.
+
+``-bootp`` (removed in 3.1)
+'''
+
+The ``-bootp /some/file`` argument is replaced by either
+``-netdev user,id=x,bootp=/some/file`` (for pluggable NICs, accompanied with
+``-device ...,netdev=x``), or ``-nic user,bootp=/some/file`` (for on-board 
NICs).
+The new syntax allows different settings to be provided per NIC.
+
+``-redir`` (removed in 3.1)
+'''
+
+The ``-redir [tcp|udp]:hostport:[guestaddr]:guestport`` option is replaced
+by either ``-netdev
+user,id=x,hostfwd=[tcp|udp]:[hostaddr]:hostport-[guestaddr]:guestport``
+(for pluggable NICs, accompanied with ``-device ...,netdev=x``) or by the 
option
+``-nic user,hostfwd=[tcp|udp]:[hostaddr]:hostport-[guestaddr]:guestport``
+(for on-board NICs). The new syntax allows different settings to be provided
+per NIC.
+
+``-smb`` (removed in 3.1)
+'
+
+The ``-smb /some/dir`` argument is replaced by either
+``-netdev user,id=x,smb=/some/dir`` (for pluggable NICs, accompanied with
+``-device ...,netdev=x``), or ``-nic user,smb=/some/dir`` (for on-board NICs).
+The new syntax allows different settings to be provided per NIC.
+
+``-tftp`` (removed in 3.1)
+''
+
+The ``-tftp /some/dir`` argument is replaced by either
+``-netdev user,id=x,tftp=/some/dir`` (for pluggable NICs, accompanied with
+``-device ...,netdev=x``), or ``-nic user,tftp=/some/dir`` (for embedded NICs).
+The new syntax allows different settings to be provided per NIC.
+
+``-localtime`` (removed in 3.1)
+'''
+
+Replaced by ``-rtc base=localtime``.
+
+``-nodefconfig`` (removed in 3.1)
+'
+
+Use ``-no-user-config`` instead.
+
+``-rtc-td-hack`` (removed in 3.1)
+'
+
+Use ``-rtc driftfix=slew`` instead.
+
+``-startdate`` (removed in 3.1)
+'''
+
+Replaced by ``-rtc base=date``.
+
+``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...``
+'
+
+The "tls-creds" option should be used instead to point to a "tls-creds-x509"
+object created using "-object".
+
 ``-net ...,name=...`` (removed in 5.1)
 ''
 
-- 
2.27.0

[PATCH v2 2/6] docs/about/removed-features: Document removed CLI options from QEMU v3.0

[Thomas Huth]

These CLI options had been removed/replaced in QEMU v3.0. Still, some
people might want to update from older versions to the recent QEMU version,
so we should give some recommendations for the replacements in our
documentation.

Signed-off-by: Thomas Huth 
---
 docs/about/removed-features.rst | 35 +
 1 file changed, 35 insertions(+)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index c18af3c76f..c4b702968e 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -44,6 +44,41 @@ behaviour that could be changed by this option in qemu-kvm 
is now the default
 when using the KVM PIT. It still can be requested explicitly using
 ``-global kvm-pit.lost_tick_policy=delay``.
 
+``-drive secs=s``, ``-drive heads=h`` & ``-drive cyls=c`` (removed in 3.0)
+''
+
+The drive geometry should now be specified via
+``-device ...,drive=dr,cyls=c,heads=h,secs=s`` (together with
+``-drive if=none,id=dr,...``).
+
+``-drive serial=``, ``-drive trans=`` & ``-drive addr=`` (removed in 3.0)
+'
+
+Use ``-device ...,drive=dr,serial=r,bios-chs-trans=t,addr=a`` instead
+(together with ``-drive if=none,id=dr,...``).
+
+``-net ...,vlan=x`` (removed in 3.0)
+
+
+The term "vlan" was very confusing for most users in this context (it's about
+specifying a hub ID, not about IEEE 802.1Q or something similar), so this
+has been removed. To connect one NIC frontend with a network backend, either
+use ``-nic ...`` (e.g. for on-board NICs) or use ``-netdev ...,id=n`` together
+with ``-device ...,netdev=n`` (for full control over pluggable NICs). To
+connect multiple NICs or network backends via a hub device (which is what
+vlan did), use ``-nic hubport,hubid=x,...`` or
+``-netdev hubport,id=n,hubid=x,...`` (with ``-device ...,netdev=n``) instead.
+
+``-no-kvm-irqchip`` (removed in 3.0)
+
+
+Use ``-machine kernel_irqchip=off`` instead.
+
+``-no-kvm-pit-reinjection`` (removed in 3.0)
+
+
+Use ``-global kvm-pit.lost_tick_policy=discard`` instead.
+
 ``-net ...,name=...`` (removed in 5.1)
 ''
 
-- 
2.27.0

Re: [PATCH] build: remove redundant 'check-build' make target

[Daniel P . Berrangé]

On Wed, Aug 11, 2021 at 08:02:44AM +0200, Thomas Huth wrote:
> On 10/08/2021 18.44, Daniel P. Berrangé wrote:
> > The 'check-build' make target was added as a way to build all the unit
> > test binaries, since the standard 'all' target would not trigger this.
> > 
> > Since the switch to meson, however, 'all' will now include the 'test'
> > binaries. As a result, 'check-build' is a no-op:
> > 
> > $ make all check-build
> > ..snip lots of output...
> > make: Nothing to be done for 'check-build'.
> 
> I think it would be better to restore the previous behaviour, so that "all"
> does not build the test files by default. Most normal users don't need the
> tests, so compiling them by default wastes precious CPU cycles.

Building tests by default is a good idea. If I'm refactoring code I want
to see straight away if I've broken test binaries, just as much as if
I've broken a part of the emulator code. I wouldn't want QEMU to go back
to the old behaviour as IMHO that is broken.

If someone doesn't want to run tests in a particular scenario, then by
all means have a meson option to disable tests entirely - neither build
nor run them - which wouldn't require a target to trigger test builds
manually.


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

Re: [PULL ppc] powernv queue

[Cédric Le Goater]

On 8/11/21 10:15 AM, Peter Maydell wrote:
> On Tue, 10 Aug 2021 at 20:36, Cédric Le Goater  wrote:
>> Yes. I should have added a 'ppc-for-6.2' prefix.
> 
> If this wasn't intended for master, can you make sure the
> cover letter includes the "PULL SUBSYSTEM whatever" subject
> tag and has "not for master" in the body, as noted in
> https://wiki.qemu.org/Contribute/SubmitAPullRequest
> please? That way my email filters will ignore it.

ok. That's what I needed to know. Thanks and sorry again for the
false alarm.

C. 

Re: [PULL ppc] powernv queue

[Peter Maydell]

On Tue, 10 Aug 2021 at 20:36, Cédric Le Goater  wrote:
> Yes. I should have added a 'ppc-for-6.2' prefix.

If this wasn't intended for master, can you make sure the
cover letter includes the "PULL SUBSYSTEM whatever" subject
tag and has "not for master" in the body, as noted in
https://wiki.qemu.org/Contribute/SubmitAPullRequest
please? That way my email filters will ignore it.

thanks
-- PMM

Re: [PATCH 08/10] aspeed: Emulate the AST2600A3

[Cédric Le Goater]

On 8/9/21 5:55 PM, Philippe Mathieu-Daudé wrote:
> On 8/9/21 3:15 PM, Cédric Le Goater wrote:
>> From: Joel Stanley 
>>
>> This is the latest revision of the ASPEED 2600 SoC. As there is no
>> need to model multiple revisions of the same SoC for the moment,
>> update the SCU AST2600 to model the A3 revision instead of the A1 and
>> adapt the AST2600 SoC and machines.
>>
>> Reset values are taken from v8 of the datasheet.
>>
>> Signed-off-by: Joel Stanley 
>> [ clg: - Introduced an Aspeed "ast2600-a3" SoC class
>>- Commit log update ]
>> Message-Id: <20210407171637.43-21-...@kaod.org>
>> Signed-off-by: Cédric Le Goater 
>> Message-Id: <20210629142336.750058-3-...@kaod.org>
>> Signed-off-by: Cédric Le Goater 
>> ---
>>  include/hw/misc/aspeed_scu.h |  2 ++
>>  hw/arm/aspeed.c  |  6 +++---
>>  hw/arm/aspeed_ast2600.c  |  6 +++---
>>  hw/misc/aspeed_scu.c | 36 +---
>>  4 files changed, 37 insertions(+), 13 deletions(-)
> 
>> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
>> index 40a38ebd8549..05edebedeb46 100644
>> --- a/hw/misc/aspeed_scu.c
>> +++ b/hw/misc/aspeed_scu.c
>> @@ -101,14 +101,24 @@
>>  #define AST2600_CLK_STOP_CTRL_CLR TO_REG(0x84)
>>  #define AST2600_CLK_STOP_CTRL2 TO_REG(0x90)
>>  #define AST2600_CLK_STOP_CTRL2_CLR TO_REG(0x94)
>> +#define AST2600_DEBUG_CTRLTO_REG(0xC8)
>> +#define AST2600_DEBUG_CTRL2   TO_REG(0xD8)
>>  #define AST2600_SDRAM_HANDSHAKE   TO_REG(0x100)
>>  #define AST2600_HPLL_PARAMTO_REG(0x200)
>>  #define AST2600_HPLL_EXT  TO_REG(0x204)
>> +#define AST2600_APLL_PARAMTO_REG(0x210)
>> +#define AST2600_APLL_EXT  TO_REG(0x214)
>> +#define AST2600_MPLL_PARAMTO_REG(0x220)
>>  #define AST2600_MPLL_EXT  TO_REG(0x224)
>> +#define AST2600_EPLL_PARAMTO_REG(0x240)
>>  #define AST2600_EPLL_EXT  TO_REG(0x244)
>> +#define AST2600_DPLL_PARAMTO_REG(0x260)
>> +#define AST2600_DPLL_EXT  TO_REG(0x264)
>>  #define AST2600_CLK_SEL   TO_REG(0x300)
>>  #define AST2600_CLK_SEL2  TO_REG(0x304)
>> -#define AST2600_CLK_SEL3  TO_REG(0x310)
>> +#define AST2600_CLK_SEL3  TO_REG(0x308)
> 
> Is it a bugfix? Otherwise this is annoying.

This is a bug in the model. These registers have the same layout
on the A1.

Thanks,

C.

> 
> Maybe:
> 
>  #define AST2600A1_CLK_SEL3  TO_REG(0x310)
>  #define AST2600A3_CLK_SEL3  TO_REG(0x308)
> 
> and...
> 
>> +#define AST2600_CLK_SEL4  TO_REG(0x310)
>> +#define AST2600_CLK_SEL5  TO_REG(0x314)
>>  #define AST2600_HW_STRAP1 TO_REG(0x500)
>>  #define AST2600_HW_STRAP1_CLR TO_REG(0x504)
>>  #define AST2600_HW_STRAP1_PROTTO_REG(0x508)
>> @@ -433,6 +443,8 @@ static uint32_t aspeed_silicon_revs[] = {
>>  AST2500_A1_SILICON_REV,
>>  AST2600_A0_SILICON_REV,
>>  AST2600_A1_SILICON_REV,
>> +AST2600_A2_SILICON_REV,
>> +AST2600_A3_SILICON_REV,
>>  };
>>  
>>  bool is_supported_silicon_rev(uint32_t silicon_rev)
>> @@ -651,16 +663,26 @@ static const MemoryRegionOps aspeed_ast2600_scu_ops = {
>>  .valid.unaligned = false,
>>  };
>>  
>> -static const uint32_t ast2600_a1_resets[ASPEED_AST2600_SCU_NR_REGS] = {
>> +static const uint32_t ast2600_a3_resets[ASPEED_AST2600_SCU_NR_REGS] = {
>>  [AST2600_SYS_RST_CTRL]  = 0xF7C3FED8,
>> -[AST2600_SYS_RST_CTRL2] = 0xFFFC,
>> +[AST2600_SYS_RST_CTRL2] = 0x0DFC,
>>  [AST2600_CLK_STOP_CTRL] = 0x7F8A,
>>  [AST2600_CLK_STOP_CTRL2]= 0xFFF0FFF0,
>> +[AST2600_DEBUG_CTRL]= 0x0FFF,
>> +[AST2600_DEBUG_CTRL2]   = 0x00FF,
>>  [AST2600_SDRAM_HANDSHAKE]   = 0x,
>> -[AST2600_HPLL_PARAM]= 0x1000405F,
>> +[AST2600_HPLL_PARAM]= 0x1000408F,
>> +[AST2600_APLL_PARAM]= 0x1000405F,
>> +[AST2600_MPLL_PARAM]= 0x1008405F,
>> +[AST2600_EPLL_PARAM]= 0x1004077F,
>> +[AST2600_DPLL_PARAM]= 0x1078405F,
>> +[AST2600_CLK_SEL]   = 0xF394,
>> +[AST2600_CLK_SEL2]  = 0x0070,
>> +[AST2600_CLK_SEL3]  = 0x,
> 
> ... use AST2600A3_CLK_SEL3 here?
> 
> So someone wanting the emulate the A1 doesn't get
> the nasty bug of having CLK_SEL3 misplaced.
> 
>> +[AST2600_CLK_SEL4]  = 0xF3F4,
>> +[AST2600_CLK_SEL5]  = 0x3000,
>>  [AST2600_CHIP_ID0]  = 0x1234ABCD,
>>  [AST2600_CHIP_ID1]  = 0x,
>> -
>>  };

RE: [PATCH] Use EGL device extension in display initialization.

[Eugene Huang via]

Hi,

I have had some hard time to set up git send-email. I am not even sure if it is 
doable here. I read that attachments can be used a last resort for first 
timers. Here are the attachments. Hope it works.

Thanks,
Eugene

From: Marc-André Lureau 
Sent: Friday, August 6, 2021 12:25 AM
To: Eugene Huang 
Cc: qemu-devel@nongnu.org
Subject: Re: [PATCH] Use EGL device extension in display initialization.

External email: Use caution opening links or attachments

Hi

On Fri, Aug 6, 2021 at 2:28 AM Eugene Huang 
mailto:euge...@nvidia.com>> wrote:

This patch enables running generic EGL devices such as Nvidia's in headless 
mode. It assumes single device. More work is needed to support multiple devices.



Signed-off-by: Eugene Huang mailto:euge...@nvidia.com>>

Thanks for the patch. It isn't correctly formatted and git apply fails  
(https://patchew.org/QEMU/byapr12mb319275649a1403c254a9ea43d9...@byapr12mb3192.namprd12.prod.outlook.com/).
 Please use git send-email.


---

ui/egl-helpers.c | 41 +

1 file changed, 37 insertions(+), 4 deletions(-)



diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c

index 7c530c2825..c11610c083 100644

--- a/ui/egl-helpers.c

+++ b/ui/egl-helpers.c

@@ -1,6 +1,8 @@

/*

  * Copyright (C) 2015-2016 Gerd Hoffmann 
mailto:kra...@redhat.com>>

  *

+ * Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

+ *

  * This library is free software; you can redistribute it and/or

  * modify it under the terms of the GNU Lesser General Public

  * License as published by the Free Software Foundation; either

@@ -349,11 +351,26 @@ static EGLDisplay 
qemu_egl_get_display(EGLNativeDisplayType native,

 EGLDisplay dpy = EGL_NO_DISPLAY;

 /* In practise any EGL 1.5 implementation would support the EXT extension 
*/

-if (epoxy_has_egl_extension(NULL, "EGL_EXT_platform_base")) {

+if (epoxy_has_egl_extension(NULL, "EGL_EXT_platform_base")

+&& epoxy_has_egl_extension(NULL, "EGL_EXT_platform_device")

+&& (epoxy_has_egl_extension(NULL, "EGL_EXT_device_base")

+|| epoxy_has_egl_extension(NULL, "EGL_EXT_device_enumeration"))) {

 PFNEGLGETPLATFORMDISPLAYEXTPROC getPlatformDisplayEXT =

 (void *) eglGetProcAddress("eglGetPlatformDisplayEXT");

 if (getPlatformDisplayEXT && platform != 0) {

-dpy = getPlatformDisplayEXT(platform, native, NULL);

+if (platform == EGL_PLATFORM_DEVICE_EXT) {

+static const int MAX_DEVICES = 4;

+EGLDeviceEXT eglDevs[MAX_DEVICES];

+EGLint numDevices;

+

+PFNEGLQUERYDEVICESEXTPROC eglQueryDevicesEXT =

+(PFNEGLQUERYDEVICESEXTPROC)

+eglGetProcAddress("eglQueryDevicesEXT");

+eglQueryDevicesEXT(MAX_DEVICES, eglDevs, &numDevices);

+dpy = getPlatformDisplayEXT(platform, eglDevs[0], 0);

Given that the function has a lengthy comment to explain it, and this is quite 
archaic stuff, I think you should update the comments with your additions.


+} else {

+dpy = getPlatformDisplayEXT(platform, native, NULL);

+}

 }

 }

@@ -386,6 +403,17 @@ static int qemu_egl_init_dpy(EGLNativeDisplayType dpy,

 EGL_ALPHA_SIZE, 0,

 EGL_NONE,

 };

+

+static const EGLint conf_att_pbuffer[] = {

+EGL_SURFACE_TYPE, EGL_PBUFFER_BIT,

+EGL_RED_SIZE, 8,

+EGL_GREEN_SIZE, 8,

+EGL_BLUE_SIZE, 8,

+EGL_DEPTH_SIZE, 1,

+EGL_RENDERABLE_TYPE, EGL_OPENGL_BIT,

+EGL_NONE

+};

+

 EGLint major, minor;

 EGLBoolean b;

 EGLint n;

@@ -411,8 +439,8 @@ static int qemu_egl_init_dpy(EGLNativeDisplayType dpy,

 }

 b = eglChooseConfig(qemu_egl_display,

-gles ? conf_att_gles : conf_att_core,

-&qemu_egl_config, 1, &n);

+gles ? conf_att_gles : (platform == EGL_PLATFORM_DEVICE_EXT ? 
conf_att_pbuffer : conf_att_core),

+&qemu_egl_config, 1, &n);

 if (b == EGL_FALSE || n != 1) {

 error_report("egl: eglChooseConfig failed (%s mode)",

  gles ? "gles" : "core");

@@ -434,6 +462,11 @@ int qemu_egl_init_dpy_x11(EGLNativeDisplayType dpy, 
DisplayGLMode mode)

 int qemu_egl_init_dpy_mesa(EGLNativeDisplayType dpy, DisplayGLMode mode)

{

+// Try EGL Device Extension

+if (qemu_egl_init_dpy(dpy, EGL_PLATFORM_DEVICE_EX

Re: [PATCH 1/7] MAINTAINERS: update edk2 entry.

[Philippe Mathieu-Daudé]

On 8/10/21 10:34 AM, Gerd Hoffmann wrote:
> I want keep an eye on the edk2 things happening in qemu.
> 
> Cc: Philippe Mathieu-Daudé 
> Signed-off-by: Gerd Hoffmann 
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 37b1a8e4428c..9d31e42786b5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2947,6 +2947,7 @@ F: docs/interop/firmware.json
>  
>  EDK2 Firmware
>  M: Philippe Mathieu-Daudé 
> +R: Gerd Hoffmann 

Let's be honest and add you a M: tag here ;)

Whichever you prefer:
Reviewed-by: Philippe Mathieu-Daudé 

>  S: Supported
>  F: hw/i386/*ovmf*
>  F: pc-bios/descriptors/??-edk2-*.json
> 

Re: [PATCH 09/19] PPC64/TCG: Implement 'rfebb' instruction

[David Gibson]

On Tue, Aug 10, 2021 at 04:32:35PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/10/21 12:50 AM, David Gibson wrote:
> > On Mon, Aug 09, 2021 at 10:10:47AM -0300, Daniel Henrique Barboza wrote:
> > > From: Gustavo Romero 
> > > 
> > > An Event-Based Branch (EBB) allows applications to change the NIA when a
> > > event-based exception occurs. Event-based exceptions are enabled by
> > > setting the Branch Event Status and Control Register (BESCR). If the
> > > event-based exception is enabled when the exception occurs, an EBB
> > > happens.
> > > 
> > > The EBB will:
> > > 
> > > - set the Global Enable (GE) bit of BESCR to 0;
> > > - set bits 0-61 of the Event-Based Branch Return Register (EBBRR) to the
> > >effective address of the NIA that would have executed if the EBB
> > >didn't happen;
> > > - Instruction fetch and execution will continue in the effective address
> > >contained in the Event-Based Branch Handler Register (EBBHR).
> > > 
> > > The EBB Handler will process the event and then execute the Return From
> > > Event-Based Branch (rfebb) instruction. rfebb sets BESCR_GE and then
> > > redirects execution to the address pointed in EBBRR. This process is
> > > described in the PowerISA v3.1, Book II, Chapter 6 [1].
> > > 
> > > This patch implements the rfebb instruction. Descriptions of all
> > > relevant BESCR bits are also added - this patch is only using BESCR_GE,
> > > but next patches will use the remaining bits.
> > > 
> > > Note that we're implementing the extended rfebb mnemonic (BESCR_GE is
> > > being always set to 1). The basic rfebb instruction would accept an
> > > operand that would be used to set GE.
> > > 
> > > [1] https://wiki.raptorcs.com/w/images/f/f5/PowerISA_public.v3.1.pdf
> > > 
> > > CC: Gustavo Romero 
> > > Signed-off-by: Gustavo Romero 
> > > Signed-off-by: Daniel Henrique Barboza 
> > > ---
> > >   target/ppc/cpu.h   | 12 
> > >   target/ppc/translate.c | 21 +
> > >   2 files changed, 33 insertions(+)
> > > 
> > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > index afd9cd402b..ae431e65be 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -358,6 +358,18 @@ typedef struct ppc_v3_pate_t {
> > >   #define MMCR1_PMC3SEL PPC_BITMASK(48, 55)
> > >   #define MMCR1_PMC4SEL PPC_BITMASK(56, 63)
> > > +/* EBB/BESCR bits */
> > > +/* Global Enable */
> > > +#define BESCR_GE PPC_BIT(0)
> > > +/* External Event-based Exception Enable */
> > > +#define BESCR_EE PPC_BIT(30)
> > > +/* Performance Monitor Event-based Exception Enable */
> > > +#define BESCR_PME PPC_BIT(31)
> > > +/* External Event-based Exception Occurred */
> > > +#define BESCR_EEO PPC_BIT(62)
> > > +/* Performance Monitor Event-based Exception Occurred */
> > > +#define BESCR_PMEO PPC_BIT(63)
> > > +
> > >   /* LPCR bits */
> > >   #define LPCR_VPM0 PPC_BIT(0)
> > >   #define LPCR_VPM1 PPC_BIT(1)
> > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > index 62356cfadf..afc254a03f 100644
> > > --- a/target/ppc/translate.c
> > > +++ b/target/ppc/translate.c
> > > @@ -2701,6 +2701,26 @@ static void gen_darn(DisasContext *ctx)
> > >   }
> > >   }
> > >   }
> > > +
> > > +/* rfebb */
> > > +static void gen_rfebb(DisasContext *ctx)
> > 
> > Oof.. not necessarily a nack, but it would be nice to implement any
> > new instructions using the disastree path rather than the old ppc
> > specific decode logic.
> 
> 
> I'm not sure what is the disastree path. Is it similar to how rfscv is
> implemented?

No, it's a generic system for decoding instructions.  A few things in
POWER have been moved over to it: that's the stuff in insn64.decode
and the trans_*() functions, rather than the gen_*() functions.

So far only the 64-bit prefixed instructions + instructions that were
immediately related to them were converted.  ppc_tr_translate_insn()
attempts first to decode using decode_insn32() (which is decodetree)
then if that fails, falls back to decode_legacy().

-- 
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