Re: [PATCH v7 13/14] arm/mem_access: Add short-descriptor based gpt

[Sergej Proskurin <proskurin@sec.in.tum.de>]

On 08/08/2017 06:20 PM, Andrew Cooper wrote:
> On 08/08/17 16:28, Sergej Proskurin wrote:
>> On 08/08/2017 05:18 PM, Julien Grall wrote:
>>> On 08/08/17 16:17, Sergej Proskurin wrote:
>>>> Hi Julien,
>>>>
>>>>
>>>> On 07/18/2017 02:25 PM, Sergej Proskurin wrote:
>>>>> This commit adds functionality to walk the guest's page tables using
>>>>> the
>>>>> short-descriptor translation table format for both ARMv7 and ARMv8. The
>>>>> implementation is based on ARM DDI 0487B-a J1-6002 and ARM DDI 0406C-b
>>>>> B3-1506.
>>>>>
>>>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>>>>> Acked-by: Julien Grall <julien.grall@arm.com>
>>>> As you have already Acked this patch I would like you to ask whether I
>>>> should remove your Acked-by for now as I have extended the previous
>>>> patch by additional casts of the pte.*.base fields to (paddr_t) as
>>>> discussed in patch 00/14.
>>> I am fine with this, assuming this is the only change made.
>> The changes are limited to 4 similar casts to (paddr_t) in total and an
>> additional comment. Here are the only changes in this patch:
>>
>> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
>> index b258248322..7f34a2b1d3 100644
>> --- a/xen/arch/arm/guest_walk.c
>> +++ b/xen/arch/arm/guest_walk.c
>> @@ -112,7 +112,12 @@ 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);
>> +        /*
>> +         * Cast pte.walk.base to paddr_t to cope with C type promotion
>> of types
>> +         * smaller than int. Otherwise pte.walk.base would be casted to
>> int and
>> +         * subsequently sign extended, thus leading to a wrong value.
>> +         */
>> +        paddr = ((paddr_t)pte.walk.base << 10) | ((gva & mask) >> 10);
> Why not change the bitfield type from unsigned int to paddr_t ?
>
> The result is 100% less liable to go wrong in this way.
>

I absolutely agree :)

Julien, would that be ok for you if I changed the type of the base field
in short_desc_* structs accordingly? Or shall I remove your Acked-by for
this?

Thanks,
~Sergej

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