linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/4] ARM64/KVM: Minor cleanup and refactoring
@ 2019-02-22  8:23 Leo Yan
  2019-02-22  8:23 ` [PATCH v1 1/4] KVM: arm64: Use macro to replace hard number Leo Yan
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Leo Yan @ 2019-02-22  8:23 UTC (permalink / raw)
  To: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
	Mark Rutland, James Morse, Andre Przywara, Jun Yao,
	Paolo Bonzini, linux-arm-kernel, kvmarm, linux-kernel
  Cc: Leo Yan

When I read KVM/ARM64 related code I cannot find any issue or something
could be improved for performance; so this patch series is only for
minor cleaning up and refactoring code.

Hope this is helpful and can be picked up into mainline kernel,
otherwise it's also fine for me if the maintainer or main developers
could note these points and fix them in appropriate time.


Leo Yan (4):
  KVM: arm64: Use macro to replace hard number
  KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq
  KVM: arm/arm64: Define TCR_EL2_T0SZ_MASK as TCR_T0SZ_MASK
  KVM: arm/arm64: Fix comment on create_hyp_mappings()

 arch/arm64/include/asm/kvm_arm.h | 2 +-
 arch/arm64/kernel/head.S         | 2 +-
 virt/kvm/arm/mmu.c               | 2 +-
 virt/kvm/arm/vgic/vgic.c         | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v1 1/4] KVM: arm64: Use macro to replace hard number
  2019-02-22  8:23 [PATCH v1 0/4] ARM64/KVM: Minor cleanup and refactoring Leo Yan
@ 2019-02-22  8:23 ` Leo Yan
  2019-02-22 11:40   ` Mark Rutland
  2019-02-22  8:23 ` [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq Leo Yan
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Leo Yan @ 2019-02-22  8:23 UTC (permalink / raw)
  To: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
	Mark Rutland, James Morse, Andre Przywara, Jun Yao,
	Paolo Bonzini, linux-arm-kernel, kvmarm, linux-kernel
  Cc: Leo Yan

Use macro for ID_AA64MMFR1_EL1.VH bits shift instead of 8 directly.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/kernel/head.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 4471f570a295..3ac377e9fd28 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -490,7 +490,7 @@ ENTRY(el2_setup)
 	 * kernel is intended to run at EL2.
 	 */
 	mrs	x2, id_aa64mmfr1_el1
-	ubfx	x2, x2, #8, #4
+	ubfx	x2, x2, #ID_AA64MMFR1_VHE_SHIFT, #4
 #else
 	mov	x2, xzr
 #endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq
  2019-02-22  8:23 [PATCH v1 0/4] ARM64/KVM: Minor cleanup and refactoring Leo Yan
  2019-02-22  8:23 ` [PATCH v1 1/4] KVM: arm64: Use macro to replace hard number Leo Yan
@ 2019-02-22  8:23 ` Leo Yan
  2019-02-22  8:37   ` Marc Zyngier
  2019-02-22  8:23 ` [PATCH v1 3/4] KVM: arm/arm64: Define TCR_EL2_T0SZ_MASK as TCR_T0SZ_MASK Leo Yan
  2019-02-22  8:23 ` [PATCH v1 4/4] KVM: arm/arm64: Fix comment on create_hyp_mappings() Leo Yan
  3 siblings, 1 reply; 13+ messages in thread
From: Leo Yan @ 2019-02-22  8:23 UTC (permalink / raw)
  To: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
	Mark Rutland, James Morse, Andre Przywara, Jun Yao,
	Paolo Bonzini, linux-arm-kernel, kvmarm, linux-kernel
  Cc: Leo Yan

The function kvm_vgic_inject_irq() is not only used by PPIs but also can
be used to inject interrupt for SPIs; this patch improves comment for
argument @cpuid to reflect support SPIs as well.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 virt/kvm/arm/vgic/vgic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 7cfdfbc910e0..79fe64c15051 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -394,7 +394,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
 /**
  * kvm_vgic_inject_irq - Inject an IRQ from a device to the vgic
  * @kvm:     The VM structure pointer
- * @cpuid:   The CPU for PPIs
+ * @cpuid:   The CPU for PPIs and SPIs
  * @intid:   The INTID to inject a new state to.
  * @level:   Edge-triggered:  true:  to trigger the interrupt
  *			      false: to ignore the call
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 3/4] KVM: arm/arm64: Define TCR_EL2_T0SZ_MASK as TCR_T0SZ_MASK
  2019-02-22  8:23 [PATCH v1 0/4] ARM64/KVM: Minor cleanup and refactoring Leo Yan
  2019-02-22  8:23 ` [PATCH v1 1/4] KVM: arm64: Use macro to replace hard number Leo Yan
  2019-02-22  8:23 ` [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq Leo Yan
@ 2019-02-22  8:23 ` Leo Yan
  2019-02-22 11:43   ` Mark Rutland
  2019-02-22  8:23 ` [PATCH v1 4/4] KVM: arm/arm64: Fix comment on create_hyp_mappings() Leo Yan
  3 siblings, 1 reply; 13+ messages in thread
From: Leo Yan @ 2019-02-22  8:23 UTC (permalink / raw)
  To: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
	Mark Rutland, James Morse, Andre Przywara, Jun Yao,
	Paolo Bonzini, linux-arm-kernel, kvmarm, linux-kernel
  Cc: Leo Yan

Define macro TCR_EL2_T0SZ_MASK as TCR_T0SZ_MASK, so can remove the hard
number 0x3f.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/include/asm/kvm_arm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 6f602af5263c..d945a787f36e 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -99,7 +99,7 @@
 #define TCR_EL2_SH0_MASK	TCR_SH0_MASK
 #define TCR_EL2_ORGN0_MASK	TCR_ORGN0_MASK
 #define TCR_EL2_IRGN0_MASK	TCR_IRGN0_MASK
-#define TCR_EL2_T0SZ_MASK	0x3f
+#define TCR_EL2_T0SZ_MASK	TCR_T0SZ_MASK
 #define TCR_EL2_MASK	(TCR_EL2_TG0_MASK | TCR_EL2_SH0_MASK | \
 			 TCR_EL2_ORGN0_MASK | TCR_EL2_IRGN0_MASK | TCR_EL2_T0SZ_MASK)
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 4/4] KVM: arm/arm64: Fix comment on create_hyp_mappings()
  2019-02-22  8:23 [PATCH v1 0/4] ARM64/KVM: Minor cleanup and refactoring Leo Yan
                   ` (2 preceding siblings ...)
  2019-02-22  8:23 ` [PATCH v1 3/4] KVM: arm/arm64: Define TCR_EL2_T0SZ_MASK as TCR_T0SZ_MASK Leo Yan
@ 2019-02-22  8:23 ` Leo Yan
  3 siblings, 0 replies; 13+ messages in thread
From: Leo Yan @ 2019-02-22  8:23 UTC (permalink / raw)
  To: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
	Mark Rutland, James Morse, Andre Przywara, Jun Yao,
	Paolo Bonzini, linux-arm-kernel, kvmarm, linux-kernel
  Cc: Leo Yan

Since commit f7bec68d2fae ("arm/arm64: KVM: Prune unused #defines"), the
macro HYP_PAGE_OFFSET has been removed, but it's kept in the comment for
the function create_hyp_mappings().

This patch uses PAGE_OFFSET to replace HYP_PAGE_OFFSET and this is
consistent with what is doing in the function.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 virt/kvm/arm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index 5eca48bdb1a6..9634f47c2c8b 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -734,7 +734,7 @@ static phys_addr_t kvm_kaddr_to_phys(void *kaddr)
  * @prot:	The protection to be applied to this range
  *
  * The same virtual address as the kernel virtual address is also used
- * in Hyp-mode mapping (modulo HYP_PAGE_OFFSET) to the same underlying
+ * in Hyp-mode mapping (modulo PAGE_OFFSET) to the same underlying
  * physical pages.
  */
 int create_hyp_mappings(void *from, void *to, pgprot_t prot)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq
  2019-02-22  8:23 ` [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq Leo Yan
@ 2019-02-22  8:37   ` Marc Zyngier
  2019-02-22  8:54     ` Leo Yan
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Zyngier @ 2019-02-22  8:37 UTC (permalink / raw)
  To: Leo Yan
  Cc: Christoffer Dall, Catalin Marinas, Will Deacon, Mark Rutland,
	James Morse, Andre Przywara, Jun Yao, Paolo Bonzini,
	linux-arm-kernel, kvmarm, linux-kernel

On Fri, 22 Feb 2019 16:23:24 +0800
Leo Yan <leo.yan@linaro.org> wrote:

> The function kvm_vgic_inject_irq() is not only used by PPIs but also can
> be used to inject interrupt for SPIs; this patch improves comment for
> argument @cpuid to reflect support SPIs as well.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  virt/kvm/arm/vgic/vgic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 7cfdfbc910e0..79fe64c15051 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -394,7 +394,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
>  /**
>   * kvm_vgic_inject_irq - Inject an IRQ from a device to the vgic
>   * @kvm:     The VM structure pointer
> - * @cpuid:   The CPU for PPIs
> + * @cpuid:   The CPU for PPIs and SPIs
>   * @intid:   The INTID to inject a new state to.
>   * @level:   Edge-triggered:  true:  to trigger the interrupt
>   *			      false: to ignore the call

What does the CPU mean for SPIs? By definition, the routing of an SPI
is defined by the distributor configuration. And what about LPIs? SGIs?

I'm afraid you've misunderstood what cpuid is for.

	M.
-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq
  2019-02-22  8:37   ` Marc Zyngier
@ 2019-02-22  8:54     ` Leo Yan
  2019-02-22  9:39       ` Marc Zyngier
  0 siblings, 1 reply; 13+ messages in thread
From: Leo Yan @ 2019-02-22  8:54 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Christoffer Dall, Catalin Marinas, Will Deacon, Mark Rutland,
	James Morse, Andre Przywara, Jun Yao, Paolo Bonzini,
	linux-arm-kernel, kvmarm, linux-kernel

Hi Marc,

On Fri, Feb 22, 2019 at 08:37:56AM +0000, Marc Zyngier wrote:
> On Fri, 22 Feb 2019 16:23:24 +0800
> Leo Yan <leo.yan@linaro.org> wrote:
> 
> > The function kvm_vgic_inject_irq() is not only used by PPIs but also can
> > be used to inject interrupt for SPIs; this patch improves comment for
> > argument @cpuid to reflect support SPIs as well.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  virt/kvm/arm/vgic/vgic.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index 7cfdfbc910e0..79fe64c15051 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -394,7 +394,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
> >  /**
> >   * kvm_vgic_inject_irq - Inject an IRQ from a device to the vgic
> >   * @kvm:     The VM structure pointer
> > - * @cpuid:   The CPU for PPIs
> > + * @cpuid:   The CPU for PPIs and SPIs
> >   * @intid:   The INTID to inject a new state to.
> >   * @level:   Edge-triggered:  true:  to trigger the interrupt
> >   *			      false: to ignore the call
> 
> What does the CPU mean for SPIs? By definition, the routing of an SPI
> is defined by the distributor configuration.

In the code, KVM injects PPIs by specifying CPU id, so that every PPI
is bound to specific target CPU.  But for SPIs, it always pass '0' for
cpuid, from my understanding this means VM will set interrupt affinity
to VCPU0 by default; in theory we also can set different cpuid for
SPIs so that the SPIs also can be handled by other secondary VCPUs;
this is why I think @cpuid also can be used by SPIs.

> And what about LPIs? SGIs?

TBH, I don't know LPIs and didn't use it before.

For SGIs, I read the code, it uses different path to handle SGI in KVM
so kvm_vgic_inject_irq() is not used for SGIs.

> I'm afraid you've misunderstood what cpuid is for.

Thanks for guidance.

Thanks,
Leo Yan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq
  2019-02-22  8:54     ` Leo Yan
@ 2019-02-22  9:39       ` Marc Zyngier
  2019-02-22 12:49         ` Leo Yan
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Zyngier @ 2019-02-22  9:39 UTC (permalink / raw)
  To: Leo Yan
  Cc: Christoffer Dall, Catalin Marinas, Will Deacon, Mark Rutland,
	James Morse, Andre Przywara, Jun Yao, Paolo Bonzini,
	linux-arm-kernel, kvmarm, linux-kernel

On Fri, 22 Feb 2019 16:54:39 +0800
Leo Yan <leo.yan@linaro.org> wrote:

> Hi Marc,
> 
> On Fri, Feb 22, 2019 at 08:37:56AM +0000, Marc Zyngier wrote:
> > On Fri, 22 Feb 2019 16:23:24 +0800
> > Leo Yan <leo.yan@linaro.org> wrote:
> >   
> > > The function kvm_vgic_inject_irq() is not only used by PPIs but also can
> > > be used to inject interrupt for SPIs; this patch improves comment for
> > > argument @cpuid to reflect support SPIs as well.
> > > 
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > >  virt/kvm/arm/vgic/vgic.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > > index 7cfdfbc910e0..79fe64c15051 100644
> > > --- a/virt/kvm/arm/vgic/vgic.c
> > > +++ b/virt/kvm/arm/vgic/vgic.c
> > > @@ -394,7 +394,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
> > >  /**
> > >   * kvm_vgic_inject_irq - Inject an IRQ from a device to the vgic
> > >   * @kvm:     The VM structure pointer
> > > - * @cpuid:   The CPU for PPIs
> > > + * @cpuid:   The CPU for PPIs and SPIs
> > >   * @intid:   The INTID to inject a new state to.
> > >   * @level:   Edge-triggered:  true:  to trigger the interrupt
> > >   *			      false: to ignore the call  
> > 
> > What does the CPU mean for SPIs? By definition, the routing of an SPI
> > is defined by the distributor configuration.  
> 
> In the code, KVM injects PPIs by specifying CPU id, so that every PPI
> is bound to specific target CPU.  But for SPIs, it always pass '0' for
> cpuid, from my understanding this means VM will set interrupt affinity
> to VCPU0 by default; in theory we also can set different cpuid for
> SPIs so that the SPIs also can be handled by other secondary VCPUs;
> this is why I think @cpuid also can be used by SPIs.

SPIs are not hardcoded to vcpu0. This would be a gross violation of the
architecture. To convince yourself of this, just run a guest:

root@unassigned-hostname:~# cat /proc/interrupts 
           CPU0       CPU1       
  2:       7315       7353     GIC-0  27 Level     arch_timer
  4:        158          0     GIC-0  33 Level     uart-pl011
 42:          0          0     GIC-0  23 Level     arm-pmu
 43:          0          0     pl061   3 Edge      ACPI:Event
 44:          0          0       MSI 32768 Edge      virtio1-config
 45:      10476          0       MSI 32769 Edge      virtio1-req.0
 46:          0          0       MSI 16384 Edge      virtio0-config
 47:          3         10       MSI 16385 Edge      virtio0-input.0
[...]

On this last line, you can see an SPI being routed to both of these
vcpus.

I urge you to read the code further, and understand that for any other
interrupt class, the cpuid parameter is *ignored*. Yes, we pass zero in
that case. We could also pass an approximation of PI with the same
effect. The interrupt affinity is either defined by the distributor
configuration (SPIs) or the ITS configuration (LPIs).

	M.
-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/4] KVM: arm64: Use macro to replace hard number
  2019-02-22  8:23 ` [PATCH v1 1/4] KVM: arm64: Use macro to replace hard number Leo Yan
@ 2019-02-22 11:40   ` Mark Rutland
  0 siblings, 0 replies; 13+ messages in thread
From: Mark Rutland @ 2019-02-22 11:40 UTC (permalink / raw)
  To: Leo Yan
  Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
	James Morse, Andre Przywara, Jun Yao, Paolo Bonzini,
	linux-arm-kernel, kvmarm, linux-kernel

On Fri, Feb 22, 2019 at 04:23:23PM +0800, Leo Yan wrote:
> Use macro for ID_AA64MMFR1_EL1.VH bits shift instead of 8 directly.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

It's always nice to get rid of magic numbers, and this is correct
AFAICT. FWIW:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/head.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 4471f570a295..3ac377e9fd28 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -490,7 +490,7 @@ ENTRY(el2_setup)
>  	 * kernel is intended to run at EL2.
>  	 */
>  	mrs	x2, id_aa64mmfr1_el1
> -	ubfx	x2, x2, #8, #4
> +	ubfx	x2, x2, #ID_AA64MMFR1_VHE_SHIFT, #4
>  #else
>  	mov	x2, xzr
>  #endif
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 3/4] KVM: arm/arm64: Define TCR_EL2_T0SZ_MASK as TCR_T0SZ_MASK
  2019-02-22  8:23 ` [PATCH v1 3/4] KVM: arm/arm64: Define TCR_EL2_T0SZ_MASK as TCR_T0SZ_MASK Leo Yan
@ 2019-02-22 11:43   ` Mark Rutland
  0 siblings, 0 replies; 13+ messages in thread
From: Mark Rutland @ 2019-02-22 11:43 UTC (permalink / raw)
  To: Leo Yan
  Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
	James Morse, Andre Przywara, Jun Yao, Paolo Bonzini,
	linux-arm-kernel, kvmarm, linux-kernel

On Fri, Feb 22, 2019 at 04:23:25PM +0800, Leo Yan wrote:
> Define macro TCR_EL2_T0SZ_MASK as TCR_T0SZ_MASK, so can remove the hard
> number 0x3f.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Given we do this for the other TCR fields, and the value is correct
AFAICT, this makes sense to me. FWIW:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/include/asm/kvm_arm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 6f602af5263c..d945a787f36e 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -99,7 +99,7 @@
>  #define TCR_EL2_SH0_MASK	TCR_SH0_MASK
>  #define TCR_EL2_ORGN0_MASK	TCR_ORGN0_MASK
>  #define TCR_EL2_IRGN0_MASK	TCR_IRGN0_MASK
> -#define TCR_EL2_T0SZ_MASK	0x3f
> +#define TCR_EL2_T0SZ_MASK	TCR_T0SZ_MASK
>  #define TCR_EL2_MASK	(TCR_EL2_TG0_MASK | TCR_EL2_SH0_MASK | \
>  			 TCR_EL2_ORGN0_MASK | TCR_EL2_IRGN0_MASK | TCR_EL2_T0SZ_MASK)
>  
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq
  2019-02-22  9:39       ` Marc Zyngier
@ 2019-02-22 12:49         ` Leo Yan
  2019-02-22 15:40           ` Marc Zyngier
  0 siblings, 1 reply; 13+ messages in thread
From: Leo Yan @ 2019-02-22 12:49 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Christoffer Dall, Catalin Marinas, Will Deacon, Mark Rutland,
	James Morse, Andre Przywara, Jun Yao, Paolo Bonzini,
	linux-arm-kernel, kvmarm, linux-kernel

On Fri, Feb 22, 2019 at 09:39:23AM +0000, Marc Zyngier wrote:

[...]

> > > > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > > > index 7cfdfbc910e0..79fe64c15051 100644
> > > > --- a/virt/kvm/arm/vgic/vgic.c
> > > > +++ b/virt/kvm/arm/vgic/vgic.c
> > > > @@ -394,7 +394,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
> > > >  /**
> > > >   * kvm_vgic_inject_irq - Inject an IRQ from a device to the vgic
> > > >   * @kvm:     The VM structure pointer
> > > > - * @cpuid:   The CPU for PPIs
> > > > + * @cpuid:   The CPU for PPIs and SPIs
> > > >   * @intid:   The INTID to inject a new state to.
> > > >   * @level:   Edge-triggered:  true:  to trigger the interrupt
> > > >   *			      false: to ignore the call  
> > > 
> > > What does the CPU mean for SPIs? By definition, the routing of an SPI
> > > is defined by the distributor configuration.  
> > 
> > In the code, KVM injects PPIs by specifying CPU id, so that every PPI
> > is bound to specific target CPU.  But for SPIs, it always pass '0' for
> > cpuid, from my understanding this means VM will set interrupt affinity
> > to VCPU0 by default; in theory we also can set different cpuid for
> > SPIs so that the SPIs also can be handled by other secondary VCPUs;
> > this is why I think @cpuid also can be used by SPIs.
> 
> SPIs are not hardcoded to vcpu0. This would be a gross violation of the
> architecture. To convince yourself of this, just run a guest:
> 
> root@unassigned-hostname:~# cat /proc/interrupts 
>            CPU0       CPU1       
>   2:       7315       7353     GIC-0  27 Level     arch_timer
>   4:        158          0     GIC-0  33 Level     uart-pl011
>  42:          0          0     GIC-0  23 Level     arm-pmu
>  43:          0          0     pl061   3 Edge      ACPI:Event
>  44:          0          0       MSI 32768 Edge      virtio1-config
>  45:      10476          0       MSI 32769 Edge      virtio1-req.0
>  46:          0          0       MSI 16384 Edge      virtio0-config
>  47:          3         10       MSI 16385 Edge      virtio0-input.0
> [...]
> 
> On this last line, you can see an SPI being routed to both of these
> vcpus.
> 
> I urge you to read the code further, and understand that for any other
> interrupt class, the cpuid parameter is *ignored*. Yes, we pass zero in
> that case. We could also pass an approximation of PI with the same
> effect.

Very appreciate for the elaborated example; will read the code
furthermore.

> The interrupt affinity is either defined by the distributor
> configuration (SPIs) or the ITS configuration (LPIs).

Given to the up example, I am struggling to understand how you can set
the interrupt affinity for virtio device.

Do you set the physical interrupt affinity to CPU0/1 in host OS and
forward it to guest OS?  Or set interrupt affinity in guest OS (I
tried on Juno board to set irq affinity in guest OS from
'/proc/irq/xxx/smp_affinity' but failed)?   Or this is accomplished by
user space tool (lkvm or qemu)?

Sorry if I am asking a stupid question :)

Thanks,
Leo Yan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq
  2019-02-22 12:49         ` Leo Yan
@ 2019-02-22 15:40           ` Marc Zyngier
  2019-02-25  0:09             ` Leo Yan
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Zyngier @ 2019-02-22 15:40 UTC (permalink / raw)
  To: Leo Yan
  Cc: Christoffer Dall, Catalin Marinas, Will Deacon, Mark Rutland,
	James Morse, Andre Przywara, Jun Yao, Paolo Bonzini,
	linux-arm-kernel, kvmarm, linux-kernel

On Fri, 22 Feb 2019 12:49:06 +0000,
Leo Yan <leo.yan@linaro.org> wrote:
> 
> On Fri, Feb 22, 2019 at 09:39:23AM +0000, Marc Zyngier wrote:
> 
> [...]
> 
> > > > > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > > > > index 7cfdfbc910e0..79fe64c15051 100644
> > > > > --- a/virt/kvm/arm/vgic/vgic.c
> > > > > +++ b/virt/kvm/arm/vgic/vgic.c
> > > > > @@ -394,7 +394,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
> > > > >  /**
> > > > >   * kvm_vgic_inject_irq - Inject an IRQ from a device to the vgic
> > > > >   * @kvm:     The VM structure pointer
> > > > > - * @cpuid:   The CPU for PPIs
> > > > > + * @cpuid:   The CPU for PPIs and SPIs
> > > > >   * @intid:   The INTID to inject a new state to.
> > > > >   * @level:   Edge-triggered:  true:  to trigger the interrupt
> > > > >   *			      false: to ignore the call  
> > > > 
> > > > What does the CPU mean for SPIs? By definition, the routing of an SPI
> > > > is defined by the distributor configuration.  
> > > 
> > > In the code, KVM injects PPIs by specifying CPU id, so that every PPI
> > > is bound to specific target CPU.  But for SPIs, it always pass '0' for
> > > cpuid, from my understanding this means VM will set interrupt affinity
> > > to VCPU0 by default; in theory we also can set different cpuid for
> > > SPIs so that the SPIs also can be handled by other secondary VCPUs;
> > > this is why I think @cpuid also can be used by SPIs.
> > 
> > SPIs are not hardcoded to vcpu0. This would be a gross violation of the
> > architecture. To convince yourself of this, just run a guest:
> > 
> > root@unassigned-hostname:~# cat /proc/interrupts 
> >            CPU0       CPU1       
> >   2:       7315       7353     GIC-0  27 Level     arch_timer
> >   4:        158          0     GIC-0  33 Level     uart-pl011
> >  42:          0          0     GIC-0  23 Level     arm-pmu
> >  43:          0          0     pl061   3 Edge      ACPI:Event
> >  44:          0          0       MSI 32768 Edge      virtio1-config
> >  45:      10476          0       MSI 32769 Edge      virtio1-req.0
> >  46:          0          0       MSI 16384 Edge      virtio0-config
> >  47:          3         10       MSI 16385 Edge      virtio0-input.0
> > [...]
> > 
> > On this last line, you can see an SPI being routed to both of these
> > vcpus.
> > 
> > I urge you to read the code further, and understand that for any other
> > interrupt class, the cpuid parameter is *ignored*. Yes, we pass zero in
> > that case. We could also pass an approximation of PI with the same
> > effect.
> 
> Very appreciate for the elaborated example; will read the code
> furthermore.
> 
> > The interrupt affinity is either defined by the distributor
> > configuration (SPIs) or the ITS configuration (LPIs).
> 
> Given to the up example, I am struggling to understand how you can set
> the interrupt affinity for virtio device.
> 
> Do you set the physical interrupt affinity to CPU0/1 in host OS and
> forward it to guest OS?  Or set interrupt affinity in guest OS (I
> tried on Juno board to set irq affinity in guest OS from
> '/proc/irq/xxx/smp_affinity' but failed)?   Or this is accomplished by
> user space tool (lkvm or qemu)?

virtio interrupts are purely virtual, and the host plays no part in
their routing (nor does userspace). As for their affinity, that
depends on the virtio driver. Some virtio devices allow their affinity
to be changed, some don't. Here, this is a virtio-net device, which is
perfectly happy to see its queue interrupts moved to a different vcpu.

I tend to run irqbalance in my guests so that it actually exercises
the affinity setting in the background.

> Sorry if I am asking a stupid question :)

It's not stupid. You're simply confusing multiple independent layers.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq
  2019-02-22 15:40           ` Marc Zyngier
@ 2019-02-25  0:09             ` Leo Yan
  0 siblings, 0 replies; 13+ messages in thread
From: Leo Yan @ 2019-02-25  0:09 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Christoffer Dall, Catalin Marinas, Will Deacon, Mark Rutland,
	James Morse, Andre Przywara, Jun Yao, Paolo Bonzini,
	linux-arm-kernel, kvmarm, linux-kernel

On Fri, Feb 22, 2019 at 03:40:50PM +0000, Marc Zyngier wrote:

[...]

> > > The interrupt affinity is either defined by the distributor
> > > configuration (SPIs) or the ITS configuration (LPIs).
> > 
> > Given to the up example, I am struggling to understand how you can set
> > the interrupt affinity for virtio device.
> > 
> > Do you set the physical interrupt affinity to CPU0/1 in host OS and
> > forward it to guest OS?  Or set interrupt affinity in guest OS (I
> > tried on Juno board to set irq affinity in guest OS from
> > '/proc/irq/xxx/smp_affinity' but failed)?   Or this is accomplished by
> > user space tool (lkvm or qemu)?
> 
> virtio interrupts are purely virtual, and the host plays no part in
> their routing (nor does userspace). As for their affinity, that
> depends on the virtio driver. Some virtio devices allow their affinity
> to be changed, some don't. Here, this is a virtio-net device, which is
> perfectly happy to see its queue interrupts moved to a different vcpu.
> 
> I tend to run irqbalance in my guests so that it actually exercises
> the affinity setting in the background.
> 
> > Sorry if I am asking a stupid question :)
> 
> It's not stupid. You're simply confusing multiple independent layers.

Thanks a lot for explaination, Marc.

Thanks,
Leo Yan

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-02-25  0:19 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-22  8:23 [PATCH v1 0/4] ARM64/KVM: Minor cleanup and refactoring Leo Yan
2019-02-22  8:23 ` [PATCH v1 1/4] KVM: arm64: Use macro to replace hard number Leo Yan
2019-02-22 11:40   ` Mark Rutland
2019-02-22  8:23 ` [PATCH v1 2/4] KVM: arm/arm64: vgic: Improve comment on kvm_vgic_inject_irq Leo Yan
2019-02-22  8:37   ` Marc Zyngier
2019-02-22  8:54     ` Leo Yan
2019-02-22  9:39       ` Marc Zyngier
2019-02-22 12:49         ` Leo Yan
2019-02-22 15:40           ` Marc Zyngier
2019-02-25  0:09             ` Leo Yan
2019-02-22  8:23 ` [PATCH v1 3/4] KVM: arm/arm64: Define TCR_EL2_T0SZ_MASK as TCR_T0SZ_MASK Leo Yan
2019-02-22 11:43   ` Mark Rutland
2019-02-22  8:23 ` [PATCH v1 4/4] KVM: arm/arm64: Fix comment on create_hyp_mappings() Leo Yan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).