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: Thu, 04 Sep 2014 19:56:45 +0300 Message-ID: <540899CD.9000508@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XPaL6-0005WT-Su for xen-devel@lists.xenproject.org; Thu, 04 Sep 2014 16:56:53 +0000 In-Reply-To: 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" , "Nakajima, Jun" , "Dong, Eddie" , "ian.jackson@eu.citrix.com" , "tim@xen.org" , "jbeulich@suse.com" , "andrew.cooper3@citrix.com" List-Id: xen-devel@lists.xenproject.org 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 have no problem at all reverting the patch back to that implementation if the consensus becomes that that design is preferable. >> It's difficult to check for this case beforehand, because at the time of >> the per-domain hypercall we don't know which VCPU will end up having to >> do the work of injecting the trap. > > If the policy is simply new injection needs get EBUSY as long as a valid > earlier injection request is pending, then you need scan both domain > structure and all vcpu structures. Currently you only do that for domain > structure. I just want to see a consistent policy. :-) I'll run some tests tomorrow and switch to that policy for the next version of the series if it works, and if you don't decide that the original design (xc_domain_request_pagefault()) is more appropriate here. Please let me know. Thanks, Razvan Cojocaru