| Message ID | 1493530677-4919-1-git-send-email-gengdongjiu@huawei.com |
|---|---|
| State | New |
| Headers | show |
Hi Dongjiu, Please send a cover letter for patch series with more than a single patch. The subject and description of these patches are also misleading. Hopefully this is in no way tied to kvmtool, but to userspace generically, for example also to be used by QEMU? On Sun, Apr 30, 2017 at 01:37:55PM +0800, Dongjiu Geng wrote: > Handle kvmtool's detection for RAS extension, because sometimes > the APP needs to know the CPU's capacity the APP ? the CPU's capacity? > > Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> > --- > arch/arm64/kvm/reset.c | 11 +++++++++++ > include/uapi/linux/kvm.h | 1 + > 2 files changed, 12 insertions(+) > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c > index d9e9697..1004039 100644 > --- a/arch/arm64/kvm/reset.c > +++ b/arch/arm64/kvm/reset.c > @@ -64,6 +64,14 @@ static bool cpu_has_32bit_el1(void) > return !!(pfr0 & 0x20); > } > > +static bool kvm_arm_support_ras_extension(void) > +{ > + u64 pfr0; > + > + pfr0 = read_system_reg(SYS_ID_AA64PFR0_EL1); > + return !!(pfr0 & 0x10000000); > +} > + > /** > * kvm_arch_dev_ioctl_check_extension > * > @@ -87,6 +95,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_RAS_EXTENSION: > + r = kvm_arm_support_ras_extension(); > + break; You need to document this capability and API in Documentation/virtual/kvm/api.txt and explain how this works. > case KVM_CAP_SET_GUEST_DEBUG: > case KVM_CAP_VCPU_ATTRIBUTES: > r = 1; > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index f51d508..27fe556 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -883,6 +883,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_PPC_MMU_RADIX 134 > #define KVM_CAP_PPC_MMU_HASH_V3 135 > #define KVM_CAP_IMMEDIATE_EXIT 136 > +#define KVM_CAP_ARM_RAS_EXTENSION 137 > > #ifdef KVM_CAP_IRQ_ROUTING > > -- > 2.10.1 > Thanks, -Christoffer
Hi Christoffer, thanks for your review and comments On 2017/5/2 15:56, Christoffer Dall wrote: > Hi Dongjiu, > > Please send a cover letter for patch series with more than a single > patch. OK, got it. > > The subject and description of these patches are also misleading. > Hopefully this is in no way tied to kvmtool, but to userspace > generically, for example also to be used by QEMU? yes, it is also used by QEMU, it should be userspace. > > On Sun, Apr 30, 2017 at 01:37:55PM +0800, Dongjiu Geng wrote: >> Handle kvmtool's detection for RAS extension, because sometimes >> the APP needs to know the CPU's capacity > > the APP ? > > the CPU's capacity? I will fix it. > >> >> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> >> --- >> arch/arm64/kvm/reset.c | 11 +++++++++++ >> include/uapi/linux/kvm.h | 1 + >> 2 files changed, 12 insertions(+) >> >> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c >> index d9e9697..1004039 100644 >> --- a/arch/arm64/kvm/reset.c >> +++ b/arch/arm64/kvm/reset.c >> @@ -64,6 +64,14 @@ static bool cpu_has_32bit_el1(void) >> return !!(pfr0 & 0x20); >> } >> >> +static bool kvm_arm_support_ras_extension(void) >> +{ >> + u64 pfr0; >> + >> + pfr0 = read_system_reg(SYS_ID_AA64PFR0_EL1); >> + return !!(pfr0 & 0x10000000); >> +} >> + >> /** >> * kvm_arch_dev_ioctl_check_extension >> * >> @@ -87,6 +95,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_RAS_EXTENSION: >> + r = kvm_arm_support_ras_extension(); >> + break; > > You need to document this capability and API in > Documentation/virtual/kvm/api.txt and explain how this works. Ok, thanks for your suggestion. > > > >> case KVM_CAP_SET_GUEST_DEBUG: >> case KVM_CAP_VCPU_ATTRIBUTES: >> r = 1; >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index f51d508..27fe556 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -883,6 +883,7 @@ struct kvm_ppc_resize_hpt { >> #define KVM_CAP_PPC_MMU_RADIX 134 >> #define KVM_CAP_PPC_MMU_HASH_V3 135 >> #define KVM_CAP_IMMEDIATE_EXIT 136 >> +#define KVM_CAP_ARM_RAS_EXTENSION 137 >> >> #ifdef KVM_CAP_IRQ_ROUTING >> >> -- >> 2.10.1 >> > > Thanks, > -Christoffer > > . >
Hi Christoffer, thanks for your review and comments. On 2017/5/2 15:56, Christoffer Dall wrote: > Hi Dongjiu, > > Please send a cover letter for patch series with more than a single > patch. OK, got it. > > The subject and description of these patches are also misleading. > Hopefully this is in no way tied to kvmtool, but to userspace > generically, for example also to be used by QEMU? > > On Sun, Apr 30, 2017 at 01:37:55PM +0800, Dongjiu Geng wrote: >> Handle kvmtool's detection for RAS extension, because sometimes >> the APP needs to know the CPU's capacity > > the APP ? > > the CPU's capacity? I will fix it. > >> >> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> >> --- >> arch/arm64/kvm/reset.c | 11 +++++++++++ >> include/uapi/linux/kvm.h | 1 + >> 2 files changed, 12 insertions(+) >> >> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c >> index d9e9697..1004039 100644 >> --- a/arch/arm64/kvm/reset.c >> +++ b/arch/arm64/kvm/reset.c >> @@ -64,6 +64,14 @@ static bool cpu_has_32bit_el1(void) >> return !!(pfr0 & 0x20); >> } >> >> +static bool kvm_arm_support_ras_extension(void) >> +{ >> + u64 pfr0; >> + >> + pfr0 = read_system_reg(SYS_ID_AA64PFR0_EL1); >> + return !!(pfr0 & 0x10000000); >> +} >> + >> /** >> * kvm_arch_dev_ioctl_check_extension >> * >> @@ -87,6 +95,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_RAS_EXTENSION: >> + r = kvm_arm_support_ras_extension(); >> + break; > > You need to document this capability and API in > Documentation/virtual/kvm/api.txt and explain how this works. Ok, thanks for your suggestion. > > > >> case KVM_CAP_SET_GUEST_DEBUG: >> case KVM_CAP_VCPU_ATTRIBUTES: >> r = 1; >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index f51d508..27fe556 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -883,6 +883,7 @@ struct kvm_ppc_resize_hpt { >> #define KVM_CAP_PPC_MMU_RADIX 134 >> #define KVM_CAP_PPC_MMU_HASH_V3 135 >> #define KVM_CAP_IMMEDIATE_EXIT 136 >> +#define KVM_CAP_ARM_RAS_EXTENSION 137 >> >> #ifdef KVM_CAP_IRQ_ROUTING >> >> -- >> 2.10.1 >> > > Thanks, > -Christoffer > > . >
Hi Dongjiu Geng, On 30/04/17 06:37, Dongjiu Geng wrote: > Handle kvmtool's detection for RAS extension, because sometimes > the APP needs to know the CPU's capacity > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c > index d9e9697..1004039 100644 > --- a/arch/arm64/kvm/reset.c > +++ b/arch/arm64/kvm/reset.c > @@ -64,6 +64,14 @@ static bool cpu_has_32bit_el1(void) > return !!(pfr0 & 0x20); > } > > +static bool kvm_arm_support_ras_extension(void) > +{ > + u64 pfr0; > + > + pfr0 = read_system_reg(SYS_ID_AA64PFR0_EL1); > + return !!(pfr0 & 0x10000000); > +} Why are we telling user-space that the CPU has RAS extensions? EL0 can't do anything with this and the guest EL1 can detect it from the id registers. Are you using this to decide whether or not to generate a HEST for the guest? If Qemu/kvmtool supports handling memory-failure notifications from signals you should always generate a HEST. The GHES notification method could be anything Qemu can deliver to the guest using the KVM APIs. Notifications from Qemu to the guest don't depend on the RAS extensions. KVM has APIs for IRQ and SEA (you can use KVM_SET_ONE_REG). I think we need a new API for injecting SError for SEI from Qemu/kvmtool, but it shouldn't be related to the RAS extensions. All v8.0 CPUs have HCR_EL2.VSE, so we need to know KVM supports this API. Your later patch adds code to set VSESR to make virtual RAS SErrors work, I think we need to expose that to user-space. Thanks, James
On 02/05/2017 09:56, Christoffer Dall wrote: > Hi Dongjiu, > > Please send a cover letter for patch series with more than a single > patch. > > The subject and description of these patches are also misleading. > Hopefully this is in no way tied to kvmtool, but to userspace > generically, for example also to be used by QEMU? Yes, QEMU already has a similar capability on x86. Does ARM support background scrubbing of memory to detect errors? If so, are there any plans to support action-optional SIGBUS on ARM? Paolo > On Sun, Apr 30, 2017 at 01:37:55PM +0800, Dongjiu Geng wrote: >> Handle kvmtool's detection for RAS extension, because sometimes >> the APP needs to know the CPU's capacity > > the APP ? > > the CPU's capacity? > >> >> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> >> --- >> arch/arm64/kvm/reset.c | 11 +++++++++++ >> include/uapi/linux/kvm.h | 1 + >> 2 files changed, 12 insertions(+) >> >> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c >> index d9e9697..1004039 100644 >> --- a/arch/arm64/kvm/reset.c >> +++ b/arch/arm64/kvm/reset.c >> @@ -64,6 +64,14 @@ static bool cpu_has_32bit_el1(void) >> return !!(pfr0 & 0x20); >> } >> >> +static bool kvm_arm_support_ras_extension(void) >> +{ >> + u64 pfr0; >> + >> + pfr0 = read_system_reg(SYS_ID_AA64PFR0_EL1); >> + return !!(pfr0 & 0x10000000); >> +} >> + >> /** >> * kvm_arch_dev_ioctl_check_extension >> * >> @@ -87,6 +95,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_RAS_EXTENSION: >> + r = kvm_arm_support_ras_extension(); >> + break; > > You need to document this capability and API in > Documentation/virtual/kvm/api.txt and explain how this works. > > > >> case KVM_CAP_SET_GUEST_DEBUG: >> case KVM_CAP_VCPU_ATTRIBUTES: >> r = 1; >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index f51d508..27fe556 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -883,6 +883,7 @@ struct kvm_ppc_resize_hpt { >> #define KVM_CAP_PPC_MMU_RADIX 134 >> #define KVM_CAP_PPC_MMU_HASH_V3 135 >> #define KVM_CAP_IMMEDIATE_EXIT 136 >> +#define KVM_CAP_ARM_RAS_EXTENSION 137 >> >> #ifdef KVM_CAP_IRQ_ROUTING >> >> -- >> 2.10.1 >> > > Thanks, > -Christoffer >
Hi Paolo, On 02/05/17 16:48, Paolo Bonzini wrote: > On 02/05/2017 09:56, Christoffer Dall wrote: >> The subject and description of these patches are also misleading. >> Hopefully this is in no way tied to kvmtool, but to userspace >> generically, for example also to be used by QEMU? > > Yes, QEMU already has a similar capability on x86. > > Does ARM support background scrubbing of memory to detect errors? As part of RAS support, yes. A way for firmware to notify the OS about these events was recently added to the ACPI specification. We are aiming to turn on ARCH_SUPPORTS_MEMORY_FAILURE which does the Linux end of things. Punit has a series here: https://www.spinics.net/lists/arm-kernel/msg575944.html > If > so, are there any plans to support action-optional SIGBUS on ARM? It looks like ARCH_SUPPORTS_MEMORY_FAILURE will bring that in, so yes. Thanks, James
Hi Dongjiu Geng, On 02/05/17 16:29, James Morse wrote: > I think we need a new API for injecting SError for SEI from Qemu/kvmtool, but it > shouldn't be related to the RAS extensions. All v8.0 CPUs have HCR_EL2.VSE, so > we need to know KVM supports this API. Thinking about this some more, it is slightly more nuanced, KVM can always provide an API to inject SError, but it can only set the VSESR if the CPU has the RAS Extensions. Only offering the inject-SError API call if we can also set the VSESR looks a bit funny, but no-one has needed the no-ESR version so far. I still don't think we should let user-space make the 'RAS Extensions means VSESR' logical step. So my comments on this patch become: Don't read id registers directly, use cpus_have_cap() which handles features that differ across CPUs or were turned off at compile time. Please don't call this 'KVM_CAP_ARM_RAS_EXTENSION', if we are building an API to inject SError, call it that instead. Thanks, James
Hi James,
Thanks a lot for your comments.
2017-05-04 23:49 GMT+08:00 James Morse <james.morse@arm.com>:
> Hi Dongjiu Geng,
>
> On 02/05/17 16:29, James Morse wrote:
>> I think we need a new API for injecting SError for SEI from Qemu/kvmtool, but it
>> shouldn't be related to the RAS extensions. All v8.0 CPUs have HCR_EL2.VSE, so
>> we need to know KVM supports this API.
>
> Thinking about this some more, it is slightly more nuanced, KVM can always
> provide an API to inject SError, but it can only set the VSESR if the CPU has
> the RAS Extensions.
James, do you mean we need to add a new API instead of adding the
VSESR in the old API kvm_inject_vabt?
/**
* kvm_inject_vabt - inject an async abort / SError into the guest
* @vcpu: The VCPU to receive the exception
*
* It is assumed that this code is called from the VCPU thread and that the
* VCPU therefore is not currently executing guest code.
*/
void kvm_inject_vabt(struct kvm_vcpu *vcpu)
{
vcpu_set_hcr(vcpu, vcpu_get_hcr(vcpu) | HCR_VSE);
}
>
> Only offering the inject-SError API call if we can also set the VSESR looks a
> bit funny, but no-one has needed the no-ESR version so far.
>
> I still don't think we should let user-space make the 'RAS Extensions means
> VSESR' logical step.
Ok. got it.
>
> So my comments on this patch become:
> Don't read id registers directly, use cpus_have_cap() which handles features
> that differ across CPUs or were turned off at compile time.
>
> Please don't call this 'KVM_CAP_ARM_RAS_EXTENSION', if we are building an API to
> inject SError, call it that instead.
Ok, thanks for your suggestion.
>
>
> Thanks,
>
> James
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Hi James, I have changed the SEA/SEI injection method according you suggestion, but I think this patch may also be needed. Now for the SEI, the virtual ESR value is specified by the userspace. only RAS extension support to set the virtual ESR value. so user space will check it to decide whether pass the virtual ESR value. At the same time, reserve this interface for other possible usage by user space. what do you think about this patch? On 2017/5/2 23:29, James Morse wrote: > Hi Dongjiu Geng, > > On 30/04/17 06:37, Dongjiu Geng wrote: >> Handle kvmtool's detection for RAS extension, because sometimes >> the APP needs to know the CPU's capacity > >> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c >> index d9e9697..1004039 100644 >> --- a/arch/arm64/kvm/reset.c >> +++ b/arch/arm64/kvm/reset.c >> @@ -64,6 +64,14 @@ static bool cpu_has_32bit_el1(void) >> return !!(pfr0 & 0x20); >> } >> >> +static bool kvm_arm_support_ras_extension(void) >> +{ >> + u64 pfr0; >> + >> + pfr0 = read_system_reg(SYS_ID_AA64PFR0_EL1); >> + return !!(pfr0 & 0x10000000); >> +} > > Why are we telling user-space that the CPU has RAS extensions? EL0 can't do > anything with this and the guest EL1 can detect it from the id registers. > > > Are you using this to decide whether or not to generate a HEST for the guest? > > If Qemu/kvmtool supports handling memory-failure notifications from signals you > should always generate a HEST. The GHES notification method could be anything > Qemu can deliver to the guest using the KVM APIs. Notifications from Qemu to the > guest don't depend on the RAS extensions. KVM has APIs for IRQ and SEA (you can > use KVM_SET_ONE_REG). > > > I think we need a new API for injecting SError for SEI from Qemu/kvmtool, but it > shouldn't be related to the RAS extensions. All v8.0 CPUs have HCR_EL2.VSE, so > we need to know KVM supports this API. > > Your later patch adds code to set VSESR to make virtual RAS SErrors work, I > think we need to expose that to user-space. > > > Thanks, > > James > . >
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c index d9e9697..1004039 100644 --- a/arch/arm64/kvm/reset.c +++ b/arch/arm64/kvm/reset.c @@ -64,6 +64,14 @@ static bool cpu_has_32bit_el1(void) return !!(pfr0 & 0x20); } +static bool kvm_arm_support_ras_extension(void) +{ + u64 pfr0; + + pfr0 = read_system_reg(SYS_ID_AA64PFR0_EL1); + return !!(pfr0 & 0x10000000); +} + /** * kvm_arch_dev_ioctl_check_extension * @@ -87,6 +95,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_RAS_EXTENSION: + r = kvm_arm_support_ras_extension(); + break; case KVM_CAP_SET_GUEST_DEBUG: case KVM_CAP_VCPU_ATTRIBUTES: r = 1; diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index f51d508..27fe556 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -883,6 +883,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_PPC_MMU_RADIX 134 #define KVM_CAP_PPC_MMU_HASH_V3 135 #define KVM_CAP_IMMEDIATE_EXIT 136 +#define KVM_CAP_ARM_RAS_EXTENSION 137 #ifdef KVM_CAP_IRQ_ROUTING
Handle kvmtool's detection for RAS extension, because sometimes the APP needs to know the CPU's capacity Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> --- arch/arm64/kvm/reset.c | 11 +++++++++++ include/uapi/linux/kvm.h | 1 + 2 files changed, 12 insertions(+)