Re: [PATCH 7/7] vm-event/arm: implement support for control-register write vm-events

[Julien Grall <julien.grall@arm.com>]

Hello Corneliu,

On 16/06/16 15:13, Corneliu ZUZU wrote:
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 8c50685..af61ac3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -43,6 +43,7 @@
>   #include <asm/mmio.h>
>   #include <asm/cpufeature.h>
>   #include <asm/flushtlb.h>
> +#include <asm/traps.h>
>
>   #include "decode.h"
>   #include "vtimer.h"
> @@ -124,7 +125,12 @@ void init_traps(void)
>       WRITE_SYSREG((HCPTR_CP_MASK & ~(HCPTR_CP(10) | HCPTR_CP(11))) | HCPTR_TTA,
>                    CPTR_EL2);
>
> -    /* Setup hypervisor traps */
> +    /* Setup hypervisor traps
> +     *
> +     * Note: HCR_TVM bit is also set for system-register write monitoring
> +     * purposes (see vm_event_monitor_cr), but (for performance reasons) that's
> +     * done selectively (see vcpu_enter_adjust_traps).
> +     */
>       WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
>                    HCR_TWE|HCR_TWI|HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,
>                    HCR_EL2);
> @@ -1720,6 +1726,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>       const struct hsr_cp32 cp32 = hsr.cp32;
>       int regidx = cp32.reg;
>       struct vcpu *v = current;
> +    register_t r = get_user_reg(regs, regidx);
>
>       if ( !check_conditional_instr(regs, hsr) )
>       {
> @@ -1730,6 +1737,61 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>       switch ( hsr.bits & HSR_CP32_REGS_MASK )
>       {
>       /*
> +     * HCR_EL2.TVM / HCR.TVM
> +     *
> +     * ARMv7 (DDI 0406C.b): B1.14.13

Please try to mention the most recent spec. This was published in 2012, 
the latest release (C.c) was published in 2014.

> +     * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34

Ditto. This was published in 2014, whilst the most recent was published 
in 2016.

> +     */
> +    case HSR_CPREG32(SCTLR):
> +        TVM_EMUL_VMEVT(v, regs, hsr, r, SCTLR);
> +        break;
> +    case HSR_CPREG32(TTBR0_32):
> +        TVM_EMUL_VMEVT(v, regs, hsr, r, TTBR0_32);
> +        break;
> +    case HSR_CPREG32(TTBR1_32):
> +        TVM_EMUL_VMEVT(v, regs, hsr, r, TTBR1_32);
> +        break;
> +    case HSR_CPREG32(TTBCR):
> +        TVM_EMUL_VMEVT(v, regs, hsr, r, TTBCR);
> +        break;
> +    case HSR_CPREG32(DACR):
> +        TVM_EMUL(regs, hsr, r, DACR);
> +        break;
> +    case HSR_CPREG32(DFSR):
> +        TVM_EMUL(regs, hsr, r, DFSR);
> +        break;
> +    case HSR_CPREG32(IFSR):
> +        TVM_EMUL(regs, hsr, r, IFSR);
> +        break;
> +    case HSR_CPREG32(DFAR):
> +        TVM_EMUL(regs, hsr, r, DFAR);
> +        break;
> +    case HSR_CPREG32(IFAR):
> +        TVM_EMUL(regs, hsr, r, IFAR);
> +        break;
[...]
> +    case HSR_CPREG32(ADFSR):
> +        TVM_EMUL(regs, hsr, r, ADFSR);
> +        break;
> +    case HSR_CPREG32(AIFSR):
> +        TVM_EMUL(regs, hsr, r, AIFSR);
> +        break;
> +    case HSR_CPREG32(MAIR0):
> +        TVM_EMUL(regs, hsr, r, MAIR0);
> +        break;
> +    case HSR_CPREG32(MAIR1):
> +        TVM_EMUL(regs, hsr, r, MAIR1);
> +        break;

Please mention that PRRR and NMRR are aliased to respectively MAIR0 and 
MAIR1. This will avoid to spend time trying to understanding why the 
spec says they are trapped but you don't "handle" them.

> +    case HSR_CPREG32(AMAIR0):
> +        TVM_EMUL(regs, hsr, r, AMAIR0);
> +        break;
> +    case HSR_CPREG32(AMAIR1):
> +        TVM_EMUL(regs, hsr, r, AMAIR1);
> +        break;
> +    case HSR_CPREG32(CONTEXTIDR):
> +        TVM_EMUL(regs, hsr, r, CONTEXTIDR);
> +        break;
> +
> +    /*
>        * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
>        *
>        * ARMv7 (DDI 0406C.b): B4.1.22
> @@ -1853,6 +1915,13 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>   static void do_cp15_64(struct cpu_user_regs *regs,
>                          const union hsr hsr)
>   {
> +    struct vcpu *v = current;
> +    const struct hsr_cp64 cp64 = hsr.cp64;
> +    sysreg64_t r = {
> +        .low32 = (uint32_t) get_user_reg(regs, cp64.reg1),

The cast is not necessary.

> +        .high32 = (uint32_t) get_user_reg(regs, cp64.reg2)

Ditto.

> +    };
> +
>       if ( !check_conditional_instr(regs, hsr) )
>       {
>           advance_pc(regs, hsr);
> @@ -1862,6 +1931,19 @@ static void do_cp15_64(struct cpu_user_regs *regs,
>       switch ( hsr.bits & HSR_CP64_REGS_MASK )
>       {
>       /*
> +     * HCR_EL2.TVM / HCR.TVM
> +     *
> +     * ARMv7 (DDI 0406C.b): B1.14.13
> +     * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34

Same remarks as above for the spec version.

> +     */
> +    case HSR_CPREG64(TTBR0):
> +        TVM_EMUL_VMEVT(v, regs, hsr, r.val64, TTBR0_64);
> +        break;
> +    case HSR_CPREG64(TTBR1):
> +        TVM_EMUL_VMEVT(v, regs, hsr, r.val64, TTBR1_64);
> +        break;
> +
> +    /*
>        * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
>        *
>        * ARMv7 (DDI 0406C.b): B4.1.22
> @@ -1891,8 +1973,6 @@ static void do_cp15_64(struct cpu_user_regs *regs,
>        */
>       default:
>           {
> -            const struct hsr_cp64 cp64 = hsr.cp64;
> -
>               gdprintk(XENLOG_ERR,
>                        "%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
>                        cp64.read ? "mrrc" : "mcrr",
> @@ -2128,10 +2208,50 @@ static void do_sysreg(struct cpu_user_regs *regs,
>   {
>       int regidx = hsr.sysreg.reg;
>       struct vcpu *v = current;
> +    register_t r = get_user_reg(regs, regidx);
>
>       switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>       {
>       /*
> +     * HCR_EL2.TVM
> +     *
> +     * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34

Ditto for the spec version.

[...]

> diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
> new file mode 100644
> index 0000000..3f23fec
> --- /dev/null
> +++ b/xen/arch/arm/vm_event.c
> @@ -0,0 +1,112 @@

[...]

> +#include <xen/vm_event.h>
> +#include <asm/traps.h>
> +
> +#if CONFIG_ARM_64
> +
> +#define MWSINF_SCTLR    32,SCTLR_EL1
> +#define MWSINF_TTBR0    64,TTBR0_EL1
> +#define MWSINF_TTBR1    64,TTBR1_EL1
> +#define MWSINF_TTBCR    64,TCR_EL1
> +
> +#elif CONFIG_ARM_32
> +
> +#define MWSINF_SCTLR    32,SCTLR
> +#define MWSINF_TTBR0    64,TTBR0
> +#define MWSINF_TTBR1    64,TTBR1

The values are the same as for arm64 (*_EL1 is aliased to * for arm32). 
Please avoid duplication.

> +#define MWSINF_TTBR0_32 32,TTBR0_32
> +#define MWSINF_TTBR1_32 32,TTBR1_32
> +#define MWSINF_TTBCR    32,TTBCR
> +
> +#endif
> +
> +#define MWS_EMUL_(val, sz, r...)    WRITE_SYSREG##sz((uint##sz##_t) (val), r)

The cast is not necessary.

> +#define MWS_EMUL(r)                 CALL_MACRO(MWS_EMUL_, w->value, MWSINF_##r)
> +
> +static inline void vcpu_enter_write_data(struct vcpu *v)
> +{
> +    struct monitor_write_data *w = &v->arch.vm_event.write_data;
> +
> +    if ( likely(MWS_NOWRITE == w->status) )
> +        return;
> +
> +    switch ( w->status )
> +    {
> +    case MWS_SCTLR:
> +        MWS_EMUL(SCTLR);
> +        break;
> +    case MWS_TTBR0:
> +        MWS_EMUL(TTBR0);
> +        break;
> +    case MWS_TTBR1:
> +        MWS_EMUL(TTBR1);
> +        break;
> +#if CONFIG_ARM_32
> +    case MWS_TTBR0_32:
> +        MWS_EMUL(TTBR0_32);
> +        break;
> +    case MWS_TTBR1_32:
> +        MWS_EMUL(TTBR1_32);
> +        break;
> +#endif

Aarch32 kernel can return on an AArch64 Xen. This means that 
TTBR{0,1}_32 could be trapped and the write therefore be properly emulated.

> +    case MWS_TTBCR:
> +        MWS_EMUL(TTBCR);
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    w->status = MWS_NOWRITE;
> +}
> +
> +static inline void vcpu_enter_adjust_traps(struct vcpu *v)
> +{
> +    register_t old_hcr, hcr;
> +
> +    hcr = (old_hcr = READ_SYSREG(HCR_EL2));
> +
> +    if ( unlikely(0 != v->domain->arch.monitor.write_ctrlreg_enabled) )
> +        hcr |= HCR_TVM;
> +    else
> +        hcr &= ~HCR_TVM;
> +
> +    if ( unlikely(hcr != old_hcr) )
> +    {
> +        WRITE_SYSREG(hcr, HCR_EL2);
> +        isb();
> +    }

The HCR register is also updated in p2m_restore_state to set HCR_EL2.RW. 
I would prefer to have a single place where HCR is updated.

Maybe we should store the HCR value per-domain?

> +}
> +
> +void arch_vm_event_vcpu_enter(struct vcpu *v)
> +{
> +    vcpu_enter_write_data(v);
> +    vcpu_enter_adjust_traps(v);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> index 2366bae..c35a717 100644
> --- a/xen/common/monitor.c
> +++ b/xen/common/monitor.c
> @@ -62,7 +62,6 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>
>       switch ( mop->event )
>       {
> -#if CONFIG_X86
>       case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
>       {
>           struct arch_domain *ad = &d->arch;
> @@ -100,7 +99,6 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>
>           break;
>       }
> -#endif
>
>       case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
>       {
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 53dc048..e0f999e 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -824,7 +824,6 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync,
>       return 1;
>   }
>
> -#if CONFIG_X86
>   bool_t vm_event_monitor_cr(unsigned int index, unsigned long value,
>                              unsigned long old)
>   {
> @@ -852,7 +851,6 @@ bool_t vm_event_monitor_cr(unsigned int index, unsigned long value,
>
>       return 0;
>   }
> -#endif
>
>   void vm_event_monitor_guest_request(void)
>   {

[...]

> diff --git a/xen/include/asm-arm/traps.h b/xen/include/asm-arm/traps.h
> new file mode 100644
> index 0000000..9e246a7
> --- /dev/null
> +++ b/xen/include/asm-arm/traps.h

This file is very difficult to review. Can you explain why you need to 
duplicate most of the things between the AArch64 and AArch32 port?

Whilst I understand that some registers are accessible in a different 
way between AArch64 and Aarch32, we were able to get rid of most of them 
in the Xen port by aliasing registers (see asm-arm/cpregs.h).

 From a quick look, I believe this is possible to do and would reduce a 
lot the size/complexity of this file.

> @@ -0,0 +1,253 @@

[...]

> +/*
> + * Emulation of system-register trapped writes that do not cause
> + * VM_EVENT_REASON_WRITE_CTRLREG monitor vm-events.
> + * Such writes are collaterally trapped due to setting the HCR_EL2.TVM bit.
> + *
> + * Regarding aarch32 domains, note that from Xen's perspective system-registers
> + * of such domains are architecturally-mapped to aarch64 registers in one of
> + * three ways:
> + *  - low 32-bits mapping   (e.g. aarch32 DFAR -> aarch64 FAR_EL1[31:0])
> + *  - high 32-bits mapping  (e.g. aarch32 IFAR -> aarch64 FAR_EL1[63:32])
> + *  - full mapping          (e.g. aarch32 SCTLR -> aarch64 SCTLR_EL1)
> + *
> + * Hence we define 2 macro variants:
> + *  - TVM_EMUL_SZ variant, for full mappings
> + *  - TVM_EMUL_LH variant, for low/high 32-bits mappings
> + */
> +#define TVM_EMUL_SZ(regs, hsr, val, sz, r...)                           \
> +{                                                                       \
> +    if ( psr_mode_is_user(regs) )                                       \

Those registers are not accessible at EL0.

> +        return inject_undef_exception(regs, hsr);                       \
> +    WRITE_SYSREG##sz((uint##sz##_t) (val), r);

[...]

> diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
> index 4e5a272..edf9654 100644
> --- a/xen/include/asm-arm/vm_event.h
> +++ b/xen/include/asm-arm/vm_event.h
> @@ -30,6 +30,12 @@ static inline int vm_event_init_domain(struct domain *d)
>
>   static inline void vm_event_cleanup_domain(struct domain *d)
>   {
> +    struct vcpu *v;
> +
> +    for_each_vcpu ( d, v )
> +        memset(&v->arch.vm_event, 0, sizeof(v->arch.vm_event));
> +
> +    memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
>       memset(&d->monitor, 0, sizeof(d->monitor));
>   }
>
> @@ -41,7 +47,13 @@ static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
>   static inline
>   void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
>   {
> -    /* Not supported on ARM. */
> +    /* X86 VM_EVENT_REASON_MOV_TO_MSR could (but shouldn't) end-up here too. */

This should be an ASSERT/BUG_ON then.

> +    if ( unlikely(VM_EVENT_REASON_WRITE_CTRLREG != rsp->reason) )
> +        return;
> +
> +    if ( (rsp->flags & VM_EVENT_FLAG_DENY) &&
> +         (rsp->flags & VM_EVENT_FLAG_VCPU_PAUSED) )
> +        v->arch.vm_event.write_data.status = MWS_NOWRITE;
>   }
>
>   static inline
> @@ -55,10 +67,7 @@ static inline void vm_event_fill_regs(vm_event_request_t *req)
>       /* Not supported on ARM. */
>   }
>
> -static inline void arch_vm_event_vcpu_enter(struct vcpu *v)
> -{
> -    /* Nothing to do. */
> -}
> +void arch_vm_event_vcpu_enter(struct vcpu *v);
>
>   /*
>    * Monitor vm-events.
> @@ -67,7 +76,8 @@ static inline uint32_t vm_event_monitor_get_capabilities(struct domain *d)
>   {
>       uint32_t capabilities = 0;
>
> -    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
> +    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
> +                   (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
>
>       return capabilities;
>   }
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index 8f94e20..ec3eaca 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h

[...]

-- 
Julien Grall

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