diff mbox series

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

Message ID 20250320223002.2915728-18-pierrick.bouvier@linaro.org (mailing list archive)
State New
Headers show
Series single-binary: start make hw/arm/ common | expand

Commit Message

Pierrick Bouvier March 20, 2025, 10:29 p.m. UTC
We introduce later a mechanism to skip cpu definitions inclusion, so we
can detect it here, and call the correct runtime function instead.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/exec/target_page.h | 3 +++
 1 file changed, 3 insertions(+)

Comments

Richard Henderson March 21, 2025, 6:05 p.m. UTC | #1
On 3/20/25 15:29, Pierrick Bouvier wrote:
> We introduce later a mechanism to skip cpu definitions inclusion, so we
> can detect it here, and call the correct runtime function instead.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   include/exec/target_page.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/include/exec/target_page.h b/include/exec/target_page.h
> index 8e89e5cbe6f..aeddb25c743 100644
> --- a/include/exec/target_page.h
> +++ b/include/exec/target_page.h
> @@ -40,6 +40,9 @@ extern const TargetPageBits target_page;
>   #  define TARGET_PAGE_MASK   ((TARGET_PAGE_TYPE)target_page.mask)
>   # endif
>   # define TARGET_PAGE_SIZE    (-(int)TARGET_PAGE_MASK)
> +# ifndef TARGET_PAGE_BITS_MIN
> +#  define TARGET_PAGE_BITS_MIN qemu_target_page_bits_min()
> +# endif
>   #else
>   # define TARGET_PAGE_BITS_MIN TARGET_PAGE_BITS
>   # define TARGET_PAGE_SIZE    (1 << TARGET_PAGE_BITS)

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.


r~
Pierrick Bouvier March 21, 2025, 6:09 p.m. UTC | #2
On 3/21/25 11:05, Richard Henderson wrote:
> On 3/20/25 15:29, Pierrick Bouvier wrote:
>> We introduce later a mechanism to skip cpu definitions inclusion, so we
>> can detect it here, and call the correct runtime function instead.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    include/exec/target_page.h | 3 +++
>>    1 file changed, 3 insertions(+)
>>
>> diff --git a/include/exec/target_page.h b/include/exec/target_page.h
>> index 8e89e5cbe6f..aeddb25c743 100644
>> --- a/include/exec/target_page.h
>> +++ b/include/exec/target_page.h
>> @@ -40,6 +40,9 @@ extern const TargetPageBits target_page;
>>    #  define TARGET_PAGE_MASK   ((TARGET_PAGE_TYPE)target_page.mask)
>>    # endif
>>    # define TARGET_PAGE_SIZE    (-(int)TARGET_PAGE_MASK)
>> +# ifndef TARGET_PAGE_BITS_MIN
>> +#  define TARGET_PAGE_BITS_MIN qemu_target_page_bits_min()
>> +# endif
>>    #else
>>    # define TARGET_PAGE_BITS_MIN TARGET_PAGE_BITS
>>    # define TARGET_PAGE_SIZE    (1 << TARGET_PAGE_BITS)
> 
> 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?

> 
> r~
Richard Henderson March 21, 2025, 7:27 p.m. UTC | #3
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.


r~
Pierrick Bouvier March 21, 2025, 8:11 p.m. UTC | #4
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?

I went through that question, and was not sure what should be the answer.

> 
> r~
Richard Henderson March 21, 2025, 10:19 p.m. UTC | #5
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.


r~
Pierrick Bouvier March 22, 2025, 12:01 a.m. UTC | #6
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?

Thanks,
Pierrick

> 
> r~
Pierrick Bouvier March 22, 2025, 12:20 a.m. UTC | #7
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?

> Thanks,
> Pierrick
> 
>>
>> r~
>
diff mbox series

Patch

diff --git a/include/exec/target_page.h b/include/exec/target_page.h
index 8e89e5cbe6f..aeddb25c743 100644
--- a/include/exec/target_page.h
+++ b/include/exec/target_page.h
@@ -40,6 +40,9 @@  extern const TargetPageBits target_page;
 #  define TARGET_PAGE_MASK   ((TARGET_PAGE_TYPE)target_page.mask)
 # endif
 # define TARGET_PAGE_SIZE    (-(int)TARGET_PAGE_MASK)
+# ifndef TARGET_PAGE_BITS_MIN
+#  define TARGET_PAGE_BITS_MIN qemu_target_page_bits_min()
+# endif
 #else
 # define TARGET_PAGE_BITS_MIN TARGET_PAGE_BITS
 # define TARGET_PAGE_SIZE    (1 << TARGET_PAGE_BITS)