Re: [PATCH v3 6/6] Convert x86_mmu_translate() to use common code.
On 6/6/24 07:02, Don Porter wrote:
Signed-off-by: Don Porter
---
target/i386/arch_memory_mapping.c| 44 +++-
target/i386/cpu.h| 5 +-
target/i386/helper.c | 374 +++
target/i386/tcg/sysemu/excp_helper.c | 2 +-
4 files changed, 129 insertions(+), 296 deletions(-)
diff --git a/target/i386/arch_memory_mapping.c
b/target/i386/arch_memory_mapping.c
index b52e98133c..bccd290b9f 100644
--- a/target/i386/arch_memory_mapping.c
+++ b/target/i386/arch_memory_mapping.c
@@ -228,9 +228,38 @@ static void _mmu_decode_va_parameters(CPUState *cs, int
height,
}
/**
- * get_pte - Copy the contents of the page table entry at node[i] into
pt_entry.
- * Optionally, add the relevant bits to the virtual address in
- * vaddr_pte.
+ * x86_virtual_to_pte_index - Given a virtual address and height in
+ * the page table radix tree, return the index that should be
+ * used to look up the next page table entry (pte) in
+ * translating an address.
+ *
+ * @cs - CPU state
+ * @vaddr - The virtual address to translate
+ * @height - height of node within the tree (leaves are 1, not 0).
+ *
+ * Example: In 32-bit x86 page tables, the virtual address is split
+ * into 10 bits at height 2, 10 bits at height 1, and 12 offset bits.
+ * So a call with VA and height 2 would return the first 10 bits of va,
+ * right shifted by 22.
+ */
+
+int x86_virtual_to_pte_index(CPUState *cs, target_ulong vaddr, int height)
+{
+int shift = 0;
+int width = 0;
+int mask = 0;
+
+_mmu_decode_va_parameters(cs, height, , );
+
+mask = (1 << width) - 1;
+
+return (vaddr >> shift) & mask;
+}
+
+/**
+ * x86_get_pte - Copy the contents of the page table entry at node[i]
+ * into pt_entry. Optionally, add the relevant bits to
+ * the virtual address in vaddr_pte.
*
* @cs - CPU state
* @node - physical address of the current page table node
@@ -249,7 +278,6 @@ void
x86_get_pte(CPUState *cs, hwaddr node, int i, int height,
PTE_t *pt_entry, vaddr vaddr_parent, vaddr *vaddr_pte,
hwaddr *pte_paddr)
-
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = >env;
Some fixes to be merged back into previous patches.
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -308,7 +308,8 @@ static bool ptw_translate(X86PTETranslate *inout, hwaddr
addr, uint64_t ra)
static bool x86_mmu_translate(CPUX86State *env, const X86TranslateParams *in,
X86TranslateResult *out,
- X86TranslateFault *err, uint64_t ra)
+ X86TranslateFault *err, uint64_t ra,
+ bool read_only)
{
const target_ulong addr = in->addr;
const int pg_mode = in->pg_mode;
@@ -324,6 +325,10 @@ static bool x86_mmu_translate(CPUX86State *env, const
X86TranslateParams *in,
uint32_t pkr;
int page_size;
int error_code;
+CPUState *cs = env_cpu(env);
+int height;
+bool pae_enabled = env->cr[4] & CR4_PAE_MASK;
+bool long_mode_enabled = env->hflags & HF_LMA_MASK;
Incorrect. These bits are in pg_mode...
-if (pg_mode & PG_MODE_PAE) {
-#ifdef TARGET_X86_64
-if (pg_mode & PG_MODE_LMA) {
-if (pg_mode & PG_MODE_LA57) {
... like so.
+/*
+ * ptep is really an accumulator for the permission bits.
+ * Thus, the xor-ing totally trashes the high bits, and that is
+ * ok - we only care about the low ones.
+ */
+ptep = PG_NX_MASK | PG_USER_MASK | PG_RW_MASK;
+hwaddr pt_node = x86_page_table_root(cs, );
+/* Special case for PAE paging */
+if (height == 3 && pg_mode & PG_MODE_PAE) {
+rsvd_mask |= PG_HI_USER_MASK;
+}
+int i = height;
+do {
+int index = x86_virtual_to_pte_index(cs, addr, i);
+PTE_t pt_entry;
+uint64_t my_rsvd_mask = rsvd_mask;
+
+x86_get_pte(cs, pt_node, index, i, _entry, 0, NULL, _addr);
+/* Check that we can access the page table entry */
if (!ptw_translate(_trans, pte_addr, ra)) {
return false;
}
You "get" the pte and only afterward you check that it is accessible.
I think you've missed the point of ptw_translate.
+
+restart:
+if (!x86_pte_present(cs, _entry)) {
goto do_fault;
}
+/* For height > 3, check and reject PSE mask */
+if (i > 3) {
+my_rsvd_mask |= PG_PSE_MASK;
}
+
+if (x86_pte_check_bits(cs, _entry, my_rsvd_mask)) {
goto do_fault_rsvd;
}
Surely the reserved bit checking should be part of the generic walker.
Is there some reason those should be ignored for "info pg", for example?
+if (long_mode_enabled) {
+pte = pt_entry.pte64_t;
+} else {
+pte = pt_entry.pte32_t;
}
This is pretty ugly.
[PATCH v3 6/6] Convert x86_mmu_translate() to use common code.
Signed-off-by: Don Porter
---
target/i386/arch_memory_mapping.c| 44 +++-
target/i386/cpu.h| 5 +-
target/i386/helper.c | 374 +++
target/i386/tcg/sysemu/excp_helper.c | 2 +-
4 files changed, 129 insertions(+), 296 deletions(-)
diff --git a/target/i386/arch_memory_mapping.c
b/target/i386/arch_memory_mapping.c
index b52e98133c..bccd290b9f 100644
--- a/target/i386/arch_memory_mapping.c
+++ b/target/i386/arch_memory_mapping.c
@@ -228,9 +228,38 @@ static void _mmu_decode_va_parameters(CPUState *cs, int
height,
}
/**
- * get_pte - Copy the contents of the page table entry at node[i] into
pt_entry.
- * Optionally, add the relevant bits to the virtual address in
- * vaddr_pte.
+ * x86_virtual_to_pte_index - Given a virtual address and height in
+ * the page table radix tree, return the index that should be
+ * used to look up the next page table entry (pte) in
+ * translating an address.
+ *
+ * @cs - CPU state
+ * @vaddr - The virtual address to translate
+ * @height - height of node within the tree (leaves are 1, not 0).
+ *
+ * Example: In 32-bit x86 page tables, the virtual address is split
+ * into 10 bits at height 2, 10 bits at height 1, and 12 offset bits.
+ * So a call with VA and height 2 would return the first 10 bits of va,
+ * right shifted by 22.
+ */
+
+int x86_virtual_to_pte_index(CPUState *cs, target_ulong vaddr, int height)
+{
+int shift = 0;
+int width = 0;
+int mask = 0;
+
+_mmu_decode_va_parameters(cs, height, , );
+
+mask = (1 << width) - 1;
+
+return (vaddr >> shift) & mask;
+}
+
+/**
+ * x86_get_pte - Copy the contents of the page table entry at node[i]
+ * into pt_entry. Optionally, add the relevant bits to
+ * the virtual address in vaddr_pte.
*
* @cs - CPU state
* @node - physical address of the current page table node
@@ -249,7 +278,6 @@ void
x86_get_pte(CPUState *cs, hwaddr node, int i, int height,
PTE_t *pt_entry, vaddr vaddr_parent, vaddr *vaddr_pte,
hwaddr *pte_paddr)
-
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = >env;
@@ -282,8 +310,8 @@ x86_get_pte(CPUState *cs, hwaddr node, int i, int height,
}
-static bool
-mmu_pte_check_bits(CPUState *cs, PTE_t *pte, int64_t mask)
+bool
+x86_pte_check_bits(CPUState *cs, PTE_t *pte, int64_t mask)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = >env;
@@ -300,7 +328,7 @@ mmu_pte_check_bits(CPUState *cs, PTE_t *pte, int64_t mask)
bool
x86_pte_present(CPUState *cs, PTE_t *pte)
{
-return mmu_pte_check_bits(cs, pte, PG_PRESENT_MASK);
+return x86_pte_check_bits(cs, pte, PG_PRESENT_MASK);
}
/**
@@ -312,7 +340,7 @@ x86_pte_present(CPUState *cs, PTE_t *pte)
bool
x86_pte_leaf(CPUState *cs, int height, PTE_t *pte)
{
-return height == 1 || mmu_pte_check_bits(cs, pte, PG_PSE_MASK);
+return height == 1 || x86_pte_check_bits(cs, pte, PG_PSE_MASK);
}
/**
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 2c7cfe7901..978841a624 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2198,6 +2198,8 @@ bool x86_pte_present(CPUState *cs, PTE_t *pte);
bool x86_pte_leaf(CPUState *cs, int height, PTE_t *pte);
hwaddr x86_pte_child(CPUState *cs, PTE_t *pte, int height);
uint64_t x86_pte_flags(uint64_t pte);
+bool x86_pte_check_bits(CPUState *cs, PTE_t *pte, int64_t mask);
+int x86_virtual_to_pte_index(CPUState *cs, target_ulong vaddr, int height);
bool x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list,
Error **errp);
bool x86_mon_init_page_table_iterator(Monitor *mon,
@@ -2220,7 +,8 @@ int cpu_x86_support_mca_broadcast(CPUX86State *env);
bool x86_cpu_get_physical_address(CPUX86State *env, vaddr addr,
MMUAccessType access_type, int mmu_idx,
X86TranslateResult *out,
- X86TranslateFault *err, uint64_t ra);
+ X86TranslateFault *err, uint64_t ra,
+ bool read_only);
hwaddr x86_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
MemTxAttrs *attrs);
diff --git a/target/i386/helper.c b/target/i386/helper.c
index 746570a442..4e5467ee57 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -308,7 +308,8 @@ static bool ptw_translate(X86PTETranslate *inout, hwaddr
addr, uint64_t ra)
static bool x86_mmu_translate(CPUX86State *env, const X86TranslateParams *in,
X86TranslateResult *out,
- X86TranslateFault *err, uint64_t ra)
+ X86TranslateFault *err, uint64_t ra,
+ bool read_only)
{
const target_ulong addr = in->addr;
const int pg_mode = in->pg_mode;
@@ -324,6 +325,10 @@ static bool
