Re: [PATCH] target/ppc: Fix truncation of env->hflags

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 1/23/21 6:46 PM, David Gibson wrote:
>> On Sat, Jan 23, 2021 at 05:24:22PM -1000, Richard Henderson wrote:
>>> Use the cs_base field, because it happens to be the same
>>> size as hflags (and MSR, from which hflags is derived).
>>>
>>> In translate, extract most bits from a local hflags variable.
>>> Mark several cases where code generation is *not* derived from
>>> data stored within the hashed elements of the TranslationBlock.
>> 
>> My knowledge of TCG isn't great, so I'm pretty much prepared to accept
>> this is correct on your say so.
>> 
>> But that commit message feels like it's following on from a
>> conversation that's not here, nor linked.  It'd be great if it
>> explained how said hflags truncation is happening, because it's
>> certainly not obvious to someone with only a fair to middling
>> understanding of TCG.
>
> Mm, fair.
>
> How about:
>
> The assignment from env->hflags to tb->flags truncates
> target_ulong to uint32_t.  This loses important bits from
> the top of hflags, which results in incorrect tb selection.

We are just putting off the day we declare tb->flags to be 64 bit or end
up renaming cs_base to
tb->cs_base_or_extra_flag_bits_we_could_not_fit_in_flags. The fact that
cs_base is expressed in terms of target_ulong worries me if there is
ever any hflag state above bit 32 for the ppc32 targets.

>
> Use the cs_base field instead, because it happens to be the
> same size as hflags (and MSR fom which hflags is derived).
>
> In translate, extract most bits from a local hflags variable.
> All of the checks vs env->flags are redundant with env->msr_mask
> in that msr bits cannot be set when the feature is not available.
> Mark several cases where code generation is *not* derived from
> data stored within hashed elements of the tb.
>
>
> r~


-- 
Alex Bennée