Re: [PATCH 1/3] arm/mpu: implement setup_virt_paging for MPU system

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Ayan,

>>    void __init setup_virt_paging(void)
>>  {
>> -    BUG_ON("unimplemented");
>> +    uint64_t vtcr_el2 = READ_SYSREG(VTCR_EL2),
> 
> I think this register is redefined for R82 . See
> 
> https://developer.arm.com/documentation/102670/0301/AArch64-registers/AArch64-register-descriptions/AArch64-Generic-System-control-register-description/VTCR-EL2--Virtualization-Translation-Control-Register?lang=en
>  . So
> 
>>  vstcr_el2 = READ_SYSREG(VSTCR_EL2);
>> +
>> +    /* PA size */
>> +    const unsigned int pa_range_info[] = {32, 36, 40, 42, 44, 48, 52, 0,
>> +                                          /* Invalid */};
>> +
>> +    /*
>> +     * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured
>> +     * with IPA bits == PA bits, compare against "pabits".
>> +     */
>> +    if ( pa_range_info[system_cpuinfo.mm64.pa_range] < p2m_ipa_bits )
>> +        p2m_ipa_bits = pa_range_info[system_cpuinfo.mm64.pa_range];
>> +
>> +    /*
>> +     * Clear VTCR_EL2.NSA bit to configure non-secure stage 2 translation 
>> output
>> +     * address space to access the Secure PA space as Armv8r only implements
>> +     * secure state.
>> +     */
>> +    vtcr_el2 &= ~VTCR_NSA;
> 
> But the TRM says this
> 
> This bit behaves as 1 for all purposes other than reading back the value of 
> the bit when the value of AArch64-VSTCR_EL2.SA is 1.
> 
> So shouldn't we keep it as 1.
> 

The way I’m reading the armv8-r aarch64 reference manual [1] is that if we set
VSTCR_EL2.SA to 0, we need to clear also this one.


>> +
>> +    /*
>> +     * The MSA and MSA_frac fields in the ID_AA64MMFR0_EL1 register 
>> identify the
>> +     * memory system configurations supported. In Armv8-R AArch64, the
>> +     * only permitted value for ID_AA64MMFR0_EL1.MSA is 0b1111.
>> +     */
>> +    if ( system_cpuinfo.mm64.msa != MM64_MSA_PMSA_SUPPORT )
>> +        goto fault;
>> +
>> +    /* Permitted values for ID_AA64MMFR0_EL1.MSA_frac are 0b0001 and 
>> 0b0010. */
>> +    if ( system_cpuinfo.mm64.msa_frac == MM64_MSA_FRAC_NONE_SUPPORT )
>> +        goto fault;
>> +
>> +    /*
>> +     * cpuinfo sanitization makes sure we support 16bits VMID only if all 
>> cores
>> +     * are supporting it.
>> +     */
>> +    if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
>> +        max_vmid = MAX_VMID_16_BIT;
>> +
>> +    /* Set the VS bit only if 16 bit VMID is supported. */
>> +    if ( max_vmid == MAX_VMID_16_BIT )
>> +        vtcr_el2 |= VTCR_VS;
> this field does not exist.

yes this exists in armv8-r aarch64 [1]

>> 
>> +/* Not used on MPU system */
>>  static inline void p2m_clear_root_pages(struct p2m_domain *p2m) {}
>>    static inline void p2m_tlb_flush_sync(struct p2m_domain *p2m) {}
>> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
>> index 010ce8c9eb..ed1b6dd40f 100644
>> --- a/xen/arch/arm/include/asm/p2m.h
>> +++ b/xen/arch/arm/include/asm/p2m.h
>> @@ -48,8 +48,13 @@ struct p2m_domain {
>>      /* Current VMID in use */
>>      uint16_t vmid;
>>  +#ifdef CONFIG_MMU
>>      /* Current Translation Table Base Register for the p2m */
>>      uint64_t vttbr;
>> +#else
>> +    /* Current Virtualization System Control Register for the p2m */
>> +    register_t vsctlr;
> This exist only for Arm64 MPU. Should we enclose this in ARM_64  ?

I can see it exists in armv8-r aarch32 [2].

Cheers,
Luca

[1] https://developer.arm.com/documentation/ddi0628/latest
[2] https://developer.arm.com/documentation/ddi0568/latest/