From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V2 4/5] xen, libxc: Request page fault injection via libxc Date: Fri, 05 Sep 2014 12:34:18 +0300 Message-ID: <5409839A.9090202@bitdefender.com> References: <1409743241-14633-1-git-send-email-rcojocaru@bitdefender.com> <1409743241-14633-4-git-send-email-rcojocaru@bitdefender.com> <54081ABA.8040007@bitdefender.com> <54081C5D.2050206@bitdefender.com> <540899CD.9000508@bitdefender.com> <54089B20.6070102@bitdefender.com> <5409802C.90103@bitdefender.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 1XPpuR-00033l-Qr for xen-devel@lists.xenproject.org; Fri, 05 Sep 2014 09:34:24 +0000 Received: from smtp03.buh.bitdefender.org (smtp.bitdefender.biz [10.17.80.77]) by mx-sr.buh.bitdefender.com (Postfix) with ESMTP id 8B5C480062 for ; Fri, 5 Sep 2014 12:34:20 +0300 (EEST) In-Reply-To: <5409802C.90103@bitdefender.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: "Tian, Kevin" , "xen-devel@lists.xenproject.org" Cc: "keir@xen.org" , "ian.campbell@citrix.com" , "stefano.stabellini@eu.citrix.com" , "Dong, Eddie" , "ian.jackson@eu.citrix.com" , "tim@xen.org" , "jbeulich@suse.com" , "Nakajima, Jun" , "andrew.cooper3@citrix.com" List-Id: xen-devel@lists.xenproject.org On 09/05/2014 12:19 PM, Razvan Cojocaru wrote: > On 09/04/2014 08:08 PM, Tian, Kevin wrote: >>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com] >>> Sent: Thursday, September 04, 2014 10:02 AM >>> >>> On 09/04/14 19:56, Razvan Cojocaru wrote: >>>> On 09/04/14 18:58, Tian, Kevin wrote: >>>>>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com] >>>>>> Sent: Thursday, September 04, 2014 1:02 AM >>>>>> >>>>>> On 09/04/2014 10:54 AM, Razvan Cojocaru wrote: >>>>>>> On 09/04/2014 04:25 AM, Tian, Kevin wrote: >>>>>>>>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com] >>>>>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >>>>>>>>> index 5761ff9..5d3e4d4 100644 >>>>>>>>> --- a/xen/arch/x86/hvm/hvm.c >>>>>>>>> +++ b/xen/arch/x86/hvm/hvm.c >>>>>>>>> @@ -420,6 +420,31 @@ static bool_t hvm_wait_for_io(struct >>>>>>>>> hvm_ioreq_vcpu *sv, ioreq_t *p) >>>>>>>>> return 1; >>>>>>>>> } >>>>>>>>> >>>>>>>>> +static bool_t hvm_is_pf_requested(struct vcpu *v) >>>>>>>>> +{ >>>>>>>>> + const struct domain *d = v->domain; >>>>>>>>> + struct segment_register seg; >>>>>>>>> + uint64_t mask; >>>>>>>>> + >>>>>>>>> + hvm_get_segment_register(v, x86_seg_ss, &seg); >>>>>>>>> + >>>>>>>>> + if ( seg.attr.fields.dpl != 3 ) /* Guest is not in user mode */ >>>>>>>>> + return 0; >>>>>>>>> + >>>>>>>>> + if ( hvm_long_mode_enabled(v) ) >>>>>>>>> + mask = PADDR_MASK & PAGE_MASK; /* Bits 51:12. */ >>>>>>>>> + else if ( hvm_pae_enabled(v) ) >>>>>>>>> + mask = 0x00000000ffffffe0; /* Bits 31:5. */ >>>>>>>>> + else >>>>>>>>> + mask = (uint32_t)PAGE_MASK; /* Bits 31:12. */ >>>>>>>>> + >>>>>>>>> + if ( (v->arch.hvm_vcpu.guest_cr[3] & mask) != >>>>>>>>> + (d->arch.hvm_domain.inject_trap.cr3 & mask) ) >>>>>>>>> + return 0; >>>>>>>>> + >>>>>>>>> + return 1; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> void hvm_do_resume(struct vcpu *v) >>>>>>>>> { >>>>>>>>> struct domain *d = v->domain; >>>>>>>>> @@ -451,6 +476,15 @@ void hvm_do_resume(struct vcpu *v) >>>>>>>>> } >>>>>>>>> >>>>>>>>> /* Inject pending hw/sw trap */ >>>>>>>> >>>>>>>> you want to make comment more introspection related. >>>>>>>> >>>>>>>>> + if ( d->arch.hvm_domain.inject_trap.vector != -1 && >>>>>>>>> + v->arch.hvm_vcpu.inject_trap.vector == -1 && >>>>>>>>> + hvm_is_pf_requested(v) ) >>>>>>>>> + { >>>>>>>>> + hvm_inject_trap(&d->arch.hvm_domain.inject_trap); >>>>>>>>> + d->arch.hvm_domain.inject_trap.vector = -1; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + /* Inject pending hw/sw trap */ >>>>>>>>> if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) >>>>>>>>> { >>>>>>>>> hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap); >>>>>>>>> @@ -1473,9 +1507,10 @@ int hvm_domain_initialise(struct domain >>> *d) >>>>>>>>> printk(XENLOG_G_INFO "PVH guest must have HAP >>>>>> on\n"); >>>>>>>>> return -EINVAL; >>>>>>>>> } >>>>>>>>> - >>>>>>>>> } >>>>>>>>> >>>>>>>>> + d->arch.hvm_domain.inject_trap.vector = -1; >>>>>>>>> + >>>>>>>> >>>>>>>> A question here. With new design, now you have two places which may >>>>>> cache >>>>>>>> fault injection information. If unfortunately two consecutive hypercalls >>> with >>>>>>>> one having vcpuid=-1 and the other having vcpuid=n, it's possible two >>>>>> injections >>>>>>>> will be handled together if same vcpu is chosen for two pending >>> injections. >>>>>>>> >>>>>>>> I think the hypercall needs a check of all previous pending requests, not >>>>>> only >>>>>>>> in domain specific structure as what you did in this version. >>>>>> >>>>>> If two consecutive hypercalls with a wildcard, and then a specific >>>>>> vcpuid take place, the per-domain injection data will indeed be set >>>>>> along with the VCPU-specific injection data. >>>>> >>>>> I'm thinking one hypercall with a wildchard, while another hypercall >>>>> with a specific vcpuid. >>>> >>>> If I understand this correctly, that's what my original design did, with >>>> xc_domain_request_pagefault(). Please see: >>>> >>>> http://lists.xen.org/archives/html/xen-devel/2014-08/msg02782.html >>>> >>>> But it has been argued that Xen already has HVMOP_inject_trap and that a >>>> new hypercall should only be added if it's not possible to extend the >>>> existing one. >>> >>> I might have misunderstood your comment - you were probably just >>> clarifying the sequence of hypercall calls, not suggesting implementing >>> a separate hypercall for the wildcard case. >>> >>> >> >> Exactly. Just want to see a consistent policy. In current hypercall implementation, >> you check pending domain structure which implies only one pending injection >> allowed. But with vcpu structure it's possible to have two or even more pending >> in the same time. Need clarify your policy and make it consistent. :-) > > I've modified the code to do this: > > rc = -ENOENT; > > if ( tr.vcpuid == (uint32_t)~0 ) /* Any VCPU. */ > { > unsigned int i; > > for ( i = 0; i < d->max_vcpus; i++ ) > if ( (v = d->vcpu[i]) != NULL && > v->arch.hvm_vcpu.inject_trap.vector == -1 ) > { > printk("Busy cpu\n"); > rc = -EBUSY; > break; > } Please ignore my previous message, the test should have been, of course, v->arch.hvm_vcpu.inject_trap.vector != -1. Will do more testing and send a new series. Thanks, Razvan Cojocaru