On 07/03/2023 17:44, Oliver Upton wrote:
> Hi Ryan,
>
> Thanks for fixing this. Couple of nits:
>
> On Tue, Feb 28, 2023 at 05:07:56PM +0000, Ryan Roberts wrote:
>> The high bits [51:48] of a physical address should appear at [15:12] in
>> a 64K pte, not at [51:48] as was previously being programmed. Fix this
>> with new helper functions that do the conversion correctly. This also
>> sets us up nicely for adding LPA2 encodings in future.
>>
>> Fixes: 7a6629ef746d ("kvm: selftests: add virt mem support for aarch64")
>>
>> Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>
>
> nit: no whitespace between footers.
Sorry my bad; I'm slowly learning the conventions - I'll get there eventually!
>
>> ---
>> .../selftests/kvm/lib/aarch64/processor.c | 32 ++++++++++++++-----
>> 1 file changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
>> index 5972a23b2765..13f28d96521c 100644
>> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
>> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
>> @@ -58,10 +58,27 @@ static uint64_t pte_index(struct kvm_vm *vm, vm_vaddr_t gva)
>> return (gva >> vm->page_shift) & mask;
>> }
>>
>> -static uint64_t pte_addr(struct kvm_vm *vm, uint64_t entry)
>> +static uint64_t addr_pte(struct kvm_vm *vm, uint64_t pa, uint64_t attrs)
>> {
>> - uint64_t mask = ((1UL << (vm->va_bits - vm->page_shift)) - 1) << vm->page_shift;
>> - return entry & mask;
>> + uint64_t pte;
>> +
>> + pte = pa & GENMASK(47, vm->page_shift);
>> + if (vm->page_shift == 16)
>> + pte |= (pa & GENMASK(51, 48)) >> (48 - 12);
>
> nit: this is a bit of an odd transformation, of course courtesy of the
> architecture. FIELD_GET() might make it a bit more readable:
>
> pte |= FIELD_GET(GENMASK(51, 48), pa) << 12;
>
Ahh yes, that does look better. I did consider this originally, but thought I
would also need FIELD_PREP() which selftests is not currently using anywhere. So
thought I would steer clear entirely. Anyway, on review, I don't need
FIELD_PREP(), so I've just sent you a respun series using FIELD_GET() in all the
sensible places. I hope I'm not jumping the gun by respinning so quickly - I
didn't think the series was particularly controversial so unlikely to get any
more comments.
