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

["Jan Beulich" <JBeulich@xxxxxxxx>]

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3114,6 +3114,59 @@ out:
>          nvmx_idtv_handling();
>  }
>  
> +static bool_t vmx_check_pf_injection(void)
> +{
> +    struct vcpu *curr = current;
> +    struct domain *d = curr->domain;

currd please.

> +    struct segment_register seg;
> +    unsigned long ev;
> +    uint32_t pending_event = 0;
> +
> +    if ( !is_hvm_domain(d) ||
> +         likely(d->arch.hvm_domain.fault_info.valid == 0) )

The "valid" field being a boolean one, please use ! instead of == 0.

> +        return 0;
> +
> +    hvm_get_segment_register(curr, x86_seg_ss, &seg);

You can directly use the VMX variant here I think, or abstract out the
generic parts.

> +
> +    if ( seg.attr.fields.dpl != 3 ) /* Guest is not in user mode */
> +        return 0;
> +
> +

Double blank line.Didn't I ask you to clean up your patches in this
regard already?

> +    if ( curr->arch.hvm_vcpu.guest_cr[3]
> +         != d->arch.hvm_domain.fault_info.address_space )

Do you really mean to compare the full CR3 value here, rather than
just bits 12...51? In which case the address_space field likely would
better be a GPFN.

> +        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);

If you pulled this up (I don't think it needs to be here), it would
perhaps indeed be worth abstracting out the non-VMX bits from
here to a generic function.

> +    return (pending_event == 0);

Pointless parentheses.

> +}
> +
> +static void vmx_inject_pf(void)
> +{
> +    struct vcpu *curr = current;
> +    struct domain *d = curr->domain;
> +    int errcode = PFEC_user_mode;
> +    uint64_t virtual_address = d->arch.hvm_domain.fault_info.virtual_address;
> +    uint32_t write_access = d->arch.hvm_domain.fault_info.write_access;
> +
> +    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;

Are these necessary? Because ...

> +    d->arch.hvm_domain.fault_info.valid = 0;

... I would hope that this one is properly guarding all uses of the
other fields.

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -967,6 +967,34 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>      }
>      break;
>  
> +    case XEN_DOMCTL_set_pagefault_info:
> +    {
> +        struct domain *d;

There already is a suitable variable available throughout the entire
function.

> +
> +        ret = -ESRCH;
> +        d = rcu_lock_domain_by_id(op->domain);

And this is already being done at the function level too. If you're
porting patches from older versions, please pay attention to
changes in context other than just those immediately visible
during patch application.

Jan


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