From 759a5d3be0ca447514bc774f97218d3a0a8ed654 Mon Sep 17 00:00:00 2001 From: Cornelia Huck Date: Mon, 5 Jul 2021 18:39:51 +0200 Subject: [PATCH 01/20] vfio-ccw: forward halt/clear errors hsch and csch basically have two parts: execute the command, and perform the halt/clear function. For fully emulated subchannels, it is pretty clear how it will work: check the subchannel state, and actually 'perform the halt/clear function' and set cc 0 if everything looks good. For passthrough subchannels, some of the checking is done within QEMU, but some has to be done within the kernel. QEMU's subchannel state may be such that we can perform the async function, but the kernel may still get a cc != 0 when it is actually executing the instruction. In that case, we need to set the condition actually encountered by the kernel; if we set cc 0 on error, we would actually need to inject an interrupt as well. Signed-off-by: Cornelia Huck Reviewed-by: Matthew Rosato Tested-by: Jared Rossi Message-Id: <20210705163952.736020-2-cohuck@redhat.com> Signed-off-by: Thomas Huth --- hw/s390x/css.c | 38 ++++++++++++++++++++++++++++++++++---- hw/vfio/ccw.c | 4 ++-- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 133ddea575..7d9523f811 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -1206,23 +1206,53 @@ static void sch_handle_start_func_virtual(SubchDev *sch) } -static void sch_handle_halt_func_passthrough(SubchDev *sch) +static IOInstEnding sch_handle_halt_func_passthrough(SubchDev *sch) { int ret; ret = s390_ccw_halt(sch); if (ret == -ENOSYS) { sch_handle_halt_func(sch); + return IOINST_CC_EXPECTED; + } + /* + * Some conditions may have been detected prior to starting the halt + * function; map them to the correct cc. + * Note that we map both -ENODEV and -EACCES to cc 3 (there's not really + * anything else we can do.) + */ + switch (ret) { + case -EBUSY: + return IOINST_CC_BUSY; + case -ENODEV: + case -EACCES: + return IOINST_CC_NOT_OPERATIONAL; + default: + return IOINST_CC_EXPECTED; } } -static void sch_handle_clear_func_passthrough(SubchDev *sch) +static IOInstEnding sch_handle_clear_func_passthrough(SubchDev *sch) { int ret; ret = s390_ccw_clear(sch); if (ret == -ENOSYS) { sch_handle_clear_func(sch); + return IOINST_CC_EXPECTED; + } + /* + * Some conditions may have been detected prior to starting the clear + * function; map them to the correct cc. + * Note that we map both -ENODEV and -EACCES to cc 3 (there's not really + * anything else we can do.) + */ + switch (ret) { + case -ENODEV: + case -EACCES: + return IOINST_CC_NOT_OPERATIONAL; + default: + return IOINST_CC_EXPECTED; } } @@ -1265,9 +1295,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) SCHIB *schib = &sch->curr_status; if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) { - sch_handle_clear_func_passthrough(sch); + return sch_handle_clear_func_passthrough(sch); } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) { - sch_handle_halt_func_passthrough(sch); + return sch_handle_halt_func_passthrough(sch); } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) { return sch_handle_start_func_passthrough(sch); } diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 000992fb9f..0354737666 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -199,7 +199,7 @@ again: case 0: case -ENODEV: case -EACCES: - return 0; + return ret; case -EFAULT: default: sch_gen_unit_exception(sch); @@ -240,7 +240,7 @@ again: case -EBUSY: case -ENODEV: case -EACCES: - return 0; + return ret; case -EFAULT: default: sch_gen_unit_exception(sch); From 89c6722da24761d14db1b19250919c8004758ba5 Mon Sep 17 00:00:00 2001 From: Cornelia Huck Date: Mon, 5 Jul 2021 18:39:52 +0200 Subject: [PATCH 02/20] css: fix actl handling for unit exceptions When a subchannel becomes pending with unit exception, start pending (and for that matter, halt or clear pending) are not removed in the actl. Device active and subchannel active, however, are (due to the subchannel becoming status pending with primary respectively secondary status). The other conditions in the actl are only cleared when the guest executes tsch on the subchannel. Signed-off-by: Cornelia Huck Reviewed-by: Matthew Rosato Tested-by: Jared Rossi Message-Id: <20210705163952.736020-3-cohuck@redhat.com> Signed-off-by: Thomas Huth --- include/hw/s390x/css.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index 10ed1df1bb..75e5381613 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -146,7 +146,8 @@ struct SubchDev { static inline void sch_gen_unit_exception(SubchDev *sch) { - sch->curr_status.scsw.ctrl &= ~SCSW_ACTL_START_PEND; + sch->curr_status.scsw.ctrl &= ~(SCSW_ACTL_DEVICE_ACTIVE | + SCSW_ACTL_SUBCH_ACTIVE); sch->curr_status.scsw.ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY | SCSW_STCTL_ALERT | From e7f8a3aae271d279edb1c0c318c6d83b0b3924ce Mon Sep 17 00:00:00 2001 From: Ilya Leoshkevich Date: Thu, 5 Aug 2021 00:51:46 +0200 Subject: [PATCH 03/20] tests/tcg/s390x: Test SIGILL and SIGSEGV handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verify that s390x-specific uc_mcontext.psw.addr is reported correctly and that signal handling interacts properly with debugging. Signed-off-by: Ilya Leoshkevich Acked-by: Alex Bennée Acked-by: David Hildenbrand Message-Id: <20210804225146.154513-1-iii@linux.ibm.com> Signed-off-by: Cornelia Huck Signed-off-by: Thomas Huth --- tests/tcg/s390x/Makefile.target | 17 +- tests/tcg/s390x/gdbstub/test-signals-s390x.py | 76 ++++++++ tests/tcg/s390x/signals-s390x.c | 165 ++++++++++++++++++ 3 files changed, 257 insertions(+), 1 deletion(-) create mode 100644 tests/tcg/s390x/gdbstub/test-signals-s390x.py create mode 100644 tests/tcg/s390x/signals-s390x.c diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target index bd084c7840..cc64dd32d2 100644 --- a/tests/tcg/s390x/Makefile.target +++ b/tests/tcg/s390x/Makefile.target @@ -1,4 +1,5 @@ -VPATH+=$(SRC_PATH)/tests/tcg/s390x +S390X_SRC=$(SRC_PATH)/tests/tcg/s390x +VPATH+=$(S390X_SRC) CFLAGS+=-march=zEC12 -m64 TESTS+=hello-s390x TESTS+=csst @@ -9,3 +10,17 @@ TESTS+=pack TESTS+=mvo TESTS+=mvc TESTS+=trap +TESTS+=signals-s390x + +ifneq ($(HAVE_GDB_BIN),) +GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py + +run-gdbstub-signals-s390x: signals-s390x + $(call run-test, $@, $(GDB_SCRIPT) \ + --gdb $(HAVE_GDB_BIN) \ + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ + --bin $< --test $(S390X_SRC)/gdbstub/test-signals-s390x.py, \ + "mixing signals and debugging on s390x") + +EXTRA_RUNS += run-gdbstub-signals-s390x +endif diff --git a/tests/tcg/s390x/gdbstub/test-signals-s390x.py b/tests/tcg/s390x/gdbstub/test-signals-s390x.py new file mode 100644 index 0000000000..80a284b475 --- /dev/null +++ b/tests/tcg/s390x/gdbstub/test-signals-s390x.py @@ -0,0 +1,76 @@ +from __future__ import print_function + +# +# Test that signals and debugging mix well together on s390x. +# +# This is launched via tests/guest-debug/run-test.py +# + +import gdb +import sys + +failcount = 0 + + +def report(cond, msg): + """Report success/fail of test""" + if cond: + print("PASS: %s" % (msg)) + else: + print("FAIL: %s" % (msg)) + global failcount + failcount += 1 + + +def run_test(): + """Run through the tests one by one""" + illegal_op = gdb.Breakpoint("illegal_op") + stg = gdb.Breakpoint("stg") + mvc_8 = gdb.Breakpoint("mvc_8") + + # Expect the following events: + # 1x illegal_op breakpoint + # 2x stg breakpoint, segv, breakpoint + # 2x mvc_8 breakpoint, segv, breakpoint + for _ in range(14): + gdb.execute("c") + report(illegal_op.hit_count == 1, "illegal_op.hit_count == 1") + report(stg.hit_count == 4, "stg.hit_count == 4") + report(mvc_8.hit_count == 4, "mvc_8.hit_count == 4") + + # The test must succeed. + gdb.Breakpoint("_exit") + gdb.execute("c") + status = int(gdb.parse_and_eval("$r2")) + report(status == 0, "status == 0"); + + +# +# This runs as the script it sourced (via -x, via run-test.py) +# +try: + inferior = gdb.selected_inferior() + arch = inferior.architecture() + print("ATTACHED: %s" % arch.name()) +except (gdb.error, AttributeError): + print("SKIPPING (not connected)", file=sys.stderr) + exit(0) + +if gdb.parse_and_eval("$pc") == 0: + print("SKIP: PC not set") + exit(0) + +try: + # These are not very useful in scripts + gdb.execute("set pagination off") + gdb.execute("set confirm off") + + # Run the actual tests + run_test() +except (gdb.error): + print("GDB Exception: %s" % (sys.exc_info()[0])) + failcount += 1 + pass + +print("All tests complete: %d failures" % failcount) +exit(failcount) diff --git a/tests/tcg/s390x/signals-s390x.c b/tests/tcg/s390x/signals-s390x.c new file mode 100644 index 0000000000..dc2f8ee59a --- /dev/null +++ b/tests/tcg/s390x/signals-s390x.c @@ -0,0 +1,165 @@ +#include +#include +#include +#include +#include +#include + +/* + * Various instructions that generate SIGILL and SIGSEGV. They could have been + * defined in a separate .s file, but this would complicate the build, so the + * inline asm is used instead. + */ + +void illegal_op(void); +void after_illegal_op(void); +asm(".globl\tillegal_op\n" + "illegal_op:\t.byte\t0x00,0x00\n" + "\t.globl\tafter_illegal_op\n" + "after_illegal_op:\tbr\t%r14"); + +void stg(void *dst, unsigned long src); +asm(".globl\tstg\n" + "stg:\tstg\t%r3,0(%r2)\n" + "\tbr\t%r14"); + +void mvc_8(void *dst, void *src); +asm(".globl\tmvc_8\n" + "mvc_8:\tmvc\t0(8,%r2),0(%r3)\n" + "\tbr\t%r14"); + +static void safe_puts(const char *s) +{ + write(0, s, strlen(s)); + write(0, "\n", 1); +} + +enum exception { + exception_operation, + exception_translation, + exception_protection, +}; + +static struct { + int sig; + void *addr; + unsigned long psw_addr; + enum exception exception; +} expected; + +static void handle_signal(int sig, siginfo_t *info, void *ucontext) +{ + void *page; + int err; + + if (sig != expected.sig) { + safe_puts("[ FAILED ] wrong signal"); + _exit(1); + } + + if (info->si_addr != expected.addr) { + safe_puts("[ FAILED ] wrong si_addr"); + _exit(1); + } + + if (((ucontext_t *)ucontext)->uc_mcontext.psw.addr != expected.psw_addr) { + safe_puts("[ FAILED ] wrong psw.addr"); + _exit(1); + } + + switch (expected.exception) { + case exception_translation: + page = mmap(expected.addr, 4096, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); + if (page != expected.addr) { + safe_puts("[ FAILED ] mmap() failed"); + _exit(1); + } + break; + case exception_protection: + err = mprotect(expected.addr, 4096, PROT_READ | PROT_WRITE); + if (err != 0) { + safe_puts("[ FAILED ] mprotect() failed"); + _exit(1); + } + break; + default: + break; + } +} + +static void check_sigsegv(void *func, enum exception exception, + unsigned long val) +{ + int prot; + unsigned long *page; + unsigned long *addr; + int err; + + prot = exception == exception_translation ? PROT_NONE : PROT_READ; + page = mmap(NULL, 4096, prot, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + assert(page != MAP_FAILED); + if (exception == exception_translation) { + /* Hopefully nothing will be mapped at this address. */ + err = munmap(page, 4096); + assert(err == 0); + } + addr = page + (val & 0x1ff); + + expected.sig = SIGSEGV; + expected.addr = page; + expected.psw_addr = (unsigned long)func; + expected.exception = exception; + if (func == stg) { + stg(addr, val); + } else { + assert(func == mvc_8); + mvc_8(addr, &val); + } + assert(*addr == val); + + err = munmap(page, 4096); + assert(err == 0); +} + +int main(void) +{ + struct sigaction act; + int err; + + memset(&act, 0, sizeof(act)); + act.sa_sigaction = handle_signal; + act.sa_flags = SA_SIGINFO; + err = sigaction(SIGILL, &act, NULL); + assert(err == 0); + err = sigaction(SIGSEGV, &act, NULL); + assert(err == 0); + + safe_puts("[ RUN ] Operation exception"); + expected.sig = SIGILL; + expected.addr = illegal_op; + expected.psw_addr = (unsigned long)after_illegal_op; + expected.exception = exception_operation; + illegal_op(); + safe_puts("[ OK ]"); + + safe_puts("[ RUN ] Translation exception from stg"); + check_sigsegv(stg, exception_translation, 42); + safe_puts("[ OK ]"); + + safe_puts("[ RUN ] Translation exception from mvc"); + check_sigsegv(mvc_8, exception_translation, 4242); + safe_puts("[ OK ]"); + + safe_puts("[ RUN ] Protection exception from stg"); + check_sigsegv(stg, exception_protection, 424242); + safe_puts("[ OK ]"); + + safe_puts("[ RUN ] Protection exception from mvc"); + check_sigsegv(mvc_8, exception_protection, 42424242); + safe_puts("[ OK ]"); + + safe_puts("[ PASSED ]"); + + _exit(0); +} From 6b01606f0e35827fb7b608b9e56e63ed4b88a0a7 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Thu, 5 Aug 2021 14:59:38 +0200 Subject: [PATCH 04/20] s390x/tcg: fix and optimize SPX (SET PREFIX) We not only invalidate the translation of the range 0x0-0x2000, we also invalidate the translation of the new prefix range and the translation of the old prefix range -- because real2abs would return different results for all of these ranges when changing the prefix location. This fixes the kvm-unit-tests "edat" test that just hangs before this patch because we end up clearing the new prefix area instead of the old prefix area. While at it, let's not do anything in case the prefix doesn't change. Cc: Richard Henderson Cc: David Hildenbrand Cc: Cornelia Huck Cc: Thomas Huth Cc: Claudio Imbrenda Cc: qemu-s390x@nongnu.org Signed-off-by: David Hildenbrand Reviewed-by: Thomas Huth Reviewed-by: Claudio Imbrenda Message-Id: <20210805125938.74034-1-david@redhat.com> Signed-off-by: Cornelia Huck Signed-off-by: Thomas Huth --- target/s390x/tcg/misc_helper.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c index 33e6999e15..aab9c47747 100644 --- a/target/s390x/tcg/misc_helper.c +++ b/target/s390x/tcg/misc_helper.c @@ -151,13 +151,26 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num) /* Set Prefix */ void HELPER(spx)(CPUS390XState *env, uint64_t a1) { + const uint32_t prefix = a1 & 0x7fffe000; + const uint32_t old_prefix = env->psa; CPUState *cs = env_cpu(env); - uint32_t prefix = a1 & 0x7fffe000; + + if (prefix == old_prefix) { + return; + } env->psa = prefix; HELPER_LOG("prefix: %#x\n", prefix); tlb_flush_page(cs, 0); tlb_flush_page(cs, TARGET_PAGE_SIZE); + if (prefix != 0) { + tlb_flush_page(cs, prefix); + tlb_flush_page(cs, prefix + TARGET_PAGE_SIZE); + } + if (old_prefix != 0) { + tlb_flush_page(cs, old_prefix); + tlb_flush_page(cs, old_prefix + TARGET_PAGE_SIZE); + } } static void update_ckc_timer(CPUS390XState *env) From 0dd05d0606c880200d1683364c5eb558898e50de Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Thu, 5 Aug 2021 16:37:53 +0200 Subject: [PATCH 05/20] s390x/ioinst: Fix wrong MSCH alignment check on little endian schib->pmcw.chars is 32bit, not 16bit. This fixes the kvm-unit-tests "css" test, which fails with: FAIL: Channel Subsystem: measurement block format1: Unaligned MB origin: Program interrupt: expected(21) == received(0) Because we end up not injecting an operand program exception. Fixes: a54b8ac340c2 ("css: SCHIB measurement block origin must be aligned") Cc: Halil Pasic Cc: Cornelia Huck Cc: Christian Borntraeger Cc: Richard Henderson Cc: Thomas Huth Cc: Pierre Morel Cc: qemu-s390x@nongnu.org Signed-off-by: David Hildenbrand Reviewed-by: Halil Pasic Reviewed-by: Thomas Huth Reviewed-by: Pierre Morel Message-Id: <20210805143753.86520-1-david@redhat.com> Signed-off-by: Cornelia Huck Signed-off-by: Thomas Huth --- target/s390x/ioinst.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c index 4eb0a7a9f8..bdae5090bc 100644 --- a/target/s390x/ioinst.c +++ b/target/s390x/ioinst.c @@ -123,7 +123,7 @@ static int ioinst_schib_valid(SCHIB *schib) } /* for MB format 1 bits 26-31 of word 11 must be 0 */ /* MBA uses words 10 and 11, it means align on 2**6 */ - if ((be16_to_cpu(schib->pmcw.chars) & PMCW_CHARS_MASK_MBFC) && + if ((be32_to_cpu(schib->pmcw.chars) & PMCW_CHARS_MASK_MBFC) && (be64_to_cpu(schib->mba) & 0x03fUL)) { return 0; } From 634a0b51cb25800ebc585862121f764d02de7a28 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:02 +0200 Subject: [PATCH 06/20] s390x/tcg: wrap address for RRBE Let's wrap the address just like for SSKE and ISKE. Signed-off-by: David Hildenbrand Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-2-david@redhat.com> Signed-off-by: Thomas Huth --- 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; } /* From fe00c705fec7bf20f5832fd6d8245fd554097cfc Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:03 +0200 Subject: [PATCH 07/20] s390x/tcg: fix ignoring bit 63 when setting the storage key in SSKE 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 Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-3-david@redhat.com> Signed-off-by: Thomas Huth --- 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 From 06d8a10a70b7ef14ebf88b874858f73f2dee1109 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:04 +0200 Subject: [PATCH 08/20] s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE 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 Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-4-david@redhat.com> Signed-off-by: Thomas Huth --- 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; } From eaa0feea7532429c3a9bec357ec4f77cb156fab0 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:05 +0200 Subject: [PATCH 09/20] s390x/tcg: check for addressing exceptions for RRBE, SSKE and ISKE 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 Acked-by: Thomas Huth Message-Id: <20210903155514.44772-5-david@redhat.com> Signed-off-by: Thomas Huth --- 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..a44a107374 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)) { + tcg_s390_program_interrupt(env, PGM_ADDRESSING, GETPC()); } 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)) { + tcg_s390_program_interrupt(env, PGM_ADDRESSING, GETPC()); } 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); + rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); + if (rc) { + trace_set_skeys_nonzero(rc); + } /* * As we can only flush by virtual address and not all the entries * that point to a physical address we have to flush the whole TLB. @@ -2224,15 +2230,15 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2) /* reset reference bit extended */ 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; + int rc; addr = mmu_real2abs(env, addr); - if (addr > ms->ram_size) { - return 0; + if (!mmu_absolute_addr_valid(addr, false)) { + tcg_s390_program_interrupt(env, PGM_ADDRESSING, GETPC()); } if (unlikely(!ss)) { @@ -2240,14 +2246,18 @@ uint32_t HELPER(rrbe)(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; } re = key & (SK_R | SK_C); key &= ~SK_R; - if (skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) { + rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); + if (rc) { + trace_set_skeys_nonzero(rc); return 0; } /* From e039992f9aa355bfb20d9797d4ce9eb04b7b0b2c Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:06 +0200 Subject: [PATCH 10/20] s390x/mmu_helper: no need to pass access type to mmu_translate_asce() The access type is unused since commit 81d7e3bc45 ("s390x/mmu: Inject DAT exceptions from a single place"). Signed-off-by: David Hildenbrand Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-6-david@redhat.com> Signed-off-by: Thomas Huth --- 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; } From e0b11f2df19c1e9341e9cec78429e45e5af5901b Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:07 +0200 Subject: [PATCH 11/20] s390x/mmu_helper: fixup mmu_translate() documentation Looks like we forgot to adjust documentation of one parameter. Signed-off-by: David Hildenbrand Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-7-david@redhat.com> Signed-off-by: Thomas Huth --- 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 asc address 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 exc true = inject a program check if a fault occurred + * @param tec the 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, From 390191c6f6e2e912f45bc312464d59b84ca12db3 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:08 +0200 Subject: [PATCH 12/20] s390x/mmu_helper: move address validation into mmu_translate*() 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. Signed-off-by: David Hildenbrand Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-8-david@redhat.com> Signed-off-by: Thomas Huth --- 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 asc address 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 %" PRIx64 "\n", - __func__, (uint64_t)raddr, (uint64_t)ms->ram_size); - excp = PGM_ADDRESSING; - tec = 0; /* unused */ - } - env->tlb_fill_exc = excp; env->tlb_fill_tec = tec; diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c index a44a107374..4f9f3e1f63 100644 --- a/target/s390x/tcg/mem_helper.c +++ b/target/s390x/tcg/mem_helper.c @@ -2455,7 +2455,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr) tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC()); } - exc = mmu_translate(env, addr, 0, asc, &ret, &flags, &tec); + exc = mmu_translate(env, addr, MMU_S390_LRA, asc, &ret, &flags, &tec); if (exc) { cc = 3; ret = exc | 0x80000000; From 380ac2bcce466f3b8f93eb749c7e85bfcf476cd6 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:09 +0200 Subject: [PATCH 13/20] s390x/mmu_helper: avoid setting the storage key if nothing changed Avoid setting the key if nothing changed. Signed-off-by: David Hildenbrand Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-9-david@redhat.com> Signed-off-by: Thomas Huth --- 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); + } } } From 67db1306a253b87ea4a1fc4e4fc13758b74aa8b2 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:10 +0200 Subject: [PATCH 14/20] hw/s390x/s390-skeys: use memory mapping to detect which storage keys to migrate 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. Signed-off-by: David Hildenbrand Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-10-david@redhat.com> Signed-off-by: Thomas Huth --- hw/s390x/s390-skeys.c | 66 +++++++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 25 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); + guest_phys_blocks_init(&guest_phys_blocks); + guest_phys_blocks_append(&guest_phys_blocks); - while (pages_left) { - read_count = MIN(pages_left, S390_SKEYS_BUFFER_SIZE); + /* 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)); - 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; + 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) From 78eedc60aa01df7fa302b882f8f7e06901d24123 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:11 +0200 Subject: [PATCH 15/20] hw/s390x/s390-skeys: use memory mapping to detect which storage keys to dump Handle it similar to migration. Assert that we're holding the BQL, to make sure we don't see concurrent modifications. Signed-off-by: David Hildenbrand Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-11-david@redhat.com> Signed-off-by: Thomas Huth --- hw/s390x/s390-skeys.c | 52 ++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 21 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)); + + gfn = block->target_start / TARGET_PAGE_SIZE; + pages = (block->target_end - block->target_start) / TARGET_PAGE_SIZE; + + 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; } - - /* write keys to stream */ - write_keys(f, buf, cur_gfn, cur_count, &lerr); - if (lerr) { - goto out_free; - } - - cur_gfn += cur_count; - handled_count += cur_count; } out_free: + guest_phys_blocks_free(&guest_phys_blocks); error_propagate(errp, lerr); g_free(buf); out: From 2162faf77ef3b2e17eb209756f3886adfcc8d9b1 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:12 +0200 Subject: [PATCH 16/20] hw/s390x/s390-skeys: check if an address is valid before dumping the key 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 Acked-by: Thomas Huth Message-Id: <20210903155514.44772-12-david@redhat.com> Signed-off-by: Thomas Huth --- 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) { From 5227b3260144657b48c2e215f1b38f0a5a7fbcac Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:13 +0200 Subject: [PATCH 17/20] hw/s390x/s390-skeys: rename skeys_enabled to skeys_are_enabled ... and make it return a bool instead. Signed-off-by: David Hildenbrand Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-13-david@redhat.com> Signed-off-by: Thomas Huth --- 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, From c35622387efe39214ccd14fab0f280b6c53f1654 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 3 Sep 2021 17:55:14 +0200 Subject: [PATCH 18/20] hw/s390x/s390-skeys: lazy storage key enablement under TCG 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 Reviewed-by: Thomas Huth Message-Id: <20210903155514.44772-14-david@redhat.com> Signed-off-by: Thomas Huth --- hw/s390x/s390-skeys.c | 71 +++++++++++++++++++++++++-------- include/hw/s390x/storage-keys.h | 63 +++++++++++++++++++++++++++++ target/s390x/mmu_helper.c | 8 ++++ target/s390x/tcg/mem_helper.c | 9 +++++ 4 files changed, 134 insertions(+), 17 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) -{ - QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj); - MachineState *machine = MACHINE(qdev_get_machine()); - - skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE; - skeys->keydata = g_malloc0(skeys->key_count); -} - static bool qemu_s390_skeys_are_enabled(S390SKeysState *ss) { - return true; + QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss); + + /* Lockless check is sufficient. */ + return !!skeys->keydata; +} + +static bool qemu_s390_enable_skeys(S390SKeysState *ss) +{ + 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_QEMU_S390_SKEYS, .parent = TYPE_S390_SKEYS, - .instance_init = qemu_s390_skeys_init, .instance_size = sizeof(QEMUS390SKeysState), .class_init = qemu_s390_skeys_class_init, .class_size = sizeof(S390SKeysClass), @@ -341,6 +370,14 @@ static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id) S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss); int ret = 0; + /* + * Make sure to lazy-enable if required to be done explicitly. No need to + * flush any TLB as the VM is not running yet. + */ + if (skeyclass->enable_skeys) { + skeyclass->enable_skeys(ss); + } + while (!ret) { ram_addr_t addr; int flags; diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h index eb091842c8..aa2ec2aae5 100644 --- a/include/hw/s390x/storage-keys.h +++ b/include/hw/s390x/storage-keys.h @@ -28,9 +28,72 @@ struct S390SKeysState { struct S390SKeysClass { DeviceClass parent_class; + + /** + * @skeys_are_enabled: + * + * Check whether storage keys are enabled. If not enabled, they were not + * enabled lazily either by the guest via a storage key instruction or + * by the host during migration. + * + * If disabled, everything not explicitly triggered by the guest, + * such as outgoing migration or dirty/change tracking, should not touch + * storage keys and should not lazily enable it. + * + * @ks: the #S390SKeysState + * + * Returns false if not enabled and true if enabled. + */ bool (*skeys_are_enabled)(S390SKeysState *ks); + + /** + * @enable_skeys: + * + * Lazily enable storage keys. If this function is not implemented, + * setting a storage key will lazily enable storage keys implicitly + * instead. TCG guests have to make sure to flush the TLB of all CPUs + * if storage keys were not enabled before this call. + * + * @ks: the #S390SKeysState + * + * Returns false if not enabled before this call, and true if already + * enabled. + */ + bool (*enable_skeys)(S390SKeysState *ks); + + /** + * @get_skeys: + * + * Get storage keys for the given PFN range. This call will fail if + * storage keys have not been lazily enabled yet. + * + * Callers have to validate that a GFN is valid before this call. + * + * @ks: the #S390SKeysState + * @start_gfn: the start GFN to get storage keys for + * @count: the number of storage keys to get + * @keys: the byte array where storage keys will be stored to + * + * Returns 0 on success, returns an error if getting a storage key failed. + */ int (*get_skeys)(S390SKeysState *ks, uint64_t start_gfn, uint64_t count, uint8_t *keys); + /** + * @set_skeys: + * + * Set storage keys for the given PFN range. This call will fail if + * storage keys have not been lazily enabled yet and implicit + * enablement is not supported. + * + * Callers have to validate that a GFN is valid before this call. + * + * @ks: the #S390SKeysState + * @start_gfn: the start GFN to set storage keys for + * @count: the number of storage keys to set + * @keys: the byte array where storage keys will be read from + * + * Returns 0 on success, returns an error if setting a storage key failed. + */ int (*set_skeys)(S390SKeysState *ks, uint64_t start_gfn, uint64_t count, uint8_t *keys); }; diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c index e2b372efd9..b04b57c235 100644 --- a/target/s390x/mmu_helper.c +++ b/target/s390x/mmu_helper.c @@ -313,6 +313,14 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags) skeyclass = S390_SKEYS_GET_CLASS(ss); } + /* + * Don't enable storage keys if they are still disabled, i.e., no actual + * storage key instruction was issued yet. + */ + if (!skeyclass->skeys_are_enabled(ss)) { + return; + } + /* * Whenever we create a new TLB entry, we set the storage key reference * bit. In case we allow write accesses, we set the storage key change diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c index 4f9f3e1f63..0bf775a37d 100644 --- a/target/s390x/tcg/mem_helper.c +++ b/target/s390x/tcg/mem_helper.c @@ -2186,6 +2186,9 @@ uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2) if (unlikely(!ss)) { ss = s390_get_skeys_device(); skeyclass = S390_SKEYS_GET_CLASS(ss); + if (skeyclass->enable_skeys && !skeyclass->enable_skeys(ss)) { + tlb_flush_all_cpus_synced(env_cpu(env)); + } } rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); @@ -2213,6 +2216,9 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2) if (unlikely(!ss)) { ss = s390_get_skeys_device(); skeyclass = S390_SKEYS_GET_CLASS(ss); + if (skeyclass->enable_skeys && !skeyclass->enable_skeys(ss)) { + tlb_flush_all_cpus_synced(env_cpu(env)); + } } key = r1 & 0xfe; @@ -2244,6 +2250,9 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2) if (unlikely(!ss)) { ss = s390_get_skeys_device(); skeyclass = S390_SKEYS_GET_CLASS(ss); + if (skeyclass->enable_skeys && !skeyclass->enable_skeys(ss)) { + tlb_flush_all_cpus_synced(env_cpu(env)); + } } rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); From ed3288ff8f8aab9b6602484dfcc09d5b59fb84fc Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 1 Sep 2021 14:58:00 +0200 Subject: [PATCH 19/20] s390x: Replace PAGE_SIZE, PAGE_SHIFT and PAGE_MASK MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PAGE_SIZE macro is causing trouble on Alpine Linux since it clashes with a macro from a system header there. We already have the TARGET_PAGE_SIZE, TARGET_PAGE_MASK and TARGET_PAGE_BITS macros in QEMU anyway, so let's simply replace the PAGE_SIZE, PAGE_MASK and PAGE_SHIFT macro with their TARGET_* counterparts. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/572 Message-Id: <20210901125800.611183-1-thuth@redhat.com> Reviewed-by: Halil Pasic Reviewed-by: Matthew Rosato Reviewed-by: Eric Farman Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: David Hildenbrand Signed-off-by: Thomas Huth --- hw/s390x/s390-pci-bus.c | 10 +++++----- hw/s390x/s390-pci-inst.c | 8 ++++---- hw/s390x/sclp.c | 2 +- include/hw/s390x/s390-pci-bus.h | 5 +---- 4 files changed, 11 insertions(+), 14 deletions(-) diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index 7db1c5943f..6c0225c3a0 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -330,7 +330,7 @@ static unsigned int calc_sx(dma_addr_t ptr) static unsigned int calc_px(dma_addr_t ptr) { - return ((unsigned long) ptr >> PAGE_SHIFT) & ZPCI_PT_MASK; + return ((unsigned long) ptr >> TARGET_PAGE_BITS) & ZPCI_PT_MASK; } static uint64_t get_rt_sto(uint64_t entry) @@ -506,7 +506,7 @@ uint16_t s390_guest_io_table_walk(uint64_t g_iota, hwaddr addr, int8_t ett = 1; uint16_t error = 0; - entry->iova = addr & PAGE_MASK; + entry->iova = addr & TARGET_PAGE_MASK; entry->translated_addr = 0; entry->perm = IOMMU_RW; @@ -526,7 +526,7 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr, { S390PCIIOMMU *iommu = container_of(mr, S390PCIIOMMU, iommu_mr); S390IOTLBEntry *entry; - uint64_t iova = addr & PAGE_MASK; + uint64_t iova = addr & TARGET_PAGE_MASK; uint16_t error = 0; IOMMUTLBEntry ret = { .target_as = &address_space_memory, @@ -562,7 +562,7 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr, ret.perm = entry->perm; } else { ret.iova = iova; - ret.addr_mask = ~PAGE_MASK; + ret.addr_mask = ~TARGET_PAGE_MASK; ret.perm = IOMMU_NONE; } @@ -868,7 +868,7 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev) name = g_strdup_printf("msix-s390-%04x", pbdev->uid); memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev), - &s390_msi_ctrl_ops, pbdev, name, PAGE_SIZE); + &s390_msi_ctrl_ops, pbdev, name, TARGET_PAGE_SIZE); memory_region_add_subregion(&pbdev->iommu->mr, pbdev->pci_group->zpci_group.msia, &pbdev->msix_notify_mr); diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 9ec277d50e..1c8ad91175 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -613,7 +613,7 @@ static uint32_t s390_pci_update_iotlb(S390PCIIOMMU *iommu, .iova = entry->iova, .translated_addr = entry->translated_addr, .perm = entry->perm, - .addr_mask = ~PAGE_MASK, + .addr_mask = ~TARGET_PAGE_MASK, }, }; @@ -640,7 +640,7 @@ static uint32_t s390_pci_update_iotlb(S390PCIIOMMU *iommu, cache = g_new(S390IOTLBEntry, 1); cache->iova = entry->iova; cache->translated_addr = entry->translated_addr; - cache->len = PAGE_SIZE; + cache->len = TARGET_PAGE_SIZE; cache->perm = entry->perm; g_hash_table_replace(iommu->iotlb, &cache->iova, cache); dec_dma_avail(iommu); @@ -725,8 +725,8 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra) while (entry.iova < start && entry.iova < end && (dma_avail > 0 || entry.perm == IOMMU_NONE)) { dma_avail = s390_pci_update_iotlb(iommu, &entry); - entry.iova += PAGE_SIZE; - entry.translated_addr += PAGE_SIZE; + entry.iova += TARGET_PAGE_SIZE; + entry.translated_addr += TARGET_PAGE_SIZE; } } err: diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index edb6e3ea01..89c30a8a91 100644 --- a/hw/s390x/sclp.c +++ b/hw/s390x/sclp.c @@ -51,7 +51,7 @@ static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t sccb_len, uint32_t code) { uint64_t sccb_max_addr = sccb_addr + sccb_len - 1; - uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE; + uint64_t sccb_boundary = (sccb_addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE; switch (code & SCLP_CMD_CODE_MASK) { case SCLP_CMDW_READ_SCP_INFO: diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h index 49ae9f03d3..aa891c178d 100644 --- a/include/hw/s390x/s390-pci-bus.h +++ b/include/hw/s390x/s390-pci-bus.h @@ -81,9 +81,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(S390PCIIOMMU, S390_PCI_IOMMU) #define ZPCI_SDMA_ADDR 0x100000000ULL #define ZPCI_EDMA_ADDR 0x1ffffffffffffffULL -#define PAGE_SHIFT 12 -#define PAGE_SIZE (1 << PAGE_SHIFT) -#define PAGE_MASK (~(PAGE_SIZE-1)) #define PAGE_DEFAULT_ACC 0 #define PAGE_DEFAULT_KEY (PAGE_DEFAULT_ACC << 4) @@ -137,7 +134,7 @@ enum ZpciIoatDtype { #define ZPCI_TABLE_BITS 11 #define ZPCI_PT_BITS 8 -#define ZPCI_ST_SHIFT (ZPCI_PT_BITS + PAGE_SHIFT) +#define ZPCI_ST_SHIFT (ZPCI_PT_BITS + TARGET_PAGE_BITS) #define ZPCI_RT_SHIFT (ZPCI_ST_SHIFT + ZPCI_TABLE_BITS) #define ZPCI_RTE_FLAG_MASK 0x3fffULL From 30e398f796d882d829162a16ab7c920f7422da3b Mon Sep 17 00:00:00 2001 From: Christian Borntraeger Date: Tue, 7 Sep 2021 10:10:17 +0000 Subject: [PATCH 20/20] s390x/cpumodel: Add more feature to gen16 default model Add the new gen16 features to the default model and fence them for machine version 6.1 and earlier. Signed-off-by: Christian Borntraeger Reviewed-by: David Hildenbrand Message-Id: <20210907101017.27126-1-borntraeger@de.ibm.com> Signed-off-by: Thomas Huth --- hw/s390x/s390-virtio-ccw.c | 5 +++++ target/s390x/gen-features.c | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 4d25278cf2..61aeccb163 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -803,6 +803,11 @@ DEFINE_CCW_MACHINE(6_2, "6.2", true); static void ccw_machine_6_1_instance_options(MachineState *machine) { ccw_machine_6_2_instance_options(machine); + s390_cpudef_featoff_greater(16, 1, S390_FEAT_NNPA); + s390_cpudef_featoff_greater(16, 1, S390_FEAT_VECTOR_PACKED_DECIMAL_ENH2); + s390_cpudef_featoff_greater(16, 1, S390_FEAT_BEAR_ENH); + s390_cpudef_featoff_greater(16, 1, S390_FEAT_RDP); + s390_cpudef_featoff_greater(16, 1, S390_FEAT_PAI); } static void ccw_machine_6_1_class_options(MachineClass *mc) diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c index 7d85322d68..7cb1a6ec10 100644 --- a/target/s390x/gen-features.c +++ b/target/s390x/gen-features.c @@ -663,7 +663,13 @@ static uint16_t default_GEN15_GA1[] = { S390_FEAT_ETOKEN, }; -#define default_GEN16_GA1 EmptyFeat +static uint16_t default_GEN16_GA1[] = { + S390_FEAT_NNPA, + S390_FEAT_VECTOR_PACKED_DECIMAL_ENH2, + S390_FEAT_BEAR_ENH, + S390_FEAT_RDP, + S390_FEAT_PAI, +}; /* QEMU (CPU model) features */