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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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