Re: [PATCH v5 16/16] KVM: PPC: Book3S HV: XIVE: introduce a 'release' device operation

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]

On Thu, Apr 11, 2019 at 01:16:25PM +1000, Paul Mackerras wrote:
> On Wed, Apr 10, 2019 at 07:04:48PM +0200, Cédric Le Goater wrote:
> > When a P9 sPAPR VM boots, the CAS negotiation process determines which
> > interrupt mode to use (XICS legacy or XIVE native) and invokes a
> > machine reset to activate the chosen mode.
> > 
> > To be able to switch from one mode to another, we introduce the
> > capability to release a KVM device without destroying the VM. The KVM
> > device interface is extended with a new 'release' operation which is
> > called when the file descriptor of the device is closed.
> 
> I believe the release operation is not called until all of the mmaps
> using the fd are unmapped - which is a good thing for us, since it
> means the guest can't possibly be accessing the XIVE directly.
> You might want to reword that last paragraph to mention that.
> 
> > Such operations are defined for the XICS-on-XIVE and the XIVE native
> > KVM devices. They clear the vCPU interrupt presenters that could be
> > attached and then destroy the device.
> > 
> > This is not considered as a safe operation as the vCPUs are still
> > running and could be referencing the KVM device through their
> > presenters. To protect the system from any breakage, the kvmppc_xive
> > objects representing both KVM devices are now stored in an array under
> > the VM. Allocation is performed on first usage and memory is freed
> > only when the VM exits.
> 
> One quick comment below:
> 
> > diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> > index 480a3fc6b9fd..064a9f2ae678 100644
> > --- a/arch/powerpc/kvm/book3s_xive.c
> > +++ b/arch/powerpc/kvm/book3s_xive.c
> > @@ -1100,11 +1100,19 @@ void kvmppc_xive_disable_vcpu_interrupts(struct kvm_vcpu *vcpu)
> >  void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
> >  {
> >  	struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> > -	struct kvmppc_xive *xive = xc->xive;
> > +	struct kvmppc_xive *xive;
> >  	int i;
> >  
> > +	if (!kvmppc_xics_enabled(vcpu))
> > +		return;
> 
> Should that be kvmppc_xive_enabled() rather than xics?

I think I asked that on an earlier iteration, and the answer is no.
The names are confusing, but this file is all about xics-on-xive
rather than xive native.  So here we're checking what's available from
the guest's point of view, so "xics", but most of the surrounding
functions are named "xive" because that's the backend.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]