[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 11:17, Jan Beulich wrote: >>>> 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. 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |