[PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Pavel Fedin]

This patch set brings back functionality which was broken in v4.1. The
overall goal is to eventually enable using virtual timer too, but for now
changes affect only a possibility to run KVM itself. The guest currently
has to use another timer because there's no API to propagate interrupts
from in-kernel virtual timer to the interrupt controller emulated in
userspace.

v1=>v2:
- Do not use defensive approach in patch 0001. Use correct conditions in
  callers instead
- Added ARM64-specific code, without which attempt to run a VM ends in a
  HYP crash because of unset vGIC save/restore function pointers

Christoffer Dall: I decided to leave original approach of 0002 as it is,
because:
- It is much simpler to implement than doing changes inside both probe
  functions.
- These are all use cases for vgic_present flag. Nothing else is going to
  rely on it. IMHO this switch is very short and stylish :) If you don't
  like two error codes, we could change -ENXIO to -ENODEV where appropriate,
  this change would be small enough.
Actually i tried to discuss some things with you, but you stopped replying,
therefore i respin in order to revive the thing. On top of this i have
complete implementation of API which allows to emulate GIC in userspace by
qemu, and now i can run any virtual machine, including generic timer, on
vGIC-less machine. RasPI-2 is expected to benefit too.

Pavel Fedin (3):
  KVM: arm: Fix NULL pointer dereference if KVM is used without
    in-kernel irqchip
  KVM: arm: Detect vGIC presence at runtime
  KVM: arm64: Introduce default dummy save/restore functions

 arch/arm/include/asm/kvm_host.h   |  5 +++++
 arch/arm/kvm/arm.c                | 24 +++++++++++++++++++++---
 arch/arm64/include/asm/kvm_asm.h  |  1 +
 arch/arm64/include/asm/kvm_host.h |  8 ++++++++
 arch/arm64/kvm/vgic-v2-switch.S   |  5 +++++
 virt/kvm/arm/vgic.c               |  2 ++
 6 files changed, 42 insertions(+), 3 deletions(-)

-- 
2.4.4

[PATCH v2 1/3] KVM: arm: Fix NULL pointer dereference if KVM is used without in-kernel irqchip

[Pavel Fedin]

Makes qemu working again with kernel-irqchip=off option

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/kvm/arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index d9631ec..5668c4e 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -450,7 +450,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 	 * Map the VGIC hardware resources before running a vcpu the first
 	 * time on this VM.
 	 */
-	if (unlikely(!vgic_ready(kvm))) {
+	if (irqchip_in_kernel(kvm) && unlikely(!vgic_ready(kvm))) {
 		ret = kvm_vgic_map_resources(kvm);
 		if (ret)
 			return ret;
-- 
2.4.4

[PATCH v2 2/3] KVM: arm: Detect vGIC presence at runtime

[Pavel Fedin]

Allows to use KVM on hardware without vGIC. Interrupt controller has to be
emulated in userspace in this case.

-ENODEV return code from probe function means there's no GIC at all. -ENXIO
happens when, for example, there is GIC node in the device tree, but it does
not specify vGIC resources. Normally this means that vGIC hardware is defunct.
Any other code is still treated as full stop because it might mean some really
serious problems.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/kvm/arm.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 5668c4e..7e389fd 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -61,6 +61,8 @@ static atomic64_t kvm_vmid_gen = ATOMIC64_INIT(1);
 static u8 kvm_next_vmid;
 static DEFINE_SPINLOCK(kvm_vmid_lock);
 
+static bool vgic_present;
+
 static void kvm_arm_set_running_vcpu(struct kvm_vcpu *vcpu)
 {
 	BUG_ON(preemptible());
@@ -131,7 +133,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	kvm->arch.vmid_gen = 0;
 
 	/* The maximum number of VCPUs is limited by the host's GIC model */
-	kvm->arch.max_vcpus = kvm_vgic_get_max_vcpus();
+	kvm->arch.max_vcpus = vgic_present ?
+				kvm_vgic_get_max_vcpus() : KVM_MAX_VCPUS;
 
 	return ret;
 out_free_stage2_pgd:
@@ -172,6 +175,8 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	switch (ext) {
 	case KVM_CAP_IRQCHIP:
 	case KVM_CAP_IRQFD:
+		r = vgic_present;
+		break;
 	case KVM_CAP_IOEVENTFD:
 	case KVM_CAP_DEVICE_CTRL:
 	case KVM_CAP_USER_MEMORY:
@@ -850,6 +855,8 @@ static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,
 
 	switch (dev_id) {
 	case KVM_ARM_DEVICE_VGIC_V2:
+		if (!vgic_present)
+			return -ENXIO;
 		return kvm_vgic_addr(kvm, type, &dev_addr->addr, true);
 	default:
 		return -ENODEV;
@@ -864,6 +871,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 
 	switch (ioctl) {
 	case KVM_CREATE_IRQCHIP: {
+		if (!vgic_present)
+			return -ENXIO;
 		return kvm_vgic_create(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
 	}
 	case KVM_ARM_SET_DEVICE_ADDR: {
@@ -1046,8 +1055,17 @@ static int init_hyp_mode(void)
 	 * Init HYP view of VGIC
 	 */
 	err = kvm_vgic_hyp_init();
-	if (err)
+	switch (err) {
+	case 0:
+		vgic_present = true;
+		break;
+	case -ENODEV:
+	case -ENXIO:
+		vgic_present = false;
+		break;
+	default:
 		goto out_free_context;
+	}
 
 	/*
 	 * Init HYP architected timer support
-- 
2.4.4

[PATCH v2 3/3] KVM: arm64: Introduce default dummy save/restore functions

[Pavel Fedin]

Necessary for KVM without irqchip to function on ARM64. Allows to use
software GICv2 emulation where hardware acceleration is not available.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/include/asm/kvm_host.h   | 5 +++++
 arch/arm64/include/asm/kvm_asm.h  | 1 +
 arch/arm64/include/asm/kvm_host.h | 8 ++++++++
 arch/arm64/kvm/vgic-v2-switch.S   | 5 +++++
 virt/kvm/arm/vgic.c               | 2 ++
 5 files changed, 21 insertions(+)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index d71607c..c469c95 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -218,6 +218,11 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 	return 0;
 }
 
+static inline void vgic_arch_pre_setup(void)
+{
+
+}
+
 static inline void vgic_arch_setup(const struct vgic_params *vgic)
 {
 	BUG_ON(vgic->type != VGIC_V2);
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 4f7310f..933a442 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -132,6 +132,7 @@ extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
 
 extern u64 __vgic_v3_get_ich_vtr_el2(void);
 
+extern char __dummy_vgic_save_restore[];
 extern char __save_vgic_v2_state[];
 extern char __restore_vgic_v2_state[];
 extern char __save_vgic_v3_state[];
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f0f58c9..b322895 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -221,6 +221,14 @@ struct vgic_sr_vectors {
 	void	*restore_vgic;
 };
 
+static inline void vgic_arch_pre_setup(void)
+{
+	extern struct vgic_sr_vectors __vgic_sr_vectors;
+
+	__vgic_sr_vectors.save_vgic	= __dummy_vgic_save_restore;
+	__vgic_sr_vectors.restore_vgic	= __dummy_vgic_save_restore;
+}
+
 static inline void vgic_arch_setup(const struct vgic_params *vgic)
 {
 	extern struct vgic_sr_vectors __vgic_sr_vectors;
diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S
index f002fe1..f9280f9 100644
--- a/arch/arm64/kvm/vgic-v2-switch.S
+++ b/arch/arm64/kvm/vgic-v2-switch.S
@@ -134,4 +134,9 @@ CPU_BE(	rev	w5, w5 )
 	ret
 ENDPROC(__restore_vgic_v2_state)
 
+ENTRY(__dummy_vgic_save_restore)
+__dummy_vgic_save_restore:
+	ret
+ENDPROC(__dummy_vgic_save_restore)
+
 	.popsection
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 950064a..e3f9fae 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -2103,6 +2103,8 @@ int kvm_vgic_hyp_init(void)
 	struct device_node *vgic_node;
 	int ret;
 
+	vgic_arch_pre_setup();
+
 	vgic_node = of_find_matching_node_and_match(NULL,
 						    vgic_ids, &matched_id);
 	if (!vgic_node) {
-- 
2.4.4

Re: [PATCH v2 3/3] KVM: arm64: Introduce default dummy save/restore functions

[Marc Zyngier]

On 14/07/15 13:06, Pavel Fedin wrote:
> Necessary for KVM without irqchip to function on ARM64. Allows to use
> software GICv2 emulation where hardware acceleration is not available.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm/include/asm/kvm_host.h   | 5 +++++
>  arch/arm64/include/asm/kvm_asm.h  | 1 +
>  arch/arm64/include/asm/kvm_host.h | 8 ++++++++
>  arch/arm64/kvm/vgic-v2-switch.S   | 5 +++++
>  virt/kvm/arm/vgic.c               | 2 ++
>  5 files changed, 21 insertions(+)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index d71607c..c469c95 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -218,6 +218,11 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
>  	return 0;
>  }
>  
> +static inline void vgic_arch_pre_setup(void)
> +{
> +
> +}
> +
>  static inline void vgic_arch_setup(const struct vgic_params *vgic)
>  {
>  	BUG_ON(vgic->type != VGIC_V2);
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 4f7310f..933a442 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -132,6 +132,7 @@ extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
>  
>  extern u64 __vgic_v3_get_ich_vtr_el2(void);
>  
> +extern char __dummy_vgic_save_restore[];
>  extern char __save_vgic_v2_state[];
>  extern char __restore_vgic_v2_state[];
>  extern char __save_vgic_v3_state[];
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f0f58c9..b322895 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -221,6 +221,14 @@ struct vgic_sr_vectors {
>  	void	*restore_vgic;
>  };
>  
> +static inline void vgic_arch_pre_setup(void)
> +{
> +	extern struct vgic_sr_vectors __vgic_sr_vectors;
> +
> +	__vgic_sr_vectors.save_vgic	= __dummy_vgic_save_restore;
> +	__vgic_sr_vectors.restore_vgic	= __dummy_vgic_save_restore;
> +}
> +

You may want to check commit 8a14849, which makes this whole patch
irrelevant.

Also, we do check whether or not the GIC is enabled for a given VM, and
not touching the registers if they are not. What is wrong with that?

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v2 3/3] KVM: arm64: Introduce default dummy save/restore functions

[Pavel Fedin]

 Hello!

> You may want to check commit 8a14849, which makes this whole patch
> irrelevant.

 Thank you very much for pointing out, i will recheck.
 My patches are based on linux-stable.git, should i base them on something else?

> Also, we do check whether or not the GIC is enabled for a given VM, and
> not touching the registers if they are not. What is wrong with that?

 Look at arch/arm64/kvm/hyp.S, __kvm_vcpu_run function. It unconditionally calls entries in
__vgic_sr_vectors table. If vGIC initialization did not complete, this table stays unset and
contains zeroes. That was the problem.
 Ok, this is 64-bit-specific, i can fix this up later. My patch set can be used without 0003, in
this case it will fix only ARM32 architecture. So if you would like to accept the rest, you can do
it.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH v2 3/3] KVM: arm64: Introduce default dummy save/restore functions

[Pavel Fedin]

 Hello!

> You may want to check commit 8a14849, which makes this whole patch
> irrelevant.

 I have tested, the new code just works without patch No 0003. Simply throw it away from the set.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v2 3/3] KVM: arm64: Introduce default dummy save/restore functions

[Christoffer Dall]

On Tue, Jul 14, 2015 at 04:02:39PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > You may want to check commit 8a14849, which makes this whole patch
> > irrelevant.
> 
>  Thank you very much for pointing out, i will recheck.
>  My patches are based on linux-stable.git, should i base them on something else?
> 

A recent mainline -rc or kvm/next or kvmarm/next is generally preferred.

Basing upstream patches on stable is a bit weird.

-Christoffer

Re: [PATCH v2 1/3] KVM: arm: Fix NULL pointer dereference if KVM is used without in-kernel irqchip

[Marc Zyngier]

On 14/07/15 13:06, Pavel Fedin wrote:
> Makes qemu working again with kernel-irqchip=off option

I'd appreciate a better commit log. Which patch broke it, why is that
necessary.

> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm/kvm/arm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index d9631ec..5668c4e 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -450,7 +450,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  	 * Map the VGIC hardware resources before running a vcpu the first
>  	 * time on this VM.
>  	 */
> -	if (unlikely(!vgic_ready(kvm))) {
> +	if (irqchip_in_kernel(kvm) && unlikely(!vgic_ready(kvm))) {

I think you should factor the irqchip_in_kernel() inside the unlikely
clause. Something like

bool vgic_needs_init = irqchip_in_kernel(kvm) && !vgic_ready(kvm);

if (unlikely(vfic_needs_init)) {
...
}

>  		ret = kvm_vgic_map_resources(kvm);
>  		if (ret)
>  			return ret;
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Marc Zyngier]

On 14/07/15 13:06, Pavel Fedin wrote:
> This patch set brings back functionality which was broken in v4.1. The
> overall goal is to eventually enable using virtual timer too, but for now
> changes affect only a possibility to run KVM itself. The guest currently
> has to use another timer because there's no API to propagate interrupts
> from in-kernel virtual timer to the interrupt controller emulated in
> userspace.
> 
> v1=>v2:
> - Do not use defensive approach in patch 0001. Use correct conditions in
>   callers instead
> - Added ARM64-specific code, without which attempt to run a VM ends in a
>   HYP crash because of unset vGIC save/restore function pointers
> 
> Christoffer Dall: I decided to leave original approach of 0002 as it is,
> because:
> - It is much simpler to implement than doing changes inside both probe
>   functions.
> - These are all use cases for vgic_present flag. Nothing else is going to
>   rely on it. IMHO this switch is very short and stylish :) If you don't
>   like two error codes, we could change -ENXIO to -ENODEV where appropriate,
>   this change would be small enough.
> Actually i tried to discuss some things with you, but you stopped replying,
> therefore i respin in order to revive the thing. On top of this i have

I'm not Christoffer, but I'm going to take that one for the team.

Believe it or not, we're not only patch reviewing machines, and if you
count the number of pending patches, you'll quickly notice that yours
are basically noise in the grand scheme of things.

So please cut us some slack.

> complete implementation of API which allows to emulate GIC in userspace by
> qemu, and now i can run any virtual machine, including generic timer, on
> vGIC-less machine. RasPI-2 is expected to benefit too.

Do you mean feeding interrupts back to userspace? How is that going to
work with the active-timer series that really mandates a full blown GIC?
Your pet platform might cope with it, but I can't see that happening on
the RPi.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 2/3] KVM: arm: Detect vGIC presence at runtime

[Marc Zyngier]

On 14/07/15 13:06, Pavel Fedin wrote:
> Allows to use KVM on hardware without vGIC. Interrupt controller has to be
> emulated in userspace in this case.
> 
> -ENODEV return code from probe function means there's no GIC at all. -ENXIO
> happens when, for example, there is GIC node in the device tree, but it does
> not specify vGIC resources. Normally this means that vGIC hardware is defunct.
> Any other code is still treated as full stop because it might mean some really
> serious problems.

As mentioned in a previous email, supporting systems that don't even
have a GIC at all (hence don't support the management of an ACTIVE
state) is going to keep us in the dark ages. See the active timer series
that mandates it.

So keeping KVM alive when we get an -ENODEV is not acceptable. No GIC,
no fun.

> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm/kvm/arm.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 5668c4e..7e389fd 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -61,6 +61,8 @@ static atomic64_t kvm_vmid_gen = ATOMIC64_INIT(1);
>  static u8 kvm_next_vmid;
>  static DEFINE_SPINLOCK(kvm_vmid_lock);
>  
> +static bool vgic_present;
> +

This needs documentation.

>  static void kvm_arm_set_running_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	BUG_ON(preemptible());
> @@ -131,7 +133,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	kvm->arch.vmid_gen = 0;
>  
>  	/* The maximum number of VCPUs is limited by the host's GIC model */
> -	kvm->arch.max_vcpus = kvm_vgic_get_max_vcpus();
> +	kvm->arch.max_vcpus = vgic_present ?
> +				kvm_vgic_get_max_vcpus() : KVM_MAX_VCPUS;
>  
>  	return ret;
>  out_free_stage2_pgd:
> @@ -172,6 +175,8 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	switch (ext) {
>  	case KVM_CAP_IRQCHIP:
>  	case KVM_CAP_IRQFD:
> +		r = vgic_present;
> +		break;
>  	case KVM_CAP_IOEVENTFD:
>  	case KVM_CAP_DEVICE_CTRL:
>  	case KVM_CAP_USER_MEMORY:
> @@ -850,6 +855,8 @@ static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,
>  
>  	switch (dev_id) {
>  	case KVM_ARM_DEVICE_VGIC_V2:
> +		if (!vgic_present)
> +			return -ENXIO;
>  		return kvm_vgic_addr(kvm, type, &dev_addr->addr, true);
>  	default:
>  		return -ENODEV;
> @@ -864,6 +871,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  
>  	switch (ioctl) {
>  	case KVM_CREATE_IRQCHIP: {
> +		if (!vgic_present)
> +			return -ENXIO;
>  		return kvm_vgic_create(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
>  	}
>  	case KVM_ARM_SET_DEVICE_ADDR: {
> @@ -1046,8 +1055,17 @@ static int init_hyp_mode(void)
>  	 * Init HYP view of VGIC
>  	 */
>  	err = kvm_vgic_hyp_init();
> -	if (err)
> +	switch (err) {
> +	case 0:
> +		vgic_present = true;
> +		break;
> +	case -ENODEV:
> +	case -ENXIO:
> +		vgic_present = false;
> +		break;
> +	default:
>  		goto out_free_context;
> +	}
>  
>  	/*
>  	 * Init HYP architected timer support
> 


-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Pavel Fedin]

 Hello!

> Believe it or not, we're not only patch reviewing machines, and if you
> count the number of pending patches, you'll quickly notice that yours
> are basically noise in the grand scheme of things.
> 
> So please cut us some slack.

 What do you mean exactly? Stop discussions and just do everything you suggest before respin? Plz
don't attempt to read this between lines, i can be bad at communication; this is not a complain
and/or hurt indication, i am just asking. :)

> > complete implementation of API which allows to emulate GIC in userspace by
> > qemu, and now i can run any virtual machine, including generic timer, on
> > vGIC-less machine. RasPI-2 is expected to benefit too.
> 
> Do you mean feeding interrupts back to userspace?

 Yes, exactly. I have a working proof-of-concept here.

> How is that going to
> work with the active-timer series that really mandates a full blown GIC?
> Your pet platform might cope with it, but I can't see that happening on
> the RPi.

 I don't know, sorry, things are happening too fast and i can't follow everything.
 Well, actually it is possible to work without virtual timer at all. Yes, this means qemu has to
implement some memory-mapped timer. With HW models like vexpress this is already done and works
fine. I even tried to address this:
https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06627.html, but Peter suggested to complete
an API instead.
 I have done this as a proof-of-concept and case study for my project, so i want to upstream this in
order not to be lost.
 After all, recent specs say that generic timer can also be memory-mapped. So, making KVM working
without both vGIC and vTimer is not a real problem. This patchset addresses this scenario too, just
needs some extra bits on vtimer side (ability to init KVM without it).

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH v2 1/3] KVM: arm: Fix NULL pointer dereference if KVM is used without in-kernel irqchip

[Pavel Fedin]

 Hello!

> I'd appreciate a better commit log. Which patch broke it, why is that
> necessary.

 It was broken by 662d9715840aef44dcb573b0f9fab9e8319c868a.
 Actually, runtime detection was never implemented. The patch message just says that it can be done.
And in order to run KVM on my lame board i had to disable VGIC and VTIMER in kernel .config. The
actual breakage happened because without these definitions kvm_vgic_map_resources() was just a
do-nothing inline.

 vGIC can perfectly be emulated by userspace. And even generic timer can, with some tricks. So i
decided to restore this functionality.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Marc Zyngier]

On 19/07/15 15:05, Pavel Fedin wrote:
>  Hello!
> 
>> Believe it or not, we're not only patch reviewing machines, and if you
>> count the number of pending patches, you'll quickly notice that yours
>> are basically noise in the grand scheme of things.
>>
>> So please cut us some slack.
> 
> What do you mean exactly? Stop discussions and just do everything you
> suggest before respin? Plz don't attempt to read this between lines,
> i can be bad at communication; this is not a complain and/or hurt
> indication, i am just asking. :)

It means that a pause in the discussion for a week (or even more) is not
uncommon at all. It just means that the other party is busy with things
of higher priority. In all cases, a gentle "ping" will be better
received than this "you've stopped replying so I'm going to repost until
you stop ignoring me" kind of behaviour. It is unnecessary, irritating,
and overall counter-productive.

>>> complete implementation of API which allows to emulate GIC in userspace by
>>> qemu, and now i can run any virtual machine, including generic timer, on
>>> vGIC-less machine. RasPI-2 is expected to benefit too.
>>
>> Do you mean feeding interrupts back to userspace?
> 
>  Yes, exactly. I have a working proof-of-concept here.
> 
>> How is that going to
>> work with the active-timer series that really mandates a full blown GIC?
>> Your pet platform might cope with it, but I can't see that happening on
>> the RPi.
> 
> I don't know, sorry, things are happening too fast and i can't follow
> everything.

It uses the host GIC to perform a context switch of the ACTIVE state of
the timer interrupt instead of the ugly hack we have. Without a GIC, the
is no context switching, and the timer is useless.

> Well, actually it is possible to work without virtual timer at all.
> Yes, this means qemu has to implement some memory-mapped timer. With
> HW models like vexpress this is already done and works fine. I even
> tried to address this:
> https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06627.html,
> but Peter suggested to complete an API instead.
> I have done this as a proof-of-concept and case study for my project,
> so i want to upstream this in order not to be lost.

I don't care much for such an API, mostly because it will obviously
bitrot very quickly (your "broken VGIC" platform is hopefully a one off
that won't be repeated again, and the RPi is out of scope anyway).

It would have to be extremely non-intrusive and completely safe for this
to be taken in...

> After all, recent specs say that generic timer can also be
> memory-mapped. So, making KVM working without both vGIC and vTimer is
> not a real problem. This patchset addresses this scenario too, just 
> needs some extra bits on vtimer side (ability to init KVM without
> it).

Different generic timer. That's the memory-mapped one, which is not
per-CPU. Per-CPU timer is what we care about.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Pavel Fedin]

 Hello!

> It means that a pause in the discussion for a week (or even more) is not
> uncommon at all. It just means that the other party is busy with things
> of higher priority. In all cases, a gentle "ping" will be better
> received than this "you've stopped replying so I'm going to repost until
> you stop ignoring me" kind of behaviour.

 Ok, sorry for that.

> I don't care much for such an API, mostly because it will obviously
> bitrot very quickly (your "broken VGIC" platform is hopefully a one off
> that won't be repeated again, and the RPi is out of scope anyway).
> 
> It would have to be extremely non-intrusive and completely safe for this
> to be taken in...

 Ok, let's leave this API alone then for now...
 Will then be a concensus if i tweak the thing a little bit and we just enable KVM without both vGIC
and vTimer ? It will be an emulator's problem how to handle them then.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Marc Zyngier]

On 22/07/15 08:19, Pavel Fedin wrote:

>> I don't care much for such an API, mostly because it will obviously
>> bitrot very quickly (your "broken VGIC" platform is hopefully a one off
>> that won't be repeated again, and the RPi is out of scope anyway).
>>
>> It would have to be extremely non-intrusive and completely safe for this
>> to be taken in...
> 
>  Ok, let's leave this API alone then for now...
>  Will then be a concensus if i tweak the thing a little bit and we just enable KVM without both vGIC
> and vTimer ? It will be an emulator's problem how to handle them then.

Well, let's see the patches first, and how invasive they are.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Pavel Fedin]

> >  Ok, let's leave this API alone then for now...
> >  Will then be a concensus if i tweak the thing a little bit and we just enable KVM without both
vGIC
> > and vTimer ? It will be an emulator's problem how to handle them then.
> 
> Well, let's see the patches first, and how invasive they are.

 Please, i've just posted an RFC and cc'ed you.
 I would say the implementation is very small. However, perhaps, it's not going to work with active
state switching patch, because as far as i understand the whole thing depends on manipulations with
ARCH_TIMER_CTRL_IT_MASK. Without it i'm going to get multiple interrupts instead of only one. It
would be possible to signal the interrupt as real level-sensitive (since we will be tracking active
state), but... it's not possible to know when to deactivate the line, hardware doesn't report that.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Marc Zyngier]

On 24/07/15 16:27, Pavel Fedin wrote:
>>>  Ok, let's leave this API alone then for now...
>>>  Will then be a concensus if i tweak the thing a little bit and we just enable KVM without both
> vGIC
>>> and vTimer ? It will be an emulator's problem how to handle them then.
>>
>> Well, let's see the patches first, and how invasive they are.
> 
>  Please, i've just posted an RFC and cc'ed you.
>  I would say the implementation is very small. However, perhaps, it's not going to work with active
> state switching patch, because as far as i understand the whole thing depends on manipulations with
> ARCH_TIMER_CTRL_IT_MASK. Without it i'm going to get multiple interrupts instead of only one. It
> would be possible to signal the interrupt as real level-sensitive (since we will be tracking active
> state), but... it's not possible to know when to deactivate the line, hardware doesn't report that.

Then it is likely to be a lost cause. We need to get rid of the timer
masking crap, as it prevents us from running perfectly valid guests (QNX
being one). It also imposes the same hack on other guests (UEFI was
forced to implement the same horror).

As I said before, not having a VGIC pretty much puts you in an uncharted
territory, and the amount of hacks you have to introduce are not really
worth the effort in mainline.

It is rather sad, but I hope this serves as a lesson for whoever decides
to cut corner on HW implementation while thinking "SW will take care of
it". They usually shoot themselves in the foot.

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Pavel Fedin]

 Hello!

> As I said before, not having a VGIC pretty much puts you in an uncharted
> territory, and the amount of hacks you have to introduce are not really
> worth the effort in mainline.

 And i agreed. So, i repeat the question. Will it be OK if we just enable KVM without both vGIC and
vTimer, and that's all we do? It will be userland's problem how to handle them. I need to tweak my
first patchset a very small bit in order to do this.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Marc Zyngier]

On 26/07/15 13:50, Pavel Fedin wrote:
>  Hello!
> 
>> As I said before, not having a VGIC pretty much puts you in an uncharted
>> territory, and the amount of hacks you have to introduce are not really
>> worth the effort in mainline.
> 
> And i agreed. So, i repeat the question. Will it be OK if we just
> enable KVM without both vGIC and vTimer, and that's all we do? It
> will be userland's problem how to handle them. I need to tweak my 
> first patchset a very small bit in order to do this.

I I'll repeat what I already said: "Show me the patches".

I'll only give you an answer once I exactly know what you're up to.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Pavel Fedin]

 Hello!

> > And i agreed. So, i repeat the question. Will it be OK if we just
> > enable KVM without both vGIC and vTimer, and that's all we do? It
> > will be userland's problem how to handle them. I need to tweak my
> > first patchset a very small bit in order to do this.
> 
> I I'll repeat what I already said: "Show me the patches".
> 
> I'll only give you an answer once I exactly know what you're up to.

 You have already seen them, it is the very same patchset we are discussing in this thread.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v2 0/3] KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

[Marc Zyngier]

On 27/07/15 09:36, Pavel Fedin wrote:
>  Hello!
> 
>>> And i agreed. So, i repeat the question. Will it be OK if we just
>>> enable KVM without both vGIC and vTimer, and that's all we do? It
>>> will be userland's problem how to handle them. I need to tweak my
>>> first patchset a very small bit in order to do this.
>>
>> I I'll repeat what I already said: "Show me the patches".
>>
>> I'll only give you an answer once I exactly know what you're up to.
> 
>  You have already seen them, it is the very same patchset we are discussing in this thread.

And I've replied ten days ago. So far, I haven't seen a v3 that
addresses the comments I had.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...