Message-ID: <Z0UVbpcYJIbsRxp2@google.com>
Date: Mon, 25 Nov 2024 16:25:18 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Maxim Levitsky <mlevitsk@...hat.com>
Cc: kvm@...r.kernel.org, Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org, 
	Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	Ingo Molnar <mingo@...hat.com>, "H. Peter Anvin" <hpa@...or.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Joerg Roedel <joro@...tes.org>, Suravee Suthikulpanit <suravee.suthikulpanit@....com>, 
	Robin Murphy <robin.murphy@....com>, iommu@...ts.linux.dev, 
	Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH v3 0/4] Allow AVIC's IPI virtualization to be optional

On Thu, Nov 21, 2024, Maxim Levitsky wrote:
> On Tue, 2024-10-22 at 12:00 -0700, Sean Christopherson wrote:
> > On Mon, Oct 21, 2024, Sean Christopherson wrote:
> > > On Wed, Oct 04, 2023, Maxim Levitsky wrote:
> > > > About the added 'vcpu->loaded' variable, I added it also because it is
> > > > something that is long overdue to be added, I remember that in IPIv code
> > > > there was also a need for this, and probalby more places in KVM can be
> > > > refactored to take advantage of it, instead of various hacks.
> > > 
> > > I don't view using the information from the Physical ID table as a hack.  It very
> > > explicitly uses the ir_list_lock to ensure that the pCPU that's programmed into
> > > the IRTE is the pCPU on which the vCPU is loaded, and provides rather strict
> > > ordering between task migration and device assignment.  It's not a super hot path,
> > > so I don't think lockless programming is justified.
> 
> If you strongly prefer this I won't argue. KVM does read back its SPTE entries,
> which is also something I can't say that I like that much.

Heh, ignoring the conundrum with SPTEs being writable by hardware for A/D assists,
not reading SPTEs would add an almost absurd amount of complexity due to the need
to manage mappings in a separate data structure.  E.g. see TDX's S-EPT implementation
for how messy things get.

> > > I also think we should keep IsRunning=1 when the vCPU is unloaded.  That approach
> > > won't run afoul of your concern with signaling the wrong pCPU, because KVM can
> > > still keep the ID up-to-date, e.g. if the task is migrated when a pCPU is being
> > > offlined.
> > > 
> > > The motiviation for keeping IsRunning=1 is to avoid unnecessary VM-Exits and GA
> > > log IRQs.  E.g. if a vCPU exits to userspace, there's zero reason to force IPI
> > > senders to exit, because KVM can't/won't notify userspace, and the pending virtual
> > > interrupt will be processed on the next VMRUN.
> > 
> > My only hesitation to keeping IsRunning=1 is that there could, in theory, be a
> > noisy neighbor problem.  E.g. if there is meaningful overhead when the CPU responds
> > to the doorbell. 
> 
> I once measured this by bombarding a regular CPU, which is not running any
> guests, with AVIC doorbells. It was like 60% reduction of its performance if
> I remember correctly.

Ah, right, I keep forgetting the Intel's posted interrupts limits the spam to a
single IRQ thanks to the PID.ON behavior, which is why it's ok-ish to keep posted
interrupts active when a vCPU is put.

> So physical id table entries of a VM can't point to a CPU which doesn't run
> the VM's vCPU thread, because only in this case this doesn't pose a DOS risk.
> 
> Same with IOMMU (malicious guest can in theory make an assigned device
> generate an interrupt storm, and then this storm can get redirected to a
> doorbell of a CPU which doesn't belong to a VM).

Powered by blists - more mailing lists