Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination

[Peter Maydell <peter.maydell@linaro.org>]

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