Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

From: Marc Zyngier <maz@kernel.org>
To: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
Cc: catalin.marinas@arm.com, will@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	scott@os.amperecomputing.com,
	Darren Hart <darren@os.amperecomputing.com>
Subject: Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues
Date: Wed, 11 Jan 2023 07:54:03 +0000	[thread overview]
Message-ID: <87mt6pecbo.wl-maz@kernel.org> (raw)
In-Reply-To: <87o7r6dpi8.wl-maz@kernel.org>

On Tue, 10 Jan 2023 21:54:39 +0000,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Tue, 10 Jan 2023 12:17:20 +0000,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> > 
> > I am currently working around this with "nohlt" kernel param to
> > NestedVM. Any suggestions to handle/fix this case/issue and avoid the
> > slowness of booting of NestedVM with more cores?
> > 
> > Note: Guest-Hypervisor and NestedVM are using default kernel installed
> > using Fedora 36 iso.
> 
> Despite what I said earlier, I have a vague idea here, thanks to the
> interesting call traces that you provided (this is really awesome work
> BTW, given how hard it is to trace things across 3 different kernels).
> 
> We can slightly limit the impact of the prepare/finish sequence if the
> guest hypervisor only accesses the active registers for SGIs/PPIs on
> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
> access.
> 
> Something along these lines, which is only boot-tested. Let me know
> how this fares for you.
> 
> Thanks,
> 
> 	M.
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
> index b32d434c1d4a..1cca45be5335 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
>   * active state can be overwritten when the VCPU's state is synced coming back
>   * from the guest.
>   *
> - * For shared interrupts as well as GICv3 private interrupts, we have to
> - * stop all the VCPUs because interrupts can be migrated while we don't hold
> - * the IRQ locks and we don't want to be chasing moving targets.
> + * For shared interrupts as well as GICv3 private interrupts accessed from the
> + * non-owning CPU, we have to stop all the VCPUs because interrupts can be
> + * migrated while we don't hold the IRQ locks and we don't want to be chasing
> + * moving targets.
>   *
>   * For GICv2 private interrupts we don't have to do anything because
>   * userspace accesses to the VGIC state already require all VCPUs to be
> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
>   */
>  static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>  {
> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
> +	     vcpu == kvm_get_running_vcpu()) ||

This should obviously be

+	     vcpu != kvm_get_running_vcpu()) ||

>  	    intid >= VGIC_NR_PRIVATE_IRQS)
>  		kvm_arm_halt_guest(vcpu->kvm);
>  }
> @@ -492,7 +494,8 @@ static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>  /* See vgic_access_active_prepare */
>  static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
>  {
> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
> +	     vcpu == kvm_get_running_vcpu()) ||

Same here.

>  	    intid >= VGIC_NR_PRIVATE_IRQS)
>  		kvm_arm_resume_guest(vcpu->kvm);
>  }

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel