Re: [PATCH v7 00/14] KVM: mm: fd-based approach for supporting KVM guest private memory
On Sat, 20 Aug 2022, Kirill A. Shutemov wrote: > > Yes, INACCESSIBLE is increase of complexity which you do not want to deal > with in shmem.c. It get it. It's not so much that INACCESSIBLE increases the complexity of memfd/shmem/tmpfs, as that it is completely foreign to it. And by handling all those foreign needs at the KVM end (where you can be sure that the mem attached to the fd is INACCESSIBLE because you have given nobody access to it - no handshaking with 3rd party required). > > I will try next week to rework it as shim to top of shmem. Does it work > for you? Yes, please do, thanks. It's a compromise between us: the initial TDX case has no justification to use shmem at all, but doing it that way will help you with some of the infrastructure, and will probably be easiest for KVM to extend to other more relaxed fd cases later. > > But I think it is wrong to throw it over the fence to KVM folks and say it > is your problem. Core MM has to manage it. We disagree on who is throwing over the fence to whom :) Core MM should manage the core MM parts and KVM should manage the KVM parts. What makes this rather different from most driver usage of MM, is that KVM seems likely to use a great proportion of memory this way. With great memory usage comes great responsibility: I don't think all those flags and seals and notifiers let KVM escape from that. Hugh
Re: Teensy 4.1 Implementation
Hello! Thank you all again for your initial guidance. I've been able to write an initial Teensy 4.1 machine, for now with only the few important memory regions initialized, and successfully ran some hand-written ARM code on it. I was wondering what your opinions might be for implementing the next step, which in my opinion should be bootstrapping a "kernel" in the format the board actually expects. This is however the current roadblock I am hitting. The documentation ( https://www.pjrc.com/teensy/IMXRT1060RM_rev3.pdf ), in section 9.7.1, gives some informations on how, in the actual Teensy board, the ROM, executed at boot, initialize the board peripherals, and also reads from a data structure included in the Flash memory (the user-provided program) where the CPU should jump to after the ROM has done its work (somewhere in that same Flash memory, usually). I was able to successfully dump the ROM of the real board and confirm this behavior. Given that the current plan is not to emulate every peripherals, I am of the opinion that writing a very simple ROM that merely reads this Flash provided data structure and jumps to the provided address sounds like a good starting point, so that I can keep iterating on writing more and more complex code through the provided Teensy toolchain, and implementing needed peripherals. As such, I have several questions: 1/ To replicate this behaviour, is this considered the correct approach by the current QEMU maintainers? 2/ If so, I have not been able to find any function that would be able to load data into a memory region "statically". Does one exist? Is there an alternative to this process? 3/ Regarding loading the "kernel" of the board, as part of the init process, I am calling the usual "armv7m_load_kernel" function with its usual parameters. However, it seems to load it as the very start of the address space, which is not where the flash memory is, and so is not where the kernel should be loaded. I wasn't able to find a workaround. Is there something I'm missing? Sorry to bother you with so many questions. Thanks again, Saana Le mar. 16 août 2022 à 12:06, Peter Maydell a écrit : > On Tue, 16 Aug 2022 at 10:59, Alex Bennée wrote: > > Shiny Saana writes: > > > I personally don't need any of the GPIO interfaces, but if needed > > > by someone else, that could be a good second step to > > > work on once that part of the board is implemented. > > > > Handling GPIOs in QEMU is fine (we have things like the qdev_init_gpio_* > > functions to handle them). The problem is usually what to do with the > > actual general purpose pins which aren't wired to something we emulate > > in the board. Some boards expose their values via QMP properties but I > > suspect whats really needed is a generic mechanism for exposing GPIO to > > external scripts rather than have every board define it's own thing. > > Yes. However one key thing for trying to get a new board model > in is not to get tangled up in trying to improve/extend > the core QEMU facilities for something. That's much harder > than "my board model supports GPIO output lines to the same > extent as the other existing board models" :-) > > thanks > -- PMM >
[PATCH 0/1] hw/i2c/aspeed: Fix old reg slave receive
Hey everyone, I haven't gotten a chance to work on the Aspeed I2C controller in a little while, but I finally started looking at it again and noticed the old-register mode slave receive function (master-send-to-slave) does the wrong thing for the first byte. See the commit message for details. I noticed this because I have a qtest for slave mode rx in old-register mode downstream [1] (I'm also working on a version of the test that can be upstreamed) that broke when I updated our QEMU branch to the 7.1 release. Previously I was using Klaus's original slave I2C patches from [2]. An example of the test running successfully with this change is pasted below, for whatever that's worth. Thanks, Peter [1]: https://github.com/facebook/openbmc/blob/helium/common/recipes-devtools/qemu/qemu/0008-hw-misc-Add-byte-by-byte-i2c-network-device.patch [2]: https://lore.kernel.org/qemu-devel/20220331165737.1073520-4-...@irrelevant.dk/ # random seed: R02S5d2728d1347dc8b50533a0d85ebb1b02 # starting QEMU: exec build/qemu-system-arm -qtest unix:/tmp/qtest-711521.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-711521.qmp,id=char0 -mon chardev=char0,mode=control -display none -machine fby35-bmc -netdev socket,id=socket0,udp=127.0.0.1:5000,localaddr=127.0.0.1:6000 -device i2c-netdev2,bus=aspeed.i2c.bus.0,address=0x32,netdev=socket0 -accel qtest i2c_netdev2_class_init i2c_netdev2_realize i2c_netdev2_can_receive 1..2 # Start of arm tests # Start of ast2600 tests # Start of i2c tests i2c_netdev2_handle_event: 1 ../hw/misc/i2c-netdev2.c: tx [64, 00, 00] ../hw/misc/i2c-netdev2.c: tx [de] ../hw/misc/i2c-netdev2.c: tx [ad] ../hw/misc/i2c-netdev2.c: tx [be] ../hw/misc/i2c-netdev2.c: tx [ef] i2c_netdev2_handle_event: 3 ../hw/misc/i2c-netdev2.c: tx [00, 00, 00, 00] i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [01, 00] i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [01, 00] i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [01, 00] i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [01, 00] i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [01, 00] ok 1 /arm/ast2600/i2c/write_in_old_byte_mode i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [20, 00, 00] prev rx_buf: [00, 00, 00, 00, 00, 00, 00, 00, 00, 00] next rx_buf: [20, 00, 00, 00, 00, 00, 00, 00, 00, 00] i2c_netdev2_slave_mode_rx: rx_len=3 i2c_netdev2_slave_mode_rx: rx_len=3 i2c_netdev2_slave_mode_rx: guest OS ack rx, clearing rx_len i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [de] prev rx_buf: [20, 00, 00, 00, 00, 00, 00, 00, 00, 00] next rx_buf: [de, 00, 00, 00, 00, 00, 00, 00, 00, 00] i2c_netdev2_slave_mode_rx: rx_len=1 i2c_netdev2_slave_mode_rx: rx_len=1 i2c_netdev2_slave_mode_rx: guest OS ack rx, clearing rx_len i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [ad] prev rx_buf: [de, 00, 00, 00, 00, 00, 00, 00, 00, 00] next rx_buf: [ad, 00, 00, 00, 00, 00, 00, 00, 00, 00] i2c_netdev2_slave_mode_rx: rx_len=1 i2c_netdev2_slave_mode_rx: rx_len=1 i2c_netdev2_slave_mode_rx: guest OS ack rx, clearing rx_len i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [be] prev rx_buf: [ad, 00, 00, 00, 00, 00, 00, 00, 00, 00] next rx_buf: [be, 00, 00, 00, 00, 00, 00, 00, 00, 00] i2c_netdev2_slave_mode_rx: rx_len=1 i2c_netdev2_slave_mode_rx: rx_len=1 i2c_netdev2_slave_mode_rx: guest OS ack rx, clearing rx_len i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [ef] prev rx_buf: [be, 00, 00, 00, 00, 00, 00, 00, 00, 00] next rx_buf: [ef, 00, 00, 00, 00, 00, 00, 00, 00, 00] i2c_netdev2_slave_mode_rx: rx_len=1 i2c_netdev2_slave_mode_rx: rx_len=1 i2c_netdev2_slave_mode_rx: guest OS ack rx, clearing rx_len i2c_netdev2_can_receive ../hw/misc/i2c-netdev2.c: rx [ef, 00, 00, 00] prev rx_buf: [ef, 00, 00, 00, 00, 00, 00, 00, 00, 00] next rx_buf: [ef, 00, 00, 00, 00, 00, 00, 00, 00, 00] i2c_netdev2_slave_mode_rx: rx_len=4 ok 2 /arm/ast2600/i2c/slave_mode_rx_byte_buf # End of i2c tests # End of ast2600 tests # End of arm tests i2c_netdev2_nic_cleanup Peter Delevoryas (1): hw/i2c/aspeed: Fix old reg slave receive hw/i2c/aspeed_i2c.c | 8 +--- include/hw/i2c/aspeed_i2c.h | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) -- 2.37.1
[PATCH 1/1] hw/i2c/aspeed: Fix old reg slave receive
I think when Klaus ported his slave mode changes from the original patch
series to the rewritten I2C module, he changed the behavior of the first
byte that is received by the slave device.
What's supposed to happen is that the AspeedI2CBus's slave device's
i2c_event callback should run, and if the event is "send_async", then it
should populate the byte buffer with the 8-bit I2C address that is being
sent to. Since we only support "send_async", the lowest bit should
always be 0 (indicating that the master is requesting to send data).
This is the code Klaus had previously, for reference. [1]
switch (event) {
case I2C_START_SEND:
bus->buf = bus->dev_addr << 1;
bus->buf &= I2CD_BYTE_BUF_RX_MASK;
bus->buf <<= I2CD_BYTE_BUF_RX_SHIFT;
bus->intr_status |= (I2CD_INTR_SLAVE_ADDR_RX_MATCH | I2CD_INTR_RX_DONE);
aspeed_i2c_set_state(bus, I2CD_STXD);
break;
[1]:
https://lore.kernel.org/qemu-devel/20220331165737.1073520-4-...@irrelevant.dk/
Signed-off-by: Peter Delevoryas
Fixes: a8d48f59cd021b25 ("hw/i2c/aspeed: add slave device in old register mode")
---
hw/i2c/aspeed_i2c.c | 8 +---
include/hw/i2c/aspeed_i2c.h | 1 +
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 42c6d69b82..c166fd20fa 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -1131,7 +1131,9 @@ static int aspeed_i2c_bus_slave_event(I2CSlave *slave,
enum i2c_event event)
AspeedI2CBus *bus = ASPEED_I2C_BUS(qbus->parent);
uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
-uint32_t value;
+uint32_t reg_dev_addr = aspeed_i2c_bus_dev_addr_offset(bus);
+uint32_t dev_addr = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_dev_addr,
+SLAVE_DEV_ADDR1);
if (aspeed_i2c_is_new_mode(bus->controller)) {
return aspeed_i2c_bus_new_slave_event(bus, event);
@@ -1139,8 +1141,8 @@ static int aspeed_i2c_bus_slave_event(I2CSlave *slave,
enum i2c_event event)
switch (event) {
case I2C_START_SEND_ASYNC:
-value = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_byte_buf, TX_BUF);
-SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf, RX_BUF, value << 1);
+/* Bit[0] == 0 indicates "send". */
+SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf, RX_BUF, dev_addr <<
1);
ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, SLAVE_ADDR_RX_MATCH, 1);
SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, RX_DONE, 1);
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 300a89b343..adc904d6c1 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -130,6 +130,7 @@ REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
SHARED_FIELD(M_TX_CMD, 1, 1)
SHARED_FIELD(M_START_CMD, 0, 1)
REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
+SHARED_FIELD(SLAVE_DEV_ADDR1, 0, 7)
REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
SHARED_FIELD(RX_COUNT, 24, 5)
SHARED_FIELD(RX_SIZE, 16, 5)
--
2.37.1
Re: [PATCH 10/10] target/arm: Report FEAT_PMUv3p5 for TCG '-cpu max'
On 8/11/22 10:26, Peter Maydell wrote: On Thu, 11 Aug 2022 at 18:16, Peter Maydell wrote: Update the ID registers for TCG's '-cpu max' to report a FEAT_PMUv3p5 compliant PMU. Signed-off-by: Peter Maydell Oops, forgot the docs update: --- a/docs/system/arm/emulation.rst +++ b/docs/system/arm/emulation.rst @@ -52,6 +52,7 @@ the following architecture extensions: - FEAT_PMULL (PMULL, PMULL2 instructions) - FEAT_PMUv3p1 (PMU Extensions v3.1) - FEAT_PMUv3p4 (PMU Extensions v3.4) +- FEAT_PMUv3p5 (PMU Extensions v3.5) - FEAT_RAS (Reliability, availability, and serviceability) - FEAT_RASv1p1 (RAS Extension v1.1) - FEAT_RDM (Advanced SIMD rounding double multiply accumulate instructions) Reviewed-by: Richard Henderson r~
Re: [PATCH 09/10] target/arm: Support 64-bit event counters for FEAT_PMUv3p5
On 8/11/22 10:16, Peter Maydell wrote:
+static bool pmevcntr_is_64_bit(CPUARMState *env, int counter)
+{
+/* Return true if the specified event counter is configured to be 64 bit */
+
+/* This isn't intended to be used with the cycle counter */
+assert(counter < 31);
+
+if (!cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
+return false;
+}
+
+if (arm_feature(env, ARM_FEATURE_EL2)) {
+/*
+ * MDCR_EL2.HLP still applies even when EL2 is disabled in the
+ * current security state, so we don't use arm_mdcr_el2_eff() here.
+ */
+bool hlp = env->cp15.mdcr_el2 & MDCR_HLP;
+int hpmn = env->cp15.mdcr_el2 & MDCR_HPMN;
The specs could be improved here, as the top of MDCR_EL2 says it doesn't apply if EL2
isn't enabled in the security state, HLP has the exception noted above, but HPMN does not.
I conclude that HPMN is missing the exception, because nothing else makes sense.
Reviewed-by: Richard Henderson
r~
Re: [PATCH 08/10] target/arm: Implement FEAT_PMUv3p5 cycle counter disable bits
On 8/11/22 10:16, Peter Maydell wrote:
FEAT_PMUv3p5 introduces new bits MDCR_EL2.HCCD and MDCR_EL3.SCCD,
which disable the cycle counter from counting at EL2 and EL3.
Add the code to support these bits.
While HCCD is v3p5, it seems MCCD (typo above) is v3p7.
+if (counter == 31) {
+/*
+ * The cycle counter defaults to running. PMCR.DP says "disable
+ * the cycle counter when event counting is prohibited".
+ * Some MDCR bits disable the cycle counter specifically.
+ */
+prohibited = prohibited && env->cp15.c9_pmcr & PMCRDP;
+if (cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
+if (el == 3) {
+prohibited = prohibited || (env->cp15.mdcr_el3 & MDCR_MCCD);
+} else if (el == 2) {
+prohibited = prohibited || (mdcr_el2 & MDCR_HCCD);
+}
But modulo the feature test, the behaviour looks right.
r~
[PATCH v3 8/9] target/arm: Add PMSAv8r functionality
From: Tobias Röhmel
Add PMSAv8r translation.
Signed-off-by: Tobias Röhmel
---
target/arm/ptw.c | 171 +--
1 file changed, 150 insertions(+), 21 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index c4f5721012..c7e37c66d0 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -140,6 +140,9 @@ static bool regime_translation_disabled(CPUARMState *env,
ARMMMUIdx mmu_idx)
*/
return true;
}
+} else if (arm_feature(env, ARM_FEATURE_V8_R)) {
+return !(regime_sctlr(env, mmu_idx) & SCTLR_M) ||
+(!(regime_el(env, mmu_idx) == 2) && arm_hcr_el2_eff(env) & HCR_TGE);
}
hcr_el2 = arm_hcr_el2_eff(env);
@@ -1504,6 +1507,8 @@ static bool pmsav7_use_background_region(ARMCPU *cpu,
ARMMMUIdx mmu_idx,
if (arm_feature(env, ARM_FEATURE_M)) {
return env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)]
& R_V7M_MPU_CTRL_PRIVDEFENA_MASK;
+} else if (arm_feature(env, ARM_FEATURE_V8_R)) {
+return false;
} else {
return regime_sctlr(env, mmu_idx) & SCTLR_BR;
}
@@ -1698,6 +1703,77 @@ static bool get_phys_addr_pmsav7(CPUARMState *env,
uint32_t address,
return !(*prot & (1 << access_type));
}
+static uint32_t *regime_rbar(CPUARMState *env, ARMMMUIdx mmu_idx,
+ uint32_t secure)
+{
+if (arm_feature(env, ARM_FEATURE_V8_R)) {
+if (regime_el(env, mmu_idx) == 2) {
+return env->pmsav8.hprbarn;
+} else {
+return env->pmsav8.prbarn;
+}
+} else {
+ return env->pmsav8.rbar[secure];
+}
+}
+
+static uint32_t *regime_rlar(CPUARMState *env, ARMMMUIdx mmu_idx,
+ uint32_t secure)
+{
+if (arm_feature(env, ARM_FEATURE_V8_R)) {
+if (regime_el(env, mmu_idx) == 2) {
+return env->pmsav8.hprlarn;
+} else {
+return env->pmsav8.prlarn;
+}
+} else {
+return env->pmsav8.rlar[secure];
+}
+}
+
+static inline void get_phys_addr_pmsav8_default(CPUARMState *env,
+ARMMMUIdx mmu_idx,
+uint32_t address, int *prot)
+{
+if (arm_feature(env, ARM_FEATURE_V8_R)) {
+*prot = PAGE_READ | PAGE_WRITE;
+if (address <= 0x7FFF) {
+*prot |= PAGE_EXEC;
+}
+if ((regime_el(env, mmu_idx) == 2)
+&& (regime_sctlr(env, mmu_idx) & SCTLR_WXN)
+&& (regime_sctlr(env, mmu_idx) & SCTLR_M)) {
+*prot &= ~PAGE_EXEC;
+}
+} else {
+get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
+}
+}
+
+static bool pmsav8_fault(bool hit, CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+if (arm_feature(env, ARM_FEATURE_V8_R)) {
+if (regime_el(env, mmu_idx) == 2) {
+if (!hit && (mmu_idx != ARMMMUIdx_E2)) {
+return true;
+} else if (!hit && (mmu_idx == ARMMMUIdx_E2)
+ &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
+return true;
+}
+} else {
+if (!hit && (mmu_idx != ARMMMUIdx_Stage1_E1)) {
+return true;
+} else if (!hit && (mmu_idx == ARMMMUIdx_Stage1_E1)
+ &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
+return true;
+}
+}
+return false;
+} else {
+return !hit;
+}
+}
+
bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
MMUAccessType access_type, ARMMMUIdx mmu_idx,
hwaddr *phys_ptr, MemTxAttrs *txattrs,
@@ -1730,6 +1806,12 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t
address,
*mregion = -1;
}
+if (arm_feature(env, ARM_FEATURE_V8_R)) {
+if (mmu_idx == ARMMMUIdx_Stage2) {
+fi->stage2 = true;
+}
+}
+
/*
* Unlike the ARM ARM pseudocode, we don't need to check whether this
* was an exception vector read from the vector table (which is always
@@ -1746,17 +1828,26 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t
address,
hit = true;
}
+uint32_t bitmask;
+if (arm_feature(env, ARM_FEATURE_V8_R)) {
+bitmask = 0x3f;
+} else {
+bitmask = 0x1f;
+}
+
+
for (n = (int)cpu->pmsav7_dregion - 1; n >= 0; n--) {
/* region search */
/*
- * Note that the base address is bits [31:5] from the register
- * with bits [4:0] all zeroes, but the limit address is bits
- * [31:5] from the register with bits [4:0] all ones.
+ * Note that the base address is bits [31:x] from the register
+ * with bits [x-1:0] all zeroes, but the limit address is bits
+ * [31:x] from the register with bits [x:0] all ones. Where x is
+
[PATCH v3 5/9] target/arm: Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup
From: Tobias Röhmel
Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup to prepare
for the Cortex-R52 MPU which uses and combines cache attributes
of different translation levels.
Signed-off-by: Tobias Röhmel
---
target/arm/internals.h | 13 +++--
target/arm/m_helper.c | 3 ++-
target/arm/ptw.c | 11 +++
3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 6f94f3019d..b03049d920 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1109,12 +1109,6 @@ void v8m_security_lookup(CPUARMState *env, uint32_t
address,
MMUAccessType access_type, ARMMMUIdx mmu_idx,
V8M_SAttributes *sattrs);
-bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
- MMUAccessType access_type, ARMMMUIdx mmu_idx,
- hwaddr *phys_ptr, MemTxAttrs *txattrs,
- int *prot, bool *is_subpage,
- ARMMMUFaultInfo *fi, uint32_t *mregion);
-
/* Cacheability and shareability attributes for a memory access */
typedef struct ARMCacheAttrs {
/*
@@ -1126,6 +1120,13 @@ typedef struct ARMCacheAttrs {
bool is_s2_format:1;
} ARMCacheAttrs;
+bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
+ MMUAccessType access_type, ARMMMUIdx mmu_idx,
+ hwaddr *phys_ptr, MemTxAttrs *txattrs,
+ int *prot, bool *is_subpage,
+ ARMMMUFaultInfo *fi, uint32_t *mregion,
+ ARMCacheAttrs *cacheattrs);
+
bool get_phys_addr(CPUARMState *env, target_ulong address,
MMUAccessType access_type, ARMMMUIdx mmu_idx,
hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index a740c3e160..44c80d733a 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2829,10 +2829,11 @@ uint32_t HELPER(v7m_tt)(CPUARMState *env, uint32_t
addr, uint32_t op)
* inspecting the other MPU state.
*/
if (arm_current_el(env) != 0 || alt) {
+ARMCacheAttrs cacheattrs = {0};
/* We can ignore the return value as prot is always set */
pmsav8_mpu_lookup(env, addr, MMU_DATA_LOAD, mmu_idx,
_addr, , , _subpage,
- , );
+ , , );
if (mregion == -1) {
mrvalid = false;
mregion = 0;
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 8b037c1f55..c4f5721012 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1702,7 +1702,8 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
MMUAccessType access_type, ARMMMUIdx mmu_idx,
hwaddr *phys_ptr, MemTxAttrs *txattrs,
int *prot, bool *is_subpage,
- ARMMMUFaultInfo *fi, uint32_t *mregion)
+ ARMMMUFaultInfo *fi, uint32_t *mregion,
+ ARMCacheAttrs *cacheattrs)
{
/*
* Perform a PMSAv8 MPU lookup (without also doing the SAU check
@@ -1968,7 +1969,7 @@ static bool get_phys_addr_pmsav8(CPUARMState *env,
uint32_t address,
MMUAccessType access_type, ARMMMUIdx mmu_idx,
hwaddr *phys_ptr, MemTxAttrs *txattrs,
int *prot, target_ulong *page_size,
- ARMMMUFaultInfo *fi)
+ ARMMMUFaultInfo *fi, ARMCacheAttrs
*cacheattrs)
{
uint32_t secure = regime_is_secure(env, mmu_idx);
V8M_SAttributes sattrs = {};
@@ -2036,7 +2037,8 @@ static bool get_phys_addr_pmsav8(CPUARMState *env,
uint32_t address,
}
ret = pmsav8_mpu_lookup(env, address, access_type, mmu_idx, phys_ptr,
-txattrs, prot, _is_subpage, fi, NULL);
+txattrs, prot, _is_subpage, fi,
+NULL, cacheattrs);
*page_size = sattrs.subpage || mpu_is_subpage ? 1 : TARGET_PAGE_SIZE;
return ret;
}
@@ -2416,7 +2418,8 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
if (arm_feature(env, ARM_FEATURE_V8)) {
/* PMSAv8 */
ret = get_phys_addr_pmsav8(env, address, access_type, mmu_idx,
- phys_ptr, attrs, prot, page_size, fi);
+ phys_ptr, attrs, prot, page_size,
+ fi, cacheattrs);
} else if (arm_feature(env, ARM_FEATURE_V7)) {
/* PMSAv7 */
ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
--
2.25.1
[PATCH v3 1/9] target/arm: Add ARM_FEATURE_V8_R
From: Tobias Röhmel
This flag is necessary to add features for the Cortex-R52.
Signed-off-by: Tobias Röhmel
---
target/arm/cpu.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index df677b2d5d..86e06116a9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2287,6 +2287,7 @@ enum arm_features {
ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
ARM_FEATURE_M_MAIN, /* M profile Main Extension */
ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
+ARM_FEATURE_V8_R,
};
static inline int arm_feature(CPUARMState *env, int feature)
--
2.25.1
[PATCH v3 2/9] target/arm: Don't add all MIDR aliases for cores that immplement PMSA
From: Tobias Röhmel
Cores with PMSA have the MPUIR register which has the
same encoding as the MIDR alias with opc2=4. So we only
add that alias if we are not realizing a core that
implements PMSA.
Signed-off-by: Tobias Röhmel
---
target/arm/helper.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6457e6301c..b9547594ae 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8188,10 +8188,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
.access = PL1_R, .type = ARM_CP_NO_RAW, .resetvalue = cpu->midr,
.fieldoffset = offsetof(CPUARMState, cp15.c0_cpuid),
.readfn = midr_read },
-/* crn = 0 op1 = 0 crm = 0 op2 = 4,7 : AArch32 aliases of MIDR */
-{ .name = "MIDR", .type = ARM_CP_ALIAS | ARM_CP_CONST,
- .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
- .access = PL1_R, .resetvalue = cpu->midr },
+/* crn = 0 op1 = 0 crm = 0 op2 = 7 : AArch32 aliases of MIDR */
{ .name = "MIDR", .type = ARM_CP_ALIAS | ARM_CP_CONST,
.cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 7,
.access = PL1_R, .resetvalue = cpu->midr },
@@ -8201,6 +8198,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
.accessfn = access_aa64_tid1,
.type = ARM_CP_CONST, .resetvalue = cpu->revidr },
};
+ARMCPRegInfo id_v8_midr_alias_cp_reginfo = {
+ .name = "MIDR", .type = ARM_CP_ALIAS | ARM_CP_CONST,
+ .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
+ .access = PL1_R, .resetvalue = cpu->midr
+};
ARMCPRegInfo id_cp_reginfo[] = {
/* These are common to v8 and pre-v8 */
{ .name = "CTR",
@@ -8264,8 +8266,12 @@ void register_cp_regs_for_features(ARMCPU *cpu)
id_mpuir_reginfo.access = PL1_RW;
id_tlbtr_reginfo.access = PL1_RW;
}
+
if (arm_feature(env, ARM_FEATURE_V8)) {
define_arm_cp_regs(cpu, id_v8_midr_cp_reginfo);
+if (!arm_feature(env, ARM_FEATURE_PMSA)) {
+define_one_arm_cp_reg(cpu, _v8_midr_alias_cp_reginfo);
+}
} else {
define_arm_cp_regs(cpu, id_pre_v8_midr_cp_reginfo);
}
--
2.25.1
[PATCH v3 9/9] target/arm: Add ARM Cortex-R52 cpu
From: Tobias Röhmel
All constants are taken from the ARM Cortex-R52 Processor TRM Revision: r1p3
Signed-off-by: Tobias Röhmel
---
target/arm/cpu_tcg.c | 42 ++
1 file changed, 42 insertions(+)
diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
index b751a19c8a..e0f445dc91 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -843,6 +843,47 @@ static void cortex_r5_initfn(Object *obj)
define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
}
+static void cortex_r52_initfn(Object *obj)
+{
+ARMCPU *cpu = ARM_CPU(obj);
+
+set_feature(>env, ARM_FEATURE_V8);
+set_feature(>env, ARM_FEATURE_V8_R);
+set_feature(>env, ARM_FEATURE_EL2);
+set_feature(>env, ARM_FEATURE_PMSA);
+set_feature(>env, ARM_FEATURE_NEON);
+set_feature(>env, ARM_FEATURE_GENERIC_TIMER);
+cpu->midr = 0x411fd133; /* r1p3 */
+cpu->revidr = 0x;
+cpu->reset_fpsid = 0x41034023;
+cpu->isar.mvfr0 = 0x10110222;
+cpu->isar.mvfr1 = 0x1211;
+cpu->isar.mvfr2 = 0x0043;
+cpu->ctr = 0x8144c004;
+cpu->reset_sctlr = 0x30c50838;
+cpu->isar.id_pfr0 = 0x0131;
+cpu->isar.id_pfr1 = 0x10111001;
+cpu->isar.id_dfr0 = 0x03010006;
+cpu->id_afr0 = 0x;
+cpu->isar.id_mmfr0 = 0x00211040;
+cpu->isar.id_mmfr1 = 0x4000;
+cpu->isar.id_mmfr2 = 0x0120;
+cpu->isar.id_mmfr3 = 0xf0102211;
+cpu->isar.id_mmfr4 = 0x0010;
+cpu->isar.id_isar0 = 0x02101110;
+cpu->isar.id_isar1 = 0x13112111;
+cpu->isar.id_isar2 = 0x21232142;
+cpu->isar.id_isar3 = 0x01112131;
+cpu->isar.id_isar4 = 0x00010142;
+cpu->isar.id_isar5 = 0x00010001;
+cpu->isar.dbgdidr = 0x77168000;
+cpu->clidr = (1 << 27) | (1 << 24) | 0x3;
+cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
+cpu->ccsidr[1] = 0x201fe00a; /* 32KB L1 icache */
+
+cpu->pmsav7_dregion = 16;
+}
+
static void cortex_r5f_initfn(Object *obj)
{
ARMCPU *cpu = ARM_CPU(obj);
@@ -1149,6 +1190,7 @@ static const ARMCPUInfo arm_tcg_cpus[] = {
.class_init = arm_v7m_class_init },
{ .name = "cortex-r5", .initfn = cortex_r5_initfn },
{ .name = "cortex-r5f", .initfn = cortex_r5f_initfn },
+{ .name = "cortex-r52", .initfn = cortex_r52_initfn },
{ .name = "ti925t", .initfn = ti925t_initfn },
{ .name = "sa1100", .initfn = sa1100_initfn },
{ .name = "sa1110", .initfn = sa1110_initfn },
--
2.25.1
[PATCH v3 7/9] target/arm: Add PMSAv8r registers
From: Tobias Röhmel
Signed-off-by: Tobias Röhmel
---
target/arm/cpu.h| 10 +++
target/arm/helper.c | 171
2 files changed, 181 insertions(+)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 86e06116a9..632d0d13c6 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -726,8 +726,18 @@ typedef struct CPUArchState {
*/
uint32_t *rbar[M_REG_NUM_BANKS];
uint32_t *rlar[M_REG_NUM_BANKS];
+uint32_t prbarn[255];
+uint32_t prlarn[255];
+uint32_t hprbarn[255];
+uint32_t hprlarn[255];
uint32_t mair0[M_REG_NUM_BANKS];
uint32_t mair1[M_REG_NUM_BANKS];
+uint32_t prbar;
+uint32_t prlar;
+uint32_t prselr;
+uint32_t hprbar;
+uint32_t hprlar;
+uint32_t hprselr;
} pmsav8;
/* v8M SAU */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 23461397e0..1730383f28 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7422,6 +7422,78 @@ static CPAccessResult access_joscr_jmcr(CPUARMState *env,
return CP_ACCESS_OK;
}
+static void prbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+env->pmsav8.prbarn[env->pmsav8.prselr] = value;
+}
+
+static void prlar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+env->pmsav8.prlarn[env->pmsav8.prselr] = value;
+}
+
+static uint64_t prbar_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+return env->pmsav8.prbarn[env->pmsav8.prselr];
+}
+
+static uint64_t prlar_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+return env->pmsav8.prlarn[env->pmsav8.prselr];
+}
+
+static void hprbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+env->pmsav8.hprbarn[env->pmsav8.hprselr] = value;
+}
+
+static void hprlar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+env->pmsav8.hprlarn[env->pmsav8.hprselr] = value;
+}
+
+static void hprenr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+uint32_t n;
+ARMCPU *cpu = env_archcpu(env);
+for (n = 0; n < (int)cpu->pmsav7_dregion; ++n) {
+if (value & (1 << n)) {
+env->pmsav8.hprlarn[n] |= 0x1;
+} else {
+env->pmsav8.hprlarn[n] &= (~0x1);
+}
+}
+}
+
+static uint64_t hprbar_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+return env->pmsav8.hprbarn[env->pmsav8.hprselr];
+}
+
+static uint64_t hprlar_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+return env->pmsav8.hprlarn[env->pmsav8.hprselr];
+}
+
+static uint64_t hprenr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+uint32_t n;
+uint32_t result = 0x0;
+ARMCPU *cpu = env_archcpu(env);
+
+for (n = 0; n < (int)cpu->pmsav7_dregion; ++n) {
+if (env->pmsav8.hprlarn[n] & 0x1) {
+result |= (0x1 << n);
+}
+}
+return result;
+}
+
static const ARMCPRegInfo jazelle_regs[] = {
{ .name = "JIDR",
.cp = 14, .crn = 0, .crm = 0, .opc1 = 7, .opc2 = 0,
@@ -8249,6 +8321,46 @@ void register_cp_regs_for_features(ARMCPU *cpu)
.access = PL1_R, .type = ARM_CP_CONST,
.resetvalue = cpu->pmsav7_dregion << 8
};
+/* PMSAv8-R registers*/
+ARMCPRegInfo id_pmsav8_r_reginfo[] = {
+{ .name = "HMPUIR",
+ .cp = 15, .crn = 0, .crm = 0, .opc1 = 4, .opc2 = 4,
+ .access = PL2_R, .type = ARM_CP_CONST,
+ .resetvalue = cpu->pmsav7_dregion},
+ /* PMSAv8-R registers */
+{ .name = "PRBAR",
+ .cp = 15, .opc1 = 0, .crn = 6, .crm = 3, .opc2 = 0,
+ .access = PL1_RW, .resetvalue = 0,
+ .readfn = prbar_read, .writefn = prbar_write,
+ .fieldoffset = offsetof(CPUARMState, pmsav8.prbar)},
+{ .name = "PRLAR",
+ .cp = 15, .opc1 = 0, .crn = 6, .crm = 3, .opc2 = 1,
+ .access = PL1_RW, .resetvalue = 0,
+ .readfn = prlar_read, .writefn = prlar_write,
+ .fieldoffset = offsetof(CPUARMState, pmsav8.prlar)},
+{ .name = "PRSELR", .resetvalue = 0,
+ .cp = 15, .opc1 = 0, .crn = 6, .crm = 2, .opc2 = 1,
+ .access = PL1_RW, .accessfn = access_tvm_trvm,
+ .fieldoffset = offsetof(CPUARMState, pmsav8.prselr)},
+{ .name = "HPRBAR", .resetvalue = 0,
+ .readfn = hprbar_read, .writefn = hprbar_write,
+ .cp = 15, .opc1 = 4, .crn = 6, .crm = 3, .opc2 = 0,
+ .access = PL2_RW, .resetvalue = 0,
+ .fieldoffset = offsetof(CPUARMState, pmsav8.hprbar)},
+{ .name = "HPRLAR",
+ .cp = 15, .opc1 = 4, .crn = 6, .crm = 3, .opc2 = 1,
+ .access = PL2_RW, .resetvalue = 0,
+ .readfn =
[PATCH v3 3/9] target/arm: Make RVBAR available for all ARMv8 CPUs
From: Tobias Röhmel
RVBAR shadows RVBAR_ELx where x is the highest exception
level if the highest EL is not EL3. This patch also allows
ARMv8 CPUs to change the reset address to be changed with
the rvbar property.
Signed-off-by: Tobias Röhmel
---
target/arm/cpu.c| 6 +-
target/arm/helper.c | 38 ++
2 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 1b5d535788..9007768418 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -258,6 +258,10 @@ static void arm_cpu_reset(DeviceState *dev)
env->cp15.cpacr_el1 = FIELD_DP64(env->cp15.cpacr_el1,
CPACR, CP11, 3);
#endif
+if (arm_feature(env, ARM_FEATURE_V8)) {
+env->cp15.rvbar = cpu->rvbar_prop;
+env->regs[15] = cpu->rvbar_prop;
+}
}
#if defined(CONFIG_USER_ONLY)
@@ -1273,7 +1277,7 @@ void arm_cpu_post_init(Object *obj)
qdev_property_add_static(DEVICE(obj), _cpu_reset_hivecs_property);
}
-if (arm_feature(>env, ARM_FEATURE_AARCH64)) {
+if (arm_feature(>env, ARM_FEATURE_V8)) {
object_property_add_uint64_ptr(obj, "rvbar",
>rvbar_prop,
OBJ_PROP_FLAG_READWRITE);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index b9547594ae..23461397e0 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7954,13 +7954,20 @@ void register_cp_regs_for_features(ARMCPU *cpu)
/* RVBAR_EL1 is only implemented if EL1 is the highest EL */
if (!arm_feature(env, ARM_FEATURE_EL3) &&
!arm_feature(env, ARM_FEATURE_EL2)) {
-ARMCPRegInfo rvbar = {
-.name = "RVBAR_EL1", .state = ARM_CP_STATE_AA64,
-.opc0 = 3, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
-.access = PL1_R,
-.fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+ARMCPRegInfo rvbar[] = {
+{
+.name = "RVBAR_EL1", .state = ARM_CP_STATE_AA64,
+.opc0 = 3, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
+.access = PL1_R,
+.fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+},
+{ .name = "RVBAR", .type = ARM_CP_ALIAS,
+.cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 1,
+.access = PL1_R,
+.fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+},
};
-define_one_arm_cp_reg(cpu, );
+define_arm_cp_regs(cpu, rvbar);
}
define_arm_cp_regs(cpu, v8_idregs);
define_arm_cp_regs(cpu, v8_cp_reginfo);
@@ -8022,13 +8029,20 @@ void register_cp_regs_for_features(ARMCPU *cpu)
}
/* RVBAR_EL2 is only implemented if EL2 is the highest EL */
if (!arm_feature(env, ARM_FEATURE_EL3)) {
-ARMCPRegInfo rvbar = {
-.name = "RVBAR_EL2", .state = ARM_CP_STATE_AA64,
-.opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 1,
-.access = PL2_R,
-.fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+ARMCPRegInfo rvbar[] = {
+{
+.name = "RVBAR_EL2", .state = ARM_CP_STATE_AA64,
+.opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 1,
+.access = PL2_R,
+.fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+},
+{ .name = "RVBAR", .type = ARM_CP_ALIAS,
+.cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 1,
+.access = PL2_R,
+.fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+},
};
-define_one_arm_cp_reg(cpu, );
+define_arm_cp_regs(cpu, rvbar);
}
}
--
2.25.1
[PATCH v3 4/9] target/arm: Make stage_2_format for cache attributes optional
From: Tobias Röhmel
The Cortex-R52 has a 2 stage MPU translation process but doesn't have the
FEAT_S2FWB feature. This makes it neccessary to allow for the old cache
attribut combination. This is facilitated by changing the control path
of combine_cacheattrs instead of failing if the second cache attributes
struct is not in that format.
Signed-off-by: Tobias Röhmel
---
target/arm/ptw.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 4d97a24808..8b037c1f55 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2108,7 +2108,11 @@ static uint8_t combined_attrs_nofwb(CPUARMState *env,
{
uint8_t s1lo, s2lo, s1hi, s2hi, s2_mair_attrs, ret_attrs;
-s2_mair_attrs = convert_stage2_attrs(env, s2.attrs);
+if (s2.is_s2_format) {
+s2_mair_attrs = convert_stage2_attrs(env, s2.attrs);
+} else {
+s2_mair_attrs = s2.attrs;
+}
s1lo = extract32(s1.attrs, 0, 4);
s2lo = extract32(s2_mair_attrs, 0, 4);
@@ -2166,6 +2170,8 @@ static uint8_t force_cacheattr_nibble_wb(uint8_t attr)
static uint8_t combined_attrs_fwb(CPUARMState *env,
ARMCacheAttrs s1, ARMCacheAttrs s2)
{
+assert(s2.is_s2_format && !s1.is_s2_format);
+
switch (s2.attrs) {
case 7:
/* Use stage 1 attributes */
@@ -2215,7 +2221,6 @@ static ARMCacheAttrs combine_cacheattrs(CPUARMState *env,
ARMCacheAttrs ret;
bool tagged = false;
-assert(s2.is_s2_format && !s1.is_s2_format);
ret.is_s2_format = false;
if (s1.attrs == 0xf0) {
--
2.25.1
[PATCH v3 0/9] Add ARM Cortex-R52 cpu
From: Tobias Röhmel Thanks for the review! Here is v3: v3: PATCH 2 (Don't add all MIDR aliases for cores that immplement PMSA): fixed the comment and changed to single element instead of array. Also the alias is not added for all PMSA CPUs as Peter suggested. PATCH 3 (Make RVBAR available for all ARMv8 CPUs): Added the missing RVBAR register as alias to RVBAR_ELx. Tested that this code actually takes the lower 32 bits of RVBAR_ELx as RVBAR. v2: PATCH 1: I have left the flag in for now because there there is a lot of use for it in the MPU translation code. The PMSAv8r differs in quite a view ways from the implementation in the Cortex-M. I think using !ARM_FEATURE_M in all of those cases might run into problems down the road when new things are added. But I'll gladly change that if those concerns are not valid. PATCH 2: Patch was moved and I removed the ATCM... registers. PATCH 3: The issue here is that the R52 has the MPUIR register which shares the encoding with one of the MIDR alias registers. It's now changed to not add that register for ARM_FEATURE_V8_R. PATCH 4: Added RVBAR for all v8 CPUs instead of just ARMv8r PATCH 7: Instead of setting TTBCR_EAE to 1, change all functions that rely on that value and are relevant for Cortex-R52 PATCH 10: SPSR_hyp changes removed PATCH 11: Removed the r52_machine. The issue with adding the Cortex-R52 to the virt board is that target_page.bits is expected to be 12 but is set to 10 for ARM_FEATURE_PMSA. Simply changing that or using virt2.7(which doesn't have that restriction) leads to problems with memory accesses. It might be best to model an existing board. These patches add the ARM Cortex-R52. The biggest addition is an implementation of the armv8-r MPU. All information is taken from: - ARM Cortex-R52 TRM revision r1p3 - ARM Architecture Reference Manual Supplement -ARMv8 for the ARMv8-R AArch32 architecture profile Version A.c Functionality that is not implemented: - Changing between single and double precision floats - Some hypervisor related functionality (HCR.T(R)VM,HADFSR,...) Tobias Röhmel (9): target/arm: Add ARM_FEATURE_V8_R target/arm: Don't add all MIDR aliases for cores that immplement PMSA target/arm: Make RVBAR available for all ARMv8 CPUs target/arm: Make stage_2_format for cache attributes optional target/arm: Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup target/arm: Enable TTBCR_EAE for ARMv8-R AArch32 target/arm: Add PMSAv8r registers target/arm: Add PMSAv8r functionality target/arm: Add ARM Cortex-R52 cpu target/arm/cpu.c | 6 +- target/arm/cpu.h | 11 ++ target/arm/cpu_tcg.c | 42 +++ target/arm/debug_helper.c | 3 +- target/arm/helper.c | 223 +++--- target/arm/internals.h| 16 +-- target/arm/m_helper.c | 3 +- target/arm/ptw.c | 191 +++- target/arm/tlb_helper.c | 3 +- 9 files changed, 444 insertions(+), 54 deletions(-) -- 2.25.1
[PATCH v3 6/9] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
From: Tobias Röhmel
ARMv8-R AArch32 CPUs behave as if TTBCR.EAE is always 1 even
tough they don't have the TTBCR register.
See ARM Architecture Reference Manual Supplement - ARMv8, for the ARMv8-R
AArch32 architecture profile Version:A.c section C1.2.
Signed-off-by: Tobias Röhmel
---
target/arm/debug_helper.c | 3 ++-
target/arm/internals.h| 3 ++-
target/arm/tlb_helper.c | 3 ++-
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index b18a6bd3a2..44b1e32974 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -434,7 +434,8 @@ static uint32_t arm_debug_exception_fsr(CPUARMState *env)
using_lpae = true;
} else {
if (arm_feature(env, ARM_FEATURE_LPAE) &&
-(env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)) {
+((env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)
+|| arm_feature(env, ARM_FEATURE_V8_R))) {
using_lpae = true;
}
}
diff --git a/target/arm/internals.h b/target/arm/internals.h
index b03049d920..e2a2b03d41 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -254,7 +254,8 @@ static inline bool extended_addresses_enabled(CPUARMState
*env)
{
TCR *tcr = >cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
return arm_el_is_aa64(env, 1) ||
- (arm_feature(env, ARM_FEATURE_LPAE) && (tcr->raw_tcr & TTBCR_EAE));
+ (arm_feature(env, ARM_FEATURE_LPAE) && ((tcr->raw_tcr & TTBCR_EAE)
+ || arm_feature(env, ARM_FEATURE_V8_R)));
}
/* Update a QEMU watchpoint based on the information the guest has set in the
diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
index 7d8a86b3c4..891326edb8 100644
--- a/target/arm/tlb_helper.c
+++ b/target/arm/tlb_helper.c
@@ -20,7 +20,8 @@ bool regime_using_lpae_format(CPUARMState *env, ARMMMUIdx
mmu_idx)
return true;
}
if (arm_feature(env, ARM_FEATURE_LPAE)
-&& (regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)) {
+&& ((regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)
+|| arm_feature(env, ARM_FEATURE_V8_R))) {
return true;
}
return false;
--
2.25.1
Re: [PATCH v2] scsi-disk: support setting CD-ROM block size via device options
No, I had not seen it indeed. Queued now, thanks.
Paolo
Il gio 4 ago 2022, 14:39 John Millikin ha scritto:
> SunOS expects CD-ROM devices to have a block size of 512, and will
> fail to mount or install using QEMU's default block size of 2048.
>
> When initializing the SCSI device, allow the `physical_block_size'
> block device option to override the default block size.
>
> Signed-off-by: John Millikin
> ---
> hw/scsi/scsi-disk.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> This is the same diff as sent to qemu-devel@ about a week ago. That
> first email seems to have been eaten by a grue, but replying to it
> worked, so maybe the grue is gone now.
>
> See https://gitlab.com/qemu-project/qemu/-/issues/1127 for some
> related discussion about SunOS CD-ROM compatibility.
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index f5cdb9ad4b..acdf8dc05c 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2533,6 +2533,7 @@ static void scsi_cd_realize(SCSIDevice *dev, Error
> **errp)
> SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
> AioContext *ctx;
> int ret;
> +uint32_t blocksize = 2048;
>
> if (!dev->conf.blk) {
> /* Anonymous BlockBackend for an empty drive. As we put it into
> @@ -2542,9 +2543,13 @@ static void scsi_cd_realize(SCSIDevice *dev, Error
> **errp)
> assert(ret == 0);
> }
>
> +if (dev->conf.physical_block_size != 0) {
> +blocksize = dev->conf.physical_block_size;
> +}
> +
> ctx = blk_get_aio_context(dev->conf.blk);
> aio_context_acquire(ctx);
> -s->qdev.blocksize = 2048;
> +s->qdev.blocksize = blocksize;
> s->qdev.type = TYPE_ROM;
> s->features |= 1 << SCSI_DISK_F_REMOVABLE;
> if (!s->product) {
> --
> 2.25.1
>
>
Re: [PATCH v7 07/12] multifd: Prepare to send a packet without the mutex held
On Fri, Aug 19, 2022 at 8:32 AM Juan Quintela wrote:
>
> Leonardo Brás wrote:
> > On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> >> We do the send_prepare() and the fill of the head packet without the
> >> mutex held. It will help a lot for compression and later in the
> >> series for zero pages.
> >>
> >> Notice that we can use p->pages without holding p->mutex because
> >> p->pending_job == 1.
> >>
> >> Signed-off-by: Juan Quintela
> >> ---
> >> migration/multifd.h | 2 ++
> >> migration/multifd.c | 11 ++-
> >> 2 files changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/migration/multifd.h b/migration/multifd.h
> >> index a67cefc0a2..cd389d18d2 100644
> >> --- a/migration/multifd.h
> >> +++ b/migration/multifd.h
> >> @@ -109,7 +109,9 @@ typedef struct {
> >> /* array of pages to sent.
> >> * The owner of 'pages' depends of 'pending_job' value:
> >> * pending_job == 0 -> migration_thread can use it.
> >> + * No need for mutex lock.
> >> * pending_job != 0 -> multifd_channel can use it.
> >> + * No need for mutex lock.
> >> */
> >> MultiFDPages_t *pages;
> >>
> >> diff --git a/migration/multifd.c b/migration/multifd.c
> >> index 09a40a9135..68fc9f8e88 100644
> >> --- a/migration/multifd.c
> >> +++ b/migration/multifd.c
> >> @@ -663,6 +663,8 @@ static void *multifd_send_thread(void *opaque)
> >> p->flags |= MULTIFD_FLAG_SYNC;
> >> p->sync_needed = false;
> >> }
> >> +qemu_mutex_unlock(>mutex);
> >> +
> >
> > If it unlocks here, we will have unprotected:
> > for (int i = 0; i < p->pages->num; i++) {
> > p->normal[p->normal_num] = p->pages->offset[i];
> > p->normal_num++;
> > }
> >
> > And p->pages seems to be in the mutex-protected area.
> > Should it be ok?
>
> From the documentation:
>
> /* array of pages to sent.
> * The owner of 'pages' depends of 'pending_job' value:
> * pending_job == 0 -> migration_thread can use it.
> * No need for mutex lock.
> * pending_job != 0 -> multifd_channel can use it.
> * No need for mutex lock.
> */
> MultiFDPages_t *pages;
>
> So, it is right.
Oh, right. I missed that part earlier .
>
> > Also, under that we have:
> > if (p->normal_num) {
> > ret = multifd_send_state->ops->send_prepare(p, _err);
> > if (ret != 0) {
> > qemu_mutex_unlock(>mutex);
> > break;
> > }
> > }
> >
> > Calling mutex_unlock() here, even though the unlock already happened before,
> > could cause any issue?
>
> Good catch. Never got an error there.
>
> Removing that bit.
Thanks!
Best regards,
Leo
>
> > Best regards,
>
>
> Thanks, Juan.
>
Re: [PATCH v7 06/12] multifd: Make flags field thread local
On Fri, Aug 19, 2022 at 7:03 AM Juan Quintela wrote:
>
> Leonardo Brás wrote:
> > On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> >> Use of flags with respect to locking was incensistant. For the
> >> sending side:
> >> - it was set to 0 with mutex held on the multifd channel.
> >> - MULTIFD_FLAG_SYNC was set with mutex held on the migration thread.
> >> - Everything else was done without the mutex held on the multifd channel.
> >>
> >> On the reception side, it is not used on the migration thread, only on
> >> the multifd channels threads.
> >>
> >> So we move it to the multifd channels thread only variables, and we
> >> introduce a new bool sync_needed on the send side to pass that information.
> >>
> >> Signed-off-by: Juan Quintela
> >> ---
> >> migration/multifd.h | 10 ++
> >> migration/multifd.c | 23 +--
> >> 2 files changed, 19 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/migration/multifd.h b/migration/multifd.h
> >> index 36f899c56f..a67cefc0a2 100644
> >> --- a/migration/multifd.h
> >> +++ b/migration/multifd.h
> >> @@ -98,12 +98,12 @@ typedef struct {
> >
> > Just noticed having no name in 'typedef struct' line makes it harder to
> > understand what is going on.
>
> It is common idiom in QEMU. The principal reason is that if you don't
> want anyone to use "struct MultiFDSendParams" but MultiFDSendParams, the
> best way to achieve that is to do it this way.
I agree, but a comment after the typedef could help reviewing. Something like
typedef struct { /* MultiFDSendParams */
...
} MultiFDSendParams
Becomes this in diff:
diff --git a/migration/multifd.h b/migration/multifd.h
index 134e6a7f19..93bb3a7f4a 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -90,6 +90,7 @@ typedef struct { /* MultiFDSendParams */
[...]
>
> >> @@ -172,6 +172,8 @@ typedef struct {
> >>
> >> /* pointer to the packet */
> >> MultiFDPacket_t *packet;
> >> +/* multifd flags for each packet */
> >> +uint32_t flags;
> >> /* size of the next packet that contains pages */
> >> uint32_t next_packet_size;
> >> /* packets sent through this channel */
> >
> > So, IIUC, the struct member flags got moved down (same struct) to an area
> > described as thread-local, meaning it does not need locking.
> >
> > Interesting, I haven't noticed this different areas in the same struct.
>
> It has changed in the last two weeks or so in upstream (it has been on
> this patchset for several months.)
Nice :)
>
>
> >
> >> diff --git a/migration/multifd.c b/migration/multifd.c
> >> index e25b529235..09a40a9135 100644
> >> --- a/migration/multifd.c
> >> +++ b/migration/multifd.c
> >> @@ -602,7 +602,7 @@ int multifd_send_sync_main(QEMUFile *f)
> >> }
> >>
> >> p->packet_num = multifd_send_state->packet_num++;
> >> -p->flags |= MULTIFD_FLAG_SYNC;
> >> +p->sync_needed = true;
> >> p->pending_job++;
> >> qemu_mutex_unlock(>mutex);
> >> qemu_sem_post(>sem);
> >> @@ -658,7 +658,11 @@ static void *multifd_send_thread(void *opaque)
> >>
> >> if (p->pending_job) {
> >> uint64_t packet_num = p->packet_num;
> >> -uint32_t flags = p->flags;
> >> +p->flags = 0;
> >> +if (p->sync_needed) {
> >> +p->flags |= MULTIFD_FLAG_SYNC;
> >> +p->sync_needed = false;
> >> +}
> >
> > Any particular reason why doing p->flags = 0, then p->flags |=
> > MULTIFD_FLAG_SYNC
> > ?
>
> It is a bitmap field, and if there is anything on the future, we need to
> set it. I agree that when there is only one flag, it seems "weird".
>
> > [1] Couldn't it be done without the |= , since it's already being set to
> > zero
> > before? (becoming "p->flags = MULTIFD_FLAG_SYNC" )
>
> As said, easier to modify later, and also easier if we want to setup a
> flag by default.
Yeah, I agree. It makes sense now.
Thanks
>
> I agree that it is a matter of style/taste.
>
> >> p->normal_num = 0;
> >>
> >> if (use_zero_copy_send) {
> >> @@ -680,14 +684,13 @@ static void *multifd_send_thread(void *opaque)
> >> }
> >> }
> >> multifd_send_fill_packet(p);
> >> -p->flags = 0;
> >> p->num_packets++;
> >> p->total_normal_pages += p->normal_num;
> >> p->pages->num = 0;
> >> p->pages->block = NULL;
> >> qemu_mutex_unlock(>mutex);
> >>
> >> -trace_multifd_send(p->id, packet_num, p->normal_num, flags,
> >> +trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
> >> p->next_packet_size);
> >>
> >> if (use_zero_copy_send) {
> >> @@ -715,7 +718,7 @@ static void *multifd_send_thread(void *opaque)
> >> p->pending_job--;
> >> qemu_mutex_unlock(>mutex);
> >>
> >> -if (flags & MULTIFD_FLAG_SYNC) {
> >> +
Re: [PATCH v7 05/12] migration: Make ram_save_target_page() a pointer
On Fri, Aug 19, 2022 at 6:52 AM Juan Quintela wrote:
>
> Leonardo Brás wrote:
> > On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> >> We are going to create a new function for multifd latest in the series.
> >>
> >> Signed-off-by: Juan Quintela
> >> Reviewed-by: Dr. David Alan Gilbert
> >> Signed-off-by: Juan Quintela
> >
> > Double Signed-off-by again.
> >
> >> ---
> >> migration/ram.c | 13 +
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 85d89d61ac..499d9b2a90 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -310,6 +310,9 @@ typedef struct {
> >> bool preempted;
> >> } PostcopyPreemptState;
> >>
> >> +typedef struct RAMState RAMState;
> >> +typedef struct PageSearchStatus PageSearchStatus;
> >> +
> >> /* State of RAM for migration */
> >> struct RAMState {
> >> /* QEMUFile used for this migration */
> >> @@ -372,8 +375,9 @@ struct RAMState {
> >> * is enabled.
> >> */
> >> unsigned int postcopy_channel;
> >> +
> >> +int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
> >> };
> >> -typedef struct RAMState RAMState;
> >>
> >> static RAMState *ram_state;
> >>
> >> @@ -2255,14 +2259,14 @@ static bool save_compress_page(RAMState *rs,
> >> RAMBlock *block, ram_addr_t offset)
> >> }
> >>
> >> /**
> >> - * ram_save_target_page: save one target page
> >> + * ram_save_target_page_legacy: save one target page
> >> *
> >> * Returns the number of pages written
> >> *
> >> * @rs: current RAM state
> >> * @pss: data about the page we want to send
> >> */
> >> -static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
> >> +static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus
> >> *pss)
> >> {
> >> RAMBlock *block = pss->block;
> >> ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
> >> @@ -2469,7 +2473,7 @@ static int ram_save_host_page(RAMState *rs,
> >> PageSearchStatus *pss)
> >>
> >> /* Check the pages is dirty and if it is send it */
> >> if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
> >> -tmppages = ram_save_target_page(rs, pss);
> >> +tmppages = rs->ram_save_target_page(rs, pss);
> >> if (tmppages < 0) {
> >> return tmppages;
> >> }
> >> @@ -3223,6 +3227,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >> ram_control_before_iterate(f, RAM_CONTROL_SETUP);
> >> ram_control_after_iterate(f, RAM_CONTROL_SETUP);
> >>
> >> +(*rsp)->ram_save_target_page = ram_save_target_page_legacy;
> >> ret = multifd_send_sync_main(f);
> >> if (ret < 0) {
> >> return ret;
> >
> >
> > So, IIUC:
> > - Rename ram_save_target_page -> ram_save_target_page_legacy
> > - Add a function pointer to RAMState (or a callback)
> > - Assign function pointer = ram_save_target_page_legacy at setup
> > - Replace ram_save_target_page() by indirect function call using above
> > pointer.
> >
> > I could see no issue in this, so I belive it works fine.
> >
> > The only thing that concerns me is the name RAMState.
>
> Every device state is setup in RAMState.
>
> > IMHO, a struct named RAMState is supposed to just reflect the state of ram
> > (or
> > according to this struct's comments, the state of RAM for migration. Having
> > a
> > function pointer here that saves a page seems counterintuitive, since it
> > does
> > not reflect the state of RAM.
>
> The big problem for adding another struct is that we would have to
> change all the callers, or yet another global variable. Both are bad
> idea in my humble opinion.
>
> > Maybe we could rename the struct, or even better, create another struct that
> > could look something like this:
> >
> > struct RAMMigration {
> > RAMState state;
> > int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
> > /* Other callbacks or further info.*/
> > }
> >
> > What do you think about it?
>
> Really this depends on configuration. What is setup for qemu
> migration. I think this is the easiest way to do it, we can add a new
> struct, but it gets everything much more complicated:
>
> - the value that we receive in ram_save_setup() is a RAMState
> - We would have to change all the callers form
> * ram_save_iterate()
> * ram_find_and_save_block()
> * ram_save_host_page()
Maybe RAMState could be part of a bigger struct, and we could use
something like a container_of().
So whenever you want to use it, it would be available.
What about that?
>
> So I think it is quite a bit of churn for not a lot of gain.
>
> Later, Juan.
>
