Subject: Re: [PATCH v1 4/4] KVM: MMU: Expose the LA57 feature to VM.
To: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, rkrcmar@redhat.com, tglx@linutronix.de,
        mingo@redhat.com, hpa@zytor.com, xiaoguangrong@tencent.com,
        joro@8bytes.org
References: <1502544906-1108-1-git-send-email-yu.c.zhang@linux.intel.com>
 <1502544906-1108-5-git-send-email-yu.c.zhang@linux.intel.com>
 <db1f8c46-de64-ee2b-6694-583111dec136@redhat.com>
 <040155e8-510a-3ada-9d83-f4b489d0981f@linux.intel.com>
 <517efada-20b8-5746-e62c-cca6f2a8a274@redhat.com>
 <fb4f529b-17d2-494b-818e-45786c55f9e8@linux.intel.com>
 <832539da-a1cd-9bf3-0f5b-e46d65e9ff3d@redhat.com>
 <13ecdbdb-8770-27a7-aea9-f143be91fa78@linux.intel.com>
 <5f56a5dd-e2ca-07f4-26bf-42fe8e18d901@redhat.com>
From: Yu Zhang <yu.c.zhang@linux.intel.com>
Message-ID: <5779211d-7f4c-8f06-153c-a7c0e595582d@linux.intel.com>
Date: Mon, 21 Aug 2017 20:11:59 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <5f56a5dd-e2ca-07f4-26bf-42fe8e18d901@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3123
Lines: 81


On 8/21/2017 6:12 PM, Paolo Bonzini wrote:
> On 21/08/2017 09:27, Yu Zhang wrote:
>>
>> On 8/18/2017 8:50 PM, Paolo Bonzini wrote:
>>> On 18/08/2017 10:28, Yu Zhang wrote:
>>>> 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);
>>> The "~0u" part is simply because max_size has 32-bit size (it's an
>>> unsigned int variable), while (1ull << 48) - la has 64-bit size.  It
>>> protects from the overflow.
>> But what if value of "la" falls in between 0xFFFFFFFFFFFFFFFF and
>> 0xFFFF000000000000? (1ull << 48) - la may result in something between
>> 0x1000000000001 and> 0x2000000000000, and the *max_size would be 4G - 1
>> in this scenario.  For instance, when "la" is 0xFFFFFFFFFFFFFFF0 (unlikely
>> in practice though), the *max_size we are expecting should be 15, instead
>> of 4G - 1.
> No, it is possible to wrap a memory access from the top half of the
> address space to the bottom half, so there's no limit at 0xFFFFFFFFFFFFFFF0.

Oh? So you mean it is allowed for one instruction to reside both in the 
top half of
the address space and in the bottom half?

If this is possible, I guess the code should be

*max_size = min_t(u64, ~0u, (1ull << va_bits) - la);

But I wonder, why should such scenario be allowed? :-)

Thanks
Yu


>
> Paolo
>