Re: [Qemu-devel] [PATCH 03/15] hw/intc/arm_gic: Add Security Extensions property
On 25 Aug 2014, at 11:20, Sergey Fedorov wrote:
> On 22.08.2014 14:29, Fabian Aggeler wrote:
>> The existing implementation does not support Security Extensions mentioned
>> in the GICv1 and GICv2 architecture specification. Security Extensions are
>> not available on all GICs. This property makes it possible to enable
>> Security Extensions.
>>
>> It also makes GICD_TYPER/ICDICTR.SecurityExtn RAO for GICs which implement
>> Security Extensions.
>>
>> Signed-off-by: Fabian Aggeler
>> ---
>> hw/intc/arm_gic.c| 5 -
>> hw/intc/arm_gic_common.c | 1 +
>> include/hw/intc/arm_gic_common.h | 1 +
>> 3 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index b27bd0e..75b5121 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -297,7 +297,10 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr
>> offset)
>> if (offset == 0)
>> return s->enabled;
>> if (offset == 4)
>> -return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5);
>> +/* Interrupt Controller Type Register */
>> +return ((s->num_irq / 32) - 1)
>> +| ((NUM_CPU(s) - 1) << 5)
>> +| (s->security_extn << 10);
>> if (offset < 0x08)
>> return 0;
>> if (offset >= 0x80) {
>> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
>> index 6d884ec..302a056 100644
>> --- a/hw/intc/arm_gic_common.c
>> +++ b/hw/intc/arm_gic_common.c
>> @@ -149,6 +149,7 @@ static Property arm_gic_common_properties[] = {
>> * (Internally, 0x also indicates "not a GIC but an NVIC".)
>> */
>> DEFINE_PROP_UINT32("revision", GICState, revision, 1),
>> +DEFINE_PROP_UINT8("security-extn", GICState, security_extn, 0),
>
> Why don't use bool type and DEFINE_PROP_BOOL for this field?
>
> Regards,
> Sergey.
I used an uint8 to be able to shift the security_extn field in the return value
of the
Interrupt Controller Type Register (see above).
@Edgar: how did you add Virtualization Extensions to the GIC implementation?
Does it make sense to combine the extensions into one QOM property?
Best,
Fabian
>
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> diff --git a/include/hw/intc/arm_gic_common.h
>> b/include/hw/intc/arm_gic_common.h
>> index 01c6f24..4e25017 100644
>> --- a/include/hw/intc/arm_gic_common.h
>> +++ b/include/hw/intc/arm_gic_common.h
>> @@ -105,6 +105,7 @@ typedef struct GICState {
>> MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
>> uint32_t num_irq;
>> uint32_t revision;
>> +uint8_t security_extn;
>> int dev_fd; /* kvm device fd if backed by kvm vgic support */
>> } GICState;
Re: [Qemu-devel] [PATCH v3 1/2] hw/misc/arm_sp810: Create SP810 device
On 19 Aug 2014, at 16:03, Peter Maydell wrote:
> On 17 August 2014 15:24, Fabian Aggeler wrote:
>> This adds a device model for the PrimeXsys System Controller (SP810)
>> which is present in the Versatile Express motherboards. It is
>> so far read-only but allows to read the SCCTRL register.
>>
>> Signed-off-by: Fabian Aggeler
>> ---
>> default-configs/arm-softmmu.mak | 1 +
>> hw/misc/Makefile.objs | 1 +
>> hw/misc/arm_sp810.c | 109
>>
>> include/hw/misc/arm_sp810.h | 80 +
>> 4 files changed, 191 insertions(+)
>> create mode 100644 hw/misc/arm_sp810.c
>> create mode 100644 include/hw/misc/arm_sp810.h
>>
>> diff --git a/default-configs/arm-softmmu.mak
>> b/default-configs/arm-softmmu.mak
>> index f3513fa..67ba99a 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -55,6 +55,7 @@ CONFIG_PL181=y
>> CONFIG_PL190=y
>> CONFIG_PL310=y
>> CONFIG_PL330=y
>> +CONFIG_SP810=y
>> CONFIG_CADENCE=y
>> CONFIG_XGMAC=y
>> CONFIG_EXYNOS4=y
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 979e532..49d023b 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -13,6 +13,7 @@ common-obj-$(CONFIG_PL310) += arm_l2x0.o
>> common-obj-$(CONFIG_INTEGRATOR_DEBUG) += arm_integrator_debug.o
>> common-obj-$(CONFIG_A9SCU) += a9scu.o
>> common-obj-$(CONFIG_ARM11SCU) += arm11scu.o
>> +common-obj-$(CONFIG_SP810) += arm_sp810.o
>
> We mostly don't use the 'arm' prefix (compare pl330, pl011,
> and other examples for other architectures), so you can
> remove it from filenames and variable names here.
Ok, will remove it then.
>
>>
>> # PKUnity SoC devices
>> common-obj-$(CONFIG_PUV3) += puv3_pm.o
>> diff --git a/hw/misc/arm_sp810.c b/hw/misc/arm_sp810.c
>> new file mode 100644
>> index 000..7aff9bd
>> --- /dev/null
>> +++ b/hw/misc/arm_sp810.c
>> @@ -0,0 +1,109 @@
>> +/*
>> + * ARM PrimeXsys System Controller (SP810)
>> + *
>> + * Copyright (C) 2014 Fabian Aggeler
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include "hw/misc/arm_sp810.h"
>> +
>> +static const VMStateDescription vmstate_arm_sysctl = {
>
> ^^^ should be vmstate_sp810.
Thanks for catching this. I will change it.
>
>> +.name = "arm_sp810",
>> +.version_id = 1,
>> +.minimum_version_id = 1,
>> +.fields = (VMStateField[]) {
>> +VMSTATE_UINT8(timeren0_sel, arm_sp810_state),
>> +VMSTATE_UINT8(timeren1_sel, arm_sp810_state),
>> +VMSTATE_UINT8(timeren2_sel, arm_sp810_state),
>> +VMSTATE_UINT8(timeren3_sel, arm_sp810_state),
>
> These are read only properties, so you don't need to save them.
Okay.
>
>> +VMSTATE_END_OF_LIST()
>> +}
>> +};
>> +
>> +static uint64_t arm_sp810_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> +arm_sp810_state *s = opaque;
>> +
>> +switch (offset) {
>> +case SCCTRL:
>> +return (s->timeren3_sel << 21) | (s->timeren2_sel << 19)
>> +| (s->timeren1_sel << 17) | (s->timeren0_sel << 15);
>> +default:
>> +qemu_log_mask(LOG_UNIMP,
>> +"arm_sp810_read: Register not yet implemented: %s\n",
>> +HWADDR_PRIx);
>> +return 0;
>> +}
>> +}
>> +
>> +static void arm_sp810_write(void *opaque, hwaddr offset,
>> +uint64_t val, unsigned size)
>> +{
>> +switch (offset) {
>> +default:
>> +qemu_log_mask(LOG_UNIMP,
>> +"arm_sp810_write: Register not yet implemented: %s\n",
>> +HWADDR_PRIx);
>> +return;
>> +}
>> +}
>> +
>> +static const MemoryRegionOps arm_sp810_ops = {
>> +.read = arm_sp810_read,
>> +.write = arm_sp810_write,
>> +.endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static void arm_sp810_init(Object *obj)
>> +{
>> +arm_sp810_state *s = ARM_SP810(obj);
>> +
>> +memory_region_init_io(&s->iomem, obj, &arm_sp810_ops, s, "arm-sp810",
>> + 0x1000);
>> +sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>> +}
>> +
>> +static Property arm_sp810_properties[] = {
>> +DEFINE_PROP_UINT8("timeren0-sel", arm_sp810_state, timeren0_sel,
>> + SCCTRL_TIMER_REFCLK),
>> +DEFINE_PROP_UINT8("timeren1-sel", arm_sp810_state, timeren1_sel,
>> + SCCTRL_TIMER_REFCLK),
>> +DE
Re: [Qemu-devel] [PATCH v2 1/2] hw/misc/arm_sp810: Create SP810 device
On 06 Aug 2014, at 01:03, Peter Crosthwaite
wrote:
> On Tue, Aug 5, 2014 at 7:32 PM, Fabian Aggeler wrote:
>> This adds a device model for the PrimeXsys System Controller (SP810)
>> which is present in the Versatile Express motherboards. It is
>> so far read-only but allows to set the SCCTRL register.
>>
>> Signed-off-by: Fabian Aggeler
>> ---
>> default-configs/arm-softmmu.mak | 1 +
>> hw/misc/Makefile.objs | 1 +
>> hw/misc/arm_sp810.c | 98
>> +
>> include/hw/misc/arm_sp810.h | 85 +++
>> 4 files changed, 185 insertions(+)
>> create mode 100644 hw/misc/arm_sp810.c
>> create mode 100644 include/hw/misc/arm_sp810.h
>>
>> diff --git a/default-configs/arm-softmmu.mak
>> b/default-configs/arm-softmmu.mak
>> index f3513fa..67ba99a 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -55,6 +55,7 @@ CONFIG_PL181=y
>> CONFIG_PL190=y
>> CONFIG_PL310=y
>> CONFIG_PL330=y
>> +CONFIG_SP810=y
>> CONFIG_CADENCE=y
>> CONFIG_XGMAC=y
>> CONFIG_EXYNOS4=y
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 979e532..49d023b 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -13,6 +13,7 @@ common-obj-$(CONFIG_PL310) += arm_l2x0.o
>> common-obj-$(CONFIG_INTEGRATOR_DEBUG) += arm_integrator_debug.o
>> common-obj-$(CONFIG_A9SCU) += a9scu.o
>> common-obj-$(CONFIG_ARM11SCU) += arm11scu.o
>> +common-obj-$(CONFIG_SP810) += arm_sp810.o
>>
>> # PKUnity SoC devices
>> common-obj-$(CONFIG_PUV3) += puv3_pm.o
>> diff --git a/hw/misc/arm_sp810.c b/hw/misc/arm_sp810.c
>> new file mode 100644
>> index 000..21eb816
>> --- /dev/null
>> +++ b/hw/misc/arm_sp810.c
>> @@ -0,0 +1,98 @@
>> +/*
>> + * ARM PrimeXsys System Controller (SP810)
>> + *
>> + * Copyright (C) 2014 Fabian Aggeler
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include "hw/misc/arm_sp810.h"
>> +
>> +static const VMStateDescription vmstate_arm_sysctl = {
>> +.name = "arm_sp810",
>> +.version_id = 1,
>> +.minimum_version_id = 1,
>> +.fields = (VMStateField[]) {
>> +VMSTATE_UINT32(sc_ctrl, arm_sp810_state),
>> +VMSTATE_END_OF_LIST()
>> +}
>> +};
>> +
>> +static uint64_t arm_sp810_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> +arm_sp810_state *s = opaque;
>> +
>> +switch (offset) {
>> +case SCCTRL:
>> +return s->sc_ctrl;
>> +default:
>> +qemu_log_mask(LOG_UNIMP,
>> +"arm_sp810_read: Register not yet implemented: %s\n",
>> +HWADDR_PRIx);
>> +return 0;
>> +}
>> +}
>> +
>> +static void arm_sp810_write(void *opaque, hwaddr offset,
>> +uint64_t val, unsigned size)
>> +{
>> +switch (offset) {
>> +default:
>> +qemu_log_mask(LOG_UNIMP,
>> +"arm_sp810_write: Register not yet implemented: %s\n",
>> +HWADDR_PRIx);
>> +return;
>> +}
>> +}
>> +
>> +static const MemoryRegionOps arm_sp810_ops = {
>> +.read = arm_sp810_read,
>> +.write = arm_sp810_write,
>> +.endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static void arm_sp810_init(Object *obj)
>> +{
>> +arm_sp810_state *s = ARM_SP810(obj);
>> +
>> +memory_region_init_io(&s->iomem, obj, &arm_sp810_ops, s, "arm-sp810",
>> + 0x1000);
>> +sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>> +}
>> +
>> +static Property arm_sp810_properties[] = {
>> +DEFINE_PROP_UINT32("sc-ctrl", arm_sp810_state, sc_ctrl, 0x09),
>> +DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>
> So it looks to me like the SCCTRL contains multiple register fields
> for multiple configurable options. Although i'm having trouble getting
> my head around it as I could not easily find public docco for this
> core (have a link handy?).
Unfortunately not as it seems ARM marked it as obsolete. We found
the document offline. I could not find it on the web either.
See also
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/038722.html
Indeed SCCTRL has many configuration bits, not just these 4 configuration
options.
>
> Anyways, the thinking is you should define multiple configurable
> options as invididual QOM properties, rather than have board level
> code init registers. This would mean that you
>
> DEFINE_PROP_UINT8("timeren0-sel", ...)
> DEFINE_PROP_UINT8("timeren1-sel", ...)
>
> Have a look
Re: [Qemu-devel] [PATCH 2/2] hw/arm/vexpress: add SP810 to the vexpress
On 17 Jul 2014, at 01:29, Peter Crosthwaite
wrote:
> On Thu, Jul 17, 2014 at 1:05 AM, Alex Bennée wrote:
>>
>> Fabian Aggeler writes:
>>
>>> The SP810, which is present in the Versatile Express motherboards,
>>> allows to set the timing reference to either REFCLK or TIMCLK.
>>> QEMU currently sets the SP804 timer to 1MHz by default. To reflect
>>> this, we set the TimerEn0Sel, TimerEn1Sel, TimerEn2Sel, and
>>> TimerEn3Sel of the system control register (SCCTRL) to TIMCLK (1).
>>>
>>> Signed-off-by: Fabian Aggeler
>>> ---
>>> hw/arm/vexpress.c | 11 +--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
>>> index a88732c..b96c3fd 100644
>>> --- a/hw/arm/vexpress.c
>>> +++ b/hw/arm/vexpress.c
>>> @@ -512,7 +512,7 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base,
>>> const char *name,
>>> static void vexpress_common_init(VEDBoardInfo *daughterboard,
>>> MachineState *machine)
>>> {
>>> -DeviceState *dev, *sysctl, *pl041;
>>> +DeviceState *dev, *sysctl, *pl041, *sp810;
>>> qemu_irq pic[64];
>>> uint32_t sys_id;
>>> DriveInfo *dinfo;
>>> @@ -575,7 +575,14 @@ static void vexpress_common_init(VEDBoardInfo
>>> *daughterboard,
>>> qdev_init_nofail(sysctl);
>>> sysbus_mmio_map(SYS_BUS_DEVICE(sysctl), 0, map[VE_SYSREGS]);
>>>
>>> -/* VE_SP810: not modelled */
>>> +/* VE_SP810 */
>>> +sp810 = qdev_create(NULL, "arm_sp810");
>
> Move the the type definition macro to header as well.
>
> Regards,
> Peter
Thanks for your comments. I addressed them in v2 which I am going to send
shortly.
Best,
Fabian
>
>>> +/* SP804 is already running at 1MHz (TIMCLK) so SCCTRL TimerEnXSel=1 */
>>> +qdev_prop_set_uint32(sp810, "sc-ctrl", (1 << 15) | (1 << 17)
>>> + | (1 << 19) | (1 << 21));
>>
>>
>> Could the #defines in the first patch be moved into a header and used
>> here rather than manually setting these bits?
>>
>> --
>> Alex Bennée
Re: [Qemu-devel] [PATCH v4 15/33] target-arm: add NSACR register
On 01 Jul 2014, at 01:09, greg.bell...@linaro.org wrote:
> From: Fabian Aggeler
>
> Implements NSACR register with corresponding read/write functions
> for ARMv7 and ARMv8.
>
Actually, in this patch we could add a check in cpu_get_tb_cpu_state() (cpu.h)
to not set
the ARM_TBFLAG_CPACR_FPEN_MASK if NSACR disables it.
What do you think?
> Signed-off-by: Sergey Fedorov
> Signed-off-by: Fabian Aggeler
> Signed-off-by: Greg Bellows
> ---
> target-arm/cpu.h| 6 +
> target-arm/helper.c | 68 -
> 2 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1e8d5ee..4625088 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -182,6 +182,7 @@ typedef struct CPUARMState {
> uint64_t c1_coproc; /* Coprocessor access register. */
> uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */
> uint32_t c1_scr; /* secure config register. */
> +uint32_t c1_nsacr; /* Non-secure access control register. */
> uint64_t ttbr0_el1; /* MMU translation table base 0. */
> uint64_t ttbr1_el1; /* MMU translation table base 1. */
> uint64_t c2_control; /* MMU translation table base control. */
> @@ -609,6 +610,11 @@ static inline void xpsr_write(CPUARMState *env, uint32_t
> val, uint32_t mask)
> #define SCR_RES1_MASK (3U << 4)
> #define SCR_MASK (0x3fff & ~SCR_RES1_MASK)
>
> +#define NSACR_NSTRCDIS (1U << 20)
> +#define NSACR_RFR (1U << 19)
> +#define NSACR_NSASEDIS (1U << 15)
> +#define NSACR_NSD32DIS (1U << 14)
> +
> /* Return the current FPSCR value. */
> uint32_t vfp_get_fpscr(CPUARMState *env);
> void vfp_set_fpscr(CPUARMState *env, uint32_t val);
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index e43545a..6342dbf 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -489,7 +489,19 @@ static void cpacr_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> /* VFP coprocessor: cp10 & cp11 [23:20] */
> mask |= (1 << 31) | (1 << 30) | (0xf << 20);
>
> -if (!arm_feature(env, ARM_FEATURE_NEON)) {
> +if (arm_feature(env, ARM_FEATURE_NEON)) {
> +/* NSACR can disable non-secure writes to
> + * ASEDIS [31] or D32DIS [30]
> + */
> +if (arm_feature(env, ARM_FEATURE_EL3) &&
> !arm_is_secure(env)) {
> +if ((env->cp15.c1_nsacr & NSACR_NSASEDIS)) {
> +mask &= ~(1 << 31);
> +}
> +if ((env->cp15.c1_nsacr & NSACR_NSD32DIS)) {
> +mask &= ~(1 << 30);
> +}
> +}
> +} else {
> /* ASEDIS [31] bit is RAO/WI */
> value |= (1 << 31);
> }
> @@ -501,6 +513,7 @@ static void cpacr_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> !arm_feature(env, ARM_FEATURE_VFP3)) {
> /* D32DIS [30] is RAO/WI if D16-31 are not implemented. */
> value |= (1 << 30);
> +mask |= (1 << 30);
> }
> }
> value &= mask;
> @@ -2195,6 +2208,55 @@ static void scr_write(CPUARMState *env, const
> ARMCPRegInfo *ri, uint64_t value)
> raw_write(env, ri, value);
> }
>
> +static void nsacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> +uint32_t mask = 0;
> +
> +/* Pre ARMv8 some bits are RAO or UNK/SBZP */
> +if (!arm_feature(env, ARM_FEATURE_V8)) {
> +
> +if (arm_feature(env, ARM_FEATURE_VFP)) {
> +mask |= NSACR_NSASEDIS | NSACR_NSD32DIS;
> +
> +if (!arm_feature(env, ARM_FEATURE_NEON)) {
> +/* NSASEDIS are RAO/WI */
> +value |= NSACR_NSASEDIS;
> +}
> +
> +/* VFPv3 and upwards with NEON implement 32 double precision
> + * registers (D0-D31).
> + */
> +if (!arm_feature(env, ARM_FEATURE_NEON) ||
> +!arm_feature(env, ARM_FEATURE_VFP3)) {
> +/* NSD32DIS is RAO/WI if D16-31 are not implemented. */
> +value |= NSACR_NSD32DIS;
> +}
> +}
> +
> +/* cpn bits [13:0] */
> +mask = 0x3fff;
> +
> +value &= mask;
> +}
> +
> +raw_write(env, ri, value);
> +}
> +
> +static uint64_t nsacr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +uint64_t ret = raw_read(env, ri);
> +
> +if (arm_feature(env, ARM_FEATURE_V8)) {
> +if (!arm_feature(env, ARM_FEATURE_EL3) || (
> +arm_el_is_aa64(env, 3) && !is_a64(env) &&
> +arm_current_pl(env) != 3)) {
> +ret = 0xC00;
> +}
> +}
> +return ret;
> +}
> +
> static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
> { .name = "ELR_EL3", .state = ARM_CP_STATE_AA64,
>
Re: [Qemu-devel] [PATCH v4 00/33] target-arm: add Security Extensions for CPUs
Hey Greg
Great to see this version. I will try to go through it in the next days.
Best,
Fabian
On 01 Jul 2014, at 01:09, greg.bell...@linaro.org wrote:
> From: Greg Bellows
>
> Updated Fabian's v3 patchset for review comments. This patchset includes
> changes in support of the security extension on v7 aarch32 with hooks for
> later
> enabling v8 aarch64.
>
> The patches are built upon and therefore dependent on v3 of Xilinx's second
> round of EL2/3 patches.
>
> Summary of the changes from v3 -> v4:
> * Conditionally register security CP registers.
> * Fixed various bugs found in review
> * Reverted back to EL array-notation in combination with explicit v7 naming
> * Add functionality to handle migration of duplicate CP registrations
>
> Fabian Aggeler (29):
> target-arm: add cpu feature EL3 to CPUs with Security Extensions
> target-arm: move Aarch32 SCR into security reglist
> target-arm: increase arrays of registers R13 & R14
> target-arm: add arm_is_secure() function
> target-arm: make arm_current_pl() return PL3
> target-arm: A32: Emulate the SMC instruction
> target-arm: extend Aarch32 async excp masking
> target-arm: extend Aarch64 SCR.{FIQ|IRQ} handling
> target-arm: add async excp target_el&mode function
> target-arm: use dedicated target_el function
> target-arm: implement IRQ/FIQ routing to Monitor mode
> target-arm: Respect SCR.FW, SCR.AW and SCTLR.NMFI
> target-arm: add NSACR register
> target-arm: add MVBAR support
> target-arm: add macros to access banked registers
> target-arm: insert Aarch32 cpregs twice into hashtable
> target-arm: arrayfying fieldoffset for banking
> target-arm: add SCTLR_EL3 and make SCTLR banked
> target-arm: make CSSELR banked
> target-arm: add TTBR0_EL3 and make TTBR0/1 banked
> target-arm: add TCR_EL3 and make TTBCR banked
> target-arm: make c2_mask and c2_base_mask banked
> target-arm: make DACR banked
> target-arm: make IFSR banked
> target-arm: make DFSR banked
> target-arm: make IFAR/DFAR banked
> target-arm: make PAR banked
> target-arm: make VBAR banked
> target-arm: make c13 cp regs banked (FCSEIDR, ...)
>
> Greg Bellows (1):
> target-arm: Limit migration of duplicate CP regs
>
> Sergey Fedorov (3):
> target-arm: reject switching to monitor mode
> target-arm: add non-secure Translation Block flag
> target-arm: add SDER definition
>
> hw/arm/pxa2xx.c| 4 +-
> target-arm/cpu.c | 11 +-
> target-arm/cpu.h | 446 +---
> target-arm/helper.c| 722 +++--
> target-arm/internals.h | 5 +
> target-arm/machine.c | 4 +-
> target-arm/op_helper.c | 2 +-
> target-arm/translate-a64.c | 1 +
> target-arm/translate.c | 57 +++-
> target-arm/translate.h | 1 +
> 10 files changed, 1019 insertions(+), 234 deletions(-)
>
> --
> 1.8.3.2
>
>
>
Re: [Qemu-devel] [PATCH v3 05/32] target-arm: reject switching to monitor mode
Hm…yes, this case is missing, but it is only missing for ARMv7 as this bit is RES0 in ARMv8. Even in ARMv7 it is IMPDEF whether this bit is supported. And since ARMv7 mentions, that this bit is deprecated from the introduction of Virtualization Extensions I did not care to add this special case. On 12 Jun 2014, at 23:55, Greg Bellows mailto:greg.bell...@linaro.org>> wrote: Missing case where it is UNPREDICTABLE to enter FIQ mode from non-secure state if NSACR.RFR is 1. On 10 June 2014 18:54, Fabian Aggeler mailto:aggel...@ethz.ch>> wrote: From: Sergey Fedorov mailto:s.fedo...@samsung.com>> ...from non-secure state. Signed-off-by: Sergey Fedorov mailto:s.fedo...@samsung.com>> Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>> --- target-arm/helper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/target-arm/helper.c b/target-arm/helper.c index d8d6637..ace8d8b 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -3049,6 +3049,8 @@ static int bad_mode_switch(CPUARMState *env, int mode) case ARM_CPU_MODE_IRQ: case ARM_CPU_MODE_FIQ: return 0; +case ARM_CPU_MODE_MON: +return !arm_is_secure(env); default: return 1; } -- 1.8.3.2
Re: [Qemu-devel] [PATCH v3 32/32] target-arm: make c13 cp regs banked (FCSEIDR, ...)
On 23 Jun 2014, at 23:40, Greg Bellows
mailto:greg.bell...@linaro.org>> wrote:
On 10 June 2014 18:55, Fabian Aggeler
mailto:aggel...@ethz.ch>> wrote:
When EL3 is running in Aarch32 (or ARMv7 with Security Extensions)
FCSEIDR, CONTEXTIDR, TPIDRURW, TPIDRURO and TPIDRPRW have a secure
and a non-secure instance.
Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>>
---
target-arm/cpu.h| 45 -
target-arm/helper.c | 27 +--
2 files changed, 57 insertions(+), 15 deletions(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index c7d606e..13fa966 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -329,11 +329,46 @@ typedef struct CPUARMState {
};
uint64_t vbar_el2;
uint64_t mvbar; /* (monitor) vector base address register */
-uint32_t c13_fcse; /* FCSE PID. */
-uint64_t contextidr_el1; /* Context ID. */
-uint64_t tpidr_el0; /* User RW Thread register. */
-uint64_t tpidrro_el0; /* User RO Thread register. */
-uint64_t tpidr_el1; /* Privileged Thread register. */
+struct { /* FCSE PID. */
+uint32_t c13_fcseidr_ns;
+uint32_t c13_fcseidr_s;
+};
+union { /* Context ID. */
+struct {
+uint64_t contextidr_ns;
+uint64_t contextidr_s;
+};
+struct {
+uint64_t contextidr_el1;
+};
+};
+union { /* User RW Thread register. */
+struct {
+uint64_t tpidrurw_ns;
+uint64_t tpidrurw_s;
+};
+struct {
+uint64_t tpidr_el0;
+};
+};
+union { /* User RO Thread register. */
+struct {
+uint64_t tpidruro_ns;
+uint64_t tpidruro_s;
+};
+struct {
+uint64_t tpidrro_el0;
+};
+};
+union { /* Privileged Thread register. */
+struct {
+uint64_t tpidrprw_ns;
+uint64_t tpidrprw_s;
+};
+struct {
+uint64_t tpidr_el1;
+};
+};
uint64_t c14_cntfrq; /* Counter Frequency register */
uint64_t c14_cntkctl; /* Timer Control register */
ARMGenericTimer c14_timer[NUM_GTIMERS];
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 2d085aa..aebcc62 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -396,12 +396,15 @@ static const ARMCPRegInfo cp_reginfo[] = {
{ .name = "DBGDIDR", .cp = 14, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 0,
.access = PL0_R, .type = ARM_CP_CONST, .resetvalue = 0 },
{ .name = "FCSEIDR", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 0,
- .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
+ .access = PL1_RW,
+ .bank_fieldoffsets = { offsetof(CPUARMState, cp15.c13_fcseidr_s),
+ offsetof(CPUARMState, cp15.c13_fcseidr_ns) },
.resetvalue = 0, .writefn = fcse_write, .raw_writefn = raw_write, },
{ .name = "CONTEXTIDR", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 13, .crm = 0, .opc2 = 1,
.access = PL1_RW,
- .fieldoffset = offsetof(CPUARMState, cp15.contextidr_el1),
+ .bank_fieldoffsets = { offsetof(CPUARMState, cp15.contextidr_s),
+ offsetof(CPUARMState, cp15.contextidr_ns) },
.resetvalue = 0, .writefn = contextidr_write, .raw_writefn = raw_write,
},
REGINFO_SENTINEL
};
@@ -889,21 +892,25 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
.access = PL0_RW,
.fieldoffset = offsetof(CPUARMState, cp15.tpidr_el0), .resetvalue = 0 },
{ .name = "TPIDRURW", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 2,
- .access = PL0_RW,
- .fieldoffset = offsetoflow32(CPUARMState, cp15.tpidr_el0),
- .resetfn = arm_cp_reset_ignore },
+ .access = PL0_RW, .resetfn = arm_cp_reset_ignore,
+ .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.tpidrurw_s),
+ offsetoflow32(CPUARMState, cp15.tpidrurw_ns) } },
{ .name = "TPIDRRO_EL0", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 3, .opc2 = 3, .crn = 13, .crm = 0,
.access = PL0_R|PL1_W,
.fieldoffset = offsetof(CPUARMState, cp15.tpidrro_el0), .resetvalue = 0
},
{ .name = "TPIDRURO", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 3,
- .access = PL0_R|PL1_W,
- .fieldoffset = offsetoflow32(CPUARMState, cp15.tpidrro_el0),
- .resetfn = arm_cp_reset_ignore },
-{ .name = "TPIDR_EL1", .state = ARM_CP_STATE_BOTH,
+ .access = PL0_R|PL1_W, .resetfn = arm_cp_reset_ignore,
+ .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.tpidruro_s),
+ offsetoflow32(CPUARMState, cp15.tpidruro_ns) } },
+{ .name = "TPIDR_EL1", .state =
Re: [Qemu-devel] [PATCH v3 28/32] target-arm: make DFSR banked
On 17 Jun 2014, at 08:12, Edgar E. Iglesias wrote:
> On Fri, Jun 13, 2014 at 05:06:15PM -0500, Greg Bellows wrote:
>> I just wanted to point out that the change from array-notation to hard-code
>> numbers in the names undoes Edgar's EL2/EL3 changes. I prefer this way
>> over the array notation.
>
> Hi,
>
> This was discussed briefly here
> http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03561.html
>
> IMO, for some regs the array version doesn't make sense but for regs
> that need to be indexed by EL it does. Just look at what this patch
> results in for aarch64_cpu_do_interrupt(). AArch64 has a simpler/cleaner
> architecture in this respect, IMO we should keep the
> AArch64 simple and clean and take the banking pain in the AArch32 port.
>
You’re right. I didn’t like the outcome of this change either.
Since we can combine the advantages of both concepts in this compromise I
also vote for changing it to your proposed solution.
>>
>>
>> On 10 June 2014 18:55, Fabian Aggeler wrote:
>>
>>> When EL3 is running in Aarch32 (or ARMv7 with Security Extensions)
>>> DFSR has a secure and a non-secure instance.
>>>
>>> Signed-off-by: Fabian Aggeler
>>> ---
>>> target-arm/cpu.h| 13 -
>>> target-arm/helper-a64.c | 17 ++---
>>> target-arm/helper.c | 15 ---
>>> 3 files changed, 34 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index 54c51a4..71782cf 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -266,7 +266,18 @@ typedef struct CPUARMState {
>>> uint32_t ifsr32_el2;
>>> };
>>> };
>>> -uint64_t esr_el[4];
>>> +union {
>>> +struct {
>>> +uint64_t dfsr_ns;
>>> +uint64_t hsr;
>>> +uint64_t dfsr_s;
>>> +};
>>> +struct {
>>> +uint64_t esr_el1;
>>> +uint64_t esr_el2;
>>> +uint64_t esr_el3;
>>> +};
>>> +};
>
> I'd prefer:
>
> -uint64_t esr_el[4];
> +union {
> +struct {
> +uint64_t dummy
> +uint64_t dfsr_ns;
> +uint64_t hsr;
> +uint64_t dfsr_s;
> +};
> +struct {
> +uint64_t esr_el[4];
> +};
> +};
>
> And avoid the whole target_esr pointer thing in aarch64_cpu_do_interrupt().
>
> Cheers,
> Edgar
>
>
>>> uint32_t c6_region[8]; /* MPU base/size registers. */
>>> uint64_t far_el[4]; /* Fault address registers. */
>>> uint64_t par_el1; /* Translation result. */
>>> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
>>> index d7522b6..dbbf012 100644
>>> --- a/target-arm/helper-a64.c
>>> +++ b/target-arm/helper-a64.c
>>> @@ -447,6 +447,18 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>>> target_ulong addr = env->cp15.vbar_el[new_el];
>>> unsigned int new_mode = aarch64_pstate_mode(new_el, true);
>>> int i;
>>> +uint64_t *target_esr;
>>> +switch (new_el) {
>>> +case 3:
>>> +target_esr = &env->cp15.esr_el3;
>>> +break;
>>> +case 2:
>>> +target_esr = &env->cp15.esr_el2;
>>> +break;
>>> +case 1:
>>> +target_esr = &env->cp15.esr_el1;
>>> +break;
>>> +}
>>>
>>> if (arm_current_pl(env) < new_el) {
>>> if (env->aarch64) {
>>> @@ -477,8 +489,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>>> case EXCP_SWI:
>>> case EXCP_HVC:
>>> case EXCP_SMC:
>>> -env->cp15.esr_el[new_el] = env->exception.syndrome;
>>> -break;
>>> +*target_esr = env->exception.syndrome;
>>> case EXCP_IRQ:
>>> case EXCP_VIRQ:
>>> addr += 0x80;
>>> @@ -498,7 +509,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>>> } else {
>>> env->banked_spsr[0] = cpsr_read(env);
>>> if (!env->thumb) {
>>> -env->cp15.esr_el[new_el] |= 1 << 25;
>>> +*target_esr |= 1 << 25;
>>> }
>>> env->elr_el[new_el] = env->regs[15];
>>>
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index f51498a..793985e 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -1492,7 +1492,8 @@ static void vmsa_ttbr_write(CPUARMState *env, const
>>> ARMCPRegInfo *ri,
>>> static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>>> { .name = "DFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
>>> .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
>>> - .fieldoffset = offsetoflow32(CPUARMState, cp15.esr_el[1]),
>>> + .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.dfsr_s),
>>> + offsetoflow32(CPUARMState, cp15.dfsr_ns) },
>>> .resetfn = arm_cp_reset_ignore, },
>>> { .name = "IFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 1,
>>> .access = PL1_RW, .resetvalue = 0,
>>> @@ -1501,7 +150
Re: [Qemu-devel] [PATCH v3 15/32] target-arm: add NSACR register
On 13 Jun 2014, at 20:27, Greg Bellows
mailto:greg.bell...@linaro.org>> wrote:
On 10 June 2014 18:54, Fabian Aggeler
mailto:aggel...@ethz.ch>> wrote:
Implements NSACR register with corresponding read/write functions
for ARMv7 and ARMv8.
Signed-off-by: Sergey Fedorov
mailto:s.fedo...@samsung.com>>
Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>>
---
target-arm/cpu.h| 6 +
target-arm/helper.c | 68 -
2 files changed, 73 insertions(+), 1 deletion(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 52e679f..bc9edaa 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -182,6 +182,7 @@ typedef struct CPUARMState {
uint64_t c1_coproc; /* Coprocessor access register. */
uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */
uint32_t c1_scr; /* secure config register. */
+uint32_t c1_nsacr; /* Non-secure access control register. */
uint64_t ttbr0_el1; /* MMU translation table base 0. */
uint64_t ttbr1_el1; /* MMU translation table base 1. */
uint64_t c2_control; /* MMU translation table base control. */
@@ -593,6 +594,11 @@ static inline void xpsr_write(CPUARMState *env, uint32_t
val, uint32_t mask)
#define SCR_RES1_MASK (3U << 4)
#define SCR_MASK (0x3fff & ~SCR_RES1_MASK)
+#define NSACR_NSTRCDIS (1U << 20)
+#define NSACR_RFR (1U << 19)
+#define NSACR_NSASEDIS (1U << 15)
+#define NSACR_NSD32DIS (1U << 14)
+
/* Return the current FPSCR value. */
uint32_t vfp_get_fpscr(CPUARMState *env);
void vfp_set_fpscr(CPUARMState *env, uint32_t val);
diff --git a/target-arm/helper.c b/target-arm/helper.c
index f6ff4aa..9671f9f 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -489,7 +489,19 @@ static void cpacr_write(CPUARMState *env, const
ARMCPRegInfo *ri,
/* VFP coprocessor: cp10 & cp11 [23:20] */
mask |= (1 << 31) | (1 << 30) | (0xf << 20);
-if (!arm_feature(env, ARM_FEATURE_NEON)) {
+if (arm_feature(env, ARM_FEATURE_NEON)) {
+/* NSACR can disable non-secure writes to
+ * ASEDIS [31] or D32DIS [30]
+ */
+if (arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env)) {
+if ((env->cp15.c1_nsacr & NSACR_NSASEDIS)) {
+mask &= ~(1 << 31);
+}
+if ((env->cp15.c1_nsacr & NSACR_NSD32DIS)) {
+mask &= ~(1 << 30);
+}
+}
+} else {
/* ASEDIS [31] bit is RAO/WI */
value |= (1 << 31);
}
@@ -501,6 +513,7 @@ static void cpacr_write(CPUARMState *env, const
ARMCPRegInfo *ri,
!arm_feature(env, ARM_FEATURE_VFP3)) {
/* D32DIS [30] is RAO/WI if D16-31 are not implemented. */
value |= (1 << 30);
+mask |= (1 << 30);
}
}
value &= mask;
@@ -2184,6 +2197,55 @@ static void scr_write(CPUARMState *env, const
ARMCPRegInfo *ri, uint64_t value)
raw_write(env, ri, value);
}
+static void nsacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+uint32_t mask = 0;
+
+/* Pre ARMv8 some bits are RAO or UNK/SBZP */
+if (!arm_feature(env, ARM_FEATURE_V8)) {
+
+if (arm_feature(env, ARM_FEATURE_VFP)) {
+mask |= NSACR_NSASEDIS | NSACR_NSD32DIS;
+
+if (!arm_feature(env, ARM_FEATURE_NEON)) {
+/* NSASEDIS are RAO/WI */
+value |= NSACR_NSASEDIS;
+}
+
+/* VFPv3 and upwards with NEON implement 32 double precision
+ * registers (D0-D31).
+ */
+if (!arm_feature(env, ARM_FEATURE_NEON) ||
+!arm_feature(env, ARM_FEATURE_VFP3)) {
+/* NSD32DIS is RAO/WI if D16-31 are not implemented. */
+value |= NSACR_NSD32DIS;
+}
+}
+
+/* cpn bits [13:0] */
+mask = 0x3fff;
+
+value &= mask;
+}
+
+raw_write(env, ri, value);
+}
+
+static uint64_t nsacr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+uint64_t ret = raw_read(env, ri);
+
+if (arm_feature(env, ARM_FEATURE_V8)) {
+if (!arm_feature(env, ARM_FEATURE_EL3) || (
+arm_el_is_aa64(env, 3) && !is_a64(env) &&
+arm_current_pl(env) != 3)) {
+ret = 0xC00;
+}
It appears we are missing a case where 0xc00 is returned because we check for
the non-existence of EL3.
Right, this case is missing.
+}
+return ret;
+}
+
The ARMv8 spec suggests that if EL3 is aarch64 that a read or write of this
register occurs from secure EL1 when aarch32 it is trapped as an exception to
EL3. Is this omitted?
It seems I skipped that line when reading the ARMv8 spec. Should be added of
course.
sta
Re: [Qemu-devel] [PATCH v3 14/32] target-arm: Respect SCR.FW, SCR.AW and SCTLR.NMFI
On 13 Jun 2014, at 00:43, Greg Bellows
mailto:greg.bell...@linaro.org>> wrote:
On 10 June 2014 18:54, Fabian Aggeler
mailto:aggel...@ethz.ch>> wrote:
bits when modifying CPSR.
Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>>
---
target-arm/helper.c | 42 +++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 2fbecfa..f6ff4aa 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3091,9 +3091,6 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t
mask)
env->GE = (val >> 16) & 0xf;
}
-env->daif &= ~(CPSR_AIF & mask);
-env->daif |= val & CPSR_AIF & mask;
-
if ((env->uncached_cpsr ^ val) & mask & CPSR_M) {
if (bad_mode_switch(env, val & CPSR_M)) {
/* Attempt to switch to an invalid mode: this is UNPREDICTABLE.
@@ -3105,6 +3102,45 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t
mask)
switch_mode(env, val & CPSR_M);
}
}
+
+/* In an implementation that does not include Virtualization Extensions
+ * the SCR.FW and SCR.AW
Re: [Qemu-devel] [PATCH v3 11/32] target-arm: add async excp target_el&mode function
On 12 Jun 2014, at 23:56, Greg Bellows
mailto:greg.bell...@linaro.org>> wrote:
On 10 June 2014 18:54, Fabian Aggeler
mailto:aggel...@ethz.ch>> wrote:
Adds a dedicated function for IRQ and FIQ exceptions to determine
target_el and mode (Aarch32) according to tables in ARM ARMv8 and
ARM ARM v7.
Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>>
---
target-arm/cpu.h| 3 ++
target-arm/helper.c | 137
2 files changed, 140 insertions(+)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index b786a5a..52e679f 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -768,6 +768,9 @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el)
void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx);
+inline uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t *target_mode,
+uint32_t excp_idx, uint32_t cur_el,
+bool secure);
/* Interface between CPU and Interrupt controller. */
void armv7m_nvic_set_pending(void *opaque, int irq);
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 5822353..8333b52 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3224,6 +3224,21 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env,
uint32_t mode)
return 0;
}
+inline uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t *target_mode,
+uint32_t excp_idx, uint32_t cur_el,
+bool secure)
+{
+switch (excp_idx) {
+case EXCP_IRQ:
+*target_mode = ARM_CPU_MODE_IRQ;
+break;
+case EXCP_FIQ:
+*target_mode = ARM_CPU_MODE_FIQ;
+break;
+}
+return 1;
+}
+
unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
{
return 1;
@@ -3285,6 +3300,128 @@ void switch_mode(CPUARMState *env, int mode)
}
/*
+ * Determine the target EL for physical exceptions
+ */
+inline uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t *target_mode,
+uint32_t excp_idx, uint32_t cur_el,
+bool secure)
+{
+CPUARMState *env = cs->env_ptr;
+uint32_t target_el = 1;
+uint32_t excp_mode = 0;
+
+bool scr_routing = 0; /* IRQ, FIQ, EA */
+bool hcr_routing = 0; /* IMO, FMO, AMO */
+
+switch (excp_idx) {
+case EXCP_IRQ:
+scr_routing = (env->cp15.scr_el3 & SCR_IRQ);
+hcr_routing = (env->cp15.hcr_el2 & HCR_IMO);
+excp_mode = ARM_CPU_MODE_IRQ;
+break;
+case EXCP_FIQ:
+scr_routing = (env->cp15.scr_el3 & SCR_FIQ);
+hcr_routing = (env->cp15.hcr_el2 & HCR_FMO);
+excp_mode = ARM_CPU_MODE_FIQ;
+}
+
+/* If HCR.TGE is set all exceptions that would be routed to EL1 are
+ * routed to EL2 (in non-secure world).
+ */
+if (arm_feature(env, ARM_FEATURE_EL2) && (env->cp15.hcr_el2 & HCR_TGE)) {
+hcr_routing = 1;
+}
+
+/* Determine target EL according to ARM ARMv8 tables G1-15 and G1-16 */
+if (arm_el_is_aa64(env, 3)) {
+/* EL3 in Aarch64 */
+if (scr_routing) {
+/* IRQ|FIQ|EA == 1 */
+target_el = 3;
+} else {
+if (hcr_routing) {
+/* IRQ|FIQ|EA == 0
+ * IMO|FMO|AMO == 1 */
+if (secure) {
+/* Secure */
+target_el = 1;
+if (!arm_el_is_aa64(env, 1)) {
+/* EL1 using Aarch32 */
+*target_mode = ARM_CPU_MODE_ABT;
+}
+} else if (cur_el < 2) {
+/* Non-Secure goes to EL2 */
+target_el = 2;
+if (!arm_el_is_aa64(env, 2)) {
+/* EL2 using Aarch32 */
+*target_mode = ARM_CPU_MODE_HYP;
+}
+}
+} else if (env->cp15.scr_el3 & SCR_RW) {
+/* IRQ|FIQ|EA == 0
+ * IMO|FMO|AMO == 0
+ * RW == 1 (Next lower level is Aarch64)
+ */
+if (cur_el < 2) {
+target_el = 1;
+} else {
+/* Interrupt not taken but remains pending */
+}
+} else {
+/* IRQ|FIQ|EA == 0
+ * IMO|FMO|AMO == 0
+ * RW == 0 (Next lower level is Aarch64)
+ */
+if (cur_el < 2) {
+target_el = 1;
+*target_mode = ARM_CPU_MODE_ABT;
According to the aforementioned tables, the target mode should be excp_mode,
not always abort.
True, this is wrong and should be excp_mode, as you said.
+} else if (cur_el == 2) {
+target_el = 2;
+
Re: [Qemu-devel] [PATCH v3 02/32] target-arm: move Aarch32 SCR into security reglist
On 12 Jun 2014, at 23:55, Greg Bellows
mailto:greg.bell...@linaro.org>> wrote:
Conflict with Edgar's changes around the name of the security register group
v8_el3_cp_reginfo vs. security_cp_reginfo.
Given that there is a difference between the v7 regs and their v8 equivalents
such as encoding, I propose we create 2 separate groups, but map them to the
same storage where applicable. This somewhat follows on the SCR mapping
discussion.
That’s actually what I was trying to do here. I separated Aarch32 Security
Extension registers into a separate group.
They do map to the same storage already in this patch. I guess we could rename
the group to v8_el3_a32_cp_reginfo
or something like this. What do you think?
On 10 June 2014 18:54, Fabian Aggeler
mailto:aggel...@ethz.ch>> wrote:
Define a new ARM CP register info list for the ARMv7 Security Extension
feature. Register that list only for ARM cores with Security Extension/EL3
support. Moving Aarch32 SCR into Security Extension register group.
Signed-off-by: Sergey Fedorov
mailto:s.fedo...@samsung.com>>
Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>>
---
target-arm/helper.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index e157cc2..d8d6637 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -792,9 +792,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
.access = PL1_RW, .writefn = vbar_write,
.fieldoffset = offsetof(CPUARMState, cp15.vbar_el[1]),
.resetvalue = 0 },
-{ .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
- .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
- .resetvalue = 0, },
{ .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
.access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE },
@@ -2216,6 +2213,13 @@ static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
REGINFO_SENTINEL
};
+static const ARMCPRegInfo security_cp_reginfo[] = {
+{ .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
+ .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.scr_el3),
+ .resetvalue = 0, },
+REGINFO_SENTINEL
+};
+
static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t value)
{
@@ -2479,6 +2483,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
}
if (arm_feature(env, ARM_FEATURE_EL3)) {
define_arm_cp_regs(cpu, v8_el3_cp_reginfo);
+define_arm_cp_regs(cpu, security_cp_reginfo);
}
if (arm_feature(env, ARM_FEATURE_MPU)) {
/* These are the MPU registers prior to PMSAv6. Any new
--
1.8.3.2
Re: [Qemu-devel] [PATCH v3 31/32] target-arm: make VBAR banked
On 14 Jun 2014, at 00:43, Greg Bellows
mailto:greg.bell...@linaro.org>> wrote:
On 10 June 2014 18:55, Fabian Aggeler
mailto:aggel...@ethz.ch>> wrote:
When EL3 is running in Aarch32 (or ARMv7 with Security Extensions)
VBAR has a secure and a non-secure instance, which are mapped to
VBAR_EL1 and VBAR_EL3.
Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>>
---
target-arm/cpu.h| 12 +++-
target-arm/helper-a64.c | 6 +-
target-arm/helper.c | 14 +++---
3 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 048ede9..c7d606e 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -317,7 +317,17 @@ typedef struct CPUARMState {
uint32_t c9_pmuserenr; /* perf monitor user enable */
uint32_t c9_pminten; /* perf monitor interrupt enables */
uint64_t mair_el1;
-uint64_t vbar_el[4]; /* vector base address register */
+struct { /* vector base address register */
+union {
+uint64_t vbar_ns;
+uint64_t vbar_s;
+};
+union {
+uint64_t vbar_el1;
+uint64_t vbar_el3;
+};
+};
+uint64_t vbar_el2;
This is broken. I think the intent is a union of 2 structs rather than a
struct of two unions. Plus, vbar_el2 should be added in and hvbar made a union
of it.
union {
struct {
uint64_t vbar_ns;
uint64_t hvbar;
uint64_t vbar_s;
};
struct {
uint64_t vbar_el1;
uint64_t vbar_el2;
uint64_t vbar_el3;
};
};
Indeed. I left out the hvbar mapping to avoid introducing Virtualization
Extension changes in
this patchset but I guess it makes sense since we are touching it anyways.
uint64_t mvbar; /* (monitor) vector base address register */
uint32_t c13_fcse; /* FCSE PID. */
uint64_t contextidr_el1; /* Context ID. */
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 1fc0d3c..a66ec94 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -444,19 +444,23 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
ARMCPU *cpu = ARM_CPU(cs);
CPUARMState *env = &cpu->env;
unsigned int new_el = arm_excp_target_el(cs, cs->exception_index);
-target_ulong addr = env->cp15.vbar_el[new_el];
+target_ulong addr = 0;
unsigned int new_mode = aarch64_pstate_mode(new_el, true);
int i;
uint64_t *target_esr;
+
switch (new_el) {
case 3:
target_esr = &env->cp15.esr_el3;
+addr = env->cp15.vbar_el3;
break;
case 2:
target_esr = &env->cp15.esr_el2;
+addr = env->cp15.vbar_el2;
break;
case 1:
target_esr = &env->cp15.esr_el1;
+addr = env->cp15.vbar_el1;
break;
}
diff --git a/target-arm/helper.c b/target-arm/helper.c
index c3195bd..2d085aa 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -803,11 +803,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
.access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
.fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
.resetvalue = 0, .writefn = pmintenclr_write, },
-{ .name = "VBAR", .state = ARM_CP_STATE_BOTH,
+{ .name = "VBAR_EL1", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
- .access = PL1_RW, .writefn = vbar_write,
- .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[1]),
- .resetvalue = 0 },
+ .access = PL1_RW, .writefn = vbar_write, .resetvalue = 0,
+ .bank_fieldoffsets = { offsetof(CPUARMState, cp15.vbar_s),
+ offsetof(CPUARMState, cp15.vbar_ns) } },
In the cases where we are registering banked registers, it may be clearer to
keep the v7 name such as VBAR, because a banked VBAR_EL1 is counter intuitive.
{ .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
.access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE },
@@ -2207,7 +2207,7 @@ static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
{ .name = "VBAR_EL2", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
.access = PL2_RW, .writefn = vbar_write,
- .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[2]),
+ .fieldoffset = offsetof(CPUARMState, cp15.vbar_el2),
.resetvalue = 0 },
REGINFO_SENTINEL
};
@@ -2319,7 +2319,7 @@ static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
{ .name = "VBAR_EL3", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 6, .crn = 12, .crm = 0, .opc2 = 0,
.access = PL3_RW, .writefn = vbar_write,
- .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[3]),
+ .fieldoffset = offsetof(CPUARMState, cp15.vbar_el3),
.resetvalue = 0 },
{ .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
Re: [Qemu-devel] [PATCH v3 30/32] target-arm: make PAR banked
On 14 Jun 2014, at 00:49, Greg Bellows
mailto:greg.bell...@linaro.org>> wrote:
On 10 June 2014 18:55, Fabian Aggeler
mailto:aggel...@ethz.ch>> wrote:
When EL3 is running in Aarch32 (or ARMv7 with Security Extensions)
PAR has a secure and a non-secure instance.
Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>>
---
target-arm/cpu.h| 10 +-
target-arm/helper.c | 25 ++---
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 7f5124c..048ede9 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -299,7 +299,15 @@ typedef struct CPUARMState {
};
};
uint64_t far_el3;
-uint64_t par_el1; /* Translation result. */
+struct { /* Translation result. */
+union {
+uint64_t par_ns;
+uint64_t par_s;
+};
+union {
+uint64_t par_el1;
+};
+};
This is broken. This should be a union of structs rather than a struct of
unions. Should instead be.
union {
struct {
uint64_t par_ns;
uint64_t par_s;
};
struct {
uint64_t par_el1;
};
};
Right, that should have been the other way round, like you said.
uint32_t c9_insn; /* Cache lockdown registers. */
uint32_t c9_data;
uint32_t c9_pmcr; /* performance monitor control register */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 47bf7a7..c3195bd 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1281,7 +1281,7 @@ static void ats_write(CPUARMState *env, const
ARMCPRegInfo *ri, uint64_t value)
* fault.
*/
}
-env->cp15.par_el1 = par64;
+A32_BANKED_CURRENT_REG_SET(env, par, par64);
} else {
/* ret is a DFSR/IFSR value for the short descriptor
* translation table format (with WnR always clear).
@@ -1291,14 +1291,16 @@ static void ats_write(CPUARMState *env, const
ARMCPRegInfo *ri, uint64_t value)
/* We do not set any attribute bits in the PAR */
if (page_size == (1 << 24)
&& arm_feature(env, ARM_FEATURE_V7)) {
-env->cp15.par_el1 = (phys_addr & 0xff00) | 1 << 1;
+A32_BANKED_CURRENT_REG_SET(env, par,
+(phys_addr & 0xff00) | 1 << 1);
} else {
-env->cp15.par_el1 = phys_addr & 0xf000;
+A32_BANKED_CURRENT_REG_SET(env, par, phys_addr & 0xf000);
}
} else {
-env->cp15.par_el1 = ((ret & (1 << 10)) >> 5) |
-((ret & (1 << 12)) >> 6) |
-((ret & 0xf) << 1) | 1;
+A32_BANKED_CURRENT_REG_SET(env, par,
+((ret & (1 << 10)) >> 5) |
+((ret & (1 << 12)) >> 6) |
+((ret & 0xf) << 1) | 1);
}
}
}
@@ -1306,9 +1308,9 @@ static void ats_write(CPUARMState *env, const
ARMCPRegInfo *ri, uint64_t value)
static const ARMCPRegInfo vapa_cp_reginfo[] = {
{ .name = "PAR", .cp = 15, .crn = 7, .crm = 4, .opc1 = 0, .opc2 = 0,
- .access = PL1_RW, .resetvalue = 0,
- .fieldoffset = offsetoflow32(CPUARMState, cp15.par_el1),
- .writefn = par_write },
+ .access = PL1_RW, .resetvalue = 0, .writefn = par_write,
+ .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.par_s),
+ offsetoflow32(CPUARMState, cp15.par_ns) } },
#ifndef CONFIG_USER_ONLY
{ .name = "ATS", .cp = 15, .crn = 7, .crm = 8, .opc1 = 0, .opc2 = CP_ANY,
.access = PL1_W, .accessfn = ats_access,
@@ -1755,8 +1757,9 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
{ .name = "DBGDSAR", .cp = 14, .crm = 2, .opc1 = 0,
.access = PL0_R, .type = ARM_CP_CONST|ARM_CP_64BIT, .resetvalue = 0 },
{ .name = "PAR", .cp = 15, .crm = 7, .opc1 = 0,
- .access = PL1_RW, .type = ARM_CP_64BIT,
- .fieldoffset = offsetof(CPUARMState, cp15.par_el1), .resetvalue = 0 },
+ .access = PL1_RW, .type = ARM_CP_64BIT, .resetvalue = 0,
+ .bank_fieldoffsets = { offsetof(CPUARMState, cp15.par_s),
+ offsetof(CPUARMState, cp15.par_ns)} },
{ .name = "TTBR0", .cp = 15, .crm = 2, .opc1 = 0,
.access = PL1_RW, .type = ARM_CP_64BIT | ARM_CP_NO_MIGRATE,
.bank_fieldoffsets = { offsetof(CPUARMState, cp15.ttbr0_s),
--
1.8.3.2
Re: [Qemu-devel] [PATCH v3 06/32] target-arm: make arm_current_pl() return PL3
On 17 Jun 2014, at 07:40, Edgar E. Iglesias wrote:
> On Wed, Jun 11, 2014 at 01:54:48AM +0200, Fabian Aggeler wrote:
>> Make arm_current_pl() return PL3 for secure PL1 and monitor mode.
>> Increase MMU modes since mmu_index is directly infered from arm_
>> current_pl(). Changes assertion in arm_el_is_aa64() to allow EL3.
>>
>> Signed-off-by: Fabian Aggeler
>> ---
>> target-arm/cpu.h | 15 +--
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index cb0da6b..14007a9 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -100,7 +100,7 @@ typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
>>
>> struct arm_boot_info;
>>
>> -#define NB_MMU_MODES 2
>> +#define NB_MMU_MODES 4
>
> I guess this was unintentional here?
Not really. Well, it was first in a separate commit but I squashed it. We need
to increase
the number of MMU_MODES to 4 because arm_current_pl() now returns 3 for monitor
mode and secure PL1 (Aarch32) and EL3 (Aarch64), which is directly used as
mmu_index
since your last patchset. Is my reasoning wrong?
>
>
>>
>> /* We currently assume float and double are IEEE single and double
>>precision respectively.
>> @@ -710,7 +710,6 @@ static inline int arm_feature(CPUARMState *env, int
>> feature)
>> return (env->features & (1ULL << feature)) != 0;
>> }
>>
>> -
>
> This too.
>
>
>> /* Return true if exception level below EL3 is in secure state */
>> static inline bool arm_is_secure_below_el3(CPUARMState *env)
>> {
>> @@ -751,11 +750,12 @@ static inline bool arm_is_secure(CPUARMState *env)
>> /* Return true if the specified exception level is running in AArch64 state.
>> */
>> static inline bool arm_el_is_aa64(CPUARMState *env, int el)
>> {
>> -/* We don't currently support EL2 or EL3, and this isn't valid for EL0
>> +/* We don't currently support EL2, and this isn't valid for EL0
>> * (if we're in EL0, is_a64() is what you want, and if we're not in EL0
>> * then the state of EL0 isn't well defined.)
>> */
>> -assert(el == 1);
>> +assert(el == 1 || el == 3);
>> +
>> /* AArch64-capable CPUs always run with EL1 in AArch64 mode. This
>> * is a QEMU-imposed simplification which we may wish to change later.
>> * If we in future support EL2 and/or EL3, then the state of lower
>> @@ -947,9 +947,12 @@ static inline int arm_current_pl(CPUARMState *env)
>>
>> if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
>> return 0;
>> +} else if (arm_is_secure(env)) {
>> +/* Secure PL1 and monitor mode are mapped to PL3 */
>> +return 3;
>> }
>> -/* We don't currently implement the Virtualization or TrustZone
>> - * extensions, so PL2 and PL3 don't exist for us.
>> +/* We currently do not implement the Virtualization extensions, so PL2
>> does
>> + * not exist for us.
>> */
>> return 1;
>> }
>> --
>> 1.8.3.2
Re: [Qemu-devel] [PATCH v2 08/17] target-arm: Add SCR_EL3
On 09 Jun 2014, at 17:04, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Signed-off-by: Edgar E. Iglesias
> ---
> target-arm/cpu.h| 15 +++
> target-arm/helper.c | 29 +
> 2 files changed, 44 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cd8c9a7..111577c 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -185,6 +185,7 @@ typedef struct CPUARMState {
> uint32_t pmsav5_data_ap; /* PMSAv5 MPU data access permissions */
> uint32_t pmsav5_insn_ap; /* PMSAv5 MPU insn access permissions */
> uint64_t hcr_el2; /* Hypervisor configuration register */
> +uint32_t scr_el3; /* Secure configuration register. */
Is there a reason why we cannot map the Aarch32 SCR (c1_scr) to SCR_EL3?
Otherwise I suggest removing the existing c1_scr in this patch and adjusting
the
.fieldoffset of the Aarch32 SCR register definition.
Best,
Fabian
> uint32_t ifsr_el2; /* Fault status registers. */
> uint64_t esr_el[4];
> uint32_t c6_region[8]; /* MPU base/size registers. */
> @@ -561,6 +562,20 @@ static inline void xpsr_write(CPUARMState *env, uint32_t
> val, uint32_t mask)
> #define HCR_ID(1ULL << 33)
> #define HCR_MASK ((1ULL << 34) - 1)
>
> +#define SCR_NS(1U << 0)
> +#define SCR_IRQ (1U << 1)
> +#define SCR_FIQ (1U << 2)
> +#define SCR_EA(1U << 3)
> +#define SCR_SMD (1U << 7)
> +#define SCR_HCE (1U << 8)
> +#define SCR_SIF (1U << 9)
> +#define SCR_RW(1U << 10)
> +#define SCR_ST(1U << 11)
> +#define SCR_TWI (1U << 12)
> +#define SCR_TWE (1U << 13)
> +#define SCR_RES1_MASK (3U << 4)
> +#define SCR_MASK (0x3fff & ~SCR_RES1_MASK)
> +
> /* Return the current FPSCR value. */
> uint32_t vfp_get_fpscr(CPUARMState *env);
> void vfp_set_fpscr(CPUARMState *env, uint32_t val);
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d28951a..17cf80e 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2162,6 +2162,31 @@ static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
> REGINFO_SENTINEL
> };
>
> +static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t
> value)
> +{
> +uint32_t valid_mask = SCR_MASK;
> +
> +if (!arm_feature(env, ARM_FEATURE_EL2)) {
> +valid_mask &= ~SCR_HCE;
> +
> +/* On ARMv7, SMD (or SCD as it is called in v7) is only
> + * supported if EL2 exists. The bit is UNK/SBZP when
> + * EL2 is unavailable. In QEMU ARMv7, we force it to always zero
> + * when EL2 is unavailable.
> + */
> +if (arm_feature(env, ARM_FEATURE_V7)) {
> +valid_mask &= ~SCR_SMD;
> +}
> +}
> +
> +/* Set RES1 bits. */
> +value |= SCR_RES1_MASK;
> +
> +/* Clear RES0 bits. */
> +value &= valid_mask;
> +raw_write(env, ri, value);
> +}
> +
> static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
> { .name = "ELR_EL3", .state = ARM_CP_STATE_AA64,
> .type = ARM_CP_NO_MIGRATE,
> @@ -2184,6 +2209,10 @@ static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
> .access = PL3_RW, .writefn = vbar_write,
> .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[3]),
> .resetvalue = 0 },
> +{ .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
> + .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.scr_el3),
> + .writefn = scr_write },
> REGINFO_SENTINEL
> };
>
> --
> 1.8.3.2
>
Re: [Qemu-devel] [PATCH v1 11/16] target-arm: Don't take interrupts targeting lower ELs
On 09 Jun 2014, at 01:43, Edgar E. Iglesias wrote:
> On Sun, Jun 08, 2014 at 03:51:24PM +0000, Aggeler Fabian wrote:
>>
>> On 30 May 2014, at 09:28, Edgar E. Iglesias wrote:
>>
>>> From: "Edgar E. Iglesias"
>>>
>>> Signed-off-by: Edgar E. Iglesias
>>> ---
>>> target-arm/cpu.h | 7 +++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index 9eddcc1..66c58bd 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -1133,6 +1133,13 @@ bool write_cpustate_to_list(ARMCPU *cpu);
>>> static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
>>> {
>>>CPUARMState *env = cs->env_ptr;
>>> +unsigned int cur_el = arm_current_pl(env);
>>> +unsigned int target_el = arm_excp_target_el(cs, excp_idx);
>>> +
>>> +/* Don't take exceptions if they target a lower EL. */
>>> +if (cur_el > target_el) {
>>> +return false;
>>> +}
>>
>> Hi Edgar
>
> Hi Fabian,
>
>>
>> When making arm_excp_unmasked() reflect tables D1-13, D1-14, D1-15
>> and G1-18, G1-19 in ARM ARMv8 this should not be necessary if I am
>> not mistaken. Cases in which target_el is lower than cur_el are marked with
>> a P (pending) in the table. Or am I missing something interpreting the
>> tables?
>
> This function is called to check if we can take a pending interrupt or
> exception with current CPU state. It does not clear pending exceptions.
> In this case, if the target_el is lower than the current EL we return false
> and leave the exception pending (to be taken later).
I know, but what I want to say is, doesn’t the table which I mentioned reflect
this
by marking it with a P for pending for cases where target_el would be lower than
the current EL it is taken from?
So if we handle these cases in arm_excp_unmasked() we don’t need to call
arm_excp_target_el() to check.
>
>>
>> I extended your arm_excp_unmasked() and arm_excp_target_el() to reflect
>> the behaviour shown in the tables in ARM ARMv8 and ARM ARMv7. I will
>> send them with the TZ patches.
>
> Great, thanks.
>
> Cheers,
> Edgar
>
>
>>
>> Best,
>> Fabian
>>
>>>
>>>switch (excp_idx) {
>>>case EXCP_FIQ:
>>> --
>>> 1.8.3.2
Re: [Qemu-devel] [PATCH v2] target-arm: implement PD0/PD1 bits for TTBCR
Obviously not v2 but v3 (subject). Sorry for that, please ignore it as I am
going to resend.
Regards,
Fabian
On 10 Jun 2014, at 16:12, Fabian Aggeler wrote:
> Corrected handling of writes to TTBCR for ARMv8 (previously UNK/SBZP
> bits are not RES0) and ARMv7 (new bits PD0/PD1 for CPUs with Security
> Extensions).
>
> Bits PD0/PD1 are now respected in get_phys_addr_v6/v5() and
> get_level1_table_address.
>
> Signed-off-by: Fabian Aggeler
> ---
> v2 -> v3:
> * rebased
> * initializing domain in get_phys_addr_v5() as done in get_phys_addr_v6()
>
> v2: http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg01197.html
>
> target-arm/cpu.h| 16 ++
> target-arm/helper.c | 62 +
> 2 files changed, 60 insertions(+), 18 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 79e7f82..369d472 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -430,6 +430,22 @@ int arm_cpu_handle_mmu_fault(CPUState *cpu, vaddr
> address, int rw,
> /* Execution state bits. MRS read as zero, MSR writes ignored. */
> #define CPSR_EXEC (CPSR_T | CPSR_IT | CPSR_J)
>
> +#define TTBCR_N (7U << 0) /* TTBCR.EAE==0 */
> +#define TTBCR_T0SZ (7U << 0) /* TTBCR.EAE==1 */
> +#define TTBCR_PD0(1U << 4)
> +#define TTBCR_PD1(1U << 5)
> +#define TTBCR_EPD0 (1U << 7)
> +#define TTBCR_IRGN0 (3U << 8)
> +#define TTBCR_ORGN0 (3U << 10)
> +#define TTBCR_SH0(3U << 12)
> +#define TTBCR_T1SZ (3U << 16)
> +#define TTBCR_A1 (1U << 22)
> +#define TTBCR_EPD1 (1U << 23)
> +#define TTBCR_IRGN1 (3U << 24)
> +#define TTBCR_ORGN1 (3U << 26)
> +#define TTBCR_SH1(1U << 28)
> +#define TTBCR_EAE(1U << 31)
> +
> /* Bit definitions for ARMv8 SPSR (PSTATE) format.
> * Only these are valid when in AArch64 mode; in
> * AArch32 mode SPSRs are basically CPSR-format.
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 050c409..12285cd 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -312,7 +312,7 @@ static inline bool extended_addresses_enabled(CPUARMState
> *env)
> {
> return arm_el_is_aa64(env, 1)
> || ((arm_feature(env, ARM_FEATURE_LPAE)
> - && (env->cp15.c2_control & (1U << 31;
> + && (env->cp15.c2_control & TTBCR_EAE)));
> }
>
> static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t
> value)
> @@ -1413,11 +1413,22 @@ static void vmsa_ttbcr_raw_write(CPUARMState *env,
> const ARMCPRegInfo *ri,
> {
> int maskshift = extract32(value, 0, 3);
>
> -if (arm_feature(env, ARM_FEATURE_LPAE) && (value & (1 << 31))) {
> -value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
> -} else {
> -value &= 7;
> +if (!arm_feature(env, ARM_FEATURE_V8)) {
> +if (arm_feature(env, ARM_FEATURE_LPAE) && (value & TTBCR_EAE)) {
> +/* Pre ARMv8 bits [21:19], [15:14] and [6:3] are UNK/SBZP when
> + * using Long-desciptor translation table format */
> +value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
> +} else if (arm_feature(env, ARM_FEATURE_EL3)) {
> +/* In an implementation that includes the Security Extensions
> + * TTBCR has additional fields PD0 [4] and PD1 [5] for
> + * Short-descriptor translation table format.
> + */
> +value &= TTBCR_PD1 | TTBCR_PD0 | TTBCR_N;
> +} else {
> +value &= TTBCR_N;
> +}
> }
> +
> /* Note that we always calculate c2_mask and c2_base_mask, but
> * they are only used for short-descriptor tables (ie if EAE is 0);
> * for long-descriptor tables the TTBCR fields are used differently
> @@ -3540,17 +3551,24 @@ static inline int check_ap(CPUARMState *env, int ap,
> int domain_prot,
> }
> }
>
> -static uint32_t get_level1_table_address(CPUARMState *env, uint32_t address)
> +static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
> + uint32_t address)
> {
> -uint32_t table;
> -
> -if (address & env->cp15.c2_mask)
> -table = env->cp15.ttbr1_el1 & 0xc000;
> -else
> -table = env->cp15.ttbr0_el1 & env->cp15.c2_base_mask;
> -
> -table |= (address >> 18) & 0x3ffc;
> -return table;
> +if (address & env->cp15.c2_mask) {
> +if ((env->cp15.c2_control & TTBCR_PD1)) {
> +/* Translation table walk disabled for TTBR1 */
> +return false;
> +}
> +*table = env->cp15.ttbr1_el1 & 0xc000;
> +} else {
> +if ((env->cp15.c2_control & TTBCR_PD0)) {
> +/* Translation table walk disabled for TTBR0 */
> +return false;
> +}
> +*table = env->cp15.ttbr0_el1 & env->cp15.c2_base_mask;
> +}
> +*table |= (address >> 18) & 0x3ffc;
> +return true;
> }
>
> static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int
> access_type,
> @@ -3563,13 +3581,17 @@ static int g
Re: [Qemu-devel] [PATCH v2] target-arm: implement PD0/PD1 bits for TTBCR
On 09 Jun 2014, at 17:18, Peter Maydell wrote: > On 9 June 2014 12:27, Peter Maydell wrote: >> On 5 June 2014 11:39, Fabian Aggeler wrote: >>> Corrected handling of writes to TTBCR for ARMv8 (previously UNK/SBZP >>> bits are not RES0) and ARMv7 (new bits PD0/PD1 for CPUs with Security >>> Extensions). >>> >>> Bits PD0/PD1 are now respected in get_phys_addr_v6/v5() and >>> get_level1_table_address. >>> >>> Signed-off-by: Fabian Aggeler >> >> Applied to target-arm.next. > > Unapplied -- the 'goto do_fault' codepaths in get_phys_addr_v5() > end up using 'domain' in constructing the returned fault status > code without ever initialising it. > > Since this is a first level Translation fault, the DFSR Domain > field is UNKNOWN, so it should be sufficient to set it to 0. > You might as well just set it to 0 when it's defined at the top > of the function, as get_phys_addr_v6() does. > > thanks > -- PMM Thanks, I will send a v3 with domain initialised. Fabian
Re: [Qemu-devel] [PATCH v1 11/16] target-arm: Don't take interrupts targeting lower ELs
On 30 May 2014, at 09:28, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Signed-off-by: Edgar E. Iglesias
> ---
> target-arm/cpu.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 9eddcc1..66c58bd 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1133,6 +1133,13 @@ bool write_cpustate_to_list(ARMCPU *cpu);
> static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
> {
> CPUARMState *env = cs->env_ptr;
> +unsigned int cur_el = arm_current_pl(env);
> +unsigned int target_el = arm_excp_target_el(cs, excp_idx);
> +
> +/* Don't take exceptions if they target a lower EL. */
> +if (cur_el > target_el) {
> +return false;
> +}
Hi Edgar
When making arm_excp_unmasked() reflect tables D1-13, D1-14, D1-15
and G1-18, G1-19 in ARM ARMv8 this should not be necessary if I am
not mistaken. Cases in which target_el is lower than cur_el are marked with
a P (pending) in the table. Or am I missing something interpreting the
tables?
I extended your arm_excp_unmasked() and arm_excp_target_el() to reflect
the behaviour shown in the tables in ARM ARMv8 and ARM ARMv7. I will
send them with the TZ patches.
Best,
Fabian
>
> switch (excp_idx) {
> case EXCP_FIQ:
> --
> 1.8.3.2
>
Re: [Qemu-devel] [PATCH] target-arm: Prepare cpreg writefns/readfns for EL3/SecExt
On 31 May 2014, at 02:09, Peter Crosthwaite
wrote:
> On Fri, May 16, 2014 at 10:43 PM, Fabian Aggeler wrote:
>> This patch changes some readfns/writefns to use raw_write
>> and raw_read functions, wich use the fieldoffset specified
>
> "which"
>
>> in ARMCPRegInfo instead of directly accessing the field.
>> This will simplify patches for EL3 & Security Extensions.
>>
>
> Yes I like this idea is generally and universally. It makes the code
> more self documenting as these raw_write/raw_read sites clearly
> indicate that "this is the actual register state value", which
> everything else in the fn is then side effects. It does also mean any
> renaming of variables in the env now only have to be changed twice (in
> the env and in the .fieldoffset of CPRegInfo) rather than three/four
> times (in read/write handlers as well).
>
>> Signed-off-by: Fabian Aggeler
>> ---
>> This patch was previously part of the Security Extension patchset
>> but is not really Sec-Ext specific.
>>
>> target-arm/helper.c | 30 +++---
>> 1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 417161e..6302d67 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -319,7 +319,7 @@ static void dacr_write(CPUARMState *env, const
>> ARMCPRegInfo *ri, uint64_t value)
>> {
>> ARMCPU *cpu = arm_env_get_cpu(env);
>>
>> -env->cp15.c3 = value;
>> +raw_write(env, ri, value);
>> tlb_flush(CPU(cpu), 1); /* Flush TLB as domain not tracked in TLB */
>> }
>>
>> @@ -327,12 +327,12 @@ static void fcse_write(CPUARMState *env, const
>> ARMCPRegInfo *ri, uint64_t value)
>> {
>> ARMCPU *cpu = arm_env_get_cpu(env);
>>
>> -if (env->cp15.c13_fcse != value) {
>> +if (raw_read(env, ri) != value) {
>> /* Unlike real hardware the qemu TLB uses virtual addresses,
>> * not modified virtual addresses, so this causes a TLB flush.
>> */
>> tlb_flush(CPU(cpu), 1);
>> -env->cp15.c13_fcse = value;
>> +raw_write(env, ri, value);
>> }
>> }
>>
>> @@ -341,7 +341,7 @@ static void contextidr_write(CPUARMState *env, const
>> ARMCPRegInfo *ri,
>> {
>> ARMCPU *cpu = arm_env_get_cpu(env);
>>
>> -if (env->cp15.contextidr_el1 != value && !arm_feature(env,
>> ARM_FEATURE_MPU)
>> +if (raw_read(env, ri) != value && !arm_feature(env, ARM_FEATURE_MPU)
>> && !extended_addresses_enabled(env)) {
>> /* For VMSA (when not using the LPAE long descriptor page table
>> * format) this register includes the ASID, so do a TLB flush.
>> @@ -349,7 +349,7 @@ static void contextidr_write(CPUARMState *env, const
>> ARMCPRegInfo *ri,
>> */
>> tlb_flush(CPU(cpu), 1);
>> }
>> -env->cp15.contextidr_el1 = value;
>> +raw_write(env, ri, value);
>> }
>>
>> static void tlbiall_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> @@ -657,7 +657,7 @@ static void vbar_write(CPUARMState *env, const
>> ARMCPRegInfo *ri,
>> * contexts. (ARMv8 would permit us to do no masking at all, but ARMv7
>> * requires the bottom five bits to be RAZ/WI because they're UNK/SBZP.)
>> */
>> -env->cp15.c12_vbar = value & ~0x1FULL;
>> +raw_write(env, ri, value & ~0x1Ful);
>
> This one was already done in Edgar's series (now merged) so best to
> rebase to catch any other conflicts.
>
> But otherwise,
>
> Reviewed-by: Peter Crosthwaite
Thanks, I will rebase and send v2 to avoid conflicts.
Fabian
>
>> }
>>
>> static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> @@ -669,7 +669,7 @@ static uint64_t ccsidr_read(CPUARMState *env, const
>> ARMCPRegInfo *ri)
>> static void csselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> uint64_t value)
>> {
>> -env->cp15.c0_cssel = value & 0xf;
>> +raw_write(env, ri, value & 0xf);
>> }
>>
>> static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> @@ -1192,11 +1192,11 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[]
>> = {
>> static void par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t
>> value)
>> {
>> if (arm_feature(env, ARM_FEATURE_LPAE)) {
>> -env->cp15.par_el1 = value;
>> +raw_write(env, ri, value);
>> } else if (arm_feature(env, ARM_FEATURE_V7)) {
>> -env->cp15.par_el1 = value & 0xf6ff;
>> +raw_write(env, ri, value & 0xf6ff);
>> } else {
>> -env->cp15.par_el1 = value & 0xf1ff;
>> +raw_write(env, ri, value & 0xf1ff);
>> }
>> }
>>
>> @@ -1399,7 +1399,7 @@ static void vmsa_ttbcr_raw_write(CPUARMState *env,
>> const ARMCPRegInfo *ri,
>> * for long-descriptor tables the TTBCR fields are used differently
>> * and the c2_mask and c2_base_mask values are meaningless.
>> */
>> -env->cp15.c2_control = value;
>> +raw_write(env, ri, value);
>> env->cp15.c2_mask = ~(((uint32_t)0xu) >> maskshift);
>> en
Re: [Qemu-devel] [PATCH] target-arm: implement PD0/PD1 bits for TTBCR
On 02 Jun 2014, at 18:02, Peter Maydell wrote:
> On 30 May 2014 16:15, Fabian Aggeler wrote:
>> Corrected handling of writes to TTBCR for ARMv8 (previously UNK/SBZP
>> bits are not RES0) and ARMv7 (new bits PD0/PD1 for CPUs with Security
>> Extensions). Extracting T0SZ/T1SZ now uses 3 bits in Aarch32 and 6 bits
>> in Aarch64 as bits [5:3] are now RES0 when writing to Aarch32 TTBCR,
>> and not guaranteed to be zero anymore.
>>
>> Bits PD0/PD1 are now respected in get_phys_addr_lpae() and
>> get_phys_addr_v6/v5().
>>
>> Signed-off-by: Fabian Aggeler
>> ---
>> Parts of this patch were previously part of the TZ patchset but
>> were rewritten to include ARMv8 RES0 and PD0/PD1 handling.
>>
>> target-arm/cpu.h| 16
>> target-arm/helper.c | 70
>> +
>> 2 files changed, 71 insertions(+), 15 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 17a1ddd..fc5771e 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -441,6 +441,22 @@ int arm_cpu_handle_mmu_fault(CPUState *cpu, vaddr
>> address, int rw,
>> /* Execution state bits. MRS read as zero, MSR writes ignored. */
>> #define CPSR_EXEC (CPSR_T | CPSR_IT | CPSR_J)
>>
>> +#define TTBCR_N (7U << 0) /* TTBCR.EAE==0 */
>> +#define TTBCR_T0SZ (7U << 0) /* TTBCR.EAE==1 */
>> +#define TTBCR_PD0(1U << 4)
>> +#define TTBCR_PD1(1U << 5)
>> +#define TTBCR_EPD0 (1U << 7)
>> +#define TTBCR_IRGN0 (3U << 8)
>> +#define TTBCR_ORGN0 (3U << 10)
>> +#define TTBCR_SH0(3U << 12)
>> +#define TTBCR_T1SZ (3U << 16)
>> +#define TTBCR_A1 (1U << 22)
>> +#define TTBCR_EPD1 (1U << 23)
>> +#define TTBCR_IRGN1 (3U << 24)
>> +#define TTBCR_ORGN1 (3U << 26)
>> +#define TTBCR_SH1(1U << 28)
>> +#define TTBCR_EAE(1U << 31)
>> +
>> /* Bit definitions for ARMv8 SPSR (PSTATE) format.
>> * Only these are valid when in AArch64 mode; in
>> * AArch32 mode SPSRs are basically CPSR-format.
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 4e52145..10b965e 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -312,7 +312,7 @@ static inline bool
>> extended_addresses_enabled(CPUARMState *env)
>> {
>> return arm_el_is_aa64(env, 1)
>> || ((arm_feature(env, ARM_FEATURE_LPAE)
>> - && (env->cp15.c2_control & (1U << 31;
>> + && (env->cp15.c2_control & TTBCR_EAE)));
>> }
>>
>> static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t
>> value)
>> @@ -1410,11 +1410,22 @@ static void vmsa_ttbcr_raw_write(CPUARMState *env,
>> const ARMCPRegInfo *ri,
>> {
>> int maskshift = extract32(value, 0, 3);
>>
>> -if (arm_feature(env, ARM_FEATURE_LPAE) && (value & (1 << 31))) {
>> -value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
>> -} else {
>> -value &= 7;
>> +if (!arm_feature(env, ARM_FEATURE_V8)){
>
> Missing space before '{' (checkpatch finds this).
Thanks for catching this. I will correct it in v2.
>
>> +if (arm_feature(env, ARM_FEATURE_LPAE) && (value & TTBCR_EAE)) {
>> +/* Pre ARMv8 bits [21:19], [15:14] and [6:3] are UNK/SBZP when
>> + * using Long-desciptor translation table format */
>> +value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
>> +} else if (arm_feature(env, ARM_FEATURE_EL3)) {
>> +/* In an implementation that includes the Security Extensions
>> + * TTBCR has additional fields PD0 [4] and PD1 [5] for
>> + * Short-descriptor translation table format.
>> + */
>> +value &= TTBCR_PD1 | TTBCR_PD0 | TTBCR_N;
>> +} else {
>> +value &= TTBCR_N;
>> +}
>> }
>> +
>> /* Note that we always calculate c2_mask and c2_base_mask, but
>> * they are only used for short-descriptor tables (ie if EAE is 0);
>> * for long-descriptor tables the TTBCR fields are used differently
>> @@ -3670,15 +3681,18 @@ static inline int check_ap(CPUARMState *env, int ap,
>> int domain_prot,
>> }
>> }
>>
>> -static uint32_t get_level1_table_address(CPUARMState *env, uint32_t address)
>> +static uint32_t get_level1_table_address(CPUARMState *env, uint32_t address,
>> + int *ttbr_id)
>> {
>> uint32_t table;
>>
>> -if (address & env->cp15.c2_mask)
>> +if (address & env->cp15.c2_mask) {
>> table = env->cp15.ttbr1_el1 & 0xc000;
>> -else
>> +*ttbr_id = 1;
>> +} else {
>> table = env->cp15.ttbr0_el1 & env->cp15.c2_base_mask;
>> -
>> +*ttbr_id = 0;
>> +}
>> table |= (address >> 18) & 0x3ffc;
>> return table;
>> }
>> @@ -3691,6 +3705,7 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t
>> address, int access_type,
>> int code;
>> uint32_t table;
>> uint32_t desc;
>> +int ttbr_id;
>> int type;
>> int ap;
>> int domain;
>> @@ -3699,7 +3714,14 @@ static int get_phys_addr_v5(CPUAR
Re: [Qemu-devel] [PATCH] target-arm: Prepare cpreg writefns/readfns for EL3/SecExt
Ping. This patch was suggested to be sent separately but I can also
include it to the TZ patchset again.
http://patchwork.ozlabs.org/patch/349592/
Thanks,
Fabian
On 16 May 2014, at 14:43, Fabian Aggeler wrote:
> This patch changes some readfns/writefns to use raw_write
> and raw_read functions, wich use the fieldoffset specified
> in ARMCPRegInfo instead of directly accessing the field.
> This will simplify patches for EL3 & Security Extensions.
>
> Signed-off-by: Fabian Aggeler
> ---
> This patch was previously part of the Security Extension patchset
> but is not really Sec-Ext specific.
>
> target-arm/helper.c | 30 +++---
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 417161e..6302d67 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -319,7 +319,7 @@ static void dacr_write(CPUARMState *env, const
> ARMCPRegInfo *ri, uint64_t value)
> {
> ARMCPU *cpu = arm_env_get_cpu(env);
>
> -env->cp15.c3 = value;
> +raw_write(env, ri, value);
> tlb_flush(CPU(cpu), 1); /* Flush TLB as domain not tracked in TLB */
> }
>
> @@ -327,12 +327,12 @@ static void fcse_write(CPUARMState *env, const
> ARMCPRegInfo *ri, uint64_t value)
> {
> ARMCPU *cpu = arm_env_get_cpu(env);
>
> -if (env->cp15.c13_fcse != value) {
> +if (raw_read(env, ri) != value) {
> /* Unlike real hardware the qemu TLB uses virtual addresses,
> * not modified virtual addresses, so this causes a TLB flush.
> */
> tlb_flush(CPU(cpu), 1);
> -env->cp15.c13_fcse = value;
> +raw_write(env, ri, value);
> }
> }
>
> @@ -341,7 +341,7 @@ static void contextidr_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> {
> ARMCPU *cpu = arm_env_get_cpu(env);
>
> -if (env->cp15.contextidr_el1 != value && !arm_feature(env,
> ARM_FEATURE_MPU)
> +if (raw_read(env, ri) != value && !arm_feature(env, ARM_FEATURE_MPU)
> && !extended_addresses_enabled(env)) {
> /* For VMSA (when not using the LPAE long descriptor page table
> * format) this register includes the ASID, so do a TLB flush.
> @@ -349,7 +349,7 @@ static void contextidr_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> */
> tlb_flush(CPU(cpu), 1);
> }
> -env->cp15.contextidr_el1 = value;
> +raw_write(env, ri, value);
> }
>
> static void tlbiall_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -657,7 +657,7 @@ static void vbar_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> * contexts. (ARMv8 would permit us to do no masking at all, but ARMv7
> * requires the bottom five bits to be RAZ/WI because they're UNK/SBZP.)
> */
> -env->cp15.c12_vbar = value & ~0x1FULL;
> +raw_write(env, ri, value & ~0x1Ful);
> }
>
> static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> @@ -669,7 +669,7 @@ static uint64_t ccsidr_read(CPUARMState *env, const
> ARMCPRegInfo *ri)
> static void csselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> uint64_t value)
> {
> -env->cp15.c0_cssel = value & 0xf;
> +raw_write(env, ri, value & 0xf);
> }
>
> static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> @@ -1192,11 +1192,11 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[]
> = {
> static void par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t
> value)
> {
> if (arm_feature(env, ARM_FEATURE_LPAE)) {
> -env->cp15.par_el1 = value;
> +raw_write(env, ri, value);
> } else if (arm_feature(env, ARM_FEATURE_V7)) {
> -env->cp15.par_el1 = value & 0xf6ff;
> +raw_write(env, ri, value & 0xf6ff);
> } else {
> -env->cp15.par_el1 = value & 0xf1ff;
> +raw_write(env, ri, value & 0xf1ff);
> }
> }
>
> @@ -1399,7 +1399,7 @@ static void vmsa_ttbcr_raw_write(CPUARMState *env,
> const ARMCPRegInfo *ri,
> * for long-descriptor tables the TTBCR fields are used differently
> * and the c2_mask and c2_base_mask values are meaningless.
> */
> -env->cp15.c2_control = value;
> +raw_write(env, ri, value);
> env->cp15.c2_mask = ~(((uint32_t)0xu) >> maskshift);
> env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift);
> }
> @@ -1421,7 +1421,7 @@ static void vmsa_ttbcr_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> static void vmsa_ttbcr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> {
> env->cp15.c2_base_mask = 0xc000u;
> -env->cp15.c2_control = 0;
> +raw_write(env, ri, 0);
> env->cp15.c2_mask = 0;
> }
>
> @@ -1432,7 +1432,7 @@ static void vmsa_tcr_el1_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
>
> /* For AArch64 the A1 bit could result in a change of ASID, so TLB flush.
> */
> tlb_flush(CPU(cpu), 1);
> -env->cp15.c2_control = value;
> +raw_write(env, ri, value);
> }
>
> static void vmsa_ttbr_write(CPUARMState *env, c
Re: [Qemu-devel] [PATCH v2 16/23] target-arm: Use arm_current_sctlr to access SCTLR
On 22 May 2014, at 09:33, Edgar E. Iglesias wrote:
> On Tue, May 13, 2014 at 06:16:01PM +0200, Fabian Aggeler wrote:
>> Add SCTLR_EL3 and introduce new function to access correct
>> instance of SCTLR in different modes/worlds.
>
> Hi,
>
> AArch64 has a couple of insn/regs that do address translation
> as seen by other ELs. E.g, from EL3 you can perform address
> translation as seen by EL0. See for example ATS12E0R.
Good point, didn’t know about them.
> AArch32 has similar features, it can also lower S to NS when in S mode.
You mean other features than AT* operations? Or do you mean the possibility
to access both secure and non-secure instances from monitor mode by setting the
SCR.NS bit? This is currently handled by using the A32_MAPPED_EL3_REG_GET
which only gets the secure instance when the SCR.NS bit is clear.
>
> With regards to arm_current_sctlr() and the use in get_phys_addr, I
> don't think it is enough here.
>
> I was planning to post was patches that add new args to get_phys_addr()
> with the translation-EL and flags to control if stage-2 translation
> should be done or not. That way ats_write() can keep reusing
> get_phys_addr(). We would need to pass the security state as an argument
> aswell to handle AArch32. Does that make sense?
>From my view that makes sense.
I went through my changes again and in most of the locations where I changed
SCTLR access to use arm_current_sctlr() we should actually just access
SCTLR_EL1 (aa64_*_access() or ctr_el0_access()).
And otherwise:
- arm_cpu_do_interrupt() we still need to get distinguish between EL3 using
- Aarch32 (get secure/non-secure instance depending on arm_is_secure) or
- Aarch64 (get SCTLR_EL1).
- check_ap() is only used by get_phys_addr_v5/v6() so just return
secure / non-secure instance.
- arm_cpu_reset() in cpu.c we only need to check the SCTLR.V bit [13] for CPUs
without Aarch64, but need to use secure or non-secure instance depending on
the which world we are booting in.
If I didn’t miss or misinterpret something in this list I guess this does not
really need
its own function then as we don’t have much duplication.
Best,
Fabian
>
> Cheers,
> Edgar
>
>>
>> Signed-off-by: Fabian Aggeler
>> ---
>> hw/arm/pxa2xx.c| 2 +-
>> target-arm/cpu-qom.h | 1 +
>> target-arm/cpu.c | 4 +--
>> target-arm/cpu.h | 14 ++-
>> target-arm/helper.c| 67
>> ++
>> target-arm/op_helper.c | 2 +-
>> 6 files changed, 69 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>> index e0cd847..8b69e72 100644
>> --- a/hw/arm/pxa2xx.c
>> +++ b/hw/arm/pxa2xx.c
>> @@ -274,7 +274,7 @@ static void pxa2xx_pwrmode_write(CPUARMState *env, const
>> ARMCPRegInfo *ri,
>> case 3:
>> s->cpu->env.uncached_cpsr = ARM_CPU_MODE_SVC;
>> s->cpu->env.daif = PSTATE_A | PSTATE_F | PSTATE_I;
>> -s->cpu->env.cp15.c1_sys = 0;
>> +s->cpu->env.cp15.c1_sys_el1 = 0;
>> s->cpu->env.cp15.c1_coproc = 0;
>> s->cpu->env.cp15.ttbr0_el1 = 0;
>> s->cpu->env.cp15.c3 = 0;
>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>> index edc7f26..38cbd43 100644
>> --- a/target-arm/cpu-qom.h
>> +++ b/target-arm/cpu-qom.h
>> @@ -119,6 +119,7 @@ typedef struct ARMCPU {
>> uint32_t mvfr2;
>> uint32_t ctr;
>> uint32_t reset_sctlr;
>> +uint32_t reset_sctlr_el3;
>> uint32_t id_pfr0;
>> uint32_t id_pfr1;
>> uint32_t id_dfr0;
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index b0d4a9b..bdbdbb1 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -100,7 +100,7 @@ static void arm_cpu_reset(CPUState *s)
>> #if defined(CONFIG_USER_ONLY)
>> env->pstate = PSTATE_MODE_EL0t;
>> /* Userspace expects access to CTL_EL0 and the cache ops */
>> -env->cp15.c1_sys |= SCTLR_UCT | SCTLR_UCI;
>> +env->cp15.c1_sys_el1 |= SCTLR_UCT | SCTLR_UCI;
>> /* and to the FP/Neon instructions */
>> env->cp15.c1_coproc = deposit64(env->cp15.c1_coproc, 20, 2, 3);
>> #else
>> @@ -146,7 +146,7 @@ static void arm_cpu_reset(CPUState *s)
>> }
>> }
>>
>> -if (env->cp15.c1_sys & SCTLR_V) {
>> +if (arm_current_sctlr(env) & SCTLR_V) {
>> env->regs[15] = 0x;
>> }
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index a4bb6bd..780c1f5 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -180,7 +180,8 @@ typedef struct CPUARMState {
>> struct {
>> uint32_t c0_cpuid;
>> uint64_t c0_cssel; /* Cache size selection. */
>> -uint64_t c1_sys; /* System control register. */
>> +uint64_t c1_sys_el1; /* System control register (EL1). */
>> +uint64_t c1_sys_el3; /* System control register (EL3). */
>> uint64_t c1_coproc; /* Coprocessor access register. */
>> uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */
>>
Re: [Qemu-devel] [PATCH v2 14/23] target-arm: add banked coprocessor register type and macros
On 22 May 2014, at 14:18, Sergey Fedorov wrote:
>
> On 22.05.2014 15:49, Aggeler Fabian wrote:
>> On 22 May 2014, at 09:41, Edgar E. Iglesias wrote:
>>
>>> On Tue, May 13, 2014 at 06:15:59PM +0200, Fabian Aggeler wrote:
>>>> Banked CP registers can be defined with a A32_BANKED_REG macro which
>>>> defines
>>>> a non-secure instance of the register followed by an adjacent secure
>>>> instance.
>>>> Using a union makes the code backwards-compatible since the non-secure
>>>> instance can normally be accessed by its existing name.
>>>>
>>>> When translating a banked CP register access instruction in monitor mode,
>>>> the SCR.NS bit determines which instance is going to be accessed.
>>>>
>>>> If EL3 is operating in Aarch64 state coprocessor registers are not
>>>> banked anymore but in some cases have its own _EL3 instance.
>>> Hi
>>>
>>> Regarding the sctlr regs and the banking of S/NS regs in general, I
>>> think the general pattern should be to arrayify the regs that need
>>> to be indexed by EL.
>>>
>>> This is an example of a structure (indexed by EL) with the aarch32
>>> struct beeing here to help clarify.
>>> union {
>>> struct {
>>> uint64_t pad;
>>> uint64_t sctlr_ns;
>>> uint64_t hsctlr;
>>> uint64_t sctlr_s;
>>> } aarch32;
>>> uint64_t sctlr_el[4];
>>> }
>>>
>>> I think we would naturally want to register this for AArch32 as banked
>>> with NS=sctlr_el[1] and S=sctlr_el[3].
>>>
>>> Another register example is FAR. For FAR, things look like this
>>> (when EL2 is available and ignoring DFAR for clarity):
>>> union {
>>> struct {
>>> uint64_t pad;
>>> uint64_t ifar_ns;
>>> uint64_t ifar_s;
>>> } aarch32;
>>> uint64_t far_el[4];
>>> }
>>>
>>> Preferably we need something that can handle both cases.
>>> An option could be to arrayify the .fieldoffset in reginfos?
>>> If we don't want hardcoded TZ knowledge in the generic cpreg accessors,
>>> maybe there could be a generic function that returns the .fieldoffset
>>> index based on CPUState (e.g NS=0, S=1 etc). Or maybe specialized
>>> read/write fns would be enough.
>>>
>>> Just an idea to get the discussion going.
>>>
>>> struct ARMCPRegInfo {
>>>
>>> union {
>>> ptrdiff_t fieldoffset;
>>> ptrdiff_t fieldoffsets[2];
>>> };
>>> };
>>>
>>> unsigned int arm_cpreg_tzbank_idx(CPUARMState *env)
>>> {
>>> return is_a64(env) ? 0 : arm_is_secure(env);
>>> }
>>>
>>> Example:
>>> { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH,
>>> .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
>>> .access = PL1_RW,
>>> .fieldindex_fn = arm_cpreg_tzbank_idx,
>>> .fieldoffsets[] = { offsetof(CPUARMState, cp15.far_el[1]),
>>> offsetof(CPUARMState, cp15.far_el[2])},
>>> .resetvalue = 0, },
>>>
>>> ARMCPRegInfo sctlr = {
>>> .name = "SCTLR", .state = ARM_CP_STATE_BOTH,
>>> .opc0 = 3, .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 0,
>>> .access = PL1_RW,
>>> .fieldindex_fn = arm_cpreg_tzbank_idx,
>>> .fieldoffsets[] = { offsetof(CPUARMState, cp15.sctlr_el[1]),
>>> offsetof(CPUARMState, cp15.sctlr_el[3]),
>>> },
>>> /* Assuming raw_write and raw_read respect the indexing. */
>>> .writefn = sctlr_write, .resetvalue = cpu->reset_sctlr,
>>> .raw_writefn = raw_write,
>>> };
>>>
>>> Best regards,
>>> Edgar
>>>
>> Hi Edgar
>>
>> Thanks for joining the discussion. I like the idea of arrayifying the cp
>> regs, also for banking.
>> Since your patches are doing this anyways for EL registers I wanted to
>> change the registers
>> which do not have EL3/EL2 equivalents (DACR, PAR,…) to use the same
>> mechanism. These
>> registers are the third case which you haven’t mentioned in your examples
>> above, where we only
>> have one reg in Aarch64 but two (s/ns) in Aarch32. So I in my eyes it didn’t
>> make sense to make
>> the array bigger than needed, that’s why I went for 2 ent
Re: [Qemu-devel] [PATCH v2 14/23] target-arm: add banked coprocessor register type and macros
On 22 May 2014, at 09:41, Edgar E. Iglesias wrote:
> On Tue, May 13, 2014 at 06:15:59PM +0200, Fabian Aggeler wrote:
>> Banked CP registers can be defined with a A32_BANKED_REG macro which defines
>> a non-secure instance of the register followed by an adjacent secure
>> instance.
>> Using a union makes the code backwards-compatible since the non-secure
>> instance can normally be accessed by its existing name.
>>
>> When translating a banked CP register access instruction in monitor mode,
>> the SCR.NS bit determines which instance is going to be accessed.
>>
>> If EL3 is operating in Aarch64 state coprocessor registers are not
>> banked anymore but in some cases have its own _EL3 instance.
>
> Hi
>
> Regarding the sctlr regs and the banking of S/NS regs in general, I
> think the general pattern should be to arrayify the regs that need
> to be indexed by EL.
>
> This is an example of a structure (indexed by EL) with the aarch32
> struct beeing here to help clarify.
> union {
>struct {
>uint64_t pad;
>uint64_t sctlr_ns;
>uint64_t hsctlr;
>uint64_t sctlr_s;
>} aarch32;
>uint64_t sctlr_el[4];
> }
>
> I think we would naturally want to register this for AArch32 as banked
> with NS=sctlr_el[1] and S=sctlr_el[3].
>
> Another register example is FAR. For FAR, things look like this
> (when EL2 is available and ignoring DFAR for clarity):
> union {
>struct {
>uint64_t pad;
>uint64_t ifar_ns;
>uint64_t ifar_s;
>} aarch32;
>uint64_t far_el[4];
> }
>
> Preferably we need something that can handle both cases.
> An option could be to arrayify the .fieldoffset in reginfos?
> If we don't want hardcoded TZ knowledge in the generic cpreg accessors,
> maybe there could be a generic function that returns the .fieldoffset
> index based on CPUState (e.g NS=0, S=1 etc). Or maybe specialized
> read/write fns would be enough.
>
> Just an idea to get the discussion going.
>
> struct ARMCPRegInfo {
>
>union {
>ptrdiff_t fieldoffset;
>ptrdiff_t fieldoffsets[2];
>};
> };
>
> unsigned int arm_cpreg_tzbank_idx(CPUARMState *env)
> {
>return is_a64(env) ? 0 : arm_is_secure(env);
> }
>
> Example:
>{ .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH,
> .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
> .access = PL1_RW,
> .fieldindex_fn = arm_cpreg_tzbank_idx,
> .fieldoffsets[] = { offsetof(CPUARMState, cp15.far_el[1]),
> offsetof(CPUARMState, cp15.far_el[2])},
> .resetvalue = 0, },
>
> ARMCPRegInfo sctlr = {
> .name = "SCTLR", .state = ARM_CP_STATE_BOTH,
> .opc0 = 3, .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 0,
> .access = PL1_RW,
> .fieldindex_fn = arm_cpreg_tzbank_idx,
> .fieldoffsets[] = { offsetof(CPUARMState, cp15.sctlr_el[1]),
> offsetof(CPUARMState, cp15.sctlr_el[3]),
> },
> /* Assuming raw_write and raw_read respect the indexing. */
> .writefn = sctlr_write, .resetvalue = cpu->reset_sctlr,
> .raw_writefn = raw_write,
> };
>
> Best regards,
> Edgar
>
Hi Edgar
Thanks for joining the discussion. I like the idea of arrayifying the cp regs,
also for banking.
Since your patches are doing this anyways for EL registers I wanted to change
the registers
which do not have EL3/EL2 equivalents (DACR, PAR,…) to use the same mechanism.
These
registers are the third case which you haven’t mentioned in your examples
above, where we only
have one reg in Aarch64 but two (s/ns) in Aarch32. So I in my eyes it didn’t
make sense to make
the array bigger than needed, that’s why I went for 2 entries only. But if it
allows us map it easier
or in a more consistent way I don’t see why we cannot do it.
union {
struct {
uint64_t par_ns;
uint64_t par_s;
} aarch32;
uint64_t par_el[2];
}
We should probably also get rid of the macros I used to define the banked
registers, to make the code
look more uniform. Using your idea of arrayifying fieldset too, we could get
rid of the additional cpreg
definitions. Do we need to specify a .fieldindex_fn for every cpreg in this
case?
Isn’t it the same for all the cpregs which are banked (two fieldoffsets, the
first one for non-secure and
the second entry for secure)? But then we still need to know whether this
register is banked or common.
But what about accessing them not from within a cpreg read/write instruction?
We will have at least 3
cases of different indexes ({ns=1, s=2}, {ns=1, s=3}, {ns=0, s=1}). Although by
padding the last case
we could merge it with the first one so we only have 2 ways of accessing a
banked register, which was
also the case in my patches, for which I introduced macros. Any ideas how to
simplify that?
Thanks,
Fabian
>
>
>
>
>>
>> Signed-off-by: Sergey Fedorov
>> Signed-off-by: Fabian Aggeler
>> ---
>> target-arm/cpu.h | 121
>>
Re: [Qemu-devel] [PATCH v2 01/23] target-arm: add new CPU feature for Security Extensions
On 21 May 2014, at 16:51, Peter Maydell wrote:
> On 13 May 2014 17:15, Fabian Aggeler wrote:
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -631,6 +631,7 @@ enum arm_features {
>> ARM_FEATURE_CBAR, /* has cp15 CBAR */
>> ARM_FEATURE_CRC, /* ARMv8 CRC instructions */
>> ARM_FEATURE_CBAR_RO, /* has cp15 CBAR and it is read-only */
>> +ARM_FEATURE_SECURITY_EXTENSIONS, /* has Security Extensions */
>> };
>
> Also this feature name is pretty long and unwieldy; something
> shorter would be nice. I think we should probably go with
> ARM_FEATURE_EL2/ARM_FEATURE_EL3 as per Edgar's
> patchset.
>
> thanks
> -- PMM
Makes sense, I will use Edgar’s ARM_FEATURE_EL3 in the next version.
Fabian
Re: [Qemu-devel] [PATCH v2 04/23] target-arm: preserve RAO/WI bits of ARMv7 SCTLR
On 21 May 2014, at 18:12, Peter Maydell wrote:
> On 14 May 2014 06:43, Sergey Fedorov wrote:
>>
>> On 13.05.2014 20:15, Fabian Aggeler wrote:
>>> From: Svetlana Fedoseeva
>>>
>>> Signed-off-by: Svetlana Fedoseeva
>>> Signed-off-by: Sergey Fedorov
>>> Signed-off-by: Fabian Aggeler
>>> ---
>>> target-arm/helper.c | 5 +
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 9c3269f..2b57ad9 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -2083,6 +2083,11 @@ static void sctlr_write(CPUARMState *env, const
>>> ARMCPRegInfo *ri,
>>> {
>>> ARMCPU *cpu = arm_env_get_cpu(env);
>>>
>>> +if (arm_feature(env, ARM_FEATURE_V7)) {
>>> +value |= SCTLR_XP | SCTLR_U | SCTLR_nTWE | SCTLR_nTWI | SCTLR_L
>>> +| SCTLR_CP15BEN | SCTLR_P; /* These bits are RAO/WI */
>>
>> Actually, some of these bits are RAO/WI since v6. Also, there are some
>> RAZ/WI bits varying over architecture variants. There is some overview
>> at ARM ARM v7-AP section L.7.4. Maybe it is worth to fix more precisely
>> over supported architecture variants? By the way, this patch could be
>> separated from security extensions support patch set.
>
> Agreed. Our compliance for bits that should-be-0/1 is not great,
> but if we don't actually need to do those fixes for TZ support
> then they're probably better separated out (ie drop them from
> this patchset for the moment and submit them separately or
> later...)
>
> Also for v8 many of these RAZ/RAO bits become RES0/RES1 and
> the rules are different...
>
> thanks
> — PMM
Okay, I will separate them and submit them separately.
Thanks,
Fabian
Re: [Qemu-devel] [PATCH v2 00/23] target-arm: add Security Extensions for CPUs
que, state, ns, + crm, opc1, opc2); +} } } } @@@ -3501,25 -3319,22 +3501,25 @@@ void arm_cpu_do_interrupt(CPUState *cs env->exception.fsr = 2; /* Fall through to prefetch abort. */ case EXCP_PREFETCH_ABORT: -env->cp15.ifsr_el2 = env->exception.fsr; -env->cp15.far_el1 = deposit64(env->cp15.far_el1, 32, 32, - env->exception.vaddress); +BANKED_CP15_REG_SET(env, ifsr_el2, env->exception.fsr); +BANKED_CP15_REG_SET(env, far_el1, +deposit64(BANKED_CP15_REG_GET(env, far_el1), +32, 32, env->exception.vaddress)); qemu_log_mask(CPU_LOG_INT, "...with IFSR 0x%x IFAR 0x%x\n", - env->cp15.ifsr_el2, (uint32_t)env->exception.vaddress); + BANKED_CP15_REG_GET(env, ifsr_el2), + (uint32_t)env->exception.vaddress); new_mode = ARM_CPU_MODE_ABT; addr = 0x0c; mask = CPSR_A | CPSR_I; offset = 4; break; case EXCP_DATA_ABORT: -env->cp15.esr_el1 = env->exception.fsr; -env->cp15.far_el1 = deposit64(env->cp15.far_el1, 0, 32, - env->exception.vaddress); +BANKED_CP15_REG_SET(env, esr_el1, env->exception.fsr); +BANKED_CP15_REG_SET(env, far_el1, +deposit64(BANKED_CP15_REG_GET(env, far_el1), +0, 32, env->exception.vaddress)); qemu_log_mask(CPU_LOG_INT, "...with DFSR 0x%x DFAR 0x%x\n", - (uint32_t)env->cp15.esr_el1, + (uint32_t)BANKED_CP15_REG_GET(env, esr_el1), (uint32_t)env->exception.vaddress); new_mode = ARM_CPU_MODE_ABT; addr = 0x10; On 16 May 2014 01:00, Aggeler Fabian mailto:aggel...@student.ethz.ch>> wrote: Yes, sorry about that. Best, Fabian > On 15.05.2014, at 20:58, "Sergey Fedorov" > mailto:serge.f...@gmail.com>> wrote: > > Can s.fedo...@samsung.com<mailto:s.fedo...@samsung.com> be removed from CC > list since this mailbox has > been deleted? That was my address when I worked for Samsung. Now sending > to this address results with annoying delivery failure notification. > > Thanks, > Sergey. > > 13.05.2014 20:15, Fabian Aggeler wrote: >> Hi, >> >> This is a rework of the Samsung patches sent last year to add Security >> Extensions. The patches have been changed based on the discussion on >> the mailing list. Other changes became necessary because of Aarch64 >> support which got added in the meantime. This patchset makes it possible >> to run a kernel in the secure world and then switch to non-secure >> on CPUs that implement Security Extensions. It works for EL3 in Aarch32 >> state, but may add _EL3 registers where necessary to reflect the mapping >> of secure instances of cp registers to _EL3 registers. >> >> Banking of cp registers has been changed from active mass-swapping to >> the mechanism discussed on the mailing list, where every Aarch32 cp >> register goes into the hashtable twice. A ns-bit is added to the key >> of the register which is used when accessing a cp register to get the >> correct instance. >> >> Magic numbers have been changed to bitshifted constants or macros to make >> the code easier to read. >> >> The whole patchset now uses the term Security Extensions instead of >> TrustZone as this is the term which is used in the ARM ARM. >> >> I am happy for any feedback, especially for the banking of course. It should >> not be too hard to combine these changes with the recent effort towards EL3 >> in A64. >> >> Thanks, >> Fabian >> >> Fabian Aggeler (12): >> target-arm: add arm_is_secure() function >> target-arm: add NSACR support >> target-arm: Split TLB for secure state and EL3 in Aarch64 >> target-arm: add banked coprocessor register type and macros >> target-arm: Restrict EL3 to Aarch32 state >> target-arm: Use arm_current_sctlr to access SCTLR >> target-arm: Use raw_write/raw_read whenever possible >> target-arm: Convert banked coprocessor registers >> target-arm: maintain common bits of banked CP registers >> target-arm: add MVBAR support >> target-arm: implement IRQ/FIQ routing to Monitor mode >> target-arm: Respect SCR.FW, SCR.AW<http://scr.aw/> and SCTLR.NMFI >> >> Sergey Fedorov (8): >> target-arm: move SCR into Security Extensions register list >> target-a
Re: [Qemu-devel] [PATCH v3 22/22] RFC: target-arm: Use a 1:1 mapping between EL and MMU index
I guess this makes sense. Shouldn’t we implement two more MMUs to separate
S-EL0/EL0 and S-EL1/EL1
at least for ARMv8 with EL3 running in Aarch64 state? For ARMv7 and ARMv8 with
EL3 in Aarch32 S-PL1
is mapped to PL3, so we only need one additional MMU for S-PL0. If you agree I
could add this change in
the Security Extension patches after this patch makes it into the tree.
Best,
Fabian
On 19 May 2014, at 11:23, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Signed-off-by: Edgar E. Iglesias
> ---
> target-arm/cpu.h | 26 --
> target-arm/translate.h | 2 +-
> 2 files changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 566f9ed..3b7ef32 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1084,32 +1084,14 @@ static inline CPUARMState *cpu_init(const char
> *cpu_model)
> #define cpu_list arm_cpu_list
>
> /* MMU modes definitions */
> -#define MMU_MODE0_SUFFIX _kernel
> -#define MMU_MODE1_SUFFIX _user
> -#define MMU_USER_IDX 1
> -
> -static inline int arm_el_to_mmu_idx(int current_el)
> -{
> -#ifdef CONFIG_USER_ONLY
> -return MMU_USER_IDX;
> -#else
> -switch (current_el) {
> -case 0:
> -return MMU_USER_IDX;
> -case 1:
> -return 0;
> -default:
> -/* Unsupported EL. */
> -assert(0);
> -return 0;
> -}
> -#endif
> -}
> +#define MMU_MODE0_SUFFIX _user
> +#define MMU_MODE1_SUFFIX _kernel
> +#define MMU_USER_IDX 0
>
> static inline int cpu_mmu_index (CPUARMState *env)
> {
> int cur_el = arm_current_pl(env);
> -return arm_el_to_mmu_idx(cur_el);
> +return cur_el;
> }
>
> #include "exec/cpu-all.h"
> diff --git a/target-arm/translate.h b/target-arm/translate.h
> index db6f0af..31a0104 100644
> --- a/target-arm/translate.h
> +++ b/target-arm/translate.h
> @@ -54,7 +54,7 @@ static inline int arm_dc_feature(DisasContext *dc, int
> feature)
>
> static inline int get_mem_index(DisasContext *s)
> {
> -return arm_el_to_mmu_idx(s->current_pl);
> +return s->current_pl;
> }
>
> /* target-specific extra values for is_jmp */
> --
> 1.8.3.2
>
Re: [Qemu-devel] OS X compile fix
You’re configuration works for me on 10.9 with clang from the XCode command line tools. $ xcode-select --install $ clang --version Apple LLVM version 5.1 (clang-503.0.40) (based on LLVM 3.4svn) Target: x86_64-apple-darwin13.2.0 Thread model: posix Best, Fabian On 19 May 2014, at 10:19, Peter Maydell wrote: > On 19 May 2014 01:06, Peter Bartoli wrote: >> On May 18, 2014, at 4:09 PM, Peter Maydell wrote: >> >> and looking at the preprocessor output it's defined in >> /usr/lib/clang/5.0/include/limits.h >> >> >> Thanks, Peter ... I don't have a /usr/lib/clang ... where are you getting it >> from? > > Pretty sure this is just the standard way XCode command line > tools install on 10.8. Googling suggests that 10.9 does things > differently (leaving the headers under /Applications/Xcode.app, > and using shim executables to redirect the paths it looks for). In > that case the header is likely to be > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/5.1/include/limits.h > > thanks > -- PMM > >
Re: [Qemu-devel] [PATCH v2 00/23] target-arm: add Security Extensions for CPUs
Yes, sorry about that. Best, Fabian > On 15.05.2014, at 20:58, "Sergey Fedorov" wrote: > > Can s.fedo...@samsung.com be removed from CC list since this mailbox has > been deleted? That was my address when I worked for Samsung. Now sending > to this address results with annoying delivery failure notification. > > Thanks, > Sergey. > > 13.05.2014 20:15, Fabian Aggeler wrote: >> Hi, >> >> This is a rework of the Samsung patches sent last year to add Security >> Extensions. The patches have been changed based on the discussion on >> the mailing list. Other changes became necessary because of Aarch64 >> support which got added in the meantime. This patchset makes it possible >> to run a kernel in the secure world and then switch to non-secure >> on CPUs that implement Security Extensions. It works for EL3 in Aarch32 >> state, but may add _EL3 registers where necessary to reflect the mapping >> of secure instances of cp registers to _EL3 registers. >> >> Banking of cp registers has been changed from active mass-swapping to >> the mechanism discussed on the mailing list, where every Aarch32 cp >> register goes into the hashtable twice. A ns-bit is added to the key >> of the register which is used when accessing a cp register to get the >> correct instance. >> >> Magic numbers have been changed to bitshifted constants or macros to make >> the code easier to read. >> >> The whole patchset now uses the term Security Extensions instead of >> TrustZone as this is the term which is used in the ARM ARM. >> >> I am happy for any feedback, especially for the banking of course. It should >> not be too hard to combine these changes with the recent effort towards EL3 >> in A64. >> >> Thanks, >> Fabian >> >> Fabian Aggeler (12): >> target-arm: add arm_is_secure() function >> target-arm: add NSACR support >> target-arm: Split TLB for secure state and EL3 in Aarch64 >> target-arm: add banked coprocessor register type and macros >> target-arm: Restrict EL3 to Aarch32 state >> target-arm: Use arm_current_sctlr to access SCTLR >> target-arm: Use raw_write/raw_read whenever possible >> target-arm: Convert banked coprocessor registers >> target-arm: maintain common bits of banked CP registers >> target-arm: add MVBAR support >> target-arm: implement IRQ/FIQ routing to Monitor mode >> target-arm: Respect SCR.FW, SCR.AW and SCTLR.NMFI >> >> Sergey Fedorov (8): >> target-arm: move SCR into Security Extensions register list >> target-arm: adjust TTBCR for Security Extension feature >> target-arm: reject switching to monitor mode from non-secure state >> target-arm: adjust arm_current_pl() for Security Extensions >> target-arm: add non-secure Translation Block flag >> target-arm: implement CPACR register logic >> target-arm: add SDER definition >> target-arm: implement SMC instruction >> >> Svetlana Fedoseeva (3): >> target-arm: add new CPU feature for Security Extensions >> target-arm: preserve RAO/WI bits of ARMv7 SCTLR >> target-arm: add CPU Monitor mode >> >> hw/arm/pxa2xx.c| 2 +- >> linux-user/main.c | 2 +- >> target-arm/cpu-qom.h | 1 + >> target-arm/cpu.c | 8 +- >> target-arm/cpu.h | 271 ++--- >> target-arm/helper-a64.c| 3 +- >> target-arm/helper.c| 489 >> - >> target-arm/machine.c | 6 +- >> target-arm/op_helper.c | 2 +- >> target-arm/translate-a64.c | 9 +- >> target-arm/translate.c | 342 ++- >> target-arm/translate.h | 4 + >> 12 files changed, 866 insertions(+), 273 deletions(-) >
Re: [Qemu-devel] [PATCH v2 14/23] target-arm: add banked coprocessor register type and macros
On 15 May 2014, at 20:42, Sergey Fedorov wrote: > 13.05.2014 20:15, Fabian Aggeler wrote: >> @@ -771,6 +862,14 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid) >> * IO indicates that this register does I/O and therefore its accesses >> * need to be surrounded by gen_io_start()/gen_io_end(). In particular, >> * registers which implement clocks or timers require this. >> + * In an implementation with Security Extensions supporting Aarch32 cp regs >> can >> + * be banked or common. If a register is common it references the same >> variable >> + * from both worlds (non-secure and secure). For cp regs which neither set >> + * ARM_CP_SECURE nor ARM_CP_NONSECURE (default) we assume it's common and it >> + * will be inserted twice into the hashtable. If a register has >> + * ARM_CP_BANKED/ARM_CP_BANKED_64BIT set, it will be inserted twice but with >> + * different offset respectively. This way Aarch32 registers which can be >> + * mapped to Aarch64 PL3 registers can be inserted individually. >> */ > > I think it could be done with just adding a single flag indicating that > NS tag of reginfo should be considered, otherwise insert reginfo into > the hashtable twice. > In order to distinguish 64 bit register, there is already ARM_CP_64BIT > macro defined. The distinction whether the underlying field is 64bit or 32bit was necessary because some cp regs are not of type ARM_CP_64BIT but get mapped to (the lower or upper 32 bits of) a 64bit field. So to be able to add the correct offset in the case of a banked register I came up with these two types. Since there are only a few registers left which don’t get mapped to EL3 registers we could define them explicitly and therefore get rid of ARM_CP_BANKED and ARM_CP_BANKED_64BIT as well as increasing the offset before adding them to the hashtable. But like this we would still need 2 bits, one to specify whether it is ns or not, and one whether it is common or banked. Or am I missing something here? > > Regards, > Sergey > >> #define ARM_CP_SPECIAL 1 >> #define ARM_CP_CONST 2 >> @@ -779,16 +878,20 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t >> cpregid) >> #define ARM_CP_OVERRIDE 16 >> #define ARM_CP_NO_MIGRATE 32 >> #define ARM_CP_IO 64 >> -#define ARM_CP_NOP (ARM_CP_SPECIAL | (1 << 8)) >> -#define ARM_CP_WFI (ARM_CP_SPECIAL | (2 << 8)) >> -#define ARM_CP_NZCV (ARM_CP_SPECIAL | (3 << 8)) >> -#define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 8)) >> -#define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 8)) >> +#define ARM_CP_SECURE (1 << 7) >> +#define ARM_CP_NONSECURE (1 << 8) >> +#define ARM_CP_BANKED (ARM_CP_NONSECURE | ARM_CP_SECURE) >> +#define ARM_CP_BANKED_64BIT ((1 << 9) | ARM_CP_BANKED) >> +#define ARM_CP_NOP (ARM_CP_SPECIAL | (1 << 10)) >> +#define ARM_CP_WFI (ARM_CP_SPECIAL | (2 << 10)) >> +#define ARM_CP_NZCV (ARM_CP_SPECIAL | (3 << 10)) >> +#define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 10)) >> +#define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 10)) >> #define ARM_LAST_SPECIAL ARM_CP_DC_ZVA >> /* Used only as a terminator for ARMCPRegInfo lists */ >> #define ARM_CP_SENTINEL 0x >> /* Mask of only the flag bits in a type field */ >> -#define ARM_CP_FLAG_MASK 0x7f >> +#define ARM_CP_FLAG_MASK 0x3ff >> >> /* Valid values for ARMCPRegInfo state field, indicating which of >> * the AArch32 and AArch64 execution states this register is visible in. >
Re: [Qemu-devel] [PATCH v2 19/23] target-arm: maintain common bits of banked CP registers
On 14 May 2014, at 23:20, Greg Bellows
mailto:greg.bell...@linaro.org>> wrote:
On 13 May 2014 11:16, Fabian Aggeler
mailto:aggel...@ethz.ch>> wrote:
Some of SCTRL bits are common for secure and non-secure state.
Translation table base masks are updated on NS-bit switch as
well as on TTBCR writes.
Signed-off-by: Sergey Fedorov
mailto:s.fedo...@samsung.com>>
Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>>
---
target-arm/cpu.h| 10 ++
target-arm/helper.c | 39 ++-
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index c20c44a..7893004 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -425,6 +425,14 @@ int arm_cpu_handle_mmu_fault(CPUState *cpu, vaddr address,
int rw,
#define SCTLR_AFE (1U << 29)
#define SCTLR_TE (1U << 30)
+/* Bitmask for banked bits (Security Extensions) */
+#define SCTLR_BANKED(SCTLR_TE | SCTLR_AFE | SCTLR_TRE | SCTLR_EE | \
+SCTLR_VE | SCTLR_HA | SCTLR_UWXN | SCTLR_WXN | SCTLR_V | \
+SCTLR_I | SCTLR_Z | SCTLR_SW | SCTLR_CP15BEN | SCTLR_C | \
+SCTLR_A | SCTLR_M)
+/* Treat bits that are not explicitly banked as common */
+#define SCTLR_COMMON (~SCTLR_BANKED)
+
#define CPSR_M (0x1fU)
#define CPSR_T (1U << 5)
#define CPSR_F (1U << 6)
@@ -662,6 +670,8 @@ static inline int arm_feature(CPUARMState *env, int feature)
return (env->features & (1ULL << feature)) != 0;
}
+#define SCR_NS (1U << 0)
+
/* Return true if the processor is in secure state */
static inline bool arm_is_secure(CPUARMState *env)
{
diff --git a/target-arm/helper.c b/target-arm/helper.c
index c76a86b..618fd31 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2165,12 +2165,49 @@ static void sctlr_write(CPUARMState *env, const
ARMCPRegInfo *ri,
}
raw_write(env, ri, value);
+
+if (!arm_el_is_aa64(env, 3)) {
+/* Aarch32 has some bits that are common to secure (mapped to
+ * SCTLR_EL3) and non-secure instances of the SCTLR. */
+
+env->cp15.c1_sys_el1 &= SCTLR_BANKED |
+(arm_current_sctlr(env) & SCTLR_COMMON);
+env->cp15.c1_sys_el3 &= SCTLR_BANKED |
+(arm_current_sctlr(env) & SCTLR_COMMON);
+}
+
/* ??? Lots of these bits are not implemented. */
/* This may enable/disable the MMU, so do a TLB flush. */
tlb_flush(CPU(cpu), 1);
}
#ifndef CONFIG_USER_ONLY
+static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
+{
+if (!arm_el_is_aa64(env, 3) &&
+(value & SCR_NS) != (env->cp15.c1_scr & SCR_NS)) {
+/* If EL3 is using Aarch32 switching NS-bit may make the CPU use a
+ * different instance (secure or non-secure) when accessing CP
+ * registers.
+ * Common bits of otherwise banked registers need to by synchronized
+ * at this point.
+ */
+env->cp15.c1_sys_el1 &= SCTLR_BANKED |
+(arm_current_sctlr(env) & SCTLR_COMMON);
+env->cp15.c1_sys_el3 &= SCTLR_BANKED |
+(arm_current_sctlr(env) & SCTLR_COMMON);
I must be missing something, if the common bits are sync'd across the banks in
sctlr_write(), why do we need to do it here?
Right, should not be needed. I’ll remove it.
+}
+
+env->cp15.c1_scr = value;
+
+/* After possible switch, calculate c2_mask and c2_base_mask again for the
+ * instance which is now active (secure or non-secure).
+ */
+int maskshift = extract32(A32_BANKED_REG_GET(env, c2_control), 0, 3);
+env->cp15.c2_mask = ~(((uint32_t)0xu) >> maskshift);
+env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift);
As mentioned elsewhere, does it make sense to bank the c2_mask fields so they
stay in sync with c2_control? Presumably we would not need to do these updates
in this case.
Good idea, I will bank them and remove this bit.
+
+}
static void nsacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t value)
@@ -2183,7 +2220,7 @@ static const ARMCPRegInfo tz_cp_reginfo[] = {
#ifndef CONFIG_USER_ONLY
{ .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
.access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
- .resetvalue = 0, },
+ .resetvalue = 0, .writefn = scr_write },
{ .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .crn = 6, .crm = 1, .opc1 = 0, .opc2 = 0,
.access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
--
1.8.3.2
Re: [Qemu-devel] [PATCH v2 06/23] target-arm: add arm_is_secure() function
The v8 ARM ARM only leaves it as implementation defined when EL2 is not implemented, otherwise is has only a non-secure state as Peter said. Another reason why I chose to make the default non-secure is the suggested mapping of non-secure Aarch32 registers to EL1 and secure Aarch32 registers to EL3 (D1.20.1). To me it seems that it depends whether we want to follow the ARM ARM v7 or v8. Based on Peter’s comment I’d go for the NS-only approach. Fabian On 15 May 2014, at 00:22, Greg Bellows mailto:greg.bell...@linaro.org>> wrote: Thanks for the input Peter. >From briefly looking, it actually looks like the ARMv7 spec more strictly >states that the default is secure. ARMv8 on the other hand appears to leave >it open as implementation defined, Based on out past discussions, I assumed non-secure as the default and that appears to be the direction of Fabian's code as well. Clearly if we want to follow the spec, we'd both need to change this default. Greg On 14 May 2014 16:29, Peter Maydell mailto:peter.mayd...@linaro.org>> wrote: On 14 May 2014 21:22, Greg Bellows mailto:greg.bell...@linaro.org>> wrote: > I suppose it depends on how true we want to be to the specification and > whether our default is NS=0 or NS=1 when the security extension is present > or not. The code currently assumes non-secure as the default state. The v8 ARM ARM at least allows the CPU to behave as if only NS was present if there is no implementation of the Security extensions. I haven't checked the v7 wording. (In general I think QEMU's implementation of this should follow the v8 ARM ARM and treat v7 CPUs as a sort of special degenerate case.) > Is there a convention in qemu? How closely do we attempt to stay to the > pseudo code provided in the spec? The pseudocode in the ARM ARM is part of the spec. We should strive to follow the spec. This doesn't necessarily mean matching pseudocode functions exactly -- the requirement is to be behaviourally the same, and sometimes the pseudocode is written to be clear rather than efficient or to deal with situations we don't necessarily care about. thanks -- PMM
Re: [Qemu-devel] [PATCH v2 02/23] target-arm: move SCR into Security Extensions register list
On 14 May 2014, at 16:19, Greg Bellows
mailto:greg.bell...@linaro.org>> wrote:
On 13 May 2014 11:15, Fabian Aggeler
mailto:aggel...@ethz.ch>> wrote:
From: Sergey Fedorov mailto:s.fedo...@samsung.com>>
Define a new ARM CP register info list for the Security Extension feature.
Register that list only for ARM cores with Security Extension support.
Moving SCR into Security Extension register group.
Signed-off-by: Sergey Fedorov
mailto:s.fedo...@samsung.com>>
Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>>
---
target-arm/helper.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3be917c..7898f40 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -768,9 +768,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
.access = PL1_RW, .writefn = vbar_write,
.fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
.resetvalue = 0 },
-{ .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
- .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
- .resetvalue = 0, },
{ .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
.access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE },
@@ -2087,6 +2084,15 @@ static void sctlr_write(CPUARMState *env, const
ARMCPRegInfo *ri,
tlb_flush(CPU(cpu), 1);
}
+static const ARMCPRegInfo tz_cp_reginfo[] = {
Sticking with the feature name switch from TRUSTZONE to SECURITY, for
consistency we should call this security_cp_reginfo.
Makes sense. I will change it.
+#ifndef CONFIG_USER_ONLY
+{ .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
+ .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
+ .resetvalue = 0, },
+#endif
+REGINFO_SENTINEL
+};
+
static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
{
/* Only accessible in EL0 if SCTLR.UCT is set (and only in AArch64,
@@ -2364,6 +2370,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
if (arm_feature(env, ARM_FEATURE_LPAE)) {
define_arm_cp_regs(cpu, lpae_cp_reginfo);
}
+if (arm_feature(env, ARM_FEATURE_SECURITY_EXTENSIONS)) {
+define_arm_cp_regs(cpu, tz_cp_reginfo);
+}
/* Slightly awkwardly, the OMAP and StrongARM cores need all of
* cp15 crn=0 to be writes-ignored, whereas for other cores they should
* be read-only (ie write causes UNDEF exception).
--
1.8.3.2
Re: [Qemu-devel] [PATCH v1 00/22] target-arm: Preparations for A64 EL2 and 3
Hi Greg Thanks for your comments. I still have to work through them. I am using OpenVirtualization in secure world, which then switches to a Linux kernel in non-secure world to test the patches. What about you? Best, Fabian On 14 May 2014, at 15:55, Greg Bellows mailto:greg.bell...@linaro.org>> wrote: Hi Fabian, I too had been updating the core TZ patches provided by Samsung. From looking at your changes I see a lot of similarities in our code with the exception being the mechanism for banked register support. The difference being that your approach is a bit more explicit in the declaration of the banked registers. Whereas my approach was to update the banked registers once all the other registers were registered. Both approaches I believe work. I spoke with Peter M. and he and I are okay with your approach. I will be looking closer at your patches today and making comments. One thing that held me up from committing sooner was testing my changes. Do you have a good approach for testing the changes? Regards, Greg On 14 May 2014 03:58, Aggeler Fabian mailto:aggel...@student.ethz.ch>> wrote: I see. What is Greg Bellows working on exactly? Also peripherals like TZASC, TZPC,...? My plan is to focus on them now if no one else is working on them. What do you suggest to minimize overlap? Thanks, Fabian From: Peter Maydell [peter.mayd...@linaro.org<mailto:peter.mayd...@linaro.org>] Sent: Monday, May 12, 2014 10:39 PM To: Aggeler Fabian Cc: Edgar E. Iglesias; Rob Herring; Peter Crosthwaite; QEMU Developers; Alexander Graf; John Williams; Alex Bennée; Greg Bellows Subject: Re: [Qemu-devel] [PATCH v1 00/22] target-arm: Preparations for A64 EL2 and 3 On 12 May 2014 20:13, Aggeler Fabian mailto:aggel...@student.ethz.ch>> wrote: > I’ve been reworking the Samsung patches as part of my Master thesis and I > wanted to > send them some time this week. I am currently rebasing them when I noticed > Edgar’s > patches. Is there some branch with the patches so I could rebase on them? Hmm, that makes about three lots of people trying to do similar things at this point. We should try to coordinate so we don't duplicate work. thanks -- PMM
Re: [Qemu-devel] [PATCH v2 14/23] target-arm: add banked coprocessor register type and macros
On 14 May 2014, at 18:42, Greg Bellows
mailto:greg.bell...@linaro.org>> wrote:
On 13 May 2014 11:15, Fabian Aggeler
mailto:aggel...@ethz.ch>> wrote:
Banked CP registers can be defined with a A32_BANKED_REG macro which defines
a non-secure instance of the register followed by an adjacent secure instance.
Using a union makes the code backwards-compatible since the non-secure
instance can normally be accessed by its existing name.
This comment indicates that the 0th entry or the default name is the non-secure
bank, which differs from the code below.
The code does indeed use the 0th entry as non-secure and 1st entry as secure.
Using the union the default name points to the
0th entry. Am I missing something here?
When translating a banked CP register access instruction in monitor mode,
the SCR.NS bit determines which instance is going to be accessed.
If EL3 is operating in Aarch64 state coprocessor registers are not
banked anymore but in some cases have its own _EL3 instance.
Signed-off-by: Sergey Fedorov
mailto:s.fedo...@samsung.com>>
Signed-off-by: Fabian Aggeler mailto:aggel...@ethz.ch>>
---
target-arm/cpu.h | 121 +
target-arm/helper.c| 64 --
target-arm/translate.c | 19 +---
3 files changed, 184 insertions(+), 20 deletions(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index a970d55..9e325ac 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -80,6 +80,16 @@
#define offsetofhigh32(S, M) (offsetof(S, M) + sizeof(uint32_t))
#endif
+/* Define a banked coprocessor register state field. Use %name as the
+ * register field name for the secure instance. The non-secure instance
+ * has a "_nonsecure" suffix.
Where is the "_nonsecure" suffix?
The above comment appears to be incorrect as the code assumes that the 0th
entry as the non-secure bank.
That comment is wrong and still reflects an earlier implementation where I
didn’t yet use a union to be able to retain
the existing name. I will adjust it. Thanks for catching this.
+ */
+#define A32_BANKED_REG(type, name) \
+union { \
+type name; \
+type name##_banked[2]; \
+}
+
/* Meanings of the ARMCPU object's two inbound GPIO lines */
#define ARM_CPU_IRQ 0
#define ARM_CPU_FIQ 1
@@ -89,6 +99,7 @@ typedef void ARMWriteCPFunc(void *opaque, int cp_info,
typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
int dstreg, int operand);
+
struct arm_boot_info;
#define NB_MMU_MODES 5
@@ -673,6 +684,78 @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el)
return arm_feature(env, ARM_FEATURE_AARCH64);
}
+/* When EL3 is operating in Aarch32 state, the NS-bit determines
+ * whether the secure instance of a cp-register should be used. */
+#define USE_SECURE_REG(env) ( \
+arm_feature(env, ARM_FEATURE_SECURITY_EXTENSIONS) && \
+!arm_el_is_aa64(env, 3) && \
+!((env)->cp15.c1_scr & 1/*NS*/))
+
+#define NONSECURE_BANK 0
+#define SECURE_BANK 1
+
+#define A32_BANKED_REG_GET(env, regname) \
+((USE_SECURE_REG(env)) ? \
+(env)->cp15.regname##_banked[SECURE_BANK] : \
+(env)->cp15.regname)
+
+#define A32_MAPPED_EL3_REG_GET(env, regname) \
+(((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
+ (USE_SECURE_REG(env))) ? \
+(env)->cp15.regname##_el3 : \
+(env)->cp15.regname##_el1)
+
+#define A32_BANKED_REG_SET(env, regname, val) \
+do { \
+if (USE_SECURE_REG(env)) { \
+(env)->cp15.regname##_banked[SECURE_BANK] = (val); \
+} else { \
+(env)->cp15.regname = (val); \
+} \
+} while (0)
+
+#define A32_MAPPED_EL3_REG_SET(env, regname, val) \
+do { \
+if ((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
+(USE_SECURE_REG(env))) { \
+(env)->cp15.regname##_el3 = (val); \
+} else { \
+(env)->cp15.regname##_el1 = (val); \
+} \
+} while (0)
+
+
+#define A32_BANKED_CURRENT_REG_GET(env, regname) \
+((!arm_el_is_aa64(env, 3) && arm_is_secure(env)) ? \
+(env)->cp15.regname##_banked[SECURE_BANK] : \
+(env)->cp15.regname)
+
+#define A32_MAPPED_EL3_CURRENT_REG_GET(env, regname) \
+(((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
+ (!arm_el_is_aa64(env, 3) && arm_is_secure(env))) ? \
+(env)->cp15.regname##_el3 : \
+(env)->cp15.regname##_el1)
+
+#define A32_BANKED_CURRENT_REG_SET(env, regname, val) \
+do { \
+if (!arm_el_is_aa64(env, 3) && arm_is_secure(env)) { \
+(env)->cp15.regname##_banked[SECURE_BANK] = (val); \
+} else { \
+(env)->cp15.regname = (val); \
+} \
+} while (0)
+
+#define A32_MAPPED_EL3_CURRENT_REG_SET(env, regname, v
Re: [Qemu-devel] [PATCH v1 00/22] target-arm: Preparations for A64 EL2 and 3
I see. What is Greg Bellows working on exactly? Also peripherals like TZASC, TZPC,...? My plan is to focus on them now if no one else is working on them. What do you suggest to minimize overlap? Thanks, Fabian From: Peter Maydell [peter.mayd...@linaro.org] Sent: Monday, May 12, 2014 10:39 PM To: Aggeler Fabian Cc: Edgar E. Iglesias; Rob Herring; Peter Crosthwaite; QEMU Developers; Alexander Graf; John Williams; Alex Bennée; Greg Bellows Subject: Re: [Qemu-devel] [PATCH v1 00/22] target-arm: Preparations for A64 EL2 and 3 On 12 May 2014 20:13, Aggeler Fabian wrote: > I’ve been reworking the Samsung patches as part of my Master thesis and I > wanted to > send them some time this week. I am currently rebasing them when I noticed > Edgar’s > patches. Is there some branch with the patches so I could rebase on them? Hmm, that makes about three lots of people trying to do similar things at this point. We should try to coordinate so we don't duplicate work. thanks -- PMM
Re: [Qemu-devel] [PATCH v1 00/22] target-arm: Preparations for A64 EL2 and 3
Hi I’ve been reworking the Samsung patches as part of my Master thesis and I wanted to send them some time this week. I am currently rebasing them when I noticed Edgar’s patches. Is there some branch with the patches so I could rebase on them? Thanks, Fabian On 07 May 2014, at 05:46, Edgar E. Iglesias mailto:edgar.igles...@gmail.com>> wrote: On Tue, May 06, 2014 at 08:58:43AM +0100, Peter Maydell wrote: On 6 May 2014 07:08, Edgar E. Iglesias mailto:edgar.igles...@gmail.com>> wrote: From: "Edgar E. Iglesias" mailto:edgar.igles...@xilinx.com>> Hi, I've been doing some work on modeling parts of EL2 and 3 + some of the system-wide virtualization features for ARMv8. A lot is missing but I've got a series with enough to for example run KVM A64 guests on top of EL3 firmware inside emulated QEMU A64 VMs. I'm working on cleaning things up and plan to send patches and publish things as I go. So before I start reviewing this, how does it relate to the Samsung series for AArch32 trustzone (EL3) support that was posted last year? In Linaro we've been planning to rework that and integrate it upstream... Hi Peter, AFAICT the series have some minor overlap but mostly they complement each other. The aarch64 EL3 support I've got so far is very limited. Has mode switching, separate page tables, SMC etc and that kind of things but no S/NS state yet. The A64 security state parts can be implemented on top of the Samsung series. Cheers, Edgar
