From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [Patch 2/2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs() Date: Mon, 07 Oct 2013 11:17:57 +0100 Message-ID: <5252A67502000078000F92F2@nat28.tlf.novell.com> References: <1381139316-1820-1-git-send-email-andrew.cooper3@citrix.com> <1381139316-1820-3-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VT7t3-0007fk-SW for xen-devel@lists.xenproject.org; Mon, 07 Oct 2013 10:18:02 +0000 In-Reply-To: <1381139316-1820-3-git-send-email-andrew.cooper3@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org >>> 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. > 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_...". > { > *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? > { > 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