diff mbox series

[kvm-unit-tests,08/33] arm: realm: Make uart available before MMU is enabled

Message ID 20240412103408.2706058-9-suzuki.poulose@arm.com (mailing list archive)
State New, archived
Headers show
Series Support for Arm Confidential Compute Architecture | expand

Commit Message

Suzuki K Poulose April 12, 2024, 10:33 a.m. UTC
From: Joey Gouly <joey.gouly@arm.com>

A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.
This is modelled as a PTE attribute, but is actually part of the address.

So, when MMU is disabled, the "physical address" must reflect this bit set. We
access the UART early before the MMU is enabled. So, make sure the UART is
accessed always with the bit set.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 lib/arm/asm/pgtable.h   |  5 +++++
 lib/arm/io.c            | 24 +++++++++++++++++++++++-
 lib/arm64/asm/pgtable.h |  5 +++++
 3 files changed, 33 insertions(+), 1 deletion(-)

Comments

Alexandru Elisei April 22, 2024, 11:58 a.m. UTC | #1
Hi,

On Fri, Apr 12, 2024 at 11:33:43AM +0100, Suzuki K Poulose wrote:
> From: Joey Gouly <joey.gouly@arm.com>
> 
> A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.
> This is modelled as a PTE attribute, but is actually part of the address.
> 
> So, when MMU is disabled, the "physical address" must reflect this bit set. We
> access the UART early before the MMU is enabled. So, make sure the UART is
> accessed always with the bit set.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  lib/arm/asm/pgtable.h   |  5 +++++
>  lib/arm/io.c            | 24 +++++++++++++++++++++++-
>  lib/arm64/asm/pgtable.h |  5 +++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
> index 350039ff..7e85e7c6 100644
> --- a/lib/arm/asm/pgtable.h
> +++ b/lib/arm/asm/pgtable.h
> @@ -112,4 +112,9 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
>  	return pte_offset(pmd, addr);
>  }
>  
> +static inline unsigned long arm_shared_phys_alias(void *x)
> +{
> +	return ((unsigned long)(x) | PTE_NS_SHARED);
> +}

Is it allowed for a realm to run in aarch32 mode?

> +
>  #endif /* _ASMARM_PGTABLE_H_ */
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index 836fa854..127727e4 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -15,6 +15,8 @@
>  #include <asm/psci.h>
>  #include <asm/spinlock.h>
>  #include <asm/io.h>
> +#include <asm/mmu-api.h>
> +#include <asm/pgtable.h>
>  
>  #include "io.h"
>  
> @@ -30,6 +32,24 @@ static struct spinlock uart_lock;
>  static volatile u8 *uart0_base = UART_EARLY_BASE;
>  bool is_pl011_uart;
>  
> +static inline volatile u8 *get_uart_base(void)
> +{
> +	/*
> +	 * The address of the UART base may be different
> +	 * based on whether we are running with/without
> +	 * MMU enabled.
> +	 *
> +	 * For realms, we must force to use the shared physical
> +	 * alias with MMU disabled, to make sure the I/O can
> +	 * be emulated.
> +	 * When the MMU is turned ON, the mappings are created
> +	 * appropriately.
> +	 */
> +	if (mmu_enabled())
> +		return uart0_base;
> +	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
> +}
> +
>  static void uart0_init_fdt(void)
>  {
>  	/*
> @@ -109,9 +129,11 @@ void io_init(void)
>  
>  void puts(const char *s)
>  {
> +	volatile u8 *uart_base = get_uart_base();
> +
>  	spin_lock(&uart_lock);
>  	while (*s)
> -		writeb(*s++, uart0_base);
> +		writeb(*s++, uart_base);
>  	spin_unlock(&uart_lock);
>  }
>  
> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
> index 5b9f40b0..871c03e9 100644
> --- a/lib/arm64/asm/pgtable.h
> +++ b/lib/arm64/asm/pgtable.h
> @@ -28,6 +28,11 @@ extern unsigned long prot_ns_shared;
>  */
>  #define PTE_NS_SHARED		(prot_ns_shared)
>  
> +static inline unsigned long arm_shared_phys_alias(void *addr)
> +{
> +	return ((unsigned long)addr | PTE_NS_SHARED);
> +}

Have you considered specifying the correct UART address at compile time using
./configure --earlycon?

Thanks,
Alex
> +
>  /*
>   * Highest possible physical address supported.
>   */
> -- 
> 2.34.1
>
Suzuki K Poulose April 22, 2024, 12:09 p.m. UTC | #2
Hi Alexandru

On 22/04/2024 12:58, Alexandru Elisei wrote:
> Hi,
> 
> On Fri, Apr 12, 2024 at 11:33:43AM +0100, Suzuki K Poulose wrote:
>> From: Joey Gouly <joey.gouly@arm.com>
>>
>> A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.
>> This is modelled as a PTE attribute, but is actually part of the address.
>>
>> So, when MMU is disabled, the "physical address" must reflect this bit set. We
>> access the UART early before the MMU is enabled. So, make sure the UART is
>> accessed always with the bit set.
>>
>> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   lib/arm/asm/pgtable.h   |  5 +++++
>>   lib/arm/io.c            | 24 +++++++++++++++++++++++-
>>   lib/arm64/asm/pgtable.h |  5 +++++
>>   3 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
>> index 350039ff..7e85e7c6 100644
>> --- a/lib/arm/asm/pgtable.h
>> +++ b/lib/arm/asm/pgtable.h
>> @@ -112,4 +112,9 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
>>   	return pte_offset(pmd, addr);
>>   }
>>   
>> +static inline unsigned long arm_shared_phys_alias(void *x)
>> +{
>> +	return ((unsigned long)(x) | PTE_NS_SHARED);
>> +}
> 
> Is it allowed for a realm to run in aarch32 mode?

No. Realm EL1 must be Aarch64.

> 
>> +
>>   #endif /* _ASMARM_PGTABLE_H_ */
>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>> index 836fa854..127727e4 100644
>> --- a/lib/arm/io.c
>> +++ b/lib/arm/io.c
>> @@ -15,6 +15,8 @@
>>   #include <asm/psci.h>
>>   #include <asm/spinlock.h>
>>   #include <asm/io.h>
>> +#include <asm/mmu-api.h>
>> +#include <asm/pgtable.h>
>>   
>>   #include "io.h"
>>   
>> @@ -30,6 +32,24 @@ static struct spinlock uart_lock;
>>   static volatile u8 *uart0_base = UART_EARLY_BASE;
>>   bool is_pl011_uart;
>>   
>> +static inline volatile u8 *get_uart_base(void)
>> +{
>> +	/*
>> +	 * The address of the UART base may be different
>> +	 * based on whether we are running with/without
>> +	 * MMU enabled.
>> +	 *
>> +	 * For realms, we must force to use the shared physical
>> +	 * alias with MMU disabled, to make sure the I/O can
>> +	 * be emulated.
>> +	 * When the MMU is turned ON, the mappings are created
>> +	 * appropriately.
>> +	 */
>> +	if (mmu_enabled())
>> +		return uart0_base;
>> +	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
>> +}
>> +
>>   static void uart0_init_fdt(void)
>>   {
>>   	/*
>> @@ -109,9 +129,11 @@ void io_init(void)
>>   
>>   void puts(const char *s)
>>   {
>> +	volatile u8 *uart_base = get_uart_base();
>> +
>>   	spin_lock(&uart_lock);
>>   	while (*s)
>> -		writeb(*s++, uart0_base);
>> +		writeb(*s++, uart_base);
>>   	spin_unlock(&uart_lock);
>>   }
>>   
>> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
>> index 5b9f40b0..871c03e9 100644
>> --- a/lib/arm64/asm/pgtable.h
>> +++ b/lib/arm64/asm/pgtable.h
>> @@ -28,6 +28,11 @@ extern unsigned long prot_ns_shared;
>>   */
>>   #define PTE_NS_SHARED		(prot_ns_shared)
>>   
>> +static inline unsigned long arm_shared_phys_alias(void *addr)
>> +{
>> +	return ((unsigned long)addr | PTE_NS_SHARED);
>> +}
> 
> Have you considered specifying the correct UART address at compile time using
> ./configure --earlycon?

Do you mean

./configure --earlycon=<NS-Alias-normal-UART-Address> for Realms ?

If so, there are multiple issues with that :

1. A payload could be run in a normal VM or a Realm VM. Having the above
restricts using the same payload in different worlds. (e.g., comparison).

2. The IPA width of the Realm and thus the PTE_NS_SHARED is dynamic
and really depends on what the VMM decides to choose the IPA size.
(Could be based on user input).

If any of the above fails, and a wrong earlycon address could result
in "Synchronouse External Abort" into the Realm (if it is in protected
IPA).

Suzuki
> 
> Thanks,
> Alex
>> +
>>   /*
>>    * Highest possible physical address supported.
>>    */
>> -- 
>> 2.34.1
>>
Alexandru Elisei April 22, 2024, 12:23 p.m. UTC | #3
Hi,

On Mon, Apr 22, 2024 at 01:09:13PM +0100, Suzuki K Poulose wrote:
> Hi Alexandru
> 
> On 22/04/2024 12:58, Alexandru Elisei wrote:
> > Hi,
> > 
> > On Fri, Apr 12, 2024 at 11:33:43AM +0100, Suzuki K Poulose wrote:
> > > From: Joey Gouly <joey.gouly@arm.com>
> > > 
> > > A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.
> > > This is modelled as a PTE attribute, but is actually part of the address.
> > > 
> > > So, when MMU is disabled, the "physical address" must reflect this bit set. We
> > > access the UART early before the MMU is enabled. So, make sure the UART is
> > > accessed always with the bit set.
> > > 
> > > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > ---
> > >   lib/arm/asm/pgtable.h   |  5 +++++
> > >   lib/arm/io.c            | 24 +++++++++++++++++++++++-
> > >   lib/arm64/asm/pgtable.h |  5 +++++
> > >   3 files changed, 33 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
> > > index 350039ff..7e85e7c6 100644
> > > --- a/lib/arm/asm/pgtable.h
> > > +++ b/lib/arm/asm/pgtable.h
> > > @@ -112,4 +112,9 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
> > >   	return pte_offset(pmd, addr);
> > >   }
> > > +static inline unsigned long arm_shared_phys_alias(void *x)
> > > +{
> > > +	return ((unsigned long)(x) | PTE_NS_SHARED);
> > > +}
> > 
> > Is it allowed for a realm to run in aarch32 mode?
> 
> No. Realm EL1 must be Aarch64.

Ok, then can you make the above function return the original address?

> 
> > 
> > > +
> > >   #endif /* _ASMARM_PGTABLE_H_ */
> > > diff --git a/lib/arm/io.c b/lib/arm/io.c
> > > index 836fa854..127727e4 100644
> > > --- a/lib/arm/io.c
> > > +++ b/lib/arm/io.c
> > > @@ -15,6 +15,8 @@
> > >   #include <asm/psci.h>
> > >   #include <asm/spinlock.h>
> > >   #include <asm/io.h>
> > > +#include <asm/mmu-api.h>
> > > +#include <asm/pgtable.h>
> > >   #include "io.h"
> > > @@ -30,6 +32,24 @@ static struct spinlock uart_lock;
> > >   static volatile u8 *uart0_base = UART_EARLY_BASE;
> > >   bool is_pl011_uart;
> > > +static inline volatile u8 *get_uart_base(void)
> > > +{
> > > +	/*
> > > +	 * The address of the UART base may be different
> > > +	 * based on whether we are running with/without
> > > +	 * MMU enabled.
> > > +	 *
> > > +	 * For realms, we must force to use the shared physical
> > > +	 * alias with MMU disabled, to make sure the I/O can
> > > +	 * be emulated.
> > > +	 * When the MMU is turned ON, the mappings are created
> > > +	 * appropriately.
> > > +	 */
> > > +	if (mmu_enabled())
> > > +		return uart0_base;
> > > +	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
> > > +}
> > > +
> > >   static void uart0_init_fdt(void)
> > >   {
> > >   	/*
> > > @@ -109,9 +129,11 @@ void io_init(void)
> > >   void puts(const char *s)
> > >   {
> > > +	volatile u8 *uart_base = get_uart_base();

Is it just my email client or is the indentation missing here?

> > > +
> > >   	spin_lock(&uart_lock);
> > >   	while (*s)
> > > -		writeb(*s++, uart0_base);
> > > +		writeb(*s++, uart_base);
> > >   	spin_unlock(&uart_lock);
> > >   }
> > > diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
> > > index 5b9f40b0..871c03e9 100644
> > > --- a/lib/arm64/asm/pgtable.h
> > > +++ b/lib/arm64/asm/pgtable.h
> > > @@ -28,6 +28,11 @@ extern unsigned long prot_ns_shared;
> > >   */
> > >   #define PTE_NS_SHARED		(prot_ns_shared)
> > > +static inline unsigned long arm_shared_phys_alias(void *addr)
> > > +{
> > > +	return ((unsigned long)addr | PTE_NS_SHARED);
> > > +}
> > 
> > Have you considered specifying the correct UART address at compile time using
> > ./configure --earlycon?
> 
> Do you mean
> 
> ./configure --earlycon=<NS-Alias-normal-UART-Address> for Realms ?
> 
> If so, there are multiple issues with that :
> 
> 1. A payload could be run in a normal VM or a Realm VM. Having the above
> restricts using the same payload in different worlds. (e.g., comparison).
> 
> 2. The IPA width of the Realm and thus the PTE_NS_SHARED is dynamic
> and really depends on what the VMM decides to choose the IPA size.
> (Could be based on user input).

Does it depend on the chosen IPA size, or on the VA size, or on both?
Because uart0_base is used with both the MMU on and off.

Thanks,
Alex

> 
> If any of the above fails, and a wrong earlycon address could result
> in "Synchronouse External Abort" into the Realm (if it is in protected
> IPA).
> 
> Suzuki
> > 
> > Thanks,
> > Alex
> > > +
> > >   /*
> > >    * Highest possible physical address supported.
> > >    */
> > > -- 
> > > 2.34.1
> > > 
>
Alexandru Elisei April 22, 2024, 12:36 p.m. UTC | #4
Hi,

On Mon, Apr 22, 2024 at 01:23:05PM +0100, Alexandru Elisei wrote:
> Hi,
> 
> On Mon, Apr 22, 2024 at 01:09:13PM +0100, Suzuki K Poulose wrote:
> > Hi Alexandru
> > 
> > On 22/04/2024 12:58, Alexandru Elisei wrote:
> > > Hi,
> > > 
> > > On Fri, Apr 12, 2024 at 11:33:43AM +0100, Suzuki K Poulose wrote:
> > > > From: Joey Gouly <joey.gouly@arm.com>
> > > > 
> > > > A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.
> > > > This is modelled as a PTE attribute, but is actually part of the address.
> > > > 
> > > > So, when MMU is disabled, the "physical address" must reflect this bit set. We
> > > > access the UART early before the MMU is enabled. So, make sure the UART is
> > > > accessed always with the bit set.
> > > > 
> > > > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > ---
> > > >   lib/arm/asm/pgtable.h   |  5 +++++
> > > >   lib/arm/io.c            | 24 +++++++++++++++++++++++-
> > > >   lib/arm64/asm/pgtable.h |  5 +++++
> > > >   3 files changed, 33 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
> > > > index 350039ff..7e85e7c6 100644
> > > > --- a/lib/arm/asm/pgtable.h
> > > > +++ b/lib/arm/asm/pgtable.h
> > > > @@ -112,4 +112,9 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
> > > >   	return pte_offset(pmd, addr);
> > > >   }
> > > > +static inline unsigned long arm_shared_phys_alias(void *x)
> > > > +{
> > > > +	return ((unsigned long)(x) | PTE_NS_SHARED);
> > > > +}
> > > 
> > > Is it allowed for a realm to run in aarch32 mode?
> > 
> > No. Realm EL1 must be Aarch64.
> 
> Ok, then can you make the above function return the original address?
> 
> > 
> > > 
> > > > +
> > > >   #endif /* _ASMARM_PGTABLE_H_ */
> > > > diff --git a/lib/arm/io.c b/lib/arm/io.c
> > > > index 836fa854..127727e4 100644
> > > > --- a/lib/arm/io.c
> > > > +++ b/lib/arm/io.c
> > > > @@ -15,6 +15,8 @@
> > > >   #include <asm/psci.h>
> > > >   #include <asm/spinlock.h>
> > > >   #include <asm/io.h>
> > > > +#include <asm/mmu-api.h>
> > > > +#include <asm/pgtable.h>
> > > >   #include "io.h"
> > > > @@ -30,6 +32,24 @@ static struct spinlock uart_lock;
> > > >   static volatile u8 *uart0_base = UART_EARLY_BASE;
> > > >   bool is_pl011_uart;
> > > > +static inline volatile u8 *get_uart_base(void)
> > > > +{
> > > > +	/*
> > > > +	 * The address of the UART base may be different
> > > > +	 * based on whether we are running with/without
> > > > +	 * MMU enabled.
> > > > +	 *
> > > > +	 * For realms, we must force to use the shared physical
> > > > +	 * alias with MMU disabled, to make sure the I/O can
> > > > +	 * be emulated.
> > > > +	 * When the MMU is turned ON, the mappings are created
> > > > +	 * appropriately.
> > > > +	 */
> > > > +	if (mmu_enabled())
> > > > +		return uart0_base;
> > > > +	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
> > > > +}
> > > > +
> > > >   static void uart0_init_fdt(void)
> > > >   {
> > > >   	/*
> > > > @@ -109,9 +129,11 @@ void io_init(void)
> > > >   void puts(const char *s)
> > > >   {
> > > > +	volatile u8 *uart_base = get_uart_base();
> 
> Is it just my email client or is the indentation missing here?
> 
> > > > +
> > > >   	spin_lock(&uart_lock);
> > > >   	while (*s)
> > > > -		writeb(*s++, uart0_base);
> > > > +		writeb(*s++, uart_base);
> > > >   	spin_unlock(&uart_lock);
> > > >   }
> > > > diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
> > > > index 5b9f40b0..871c03e9 100644
> > > > --- a/lib/arm64/asm/pgtable.h
> > > > +++ b/lib/arm64/asm/pgtable.h
> > > > @@ -28,6 +28,11 @@ extern unsigned long prot_ns_shared;
> > > >   */
> > > >   #define PTE_NS_SHARED		(prot_ns_shared)
> > > > +static inline unsigned long arm_shared_phys_alias(void *addr)
> > > > +{
> > > > +	return ((unsigned long)addr | PTE_NS_SHARED);
> > > > +}
> > > 
> > > Have you considered specifying the correct UART address at compile time using
> > > ./configure --earlycon?
> > 
> > Do you mean
> > 
> > ./configure --earlycon=<NS-Alias-normal-UART-Address> for Realms ?
> > 
> > If so, there are multiple issues with that :
> > 
> > 1. A payload could be run in a normal VM or a Realm VM. Having the above
> > restricts using the same payload in different worlds. (e.g., comparison).
> > 
> > 2. The IPA width of the Realm and thus the PTE_NS_SHARED is dynamic
> > and really depends on what the VMM decides to choose the IPA size.
> > (Could be based on user input).
> 
> Does it depend on the chosen IPA size, or on the VA size, or on both?
> Because uart0_base is used with both the MMU on and off.

Oh, ok, my bad, I hadn't realized that the uart address is modified only if the
MMU is not enabled. So it's actually only the IPA that must have the bit set.

I have to say that a PTE attribute, as it is called in the commit message, which
has a variable bit position that depends on the output address size is quite
surprising. I was under the impression that the bit position is constant when I
made the suggestion.

Yeah, it makes sense to have a helper to set the bit based on whether or not the
MMU is enabled.

Thanks,
Alex

> 
> Thanks,
> Alex
> 
> > 
> > If any of the above fails, and a wrong earlycon address could result
> > in "Synchronouse External Abort" into the Realm (if it is in protected
> > IPA).
> > 
> > Suzuki
> > > 
> > > Thanks,
> > > Alex
> > > > +
> > > >   /*
> > > >    * Highest possible physical address supported.
> > > >    */
> > > > -- 
> > > > 2.34.1
> > > > 
> > 
>
Suzuki K Poulose April 22, 2024, 1:09 p.m. UTC | #5
Hi Alex

On 22/04/2024 13:36, Alexandru Elisei wrote:
> Hi,
> 
> On Mon, Apr 22, 2024 at 01:23:05PM +0100, Alexandru Elisei wrote:
>> Hi,
>>
>> On Mon, Apr 22, 2024 at 01:09:13PM +0100, Suzuki K Poulose wrote:
>>> Hi Alexandru
>>>
>>> On 22/04/2024 12:58, Alexandru Elisei wrote:
>>>> Hi,
>>>>
>>>> On Fri, Apr 12, 2024 at 11:33:43AM +0100, Suzuki K Poulose wrote:
>>>>> From: Joey Gouly <joey.gouly@arm.com>
>>>>>
>>>>> A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.
>>>>> This is modelled as a PTE attribute, but is actually part of the address.
>>>>>
>>>>> So, when MMU is disabled, the "physical address" must reflect this bit set. We
>>>>> access the UART early before the MMU is enabled. So, make sure the UART is
>>>>> accessed always with the bit set.
>>>>>
>>>>> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>> ---
>>>>>    lib/arm/asm/pgtable.h   |  5 +++++
>>>>>    lib/arm/io.c            | 24 +++++++++++++++++++++++-
>>>>>    lib/arm64/asm/pgtable.h |  5 +++++
>>>>>    3 files changed, 33 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
>>>>> index 350039ff..7e85e7c6 100644
>>>>> --- a/lib/arm/asm/pgtable.h
>>>>> +++ b/lib/arm/asm/pgtable.h
>>>>> @@ -112,4 +112,9 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
>>>>>    	return pte_offset(pmd, addr);
>>>>>    }
>>>>> +static inline unsigned long arm_shared_phys_alias(void *x)
>>>>> +{
>>>>> +	return ((unsigned long)(x) | PTE_NS_SHARED);
>>>>> +}
>>>>
>>>> Is it allowed for a realm to run in aarch32 mode?
>>>
>>> No. Realm EL1 must be Aarch64.
>>
>> Ok, then can you make the above function return the original address?
>>
>>>
>>>>
>>>>> +
>>>>>    #endif /* _ASMARM_PGTABLE_H_ */
>>>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>>>>> index 836fa854..127727e4 100644
>>>>> --- a/lib/arm/io.c
>>>>> +++ b/lib/arm/io.c
>>>>> @@ -15,6 +15,8 @@
>>>>>    #include <asm/psci.h>
>>>>>    #include <asm/spinlock.h>
>>>>>    #include <asm/io.h>
>>>>> +#include <asm/mmu-api.h>
>>>>> +#include <asm/pgtable.h>
>>>>>    #include "io.h"
>>>>> @@ -30,6 +32,24 @@ static struct spinlock uart_lock;
>>>>>    static volatile u8 *uart0_base = UART_EARLY_BASE;
>>>>>    bool is_pl011_uart;
>>>>> +static inline volatile u8 *get_uart_base(void)
>>>>> +{
>>>>> +	/*
>>>>> +	 * The address of the UART base may be different
>>>>> +	 * based on whether we are running with/without
>>>>> +	 * MMU enabled.
>>>>> +	 *
>>>>> +	 * For realms, we must force to use the shared physical
>>>>> +	 * alias with MMU disabled, to make sure the I/O can
>>>>> +	 * be emulated.
>>>>> +	 * When the MMU is turned ON, the mappings are created
>>>>> +	 * appropriately.
>>>>> +	 */
>>>>> +	if (mmu_enabled())
>>>>> +		return uart0_base;
>>>>> +	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
>>>>> +}
>>>>> +
>>>>>    static void uart0_init_fdt(void)
>>>>>    {
>>>>>    	/*
>>>>> @@ -109,9 +129,11 @@ void io_init(void)
>>>>>    void puts(const char *s)
>>>>>    {
>>>>> +	volatile u8 *uart_base = get_uart_base();
>>
>> Is it just my email client or is the indentation missing here?

I can confirm that the original patch/email is correctly indented
with TAB.

>>
>>>>> +
>>>>>    	spin_lock(&uart_lock);
>>>>>    	while (*s)
>>>>> -		writeb(*s++, uart0_base);
>>>>> +		writeb(*s++, uart_base);
>>>>>    	spin_unlock(&uart_lock);
>>>>>    }
>>>>> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
>>>>> index 5b9f40b0..871c03e9 100644
>>>>> --- a/lib/arm64/asm/pgtable.h
>>>>> +++ b/lib/arm64/asm/pgtable.h
>>>>> @@ -28,6 +28,11 @@ extern unsigned long prot_ns_shared;
>>>>>    */
>>>>>    #define PTE_NS_SHARED		(prot_ns_shared)
>>>>> +static inline unsigned long arm_shared_phys_alias(void *addr)
>>>>> +{
>>>>> +	return ((unsigned long)addr | PTE_NS_SHARED);
>>>>> +}
>>>>
>>>> Have you considered specifying the correct UART address at compile time using
>>>> ./configure --earlycon?
>>>
>>> Do you mean
>>>
>>> ./configure --earlycon=<NS-Alias-normal-UART-Address> for Realms ?
>>>
>>> If so, there are multiple issues with that :
>>>
>>> 1. A payload could be run in a normal VM or a Realm VM. Having the above
>>> restricts using the same payload in different worlds. (e.g., comparison).
>>>
>>> 2. The IPA width of the Realm and thus the PTE_NS_SHARED is dynamic
>>> and really depends on what the VMM decides to choose the IPA size.
>>> (Could be based on user input).
>>
>> Does it depend on the chosen IPA size, or on the VA size, or on both?
>> Because uart0_base is used with both the MMU on and off.
> 
> Oh, ok, my bad, I hadn't realized that the uart address is modified only if the
> MMU is not enabled. So it's actually only the IPA that must have the bit set.
> 
> I have to say that a PTE attribute, as it is called in the commit message, which
> has a variable bit position that depends on the output address size is quite
> surprising. I was under the impression that the bit position is constant when I
> made the suggestion.

I understand, may be we could make that clear in the commit description.

> 
> Yeah, it makes sense to have a helper to set the bit based on whether or not the
> MMU is enabled.

Thanks

Suzuki
Alexandru Elisei April 22, 2024, 3:38 p.m. UTC | #6
Hi,

On Fri, Apr 12, 2024 at 11:33:43AM +0100, Suzuki K Poulose wrote:
> From: Joey Gouly <joey.gouly@arm.com>
> 
> A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.

What entity requires that a realm must access I/O mappings with the
PTE_NS_SHARED bit set? Is that an architectural requirement? Or is it an
implementation choice made by the VMM and/or KVM?

Thanks,
Alex

> This is modelled as a PTE attribute, but is actually part of the address.
> 
> So, when MMU is disabled, the "physical address" must reflect this bit set. We
> access the UART early before the MMU is enabled. So, make sure the UART is
> accessed always with the bit set.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  lib/arm/asm/pgtable.h   |  5 +++++
>  lib/arm/io.c            | 24 +++++++++++++++++++++++-
>  lib/arm64/asm/pgtable.h |  5 +++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
> index 350039ff..7e85e7c6 100644
> --- a/lib/arm/asm/pgtable.h
> +++ b/lib/arm/asm/pgtable.h
> @@ -112,4 +112,9 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
>  	return pte_offset(pmd, addr);
>  }
>  
> +static inline unsigned long arm_shared_phys_alias(void *x)
> +{
> +	return ((unsigned long)(x) | PTE_NS_SHARED);
> +}
> +
>  #endif /* _ASMARM_PGTABLE_H_ */
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index 836fa854..127727e4 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -15,6 +15,8 @@
>  #include <asm/psci.h>
>  #include <asm/spinlock.h>
>  #include <asm/io.h>
> +#include <asm/mmu-api.h>
> +#include <asm/pgtable.h>
>  
>  #include "io.h"
>  
> @@ -30,6 +32,24 @@ static struct spinlock uart_lock;
>  static volatile u8 *uart0_base = UART_EARLY_BASE;
>  bool is_pl011_uart;
>  
> +static inline volatile u8 *get_uart_base(void)
> +{
> +	/*
> +	 * The address of the UART base may be different
> +	 * based on whether we are running with/without
> +	 * MMU enabled.
> +	 *
> +	 * For realms, we must force to use the shared physical
> +	 * alias with MMU disabled, to make sure the I/O can
> +	 * be emulated.
> +	 * When the MMU is turned ON, the mappings are created
> +	 * appropriately.
> +	 */
> +	if (mmu_enabled())
> +		return uart0_base;
> +	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
> +}
> +
>  static void uart0_init_fdt(void)
>  {
>  	/*
> @@ -109,9 +129,11 @@ void io_init(void)
>  
>  void puts(const char *s)
>  {
> +	volatile u8 *uart_base = get_uart_base();
> +
>  	spin_lock(&uart_lock);
>  	while (*s)
> -		writeb(*s++, uart0_base);
> +		writeb(*s++, uart_base);
>  	spin_unlock(&uart_lock);
>  }
>  
> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
> index 5b9f40b0..871c03e9 100644
> --- a/lib/arm64/asm/pgtable.h
> +++ b/lib/arm64/asm/pgtable.h
> @@ -28,6 +28,11 @@ extern unsigned long prot_ns_shared;
>  */
>  #define PTE_NS_SHARED		(prot_ns_shared)
>  
> +static inline unsigned long arm_shared_phys_alias(void *addr)
> +{
> +	return ((unsigned long)addr | PTE_NS_SHARED);
> +}
> +
>  /*
>   * Highest possible physical address supported.
>   */
> -- 
> 2.34.1
>
Suzuki K Poulose April 22, 2024, 4:05 p.m. UTC | #7
On 22/04/2024 16:38, Alexandru Elisei wrote:
> Hi,
> 
> On Fri, Apr 12, 2024 at 11:33:43AM +0100, Suzuki K Poulose wrote:
>> From: Joey Gouly <joey.gouly@arm.com>
>>
>> A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.
> 
> What entity requires that a realm must access I/O mappings with the
> PTE_NS_SHARED bit set? Is that an architectural requirement? Or is it an
> implementation choice made by the VMM and/or KVM?

RMM spec. An MMIO access in the Protected IPA must be emulated by Realm
world. If an MMIO access must be emulated by NS Host, it must be in the 
Unprotected IPA.

Technically, a VMM could create a memory map, where the NS emulated I/O
are kept in the unprotected (upper) half.

Or the VMM retains the current model and expects the Realm to use the 
"Unprotected" alias.


Either way, applying the PTE_NS_SHARED attribute doesn't change anything 
and works for both the models, as far as the Realm is concerned.


Suzuki




> 
> Thanks,
> Alex
> 
>> This is modelled as a PTE attribute, but is actually part of the address.
>>
>> So, when MMU is disabled, the "physical address" must reflect this bit set. We
>> access the UART early before the MMU is enabled. So, make sure the UART is
>> accessed always with the bit set.
>>
>> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   lib/arm/asm/pgtable.h   |  5 +++++
>>   lib/arm/io.c            | 24 +++++++++++++++++++++++-
>>   lib/arm64/asm/pgtable.h |  5 +++++
>>   3 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
>> index 350039ff..7e85e7c6 100644
>> --- a/lib/arm/asm/pgtable.h
>> +++ b/lib/arm/asm/pgtable.h
>> @@ -112,4 +112,9 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
>>   	return pte_offset(pmd, addr);
>>   }
>>   
>> +static inline unsigned long arm_shared_phys_alias(void *x)
>> +{
>> +	return ((unsigned long)(x) | PTE_NS_SHARED);
>> +}
>> +
>>   #endif /* _ASMARM_PGTABLE_H_ */
>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>> index 836fa854..127727e4 100644
>> --- a/lib/arm/io.c
>> +++ b/lib/arm/io.c
>> @@ -15,6 +15,8 @@
>>   #include <asm/psci.h>
>>   #include <asm/spinlock.h>
>>   #include <asm/io.h>
>> +#include <asm/mmu-api.h>
>> +#include <asm/pgtable.h>
>>   
>>   #include "io.h"
>>   
>> @@ -30,6 +32,24 @@ static struct spinlock uart_lock;
>>   static volatile u8 *uart0_base = UART_EARLY_BASE;
>>   bool is_pl011_uart;
>>   
>> +static inline volatile u8 *get_uart_base(void)
>> +{
>> +	/*
>> +	 * The address of the UART base may be different
>> +	 * based on whether we are running with/without
>> +	 * MMU enabled.
>> +	 *
>> +	 * For realms, we must force to use the shared physical
>> +	 * alias with MMU disabled, to make sure the I/O can
>> +	 * be emulated.
>> +	 * When the MMU is turned ON, the mappings are created
>> +	 * appropriately.
>> +	 */
>> +	if (mmu_enabled())
>> +		return uart0_base;
>> +	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
>> +}
>> +
>>   static void uart0_init_fdt(void)
>>   {
>>   	/*
>> @@ -109,9 +129,11 @@ void io_init(void)
>>   
>>   void puts(const char *s)
>>   {
>> +	volatile u8 *uart_base = get_uart_base();
>> +
>>   	spin_lock(&uart_lock);
>>   	while (*s)
>> -		writeb(*s++, uart0_base);
>> +		writeb(*s++, uart_base);
>>   	spin_unlock(&uart_lock);
>>   }
>>   
>> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
>> index 5b9f40b0..871c03e9 100644
>> --- a/lib/arm64/asm/pgtable.h
>> +++ b/lib/arm64/asm/pgtable.h
>> @@ -28,6 +28,11 @@ extern unsigned long prot_ns_shared;
>>   */
>>   #define PTE_NS_SHARED		(prot_ns_shared)
>>   
>> +static inline unsigned long arm_shared_phys_alias(void *addr)
>> +{
>> +	return ((unsigned long)addr | PTE_NS_SHARED);
>> +}
>> +
>>   /*
>>    * Highest possible physical address supported.
>>    */
>> -- 
>> 2.34.1
>>
Alexandru Elisei April 22, 2024, 4:15 p.m. UTC | #8
Hi,

On Mon, Apr 22, 2024 at 05:05:54PM +0100, Suzuki K Poulose wrote:
> On 22/04/2024 16:38, Alexandru Elisei wrote:
> > Hi,
> > 
> > On Fri, Apr 12, 2024 at 11:33:43AM +0100, Suzuki K Poulose wrote:
> > > From: Joey Gouly <joey.gouly@arm.com>
> > > 
> > > A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.
> > 
> > What entity requires that a realm must access I/O mappings with the
> > PTE_NS_SHARED bit set? Is that an architectural requirement? Or is it an
> > implementation choice made by the VMM and/or KVM?
> 
> RMM spec. An MMIO access in the Protected IPA must be emulated by Realm
> world. If an MMIO access must be emulated by NS Host, it must be in the
> Unprotected IPA.

That's not exactly what I was asking. I was curious to know how a realm
knows if a MMIO access is emulated by the Realm, and thus it must use a
protected address, or it's emulated by the non-secure host, and it must use
an unprotected address.

Thanks,
Alex

> 
> Technically, a VMM could create a memory map, where the NS emulated I/O
> are kept in the unprotected (upper) half.
> 
> Or the VMM retains the current model and expects the Realm to use the
> "Unprotected" alias.
> 
> 
> Either way, applying the PTE_NS_SHARED attribute doesn't change anything and
> works for both the models, as far as the Realm is concerned.
> 
> 
> Suzuki
> 
> 
> 
> 
> > 
> > Thanks,
> > Alex
> > 
> > > This is modelled as a PTE attribute, but is actually part of the address.
> > > 
> > > So, when MMU is disabled, the "physical address" must reflect this bit set. We
> > > access the UART early before the MMU is enabled. So, make sure the UART is
> > > accessed always with the bit set.
> > > 
> > > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > ---
> > >   lib/arm/asm/pgtable.h   |  5 +++++
> > >   lib/arm/io.c            | 24 +++++++++++++++++++++++-
> > >   lib/arm64/asm/pgtable.h |  5 +++++
> > >   3 files changed, 33 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
> > > index 350039ff..7e85e7c6 100644
> > > --- a/lib/arm/asm/pgtable.h
> > > +++ b/lib/arm/asm/pgtable.h
> > > @@ -112,4 +112,9 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
> > >   	return pte_offset(pmd, addr);
> > >   }
> > > +static inline unsigned long arm_shared_phys_alias(void *x)
> > > +{
> > > +	return ((unsigned long)(x) | PTE_NS_SHARED);
> > > +}
> > > +
> > >   #endif /* _ASMARM_PGTABLE_H_ */
> > > diff --git a/lib/arm/io.c b/lib/arm/io.c
> > > index 836fa854..127727e4 100644
> > > --- a/lib/arm/io.c
> > > +++ b/lib/arm/io.c
> > > @@ -15,6 +15,8 @@
> > >   #include <asm/psci.h>
> > >   #include <asm/spinlock.h>
> > >   #include <asm/io.h>
> > > +#include <asm/mmu-api.h>
> > > +#include <asm/pgtable.h>
> > >   #include "io.h"
> > > @@ -30,6 +32,24 @@ static struct spinlock uart_lock;
> > >   static volatile u8 *uart0_base = UART_EARLY_BASE;
> > >   bool is_pl011_uart;
> > > +static inline volatile u8 *get_uart_base(void)
> > > +{
> > > +	/*
> > > +	 * The address of the UART base may be different
> > > +	 * based on whether we are running with/without
> > > +	 * MMU enabled.
> > > +	 *
> > > +	 * For realms, we must force to use the shared physical
> > > +	 * alias with MMU disabled, to make sure the I/O can
> > > +	 * be emulated.
> > > +	 * When the MMU is turned ON, the mappings are created
> > > +	 * appropriately.
> > > +	 */
> > > +	if (mmu_enabled())
> > > +		return uart0_base;
> > > +	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
> > > +}
> > > +
> > >   static void uart0_init_fdt(void)
> > >   {
> > >   	/*
> > > @@ -109,9 +129,11 @@ void io_init(void)
> > >   void puts(const char *s)
> > >   {
> > > +	volatile u8 *uart_base = get_uart_base();
> > > +
> > >   	spin_lock(&uart_lock);
> > >   	while (*s)
> > > -		writeb(*s++, uart0_base);
> > > +		writeb(*s++, uart_base);
> > >   	spin_unlock(&uart_lock);
> > >   }
> > > diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
> > > index 5b9f40b0..871c03e9 100644
> > > --- a/lib/arm64/asm/pgtable.h
> > > +++ b/lib/arm64/asm/pgtable.h
> > > @@ -28,6 +28,11 @@ extern unsigned long prot_ns_shared;
> > >   */
> > >   #define PTE_NS_SHARED		(prot_ns_shared)
> > > +static inline unsigned long arm_shared_phys_alias(void *addr)
> > > +{
> > > +	return ((unsigned long)addr | PTE_NS_SHARED);
> > > +}
> > > +
> > >   /*
> > >    * Highest possible physical address supported.
> > >    */
> > > -- 
> > > 2.34.1
> > > 
> 
>
Suzuki K Poulose April 26, 2024, 11:15 a.m. UTC | #9
On 22/04/2024 17:15, Alexandru Elisei wrote:
> Hi,
> 
> On Mon, Apr 22, 2024 at 05:05:54PM +0100, Suzuki K Poulose wrote:
>> On 22/04/2024 16:38, Alexandru Elisei wrote:
>>> Hi,
>>>
>>> On Fri, Apr 12, 2024 at 11:33:43AM +0100, Suzuki K Poulose wrote:
>>>> From: Joey Gouly <joey.gouly@arm.com>
>>>>
>>>> A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.
>>>
>>> What entity requires that a realm must access I/O mappings with the
>>> PTE_NS_SHARED bit set? Is that an architectural requirement? Or is it an
>>> implementation choice made by the VMM and/or KVM?
>>
>> RMM spec. An MMIO access in the Protected IPA must be emulated by Realm
>> world. If an MMIO access must be emulated by NS Host, it must be in the
>> Unprotected IPA.
> 
> That's not exactly what I was asking. I was curious to know how a realm
> knows if a MMIO access is emulated by the Realm, and thus it must use a
> protected address, or it's emulated by the non-secure host, and it must use
> an unprotected address.

With Arm CCA-1.0, there cannot be an emulation in the Realm world. 
Everything is by the NS Host. It may change in the future, but the
RSI version would make the Realm aware of the difference.


Suzuki



> 
> Thanks,
> Alex
> 
>>
>> Technically, a VMM could create a memory map, where the NS emulated I/O
>> are kept in the unprotected (upper) half.
>>
>> Or the VMM retains the current model and expects the Realm to use the
>> "Unprotected" alias.
>>
>>
>> Either way, applying the PTE_NS_SHARED attribute doesn't change anything and
>> works for both the models, as far as the Realm is concerned.
>>
>>
>> Suzuki
>>
>>
>>
>>
>>>
>>> Thanks,
>>> Alex
>>>
>>>> This is modelled as a PTE attribute, but is actually part of the address.
>>>>
>>>> So, when MMU is disabled, the "physical address" must reflect this bit set. We
>>>> access the UART early before the MMU is enabled. So, make sure the UART is
>>>> accessed always with the bit set.
>>>>
>>>> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> ---
>>>>    lib/arm/asm/pgtable.h   |  5 +++++
>>>>    lib/arm/io.c            | 24 +++++++++++++++++++++++-
>>>>    lib/arm64/asm/pgtable.h |  5 +++++
>>>>    3 files changed, 33 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
>>>> index 350039ff..7e85e7c6 100644
>>>> --- a/lib/arm/asm/pgtable.h
>>>> +++ b/lib/arm/asm/pgtable.h
>>>> @@ -112,4 +112,9 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
>>>>    	return pte_offset(pmd, addr);
>>>>    }
>>>> +static inline unsigned long arm_shared_phys_alias(void *x)
>>>> +{
>>>> +	return ((unsigned long)(x) | PTE_NS_SHARED);
>>>> +}
>>>> +
>>>>    #endif /* _ASMARM_PGTABLE_H_ */
>>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>>>> index 836fa854..127727e4 100644
>>>> --- a/lib/arm/io.c
>>>> +++ b/lib/arm/io.c
>>>> @@ -15,6 +15,8 @@
>>>>    #include <asm/psci.h>
>>>>    #include <asm/spinlock.h>
>>>>    #include <asm/io.h>
>>>> +#include <asm/mmu-api.h>
>>>> +#include <asm/pgtable.h>
>>>>    #include "io.h"
>>>> @@ -30,6 +32,24 @@ static struct spinlock uart_lock;
>>>>    static volatile u8 *uart0_base = UART_EARLY_BASE;
>>>>    bool is_pl011_uart;
>>>> +static inline volatile u8 *get_uart_base(void)
>>>> +{
>>>> +	/*
>>>> +	 * The address of the UART base may be different
>>>> +	 * based on whether we are running with/without
>>>> +	 * MMU enabled.
>>>> +	 *
>>>> +	 * For realms, we must force to use the shared physical
>>>> +	 * alias with MMU disabled, to make sure the I/O can
>>>> +	 * be emulated.
>>>> +	 * When the MMU is turned ON, the mappings are created
>>>> +	 * appropriately.
>>>> +	 */
>>>> +	if (mmu_enabled())
>>>> +		return uart0_base;
>>>> +	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
>>>> +}
>>>> +
>>>>    static void uart0_init_fdt(void)
>>>>    {
>>>>    	/*
>>>> @@ -109,9 +129,11 @@ void io_init(void)
>>>>    void puts(const char *s)
>>>>    {
>>>> +	volatile u8 *uart_base = get_uart_base();
>>>> +
>>>>    	spin_lock(&uart_lock);
>>>>    	while (*s)
>>>> -		writeb(*s++, uart0_base);
>>>> +		writeb(*s++, uart_base);
>>>>    	spin_unlock(&uart_lock);
>>>>    }
>>>> diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
>>>> index 5b9f40b0..871c03e9 100644
>>>> --- a/lib/arm64/asm/pgtable.h
>>>> +++ b/lib/arm64/asm/pgtable.h
>>>> @@ -28,6 +28,11 @@ extern unsigned long prot_ns_shared;
>>>>    */
>>>>    #define PTE_NS_SHARED		(prot_ns_shared)
>>>> +static inline unsigned long arm_shared_phys_alias(void *addr)
>>>> +{
>>>> +	return ((unsigned long)addr | PTE_NS_SHARED);
>>>> +}
>>>> +
>>>>    /*
>>>>     * Highest possible physical address supported.
>>>>     */
>>>> -- 
>>>> 2.34.1
>>>>
>>
>>
Alexandru Elisei April 26, 2024, 1:51 p.m. UTC | #10
Hi,

On Fri, Apr 26, 2024 at 12:15:53PM +0100, Suzuki K Poulose wrote:
> On 22/04/2024 17:15, Alexandru Elisei wrote:
> > Hi,
> > 
> > On Mon, Apr 22, 2024 at 05:05:54PM +0100, Suzuki K Poulose wrote:
> > > On 22/04/2024 16:38, Alexandru Elisei wrote:
> > > > Hi,
> > > > 
> > > > On Fri, Apr 12, 2024 at 11:33:43AM +0100, Suzuki K Poulose wrote:
> > > > > From: Joey Gouly <joey.gouly@arm.com>
> > > > > 
> > > > > A Realm must access any emulated I/O mappings with the PTE_NS_SHARED bit set.
> > > > 
> > > > What entity requires that a realm must access I/O mappings with the
> > > > PTE_NS_SHARED bit set? Is that an architectural requirement? Or is it an
> > > > implementation choice made by the VMM and/or KVM?
> > > 
> > > RMM spec. An MMIO access in the Protected IPA must be emulated by Realm
> > > world. If an MMIO access must be emulated by NS Host, it must be in the
> > > Unprotected IPA.
> > 
> > That's not exactly what I was asking. I was curious to know how a realm
> > knows if a MMIO access is emulated by the Realm, and thus it must use a
> > protected address, or it's emulated by the non-secure host, and it must use
> > an unprotected address.
> 
> With Arm CCA-1.0, there cannot be an emulation in the Realm world.
> Everything is by the NS Host. It may change in the future, but the
> RSI version would make the Realm aware of the difference.

Wouldn't that be a breaking change for existing software, though? For
example, existing software running in a realm assumes that all devices use
unprotected addresses, and when the VMM/KVM/realm manager gets updated to
implement the newer spec, that ceases to be the case.

Also, would you mind pointing me to the section in the existing spec where
that is specified? I'm curious about the exact wording.

Thanks,
Alex

> 
> 
> Suzuki
> 
> 
> 
> > 
> > Thanks,
> > Alex
> > 
> > > 
> > > Technically, a VMM could create a memory map, where the NS emulated I/O
> > > are kept in the unprotected (upper) half.
> > > 
> > > Or the VMM retains the current model and expects the Realm to use the
> > > "Unprotected" alias.
> > > 
> > > 
> > > Either way, applying the PTE_NS_SHARED attribute doesn't change anything and
> > > works for both the models, as far as the Realm is concerned.
> > > 
> > > 
> > > Suzuki
> > > 
> > > 
> > > 
> > > 
> > > > 
> > > > Thanks,
> > > > Alex
> > > > 
> > > > > This is modelled as a PTE attribute, but is actually part of the address.
> > > > > 
> > > > > So, when MMU is disabled, the "physical address" must reflect this bit set. We
> > > > > access the UART early before the MMU is enabled. So, make sure the UART is
> > > > > accessed always with the bit set.
> > > > > 
> > > > > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > > > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > > ---
> > > > >    lib/arm/asm/pgtable.h   |  5 +++++
> > > > >    lib/arm/io.c            | 24 +++++++++++++++++++++++-
> > > > >    lib/arm64/asm/pgtable.h |  5 +++++
> > > > >    3 files changed, 33 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
> > > > > index 350039ff..7e85e7c6 100644
> > > > > --- a/lib/arm/asm/pgtable.h
> > > > > +++ b/lib/arm/asm/pgtable.h
> > > > > @@ -112,4 +112,9 @@ static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
> > > > >    	return pte_offset(pmd, addr);
> > > > >    }
> > > > > +static inline unsigned long arm_shared_phys_alias(void *x)
> > > > > +{
> > > > > +	return ((unsigned long)(x) | PTE_NS_SHARED);
> > > > > +}
> > > > > +
> > > > >    #endif /* _ASMARM_PGTABLE_H_ */
> > > > > diff --git a/lib/arm/io.c b/lib/arm/io.c
> > > > > index 836fa854..127727e4 100644
> > > > > --- a/lib/arm/io.c
> > > > > +++ b/lib/arm/io.c
> > > > > @@ -15,6 +15,8 @@
> > > > >    #include <asm/psci.h>
> > > > >    #include <asm/spinlock.h>
> > > > >    #include <asm/io.h>
> > > > > +#include <asm/mmu-api.h>
> > > > > +#include <asm/pgtable.h>
> > > > >    #include "io.h"
> > > > > @@ -30,6 +32,24 @@ static struct spinlock uart_lock;
> > > > >    static volatile u8 *uart0_base = UART_EARLY_BASE;
> > > > >    bool is_pl011_uart;
> > > > > +static inline volatile u8 *get_uart_base(void)
> > > > > +{
> > > > > +	/*
> > > > > +	 * The address of the UART base may be different
> > > > > +	 * based on whether we are running with/without
> > > > > +	 * MMU enabled.
> > > > > +	 *
> > > > > +	 * For realms, we must force to use the shared physical
> > > > > +	 * alias with MMU disabled, to make sure the I/O can
> > > > > +	 * be emulated.
> > > > > +	 * When the MMU is turned ON, the mappings are created
> > > > > +	 * appropriately.
> > > > > +	 */
> > > > > +	if (mmu_enabled())
> > > > > +		return uart0_base;
> > > > > +	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
> > > > > +}
> > > > > +
> > > > >    static void uart0_init_fdt(void)
> > > > >    {
> > > > >    	/*
> > > > > @@ -109,9 +129,11 @@ void io_init(void)
> > > > >    void puts(const char *s)
> > > > >    {
> > > > > +	volatile u8 *uart_base = get_uart_base();
> > > > > +
> > > > >    	spin_lock(&uart_lock);
> > > > >    	while (*s)
> > > > > -		writeb(*s++, uart0_base);
> > > > > +		writeb(*s++, uart_base);
> > > > >    	spin_unlock(&uart_lock);
> > > > >    }
> > > > > diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
> > > > > index 5b9f40b0..871c03e9 100644
> > > > > --- a/lib/arm64/asm/pgtable.h
> > > > > +++ b/lib/arm64/asm/pgtable.h
> > > > > @@ -28,6 +28,11 @@ extern unsigned long prot_ns_shared;
> > > > >    */
> > > > >    #define PTE_NS_SHARED		(prot_ns_shared)
> > > > > +static inline unsigned long arm_shared_phys_alias(void *addr)
> > > > > +{
> > > > > +	return ((unsigned long)addr | PTE_NS_SHARED);
> > > > > +}
> > > > > +
> > > > >    /*
> > > > >     * Highest possible physical address supported.
> > > > >     */
> > > > > -- 
> > > > > 2.34.1
> > > > > 
> > > 
> > > 
>
diff mbox series

Patch

diff --git a/lib/arm/asm/pgtable.h b/lib/arm/asm/pgtable.h
index 350039ff..7e85e7c6 100644
--- a/lib/arm/asm/pgtable.h
+++ b/lib/arm/asm/pgtable.h
@@ -112,4 +112,9 @@  static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
 	return pte_offset(pmd, addr);
 }
 
+static inline unsigned long arm_shared_phys_alias(void *x)
+{
+	return ((unsigned long)(x) | PTE_NS_SHARED);
+}
+
 #endif /* _ASMARM_PGTABLE_H_ */
diff --git a/lib/arm/io.c b/lib/arm/io.c
index 836fa854..127727e4 100644
--- a/lib/arm/io.c
+++ b/lib/arm/io.c
@@ -15,6 +15,8 @@ 
 #include <asm/psci.h>
 #include <asm/spinlock.h>
 #include <asm/io.h>
+#include <asm/mmu-api.h>
+#include <asm/pgtable.h>
 
 #include "io.h"
 
@@ -30,6 +32,24 @@  static struct spinlock uart_lock;
 static volatile u8 *uart0_base = UART_EARLY_BASE;
 bool is_pl011_uart;
 
+static inline volatile u8 *get_uart_base(void)
+{
+	/*
+	 * The address of the UART base may be different
+	 * based on whether we are running with/without
+	 * MMU enabled.
+	 *
+	 * For realms, we must force to use the shared physical
+	 * alias with MMU disabled, to make sure the I/O can
+	 * be emulated.
+	 * When the MMU is turned ON, the mappings are created
+	 * appropriately.
+	 */
+	if (mmu_enabled())
+		return uart0_base;
+	return (u8 *)arm_shared_phys_alias((void *)uart0_base);
+}
+
 static void uart0_init_fdt(void)
 {
 	/*
@@ -109,9 +129,11 @@  void io_init(void)
 
 void puts(const char *s)
 {
+	volatile u8 *uart_base = get_uart_base();
+
 	spin_lock(&uart_lock);
 	while (*s)
-		writeb(*s++, uart0_base);
+		writeb(*s++, uart_base);
 	spin_unlock(&uart_lock);
 }
 
diff --git a/lib/arm64/asm/pgtable.h b/lib/arm64/asm/pgtable.h
index 5b9f40b0..871c03e9 100644
--- a/lib/arm64/asm/pgtable.h
+++ b/lib/arm64/asm/pgtable.h
@@ -28,6 +28,11 @@  extern unsigned long prot_ns_shared;
 */
 #define PTE_NS_SHARED		(prot_ns_shared)
 
+static inline unsigned long arm_shared_phys_alias(void *addr)
+{
+	return ((unsigned long)addr | PTE_NS_SHARED);
+}
+
 /*
  * Highest possible physical address supported.
  */