* [PATCH v2 0/3] target-arm: Add a few more S2 MMU input checks @ 2016-01-21 11:18 ` Edgar E. Iglesias 0 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-21 11:18 UTC (permalink / raw) To: qemu-devel, peter.maydell; +Cc: alex.bennee, qemu-arm, edgar.iglesias From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> This adds the inputsize > pamax check and also fixes the startlevel checks to apply to the 64bit translations. Comments welcome! Cheers, Edgar v1 -> v2: * inputsize > pmax check only applies to AArch64 * Fix commit message typo < should be > Edgar E. Iglesias (3): target-arm: Apply S2 MMU startlevel table size check to AArch64 target-arm: Make pamax an argument to check_s2_startlevel target-arm: Implement the S2 MMU inputsize > pamax check target-arm/helper.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v2 0/3] target-arm: Add a few more S2 MMU input checks @ 2016-01-21 11:18 ` Edgar E. Iglesias 0 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-21 11:18 UTC (permalink / raw) To: qemu-devel, peter.maydell; +Cc: edgar.iglesias, qemu-arm, alex.bennee From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> This adds the inputsize > pamax check and also fixes the startlevel checks to apply to the 64bit translations. Comments welcome! Cheers, Edgar v1 -> v2: * inputsize > pmax check only applies to AArch64 * Fix commit message typo < should be > Edgar E. Iglesias (3): target-arm: Apply S2 MMU startlevel table size check to AArch64 target-arm: Make pamax an argument to check_s2_startlevel target-arm: Implement the S2 MMU inputsize > pamax check target-arm/helper.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/3] target-arm: Apply S2 MMU startlevel table size check to AArch64 2016-01-21 11:18 ` [Qemu-devel] " Edgar E. Iglesias @ 2016-01-21 11:18 ` Edgar E. Iglesias -1 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-21 11:18 UTC (permalink / raw) To: qemu-devel, peter.maydell; +Cc: alex.bennee, qemu-arm, edgar.iglesias From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> The S2 starting level table size check applies to both AArch32 and AArch64. Move it to common code. Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> --- target-arm/helper.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/target-arm/helper.c b/target-arm/helper.c index f956b67..8aedce9 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -6581,11 +6581,19 @@ typedef enum { static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, int inputsize, int stride) { + const int grainsize = stride + 3; + int startsizecheck; + /* Negative levels are never allowed. */ if (level < 0) { return false; } + startsizecheck = inputsize - ((3 - level) * stride + grainsize); + if (startsizecheck < 1 || startsizecheck > stride + 4) { + return false; + } + if (is_aa64) { unsigned int pamax = arm_pamax(cpu); @@ -6609,20 +6617,12 @@ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, g_assert_not_reached(); } } else { - const int grainsize = stride + 3; - int startsizecheck; - /* AArch32 only supports 4KB pages. Assert on that. */ assert(stride == 9); if (level == 0) { return false; } - - startsizecheck = inputsize - ((3 - level) * stride + grainsize); - if (startsizecheck < 1 || startsizecheck > stride + 4) { - return false; - } } return true; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v2 1/3] target-arm: Apply S2 MMU startlevel table size check to AArch64 @ 2016-01-21 11:18 ` Edgar E. Iglesias 0 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-21 11:18 UTC (permalink / raw) To: qemu-devel, peter.maydell; +Cc: edgar.iglesias, qemu-arm, alex.bennee From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> The S2 starting level table size check applies to both AArch32 and AArch64. Move it to common code. Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> --- target-arm/helper.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/target-arm/helper.c b/target-arm/helper.c index f956b67..8aedce9 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -6581,11 +6581,19 @@ typedef enum { static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, int inputsize, int stride) { + const int grainsize = stride + 3; + int startsizecheck; + /* Negative levels are never allowed. */ if (level < 0) { return false; } + startsizecheck = inputsize - ((3 - level) * stride + grainsize); + if (startsizecheck < 1 || startsizecheck > stride + 4) { + return false; + } + if (is_aa64) { unsigned int pamax = arm_pamax(cpu); @@ -6609,20 +6617,12 @@ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, g_assert_not_reached(); } } else { - const int grainsize = stride + 3; - int startsizecheck; - /* AArch32 only supports 4KB pages. Assert on that. */ assert(stride == 9); if (level == 0) { return false; } - - startsizecheck = inputsize - ((3 - level) * stride + grainsize); - if (startsizecheck < 1 || startsizecheck > stride + 4) { - return false; - } } return true; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/3] target-arm: Make pamax an argument to check_s2_startlevel 2016-01-21 11:18 ` [Qemu-devel] " Edgar E. Iglesias @ 2016-01-21 11:18 ` Edgar E. Iglesias -1 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-21 11:18 UTC (permalink / raw) To: qemu-devel, peter.maydell; +Cc: alex.bennee, qemu-arm, edgar.iglesias From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> Make pamax an argument to check_s2_startlevel in preparation for future reuse. No functional change. Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> --- target-arm/helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/target-arm/helper.c b/target-arm/helper.c index 8aedce9..4abeb4d 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -6579,7 +6579,8 @@ typedef enum { * Returns true if the suggested starting level is OK and false otherwise. */ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, - int inputsize, int stride) + int inputsize, int stride, + unsigned int pamax) { const int grainsize = stride + 3; int startsizecheck; @@ -6595,8 +6596,6 @@ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, } if (is_aa64) { - unsigned int pamax = arm_pamax(cpu); - switch (stride) { case 13: /* 64KB Pages. */ if (level == 0 || (level == 1 && pamax <= 42)) { @@ -6808,6 +6807,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, * VTCR_EL2.SL0 field (whose interpretation depends on the page size) */ int startlevel = extract32(tcr->raw_tcr, 6, 2); + unsigned int pamax = arm_pamax(cpu); bool ok; if (va_size == 32 || stride == 9) { @@ -6820,7 +6820,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, /* Check that the starting level is valid. */ ok = check_s2_startlevel(cpu, va_size == 64, level, - inputsize, stride); + inputsize, stride, pamax); if (!ok) { /* AArch64 reports these as level 0 faults. * AArch32 reports these as level 1 faults. -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v2 2/3] target-arm: Make pamax an argument to check_s2_startlevel @ 2016-01-21 11:18 ` Edgar E. Iglesias 0 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-21 11:18 UTC (permalink / raw) To: qemu-devel, peter.maydell; +Cc: edgar.iglesias, qemu-arm, alex.bennee From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> Make pamax an argument to check_s2_startlevel in preparation for future reuse. No functional change. Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> --- target-arm/helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/target-arm/helper.c b/target-arm/helper.c index 8aedce9..4abeb4d 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -6579,7 +6579,8 @@ typedef enum { * Returns true if the suggested starting level is OK and false otherwise. */ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, - int inputsize, int stride) + int inputsize, int stride, + unsigned int pamax) { const int grainsize = stride + 3; int startsizecheck; @@ -6595,8 +6596,6 @@ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, } if (is_aa64) { - unsigned int pamax = arm_pamax(cpu); - switch (stride) { case 13: /* 64KB Pages. */ if (level == 0 || (level == 1 && pamax <= 42)) { @@ -6808,6 +6807,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, * VTCR_EL2.SL0 field (whose interpretation depends on the page size) */ int startlevel = extract32(tcr->raw_tcr, 6, 2); + unsigned int pamax = arm_pamax(cpu); bool ok; if (va_size == 32 || stride == 9) { @@ -6820,7 +6820,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, /* Check that the starting level is valid. */ ok = check_s2_startlevel(cpu, va_size == 64, level, - inputsize, stride); + inputsize, stride, pamax); if (!ok) { /* AArch64 reports these as level 0 faults. * AArch32 reports these as level 1 faults. -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/3] target-arm: Make pamax an argument to check_s2_startlevel 2016-01-21 11:18 ` [Qemu-devel] " Edgar E. Iglesias @ 2016-01-21 15:54 ` Alex Bennée -1 siblings, 0 replies; 20+ messages in thread From: Alex Bennée @ 2016-01-21 15:54 UTC (permalink / raw) To: Edgar E. Iglesias; +Cc: qemu-devel, peter.maydell, qemu-arm, edgar.iglesias Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Make pamax an argument to check_s2_startlevel in preparation > for future reuse. > > No functional change. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target-arm/helper.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 8aedce9..4abeb4d 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -6579,7 +6579,8 @@ typedef enum { > * Returns true if the suggested starting level is OK and false otherwise. > */ > static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, > - int inputsize, int stride) > + int inputsize, int stride, > + unsigned int pamax) > { If you fix the docstring above you can have my: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > const int grainsize = stride + 3; > int startsizecheck; > @@ -6595,8 +6596,6 @@ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, > } > > if (is_aa64) { > - unsigned int pamax = arm_pamax(cpu); > - > switch (stride) { > case 13: /* 64KB Pages. */ > if (level == 0 || (level == 1 && pamax <= 42)) { > @@ -6808,6 +6807,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > * VTCR_EL2.SL0 field (whose interpretation depends on the page size) > */ > int startlevel = extract32(tcr->raw_tcr, 6, 2); > + unsigned int pamax = arm_pamax(cpu); > bool ok; > > if (va_size == 32 || stride == 9) { > @@ -6820,7 +6820,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > /* Check that the starting level is valid. */ > ok = check_s2_startlevel(cpu, va_size == 64, level, > - inputsize, stride); > + inputsize, stride, pamax); > if (!ok) { > /* AArch64 reports these as level 0 faults. > * AArch32 reports these as level 1 faults. -- Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/3] target-arm: Make pamax an argument to check_s2_startlevel @ 2016-01-21 15:54 ` Alex Bennée 0 siblings, 0 replies; 20+ messages in thread From: Alex Bennée @ 2016-01-21 15:54 UTC (permalink / raw) To: Edgar E. Iglesias; +Cc: edgar.iglesias, peter.maydell, qemu-arm, qemu-devel Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Make pamax an argument to check_s2_startlevel in preparation > for future reuse. > > No functional change. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target-arm/helper.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 8aedce9..4abeb4d 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -6579,7 +6579,8 @@ typedef enum { > * Returns true if the suggested starting level is OK and false otherwise. > */ > static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, > - int inputsize, int stride) > + int inputsize, int stride, > + unsigned int pamax) > { If you fix the docstring above you can have my: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > const int grainsize = stride + 3; > int startsizecheck; > @@ -6595,8 +6596,6 @@ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, > } > > if (is_aa64) { > - unsigned int pamax = arm_pamax(cpu); > - > switch (stride) { > case 13: /* 64KB Pages. */ > if (level == 0 || (level == 1 && pamax <= 42)) { > @@ -6808,6 +6807,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > * VTCR_EL2.SL0 field (whose interpretation depends on the page size) > */ > int startlevel = extract32(tcr->raw_tcr, 6, 2); > + unsigned int pamax = arm_pamax(cpu); > bool ok; > > if (va_size == 32 || stride == 9) { > @@ -6820,7 +6820,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > /* Check that the starting level is valid. */ > ok = check_s2_startlevel(cpu, va_size == 64, level, > - inputsize, stride); > + inputsize, stride, pamax); > if (!ok) { > /* AArch64 reports these as level 0 faults. > * AArch32 reports these as level 1 faults. -- Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/3] target-arm: Make pamax an argument to check_s2_startlevel 2016-01-21 15:54 ` [Qemu-devel] " Alex Bennée @ 2016-01-21 16:07 ` Edgar E. Iglesias -1 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-21 16:07 UTC (permalink / raw) To: Alex Bennée; +Cc: qemu-devel, peter.maydell, qemu-arm, edgar.iglesias On Thu, Jan 21, 2016 at 03:54:54PM +0000, Alex Bennée wrote: > > Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Make pamax an argument to check_s2_startlevel in preparation > > for future reuse. > > > > No functional change. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target-arm/helper.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index 8aedce9..4abeb4d 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -6579,7 +6579,8 @@ typedef enum { > > * Returns true if the suggested starting level is OK and false otherwise. > > */ > > static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, > > - int inputsize, int stride) > > + int inputsize, int stride, > > + unsigned int pamax) > > { > > If you fix the docstring above you can have my: > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> I've fixed the comment for v3. Thanks, Edgar > > > > const int grainsize = stride + 3; > > int startsizecheck; > > @@ -6595,8 +6596,6 @@ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, > > } > > > > if (is_aa64) { > > - unsigned int pamax = arm_pamax(cpu); > > - > > switch (stride) { > > case 13: /* 64KB Pages. */ > > if (level == 0 || (level == 1 && pamax <= 42)) { > > @@ -6808,6 +6807,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > * VTCR_EL2.SL0 field (whose interpretation depends on the page size) > > */ > > int startlevel = extract32(tcr->raw_tcr, 6, 2); > > + unsigned int pamax = arm_pamax(cpu); > > bool ok; > > > > if (va_size == 32 || stride == 9) { > > @@ -6820,7 +6820,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > > > /* Check that the starting level is valid. */ > > ok = check_s2_startlevel(cpu, va_size == 64, level, > > - inputsize, stride); > > + inputsize, stride, pamax); > > if (!ok) { > > /* AArch64 reports these as level 0 faults. > > * AArch32 reports these as level 1 faults. > > > -- > Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/3] target-arm: Make pamax an argument to check_s2_startlevel @ 2016-01-21 16:07 ` Edgar E. Iglesias 0 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-21 16:07 UTC (permalink / raw) To: Alex Bennée; +Cc: edgar.iglesias, peter.maydell, qemu-arm, qemu-devel On Thu, Jan 21, 2016 at 03:54:54PM +0000, Alex Bennée wrote: > > Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Make pamax an argument to check_s2_startlevel in preparation > > for future reuse. > > > > No functional change. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target-arm/helper.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index 8aedce9..4abeb4d 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -6579,7 +6579,8 @@ typedef enum { > > * Returns true if the suggested starting level is OK and false otherwise. > > */ > > static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, > > - int inputsize, int stride) > > + int inputsize, int stride, > > + unsigned int pamax) > > { > > If you fix the docstring above you can have my: > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> I've fixed the comment for v3. Thanks, Edgar > > > > const int grainsize = stride + 3; > > int startsizecheck; > > @@ -6595,8 +6596,6 @@ static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int level, > > } > > > > if (is_aa64) { > > - unsigned int pamax = arm_pamax(cpu); > > - > > switch (stride) { > > case 13: /* 64KB Pages. */ > > if (level == 0 || (level == 1 && pamax <= 42)) { > > @@ -6808,6 +6807,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > * VTCR_EL2.SL0 field (whose interpretation depends on the page size) > > */ > > int startlevel = extract32(tcr->raw_tcr, 6, 2); > > + unsigned int pamax = arm_pamax(cpu); > > bool ok; > > > > if (va_size == 32 || stride == 9) { > > @@ -6820,7 +6820,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > > > /* Check that the starting level is valid. */ > > ok = check_s2_startlevel(cpu, va_size == 64, level, > > - inputsize, stride); > > + inputsize, stride, pamax); > > if (!ok) { > > /* AArch64 reports these as level 0 faults. > > * AArch32 reports these as level 1 faults. > > > -- > Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check 2016-01-21 11:18 ` [Qemu-devel] " Edgar E. Iglesias @ 2016-01-21 11:18 ` Edgar E. Iglesias -1 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-21 11:18 UTC (permalink / raw) To: qemu-devel, peter.maydell; +Cc: alex.bennee, qemu-arm, edgar.iglesias From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> Implement the inputsize > pamax check for Stage 2 translations. We have multiple choices for how to respond to errors and choose to fault. Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> --- target-arm/helper.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/target-arm/helper.c b/target-arm/helper.c index 4abeb4d..9a7ff5e 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -6808,7 +6808,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, */ int startlevel = extract32(tcr->raw_tcr, 6, 2); unsigned int pamax = arm_pamax(cpu); - bool ok; + bool ok = true; if (va_size == 32 || stride == 9) { /* AArch32 or 4KB pages */ @@ -6818,9 +6818,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, level = 3 - startlevel; } - /* Check that the starting level is valid. */ - ok = check_s2_startlevel(cpu, va_size == 64, level, - inputsize, stride, pamax); + if (va_size == 64 && + inputsize > pamax && + (arm_el_is_aa64(env, 1) || inputsize > 40)) { + /* We have multiple choices but choose to fault. */ + ok = false; + } + if (ok) { + /* Check that the starting level is valid. */ + ok = check_s2_startlevel(cpu, va_size == 64, level, + inputsize, stride, pamax); + } if (!ok) { /* AArch64 reports these as level 0 faults. * AArch32 reports these as level 1 faults. -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check @ 2016-01-21 11:18 ` Edgar E. Iglesias 0 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-21 11:18 UTC (permalink / raw) To: qemu-devel, peter.maydell; +Cc: edgar.iglesias, qemu-arm, alex.bennee From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> Implement the inputsize > pamax check for Stage 2 translations. We have multiple choices for how to respond to errors and choose to fault. Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> --- target-arm/helper.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/target-arm/helper.c b/target-arm/helper.c index 4abeb4d..9a7ff5e 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -6808,7 +6808,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, */ int startlevel = extract32(tcr->raw_tcr, 6, 2); unsigned int pamax = arm_pamax(cpu); - bool ok; + bool ok = true; if (va_size == 32 || stride == 9) { /* AArch32 or 4KB pages */ @@ -6818,9 +6818,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, level = 3 - startlevel; } - /* Check that the starting level is valid. */ - ok = check_s2_startlevel(cpu, va_size == 64, level, - inputsize, stride, pamax); + if (va_size == 64 && + inputsize > pamax && + (arm_el_is_aa64(env, 1) || inputsize > 40)) { + /* We have multiple choices but choose to fault. */ + ok = false; + } + if (ok) { + /* Check that the starting level is valid. */ + ok = check_s2_startlevel(cpu, va_size == 64, level, + inputsize, stride, pamax); + } if (!ok) { /* AArch64 reports these as level 0 faults. * AArch32 reports these as level 1 faults. -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check 2016-01-21 11:18 ` [Qemu-devel] " Edgar E. Iglesias @ 2016-01-22 10:28 ` Alex Bennée -1 siblings, 0 replies; 20+ messages in thread From: Alex Bennée @ 2016-01-22 10:28 UTC (permalink / raw) To: Edgar E. Iglesias; +Cc: qemu-devel, peter.maydell, qemu-arm, edgar.iglesias Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Implement the inputsize > pamax check for Stage 2 translations. > We have multiple choices for how to respond to errors and > choose to fault. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target-arm/helper.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 4abeb4d..9a7ff5e 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -6808,7 +6808,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > */ > int startlevel = extract32(tcr->raw_tcr, 6, 2); > unsigned int pamax = arm_pamax(cpu); > - bool ok; > + bool ok = true; > > if (va_size == 32 || stride == 9) { > /* AArch32 or 4KB pages */ > @@ -6818,9 +6818,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > level = 3 - startlevel; > } > > - /* Check that the starting level is valid. */ > - ok = check_s2_startlevel(cpu, va_size == 64, level, > - inputsize, stride, pamax); > + if (va_size == 64 && > + inputsize > pamax && > + (arm_el_is_aa64(env, 1) || inputsize > 40)) { If va_size == 64 doesn't that imply arm_el_is_aa64(env, 1)? Looking further up the function it seems that is what sets va_size in the first place. I think that makes the inputsize > 40 check redundant. > + /* We have multiple choices but choose to fault. */ > + ok = false; > + } > + if (ok) { > + /* Check that the starting level is valid. */ > + ok = check_s2_startlevel(cpu, va_size == 64, level, > + inputsize, stride, pamax); > + } > if (!ok) { > /* AArch64 reports these as level 0 faults. > * AArch32 reports these as level 1 faults. I'm not a fan of the ok = true / ok = false / ok = check_s2_start_level() / if (!ok) ping-pong here as it is hard to follow. I'm not sure how you could make it cleaner to follow though. Maybe something like: /* For stage 2 translations the starting level is specified by the * VTCR_EL2.SL0 field (whose interpretation depends on the page size) */ int startlevel = extract32(tcr->raw_tcr, 6, 2); unsigned int pamax = arm_pamax(cpu); bool is_aarch64_regime = (va_size == 64); bool ok; if (va_size == 32 || stride == 9) { /* AArch32 or 4KB pages */ level = 2 - startlevel; } else { /* 16KB or 64KB pages */ level = 3 - startlevel; } if (is_aarch64_regime && inputsize > pamax) { /* We have multiple choices but choose to fault. */ ok = false; } else { /* Check that the starting level is valid. */ ok = check_s2_startlevel(cpu, is_aarch64_regime, level, inputsize, stride, pamax); } if (!ok) { /* AArch64 reports these as level 0 faults. * AArch32 reports these as level 1 faults. */ level = is_aarch64_regime ? 0 : 1; fault_type = translation_fault; goto do_fault; } But I'm wondering if it just makes more sense to push the: is_aarch64_regime && inputsize > pamax Check into check_s2_startlevel? Then you could just have a simple call which succeeds or falls through to a fault? /* Check that the starting level is valid. */ if (!check_s2_startlevel(cpu, is_aarch64_regime, level, inputsize, stride, pamax) ){ /* AArch64 reports these as level 0 faults. * AArch32 reports these as level 1 faults. */ level = is_aarch64_regime ? 0 : 1; fault_type = translation_fault; goto do_fault; } -- Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check @ 2016-01-22 10:28 ` Alex Bennée 0 siblings, 0 replies; 20+ messages in thread From: Alex Bennée @ 2016-01-22 10:28 UTC (permalink / raw) To: Edgar E. Iglesias; +Cc: edgar.iglesias, peter.maydell, qemu-arm, qemu-devel Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Implement the inputsize > pamax check for Stage 2 translations. > We have multiple choices for how to respond to errors and > choose to fault. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target-arm/helper.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 4abeb4d..9a7ff5e 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -6808,7 +6808,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > */ > int startlevel = extract32(tcr->raw_tcr, 6, 2); > unsigned int pamax = arm_pamax(cpu); > - bool ok; > + bool ok = true; > > if (va_size == 32 || stride == 9) { > /* AArch32 or 4KB pages */ > @@ -6818,9 +6818,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > level = 3 - startlevel; > } > > - /* Check that the starting level is valid. */ > - ok = check_s2_startlevel(cpu, va_size == 64, level, > - inputsize, stride, pamax); > + if (va_size == 64 && > + inputsize > pamax && > + (arm_el_is_aa64(env, 1) || inputsize > 40)) { If va_size == 64 doesn't that imply arm_el_is_aa64(env, 1)? Looking further up the function it seems that is what sets va_size in the first place. I think that makes the inputsize > 40 check redundant. > + /* We have multiple choices but choose to fault. */ > + ok = false; > + } > + if (ok) { > + /* Check that the starting level is valid. */ > + ok = check_s2_startlevel(cpu, va_size == 64, level, > + inputsize, stride, pamax); > + } > if (!ok) { > /* AArch64 reports these as level 0 faults. > * AArch32 reports these as level 1 faults. I'm not a fan of the ok = true / ok = false / ok = check_s2_start_level() / if (!ok) ping-pong here as it is hard to follow. I'm not sure how you could make it cleaner to follow though. Maybe something like: /* For stage 2 translations the starting level is specified by the * VTCR_EL2.SL0 field (whose interpretation depends on the page size) */ int startlevel = extract32(tcr->raw_tcr, 6, 2); unsigned int pamax = arm_pamax(cpu); bool is_aarch64_regime = (va_size == 64); bool ok; if (va_size == 32 || stride == 9) { /* AArch32 or 4KB pages */ level = 2 - startlevel; } else { /* 16KB or 64KB pages */ level = 3 - startlevel; } if (is_aarch64_regime && inputsize > pamax) { /* We have multiple choices but choose to fault. */ ok = false; } else { /* Check that the starting level is valid. */ ok = check_s2_startlevel(cpu, is_aarch64_regime, level, inputsize, stride, pamax); } if (!ok) { /* AArch64 reports these as level 0 faults. * AArch32 reports these as level 1 faults. */ level = is_aarch64_regime ? 0 : 1; fault_type = translation_fault; goto do_fault; } But I'm wondering if it just makes more sense to push the: is_aarch64_regime && inputsize > pamax Check into check_s2_startlevel? Then you could just have a simple call which succeeds or falls through to a fault? /* Check that the starting level is valid. */ if (!check_s2_startlevel(cpu, is_aarch64_regime, level, inputsize, stride, pamax) ){ /* AArch64 reports these as level 0 faults. * AArch32 reports these as level 1 faults. */ level = is_aarch64_regime ? 0 : 1; fault_type = translation_fault; goto do_fault; } -- Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check 2016-01-22 10:28 ` [Qemu-devel] " Alex Bennée @ 2016-01-22 11:16 ` Edgar E. Iglesias -1 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-22 11:16 UTC (permalink / raw) To: Alex Bennée; +Cc: qemu-devel, peter.maydell, qemu-arm, edgar.iglesias On Fri, Jan 22, 2016 at 10:28:43AM +0000, Alex Bennée wrote: > > Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Implement the inputsize > pamax check for Stage 2 translations. > > We have multiple choices for how to respond to errors and > > choose to fault. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target-arm/helper.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index 4abeb4d..9a7ff5e 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -6808,7 +6808,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > */ > > int startlevel = extract32(tcr->raw_tcr, 6, 2); > > unsigned int pamax = arm_pamax(cpu); > > - bool ok; > > + bool ok = true; > > > > if (va_size == 32 || stride == 9) { > > /* AArch32 or 4KB pages */ > > @@ -6818,9 +6818,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > level = 3 - startlevel; > > } > > > > - /* Check that the starting level is valid. */ > > - ok = check_s2_startlevel(cpu, va_size == 64, level, > > - inputsize, stride, pamax); > > + if (va_size == 64 && > > + inputsize > pamax && > > + (arm_el_is_aa64(env, 1) || inputsize > 40)) { > > If va_size == 64 doesn't that imply arm_el_is_aa64(env, 1)? Looking > further up the function it seems that is what sets va_size in the first > place. I think that makes the inputsize > 40 check redundant. va_size == 64 is true if the EL corresponding to the translation _regime_ is in 64 bit mode (in this case EL2). EL1 may very well be in 32bit mode. > > > + /* We have multiple choices but choose to fault. */ > > + ok = false; > > + } > > + if (ok) { > > + /* Check that the starting level is valid. */ > > + ok = check_s2_startlevel(cpu, va_size == 64, level, > > + inputsize, stride, pamax); > > + } > > if (!ok) { > > /* AArch64 reports these as level 0 faults. > > * AArch32 reports these as level 1 faults. > > I'm not a fan of the ok = true / ok = false / ok = > check_s2_start_level() / if (!ok) ping-pong here as it is hard to > follow. I'm not sure how you could make it cleaner to follow though. > Maybe something like: > > /* For stage 2 translations the starting level is specified by the > * VTCR_EL2.SL0 field (whose interpretation depends on the page size) > */ > int startlevel = extract32(tcr->raw_tcr, 6, 2); > unsigned int pamax = arm_pamax(cpu); > bool is_aarch64_regime = (va_size == 64); > bool ok; > > if (va_size == 32 || stride == 9) { > /* AArch32 or 4KB pages */ > level = 2 - startlevel; > } else { > /* 16KB or 64KB pages */ > level = 3 - startlevel; > } > > if (is_aarch64_regime && > inputsize > pamax) { > /* We have multiple choices but choose to fault. */ > ok = false; > } else { > /* Check that the starting level is valid. */ > ok = check_s2_startlevel(cpu, is_aarch64_regime, level, > inputsize, stride, pamax); > } > if (!ok) { > /* AArch64 reports these as level 0 faults. > * AArch32 reports these as level 1 faults. > */ > level = is_aarch64_regime ? 0 : 1; > fault_type = translation_fault; > goto do_fault; > } > > But I'm wondering if it just makes more sense to push the: > > is_aarch64_regime && inputsize > pamax > > Check into check_s2_startlevel? Then you could just have a simple call > which succeeds or falls through to a fault? Yeah, I guess we could rename check_s2_startlevel to something more generic and move all the checks there. I don't feel very strongly about either way... Thanks, Edgar > > /* Check that the starting level is valid. */ > if (!check_s2_startlevel(cpu, is_aarch64_regime, level, > inputsize, stride, pamax) ){ > /* AArch64 reports these as level 0 faults. > * AArch32 reports these as level 1 faults. > */ > level = is_aarch64_regime ? 0 : 1; > fault_type = translation_fault; > goto do_fault; > } > > -- > Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check @ 2016-01-22 11:16 ` Edgar E. Iglesias 0 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-22 11:16 UTC (permalink / raw) To: Alex Bennée; +Cc: edgar.iglesias, peter.maydell, qemu-arm, qemu-devel On Fri, Jan 22, 2016 at 10:28:43AM +0000, Alex Bennée wrote: > > Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Implement the inputsize > pamax check for Stage 2 translations. > > We have multiple choices for how to respond to errors and > > choose to fault. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target-arm/helper.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index 4abeb4d..9a7ff5e 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -6808,7 +6808,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > */ > > int startlevel = extract32(tcr->raw_tcr, 6, 2); > > unsigned int pamax = arm_pamax(cpu); > > - bool ok; > > + bool ok = true; > > > > if (va_size == 32 || stride == 9) { > > /* AArch32 or 4KB pages */ > > @@ -6818,9 +6818,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > level = 3 - startlevel; > > } > > > > - /* Check that the starting level is valid. */ > > - ok = check_s2_startlevel(cpu, va_size == 64, level, > > - inputsize, stride, pamax); > > + if (va_size == 64 && > > + inputsize > pamax && > > + (arm_el_is_aa64(env, 1) || inputsize > 40)) { > > If va_size == 64 doesn't that imply arm_el_is_aa64(env, 1)? Looking > further up the function it seems that is what sets va_size in the first > place. I think that makes the inputsize > 40 check redundant. va_size == 64 is true if the EL corresponding to the translation _regime_ is in 64 bit mode (in this case EL2). EL1 may very well be in 32bit mode. > > > + /* We have multiple choices but choose to fault. */ > > + ok = false; > > + } > > + if (ok) { > > + /* Check that the starting level is valid. */ > > + ok = check_s2_startlevel(cpu, va_size == 64, level, > > + inputsize, stride, pamax); > > + } > > if (!ok) { > > /* AArch64 reports these as level 0 faults. > > * AArch32 reports these as level 1 faults. > > I'm not a fan of the ok = true / ok = false / ok = > check_s2_start_level() / if (!ok) ping-pong here as it is hard to > follow. I'm not sure how you could make it cleaner to follow though. > Maybe something like: > > /* For stage 2 translations the starting level is specified by the > * VTCR_EL2.SL0 field (whose interpretation depends on the page size) > */ > int startlevel = extract32(tcr->raw_tcr, 6, 2); > unsigned int pamax = arm_pamax(cpu); > bool is_aarch64_regime = (va_size == 64); > bool ok; > > if (va_size == 32 || stride == 9) { > /* AArch32 or 4KB pages */ > level = 2 - startlevel; > } else { > /* 16KB or 64KB pages */ > level = 3 - startlevel; > } > > if (is_aarch64_regime && > inputsize > pamax) { > /* We have multiple choices but choose to fault. */ > ok = false; > } else { > /* Check that the starting level is valid. */ > ok = check_s2_startlevel(cpu, is_aarch64_regime, level, > inputsize, stride, pamax); > } > if (!ok) { > /* AArch64 reports these as level 0 faults. > * AArch32 reports these as level 1 faults. > */ > level = is_aarch64_regime ? 0 : 1; > fault_type = translation_fault; > goto do_fault; > } > > But I'm wondering if it just makes more sense to push the: > > is_aarch64_regime && inputsize > pamax > > Check into check_s2_startlevel? Then you could just have a simple call > which succeeds or falls through to a fault? Yeah, I guess we could rename check_s2_startlevel to something more generic and move all the checks there. I don't feel very strongly about either way... Thanks, Edgar > > /* Check that the starting level is valid. */ > if (!check_s2_startlevel(cpu, is_aarch64_regime, level, > inputsize, stride, pamax) ){ > /* AArch64 reports these as level 0 faults. > * AArch32 reports these as level 1 faults. > */ > level = is_aarch64_regime ? 0 : 1; > fault_type = translation_fault; > goto do_fault; > } > > -- > Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check 2016-01-22 11:16 ` [Qemu-devel] " Edgar E. Iglesias @ 2016-01-22 11:45 ` Alex Bennée -1 siblings, 0 replies; 20+ messages in thread From: Alex Bennée @ 2016-01-22 11:45 UTC (permalink / raw) To: Edgar E. Iglesias; +Cc: qemu-devel, peter.maydell, qemu-arm, edgar.iglesias Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > On Fri, Jan 22, 2016 at 10:28:43AM +0000, Alex Bennée wrote: >> >> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: >> >> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> >> > >> > Implement the inputsize > pamax check for Stage 2 translations. >> > We have multiple choices for how to respond to errors and >> > choose to fault. >> > >> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> >> > --- >> > target-arm/helper.c | 16 ++++++++++++---- >> > 1 file changed, 12 insertions(+), 4 deletions(-) >> > >> > diff --git a/target-arm/helper.c b/target-arm/helper.c >> > index 4abeb4d..9a7ff5e 100644 >> > --- a/target-arm/helper.c >> > +++ b/target-arm/helper.c >> > @@ -6808,7 +6808,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, >> > */ >> > int startlevel = extract32(tcr->raw_tcr, 6, 2); >> > unsigned int pamax = arm_pamax(cpu); >> > - bool ok; >> > + bool ok = true; >> > >> > if (va_size == 32 || stride == 9) { >> > /* AArch32 or 4KB pages */ >> > @@ -6818,9 +6818,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, >> > level = 3 - startlevel; >> > } >> > >> > - /* Check that the starting level is valid. */ >> > - ok = check_s2_startlevel(cpu, va_size == 64, level, >> > - inputsize, stride, pamax); >> > + if (va_size == 64 && >> > + inputsize > pamax && >> > + (arm_el_is_aa64(env, 1) || inputsize > 40)) { >> >> If va_size == 64 doesn't that imply arm_el_is_aa64(env, 1)? Looking >> further up the function it seems that is what sets va_size in the first >> place. I think that makes the inputsize > 40 check redundant. > > va_size == 64 is true if the EL corresponding to the translation _regime_ > is in 64 bit mode (in this case EL2). > > EL1 may very well be in 32bit mode. Ahh yes, I missed that on the first reading. I think it might be clearer when reading the code to have the: bool is_aarch64_regime = (va_size == 64); And use that to make it clear. And then comment on later check that it's incompatible with EL1 being aarch32. > >> >> > + /* We have multiple choices but choose to fault. */ >> > + ok = false; >> > + } >> > + if (ok) { >> > + /* Check that the starting level is valid. */ >> > + ok = check_s2_startlevel(cpu, va_size == 64, level, >> > + inputsize, stride, pamax); >> > + } >> > if (!ok) { >> > /* AArch64 reports these as level 0 faults. >> > * AArch32 reports these as level 1 faults. >> >> I'm not a fan of the ok = true / ok = false / ok = >> check_s2_start_level() / if (!ok) ping-pong here as it is hard to >> follow. I'm not sure how you could make it cleaner to follow though. >> Maybe something like: >> >> /* For stage 2 translations the starting level is specified by the >> * VTCR_EL2.SL0 field (whose interpretation depends on the page size) >> */ >> int startlevel = extract32(tcr->raw_tcr, 6, 2); >> unsigned int pamax = arm_pamax(cpu); >> bool is_aarch64_regime = (va_size == 64); >> bool ok; >> >> if (va_size == 32 || stride == 9) { >> /* AArch32 or 4KB pages */ >> level = 2 - startlevel; >> } else { >> /* 16KB or 64KB pages */ >> level = 3 - startlevel; >> } >> >> if (is_aarch64_regime && >> inputsize > pamax) { >> /* We have multiple choices but choose to fault. */ >> ok = false; >> } else { >> /* Check that the starting level is valid. */ >> ok = check_s2_startlevel(cpu, is_aarch64_regime, level, >> inputsize, stride, pamax); >> } >> if (!ok) { >> /* AArch64 reports these as level 0 faults. >> * AArch32 reports these as level 1 faults. >> */ >> level = is_aarch64_regime ? 0 : 1; >> fault_type = translation_fault; >> goto do_fault; >> } >> >> But I'm wondering if it just makes more sense to push the: >> >> is_aarch64_regime && inputsize > pamax >> >> Check into check_s2_startlevel? Then you could just have a simple call >> which succeeds or falls through to a fault? > > Yeah, I guess we could rename check_s2_startlevel to something more generic > and move all the checks there. I don't feel very strongly about either way... I think it would be cleaner to follow. get_phys_addr_lpae is already a bit of a monster so the less conditions to keep track of while reading it the better IMHO. > Thanks, > Edgar > > > >> >> /* Check that the starting level is valid. */ >> if (!check_s2_startlevel(cpu, is_aarch64_regime, level, >> inputsize, stride, pamax) ){ >> /* AArch64 reports these as level 0 faults. >> * AArch32 reports these as level 1 faults. >> */ >> level = is_aarch64_regime ? 0 : 1; >> fault_type = translation_fault; >> goto do_fault; >> } >> >> -- >> Alex Bennée -- Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check @ 2016-01-22 11:45 ` Alex Bennée 0 siblings, 0 replies; 20+ messages in thread From: Alex Bennée @ 2016-01-22 11:45 UTC (permalink / raw) To: Edgar E. Iglesias; +Cc: edgar.iglesias, peter.maydell, qemu-arm, qemu-devel Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > On Fri, Jan 22, 2016 at 10:28:43AM +0000, Alex Bennée wrote: >> >> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: >> >> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> >> > >> > Implement the inputsize > pamax check for Stage 2 translations. >> > We have multiple choices for how to respond to errors and >> > choose to fault. >> > >> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> >> > --- >> > target-arm/helper.c | 16 ++++++++++++---- >> > 1 file changed, 12 insertions(+), 4 deletions(-) >> > >> > diff --git a/target-arm/helper.c b/target-arm/helper.c >> > index 4abeb4d..9a7ff5e 100644 >> > --- a/target-arm/helper.c >> > +++ b/target-arm/helper.c >> > @@ -6808,7 +6808,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, >> > */ >> > int startlevel = extract32(tcr->raw_tcr, 6, 2); >> > unsigned int pamax = arm_pamax(cpu); >> > - bool ok; >> > + bool ok = true; >> > >> > if (va_size == 32 || stride == 9) { >> > /* AArch32 or 4KB pages */ >> > @@ -6818,9 +6818,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, >> > level = 3 - startlevel; >> > } >> > >> > - /* Check that the starting level is valid. */ >> > - ok = check_s2_startlevel(cpu, va_size == 64, level, >> > - inputsize, stride, pamax); >> > + if (va_size == 64 && >> > + inputsize > pamax && >> > + (arm_el_is_aa64(env, 1) || inputsize > 40)) { >> >> If va_size == 64 doesn't that imply arm_el_is_aa64(env, 1)? Looking >> further up the function it seems that is what sets va_size in the first >> place. I think that makes the inputsize > 40 check redundant. > > va_size == 64 is true if the EL corresponding to the translation _regime_ > is in 64 bit mode (in this case EL2). > > EL1 may very well be in 32bit mode. Ahh yes, I missed that on the first reading. I think it might be clearer when reading the code to have the: bool is_aarch64_regime = (va_size == 64); And use that to make it clear. And then comment on later check that it's incompatible with EL1 being aarch32. > >> >> > + /* We have multiple choices but choose to fault. */ >> > + ok = false; >> > + } >> > + if (ok) { >> > + /* Check that the starting level is valid. */ >> > + ok = check_s2_startlevel(cpu, va_size == 64, level, >> > + inputsize, stride, pamax); >> > + } >> > if (!ok) { >> > /* AArch64 reports these as level 0 faults. >> > * AArch32 reports these as level 1 faults. >> >> I'm not a fan of the ok = true / ok = false / ok = >> check_s2_start_level() / if (!ok) ping-pong here as it is hard to >> follow. I'm not sure how you could make it cleaner to follow though. >> Maybe something like: >> >> /* For stage 2 translations the starting level is specified by the >> * VTCR_EL2.SL0 field (whose interpretation depends on the page size) >> */ >> int startlevel = extract32(tcr->raw_tcr, 6, 2); >> unsigned int pamax = arm_pamax(cpu); >> bool is_aarch64_regime = (va_size == 64); >> bool ok; >> >> if (va_size == 32 || stride == 9) { >> /* AArch32 or 4KB pages */ >> level = 2 - startlevel; >> } else { >> /* 16KB or 64KB pages */ >> level = 3 - startlevel; >> } >> >> if (is_aarch64_regime && >> inputsize > pamax) { >> /* We have multiple choices but choose to fault. */ >> ok = false; >> } else { >> /* Check that the starting level is valid. */ >> ok = check_s2_startlevel(cpu, is_aarch64_regime, level, >> inputsize, stride, pamax); >> } >> if (!ok) { >> /* AArch64 reports these as level 0 faults. >> * AArch32 reports these as level 1 faults. >> */ >> level = is_aarch64_regime ? 0 : 1; >> fault_type = translation_fault; >> goto do_fault; >> } >> >> But I'm wondering if it just makes more sense to push the: >> >> is_aarch64_regime && inputsize > pamax >> >> Check into check_s2_startlevel? Then you could just have a simple call >> which succeeds or falls through to a fault? > > Yeah, I guess we could rename check_s2_startlevel to something more generic > and move all the checks there. I don't feel very strongly about either way... I think it would be cleaner to follow. get_phys_addr_lpae is already a bit of a monster so the less conditions to keep track of while reading it the better IMHO. > Thanks, > Edgar > > > >> >> /* Check that the starting level is valid. */ >> if (!check_s2_startlevel(cpu, is_aarch64_regime, level, >> inputsize, stride, pamax) ){ >> /* AArch64 reports these as level 0 faults. >> * AArch32 reports these as level 1 faults. >> */ >> level = is_aarch64_regime ? 0 : 1; >> fault_type = translation_fault; >> goto do_fault; >> } >> >> -- >> Alex Bennée -- Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check 2016-01-22 11:45 ` [Qemu-devel] " Alex Bennée @ 2016-01-22 12:54 ` Edgar E. Iglesias -1 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-22 12:54 UTC (permalink / raw) To: Alex Bennée; +Cc: qemu-devel, peter.maydell, qemu-arm, edgar.iglesias On Fri, Jan 22, 2016 at 11:45:57AM +0000, Alex Bennée wrote: > > Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > > > On Fri, Jan 22, 2016 at 10:28:43AM +0000, Alex Bennée wrote: > >> > >> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > >> > >> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > >> > > >> > Implement the inputsize > pamax check for Stage 2 translations. > >> > We have multiple choices for how to respond to errors and > >> > choose to fault. > >> > > >> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > >> > --- > >> > target-arm/helper.c | 16 ++++++++++++---- > >> > 1 file changed, 12 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/target-arm/helper.c b/target-arm/helper.c > >> > index 4abeb4d..9a7ff5e 100644 > >> > --- a/target-arm/helper.c > >> > +++ b/target-arm/helper.c > >> > @@ -6808,7 +6808,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > >> > */ > >> > int startlevel = extract32(tcr->raw_tcr, 6, 2); > >> > unsigned int pamax = arm_pamax(cpu); > >> > - bool ok; > >> > + bool ok = true; > >> > > >> > if (va_size == 32 || stride == 9) { > >> > /* AArch32 or 4KB pages */ > >> > @@ -6818,9 +6818,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > >> > level = 3 - startlevel; > >> > } > >> > > >> > - /* Check that the starting level is valid. */ > >> > - ok = check_s2_startlevel(cpu, va_size == 64, level, > >> > - inputsize, stride, pamax); > >> > + if (va_size == 64 && > >> > + inputsize > pamax && > >> > + (arm_el_is_aa64(env, 1) || inputsize > 40)) { > >> > >> If va_size == 64 doesn't that imply arm_el_is_aa64(env, 1)? Looking > >> further up the function it seems that is what sets va_size in the first > >> place. I think that makes the inputsize > 40 check redundant. > > > > va_size == 64 is true if the EL corresponding to the translation _regime_ > > is in 64 bit mode (in this case EL2). > > > > EL1 may very well be in 32bit mode. > > Ahh yes, I missed that on the first reading. I think it might be clearer > when reading the code to have the: > > bool is_aarch64_regime = (va_size == 64); > > And use that to make it clear. And then comment on later check that it's > incompatible with EL1 being aarch32. > > > > >> > >> > + /* We have multiple choices but choose to fault. */ > >> > + ok = false; > >> > + } > >> > + if (ok) { > >> > + /* Check that the starting level is valid. */ > >> > + ok = check_s2_startlevel(cpu, va_size == 64, level, > >> > + inputsize, stride, pamax); > >> > + } > >> > if (!ok) { > >> > /* AArch64 reports these as level 0 faults. > >> > * AArch32 reports these as level 1 faults. > >> > >> I'm not a fan of the ok = true / ok = false / ok = > >> check_s2_start_level() / if (!ok) ping-pong here as it is hard to > >> follow. I'm not sure how you could make it cleaner to follow though. > >> Maybe something like: > >> > >> /* For stage 2 translations the starting level is specified by the > >> * VTCR_EL2.SL0 field (whose interpretation depends on the page size) > >> */ > >> int startlevel = extract32(tcr->raw_tcr, 6, 2); > >> unsigned int pamax = arm_pamax(cpu); > >> bool is_aarch64_regime = (va_size == 64); > >> bool ok; > >> > >> if (va_size == 32 || stride == 9) { > >> /* AArch32 or 4KB pages */ > >> level = 2 - startlevel; > >> } else { > >> /* 16KB or 64KB pages */ > >> level = 3 - startlevel; > >> } > >> > >> if (is_aarch64_regime && > >> inputsize > pamax) { > >> /* We have multiple choices but choose to fault. */ > >> ok = false; > >> } else { > >> /* Check that the starting level is valid. */ > >> ok = check_s2_startlevel(cpu, is_aarch64_regime, level, > >> inputsize, stride, pamax); > >> } > >> if (!ok) { > >> /* AArch64 reports these as level 0 faults. > >> * AArch32 reports these as level 1 faults. > >> */ > >> level = is_aarch64_regime ? 0 : 1; > >> fault_type = translation_fault; > >> goto do_fault; > >> } > >> > >> But I'm wondering if it just makes more sense to push the: > >> > >> is_aarch64_regime && inputsize > pamax > >> > >> Check into check_s2_startlevel? Then you could just have a simple call > >> which succeeds or falls through to a fault? > > > > Yeah, I guess we could rename check_s2_startlevel to something more generic > > and move all the checks there. I don't feel very strongly about either way... > > I think it would be cleaner to follow. get_phys_addr_lpae is already a > bit of a monster so the less conditions to keep track of while reading > it the better IMHO. OK, I'll have a look at that for v4. Thanks! Edgar > > > Thanks, > > Edgar > > > > > > > >> > >> /* Check that the starting level is valid. */ > >> if (!check_s2_startlevel(cpu, is_aarch64_regime, level, > >> inputsize, stride, pamax) ){ > >> /* AArch64 reports these as level 0 faults. > >> * AArch32 reports these as level 1 faults. > >> */ > >> level = is_aarch64_regime ? 0 : 1; > >> fault_type = translation_fault; > >> goto do_fault; > >> } > >> > >> -- > >> Alex Bennée > > > -- > Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check @ 2016-01-22 12:54 ` Edgar E. Iglesias 0 siblings, 0 replies; 20+ messages in thread From: Edgar E. Iglesias @ 2016-01-22 12:54 UTC (permalink / raw) To: Alex Bennée; +Cc: edgar.iglesias, peter.maydell, qemu-arm, qemu-devel On Fri, Jan 22, 2016 at 11:45:57AM +0000, Alex Bennée wrote: > > Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > > > On Fri, Jan 22, 2016 at 10:28:43AM +0000, Alex Bennée wrote: > >> > >> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes: > >> > >> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > >> > > >> > Implement the inputsize > pamax check for Stage 2 translations. > >> > We have multiple choices for how to respond to errors and > >> > choose to fault. > >> > > >> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > >> > --- > >> > target-arm/helper.c | 16 ++++++++++++---- > >> > 1 file changed, 12 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/target-arm/helper.c b/target-arm/helper.c > >> > index 4abeb4d..9a7ff5e 100644 > >> > --- a/target-arm/helper.c > >> > +++ b/target-arm/helper.c > >> > @@ -6808,7 +6808,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > >> > */ > >> > int startlevel = extract32(tcr->raw_tcr, 6, 2); > >> > unsigned int pamax = arm_pamax(cpu); > >> > - bool ok; > >> > + bool ok = true; > >> > > >> > if (va_size == 32 || stride == 9) { > >> > /* AArch32 or 4KB pages */ > >> > @@ -6818,9 +6818,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > >> > level = 3 - startlevel; > >> > } > >> > > >> > - /* Check that the starting level is valid. */ > >> > - ok = check_s2_startlevel(cpu, va_size == 64, level, > >> > - inputsize, stride, pamax); > >> > + if (va_size == 64 && > >> > + inputsize > pamax && > >> > + (arm_el_is_aa64(env, 1) || inputsize > 40)) { > >> > >> If va_size == 64 doesn't that imply arm_el_is_aa64(env, 1)? Looking > >> further up the function it seems that is what sets va_size in the first > >> place. I think that makes the inputsize > 40 check redundant. > > > > va_size == 64 is true if the EL corresponding to the translation _regime_ > > is in 64 bit mode (in this case EL2). > > > > EL1 may very well be in 32bit mode. > > Ahh yes, I missed that on the first reading. I think it might be clearer > when reading the code to have the: > > bool is_aarch64_regime = (va_size == 64); > > And use that to make it clear. And then comment on later check that it's > incompatible with EL1 being aarch32. > > > > >> > >> > + /* We have multiple choices but choose to fault. */ > >> > + ok = false; > >> > + } > >> > + if (ok) { > >> > + /* Check that the starting level is valid. */ > >> > + ok = check_s2_startlevel(cpu, va_size == 64, level, > >> > + inputsize, stride, pamax); > >> > + } > >> > if (!ok) { > >> > /* AArch64 reports these as level 0 faults. > >> > * AArch32 reports these as level 1 faults. > >> > >> I'm not a fan of the ok = true / ok = false / ok = > >> check_s2_start_level() / if (!ok) ping-pong here as it is hard to > >> follow. I'm not sure how you could make it cleaner to follow though. > >> Maybe something like: > >> > >> /* For stage 2 translations the starting level is specified by the > >> * VTCR_EL2.SL0 field (whose interpretation depends on the page size) > >> */ > >> int startlevel = extract32(tcr->raw_tcr, 6, 2); > >> unsigned int pamax = arm_pamax(cpu); > >> bool is_aarch64_regime = (va_size == 64); > >> bool ok; > >> > >> if (va_size == 32 || stride == 9) { > >> /* AArch32 or 4KB pages */ > >> level = 2 - startlevel; > >> } else { > >> /* 16KB or 64KB pages */ > >> level = 3 - startlevel; > >> } > >> > >> if (is_aarch64_regime && > >> inputsize > pamax) { > >> /* We have multiple choices but choose to fault. */ > >> ok = false; > >> } else { > >> /* Check that the starting level is valid. */ > >> ok = check_s2_startlevel(cpu, is_aarch64_regime, level, > >> inputsize, stride, pamax); > >> } > >> if (!ok) { > >> /* AArch64 reports these as level 0 faults. > >> * AArch32 reports these as level 1 faults. > >> */ > >> level = is_aarch64_regime ? 0 : 1; > >> fault_type = translation_fault; > >> goto do_fault; > >> } > >> > >> But I'm wondering if it just makes more sense to push the: > >> > >> is_aarch64_regime && inputsize > pamax > >> > >> Check into check_s2_startlevel? Then you could just have a simple call > >> which succeeds or falls through to a fault? > > > > Yeah, I guess we could rename check_s2_startlevel to something more generic > > and move all the checks there. I don't feel very strongly about either way... > > I think it would be cleaner to follow. get_phys_addr_lpae is already a > bit of a monster so the less conditions to keep track of while reading > it the better IMHO. OK, I'll have a look at that for v4. Thanks! Edgar > > > Thanks, > > Edgar > > > > > > > >> > >> /* Check that the starting level is valid. */ > >> if (!check_s2_startlevel(cpu, is_aarch64_regime, level, > >> inputsize, stride, pamax) ){ > >> /* AArch64 reports these as level 0 faults. > >> * AArch32 reports these as level 1 faults. > >> */ > >> level = is_aarch64_regime ? 0 : 1; > >> fault_type = translation_fault; > >> goto do_fault; > >> } > >> > >> -- > >> Alex Bennée > > > -- > Alex Bennée ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-01-22 12:54 UTC | newest] Thread overview: 20+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-21 11:18 [PATCH v2 0/3] target-arm: Add a few more S2 MMU input checks Edgar E. Iglesias 2016-01-21 11:18 ` [Qemu-devel] " Edgar E. Iglesias 2016-01-21 11:18 ` [PATCH v2 1/3] target-arm: Apply S2 MMU startlevel table size check to AArch64 Edgar E. Iglesias 2016-01-21 11:18 ` [Qemu-devel] " Edgar E. Iglesias 2016-01-21 11:18 ` [PATCH v2 2/3] target-arm: Make pamax an argument to check_s2_startlevel Edgar E. Iglesias 2016-01-21 11:18 ` [Qemu-devel] " Edgar E. Iglesias 2016-01-21 15:54 ` Alex Bennée 2016-01-21 15:54 ` [Qemu-devel] " Alex Bennée 2016-01-21 16:07 ` Edgar E. Iglesias 2016-01-21 16:07 ` [Qemu-devel] " Edgar E. Iglesias 2016-01-21 11:18 ` [PATCH v2 3/3] target-arm: Implement the S2 MMU inputsize > pamax check Edgar E. Iglesias 2016-01-21 11:18 ` [Qemu-devel] " Edgar E. Iglesias 2016-01-22 10:28 ` Alex Bennée 2016-01-22 10:28 ` [Qemu-devel] " Alex Bennée 2016-01-22 11:16 ` Edgar E. Iglesias 2016-01-22 11:16 ` [Qemu-devel] " Edgar E. Iglesias 2016-01-22 11:45 ` Alex Bennée 2016-01-22 11:45 ` [Qemu-devel] " Alex Bennée 2016-01-22 12:54 ` Edgar E. Iglesias 2016-01-22 12:54 ` [Qemu-devel] " Edgar E. Iglesias
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.