| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0184EA26B2509940AA629AE1405DD7F201CAF42E@DGGEMA503-MBX.china.huawei.com>
Date: Fri, 1 Jun 2018 15:04:53 +0000
From: gengdongjiu <gengdongjiu@...wei.com>
To: Marc Zyngier <marc.zyngier@....com>,
"rkrcmar@...hat.com" <rkrcmar@...hat.com>,
"corbet@....net" <corbet@....net>,
"christoffer.dall@....com" <christoffer.dall@....com>,
"linux@...linux.org.uk" <linux@...linux.org.uk>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"will.deacon@....com" <will.deacon@....com>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"james.morse@....com" <james.morse@....com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v1 2/2] arm/arm64: KVM: Add KVM_GET/SET_VCPU_EVENTS
Hi Marc,
>
> On 31/05/18 14:08, Dongjiu Geng wrote:
> > For the migrating VMs, user space may need to know the exception
> > state. For example, in the machine A, KVM make an SError pending, when
> > migrate to B, KVM also needs to pend an SError.
> >
> > This new IOCTL exports user-invisible states related to SError.
> > Together with appropriate user space changes, user space can get/set
> > the SError exception state to do migrate/snapshot/suspend.
> >
> > Signed-off-by: Dongjiu Geng <gengdongjiu@...wei.com>
> > --
> > this series patch is separated from
> > https://www.spinics.net/lists/kvm/msg168917.html
> > change since V12:
> > 1. change (vcpu->arch.hcr_el2 & HCR_VSE) to !!(vcpu->arch.hcr_el2 &
> > HCR_VSE) in kvm_arm_vcpu_get_events()
> >
> > Change since V11:
> > Address James's comments, thanks James 1. Align the struct of
> > kvm_vcpu_events to 64 bytes 2. Avoid exposing the stale ESR value in
> > the kvm_arm_vcpu_get_events() 3. Change variables 'injected' name to
> > 'serror_pending' in the kvm_arm_vcpu_set_events() 4. Change to
> > sizeof(events) from sizeof(struct kvm_vcpu_events) in
> > kvm_arch_vcpu_ioctl()
> >
> > Change since V10:
> > Address James's comments, thanks James 1. Merge the helper function
> > with the user.
> > 2. Move the ISS_MASK into pend_guest_serror() to clear top bits 3.
> > Make kvm_vcpu_events struct align to 4 bytes 4. Add something check in
> > the kvm_arm_vcpu_set_events() 5. Check kvm_arm_vcpu_get/set_events()'s
> > return value.
> > 6. Initialise kvm_vcpu_events to 0 so that padding transferred to user-space doesn't
> > contain kernel stack.
> > ---
> > Documentation/virtual/kvm/api.txt | 31 ++++++++++++++++++++++++++++---
> > arch/arm/include/asm/kvm_host.h | 6 ++++++
> > arch/arm/kvm/guest.c | 12 ++++++++++++
> > arch/arm64/include/asm/kvm_emulate.h | 5 +++++
> > arch/arm64/include/asm/kvm_host.h | 7 +++++++
> > arch/arm64/include/uapi/asm/kvm.h | 13 +++++++++++++
> > arch/arm64/kvm/guest.c | 36 ++++++++++++++++++++++++++++++++++++
> > arch/arm64/kvm/inject_fault.c | 7 ++++++-
> > arch/arm64/kvm/reset.c | 1 +
> > virt/kvm/arm/arm.c | 21 +++++++++++++++++++++
> > 10 files changed, 135 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/virtual/kvm/api.txt
> > b/Documentation/virtual/kvm/api.txt
> > index fdac969..8896737 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -835,11 +835,13 @@ struct kvm_clock_data {
> >
> > Capability: KVM_CAP_VCPU_EVENTS
> > Extended by: KVM_CAP_INTR_SHADOW
> > -Architectures: x86
> > +Architectures: x86, arm, arm64
> > Type: vm ioctl
> > Parameters: struct kvm_vcpu_event (out)
> > Returns: 0 on success, -1 on error
> >
> > +X86:
> > +
> > Gets currently pending exceptions, interrupts, and NMIs as well as
> > related states of the vcpu.
> >
> > @@ -881,15 +883,32 @@ Only two fields are defined in the flags field:
> > - KVM_VCPUEVENT_VALID_SMM may be set in the flags field to signal that
> > smi contains a valid state.
> >
> > +ARM, ARM64:
> > +
> > +Gets currently pending SError exceptions as well as related states of the vcpu.
> > +
> > +struct kvm_vcpu_events {
> > + struct {
> > + __u8 serror_pending;
> > + __u8 serror_has_esr;
> > + /* Align it to 8 bytes */
> > + __u8 pad[6];
> > + __u64 serror_esr;
> > + } exception;
> > + __u32 reserved[12];
> > +};
> > +
> > 4.32 KVM_SET_VCPU_EVENTS
> >
> > -Capability: KVM_CAP_VCPU_EVENTS
> > +Capebility: KVM_CAP_VCPU_EVENTS
> > Extended by: KVM_CAP_INTR_SHADOW
> > -Architectures: x86
> > +Architectures: x86, arm, arm64
> > Type: vm ioctl
> > Parameters: struct kvm_vcpu_event (in)
> > Returns: 0 on success, -1 on error
> >
> > +X86:
> > +
> > Set pending exceptions, interrupts, and NMIs as well as related
> > states of the vcpu.
> >
> > @@ -910,6 +929,12 @@ shall be written into the VCPU.
> >
> > KVM_VCPUEVENT_VALID_SMM can only be set if KVM_CAP_X86_SMM is available.
> >
> > +ARM, ARM64:
> > +
> > +Set pending SError exceptions as well as related states of the vcpu.
> > +
> > +See KVM_GET_VCPU_EVENTS for the data structure.
> > +
> >
> > 4.33 KVM_GET_DEBUGREGS
> >
> > diff --git a/arch/arm/include/asm/kvm_host.h
> > b/arch/arm/include/asm/kvm_host.h index c7c28c8..39f9901 100644
> > --- a/arch/arm/include/asm/kvm_host.h
> > +++ b/arch/arm/include/asm/kvm_host.h
> > @@ -213,6 +213,12 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu
> > *vcpu); int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64
> > __user *indices); int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const
> > struct kvm_one_reg *reg); int kvm_arm_set_reg(struct kvm_vcpu *vcpu,
> > const struct kvm_one_reg *reg);
> > +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
> > + struct kvm_vcpu_events *events);
> > +
> > +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> > + struct kvm_vcpu_events *events);
> > +
> > unsigned long kvm_call_hyp(void *hypfn, ...); void
> > force_vm_exit(const cpumask_t *mask);
> >
> > diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c index
> > a18f33e..c685f0e 100644
> > --- a/arch/arm/kvm/guest.c
> > +++ b/arch/arm/kvm/guest.c
> > @@ -261,6 +261,18 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
> > return -EINVAL;
> > }
> >
> > +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
> > + struct kvm_vcpu_events *events)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> > + struct kvm_vcpu_events *events)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > int __attribute_const__ kvm_target_cpu(void) {
> > switch (read_cpuid_part()) {
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h
> > b/arch/arm64/include/asm/kvm_emulate.h
> > index 1dab3a9..18f61ff 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -81,6 +81,11 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
> > return (unsigned long *)&vcpu->arch.hcr_el2; }
> >
> > +static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu) {
> > + return vcpu->arch.vsesr_el2;
> > +}
> > +
> > static inline void vcpu_set_vsesr(struct kvm_vcpu *vcpu, u64 vsesr)
> > {
> > vcpu->arch.vsesr_el2 = vsesr;
> > diff --git a/arch/arm64/include/asm/kvm_host.h
> > b/arch/arm64/include/asm/kvm_host.h
> > index 469de8a..357304a 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -335,6 +335,11 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu
> > *vcpu); int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64
> > __user *indices); int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const
> > struct kvm_one_reg *reg); int kvm_arm_set_reg(struct kvm_vcpu *vcpu,
> > const struct kvm_one_reg *reg);
> > +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
> > + struct kvm_vcpu_events *events);
> > +
> > +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> > + struct kvm_vcpu_events *events);
> >
> > #define KVM_ARCH_WANT_MMU_NOTIFIER
> > int kvm_unmap_hva(struct kvm *kvm, unsigned long hva); @@ -363,6
> > +368,8 @@ void handle_exit_early(struct kvm_vcpu *vcpu, struct kvm_run
> > *run, int kvm_perf_init(void); int kvm_perf_teardown(void);
> >
> > +void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
> > +
> > struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long
> > mpidr);
> >
> > void __kvm_set_tpidr_el2(u64 tpidr_el2); diff --git
> > a/arch/arm64/include/uapi/asm/kvm.h
> > b/arch/arm64/include/uapi/asm/kvm.h
> > index 04b3256..df4faee 100644
> > --- a/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > @@ -39,6 +39,7 @@
> > #define __KVM_HAVE_GUEST_DEBUG
> > #define __KVM_HAVE_IRQ_LINE
> > #define __KVM_HAVE_READONLY_MEM
> > +#define __KVM_HAVE_VCPU_EVENTS
> >
> > #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
> >
> > @@ -153,6 +154,18 @@ struct kvm_sync_regs { struct
> > kvm_arch_memory_slot { };
> >
> > +/* for KVM_GET/SET_VCPU_EVENTS */
> > +struct kvm_vcpu_events {
> > + struct {
> > + __u8 serror_pending;
> > + __u8 serror_has_esr;
> > + /* Align it to 8 bytes */
> > + __u8 pad[6];
> > + __u64 serror_esr;
> > + } exception;
> > + __u32 reserved[12];
> > +};
> > +
> > /* If you need to interpret the index values, here is the key: */
> > #define KVM_REG_ARM_COPROC_MASK 0x000000000FFF0000
> > #define KVM_REG_ARM_COPROC_SHIFT 16
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index
> > 56a0260..71d3841 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -289,6 +289,42 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
> > return -EINVAL;
> > }
> >
> > +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
> > + struct kvm_vcpu_events *events)
> > +{
> > + events->exception.serror_pending = !!(vcpu->arch.hcr_el2 & HCR_VSE);
> > + events->exception.serror_has_esr =
> > + cpus_have_const_cap(ARM64_HAS_RAS_EXTN) &&
> > + (!!vcpu_get_vsesr(vcpu));
>
> This is odd. Isn't VSESR==0 a valid value? And isn't serror_has_esr always true when ARM64_HAS_RAS_EXTN is set?
An all-zero SError ESR now means: 'RAS error: Uncategorized' instead of 'no valid ISS'. yes, I think the VSESR can be 0. Thanks for the pointing out. So it is better to write as shown blow:
events->exception.serror_has_esr = cpus_have_const_cap(ARM64_HAS_RAS_EXTN);
> > +
> > + if (events->exception.serror_pending &&
> > + events->exception.serror_has_esr)
> > + events->exception.serror_esr = vcpu_get_vsesr(vcpu);
> > + else
> > + events->exception.serror_esr = 0;
> > +
> > + return 0;
> > +}
> > +
> > +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> > + struct kvm_vcpu_events *events)
> > +{
> > + bool serror_pending = events->exception.serror_pending;
> > + bool has_esr = events->exception.serror_has_esr;
> > +
> > + if (serror_pending && has_esr) {
> > + if (!cpus_have_const_cap(ARM64_HAS_RAS_EXTN))
> > + return -EINVAL;
> > +
> > + kvm_set_sei_esr(vcpu, events->exception.serror_esr);
> > +
>
> Spurious blank line
I will remove this blank line
>
> > + } else if (serror_pending) {
> > + kvm_inject_vabt(vcpu);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int __attribute_const__ kvm_target_cpu(void) {
> > unsigned long implementor = read_cpuid_implementor(); diff --git
> > a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c index
> > d8e7165..9e0ca56 100644
> > --- a/arch/arm64/kvm/inject_fault.c
> > +++ b/arch/arm64/kvm/inject_fault.c
> > @@ -166,7 +166,7 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu)
> >
> > static void pend_guest_serror(struct kvm_vcpu *vcpu, u64 esr) {
> > - vcpu_set_vsesr(vcpu, esr);
> > + vcpu_set_vsesr(vcpu, esr & ESR_ELx_ISS_MASK);
> > *vcpu_hcr(vcpu) |= HCR_VSE;
> > }
> >
> > @@ -186,3 +186,8 @@ void kvm_inject_vabt(struct kvm_vcpu *vcpu) {
> > pend_guest_serror(vcpu, ESR_ELx_ISV); }
> > +
> > +void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome) {
> > + pend_guest_serror(vcpu, syndrome);
> > +}
>
> I think it'd make more sense to rename pend_guest_serror to kvm_set_sei_esr and be done with it.
Ok, got it.
>
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c index
> > 38c8a64..20e919a 100644
> > --- a/arch/arm64/kvm/reset.c
> > +++ b/arch/arm64/kvm/reset.c
> > @@ -82,6 +82,7 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
> > break;
> > case KVM_CAP_SET_GUEST_DEBUG:
> > case KVM_CAP_VCPU_ATTRIBUTES:
> > + case KVM_CAP_VCPU_EVENTS:
> > r = 1;
> > break;
> > default:
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index
> > a4c1b76..8b43968 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -1107,6 +1107,27 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> > r = kvm_arm_vcpu_has_attr(vcpu, &attr);
> > break;
> > }
> > + case KVM_GET_VCPU_EVENTS: {
> > + struct kvm_vcpu_events events;
> > +
> > + memset(&events, 0, sizeof(events));
>
> You could write this as
>
> struct kvm_cpu_events events = { };
>
> but it'd make more sense if kvm_arm_vcpu_get_events() did all the work rather than having this split responsibility.
Ok, thanks for the good suggestion.
>
> > + if (kvm_arm_vcpu_get_events(vcpu, &events))
> > + return -EINVAL;
> > +
> > + if (copy_to_user(argp, &events, sizeof(events)))
> > + return -EFAULT;
> > +
> > + return 0;
> > + }
> > + case KVM_SET_VCPU_EVENTS: {
> > + struct kvm_vcpu_events events;
> > +
> > + if (copy_from_user(&events, argp,
> > + sizeof(struct kvm_vcpu_events)))
>
> Prefer using sizeof(events) instead.
Yes, it should, It is my careless. Thanks for the pointing out.
>
> > + return -EFAULT;
> > +
> > + return kvm_arm_vcpu_set_events(vcpu, &events);
> > + }
> > default:
> > r = -EINVAL;
> > }
> >
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists