From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [Patch 2/2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs() Date: Mon, 7 Oct 2013 11:51:58 +0100 Message-ID: <5252924E.9020707@citrix.com> References: <1381139316-1820-1-git-send-email-andrew.cooper3@citrix.com> <1381139316-1820-3-git-send-email-andrew.cooper3@citrix.com> <5252A67502000078000F92F2@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VT8Pz-0001SO-Bm for xen-devel@lists.xenproject.org; Mon, 07 Oct 2013 10:52:03 +0000 In-Reply-To: <5252A67502000078000F92F2@nat28.tlf.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 , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 07/10/13 11:17, Jan Beulich wrote: >>>> On 07.10.13 at 11:48, Andrew Cooper wrote: >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -595,55 +595,45 @@ DO_ERROR_NOCODE(TRAP_copro_error, coprocessor_error) >> DO_ERROR( TRAP_alignment_check, alignment_check) >> DO_ERROR_NOCODE(TRAP_simd_error, simd_coprocessor_error) >> >> +/* Returns 1 if handled, 0 if not and -Exx for error. */ > This comment is not in line with all current uses of the function. > Either you fix the comment, or you fix the callers. Hmm yes - the error case isn't dealt with properly. I shall fix the comment in preference to editing the callsites at the moment. I have a separate proposal for a change in the way this msr handling code works, and will fix this up then. > >> int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val) >> { >> struct domain *d = current->domain; >> /* Optionally shift out of the way of Viridian architectural MSRs. */ >> uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000; >> >> - idx -= base; >> - if ( idx > 0 ) >> - return 0; >> - >> - switch ( idx ) >> + switch ( idx - base ) >> { >> - case 0: >> + case 0: /* Write hypercall page */ > This comment looks more confusing that clarifying considering that > we're in "rdmsr_...". Very true. I shall adjust. > >> { >> *val = 0; >> - break; >> + return 1; >> } >> default: >> - BUG(); >> + return 0; > In a situation like this I think it is better to not have a "default:" at > all, and instead have the "return" at the end of the function deal > with all cases not getting handled inside the switch statement. > But yes, this is a matter of taste. > >> } >> - >> - return 1; >> } >> >> +/* Returns 1 if handled, 0 if not and -Exx for error. */ >> int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val) >> { >> struct domain *d = current->domain; >> /* Optionally shift out of the way of Viridian architectural MSRs. */ >> uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000; >> >> - idx -= base; >> - if ( idx > 0 ) >> - return 0; >> - >> - switch ( idx ) >> + switch ( idx - base ) >> { >> - case 0: >> + case 0: /* Write hypercall page */ >> { >> void *hypercall_page; >> - unsigned long gmfn = val >> 12; >> - unsigned int idx = val & 0xfff; >> + unsigned long gmfn = val >> PAGE_SHIFT; >> struct page_info *page; >> p2m_type_t t; >> >> - if ( idx > 0 ) >> + if ( val & PAGE_MASK ) > Did you mean ~PAGE_MASK? And in the light of this - did you test > the change? I did indeed mean ~PAGE_MASK, but also had that in the version of the code tested. I think I lost that in a botched rebase, and shall try to be more careful in the future. > >> { >> gdprintk(XENLOG_WARNING, >> - "Out of range index %u to MSR %08x\n", >> - idx, 0x40000000); >> + "Expected aligned frame for writing hypercall page\n"); > I don't think that's the intention here. Instead the low bits are > specifying the n-th hypercall page, and hence talking about > alignment here seems wrong. > > Jan > Is this documented anywhere? The cpuid documentation describes how to locate the base address. Looking at things more closely, that does make sense. I did mistake it for an alignment check, given unconditionally 1 hypercall page. I will return it back to what it was intended, but without shadowing the idx function parameter. ~Andrew