| Message ID | 20210303132850.459687-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] target/s390x: Implement the MVPG condition-code-option bit | expand |
On 3/3/21 5:28 AM, Thomas Huth wrote: > From: Richard Henderson<richard.henderson@linaro.org> > > If the CCO bit is set, MVPG should not generate an exception but > report page translation faults via a CC code. > > Create a new helper, access_prepare_nf, which can use probe_access_flags > in non-faulting mode, and then handle watchpoints. > > Signed-off-by: Richard Henderson<richard.henderson@linaro.org> > [thuth: Added logic to still inject protection exceptions] > Signed-off-by: Thomas Huth<thuth@redhat.com> > --- > v4: Add logic to inject protection exceptions if necessary Works for me. I was considering adding yet another cputlb interface to help with this, but this is sufficiently clean that I don't think it's required. r~
On 03.03.21 14:28, Thomas Huth wrote: > From: Richard Henderson <richard.henderson@linaro.org> > > If the CCO bit is set, MVPG should not generate an exception but > report page translation faults via a CC code. > > Create a new helper, access_prepare_nf, which can use probe_access_flags > in non-faulting mode, and then handle watchpoints. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > [thuth: Added logic to still inject protection exceptions] > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v4: Add logic to inject protection exceptions if necessary > > target/s390x/cpu.h | 3 ++ > target/s390x/excp_helper.c | 3 ++ > target/s390x/mem_helper.c | 93 ++++++++++++++++++++++++++++---------- > 3 files changed, 76 insertions(+), 23 deletions(-) > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index 60d434d5ed..825503c6c0 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -114,6 +114,9 @@ struct CPUS390XState { > > uint64_t diag318_info; > Should we start wrapping that stuff into #ifdef CONFIG_TCG ? > + uint64_t tlb_fill_tec; /* translation exception code during tlb_fill */ > + int tlb_fill_exc; /* exception number seen during tlb_fill */ > + > /* Fields up to this point are cleared by a CPU reset */ > struct {} end_reset_fields; > > diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c > index ce16af394b..c48cd6b46f 100644 > --- a/target/s390x/excp_helper.c > +++ b/target/s390x/excp_helper.c > @@ -164,6 +164,9 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > tec = 0; /* unused */ > } > > + env->tlb_fill_exc = excp; > + env->tlb_fill_tec = tec; > + Just what I had in mind. > if (!excp) { > qemu_log_mask(CPU_LOG_MMU, > "%s: set tlb %" PRIx64 " -> %" PRIx64 " (%x)\n", > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > index 25cfede806..cf741541d3 100644 > --- a/target/s390x/mem_helper.c > +++ b/target/s390x/mem_helper.c > @@ -130,28 +130,62 @@ typedef struct S390Access { > int mmu_idx; > } S390Access; > > +static bool access_prepare_nf(S390Access *access, CPUS390XState *env, > + bool nofault, vaddr vaddr1, int size, > + MMUAccessType access_type, > + int mmu_idx, uintptr_t ra) > +{ > + void *haddr1, *haddr2 = NULL; > + int size1, size2; > + vaddr vaddr2 = 0; > + int flags; > + > + assert(size > 0 && size <= 4096); > + > + size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)), > + size2 = size - size1; > + > + flags = probe_access_flags(env, vaddr1, access_type, mmu_idx, > + nofault, &haddr1, ra); > + if (unlikely(size2)) { > + /* The access crosses page boundaries. */ > + vaddr2 = wrap_address(env, vaddr1 + size1); > + flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx, > + nofault, &haddr2, ra); > + } > + > + if (unlikely(flags & TLB_INVALID_MASK)) { > + return false; ^ I recall PAGE_WRITE_INV handling where we immediately set TLB_INVALID_MASK again on write access (to handle low-address protection cleanly). I suspect that TLB_INVALID_MASK will be set in that case (I could be wrong, though). What certainly would work is checking for "haddr != NULL". /* Don't rely on TLB_INVALID_MASK - see PAGE_WRITE_INV handling. */ if (unlikely(!haddr1)) { return false; } > + } > + if (unlikely(flags & TLB_WATCHPOINT)) { > + /* S390 does not presently use transaction attributes. */ > + cpu_check_watchpoint(env_cpu(env), vaddr1, size, > + MEMTXATTRS_UNSPECIFIED, > + (access_type == MMU_DATA_STORE > + ? BP_MEM_WRITE : BP_MEM_READ), ra); > + } > + [...] > /* Helper to handle memset on a single page. */ > @@ -845,8 +879,10 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) > const int mmu_idx = cpu_mmu_index(env, false); > const bool f = extract64(r0, 11, 1); > const bool s = extract64(r0, 10, 1); > + const bool cco = extract64(r0, 8, 1); > uintptr_t ra = GETPC(); > S390Access srca, desta; > + bool ok; > > if ((f && s) || extract64(r0, 12, 4)) { > tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC()); > @@ -858,13 +894,24 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) > /* > * TODO: > * - Access key handling > - * - CC-option with surpression of page-translation exceptions > * - Store r1/r2 register identifiers at real location 162 > */ > - srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx, > - ra); > - desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx, > - ra); > + ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE, > + MMU_DATA_LOAD, mmu_idx, ra); > + if (!ok) { > + return 2; > + } > + ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE, > + MMU_DATA_STORE, mmu_idx, ra); > + if (!ok) { > + if (env->tlb_fill_exc == PGM_PROTECTION) { > + stq_phys(env_cpu(env)->as, > + env->psa + offsetof(LowCore, trans_exc_code), > + env->tlb_fill_tec); > + tcg_s390_program_interrupt(env, PGM_PROTECTION, ra); > + } > + return 1; > + } > access_memmove(env, &desta, &srca, ra); > return 0; /* data moved */ > } > Apart from that, looks good to me. -- Thanks, David / dhildenb
On 3/3/21 11:39 AM, David Hildenbrand wrote: > Should we start wrapping that stuff into #ifdef CONFIG_TCG ? > >> + uint64_t tlb_fill_tec; /* translation exception code during tlb_fill */ >> + int tlb_fill_exc; /* exception number seen during tlb_fill */ Eh, probably not. At least not until we elide the softmmu tlb, which is fantastically larger. >> + if (unlikely(flags & TLB_INVALID_MASK)) { >> + return false; > > ^ I recall PAGE_WRITE_INV handling where we immediately set TLB_INVALID_MASK > again on write access (to handle low-address protection cleanly). I suspect > that TLB_INVALID_MASK will be set in that case (I could be wrong, though). > > What certainly would work is checking for "haddr != NULL". > > /* Don't rely on TLB_INVALID_MASK - see PAGE_WRITE_INV handling. */ > if (unlikely(!haddr1)) { > return false; > } Ah, right. I consider TLB_INVALID_MASK being set in the return from probe_access_flags for PAGE_WRITE_INV a bug. I'm not sure how to fix that right away. Well, !haddr1 is also false for TLB_MMIO, so you'd need to check for that as well. r~
> Am 03.03.2021 um 22:05 schrieb Richard Henderson <richard.henderson@linaro.org>: > > On 3/3/21 11:39 AM, David Hildenbrand wrote: >> Should we start wrapping that stuff into #ifdef CONFIG_TCG ? >>> + uint64_t tlb_fill_tec; /* translation exception code during tlb_fill */ >>> + int tlb_fill_exc; /* exception number seen during tlb_fill */ > > Eh, probably not. At least not until we elide the softmmu tlb, which is fantastically larger. > >>> + if (unlikely(flags & TLB_INVALID_MASK)) { >>> + return false; >> ^ I recall PAGE_WRITE_INV handling where we immediately set TLB_INVALID_MASK again on write access (to handle low-address protection cleanly). I suspect that TLB_INVALID_MASK will be set in that case (I could be wrong, though). >> What certainly would work is checking for "haddr != NULL". >> /* Don't rely on TLB_INVALID_MASK - see PAGE_WRITE_INV handling. */ >> if (unlikely(!haddr1)) { >> return false; >> } > > Ah, right. I consider TLB_INVALID_MASK being set in the return from probe_access_flags for PAGE_WRITE_INV a bug. I'm not sure how to fix that right away. > > Well, !haddr1 is also false for TLB_MMIO, so you'd need to check for that as well. MMIO on s390x? :)
On 3/3/21 1:11 PM, David Hildenbrand wrote:
> MMIO on s390x? :)
hw/s390x/s390-pci-bus.c, memory_region_init_io*().
r~
> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>: > > On 3/3/21 1:11 PM, David Hildenbrand wrote: >> MMIO on s390x? :) > > hw/s390x/s390-pci-bus.c, memory_region_init_io*(). > ... part of system address space where a CPU could stumble over it? > r~ >
On 3/3/21 1:22 PM, David Hildenbrand wrote: > >> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>: >> >> On 3/3/21 1:11 PM, David Hildenbrand wrote: >>> MMIO on s390x? :) >> >> hw/s390x/s390-pci-bus.c, memory_region_init_io*(). >> > > ... part of system address space where a CPU could stumble over it? Impossible to tell within 3 layers of object wrappers. :-( I suppose I have no idea how "pci" was hacked onto s390x. r~
On 03.03.21 22:36, Richard Henderson wrote: > On 3/3/21 1:22 PM, David Hildenbrand wrote: >> >>> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>: >>> >>> On 3/3/21 1:11 PM, David Hildenbrand wrote: >>>> MMIO on s390x? :) >>> >>> hw/s390x/s390-pci-bus.c, memory_region_init_io*(). >>> >> >> ... part of system address space where a CPU could stumble over it? > > Impossible to tell within 3 layers of object wrappers. :-( > I suppose I have no idea how "pci" was hacked onto s390x. You've used the right words to describe "pci" (!) on s390x. IIRC, there is no MMIO: configuration space accesses etc. are performed using special access instructions - which will "emulate" the MMIO access performed on other archs via simple read/write instructions. Ordinary instructions (e.g., mvpg) that operate on the system address space should never stumble over MMIO regions - because that concept does not exist on s390x. -- Thanks, David / dhildenb
On Thu, 4 Mar 2021 09:10:44 +0100 David Hildenbrand <david@redhat.com> wrote: > On 03.03.21 22:36, Richard Henderson wrote: > > On 3/3/21 1:22 PM, David Hildenbrand wrote: > >> > >>> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>: > >>> > >>> On 3/3/21 1:11 PM, David Hildenbrand wrote: > >>>> MMIO on s390x? :) > >>> > >>> hw/s390x/s390-pci-bus.c, memory_region_init_io*(). > >>> > >> > >> ... part of system address space where a CPU could stumble over it? > > > > Impossible to tell within 3 layers of object wrappers. :-( > > I suppose I have no idea how "pci" was hacked onto s390x. > > You've used the right words to describe "pci" (!) on s390x. > > IIRC, there is no MMIO: configuration space accesses etc. are performed > using special access instructions - which will "emulate" the MMIO access > performed on other archs via simple read/write instructions. Yes, it's all specialized instructions. (QEMU emulates one set, there's a newer one supported by the Linux kernel that is a bit more mmio-like in semantics, but I don't know the details.) > > Ordinary instructions (e.g., mvpg) that operate on the system address > space should never stumble over MMIO regions - because that concept does > not exist on s390x. > That's my understanding as well.
On 04/03/2021 09.10, David Hildenbrand wrote: > On 03.03.21 22:36, Richard Henderson wrote: >> On 3/3/21 1:22 PM, David Hildenbrand wrote: >>> >>>> Am 03.03.2021 um 22:19 schrieb Richard Henderson >>>> <richard.henderson@linaro.org>: >>>> >>>> On 3/3/21 1:11 PM, David Hildenbrand wrote: >>>>> MMIO on s390x? :) >>>> >>>> hw/s390x/s390-pci-bus.c, memory_region_init_io*(). >>>> >>> >>> ... part of system address space where a CPU could stumble over it? >> >> Impossible to tell within 3 layers of object wrappers. :-( >> I suppose I have no idea how "pci" was hacked onto s390x. > > You've used the right words to describe "pci" (!) on s390x. > > IIRC, there is no MMIO: configuration space accesses etc. are performed > using special access instructions - which will "emulate" the MMIO access > performed on other archs via simple read/write instructions. > > Ordinary instructions (e.g., mvpg) that operate on the system address space > should never stumble over MMIO regions - because that concept does not exist > on s390x. Sorry, guys, you've lost me here ... is there now something left to do for this patch or is it good as it is? Thomas
> Am 09.03.2021 um 22:05 schrieb Thomas Huth <thuth@redhat.com>: > > On 04/03/2021 09.10, David Hildenbrand wrote: >>> On 03.03.21 22:36, Richard Henderson wrote: >>> On 3/3/21 1:22 PM, David Hildenbrand wrote: >>>> >>>>> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>: >>>>> >>>>> On 3/3/21 1:11 PM, David Hildenbrand wrote: >>>>>> MMIO on s390x? :) >>>>> >>>>> hw/s390x/s390-pci-bus.c, memory_region_init_io*(). >>>>> >>>> >>>> ... part of system address space where a CPU could stumble over it? >>> >>> Impossible to tell within 3 layers of object wrappers. :-( >>> I suppose I have no idea how "pci" was hacked onto s390x. >> You've used the right words to describe "pci" (!) on s390x. >> IIRC, there is no MMIO: configuration space accesses etc. are performed using special access instructions - which will "emulate" the MMIO access performed on other archs via simple read/write instructions. >> Ordinary instructions (e.g., mvpg) that operate on the system address space should never stumble over MMIO regions - because that concept does not exist on s390x. > > Sorry, guys, you've lost me here ... is there now something left to do for this patch or is it good as it is? I think that check for invalid TLB should be replaced by a check against host == NULL.
On Wed, 10 Mar 2021 21:49:22 +0100 David Hildenbrand <david@redhat.com> wrote: > > Am 09.03.2021 um 22:05 schrieb Thomas Huth <thuth@redhat.com>: > > > > On 04/03/2021 09.10, David Hildenbrand wrote: > >>> On 03.03.21 22:36, Richard Henderson wrote: > >>> On 3/3/21 1:22 PM, David Hildenbrand wrote: > >>>> > >>>>> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>: > >>>>> > >>>>> On 3/3/21 1:11 PM, David Hildenbrand wrote: > >>>>>> MMIO on s390x? :) > >>>>> > >>>>> hw/s390x/s390-pci-bus.c, memory_region_init_io*(). > >>>>> > >>>> > >>>> ... part of system address space where a CPU could stumble over it? > >>> > >>> Impossible to tell within 3 layers of object wrappers. :-( > >>> I suppose I have no idea how "pci" was hacked onto s390x. > >> You've used the right words to describe "pci" (!) on s390x. > >> IIRC, there is no MMIO: configuration space accesses etc. are performed using special access instructions - which will "emulate" the MMIO access performed on other archs via simple read/write instructions. > >> Ordinary instructions (e.g., mvpg) that operate on the system address space should never stumble over MMIO regions - because that concept does not exist on s390x. > > > > Sorry, guys, you've lost me here ... is there now something left to do for this patch or is it good as it is? > > I think that check for invalid TLB should be replaced by a check against host == NULL. > Just a reminder that softfreeze is on Tuesday next week, and I'd like to send a pull request by Monday. So a v5 should arrive soon to make it :)
On 03.03.21 14:28, Thomas Huth wrote: > From: Richard Henderson <richard.henderson@linaro.org> > > If the CCO bit is set, MVPG should not generate an exception but > report page translation faults via a CC code. > > Create a new helper, access_prepare_nf, which can use probe_access_flags > in non-faulting mode, and then handle watchpoints. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > [thuth: Added logic to still inject protection exceptions] > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v4: Add logic to inject protection exceptions if necessary > > target/s390x/cpu.h | 3 ++ > target/s390x/excp_helper.c | 3 ++ > target/s390x/mem_helper.c | 93 ++++++++++++++++++++++++++++---------- > 3 files changed, 76 insertions(+), 23 deletions(-) > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index 60d434d5ed..825503c6c0 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -114,6 +114,9 @@ struct CPUS390XState { > > uint64_t diag318_info; > > + uint64_t tlb_fill_tec; /* translation exception code during tlb_fill */ > + int tlb_fill_exc; /* exception number seen during tlb_fill */ > + > /* Fields up to this point are cleared by a CPU reset */ > struct {} end_reset_fields; > > diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c > index ce16af394b..c48cd6b46f 100644 > --- a/target/s390x/excp_helper.c > +++ b/target/s390x/excp_helper.c > @@ -164,6 +164,9 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > tec = 0; /* unused */ > } > > + env->tlb_fill_exc = excp; > + env->tlb_fill_tec = tec; > + > if (!excp) { > qemu_log_mask(CPU_LOG_MMU, > "%s: set tlb %" PRIx64 " -> %" PRIx64 " (%x)\n", > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > index 25cfede806..cf741541d3 100644 > --- a/target/s390x/mem_helper.c > +++ b/target/s390x/mem_helper.c > @@ -130,28 +130,62 @@ typedef struct S390Access { > int mmu_idx; > } S390Access; > > +static bool access_prepare_nf(S390Access *access, CPUS390XState *env, > + bool nofault, vaddr vaddr1, int size, > + MMUAccessType access_type, > + int mmu_idx, uintptr_t ra) > +{ > + void *haddr1, *haddr2 = NULL; > + int size1, size2; > + vaddr vaddr2 = 0; > + int flags; > + > + assert(size > 0 && size <= 4096); > + > + size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)), > + size2 = size - size1; > + > + flags = probe_access_flags(env, vaddr1, access_type, mmu_idx, > + nofault, &haddr1, ra); > + if (unlikely(size2)) { > + /* The access crosses page boundaries. */ > + vaddr2 = wrap_address(env, vaddr1 + size1); > + flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx, > + nofault, &haddr2, ra); > + } > + > + if (unlikely(flags & TLB_INVALID_MASK)) { > + return false; > + } > + if (unlikely(flags & TLB_WATCHPOINT)) { > + /* S390 does not presently use transaction attributes. */ > + cpu_check_watchpoint(env_cpu(env), vaddr1, size, > + MEMTXATTRS_UNSPECIFIED, > + (access_type == MMU_DATA_STORE > + ? BP_MEM_WRITE : BP_MEM_READ), ra); > + } > + > + *access = (S390Access) { > + .vaddr1 = vaddr1, > + .vaddr2 = vaddr2, > + .haddr1 = haddr1, > + .haddr2 = haddr2, > + .size1 = size1, > + .size2 = size2, > + .mmu_idx = mmu_idx > + }; > + return true; > +} > + > static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size, > MMUAccessType access_type, int mmu_idx, > uintptr_t ra) > { > - S390Access access = { > - .vaddr1 = vaddr, > - .size1 = MIN(size, -(vaddr | TARGET_PAGE_MASK)), > - .mmu_idx = mmu_idx, > - }; > - > - g_assert(size > 0 && size <= 4096); > - access.haddr1 = probe_access(env, access.vaddr1, access.size1, access_type, > - mmu_idx, ra); > - > - if (unlikely(access.size1 != size)) { > - /* The access crosses page boundaries. */ > - access.vaddr2 = wrap_address(env, vaddr + access.size1); > - access.size2 = size - access.size1; > - access.haddr2 = probe_access(env, access.vaddr2, access.size2, > - access_type, mmu_idx, ra); > - } > - return access; > + S390Access ret; > + bool ok = access_prepare_nf(&ret, env, false, vaddr, size, > + access_type, mmu_idx, ra); > + assert(ok); > + return ret; > } > > /* Helper to handle memset on a single page. */ > @@ -845,8 +879,10 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) > const int mmu_idx = cpu_mmu_index(env, false); > const bool f = extract64(r0, 11, 1); > const bool s = extract64(r0, 10, 1); > + const bool cco = extract64(r0, 8, 1); > uintptr_t ra = GETPC(); > S390Access srca, desta; > + bool ok; > > if ((f && s) || extract64(r0, 12, 4)) { > tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC()); > @@ -858,13 +894,24 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) > /* > * TODO: > * - Access key handling > - * - CC-option with surpression of page-translation exceptions > * - Store r1/r2 register identifiers at real location 162 > */ > - srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx, > - ra); > - desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx, > - ra); > + ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE, > + MMU_DATA_LOAD, mmu_idx, ra); > + if (!ok) { > + return 2; > + } > + ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE, > + MMU_DATA_STORE, mmu_idx, ra); > + if (!ok) { > + if (env->tlb_fill_exc == PGM_PROTECTION) { > + stq_phys(env_cpu(env)->as, > + env->psa + offsetof(LowCore, trans_exc_code), > + env->tlb_fill_tec); > + tcg_s390_program_interrupt(env, PGM_PROTECTION, ra); > + } > + return 1; > + } > access_memmove(env, &desta, &srca, ra); > return 0; /* data moved */ > } > As talked with Thomas off-list, there is no trusting on host==NULL as well (see comment in struct S390Access). host==NULL simply means we have to do individual ld/st. The following on top should work. Not perfect, but seems to get the job done. From 056b3c9f2ffd43b10d8293e7143cf7af5d1d5022 Mon Sep 17 00:00:00 2001 From: David Hildenbrand <david@redhat.com> Date: Thu, 11 Mar 2021 14:44:45 +0100 Subject: [PATCH] fixup Signed-off-by: David Hildenbrand <david@redhat.com> --- target/s390x/mem_helper.c | 48 ++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index cf741541d3..0a9b15ea90 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -130,10 +130,15 @@ typedef struct S390Access { int mmu_idx; } S390Access; -static bool access_prepare_nf(S390Access *access, CPUS390XState *env, - bool nofault, vaddr vaddr1, int size, - MMUAccessType access_type, - int mmu_idx, uintptr_t ra) +/* + * With nofault=1, return the generated PGM_ exception that would have + * been injected into the guest (tec stored in env->tlb_fill_tec); + * return 0 if no exception was detected. + */ +static int access_prepare_nf(S390Access *access, CPUS390XState *env, + bool nofault, vaddr vaddr1, int size, + MMUAccessType access_type, + int mmu_idx, uintptr_t ra) { void *haddr1, *haddr2 = NULL; int size1, size2; @@ -145,18 +150,24 @@ static bool access_prepare_nf(S390Access *access, CPUS390XState *env, size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)), size2 = size - size1; + env->tlb_fill_exc = 0; flags = probe_access_flags(env, vaddr1, access_type, mmu_idx, nofault, &haddr1, ra); + if (env->tlb_fill_exc) { + /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */ + return env->tlb_fill_exc; + } if (unlikely(size2)) { /* The access crosses page boundaries. */ vaddr2 = wrap_address(env, vaddr1 + size1); flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx, nofault, &haddr2, ra); + if (env->tlb_fill_exc) { + /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */ + return env->tlb_fill_exc; + } } - if (unlikely(flags & TLB_INVALID_MASK)) { - return false; - } if (unlikely(flags & TLB_WATCHPOINT)) { /* S390 does not presently use transaction attributes. */ cpu_check_watchpoint(env_cpu(env), vaddr1, size, @@ -174,7 +185,7 @@ static bool access_prepare_nf(S390Access *access, CPUS390XState *env, .size2 = size2, .mmu_idx = mmu_idx }; - return true; + return 0; } static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size, @@ -182,9 +193,9 @@ static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size, uintptr_t ra) { S390Access ret; - bool ok = access_prepare_nf(&ret, env, false, vaddr, size, + int exc = access_prepare_nf(&ret, env, false, vaddr, size, access_type, mmu_idx, ra); - assert(ok); + assert(!exc); return ret; } @@ -882,7 +893,7 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) const bool cco = extract64(r0, 8, 1); uintptr_t ra = GETPC(); S390Access srca, desta; - bool ok; + int exc; if ((f && s) || extract64(r0, 12, 4)) { tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC()); @@ -896,15 +907,16 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) * - Access key handling * - Store r1/r2 register identifiers at real location 162 */ - ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE, - MMU_DATA_LOAD, mmu_idx, ra); - if (!ok) { + exc = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE, + MMU_DATA_LOAD, mmu_idx, ra); + if (exc) { return 2; } - ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE, - MMU_DATA_STORE, mmu_idx, ra); - if (!ok) { - if (env->tlb_fill_exc == PGM_PROTECTION) { + exc = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE, + MMU_DATA_STORE, mmu_idx, ra); + if (exc) { + fprintf(stderr, "Exception: %d\n", exc); + if (exc == PGM_PROTECTION) { stq_phys(env_cpu(env)->as, env->psa + offsetof(LowCore, trans_exc_code), env->tlb_fill_tec); -- 2.29.2 -- Thanks, David / dhildenb
On 3/11/21 8:03 AM, David Hildenbrand wrote: > As talked with Thomas off-list, there is no trusting on host==NULL > as well (see comment in struct S390Access). host==NULL simply > means we have to do individual ld/st. I think that comment is stale with the use of probe_access instead of tlb_vaddr_to_host -- TLB_DIRTY is in fact handled now. > + env->tlb_fill_exc = 0; > flags = probe_access_flags(env, vaddr1, access_type, mmu_idx, > nofault, &haddr1, ra); > + if (env->tlb_fill_exc) { > + /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */ > + return env->tlb_fill_exc; > + } > if (unlikely(size2)) { > /* The access crosses page boundaries. */ > vaddr2 = wrap_address(env, vaddr1 + size1); > flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx, > nofault, &haddr2, ra); > + if (env->tlb_fill_exc) { > + /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */ > + return env->tlb_fill_exc; But this is pretty clean, and definitely works. r~
On 11.03.21 16:58, Richard Henderson wrote: > On 3/11/21 8:03 AM, David Hildenbrand wrote: >> As talked with Thomas off-list, there is no trusting on host==NULL >> as well (see comment in struct S390Access). host==NULL simply >> means we have to do individual ld/st. > > I think that comment is stale with the use of probe_access instead of > tlb_vaddr_to_host -- TLB_DIRTY is in fact handled now. Yes, it might be worth exploring in which cases we will still have issues and updating the comment. LAP is certainly one. >> + env->tlb_fill_exc = 0; >> flags = probe_access_flags(env, vaddr1, access_type, mmu_idx, >> nofault, &haddr1, ra); >> + if (env->tlb_fill_exc) { >> + /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */ >> + return env->tlb_fill_exc; >> + } >> if (unlikely(size2)) { >> /* The access crosses page boundaries. */ >> vaddr2 = wrap_address(env, vaddr1 + size1); >> flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx, >> nofault, &haddr2, ra); >> + if (env->tlb_fill_exc) { >> + /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */ >> + return env->tlb_fill_exc; > > But this is pretty clean, and definitely works. Except that I need to special case CONFIG_USER_ONLY .... I'll send a v5 later, then we can discuss when looking at the full diff (including an addon patch to handle r1/r2). -- Thanks, David / dhildenb
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 60d434d5ed..825503c6c0 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -114,6 +114,9 @@ struct CPUS390XState { uint64_t diag318_info; + uint64_t tlb_fill_tec; /* translation exception code during tlb_fill */ + int tlb_fill_exc; /* exception number seen during tlb_fill */ + /* Fields up to this point are cleared by a CPU reset */ struct {} end_reset_fields; diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c index ce16af394b..c48cd6b46f 100644 --- a/target/s390x/excp_helper.c +++ b/target/s390x/excp_helper.c @@ -164,6 +164,9 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, tec = 0; /* unused */ } + env->tlb_fill_exc = excp; + env->tlb_fill_tec = tec; + if (!excp) { qemu_log_mask(CPU_LOG_MMU, "%s: set tlb %" PRIx64 " -> %" PRIx64 " (%x)\n", diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 25cfede806..cf741541d3 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -130,28 +130,62 @@ typedef struct S390Access { int mmu_idx; } S390Access; +static bool access_prepare_nf(S390Access *access, CPUS390XState *env, + bool nofault, vaddr vaddr1, int size, + MMUAccessType access_type, + int mmu_idx, uintptr_t ra) +{ + void *haddr1, *haddr2 = NULL; + int size1, size2; + vaddr vaddr2 = 0; + int flags; + + assert(size > 0 && size <= 4096); + + size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)), + size2 = size - size1; + + flags = probe_access_flags(env, vaddr1, access_type, mmu_idx, + nofault, &haddr1, ra); + if (unlikely(size2)) { + /* The access crosses page boundaries. */ + vaddr2 = wrap_address(env, vaddr1 + size1); + flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx, + nofault, &haddr2, ra); + } + + if (unlikely(flags & TLB_INVALID_MASK)) { + return false; + } + if (unlikely(flags & TLB_WATCHPOINT)) { + /* S390 does not presently use transaction attributes. */ + cpu_check_watchpoint(env_cpu(env), vaddr1, size, + MEMTXATTRS_UNSPECIFIED, + (access_type == MMU_DATA_STORE + ? BP_MEM_WRITE : BP_MEM_READ), ra); + } + + *access = (S390Access) { + .vaddr1 = vaddr1, + .vaddr2 = vaddr2, + .haddr1 = haddr1, + .haddr2 = haddr2, + .size1 = size1, + .size2 = size2, + .mmu_idx = mmu_idx + }; + return true; +} + static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size, MMUAccessType access_type, int mmu_idx, uintptr_t ra) { - S390Access access = { - .vaddr1 = vaddr, - .size1 = MIN(size, -(vaddr | TARGET_PAGE_MASK)), - .mmu_idx = mmu_idx, - }; - - g_assert(size > 0 && size <= 4096); - access.haddr1 = probe_access(env, access.vaddr1, access.size1, access_type, - mmu_idx, ra); - - if (unlikely(access.size1 != size)) { - /* The access crosses page boundaries. */ - access.vaddr2 = wrap_address(env, vaddr + access.size1); - access.size2 = size - access.size1; - access.haddr2 = probe_access(env, access.vaddr2, access.size2, - access_type, mmu_idx, ra); - } - return access; + S390Access ret; + bool ok = access_prepare_nf(&ret, env, false, vaddr, size, + access_type, mmu_idx, ra); + assert(ok); + return ret; } /* Helper to handle memset on a single page. */ @@ -845,8 +879,10 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) const int mmu_idx = cpu_mmu_index(env, false); const bool f = extract64(r0, 11, 1); const bool s = extract64(r0, 10, 1); + const bool cco = extract64(r0, 8, 1); uintptr_t ra = GETPC(); S390Access srca, desta; + bool ok; if ((f && s) || extract64(r0, 12, 4)) { tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC()); @@ -858,13 +894,24 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) /* * TODO: * - Access key handling - * - CC-option with surpression of page-translation exceptions * - Store r1/r2 register identifiers at real location 162 */ - srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx, - ra); - desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx, - ra); + ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE, + MMU_DATA_LOAD, mmu_idx, ra); + if (!ok) { + return 2; + } + ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE, + MMU_DATA_STORE, mmu_idx, ra); + if (!ok) { + if (env->tlb_fill_exc == PGM_PROTECTION) { + stq_phys(env_cpu(env)->as, + env->psa + offsetof(LowCore, trans_exc_code), + env->tlb_fill_tec); + tcg_s390_program_interrupt(env, PGM_PROTECTION, ra); + } + return 1; + } access_memmove(env, &desta, &srca, ra); return 0; /* data moved */ }