| Message ID | 20190925125236.4043-5-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/mmu: DAT translation rewrite | expand |
On 25/09/2019 14.52, David Hildenbrand wrote: > Let's document how it works and inject PGM_ADDRESSING if reading of > table entries fails. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/mmu_helper.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) Reviewed-by: Thomas Huth <thuth@redhat.com>
On 9/25/19 5:52 AM, David Hildenbrand wrote: > +static inline int read_table_entry(hwaddr gaddr, uint64_t *entry) > +{ > + /* > + * According to the PoP, these table addresses are "unpredictably real > + * or absolute". Also, "it is unpredictable whether the address wraps > + * or an addressing exception is recognized". > + * > + * We treat them as absolute addresses and don't wrap them. > + */ > + if (unlikely(address_space_read(&address_space_memory, gaddr, > + MEMTXATTRS_UNSPECIFIED, (uint8_t *)entry, sizeof(*entry)) != > + MEMTX_OK)) { > + return -EFAULT; > + } > + *entry = be64_to_cpu(*entry); > + return 0; > +} Maybe I've been away from the kernel too long, but I don't find returning -EFAULT helpful. I would return true/false for success/failure so that... > + if (read_table_entry(origin + offs, &pt_entry)) { > + return PGM_ADDRESSING; > + } ... this gets written if (!read_table_entry(...)) { return PGM_ADDRESSING; } This statement, to me, reads "If we did not read_table_entry, return an addressing exception." If you *really* want to return non-zero on failure, I would prefer returning PGM_ADDRESSING instead of the out-of-context -EFAULT. > - new_entry = ldq_phys(cs->as, origin + offs); > + if (read_table_entry(origin + offs, &new_entry)) { Do you really want to replace cs->as with address_space_memory? r~
On 25.09.19 21:25, Richard Henderson wrote: > On 9/25/19 5:52 AM, David Hildenbrand wrote: >> +static inline int read_table_entry(hwaddr gaddr, uint64_t *entry) >> +{ >> + /* >> + * According to the PoP, these table addresses are "unpredictably real >> + * or absolute". Also, "it is unpredictable whether the address wraps >> + * or an addressing exception is recognized". >> + * >> + * We treat them as absolute addresses and don't wrap them. >> + */ >> + if (unlikely(address_space_read(&address_space_memory, gaddr, >> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)entry, sizeof(*entry)) != >> + MEMTX_OK)) { >> + return -EFAULT; >> + } >> + *entry = be64_to_cpu(*entry); >> + return 0; >> +} > > Maybe I've been away from the kernel too long, but I don't find returning > -EFAULT helpful. I would return true/false for success/failure so that... > > >> + if (read_table_entry(origin + offs, &pt_entry)) { >> + return PGM_ADDRESSING; >> + } > > ... this gets written > > if (!read_table_entry(...)) { > return PGM_ADDRESSING; > } > > This statement, to me, reads "If we did not read_table_entry, return an > addressing exception." > > If you *really* want to return non-zero on failure, I would prefer returning > PGM_ADDRESSING instead of the out-of-context -EFAULT. I'll go for your suggestion with a bool! > >> - new_entry = ldq_phys(cs->as, origin + offs); >> + if (read_table_entry(origin + offs, &new_entry)) { > > Do you really want to replace cs->as with address_space_memory? > I guess it shouldn't make a difference (unless I am missing something), but I can just keep using cs->as. Thanks! > > r~ >
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c index f6ae444655..c9fde78614 100644 --- a/target/s390x/mmu_helper.c +++ b/target/s390x/mmu_helper.c @@ -93,6 +93,24 @@ target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr) return raddr; } +static inline int read_table_entry(hwaddr gaddr, uint64_t *entry) +{ + /* + * According to the PoP, these table addresses are "unpredictably real + * or absolute". Also, "it is unpredictable whether the address wraps + * or an addressing exception is recognized". + * + * We treat them as absolute addresses and don't wrap them. + */ + if (unlikely(address_space_read(&address_space_memory, gaddr, + MEMTXATTRS_UNSPECIFIED, (uint8_t *)entry, sizeof(*entry)) != + MEMTX_OK)) { + return -EFAULT; + } + *entry = be64_to_cpu(*entry); + return 0; +} + /* Decode page table entry (normal 4KB page) */ static int mmu_translate_pte(CPUS390XState *env, target_ulong vaddr, uint64_t asc, uint64_t pt_entry, @@ -118,7 +136,6 @@ static int mmu_translate_segment(CPUS390XState *env, target_ulong vaddr, target_ulong *raddr, int *flags, int rw, bool exc) { - CPUState *cs = env_cpu(env); uint64_t origin, offs, pt_entry; if (st_entry & SEGMENT_ENTRY_RO) { @@ -134,7 +151,9 @@ static int mmu_translate_segment(CPUS390XState *env, target_ulong vaddr, /* Look up 4KB page entry */ origin = st_entry & SEGMENT_ENTRY_ORIGIN; offs = (vaddr & VADDR_PX) >> 9; - pt_entry = ldq_phys(cs->as, origin + offs); + if (read_table_entry(origin + offs, &pt_entry)) { + return PGM_ADDRESSING; + } return mmu_translate_pte(env, vaddr, asc, pt_entry, raddr, flags, rw, exc); } @@ -144,7 +163,6 @@ static int mmu_translate_region(CPUS390XState *env, target_ulong vaddr, target_ulong *raddr, int *flags, int rw, bool exc) { - CPUState *cs = env_cpu(env); uint64_t origin, offs, new_entry; const int pchks[4] = { PGM_SEGMENT_TRANS, PGM_REG_THIRD_TRANS, @@ -154,7 +172,9 @@ static int mmu_translate_region(CPUS390XState *env, target_ulong vaddr, origin = entry & REGION_ENTRY_ORIGIN; offs = (vaddr >> (17 + 11 * level / 4)) & 0x3ff8; - new_entry = ldq_phys(cs->as, origin + offs); + if (read_table_entry(origin + offs, &new_entry)) { + return PGM_ADDRESSING; + } if ((new_entry & REGION_ENTRY_INV) != 0) { return pchks[level / 4];
Let's document how it works and inject PGM_ADDRESSING if reading of table entries fails. Signed-off-by: David Hildenbrand <david@redhat.com> --- target/s390x/mmu_helper.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)