| Message ID | 20190107184331.8429-9-clg@kaod.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | KVM: PPC: Book3S HV: add XIVE native exploitation mode | expand |
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.
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.
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.
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.
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)
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(+)