From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corneliu ZUZU Subject: Re: [PATCH] arm/monitor vm-events: Implement guest-request support Date: Tue, 23 Feb 2016 11:09:05 +0200 Message-ID: <56CC21B1.70202@bitdefender.com> References: <1455824116-13783-1-git-send-email-czuzu@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1455824116-13783-1-git-send-email-czuzu@bitdefender.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xen.org Cc: Tamas K Lengyel , Keir Fraser , Ian Campbell , Razvan Cojocaru , Andrew Cooper , Stefano Stabellini , Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 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.