Re: [PATCH v2 17/30] exec/target_page: runtime defintion for TARGET_PAGE_BITS_MIN

[Richard Henderson <richard.henderson@linaro.org>]

On 3/21/25 17:20, Pierrick Bouvier wrote:
> On 3/21/25 17:01, Pierrick Bouvier wrote:
>> On 3/21/25 15:19, Richard Henderson wrote:
>>> On 3/21/25 13:11, Pierrick Bouvier wrote:
>>>> On 3/21/25 12:27, Richard Henderson wrote:
>>>>> On 3/21/25 11:09, Pierrick Bouvier wrote:
>>>>>>> Mmm, ok I guess.  Yesterday I would have suggested merging this with page-vary.h, but
>>>>>>> today I'm actively working on making TARGET_PAGE_BITS_MIN a global constant.
>>>>>>>
>>>>>>
>>>>>> When you mention this, do you mean "constant accross all architectures", or a global
>>>>>> (const) variable vs having a function call?
>>>>> The first -- constant across all architectures.
>>>>>
>>>>
>>>> That's great.
>>>> Does choosing the min(set_of(TARGET_PAGE_BITS_MIN)) is what we want there, or is the
>>>> answer more subtle than that?
>>>
>>> It will be, yes.
>>>
>>> This isn't as hard as it seems, because there are exactly two targets with
>>> TARGET_PAGE_BITS < 12: arm and avr.
>>>
>>> Because we still support armv4, TARGET_PAGE_BITS_MIN must be <= 10.
>>>
>>> AVR currently has TARGET_PAGE_BITS == 8, which is a bit of a problem.
>>> My first task is to allow avr to choose TARGET_PAGE_BITS_MIN >= 10.
>>>
>>> Which will leave us with TARGET_PAGE_BITS_MIN == 10.
>>>
>>
>> Ok.
>>
>>   From what I understand, we make sure tlb flags are stored in an
>> immutable position, within virtual addresses related to guest, by using
>> lower bits belonging to address range inside a given page, since page
>> addresses are aligned on page size, leaving those bits free.
>>
>> bits [0..2) are bswap, watchpoint and check_aligned.
>> bits [TARGET_PAGE_BITS_MIN - 5..TARGET_PAGE_BITS_MIN) are slow,
>> discard_write, mmio, notdirty, and invalid mask.
>> And the compile time check we have is to make sure we don't overlap
>> those sets (would happen in TARGET_PAGE_BITS_MIN <= 7).
>>
>> I wonder why we can't use bits [3..8) everywhere, like it's done for
>> AVR, even for bigger page sizes. I noticed the comment about "address
>> alignment bits", but I'm confused why bits [0..2) can be used, and not
>> upper ones.
>>
>> Are we storing something else in the middle on other archs, or did I
>> miss some piece of the puzzle?
>>
> 
> After looking better, TLB_SLOW_FLAGS are not part of address, so we don't use bits [0..2).
> 
> For a given TARGET_PAGE_SIZE, how do we define alignment bits?

Alignment bits are the least significant bits that must be 0 in order to enforce a 
particular alignment.  The specific alignment is requested via MO_ALIGN et al as part of 
the guest memory reference.

I think the piece you're missing is the softmmu fast path test in the generated code.

We begin by indexing the tlb to find an entry.  At that index, the entry may or may not 
match because (1) we have never looked up the page so the entry is empty, (2) we have 
looked up a different page that aliases, or (3) the page is present and (3a) correct, or 
(3b) invalidated, or (3c) some other condition that forces the slow path.

The target address and the comparator have several fields:

   page address   [63 ... TARGET_PAGE_BITS]
   page flags     [TARGET_PAGE_BITS - 1 ... TARGET_PAGE_BITS - 5]
   unused         [TARGET_PAGE_BITS - 6 ... align_bits], or empty.
   alignment      [align_bits - 1 ... 0], or empty

In the comparator, the unused and alignment bits are always zero; the page flags may be 
non-zero in order to force the comparison to fail.

In the target address, we mask the page flags and unused bits; if the alignment bits of 
the address are set, then the address is of course unaligned and so the comparison fails.

In order for all this work, the alignment field cannot overlap the page flags.

The maximum alignment currently used by any guest is 5 bits, for Arm Neon,
which means the minimum value for TARGET_PAGE_BITS_MIN is 10.


r~