Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751113AbdHRIuc (ORCPT <rfc822;w@1wt.eu>);
        Fri, 18 Aug 2017 04:50:32 -0400
Received: from mga14.intel.com ([192.55.52.115]:64377 "EHLO mga14.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1750709AbdHRIu3 (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 18 Aug 2017 04:50:29 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.41,392,1498546800"; 
   d="scan'208";a="141179076"
Subject: Re: [PATCH v1 4/4] KVM: MMU: Expose the LA57 feature to VM.
To: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.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>
Cc: linux-kernel@vger.kernel.org, rkrcmar@redhat.com, tglx@linutronix.de,
        mingo@redhat.com, hpa@zytor.com, xiaoguangrong@tencent.com,
        joro@8bytes.org
From: Yu Zhang <yu.c.zhang@linux.intel.com>
Message-ID: <fb4f529b-17d2-494b-818e-45786c55f9e8@linux.intel.com>
Date: Fri, 18 Aug 2017 16:28:04 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <517efada-20b8-5746-e62c-cca6f2a8a274@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2043
Lines: 60



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
>