diff mbox series

[1/6] firmware/smccc: Call arch-specific hook on discovering KVM services

Message ID 20240730151113.1497-2-will@kernel.org (mailing list archive)
State New, archived
Headers show
Series Support for running as a pKVM protected guest | expand

Commit Message

Will Deacon July 30, 2024, 3:11 p.m. UTC
From: Marc Zyngier <maz@kernel.org>

arm64 will soon require its own callback to initialise services
that are only available on this architecture. Introduce a hook
that can be overloaded by the architecture.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm/include/asm/hypervisor.h   | 2 ++
 arch/arm64/include/asm/hypervisor.h | 4 ++++
 drivers/firmware/smccc/kvm_guest.c  | 2 ++
 3 files changed, 8 insertions(+)

Comments

Aneesh Kumar K.V July 31, 2024, 2:41 p.m. UTC | #1
Will Deacon <will@kernel.org> writes:

> From: Marc Zyngier <maz@kernel.org>
>
> arm64 will soon require its own callback to initialise services
> that are only available on this architecture. Introduce a hook
> that can be overloaded by the architecture.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm/include/asm/hypervisor.h   | 2 ++
>  arch/arm64/include/asm/hypervisor.h | 4 ++++
>  drivers/firmware/smccc/kvm_guest.c  | 2 ++
>  3 files changed, 8 insertions(+)
>
> diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h
> index bd61502b9715..8a648e506540 100644
> --- a/arch/arm/include/asm/hypervisor.h
> +++ b/arch/arm/include/asm/hypervisor.h
> @@ -7,4 +7,6 @@
>  void kvm_init_hyp_services(void);
>  bool kvm_arm_hyp_service_available(u32 func_id);
>  
> +static inline void kvm_arch_init_hyp_services(void) { };
> +
>  #endif
> diff --git a/arch/arm64/include/asm/hypervisor.h b/arch/arm64/include/asm/hypervisor.h
> index 0ae427f352c8..8cab2ab535b7 100644
> --- a/arch/arm64/include/asm/hypervisor.h
> +++ b/arch/arm64/include/asm/hypervisor.h
> @@ -7,4 +7,8 @@
>  void kvm_init_hyp_services(void);
>  bool kvm_arm_hyp_service_available(u32 func_id);
>  
> +static inline void kvm_arch_init_hyp_services(void)
> +{
> +};
> +
>  #endif
> diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
> index 89a68e7eeaa6..f3319be20b36 100644
> --- a/drivers/firmware/smccc/kvm_guest.c
> +++ b/drivers/firmware/smccc/kvm_guest.c
> @@ -39,6 +39,8 @@ void __init kvm_init_hyp_services(void)
>  
>  	pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
>  		 res.a3, res.a2, res.a1, res.a0);
> +
> +	kvm_arch_init_hyp_services();
>  }
>

That is a bit late to detect RMM? One of the requirements is to
figure out the pgprot_t flags for early_ioremap so that "earlycon" will
work (by mapping the address as shared alias). To do that we need to
make an RSI call to detect PROT_NS_SHARED mask as below. 

	if (rsi_get_realm_config(&config))
		return;
	prot_ns_shared = BIT(config.ipa_bits - 1);

-aneesh
Will Deacon July 31, 2024, 3:50 p.m. UTC | #2
On Wed, Jul 31, 2024 at 08:11:16PM +0530, Aneesh Kumar K.V wrote:
> Will Deacon <will@kernel.org> writes:
> 
> > From: Marc Zyngier <maz@kernel.org>
> >
> > arm64 will soon require its own callback to initialise services
> > that are only available on this architecture. Introduce a hook
> > that can be overloaded by the architecture.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm/include/asm/hypervisor.h   | 2 ++
> >  arch/arm64/include/asm/hypervisor.h | 4 ++++
> >  drivers/firmware/smccc/kvm_guest.c  | 2 ++
> >  3 files changed, 8 insertions(+)
> >
> > diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h
> > index bd61502b9715..8a648e506540 100644
> > --- a/arch/arm/include/asm/hypervisor.h
> > +++ b/arch/arm/include/asm/hypervisor.h
> > @@ -7,4 +7,6 @@
> >  void kvm_init_hyp_services(void);
> >  bool kvm_arm_hyp_service_available(u32 func_id);
> >  
> > +static inline void kvm_arch_init_hyp_services(void) { };
> > +
> >  #endif
> > diff --git a/arch/arm64/include/asm/hypervisor.h b/arch/arm64/include/asm/hypervisor.h
> > index 0ae427f352c8..8cab2ab535b7 100644
> > --- a/arch/arm64/include/asm/hypervisor.h
> > +++ b/arch/arm64/include/asm/hypervisor.h
> > @@ -7,4 +7,8 @@
> >  void kvm_init_hyp_services(void);
> >  bool kvm_arm_hyp_service_available(u32 func_id);
> >  
> > +static inline void kvm_arch_init_hyp_services(void)
> > +{
> > +};
> > +
> >  #endif
> > diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
> > index 89a68e7eeaa6..f3319be20b36 100644
> > --- a/drivers/firmware/smccc/kvm_guest.c
> > +++ b/drivers/firmware/smccc/kvm_guest.c
> > @@ -39,6 +39,8 @@ void __init kvm_init_hyp_services(void)
> >  
> >  	pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
> >  		 res.a3, res.a2, res.a1, res.a0);
> > +
> > +	kvm_arch_init_hyp_services();
> >  }
> >
> 
> That is a bit late to detect RMM? One of the requirements is to
> figure out the pgprot_t flags for early_ioremap so that "earlycon" will
> work (by mapping the address as shared alias). To do that we need to
> make an RSI call to detect PROT_NS_SHARED mask as below. 
> 
> 	if (rsi_get_realm_config(&config))
> 		return;
> 	prot_ns_shared = BIT(config.ipa_bits - 1);

Why can't the earlycon MMIO address just have that high bit set?

I think it's horribly fragile to try detecting all of this stuff before
we're allowed to touch the console. We don't even bother with pKVM --
it's the guest firmware's responsibility to MMIO_GUARD the UART if it
detects a debuggable payload.

Will
Aneesh Kumar K.V July 31, 2024, 3:53 p.m. UTC | #3
Will Deacon <will@kernel.org> writes:

> On Wed, Jul 31, 2024 at 08:11:16PM +0530, Aneesh Kumar K.V wrote:
>> Will Deacon <will@kernel.org> writes:
>> 
>> > From: Marc Zyngier <maz@kernel.org>
>> >
>> > arm64 will soon require its own callback to initialise services
>> > that are only available on this architecture. Introduce a hook
>> > that can be overloaded by the architecture.
>> >
>> > Signed-off-by: Marc Zyngier <maz@kernel.org>
>> > Signed-off-by: Will Deacon <will@kernel.org>
>> > ---
>> >  arch/arm/include/asm/hypervisor.h   | 2 ++
>> >  arch/arm64/include/asm/hypervisor.h | 4 ++++
>> >  drivers/firmware/smccc/kvm_guest.c  | 2 ++
>> >  3 files changed, 8 insertions(+)
>> >
>> > diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h
>> > index bd61502b9715..8a648e506540 100644
>> > --- a/arch/arm/include/asm/hypervisor.h
>> > +++ b/arch/arm/include/asm/hypervisor.h
>> > @@ -7,4 +7,6 @@
>> >  void kvm_init_hyp_services(void);
>> >  bool kvm_arm_hyp_service_available(u32 func_id);
>> >  
>> > +static inline void kvm_arch_init_hyp_services(void) { };
>> > +
>> >  #endif
>> > diff --git a/arch/arm64/include/asm/hypervisor.h b/arch/arm64/include/asm/hypervisor.h
>> > index 0ae427f352c8..8cab2ab535b7 100644
>> > --- a/arch/arm64/include/asm/hypervisor.h
>> > +++ b/arch/arm64/include/asm/hypervisor.h
>> > @@ -7,4 +7,8 @@
>> >  void kvm_init_hyp_services(void);
>> >  bool kvm_arm_hyp_service_available(u32 func_id);
>> >  
>> > +static inline void kvm_arch_init_hyp_services(void)
>> > +{
>> > +};
>> > +
>> >  #endif
>> > diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
>> > index 89a68e7eeaa6..f3319be20b36 100644
>> > --- a/drivers/firmware/smccc/kvm_guest.c
>> > +++ b/drivers/firmware/smccc/kvm_guest.c
>> > @@ -39,6 +39,8 @@ void __init kvm_init_hyp_services(void)
>> >  
>> >  	pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
>> >  		 res.a3, res.a2, res.a1, res.a0);
>> > +
>> > +	kvm_arch_init_hyp_services();
>> >  }
>> >
>> 
>> That is a bit late to detect RMM? One of the requirements is to
>> figure out the pgprot_t flags for early_ioremap so that "earlycon" will
>> work (by mapping the address as shared alias). To do that we need to
>> make an RSI call to detect PROT_NS_SHARED mask as below. 
>> 
>> 	if (rsi_get_realm_config(&config))
>> 		return;
>> 	prot_ns_shared = BIT(config.ipa_bits - 1);
>
> Why can't the earlycon MMIO address just have that high bit set?
>
> I think it's horribly fragile to try detecting all of this stuff before
> we're allowed to touch the console. We don't even bother with pKVM --
> it's the guest firmware's responsibility to MMIO_GUARD the UART if it
> detects a debuggable payload.
>

To mark something shared, we need to know the mask value which is
returned via rsi_get_realm_config() call.

-aneesh
Aneesh Kumar K.V July 31, 2024, 3:56 p.m. UTC | #4
Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:

> Will Deacon <will@kernel.org> writes:
>
>> On Wed, Jul 31, 2024 at 08:11:16PM +0530, Aneesh Kumar K.V wrote:
>>> Will Deacon <will@kernel.org> writes:
>>> 
>>> > From: Marc Zyngier <maz@kernel.org>
>>> >
>>> > arm64 will soon require its own callback to initialise services
>>> > that are only available on this architecture. Introduce a hook
>>> > that can be overloaded by the architecture.
>>> >
>>> > Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> > Signed-off-by: Will Deacon <will@kernel.org>
>>> > ---
>>> >  arch/arm/include/asm/hypervisor.h   | 2 ++
>>> >  arch/arm64/include/asm/hypervisor.h | 4 ++++
>>> >  drivers/firmware/smccc/kvm_guest.c  | 2 ++
>>> >  3 files changed, 8 insertions(+)
>>> >
>>> > diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h
>>> > index bd61502b9715..8a648e506540 100644
>>> > --- a/arch/arm/include/asm/hypervisor.h
>>> > +++ b/arch/arm/include/asm/hypervisor.h
>>> > @@ -7,4 +7,6 @@
>>> >  void kvm_init_hyp_services(void);
>>> >  bool kvm_arm_hyp_service_available(u32 func_id);
>>> >  
>>> > +static inline void kvm_arch_init_hyp_services(void) { };
>>> > +
>>> >  #endif
>>> > diff --git a/arch/arm64/include/asm/hypervisor.h b/arch/arm64/include/asm/hypervisor.h
>>> > index 0ae427f352c8..8cab2ab535b7 100644
>>> > --- a/arch/arm64/include/asm/hypervisor.h
>>> > +++ b/arch/arm64/include/asm/hypervisor.h
>>> > @@ -7,4 +7,8 @@
>>> >  void kvm_init_hyp_services(void);
>>> >  bool kvm_arm_hyp_service_available(u32 func_id);
>>> >  
>>> > +static inline void kvm_arch_init_hyp_services(void)
>>> > +{
>>> > +};
>>> > +
>>> >  #endif
>>> > diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
>>> > index 89a68e7eeaa6..f3319be20b36 100644
>>> > --- a/drivers/firmware/smccc/kvm_guest.c
>>> > +++ b/drivers/firmware/smccc/kvm_guest.c
>>> > @@ -39,6 +39,8 @@ void __init kvm_init_hyp_services(void)
>>> >  
>>> >  	pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
>>> >  		 res.a3, res.a2, res.a1, res.a0);
>>> > +
>>> > +	kvm_arch_init_hyp_services();
>>> >  }
>>> >
>>> 
>>> That is a bit late to detect RMM? One of the requirements is to
>>> figure out the pgprot_t flags for early_ioremap so that "earlycon" will
>>> work (by mapping the address as shared alias). To do that we need to
>>> make an RSI call to detect PROT_NS_SHARED mask as below. 
>>> 
>>> 	if (rsi_get_realm_config(&config))
>>> 		return;
>>> 	prot_ns_shared = BIT(config.ipa_bits - 1);
>>
>> Why can't the earlycon MMIO address just have that high bit set?
>>
>> I think it's horribly fragile to try detecting all of this stuff before
>> we're allowed to touch the console. We don't even bother with pKVM --
>> it's the guest firmware's responsibility to MMIO_GUARD the UART if it
>> detects a debuggable payload.
>>
>
> To mark something shared, we need to know the mask value which is
> returned via rsi_get_realm_config() call.
>

I guess you are suggesting to leave it to firmware to set up the device
tree "reg-offset" with shared bit set?

-aneesh
Catalin Marinas Aug. 2, 2024, 3:13 p.m. UTC | #5
On Wed, Jul 31, 2024 at 08:11:16PM +0530, Aneesh Kumar K.V wrote:
> Will Deacon <will@kernel.org> writes:
> > diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
> > index 89a68e7eeaa6..f3319be20b36 100644
> > --- a/drivers/firmware/smccc/kvm_guest.c
> > +++ b/drivers/firmware/smccc/kvm_guest.c
> > @@ -39,6 +39,8 @@ void __init kvm_init_hyp_services(void)
> >  
> >  	pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
> >  		 res.a3, res.a2, res.a1, res.a0);
> > +
> > +	kvm_arch_init_hyp_services();
> >  }
> 
> That is a bit late to detect RMM?

It may be late for RMM but I'm not sure that's relevant. This function
is about KVM services provided to a guest. The RMM is meant (in theory
at least) to be hypervisor-agnostic. We shouldn't place any realm guest
initialisation in the kvm_guest.c file.
Suzuki K Poulose Aug. 2, 2024, 3:30 p.m. UTC | #6
On 31/07/2024 16:50, Will Deacon wrote:
> On Wed, Jul 31, 2024 at 08:11:16PM +0530, Aneesh Kumar K.V wrote:
>> Will Deacon <will@kernel.org> writes:
>>
>>> From: Marc Zyngier <maz@kernel.org>
>>>
>>> arm64 will soon require its own callback to initialise services
>>> that are only available on this architecture. Introduce a hook
>>> that can be overloaded by the architecture.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> Signed-off-by: Will Deacon <will@kernel.org>
>>> ---
>>>   arch/arm/include/asm/hypervisor.h   | 2 ++
>>>   arch/arm64/include/asm/hypervisor.h | 4 ++++
>>>   drivers/firmware/smccc/kvm_guest.c  | 2 ++
>>>   3 files changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h
>>> index bd61502b9715..8a648e506540 100644
>>> --- a/arch/arm/include/asm/hypervisor.h
>>> +++ b/arch/arm/include/asm/hypervisor.h
>>> @@ -7,4 +7,6 @@
>>>   void kvm_init_hyp_services(void);
>>>   bool kvm_arm_hyp_service_available(u32 func_id);
>>>   
>>> +static inline void kvm_arch_init_hyp_services(void) { };
>>> +
>>>   #endif
>>> diff --git a/arch/arm64/include/asm/hypervisor.h b/arch/arm64/include/asm/hypervisor.h
>>> index 0ae427f352c8..8cab2ab535b7 100644
>>> --- a/arch/arm64/include/asm/hypervisor.h
>>> +++ b/arch/arm64/include/asm/hypervisor.h
>>> @@ -7,4 +7,8 @@
>>>   void kvm_init_hyp_services(void);
>>>   bool kvm_arm_hyp_service_available(u32 func_id);
>>>   
>>> +static inline void kvm_arch_init_hyp_services(void)
>>> +{
>>> +};
>>> +
>>>   #endif
>>> diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
>>> index 89a68e7eeaa6..f3319be20b36 100644
>>> --- a/drivers/firmware/smccc/kvm_guest.c
>>> +++ b/drivers/firmware/smccc/kvm_guest.c
>>> @@ -39,6 +39,8 @@ void __init kvm_init_hyp_services(void)
>>>   
>>>   	pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
>>>   		 res.a3, res.a2, res.a1, res.a0);
>>> +
>>> +	kvm_arch_init_hyp_services();
>>>   }
>>>
>>
>> That is a bit late to detect RMM? One of the requirements is to
>> figure out the pgprot_t flags for early_ioremap so that "earlycon" will
>> work (by mapping the address as shared alias). To do that we need to
>> make an RSI call to detect PROT_NS_SHARED mask as below.
>>
>> 	if (rsi_get_realm_config(&config))
>> 		return;
>> 	prot_ns_shared = BIT(config.ipa_bits - 1);
> 
> Why can't the earlycon MMIO address just have that high bit set?

Do you mean the "earlycon=<MMIO-address-to-use>" ? That could work,
except that :
1. It breaks the Realm's view of the {I}"PA" space
2. If the address is missed, it is fatal for the Realm.
3. All higher level tools that specify the command line parameter now
    need to fixup the "MMIO" address, based on the "ipa_bits" chosen by
    the VMM (which could vary with the VMMs and Hyp/System)

Also, we are making some changes to the guest support to make it future
proof for running the same guest in a less privileged context within
R_EL1 (read unprivileged plane with RMM v1.1), where the console
could be "private". And we are modifying the code to "apply" the prot_ns
dynamically (instead of hard coding it for all I/O).

Suzuki

> 
> I think it's horribly fragile to try detecting all of this stuff before
> we're allowed to touch the console. We don't even bother with pKVM --
> it's the guest firmware's responsibility to MMIO_GUARD the UART if it
> detects a debuggable payload.
> 
> Will
>
Catalin Marinas Aug. 2, 2024, 3:44 p.m. UTC | #7
On Wed, Jul 31, 2024 at 09:26:31PM +0530, Aneesh Kumar K.V wrote:
> Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:
> > Will Deacon <will@kernel.org> writes:
> >> On Wed, Jul 31, 2024 at 08:11:16PM +0530, Aneesh Kumar K.V wrote:
> >>> Will Deacon <will@kernel.org> writes:
> >>> > diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
> >>> > index 89a68e7eeaa6..f3319be20b36 100644
> >>> > --- a/drivers/firmware/smccc/kvm_guest.c
> >>> > +++ b/drivers/firmware/smccc/kvm_guest.c
> >>> > @@ -39,6 +39,8 @@ void __init kvm_init_hyp_services(void)
> >>> >  
> >>> >  	pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
> >>> >  		 res.a3, res.a2, res.a1, res.a0);
> >>> > +
> >>> > +	kvm_arch_init_hyp_services();
> >>> >  }
> >>> >
> >>> 
> >>> That is a bit late to detect RMM? One of the requirements is to
> >>> figure out the pgprot_t flags for early_ioremap so that "earlycon" will
> >>> work (by mapping the address as shared alias). To do that we need to
> >>> make an RSI call to detect PROT_NS_SHARED mask as below. 
> >>> 
> >>> 	if (rsi_get_realm_config(&config))
> >>> 		return;
> >>> 	prot_ns_shared = BIT(config.ipa_bits - 1);
> >>
> >> Why can't the earlycon MMIO address just have that high bit set?
> >>
> >> I think it's horribly fragile to try detecting all of this stuff before
> >> we're allowed to touch the console. We don't even bother with pKVM --
> >> it's the guest firmware's responsibility to MMIO_GUARD the UART if it
> >> detects a debuggable payload.
> >
> > To mark something shared, we need to know the mask value which is
> > returned via rsi_get_realm_config() call.
> 
> I guess you are suggesting to leave it to firmware to set up the device
> tree "reg-offset" with shared bit set?

As you know, we've been through these options internally and we
concluded not to encode this information in the DT for various reasons.

Personally I don't like this IPA split but that's too late to change it
now in the RMM spec.
Aneesh Kumar K.V Aug. 2, 2024, 4:16 p.m. UTC | #8
Catalin Marinas <catalin.marinas@arm.com> writes:

> On Wed, Jul 31, 2024 at 09:26:31PM +0530, Aneesh Kumar K.V wrote:
>> Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:
>> > Will Deacon <will@kernel.org> writes:
>> >> On Wed, Jul 31, 2024 at 08:11:16PM +0530, Aneesh Kumar K.V wrote:
>> >>> Will Deacon <will@kernel.org> writes:
>> >>> > diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
>> >>> > index 89a68e7eeaa6..f3319be20b36 100644
>> >>> > --- a/drivers/firmware/smccc/kvm_guest.c
>> >>> > +++ b/drivers/firmware/smccc/kvm_guest.c
>> >>> > @@ -39,6 +39,8 @@ void __init kvm_init_hyp_services(void)
>> >>> >
>> >>> >  	pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
>> >>> >  		 res.a3, res.a2, res.a1, res.a0);
>> >>> > +
>> >>> > +	kvm_arch_init_hyp_services();
>> >>> >  }
>> >>> >
>> >>>
>> >>> That is a bit late to detect RMM? One of the requirements is to
>> >>> figure out the pgprot_t flags for early_ioremap so that "earlycon" will
>> >>> work (by mapping the address as shared alias). To do that we need to
>> >>> make an RSI call to detect PROT_NS_SHARED mask as below.
>> >>>
>> >>> 	if (rsi_get_realm_config(&config))
>> >>> 		return;
>> >>> 	prot_ns_shared = BIT(config.ipa_bits - 1);
>> >>
>> >> Why can't the earlycon MMIO address just have that high bit set?
>> >>
>> >> I think it's horribly fragile to try detecting all of this stuff before
>> >> we're allowed to touch the console. We don't even bother with pKVM --
>> >> it's the guest firmware's responsibility to MMIO_GUARD the UART if it
>> >> detects a debuggable payload.
>> >
>> > To mark something shared, we need to know the mask value which is
>> > returned via rsi_get_realm_config() call.
>>
>> I guess you are suggesting to leave it to firmware to set up the device
>> tree "reg-offset" with shared bit set?
>
> As you know, we've been through these options internally and we
> concluded not to encode this information in the DT for various reasons.
>

I was trying to find out what a guest firmware-based control means.

>
> Personally I don't like this IPA split but that's too late to change it
> now in the RMM spec.
>

Agreed. This also makes supporting both secure and non secure devices
difficult.

-aneesh
Suzuki K Poulose Aug. 7, 2024, 12:43 p.m. UTC | #9
On 02/08/2024 16:30, Suzuki K Poulose wrote:
> On 31/07/2024 16:50, Will Deacon wrote:
>> On Wed, Jul 31, 2024 at 08:11:16PM +0530, Aneesh Kumar K.V wrote:
>>> Will Deacon <will@kernel.org> writes:
>>>
>>>> From: Marc Zyngier <maz@kernel.org>
>>>>
>>>> arm64 will soon require its own callback to initialise services
>>>> that are only available on this architecture. Introduce a hook
>>>> that can be overloaded by the architecture.
>>>>
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> Signed-off-by: Will Deacon <will@kernel.org>
>>>> ---
>>>>   arch/arm/include/asm/hypervisor.h   | 2 ++
>>>>   arch/arm64/include/asm/hypervisor.h | 4 ++++
>>>>   drivers/firmware/smccc/kvm_guest.c  | 2 ++
>>>>   3 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/arch/arm/include/asm/hypervisor.h 
>>>> b/arch/arm/include/asm/hypervisor.h
>>>> index bd61502b9715..8a648e506540 100644
>>>> --- a/arch/arm/include/asm/hypervisor.h
>>>> +++ b/arch/arm/include/asm/hypervisor.h
>>>> @@ -7,4 +7,6 @@
>>>>   void kvm_init_hyp_services(void);
>>>>   bool kvm_arm_hyp_service_available(u32 func_id);
>>>> +static inline void kvm_arch_init_hyp_services(void) { };
>>>> +
>>>>   #endif
>>>> diff --git a/arch/arm64/include/asm/hypervisor.h 
>>>> b/arch/arm64/include/asm/hypervisor.h
>>>> index 0ae427f352c8..8cab2ab535b7 100644
>>>> --- a/arch/arm64/include/asm/hypervisor.h
>>>> +++ b/arch/arm64/include/asm/hypervisor.h
>>>> @@ -7,4 +7,8 @@
>>>>   void kvm_init_hyp_services(void);
>>>>   bool kvm_arm_hyp_service_available(u32 func_id);
>>>> +static inline void kvm_arch_init_hyp_services(void)
>>>> +{
>>>> +};
>>>> +
>>>>   #endif
>>>> diff --git a/drivers/firmware/smccc/kvm_guest.c 
>>>> b/drivers/firmware/smccc/kvm_guest.c
>>>> index 89a68e7eeaa6..f3319be20b36 100644
>>>> --- a/drivers/firmware/smccc/kvm_guest.c
>>>> +++ b/drivers/firmware/smccc/kvm_guest.c
>>>> @@ -39,6 +39,8 @@ void __init kvm_init_hyp_services(void)
>>>>       pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 
>>>> 0x%08lx)\n",
>>>>            res.a3, res.a2, res.a1, res.a0);
>>>> +
>>>> +    kvm_arch_init_hyp_services();
>>>>   }
>>>>
>>>
>>> That is a bit late to detect RMM? One of the requirements is to
>>> figure out the pgprot_t flags for early_ioremap so that "earlycon" will
>>> work (by mapping the address as shared alias). To do that we need to
>>> make an RSI call to detect PROT_NS_SHARED mask as below.
>>>
>>>     if (rsi_get_realm_config(&config))
>>>         return;
>>>     prot_ns_shared = BIT(config.ipa_bits - 1);
>>
>> Why can't the earlycon MMIO address just have that high bit set?
> 
> Do you mean the "earlycon=<MMIO-address-to-use>" ? That could work,
> except that :
> 1. It breaks the Realm's view of the {I}"PA" space
> 2. If the address is missed, it is fatal for the Realm.
> 3. All higher level tools that specify the command line parameter now
>     need to fixup the "MMIO" address, based on the "ipa_bits" chosen by
>     the VMM (which could vary with the VMMs and Hyp/System)
> 
> Also, we are making some changes to the guest support to make it future
> proof for running the same guest in a less privileged context within
> R_EL1 (read unprivileged plane with RMM v1.1), where the console
> could be "private". And we are modifying the code to "apply" the prot_ns
> dynamically (instead of hard coding it for all I/O).

Also, forgot to add, we do need this for EFI runtim services where EFI
mapping may need to apply the "Shared" bit for MMIO.


See:

https://lore.kernel.org/all/20240701095505.165383-12-steven.price@arm.com/

Suzuki


> 
> Suzuki
> 
>>
>> I think it's horribly fragile to try detecting all of this stuff before
>> we're allowed to touch the console. We don't even bother with pKVM --
>> it's the guest firmware's responsibility to MMIO_GUARD the UART if it
>> detects a debuggable payload.
>>
>> Will
>>
>
Will Deacon Aug. 23, 2024, 1:13 p.m. UTC | #10
On Fri, Aug 02, 2024 at 04:30:30PM +0100, Suzuki K Poulose wrote:
> On 31/07/2024 16:50, Will Deacon wrote:
> > On Wed, Jul 31, 2024 at 08:11:16PM +0530, Aneesh Kumar K.V wrote:
> > > Will Deacon <will@kernel.org> writes:
> > > 
> > > > From: Marc Zyngier <maz@kernel.org>
> > > > 
> > > > arm64 will soon require its own callback to initialise services
> > > > that are only available on this architecture. Introduce a hook
> > > > that can be overloaded by the architecture.
> > > > 
> > > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > > Signed-off-by: Will Deacon <will@kernel.org>
> > > > ---
> > > >   arch/arm/include/asm/hypervisor.h   | 2 ++
> > > >   arch/arm64/include/asm/hypervisor.h | 4 ++++
> > > >   drivers/firmware/smccc/kvm_guest.c  | 2 ++
> > > >   3 files changed, 8 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h
> > > > index bd61502b9715..8a648e506540 100644
> > > > --- a/arch/arm/include/asm/hypervisor.h
> > > > +++ b/arch/arm/include/asm/hypervisor.h
> > > > @@ -7,4 +7,6 @@
> > > >   void kvm_init_hyp_services(void);
> > > >   bool kvm_arm_hyp_service_available(u32 func_id);
> > > > +static inline void kvm_arch_init_hyp_services(void) { };
> > > > +
> > > >   #endif
> > > > diff --git a/arch/arm64/include/asm/hypervisor.h b/arch/arm64/include/asm/hypervisor.h
> > > > index 0ae427f352c8..8cab2ab535b7 100644
> > > > --- a/arch/arm64/include/asm/hypervisor.h
> > > > +++ b/arch/arm64/include/asm/hypervisor.h
> > > > @@ -7,4 +7,8 @@
> > > >   void kvm_init_hyp_services(void);
> > > >   bool kvm_arm_hyp_service_available(u32 func_id);
> > > > +static inline void kvm_arch_init_hyp_services(void)
> > > > +{
> > > > +};
> > > > +
> > > >   #endif
> > > > diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
> > > > index 89a68e7eeaa6..f3319be20b36 100644
> > > > --- a/drivers/firmware/smccc/kvm_guest.c
> > > > +++ b/drivers/firmware/smccc/kvm_guest.c
> > > > @@ -39,6 +39,8 @@ void __init kvm_init_hyp_services(void)
> > > >   	pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
> > > >   		 res.a3, res.a2, res.a1, res.a0);
> > > > +
> > > > +	kvm_arch_init_hyp_services();
> > > >   }
> > > > 
> > > 
> > > That is a bit late to detect RMM? One of the requirements is to
> > > figure out the pgprot_t flags for early_ioremap so that "earlycon" will
> > > work (by mapping the address as shared alias). To do that we need to
> > > make an RSI call to detect PROT_NS_SHARED mask as below.
> > > 
> > > 	if (rsi_get_realm_config(&config))
> > > 		return;
> > > 	prot_ns_shared = BIT(config.ipa_bits - 1);
> > 
> > Why can't the earlycon MMIO address just have that high bit set?
> 
> Do you mean the "earlycon=<MMIO-address-to-use>" ? That could work,
> except that :
> 1. It breaks the Realm's view of the {I}"PA" space

Breaks in what sense? It won't work?

> 2. If the address is missed, it is fatal for the Realm.

If earlycon= has broken/missing addresses, it's going to be fatal for
whoever is consuming it anyway, no?

> 3. All higher level tools that specify the command line parameter now
>    need to fixup the "MMIO" address, based on the "ipa_bits" chosen by
>    the VMM (which could vary with the VMMs and Hyp/System)

Should be fine for earlycon, though?

If not, then perhaps the RSI should include calls for putchar() so that
earlycon can be implemented using a service rather than trying to use
MMIO with a bunch of awkward caveats.

Will
diff mbox series

Patch

diff --git a/arch/arm/include/asm/hypervisor.h b/arch/arm/include/asm/hypervisor.h
index bd61502b9715..8a648e506540 100644
--- a/arch/arm/include/asm/hypervisor.h
+++ b/arch/arm/include/asm/hypervisor.h
@@ -7,4 +7,6 @@ 
 void kvm_init_hyp_services(void);
 bool kvm_arm_hyp_service_available(u32 func_id);
 
+static inline void kvm_arch_init_hyp_services(void) { };
+
 #endif
diff --git a/arch/arm64/include/asm/hypervisor.h b/arch/arm64/include/asm/hypervisor.h
index 0ae427f352c8..8cab2ab535b7 100644
--- a/arch/arm64/include/asm/hypervisor.h
+++ b/arch/arm64/include/asm/hypervisor.h
@@ -7,4 +7,8 @@ 
 void kvm_init_hyp_services(void);
 bool kvm_arm_hyp_service_available(u32 func_id);
 
+static inline void kvm_arch_init_hyp_services(void)
+{
+};
+
 #endif
diff --git a/drivers/firmware/smccc/kvm_guest.c b/drivers/firmware/smccc/kvm_guest.c
index 89a68e7eeaa6..f3319be20b36 100644
--- a/drivers/firmware/smccc/kvm_guest.c
+++ b/drivers/firmware/smccc/kvm_guest.c
@@ -39,6 +39,8 @@  void __init kvm_init_hyp_services(void)
 
 	pr_info("hypervisor services detected (0x%08lx 0x%08lx 0x%08lx 0x%08lx)\n",
 		 res.a3, res.a2, res.a1, res.a0);
+
+	kvm_arch_init_hyp_services();
 }
 
 bool kvm_arm_hyp_service_available(u32 func_id)