diff mbox

[v8,2/4] arm64: KVM: export the capability to set guest SError syndrome

Message ID 20180713154751.18131-3-james.morse@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

James Morse July 13, 2018, 3:47 p.m. UTC 
From: Dongjiu Geng <gengdongjiu@huawei.com>

For the arm64 RAS Extension, user space can inject a virtual-SError
with specified ESR. So user space needs to know whether KVM support
to inject such SError, this interface adds this query for this capability.

KVM will check whether system support RAS Extension, if supported, KVM
returns true to user space, otherwise returns false.

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
Reviewed-by: James Morse <james.morse@arm.com>
Signed-off-by: James Morse <james.morse@arm.com>
---
 Documentation/virtual/kvm/api.txt | 11 +++++++++++
 arch/arm64/kvm/reset.c            |  3 +++
 include/uapi/linux/kvm.h          |  1 +
 3 files changed, 15 insertions(+)

Comments

Peter Maydell July 17, 2018, 4:36 p.m. UTC | #1 
On 13 July 2018 at 16:47, James Morse <james.morse@arm.com> wrote:
> From: Dongjiu Geng <gengdongjiu@huawei.com>
>
> For the arm64 RAS Extension, user space can inject a virtual-SError
> with specified ESR. So user space needs to know whether KVM support
> to inject such SError, this interface adds this query for this capability.
>
> KVM will check whether system support RAS Extension, if supported, KVM
> returns true to user space, otherwise returns false.
>
> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  Documentation/virtual/kvm/api.txt | 11 +++++++++++
>  arch/arm64/kvm/reset.c            |  3 +++
>  include/uapi/linux/kvm.h          |  1 +
>  3 files changed, 15 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index e3940f8715a5..480fa64eb52a 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -4643,3 +4643,14 @@ This capability indicates that KVM supports paravirtualized Hyper-V TLB Flush
>  hypercalls:
>  HvFlushVirtualAddressSpace, HvFlushVirtualAddressSpaceEx,
>  HvFlushVirtualAddressList, HvFlushVirtualAddressListEx.
> +
> +8.19 KVM_CAP_ARM_SET_SERROR_ESR
> +
> +Architectures: arm, arm64
> +
> +This capability indicates that userspace can specify the syndrome value reported
> +to the guest OS when guest takes a virtual SError interrupt exception.

"the guest"

> +If KVM has this capability, userspace can only specify the ISS field for the ESR
> +syndrome, it can not specify the EC field which is not under control by KVM.

Full stop or semicolon, not comma.
Not sure what the "which is not under control by KVM" part means.

> +If this virtual SError is taken to EL1 using AArch64, this value will be reported
> +in ISS filed of ESR_EL1.

"the ISS field".

How does this capability interact with the KVM_CAP_VCPU_EVENTS
API in patch 1? Is it saying "you can specify the ESR via
that API", or talking about something else?

What happens if userspace does try to specify the EC field value ?
Is this an error, or is it ignored ?

> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index a3db01a28062..067c6ba969bd 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -77,6 +77,9 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
>         case KVM_CAP_ARM_PMU_V3:
>                 r = kvm_arm_support_pmu_v3();
>                 break;
> +       case KVM_CAP_ARM_INJECT_SERROR_ESR:
> +               r = cpus_have_const_cap(ARM64_HAS_RAS_EXTN);
> +               break;
>         case KVM_CAP_SET_GUEST_DEBUG:
>         case KVM_CAP_VCPU_ATTRIBUTES:
>         case KVM_CAP_VCPU_EVENTS:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index b6270a3b38e9..a7d9bc4e4068 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -949,6 +949,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_GET_MSR_FEATURES 153
>  #define KVM_CAP_HYPERV_EVENTFD 154
>  #define KVM_CAP_HYPERV_TLBFLUSH 155
> +#define KVM_CAP_ARM_INJECT_SERROR_ESR 156
>
>  #ifdef KVM_CAP_IRQ_ROUTING
>
> --
> 2.18.0
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

thanks
-- PMM
Dongjiu Geng July 19, 2018, 8:54 a.m. UTC | #2 
Hi peter,
  Thanks for the review.

On 2018/7/18 0:36, Peter Maydell wrote:
> On 13 July 2018 at 16:47, James Morse <james.morse@arm.com> wrote:
>> From: Dongjiu Geng <gengdongjiu@huawei.com>
>>
>> For the arm64 RAS Extension, user space can inject a virtual-SError
>> with specified ESR. So user space needs to know whether KVM support
>> to inject such SError, this interface adds this query for this capability.
>>
>> KVM will check whether system support RAS Extension, if supported, KVM
>> returns true to user space, otherwise returns false.
>>
>> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
>> Reviewed-by: James Morse <james.morse@arm.com>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> ---
>>  Documentation/virtual/kvm/api.txt | 11 +++++++++++
>>  arch/arm64/kvm/reset.c            |  3 +++
>>  include/uapi/linux/kvm.h          |  1 +
>>  3 files changed, 15 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index e3940f8715a5..480fa64eb52a 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -4643,3 +4643,14 @@ This capability indicates that KVM supports paravirtualized Hyper-V TLB Flush
>>  hypercalls:
>>  HvFlushVirtualAddressSpace, HvFlushVirtualAddressSpaceEx,
>>  HvFlushVirtualAddressList, HvFlushVirtualAddressListEx.
>> +
>> +8.19 KVM_CAP_ARM_SET_SERROR_ESR
>> +
>> +Architectures: arm, arm64
>> +
>> +This capability indicates that userspace can specify the syndrome value reported
>> +to the guest OS when guest takes a virtual SError interrupt exception.
> 
> "the guest"
  got it, thanks

> 
>> +If KVM has this capability, userspace can only specify the ISS field for the ESR
>> +syndrome, it can not specify the EC field which is not under control by KVM.
> 
> Full stop or semicolon, not comma.
> Not sure what the "which is not under control by KVM" part means.
> 
>> +If this virtual SError is taken to EL1 using AArch64, this value will be reported
>> +in ISS filed of ESR_EL1.
> 
> "the ISS field".
  got it, thanks

> 
> How does this capability interact with the KVM_CAP_VCPU_EVENTS
> API in patch 1? Is it saying "you can specify the ESR via
> that API", or talking about something else?

The user space will firstly check the  KVM_CAP_VCPU_EVENTS through kvm_has_vcpu_events() to know whether KVM can support get/set vcpu events,
If KVM supports get/set vcpu events, it will check KVM_CAP_ARM_SET_SERROR_ESR to know whether can set guest SError ESR, as shown the user space code in [1].

[1]:
+static int kvm_put_vcpu_events(ARMCPU *cpu)
+{
+    CPUARMState *env = &cpu->env;
+    struct kvm_vcpu_events events = {};
+
+    if (!kvm_has_vcpu_events()) {
+        return 0;
+    }
+
+    memset(&events, 0, sizeof(events));
+    events.exception.serror_pending = env->serror.pending;
+
+    if (arm_feature(env, ARM_FEATURE_RAS)) {
+        events.exception.serror_has_esr = env->serror.has_esr;
+        events.exception.serror_esr = env->serror.esr;
+    }
+
+    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_VCPU_EVENTS, &events);
+}

> 
> What happens if userspace does try to specify the EC field value ?
> Is this an error, or is it ignored ?
if userspace try to specify the EC field value, KVM will return error to userspace, do not be ignored,


> 
>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>> index a3db01a28062..067c6ba969bd 100644
>> --- a/arch/arm64/kvm/reset.c
>> +++ b/arch/arm64/kvm/reset.c
>> @@ -77,6 +77,9 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
>>         case KVM_CAP_ARM_PMU_V3:
>>                 r = kvm_arm_support_pmu_v3();
>>                 break;
>> +       case KVM_CAP_ARM_INJECT_SERROR_ESR:
>> +               r = cpus_have_const_cap(ARM64_HAS_RAS_EXTN);
>> +               break;
>>         case KVM_CAP_SET_GUEST_DEBUG:
>>         case KVM_CAP_VCPU_ATTRIBUTES:
>>         case KVM_CAP_VCPU_EVENTS:
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index b6270a3b38e9..a7d9bc4e4068 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -949,6 +949,7 @@ struct kvm_ppc_resize_hpt {
>>  #define KVM_CAP_GET_MSR_FEATURES 153
>>  #define KVM_CAP_HYPERV_EVENTFD 154
>>  #define KVM_CAP_HYPERV_TLBFLUSH 155
>> +#define KVM_CAP_ARM_INJECT_SERROR_ESR 156
>>
>>  #ifdef KVM_CAP_IRQ_ROUTING
>>
>> --
>> 2.18.0
>>
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 
> thanks
> -- PMM
> 
> .
>
Peter Maydell July 19, 2018, 9:10 a.m. UTC | #3 
On 19 July 2018 at 09:54, gengdongjiu <gengdongjiu@huawei.com> wrote:
> Hi peter,
>   Thanks for the review.
>
> On 2018/7/18 0:36, Peter Maydell wrote:
>> How does this capability interact with the KVM_CAP_VCPU_EVENTS
>> API in patch 1? Is it saying "you can specify the ESR via
>> that API", or talking about something else?
>
> The user space will firstly check the  KVM_CAP_VCPU_EVENTS through kvm_has_vcpu_events() to know whether KVM can support get/set vcpu events,
> If KVM supports get/set vcpu events, it will check KVM_CAP_ARM_SET_SERROR_ESR to know whether can set guest SError ESR, as shown the user space code in [1].

OK, so this capability affects the behaviour of
KVM_SET_VCPU_EVENTS. Can you make sure the documentation
of KVM_SET_VCPU_EVENTS clearly explains what happens
and the effect of whether this capability is present,
please? People trying to understand the behaviour of
that ioctl are not going to know that they need to
look here as well.

thanks
-- PMM
Dongjiu Geng July 19, 2018, 9:29 a.m. UTC | #4 
On 2018/7/19 17:10, Peter Maydell wrote:
>> The user space will firstly check the  KVM_CAP_VCPU_EVENTS through kvm_has_vcpu_events() to know whether KVM can support get/set vcpu events,
>> If KVM supports get/set vcpu events, it will check KVM_CAP_ARM_SET_SERROR_ESR to know whether can set guest SError ESR, as shown the user space code in [1].
> OK, so this capability affects the behaviour of
> KVM_SET_VCPU_EVENTS. Can you make sure the documentation
> of KVM_SET_VCPU_EVENTS clearly explains what happens
> and the effect of whether this capability is present,
> please? People trying to understand the behaviour of
> that ioctl are not going to know that they need to
> look here as well.
sure, I will. Thanks peter's reminder and comments.
James Morse July 19, 2018, 2:15 p.m. UTC | #5 
Hi guys,

On 19/07/18 10:29, gengdongjiu wrote:
> On 2018/7/19 17:10, Peter Maydell wrote:
>>> The user space will firstly check the  KVM_CAP_VCPU_EVENTS through kvm_has_vcpu_events() to know whether KVM can support get/set vcpu events,
>>> If KVM supports get/set vcpu events, it will check KVM_CAP_ARM_SET_SERROR_ESR to know whether can set guest SError ESR, as shown the user space code in [1].
>> OK, so this capability affects the behaviour of
>> KVM_SET_VCPU_EVENTS. Can you make sure the documentation
>> of KVM_SET_VCPU_EVENTS clearly explains what happens
>> and the effect of whether this capability is present,
>> please? People trying to understand the behaviour of
>> that ioctl are not going to know that they need to
>> look here as well.

> sure, I will. Thanks peter's reminder and comments.

I've collected these changes, this description currently reads:
------------------%<------------------
This capability indicates that userspace can specify the syndrome value reported
to the guest when it takes a virtual SError interrupt exception.
If KVM advertises this capability, userspace can only specify the ISS field for
the ESR syndrome. Other parts of the ESR, such as the EC are generated by the
CPU when the exception is taken. If this virtual SError is taken to EL1 using
AArch64, this value will be reported in the ISS field of ESR_ELx.

See KVM_CAP_VCPU_EVENTS for more details.
------------------%<------------------

and this patch adds to the GET/SET API section with:
------------------%<------------------
SError exceptions always have an ESR value. Some CPUs have the ability to
specify what the virtual SError's ESR value should be. These systems will
advertise KVM_CAP_ARM_SET_SERROR_ESR. In this case exception.has_esr will
always have a non-zero value when read, and the agent making an SError
pending should specify the ISS field in the lower 24 bits of
exception.serror_esr. If the system supports KVM_CAP_ARM_SET_SERROR_ESR, but
user-space sets the events with exception.has_esr as zero, KVM will choose an ESR.

Specifying exception.has_esr on a system that does not support it will return an
error. Settings anything other than the lower 24bits of exception.serror_esr
will return an error.
------------------%<------------------

because as Peter points out, this is where people will expect to find this.

I'll kick this out as a v9 later today.

Thanks!

James
Peter Maydell July 19, 2018, 2:37 p.m. UTC | #6 
On 19 July 2018 at 15:15, James Morse <james.morse@arm.com> wrote:
> Hi guys,
>
> On 19/07/18 10:29, gengdongjiu wrote:
>> On 2018/7/19 17:10, Peter Maydell wrote:
>>>> The user space will firstly check the  KVM_CAP_VCPU_EVENTS through kvm_has_vcpu_events() to know whether KVM can support get/set vcpu events,
>>>> If KVM supports get/set vcpu events, it will check KVM_CAP_ARM_SET_SERROR_ESR to know whether can set guest SError ESR, as shown the user space code in [1].
>>> OK, so this capability affects the behaviour of
>>> KVM_SET_VCPU_EVENTS. Can you make sure the documentation
>>> of KVM_SET_VCPU_EVENTS clearly explains what happens
>>> and the effect of whether this capability is present,
>>> please? People trying to understand the behaviour of
>>> that ioctl are not going to know that they need to
>>> look here as well.
>
>> sure, I will. Thanks peter's reminder and comments.
>
> I've collected these changes, this description currently reads:
> ------------------%<------------------
> This capability indicates that userspace can specify the syndrome value reported
> to the guest when it takes a virtual SError interrupt exception.

Perhaps "specify (via the KVM_SET_VCPU_EVENTS ioctl)" ?

> If KVM advertises this capability, userspace can only specify the ISS field for
> the ESR syndrome. Other parts of the ESR, such as the EC are generated by the
> CPU when the exception is taken. If this virtual SError is taken to EL1 using
> AArch64, this value will be reported in the ISS field of ESR_ELx.
>
> See KVM_CAP_VCPU_EVENTS for more details.
> ------------------%<------------------
>
> and this patch adds to the GET/SET API section with:
> ------------------%<------------------
> SError exceptions always have an ESR value. Some CPUs have the ability to
> specify what the virtual SError's ESR value should be. These systems will
> advertise KVM_CAP_ARM_SET_SERROR_ESR. In this case exception.has_esr will
> always have a non-zero value when read, and the agent making an SError
> pending should specify the ISS field in the lower 24 bits of
> exception.serror_esr. If the system supports KVM_CAP_ARM_SET_SERROR_ESR, but
> user-space sets the events with exception.has_esr as zero, KVM will choose an ESR.
>
> Specifying exception.has_esr on a system that does not support it will return an
> error. Settings anything other than the lower 24bits of exception.serror_esr

"Setting"

> will return an error.

I think you should state which error (presumably EINVAL?)

> ------------------%<------------------
>
> because as Peter points out, this is where people will expect to find this.

thanks
-- PMM
diff mbox

Patch

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index e3940f8715a5..480fa64eb52a 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -4643,3 +4643,14 @@  This capability indicates that KVM supports paravirtualized Hyper-V TLB Flush
 hypercalls:
 HvFlushVirtualAddressSpace, HvFlushVirtualAddressSpaceEx,
 HvFlushVirtualAddressList, HvFlushVirtualAddressListEx.
+
+8.19 KVM_CAP_ARM_SET_SERROR_ESR
+
+Architectures: arm, arm64
+
+This capability indicates that userspace can specify the syndrome value reported
+to the guest OS when guest takes a virtual SError interrupt exception.
+If KVM has this capability, userspace can only specify the ISS field for the ESR
+syndrome, it can not specify the EC field which is not under control by KVM.
+If this virtual SError is taken to EL1 using AArch64, this value will be reported
+in ISS filed of ESR_EL1.
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index a3db01a28062..067c6ba969bd 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -77,6 +77,9 @@  int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_PMU_V3:
 		r = kvm_arm_support_pmu_v3();
 		break;
+	case KVM_CAP_ARM_INJECT_SERROR_ESR:
+		r = cpus_have_const_cap(ARM64_HAS_RAS_EXTN);
+		break;
 	case KVM_CAP_SET_GUEST_DEBUG:
 	case KVM_CAP_VCPU_ATTRIBUTES:
 	case KVM_CAP_VCPU_EVENTS:
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index b6270a3b38e9..a7d9bc4e4068 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -949,6 +949,7 @@  struct kvm_ppc_resize_hpt {
 #define KVM_CAP_GET_MSR_FEATURES 153
 #define KVM_CAP_HYPERV_EVENTFD 154
 #define KVM_CAP_HYPERV_TLBFLUSH 155
+#define KVM_CAP_ARM_INJECT_SERROR_ESR 156
 
 #ifdef KVM_CAP_IRQ_ROUTING