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 |
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
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
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 <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
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.
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 >
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.
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
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 >> >
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 --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)