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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/24/2014 06:27 PM, Jan Beulich wrote:
>>>> On 23.07.14 at 14:34, <rcojocaru@bitdefender.com> wrote:
>> +static void check_pf_injection(void)
>> +{
>> +    struct vcpu *curr = current;
>> +    struct domain *d = curr->domain;
>> +    struct hvm_hw_cpu ctxt;
>> +    struct segment_register seg;
>> +    int errcode = PFEC_user_mode;
>> +
>> +    if ( !is_hvm_domain(d)
>> +         || d->arch.hvm_domain.fault_info.virtual_address == 0 )
>> +        return;
>> +
>> +    hvm_funcs.save_cpu_ctxt(curr, &ctxt);
> 
> Isn't this a little heavy handed?

It did seem that way to me too, but I couldn't find another way to get
to ctxt.pending_event and CR3 at this point in the code. Is there an
alternative I'm not aware of?

>> +    hvm_get_segment_register(curr, x86_seg_ss, &seg);
>> +
>> +    if ( seg.attr.fields.dpl == 3 /* Guest is in user mode */
> 
> Did you verify that this covers VM86 mode too?

Assuming CPL is SS.DPL (I've been previously been using CS.DPL), it did
work in our tests, yes.

>> +         && !ctxt.pending_event
>> +         && ctxt.cr3 == d->arch.hvm_domain.fault_info.address_space )
>> +    {
>> +        /* Cache */
> 
> Cache? Did you mean "Latch" or some such perhaps?
> 
>> +        uint64_t virtual_address = d->arch.hvm_domain.fault_info.virtual_address;
>> +        uint32_t write_access = d->arch.hvm_domain.fault_info.write_access;

I meant "cache", as in "I'm caching
d->arch.hvm_domain.fault_info.virtual_address into virtual_address"
before resetting it, but the comment is not only unimportant but
obviously confusing, so I'll take it out.

>> +        /* Reset */
>> +        d->arch.hvm_domain.fault_info.address_space = 0;
>> +        d->arch.hvm_domain.fault_info.virtual_address = 0;
>> +        d->arch.hvm_domain.fault_info.write_access = 0;
>> +
>> +        if ( write_access )
>> +            errcode |= PFEC_write_access;
>> +
>> +        hvm_inject_page_fault(errcode, virtual_address);
>> +    }
>> +}
> 
> Even together with its call site it's remaining unclear without knowing
> almost all of the rest of your code why this function would inject
> only two types of page faults (and I'm still not finally sure this is
> intended/correct). A comment would certainly help, short of a
> more descriptive name for the function (which I suppose would get
> unreasonably long).

The function, as previously suggested, will be split into the check part
and the injection part.

What our code does in this case has been briefly described here:
http://lists.xen.org/archives/html/xen-devel/2014-07/msg03013.html

>> @@ -1012,6 +1024,7 @@ struct xen_domctl {
>>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
>>  #define XEN_DOMCTL_gdbsx_domstatus             1003
>> +#define XEN_DOMCTL_set_pagefault_info          1004
> 
> That doesn't look like the range you want to extend.

Could you please elaborate? Thank you.


Thanks,
Razvan Cojocaru