| Message ID | 1498830302-19274-3-git-send-email-edgar.iglesias@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30 June 2017 at 14:45, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Extend PAR format determination to handle more cases. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target/arm/helper.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index fd1027e..6a1fffe 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -2345,12 +2345,40 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, > uint32_t fsr; > bool ret; > uint64_t par64; > + bool format64 = false; > MemTxAttrs attrs = {}; > ARMMMUFaultInfo fi = {}; > > ret = get_phys_addr(env, value, access_type, mmu_idx, > &phys_addr, &attrs, &prot, &page_size, &fsr, &fi); > - if (extended_addresses_enabled(env)) { > + > + if (is_a64(env)) { > + format64 = true; > + } else if (arm_feature(env, ARM_FEATURE_LPAE)) { > + /* > + * ATS1Cxx: > + * * TTBCR.EAE determines whether the result is returned using the > + * 32-bit or the 64-bit PAR format > + * * Instructions executed in Hyp mode always use the 64bit format > + * > + * ATS1S2NSOxx uses the 64bit format if any of the following is true: > + * * The Non-secure TTBCR.EAE bit is set to 1 > + * * The implementation includes EL2, and the value of HCR.VM is 1 > + * > + * ATS1Hx always uses the 64bit format (not supported yet). > + */ > + format64 = regime_using_lpae_format(env, mmu_idx); > + > + if (arm_feature(env, ARM_FEATURE_EL2)) { > + if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { > + format64 |= env->cp15.hcr_el2 & HCR_VM; > + } else { > + format64 |= arm_current_el(env) == 2; > + } > + } > + } So this kind of worries me, because what it's coded as is "determine whether architecturally we should be returning a 64-bit or 32-bit PAR format", but what the code below it uses the format64 flag for is "manipulate whatever PAR we got handed back by get_phys_addr()". So we have two separate bits of code that are both choosing 32 vs 64 bit PAR (the code in this patch, and the logic inside get_phys_addr()), and they have to come to the same conclusion or we'll silently mangle the PAR. It seems like it would be better to either have get_phys_addr() explicitly tell us what kind of format it is returning to us, or to have the caller tell it what kind of PAR it needs. PS: on the subject of virtualization, I notice there's a comment a bit later on in do_ats_write() that says /* Note that S2WLK and FSTAGE are always zero, because we don't * implement virtualization and therefore there can't be a stage 2 * fault. */ so we'll need to address that too at some point... thanks -- PMM
On Mon, Jul 10, 2017 at 04:11:29PM +0100, Peter Maydell wrote: > On 30 June 2017 at 14:45, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Extend PAR format determination to handle more cases. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target/arm/helper.c | 30 +++++++++++++++++++++++++++++- > > 1 file changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/target/arm/helper.c b/target/arm/helper.c > > index fd1027e..6a1fffe 100644 > > --- a/target/arm/helper.c > > +++ b/target/arm/helper.c > > @@ -2345,12 +2345,40 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, > > uint32_t fsr; > > bool ret; > > uint64_t par64; > > + bool format64 = false; > > MemTxAttrs attrs = {}; > > ARMMMUFaultInfo fi = {}; > > > > ret = get_phys_addr(env, value, access_type, mmu_idx, > > &phys_addr, &attrs, &prot, &page_size, &fsr, &fi); > > - if (extended_addresses_enabled(env)) { > > + > > + if (is_a64(env)) { > > + format64 = true; > > + } else if (arm_feature(env, ARM_FEATURE_LPAE)) { > > + /* > > + * ATS1Cxx: > > + * * TTBCR.EAE determines whether the result is returned using the > > + * 32-bit or the 64-bit PAR format > > + * * Instructions executed in Hyp mode always use the 64bit format > > + * > > + * ATS1S2NSOxx uses the 64bit format if any of the following is true: > > + * * The Non-secure TTBCR.EAE bit is set to 1 > > + * * The implementation includes EL2, and the value of HCR.VM is 1 > > + * > > + * ATS1Hx always uses the 64bit format (not supported yet). > > + */ > > + format64 = regime_using_lpae_format(env, mmu_idx); > > + > > + if (arm_feature(env, ARM_FEATURE_EL2)) { > > + if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { > > + format64 |= env->cp15.hcr_el2 & HCR_VM; > > + } else { > > + format64 |= arm_current_el(env) == 2; > > + } > > + } > > + } > > So this kind of worries me, because what it's coded as is "determine > whether architecturally we should be returning a 64-bit or 32-bit > PAR format", but what the code below it uses the format64 flag for is > "manipulate whatever PAR we got handed back by get_phys_addr()". > So we have two separate bits of code that are both choosing > 32 vs 64 bit PAR (the code in this patch, and the logic inside > get_phys_addr()), and they have to come to the same conclusion > or we'll silently mangle the PAR. It seems like it would be > better to either have get_phys_addr() explicitly tell us what kind > of format it is returning to us, or to have the caller tell it > what kind of PAR it needs. Yes, I see your point and that's exactly what's happenning before the patch. Some of these new checks are generic in the sense that they check for LPAE/64bitness but others are I guess ATS specific for lack of a better term. It feels a bit weird to put the ATS specific PAR format logic into get_phys_addr. The basic idea here is that we never downgrade to the 32bit format, we only uprgade. The following line was meant to get the initial format I think you are requesting: format64 = regime_using_lpae_format(env, mmu_idx); After that, we apply possible ATS specfic upgrades to 64bit PAR format if needed. For clarity, perhaps we could make get_phys_addr return this same initial format, and then we can follow up with the ATS specific upgrades. E.g: ret = get_phys_addr(env, value, access_type, mmu_idx, &phys_addr, &attrs, &prot, &page_size, &fsr, &fi, &format64); /* Apply possible ATS/PAR 64bit upgrades if format64 is false. */ if (is_a64(env)) { format64 = true; } else if (arm_feature(env, ARM_FEATURE_LPAE)) { if (arm_feature(env, ARM_FEATURE_EL2)) { if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { format64 |= env->cp15.hcr_el2 & HCR_VM; } else { format64 |= arm_current_el(env) == 2; } } } Does something like that sound OK? Cheers, Edgar > > ret = get_phys_addr(env, value, access_type, mmu_idx, > > &phys_addr, &attrs, &prot, &page_size, &fsr, &fi); > > - if (extended_addresses_enabled(env)) { > > + > > + if (is_a64(env)) { > > + format64 = true; > > + } else if (arm_feature(env, ARM_FEATURE_LPAE)) { > > + /* > > + * ATS1Cxx: > > + * * TTBCR.EAE determines whether the result is returned using the > > + * 32-bit or the 64-bit PAR format > > + * * Instructions executed in Hyp mode always use the 64bit format > > + * > > + * ATS1S2NSOxx uses the 64bit format if any of the following is true: > > + * * The Non-secure TTBCR.EAE bit is set to 1 > > + * * The implementation includes EL2, and the value of HCR.VM is 1 > > + * > > + * ATS1Hx always uses the 64bit format (not supported yet). > > + */ > > + format64 = regime_using_lpae_format(env, mmu_idx); > > + > > + if (arm_feature(env, ARM_FEATURE_EL2)) { > > + if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { > > + format64 |= env->cp15.hcr_el2 & HCR_VM; > > + } else { > > + format64 |= arm_current_el(env) == 2; > > + } > > > PS: on the subject of virtualization, I notice there's a comment > a bit later on in do_ats_write() that says > /* Note that S2WLK and FSTAGE are always zero, because we don't > * implement virtualization and therefore there can't be a stage 2 > * fault. > */ > so we'll need to address that too at some point... > > thanks > -- PMM
On 11 July 2017 at 11:03, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > On Mon, Jul 10, 2017 at 04:11:29PM +0100, Peter Maydell wrote: >> So this kind of worries me, because what it's coded as is "determine >> whether architecturally we should be returning a 64-bit or 32-bit >> PAR format", but what the code below it uses the format64 flag for is >> "manipulate whatever PAR we got handed back by get_phys_addr()". >> So we have two separate bits of code that are both choosing >> 32 vs 64 bit PAR (the code in this patch, and the logic inside >> get_phys_addr()), and they have to come to the same conclusion >> or we'll silently mangle the PAR. It seems like it would be >> better to either have get_phys_addr() explicitly tell us what kind >> of format it is returning to us, or to have the caller tell it >> what kind of PAR it needs. > > Yes, I see your point and that's exactly what's happenning before the patch. > Some of these new checks are generic in the sense that they check for LPAE/64bitness > but others are I guess ATS specific for lack of a better term. > It feels a bit weird to put the ATS specific PAR format logic into get_phys_addr. > > The basic idea here is that we never downgrade to the 32bit format, we only uprgade. > The following line was meant to get the initial format I think you are requesting: > format64 = regime_using_lpae_format(env, mmu_idx); > > After that, we apply possible ATS specfic upgrades to 64bit PAR format if needed. > > For clarity, perhaps we could make get_phys_addr return this same initial format, and then > we can follow up with the ATS specific upgrades. E.g: > > ret = get_phys_addr(env, value, access_type, mmu_idx, > &phys_addr, &attrs, &prot, &page_size, &fsr, &fi, > &format64); > > /* Apply possible ATS/PAR 64bit upgrades if format64 is false. */ > if (is_a64(env)) { > format64 = true; > } else if (arm_feature(env, ARM_FEATURE_LPAE)) { > if (arm_feature(env, ARM_FEATURE_EL2)) { > if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { > format64 |= env->cp15.hcr_el2 & HCR_VM; > } else { > format64 |= arm_current_el(env) == 2; > } > } > } This still has the same problem, doesn't it? If get_phys_addr() has given you back a short-descriptor format PAR then you cannot simply "upgrade" it to a long-descriptor format PAR -- the fault status codes are all different. I think the "short desc vs long desc" condition used to be simple but the various upgrades to get_phys_addr() to handle EL2 have made it much more complicated, and so we'll be much better off just handing get_phys_addr() a flag to say how we want the status reported, if it's really dependent on ATS vs not-ATS. thanks -- PMM
On Tue, Jul 11, 2017 at 11:14:04AM +0100, Peter Maydell wrote: > On 11 July 2017 at 11:03, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > > On Mon, Jul 10, 2017 at 04:11:29PM +0100, Peter Maydell wrote: > >> So this kind of worries me, because what it's coded as is "determine > >> whether architecturally we should be returning a 64-bit or 32-bit > >> PAR format", but what the code below it uses the format64 flag for is > >> "manipulate whatever PAR we got handed back by get_phys_addr()". > >> So we have two separate bits of code that are both choosing > >> 32 vs 64 bit PAR (the code in this patch, and the logic inside > >> get_phys_addr()), and they have to come to the same conclusion > >> or we'll silently mangle the PAR. It seems like it would be > >> better to either have get_phys_addr() explicitly tell us what kind > >> of format it is returning to us, or to have the caller tell it > >> what kind of PAR it needs. > > > > Yes, I see your point and that's exactly what's happenning before the patch. > > Some of these new checks are generic in the sense that they check for LPAE/64bitness > > but others are I guess ATS specific for lack of a better term. > > It feels a bit weird to put the ATS specific PAR format logic into get_phys_addr. > > > > The basic idea here is that we never downgrade to the 32bit format, we only uprgade. > > The following line was meant to get the initial format I think you are requesting: > > format64 = regime_using_lpae_format(env, mmu_idx); > > > > After that, we apply possible ATS specfic upgrades to 64bit PAR format if needed. > > > > For clarity, perhaps we could make get_phys_addr return this same initial format, and then > > we can follow up with the ATS specific upgrades. E.g: > > > > ret = get_phys_addr(env, value, access_type, mmu_idx, > > &phys_addr, &attrs, &prot, &page_size, &fsr, &fi, > > &format64); > > > > /* Apply possible ATS/PAR 64bit upgrades if format64 is false. */ > > if (is_a64(env)) { > > format64 = true; > > } else if (arm_feature(env, ARM_FEATURE_LPAE)) { > > if (arm_feature(env, ARM_FEATURE_EL2)) { > > if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { > > format64 |= env->cp15.hcr_el2 & HCR_VM; > > } else { > > format64 |= arm_current_el(env) == 2; > > } > > } > > } > > This still has the same problem, doesn't it? If get_phys_addr() > has given you back a short-descriptor format PAR then you cannot > simply "upgrade" it to a long-descriptor format PAR -- the > fault status codes are all different. I think the "short desc > vs long desc" condition used to be simple but the various > upgrades to get_phys_addr() to handle EL2 have made it much > more complicated, and so we'll be much better off just handing > get_phys_addr() a flag to say how we want the status reported, OK, yes, the codes will be a problem. Telling get_phys_addr() what formatsounds good then. Will have a look. Thanks, Edgar
On Tue, Jul 11, 2017 at 11:14:04AM +0100, Peter Maydell wrote: > On 11 July 2017 at 11:03, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > > On Mon, Jul 10, 2017 at 04:11:29PM +0100, Peter Maydell wrote: > >> So this kind of worries me, because what it's coded as is "determine > >> whether architecturally we should be returning a 64-bit or 32-bit > >> PAR format", but what the code below it uses the format64 flag for is > >> "manipulate whatever PAR we got handed back by get_phys_addr()". > >> So we have two separate bits of code that are both choosing > >> 32 vs 64 bit PAR (the code in this patch, and the logic inside > >> get_phys_addr()), and they have to come to the same conclusion > >> or we'll silently mangle the PAR. It seems like it would be > >> better to either have get_phys_addr() explicitly tell us what kind > >> of format it is returning to us, or to have the caller tell it > >> what kind of PAR it needs. > > > > Yes, I see your point and that's exactly what's happenning before the patch. > > Some of these new checks are generic in the sense that they check for LPAE/64bitness > > but others are I guess ATS specific for lack of a better term. > > It feels a bit weird to put the ATS specific PAR format logic into get_phys_addr. > > > > The basic idea here is that we never downgrade to the 32bit format, we only uprgade. > > The following line was meant to get the initial format I think you are requesting: > > format64 = regime_using_lpae_format(env, mmu_idx); > > > > After that, we apply possible ATS specfic upgrades to 64bit PAR format if needed. > > > > For clarity, perhaps we could make get_phys_addr return this same initial format, and then > > we can follow up with the ATS specific upgrades. E.g: > > > > ret = get_phys_addr(env, value, access_type, mmu_idx, > > &phys_addr, &attrs, &prot, &page_size, &fsr, &fi, > > &format64); > > > > /* Apply possible ATS/PAR 64bit upgrades if format64 is false. */ > > if (is_a64(env)) { > > format64 = true; > > } else if (arm_feature(env, ARM_FEATURE_LPAE)) { > > if (arm_feature(env, ARM_FEATURE_EL2)) { > > if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { > > format64 |= env->cp15.hcr_el2 & HCR_VM; > > } else { > > format64 |= arm_current_el(env) == 2; > > } > > } > > } > > This still has the same problem, doesn't it? If get_phys_addr() > has given you back a short-descriptor format PAR then you cannot > simply "upgrade" it to a long-descriptor format PAR -- the > fault status codes are all different. I think the "short desc > vs long desc" condition used to be simple but the various > upgrades to get_phys_addr() to handle EL2 have made it much > more complicated, and so we'll be much better off just handing > get_phys_addr() a flag to say how we want the status reported, > if it's really dependent on ATS vs not-ATS. > Another way could also be to have get_phys_addr() fill in generic fields in the FaultInfo struct and then have a faultinfo_to_fsr mapping function to populate FSR/PAR. Do you see any issues with that? Perhaps that's better, not sure. Best regards, Edgar
On 11 July 2017 at 11:38, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > Another way could also be to have get_phys_addr() fill in generic > fields in the FaultInfo struct and then have a faultinfo_to_fsr > mapping function to populate FSR/PAR. Do you see any issues with that? Yes, that's probably better. It's how the pseudocode in the ARM ARM deals with the problem: there's a FaultRecord structure which is populated by AArch32.CreateFaultRecord() and AArch64.CreateFaultRecord(), and then it isn't actually turned into an FSR or ESR_ELx format value until needed (eg in AArch64.FaultSyndrome which calls EncodeLDFSC to get the long-form fault code bits, or similar functions on the AArch32 side which end up calling EncodeSDFSC() for short-form code bits). thanks -- PMM
On 11 July 2017 at 11:38, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > Another way could also be to have get_phys_addr() fill in generic > fields in the FaultInfo struct and then have a faultinfo_to_fsr > mapping function to populate FSR/PAR. Do you see any issues with that? Edgar, did you ever have a go at implementing this? I'm currently running into a similar issue with M profile, where at the moment we stuff the information about what kind of fault the MPU generates into a v7PMSA format FSR value and reinterpret it into M profile exception types and fault status register bits later. This works OK, but for v8M we want to start reporting kinds of fault (like SecureFault) that don't have equivalents in v7PMSA at all, and maybe it would be better to clean this up rather than assigning arbitrary bogus fsr values for internal use... thanks -- PMM
On Mon, Sep 18, 2017 at 04:50:23PM +0100, Peter Maydell wrote: > On 11 July 2017 at 11:38, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > > Another way could also be to have get_phys_addr() fill in generic > > fields in the FaultInfo struct and then have a faultinfo_to_fsr > > mapping function to populate FSR/PAR. Do you see any issues with that? > > Edgar, did you ever have a go at implementing this? Hi Peter, No, I haven't looked at it yet. I'm a bit behind on everything here so I probably won't get a chance to look at it soonish... > I'm currently running into a similar issue with M profile, > where at the moment we stuff the information about what > kind of fault the MPU generates into a v7PMSA format > FSR value and reinterpret it into M profile exception > types and fault status register bits later. This works > OK, but for v8M we want to start reporting kinds of fault > (like SecureFault) that don't have equivalents in v7PMSA > at all, and maybe it would be better to clean this up rather > than assigning arbitrary bogus fsr values for internal use... I see, yes that sounds like a similar issue. If you'd like to take over this, that'd be great :-) Cheers, Edgar
On 19 September 2017 at 05:43, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > On Mon, Sep 18, 2017 at 04:50:23PM +0100, Peter Maydell wrote: >> I'm currently running into a similar issue with M profile, >> where at the moment we stuff the information about what >> kind of fault the MPU generates into a v7PMSA format >> FSR value and reinterpret it into M profile exception >> types and fault status register bits later. This works >> OK, but for v8M we want to start reporting kinds of fault >> (like SecureFault) that don't have equivalents in v7PMSA >> at all, and maybe it would be better to clean this up rather >> than assigning arbitrary bogus fsr values for internal use... > > I see, yes that sounds like a similar issue. > If you'd like to take over this, that'd be great :-) For the moment I've taken the easy approach of using dummy FSR values. We'll see if that gets through code review :-) thanks -- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index fd1027e..6a1fffe 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -2345,12 +2345,40 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, uint32_t fsr; bool ret; uint64_t par64; + bool format64 = false; MemTxAttrs attrs = {}; ARMMMUFaultInfo fi = {}; ret = get_phys_addr(env, value, access_type, mmu_idx, &phys_addr, &attrs, &prot, &page_size, &fsr, &fi); - if (extended_addresses_enabled(env)) { + + if (is_a64(env)) { + format64 = true; + } else if (arm_feature(env, ARM_FEATURE_LPAE)) { + /* + * ATS1Cxx: + * * TTBCR.EAE determines whether the result is returned using the + * 32-bit or the 64-bit PAR format + * * Instructions executed in Hyp mode always use the 64bit format + * + * ATS1S2NSOxx uses the 64bit format if any of the following is true: + * * The Non-secure TTBCR.EAE bit is set to 1 + * * The implementation includes EL2, and the value of HCR.VM is 1 + * + * ATS1Hx always uses the 64bit format (not supported yet). + */ + format64 = regime_using_lpae_format(env, mmu_idx); + + if (arm_feature(env, ARM_FEATURE_EL2)) { + if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { + format64 |= env->cp15.hcr_el2 & HCR_VM; + } else { + format64 |= arm_current_el(env) == 2; + } + } + } + + if (format64) { /* 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.