Re: [Qemu-devel] [PATCH v2 00/11] target-arm: handle mmu_idx/translation regimes properly

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Thu, Jan 29, 2015 at 06:55:06PM +0000, Peter Maydell wrote:
> This patchseries fixes up our somewhat broken handling of mmu_idx values:
>  * implement the full set of 7 mmu_idxes we need for supporting EL2 and EL3
>  * pass the mmu_idx in the TB flags rather than EL or a priv flag,
>    so we can generate code with the correct kind of access
>  * identify the correct mmu_idx to use for AT/ATS system insns
>  * pass mmu_idx into get_phys_addr() and use it within that family
>    of functions as an indication of which translation regime to do
>    a v-to-p lookup for, instead of relying on an is_user flag plus the
>    current CPU state
>  * some minor indent stuff on the end


Hi Peter,

A little bit of general feedback.

IIRC, last time the dedicated S-EL0 and S-EL1 MMU idx came up the
discussion went around flushing the qemu tlbs when switching between
S/NS. Having the dedicated MMU-idx is faster but for Aarch64 I think
we would need logic in at least the TTBRx access handlers to make use
of the dedicated secure MMU idx as Aarch64 secure monitors need to
reprogram the MMU when world switching.

Another thing around the ARMMMUIdx_S2NS index.
>From what I've seen, what would really help is having a fast
way to go from VM mode to non-vm mode. In particular for KVM.
For example when a guest writes to a virtio console there is alot
of ping-ponging between NS-S12(Guest) and NS-S1(Linux/KVM).

Similary for XEN, it would really help to have that ASID/VMID indexed TLB I
think you suggested at some point. In XEN's case the ping-ponging
goes between two guests, domUs and dom0.

I'm not try to indicate that you should add any of that now,
I'm just not sure sure it's worth adding the ARMMMUIdx_S2NS without
trying if it will actually give any real life improvements in
QEMU.

Cheers,
Edgar


> 
> It does not contain:
>  * complete support for EL2 or 64-bit EL3; in some places I have added
>    the code where it was obvious and easy; in others I have just left
>    TODO marker comments
>  * the 'tlb_flush_for_mmuidx' functionality I proposed in a previous mail;
>    I preferred to get the semantics right in this patchset first before
>    improving the efficiency later
> 
> Changes v1->v2:
>  * use the correct FCSEIDR for the translation regime
>  * fix typo in patch 1 for MEMSUFFIX to use for new index 6
>  * a few new comments and other minor nits as per review of v1
> 
> Peter Maydell (11):
>   cpu_ldst.h: Allow NB_MMU_MODES to be 7
>   target-arm: Make arm_current_el() return sensible values for M profile
>   target-arm/translate-a64: Fix wrong mmu_idx usage for LDT/STT
>   target-arm: Define correct mmu_idx values and pass them in TB flags
>   target-arm: Use correct mmu_idx for unprivileged loads and stores
>   target-arm: Don't define any MMU_MODE*_SUFFIXes
>   target-arm: Split AArch64 cases out of ats_write()
>   target-arm: Pass mmu_idx to get_phys_addr()
>   target-arm: Use mmu_idx in get_phys_addr()
>   target-arm: Reindent ancient page-table-walk code
>   target-arm: Fix brace style in reindented code
> 
>  include/exec/cpu_ldst.h    |  28 ++-
>  target-arm/cpu.h           | 121 +++++++---
>  target-arm/helper.c        | 548 +++++++++++++++++++++++++++++++--------------
>  target-arm/translate-a64.c |  24 +-
>  target-arm/translate.c     |  31 ++-
>  target-arm/translate.h     |   3 +-
>  6 files changed, 557 insertions(+), 198 deletions(-)
> 
> -- 
> 1.9.1
>