Re: [PATCH v2 1/2] target/arm: Fix select for aa64_va_parameters_both
On 2/13/20 5:25 AM, Peter Maydell wrote:
> On Thu, 13 Feb 2020 at 13:12, Peter Maydell wrote:
>>
>> On Tue, 11 Feb 2020 at 19:42, Richard Henderson
>> wrote:
>>>
>>> Select should always be 0 for a regime with one range.
>>>
>>> Signed-off-by: Richard Henderson
>>
>> This change makes sense, and matches what aa32_va_parameters() does,
>> but I think we need to update some of the callsites.
>>
>> (1) In get_phys_addr_lpae() we have the check:
>>
>> if (-top_bits != param.select || (param.select && !ttbr1_valid)) {
>>
>> where ttbr1_valid is the return value of (effectively)
>> aarch64 ? regime_has_2_ranges() : (el != 2);
>> but I think it's no longer possible to get here with param.select == 1
>> and !ttbr1_valid, so this becomes a dead check.
>
> ...or should the code instead be checking literal pointer bit 55
> against ttbr1_valid now ?
No, I think the first expression now covers everything, as you suggested in the
first reply.
r~
Re: [PATCH v2 1/2] target/arm: Fix select for aa64_va_parameters_both
On Thu, 13 Feb 2020 at 13:12, Peter Maydell wrote:
>
> On Tue, 11 Feb 2020 at 19:42, Richard Henderson
> wrote:
> >
> > Select should always be 0 for a regime with one range.
> >
> > Signed-off-by: Richard Henderson
>
> This change makes sense, and matches what aa32_va_parameters() does,
> but I think we need to update some of the callsites.
>
> (1) In get_phys_addr_lpae() we have the check:
>
> if (-top_bits != param.select || (param.select && !ttbr1_valid)) {
>
> where ttbr1_valid is the return value of (effectively)
> aarch64 ? regime_has_2_ranges() : (el != 2);
> but I think it's no longer possible to get here with param.select == 1
> and !ttbr1_valid, so this becomes a dead check.
...or should the code instead be checking literal pointer bit 55
against ttbr1_valid now ?
thanks
-- PMM
Re: [PATCH v2 1/2] target/arm: Fix select for aa64_va_parameters_both
On Thu, 13 Feb 2020 at 13:12, Peter Maydell wrote: > > On Tue, 11 Feb 2020 at 19:42, Richard Henderson > wrote: > > > > Select should always be 0 for a regime with one range. > > > > Signed-off-by: Richard Henderson > > This change makes sense, and matches what aa32_va_parameters() does, > but I think we need to update some of the callsites. Assuming those changes are done in separate patches, for this patch itself: Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH v2 1/2] target/arm: Fix select for aa64_va_parameters_both
On Tue, 11 Feb 2020 at 19:42, Richard Henderson
wrote:
>
> Select should always be 0 for a regime with one range.
>
> Signed-off-by: Richard Henderson
This change makes sense, and matches what aa32_va_parameters() does,
but I think we need to update some of the callsites.
(1) In get_phys_addr_lpae() we have the check:
if (-top_bits != param.select || (param.select && !ttbr1_valid)) {
where ttbr1_valid is the return value of (effectively)
aarch64 ? regime_has_2_ranges() : (el != 2);
but I think it's no longer possible to get here with param.select == 1
and !ttbr1_valid, so this becomes a dead check.
(Side note, could we pull "ttbr1_valid = regime_has_2_ranges(mmu_idx);"
out of the "if (aarch64) {...} else {...}" ? -- I think it works for
aarch32 too, right?)
(2) in pauth_original_ptr() we do
uint64_t extfield = -param.select;
but in the pseudocode Auth() function the extfield is unconditionally
calculated based on bit 55 of the address, regardless of whether
the regime has 1 range or 2. So I think this code can't use
param.select any more but should simply pull out and replicate
bit 55 of its 'ptr' argument, now that param.select is not simply
the value of bit 55.
Change 1 would need to be done after this patch and change 2 before it.
thanks
-- PMM
[PATCH v2 1/2] target/arm: Fix select for aa64_va_parameters_both
Select should always be 0 for a regime with one range.
Signed-off-by: Richard Henderson
---
target/arm/helper.c | 46 +++--
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7d15d5c933..a008c70c87 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10074,13 +10074,8 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState
*env, uint64_t va,
bool tbi, tbid, epd, hpd, using16k, using64k;
int select, tsz;
-/*
- * Bit 55 is always between the two regions, and is canonical for
- * determining if address tagging is enabled.
- */
-select = extract64(va, 55, 1);
-
if (!regime_has_2_ranges(mmu_idx)) {
+select = 0;
tsz = extract32(tcr, 0, 6);
using64k = extract32(tcr, 14, 1);
using16k = extract32(tcr, 15, 1);
@@ -10093,23 +10088,30 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState
*env, uint64_t va,
tbid = extract32(tcr, 29, 1);
}
epd = false;
-} else if (!select) {
-tsz = extract32(tcr, 0, 6);
-epd = extract32(tcr, 7, 1);
-using64k = extract32(tcr, 14, 1);
-using16k = extract32(tcr, 15, 1);
-tbi = extract64(tcr, 37, 1);
-hpd = extract64(tcr, 41, 1);
-tbid = extract64(tcr, 51, 1);
} else {
-int tg = extract32(tcr, 30, 2);
-using16k = tg == 1;
-using64k = tg == 3;
-tsz = extract32(tcr, 16, 6);
-epd = extract32(tcr, 23, 1);
-tbi = extract64(tcr, 38, 1);
-hpd = extract64(tcr, 42, 1);
-tbid = extract64(tcr, 52, 1);
+/*
+ * Bit 55 is always between the two regions, and is canonical for
+ * determining if address tagging is enabled.
+ */
+select = extract64(va, 55, 1);
+if (!select) {
+tsz = extract32(tcr, 0, 6);
+epd = extract32(tcr, 7, 1);
+using64k = extract32(tcr, 14, 1);
+using16k = extract32(tcr, 15, 1);
+tbi = extract64(tcr, 37, 1);
+hpd = extract64(tcr, 41, 1);
+tbid = extract64(tcr, 51, 1);
+} else {
+int tg = extract32(tcr, 30, 2);
+using16k = tg == 1;
+using64k = tg == 3;
+tsz = extract32(tcr, 16, 6);
+epd = extract32(tcr, 23, 1);
+tbi = extract64(tcr, 38, 1);
+hpd = extract64(tcr, 42, 1);
+tbid = extract64(tcr, 52, 1);
+}
}
tsz = MIN(tsz, 39); /* TODO: ARMv8.4-TTST */
tsz = MAX(tsz, 16); /* TODO: ARMv8.2-LVA */
--
2.20.1
