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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 11.08.14 at 17:08, <rcojocaru@bitdefender.com> wrote:
> +static bool_t vmx_check_pf_injection(void)
> +{
> +    struct vcpu *curr = current;
> +    const struct domain *currd = curr->domain;
> +    struct segment_register seg;
> +    unsigned long ev;
> +    uint32_t pending_event = 0;
> +    uint64_t mask;
> +
> +    if ( !is_hvm_domain(currd) ||
> +         likely(!currd->arch.hvm_domain.fault_info.valid) )
> +        return 0;
> +
> +    vmx_get_segment_register(curr, x86_seg_ss, &seg);
> +
> +    if ( seg.attr.fields.dpl != 3 ) /* Guest is not in user mode */
> +        return 0;
> +
> +    if ( hvm_long_mode_enabled(curr) )
> +        mask = 0x000ffffffffff000; /* Bits 51:12. */

PADDR_MASK & PAGE_MASK

> +    else if ( hvm_pae_enabled(curr) )
> +        mask = 0x00000000ffffffe0; /* Bits 31:5. */
> +    else
> +        mask = 0x00000000fffff000; /* Bits 31:12. */

And this would probably also better be (uint32_t)PAGE_MASK.

> +
> +    if ( (curr->arch.hvm_vcpu.guest_cr[3] & mask) !=
> +         (currd->arch.hvm_domain.fault_info.address_space & mask) )
> +        return 0;
> +
> +    vmx_vmcs_enter(curr);
> +    __vmread(VM_ENTRY_INTR_INFO, &ev);
> +
> +    if ( (ev & INTR_INFO_VALID_MASK) &&
> +         hvm_event_needs_reinjection(MASK_EXTR(ev, INTR_INFO_INTR_TYPE_MASK),
> +                                     ev & INTR_INFO_VECTOR_MASK) )
> +        pending_event = ev;
> +
> +    vmx_vmcs_exit(curr);

Again - is there a reason not to move this right after the __vmread()
above?

> +    return pending_event == 0;
> +}
> +
> +static void vmx_inject_pf(void)
> +{
> +    struct vcpu *curr = current;
> +    struct domain *currd = curr->domain;
> +    int errcode = PFEC_user_mode;

unsigned

> +    uint64_t virtual_address = currd->arch.hvm_domain.fault_info.virtual_address;
> +
> +    currd->arch.hvm_domain.fault_info.valid = 0;
> +
> +    if ( currd->arch.hvm_domain.fault_info.write_access )
> +        errcode |= PFEC_write_access;

Isn't this pretty limited a set of error codes you're able to generate
here?

> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -141,6 +141,14 @@ struct hvm_domain {
>       */
>      uint64_t sync_tsc;
>  
> +    /* Memory introspection page fault injection data. */
> +    struct {
> +        uint64_t address_space;
> +        uint64_t virtual_address;
> +        uint32_t write_access;

Is this meaningfully a 32-bit field (rather than just a boolean one)?

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -937,6 +937,19 @@ 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;

I guess you want this to be a fixed size one in the public interface,
but if only zero/non-zero counts, this could (a) be a uint8_t and
(b) get evaluated using the canonical !! when converting to the
internal structure.

Jan