Re: [Qemu-devel] [PATCH 10/20] nvic: Add NS alias SCS region
On Sun, 31 May 2020 at 16:58, Magdy, Mina wrote: > I’m currently going through this QEMU patch that related to adding the NS > alias for SCS region in ARMv8m cores. > Why did you assumed that secure accesses to the alias act like NS accesses to > the real region registers. I discovered the any register that is accessed > through its alias address behaves like accessing it in no secure state > (except some cases where current security state is also considered along with > the accessed region) ? I believe that this is the architecturally required behaviour. In the v8M Arm ARM (DDI0553B.j) rule R_CFPK says "The Secure view of the NS alias is identical to the Non-secure view of normal addresses unless otherwise stated". More generally, the reason that the NS aliases exist is so that secure software can read and write the NS versions of the registers. If secure software wants to be able to make changes to bits that are only writeable in the secure version of the register, it can do that already via the normal non-alias register. > My question was come up due to an issue while setting Application > Interrupt and Reset Control Register (AIRCR). BFHFNMINS , I tried > to set in secure state but through the alias register. But as > investigated from QEMU code, the current security state is set to > 0 (no secure) but the CPU state is still secure and wasn’t switched > at this point. This is what I would expect. The alias is provided so that Secure code can access the Non-secure version of the AIRCR register. In the Non-secure version of the AIRCR register, BFHFNMINS is read-only. Do you find that real hardware behaves differently to QEMU? thanks -- PMM
Re: [Qemu-devel] [PATCH 10/20] nvic: Add NS alias SCS region
On 5 September 2017 at 17:48, Richard Henderson wrote: > On 09/05/2017 09:26 AM, Peter Maydell wrote: >> We don't map the hole. The container is 0x21000 in size, the normal >> nvic_sysregs region is 0x1000 at offset 0x0 (which will be 0xe000e000 >> in the system address space), and the NS alias region >> is 0x1000 at offset 0x2 (0xe002e000 in the system address space). >> There's nothing mapped in the hole in the container, so accesses >> there will busfault, as they will for other PPB accesses before or >> after the SCSes. > > Ok, it's not wrong, but I still don't understand the need for the container. It lets us assemble all the parts of this bit of the address space -- SCS, systick (which sits on top of some of the SCS), NS SCS -- in the right place, rather than requiring any caller that wants to instantiate an NVIC to do it (and to check whether the cpu has the security extension to figure out whether the NS alias exists, and so on). Now that we've QOMified hw/arm/armv7m.c it's less important that the NVIC in particular does that, and it's partly historical legacy that most of this is done in the NVIC realize function rather than in armv7m_instance_init() (we used to implement systick directly in the NVIC source file rather than as its own device). It doesn't seem worth shuffling the code around now, though (the container already existed prior to this patch, we're just making it a bit bigger and adding another thing to it.) thanks -- PMM
Re: [Qemu-devel] [PATCH 10/20] nvic: Add NS alias SCS region
On 09/05/2017 09:26 AM, Peter Maydell wrote: > We don't map the hole. The container is 0x21000 in size, the normal > nvic_sysregs region is 0x1000 at offset 0x0 (which will be 0xe000e000 > in the system address space), and the NS alias region > is 0x1000 at offset 0x2 (0xe002e000 in the system address space). > There's nothing mapped in the hole in the container, so accesses > there will busfault, as they will for other PPB accesses before or > after the SCSes. Ok, it's not wrong, but I still don't understand the need for the container. r~
Re: [Qemu-devel] [PATCH 10/20] nvic: Add NS alias SCS region
On 29 August 2017 at 17:00, Richard Henderson
wrote:
> On 08/22/2017 08:08 AM, Peter Maydell wrote:
>> +regionlen = arm_feature(&s->cpu->env, ARM_FEATURE_V8) ? 0x21000 :
>> 0x1000;
>> +memory_region_init(&s->container, OBJECT(s), "nvic", regionlen);
>> /* The system register region goes at the bottom of the priority
>> * stack as it covers the whole page.
>> */
>> @@ -1185,6 +1242,13 @@ static void armv7m_nvic_realize(DeviceState *dev,
>> Error **errp)
>> sysbus_mmio_get_region(systick_sbd,
>> 0),
>> 1);
>>
>> +if (arm_feature(&s->cpu->env, ARM_FEATURE_V8)) {
>> +memory_region_init_io(&s->sysreg_ns_mem, OBJECT(s),
>> + &nvic_sysreg_ns_ops, s,
>> + "nvic_sysregs_ns", 0x1000);
>> +memory_region_add_subregion(&s->container, 0x2,
>> &s->sysreg_ns_mem);
>
> There's a whole in between the two regions, which you are leaving mapped. Why
> create a sub-region instead of two separate top-level regions for which you
> can
> leave the whole unmapped?
We don't map the hole. The container is 0x21000 in size, the normal
nvic_sysregs region is 0x1000 at offset 0x0 (which will be 0xe000e000
in the system address space), and the NS alias region
is 0x1000 at offset 0x2 (0xe002e000 in the system address space).
There's nothing mapped in the hole in the container, so accesses
there will busfault, as they will for other PPB accesses before or
after the SCSes.
thanks
-- PMM
Re: [Qemu-devel] [PATCH 10/20] nvic: Add NS alias SCS region
On 08/22/2017 08:08 AM, Peter Maydell wrote:
> +regionlen = arm_feature(&s->cpu->env, ARM_FEATURE_V8) ? 0x21000 : 0x1000;
> +memory_region_init(&s->container, OBJECT(s), "nvic", regionlen);
> /* The system register region goes at the bottom of the priority
> * stack as it covers the whole page.
> */
> @@ -1185,6 +1242,13 @@ static void armv7m_nvic_realize(DeviceState *dev,
> Error **errp)
> sysbus_mmio_get_region(systick_sbd,
> 0),
> 1);
>
> +if (arm_feature(&s->cpu->env, ARM_FEATURE_V8)) {
> +memory_region_init_io(&s->sysreg_ns_mem, OBJECT(s),
> + &nvic_sysreg_ns_ops, s,
> + "nvic_sysregs_ns", 0x1000);
> +memory_region_add_subregion(&s->container, 0x2,
> &s->sysreg_ns_mem);
There's a whole in between the two regions, which you are leaving mapped. Why
create a sub-region instead of two separate top-level regions for which you can
leave the whole unmapped?
r~
[Qemu-devel] [PATCH 10/20] nvic: Add NS alias SCS region
For v8M the range 0xe002e000..0xe002efff is an alias region which
for secure accesses behaves like a NonSecure access to the main
SCS region. (For nonsecure accesses including when the security
extension is not implemented, it is RAZ/WI.)
Signed-off-by: Peter Maydell
---
include/hw/intc/armv7m_nvic.h | 1 +
hw/intc/armv7m_nvic.c | 66 ++-
2 files changed, 66 insertions(+), 1 deletion(-)
diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h
index 1d145fb..1a4cce7 100644
--- a/include/hw/intc/armv7m_nvic.h
+++ b/include/hw/intc/armv7m_nvic.h
@@ -50,6 +50,7 @@ typedef struct NVICState {
int exception_prio; /* group prio of the highest prio active exception */
MemoryRegion sysregmem;
+MemoryRegion sysreg_ns_mem;
MemoryRegion container;
uint32_t num_irq;
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index babdc3b..2b0b328 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -1040,6 +1040,47 @@ static const MemoryRegionOps nvic_sysreg_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
+static MemTxResult nvic_sysreg_ns_write(void *opaque, hwaddr addr,
+uint64_t value, unsigned size,
+MemTxAttrs attrs)
+{
+if (attrs.secure) {
+/* S accesses to the alias act like NS accesses to the real region */
+attrs.secure = 0;
+return nvic_sysreg_write(opaque, addr, value, size, attrs);
+} else {
+/* NS attrs are RAZ/WI for privileged, and BusFault for user */
+if (attrs.user) {
+return MEMTX_ERROR;
+}
+return MEMTX_OK;
+}
+}
+
+static MemTxResult nvic_sysreg_ns_read(void *opaque, hwaddr addr,
+ uint64_t *data, unsigned size,
+ MemTxAttrs attrs)
+{
+if (attrs.secure) {
+/* S accesses to the alias act like NS accesses to the real region */
+attrs.secure = 0;
+return nvic_sysreg_read(opaque, addr, data, size, attrs);
+} else {
+/* NS attrs are RAZ/WI for privileged, and BusFault for user */
+if (attrs.user) {
+return MEMTX_ERROR;
+}
+*data = 0;
+return MEMTX_OK;
+}
+}
+
+static const MemoryRegionOps nvic_sysreg_ns_ops = {
+.read_with_attrs = nvic_sysreg_ns_read,
+.write_with_attrs = nvic_sysreg_ns_write,
+.endianness = DEVICE_NATIVE_ENDIAN,
+};
+
static int nvic_post_load(void *opaque, int version_id)
{
NVICState *s = opaque;
@@ -1141,6 +1182,7 @@ static void armv7m_nvic_realize(DeviceState *dev, Error
**errp)
NVICState *s = NVIC(dev);
SysBusDevice *systick_sbd;
Error *err = NULL;
+int regionlen;
s->cpu = ARM_CPU(qemu_get_cpu(0));
assert(s->cpu);
@@ -1173,8 +1215,23 @@ static void armv7m_nvic_realize(DeviceState *dev, Error
**errp)
* 0xd00..0xd3c - SCS registers
* 0xd40..0xeff - Reserved or Not implemented
* 0xf00 - STIR
+ *
+ * Some registers within this space are banked between security states.
+ * In v8M there is a second range 0xe002e000..0xe002efff which is the
+ * NonSecure alias SCS; secure accesses to this behave like NS accesses
+ * to the main SCS range, and non-secure accesses (including when
+ * the security extension is not implemented) are RAZ/WI.
+ * Note that both the main SCS range and the alias range are defined
+ * to be exempt from memory attribution (R_BLJT) and so the memory
+ * transaction attribute always matches the current CPU security
+ * state (attrs.secure == env->v7m.secure). In the nvic_sysreg_ns_ops
+ * wrappers we change attrs.secure to indicate the NS access; so
+ * generally code determining which banked register to use should
+ * use attrs.secure; code determining actual behaviour of the system
+ * should use env->v7m.secure.
*/
-memory_region_init(&s->container, OBJECT(s), "nvic", 0x1000);
+regionlen = arm_feature(&s->cpu->env, ARM_FEATURE_V8) ? 0x21000 : 0x1000;
+memory_region_init(&s->container, OBJECT(s), "nvic", regionlen);
/* The system register region goes at the bottom of the priority
* stack as it covers the whole page.
*/
@@ -1185,6 +1242,13 @@ static void armv7m_nvic_realize(DeviceState *dev, Error
**errp)
sysbus_mmio_get_region(systick_sbd, 0),
1);
+if (arm_feature(&s->cpu->env, ARM_FEATURE_V8)) {
+memory_region_init_io(&s->sysreg_ns_mem, OBJECT(s),
+ &nvic_sysreg_ns_ops, s,
+ "nvic_sysregs_ns", 0x1000);
+memory_region_add_subregion(&s->container, 0x2, &s->sysreg_ns_mem);
+}
+
sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container);
}
--
2.7.4
