On 6/6/24 07:02, Don Porter wrote:
Signed-off-by: Don Porter <por...@cs.unc.edu>
---
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, &shift, &width);
+
+ 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 = &cpu->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, &height);
+ /* 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, &pt_entry, 0, NULL, &pte_addr);
+ /* Check that we can access the page table entry */
if (!ptw_translate(&pte_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, &pt_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, &pt_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. Ignoring 128-bit ptes for the moment, surely we should just
zero-extend 32-bit ptes into the 64-bit entry data slot right from the start.
+ break; /* goto do_check_protect; */
What's with the comment.
+ if ((!read_only) &&
+ (!ptw_setl(&pte_trans, pte, PG_ACCESSED_MASK))) {
Again with the extra ().
r~
