From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86: don't drop guest visible state updates when 64-bit PV guest is in user mode Date: Tue, 21 Jan 2014 17:06:03 +0000 Message-ID: <52DEA8FB.7010307@citrix.com> References: <52DEA16B02000078001156E8@nat28.tlf.novell.com> <52DEA69D.4010102@citrix.com> <52DEB6170200007800115796@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1W5em8-0007K4-Nc for xen-devel@lists.xenproject.org; Tue, 21 Jan 2014 17:06:08 +0000 In-Reply-To: <52DEB6170200007800115796@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 21/01/2014 17:01, Jan Beulich wrote: >>>> On 21.01.14 at 17:55, Andrew Cooper wrote: >> On 21/01/2014 15:33, Jan Beulich wrote: >>> Since 64-bit PV uses separate kernel and user mode page tables, kernel >>> addresses (as usually provided via VCPUOP_register_runstate_memory_area >>> and possibly via VCPUOP_register_vcpu_time_memory_area) aren't >>> necessarily accessible when the respective updating occurs. Add logic >>> for toggle_guest_mode() to take care of this (if necessary) the next >>> time the vCPU switches to kernel mode. >>> >>> Signed-off-by: Jan Beulich >>> >>> --- a/xen/arch/x86/domain.c >>> +++ b/xen/arch/x86/domain.c >>> @@ -1323,10 +1323,10 @@ static void paravirt_ctxt_switch_to(stru >>> } >>> >>> /* Update per-VCPU guest runstate shared memory area (if registered). */ >>> -static void update_runstate_area(struct vcpu *v) >>> +bool_t update_runstate_area(const struct vcpu *v) >> Can you adjust the comment to indicate what the return value means. The >> logic is quite opaque, but I believe it is "true if the runstate has >> been suitably updated". > Sigh - yes, I could of course. But it looks quite obvious to me. > >>> --- a/xen/arch/x86/x86_64/traps.c >>> +++ b/xen/arch/x86/x86_64/traps.c >>> @@ -266,6 +266,18 @@ void toggle_guest_mode(struct vcpu *v) >>> #else >>> write_ptbase(v); >>> #endif >>> + >>> + if ( !(v->arch.flags & TF_kernel_mode) ) >>> + return; >>> + >>> + if ( v->arch.pv_vcpu.need_update_runstate_area && >>> + update_runstate_area(v) ) >>> + v->arch.pv_vcpu.need_update_runstate_area = 0; >>> + >>> + if ( v->arch.pv_vcpu.pending_system_time.version && >>> + update_secondary_system_time(v, >>> + &v->arch.pv_vcpu.pending_system_time) ) >>> + v->arch.pv_vcpu.pending_system_time.version = 0; >> What would happen now if a guest kernel loads a faulting address for its >> runstate info (or edits its pagetables so a previously good address now >> faults)? It appears as if we could retry writing to the same bad >> address each time we try to toggle into kernel mode. >> >> Given that we know we are running on the correct set of pagetables, I >> think both of these pending variable can be unconditionally cleared, >> whether or not update{runstate_area,secondary_system_time} succeed or fail. > We could, but do you really think there's much harm in us keeping > this logically consistent? The only entity impacted is going to be > the "offending" domain, as we're wasting more of its time doing the > mode switch for it. > > Jan > I suppose not - an extra pagefault or two for a toggle into kernel mode is hardly the biggest of overheads. Reviewed-by: Andrew Cooper