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(-)
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];
--
2.21.0
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~
>
--
Thanks,
David / dhildenb
© 2016 - 2026 Red Hat, Inc.