diff mbox

[v7,00/14] arm/mem_access: Walk guest page tables in SW if mem_access is active

Message ID 0c80dc19-8f01-3561-aa05-9b34001faeb4@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Julien Grall Aug. 8, 2017, 1:24 p.m. UTC 
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:


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,

Comments

Sergej Proskurin Aug. 8, 2017, 2:47 p.m. UTC | #1 
Hi Julien,

> 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.
>


Thats quite an interesting phenomenon :) I have just played around with
this and it does indeed appear that the value is casted to a signed
result! What I don't yet understand is the following: An unsigned int
with the length of 22 bit should actually exactly fit an integer after a
left shift of 10 (or do I miss s.th.?).

Anyway, thanks for the patch! V8 containing this change will follow soon.

Thanks,
~Sergej
Andrew Cooper Aug. 8, 2017, 2:58 p.m. UTC | #2 
On 08/08/17 15:47, Sergej Proskurin wrote:
> Hi Julien,
>
>> 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.
>>
>
> Thats quite an interesting phenomenon :) I have just played around with
> this and it does indeed appear that the value is casted to a signed
> result! What I don't yet understand is the following: An unsigned int
> with the length of 22 bit should actually exactly fit an integer after a
> left shift of 10 (or do I miss s.th.?).

C type promotion ftw!

All integral types smaller than int are promoted to int before any
operations on them.  This includes things like unsigned char/short etc.

Then, the type is promoted to match that of the other operand, which
might be a wider type (e.g. long) or an unsigned version of the same type.

~Andrew
Sergej Proskurin Aug. 8, 2017, 3:04 p.m. UTC | #3 
On 08/08/2017 04:58 PM, Andrew Cooper wrote:
> On 08/08/17 15:47, Sergej Proskurin wrote:
>> Hi Julien,
>>
>>> 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.
>>>
>> Thats quite an interesting phenomenon :) I have just played around with
>> this and it does indeed appear that the value is casted to a signed
>> result! What I don't yet understand is the following: An unsigned int
>> with the length of 22 bit should actually exactly fit an integer after a
>> left shift of 10 (or do I miss s.th.?).
> C type promotion ftw!
>
> All integral types smaller than int are promoted to int before any
> operations on them.  This includes things like unsigned char/short etc.
>
> Then, the type is promoted to match that of the other operand, which
> might be a wider type (e.g. long) or an unsigned version of the same type.

Thanks Andrew, I did not know that!

Cheers,
~Sergej
diff mbox

Patch

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);