[Qemu-devel] [RFC 0/3] split core mmu_idx from ARMMMUIdx values

Peter Maydell posted 3 patches 6 years, 12 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1493051615-30715-1-git-send-email-peter.maydell@linaro.org
Test checkpatch passed
Test docker passed
Test s390x passed
target/arm/cpu.h           |  77 ++++++++++++++++++----
target/arm/translate.h     |   2 +-
target/arm/helper.c        | 156 ++++++++++++++++++++++++---------------------
target/arm/op_helper.c     |   3 +-
target/arm/translate-a64.c |  18 ++++--
target/arm/translate.c     |  13 ++--
6 files changed, 171 insertions(+), 98 deletions(-)
[Qemu-devel] [RFC 0/3] split core mmu_idx from ARMMMUIdx values
Posted by Peter Maydell 6 years, 12 months ago
For M profile, we're eventually going to want some
new MMU index values:
     non secure User
     non secure Privileged
     non secure Privileged, execution priority < 0
     secure User
     secure Privileged
     secure Privileged, execution priority < 0

(currently we have ns User and ns Priv). Although we
can reasonably cleanly think of 4 of those as being
like A profile NS EL0/NS EL1/S EL0/S EL1, the "privileged
and execution priority is < 0" cases are entirely M profile
specific. This gives us two choices of implementation:

(1) Add new ARMMMUIdx values, bringing NB_MMU_MODES up to 9.
This is large enough that it would mean the size of a TLB on
MIPS, PPC and 32-bit ARM hosts would be halved.

(2) Allow A profile and M profile cores to allocate the mmu_idx
values to different semantics, since any particular CPU will
be definitely one kind or the other and doesn't need all 9.

This patchset is an RFC for choice (2), to see whether people
think it looks sufficiently maintainable. The idea is that
rather than just having
  ARMMMUIdx_S12NSE0 = 0,
  ARMMMUIdx_MUser = 0,
and relying on all the target/arm code to remember to disambiguate
(which seemed very error prone to me), we have different values:
    ARMMMUIdx_S12NSE0 = 0 | ARM_MMU_IDX_A,
    ARMMMUIdx_MUser = 0 | ARM_MMU_IDX_M,
so instead of ARMMMUIdx enum values being identical to core QEMU
MMU index values, they are now the core index values with some
high bits set. Any particular CPU always uses the same high
bits. New functions arm_to_core_mmu_idx() and core_to_arm_mmu_idx()
convert between the two.

This means that if a function that does
  switch (mmu_idx) {
  case ARMMMUIdx_S12NSE0:
  ...
  }
is inadvertently called with an M profile mmu index and it wasn't
expecting it, it will at least assert rather than just doing
something odd, and it doesn't have to explicitly look at the
feature bits to make a decision.

In general core index values are stored in 'int' types, and
ARM values are stored in ARMMMUIdx types. QEMU core code,
the TB flags value and the translate.c code which passes
mmu_idx values to gen_qemu_ld/st_* all use core index values,
and the rest uses ARMMMUIdx values.
    

Overall questions:
 * does this look like it's workable enough to justify taking
   it rather than the simple-but-worse-perf-on-some-archs
   option 1 ?
 * this does risk breaking A profile in corner cases, because
   C unfortunately doesn't typecheck int vs enum so if I forgot
   a call to an index conversion function somewhere things will
   break. are we happy the risk isn't too huge?


thanks
-- PMM

Peter Maydell (3):
  arm: Use the mmu_idx we're passed in arm_cpu_do_unaligned_access()
  arm: Add support for M profile CPUs having different MMU index
    semantics
  arm: Use different ARMMMUIdx values for M profile

 target/arm/cpu.h           |  77 ++++++++++++++++++----
 target/arm/translate.h     |   2 +-
 target/arm/helper.c        | 156 ++++++++++++++++++++++++---------------------
 target/arm/op_helper.c     |   3 +-
 target/arm/translate-a64.c |  18 ++++--
 target/arm/translate.c     |  13 ++--
 6 files changed, 171 insertions(+), 98 deletions(-)

-- 
2.7.4


Re: [Qemu-devel] [Qemu-arm] [RFC 0/3] split core mmu_idx from ARMMMUIdx values
Posted by Peter Maydell 6 years, 12 months ago
On 24 April 2017 at 17:33, Peter Maydell <peter.maydell@linaro.org> wrote:
> For M profile, we're eventually going to want some
> new MMU index values:
>      non secure User
>      non secure Privileged
>      non secure Privileged, execution priority < 0
>      secure User
>      secure Privileged
>      secure Privileged, execution priority < 0

Hmm, Alex and Edgar were definitely on the cc when I sent it but
seem to have fallen off it. (If you got a copy anyway then you
should probably check whether you have accidentally told mailman to
"filter out duplicate messages": you never want this setting
enabled, because it means "silently drop me from the cc list".)

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] split core mmu_idx from ARMMMUIdx values
Posted by Richard Henderson 6 years, 12 months ago
On 04/24/2017 06:33 PM, Peter Maydell wrote:
> Overall questions:
>   * does this look like it's workable enough to justify taking
>     it rather than the simple-but-worse-perf-on-some-archs
>     option 1 ?

I think it's workable.

>   * this does risk breaking A profile in corner cases, because
>     C unfortunately doesn't typecheck int vs enum so if I forgot
>     a call to an index conversion function somewhere things will
>     break. are we happy the risk isn't too huge?

If you like, we could add a tcg_debug assert somewhere that verifies that the 
given index is < NB_MMU_MODES...


r~

Re: [Qemu-devel] [RFC 0/3] split core mmu_idx from ARMMMUIdx values
Posted by Peter Maydell 6 years, 12 months ago
On 26 April 2017 at 11:07, Richard Henderson <rth@twiddle.net> wrote:
> On 04/24/2017 06:33 PM, Peter Maydell wrote:
>>
>> Overall questions:
>>   * does this look like it's workable enough to justify taking
>>     it rather than the simple-but-worse-perf-on-some-archs
>>     option 1 ?
>
>
> I think it's workable.

Yes, I think I agree overall.

>>   * this does risk breaking A profile in corner cases, because
>>     C unfortunately doesn't typecheck int vs enum so if I forgot
>>     a call to an index conversion function somewhere things will
>>     break. are we happy the risk isn't too huge?
>
>
> If you like, we could add a tcg_debug assert somewhere that verifies that
> the given index is < NB_MMU_MODES...

There is one already, because in my testing I ran into it.
The problem is that since it's a runtime assert, it's not
certain that I caught everything.

thanks
-- PMM