On 11/11/2019 10:49, Greg Kurz wrote:
The EQ page is allocated by the guest and then passed to the hypervisor with the H_INT_SET_QUEUE_CONFIG hcall. A reference is taken on the page before handing it over to the HW. This reference is dropped either when the guest issues the H_INT_RESET hcall or when the KVM device is released. But, the guest can legitimately call H_INT_SET_QUEUE_CONFIG several times to reset the EQ (vCPU hot unplug) or set a new EQ (guest reboot). In both cases the EQ page reference is leaked. This is especially visible when the guest memory is backed with huge pages: start a VM up to the guest userspace, either reboot it or unplug a vCPU, quit QEMU. The leak is observed by comparing the value of HugePages_Free in /proc/meminfo before and after the VM is run.
Note that the EQ reset path seems to be calling put_page() but this is done after xive_native_configure_queue() which clears the qpage field in the XIVE queue structure, ie. the put_page() block is a nop and the previous page pointer was just overwritten anyway. In the other case of configuring a new EQ page, nothing seems to be done to release the old one.
Yes. Nice catch. I think we should try to fix the problem differently.
The routine xive_native_configure_queue() is only suited for XIVE drivers doing their own EQ page allocation: Linux PowerNV and the KVM XICS-over-XIVE device. The KVM XIVE device acts as a proxy for the guest OS doing the allocation and it has different needs.
Having a specific xive_native_configure_queue() for the KVM XIVE device seems overkill. May be, we could introduce a helper routine in KVM XIVE device calling xive_native_configure_queue() and handling the page reference how it should be ? That is to drop the previous page reference in case of a change on q->qpage.
Also, we should try to preserve the previous setting until the whole configuration is in place. That seems possible up to the call to xive_native_configure_queue(). If kvmppc_xive_attach_escalation() fails I think it is too late, as the HW has been configured by xive_native_configure_queue(), and we should just cleanup everything.
Thanks,
C.
Fix both cases by always calling put_page() on the existing EQ page in kvmppc_xive_native_set_queue_config(). This is a seemless change for the EQ reset case. However this causes xive_native_configure_queue() to be called twice for the new EQ page case: one time to reset the EQ and another time to configure the new page. This is needed because we cannot release the EQ page before calling xive_native_configure_queue() since it may still be used by the HW. We cannot modify xive_native_configure_queue() to drop the reference either because this function is also used by the XICS-on-XIVE device which requires free_pages() instead of put_page(). This isn't a big deal anyway since H_INT_SET_QUEUE_CONFIG isn't a hot path.
Reported-by: Satheesh Rajendran sathnaga@linux.vnet.ibm.com Cc: stable@vger.kernel.org # v5.2 Fixes: 13ce3297c576 ("KVM: PPC: Book3S HV: XIVE: Add controls for the EQ configuration") Signed-off-by: Greg Kurz groug@kaod.org
arch/powerpc/kvm/book3s_xive_native.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c index 34bd123fa024..8ab908d23dc2 100644 --- a/arch/powerpc/kvm/book3s_xive_native.c +++ b/arch/powerpc/kvm/book3s_xive_native.c @@ -570,10 +570,12 @@ static int kvmppc_xive_native_set_queue_config(struct kvmppc_xive *xive, __func__, server, priority, kvm_eq.flags, kvm_eq.qshift, kvm_eq.qaddr, kvm_eq.qtoggle, kvm_eq.qindex);
- /* reset queue and disable queueing */
- if (!kvm_eq.qshift) {
q->guest_qaddr = 0;q->guest_qshift = 0;
- /*
* Reset queue and disable queueing. It will be re-enabled* later on if the guest is configuring a new EQ page.*/- if (q->guest_qshift) {
page = virt_to_page(q->qpage);rc = xive_native_configure_queue(xc->vp_id, q, priority, NULL, 0, true); @@ -583,12 +585,13 @@ static int kvmppc_xive_native_set_queue_config(struct kvmppc_xive *xive, return rc; }
if (q->qpage) {put_page(virt_to_page(q->qpage));q->qpage = NULL;}
put_page(page);
return 0;
if (!kvm_eq.qshift) {q->guest_qaddr = 0;q->guest_qshift = 0;return 0;}/*