Re: [PATCH 02/62] target/arm: Enable PageEntryExtra

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

On 7/4/22 20:52, Peter Maydell wrote:
> On Sun, 3 Jul 2022 at 09:25, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Copy attrs, sharability, and the NS bit into the TLB.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/arm/cpu-param.h  |  8 ++++++++
>>   target/arm/internals.h  |  5 +++++
>>   target/arm/tlb_helper.c | 14 ++++++++++++--
>>   3 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
>> index 68ffb12427..a14f167d11 100644
>> --- a/target/arm/cpu-param.h
>> +++ b/target/arm/cpu-param.h
>> @@ -30,6 +30,14 @@
>>    */
>>   # define TARGET_PAGE_BITS_VARY
>>   # define TARGET_PAGE_BITS_MIN  10
>> +/*
>> + * Extra information stored in softmmu page tables.
>> + */
>> +# define TARGET_PAGE_ENTRY_EXTRA
>> +struct PageEntryExtra {
>> +    /* See PAGEENTRYEXTRA fields in cpu.h */
>> +    uint64_t x;
>> +};
>>   #endif
>>
>>   #define NB_MMU_MODES 15
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index c66f74a0db..2b38a83574 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -74,6 +74,11 @@ FIELD(V7M_EXCRET, DCRS, 5, 1)
>>   FIELD(V7M_EXCRET, S, 6, 1)
>>   FIELD(V7M_EXCRET, RES1, 7, 25) /* including the must-be-1 prefix */
>>
>> +/* Bit definitions for PageEntryExtra */
>> +FIELD(PAGEENTRYEXTRA, ATTRS, 0, 8)
>> +FIELD(PAGEENTRYEXTRA, SHAREABILITY, 8, 2)
>> +FIELD(PAGEENTRYEXTRA, PA, 12, 52)
> 
> So why do we want these things in particular? It would be
> helpful to describe the intended uses in the commit message
> to save the reader having to read the next 60 patches to
> find out :-)

Heh, yes.  Basically, it's what S1_ptw_translate requires (pa, attrs), so that we can 
report a stage1 ptw failure, and what do_ats_write requires (pa, sh, attrs) for filling in 
PAR_EL1.  Although within these 62 patches I didn't came back to finish converting 
do_ats_write to use probe_access_flags_extra instead of using get_phys_addr directly, it 
was a goal.

> Is wanting to cache the physaddr an Arm-specific thing, or is it
> something we should consider having in the core softmmu code?

I'm not sure what other targets require for their 2-stage page table walks.  I guess I 
should have a look (riscv, i386, ?).

It *is* possible to recover the phys addr from the iommutlb, because I was doing that in 
mte_helper.c (see code removed in patch 5), but it's certainly not simple.

>>       if (likely(!ret)) {
>> +        PageEntryExtra extra = {};
>> +
>>           /*
>>            * Map a single [sub]page. Regions smaller than our declared
>>            * target page size are handled specially, so for those we
>> -         * pass in the exact addresses.
>> +         * pass in the exact addresses.  This only happens for M-profile,
>> +         * which does not use or require PageEntryExtra.
>>            */
> 
> Do we have to exclude M-profile here because the PageEntryExtra
> data is strictly-per-page, or because the way we've formatted
> our extra uint64_t requires the physaddr to be page-aligned, or both?

Because our extra uint64_t requires page alignment, and reuses those bits.


r~