Re: [PATCH V2 4/5] xen, libxc: Request page fault injection via libxc

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

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