[PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

* [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events
@ 2016-03-03 14:10 Corneliu ZUZU
  2016-03-03 14:11 ` [PATCH 1/1] arm/monitor vm-events: implement write-ctrlreg support Corneliu ZUZU
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Corneliu ZUZU @ 2016-03-03 14:10 UTC (permalink / raw)
  To: xen-devel
  Cc: Kevin Tian, Tamas K Lengyel, Keir Fraser, Jun Nakajima,
	Razvan Cojocaru, Andrew Cooper, Stefano Stabellini, Jan Beulich

I've decided to include a cover-letter for this patch to ask for some feedback
on a few matters (and also to detail the changes that were done here instead
of writing that in the commit message).

First,
DETAILS OF CHANGES:

== Moved to common-side:
  1) monitor_ctrlreg_bitmask macro

  2) hvm_event_cr->vm_event_monitor_cr

  3) XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG handling (moved from X86
      arch_monitor_domctl_event to common monitor_domctl)

== ARM implementations:

  1) add VM_EVENT_ARM_{SCTLR,TTBR{0,1},TTBCR} as possible
    vm_event_write_ctrlreg indexes

  2) add write_ctrlreg_* bits in arch_domain

  3) vm_event_monitor_get_capabilities updated to reflect support
    for XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG

  4) on the scheduling tail, p2m_restore_state sets the HCR_TVM bit if
    write_ctrlreg_enabled <> 0 for the domain

  5) traps.c, traps.h:
        do_cp15_32, do_cp15_64 and do_sysreg now handle HCR_TVM traps,
        emulating writes as needed and also calling vm_event_monitor_cr in
        the process for the targeted monitored registers

The patch was currently tested on an ARMv8 (CONFIG_ARM_64) machine (after
applying a fix I'll send soon). With CONFIG_ARM_32 I only checked that Xen
clean-builds ok.

Then,
QUESTIONS (FOR VM-EVENTS & ARM MAINTAINERS ESPECIALLY):

Q1) Getting rid of the "#ifdef CONFIG_X86" @
    monitor_domctl->XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case.
    To trap targeted control-registers writes, on ARM we check if
    write_ctrlreg_enabled is non-zero on the scheduling tail and set/unset the
    HCR_TVM bit accordingly (see the p2m_restore_state function).

    I see that on X86 for CR3 that's done @ vmx_update_guest_cr by this code:

        /* Trap CR3 updates if CR3 memory events are enabled. */
        if ( v->domain->arch.monitor.write_ctrlreg_enabled &
             monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
            v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;

    Couldn't we move this part on the X86 scheduling tail as it is done for ARM
    in this changeset?

Q2) About VM_EVENT_FLAG_DENY

  Q2.1)
    Doesn't it require sync = 1 (i.e. the vcpu to be paused) to work?
    If so, shouldn't we call vm_event_register_write_resume only after checking
    that VM_EVENT_FLAG_VCPU_PAUSED flag is set (vm_event_resume). Moreover, if
    we do that, wouldn't it be 'cleaner' to rename
    vm_event_register_write_resume->vm_event_deny, check for the
    VM_EVENT_FLAG_DENY flag in vm_event_resume instead and call vm_event_deny
    from there after this check?

  Q2.2)
    VM_EVENT_FLAG_DENY functionality is not implemented w/ this change-set.
    What is done instead is that the control-register write is done *before*
    sending the vm-event (vm_event_put_request). This way, the user can
    override the written register value after receiving the vm-event, which
    in effect provides the same flexibility as VM_EVENT_FLAG_DENY does.
    Using this strategy instead would simplify both Xen's code and the libxc
    user's code.
    Thoughts?

Q3) About security: i.e. making sure I don't crash Xen :)

    When a system-register write is trapped in Xen due to the HCR_TVM bit,
    besides triggering the vm-event where it's the case, we also need to emulate
    this write operation. This is done with the WRITE_SYSREG* macros (see
    TVM_EMUL/TVM_EMUL_VMEVT macros and their usage).

    A potential security issue I see here is a case where WRITE_SYSREG* would
    cause some kind of hardware fault/exception from EL2 that I did not take
    into account. Since the value that must be written is determined by the
    domain, that would mean that a 'bad' domain could willingly cause Xen to
    crash.

    I've read the ARM manuals and of course I've kept this in mind while doing
    the emulation, but I still need some clarifications.
    Concretely, this is the information I haven't found while reading the docs:
        Q3.1)
            If an AArch64 domain does:

                MSR SCTLR_EL1, <Xt>

            and given that SCTLR_EL1 is a 32-bit system register and that Xt is
            a 64-bit register, what happens if the *high* 32-bits of Xt are not
            set to zero? Are they simply ignored or would that cause a fault? If
            so, would that fault happen before trapping to EL2?

        Q3.2)
            What would happen if a domain writes an invalid value to these regs.
            By invalid I mean writing 1 to RES0/UNK/SBZP/RAZ/SBZP bits or
            vice-versa (0 to RES1,etc). Again, could such behavior cause faults
            when doing WRITE_SYSREG*? Note that this also concerns functions
            like xc_vcpu_setcontext.

Q4) The ARMv7 manual doesn't include AMAIR0/AMAIR1 in the list of registers that
    cause HYP traps on write when the HCR.TVM bit is set. Is that correct?
    If so, should I surround parts relevant to them in this changeset w/ CONFIG_ARM_64?

Corneliu ZUZU (1):
  arm/monitor vm-events: implement write-ctrlreg support

 xen/arch/arm/p2m.c              |   5 +
 xen/arch/arm/traps.c            | 128 +++++++++++++++++++++-
 xen/arch/x86/hvm/event.c        |  27 -----
 xen/arch/x86/hvm/hvm.c          |   2 +-
 xen/arch/x86/hvm/vmx/vmx.c      |   2 +-
 xen/arch/x86/monitor.c          |  45 --------
 xen/common/monitor.c            |  48 +++++++++
 xen/common/vm_event.c           |  29 +++++
 xen/include/asm-arm/domain.h    |   8 ++
 xen/include/asm-arm/traps.h     | 227 ++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vm_event.h  |   4 +-
 xen/include/asm-x86/hvm/event.h |  13 +--
 xen/include/asm-x86/monitor.h   |   2 -
 xen/include/public/vm_event.h   |   8 +-
 xen/include/xen/monitor.h       |   2 +
 xen/include/xen/vm_event.h      |   8 ++
 16 files changed, 467 insertions(+), 91 deletions(-)
 create mode 100644 xen/include/asm-arm/traps.h

-- 
2.5.0

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

^ permalink raw reply	[flat|nested] 17+ messages in thread