[RFC PATCH 0/2] Add virtual SDEI support for arm64

[RFC PATCH 0/2] Add virtual SDEI support for arm64

[Heyi Guo]

As promised, this is the first RFC patch set for arm64 virtual SDEI
support.

New kvm capability KVM_CAP_FORWARD_HYPERCALL is added to probe if kvm
supports forwarding hypercalls, and the capability should be enabled
explicitly. PSCI can be set as exception for backward compatibility.

We reuse the existing term "hypercall" for SMC/HVC, as well as the
hypercall structure in kvm_run to exchange arguments and return
values. The definition on arm64 is as below:

exit_reason: KVM_EXIT_HYPERCALL

Input:
  nr: the immediate value of SMC/HVC calls; not really used today.
  args[6]: x0..x5 (This is not fully conform with SMCCC which requires
           x6 as argument as well, but use space can use GET_ONE_REG
           ioctl for such rare case).

Return:
  args[0..3]: x0..x3 as defined in SMCCC. We need to extract
              args[0..3] and write them to x0..x3 when hypercall exit
              returns.

And there is a corresponding patch set for qemu.

Please give your comments and suggestions.

Thanks,
HG

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Dave Martin <Dave.Martin@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Morse <james.morse@arm.com>

Heyi Guo (2):
  kvm/arm: add capability to forward hypercall to user space
  kvm/arm64: expose hypercall_forwarding capability

 arch/arm/include/asm/kvm_host.h   |  5 +++++
 arch/arm64/include/asm/kvm_host.h |  5 +++++
 arch/arm64/kvm/reset.c            | 25 +++++++++++++++++++++++++
 include/kvm/arm_psci.h            |  1 +
 include/uapi/linux/kvm.h          |  3 +++
 virt/kvm/arm/arm.c                |  2 ++
 virt/kvm/arm/psci.c               | 30 ++++++++++++++++++++++++++++--
 7 files changed, 69 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[RFC PATCH 1/2] kvm/arm: add capability to forward hypercall to user space

[Heyi Guo]

As more SMC/HVC usages emerge on arm64 platforms, like SDEI, it makes
sense for kvm to have the capability of forwarding such calls to user
space for further emulation.

We reuse the existing term "hypercall" for SMC/HVC, as well as the
hypercall structure in kvm_run to exchange arguments and return
values. The definition on arm64 is as below:

exit_reason: KVM_EXIT_HYPERCALL

Input:
  nr: the immediate value of SMC/HVC calls; not really used today.
  args[6]: x0..x5 (This is not fully conform with SMCCC which requires
           x6 as argument as well, but use space can use GET_ONE_REG
           ioctl for such rare case).

Return:
  args[0..3]: x0..x3 as defined in SMCCC. We need to extract
              args[0..3] and write them to x0..x3 when hypercall exit
              returns.

Flag hypercall_forward is added to turn on/off hypercall forwarding
and the default is false. Another flag hypercall_excl_psci is to
exclude PSCI from forwarding for backward compatible, and it only
makes sense to check its value when hypercall_forward is enabled.

Signed-off-by: Heyi Guo <guoheyi@huawei.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Dave Martin <Dave.Martin@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Julien Thierry <julien.thierry.kdev@gmail.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
CC: Russell King <linux@armlinux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm/include/asm/kvm_host.h   |  5 +++++
 arch/arm64/include/asm/kvm_host.h |  5 +++++
 include/kvm/arm_psci.h            |  1 +
 virt/kvm/arm/arm.c                |  2 ++
 virt/kvm/arm/psci.c               | 30 ++++++++++++++++++++++++++++--
 5 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 8a37c8e..68ccaf0 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -76,6 +76,11 @@ struct kvm_arch {
 
 	/* Mandated version of PSCI */
 	u32 psci_version;
+
+	/* Flags to control hypercall forwarding to userspace */
+	bool hypercall_forward;
+	/* Exclude PSCI from hypercall forwarding and let kvm to handle it */
+	bool hypercall_excl_psci;
 };
 
 #define KVM_NR_MEM_OBJS     40
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f656169..e47ac25 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -83,6 +83,11 @@ struct kvm_arch {
 
 	/* Mandated version of PSCI */
 	u32 psci_version;
+
+	/* Flags to control hypercall forwarding to userspace */
+	bool hypercall_forward;
+	/* Exclude PSCI from hypercall forwarding and let kvm to handle it */
+	bool hypercall_excl_psci;
 };
 
 #define KVM_NR_MEM_OBJS     40
diff --git a/include/kvm/arm_psci.h b/include/kvm/arm_psci.h
index 632e78b..9c9a2dc 100644
--- a/include/kvm/arm_psci.h
+++ b/include/kvm/arm_psci.h
@@ -48,5 +48,6 @@ static inline int kvm_psci_version(struct kvm_vcpu *vcpu, struct kvm *kvm)
 int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+void kvm_handle_hypercall_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
 
 #endif /* __KVM_ARM_PSCI_H__ */
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 35a0698..2f4ca21 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -673,6 +673,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
 		if (ret)
 			return ret;
+	} else if (run->exit_reason == KVM_EXIT_HYPERCALL) {
+		kvm_handle_hypercall_return(vcpu, vcpu->run);
 	}
 
 	if (run->immediate_exit)
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index 87927f7..7e1f735 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -389,6 +389,7 @@ static int kvm_psci_call(struct kvm_vcpu *vcpu)
 
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
+	struct kvm *kvm = vcpu->kvm;
 	u32 func_id = smccc_get_function(vcpu);
 	u32 val = SMCCC_RET_NOT_SUPPORTED;
 	u32 feature;
@@ -428,8 +429,27 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 			break;
 		}
 		break;
-	default:
-		return kvm_psci_call(vcpu);
+	default: {
+		if (!kvm->arch.hypercall_forward ||
+		    kvm->arch.hypercall_excl_psci) {
+			u32 id = func_id & ~PSCI_0_2_64BIT;
+
+			if (id >= PSCI_0_2_FN_BASE && id <= PSCI_0_2_FN(0x1f))
+				return kvm_psci_call(vcpu);
+		}
+
+		if (kvm->arch.hypercall_forward) {
+			/* Exit to user space to process */
+			vcpu->run->exit_reason = KVM_EXIT_HYPERCALL;
+			vcpu->run->hypercall.nr = kvm_vcpu_get_hsr(vcpu) &
+						  ESR_ELx_ISS_MASK;
+			vcpu->run->hypercall.args[0] = func_id;
+			vcpu->run->hypercall.args[1] = smccc_get_arg1(vcpu);
+			vcpu->run->hypercall.args[2] = smccc_get_arg2(vcpu);
+			vcpu->run->hypercall.args[3] = smccc_get_arg3(vcpu);
+			return 0;
+		}
+	}
 	}
 
 	smccc_set_retval(vcpu, val, 0, 0, 0);
@@ -603,3 +623,9 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 	return -EINVAL;
 }
+
+void kvm_handle_hypercall_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	smccc_set_retval(vcpu, run->hypercall.args[0], run->hypercall.args[1],
+			 run->hypercall.args[2], run->hypercall.args[3]);
+}
-- 
1.8.3.1

Re: [RFC PATCH 1/2] kvm/arm: add capability to forward hypercall to user space

[James Morse]

Hi Heyi,

On 24/09/2019 16:20, Heyi Guo wrote:
> As more SMC/HVC usages emerge on arm64 platforms, like SDEI, it makes
> sense for kvm to have the capability of forwarding such calls to user
> space for further emulation.

(what do you mean by further? Doesn't user-space have to do all of it?)


> We reuse the existing term "hypercall" for SMC/HVC, as well as the
> hypercall structure in kvm_run to exchange arguments and return
> values. The definition on arm64 is as below:
> 
> exit_reason: KVM_EXIT_HYPERCALL
> 
> Input:
>   nr: the immediate value of SMC/HVC calls; not really used today.

>   args[6]: x0..x5 (This is not fully conform with SMCCC which requires
>            x6 as argument as well, but use space can use GET_ONE_REG
>            ioctl for such rare case).

If this structure isn't right for us, we could define a different one for arm/arm64.
(we did this for kvm_vcpu_events)


> Return:
>   args[0..3]: x0..x3 as defined in SMCCC. We need to extract
>               args[0..3] and write them to x0..x3 when hypercall exit
>               returns.

Are we saying that KVM_EXIT_HYPERCALL expects to be used with SMC-CC?
(if so, we should state that).

I'm not certain we should tie this to SMC-CC.

If we don't tie it to SMC-CC this selection of in/out registers looks odd, there is
nothing about HVC/SMC that uses these registers, its just the SMC convention.


> Flag hypercall_forward is added to turn on/off hypercall forwarding
> and the default is false. Another flag hypercall_excl_psci is to
> exclude PSCI from forwarding for backward compatible, and it only
> makes sense to check its value when hypercall_forward is enabled.

Calling out PSCI like this is something we shouldn't do. There will be, (are!) other
SMC-CC calls that the kernel provides emulation for, we can't easily add to this list.

I think the best way to avoid this, is to say the hypercall mechanism forwards 'unhandled
SMC/HVC' to user-space. Which things the kernel chooses to handle can change.

We need a way for user-space to know which SMC/HVC calls the kernel will handle, and will
not forward. A suggestion is to add a co-processor that lists these by #imm and r0/x0
value. User-space can then query any call to find out if it would be exported if the guest
made that call. Something like kvm_arm_get_fw_reg().

I agree it should be possible to export the PSCI range to user-space, so that user-space
can provide a newer/better version than the kernel emulates, or prevent secondary cores
coming online. (we should check how gracefully the kernel handles that... it doesn't
happen on real systems)
This could be done with something like kvm_vm_ioctl_enable_cap(), one option is to use the
args to toggle the on/off value, but it may be simpler to expose a
KVM_CAP_ARM_PSCI_TO_USER that can be enabled.


Please update Documentation/virt/kvm/api.txt as part of the patches that make user-visible
changes.

For 32bit, are we going to export SMC/HVC calls that failed their condition-code checks?

The hypercall structure should probably indicate whether the SMC/HVC call came from
aarch32 or aarch64, as the behaviour may be different.


Thanks,

James

Re: [RFC PATCH 1/2] kvm/arm: add capability to forward hypercall to user space

[Guoheyi]

Sorry for late response as we had our long holiday last week :)


On 2019/10/2 1:19, James Morse wrote:
> Hi Heyi,
>
> On 24/09/2019 16:20, Heyi Guo wrote:
>> As more SMC/HVC usages emerge on arm64 platforms, like SDEI, it makes
>> sense for kvm to have the capability of forwarding such calls to user
>> space for further emulation.
> (what do you mean by further? Doesn't user-space have to do all of it?)
For kvm will always handle hvc/smc guest exit for the first step, even 
if it is only a simple forwarding, I called the user-space processing as 
"further emulation".

>> We reuse the existing term "hypercall" for SMC/HVC, as well as the
>> hypercall structure in kvm_run to exchange arguments and return
>> values. The definition on arm64 is as below:
>>
>> exit_reason: KVM_EXIT_HYPERCALL
>>
>> Input:
>>    nr: the immediate value of SMC/HVC calls; not really used today.
>>    args[6]: x0..x5 (This is not fully conform with SMCCC which requires
>>             x6 as argument as well, but use space can use GET_ONE_REG
>>             ioctl for such rare case).
> If this structure isn't right for us, we could define a different one for arm/arm64.
> (we did this for kvm_vcpu_events)
Do you mean that we can move the hypercall struct definition to arch 
specific kvm_host.h? For it is in the common kvm_run structure, we'll 
need to change every kvm supported architectures, including x86, mips, 
powerpc, s390. Is it acceptable?

I found another solution from papr which defines its own hypercall 
structure in the kvm_run union definition:

         /* KVM_EXIT_PAPR_HCALL */
         struct {
             __u64 nr;
             __u64 ret;
             __u64 args[9];
         } papr_hcall;

How about we define a new structure for ARM/ARM64 specifically?

>
>> Return:
>>    args[0..3]: x0..x3 as defined in SMCCC. We need to extract
>>                args[0..3] and write them to x0..x3 when hypercall exit
>>                returns.
> Are we saying that KVM_EXIT_HYPERCALL expects to be used with SMC-CC?
> (if so, we should state that).
Yes I followed SMC-CC when writing this.
>
> I'm not certain we should tie this to SMC-CC.
>
> If we don't tie it to SMC-CC this selection of in/out registers looks odd, there is
> nothing about HVC/SMC that uses these registers, its just the SMC convention.
Maybe we don't need to tie it to SMC-CC, and simply load all values in 
args[6] to GP registers...
And then there is either no strong reason to extend hypercall structure 
for ARM.
>
>> Flag hypercall_forward is added to turn on/off hypercall forwarding
>> and the default is false. Another flag hypercall_excl_psci is to
>> exclude PSCI from forwarding for backward compatible, and it only
>> makes sense to check its value when hypercall_forward is enabled.
> Calling out PSCI like this is something we shouldn't do. There will be, (are!) other
> SMC-CC calls that the kernel provides emulation for, we can't easily add to this list.
Yes; I didn't figure out good way to keep compatibility and future 
extension...
> I think the best way to avoid this, is to say the hypercall mechanism forwards 'unhandled
> SMC/HVC' to user-space. Which things the kernel chooses to handle can change.
>
> We need a way for user-space to know which SMC/HVC calls the kernel will handle, and will
> not forward. A suggestion is to add a co-processor that lists these by #imm and r0/x0
> value. User-space can then query any call to find out if it would be exported if the guest
> made that call. Something like kvm_arm_get_fw_reg().
Do you mean we add only one co-processor to list all SMC/HVC calls 
kernel will handle? So the reg size should be large enough to hold the 
list, each entry of which contains a #imm and r0/x0 pair? Is the reg 
size fixed by definition or it can be queried by user-space? If it is 
fixed, what's the size should we choose?

Does it make sense to extend the entry to hold the function ID base and 
limit, so that it can describe the whole range for each function group, 
like PSCI, SDEI, etc?

>
> I agree it should be possible to export the PSCI range to user-space, so that user-space
> can provide a newer/better version than the kernel emulates, or prevent secondary cores
> coming online. (we should check how gracefully the kernel handles that... it doesn't
> happen on real systems)
> This could be done with something like kvm_vm_ioctl_enable_cap(), one option is to use the
> args to toggle the on/off value, but it may be simpler to expose a
> KVM_CAP_ARM_PSCI_TO_USER that can be enabled.
Sounds good. Then it may not be something we need to do in this patch 
set :) We can postpone this change when user-space PSCI is ready.
>
>
> Please update Documentation/virt/kvm/api.txt as part of the patches that make user-visible
> changes.
Sure; I can do that when we determine the interfaces.
>
> For 32bit, are we going to export SMC/HVC calls that failed their condition-code checks?
I'm not familiar with 32bit, either we don't have 32bit platforms to 
test the code. So my preference is not to make many changes to 32bit...
>
> The hypercall structure should probably indicate whether the SMC/HVC call came from
> aarch32 or aarch64, as the behaviour may be different.
How about to use the longmode field in hypercall structure? Standard 
service calls will indicate this in function ID, but we may need to know 
before parsing the function ID, isn't it?

Really appreciate your comments and suggestions. They are really helpful.

Heyi

>
>
> Thanks,
>
> James
>
> .
>

Re: [RFC PATCH 1/2] kvm/arm: add capability to forward hypercall to user space

[James Morse]

Hi Heyi,

On 09/10/2019 13:33, Guoheyi wrote:
> On 2019/10/2 1:19, James Morse wrote:
>> On 24/09/2019 16:20, Heyi Guo wrote:
>>> As more SMC/HVC usages emerge on arm64 platforms, like SDEI, it makes
>>> sense for kvm to have the capability of forwarding such calls to user
>>> space for further emulation.
>> (what do you mean by further? Doesn't user-space have to do all of it?)
> For kvm will always handle hvc/smc guest exit for the first step, even if it is only a
> simple forwarding, I called the user-space processing as "further emulation".
> 
>>> We reuse the existing term "hypercall" for SMC/HVC, as well as the
>>> hypercall structure in kvm_run to exchange arguments and return
>>> values. The definition on arm64 is as below:
>>>
>>> exit_reason: KVM_EXIT_HYPERCALL
>>>
>>> Input:
>>>    nr: the immediate value of SMC/HVC calls; not really used today.
>>>    args[6]: x0..x5 (This is not fully conform with SMCCC which requires
>>>             x6 as argument as well, but use space can use GET_ONE_REG
>>>             ioctl for such rare case).

>> If this structure isn't right for us, we could define a different one for arm/arm64.
>> (we did this for kvm_vcpu_events)

> Do you mean that we can move the hypercall struct definition to arch specific kvm_host.h?
> For it is in the common kvm_run structure, we'll need to change every kvm supported
> architectures, including x86, mips, powerpc, s390. Is it acceptable?

Ah! Sorry, I'd missed this was in the kvm_run structure. The get-events example doesn't
apply here as that was a separate ioctl().


>>> Return:
>>>    args[0..3]: x0..x3 as defined in SMCCC. We need to extract
>>>                args[0..3] and write them to x0..x3 when hypercall exit
>>>                returns.

>> Are we saying that KVM_EXIT_HYPERCALL expects to be used with SMC-CC?
>> (if so, we should state that).

> Yes I followed SMC-CC when writing this.

>>
>> I'm not certain we should tie this to SMC-CC.
>>
>> If we don't tie it to SMC-CC this selection of in/out registers looks odd, there is
>> nothing about HVC/SMC that uses these registers, its just the SMC convention.

> Maybe we don't need to tie it to SMC-CC, and simply load all values in args[6] to GP
> registers...
> And then there is either no strong reason to extend hypercall structure for ARM.


>>> Flag hypercall_forward is added to turn on/off hypercall forwarding
>>> and the default is false. Another flag hypercall_excl_psci is to
>>> exclude PSCI from forwarding for backward compatible, and it only
>>> makes sense to check its value when hypercall_forward is enabled.

>> Calling out PSCI like this is something we shouldn't do. There will be, (are!) other
>> SMC-CC calls that the kernel provides emulation for, we can't easily add to this list.

> Yes; I didn't figure out good way to keep compatibility and future extension...

I think the best trick is not to interpret the SMC/HVC calls from the guest. The kernel
obviously does, but the API shouldn't force us to.


>> I think the best way to avoid this, is to say the hypercall mechanism forwards 'unhandled
>> SMC/HVC' to user-space. Which things the kernel chooses to handle can change.
>>
>> We need a way for user-space to know which SMC/HVC calls the kernel will handle, and will
>> not forward. A suggestion is to add a co-processor that lists these by #imm and r0/x0
>> value. User-space can then query any call to find out if it would be exported if the guest
>> made that call. Something like kvm_arm_get_fw_reg().

> Do you mean we add only one co-processor to list all SMC/HVC calls kernel will handle?

Yes, some way of listing them.
e.g. user-space wants to handle HVC's with #imm==0 and w0==0x84000000, this co-processor
would list that as one of the things that the kernel will handle.

If we can find a way of describing 64bit register values that would save them from being a
problem in the future, but it may be too complicated to describe a 64bit register space
and 16 bits of immediate.

I think its okay for this co-processor to be SMC-CC specific, as its describing what the
kernel supports. The KVM-api in contrast should be flexible enough to describe anything
any guest may wish to do.


> So
> the reg size should be large enough to hold the list, each entry of which contains a #imm
> and r0/x0 pair? Is the reg size fixed by definition or it can be queried by user-space? If
> it is fixed, what's the size should we choose?

(fixed/not-fixed - its a trade-off for complexity now, but no-one may ever use the full
flexibility).

I think we can assume the kernel will only offer things that look like SMC-CC to the
guest. If the guest does something outside this space, its up to user-space to handle. (so
the KVM-API must support non-SMC-CC stuff). I think we should define a co-processor for
SMC/HVC where the #imm is 0. This then gives us 32bits of space we can map directly onto
the w0 values.


> Does it make sense to extend the entry to hold the function ID base and limit, so that it
> can describe the whole range for each function group, like PSCI, SDEI, etc?

This may be over-complex, user-space would always need to enumerate the whole thing. I
think commonly user-space would only want to know about one entry: For cases where we know
the structure, user-space can just query the '_VERSION' call. If that isn't supported,
user-space can assume the rest of that space is unimplemented. (the kernel shouldn't
provide an incomplete emulation of these APIs)


>> For 32bit, are we going to export SMC/HVC calls that failed their condition-code checks?

> I'm not familiar with 32bit, either we don't have 32bit platforms to test the code. So my
> preference is not to make many changes to 32bit...

I'm not that familiar with it either ... You don't have anything with aarch32 support at
EL1? I don't think we should add an API that only works with Aarch64 guests.

For 32bit, we either need to expose these condition-code bits, and say user-space should
work out if it needs to do anything. Or, handle this in the kernel, in which case we don't
need to expose the condition-code bits, but we should document that the kernel will do the
check.


Nested-virt may cause some 'fun' here. If user-space starts an aarch64 guest at EL2, it
may start its own aarch32 guest at EL1. If the aarch32 guest makes an SMC, who handles it?
If user-space's aarch64 guest didn't set the traps for SMC, I think this should be
delivered to user-space, which may be surprised by the request from an aarch32 guest.

(its also possible nested-virt has me confused, it is pretty mind bending!)


>> The hypercall structure should probably indicate whether the SMC/HVC call came from
>> aarch32 or aarch64, as the behaviour may be different.

> How about to use the longmode field in hypercall structure? Standard service calls will
> indicate this in function ID, but we may need to know before parsing the function ID,
> isn't it?

Sure, as its a __u32, we could dump the guest PSTATE from SPSR in there.


I think the last thing is 'ret', and whether we should provide a way of passing 'x0' back
to the guest, or expect user-space to use set-one-reg. Most of the time user-space will
only want to set x0, and doing this would let us initialise it to all-ones in the kernel,
which means the guest gets the unknown-smc value back if user-space ignores the exit.


Thanks,

James

Re: [RFC PATCH 1/2] kvm/arm: add capability to forward hypercall to user space

[Guoheyi]

Sorry for the late, for it took some time for me to think it over...


On 2019/10/22 0:42, James Morse wrote:
> Hi Heyi,
>
> On 09/10/2019 13:33, Guoheyi wrote:
>> On 2019/10/2 1:19, James Morse wrote:
>>> On 24/09/2019 16:20, Heyi Guo wrote:
>>>> As more SMC/HVC usages emerge on arm64 platforms, like SDEI, it makes
>>>> sense for kvm to have the capability of forwarding such calls to user
>>>> space for further emulation.
>>> (what do you mean by further? Doesn't user-space have to do all of it?)
>> For kvm will always handle hvc/smc guest exit for the first step, even if it is only a
>> simple forwarding, I called the user-space processing as "further emulation".
>>
>>>> We reuse the existing term "hypercall" for SMC/HVC, as well as the
>>>> hypercall structure in kvm_run to exchange arguments and return
>>>> values. The definition on arm64 is as below:
>>>>
>>>> exit_reason: KVM_EXIT_HYPERCALL
>>>>
>>>> Input:
>>>>     nr: the immediate value of SMC/HVC calls; not really used today.
>>>>     args[6]: x0..x5 (This is not fully conform with SMCCC which requires
>>>>              x6 as argument as well, but use space can use GET_ONE_REG
>>>>              ioctl for such rare case).
>>> If this structure isn't right for us, we could define a different one for arm/arm64.
>>> (we did this for kvm_vcpu_events)
>> Do you mean that we can move the hypercall struct definition to arch specific kvm_host.h?
>> For it is in the common kvm_run structure, we'll need to change every kvm supported
>> architectures, including x86, mips, powerpc, s390. Is it acceptable?
> Ah! Sorry, I'd missed this was in the kvm_run structure. The get-events example doesn't
> apply here as that was a separate ioctl().
>
>
>>>> Return:
>>>>     args[0..3]: x0..x3 as defined in SMCCC. We need to extract
>>>>                 args[0..3] and write them to x0..x3 when hypercall exit
>>>>                 returns.
>>> Are we saying that KVM_EXIT_HYPERCALL expects to be used with SMC-CC?
>>> (if so, we should state that).
>> Yes I followed SMC-CC when writing this.
>>> I'm not certain we should tie this to SMC-CC.
>>>
>>> If we don't tie it to SMC-CC this selection of in/out registers looks odd, there is
>>> nothing about HVC/SMC that uses these registers, its just the SMC convention.
>> Maybe we don't need to tie it to SMC-CC, and simply load all values in args[6] to GP
>> registers...
>> And then there is either no strong reason to extend hypercall structure for ARM.
>
>>>> Flag hypercall_forward is added to turn on/off hypercall forwarding
>>>> and the default is false. Another flag hypercall_excl_psci is to
>>>> exclude PSCI from forwarding for backward compatible, and it only
>>>> makes sense to check its value when hypercall_forward is enabled.
>>> Calling out PSCI like this is something we shouldn't do. There will be, (are!) other
>>> SMC-CC calls that the kernel provides emulation for, we can't easily add to this list.
>> Yes; I didn't figure out good way to keep compatibility and future extension...
> I think the best trick is not to interpret the SMC/HVC calls from the guest. The kernel
> obviously does, but the API shouldn't force us to.
>
>
>>> I think the best way to avoid this, is to say the hypercall mechanism forwards 'unhandled
>>> SMC/HVC' to user-space. Which things the kernel chooses to handle can change.
>>>
>>> We need a way for user-space to know which SMC/HVC calls the kernel will handle, and will
>>> not forward. A suggestion is to add a co-processor that lists these by #imm and r0/x0
>>> value. User-space can then query any call to find out if it would be exported if the guest
>>> made that call. Something like kvm_arm_get_fw_reg().
>> Do you mean we add only one co-processor to list all SMC/HVC calls kernel will handle?
> Yes, some way of listing them.
> e.g. user-space wants to handle HVC's with #imm==0 and w0==0x84000000, this co-processor
> would list that as one of the things that the kernel will handle.
>
> If we can find a way of describing 64bit register values that would save them from being a
> problem in the future, but it may be too complicated to describe a 64bit register space
> and 16 bits of immediate.
>
> I think its okay for this co-processor to be SMC-CC specific, as its describing what the
> kernel supports. The KVM-api in contrast should be flexible enough to describe anything
> any guest may wish to do.
>
>
>> So
>> the reg size should be large enough to hold the list, each entry of which contains a #imm
>> and r0/x0 pair? Is the reg size fixed by definition or it can be queried by user-space? If
>> it is fixed, what's the size should we choose?
> (fixed/not-fixed - its a trade-off for complexity now, but no-one may ever use the full
> flexibility).
>
> I think we can assume the kernel will only offer things that look like SMC-CC to the
> guest. If the guest does something outside this space, its up to user-space to handle. (so
> the KVM-API must support non-SMC-CC stuff). I think we should define a co-processor for
> SMC/HVC where the #imm is 0. This then gives us 32bits of space we can map directly onto
> the w0 values.
Shall we setup a new class of co-processor and use the following id bit 
patterns (assuming the type index to be 0x0016)?

0x6030 <high 16 bits of SMC-CC function ID> 0016 <low 16 bits of SMC-CC 
function ID>

And the value of the co-processor returned to user space can be 0 (KVM 
will not handle) or 1 (KVM will handle)?

>
>
>> Does it make sense to extend the entry to hold the function ID base and limit, so that it
>> can describe the whole range for each function group, like PSCI, SDEI, etc?
> This may be over-complex, user-space would always need to enumerate the whole thing. I
> think commonly user-space would only want to know about one entry: For cases where we know
> the structure, user-space can just query the '_VERSION' call. If that isn't supported,
> user-space can assume the rest of that space is unimplemented. (the kernel shouldn't
> provide an incomplete emulation of these APIs)
>
>
>>> For 32bit, are we going to export SMC/HVC calls that failed their condition-code checks?
>> I'm not familiar with 32bit, either we don't have 32bit platforms to test the code. So my
>> preference is not to make many changes to 32bit...
> I'm not that familiar with it either ... You don't have anything with aarch32 support at
> EL1? I don't think we should add an API that only works with Aarch64 guests.
We have some D05 which is based on cortex A72 and should support aarch32 
guest. I can take a try.

Our object is to support aarch32 guest on an aarch64 hypervisor, but not 
on an aarch32 hypervisor, isn't it?

>
> For 32bit, we either need to expose these condition-code bits, and say user-space should
> work out if it needs to do anything. Or, handle this in the kernel, in which case we don't
> need to expose the condition-code bits, but we should document that the kernel will do the
> check.
>
>
> Nested-virt may cause some 'fun' here. If user-space starts an aarch64 guest at EL2, it
> may start its own aarch32 guest at EL1. If the aarch32 guest makes an SMC, who handles it?
> If user-space's aarch64 guest didn't set the traps for SMC, I think this should be
> delivered to user-space, which may be surprised by the request from an aarch32 guest.
>
> (its also possible nested-virt has me confused, it is pretty mind bending!)
>
>
>>> The hypercall structure should probably indicate whether the SMC/HVC call came from
>>> aarch32 or aarch64, as the behaviour may be different.
>> How about to use the longmode field in hypercall structure? Standard service calls will
>> indicate this in function ID, but we may need to know before parsing the function ID,
>> isn't it?
> Sure, as its a __u32, we could dump the guest PSTATE from SPSR in there.
>
>
> I think the last thing is 'ret', and whether we should provide a way of passing 'x0' back
> to the guest, or expect user-space to use set-one-reg. Most of the time user-space will
> only want to set x0, and doing this would let us initialise it to all-ones in the kernel,
> which means the guest gets the unknown-smc value back if user-space ignores the exit.
The current RFC is not expecting user-space to use set-one-reg to set GP 
registers for returning, to reduce ioctl() invocations for better 
performance.
I didn't use "ret" for guest to hold the returned x0, but still used 
"args[6]" to exchange x0~x5. I agree to set quick path for x0 only, and 
kvm doesn't bother to set the other 5 GP registers.

Thanks a lot,

Heyi

>
>
> Thanks,
>
> James
>
> .
>

[RFC PATCH 2/2] kvm/arm64: expose hypercall_forwarding capability

[Heyi Guo]

Add new KVM capability "KVM_CAP_FORWARD_HYPERCALL" for user space to
probe whether KVM supports forwarding hypercall.

The capability should be enabled by user space explicitly, for we
don't want user space application to deal with unexpected hypercall
exits. We also use an additional argument to pass exception bit mask,
to request KVM to forward all hypercalls except the classes specified
in the bit mask.

Currently only PSCI can be set as exception, so that we can still keep
consistent with the old PSCI processing flow.

Signed-off-by: Heyi Guo <guoheyi@huawei.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Dave Martin <Dave.Martin@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Julien Thierry <julien.thierry.kdev@gmail.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
---
 arch/arm64/kvm/reset.c   | 25 +++++++++++++++++++++++++
 include/uapi/linux/kvm.h |  3 +++
 2 files changed, 28 insertions(+)

diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index f4a8ae9..2201b62 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -95,6 +95,9 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		r = has_vhe() && system_supports_address_auth() &&
 				 system_supports_generic_auth();
 		break;
+	case KVM_CAP_FORWARD_HYPERCALL:
+		r = 1;
+		break;
 	default:
 		r = 0;
 	}
@@ -102,6 +105,28 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	return r;
 }
 
+int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
+			    struct kvm_enable_cap *cap)
+{
+	if (cap->flags)
+		return -EINVAL;
+
+	switch (cap->cap) {
+	case KVM_CAP_FORWARD_HYPERCALL: {
+		__u64 exclude_flags = cap->args[0];
+		/* Only support excluding PSCI right now */
+		if (exclude_flags & ~KVM_CAP_FORWARD_HYPERCALL_EXCL_PSCI)
+			return -EINVAL;
+		kvm->arch.hypercall_forward = true;
+		if (exclude_flags & KVM_CAP_FORWARD_HYPERCALL_EXCL_PSCI)
+			kvm->arch.hypercall_excl_psci = true;
+		return 0;
+	}
+	}
+
+	return -EINVAL;
+}
+
 unsigned int kvm_sve_max_vl;
 
 int kvm_arm_init_sve(void)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 5e3f12d..e3e5787 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -711,6 +711,8 @@ struct kvm_enable_cap {
 	__u8  pad[64];
 };
 
+#define KVM_CAP_FORWARD_HYPERCALL_EXCL_PSCI	(1 << 0)
+
 /* for KVM_PPC_GET_PVINFO */
 
 #define KVM_PPC_PVINFO_FLAGS_EV_IDLE   (1<<0)
@@ -996,6 +998,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_ARM_PTRAUTH_ADDRESS 171
 #define KVM_CAP_ARM_PTRAUTH_GENERIC 172
 #define KVM_CAP_PMU_EVENT_FILTER 173
+#define KVM_CAP_FORWARD_HYPERCALL 174
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
1.8.3.1

Re: [RFC PATCH 2/2] kvm/arm64: expose hypercall_forwarding capability

[James Morse]

Hi Heyi,

On 24/09/2019 16:20, Heyi Guo wrote:
> Add new KVM capability "KVM_CAP_FORWARD_HYPERCALL" for user space to
> probe whether KVM supports forwarding hypercall.
> 
> The capability should be enabled by user space explicitly, for we
> don't want user space application to deal with unexpected hypercall
> exits. We also use an additional argument to pass exception bit mask,
> to request KVM to forward all hypercalls except the classes specified
> in the bit mask.
> 
> Currently only PSCI can be set as exception, so that we can still keep
> consistent with the old PSCI processing flow.

I agree this needs to be default-on, but I don't think this exclusion mechanism is extensible.


> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index f4a8ae9..2201b62 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -102,6 +105,28 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	return r;
>  }
>  
> +int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> +			    struct kvm_enable_cap *cap)
> +{
> +	if (cap->flags)
> +		return -EINVAL;
> +
> +	switch (cap->cap) {
> +	case KVM_CAP_FORWARD_HYPERCALL: {
> +		__u64 exclude_flags = cap->args[0];

and if there are more than 64 things to exclude?


> +		/* Only support excluding PSCI right now */
> +		if (exclude_flags & ~KVM_CAP_FORWARD_HYPERCALL_EXCL_PSCI)
> +			return -EINVAL;

Once we have a 65th bit, older kernels will let user-space set it, but nothing happens.


> +		kvm->arch.hypercall_forward = true;
> +		if (exclude_flags & KVM_CAP_FORWARD_HYPERCALL_EXCL_PSCI)
> +			kvm->arch.hypercall_excl_psci = true;
> +		return 0;
> +	}
> +	}
> +
> +	return -EINVAL;
> +}

While 4*64 'named bits' for SMC/HVC ranges might be enough, it is tricky to work with.
Both the kernel and user-space have to maintain a list of bit->name and
name->call-number-range, which may change over time.

A case in point: According to PSCI's History (Section 7 of DEN022D), PSCIv1.1 added
SYSTEM_RESET2, MEM_PROTECT and MEM_PROTECT_CHECK_RANGE.

I think its simpler for the HYPERCALL thing to act as a catch-all, and we provide
something to enumerate the list of function id's the kernel implements.

We can then add controls to disable the PSCI (which I think is the only one we have a case
for disabling). I think the PSCI disable should wait until it has a user.


Thanks,

James