On Tue, Sep 06, 2016 at 02:18:10PM +0100, Vladimir Murzin wrote:
> On 05/09/16 12:29, Christoffer Dall wrote:
> > On Tue, Aug 16, 2016 at 11:46:58AM +0100, Vladimir Murzin wrote:
> >> It is time to get access to common version of vgic-v3.
> >
> > common version?
> >
>
> Since patch#2 it not private to arm64 or I should rephrase this?
>
I think you should just use the full path of the file or say something
like:
Now when vgic-v3-sr.c can be shared between arm and arm64, we have to
blah blah blah...
> >>
> >> We basically would need to tell build system how to pick it up and
> >> undo KVM_ARM_VGIC_V3 guarding introduced in 4f64cb6 ("arm/arm64: KVM:
> >> Only allow 64bit hosts to build VGICv3") and remove stubs. However,
> >> since vgic got ITS support KVM_ARM_VGIC_V3 tend to protect a little
> >> bit more than just plain vgic-v3 - this guard is used for ITS too
> >> which is not supported in 32-bit world yet. So, along with removal of
> >> KVM_ARM_VGIC_V3 guard introduce the new one - KVM_ARM_VGIC_V3_ITS to
> >> protect ITS related code.
> >
> > I don't find this paragraph particularly helpful, I'm afraid.
> >
>
> Sorry for that. It seems to much for one patch, I'll split it per your
> suggestion bellow.
>
> >>
> >> The only unpleasant part is how we decide which save/restore sequence to
> >> use under __vgic_save_state() - we don't have patching framework in hand
> >> like arm64, so have to check runtime on every invocation.
> >
> > Do static keys work on 32-bit arm?
> >
>
> I'll try to use them.
>
> >>
> >> Signed-off-by: Vladimir Murzin <vladimir.murzin@xxxxxxx>
> >> ---
> >> arch/arm/include/asm/kvm_host.h | 4 +++
> >> arch/arm/include/asm/kvm_hyp.h | 5 ++++
> >> arch/arm/kvm/Makefile | 2 ++
> >> arch/arm/kvm/hyp/Makefile | 1 +
> >> arch/arm/kvm/hyp/switch.c | 20 +++++++++++--
> >> arch/arm64/kvm/Kconfig | 4 +--
> >> include/kvm/arm_vgic.h | 8 ------
> >> virt/kvm/arm/vgic/vgic-kvm-device.c | 10 ++-----
> >> virt/kvm/arm/vgic/vgic-mmio-v3.c | 2 ++
> >> virt/kvm/arm/vgic/vgic-mmio.c | 2 --
> >> virt/kvm/arm/vgic/vgic-mmio.h | 3 --
> >> virt/kvm/arm/vgic/vgic.h | 54 ++---------------------------------
> >> 12 files changed, 38 insertions(+), 77 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> >> index de338d9..1312597 100644
> >> --- a/arch/arm/include/asm/kvm_host.h
> >> +++ b/arch/arm/include/asm/kvm_host.h
> >> @@ -39,7 +39,11 @@
> >>
> >> #include <kvm/arm_vgic.h>
> >>
> >> +#ifdef CONFIG_ARM_GIC_V3
> >> +#define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
> >> +#else
> >> #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
> >> +#endif
> >>
> >> #define KVM_REQ_VCPU_EXIT 8
> >>
> >> diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
> >> index e604ad68..95669b3 100644
> >> --- a/arch/arm/include/asm/kvm_hyp.h
> >> +++ b/arch/arm/include/asm/kvm_hyp.h
> >> @@ -88,6 +88,8 @@
> >>
> >> #define VFP_FPEXC __ACCESS_VFP(FPEXC)
> >>
> >> +#define ID_PFR1 __ACCESS_CP15(c0, 0, c1, 1)
> >> +
> >> /* AArch64 compatibility macros, only for the timer so far */
> >> #define read_sysreg_el0(r) read_sysreg(r##_el0)
> >> #define write_sysreg_el0(v, r) write_sysreg(v, r##_el0)
> >> @@ -103,6 +105,9 @@ void __timer_restore_state(struct kvm_vcpu *vcpu);
> >> void __vgic_v2_save_state(struct kvm_vcpu *vcpu);
> >> void __vgic_v2_restore_state(struct kvm_vcpu *vcpu);
> >>
> >> +void __vgic_v3_save_state(struct kvm_vcpu *vcpu);
> >> +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu);
> >> +
> >> void __sysreg_save_state(struct kvm_cpu_context *ctxt);
> >> void __sysreg_restore_state(struct kvm_cpu_context *ctxt);
> >>
> >> diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
> >> index 10d77a6..043d817f 100644
> >> --- a/arch/arm/kvm/Makefile
> >> +++ b/arch/arm/kvm/Makefile
> >> @@ -26,8 +26,10 @@ obj-y += $(KVM)/arm/vgic/vgic.o
> >> obj-y += $(KVM)/arm/vgic/vgic-init.o
> >> obj-y += $(KVM)/arm/vgic/vgic-irqfd.o
> >> obj-y += $(KVM)/arm/vgic/vgic-v2.o
> >> +obj-y += $(KVM)/arm/vgic/vgic-v3.o
> >> obj-y += $(KVM)/arm/vgic/vgic-mmio.o
> >> obj-y += $(KVM)/arm/vgic/vgic-mmio-v2.o
> >> +obj-y += $(KVM)/arm/vgic/vgic-mmio-v3.o
> >> obj-y += $(KVM)/arm/vgic/vgic-kvm-device.o
> >> obj-y += $(KVM)/irqchip.o
> >> obj-y += $(KVM)/arm/arch_timer.o
> >> diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
> >> index 8dfa5f7..3023bb5 100644
> >> --- a/arch/arm/kvm/hyp/Makefile
> >> +++ b/arch/arm/kvm/hyp/Makefile
> >> @@ -5,6 +5,7 @@
> >> KVM=../../../../virt/kvm
> >>
> >> obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v2-sr.o
> >> +obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v3-sr.o
> >> obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/timer-sr.o
> >>
> >> obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
> >> diff --git a/arch/arm/kvm/hyp/switch.c b/arch/arm/kvm/hyp/switch.c
> >> index b13caa9..9666bae 100644
> >> --- a/arch/arm/kvm/hyp/switch.c
> >> +++ b/arch/arm/kvm/hyp/switch.c
> >> @@ -15,6 +15,8 @@
> >> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> >> */
> >>
> >> +#include <linux/irqchip/arm-gic-v3.h>
> >> +
> >> #include <asm/kvm_asm.h>
> >> #include <asm/kvm_hyp.h>
> >>
> >> @@ -74,14 +76,28 @@ static void __hyp_text __deactivate_vm(struct kvm_vcpu *vcpu)
> >> write_sysreg(read_sysreg(MIDR), VPIDR);
> >> }
> >>
> >> +static bool __hyp_text __has_useable_gicv3_cpuif(void)
> >> +{
> >> + if (IS_ENABLED(CONFIG_ARM_GIC_V3) && (read_sysreg(ID_PFR1) >> 28))
> >
> > Do we have a define for bit 28 we could use?
> >
> > Does this actually work on all v7 boards? The v7 ARM ARM seems to state
> > that this bitfield is Reserved, UNK. Does that somehow mean 'is going
> > to be zero'?
> >
Any thoughts on this one?
> >> + return !!(read_sysreg(ICC_HSRE) & ICC_SRE_EL2_SRE);
> >> + else
> >> + return false;
> >> +}
> >> +
> >> static void __hyp_text __vgic_save_state(struct kvm_vcpu *vcpu)
> >> {
> >> - __vgic_v2_save_state(vcpu);
> >> + if (__has_useable_gicv3_cpuif())
> >> + __vgic_v3_save_state(vcpu);
> >> + else
> >> + __vgic_v2_save_state(vcpu);
> >> }
> >>
> >> static void __hyp_text __vgic_restore_state(struct kvm_vcpu *vcpu)
> >> {
> >> - __vgic_v2_restore_state(vcpu);
> >> + if (__has_useable_gicv3_cpuif())
> >> + __vgic_v3_restore_state(vcpu);
> >> + else
> >> + __vgic_v2_restore_state(vcpu);
> >> }
> >>
> >> static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu)
> >> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> >> index 9c9edc9..6eaf12c 100644
> >> --- a/arch/arm64/kvm/Kconfig
> >> +++ b/arch/arm64/kvm/Kconfig
> >> @@ -16,7 +16,7 @@ menuconfig VIRTUALIZATION
> >>
> >> if VIRTUALIZATION
> >>
> >> -config KVM_ARM_VGIC_V3
> >> +config KVM_ARM_VGIC_V3_ITS
> >
> > I feel like this could have been simplified with adding the new ITS
> > guard in a separate patch.
> >
> > In fact, you could then have a separate patch that enables compilation
> > of the gicv3 code and removes the static inlines etc., and then finally
> > a patch that adds in the logic for the world switch.
>
> Agree. Will rework this patch.
>
Thanks, sounds good!
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
