Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.02.16 at 16:50, <czuzu@xxxxxxxxxxxxxxx> wrote:
> @@ -151,61 +154,52 @@ void hvm_event_guest_request(void)
>      }
>  }
>  
> -int hvm_event_int3(unsigned long rip)
> +static inline
> +uint64_t gfn_of_rip(unsigned long rip)

This should be a single line and the return value should be
unsigned long.

>  {
> -    int rc = 0;
>      struct vcpu *curr = current;
> +    struct segment_register sreg;
> +    uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
>  
> -    if ( curr->domain->arch.monitor.software_breakpoint_enabled )
> -    {
> -        struct segment_register sreg;
> -        uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
> -        vm_event_request_t req = {
> -            .reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT,
> -            .vcpu_id = curr->vcpu_id,
> -        };
> -
> -        hvm_get_segment_register(curr, x86_seg_ss, &sreg);
> -        if ( sreg.attr.fields.dpl == 3 )
> -            pfec |= PFEC_user_mode;
> +    hvm_get_segment_register(curr, x86_seg_ss, &sreg);
> +    if ( sreg.attr.fields.dpl == 3 )
> +        pfec |= PFEC_user_mode;
>  
> -        hvm_get_segment_register(curr, x86_seg_cs, &sreg);
> -        req.u.software_breakpoint.gfn = paging_gva_to_gfn(curr,
> -                                                          sreg.base + rip,
> -                                                          &pfec);
> -
> -        rc = hvm_event_traps(1, &req);
> -    }
> +    hvm_get_segment_register(curr, x86_seg_cs, &sreg);
>  
> -    return rc;
> +    return (uint64_t) paging_gva_to_gfn(curr, sreg.base + rip, &pfec);
>  }

Pointless cast.

> --- a/xen/include/asm-x86/hvm/event.h
> +++ b/xen/include/asm-x86/hvm/event.h
> @@ -17,6 +17,12 @@
>  #ifndef __ASM_X86_HVM_EVENT_H__
>  #define __ASM_X86_HVM_EVENT_H__
>  
> +enum hvm_event_breakpoint_type
> +{
> +    HVM_EVENT_SOFTWARE_BREAKPOINT,
> +    HVM_EVENT_SINGLESTEP_BREAKPOINT,
> +};

I don't see what good it does to put existing constants into an
enum.

> @@ -27,9 +33,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value,
>  #define hvm_event_crX(what, new, old) \
>      hvm_event_cr(VM_EVENT_X86_##what, new, old)
>  void hvm_event_msr(unsigned int msr, uint64_t value);
> -/* Called for current VCPU: returns -1 if no listener */
> -int hvm_event_int3(unsigned long rip);
> -int hvm_event_single_step(unsigned long rip);
> +int hvm_event_breakpoint(unsigned long rip,
> +                         enum hvm_event_breakpoint_type type);

I guess the comment was here for a reason, and this reason doesn't
go away with this code folding. But I'll leave it to the VM event code
maintainers to judge.

Jan


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