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

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

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