On Tue, May 08, 2018 at 12:57:56PM +0100, Marc Zyngier wrote:
> On 08/05/18 12:25, Dave Martin wrote:
> > On Tue, May 08, 2018 at 11:38:05AM +0100, Marc Zyngier wrote:
> >> On 04/05/18 17:05, Dave Martin wrote:
> >>> This patch adds SVE context saving to the hyp FPSIMD context switch
> >>> path. This means that it is no longer necessary to save the host
> >>> SVE state in advance of entering the guest, when in use.
> >>>
> >>> In order to avoid adding pointless complexity to the code, VHE is
> >>> assumed if SVE is in use. VHE is an architectural prerequisite for
> >>> SVE, so there is no good reason to turn CONFIG_ARM64_VHE off in
> >>> kernels that support both SVE and KVM.
> >>>
> >>> Historically, software models exist that can expose the
> >>> architecturally invalid configuration of SVE without VHE, so if
> >>> this situation is detected this patch warns and refuses to create a
> >>> VM. Doing this check at VM creation time avoids race issues
> >>> between KVM and SVE initialisation.
> >>>
> >>> Signed-off-by: Dave Martin <Dave.Martin@xxxxxxx>
> >>> Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxx>
> >>> ---
> >>> arch/arm64/Kconfig | 7 +++++++
> >>> arch/arm64/kvm/fpsimd.c | 1 -
> >>> arch/arm64/kvm/hyp/switch.c | 21 +++++++++++++++++++--
> >>> virt/kvm/arm/arm.c | 18 ++++++++++++++++++
> >>> 4 files changed, 44 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> >>> index eb2cf49..b0d3820 100644
> >>> --- a/arch/arm64/Kconfig
> >>> +++ b/arch/arm64/Kconfig
> >>> @@ -1130,6 +1130,7 @@ endmenu
> >>> config ARM64_SVE
> >>> bool "ARM Scalable Vector Extension support"
> >>> default y
> >>> + depends on !KVM || ARM64_VHE
> >>
> >> In that case, should we consider making ARM64_VHE "default y" as well,
> >> as KVM is "default y" too?
> >
> > Surely ARM64_VHE has always been default y?
> >
> > 1f364c8c48a0 ("arm64: VHE: Add support for running Linux in EL2 mode")
>
> Hmmm. I have no idea who this Zyngier guy from 2014 is. Oh well.
>
> >> Otherwise, I fear we end-up regressing existing configurations. Also,
> >> you still have to check for the configuration at run time, so I'm not
> >> immediately getting the point of this particular change.
> >
> > We check for the configuration, but the penalty is severe (i.e., can't
> > create VMs) and it doesn't appear to make sense to put effort into
> > working around that: the user has an easy fix in the form of setting
> > ARM64_VHE=y.
> >
> > Is there some value to supporting this configuration that I'm missing?
> > SVE and VHE are both default y.
>
> I had the (obviously wrong) idea that VHE didn't default to y.
Fair enough.
[...]
> >>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> >>> index 6cf499b..a7be7bf 100644
> >>> --- a/virt/kvm/arm/arm.c
> >>> +++ b/virt/kvm/arm/arm.c
> >>> @@ -16,6 +16,7 @@
> >>> * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> >>> */
> >>>
> >>> +#include <linux/bug.h>
> >>> #include <linux/cpu_pm.h>
> >>> #include <linux/errno.h>
> >>> #include <linux/err.h>
> >>> @@ -41,6 +42,7 @@
> >>> #include <asm/mman.h>
> >>> #include <asm/tlbflush.h>
> >>> #include <asm/cacheflush.h>
> >>> +#include <asm/cpufeature.h>
> >>> #include <asm/virt.h>
> >>> #include <asm/kvm_arm.h>
> >>> #include <asm/kvm_asm.h>
> >>> @@ -120,6 +122,22 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> >>> if (type)
> >>> return -EINVAL;
> >>>
> >>> + /*
> >>> + * VHE is a prerequisite for SVE in the Arm architecture, and
> >>> + * Kconfig ensures that if system_supports_sve() here then
> >>> + * CONFIG_ARM64_VHE is enabled, so if VHE support wasn't already
> >>> + * detected and enabled, the CPU is architecturally
> >>> + * noncompliant.
> >>> + *
> >>> + * Just in case this mismatch is seen, detect it, warn and give
> >>> + * up. Supporting this forbidden configuration in Hyp would be
> >>> + * pointless.
> >>> + */
> >>> + if (system_supports_sve() && !has_vhe()) {
> >>> + kvm_pr_unimpl("Cannot create VMs on SVE system without VHE. Broken cpu?");
> >>> + return -ENXIO;
> >>> + }
> >>
> >> You might as well fail the boot KVM initialization altogether, and not
> >> wait for a VM to be created.
> >
> > I was concerned that the SVE and KVM initialisation might happen in
> > an unpredictable order.
> >
> > KVM is initialised via module_init(), which I'm guessing is later than
> > cpufeatures (?) If so then yes, kvm_arch_init() would be a reasonable
> > place to do this.
>
> CPU features are set in stone very early, right after we've booted all
> the CPUs that can be discovered before running userspace. KVM itself
> relies on that, so you should be able to move that check pretty early.
OK, I'll have a go.
> >> But I'm more concerned with the fact that we're now have a configuration
> >> that drops functionalities on the floor, one way or another.
> >
> > Is this still a problem if that configuration is forbidden by Kconfig?
> >
> > Can you describe a scenario in which the problem config (KVM=y, VHE=n,
> > SVE=y) would be wanted?
>
> Not really. I guess it is just me being paranoid and not realising we
> had VHE on by default already.
OK, I'll leave it as-is for now, but give me a shout if you have further
concerns.
Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
