Xen project Mailing List

Re: [Xen-devel] [PATCH v7 00/14] arm/mem_access: Walk guest page tables in SW if mem_access is active

To: Julien Grall <julien.grall@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Stefano Stabellini <sstabellini@xxxxxxxxxx>

From: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>

Date: Tue, 8 Aug 2017 14:17:45 +0200

Delivery-date: Tue, 08 Aug 2017 12:18:17 +0000

List-id: Xen developer discussion <xen-devel.lists.xen.org>

Hi Julien, On 08/04/2017 11:15 AM, Sergej Proskurin wrote: > Hi Julien, > > Sorry for the late reply. > > On 07/31/2017 04:38 PM, Julien Grall wrote: >> >> On 18/07/17 13:24, Sergej Proskurin wrote: >>> Hi all, >> Hi, >> >>> The function p2m_mem_access_check_and_get_page is called from the function >>> get_page_from_gva if mem_access is active and the hardware-aided >>> translation of >>> the given guest virtual address (gva) into machine address fails. That is, >>> if >>> the stage-2 translation tables constrain access to the guests's page tables, >>> hardware-assisted translation will fail. The idea of the function >>> p2m_mem_access_check_and_get_page is thus to translate the given gva and >>> check >>> the requested access rights in software. However, as the current >>> implementation >>> of p2m_mem_access_check_and_get_page makes use of the hardware-aided gva to >>> ipa >>> translation, the translation might also fail because of reasons stated above >>> and will become equally relevant for the altp2m implementation on ARM. As >>> such, we provide a software guest translation table walk to address the >>> above >>> mentioned issue. >>> >>> The current version of the implementation supports translation of both the >>> short-descriptor as well as the long-descriptor translation table format on >>> ARMv7 and ARMv8 (AArch32/AArch64). >>> >>> This revised version incorporates the comments of the previous patch >>> series. In >>> this patch version we refine the definition of PAGE_SIZE_GRAN and >>> PAGE_MASK_GRAN. In particular, we use PAGE_SIZE_GRAN to define >>> PAGE_MASK_GRAN >>> and thus avoid these defines to have a differing type. We also changed the >>> previously introduced macro BITS_PER_LONG_LONG to BITS_PER_LLONG. Further >>> changes comprise minor adjustments in comments and renaming of macros and >>> function parameters. Some additional changes comprising code readability and >>> correct type usage have been made and stated in the individual commits. >>> >>> The following patch series can be found on Github[0]. >> I tried this series today with the change [1] in Xen to check the translation >> is valid. However, I got a failure when booting non-LPAE arm32 Dom0: >> > That's odd.. Thanks for the information. I will investigate this issue > next week, as soon as I have access to our ARMv7 board. > >> (XEN) Loading kernel from boot module @ 0000000080008000 >> (XEN) Allocating 1:1 mappings totalling 512MB for dom0: >> (XEN) BANK[0] 0x000000a0000000-0x000000c0000000 (512MB) >> (XEN) Grant table range: 0x000000ffe00000-0x000000ffe6a000 >> (XEN) Loading zImage from 0000000080008000 to >> 00000000a7800000-00000000a7f50e28 >> (XEN) Allocating PPI 16 for event channel interrupt >> (XEN) Loading dom0 DTB to 0x00000000a8000000-0x00000000a8001f8e >> (XEN) Std. Loglevel: All >> (XEN) Guest Loglevel: All >> (XEN) guest_walk_tables: gva 0xffeff018 pipa 0x1c090018 >> (XEN) access_guest_memory_by_ipa: gpa 0xa0207ff8 >> (XEN) access_guest_memory_by_ipa: gpa 0xffffffffa13aebfc >> (XEN) d0: guestcopy: failed to get table entry. >> (XEN) Xen BUG at traps.c:2737 >> (XEN) ----[ Xen-4.10-unstable arm32 debug=y Not tainted ]---- >> (XEN) CPU: 0 >> (XEN) PC: 00264dc0 do_trap_guest_sync+0x161c/0x1804 >> (XEN) CPSR: a000005a MODE:Hypervisor >> (XEN) R0: ffffffea R1: 00000000 R2: 00000000 R3: 0000004a >> (XEN) R4: 93830007 R5: 47fcff58 R6: 93830007 R7: 00000007 >> (XEN) R8: 1c090000 R9: 00000000 R10:00000000 R11:47fcff54 R12:ffffffea >> (XEN) HYP: SP: 47fcfee4 LR: 00258dec >> (XEN) >> (XEN) VTCR_EL2: 80003558 >> (XEN) VTTBR_EL2: 00010008f3ffc000 >> (XEN) >> (XEN) SCTLR_EL2: 30cd187f >> (XEN) HCR_EL2: 000000000038663f >> (XEN) TTBR0_EL2: 00000000fff02000 >> (XEN) >> (XEN) ESR_EL2: 00000000 >> (XEN) HPFAR_EL2: 00000000001c0900 >> (XEN) HDFAR: ffeff018 >> (XEN) HIFAR: 00000000 >> (XEN) >> (XEN) Xen stack trace from sp=47fcfee4: >> (XEN) 00000000 47fcff34 00256008 47fcfefc 47fcfefc 200000da 00000004 >> 47fd48f4 >> (XEN) 002d5ef0 00000004 002d1f00 00000004 00000000 002d1f00 c163f740 >> 93830007 >> (XEN) ffeff018 1c090018 00000000 47fcff44 c15e70ac 0000005b c15e70ac >> c074400c >> (XEN) 00000031 00000000 c0743ff8 47fcff58 00268ce0 c15e70ac 0000005b >> 00000031 >> (XEN) ffeff000 c15e70ac 0000005b c15e70ac c074400c 00000031 00000000 >> c0743ff8 >> (XEN) 00000000 0000001f ffffffff 00000000 c074401c 200001d3 93830007 >> 00000000 >> (XEN) c161cac0 c161cac0 c1501de0 c0735640 c161cacc c161cacc c161cad8 >> c161cad8 >> (XEN) 00000000 00000000 00000000 00000000 00000000 c161cae4 c161cae4 >> 400001d3 >> (XEN) 00000000 00000000 00000000 00000000 00000000 dfdfdfcf cfdfdfdf >> (XEN) Xen call trace: >> (XEN) [<00264dc0>] do_trap_guest_sync+0x161c/0x1804 (PC) >> (XEN) [<00258dec>] access_guest_memory_by_ipa+0x25c/0x284 (LR) >> (XEN) [<00268ce0>] entry.o#return_from_trap+0/0x4 >> (XEN) >> (XEN) >> (XEN) **************************************** >> (XEN) Panic on CPU 0: >> (XEN) Xen BUG at traps.c:2737 >> (XEN) **************************************** >> (XEN) >> (XEN) Reboot in five seconds... >> >> The IPA 0xffffffffa13aebfc is not valid for the domain. >> >> Cheers, >> >> [1] >> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c >> index 4ee07fcea3..89c5ebf3cf 100644 >> --- a/xen/arch/arm/guestcopy.c >> +++ b/xen/arch/arm/guestcopy.c >> @@ -139,6 +139,8 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t >> gpa, void *buf, >> return -EINVAL; >> } >> >> + printk("%s: gpa 0x%llx\n", __FUNCTION__, gpa); >> + >> page = get_page_from_gfn(d, paddr_to_pfn(gpa), &p2mt, P2M_ALLOC); >> if ( !page ) >> { >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >> index c07999b518..904abafcae 100644 >> --- a/xen/arch/arm/traps.c >> +++ b/xen/arch/arm/traps.c >> @@ -2688,6 +2688,8 @@ static bool try_map_mmio(gfn_t gfn) >> return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c); >> } >> >> +#include <asm/guest_walk.h> >> + >> static void do_trap_data_abort_guest(struct cpu_user_regs *regs, >> const union hsr hsr) >> { >> @@ -2725,6 +2727,17 @@ static void do_trap_data_abort_guest(struct >> cpu_user_regs *regs, >> return; /* Try again */ >> } >> >> + { >> + paddr_t ipa, pipa; >> + rc = gva_to_ipa(info.gva, &info.ipa, GV2M_READ); There is no ipa field in mmio_info_t. But even if you used info.gpa instead, the test that you have provided is unfortunately flawed: >> + BUG_ON(rc); >> + printk("guest_walk_tables: gva 0x%x pipa 0x%llx\n", >> + info.gva, pipa); >> + rc = guest_walk_tables(current, info.gva, &ipa, NULL); >> + BUG_ON(rc); >> + BUG_ON(ipa != pipa); In your test-case you don't initialize pipa at all, however you test for it in BUG_ON, which is the reason why it fails. I have adopted your test case and it runs on ARMv7 (non-LPAE guest) and ARMv8 (LPAE guest) without any issues. It would be great if you would verify this behaviour by applying the following patch to the arm-gpt-walk-v7 patch [0] as before: From a28db6321780c442b1c97aa78883dccbd84de7dd Mon Sep 17 00:00:00 2001 From: Sergej Proskurin <proskurin@xxxxxxxxxxxxx> Date: Tue, 8 Aug 2017 13:30:00 +0200 Subject: [PATCH] Julien Grall's test case --- xen/arch/arm/guestcopy.c | 2 ++ xen/arch/arm/traps.c | 13 +++++++++++++ 2 files changed, 15 insertions(+) diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c index 4ee07fcea3..f2758ebd45 100644 --- a/xen/arch/arm/guestcopy.c +++ b/xen/arch/arm/guestcopy.c @@ -139,6 +139,8 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t gpa, void *buf, return -EINVAL; } + printk("%s: gpa 0x%"PRIpaddr"\n", __FUNCTION__, gpa); + page = get_page_from_gfn(d, paddr_to_pfn(gpa), &p2mt, P2M_ALLOC); if ( !page ) { diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index c07999b518..9b0b79a3fe 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -2688,6 +2688,8 @@ static bool try_map_mmio(gfn_t gfn) return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c); } +#include <asm/guest_walk.h> + static void do_trap_data_abort_guest(struct cpu_user_regs *regs, const union hsr hsr) { @@ -2725,6 +2727,17 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs, return; /* Try again */ } + { + paddr_t ipa; + rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ); + BUG_ON(rc); + printk("guest_walk_tables: gva 0x%"PRIvaddr" pipa 0x%"PRIpaddr"\n", + info.gva, info.gpa); + rc = guest_walk_tables(current, info.gva, &ipa, NULL); + BUG_ON(rc); + BUG_ON(ipa != info.gpa); + } + switch ( fsc ) { case FSC_FLT_PERM: -- 2.13.3 Thanks, ~Sergej [0] https://github.com/sergej-proskurin/xen (branch arm-gpt-walk-v7) _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.