Re: [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU <czuzu@bitdefender.com>]

On 2/18/2016 9:35 PM, Corneliu ZUZU wrote:
> This patch adds ARM support for guest-request monitor vm-events.
>
> Summary of changes:
> == Moved to common-side:
>    * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86
>        arch_monitor_domctl_event to common monitor_domctl)
>    * hvm_event_guest_request, hvm_event_traps (also added target vcpu as param)
>    * guest-request bits from X86 'struct arch_domain' (to common 'struct domain')
> == ARM implementations:
>    * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls
>        hvm_event_guest_request (as on X86)
>    * arch_monitor_get_capabilities: updated to reflect support for
>        XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST
>    * vm_event_init_domain (does nothing), vm_event_cleanup_domain
> == Misc:
>    * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer
>        X86-specific. ARM-side implementation of this function currently does
>        nothing, that will be added in a separate patch.
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>

Before sending in the next revision of this patch, I have a few 
questions I'd like to ask.
That being the case, I thought it would be ok to also include *all* the 
changes that will be done in the next revision here for (potentially) 
additional feedback.

Already discussed changes TBD:
* Add #define altp2m_active(d) (0) and implement 
p2m_get_vcpu_altp2m_idx(v) for ARM to remove #ifdef CONFIG_X86 in 
hvm_event_traps (Stefano, Tamas)
* Remove wrong copyright comment (Jan)
* Change bitfields members type back to unsigned int (Jan)
* Move hvm_event_traps and hvm_event_guest_request to common/vm_event.c 
and rename them to vm_event_traps and vm_event_guest_request (Jan)

Questions:

1) I've noticed the practice in Xen is to prepend the arch_ prefix to 
functions that usually have a counterpart on the common-side, i.e. there 
are a lot of arch-specific functions missing this prefix. Would it then 
be advised to:
     - rename arch_monitor_get_capabilities to 
vm_event_monitor_get_capabilities and move it to vm_event.h
     - rename arch_hvm_event_fill_regs to vm_event_fill_regs and move it 
to vm_event.h

2) For Tamas: would it be ok if I put p2m_get_vcpu_altp2m_idx in 
asm/altp2m.h and rename it to altp2m_vcpu_idx(v) (to obey 
function-naming pattern in that file)?

3) I've noticed that on X86 on a guest-request monitor vm-event the RIP 
is automatically incremented. On ARM, with the current changeset, that 
doesn't seem to happen.
     X86 code path: vmx_vmexit_handler(EXIT_REASON_VMCALL) -> 
hvm_do_hypercall -> do_hvm_op(HVMOP_guest_request_vm_event) -> 
hvm_do_hypercall returns HVM_HCALL_completed -> vmx_vmexit_handler calls 
update_guest_eip(), which increments the RIP.
     ARM code path, e.g. AArch64: do_trap_hypervisor(HSR_EC_HVC64) -> 
do_trap_hypercall -> do_hvm_op(HVMOP_guest_request_vm_event). Notice 
that do_trap_hypercall checks if the PC has changed after doing the 
hypercall. If it didn't, it poisons the hypercall argument registers, as 
on X86 (DEADBEEF), which happens on a debug build, which means that the 
PC isn't updated anywhere. I've also noticed that this seems to hold for 
other hvm ops such as HVMOP_set_param/HVMOP_get_param. I'm wondering if 
the logic behind this is ok as it is or if I should call advance_pc 
after handling HVMOP_guest_request_vm_event in do_hvm_op (?)

Thank you,
Corneliu.