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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.08.14 at 13:30, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> +static bool_t vmx_check_pf_injection(void)
> +{
> +    struct vcpu *curr = current;
> +    struct domain *d = curr->domain;
> +    struct segment_register seg;
> +    unsigned long ev;
> +    uint32_t pending_event = 0;
> +
> +    if ( likely(d->arch.hvm_domain.fault_info.virtual_address == 0 )

Bad space before ). And my question stands: Why is VA zero
special?

> +         || !is_hvm_domain(d) )

Following the majority of other code, the || belong at the end of the
previous line. Also, even if not strictly required at present, the order
of the two checks would better be swapped.

> +        return 0;
> +
> +    hvm_get_segment_register(curr, x86_seg_ss, &seg);
> +
> +    if ( seg.attr.fields.dpl != 3 ) /* Guest is not in user mode */
> +        return 0;
> +
> +    vmx_vmcs_enter(curr);
> +
> +    if ( curr->arch.hvm_vcpu.guest_cr[3]
> +         != d->arch.hvm_domain.fault_info.address_space )
> +    {
> +        vmx_vmcs_exit(curr);
> +        return 0;
> +    }

I think the vmx_vmcs_enter() could be moved here, simplifying the error
handling above.

> +
> +    __vmread(VM_ENTRY_INTR_INFO, &ev);
> +
> +    if ( (ev & INTR_INFO_VALID_MASK) &&
> +         hvm_event_needs_reinjection((ev >> 8) & 7, ev & 0xff) )

Are there no manifest constants for all these plain numbers?

> +        pending_event = ev;
> +
> +    if ( pending_event )
> +    {
> +        vmx_vmcs_exit(curr);
> +        return 0;
> +    }
> +
> +    vmx_vmcs_exit(curr);
> +    return 1;

These two return paths would read better when folded together.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -936,6 +936,18 @@ typedef struct xen_domctl_vcpu_msrs 
> xen_domctl_vcpu_msrs_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_vcpu_msrs_t);
>  #endif
>  
> +/* XEN_DOMCTL_set_pagefault_info requests that a page fault occur at
> + * the next VMENTRY.
> + *  */

Coding style.

> +struct xen_domctl_set_pagefault_info {
> +    uint64_t address_space;
> +    uint64_t virtual_address;
> +    uint32_t write_access;
> +};

Missing padding (structure size currently differs between 32- and 64-bit
callers; this only doesn't matter much because callers normally pass a
full struct xen_domctl anyway).

Jan


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