Re: [RFC v1 24/38] target/arm: move aa64_va_parameter_tbi,tbid,tcma and arm_rebuild_hflags

[Claudio Fontana]

On 2/22/21 7:02 AM, Richard Henderson wrote:
> On 2/21/21 1:24 AM, Claudio Fontana wrote:
>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>
>> they are needed for KVM too, move them out of TCG helpers.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  target/arm/internals.h      |  37 +++++
>>  target/arm/tcg/helper-tcg.h |  32 -----
>>  target/arm/cpu-common.c     | 252 ++++++++++++++++++++++++++++++++++
>>  target/arm/tcg/helper.c     | 264 +-----------------------------------
>>  4 files changed, 293 insertions(+), 292 deletions(-)
>>
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index 6589b63ebc..9eb5d7fd79 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -1196,6 +1196,43 @@ static inline uint64_t 
>> useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
>>      return ptr;
>>  }
>>  
>> +/*
>> + * Convert a possible stage1+2 MMU index into the appropriate
>> + * stage 1 MMU index
>> + */
>> +static inline ARMMMUIdx stage_1_mmu_idx(ARMMMUIdx mmu_idx)
>> +{
>> +    switch (mmu_idx) {
>> +    case ARMMMUIdx_SE10_0:
>> +        return ARMMMUIdx_Stage1_SE0;
>> +    case ARMMMUIdx_SE10_1:
>> +        return ARMMMUIdx_Stage1_SE1;
>> +    case ARMMMUIdx_SE10_1_PAN:
>> +        return ARMMMUIdx_Stage1_SE1_PAN;
>> +    case ARMMMUIdx_E10_0:
>> +        return ARMMMUIdx_Stage1_E0;
>> +    case ARMMMUIdx_E10_1:
>> +        return ARMMMUIdx_Stage1_E1;
>> +    case ARMMMUIdx_E10_1_PAN:
>> +        return ARMMMUIdx_Stage1_E1_PAN;
>> +    default:
>> +        return mmu_idx;
>> +    }
>> +}
>> +
>> +int aa64_va_parameter_tbi(uint64_t tcr, ARMMMUIdx mmu_idx);
>> +int aa64_va_parameter_tbid(uint64_t tcr, ARMMMUIdx mmu_idx);
> 
> I can see these being needed for get-phys-addr -- and that probably answers my
> arm_mmu_idx_el question earlier too.
> 
> 
>> +uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
>> +                            ARMMMUIdx mmu_idx);
>> +uint32_t rebuild_hflags_a32(CPUARMState *env, int fp_el, ARMMMUIdx mmu_idx);
>> +uint32_t rebuild_hflags_m32(CPUARMState *env, int fp_el, ARMMMUIdx mmu_idx);
> 
> However these really really shouldn't be used for !tcg.  I would even wrap
> CPUARMState::hflags in #ifdef CONFIG_TCG to enforce that.
> 
> I think maybe the best option is
> 
>     if (tcg_enabled()) {
>         rebuild_hflags();
>     }
> 
> so that we don't spend the time on the rebuild for a regular build that has
> both tcg and kvm enabled, and the symbol gets
> compiled out when tcg is disabled.

is the code elimination for "if (0)" a guarantee, ie, we won't encounter 
compiler or compiler-options differences,
for the compilers we support?

This is a doubt that is bothering me since some time, since I remember I 
encountered problems like this before,
with my compiler behaving differently than Paolo's in particular.

Is there some way to force the compilers to not even look at what is in the if 
(0) block?
That should work also with --enable-debug?

This way we could avoid a lot of boilerplate/stubs...

Ciao,

Claudio


> 
> 
> r~
>