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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.07.14 at 14:34, <rcojocaru@xxxxxxxxxxxxxxx> 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?

> +    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?

> +         && !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;
> +
> +        /* 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).

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -967,6 +967,32 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>      }
>      break;
>  
> +    case XEN_DOMCTL_set_pagefault_info:
> +    {
> +        struct domain *d;
> +
> +        ret = -ESRCH;
> +        d = rcu_lock_domain_by_id(op->domain);
> +        if ( d != NULL )
> +        {
> +            ret = -EINVAL;
> +
> +            if ( has_hvm_container_domain(d) )

This is inconsistent with the earlier use of is_hvm_domain(). You
ought to decide whether you want PVH to be supported.

> @@ -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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel