From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs() Date: Mon, 7 Oct 2013 14:15:04 +0100 Message-ID: <5252B3D8.9070902@citrix.com> References: <5252924E.9020707@citrix.com> <1381147270-15890-1-git-send-email-andrew.cooper3@citrix.com> <9AAE0902D5BC7E449B7C8E4E778ABCD012A105@AMSPEX01CL01.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <9AAE0902D5BC7E449B7C8E4E778ABCD012A105@AMSPEX01CL01.citrite.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Paul Durrant Cc: "Keir (Xen.org)" , Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org On 07/10/13 13:26, Paul Durrant wrote: >> -----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 I suppose. The use of ~PAGE_MASK before was my mistaken impression that this was an alignment check rather than a hypercall index check. As this stuff is not documented anywhere I can find (other than by reading the code), I am not sure it is caring too much about. Ideally, there would be somewhere XEN_MSR_HYPERCALL_PAGE_INDEX_MASK as 0xfff and XEN_MSR_HYPERCALL_PAGE_MAX_INDEX, but as it is unlikely that Xen will ever start using multiple hypercall pages, this sounds like overkill. I am happy to implement it however people prefer, but would err on the lazy side of leaving it alone if there is no overwhelming objection. ~Andrew