[08/19] KVM: PPC: Book3S HV: add a VC_BASE control to the XIVE native device

Message ID 20190107184331.8429-9-clg@kaod.org
State Changes Requested
Headers show
Series
  • KVM: PPC: Book3S HV: add XIVE native exploitation mode
Related show

Commit Message

Cédric Le Goater Jan. 7, 2019, 6:43 p.m.
The ESB MMIO region controls the interrupt sources of the guest. QEMU
will query an fd (GET_ESB_FD ioctl) and map this region at a specific
address for the guest to use. The guest will obtain this information
using the H_INT_GET_SOURCE_INFO hcall. To inform KVM of the address
setting used by QEMU, add a VC_BASE control to the KVM XIVE device

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/powerpc/include/uapi/asm/kvm.h   |  1 +
 arch/powerpc/kvm/book3s_xive.h        |  3 +++
 arch/powerpc/kvm/book3s_xive_native.c | 39 +++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

Comments

Paul Mackerras Jan. 22, 2019, 5:14 a.m. | #1
On Mon, Jan 07, 2019 at 07:43:20PM +0100, Cédric Le Goater wrote:
> The ESB MMIO region controls the interrupt sources of the guest. QEMU
> will query an fd (GET_ESB_FD ioctl) and map this region at a specific
> address for the guest to use. The guest will obtain this information
> using the H_INT_GET_SOURCE_INFO hcall. To inform KVM of the address
> setting used by QEMU, add a VC_BASE control to the KVM XIVE device

This needs a little more explanation.  I *think* the only way this
gets used is that it gets returned to the guest by the new
hypercalls.  If that is indeed the case it would be useful to mention
that in the patch description, because otherwise taking a value that
userspace provides and which looks like it is an address, and not
doing any validation on it, looks a bit scary.

Paul.
Cédric Le Goater Jan. 23, 2019, 4:56 p.m. | #2
On 1/22/19 6:14 AM, Paul Mackerras wrote:
> On Mon, Jan 07, 2019 at 07:43:20PM +0100, Cédric Le Goater wrote:
>> The ESB MMIO region controls the interrupt sources of the guest. QEMU
>> will query an fd (GET_ESB_FD ioctl) and map this region at a specific
>> address for the guest to use. The guest will obtain this information
>> using the H_INT_GET_SOURCE_INFO hcall. To inform KVM of the address
>> setting used by QEMU, add a VC_BASE control to the KVM XIVE device
> 
> This needs a little more explanation.  I *think* the only way this
> gets used is that it gets returned to the guest by the new
> hypercalls.  If that is indeed the case it would be useful to mention
> that in the patch description, because otherwise taking a value that
> userspace provides and which looks like it is an address, and not
> doing any validation on it, looks a bit scary.

I think we have solved this problem in another email thread. 

The H_INT_GET_SOURCE_INFO hcall does not need to be implemented in KVM
as all the source information should already be available in QEMU. In
that case, there is no need to inform KVM of where the ESB pages are 
mapped in the guest address space. So we don't need that extra control
on the KVM device. This is good news. 

Thanks,

C.
David Gibson Feb. 4, 2019, 4:49 a.m. | #3
On Wed, Jan 23, 2019 at 05:56:26PM +0100, Cédric Le Goater wrote:
> On 1/22/19 6:14 AM, Paul Mackerras wrote:
> > On Mon, Jan 07, 2019 at 07:43:20PM +0100, Cédric Le Goater wrote:
> >> The ESB MMIO region controls the interrupt sources of the guest. QEMU
> >> will query an fd (GET_ESB_FD ioctl) and map this region at a specific
> >> address for the guest to use. The guest will obtain this information
> >> using the H_INT_GET_SOURCE_INFO hcall. To inform KVM of the address
> >> setting used by QEMU, add a VC_BASE control to the KVM XIVE device
> > 
> > This needs a little more explanation.  I *think* the only way this
> > gets used is that it gets returned to the guest by the new
> > hypercalls.  If that is indeed the case it would be useful to mention
> > that in the patch description, because otherwise taking a value that
> > userspace provides and which looks like it is an address, and not
> > doing any validation on it, looks a bit scary.
> 
> I think we have solved this problem in another email thread. 
> 
> The H_INT_GET_SOURCE_INFO hcall does not need to be implemented in KVM
> as all the source information should already be available in QEMU. In
> that case, there is no need to inform KVM of where the ESB pages are 
> mapped in the guest address space. So we don't need that extra control
> on the KVM device. This is good news.

Ah, good to hear.  I thought this looked strange.
Cédric Le Goater Feb. 4, 2019, 3:36 p.m. | #4
On 2/4/19 5:49 AM, David Gibson wrote:
> On Wed, Jan 23, 2019 at 05:56:26PM +0100, Cédric Le Goater wrote:
>> On 1/22/19 6:14 AM, Paul Mackerras wrote:
>>> On Mon, Jan 07, 2019 at 07:43:20PM +0100, Cédric Le Goater wrote:
>>>> The ESB MMIO region controls the interrupt sources of the guest. QEMU
>>>> will query an fd (GET_ESB_FD ioctl) and map this region at a specific
>>>> address for the guest to use. The guest will obtain this information
>>>> using the H_INT_GET_SOURCE_INFO hcall. To inform KVM of the address
>>>> setting used by QEMU, add a VC_BASE control to the KVM XIVE device
>>>
>>> This needs a little more explanation.  I *think* the only way this
>>> gets used is that it gets returned to the guest by the new
>>> hypercalls.  If that is indeed the case it would be useful to mention
>>> that in the patch description, because otherwise taking a value that
>>> userspace provides and which looks like it is an address, and not
>>> doing any validation on it, looks a bit scary.
>>
>> I think we have solved this problem in another email thread. 
>>
>> The H_INT_GET_SOURCE_INFO hcall does not need to be implemented in KVM
>> as all the source information should already be available in QEMU. In
>> that case, there is no need to inform KVM of where the ESB pages are 
>> mapped in the guest address space. So we don't need that extra control
>> on the KVM device. This is good news.
> 
> Ah, good to hear.  I thought this looked strange.

yes. I didn't know which path to choose between HV real mode, HV, QEMU. 
It's clarified now. 

But now, we have nested, and this is adding quite a bit of strangeness 
to the hcall possibilities.

C.

Patch

diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
index 89c140cb9e79..8b78b12aa118 100644
--- a/arch/powerpc/include/uapi/asm/kvm.h
+++ b/arch/powerpc/include/uapi/asm/kvm.h
@@ -679,5 +679,6 @@  struct kvm_ppc_cpu_char {
 #define KVM_DEV_XIVE_GRP_CTRL		1
 #define   KVM_DEV_XIVE_GET_ESB_FD	1
 #define   KVM_DEV_XIVE_GET_TIMA_FD	2
+#define   KVM_DEV_XIVE_VC_BASE		3
 
 #endif /* __LINUX_KVM_POWERPC_H */
diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
index 5f22415520b4..ae4a670eea63 100644
--- a/arch/powerpc/kvm/book3s_xive.h
+++ b/arch/powerpc/kvm/book3s_xive.h
@@ -125,6 +125,9 @@  struct kvmppc_xive {
 
 	/* Flags */
 	u8	single_escalation;
+
+	/* VC base address for ESBs */
+	u64     vc_base;
 };
 
 #define KVMPPC_XIVE_Q_COUNT	8
diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
index ee9d12bf2dae..29a62914de55 100644
--- a/arch/powerpc/kvm/book3s_xive_native.c
+++ b/arch/powerpc/kvm/book3s_xive_native.c
@@ -153,6 +153,25 @@  int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
 	return rc;
 }
 
+static int kvmppc_xive_native_set_vc_base(struct kvmppc_xive *xive, u64 addr)
+{
+	u64 __user *ubufp = (u64 __user *) addr;
+
+	if (get_user(xive->vc_base, ubufp))
+		return -EFAULT;
+	return 0;
+}
+
+static int kvmppc_xive_native_get_vc_base(struct kvmppc_xive *xive, u64 addr)
+{
+	u64 __user *ubufp = (u64 __user *) addr;
+
+	if (put_user(xive->vc_base, ubufp))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int xive_native_esb_fault(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
@@ -289,6 +308,16 @@  static int kvmppc_xive_native_get_tima_fd(struct kvmppc_xive *xive, u64 addr)
 static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
 				       struct kvm_device_attr *attr)
 {
+	struct kvmppc_xive *xive = dev->private;
+
+	switch (attr->group) {
+	case KVM_DEV_XIVE_GRP_CTRL:
+		switch (attr->attr) {
+		case KVM_DEV_XIVE_VC_BASE:
+			return kvmppc_xive_native_set_vc_base(xive, attr->addr);
+		}
+		break;
+	}
 	return -ENXIO;
 }
 
@@ -304,6 +333,8 @@  static int kvmppc_xive_native_get_attr(struct kvm_device *dev,
 			return kvmppc_xive_native_get_esb_fd(xive, attr->addr);
 		case KVM_DEV_XIVE_GET_TIMA_FD:
 			return kvmppc_xive_native_get_tima_fd(xive, attr->addr);
+		case KVM_DEV_XIVE_VC_BASE:
+			return kvmppc_xive_native_get_vc_base(xive, attr->addr);
 		}
 		break;
 	}
@@ -318,6 +349,7 @@  static int kvmppc_xive_native_has_attr(struct kvm_device *dev,
 		switch (attr->attr) {
 		case KVM_DEV_XIVE_GET_ESB_FD:
 		case KVM_DEV_XIVE_GET_TIMA_FD:
+		case KVM_DEV_XIVE_VC_BASE:
 			return 0;
 		}
 		break;
@@ -353,6 +385,11 @@  static void kvmppc_xive_native_free(struct kvm_device *dev)
 	kfree(dev);
 }
 
+/*
+ * ESB MMIO address of chip 0
+ */
+#define XIVE_VC_BASE   0x0006010000000000ull
+
 static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)
 {
 	struct kvmppc_xive *xive;
@@ -387,6 +424,8 @@  static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)
 	if (xive->vp_base == XIVE_INVALID_VP)
 		ret = -ENOMEM;
 
+	xive->vc_base = XIVE_VC_BASE;
+
 	xive->single_escalation = xive_native_has_single_escalation();
 
 	if (ret)