From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Durrant Subject: Re: [Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs() Date: Mon, 7 Oct 2013 12:26:40 +0000 Message-ID: <9AAE0902D5BC7E449B7C8E4E778ABCD012A105@AMSPEX01CL01.citrite.net> References: <5252924E.9020707@citrix.com> <1381147270-15890-1-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1381147270-15890-1-git-send-email-andrew.cooper3@citrix.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Xen-devel Cc: Andrew Cooper , "Keir (Xen.org)" , Jan Beulich List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- > bounces@lists.xen.org] On Behalf Of Andrew Cooper > Sent: 07 October 2013 13:01 > To: Xen-devel > Cc: Andrew Cooper; Keir (Xen.org); Jan Beulich > Subject: [Xen-devel] [Patch v3] x86/traps: improvements to {rd, > wr}msr_hypervisor_regs() > > Coverity ID: 1055249 1055250 > > Coverity was complaining that the switch statments contained dead code in > their default statements. While this is quite minor, the code flow in > wrmsr_hypervisor_regs() was sufficiently opaque that I felt it approprate to > fix. > > Other improvements include: > * not shadowing the function parameter 'idx'. > * use of PAGE_SHIFT instead of opencoded numbers. > * a more descriptive error message for attempting to write invalid indicies > for hypercall pages. > > There is no behavioural change as a result. > > Signed-off-by: Andrew Cooper > CC: Keir Fraser > CC: Jan Beulich > --- > xen/arch/x86/traps.c | 44 ++++++++++++++++++-------------------------- > 1 file changed, 18 insertions(+), 26 deletions(-) > > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c > index 6c7bd99..3bb62f0 100644 > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -595,55 +595,50 @@ 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 0 if not handled, and non-0 for error. (The calling semantics are > + * in need of some work) > + */ > 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. Reads are invalid. Hand a #GP back. */ > { > *val = 0; > - break; > + return 1; > } > - default: > - BUG(); > } > > - return 1; > + return 0; > } > > +/* 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 & 0xfff ) If you're not going to use 12, then shouldn't you use PAGE_SIZE-1 rather than 0xfff? > { > + /* Bottom 12 bits are hypercall page index. Only 0 is valid. */ And modify this comment accordingly? Paul > gdprintk(XENLOG_WARNING, > - "Out of range index %u to MSR %08x\n", > - idx, 0x40000000); > + "wrmsr hypercall page index %#lx unsupported\n", > + val & 0xfff); > return 0; > } > > @@ -671,14 +666,11 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t > val) > unmap_domain_page(hypercall_page); > > put_page_and_type(page); > - break; > + return 1; > } > - > - default: > - BUG(); > } > > - return 1; > + return 0; > } > > int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx, > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel