[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [Patch 2/2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()
>>> On 07.10.13 at 11:48, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |