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

[Cédric Le Goater]

On 4/11/19 6:38 AM, David Gibson wrote:
> 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.
yes.

>> You might want to reword that last paragraph to mention that.

ok. 

>>> 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.
> 

yes. 

The relevant part is at the end of the kvmppc_xive_connect_vcpu() routine :

  int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
  			     struct kvm_vcpu *vcpu, u32 cpu)
  {
	...
  	vcpu->arch.irq_type = KVMPPC_IRQ_XICS;
	return 0;
  }



David suggested a few cleanups that we could do in the xics-on-xive 
device. We might want to introduce a KVMPPC_IRQ_XICS_ON_XIVE flag also. 
First, I would like to get rid of references to the kvmppc_xive struct 
and remove some useless attributes to improve locking.

Once the XIVE native mode is merged, all kernels above 4.14 running on 
a P9 sPAPR guest will switch to XIVE and the xics-on-xive device will 
only be useful for nested.


C.