[v2] don't hardcode EL1 in extended_addresses_enabled

Commit Message

Stefano Stabellini Oct. 25, 2017, 11:28 p.m. UTC

extended_addresses_enabled calls arm_el_is_aa64, hardcoding exception
level 1. Instead, add an additional "el" argument to
extended_addresses_enabled.

The caller will pass the right value. In most cases, it will be
arm_current_el(env). However, arm_debug_excp_handler will
use arm_debug_target_el(env), as the target el for a debug trap can be
different from the current el.

Signed-off-by: Stefano Stabellini <[e-mail]>

Comments

Peter Maydell Oct. 30, 2017, 5:57 p.m. UTC | #1

On 26 October 2017 at 00:28, Stefano Stabellini <[e-mail]> wrote:
> extended_addresses_enabled calls arm_el_is_aa64, hardcoding exception
> level 1. Instead, add an additional "el" argument to
> extended_addresses_enabled.
>
> The caller will pass the right value. In most cases, it will be
> arm_current_el(env). However, arm_debug_excp_handler will
> use arm_debug_target_el(env), as the target el for a debug trap can be
> different from the current el.
>
> Signed-off-by: Stefano Stabellini <[e-mail]>

I have some longer comments below about what a mess this whole
area is. Fixing some of that requires some heavy refactoring,
which I don't want to do just now since we're about to go into
softfreeze for the next release.

What's the specific situation/bug that you're trying to fix with
this patch? You don't say in the commit message.
We should be able to put in a point fix to deal with whatever it is,
but it's hard to suggest what that would be without the detail
of what exactly we're getting wrong. (It's the PAR format stuff,
right? But which ATS instruction are you using, from which
exception level, with which register width, for which stage
1 page table format and stage 1 guest register width?)

> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 96113fe..2298428 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -500,7 +500,7 @@ static void contextidr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      ARMCPU *cpu = arm_env_get_cpu(env);
>
>      if (raw_read(env, ri) != value && !arm_feature(env, ARM_FEATURE_PMSA)
> -        && !extended_addresses_enabled(env)) {
> +        && !extended_addresses_enabled(env, arm_current_el(env))) {
>          /* For VMSA (when not using the LPAE long descriptor page table
>           * format) this register includes the ASID, so do a TLB flush.
>           * For PMSA it is purely a process ID and no action is needed.

This isn't really right for figuring out what to do on CONTEXTIDR writes
in the general case. What we want is something along the lines of:

need_flush = true;
if (EL3 is AArch64) {
    /* There is only one CONTEXTIDR, and it applies to EL1. We only need
     * to flush if EL1 is or will be AArch32 and has extended addresses
     * disabled.
     */
    if (tcr_el[1].TTBCR_EAE) {
        need_flush = false;
    }
} else {
    /* If extended addressing is enabled for the translation regime that
     * this CONTEXTIDR register applies to, then there is no ASID field
     * and we don't need to TLB flush. (If we later change the EAE bit
     * we'll flush then.)
     */
    bool sec = ri->secure & ARM_CP_SECSTATE_S;
    if (FEATURE_LPAE && tcr_el[sec ? 3 : 1].TTBCR_EAE) {
        need_flush = false;
    }
}
if (need_flush) {
    /* We should be cleverer about which MMU indexes need flushing here */
    tlb_flush(CPU(cpu));
}

because we need to handle the case of "EL1 is using short descriptors
and EL2 writes to CONTEXTIDR for EL1".

...but then we also need to tlb_flush when the tcr bits change, which
I don't think we do correctly. (We never notice this sort of bug because
we handle correctly the common cases of "all aarch64, or aarch64 and
LPAE aarch32" and "short descriptors in an aarch32-only EL1/EL0-only
config".)

> @@ -2162,7 +2162,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>
>      ret = get_phys_addr(env, value, access_type, mmu_idx,
>                          &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
> -    if (extended_addresses_enabled(env)) {
> +    if (extended_addresses_enabled(env, arm_current_el(env))) {

I think what you want to be checking here is
       if (arm_s1_regime_using_lpae_format(env, mmu_idx)) {

(because you are trying to determine what get_phys_addr() has
just handed you, and that function looks at the state of the
translation regime specified by mmu_idx, not at the current state
of the CPU)...

...but even this isn't really correct, because get_phys_addr() has
some broken cases where for a stage1+2 lookup where stage 1 is
using short descriptors we will return a short-format FSR value
for a stage1 failure but a long-format value for a stage2 failure.

The right long term thing here is to refactor get_phys_addr() so
that instead of returning literal fsr values it should return some
kind of internal QEMU type describing the failure, which we then
convert into the FSR we want at the point when we need to (ie
when taking an exception to a given EL, or writing a PAR value
for a given ATS* instruction).

>          /* fsr is a DFSR/IFSR value for the long descriptor
>           * translation table format, but with WnR always clear.
>           * Convert it to a 64-bit PAR.
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 43106a2..6792df2 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -217,10 +217,10 @@ static inline unsigned int arm_pamax(ARMCPU *cpu)
>   * This is always the case if our translation regime is 64 bit,
>   * but depends on TTBCR.EAE for 32 bit.
>   */
> -static inline bool extended_addresses_enabled(CPUARMState *env)
> +static inline bool extended_addresses_enabled(CPUARMState *env, unsigned int el)
>  {
> -    TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
> -    return arm_el_is_aa64(env, 1) ||
> +    TCR *tcr = &env->cp15.tcr_el[el];
> +    return arm_el_is_aa64(env, el) ||
>             (arm_feature(env, ARM_FEATURE_LPAE) && (tcr->raw_tcr & TTBCR_EAE));

(not an error introduced in this change, but)
This will return the wrong answer for the debug exception case if
we are taking the debug exception to AArch32 Secure EL1 (in which
case EL3 must be AArch64, there is no register banking, and the
correct TCR for Secure EL1 is in tcr_el[1]).

>  }
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 3914145..4f46eb8 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -1378,7 +1378,7 @@ void arm_debug_excp_handler(CPUState *cs)
>
>              cs->watchpoint_hit = NULL;
>
> -            if (extended_addresses_enabled(env)) {
> +            if (extended_addresses_enabled(env, arm_debug_target_el(env))) {
>                  env->exception.fsr = (1 << 9) | 0x22;
>              } else {
>                  env->exception.fsr = 0x2;
> @@ -1402,7 +1402,7 @@ void arm_debug_excp_handler(CPUState *cs)
>              return;
>          }
>
> -        if (extended_addresses_enabled(env)) {
> +        if (extended_addresses_enabled(env, arm_debug_target_el(env))) {
>              env->exception.fsr = (1 << 9) | 0x22;
>          } else {
>              env->exception.fsr = 0x2;

If we managed the refactoring of get_phys_addr() mentioned above then
these could turn into "don't set a specific fsr value, but use an
internal type for saying 'debug exception', and let the other end
create the right FSR for wherever they are".

thanks
-- PMM

Stefano Stabellini Oct. 30, 2017, 10:57 p.m. UTC | #2

On Mon, 30 Oct 2017, Peter Maydell wrote:
> On 26 October 2017 at 00:28, Stefano Stabellini <[e-mail]> wrote:
> > extended_addresses_enabled calls arm_el_is_aa64, hardcoding exception
> > level 1. Instead, add an additional "el" argument to
> > extended_addresses_enabled.
> >
> > The caller will pass the right value. In most cases, it will be
> > arm_current_el(env). However, arm_debug_excp_handler will
> > use arm_debug_target_el(env), as the target el for a debug trap can be
> > different from the current el.
> >
> > Signed-off-by: Stefano Stabellini <[e-mail]>
> 
> I have some longer comments below about what a mess this whole
> area is. Fixing some of that requires some heavy refactoring,
> which I don't want to do just now since we're about to go into
> softfreeze for the next release.
> 
> What's the specific situation/bug that you're trying to fix with
> this patch? You don't say in the commit message.
> We should be able to put in a point fix to deal with whatever it is,
> but it's hard to suggest what that would be without the detail
> of what exactly we're getting wrong. (It's the PAR format stuff,
> right? But which ATS instruction are you using, from which
> exception level, with which register width, for which stage
> 1 page table format and stage 1 guest register width?)

Thank you for understanding, I am not really up for heavy refactoring
in QEMU right now :-)

Yes, I am trying to fix the AT instruction, which is used by Xen for
address translations. Xen always runs at EL2. do_ats_write takes the
wrong path because extended_addresses_enabled assumes EL1.

To go more into details, virt_to_maddr translates a Xen virtual address
into a physical address. Xen implements virt_to_maddr as:

  static inline paddr_t __virt_to_maddr(vaddr_t va)
  {
      uint64_t par = va_to_par(va);
      return (par & PADDR_MASK & PAGE_MASK) | (va & ~PAGE_MASK);
  }

Where va_to_par is:

  #define ATS1HR          p15,4,c7,c8,0   /* Address Translation Stage 1 Hyp. Read */
  static inline uint64_t __va_to_par(vaddr_t va)
  {
      uint64_t par, tmp;
      tmp = READ_CP64(PAR);
      WRITE_CP32(va, ATS1HR);
      isb(); /* Ensure result is available. */
      par = READ_CP64(PAR);
      WRITE_CP64(tmp, PAR);
      return par;
  }

This is what breaks Xen 64-bit booting on qemu-system-aarch64.

For completeness, I'll also point out other uses of ATS instructions in
Xen.

Xen uses the following to translate a guest virtual address into a
physical address (Xen has no saying in the guest pagetable format or
register width):

  #define ATS12NSOPR      p15,0,c7,c8,4   /* Address Translation Stage 1+2 Non-Secure Kernel Read */
  #define ATS12NSOPW      p15,0,c7,c8,5   /* Address Translation Stage 1+2 Non-Secure Kernel Write */
  static inline uint64_t gva_to_ma_par(vaddr_t va, unsigned int flags)
  {
      uint64_t par, tmp;
      tmp = READ_CP64(PAR);
      if ( (flags & GV2M_WRITE) == GV2M_WRITE )
          WRITE_CP32(va, ATS12NSOPW);
      else
          WRITE_CP32(va, ATS12NSOPR);
      isb(); /* Ensure result is available. */
      par = READ_CP64(PAR);
      WRITE_CP64(tmp, PAR);
      return par;
  }

Finally, Xen uses the following to translate guest virtual addresses
into pseudo-physical addresses:

  #define ATS1CPR         p15,0,c7,c8,0   /* Address Translation Stage 1. Non-Secure Kernel Read */
  #define ATS1CPW         p15,0,c7,c8,1   /* Address Translation Stage 1. Non-Secure Kernel Write */
  
  static inline uint64_t gva_to_ipa_par(vaddr_t va, unsigned int flags)
  {
      uint64_t par, tmp;
      tmp = READ_CP64(PAR);
      if ( (flags & GV2M_WRITE) == GV2M_WRITE )
          WRITE_CP32(va, ATS1CPW);
      else
          WRITE_CP32(va, ATS1CPR);
      isb(); /* Ensure result is available. */
      par = READ_CP64(PAR);
      WRITE_CP64(tmp, PAR);
      return par;
  }

Peter Maydell Oct. 31, 2017, 9:48 a.m. UTC | #3

On 30 October 2017 at 22:57, Stefano Stabellini <[e-mail]> wrote:
> On Mon, 30 Oct 2017, Peter Maydell wrote:
>> What's the specific situation/bug that you're trying to fix with
>> this patch? You don't say in the commit message.
>> We should be able to put in a point fix to deal with whatever it is,
>> but it's hard to suggest what that would be without the detail
>> of what exactly we're getting wrong. (It's the PAR format stuff,
>> right? But which ATS instruction are you using, from which
>> exception level, with which register width, for which stage
>> 1 page table format and stage 1 guest register width?)
>
> Thank you for understanding, I am not really up for heavy refactoring
> in QEMU right now :-)
>
> Yes, I am trying to fix the AT instruction, which is used by Xen for
> address translations. Xen always runs at EL2. do_ats_write takes the
> wrong path because extended_addresses_enabled assumes EL1.
>
> To go more into details, virt_to_maddr translates a Xen virtual address
> into a physical address. Xen implements virt_to_maddr as:
>
>   static inline paddr_t __virt_to_maddr(vaddr_t va)
>   {
>       uint64_t par = va_to_par(va);
>       return (par & PADDR_MASK & PAGE_MASK) | (va & ~PAGE_MASK);
>   }
>
> Where va_to_par is:
>
>   #define ATS1HR          p15,4,c7,c8,0   /* Address Translation Stage 1 Hyp. Read */
>   static inline uint64_t __va_to_par(vaddr_t va)
>   {
>       uint64_t par, tmp;
>       tmp = READ_CP64(PAR);
>       WRITE_CP32(va, ATS1HR);
>       isb(); /* Ensure result is available. */
>       par = READ_CP64(PAR);
>       WRITE_CP64(tmp, PAR);
>       return par;
>   }
>
> This is what breaks Xen 64-bit booting on qemu-system-aarch64.

I'm confused. You say this is 64-bit booting, but the code you quote
here looks like it's doing 32-bit cp15 accesses, not 64-bit mrs/msr
sysreg accesses.

We definitely don't support 32-bit Hyp mode right now.

Could you please answer all of:
>> which ATS instruction are you using, from which
>> exception level, with which register width, for which stage
>> 1 page table format and stage 1 guest register width?)

thanks
-- PMM

Stefano Stabellini Oct. 31, 2017, 5:07 p.m. UTC | #4

On Tue, 31 Oct 2017, Peter Maydell wrote:
> On 30 October 2017 at 22:57, Stefano Stabellini <[e-mail]> wrote:
> > On Mon, 30 Oct 2017, Peter Maydell wrote:
> >> What's the specific situation/bug that you're trying to fix with
> >> this patch? You don't say in the commit message.
> >> We should be able to put in a point fix to deal with whatever it is,
> >> but it's hard to suggest what that would be without the detail
> >> of what exactly we're getting wrong. (It's the PAR format stuff,
> >> right? But which ATS instruction are you using, from which
> >> exception level, with which register width, for which stage
> >> 1 page table format and stage 1 guest register width?)
> >
> > Thank you for understanding, I am not really up for heavy refactoring
> > in QEMU right now :-)
> >
> > Yes, I am trying to fix the AT instruction, which is used by Xen for
> > address translations. Xen always runs at EL2. do_ats_write takes the
> > wrong path because extended_addresses_enabled assumes EL1.
> >
> > To go more into details, virt_to_maddr translates a Xen virtual address
> > into a physical address. Xen implements virt_to_maddr as:
> >
> >   static inline paddr_t __virt_to_maddr(vaddr_t va)
> >   {
> >       uint64_t par = va_to_par(va);
> >       return (par & PADDR_MASK & PAGE_MASK) | (va & ~PAGE_MASK);
> >   }
> >
> > Where va_to_par is:
> >
> >   #define ATS1HR          p15,4,c7,c8,0   /* Address Translation Stage 1 Hyp. Read */
> >   static inline uint64_t __va_to_par(vaddr_t va)
> >   {
> >       uint64_t par, tmp;
> >       tmp = READ_CP64(PAR);
> >       WRITE_CP32(va, ATS1HR);
> >       isb(); /* Ensure result is available. */
> >       par = READ_CP64(PAR);
> >       WRITE_CP64(tmp, PAR);
> >       return par;
> >   }
> >
> > This is what breaks Xen 64-bit booting on qemu-system-aarch64.
> 
> I'm confused. You say this is 64-bit booting, but the code you quote
> here looks like it's doing 32-bit cp15 accesses, not 64-bit mrs/msr
> sysreg accesses.
> 
> We definitely don't support 32-bit Hyp mode right now.
> 
> Could you please answer all of:
> >> which ATS instruction are you using, from which
> >> exception level, with which register width, for which stage
> >> 1 page table format and stage 1 guest register width?)

Sorry Peter, I copy/pasted the values from arm32/page.h instead of
arm64/page.h in Xen :-/

Xen is running at EL2, 64-bit (aarch64). The ATS instruction is "at
s1e2r", used to translate Xen virtual addresses into physical addresses.
This is what breaks.

Peter Maydell Oct. 31, 2017, 5:19 p.m. UTC | #5

On 31 October 2017 at 17:07, Stefano Stabellini <[e-mail]> wrote:
> Sorry Peter, I copy/pasted the values from arm32/page.h instead of
> arm64/page.h in Xen :-/
>
> Xen is running at EL2, 64-bit (aarch64). The ATS instruction is "at
> s1e2r", used to translate Xen virtual addresses into physical addresses.
> This is what breaks.

Thanks. OK, so if we change the line in do_ats_write() from
     if (extended_addresses_enabled(env)) {
to
     if (arm_s1_regime_using_lpae_format(env, mmu_idx)) {

does that fix your problem?

thanks
-- PMM

Stefano Stabellini Oct. 31, 2017, 6:08 p.m. UTC | #6

On Tue, 31 Oct 2017, Peter Maydell wrote:
> On 31 October 2017 at 17:07, Stefano Stabellini <[e-mail]> wrote:
> > Sorry Peter, I copy/pasted the values from arm32/page.h instead of
> > arm64/page.h in Xen :-/
> >
> > Xen is running at EL2, 64-bit (aarch64). The ATS instruction is "at
> > s1e2r", used to translate Xen virtual addresses into physical addresses.
> > This is what breaks.
> 
> Thanks. OK, so if we change the line in do_ats_write() from
>      if (extended_addresses_enabled(env)) {
> to
>      if (arm_s1_regime_using_lpae_format(env, mmu_idx)) {
> 
> does that fix your problem?

Yes, it does! Thank you! Are you going to submit a patch?

Peter Maydell Oct. 31, 2017, 6:15 p.m. UTC | #7

On 31 October 2017 at 18:08, Stefano Stabellini <[e-mail]> wrote:
> On Tue, 31 Oct 2017, Peter Maydell wrote:
>> On 31 October 2017 at 17:07, Stefano Stabellini <[e-mail]> wrote:
>> > Sorry Peter, I copy/pasted the values from arm32/page.h instead of
>> > arm64/page.h in Xen :-/
>> >
>> > Xen is running at EL2, 64-bit (aarch64). The ATS instruction is "at
>> > s1e2r", used to translate Xen virtual addresses into physical addresses.
>> > This is what breaks.
>>
>> Thanks. OK, so if we change the line in do_ats_write() from
>>      if (extended_addresses_enabled(env)) {
>> to
>>      if (arm_s1_regime_using_lpae_format(env, mmu_idx)) {
>>
>> does that fix your problem?
>
> Yes, it does! Thank you! Are you going to submit a patch?

Yes, I'll do that Thursday.

thanks
-- PMM

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 96113fe..2298428 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -500,7 +500,7 @@  static void contextidr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     ARMCPU *cpu = arm_env_get_cpu(env);
 
     if (raw_read(env, ri) != value && !arm_feature(env, ARM_FEATURE_PMSA)
-        && !extended_addresses_enabled(env)) {
+        && !extended_addresses_enabled(env, arm_current_el(env))) {
         /* For VMSA (when not using the LPAE long descriptor page table
          * format) this register includes the ASID, so do a TLB flush.
          * For PMSA it is purely a process ID and no action is needed.
@@ -2162,7 +2162,7 @@  static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
 
     ret = get_phys_addr(env, value, access_type, mmu_idx,
                         &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
-    if (extended_addresses_enabled(env)) {
+    if (extended_addresses_enabled(env, arm_current_el(env))) {
         /* fsr is a DFSR/IFSR value for the long descriptor
          * translation table format, but with WnR always clear.
          * Convert it to a 64-bit PAR.
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 43106a2..6792df2 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -217,10 +217,10 @@  static inline unsigned int arm_pamax(ARMCPU *cpu)
  * This is always the case if our translation regime is 64 bit,
  * but depends on TTBCR.EAE for 32 bit.
  */
-static inline bool extended_addresses_enabled(CPUARMState *env)
+static inline bool extended_addresses_enabled(CPUARMState *env, unsigned int el)
 {
-    TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
-    return arm_el_is_aa64(env, 1) ||
+    TCR *tcr = &env->cp15.tcr_el[el];
+    return arm_el_is_aa64(env, el) ||
            (arm_feature(env, ARM_FEATURE_LPAE) && (tcr->raw_tcr & TTBCR_EAE));
 }
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 3914145..4f46eb8 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -1378,7 +1378,7 @@  void arm_debug_excp_handler(CPUState *cs)
 
             cs->watchpoint_hit = NULL;
 
-            if (extended_addresses_enabled(env)) {
+            if (extended_addresses_enabled(env, arm_debug_target_el(env))) {
                 env->exception.fsr = (1 << 9) | 0x22;
             } else {
                 env->exception.fsr = 0x2;
@@ -1402,7 +1402,7 @@  void arm_debug_excp_handler(CPUState *cs)
             return;
         }
 
-        if (extended_addresses_enabled(env)) {
+        if (extended_addresses_enabled(env, arm_debug_target_el(env))) {
             env->exception.fsr = (1 << 9) | 0x22;
         } else {
             env->exception.fsr = 0x2;