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

[Julien Grall <julien.grall@xxxxxxx>]

On 08/08/17 13:33, Julien Grall wrote:
> 
> 
> On 08/08/17 13:17, Sergej Proskurin wrote:
>>>> 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:
> 
> Well, I copied the wrong code... info.ipa should be replaced by pipa.
> 
>>>> +        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:
> 
> I am afraid that whilst there was a bug in the code to compare ipa !=
> pipa. If you looked at the log I provided,  it was failing before:
> 
> d0: guestcopy: failed to get table entry.
> 
> And this does not even involve pipa... If you wonder your patch below
> does not help it also.

The patch belows solve my problem:

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index b258248322..6ca994e438 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -112,7 +112,7 @@ static int guest_walk_sd(const struct vcpu *v,
          * level translation table does not need to be page aligned.
          */
         mask = GENMASK(19, 12);
-        paddr = (pte.walk.base << 10) | ((gva & mask) >> 10);
+        paddr = ((paddr_t)pte.walk.base << 10) | ((gva & mask) >> 10);
 
         /* Access the guest's memory to read only one PTE. */
         ret = access_guest_memory_by_ipa(d, paddr, &pte, sizeof(short_desc_t), 
false);

This is because pte.walk.base is encoded on unsigned int:22 bits. A shift by 10 
will not
fit an integer, and my compiler seems to promote it to "signed long long". 
Hence the bogus
address.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel