qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PULL v2 0/9] s390x update
@ 2021-05-20 17:05 Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 1/9] target/s390x: Fix translation exception on illegal instruction Cornelia Huck
                   ` (9 more replies)
  0 siblings, 10 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-05-20 17:05 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-s390x, Cornelia Huck, qemu-devel

[Note: there's an unrelated hexagon failure in the CI for this tag;
 fixed by <20210520153831.11873-1-alex.bennee@linaro.org>]

The following changes since commit fea2ad71c3e23f743701741346b51fdfbbff5ebf:

  Merge remote-tracking branch 'remotes/stsquad/tags/pull-testing-and-plugin-updates-180521-2' into staging (2021-05-20 10:00:58 +0100)

are available in the Git repository at:

  https://gitlab.com/cohuck/qemu.git tags/s390x-20210520-v2

for you to fetch changes up to f66487756b0553b156d8e3e81bc6411cfc38176e:

  tests/tcg/x86_64: add vsyscall smoke test (2021-05-20 14:19:30 +0200)

----------------------------------------------------------------
s390x fixes and cleanups; also related fixes in xtensa,
arm, and x86 code

----------------------------------------------------------------

Eric Farman (2):
  vfio-ccw: Permit missing IRQs
  vfio-ccw: Attempt to clean up all IRQs on error

Ilya Leoshkevich (6):
  target/s390x: Fix translation exception on illegal instruction
  target/arm: Make sure that commpage's tb->size != 0
  target/xtensa: Make sure that tb->size != 0
  accel/tcg: Assert that tb->size != 0 after translation
  target/i386: Make sure that vsyscall's tb->size != 0
  tests/tcg/x86_64: add vsyscall smoke test

Philippe Mathieu-Daudé (1):
  hw/s390x/ccw: Register qbus type in abstract TYPE_CCW_DEVICE parent

 accel/tcg/translate-all.c        |  1 +
 hw/s390x/3270-ccw.c              |  1 -
 hw/s390x/ccw-device.c            |  1 +
 hw/s390x/ccw-device.h            |  1 +
 hw/s390x/s390-ccw.c              |  2 --
 hw/s390x/virtio-ccw.c            |  1 -
 hw/vfio/ccw.c                    | 18 +++++++++++-------
 target/arm/translate.c           |  2 ++
 target/i386/tcg/translate.c      |  1 +
 target/s390x/translate.c         | 16 +++++++++++-----
 target/xtensa/translate.c        |  3 +++
 tests/tcg/x86_64/Makefile.target |  6 +++++-
 tests/tcg/x86_64/vsyscall.c      | 12 ++++++++++++
 13 files changed, 48 insertions(+), 17 deletions(-)
 create mode 100644 tests/tcg/x86_64/vsyscall.c

-- 
2.31.1



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PULL v2 1/9] target/s390x: Fix translation exception on illegal instruction
  2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
@ 2021-05-20 17:05 ` Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 2/9] target/arm: Make sure that commpage's tb->size != 0 Cornelia Huck
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-05-20 17:05 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Cornelia Huck, qemu-s390x, David Hildenbrand, qemu-devel,
	Ilya Leoshkevich

From: Ilya Leoshkevich <iii@linux.ibm.com>

Hitting an uretprobe in a s390x TCG guest causes a SIGSEGV. What
happens is:

* uretprobe maps a userspace page containing an invalid instruction.
* uretprobe replaces the target function's return address with the
  address of that page.
* When tb_gen_code() is called on that page, tb->size ends up being 0
  (because the page starts with the invalid instruction), which causes
  virt_page2 to point to the previous page.
* The previous page is not mapped, so this causes a spurious
  translation exception.

tb->size must never be 0: even if there is an illegal instruction, the
instruction bytes that have been looked at must count towards tb->size.
So adjust s390x's translate_one() to act this way for both illegal
instructions and instructions that are known to generate exceptions.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Message-Id: <20210416154939.32404-2-iii@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/s390x/translate.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 4f953ddfbaf6..e243624d2a62 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -6412,7 +6412,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
         qemu_log_mask(LOG_UNIMP, "unimplemented opcode 0x%02x%02x\n",
                       s->fields.op, s->fields.op2);
         gen_illegal_opcode(s);
-        return DISAS_NORETURN;
+        ret = DISAS_NORETURN;
+        goto out;
     }
 
 #ifndef CONFIG_USER_ONLY
@@ -6428,7 +6429,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
         /* privileged instruction */
         if ((s->base.tb->flags & FLAG_MASK_PSTATE) && (insn->flags & IF_PRIV)) {
             gen_program_exception(s, PGM_PRIVILEGED);
-            return DISAS_NORETURN;
+            ret = DISAS_NORETURN;
+            goto out;
         }
 
         /* if AFP is not enabled, instructions and registers are forbidden */
@@ -6455,7 +6457,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
             }
             if (dxc) {
                 gen_data_exception(dxc);
-                return DISAS_NORETURN;
+                ret = DISAS_NORETURN;
+                goto out;
             }
         }
 
@@ -6463,7 +6466,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
         if (insn->flags & IF_VEC) {
             if (!((s->base.tb->flags & FLAG_MASK_VECTOR))) {
                 gen_data_exception(0xfe);
-                return DISAS_NORETURN;
+                ret = DISAS_NORETURN;
+                goto out;
             }
         }
 
@@ -6484,7 +6488,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
             (insn->spec & SPEC_r1_f128 && !is_fp_pair(get_field(s, r1))) ||
             (insn->spec & SPEC_r2_f128 && !is_fp_pair(get_field(s, r2)))) {
             gen_program_exception(s, PGM_SPECIFICATION);
-            return DISAS_NORETURN;
+            ret = DISAS_NORETURN;
+            goto out;
         }
     }
 
@@ -6544,6 +6549,7 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
     }
 #endif
 
+out:
     /* Advance to the next instruction.  */
     s->base.pc_next = s->pc_tmp;
     return ret;
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PULL v2 2/9] target/arm: Make sure that commpage's tb->size != 0
  2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 1/9] target/s390x: Fix translation exception on illegal instruction Cornelia Huck
@ 2021-05-20 17:05 ` Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 3/9] target/xtensa: Make sure that " Cornelia Huck
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-05-20 17:05 UTC (permalink / raw)
  To: Peter Maydell
  Cc: qemu-s390x, Cornelia Huck, Richard Henderson, qemu-devel,
	Ilya Leoshkevich

From: Ilya Leoshkevich <iii@linux.ibm.com>

tb_gen_code() assumes that tb->size must never be zero, otherwise it
may produce spurious exceptions. For ARM this may happen when creating
a translation block for the commpage.

Fix by pretending that commpage translation blocks have at least one
instruction.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20210416154939.32404-3-iii@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/arm/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 455352bcf60f..8e0e55c1e0f5 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8981,6 +8981,7 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     unsigned int insn;
 
     if (arm_pre_translate_insn(dc)) {
+        dc->base.pc_next += 4;
         return;
     }
 
@@ -9050,6 +9051,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     bool is_16bit;
 
     if (arm_pre_translate_insn(dc)) {
+        dc->base.pc_next += 2;
         return;
     }
 
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PULL v2 3/9] target/xtensa: Make sure that tb->size != 0
  2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 1/9] target/s390x: Fix translation exception on illegal instruction Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 2/9] target/arm: Make sure that commpage's tb->size != 0 Cornelia Huck
@ 2021-05-20 17:05 ` Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 4/9] accel/tcg: Assert that tb->size != 0 after translation Cornelia Huck
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-05-20 17:05 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Max Filippov, qemu-s390x, Cornelia Huck, qemu-devel, Ilya Leoshkevich

From: Ilya Leoshkevich <iii@linux.ibm.com>

tb_gen_code() assumes that tb->size must never be zero, otherwise it
may produce spurious exceptions. For xtensa this may happen when
decoding an unknown instruction, when handling a write into the
CCOUNT or CCOMPARE special register and when single-stepping the first
instruction of an exception handler.

Fix by pretending that the size of the respective translation block is
1 in all these cases.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Tested-by: Max Filippov <jcmvbkbc@gmail.com>
Acked-by: Max Filippov <jcmvbkbc@gmail.com>
Message-Id: <20210416154939.32404-4-iii@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/xtensa/translate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 0ae4efc48a17..73584d9d605b 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -917,6 +917,7 @@ static void disas_xtensa_insn(CPUXtensaState *env, DisasContext *dc)
                       "unknown instruction length (pc = %08x)\n",
                       dc->pc);
         gen_exception_cause(dc, ILLEGAL_INSTRUCTION_CAUSE);
+        dc->base.pc_next = dc->pc + 1;
         return;
     }
 
@@ -1274,11 +1275,13 @@ static void xtensa_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     if ((tb_cflags(dc->base.tb) & CF_USE_ICOUNT)
         && (dc->base.tb->flags & XTENSA_TBFLAG_YIELD)) {
         gen_exception(dc, EXCP_YIELD);
+        dc->base.pc_next = dc->pc + 1;
         dc->base.is_jmp = DISAS_NORETURN;
         return;
     }
     if (dc->base.tb->flags & XTENSA_TBFLAG_EXCEPTION) {
         gen_exception(dc, EXCP_DEBUG);
+        dc->base.pc_next = dc->pc + 1;
         dc->base.is_jmp = DISAS_NORETURN;
         return;
     }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PULL v2 4/9] accel/tcg: Assert that tb->size != 0 after translation
  2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
                   ` (2 preceding siblings ...)
  2021-05-20 17:05 ` [PULL v2 3/9] target/xtensa: Make sure that " Cornelia Huck
@ 2021-05-20 17:05 ` Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 5/9] vfio-ccw: Permit missing IRQs Cornelia Huck
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-05-20 17:05 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Cornelia Huck, qemu-s390x, David Hildenbrand, qemu-devel,
	Ilya Leoshkevich

From: Ilya Leoshkevich <iii@linux.ibm.com>

If arch-specific code generates a translation block of size 0,
tb_gen_code() may generate a spurious exception. Add an assertion in
order to catch such situations early.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Message-Id: <20210416154939.32404-5-iii@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 accel/tcg/translate-all.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index fbf8fc630b27..640ff6e3e706 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1912,6 +1912,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 
     tcg_ctx->cpu = env_cpu(env);
     gen_intermediate_code(cpu, tb, max_insns);
+    assert(tb->size != 0);
     tcg_ctx->cpu = NULL;
     max_insns = tb->icount;
 
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PULL v2 5/9] vfio-ccw: Permit missing IRQs
  2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
                   ` (3 preceding siblings ...)
  2021-05-20 17:05 ` [PULL v2 4/9] accel/tcg: Assert that tb->size != 0 after translation Cornelia Huck
@ 2021-05-20 17:05 ` Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 6/9] hw/s390x/ccw: Register qbus type in abstract TYPE_CCW_DEVICE parent Cornelia Huck
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-05-20 17:05 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Eric Farman, qemu-s390x, Cornelia Huck, qemu-devel

From: Eric Farman <farman@linux.ibm.com>

Commit 690e29b91102 ("vfio-ccw: Refactor ccw irq handler") changed
one of the checks for the IRQ notifier registration from saying
"the host needs to recognize the only IRQ that exists" to saying
"the host needs to recognize ANY IRQ that exists."

And this worked fine, because the subsequent change to support the
CRW IRQ notifier doesn't get into this code when running on an older
kernel, thanks to a guard by a capability region. The later addition
of the REQ(uest) IRQ by commit b2f96f9e4f5f ("vfio-ccw: Connect the
device request notifier") broke this assumption because there is no
matching capability region. Thus, running new QEMU on an older
kernel fails with:

  vfio: unexpected number of irqs 2

Let's adapt the message here so that there's a better clue of what
IRQ is missing.

Furthermore, let's make the REQ(uest) IRQ not fail when attempting
to register it, to permit running vfio-ccw on a newer QEMU with an
older kernel.

Fixes: b2f96f9e4f5f ("vfio-ccw: Connect the device request notifier")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Message-Id: <20210421152053.2379873-1-farman@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/vfio/ccw.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index e752c845e9e4..7c058d13e8ce 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -411,8 +411,8 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev,
     }
 
     if (vdev->num_irqs < irq + 1) {
-        error_setg(errp, "vfio: unexpected number of irqs %u",
-                   vdev->num_irqs);
+        error_setg(errp, "vfio: IRQ %u not available (number of irqs %u)",
+                   irq, vdev->num_irqs);
         return;
     }
 
@@ -695,13 +695,15 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
 
     vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX, &err);
     if (err) {
-        goto out_req_notifier_err;
+        /*
+         * Report this error, but do not make it a failing condition.
+         * Lack of this IRQ in the host does not prevent normal operation.
+         */
+        error_report_err(err);
     }
 
     return;
 
-out_req_notifier_err:
-    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX);
 out_crw_notifier_err:
     vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
 out_io_notifier_err:
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PULL v2 6/9] hw/s390x/ccw: Register qbus type in abstract TYPE_CCW_DEVICE parent
  2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
                   ` (4 preceding siblings ...)
  2021-05-20 17:05 ` [PULL v2 5/9] vfio-ccw: Permit missing IRQs Cornelia Huck
@ 2021-05-20 17:05 ` Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 7/9] vfio-ccw: Attempt to clean up all IRQs on error Cornelia Huck
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-05-20 17:05 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Eric Farman, qemu-s390x, Cornelia Huck, qemu-devel,
	Philippe Mathieu-Daudé

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Instead of having all TYPE_CCW_DEVICE children set the bus type to
TYPE_VIRTUAL_CSS_BUS, do it once in the abstract parent.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Eric Farman <farman@linux.ibm.com>
Message-Id: <20210424145313.3287400-1-f4bug@amsat.org>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/3270-ccw.c   | 1 -
 hw/s390x/ccw-device.c | 1 +
 hw/s390x/ccw-device.h | 1 +
 hw/s390x/s390-ccw.c   | 2 --
 hw/s390x/virtio-ccw.c | 1 -
 5 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
index 25e628f57573..13e93d8d8f61 100644
--- a/hw/s390x/3270-ccw.c
+++ b/hw/s390x/3270-ccw.c
@@ -158,7 +158,6 @@ static void emulated_ccw_3270_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     device_class_set_props(dc, emulated_ccw_3270_properties);
-    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
     dc->realize = emulated_ccw_3270_realize;
     dc->hotpluggable = false;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
index c9707110e9c8..95f269ab441e 100644
--- a/hw/s390x/ccw-device.c
+++ b/hw/s390x/ccw-device.c
@@ -59,6 +59,7 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
     k->refill_ids = ccw_device_refill_ids;
     device_class_set_props(dc, ccw_device_properties);
     dc->reset = ccw_device_reset;
+    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
 }
 
 const VMStateDescription vmstate_ccw_dev = {
diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
index 832c78cd4213..6dff95225df1 100644
--- a/hw/s390x/ccw-device.h
+++ b/hw/s390x/ccw-device.h
@@ -14,6 +14,7 @@
 #include "qom/object.h"
 #include "hw/qdev-core.h"
 #include "hw/s390x/css.h"
+#include "hw/s390x/css-bridge.h"
 
 struct CcwDevice {
     DeviceState parent_obj;
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 242491a1aea0..c227c77984ce 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -176,10 +176,8 @@ static void s390_ccw_instance_init(Object *obj)
 
 static void s390_ccw_class_init(ObjectClass *klass, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
     S390CCWDeviceClass *cdc = S390_CCW_DEVICE_CLASS(klass);
 
-    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
     cdc->realize = s390_ccw_realize;
     cdc->unrealize = s390_ccw_unrealize;
 }
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 92b950e09a13..220b9efcf945 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1234,7 +1234,6 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
     k->unplug = virtio_ccw_busdev_unplug;
     dc->realize = virtio_ccw_busdev_realize;
     dc->unrealize = virtio_ccw_busdev_unrealize;
-    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
     device_class_set_parent_reset(dc, virtio_ccw_reset, &vdc->parent_reset);
 }
 
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PULL v2 7/9] vfio-ccw: Attempt to clean up all IRQs on error
  2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
                   ` (5 preceding siblings ...)
  2021-05-20 17:05 ` [PULL v2 6/9] hw/s390x/ccw: Register qbus type in abstract TYPE_CCW_DEVICE parent Cornelia Huck
@ 2021-05-20 17:05 ` Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 8/9] target/i386: Make sure that vsyscall's tb->size != 0 Cornelia Huck
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-05-20 17:05 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Eric Farman, qemu-s390x, Cornelia Huck, qemu-devel, Matthew Rosato

From: Eric Farman <farman@linux.ibm.com>

The vfio_ccw_unrealize() routine makes an unconditional attempt to
unregister every IRQ notifier, though they may not have been registered
in the first place (when running on an older kernel, for example).

Let's mirror this behavior in the error cleanups in vfio_ccw_realize()
so that if/when new IRQs are added, it is less confusing to recognize
the necessary procedures. The worst case scenario would be some extra
messages about an undefined IRQ, but since this is an error exit that
won't be the only thing to worry about.

And regarding those messages, let's change it to a warning instead of
an error, to better reflect their severity. The existing code in both
paths handles everything anyway.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Acked-by: Matthew Rosato <mjrosato@linux.ibm.com>
Message-Id: <20210428143652.1571487-1-farman@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/vfio/ccw.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 7c058d13e8ce..139a3d9d1b95 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -469,7 +469,7 @@ static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev,
 
     if (vfio_set_irq_signaling(&vcdev->vdev, irq, 0,
                                VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
-        error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
+        warn_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
     }
 
     qemu_set_fd_handler(event_notifier_get_fd(notifier),
@@ -689,7 +689,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
     if (vcdev->crw_region) {
         vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX, &err);
         if (err) {
-            goto out_crw_notifier_err;
+            goto out_irq_notifier_err;
         }
     }
 
@@ -704,7 +704,9 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
 
     return;
 
-out_crw_notifier_err:
+out_irq_notifier_err:
+    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX);
+    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX);
     vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
 out_io_notifier_err:
     vfio_ccw_put_region(vcdev);
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PULL v2 8/9] target/i386: Make sure that vsyscall's tb->size != 0
  2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
                   ` (6 preceding siblings ...)
  2021-05-20 17:05 ` [PULL v2 7/9] vfio-ccw: Attempt to clean up all IRQs on error Cornelia Huck
@ 2021-05-20 17:05 ` Cornelia Huck
  2021-05-20 17:05 ` [PULL v2 9/9] tests/tcg/x86_64: add vsyscall smoke test Cornelia Huck
  2021-05-20 19:11 ` [PULL v2 0/9] s390x update Peter Maydell
  9 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-05-20 17:05 UTC (permalink / raw)
  To: Peter Maydell
  Cc: qemu-s390x, Cornelia Huck, Richard Henderson, qemu-devel,
	Ilya Leoshkevich

From: Ilya Leoshkevich <iii@linux.ibm.com>

tb_gen_code() assumes that tb->size must never be zero, otherwise it
may produce spurious exceptions. For x86_64 this may happen when
creating a translation block for the vsyscall page.

Fix by pretending that vsyscall translation blocks have at least one
instruction.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20210519045738.1335210-2-iii@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/i386/tcg/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index db56a483435d..3ab8c2385560 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -8583,6 +8583,7 @@ static void i386_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
      */
     if ((dc->base.pc_next & TARGET_PAGE_MASK) == TARGET_VSYSCALL_PAGE) {
         gen_exception(dc, EXCP_VSYSCALL, dc->base.pc_next);
+        dc->base.pc_next = dc->pc + 1;
         return;
     }
 #endif
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PULL v2 9/9] tests/tcg/x86_64: add vsyscall smoke test
  2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
                   ` (7 preceding siblings ...)
  2021-05-20 17:05 ` [PULL v2 8/9] target/i386: Make sure that vsyscall's tb->size != 0 Cornelia Huck
@ 2021-05-20 17:05 ` Cornelia Huck
  2021-05-20 19:11 ` [PULL v2 0/9] s390x update Peter Maydell
  9 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-05-20 17:05 UTC (permalink / raw)
  To: Peter Maydell
  Cc: qemu-s390x, Cornelia Huck, Richard Henderson, qemu-devel,
	Ilya Leoshkevich

From: Ilya Leoshkevich <iii@linux.ibm.com>

Having a small test will prevent trivial regressions in the future.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Message-Id: <20210519045738.1335210-3-iii@linux.ibm.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 tests/tcg/x86_64/Makefile.target |  6 +++++-
 tests/tcg/x86_64/vsyscall.c      | 12 ++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/x86_64/vsyscall.c

diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
index 20bf96202ad2..2151ea6302ec 100644
--- a/tests/tcg/x86_64/Makefile.target
+++ b/tests/tcg/x86_64/Makefile.target
@@ -3,14 +3,18 @@
 # x86_64 tests - included from tests/tcg/Makefile.target
 #
 # Currently we only build test-x86_64 and test-i386-ssse3 from
-# $(SRC)/tests/tcg/i386/
+# $(SRC_PATH)/tests/tcg/i386/
 #
 
 include $(SRC_PATH)/tests/tcg/i386/Makefile.target
 
+X86_64_TESTS += vsyscall
 TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
 QEMU_OPTS += -cpu max
 
 test-x86_64: LDFLAGS+=-lm -lc
 test-x86_64: test-i386.c test-i386.h test-i386-shift.h test-i386-muldiv.h
 	$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
+
+vsyscall: $(SRC_PATH)/tests/tcg/x86_64/vsyscall.c
+	$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
diff --git a/tests/tcg/x86_64/vsyscall.c b/tests/tcg/x86_64/vsyscall.c
new file mode 100644
index 000000000000..786b047053aa
--- /dev/null
+++ b/tests/tcg/x86_64/vsyscall.c
@@ -0,0 +1,12 @@
+#include <stdio.h>
+#include <time.h>
+
+#define VSYSCALL_PAGE 0xffffffffff600000
+#define TIME_OFFSET 0x400
+typedef time_t (*time_func)(time_t *);
+
+int main(void)
+{
+    printf("%ld\n", ((time_func)(VSYSCALL_PAGE + TIME_OFFSET))(NULL));
+    return 0;
+}
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PULL v2 0/9] s390x update
  2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
                   ` (8 preceding siblings ...)
  2021-05-20 17:05 ` [PULL v2 9/9] tests/tcg/x86_64: add vsyscall smoke test Cornelia Huck
@ 2021-05-20 19:11 ` Peter Maydell
  9 siblings, 0 replies; 11+ messages in thread
From: Peter Maydell @ 2021-05-20 19:11 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: qemu-s390x, QEMU Developers

On Thu, 20 May 2021 at 18:05, Cornelia Huck <cohuck@redhat.com> wrote:
>
> [Note: there's an unrelated hexagon failure in the CI for this tag;
>  fixed by <20210520153831.11873-1-alex.bennee@linaro.org>]
>
> The following changes since commit fea2ad71c3e23f743701741346b51fdfbbff5ebf:
>
>   Merge remote-tracking branch 'remotes/stsquad/tags/pull-testing-and-plugin-updates-180521-2' into staging (2021-05-20 10:00:58 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/cohuck/qemu.git tags/s390x-20210520-v2
>
> for you to fetch changes up to f66487756b0553b156d8e3e81bc6411cfc38176e:
>
>   tests/tcg/x86_64: add vsyscall smoke test (2021-05-20 14:19:30 +0200)
>
> ----------------------------------------------------------------
> s390x fixes and cleanups; also related fixes in xtensa,
> arm, and x86 code
>


Applied, thanks.

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

-- PMM


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-05-20 19:12 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-20 17:05 [PULL v2 0/9] s390x update Cornelia Huck
2021-05-20 17:05 ` [PULL v2 1/9] target/s390x: Fix translation exception on illegal instruction Cornelia Huck
2021-05-20 17:05 ` [PULL v2 2/9] target/arm: Make sure that commpage's tb->size != 0 Cornelia Huck
2021-05-20 17:05 ` [PULL v2 3/9] target/xtensa: Make sure that " Cornelia Huck
2021-05-20 17:05 ` [PULL v2 4/9] accel/tcg: Assert that tb->size != 0 after translation Cornelia Huck
2021-05-20 17:05 ` [PULL v2 5/9] vfio-ccw: Permit missing IRQs Cornelia Huck
2021-05-20 17:05 ` [PULL v2 6/9] hw/s390x/ccw: Register qbus type in abstract TYPE_CCW_DEVICE parent Cornelia Huck
2021-05-20 17:05 ` [PULL v2 7/9] vfio-ccw: Attempt to clean up all IRQs on error Cornelia Huck
2021-05-20 17:05 ` [PULL v2 8/9] target/i386: Make sure that vsyscall's tb->size != 0 Cornelia Huck
2021-05-20 17:05 ` [PULL v2 9/9] tests/tcg/x86_64: add vsyscall smoke test Cornelia Huck
2021-05-20 19:11 ` [PULL v2 0/9] s390x update Peter Maydell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).