xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Julien Grall <julien.grall@arm.com>
To: Corneliu ZUZU <czuzu@bitdefender.com>, xen-devel@lists.xen.org
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>
Subject: Re: [PATCH 7/7] vm-event/arm: implement support for control-register write vm-events
Date: Thu, 16 Jun 2016 17:49:49 +0100	[thread overview]
Message-ID: <5762D8AD.7070208@arm.com> (raw)
In-Reply-To: <1466086403-7749-1-git-send-email-czuzu@bitdefender.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

  parent reply	other threads:[~2016-06-16 16:49 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-16 14:04 [PATCH 0/7] vm-event: Implement ARM support for control-register writes Corneliu ZUZU
2016-06-16 14:06 ` [PATCH 1/7] minor (formatting) fixes Corneliu ZUZU
2016-06-16 14:24   ` Jan Beulich
2016-06-16 19:19     ` Corneliu ZUZU
2016-06-17  7:06       ` Jan Beulich
2016-06-17 10:46         ` Corneliu ZUZU
2016-06-16 16:02   ` Tamas K Lengyel
2016-06-17  8:33     ` Corneliu ZUZU
2016-06-17  8:36       ` Razvan Cojocaru
2016-06-17  9:29         ` Andrew Cooper
2016-06-17  9:35           ` Jan Beulich
2016-06-17  9:33         ` Jan Beulich
2016-06-17  9:36           ` Razvan Cojocaru
2016-06-17  9:40             ` Jan Beulich
2016-06-17  9:42               ` Razvan Cojocaru
2016-06-17 19:05           ` Tamas K Lengyel
2016-06-16 14:07 ` [PATCH 2/7] vm-event: VM_EVENT_FLAG_DENY requires VM_EVENT_FLAG_VCPU_PAUSED Corneliu ZUZU
2016-06-16 16:11   ` Tamas K Lengyel
2016-06-17  8:43     ` Corneliu ZUZU
2016-06-21 11:26     ` Corneliu ZUZU
2016-06-21 15:09       ` Tamas K Lengyel
2016-06-22  8:34         ` Corneliu ZUZU
2016-06-16 14:08 ` [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter Corneliu ZUZU
2016-06-16 14:51   ` Jan Beulich
2016-06-16 20:10     ` Corneliu ZUZU
2016-06-16 20:33       ` Razvan Cojocaru
2016-06-17 10:41         ` Corneliu ZUZU
2016-06-17  7:17       ` Jan Beulich
2016-06-17 11:13         ` Corneliu ZUZU
2016-06-17 11:27           ` Jan Beulich
2016-06-17 12:13             ` Corneliu ZUZU
2016-06-16 16:17   ` Tamas K Lengyel
2016-06-17  9:19     ` Corneliu ZUZU
2016-06-17  8:55   ` Julien Grall
2016-06-17 11:40     ` Corneliu ZUZU
2016-06-17 13:22       ` Julien Grall
2016-06-16 14:09 ` [PATCH 4/7] vm-event/x86: use vm_event_vcpu_enter properly Corneliu ZUZU
2016-06-16 15:00   ` Jan Beulich
2016-06-16 20:20     ` Corneliu ZUZU
2016-06-17  7:20       ` Jan Beulich
2016-06-17 11:23         ` Corneliu ZUZU
2016-06-16 16:27   ` Tamas K Lengyel
2016-06-17  9:24     ` Corneliu ZUZU
2016-06-16 14:10 ` [PATCH 5/7] x86: replace monitor_write_data.do_write with enum Corneliu ZUZU
2016-06-16 14:12 ` [PATCH 6/7] vm-event/arm: move hvm_event_cr->common vm_event_monitor_cr Corneliu ZUZU
2016-06-16 15:16   ` Jan Beulich
2016-06-17  8:25     ` Corneliu ZUZU
2016-06-17  8:38       ` Jan Beulich
2016-06-17 11:31         ` Corneliu ZUZU
2016-06-21  7:08       ` Corneliu ZUZU
2016-06-21  7:20         ` Jan Beulich
2016-06-21 15:22           ` Tamas K Lengyel
2016-06-22  6:33             ` Jan Beulich
2016-06-16 16:55   ` Tamas K Lengyel
2016-06-17 10:37     ` Corneliu ZUZU
2016-06-16 14:13 ` [PATCH 7/7] vm-event/arm: implement support for control-register write vm-events Corneliu ZUZU
2016-06-16 14:26   ` Julien Grall
2016-06-16 19:24     ` Corneliu ZUZU
2016-06-16 21:28       ` Julien Grall
2016-06-17 11:46         ` Corneliu ZUZU
2016-06-16 16:49   ` Julien Grall [this message]
2016-06-17 10:36     ` Corneliu ZUZU
2016-06-17 13:18       ` Julien Grall
2016-06-22 16:35       ` Corneliu ZUZU
2016-06-22 17:17         ` Julien Grall
2016-06-22 18:39           ` Corneliu ZUZU
2016-06-22 19:37             ` Corneliu ZUZU
2016-06-22 19:41               ` Julien Grall
2016-06-23  5:31                 ` Corneliu ZUZU
2016-06-23  5:49                   ` Corneliu ZUZU
2016-06-23 11:11                     ` Julien Grall
2016-06-24  9:32                       ` Corneliu ZUZU
2016-06-23 11:00           ` Julien Grall

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5762D8AD.7070208@arm.com \
    --to=julien.grall@arm.com \
    --cc=czuzu@bitdefender.com \
    --cc=rcojocaru@bitdefender.com \
    --cc=sstabellini@kernel.org \
    --cc=tamas@tklengyel.com \
    --cc=xen-devel@lists.xen.org \
    --subject='Re: [PATCH 7/7] vm-event/arm: implement support for control-register write vm-events' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).