Re: [PATCH v1 4/4] KVM: MMU: Expose the LA57 feature to VM.

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 8/17/2017 10:29 PM, Paolo Bonzini wrote:
> On 17/08/2017 13:53, Yu Zhang wrote:
>>
>> On 8/17/2017 7:57 PM, Paolo Bonzini wrote:
>>> On 12/08/2017 15:35, Yu Zhang wrote:
>>>> index a98b88a..50107ae 100644
>>>> --- a/arch/x86/kvm/emulate.c
>>>> +++ b/arch/x86/kvm/emulate.c
>>>> @@ -694,7 +694,7 @@ static __always_inline int __linearize(struct
>>>> x86_emulate_ctxt *ctxt,
>>>>        switch (mode) {
>>>>        case X86EMUL_MODE_PROT64:
>>>>            *linear = la;
>>>> -        if (is_noncanonical_address(la))
>>>> +        if (emul_is_noncanonical_address(la, ctxt))
>>>>                goto bad;
>>>>              *max_size = min_t(u64, ~0u, (1ull << 48) - la);
>>> Oops, you missed one here.  Probably best to use ctxt_virt_addr_bits and
>>> then "inline" emul_is_noncanonical_address as "get_canonical(la,
>>> va_bits) != la".
>> Sorry, I just sent out the v2 patch set without noticing this reply. :-)
>>
>> The emul_is_noncanonical() is defined in x86.h so that no
>> ctxt_virt_addr_bits needed in emulate.c, are you
>> suggesting to use ctx_virt_addr_bits in this file each time before
>> emul_is_noncanonical_address() is called?
> No, only in this instance which uses "48" after the call to
> emul_is_noncanonical_address.

Sorry, Paolo. I still do not quite get it.
Do you mean the
  *max_size = min_t(u64, ~0u, (1ull << 48) - la);
also need to be changed?

But I do not understand why this statement is used like this. My 
understanding is that
for 64 bit scenario, the *max_size is calculated to guarantee la + 
*max_size still falls in
the canonical address space.

And if above understanding is correct, I think it should be something 
like below:
   *max_size = min_t(u64, ~0u - la, (1ull << 48) - la);

And with LA57, may better be changed to:
   *max_size = min_t(u64, ~0u - la, (1ull << ctxt_virt_addr_bits(ctxt)) 
- la);

And for the above
   if (emul_is_noncanonical_address(la, ctxt))
we may just leave it as it is.

Is this understanding correct? Or did I misunderstand your comments? :-)

Thanks
Yu
> Paolo
>