Re: [PATCH v6 13/18] accel/tcg: Do not align tb->page_addr[0]

[Alex Bennée]

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

> On 10/3/22 05:47, Alex Bennée wrote:
>> Richard Henderson <richard.henderson@linaro.org> writes:
>> 
>>> Let tb->page_addr[0] contain the offset within the page of the
>>> start of the translation block.  We need to recover this value
>>> anyway at various points, and it is easier to discard the page
>>> offset when it's not needed, which happens naturally via the
>>> existing find_page shift.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>   accel/tcg/cpu-exec.c      | 16 ++++++++--------
>>>   accel/tcg/cputlb.c        |  3 ++-
>>>   accel/tcg/translate-all.c |  9 +++++----
>>>   3 files changed, 15 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>>> index 5f43b9769a..dd58a144a8 100644
>>> --- a/accel/tcg/cpu-exec.c
>>> +++ b/accel/tcg/cpu-exec.c
>>> @@ -174,7 +174,7 @@ struct tb_desc {
>>>       target_ulong pc;
>>>       target_ulong cs_base;
>>>       CPUArchState *env;
>>> -    tb_page_addr_t phys_page1;
>>> +    tb_page_addr_t page_addr0;
>> We don't actually document that this is an offset here (or indeed in
>> TranslationBlock) and the definition of tb_page_addr_t:
>>    /* Page tracking code uses ram addresses in system mode, and
>> virtual
>>       addresses in userspace mode.  Define tb_page_addr_t to be an 
>> appropriate
>>       type.  */
>>    #if defined(CONFIG_USER_ONLY)
>>    typedef abi_ulong tb_page_addr_t;
>>    #define TB_PAGE_ADDR_FMT TARGET_ABI_FMT_lx
>>    #else
>>    typedef ram_addr_t tb_page_addr_t;
>>    #define TB_PAGE_ADDR_FMT RAM_ADDR_FMT
>>    #endif
>> implies these are full size pointers into the guests address space.
>
> And that's what I've got.  What we we were storing in phys_page1
> before was a full size pointer that was page aligned.  I'm now
> dropping the page alignment and having a full size pointer to the
> exact first byte of the translated code.

OK then I'm confused by the commit message which says:

  Let tb->page_addr[0] contain the offset within the page of the
  start of the translation block

> Is that clearer?  How would you improve the wording?
>
>
> r~
>
>> Either we need a new type (tb_page_offset_t) or to properly comment the
>> structures with what they mean.
>> Otherwise:
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> 


-- 
Alex Bennée