[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()
> -----Original Message----- > From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel- > bounces@xxxxxxxxxxxxx] 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 <andrew.cooper3@xxxxxxxxxx> > CC: Keir Fraser <keir@xxxxxxx> > CC: Jan Beulich <JBeulich@xxxxxxxx> > --- > 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@xxxxxxxxxxxxx > http://lists.xen.org/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |