From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v1 4/4] KVM: MMU: Expose the LA57 feature to VM. Date: Fri, 18 Aug 2017 14:50:09 +0200 Message-ID: <832539da-a1cd-9bf3-0f5b-e46d65e9ff3d@redhat.com> 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> <040155e8-510a-3ada-9d83-f4b489d0981f@linux.intel.com> <517efada-20b8-5746-e62c-cca6f2a8a274@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: linux-kernel@vger.kernel.org, rkrcmar@redhat.com, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, xiaoguangrong@tencent.com, joro@8bytes.org To: Yu Zhang , kvm@vger.kernel.org Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 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. > 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. Yes, exactly. But since emul_is_noncanonical_address is already using ctxt_virt_addr_bits(ctxt), it may make sense to compute ctxt_virt_addr_bits(ctxt) once and then reuse it twice, once in get_canonical(la, va_bits) != la and once in (1ull << va_bits) - la. Paolo > Is this understanding correct? Or did I misunderstand your comments? :-) > > Thanks > Yu >> Paolo >> >