* [PATCH 1/3] accel/tcg: Fix unaligned stores to s390x low-address-protected lowcore
2022-07-11 18:56 [PATCH 0/3] accel/tcg: Fix unaligned stores to s390x low-address-protected lowcore Ilya Leoshkevich
@ 2022-07-11 18:56 ` Ilya Leoshkevich
2022-07-12 5:12 ` Richard Henderson
2022-07-12 7:14 ` David Hildenbrand
2022-07-11 18:56 ` [PATCH 2/3] hw/misc: Add mmio-debug-exit device Ilya Leoshkevich
2022-07-11 18:56 ` [PATCH 3/3] tests/tcg/s390x: Test unaligned accesses to lowcore Ilya Leoshkevich
2 siblings, 2 replies; 11+ messages in thread
From: Ilya Leoshkevich @ 2022-07-11 18:56 UTC (permalink / raw)
To: Richard Henderson, Paolo Bonzini, Cornelia Huck, Thomas Huth,
Halil Pasic, Christian Borntraeger, Eric Farman,
David Hildenbrand
Cc: qemu-devel, qemu-s390x, Ilya Leoshkevich
If low-address-protection is active, unaligned stores to non-protected
parts of lowcore lead to protection exceptions. The reason is that in
such cases tlb_fill() call in store_helper_unaligned() covers
[0, addr + size) range, which contains the protected portion of
lowcore. This range is too large.
The most straightforward fix would be to make sure we stay within the
original [addr, addr + size) range. However, if an unaligned access
affects a single page, we don't need to call tlb_fill() in
store_helper_unaligned() at all, since it would be identical to
the previous tlb_fill() call in store_helper(), and therefore a no-op.
If an unaligned access covers multiple pages, this situation does not
occur.
Therefore simply skip TLB handling in store_helper_unaligned() if we
are dealing with a single page.
Fixes: 2bcf018340cb ("s390x/tcg: low-address protection support")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
accel/tcg/cputlb.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index f90f4312ea..a46f3a654d 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -2248,7 +2248,7 @@ store_helper_unaligned(CPUArchState *env, target_ulong addr, uint64_t val,
const size_t tlb_off = offsetof(CPUTLBEntry, addr_write);
uintptr_t index, index2;
CPUTLBEntry *entry, *entry2;
- target_ulong page2, tlb_addr, tlb_addr2;
+ target_ulong page1, page2, tlb_addr, tlb_addr2;
MemOpIdx oi;
size_t size2;
int i;
@@ -2256,15 +2256,17 @@ store_helper_unaligned(CPUArchState *env, target_ulong addr, uint64_t val,
/*
* Ensure the second page is in the TLB. Note that the first page
* is already guaranteed to be filled, and that the second page
- * cannot evict the first.
+ * cannot evict the first. An exception to this rule is PAGE_WRITE_INV
+ * handling: the first page could have evicted itself.
*/
+ page1 = addr & TARGET_PAGE_MASK;
page2 = (addr + size) & TARGET_PAGE_MASK;
size2 = (addr + size) & ~TARGET_PAGE_MASK;
index2 = tlb_index(env, mmu_idx, page2);
entry2 = tlb_entry(env, mmu_idx, page2);
tlb_addr2 = tlb_addr_write(entry2);
- if (!tlb_hit_page(tlb_addr2, page2)) {
+ if (page1 != page2 && !tlb_hit_page(tlb_addr2, page2)) {
if (!victim_tlb_hit(env, mmu_idx, index2, tlb_off, page2)) {
tlb_fill(env_cpu(env), page2, size2, MMU_DATA_STORE,
mmu_idx, retaddr);
--
2.35.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] accel/tcg: Fix unaligned stores to s390x low-address-protected lowcore
2022-07-11 18:56 ` [PATCH 1/3] " Ilya Leoshkevich
@ 2022-07-12 5:12 ` Richard Henderson
2022-07-12 7:14 ` David Hildenbrand
1 sibling, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2022-07-12 5:12 UTC (permalink / raw)
To: Ilya Leoshkevich, Paolo Bonzini, Cornelia Huck, Thomas Huth,
Halil Pasic, Christian Borntraeger, Eric Farman,
David Hildenbrand
Cc: qemu-devel, qemu-s390x
On 7/12/22 00:26, Ilya Leoshkevich wrote:
> If low-address-protection is active, unaligned stores to non-protected
> parts of lowcore lead to protection exceptions. The reason is that in
> such cases tlb_fill() call in store_helper_unaligned() covers
> [0, addr + size) range, which contains the protected portion of
> lowcore. This range is too large.
>
> The most straightforward fix would be to make sure we stay within the
> original [addr, addr + size) range. However, if an unaligned access
> affects a single page, we don't need to call tlb_fill() in
> store_helper_unaligned() at all, since it would be identical to
> the previous tlb_fill() call in store_helper(), and therefore a no-op.
> If an unaligned access covers multiple pages, this situation does not
> occur.
>
> Therefore simply skip TLB handling in store_helper_unaligned() if we
> are dealing with a single page.
>
> Fixes: 2bcf018340cb ("s390x/tcg: low-address protection support")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Queueing to tcg-next. I'll let the test case go through s390x.
r~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] accel/tcg: Fix unaligned stores to s390x low-address-protected lowcore
2022-07-11 18:56 ` [PATCH 1/3] " Ilya Leoshkevich
2022-07-12 5:12 ` Richard Henderson
@ 2022-07-12 7:14 ` David Hildenbrand
1 sibling, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2022-07-12 7:14 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson, Paolo Bonzini, Cornelia Huck,
Thomas Huth, Halil Pasic, Christian Borntraeger, Eric Farman
Cc: qemu-devel, qemu-s390x
On 11.07.22 20:56, Ilya Leoshkevich wrote:
> If low-address-protection is active, unaligned stores to non-protected
> parts of lowcore lead to protection exceptions. The reason is that in
> such cases tlb_fill() call in store_helper_unaligned() covers
> [0, addr + size) range, which contains the protected portion of
> lowcore. This range is too large.
>
> The most straightforward fix would be to make sure we stay within the
> original [addr, addr + size) range. However, if an unaligned access
> affects a single page, we don't need to call tlb_fill() in
> store_helper_unaligned() at all, since it would be identical to
> the previous tlb_fill() call in store_helper(), and therefore a no-op.
> If an unaligned access covers multiple pages, this situation does not
> occur.
>
> Therefore simply skip TLB handling in store_helper_unaligned() if we
> are dealing with a single page.
>
> Fixes: 2bcf018340cb ("s390x/tcg: low-address protection support")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> accel/tcg/cputlb.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index f90f4312ea..a46f3a654d 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -2248,7 +2248,7 @@ store_helper_unaligned(CPUArchState *env, target_ulong addr, uint64_t val,
> const size_t tlb_off = offsetof(CPUTLBEntry, addr_write);
> uintptr_t index, index2;
> CPUTLBEntry *entry, *entry2;
> - target_ulong page2, tlb_addr, tlb_addr2;
> + target_ulong page1, page2, tlb_addr, tlb_addr2;
> MemOpIdx oi;
> size_t size2;
> int i;
> @@ -2256,15 +2256,17 @@ store_helper_unaligned(CPUArchState *env, target_ulong addr, uint64_t val,
> /*
> * Ensure the second page is in the TLB. Note that the first page
> * is already guaranteed to be filled, and that the second page
> - * cannot evict the first.
> + * cannot evict the first. An exception to this rule is PAGE_WRITE_INV
> + * handling: the first page could have evicted itself.
> */
> + page1 = addr & TARGET_PAGE_MASK;
> page2 = (addr + size) & TARGET_PAGE_MASK;
> size2 = (addr + size) & ~TARGET_PAGE_MASK;
> index2 = tlb_index(env, mmu_idx, page2);
> entry2 = tlb_entry(env, mmu_idx, page2);
>
> tlb_addr2 = tlb_addr_write(entry2);
> - if (!tlb_hit_page(tlb_addr2, page2)) {
> + if (page1 != page2 && !tlb_hit_page(tlb_addr2, page2)) {
> if (!victim_tlb_hit(env, mmu_idx, index2, tlb_off, page2)) {
> tlb_fill(env_cpu(env), page2, size2, MMU_DATA_STORE,
> mmu_idx, retaddr);
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] hw/misc: Add mmio-debug-exit device
2022-07-11 18:56 [PATCH 0/3] accel/tcg: Fix unaligned stores to s390x low-address-protected lowcore Ilya Leoshkevich
2022-07-11 18:56 ` [PATCH 1/3] " Ilya Leoshkevich
@ 2022-07-11 18:56 ` Ilya Leoshkevich
2022-07-12 5:12 ` Richard Henderson
2022-07-11 18:56 ` [PATCH 3/3] tests/tcg/s390x: Test unaligned accesses to lowcore Ilya Leoshkevich
2 siblings, 1 reply; 11+ messages in thread
From: Ilya Leoshkevich @ 2022-07-11 18:56 UTC (permalink / raw)
To: Richard Henderson, Paolo Bonzini, Cornelia Huck, Thomas Huth,
Halil Pasic, Christian Borntraeger, Eric Farman,
David Hildenbrand
Cc: qemu-devel, qemu-s390x, Ilya Leoshkevich
System tests on x86 use isa-debug-exit device in order to signal
success or failure to the test runner. Unfortunately it's not easily
usable on other architectures, since a guest needs to access
address_space_io, which may not be as straightforward as on x86.
Also, it requires adding ISA bus, which an architecture might not
otherwise need.
Introduce mmio-debug-exit device, which has the same semantics, but is
triggered by writes to memory.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
hw/misc/Kconfig | 3 ++
hw/misc/debugexit_mmio.c | 80 ++++++++++++++++++++++++++++++++++++++++
hw/misc/meson.build | 1 +
hw/s390x/Kconfig | 1 +
4 files changed, 85 insertions(+)
create mode 100644 hw/misc/debugexit_mmio.c
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index cbabe9f78c..0f12735ef7 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -15,6 +15,9 @@ config ISA_DEBUG
bool
depends on ISA_BUS
+config MMIO_DEBUGEXIT
+ bool
+
config SGA
bool
depends on ISA_BUS
diff --git a/hw/misc/debugexit_mmio.c b/hw/misc/debugexit_mmio.c
new file mode 100644
index 0000000000..5e823cc01c
--- /dev/null
+++ b/hw/misc/debugexit_mmio.c
@@ -0,0 +1,80 @@
+/*
+ * Exit with status X when the guest writes X (little-endian) to a specified
+ * address. For testing purposes only.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "exec/address-spaces.h"
+#include "exec/memory.h"
+#include "hw/qdev-core.h"
+#include "hw/qdev-properties.h"
+
+#define TYPE_MMIO_DEBUG_EXIT_DEVICE "mmio-debug-exit"
+OBJECT_DECLARE_SIMPLE_TYPE(MMIODebugExitState, MMIO_DEBUG_EXIT_DEVICE)
+
+struct MMIODebugExitState {
+ DeviceState parent_obj;
+
+ uint32_t base;
+ uint32_t size;
+ MemoryRegion region;
+};
+
+static uint64_t mmio_debug_exit_read(void *opaque, hwaddr addr, unsigned size)
+{
+ return 0;
+}
+
+static void mmio_debug_exit_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned width)
+{
+ exit(val);
+}
+
+static const MemoryRegionOps mmio_debug_exit_ops = {
+ .read = mmio_debug_exit_read,
+ .write = mmio_debug_exit_write,
+ .valid.min_access_size = 1,
+ .valid.max_access_size = 8,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void mmio_debug_exit_realizefn(DeviceState *d, Error **errp)
+{
+ MMIODebugExitState *s = MMIO_DEBUG_EXIT_DEVICE(d);
+
+ memory_region_init_io(&s->region, OBJECT(s), &mmio_debug_exit_ops, s,
+ TYPE_MMIO_DEBUG_EXIT_DEVICE, s->size);
+ memory_region_add_subregion(get_system_memory(), s->base, &s->region);
+}
+
+static Property mmio_debug_exit_properties[] = {
+ DEFINE_PROP_UINT32("base", MMIODebugExitState, base, 0),
+ DEFINE_PROP_UINT32("size", MMIODebugExitState, size, 1),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void mmio_debug_exit_class_initfn(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = mmio_debug_exit_realizefn;
+ device_class_set_props(dc, mmio_debug_exit_properties);
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static const TypeInfo mmio_debug_exit_info = {
+ .name = TYPE_MMIO_DEBUG_EXIT_DEVICE,
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(MMIODebugExitState),
+ .class_init = mmio_debug_exit_class_initfn,
+};
+
+static void mmio_debug_exit_register_types(void)
+{
+ type_register_static(&mmio_debug_exit_info);
+}
+
+type_init(mmio_debug_exit_register_types)
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 95268eddc0..1d2a1067dc 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_APPLESMC', if_true: files('applesmc.c'))
softmmu_ss.add(when: 'CONFIG_EDU', if_true: files('edu.c'))
softmmu_ss.add(when: 'CONFIG_FW_CFG_DMA', if_true: files('vmcoreinfo.c'))
softmmu_ss.add(when: 'CONFIG_ISA_DEBUG', if_true: files('debugexit.c'))
+softmmu_ss.add(when: 'CONFIG_MMIO_DEBUGEXIT', if_true: files('debugexit_mmio.c'))
softmmu_ss.add(when: 'CONFIG_ISA_TESTDEV', if_true: files('pc-testdev.c'))
softmmu_ss.add(when: 'CONFIG_PCA9552', if_true: files('pca9552.c'))
softmmu_ss.add(when: 'CONFIG_PCI_TESTDEV', if_true: files('pci-testdev.c'))
diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig
index 5e7d8a2bae..9223715dcc 100644
--- a/hw/s390x/Kconfig
+++ b/hw/s390x/Kconfig
@@ -5,6 +5,7 @@ config S390_CCW_VIRTIO
imply VFIO_AP
imply VFIO_CCW
imply WDT_DIAG288
+ imply MMIO_DEBUGEXIT
select PCI
select S390_FLIC
select SCLPCONSOLE
--
2.35.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] hw/misc: Add mmio-debug-exit device
2022-07-11 18:56 ` [PATCH 2/3] hw/misc: Add mmio-debug-exit device Ilya Leoshkevich
@ 2022-07-12 5:12 ` Richard Henderson
2022-07-12 9:52 ` Ilya Leoshkevich
0 siblings, 1 reply; 11+ messages in thread
From: Richard Henderson @ 2022-07-12 5:12 UTC (permalink / raw)
To: Ilya Leoshkevich, Paolo Bonzini, Cornelia Huck, Thomas Huth,
Halil Pasic, Christian Borntraeger, Eric Farman,
David Hildenbrand
Cc: qemu-devel, qemu-s390x
On 7/12/22 00:26, Ilya Leoshkevich wrote:
> System tests on x86 use isa-debug-exit device in order to signal
> success or failure to the test runner. Unfortunately it's not easily
> usable on other architectures, since a guest needs to access
> address_space_io, which may not be as straightforward as on x86.
> Also, it requires adding ISA bus, which an architecture might not
> otherwise need.
>
> Introduce mmio-debug-exit device, which has the same semantics, but is
> triggered by writes to memory.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
You shouldn't need this for s390x, as there are already (at least) two other paths to
qemu_system_shutdown_request.
E.g. SIGP, which has a stop option.
r~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] hw/misc: Add mmio-debug-exit device
2022-07-12 5:12 ` Richard Henderson
@ 2022-07-12 9:52 ` Ilya Leoshkevich
2022-07-12 10:08 ` David Hildenbrand
2022-07-12 10:08 ` David Hildenbrand
0 siblings, 2 replies; 11+ messages in thread
From: Ilya Leoshkevich @ 2022-07-12 9:52 UTC (permalink / raw)
To: Richard Henderson, Paolo Bonzini, Cornelia Huck, Thomas Huth,
Halil Pasic, Christian Borntraeger, Eric Farman,
David Hildenbrand
Cc: qemu-devel, qemu-s390x
On Tue, 2022-07-12 at 10:42 +0530, Richard Henderson wrote:
> On 7/12/22 00:26, Ilya Leoshkevich wrote:
> > System tests on x86 use isa-debug-exit device in order to signal
> > success or failure to the test runner. Unfortunately it's not
> > easily
> > usable on other architectures, since a guest needs to access
> > address_space_io, which may not be as straightforward as on x86.
> > Also, it requires adding ISA bus, which an architecture might not
> > otherwise need.
> >
> > Introduce mmio-debug-exit device, which has the same semantics, but
> > is
> > triggered by writes to memory.
> >
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>
> You shouldn't need this for s390x, as there are already (at least)
> two other paths to
> qemu_system_shutdown_request.
>
> E.g. SIGP, which has a stop option.
>
>
> r~
>
I would normally use lpswe + disabled wait, but this always gives me
exit status code 0, which doesn't allow easily distinguishing between
success and failure.
Code-wise SIGP seems to do roughly the same thing, and a quick
experiment with:
lgfi %r4,-1
lgfi %r5,-1
larl %r6,_cpuaddr
stap 0(%r6)
lh %r6,0(%r6)
nilh %r6,0
sigp %r4,%r6,5
_cpuaddr: .short 0
confirmed that we get exit status code 0 as well.
Best regards,
Ilya
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] hw/misc: Add mmio-debug-exit device
2022-07-12 9:52 ` Ilya Leoshkevich
@ 2022-07-12 10:08 ` David Hildenbrand
2022-07-12 10:08 ` David Hildenbrand
1 sibling, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2022-07-12 10:08 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson, Paolo Bonzini, Cornelia Huck,
Thomas Huth, Halil Pasic, Christian Borntraeger, Eric Farman
Cc: qemu-devel, qemu-s390x
On 12.07.22 11:52, Ilya Leoshkevich wrote:
> On Tue, 2022-07-12 at 10:42 +0530, Richard Henderson wrote:
>> On 7/12/22 00:26, Ilya Leoshkevich wrote:
>>> System tests on x86 use isa-debug-exit device in order to signal
>>> success or failure to the test runner. Unfortunately it's not
>>> easily
>>> usable on other architectures, since a guest needs to access
>>> address_space_io, which may not be as straightforward as on x86.
>>> Also, it requires adding ISA bus, which an architecture might not
>>> otherwise need.
>>>
>>> Introduce mmio-debug-exit device, which has the same semantics, but
>>> is
>>> triggered by writes to memory.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>
>> You shouldn't need this for s390x, as there are already (at least)
>> two other paths to
>> qemu_system_shutdown_request.
>>
>> E.g. SIGP, which has a stop option.
>>
>>
>> r~
>>
>
> I would normally use lpswe + disabled wait, but this always gives me
> exit status code 0, which doesn't allow easily distinguishing between
> success and failure.
>
> Code-wise SIGP seems to do roughly the same thing, and a quick
> experiment with:
>
> lgfi %r4,-1
> lgfi %r5,-1
> larl %r6,_cpuaddr
> stap 0(%r6)
> lh %r6,0(%r6)
> nilh %r6,0
> sigp %r4,%r6,5
> _cpuaddr: .short 0
>
> confirmed that we get exit status code 0 as well.
disabled wait should trigger a qemu_system_guest_panicked().
But "panic_action == PANIC_ACTION_SHUTDOWN" seems to only make
qemu_main_loop() return with main_loop_should_exit() == true.
main/qemu_main will always return 0.
We could return != 0 on guest panic, but not sure if that could break
existing scripts. We'd need a new QEMU toggle for that most probably ...
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] hw/misc: Add mmio-debug-exit device
2022-07-12 9:52 ` Ilya Leoshkevich
2022-07-12 10:08 ` David Hildenbrand
@ 2022-07-12 10:08 ` David Hildenbrand
2022-07-12 10:30 ` Ilya Leoshkevich
1 sibling, 1 reply; 11+ messages in thread
From: David Hildenbrand @ 2022-07-12 10:08 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson, Paolo Bonzini, Cornelia Huck,
Thomas Huth, Halil Pasic, Christian Borntraeger, Eric Farman
Cc: qemu-devel, qemu-s390x
On 12.07.22 11:52, Ilya Leoshkevich wrote:
> On Tue, 2022-07-12 at 10:42 +0530, Richard Henderson wrote:
>> On 7/12/22 00:26, Ilya Leoshkevich wrote:
>>> System tests on x86 use isa-debug-exit device in order to signal
>>> success or failure to the test runner. Unfortunately it's not
>>> easily
>>> usable on other architectures, since a guest needs to access
>>> address_space_io, which may not be as straightforward as on x86.
>>> Also, it requires adding ISA bus, which an architecture might not
>>> otherwise need.
>>>
>>> Introduce mmio-debug-exit device, which has the same semantics, but
>>> is
>>> triggered by writes to memory.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>
>> You shouldn't need this for s390x, as there are already (at least)
>> two other paths to
>> qemu_system_shutdown_request.
>>
>> E.g. SIGP, which has a stop option.
>>
>>
>> r~
>>
>
> I would normally use lpswe + disabled wait, but this always gives me
> exit status code 0, which doesn't allow easily distinguishing between
> success and failure.
>
> Code-wise SIGP seems to do roughly the same thing, and a quick
> experiment with:
>
> lgfi %r4,-1
> lgfi %r5,-1
> larl %r6,_cpuaddr
> stap 0(%r6)
> lh %r6,0(%r6)
> nilh %r6,0
> sigp %r4,%r6,5
> _cpuaddr: .short 0
>
> confirmed that we get exit status code 0 as well.
disabled wait should trigger a qemu_system_guest_panicked().
But "panic_action == PANIC_ACTION_SHUTDOWN" seems to only make
qemu_main_loop() return with main_loop_should_exit() == true.
main/qemu_main will always return 0.
We could return != 0 on guest panic, but not sure if that could break
existing scripts. We'd need a new QEMU toggle for that most probably ...
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] hw/misc: Add mmio-debug-exit device
2022-07-12 10:08 ` David Hildenbrand
@ 2022-07-12 10:30 ` Ilya Leoshkevich
0 siblings, 0 replies; 11+ messages in thread
From: Ilya Leoshkevich @ 2022-07-12 10:30 UTC (permalink / raw)
To: David Hildenbrand, Richard Henderson, Paolo Bonzini,
Cornelia Huck, Thomas Huth, Halil Pasic, Christian Borntraeger,
Eric Farman
Cc: qemu-devel, qemu-s390x
On Tue, 2022-07-12 at 12:08 +0200, David Hildenbrand wrote:
> On 12.07.22 11:52, Ilya Leoshkevich wrote:
> > On Tue, 2022-07-12 at 10:42 +0530, Richard Henderson wrote:
> > > On 7/12/22 00:26, Ilya Leoshkevich wrote:
> > > > System tests on x86 use isa-debug-exit device in order to
> > > > signal
> > > > success or failure to the test runner. Unfortunately it's not
> > > > easily
> > > > usable on other architectures, since a guest needs to access
> > > > address_space_io, which may not be as straightforward as on
> > > > x86.
> > > > Also, it requires adding ISA bus, which an architecture might
> > > > not
> > > > otherwise need.
> > > >
> > > > Introduce mmio-debug-exit device, which has the same semantics,
> > > > but
> > > > is
> > > > triggered by writes to memory.
> > > >
> > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > >
> > > You shouldn't need this for s390x, as there are already (at
> > > least)
> > > two other paths to
> > > qemu_system_shutdown_request.
> > >
> > > E.g. SIGP, which has a stop option.
> > >
> > >
> > > r~
> > >
> >
> > I would normally use lpswe + disabled wait, but this always gives
> > me
> > exit status code 0, which doesn't allow easily distinguishing
> > between
> > success and failure.
> >
> > Code-wise SIGP seems to do roughly the same thing, and a quick
> > experiment with:
> >
> > lgfi %r4,-1
> > lgfi %r5,-1
> > larl %r6,_cpuaddr
> > stap 0(%r6)
> > lh %r6,0(%r6)
> > nilh %r6,0
> > sigp %r4,%r6,5
> > _cpuaddr: .short 0
> >
> > confirmed that we get exit status code 0 as well.
>
> disabled wait should trigger a qemu_system_guest_panicked().
>
> But "panic_action == PANIC_ACTION_SHUTDOWN" seems to only make
> qemu_main_loop() return with main_loop_should_exit() == true.
>
> main/qemu_main will always return 0.
>
> We could return != 0 on guest panic, but not sure if that could break
> existing scripts. We'd need a new QEMU toggle for that most probably
> ...
>
I wonder if a device is a cleaner way to solve this? It may be used on
all architectures, so there is no need to invent per-architecture way
to exit with a specific code. Maybe we can even replace Intel's
debugexit with it.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] tests/tcg/s390x: Test unaligned accesses to lowcore
2022-07-11 18:56 [PATCH 0/3] accel/tcg: Fix unaligned stores to s390x low-address-protected lowcore Ilya Leoshkevich
2022-07-11 18:56 ` [PATCH 1/3] " Ilya Leoshkevich
2022-07-11 18:56 ` [PATCH 2/3] hw/misc: Add mmio-debug-exit device Ilya Leoshkevich
@ 2022-07-11 18:56 ` Ilya Leoshkevich
2 siblings, 0 replies; 11+ messages in thread
From: Ilya Leoshkevich @ 2022-07-11 18:56 UTC (permalink / raw)
To: Richard Henderson, Paolo Bonzini, Cornelia Huck, Thomas Huth,
Halil Pasic, Christian Borntraeger, Eric Farman,
David Hildenbrand
Cc: qemu-devel, qemu-s390x, Ilya Leoshkevich
Add a small test to avoid regressions.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tests/tcg/s390x/Makefile.softmmu-target | 9 +++++++++
tests/tcg/s390x/unaligned-lowcore.S | 24 ++++++++++++++++++++++++
2 files changed, 33 insertions(+)
create mode 100644 tests/tcg/s390x/Makefile.softmmu-target
create mode 100644 tests/tcg/s390x/unaligned-lowcore.S
diff --git a/tests/tcg/s390x/Makefile.softmmu-target b/tests/tcg/s390x/Makefile.softmmu-target
new file mode 100644
index 0000000000..fdee4a7616
--- /dev/null
+++ b/tests/tcg/s390x/Makefile.softmmu-target
@@ -0,0 +1,9 @@
+S390X_SRC=$(SRC_PATH)/tests/tcg/s390x
+VPATH+=$(S390X_SRC)
+QEMU_OPTS=-device mmio-debug-exit,base=0x2000 -kernel
+
+%: %.S
+ $(CC) -march=z13 -m64 -nostartfiles -static -Wl,-Ttext=0 \
+ -Wl,--build-id=none $< -o $@
+
+TESTS += unaligned-lowcore
diff --git a/tests/tcg/s390x/unaligned-lowcore.S b/tests/tcg/s390x/unaligned-lowcore.S
new file mode 100644
index 0000000000..2b34c3d5ef
--- /dev/null
+++ b/tests/tcg/s390x/unaligned-lowcore.S
@@ -0,0 +1,24 @@
+ .org 0x1D0 /* Program new PSW */
+ .quad 0x180000000, _pgm /* BA, EA */
+ .org 0x200 /* lowcore padding */
+
+ .globl _start
+_start:
+ lctlg %c0,%c0,_c0
+ vst %v0,_unaligned
+ mviy _exit,0
+
+_pgm:
+ mviy _exit,1
+
+ .align 8
+_c0:
+ .quad 0x10060000 /* lowcore protection, AFP, VX */
+
+ .byte 0
+_unaligned:
+ .octa 0
+
+ .org 0x2000 /* mmio-debug-exit base */
+_exit:
+ .byte 0
--
2.35.3
^ permalink raw reply related [flat|nested] 11+ messages in thread