From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v10 4/9] x86/hvm: loosen up the ASSERT in hvm_cr4_guest_reserved_bits and hvm_efer_valid Date: Tue, 8 Dec 2015 14:43:09 +0000 Message-ID: <5666EC7D.6090206@citrix.com> References: <1449506917-26426-1-git-send-email-roger.pau@citrix.com> <1449506917-26426-5-git-send-email-roger.pau@citrix.com> <5666A2C302000078000BD0A9@prv-mh.provo.novell.com> <5666C0F6.5030802@citrix.com> <5666E10D02000078000BD2FD@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a6JUZ-0002xr-Qq for xen-devel@lists.xenproject.org; Tue, 08 Dec 2015 14:43:47 +0000 In-Reply-To: <5666E10D02000078000BD2FD@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel@lists.xenproject.org, Roger Pau Monne List-Id: xen-devel@lists.xenproject.org On 08/12/15 12:54, Jan Beulich wrote: >>>> On 08.12.15 at 12:37, wrote: >> On 08/12/15 08:28, Jan Beulich wrote: >>>>>> On 07.12.15 at 17:48, wrote: >>>> --- a/xen/arch/x86/hvm/hvm.c >>>> +++ b/xen/arch/x86/hvm/hvm.c >>>> @@ -1842,7 +1842,7 @@ static const char * hvm_efer_valid(const struct vcpu >> *v, uint64_t value, >>>> { >>>> unsigned int level; >>>> >>>> - ASSERT(v == current); >>>> + ASSERT(v->domain == current->domain); >>>> hvm_cpuid(0x80000000, &level, NULL, NULL, NULL); >>>> if ( level >= 0x80000001 ) >>>> { >>>> @@ -1912,7 +1912,7 @@ static unsigned long hvm_cr4_guest_reserved_bits(const >> struct vcpu *v, >>>> { >>>> unsigned int level; >>>> >>>> - ASSERT(v == current); >>>> + ASSERT(v->domain == current->domain); >>>> hvm_cpuid(0, &level, NULL, NULL, NULL); >>>> if ( level >= 1 ) >>>> hvm_cpuid(1, NULL, NULL, &leaf1_ecx, &leaf1_edx); >>> The only reason both functions get v passed are the two ASSERT()s. >>> Relaxing them means you should now be passing a const struct >>> domain * instead. >> v is correct here. cpuid information is genuinely per-vcpu, although >> this isn't currently reflected in Xen's understanding of the matter. > You can't have both: Either the ASSERT() remains as it was, or > you stand on the position voiced along with your R-b that only > the domain matters here (in which case passing around v just to > obtain v->domain is bogus). hvm_cpuid() uses current. This behavior is problematic and I will be removing its dependence on current with future work (passing v instead and tweaking some of the lower level bits), but at the moment it is hard requirement. As a result, the ASSERT(v == current) was correct. Roger is introducing a new codepath where v != current, but v->domain == current->domain. The specific cpuid leafs requested from hvm_cpuid() do indeed only use current->domain, which is the basis of my R-b It is very definitely wrong to remove the current check; the ASSERT() needs to stay. However, the relaxation of the constraint is acceptable until the cpuid infrastructure gets fixed up, at which point the ASSERT() can disappear. As v is the eventual correct parameter to be passed here, I do not want to see it changed to a domain, just for me to revert that change shortly. Therefore, the original patch is correct and I stand by my R-b. ~Andrew