From: David Gibson <hidden> Date: 2019-02-04 02:00:05
On Wed, Jan 23, 2019 at 05:24:13PM +0100, Cédric Le Goater wrote:
On 1/22/19 5:56 AM, Paul Mackerras wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:15PM +0100, Cédric Le Goater wrote:
quoted
We will have different KVM devices for interrupts, one for the
XICS-over-XIVE mode and one for the XIVE native exploitation
mode. Let's add some checks to make sure we are not mixing the
interfaces in KVM.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/kvm/book3s_xive.c | 6 ++++++
1 file changed, 6 insertions(+)
@@ -835,6 +838,9 @@ int kvmppc_xive_set_icp(struct kvm_vcpu *vcpu, u64 icpval)u8cppr,mfrr;u32xisr;+if(!kvmppc_xics_enabled(vcpu))+return-EPERM;+if(!xc||!xive)return-ENOENT;
I can't see how these new checks could ever trigger in the code as it
stands. Is there a way at present?
It would require some custom QEMU doing silly things : create the XICS
KVM device, and then call kvm_get_one_reg(KVM_REG_PPC_ICP_STATE) or
kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE) without connecting the
vCPU to its presenter.
Today, you get a ENOENT.
TBH, ENOENT seems fine to me.
quoted
Do following patches ever add a path where the new checks could trigger,
or is this just an excess of caution?
With the following patches, QEMU could to do something even more silly,
which is to mix the interrupt mode interfaces : create a KVM XICS device
and call KVM CPU ioctls of the KVM XIVE device, or the opposite.
AFAICT, like above, that won't really differ from calling the XIVE CPU
ioctl()s when no irqchip is set up at all, and should be covered by
just a !xive check.
quoted
(Your patch description should ideally have answered these questions > for me.)
Yes. I also think that I introduced this patch to early in the series.
It make more sense when the XICS and the XIVE KVM devices are available.
Thanks,
C.
--
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
From: David Gibson <hidden> Date: 2019-02-04 06:14:53
On Mon, Jan 07, 2019 at 07:43:17PM +0100, Cédric Le Goater wrote:
quoted hunk
This is the basic framework for the new KVM device supporting the XIVE
native exploitation mode. The user interface exposes a new capability
and a new KVM device to be used by QEMU.
Internally, the interface to the new KVM device is protected with a
new interrupt mode: KVMPPC_IRQ_XIVE.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/asm/kvm_host.h | 2 +
arch/powerpc/include/asm/kvm_ppc.h | 21 ++
arch/powerpc/kvm/book3s_xive.h | 3 +
include/uapi/linux/kvm.h | 3 +
arch/powerpc/kvm/book3s.c | 7 +-
arch/powerpc/kvm/book3s_xive_native.c | 332 ++++++++++++++++++++++++++
arch/powerpc/kvm/powerpc.c | 30 +++
arch/powerpc/kvm/Makefile | 2 +-
8 files changed, 398 insertions(+), 2 deletions(-)
create mode 100644 arch/powerpc/kvm/book3s_xive_native.c
@@ -0,0 +1,332 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Copyright(c)2017-2019,IBMCorporation.+*/++#define pr_fmt(fmt) "xive-kvm: " fmt++#include<linux/anon_inodes.h>+#include<linux/kernel.h>+#include<linux/kvm_host.h>+#include<linux/err.h>+#include<linux/gfp.h>+#include<linux/spinlock.h>+#include<linux/delay.h>+#include<linux/percpu.h>+#include<linux/cpumask.h>+#include<asm/uaccess.h>+#include<asm/kvm_book3s.h>+#include<asm/kvm_ppc.h>+#include<asm/hvcall.h>+#include<asm/xics.h>+#include<asm/xive.h>+#include<asm/xive-regs.h>+#include<asm/debug.h>+#include<asm/debugfs.h>+#include<asm/time.h>+#include<asm/opal.h>++#include<linux/debugfs.h>+#include<linux/seq_file.h>++#include"book3s_xive.h"++staticvoidxive_native_cleanup_queue(structkvm_vcpu*vcpu,intprio)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+structxive_q*q=&xc->queues[prio];++xive_native_disable_queue(xc->vp_id,q,prio);+if(q->qpage){+put_page(virt_to_page(q->qpage));+q->qpage=NULL;+}+}++voidkvmppc_xive_native_cleanup_vcpu(structkvm_vcpu*vcpu)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+inti;++if(!kvmppc_xive_enabled(vcpu))+return;++if(!xc)+return;++pr_devel("native_cleanup_vcpu(cpu=%d)\n",xc->server_num);++/* Ensure no interrupt is still routed to that VP */+xc->valid=false;+kvmppc_xive_disable_vcpu_interrupts(vcpu);++/* Disable the VP */+xive_native_disable_vp(xc->vp_id);++/* Free the queues & associated interrupts */+for(i=0;i<KVMPPC_XIVE_Q_COUNT;i++){+/* Free the escalation irq */+if(xc->esc_virq[i]){+free_irq(xc->esc_virq[i],vcpu);+irq_dispose_mapping(xc->esc_virq[i]);+kfree(xc->esc_virq_names[i]);+xc->esc_virq[i]=0;+}++/* Free the queue */+xive_native_cleanup_queue(vcpu,i);+}++/* Free the VP */+kfree(xc);++/* Cleanup the vcpu */+vcpu->arch.irq_type=KVMPPC_IRQ_DEFAULT;+vcpu->arch.xive_vcpu=NULL;+}++intkvmppc_xive_native_connect_vcpu(structkvm_device*dev,+structkvm_vcpu*vcpu,u32cpu)
Why do we need both a *vcpu and a cpu number as an integer?
quoted hunk
+{+ struct kvmppc_xive *xive = dev->private;+ struct kvmppc_xive_vcpu *xc;+ int rc;++ pr_devel("native_connect_vcpu(cpu=%d)\n", cpu);++ if (dev->ops != &kvm_xive_native_ops) {+ pr_devel("Wrong ops !\n");+ return -EPERM;+ }+ if (xive->kvm != vcpu->kvm)+ return -EPERM;+ if (vcpu->arch.irq_type)
Please use an explicit == / != here so we don't have to remember which
symbolic value corresponds to 0.
quoted hunk
+ return -EBUSY;+ if (kvmppc_xive_find_server(vcpu->kvm, cpu)) {+ pr_devel("Duplicate !\n");+ return -EEXIST;+ }+ if (cpu >= KVM_MAX_VCPUS) {+ pr_devel("Out of bounds !\n");+ return -EINVAL;+ }+ xc = kzalloc(sizeof(*xc), GFP_KERNEL);+ if (!xc)+ return -ENOMEM;++ mutex_lock(&vcpu->kvm->lock);+ vcpu->arch.xive_vcpu = xc;+ xc->xive = xive;+ xc->vcpu = vcpu;+ xc->server_num = cpu;+ xc->vp_id = xive->vp_base + cpu;+ xc->valid = true;++ rc = xive_native_get_vp_info(xc->vp_id, &xc->vp_cam, &xc->vp_chip_id);+ if (rc) {+ pr_err("Failed to get VP info from OPAL: %d\n", rc);+ goto bail;+ }++ /*+ * Enable the VP first as the single escalation mode will+ * affect escalation interrupts numbering+ */+ rc = xive_native_enable_vp(xc->vp_id, xive->single_escalation);+ if (rc) {+ pr_err("Failed to enable VP in OPAL: %d\n", rc);+ goto bail;+ }++ /* Configure VCPU fields for use by assembly push/pull */+ vcpu->arch.xive_saved_state.w01 = cpu_to_be64(0xff000000);+ vcpu->arch.xive_cam_word = cpu_to_be32(xc->vp_cam | TM_QW1W2_VO);++ /* TODO: initialize queues ? */++bail:+ vcpu->arch.irq_type = KVMPPC_IRQ_XIVE;+ mutex_unlock(&vcpu->kvm->lock);+ if (rc)+ kvmppc_xive_native_cleanup_vcpu(vcpu);++ return rc;+}++static int kvmppc_xive_native_set_attr(struct kvm_device *dev,+ struct kvm_device_attr *attr)+{+ return -ENXIO;+}++static int kvmppc_xive_native_get_attr(struct kvm_device *dev,+ struct kvm_device_attr *attr)+{+ return -ENXIO;+}++static int kvmppc_xive_native_has_attr(struct kvm_device *dev,+ struct kvm_device_attr *attr)+{+ return -ENXIO;+}++static void kvmppc_xive_native_free(struct kvm_device *dev)+{+ struct kvmppc_xive *xive = dev->private;+ struct kvm *kvm = xive->kvm;+ int i;++ debugfs_remove(xive->dentry);++ pr_devel("Destroying xive native for partition\n");++ if (kvm)+ kvm->arch.xive = NULL;++ /* Mask and free interrupts */+ for (i = 0; i <= xive->max_sbid; i++) {+ if (xive->src_blocks[i])+ kvmppc_xive_free_sources(xive->src_blocks[i]);+ kfree(xive->src_blocks[i]);+ xive->src_blocks[i] = NULL;+ }++ if (xive->vp_base != XIVE_INVALID_VP)+ xive_native_free_vp_block(xive->vp_base);++ kfree(xive);+ kfree(dev);+}++static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)+{+ struct kvmppc_xive *xive;+ struct kvm *kvm = dev->kvm;+ int ret = 0;++ pr_devel("Creating xive native for partition\n");++ if (kvm->arch.xive)+ return -EEXIST;++ xive = kzalloc(sizeof(*xive), GFP_KERNEL);+ if (!xive)+ return -ENOMEM;++ dev->private = xive;+ xive->dev = dev;+ xive->kvm = kvm;+ kvm->arch.xive = xive;++ /* We use the default queue size set by the host */+ xive->q_order = xive_native_default_eq_shift();+ if (xive->q_order < PAGE_SHIFT)+ xive->q_page_order = 0;+ else+ xive->q_page_order = xive->q_order - PAGE_SHIFT;++ /* Allocate a bunch of VPs */+ xive->vp_base = xive_native_alloc_vp_block(KVM_MAX_VCPUS);+ pr_devel("VP_Base=%x\n", xive->vp_base);++ if (xive->vp_base == XIVE_INVALID_VP)+ ret = -ENOMEM;++ xive->single_escalation = xive_native_has_single_escalation();++ if (ret)+ kfree(xive);++ return ret;+}++static int xive_native_debug_show(struct seq_file *m, void *private)+{+ struct kvmppc_xive *xive = m->private;+ struct kvm *kvm = xive->kvm;+ struct kvm_vcpu *vcpu;+ unsigned int i;++ if (!kvm)+ return 0;++ seq_puts(m, "=========\nVCPU state\n=========\n");++ kvm_for_each_vcpu(i, vcpu, kvm) {+ struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;++ if (!xc)+ continue;++ seq_printf(m, "cpu server %#x NSR=%02x CPPR=%02x IBP=%02x PIPR=%02x w01=%016llx w2=%08x\n",+ xc->server_num,+ vcpu->arch.xive_saved_state.nsr,+ vcpu->arch.xive_saved_state.cppr,+ vcpu->arch.xive_saved_state.ipb,+ vcpu->arch.xive_saved_state.pipr,+ vcpu->arch.xive_saved_state.w01,+ (u32) vcpu->arch.xive_cam_word);++ kvmppc_xive_debug_show_queues(m, vcpu);+ }++ return 0;+}++static int xive_native_debug_open(struct inode *inode, struct file *file)+{+ return single_open(file, xive_native_debug_show, inode->i_private);+}++static const struct file_operations xive_native_debug_fops = {+ .open = xive_native_debug_open,+ .read = seq_read,+ .llseek = seq_lseek,+ .release = single_release,+};++static void xive_native_debugfs_init(struct kvmppc_xive *xive)+{+ char *name;++ name = kasprintf(GFP_KERNEL, "kvm-xive-%p", xive);+ if (!name) {+ pr_err("%s: no memory for name\n", __func__);+ return;+ }++ xive->dentry = debugfs_create_file(name, 0444, powerpc_debugfs_root,+ xive, &xive_native_debug_fops);++ pr_debug("%s: created %s\n", __func__, name);+ kfree(name);+}++static void kvmppc_xive_native_init(struct kvm_device *dev)+{+ struct kvmppc_xive *xive = (struct kvmppc_xive *)dev->private;++ /* Register some debug interfaces */+ xive_native_debugfs_init(xive);+}++struct kvm_device_ops kvm_xive_native_ops = {+ .name = "kvm-xive-native",+ .create = kvmppc_xive_native_create,+ .init = kvmppc_xive_native_init,+ .destroy = kvmppc_xive_native_free,+ .set_attr = kvmppc_xive_native_set_attr,+ .get_attr = kvmppc_xive_native_get_attr,+ .has_attr = kvmppc_xive_native_has_attr,+};++void kvmppc_xive_native_init_module(void)+{+ ;+}++void kvmppc_xive_native_exit_module(void)+{+ ;+}
--
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
From: David Gibson <hidden> Date: 2019-02-04 06:16:22
On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
quoted hunk
This will let the guest create a memory mapping to expose the ESB MMIO
regions used to control the interrupt sources, to trigger events, to
EOI or to turn off the sources.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 4 ++
arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Introducing a new FD for ESB and TIMA seems overkill. Can't you get
to both with an mmap() directly on the xive device fd? Using the
offset to distinguish which one to map, obviously.
@@ -153,6 +153,85 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,returnrc;}+staticintxive_native_esb_fault(structvm_fault*vmf)+{+structvm_area_struct*vma=vmf->vma;+structkvmppc_xive*xive=vma->vm_file->private_data;+structkvmppc_xive_src_block*sb;+structkvmppc_xive_irq_state*state;+structxive_irq_data*xd;+u32hw_num;+u16src;+u64page;+unsignedlongirq;++/*+*Linux/KVMusesatwopagesESBsetting,onefortriggerand+*oneforEOI+*/+irq=vmf->pgoff/2;++sb=kvmppc_xive_find_source(xive,irq,&src);+if(!sb){+pr_err("%s: source %lx not found !\n",__func__,irq);+returnVM_FAULT_SIGBUS;+}++state=&sb->irq_state[src];+kvmppc_xive_select_irq(state,&hw_num,&xd);++arch_spin_lock(&sb->lock);++/*+*first/evenpageisfortrigger+*second/oddpageisforEOIandmanagement.+*/+page=vmf->pgoff%2?xd->eoi_page:xd->trig_page;+arch_spin_unlock(&sb->lock);++if(!page){+pr_err("%s: acessing invalid ESB page for source %lx !\n",+__func__,irq);+returnVM_FAULT_SIGBUS;+}++vmf_insert_pfn(vma,vmf->address,page>>PAGE_SHIFT);+returnVM_FAULT_NOPAGE;+}++staticconststructvm_operations_structxive_native_esb_vmops={+.fault=xive_native_esb_fault,+};++staticintxive_native_esb_mmap(structfile*file,structvm_area_struct*vma)+{+/* There are two ESB pages (trigger and EOI) per IRQ */+if(vma_pages(vma)+vma->vm_pgoff>KVMPPC_XIVE_NR_IRQS*2)+return-EINVAL;++vma->vm_flags|=VM_IO|VM_PFNMAP;+vma->vm_page_prot=pgprot_noncached(vma->vm_page_prot);+vma->vm_ops=&xive_native_esb_vmops;+return0;+}++staticconststructfile_operationsxive_native_esb_fops={+.mmap=xive_native_esb_mmap,+};++staticintkvmppc_xive_native_get_esb_fd(structkvmppc_xive*xive,u64addr)+{+u64__user*ubufp=(u64__user*)addr;+intret;++ret=anon_inode_getfd("[xive-esb]",&xive_native_esb_fops,xive,+O_RDWR|O_CLOEXEC);+if(ret<0)+returnret;++returnput_user(ret,ubufp);+}+staticintkvmppc_xive_native_set_attr(structkvm_device*dev,structkvm_device_attr*attr){
@@ -162,12 +241,30 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,staticintkvmppc_xive_native_get_attr(structkvm_device*dev,structkvm_device_attr*attr){+structkvmppc_xive*xive=dev->private;++switch(attr->group){+caseKVM_DEV_XIVE_GRP_CTRL:+switch(attr->attr){+caseKVM_DEV_XIVE_GET_ESB_FD:+returnkvmppc_xive_native_get_esb_fd(xive,attr->addr);+}+break;+}return-ENXIO;}staticintkvmppc_xive_native_has_attr(structkvm_device*dev,structkvm_device_attr*attr){+switch(attr->group){+caseKVM_DEV_XIVE_GRP_CTRL:+switch(attr->attr){+caseKVM_DEV_XIVE_GET_ESB_FD:+return0;+}+break;+}return-ENXIO;}
--
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
+ sb = kvmppc_xive_create_src_block(xive, irq);+ if (!sb) {+ pr_err("Failed to create block...\n");+ return -ENOMEM;+ }+ }+ state = &sb->irq_state[idx];++ if (get_user(val, ubufp)) {+ pr_err("fault getting user info !\n");+ return -EFAULT;+ }++ /*+ * If the source doesn't already have an IPI, allocate+ * one and get the corresponding data+ */+ if (!state->ipi_number) {+ state->ipi_number = xive_native_alloc_irq();+ if (state->ipi_number == 0) {+ pr_err("Failed to allocate IRQ !\n");+ return -ENOMEM;+ }
Am I right in thinking this is the point at which a specific guest irq
number gets bound to a specific host irq number?
quoted hunk
+ xive_native_populate_irq_data(state->ipi_number,+ &state->ipi_data);+ pr_debug("%s allocated hw_irq=0x%x for irq=0x%lx\n", __func__,+ state->ipi_number, irq);+ }++ arch_spin_lock(&sb->lock);++ /* Restore LSI state */+ if (val & KVM_XIVE_LEVEL_SENSITIVE) {+ state->lsi = true;+ if (val & KVM_XIVE_LEVEL_ASSERTED)+ state->asserted = true;+ pr_devel(" LSI ! Asserted=%d\n", state->asserted);+ }++ /* Mask IRQ to start with */+ state->act_server = 0;+ state->act_priority = MASKED;+ xive_vm_esb_load(&state->ipi_data, XIVE_ESB_SET_PQ_01);+ xive_native_configure_irq(state->ipi_number, 0, MASKED, 0);++ /* Increment the number of valid sources and mark this one valid */+ if (!state->valid)+ xive->src_count++;+ state->valid = true;++ arch_spin_unlock(&sb->lock);++ return 0;+}+ static int kvmppc_xive_native_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) {
@@ -0,0 +1,27 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Copyright(c)2017-2019,IBMCorporation.+*/++/* File to be included by other .c files */++#define XGLUE(a, b) a##b+#define GLUE(a, b) XGLUE(a, b)++/*+*TODO:introduceacommontemplatefilewiththeXIVEnativelayer+*andtheXICS-on-XIVEgluefortheutilityfunctions+*/+staticu8GLUE(X_PFX,esb_load)(structxive_irq_data*xd,u32offset)+{+u64val;++if(xd->flags&XIVE_IRQ_FLAG_SHIFT_BUG)+offset|=offset<<4;++val=__x_readq(__x_eoi_page(xd)+offset);+#ifdef __LITTLE_ENDIAN__+val>>=64-8;+#endif+return(u8)val;+}
--
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
From: David Gibson <hidden> Date: 2019-02-04 06:19:10
On Mon, Jan 07, 2019 at 07:43:24PM +0100, Cédric Le Goater wrote:
The guest physical address of the event queue will be part of the
state to transfer in the migration. Cache its value when the queue is
configured, it will save us an OPAL call.
That doesn't sound like a very compelling case - migration is already
a hundreds of milliseconds type operation, I wouldn't expect a few
extra OPAL calls to be an issue.
@@ -74,6 +74,8 @@ struct xive_q {u32esc_irq;atomic_tcount;atomic_tpending_count;+u64guest_qpage;+u32guest_qsize;};/* Global enable flags for the XIVE support */
@@ -708,6 +708,10 @@ static int kvmppc_h_int_set_queue_config(struct kvm_vcpu *vcpu,}qaddr=page_to_virt(page)+(qpage&~PAGE_MASK);+/* Backup queue page address and size for migration */+q->guest_qpage=qpage;+q->guest_qsize=qsize;+rc=xive_native_configure_queue(xc->vp_id,q,priority,(__be32*)qaddr,qsize,true);if(rc){
--
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
From: David Gibson <hidden> Date: 2019-02-04 06:20:34
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:
quoted
On Mon, Jan 07, 2019 at 07:43:20PM +0100, Cédric Le Goater wrote:
quoted
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.
--
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
From: David Gibson <hidden> Date: 2019-02-04 06:22:10
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted hunk
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
@@ -583,6 +669,7 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev, break; case KVM_DEV_XIVE_GRP_SOURCES: case KVM_DEV_XIVE_GRP_SYNC:+ case KVM_DEV_XIVE_GRP_EAS: if (attr->attr >= KVMPPC_XIVE_FIRST_IRQ && attr->attr < KVMPPC_XIVE_NR_IRQS) return 0;
--
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
From: David Gibson <hidden> Date: 2019-02-04 06:23:35
On Mon, Jan 07, 2019 at 07:43:28PM +0100, Cédric Le Goater wrote:
quoted hunk
These are used to capture the XIVE END table of the KVM device. It
relies on an OPAL call to retrieve from the XIVE IC the EQ toggle bit
and index which are updated by the HW when events are enqueued in the
guest RAM.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 21 ++++
arch/powerpc/kvm/book3s_xive_native.c | 166 ++++++++++++++++++++++++++
2 files changed, 187 insertions(+)
Should we pad this in case a) we discover some fields in the EQ that
we thought weren't relevant to the guest actually are or b) future
XIVE extensions add something we need to migrate.
@@ -607,6 +607,164 @@ static int kvmppc_xive_native_get_eas(struct kvmppc_xive *xive, long irq,return0;}+staticintkvmppc_xive_native_set_queue(structkvmppc_xive*xive,longeq_idx,+u64addr)+{+structkvm*kvm=xive->kvm;+structkvm_vcpu*vcpu;+structkvmppc_xive_vcpu*xc;+void__user*ubufp=(u64__user*)addr;+u32server;+u8priority;+structkvm_ppc_xive_eqkvm_eq;+intrc;+__be32*qaddr=0;+structpage*page;+structxive_q*q;++/*+*Demanglepriority/servertuplefromtheEQindex+*/+priority=(eq_idx&KVM_XIVE_EQ_PRIORITY_MASK)>>+KVM_XIVE_EQ_PRIORITY_SHIFT;+server=(eq_idx&KVM_XIVE_EQ_SERVER_MASK)>>+KVM_XIVE_EQ_SERVER_SHIFT;++if(copy_from_user(&kvm_eq,ubufp,sizeof(kvm_eq)))+return-EFAULT;++vcpu=kvmppc_xive_find_server(kvm,server);+if(!vcpu){+pr_err("Can't find server %d\n",server);+return-ENOENT;+}+xc=vcpu->arch.xive_vcpu;++if(priority!=xive_prio_from_guest(priority)){+pr_err("Trying to restore invalid queue %d for VCPU %d\n",+priority,server);+return-EINVAL;+}+q=&xc->queues[priority];++pr_devel("%s VCPU %d priority %d fl:%x sz:%d addr:%llx g:%d idx:%d\n",+__func__,server,priority,kvm_eq.flags,+kvm_eq.qsize,kvm_eq.qpage,kvm_eq.qtoggle,kvm_eq.qindex);++rc=xive_native_validate_queue_size(kvm_eq.qsize);+if(rc||!kvm_eq.qsize){+pr_err("invalid queue size %d\n",kvm_eq.qsize);+returnrc;+}++page=gfn_to_page(kvm,gpa_to_gfn(kvm_eq.qpage));+if(is_error_page(page)){+pr_warn("Couldn't get guest page for %llx!\n",kvm_eq.qpage);+return-ENOMEM;+}+qaddr=page_to_virt(page)+(kvm_eq.qpage&~PAGE_MASK);++/* Backup queue page guest address for migration */+q->guest_qpage=kvm_eq.qpage;+q->guest_qsize=kvm_eq.qsize;++rc=xive_native_configure_queue(xc->vp_id,q,priority,+(__be32*)qaddr,kvm_eq.qsize,true);+if(rc){+pr_err("Failed to configure queue %d for VCPU %d: %d\n",+priority,xc->server_num,rc);+put_page(page);+returnrc;+}++rc=xive_native_set_queue_state(xc->vp_id,priority,kvm_eq.qtoggle,+kvm_eq.qindex);+if(rc)+gotoerror;++rc=kvmppc_xive_attach_escalation(vcpu,priority);+error:+if(rc)+xive_native_cleanup_queue(vcpu,priority);+returnrc;+}++staticintkvmppc_xive_native_get_queue(structkvmppc_xive*xive,longeq_idx,+u64addr)+{+structkvm*kvm=xive->kvm;+structkvm_vcpu*vcpu;+structkvmppc_xive_vcpu*xc;+structxive_q*q;+void__user*ubufp=(u64__user*)addr;+u32server;+u8priority;+structkvm_ppc_xive_eqkvm_eq;+u64qpage;+u64qsize;+u64qeoi_page;+u32escalate_irq;+u64qflags;+intrc;++/*+*Demanglepriority/servertuplefromtheEQindex+*/+priority=(eq_idx&KVM_XIVE_EQ_PRIORITY_MASK)>>+KVM_XIVE_EQ_PRIORITY_SHIFT;+server=(eq_idx&KVM_XIVE_EQ_SERVER_MASK)>>+KVM_XIVE_EQ_SERVER_SHIFT;++vcpu=kvmppc_xive_find_server(kvm,server);+if(!vcpu){+pr_err("Can't find server %d\n",server);+return-ENOENT;+}+xc=vcpu->arch.xive_vcpu;++if(priority!=xive_prio_from_guest(priority)){+pr_err("invalid priority for queue %d for VCPU %d\n",+priority,server);+return-EINVAL;+}+q=&xc->queues[priority];++memset(&kvm_eq,0,sizeof(kvm_eq));++if(!q->qpage)+return0;++rc=xive_native_get_queue_info(xc->vp_id,priority,&qpage,&qsize,+&qeoi_page,&escalate_irq,&qflags);+if(rc)+returnrc;++kvm_eq.flags=0;+if(qflags&OPAL_XIVE_EQ_ENABLED)+kvm_eq.flags|=KVM_XIVE_EQ_FLAG_ENABLED;+if(qflags&OPAL_XIVE_EQ_ALWAYS_NOTIFY)+kvm_eq.flags|=KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY;+if(qflags&OPAL_XIVE_EQ_ESCALATE)+kvm_eq.flags|=KVM_XIVE_EQ_FLAG_ESCALATE;++kvm_eq.qsize=q->guest_qsize;+kvm_eq.qpage=q->guest_qpage;++rc=xive_native_get_queue_state(xc->vp_id,priority,&kvm_eq.qtoggle,+&kvm_eq.qindex);+if(rc)+returnrc;++pr_devel("%s VCPU %d priority %d fl:%x sz:%d addr:%llx g:%d idx:%d\n",+__func__,server,priority,kvm_eq.flags,+kvm_eq.qsize,kvm_eq.qpage,kvm_eq.qtoggle,kvm_eq.qindex);++if(copy_to_user(ubufp,&kvm_eq,sizeof(kvm_eq)))+return-EFAULT;++return0;+}+staticintkvmppc_xive_native_set_attr(structkvm_device*dev,structkvm_device_attr*attr){
@@ -628,6 +786,9 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,returnkvmppc_xive_native_sync(xive,attr->attr,attr->addr);caseKVM_DEV_XIVE_GRP_EAS:returnkvmppc_xive_native_set_eas(xive,attr->attr,attr->addr);+caseKVM_DEV_XIVE_GRP_EQ:+returnkvmppc_xive_native_set_queue(xive,attr->attr,+attr->addr);}return-ENXIO;}
@@ -650,6 +811,9 @@ static int kvmppc_xive_native_get_attr(struct kvm_device *dev,break;caseKVM_DEV_XIVE_GRP_EAS:returnkvmppc_xive_native_get_eas(xive,attr->attr,attr->addr);+caseKVM_DEV_XIVE_GRP_EQ:+returnkvmppc_xive_native_get_queue(xive,attr->attr,+attr->addr);}return-ENXIO;}
@@ -674,6 +838,8 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,attr->attr<KVMPPC_XIVE_NR_IRQS)return0;break;+caseKVM_DEV_XIVE_GRP_EQ:+return0;}return-ENXIO;}
--
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
From: David Gibson <hidden> Date: 2019-02-04 06:25:06
On Mon, Jan 07, 2019 at 07:43:25PM +0100, Cédric Le Goater wrote:
When migration of a VM is initiated, a first copy of the RAM is
transferred to the destination before the VM is stopped. At that time,
QEMU needs to perform a XIVE quiesce sequence to stop the flow of
event notifications and stabilize the EQs. The sources are masked and
the XIVE IC is synced with the KVM ioctl KVM_DEV_XIVE_GRP_SYNC.
Don't you also need to make sure the guests queue pages are marked
dirty here, in case they were already migrated?
@@ -459,6 +459,35 @@ static int kvmppc_xive_native_set_source(struct kvmppc_xive *xive, long irq,return0;}+staticintkvmppc_xive_native_sync(structkvmppc_xive*xive,longirq,u64addr)+{+structkvmppc_xive_src_block*sb;+structkvmppc_xive_irq_state*state;+structxive_irq_data*xd;+u32hw_num;+u16src;++pr_devel("%s irq=0x%lx\n",__func__,irq);++sb=kvmppc_xive_find_source(xive,irq,&src);+if(!sb)+return-ENOENT;++state=&sb->irq_state[src];++if(!state->valid)+return-ENOENT;++arch_spin_lock(&sb->lock);++kvmppc_xive_select_irq(state,&hw_num,&xd);+xive_native_sync_source(hw_num);+xive_native_sync_queue(hw_num);++arch_spin_unlock(&sb->lock);+return0;+}+staticintkvmppc_xive_native_set_attr(structkvm_device*dev,structkvm_device_attr*attr){
@@ -474,6 +503,8 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,caseKVM_DEV_XIVE_GRP_SOURCES:returnkvmppc_xive_native_set_source(xive,attr->attr,attr->addr);+caseKVM_DEV_XIVE_GRP_SYNC:+returnkvmppc_xive_native_sync(xive,attr->attr,attr->addr);}return-ENXIO;}
@@ -511,6 +542,7 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,}break;caseKVM_DEV_XIVE_GRP_SOURCES:+caseKVM_DEV_XIVE_GRP_SYNC:if(attr->attr>=KVMPPC_XIVE_FIRST_IRQ&&attr->attr<KVMPPC_XIVE_NR_IRQS)return0;
--
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
From: David Gibson <hidden> Date: 2019-02-04 06:26:51
On Mon, Jan 07, 2019 at 07:43:26PM +0100, Cédric Le Goater wrote:
When the VM is stopped in a migration sequence, the sources are masked
and the XIVE IC is synced to stabilize the EQs. When done, the KVM
ioctl KVM_DEV_XIVE_SAVE_EQ_PAGES is called to mark dirty the EQ pages.
The migration can then transfer the remaining dirty pages to the
destination and start collecting the state of the devices.
Is there a reason to make this a separate step from the SYNC
operation?
@@ -373,6 +373,43 @@ static int kvmppc_xive_native_get_tima_fd(struct kvmppc_xive *xive, u64 addr)returnput_user(ret,ubufp);}+staticintkvmppc_xive_native_vcpu_save_eq_pages(structkvm_vcpu*vcpu)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+unsignedintprio;++if(!xc)+return-ENOENT;++for(prio=0;prio<KVMPPC_XIVE_Q_COUNT;prio++){+structxive_q*q=&xc->queues[prio];++if(!q->qpage)+continue;++/* Mark EQ page dirty for migration */+mark_page_dirty(vcpu->kvm,gpa_to_gfn(q->guest_qpage));+}+return0;+}++staticintkvmppc_xive_native_save_eq_pages(structkvmppc_xive*xive)+{+structkvm*kvm=xive->kvm;+structkvm_vcpu*vcpu;+unsignedinti;++pr_devel("%s\n",__func__);++mutex_lock(&kvm->lock);+kvm_for_each_vcpu(i,vcpu,kvm){+kvmppc_xive_native_vcpu_save_eq_pages(vcpu);+}+mutex_unlock(&kvm->lock);++return0;+}+staticintxive_native_validate_queue_size(u32qsize){switch(qsize){
@@ -498,6 +535,8 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,switch(attr->attr){caseKVM_DEV_XIVE_VC_BASE:returnkvmppc_xive_native_set_vc_base(xive,attr->addr);+caseKVM_DEV_XIVE_SAVE_EQ_PAGES:+returnkvmppc_xive_native_save_eq_pages(xive);}break;caseKVM_DEV_XIVE_GRP_SOURCES:
@@ -538,6 +577,7 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,caseKVM_DEV_XIVE_GET_ESB_FD:caseKVM_DEV_XIVE_GET_TIMA_FD:caseKVM_DEV_XIVE_VC_BASE:+caseKVM_DEV_XIVE_SAVE_EQ_PAGES:return0;}break;
--
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
From: David Gibson <hidden> Date: 2019-02-04 06:28:20
On Mon, Jan 07, 2019 at 08:10:04PM +0100, Cédric Le Goater wrote:
quoted hunk
At a VCPU level, the state of the thread context interrupt management
registers needs to be collected. These registers are cached under the
'xive_saved_state.w01' field of the VCPU when the VPCU context is
pulled from the HW thread. An OPAL call retrieves the backup of the
IPB register in the NVT structure and merges it in the KVM state.
The structures of the interface between QEMU and KVM provisions some
extra room (two u64) for further extensions if more state needs to be
transferred back to QEMU.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/asm/kvm_ppc.h | 5 ++
arch/powerpc/include/uapi/asm/kvm.h | 2 +
arch/powerpc/kvm/book3s.c | 24 +++++++++
arch/powerpc/kvm/book3s_xive_native.c | 78 +++++++++++++++++++++++++++
4 files changed, 109 insertions(+)
@@ -424,6 +424,84 @@ static int xive_native_validate_queue_size(u32 qsize)}}+#define TM_IPB_SHIFT 40+#define TM_IPB_MASK (((u64) 0xFF) << TM_IPB_SHIFT)++intkvmppc_xive_native_get_vp(structkvm_vcpu*vcpu,unionkvmppc_one_reg*val)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+u64opal_state;+intrc;++if(!kvmppc_xive_enabled(vcpu))+return-EPERM;++if(!xc)+return-ENOENT;++/* Thread context registers. We only care about IPB and CPPR */+val->xive_timaval[0]=vcpu->arch.xive_saved_state.w01;++/*+*ReturntheOSCAMlinetoprintouttheVPidentifierin+*theQEMUmonitor.Thisisnotrestored.+*/+val->xive_timaval[1]=vcpu->arch.xive_cam_word;++/* Get the VP state from OPAL */+rc=xive_native_get_vp_state(xc->vp_id,&opal_state);+if(rc)+returnrc;++/*+*CapturethebackupofIPBregisterintheNVTstructureand+*mergeitinourKVMVPstate.+*+*TODO:P10support.+*/+val->xive_timaval[0]|=cpu_to_be64(opal_state&TM_IPB_MASK);++pr_devel("%s NSR=%02x CPPR=%02x IBP=%02x PIPR=%02x w01=%016llx w2=%08x opal=%016llx\n",+__func__,+vcpu->arch.xive_saved_state.nsr,+vcpu->arch.xive_saved_state.cppr,+vcpu->arch.xive_saved_state.ipb,+vcpu->arch.xive_saved_state.pipr,+vcpu->arch.xive_saved_state.w01,+(u32)vcpu->arch.xive_cam_word,opal_state);++return0;+}++intkvmppc_xive_native_set_vp(structkvm_vcpu*vcpu,unionkvmppc_one_reg*val)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+structkvmppc_xive*xive=vcpu->kvm->arch.xive;++pr_devel("%s w01=%016llx vp=%016llx\n",__func__,+val->xive_timaval[0],val->xive_timaval[1]);++if(!kvmppc_xive_enabled(vcpu))+return-EPERM;++if(!xc||!xive)+return-ENOENT;++/* We can't update the state of a "pushed" VCPU */+if(WARN_ON(vcpu->arch.xive_pushed))+return-EIO;++/* Thread context registers. only restore IPB and CPPR ? */+vcpu->arch.xive_saved_state.w01=val->xive_timaval[0];++/*+*ThereisnoneedtorestoretheXIVEinternalstate(IPB+*storedintheNVT)astheIPBregisterwasmergedinKVMVP+*state.+*/+return0;+}+staticintkvmppc_xive_native_set_source(structkvmppc_xive*xive,longirq,u64addr){
--
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
From: David Gibson <hidden> Date: 2019-02-04 06:29:51
On Sat, Jan 26, 2019 at 09:25:04AM +0100, Cédric Le Goater wrote:
Was there a crashing.org shutdown ?
Received: from gate.crashing.org (gate.crashing.org [63.228.1.57])
by in5.mail.ovh.net (Postfix) with ESMTPS id 43mYnj0nrlz1N7KC
for [off-list ref]; Fri, 25 Jan 2019 22:38:00 +0000 (UTC)
Received: from localhost (localhost.localdomain [127.0.0.1])
by gate.crashing.org (8.14.1/8.14.1) with ESMTP id x0NLZf4K021092;
Wed, 23 Jan 2019 15:35:43 -0600
On 1/23/19 10:35 PM, Benjamin Herrenschmidt wrote:
quoted
On Wed, 2019-01-23 at 20:07 +0100, Cédric Le Goater wrote:
quoted
Event Assignment Structure, a.k.a IVE (Interrupt Virtualization Entry)
All the names changed somewhere between XIVE v1 and XIVE v2. OPAL and
Linux should be adjusted ...
All the names changed between the HW design and the "architecture"
document. The HW guys use the old names, the architecture the new
names, and Linux & OPAL mostly use the old ones because frankly the new
names suck big time.
Well, It does not make XIVE any clearer ... I did prefer the v1 names
but there was some naming overlap in the concepts.
quoted
quoted
It would be good to talk a little about the nested support (offline
may be) to make sure that we are not missing some major interface that
would require a lot of change. If we need to prepare ground, I think
the timing is good.
The size of the IRQ number space might be a problem. It seems we
would need to increase it considerably to support multiple nested
guests. That said I haven't look much how nested is designed.
The size of the VP space is a bigger concern. Even today. We really
need qemu to tell the max #cpu to KVM so we can allocate less of them.
ah yes. we would also need to reduce the number of available priorities
per CPU to have more EQ descriptors available if I recall well.
quoted
As for nesting, I suggest for the foreseeable future we stick to XICS
emulation in nested guests.
ok. so no kernel_irqchip at all. hmm.
That would certainly be step 0, making sure the capability advertises
this correctly. I think we do want to make XICs-on-XIVE emulation
work in a KVM L1 (so we'd need to have it make XIVE hcalls to the L0
instead of OPAL calls).
XIVE-on-XIVE for L1 would be nice too, which would mean implementing
the XIVE hcalls from the L2 in terms of XIVE hcalls to the L0. I
think it's ok to delay this indefinitely as long as the caps advertise
correctly so that qemu will use userspace emulation until its ready.
I was wondering how possible it was to have L2 initialize the underlying
OPAL structures in the L0 hypervisor. May be with a sort of proxy hcall
which would perform the initialization in QEMU L1 on behalf of L2.
--
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
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-04 10:18:16
On 2/4/19 1:50 AM, David Gibson wrote:
On Wed, Jan 23, 2019 at 05:24:13PM +0100, Cédric Le Goater wrote:
quoted
On 1/22/19 5:56 AM, Paul Mackerras wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:15PM +0100, Cédric Le Goater wrote:
quoted
We will have different KVM devices for interrupts, one for the
XICS-over-XIVE mode and one for the XIVE native exploitation
mode. Let's add some checks to make sure we are not mixing the
interfaces in KVM.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/kvm/book3s_xive.c | 6 ++++++
1 file changed, 6 insertions(+)
@@ -835,6 +838,9 @@ int kvmppc_xive_set_icp(struct kvm_vcpu *vcpu, u64 icpval)u8cppr,mfrr;u32xisr;+if(!kvmppc_xics_enabled(vcpu))+return-EPERM;+if(!xc||!xive)return-ENOENT;
I can't see how these new checks could ever trigger in the code as it
stands. Is there a way at present?
It would require some custom QEMU doing silly things : create the XICS
KVM device, and then call kvm_get_one_reg(KVM_REG_PPC_ICP_STATE) or
kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE) without connecting the
vCPU to its presenter.
Today, you get a ENOENT.
TBH, ENOENT seems fine to me.
quoted
quoted
Do following patches ever add a path where the new checks could trigger,
or is this just an excess of caution?
With the following patches, QEMU could to do something even more silly,
which is to mix the interrupt mode interfaces : create a KVM XICS device
and call KVM CPU ioctls of the KVM XIVE device, or the opposite.
AFAICT, like above, that won't really differ from calling the XIVE CPU
ioctl()s when no irqchip is set up at all, and should be covered by
just a !xive check.
we can drop that patch. It does not bring much.
Thanks,
C.
quoted
quoted
(Your patch description should ideally have answered these questions > for me.)
Yes. I also think that I introduced this patch to early in the series.
It make more sense when the XICS and the XIVE KVM devices are available.
Thanks,
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-04 11:29:15
On 2/4/19 5:25 AM, David Gibson wrote:
On Mon, Jan 07, 2019 at 07:43:17PM +0100, Cédric Le Goater wrote:
quoted
This is the basic framework for the new KVM device supporting the XIVE
native exploitation mode. The user interface exposes a new capability
and a new KVM device to be used by QEMU.
Internally, the interface to the new KVM device is protected with a
new interrupt mode: KVMPPC_IRQ_XIVE.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/asm/kvm_host.h | 2 +
arch/powerpc/include/asm/kvm_ppc.h | 21 ++
arch/powerpc/kvm/book3s_xive.h | 3 +
include/uapi/linux/kvm.h | 3 +
arch/powerpc/kvm/book3s.c | 7 +-
arch/powerpc/kvm/book3s_xive_native.c | 332 ++++++++++++++++++++++++++
arch/powerpc/kvm/powerpc.c | 30 +++
arch/powerpc/kvm/Makefile | 2 +-
8 files changed, 398 insertions(+), 2 deletions(-)
create mode 100644 arch/powerpc/kvm/book3s_xive_native.c
@@ -0,0 +1,332 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Copyright(c)2017-2019,IBMCorporation.+*/++#define pr_fmt(fmt) "xive-kvm: " fmt++#include<linux/anon_inodes.h>+#include<linux/kernel.h>+#include<linux/kvm_host.h>+#include<linux/err.h>+#include<linux/gfp.h>+#include<linux/spinlock.h>+#include<linux/delay.h>+#include<linux/percpu.h>+#include<linux/cpumask.h>+#include<asm/uaccess.h>+#include<asm/kvm_book3s.h>+#include<asm/kvm_ppc.h>+#include<asm/hvcall.h>+#include<asm/xics.h>+#include<asm/xive.h>+#include<asm/xive-regs.h>+#include<asm/debug.h>+#include<asm/debugfs.h>+#include<asm/time.h>+#include<asm/opal.h>++#include<linux/debugfs.h>+#include<linux/seq_file.h>++#include"book3s_xive.h"++staticvoidxive_native_cleanup_queue(structkvm_vcpu*vcpu,intprio)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+structxive_q*q=&xc->queues[prio];++xive_native_disable_queue(xc->vp_id,q,prio);+if(q->qpage){+put_page(virt_to_page(q->qpage));+q->qpage=NULL;+}+}++voidkvmppc_xive_native_cleanup_vcpu(structkvm_vcpu*vcpu)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+inti;++if(!kvmppc_xive_enabled(vcpu))+return;++if(!xc)+return;++pr_devel("native_cleanup_vcpu(cpu=%d)\n",xc->server_num);++/* Ensure no interrupt is still routed to that VP */+xc->valid=false;+kvmppc_xive_disable_vcpu_interrupts(vcpu);++/* Disable the VP */+xive_native_disable_vp(xc->vp_id);++/* Free the queues & associated interrupts */+for(i=0;i<KVMPPC_XIVE_Q_COUNT;i++){+/* Free the escalation irq */+if(xc->esc_virq[i]){+free_irq(xc->esc_virq[i],vcpu);+irq_dispose_mapping(xc->esc_virq[i]);+kfree(xc->esc_virq_names[i]);+xc->esc_virq[i]=0;+}++/* Free the queue */+xive_native_cleanup_queue(vcpu,i);+}++/* Free the VP */+kfree(xc);++/* Cleanup the vcpu */+vcpu->arch.irq_type=KVMPPC_IRQ_DEFAULT;+vcpu->arch.xive_vcpu=NULL;+}++intkvmppc_xive_native_connect_vcpu(structkvm_device*dev,+structkvm_vcpu*vcpu,u32cpu)
Why do we need both a *vcpu and a cpu number as an integer?
To be in sync with the other similar routines : kvmppc_xics_connect_vcpu()
and kvmppc_xive_connect_vcpu().
But if we consider that this 'cpu' parameter is always in sync with
vcpu->vcpu_id, we could remove it from the KVM ioctl call I suppose.
Should we do the same for the other routines ?
quoted
+{+ struct kvmppc_xive *xive = dev->private;+ struct kvmppc_xive_vcpu *xc;+ int rc;++ pr_devel("native_connect_vcpu(cpu=%d)\n", cpu);++ if (dev->ops != &kvm_xive_native_ops) {+ pr_devel("Wrong ops !\n");+ return -EPERM;+ }+ if (xive->kvm != vcpu->kvm)+ return -EPERM;+ if (vcpu->arch.irq_type)
Please use an explicit == / != here so we don't have to remember which
symbolic value corresponds to 0.
ok. I agree.
Thanks,
C.
quoted
+ return -EBUSY;+ if (kvmppc_xive_find_server(vcpu->kvm, cpu)) {+ pr_devel("Duplicate !\n");+ return -EEXIST;+ }+ if (cpu >= KVM_MAX_VCPUS) {+ pr_devel("Out of bounds !\n");+ return -EINVAL;+ }+ xc = kzalloc(sizeof(*xc), GFP_KERNEL);+ if (!xc)+ return -ENOMEM;++ mutex_lock(&vcpu->kvm->lock);+ vcpu->arch.xive_vcpu = xc;+ xc->xive = xive;+ xc->vcpu = vcpu;+ xc->server_num = cpu;+ xc->vp_id = xive->vp_base + cpu;+ xc->valid = true;++ rc = xive_native_get_vp_info(xc->vp_id, &xc->vp_cam, &xc->vp_chip_id);+ if (rc) {+ pr_err("Failed to get VP info from OPAL: %d\n", rc);+ goto bail;+ }++ /*+ * Enable the VP first as the single escalation mode will+ * affect escalation interrupts numbering+ */+ rc = xive_native_enable_vp(xc->vp_id, xive->single_escalation);+ if (rc) {+ pr_err("Failed to enable VP in OPAL: %d\n", rc);+ goto bail;+ }++ /* Configure VCPU fields for use by assembly push/pull */+ vcpu->arch.xive_saved_state.w01 = cpu_to_be64(0xff000000);+ vcpu->arch.xive_cam_word = cpu_to_be32(xc->vp_cam | TM_QW1W2_VO);++ /* TODO: initialize queues ? */++bail:+ vcpu->arch.irq_type = KVMPPC_IRQ_XIVE;+ mutex_unlock(&vcpu->kvm->lock);+ if (rc)+ kvmppc_xive_native_cleanup_vcpu(vcpu);++ return rc;+}++static int kvmppc_xive_native_set_attr(struct kvm_device *dev,+ struct kvm_device_attr *attr)+{+ return -ENXIO;+}++static int kvmppc_xive_native_get_attr(struct kvm_device *dev,+ struct kvm_device_attr *attr)+{+ return -ENXIO;+}++static int kvmppc_xive_native_has_attr(struct kvm_device *dev,+ struct kvm_device_attr *attr)+{+ return -ENXIO;+}++static void kvmppc_xive_native_free(struct kvm_device *dev)+{+ struct kvmppc_xive *xive = dev->private;+ struct kvm *kvm = xive->kvm;+ int i;++ debugfs_remove(xive->dentry);++ pr_devel("Destroying xive native for partition\n");++ if (kvm)+ kvm->arch.xive = NULL;++ /* Mask and free interrupts */+ for (i = 0; i <= xive->max_sbid; i++) {+ if (xive->src_blocks[i])+ kvmppc_xive_free_sources(xive->src_blocks[i]);+ kfree(xive->src_blocks[i]);+ xive->src_blocks[i] = NULL;+ }++ if (xive->vp_base != XIVE_INVALID_VP)+ xive_native_free_vp_block(xive->vp_base);++ kfree(xive);+ kfree(dev);+}++static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)+{+ struct kvmppc_xive *xive;+ struct kvm *kvm = dev->kvm;+ int ret = 0;++ pr_devel("Creating xive native for partition\n");++ if (kvm->arch.xive)+ return -EEXIST;++ xive = kzalloc(sizeof(*xive), GFP_KERNEL);+ if (!xive)+ return -ENOMEM;++ dev->private = xive;+ xive->dev = dev;+ xive->kvm = kvm;+ kvm->arch.xive = xive;++ /* We use the default queue size set by the host */+ xive->q_order = xive_native_default_eq_shift();+ if (xive->q_order < PAGE_SHIFT)+ xive->q_page_order = 0;+ else+ xive->q_page_order = xive->q_order - PAGE_SHIFT;++ /* Allocate a bunch of VPs */+ xive->vp_base = xive_native_alloc_vp_block(KVM_MAX_VCPUS);+ pr_devel("VP_Base=%x\n", xive->vp_base);++ if (xive->vp_base == XIVE_INVALID_VP)+ ret = -ENOMEM;++ xive->single_escalation = xive_native_has_single_escalation();++ if (ret)+ kfree(xive);++ return ret;+}++static int xive_native_debug_show(struct seq_file *m, void *private)+{+ struct kvmppc_xive *xive = m->private;+ struct kvm *kvm = xive->kvm;+ struct kvm_vcpu *vcpu;+ unsigned int i;++ if (!kvm)+ return 0;++ seq_puts(m, "=========\nVCPU state\n=========\n");++ kvm_for_each_vcpu(i, vcpu, kvm) {+ struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;++ if (!xc)+ continue;++ seq_printf(m, "cpu server %#x NSR=%02x CPPR=%02x IBP=%02x PIPR=%02x w01=%016llx w2=%08x\n",+ xc->server_num,+ vcpu->arch.xive_saved_state.nsr,+ vcpu->arch.xive_saved_state.cppr,+ vcpu->arch.xive_saved_state.ipb,+ vcpu->arch.xive_saved_state.pipr,+ vcpu->arch.xive_saved_state.w01,+ (u32) vcpu->arch.xive_cam_word);++ kvmppc_xive_debug_show_queues(m, vcpu);+ }++ return 0;+}++static int xive_native_debug_open(struct inode *inode, struct file *file)+{+ return single_open(file, xive_native_debug_show, inode->i_private);+}++static const struct file_operations xive_native_debug_fops = {+ .open = xive_native_debug_open,+ .read = seq_read,+ .llseek = seq_lseek,+ .release = single_release,+};++static void xive_native_debugfs_init(struct kvmppc_xive *xive)+{+ char *name;++ name = kasprintf(GFP_KERNEL, "kvm-xive-%p", xive);+ if (!name) {+ pr_err("%s: no memory for name\n", __func__);+ return;+ }++ xive->dentry = debugfs_create_file(name, 0444, powerpc_debugfs_root,+ xive, &xive_native_debug_fops);++ pr_debug("%s: created %s\n", __func__, name);+ kfree(name);+}++static void kvmppc_xive_native_init(struct kvm_device *dev)+{+ struct kvmppc_xive *xive = (struct kvmppc_xive *)dev->private;++ /* Register some debug interfaces */+ xive_native_debugfs_init(xive);+}++struct kvm_device_ops kvm_xive_native_ops = {+ .name = "kvm-xive-native",+ .create = kvmppc_xive_native_create,+ .init = kvmppc_xive_native_init,+ .destroy = kvmppc_xive_native_free,+ .set_attr = kvmppc_xive_native_set_attr,+ .get_attr = kvmppc_xive_native_get_attr,+ .has_attr = kvmppc_xive_native_has_attr,+};++void kvmppc_xive_native_init_module(void)+{+ ;+}++void kvmppc_xive_native_exit_module(void)+{+ ;+}
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-04 11:32:29
On 2/4/19 5:45 AM, David Gibson wrote:
On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
quoted
This will let the guest create a memory mapping to expose the ESB MMIO
regions used to control the interrupt sources, to trigger events, to
EOI or to turn off the sources.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 4 ++
arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Introducing a new FD for ESB and TIMA seems overkill. Can't you get
to both with an mmap() directly on the xive device fd? Using the
offset to distinguish which one to map, obviously.
The page offset would define some sort of user API. It seems feasible.
But I am not sure this would be practical in the future if we need to
tune the length.
The TIMA has two pages that can be exposed at guest level for interrupt
management : the OS and the USER page. That should be OK.
But we might want to map only portions of the interrupt ESB space, for
PCI passthrough for instance as Paul proposed. I am still looking at that.
Thanks,
C.
@@ -153,6 +153,85 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,returnrc;}+staticintxive_native_esb_fault(structvm_fault*vmf)+{+structvm_area_struct*vma=vmf->vma;+structkvmppc_xive*xive=vma->vm_file->private_data;+structkvmppc_xive_src_block*sb;+structkvmppc_xive_irq_state*state;+structxive_irq_data*xd;+u32hw_num;+u16src;+u64page;+unsignedlongirq;++/*+*Linux/KVMusesatwopagesESBsetting,onefortriggerand+*oneforEOI+*/+irq=vmf->pgoff/2;++sb=kvmppc_xive_find_source(xive,irq,&src);+if(!sb){+pr_err("%s: source %lx not found !\n",__func__,irq);+returnVM_FAULT_SIGBUS;+}++state=&sb->irq_state[src];+kvmppc_xive_select_irq(state,&hw_num,&xd);++arch_spin_lock(&sb->lock);++/*+*first/evenpageisfortrigger+*second/oddpageisforEOIandmanagement.+*/+page=vmf->pgoff%2?xd->eoi_page:xd->trig_page;+arch_spin_unlock(&sb->lock);++if(!page){+pr_err("%s: acessing invalid ESB page for source %lx !\n",+__func__,irq);+returnVM_FAULT_SIGBUS;+}++vmf_insert_pfn(vma,vmf->address,page>>PAGE_SHIFT);+returnVM_FAULT_NOPAGE;+}++staticconststructvm_operations_structxive_native_esb_vmops={+.fault=xive_native_esb_fault,+};++staticintxive_native_esb_mmap(structfile*file,structvm_area_struct*vma)+{+/* There are two ESB pages (trigger and EOI) per IRQ */+if(vma_pages(vma)+vma->vm_pgoff>KVMPPC_XIVE_NR_IRQS*2)+return-EINVAL;++vma->vm_flags|=VM_IO|VM_PFNMAP;+vma->vm_page_prot=pgprot_noncached(vma->vm_page_prot);+vma->vm_ops=&xive_native_esb_vmops;+return0;+}++staticconststructfile_operationsxive_native_esb_fops={+.mmap=xive_native_esb_mmap,+};++staticintkvmppc_xive_native_get_esb_fd(structkvmppc_xive*xive,u64addr)+{+u64__user*ubufp=(u64__user*)addr;+intret;++ret=anon_inode_getfd("[xive-esb]",&xive_native_esb_fops,xive,+O_RDWR|O_CLOEXEC);+if(ret<0)+returnret;++returnput_user(ret,ubufp);+}+staticintkvmppc_xive_native_set_attr(structkvm_device*dev,structkvm_device_attr*attr){
@@ -162,12 +241,30 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,staticintkvmppc_xive_native_get_attr(structkvm_device*dev,structkvm_device_attr*attr){+structkvmppc_xive*xive=dev->private;++switch(attr->group){+caseKVM_DEV_XIVE_GRP_CTRL:+switch(attr->attr){+caseKVM_DEV_XIVE_GET_ESB_FD:+returnkvmppc_xive_native_get_esb_fd(xive,attr->addr);+}+break;+}return-ENXIO;}staticintkvmppc_xive_native_has_attr(structkvm_device*dev,structkvm_device_attr*attr){+switch(attr->group){+caseKVM_DEV_XIVE_GRP_CTRL:+switch(attr->attr){+caseKVM_DEV_XIVE_GET_ESB_FD:+return0;+}+break;+}return-ENXIO;}
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-04 15:38:14
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:
quoted
On 1/22/19 6:14 AM, Paul Mackerras wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:20PM +0100, Cédric Le Goater wrote:
quoted
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.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-04 15:52:37
On 2/4/19 6:18 AM, David Gibson wrote:
On Mon, Jan 07, 2019 at 07:43:26PM +0100, Cédric Le Goater wrote:
quoted
When the VM is stopped in a migration sequence, the sources are masked
and the XIVE IC is synced to stabilize the EQs. When done, the KVM
ioctl KVM_DEV_XIVE_SAVE_EQ_PAGES is called to mark dirty the EQ pages.
The migration can then transfer the remaining dirty pages to the
destination and start collecting the state of the devices.
Is there a reason to make this a separate step from the SYNC
operation?
Hmm, apart from letting QEMU orchestrate the migration step by step, no.
We could merge the SYNC and the SAVE_EQ_PAGES in a single KVM operation.
I think that should be fine.
However, it does not make sense to call this operation without the VM
being stopped. I wonder how this can checked from KVM. May be we can't.
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-04 16:14:57
On 2/4/19 6:15 AM, David Gibson wrote:
On Mon, Jan 07, 2019 at 07:43:24PM +0100, Cédric Le Goater wrote:
quoted
The guest physical address of the event queue will be part of the
state to transfer in the migration. Cache its value when the queue is
configured, it will save us an OPAL call.
That doesn't sound like a very compelling case - migration is already
a hundreds of milliseconds type operation, I wouldn't expect a few
extra OPAL calls to be an issue.
OK. I don't think this is much a problem anyhow. Let's call OPAL.
C.
@@ -74,6 +74,8 @@ struct xive_q {u32esc_irq;atomic_tcount;atomic_tpending_count;+u64guest_qpage;+u32guest_qsize;};/* Global enable flags for the XIVE support */
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-04 16:18:45
On 2/4/19 6:21 AM, David Gibson wrote:
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
But we don't have an equivalent name in the PAPR specs for the tuple
(prio, server). We could use the generic 'target' name may be ? even
if this is usually referring to a CPU number.
Or, IVE (Interrupt Vector Entry) ? which makes some sense.
This is was the former name in HW. I think we recycle it for KVM.
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-04 18:59:13
On 2/4/19 6:26 AM, David Gibson wrote:
On Mon, Jan 07, 2019 at 08:10:04PM +0100, Cédric Le Goater wrote:
quoted
At a VCPU level, the state of the thread context interrupt management
registers needs to be collected. These registers are cached under the
'xive_saved_state.w01' field of the VCPU when the VPCU context is
pulled from the HW thread. An OPAL call retrieves the backup of the
IPB register in the NVT structure and merges it in the KVM state.
The structures of the interface between QEMU and KVM provisions some
extra room (two u64) for further extensions if more state needs to be
transferred back to QEMU.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/asm/kvm_ppc.h | 5 ++
arch/powerpc/include/uapi/asm/kvm.h | 2 +
arch/powerpc/kvm/book3s.c | 24 +++++++++
arch/powerpc/kvm/book3s_xive_native.c | 78 +++++++++++++++++++++++++++
4 files changed, 109 insertions(+)
IIRC "VP" is the old name for "TCTX". Since we're using tctx in the
rest of the XIVE code, can we use it here as well.
OK. The state we are getting or setting is indeed related to the thread
interrupt context registers.
The name VP is related to an identifier to some interrupt context under
OPAL (NVT in HW to be precise).
C.
@@ -424,6 +424,84 @@ static int xive_native_validate_queue_size(u32 qsize)}}+#define TM_IPB_SHIFT 40+#define TM_IPB_MASK (((u64) 0xFF) << TM_IPB_SHIFT)++intkvmppc_xive_native_get_vp(structkvm_vcpu*vcpu,unionkvmppc_one_reg*val)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+u64opal_state;+intrc;++if(!kvmppc_xive_enabled(vcpu))+return-EPERM;++if(!xc)+return-ENOENT;++/* Thread context registers. We only care about IPB and CPPR */+val->xive_timaval[0]=vcpu->arch.xive_saved_state.w01;++/*+*ReturntheOSCAMlinetoprintouttheVPidentifierin+*theQEMUmonitor.Thisisnotrestored.+*/+val->xive_timaval[1]=vcpu->arch.xive_cam_word;++/* Get the VP state from OPAL */+rc=xive_native_get_vp_state(xc->vp_id,&opal_state);+if(rc)+returnrc;++/*+*CapturethebackupofIPBregisterintheNVTstructureand+*mergeitinourKVMVPstate.+*+*TODO:P10support.+*/+val->xive_timaval[0]|=cpu_to_be64(opal_state&TM_IPB_MASK);++pr_devel("%s NSR=%02x CPPR=%02x IBP=%02x PIPR=%02x w01=%016llx w2=%08x opal=%016llx\n",+__func__,+vcpu->arch.xive_saved_state.nsr,+vcpu->arch.xive_saved_state.cppr,+vcpu->arch.xive_saved_state.ipb,+vcpu->arch.xive_saved_state.pipr,+vcpu->arch.xive_saved_state.w01,+(u32)vcpu->arch.xive_cam_word,opal_state);++return0;+}++intkvmppc_xive_native_set_vp(structkvm_vcpu*vcpu,unionkvmppc_one_reg*val)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+structkvmppc_xive*xive=vcpu->kvm->arch.xive;++pr_devel("%s w01=%016llx vp=%016llx\n",__func__,+val->xive_timaval[0],val->xive_timaval[1]);++if(!kvmppc_xive_enabled(vcpu))+return-EPERM;++if(!xc||!xive)+return-ENOENT;++/* We can't update the state of a "pushed" VCPU */+if(WARN_ON(vcpu->arch.xive_pushed))+return-EIO;++/* Thread context registers. only restore IPB and CPPR ? */+vcpu->arch.xive_saved_state.w01=val->xive_timaval[0];++/*+*ThereisnoneedtorestoretheXIVEinternalstate(IPB+*storedintheNVT)astheIPBregisterwasmergedinKVMVP+*state.+*/+return0;+}+staticintkvmppc_xive_native_set_source(structkvmppc_xive*xive,longirq,u64addr){
+ sb = kvmppc_xive_create_src_block(xive, irq);+ if (!sb) {+ pr_err("Failed to create block...\n");+ return -ENOMEM;+ }+ }+ state = &sb->irq_state[idx];++ if (get_user(val, ubufp)) {+ pr_err("fault getting user info !\n");+ return -EFAULT;+ }++ /*+ * If the source doesn't already have an IPI, allocate+ * one and get the corresponding data+ */+ if (!state->ipi_number) {+ state->ipi_number = xive_native_alloc_irq();+ if (state->ipi_number == 0) {+ pr_err("Failed to allocate IRQ !\n");+ return -ENOMEM;+ }
Am I right in thinking this is the point at which a specific guest irq
number gets bound to a specific host irq number?
yes. the XIVE IRQ state caches this information and 'state' should be
protected before being assigned, indeed ... The XICS-over-XIVE device
also has the same race issue.
It's not showing because where initializing the KVM device sequentially
from QEMU and only once.
Thanks,
C.
quoted
+ xive_native_populate_irq_data(state->ipi_number,+ &state->ipi_data);+ pr_debug("%s allocated hw_irq=0x%x for irq=0x%lx\n", __func__,+ state->ipi_number, irq);+ }++ arch_spin_lock(&sb->lock);++ /* Restore LSI state */+ if (val & KVM_XIVE_LEVEL_SENSITIVE) {+ state->lsi = true;+ if (val & KVM_XIVE_LEVEL_ASSERTED)+ state->asserted = true;+ pr_devel(" LSI ! Asserted=%d\n", state->asserted);+ }++ /* Mask IRQ to start with */+ state->act_server = 0;+ state->act_priority = MASKED;+ xive_vm_esb_load(&state->ipi_data, XIVE_ESB_SET_PQ_01);+ xive_native_configure_irq(state->ipi_number, 0, MASKED, 0);++ /* Increment the number of valid sources and mark this one valid */+ if (!state->valid)+ xive->src_count++;+ state->valid = true;++ arch_spin_unlock(&sb->lock);++ return 0;+}+ static int kvmppc_xive_native_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) {
@@ -0,0 +1,27 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Copyright(c)2017-2019,IBMCorporation.+*/++/* File to be included by other .c files */++#define XGLUE(a, b) a##b+#define GLUE(a, b) XGLUE(a, b)++/*+*TODO:introduceacommontemplatefilewiththeXIVEnativelayer+*andtheXICS-on-XIVEgluefortheutilityfunctions+*/+staticu8GLUE(X_PFX,esb_load)(structxive_irq_data*xd,u32offset)+{+u64val;++if(xd->flags&XIVE_IRQ_FLAG_SHIFT_BUG)+offset|=offset<<4;++val=__x_readq(__x_eoi_page(xd)+offset);+#ifdef __LITTLE_ENDIAN__+val>>=64-8;+#endif+return(u8)val;+}
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-04 22:49:26
On 2/4/19 6:17 AM, David Gibson wrote:
On Mon, Jan 07, 2019 at 07:43:25PM +0100, Cédric Le Goater wrote:
quoted
When migration of a VM is initiated, a first copy of the RAM is
transferred to the destination before the VM is stopped. At that time,
QEMU needs to perform a XIVE quiesce sequence to stop the flow of
event notifications and stabilize the EQs. The sources are masked and
the XIVE IC is synced with the KVM ioctl KVM_DEV_XIVE_GRP_SYNC.
Don't you also need to make sure the guests queue pages are marked
dirty here, in case they were already migrated?
I have added an extra KVM service to mark the EQ pages dirty. That
might be overkill as it seems you are suggesting.
C.
From: David Gibson <hidden> Date: 2019-02-05 05:40:19
On Mon, Feb 04, 2019 at 08:07:20PM +0100, Cédric Le Goater wrote:
On 2/4/19 5:57 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:21PM +0100, Cédric Le Goater wrote:
[snip]
quoted
quoted
+ sb = kvmppc_xive_create_src_block(xive, irq);+ if (!sb) {+ pr_err("Failed to create block...\n");+ return -ENOMEM;+ }+ }+ state = &sb->irq_state[idx];++ if (get_user(val, ubufp)) {+ pr_err("fault getting user info !\n");+ return -EFAULT;+ }++ /*+ * If the source doesn't already have an IPI, allocate+ * one and get the corresponding data+ */+ if (!state->ipi_number) {+ state->ipi_number = xive_native_alloc_irq();+ if (state->ipi_number == 0) {+ pr_err("Failed to allocate IRQ !\n");+ return -ENOMEM;+ }
Am I right in thinking this is the point at which a specific guest irq
number gets bound to a specific host irq number?
yes. the XIVE IRQ state caches this information and 'state' should be
protected before being assigned, indeed ... The XICS-over-XIVE device
also has the same race issue.
It's not showing because where initializing the KVM device sequentially
from QEMU and only once.
Ok.
So, for the passthrough case, what's the point at which we know that a
particular guest interrupt needs to be bound to a specific real
hardware interrupt, rather than a generic IPI?
--
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
From: David Gibson <hidden> Date: 2019-02-05 05:41:48
On Mon, Feb 04, 2019 at 07:57:26PM +0100, Cédric Le Goater wrote:
On 2/4/19 6:26 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 08:10:04PM +0100, Cédric Le Goater wrote:
quoted
At a VCPU level, the state of the thread context interrupt management
registers needs to be collected. These registers are cached under the
'xive_saved_state.w01' field of the VCPU when the VPCU context is
pulled from the HW thread. An OPAL call retrieves the backup of the
IPB register in the NVT structure and merges it in the KVM state.
The structures of the interface between QEMU and KVM provisions some
extra room (two u64) for further extensions if more state needs to be
transferred back to QEMU.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/asm/kvm_ppc.h | 5 ++
arch/powerpc/include/uapi/asm/kvm.h | 2 +
arch/powerpc/kvm/book3s.c | 24 +++++++++
arch/powerpc/kvm/book3s_xive_native.c | 78 +++++++++++++++++++++++++++
4 files changed, 109 insertions(+)
IIRC "VP" is the old name for "TCTX". Since we're using tctx in the
rest of the XIVE code, can we use it here as well.
OK. The state we are getting or setting is indeed related to the thread
interrupt context registers.
The name VP is related to an identifier to some interrupt context under
OPAL (NVT in HW to be precise).
Oh, sorry, "NVT" was the name I was looking for, not "TCTX". But in
any case, please lets standardize on one.
--
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
From: David Gibson <hidden> Date: 2019-02-05 05:43:15
On Mon, Feb 04, 2019 at 12:30:39PM +0100, Cédric Le Goater wrote:
On 2/4/19 5:45 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
quoted
This will let the guest create a memory mapping to expose the ESB MMIO
regions used to control the interrupt sources, to trigger events, to
EOI or to turn off the sources.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 4 ++
arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Introducing a new FD for ESB and TIMA seems overkill. Can't you get
to both with an mmap() directly on the xive device fd? Using the
offset to distinguish which one to map, obviously.
The page offset would define some sort of user API. It seems feasible.
But I am not sure this would be practical in the future if we need to
tune the length.
Um.. why not? I mean, yes the XIVE supports rather a lot of
interrupts, but we have 64-bits of offset we can play with - we can
leave room for billions of ESB slots and still have room for billions
of VPs.
The TIMA has two pages that can be exposed at guest level for interrupt
management : the OS and the USER page. That should be OK.
But we might want to map only portions of the interrupt ESB space, for
PCI passthrough for instance as Paul proposed. I am still looking at that.
Thanks,
C.
@@ -153,6 +153,85 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,returnrc;}+staticintxive_native_esb_fault(structvm_fault*vmf)+{+structvm_area_struct*vma=vmf->vma;+structkvmppc_xive*xive=vma->vm_file->private_data;+structkvmppc_xive_src_block*sb;+structkvmppc_xive_irq_state*state;+structxive_irq_data*xd;+u32hw_num;+u16src;+u64page;+unsignedlongirq;++/*+*Linux/KVMusesatwopagesESBsetting,onefortriggerand+*oneforEOI+*/+irq=vmf->pgoff/2;++sb=kvmppc_xive_find_source(xive,irq,&src);+if(!sb){+pr_err("%s: source %lx not found !\n",__func__,irq);+returnVM_FAULT_SIGBUS;+}++state=&sb->irq_state[src];+kvmppc_xive_select_irq(state,&hw_num,&xd);++arch_spin_lock(&sb->lock);++/*+*first/evenpageisfortrigger+*second/oddpageisforEOIandmanagement.+*/+page=vmf->pgoff%2?xd->eoi_page:xd->trig_page;+arch_spin_unlock(&sb->lock);++if(!page){+pr_err("%s: acessing invalid ESB page for source %lx !\n",+__func__,irq);+returnVM_FAULT_SIGBUS;+}++vmf_insert_pfn(vma,vmf->address,page>>PAGE_SHIFT);+returnVM_FAULT_NOPAGE;+}++staticconststructvm_operations_structxive_native_esb_vmops={+.fault=xive_native_esb_fault,+};++staticintxive_native_esb_mmap(structfile*file,structvm_area_struct*vma)+{+/* There are two ESB pages (trigger and EOI) per IRQ */+if(vma_pages(vma)+vma->vm_pgoff>KVMPPC_XIVE_NR_IRQS*2)+return-EINVAL;++vma->vm_flags|=VM_IO|VM_PFNMAP;+vma->vm_page_prot=pgprot_noncached(vma->vm_page_prot);+vma->vm_ops=&xive_native_esb_vmops;+return0;+}++staticconststructfile_operationsxive_native_esb_fops={+.mmap=xive_native_esb_mmap,+};++staticintkvmppc_xive_native_get_esb_fd(structkvmppc_xive*xive,u64addr)+{+u64__user*ubufp=(u64__user*)addr;+intret;++ret=anon_inode_getfd("[xive-esb]",&xive_native_esb_fops,xive,+O_RDWR|O_CLOEXEC);+if(ret<0)+returnret;++returnput_user(ret,ubufp);+}+staticintkvmppc_xive_native_set_attr(structkvm_device*dev,structkvm_device_attr*attr){
@@ -162,12 +241,30 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,staticintkvmppc_xive_native_get_attr(structkvm_device*dev,structkvm_device_attr*attr){+structkvmppc_xive*xive=dev->private;++switch(attr->group){+caseKVM_DEV_XIVE_GRP_CTRL:+switch(attr->attr){+caseKVM_DEV_XIVE_GET_ESB_FD:+returnkvmppc_xive_native_get_esb_fd(xive,attr->addr);+}+break;+}return-ENXIO;}staticintkvmppc_xive_native_has_attr(structkvm_device*dev,structkvm_device_attr*attr){+switch(attr->group){+caseKVM_DEV_XIVE_GRP_CTRL:+switch(attr->attr){+caseKVM_DEV_XIVE_GET_ESB_FD:+return0;+}+break;+}return-ENXIO;}
--
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
From: David Gibson <hidden> Date: 2019-02-05 05:44:41
On Mon, Feb 04, 2019 at 04:46:00PM +0100, Cédric Le Goater wrote:
On 2/4/19 6:18 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:26PM +0100, Cédric Le Goater wrote:
quoted
When the VM is stopped in a migration sequence, the sources are masked
and the XIVE IC is synced to stabilize the EQs. When done, the KVM
ioctl KVM_DEV_XIVE_SAVE_EQ_PAGES is called to mark dirty the EQ pages.
The migration can then transfer the remaining dirty pages to the
destination and start collecting the state of the devices.
Is there a reason to make this a separate step from the SYNC
operation?
Hmm, apart from letting QEMU orchestrate the migration step by step, no.
We could merge the SYNC and the SAVE_EQ_PAGES in a single KVM operation.
I think that should be fine.
I think that makes sense. SYNC is supposed to complete delivery of
any in-flight interrupts, and to me writing to the queue page and
marking it dirty as a result is a logical part of that.
However, it does not make sense to call this operation without the VM
being stopped. I wonder how this can checked from KVM. May be we
can't.
I don't think it matters. qemu is allowed to shoot itself in the
foot.
--
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
From: David Gibson <hidden> Date: 2019-02-05 05:46:09
On Mon, Feb 04, 2019 at 12:19:07PM +0100, Cédric Le Goater wrote:
On 2/4/19 5:25 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:17PM +0100, Cédric Le Goater wrote:
quoted
This is the basic framework for the new KVM device supporting the XIVE
native exploitation mode. The user interface exposes a new capability
and a new KVM device to be used by QEMU.
Internally, the interface to the new KVM device is protected with a
new interrupt mode: KVMPPC_IRQ_XIVE.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/asm/kvm_host.h | 2 +
arch/powerpc/include/asm/kvm_ppc.h | 21 ++
arch/powerpc/kvm/book3s_xive.h | 3 +
include/uapi/linux/kvm.h | 3 +
arch/powerpc/kvm/book3s.c | 7 +-
arch/powerpc/kvm/book3s_xive_native.c | 332 ++++++++++++++++++++++++++
arch/powerpc/kvm/powerpc.c | 30 +++
arch/powerpc/kvm/Makefile | 2 +-
8 files changed, 398 insertions(+), 2 deletions(-)
create mode 100644 arch/powerpc/kvm/book3s_xive_native.c
@@ -0,0 +1,332 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Copyright(c)2017-2019,IBMCorporation.+*/++#define pr_fmt(fmt) "xive-kvm: " fmt++#include<linux/anon_inodes.h>+#include<linux/kernel.h>+#include<linux/kvm_host.h>+#include<linux/err.h>+#include<linux/gfp.h>+#include<linux/spinlock.h>+#include<linux/delay.h>+#include<linux/percpu.h>+#include<linux/cpumask.h>+#include<asm/uaccess.h>+#include<asm/kvm_book3s.h>+#include<asm/kvm_ppc.h>+#include<asm/hvcall.h>+#include<asm/xics.h>+#include<asm/xive.h>+#include<asm/xive-regs.h>+#include<asm/debug.h>+#include<asm/debugfs.h>+#include<asm/time.h>+#include<asm/opal.h>++#include<linux/debugfs.h>+#include<linux/seq_file.h>++#include"book3s_xive.h"++staticvoidxive_native_cleanup_queue(structkvm_vcpu*vcpu,intprio)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+structxive_q*q=&xc->queues[prio];++xive_native_disable_queue(xc->vp_id,q,prio);+if(q->qpage){+put_page(virt_to_page(q->qpage));+q->qpage=NULL;+}+}++voidkvmppc_xive_native_cleanup_vcpu(structkvm_vcpu*vcpu)+{+structkvmppc_xive_vcpu*xc=vcpu->arch.xive_vcpu;+inti;++if(!kvmppc_xive_enabled(vcpu))+return;++if(!xc)+return;++pr_devel("native_cleanup_vcpu(cpu=%d)\n",xc->server_num);++/* Ensure no interrupt is still routed to that VP */+xc->valid=false;+kvmppc_xive_disable_vcpu_interrupts(vcpu);++/* Disable the VP */+xive_native_disable_vp(xc->vp_id);++/* Free the queues & associated interrupts */+for(i=0;i<KVMPPC_XIVE_Q_COUNT;i++){+/* Free the escalation irq */+if(xc->esc_virq[i]){+free_irq(xc->esc_virq[i],vcpu);+irq_dispose_mapping(xc->esc_virq[i]);+kfree(xc->esc_virq_names[i]);+xc->esc_virq[i]=0;+}++/* Free the queue */+xive_native_cleanup_queue(vcpu,i);+}++/* Free the VP */+kfree(xc);++/* Cleanup the vcpu */+vcpu->arch.irq_type=KVMPPC_IRQ_DEFAULT;+vcpu->arch.xive_vcpu=NULL;+}++intkvmppc_xive_native_connect_vcpu(structkvm_device*dev,+structkvm_vcpu*vcpu,u32cpu)
Why do we need both a *vcpu and a cpu number as an integer?
To be in sync with the other similar routines : kvmppc_xics_connect_vcpu()
and kvmppc_xive_connect_vcpu().
But if we consider that this 'cpu' parameter is always in sync with
vcpu->vcpu_id, we could remove it from the KVM ioctl call I suppose.
Should we do the same for the other routines ?
Well.. I don't know why they are that way. Is that int parameter the
XICS server number, which need not be the same as the vcpu_id ? Can
we set that arbitrarily in XIVE as well?
It looks like these parameters need a name change at least to make it
clearer what the distinction is.
quoted
quoted
+{+ struct kvmppc_xive *xive = dev->private;+ struct kvmppc_xive_vcpu *xc;+ int rc;++ pr_devel("native_connect_vcpu(cpu=%d)\n", cpu);++ if (dev->ops != &kvm_xive_native_ops) {+ pr_devel("Wrong ops !\n");+ return -EPERM;+ }+ if (xive->kvm != vcpu->kvm)+ return -EPERM;+ if (vcpu->arch.irq_type)
Please use an explicit == / != here so we don't have to remember which
symbolic value corresponds to 0.
ok. I agree.
Thanks,
C.
quoted
quoted
+ return -EBUSY;+ if (kvmppc_xive_find_server(vcpu->kvm, cpu)) {+ pr_devel("Duplicate !\n");+ return -EEXIST;+ }+ if (cpu >= KVM_MAX_VCPUS) {+ pr_devel("Out of bounds !\n");+ return -EINVAL;+ }+ xc = kzalloc(sizeof(*xc), GFP_KERNEL);+ if (!xc)+ return -ENOMEM;++ mutex_lock(&vcpu->kvm->lock);+ vcpu->arch.xive_vcpu = xc;+ xc->xive = xive;+ xc->vcpu = vcpu;+ xc->server_num = cpu;+ xc->vp_id = xive->vp_base + cpu;+ xc->valid = true;++ rc = xive_native_get_vp_info(xc->vp_id, &xc->vp_cam, &xc->vp_chip_id);+ if (rc) {+ pr_err("Failed to get VP info from OPAL: %d\n", rc);+ goto bail;+ }++ /*+ * Enable the VP first as the single escalation mode will+ * affect escalation interrupts numbering+ */+ rc = xive_native_enable_vp(xc->vp_id, xive->single_escalation);+ if (rc) {+ pr_err("Failed to enable VP in OPAL: %d\n", rc);+ goto bail;+ }++ /* Configure VCPU fields for use by assembly push/pull */+ vcpu->arch.xive_saved_state.w01 = cpu_to_be64(0xff000000);+ vcpu->arch.xive_cam_word = cpu_to_be32(xc->vp_cam | TM_QW1W2_VO);++ /* TODO: initialize queues ? */++bail:+ vcpu->arch.irq_type = KVMPPC_IRQ_XIVE;+ mutex_unlock(&vcpu->kvm->lock);+ if (rc)+ kvmppc_xive_native_cleanup_vcpu(vcpu);++ return rc;+}++static int kvmppc_xive_native_set_attr(struct kvm_device *dev,+ struct kvm_device_attr *attr)+{+ return -ENXIO;+}++static int kvmppc_xive_native_get_attr(struct kvm_device *dev,+ struct kvm_device_attr *attr)+{+ return -ENXIO;+}++static int kvmppc_xive_native_has_attr(struct kvm_device *dev,+ struct kvm_device_attr *attr)+{+ return -ENXIO;+}++static void kvmppc_xive_native_free(struct kvm_device *dev)+{+ struct kvmppc_xive *xive = dev->private;+ struct kvm *kvm = xive->kvm;+ int i;++ debugfs_remove(xive->dentry);++ pr_devel("Destroying xive native for partition\n");++ if (kvm)+ kvm->arch.xive = NULL;++ /* Mask and free interrupts */+ for (i = 0; i <= xive->max_sbid; i++) {+ if (xive->src_blocks[i])+ kvmppc_xive_free_sources(xive->src_blocks[i]);+ kfree(xive->src_blocks[i]);+ xive->src_blocks[i] = NULL;+ }++ if (xive->vp_base != XIVE_INVALID_VP)+ xive_native_free_vp_block(xive->vp_base);++ kfree(xive);+ kfree(dev);+}++static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)+{+ struct kvmppc_xive *xive;+ struct kvm *kvm = dev->kvm;+ int ret = 0;++ pr_devel("Creating xive native for partition\n");++ if (kvm->arch.xive)+ return -EEXIST;++ xive = kzalloc(sizeof(*xive), GFP_KERNEL);+ if (!xive)+ return -ENOMEM;++ dev->private = xive;+ xive->dev = dev;+ xive->kvm = kvm;+ kvm->arch.xive = xive;++ /* We use the default queue size set by the host */+ xive->q_order = xive_native_default_eq_shift();+ if (xive->q_order < PAGE_SHIFT)+ xive->q_page_order = 0;+ else+ xive->q_page_order = xive->q_order - PAGE_SHIFT;++ /* Allocate a bunch of VPs */+ xive->vp_base = xive_native_alloc_vp_block(KVM_MAX_VCPUS);+ pr_devel("VP_Base=%x\n", xive->vp_base);++ if (xive->vp_base == XIVE_INVALID_VP)+ ret = -ENOMEM;++ xive->single_escalation = xive_native_has_single_escalation();++ if (ret)+ kfree(xive);++ return ret;+}++static int xive_native_debug_show(struct seq_file *m, void *private)+{+ struct kvmppc_xive *xive = m->private;+ struct kvm *kvm = xive->kvm;+ struct kvm_vcpu *vcpu;+ unsigned int i;++ if (!kvm)+ return 0;++ seq_puts(m, "=========\nVCPU state\n=========\n");++ kvm_for_each_vcpu(i, vcpu, kvm) {+ struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;++ if (!xc)+ continue;++ seq_printf(m, "cpu server %#x NSR=%02x CPPR=%02x IBP=%02x PIPR=%02x w01=%016llx w2=%08x\n",+ xc->server_num,+ vcpu->arch.xive_saved_state.nsr,+ vcpu->arch.xive_saved_state.cppr,+ vcpu->arch.xive_saved_state.ipb,+ vcpu->arch.xive_saved_state.pipr,+ vcpu->arch.xive_saved_state.w01,+ (u32) vcpu->arch.xive_cam_word);++ kvmppc_xive_debug_show_queues(m, vcpu);+ }++ return 0;+}++static int xive_native_debug_open(struct inode *inode, struct file *file)+{+ return single_open(file, xive_native_debug_show, inode->i_private);+}++static const struct file_operations xive_native_debug_fops = {+ .open = xive_native_debug_open,+ .read = seq_read,+ .llseek = seq_lseek,+ .release = single_release,+};++static void xive_native_debugfs_init(struct kvmppc_xive *xive)+{+ char *name;++ name = kasprintf(GFP_KERNEL, "kvm-xive-%p", xive);+ if (!name) {+ pr_err("%s: no memory for name\n", __func__);+ return;+ }++ xive->dentry = debugfs_create_file(name, 0444, powerpc_debugfs_root,+ xive, &xive_native_debug_fops);++ pr_debug("%s: created %s\n", __func__, name);+ kfree(name);+}++static void kvmppc_xive_native_init(struct kvm_device *dev)+{+ struct kvmppc_xive *xive = (struct kvmppc_xive *)dev->private;++ /* Register some debug interfaces */+ xive_native_debugfs_init(xive);+}++struct kvm_device_ops kvm_xive_native_ops = {+ .name = "kvm-xive-native",+ .create = kvmppc_xive_native_create,+ .init = kvmppc_xive_native_init,+ .destroy = kvmppc_xive_native_free,+ .set_attr = kvmppc_xive_native_set_attr,+ .get_attr = kvmppc_xive_native_get_attr,+ .has_attr = kvmppc_xive_native_has_attr,+};++void kvmppc_xive_native_init_module(void)+{+ ;+}++void kvmppc_xive_native_exit_module(void)+{+ ;+}
--
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
From: David Gibson <hidden> Date: 2019-02-05 05:47:37
On Mon, Feb 04, 2019 at 05:07:28PM +0100, Cédric Le Goater wrote:
On 2/4/19 6:21 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
But we don't have an equivalent name in the PAPR specs for the tuple
(prio, server). We could use the generic 'target' name may be ? even
if this is usually referring to a CPU number.
Um.. what? That's about terminology for one of the fields in this
thing, not about the name for the thing itself.
Or, IVE (Interrupt Vector Entry) ? which makes some sense.
This is was the former name in HW. I think we recycle it for KVM.
That's a terrible idea, which will make a confusing situation even
more confusing.
--
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
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-05 11:34:34
quoted
quoted
As for nesting, I suggest for the foreseeable future we stick to XICS
emulation in nested guests.
ok. so no kernel_irqchip at all. hmm.
I was confused with what Paul calls 'XICS emulation'. It's not the QEMU
XICS emulated device but the XICS-over-XIVE KVM device, the KVM XICS
device KVM uses when under a P9 processor.
That would certainly be step 0, making sure the capability advertises
this correctly. I think we do want to make XICs-on-XIVE emulation
work in a KVM L1 (so we'd need to have it make XIVE hcalls to the L0
instead of OPAL calls).
With the latest patch of Paul, the KVM XICS device is available for L2
and it works quite well.
I also want to test it when L1 runs in KVM XIVE native mode, with the
current patchset, to see how it behaves.
XIVE-on-XIVE for L1 would be nice too, which would mean implementing
the XIVE hcalls from the L2 in terms of XIVE hcalls to the L0. I
think it's ok to delay this indefinitely as long as the caps advertise
correctly so that qemu will use userspace emulation until its ready.
ok. I need to fix this in the current patchset.
Thanks,
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-05 12:21:48
On 2/5/19 6:33 AM, David Gibson wrote:
On Mon, Feb 04, 2019 at 07:57:26PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 6:26 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 08:10:04PM +0100, Cédric Le Goater wrote:
quoted
At a VCPU level, the state of the thread context interrupt management
registers needs to be collected. These registers are cached under the
'xive_saved_state.w01' field of the VCPU when the VPCU context is
pulled from the HW thread. An OPAL call retrieves the backup of the
IPB register in the NVT structure and merges it in the KVM state.
The structures of the interface between QEMU and KVM provisions some
extra room (two u64) for further extensions if more state needs to be
transferred back to QEMU.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/asm/kvm_ppc.h | 5 ++
arch/powerpc/include/uapi/asm/kvm.h | 2 +
arch/powerpc/kvm/book3s.c | 24 +++++++++
arch/powerpc/kvm/book3s_xive_native.c | 78 +++++++++++++++++++++++++++
4 files changed, 109 insertions(+)
IIRC "VP" is the old name for "TCTX". Since we're using tctx in the
rest of the XIVE code, can we use it here as well.
OK. The state we are getting or setting is indeed related to the thread
interrupt context registers.
The name VP is related to an identifier to some interrupt context under
OPAL (NVT in HW to be precise).
Oh, sorry, "NVT" was the name I was looking for, not "TCTX". But in
any case, please lets standardize on one.
There is some confusion in the naming for :
- VP Virtual Processor (XIVE 1)
- VPD Virtual Processor Descriptor (XIVE 1)
- TCTX Thread interrupt context registers
- NVT Notify Virtual Target. Former VP.
- NVTS Notify Virtual Target Structure. Where the TCTX regs are cached.
I am fine with using NVT because this is indeed the name of the XIVE
structure where the HW caches the thread interrupt context registers.
But the XIVE native layer and the XICS-over-XIVE KVM device use the
name VP (the old one). I don't think we want to change these now.
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-05 13:07:19
On 2/5/19 6:32 AM, David Gibson wrote:
On Mon, Feb 04, 2019 at 05:07:28PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 6:21 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
But we don't have an equivalent name in the PAPR specs for the tuple
(prio, server). We could use the generic 'target' name may be ? even
if this is usually referring to a CPU number.
Um.. what? That's about terminology for one of the fields in this
thing, not about the name for the thing itself.
quoted
Or, IVE (Interrupt Vector Entry) ? which makes some sense.
This is was the former name in HW. I think we recycle it for KVM.
That's a terrible idea, which will make a confusing situation even
more confusing.
Let's use SOURCE_CONFIG and QUEUE_CONFIG. The KVM ioctls are very
similar to the hcalls anyhow.
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-05 13:09:46
On 2/5/19 6:28 AM, David Gibson wrote:
On Mon, Feb 04, 2019 at 12:30:39PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 5:45 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
quoted
This will let the guest create a memory mapping to expose the ESB MMIO
regions used to control the interrupt sources, to trigger events, to
EOI or to turn off the sources.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 4 ++
arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Introducing a new FD for ESB and TIMA seems overkill. Can't you get
to both with an mmap() directly on the xive device fd? Using the
offset to distinguish which one to map, obviously.
The page offset would define some sort of user API. It seems feasible.
But I am not sure this would be practical in the future if we need to
tune the length.
Um.. why not? I mean, yes the XIVE supports rather a lot of
interrupts, but we have 64-bits of offset we can play with - we can
leave room for billions of ESB slots and still have room for billions
of VPs.
So the first 4 pages could be the TIMA pages and then would come
the pages for the interrupt ESBs. I think that we can have different
vm_fault handler for each mapping.
I wonder how this will work out with pass-through. As Paul said in
a previous email, it would be better to let QEMU request a new
mapping to handle the ESB pages of the device being passed through.
I guess this is not a special case, just another offset and length.
I will give it a try.
Thanks,
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-05 13:41:38
On 2/5/19 6:35 AM, David Gibson wrote:
On Mon, Feb 04, 2019 at 08:07:20PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 5:57 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:21PM +0100, Cédric Le Goater wrote:
[snip]
quoted
quoted
quoted
+ sb = kvmppc_xive_create_src_block(xive, irq);+ if (!sb) {+ pr_err("Failed to create block...\n");+ return -ENOMEM;+ }+ }+ state = &sb->irq_state[idx];++ if (get_user(val, ubufp)) {+ pr_err("fault getting user info !\n");+ return -EFAULT;+ }++ /*+ * If the source doesn't already have an IPI, allocate+ * one and get the corresponding data+ */+ if (!state->ipi_number) {+ state->ipi_number = xive_native_alloc_irq();+ if (state->ipi_number == 0) {+ pr_err("Failed to allocate IRQ !\n");+ return -ENOMEM;+ }
Am I right in thinking this is the point at which a specific guest irq
number gets bound to a specific host irq number?
yes. the XIVE IRQ state caches this information and 'state' should be
protected before being assigned, indeed ... The XICS-over-XIVE device
also has the same race issue.
It's not showing because where initializing the KVM device sequentially
from QEMU and only once.
Ok.
So, for the passthrough case, what's the point at which we know that a
particular guest interrupt needs to be bound to a specific real
hardware interrupt, rather than a generic IPI?
when the guest driver requests MSIs, VFIO requests a mapping of the
HW irqs in the guest IRQ space. This is very briefly said as VFIO is
a huge framework.
Patch 18 adds some initial support to handle the ESB pages but this
should be done at the QEMU level.
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-05 18:08:16
On 2/4/19 6:24 AM, David Gibson wrote:
On Mon, Jan 07, 2019 at 07:43:28PM +0100, Cédric Le Goater wrote:
quoted
These are used to capture the XIVE END table of the KVM device. It
relies on an OPAL call to retrieve from the XIVE IC the EQ toggle bit
and index which are updated by the HW when events are enqueued in the
guest RAM.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 21 ++++
arch/powerpc/kvm/book3s_xive_native.c | 166 ++++++++++++++++++++++++++
2 files changed, 187 insertions(+)
Should we pad this in case a) we discover some fields in the EQ that
we thought weren't relevant to the guest actually are or b) future
XIVE extensions add something we need to migrate.
The underlying XIVE structure is composed of 32bytes. I will double the
size.
Thanks,
C.
From: Paul Mackerras <hidden> Date: 2019-02-05 22:15:29
On Tue, Feb 05, 2019 at 12:31:28PM +0100, Cédric Le Goater wrote:
quoted
quoted
quoted
As for nesting, I suggest for the foreseeable future we stick to XICS
emulation in nested guests.
ok. so no kernel_irqchip at all. hmm.
I was confused with what Paul calls 'XICS emulation'. It's not the QEMU
XICS emulated device but the XICS-over-XIVE KVM device, the KVM XICS
device KVM uses when under a P9 processor.
Actually there are two separate implementations of XICS emulation in
KVM. The first (older) one is almost entirely a software emulation
but does have some cases where it accesses an underlying XICS device
in order to make some things faster (IPIs and pass-through of a device
interrupt to a guest). The other, newer one is the XICS-on-XIVE
emulation that Ben wrote, which uses the XIVE hardware pretty heavily.
My patch was about making the the older code work when there is no
XICS available to the host.
Paul.
From: David Gibson <hidden> Date: 2019-02-06 01:48:36
On Tue, Feb 05, 2019 at 12:58:54PM +0100, Cédric Le Goater wrote:
On 2/5/19 6:33 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 07:57:26PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 6:26 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 08:10:04PM +0100, Cédric Le Goater wrote:
quoted
At a VCPU level, the state of the thread context interrupt management
registers needs to be collected. These registers are cached under the
'xive_saved_state.w01' field of the VCPU when the VPCU context is
pulled from the HW thread. An OPAL call retrieves the backup of the
IPB register in the NVT structure and merges it in the KVM state.
The structures of the interface between QEMU and KVM provisions some
extra room (two u64) for further extensions if more state needs to be
transferred back to QEMU.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/asm/kvm_ppc.h | 5 ++
arch/powerpc/include/uapi/asm/kvm.h | 2 +
arch/powerpc/kvm/book3s.c | 24 +++++++++
arch/powerpc/kvm/book3s_xive_native.c | 78 +++++++++++++++++++++++++++
4 files changed, 109 insertions(+)
IIRC "VP" is the old name for "TCTX". Since we're using tctx in the
rest of the XIVE code, can we use it here as well.
OK. The state we are getting or setting is indeed related to the thread
interrupt context registers.
The name VP is related to an identifier to some interrupt context under
OPAL (NVT in HW to be precise).
Oh, sorry, "NVT" was the name I was looking for, not "TCTX". But in
any case, please lets standardize on one.
There is some confusion in the naming for :
- VP Virtual Processor (XIVE 1)
- VPD Virtual Processor Descriptor (XIVE 1)
- TCTX Thread interrupt context registers
- NVT Notify Virtual Target. Former VP.
- NVTS Notify Virtual Target Structure. Where the TCTX regs are cached.
I am fine with using NVT because this is indeed the name of the XIVE
structure where the HW caches the thread interrupt context registers.
But the XIVE native layer and the XICS-over-XIVE KVM device use the
name VP (the old one). I don't think we want to change these now.
Ah, right. It now occurs to me that the place I've already seen NVT
used is in the qemu code, whereas this is kernel. In that case
sticking to VP here makes sense.
--
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
From: David Gibson <hidden> Date: 2019-02-06 01:50:06
On Tue, Feb 05, 2019 at 02:03:11PM +0100, Cédric Le Goater wrote:
On 2/5/19 6:32 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 05:07:28PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 6:21 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
But we don't have an equivalent name in the PAPR specs for the tuple
(prio, server). We could use the generic 'target' name may be ? even
if this is usually referring to a CPU number.
Um.. what? That's about terminology for one of the fields in this
thing, not about the name for the thing itself.
quoted
Or, IVE (Interrupt Vector Entry) ? which makes some sense.
This is was the former name in HW. I think we recycle it for KVM.
That's a terrible idea, which will make a confusing situation even
more confusing.
Let's use SOURCE_CONFIG and QUEUE_CONFIG. The KVM ioctls are very
similar to the hcalls anyhow.
Yes, I think that's a good idea.
--
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
From: David Gibson <hidden> Date: 2019-02-06 01:51:39
On Tue, Feb 05, 2019 at 01:55:40PM +0100, Cédric Le Goater wrote:
On 2/5/19 6:28 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 12:30:39PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 5:45 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
quoted
This will let the guest create a memory mapping to expose the ESB MMIO
regions used to control the interrupt sources, to trigger events, to
EOI or to turn off the sources.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 4 ++
arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Introducing a new FD for ESB and TIMA seems overkill. Can't you get
to both with an mmap() directly on the xive device fd? Using the
offset to distinguish which one to map, obviously.
The page offset would define some sort of user API. It seems feasible.
But I am not sure this would be practical in the future if we need to
tune the length.
Um.. why not? I mean, yes the XIVE supports rather a lot of
interrupts, but we have 64-bits of offset we can play with - we can
leave room for billions of ESB slots and still have room for billions
of VPs.
So the first 4 pages could be the TIMA pages and then would come
the pages for the interrupt ESBs. I think that we can have different
vm_fault handler for each mapping.
Um.. no, I'm saying you don't need to tightly pack them. So you could
have the ESB pages at 0, the TIMA at, say offset 2^60.
I wonder how this will work out with pass-through. As Paul said in
a previous email, it would be better to let QEMU request a new
mapping to handle the ESB pages of the device being passed through.
I guess this is not a special case, just another offset and length.
Right, if we need multiple "chunks" of ESB pages we can given them
each their own terabyte or several. No need to be stingy with address
space.
--
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
From: David Gibson <hidden> Date: 2019-02-06 01:53:04
On Wed, Feb 06, 2019 at 12:23:29PM +1100, David Gibson wrote:
On Tue, Feb 05, 2019 at 02:03:11PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:32 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 05:07:28PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 6:21 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
But we don't have an equivalent name in the PAPR specs for the tuple
(prio, server). We could use the generic 'target' name may be ? even
if this is usually referring to a CPU number.
Um.. what? That's about terminology for one of the fields in this
thing, not about the name for the thing itself.
quoted
Or, IVE (Interrupt Vector Entry) ? which makes some sense.
This is was the former name in HW. I think we recycle it for KVM.
That's a terrible idea, which will make a confusing situation even
more confusing.
Let's use SOURCE_CONFIG and QUEUE_CONFIG. The KVM ioctls are very
similar to the hcalls anyhow.
Yes, I think that's a good idea.
Actually... AIUI the SET_CONFIG hcalls shouldn't be a fast path. Can
we simplify things further by removing the hcall implementation from
the kernel entirely, and have qemu implement them by basically just
forwarding them to the appropriate SET_CONFIG ioctl()?
--
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
From: David Gibson <hidden> Date: 2019-02-06 01:54:31
On Wed, Feb 06, 2019 at 09:13:15AM +1100, Paul Mackerras wrote:
On Tue, Feb 05, 2019 at 12:31:28PM +0100, Cédric Le Goater wrote:
quoted
quoted
quoted
quoted
As for nesting, I suggest for the foreseeable future we stick to XICS
emulation in nested guests.
ok. so no kernel_irqchip at all. hmm.
I was confused with what Paul calls 'XICS emulation'. It's not the QEMU
XICS emulated device but the XICS-over-XIVE KVM device, the KVM XICS
device KVM uses when under a P9 processor.
Actually there are two separate implementations of XICS emulation in
KVM. The first (older) one is almost entirely a software emulation
but does have some cases where it accesses an underlying XICS device
in order to make some things faster (IPIs and pass-through of a device
interrupt to a guest). The other, newer one is the XICS-on-XIVE
emulation that Ben wrote, which uses the XIVE hardware pretty heavily.
My patch was about making the the older code work when there is no
XICS available to the host.
Ah, right. To clarify my earlier statements in light of this:
* We definitely want some sort of kernel-XICS available in a nested
guest. AIUI, this is now accomplished, so, Yay!
* Implementing the L2 XICS in terms of L1's PAPR-XIVE would be a
bonus, but it's a much lower priority.
--
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
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-06 07:14:46
On 2/6/19 2:24 AM, David Gibson wrote:
On Wed, Feb 06, 2019 at 12:23:29PM +1100, David Gibson wrote:
quoted
On Tue, Feb 05, 2019 at 02:03:11PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:32 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 05:07:28PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 6:21 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
But we don't have an equivalent name in the PAPR specs for the tuple
(prio, server). We could use the generic 'target' name may be ? even
if this is usually referring to a CPU number.
Um.. what? That's about terminology for one of the fields in this
thing, not about the name for the thing itself.
quoted
Or, IVE (Interrupt Vector Entry) ? which makes some sense.
This is was the former name in HW. I think we recycle it for KVM.
That's a terrible idea, which will make a confusing situation even
more confusing.
Let's use SOURCE_CONFIG and QUEUE_CONFIG. The KVM ioctls are very
similar to the hcalls anyhow.
Yes, I think that's a good idea.
Actually... AIUI the SET_CONFIG hcalls shouldn't be a fast path.
No indeed. I have move them to standard hcalls in the current version.
Can
we simplify things further by removing the hcall implementation from
the kernel entirely, and have qemu implement them by basically just
forwarding them to the appropriate SET_CONFIG ioctl()?
Yes. I think we could.
The hcalls H_INT_SET_SOURCE_CONFIG and H_INT_SET_QUEUE_CONFIG and
the KVM ioctls to set the EQ and the SOURCE configuration have a
lot in common. I need to look at how we can plug the KVM ioctl in
the hcalls under QEMU.
We will have to convert the returned error to respect the PAPR
specs or have the ioctls return H_* errors.
Let's dig that idea. If we choose that path, QEMU will have an
up-to-date EAT and so we won't need to synchronize its state anymore
for migration.
H_INT_GET_SOURCE_CONFIG can be implemented in QEMU without any KVM
ioctl.
H_INT_GET_QUEUE_INFO could be implemented in QEMU. I need to check
how we return the address of the END ESB in sPAPR. We haven't paid
much attention to these pages because they are not used under Linux
and today the address is returned by OPAL.
H_INT_GET_QUEUE_CONFIG is a little more problematic because we need
to query into the XIVE HW the EQ index and toggle bit. OPAL support
is required for that. But we could reduce the KVM support to the
ioctl querying these EQ information.
H_INT_ESB could be entirely done under QEMU.
H_INT_SYNC and H_INT_RESET can not.
H_INT_GET_OS_REPORTING_LINE and H_INT_SET_OS_REPORTING_LINE are not
implemented.
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-06 07:22:57
On 2/6/19 2:23 AM, David Gibson wrote:
On Tue, Feb 05, 2019 at 01:55:40PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:28 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 12:30:39PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 5:45 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
quoted
This will let the guest create a memory mapping to expose the ESB MMIO
regions used to control the interrupt sources, to trigger events, to
EOI or to turn off the sources.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 4 ++
arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Introducing a new FD for ESB and TIMA seems overkill. Can't you get
to both with an mmap() directly on the xive device fd? Using the
offset to distinguish which one to map, obviously.
The page offset would define some sort of user API. It seems feasible.
But I am not sure this would be practical in the future if we need to
tune the length.
Um.. why not? I mean, yes the XIVE supports rather a lot of
interrupts, but we have 64-bits of offset we can play with - we can
leave room for billions of ESB slots and still have room for billions
of VPs.
So the first 4 pages could be the TIMA pages and then would come
the pages for the interrupt ESBs. I think that we can have different
vm_fault handler for each mapping.
Um.. no, I'm saying you don't need to tightly pack them. So you could
have the ESB pages at 0, the TIMA at, say offset 2^60.
Well, we know that the TIMA is 4 pages wide and is "directly" related
with the KVM interrupt device. So being at offset 0 seems a good idea.
While the ESB segment is of a variable size depending on the number
of IRQs and it can come after I think.
quoted
I wonder how this will work out with pass-through. As Paul said in
a previous email, it would be better to let QEMU request a new
mapping to handle the ESB pages of the device being passed through.
I guess this is not a special case, just another offset and length.
Right, if we need multiple "chunks" of ESB pages we can given them
each their own terabyte or several. No need to be stingy with address
space.
You can not put them anywhere. They should map the same interrupt range
of ESB pages, overlapping with the underlying segment of IPI ESB pages.
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-06 07:37:12
On 2/6/19 2:18 AM, David Gibson wrote:
On Wed, Feb 06, 2019 at 09:13:15AM +1100, Paul Mackerras wrote:
quoted
On Tue, Feb 05, 2019 at 12:31:28PM +0100, Cédric Le Goater wrote:
quoted
quoted
quoted
quoted
As for nesting, I suggest for the foreseeable future we stick to XICS
emulation in nested guests.
ok. so no kernel_irqchip at all. hmm.
I was confused with what Paul calls 'XICS emulation'. It's not the QEMU
XICS emulated device but the XICS-over-XIVE KVM device, the KVM XICS
device KVM uses when under a P9 processor.
Actually there are two separate implementations of XICS emulation in
KVM. The first (older) one is almost entirely a software emulation
but does have some cases where it accesses an underlying XICS device
in order to make some things faster (IPIs and pass-through of a device
interrupt to a guest). The other, newer one is the XICS-on-XIVE
emulation that Ben wrote, which uses the XIVE hardware pretty heavily.
My patch was about making the the older code work when there is no
XICS available to the host.
Ah, right. To clarify my earlier statements in light of this:
* We definitely want some sort of kernel-XICS available in a nested
guest. AIUI, this is now accomplished, so, Yay!
* Implementing the L2 XICS in terms of L1's PAPR-XIVE would be a
bonus, but it's a much lower priority.
Yes. In this case, the L1 KVM-HV should not advertise KVM_CAP_PPC_IRQ_XIVE
to QEMU which will restrict CAS to the XICS only interrupt mode.
C.
From: David Gibson <hidden> Date: 2019-02-07 03:00:04
On Wed, Feb 06, 2019 at 08:07:36AM +0100, Cédric Le Goater wrote:
On 2/6/19 2:24 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 12:23:29PM +1100, David Gibson wrote:
quoted
On Tue, Feb 05, 2019 at 02:03:11PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:32 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 05:07:28PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 6:21 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
But we don't have an equivalent name in the PAPR specs for the tuple
(prio, server). We could use the generic 'target' name may be ? even
if this is usually referring to a CPU number.
Um.. what? That's about terminology for one of the fields in this
thing, not about the name for the thing itself.
quoted
Or, IVE (Interrupt Vector Entry) ? which makes some sense.
This is was the former name in HW. I think we recycle it for KVM.
That's a terrible idea, which will make a confusing situation even
more confusing.
Let's use SOURCE_CONFIG and QUEUE_CONFIG. The KVM ioctls are very
similar to the hcalls anyhow.
Yes, I think that's a good idea.
Actually... AIUI the SET_CONFIG hcalls shouldn't be a fast path.
No indeed. I have move them to standard hcalls in the current version.
quoted
Can
we simplify things further by removing the hcall implementation from
the kernel entirely, and have qemu implement them by basically just
forwarding them to the appropriate SET_CONFIG ioctl()?
Yes. I think we could.
Great!
The hcalls H_INT_SET_SOURCE_CONFIG and H_INT_SET_QUEUE_CONFIG and
the KVM ioctls to set the EQ and the SOURCE configuration have a
lot in common. I need to look at how we can plug the KVM ioctl in
the hcalls under QEMU.
We will have to convert the returned error to respect the PAPR
specs or have the ioctls return H_* errors.
I don't think returning H_* values from a kernel call is a good idea.
Converting errors is kinda ugly, but I still think it's the better
option. Note that we already have something like this for the HPT
resizing hcalls.
Let's dig that idea. If we choose that path, QEMU will have an
up-to-date EAT and so we won't need to synchronize its state anymore
for migration.
I guess so, though I don't see that as essential.
H_INT_GET_SOURCE_CONFIG can be implemented in QEMU without any KVM
ioctl.
H_INT_GET_QUEUE_INFO could be implemented in QEMU. I need to check
how we return the address of the END ESB in sPAPR. We haven't paid
much attention to these pages because they are not used under Linux
and today the address is returned by OPAL.
H_INT_GET_QUEUE_CONFIG is a little more problematic because we need
to query into the XIVE HW the EQ index and toggle bit. OPAL support
is required for that. But we could reduce the KVM support to the
ioctl querying these EQ information.
Right, and we'd need an ioctl() like that for migration anyway, yes?
H_INT_ESB could be entirely done under QEMU.
This one can actually happen on fairly hot paths, so I think doing
that in qemu probably isn't a good idea.
H_INT_SYNC and H_INT_RESET can not.
H_INT_GET_OS_REPORTING_LINE and H_INT_SET_OS_REPORTING_LINE are not
implemented.
C.
--
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
From: David Gibson <hidden> Date: 2019-02-07 03:01:38
On Wed, Feb 06, 2019 at 08:21:10AM +0100, Cédric Le Goater wrote:
On 2/6/19 2:23 AM, David Gibson wrote:
quoted
On Tue, Feb 05, 2019 at 01:55:40PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:28 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 12:30:39PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 5:45 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
quoted
This will let the guest create a memory mapping to expose the ESB MMIO
regions used to control the interrupt sources, to trigger events, to
EOI or to turn off the sources.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 4 ++
arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Introducing a new FD for ESB and TIMA seems overkill. Can't you get
to both with an mmap() directly on the xive device fd? Using the
offset to distinguish which one to map, obviously.
The page offset would define some sort of user API. It seems feasible.
But I am not sure this would be practical in the future if we need to
tune the length.
Um.. why not? I mean, yes the XIVE supports rather a lot of
interrupts, but we have 64-bits of offset we can play with - we can
leave room for billions of ESB slots and still have room for billions
of VPs.
So the first 4 pages could be the TIMA pages and then would come
the pages for the interrupt ESBs. I think that we can have different
vm_fault handler for each mapping.
Um.. no, I'm saying you don't need to tightly pack them. So you could
have the ESB pages at 0, the TIMA at, say offset 2^60.
Well, we know that the TIMA is 4 pages wide and is "directly" related
with the KVM interrupt device. So being at offset 0 seems a good idea.
While the ESB segment is of a variable size depending on the number
of IRQs and it can come after I think.
quoted
quoted
I wonder how this will work out with pass-through. As Paul said in
a previous email, it would be better to let QEMU request a new
mapping to handle the ESB pages of the device being passed through.
I guess this is not a special case, just another offset and length.
Right, if we need multiple "chunks" of ESB pages we can given them
each their own terabyte or several. No need to be stingy with address
space.
You can not put them anywhere. They should map the same interrupt range
of ESB pages, overlapping with the underlying segment of IPI ESB pages.
I don't really follow what you're saying here.
--
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
From: David Gibson <hidden> Date: 2019-02-07 03:03:11
On Wed, Feb 06, 2019 at 08:35:24AM +0100, Cédric Le Goater wrote:
On 2/6/19 2:18 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 09:13:15AM +1100, Paul Mackerras wrote:
quoted
On Tue, Feb 05, 2019 at 12:31:28PM +0100, Cédric Le Goater wrote:
quoted
quoted
quoted
quoted
As for nesting, I suggest for the foreseeable future we stick to XICS
emulation in nested guests.
ok. so no kernel_irqchip at all. hmm.
I was confused with what Paul calls 'XICS emulation'. It's not the QEMU
XICS emulated device but the XICS-over-XIVE KVM device, the KVM XICS
device KVM uses when under a P9 processor.
Actually there are two separate implementations of XICS emulation in
KVM. The first (older) one is almost entirely a software emulation
but does have some cases where it accesses an underlying XICS device
in order to make some things faster (IPIs and pass-through of a device
interrupt to a guest). The other, newer one is the XICS-on-XIVE
emulation that Ben wrote, which uses the XIVE hardware pretty heavily.
My patch was about making the the older code work when there is no
XICS available to the host.
Ah, right. To clarify my earlier statements in light of this:
* We definitely want some sort of kernel-XICS available in a nested
guest. AIUI, this is now accomplished, so, Yay!
* Implementing the L2 XICS in terms of L1's PAPR-XIVE would be a
bonus, but it's a much lower priority.
Yes. In this case, the L1 KVM-HV should not advertise KVM_CAP_PPC_IRQ_XIVE
to QEMU which will restrict CAS to the XICS only interrupt mode.
Uh... no... we shouldn't change what's available to the guest based on
host configuration only. We should just stop advertising the CAP
saying that *KVM implemented* is available so that qemu will fall back
to userspace XIVE emulation.
--
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
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-07 08:32:59
On 2/7/19 3:51 AM, David Gibson wrote:
On Wed, Feb 06, 2019 at 08:35:24AM +0100, Cédric Le Goater wrote:
quoted
On 2/6/19 2:18 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 09:13:15AM +1100, Paul Mackerras wrote:
quoted
On Tue, Feb 05, 2019 at 12:31:28PM +0100, Cédric Le Goater wrote:
quoted
quoted
quoted
quoted
As for nesting, I suggest for the foreseeable future we stick to XICS
emulation in nested guests.
ok. so no kernel_irqchip at all. hmm.
I was confused with what Paul calls 'XICS emulation'. It's not the QEMU
XICS emulated device but the XICS-over-XIVE KVM device, the KVM XICS
device KVM uses when under a P9 processor.
Actually there are two separate implementations of XICS emulation in
KVM. The first (older) one is almost entirely a software emulation
but does have some cases where it accesses an underlying XICS device
in order to make some things faster (IPIs and pass-through of a device
interrupt to a guest). The other, newer one is the XICS-on-XIVE
emulation that Ben wrote, which uses the XIVE hardware pretty heavily.
My patch was about making the the older code work when there is no
XICS available to the host.
Ah, right. To clarify my earlier statements in light of this:
* We definitely want some sort of kernel-XICS available in a nested
guest. AIUI, this is now accomplished, so, Yay!
* Implementing the L2 XICS in terms of L1's PAPR-XIVE would be a
bonus, but it's a much lower priority.
Yes. In this case, the L1 KVM-HV should not advertise KVM_CAP_PPC_IRQ_XIVE
to QEMU which will restrict CAS to the XICS only interrupt mode.
Uh... no... we shouldn't change what's available to the guest based on
host configuration only. We should just stop advertising the CAP
saying that *KVM implemented* is available
yes. that is what I meant.
so that qemu will fall back to userspace XIVE emulation.
even if kernel_irqchip is required ?
Today, QEMU just fails to start. With the dual mode, the interrupt mode
is negotiated at CAS time and when merged, the KVM device will be created
at reset. In case of failure, QEMU will abort.
I am not saying it is not possible but we will need some internal
infrastructure to handle dynamically the fall back to userspace emulation.
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-07 09:05:07
On 2/7/19 3:49 AM, David Gibson wrote:
On Wed, Feb 06, 2019 at 08:21:10AM +0100, Cédric Le Goater wrote:
quoted
On 2/6/19 2:23 AM, David Gibson wrote:
quoted
On Tue, Feb 05, 2019 at 01:55:40PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:28 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 12:30:39PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 5:45 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
quoted
This will let the guest create a memory mapping to expose the ESB MMIO
regions used to control the interrupt sources, to trigger events, to
EOI or to turn off the sources.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 4 ++
arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Introducing a new FD for ESB and TIMA seems overkill. Can't you get
to both with an mmap() directly on the xive device fd? Using the
offset to distinguish which one to map, obviously.
The page offset would define some sort of user API. It seems feasible.
But I am not sure this would be practical in the future if we need to
tune the length.
Um.. why not? I mean, yes the XIVE supports rather a lot of
interrupts, but we have 64-bits of offset we can play with - we can
leave room for billions of ESB slots and still have room for billions
of VPs.
So the first 4 pages could be the TIMA pages and then would come
the pages for the interrupt ESBs. I think that we can have different
vm_fault handler for each mapping.
Um.. no, I'm saying you don't need to tightly pack them. So you could
have the ESB pages at 0, the TIMA at, say offset 2^60.
Well, we know that the TIMA is 4 pages wide and is "directly" related
with the KVM interrupt device. So being at offset 0 seems a good idea.
While the ESB segment is of a variable size depending on the number
of IRQs and it can come after I think.
quoted
quoted
I wonder how this will work out with pass-through. As Paul said in
a previous email, it would be better to let QEMU request a new
mapping to handle the ESB pages of the device being passed through.
I guess this is not a special case, just another offset and length.
Right, if we need multiple "chunks" of ESB pages we can given them
each their own terabyte or several. No need to be stingy with address
space.
You can not put them anywhere. They should map the same interrupt range
of ESB pages, overlapping with the underlying segment of IPI ESB pages.
I don't really follow what you're saying here.
What we want the guest to access in terms of ESB pages is something like
below, VMA0 being the initial mapping done by QEMU at offset 0x0, the IPI
ESB pages being populated on the demand with the loads and the stores from
the guest :
0x0 0x1100 0x1200 0x1300
ranges | CPU IPIs .. | VIO | PCI LSI | MSIs
+-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
VMA0 IPI ESB | | | | | | | | | | | | | | | | | | | | | | | | |
pages +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
A device is passed through and the driver requests MSIs.
We now want the guest to access the HW ESB pages for the requested IRQs
but still the initial IPI ESB pages for the others. Something like below :
0x0 0x1100 0x1200 0x1300
ranges | CPU IPIs .. | VIO | PCI LSI | MSIs
+-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
VMA0 IPI ESB | | | | | | | | | | | | | | | | | | | | | | | | |
pages +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
VMA1 PHB ESB +-------+
pages | | | | |
+-------+
The VMA1 is the result of a new mmap() being done at an offset depending on
the first IRQ number requested by the driver.
This is because the vm_fault handler uses the page offset to find the
associated KVM IRQ struct containing the addresses of the EOI and trigger
pages in the underlying hardware, which will be the PHB in case of a
passthrough device.
From there, the VMA1 mmap() pointer will be used to create a 'ram device'
memory region which will be mapped on top of the initial ESB memory region
in QEMU. This will override the initial IPI ESB pages with the PHB ESB pages
in the guest ESB address space.
That's the plan I have in mind as suggested by Paul if I understood it well.
The mechanics are more complex than the patch zapping the PTEs from the VMA
but it's also safer.
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-07 09:17:04
On 2/7/19 3:48 AM, David Gibson wrote:
On Wed, Feb 06, 2019 at 08:07:36AM +0100, Cédric Le Goater wrote:
quoted
On 2/6/19 2:24 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 12:23:29PM +1100, David Gibson wrote:
quoted
On Tue, Feb 05, 2019 at 02:03:11PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:32 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 05:07:28PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 6:21 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
But we don't have an equivalent name in the PAPR specs for the tuple
(prio, server). We could use the generic 'target' name may be ? even
if this is usually referring to a CPU number.
Um.. what? That's about terminology for one of the fields in this
thing, not about the name for the thing itself.
quoted
Or, IVE (Interrupt Vector Entry) ? which makes some sense.
This is was the former name in HW. I think we recycle it for KVM.
That's a terrible idea, which will make a confusing situation even
more confusing.
Let's use SOURCE_CONFIG and QUEUE_CONFIG. The KVM ioctls are very
similar to the hcalls anyhow.
Yes, I think that's a good idea.
Actually... AIUI the SET_CONFIG hcalls shouldn't be a fast path.
No indeed. I have move them to standard hcalls in the current version.
quoted
Can
we simplify things further by removing the hcall implementation from
the kernel entirely, and have qemu implement them by basically just
forwarding them to the appropriate SET_CONFIG ioctl()?
Yes. I think we could.
Great!
quoted
The hcalls H_INT_SET_SOURCE_CONFIG and H_INT_SET_QUEUE_CONFIG and
the KVM ioctls to set the EQ and the SOURCE configuration have a
lot in common. I need to look at how we can plug the KVM ioctl in
the hcalls under QEMU.
We will have to convert the returned error to respect the PAPR
specs or have the ioctls return H_* errors.
I don't think returning H_* values from a kernel call is a good idea.
Converting errors is kinda ugly, but I still think it's the better
option. Note that we already have something like this for the HPT
resizing hcalls.
ok.
quoted
Let's dig that idea. If we choose that path, QEMU will have an
up-to-date EAT and so we won't need to synchronize its state anymore
for migration.
I guess so, though I don't see that as essential.
quoted
H_INT_GET_SOURCE_CONFIG can be implemented in QEMU without any KVM
ioctl.
H_INT_GET_QUEUE_INFO could be implemented in QEMU. I need to check
how we return the address of the END ESB in sPAPR. We haven't paid
much attention to these pages because they are not used under Linux
and today the address is returned by OPAL.
H_INT_GET_QUEUE_CONFIG is a little more problematic because we need
to query into the XIVE HW the EQ index and toggle bit. OPAL support
is required for that. But we could reduce the KVM support to the
ioctl querying these EQ information.
Right, and we'd need an ioctl() like that for migration anyway, yes?
Yes. it is the same need.
quoted
H_INT_ESB could be entirely done under QEMU.
This one can actually happen on fairly hot paths, so I think doing
that in qemu probably isn't a good idea.
I agree It would nice to have some performance.
This hcall is used when LSIs are involved, which is not really a common
configuration. There are no OPAL calls involved. And we are duplicating
code at the KVM level to retrigger the interrupt when the level is still
asserted.
I will benchmark the two options before making a choice.
C.
quoted
H_INT_SYNC and H_INT_RESET can not.
H_INT_GET_OS_REPORTING_LINE and H_INT_SET_OS_REPORTING_LINE are not
implemented.
C.
From: David Gibson <hidden> Date: 2019-02-08 05:21:12
On Thu, Feb 07, 2019 at 09:31:06AM +0100, Cédric Le Goater wrote:
On 2/7/19 3:51 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 08:35:24AM +0100, Cédric Le Goater wrote:
quoted
On 2/6/19 2:18 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 09:13:15AM +1100, Paul Mackerras wrote:
quoted
On Tue, Feb 05, 2019 at 12:31:28PM +0100, Cédric Le Goater wrote:
quoted
quoted
quoted
quoted
As for nesting, I suggest for the foreseeable future we stick to XICS
emulation in nested guests.
ok. so no kernel_irqchip at all. hmm.
I was confused with what Paul calls 'XICS emulation'. It's not the QEMU
XICS emulated device but the XICS-over-XIVE KVM device, the KVM XICS
device KVM uses when under a P9 processor.
Actually there are two separate implementations of XICS emulation in
KVM. The first (older) one is almost entirely a software emulation
but does have some cases where it accesses an underlying XICS device
in order to make some things faster (IPIs and pass-through of a device
interrupt to a guest). The other, newer one is the XICS-on-XIVE
emulation that Ben wrote, which uses the XIVE hardware pretty heavily.
My patch was about making the the older code work when there is no
XICS available to the host.
Ah, right. To clarify my earlier statements in light of this:
* We definitely want some sort of kernel-XICS available in a nested
guest. AIUI, this is now accomplished, so, Yay!
* Implementing the L2 XICS in terms of L1's PAPR-XIVE would be a
bonus, but it's a much lower priority.
Yes. In this case, the L1 KVM-HV should not advertise KVM_CAP_PPC_IRQ_XIVE
to QEMU which will restrict CAS to the XICS only interrupt mode.
Uh... no... we shouldn't change what's available to the guest based on
host configuration only. We should just stop advertising the CAP
saying that *KVM implemented* is available
yes. that is what I meant.
quoted
so that qemu will fall back to userspace XIVE emulation.
even if kernel_irqchip is required ?
Well, no, but if we don't specify.
Today, QEMU just fails to start.
If we specify kernel_irqchip=on but the kernel can't support that I
think that's the right thing to do.
With the dual mode, the interrupt mode
is negotiated at CAS time and when merged, the KVM device will be created
at reset. In case of failure, QEMU will abort.
I am not saying it is not possible but we will need some internal
infrastructure to handle dynamically the fall back to userspace
emulation.
Uh.. we do? I think in all cases we need to make the XICS vs. XIVE
decision first (i.e. what we present to the guest), then we should
decide how to implement it (userspace, KVM accelerated, impossible and
give up).
--
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
From: David Gibson <hidden> Date: 2019-02-08 06:31:17
On Thu, Feb 07, 2019 at 10:03:15AM +0100, Cédric Le Goater wrote:
On 2/7/19 3:49 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 08:21:10AM +0100, Cédric Le Goater wrote:
quoted
On 2/6/19 2:23 AM, David Gibson wrote:
quoted
On Tue, Feb 05, 2019 at 01:55:40PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:28 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 12:30:39PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 5:45 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
quoted
This will let the guest create a memory mapping to expose the ESB MMIO
regions used to control the interrupt sources, to trigger events, to
EOI or to turn off the sources.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 4 ++
arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Introducing a new FD for ESB and TIMA seems overkill. Can't you get
to both with an mmap() directly on the xive device fd? Using the
offset to distinguish which one to map, obviously.
The page offset would define some sort of user API. It seems feasible.
But I am not sure this would be practical in the future if we need to
tune the length.
Um.. why not? I mean, yes the XIVE supports rather a lot of
interrupts, but we have 64-bits of offset we can play with - we can
leave room for billions of ESB slots and still have room for billions
of VPs.
So the first 4 pages could be the TIMA pages and then would come
the pages for the interrupt ESBs. I think that we can have different
vm_fault handler for each mapping.
Um.. no, I'm saying you don't need to tightly pack them. So you could
have the ESB pages at 0, the TIMA at, say offset 2^60.
Well, we know that the TIMA is 4 pages wide and is "directly" related
with the KVM interrupt device. So being at offset 0 seems a good idea.
While the ESB segment is of a variable size depending on the number
of IRQs and it can come after I think.
quoted
quoted
I wonder how this will work out with pass-through. As Paul said in
a previous email, it would be better to let QEMU request a new
mapping to handle the ESB pages of the device being passed through.
I guess this is not a special case, just another offset and length.
Right, if we need multiple "chunks" of ESB pages we can given them
each their own terabyte or several. No need to be stingy with address
space.
You can not put them anywhere. They should map the same interrupt range
of ESB pages, overlapping with the underlying segment of IPI ESB pages.
I don't really follow what you're saying here.
What we want the guest to access in terms of ESB pages is something like
below, VMA0 being the initial mapping done by QEMU at offset 0x0, the IPI
ESB pages being populated on the demand with the loads and the stores from
the guest :
0x0 0x1100 0x1200 0x1300
ranges | CPU IPIs .. | VIO | PCI LSI | MSIs
+-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
VMA0 IPI ESB | | | | | | | | | | | | | | | | | | | | | | | | |
pages +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
A device is passed through and the driver requests MSIs.
We now want the guest to access the HW ESB pages for the requested IRQs
but still the initial IPI ESB pages for the others. Something like below :
0x0 0x1100 0x1200 0x1300
ranges | CPU IPIs .. | VIO | PCI LSI | MSIs
+-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
VMA0 IPI ESB | | | | | | | | | | | | | | | | | | | | | | | | |
pages +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
VMA1 PHB ESB +-------+
pages | | | | |
+-------+
Right, except of course VMA0 will be split into two pieces by
performing the mmap() over it.
The VMA1 is the result of a new mmap() being done at an offset depending on
the first IRQ number requested by the driver.
Right... that's one way we could do it. But the irq numbers are all
dynamically allocated here, so could we instead just put the
passthrough MSIs in a separate range? We'd still need a separate
mmap() for them, but we wouldn't have to deal with mapping over and
unmapping if the device is removed or whatever.
This is because the vm_fault handler uses the page offset to find the
associated KVM IRQ struct containing the addresses of the EOI and trigger
pages in the underlying hardware, which will be the PHB in case of a
passthrough device.
quoted
From there, the VMA1 mmap() pointer will be used to create a 'ram device'
memory region which will be mapped on top of the initial ESB memory region
in QEMU. This will override the initial IPI ESB pages with the PHB ESB pages
in the guest ESB address space.
Um.. what? If that qemu memory range is already mapped into the guest
we don't need to create new RAM devices or anything for the
overmapping. If we overmap in qemu that will just get carried into
the guest.
That's the plan I have in mind as suggested by Paul if I understood it well.
The mechanics are more complex than the patch zapping the PTEs from the VMA
but it's also safer.
Well, yes, where "safer" means "has the possibility to be correct".
--
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
From: David Gibson <hidden> Date: 2019-02-08 06:32:49
On Thu, Feb 07, 2019 at 10:13:48AM +0100, Cédric Le Goater wrote:
On 2/7/19 3:48 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 08:07:36AM +0100, Cédric Le Goater wrote:
quoted
On 2/6/19 2:24 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 12:23:29PM +1100, David Gibson wrote:
quoted
On Tue, Feb 05, 2019 at 02:03:11PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:32 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 05:07:28PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 6:21 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
But we don't have an equivalent name in the PAPR specs for the tuple
(prio, server). We could use the generic 'target' name may be ? even
if this is usually referring to a CPU number.
Um.. what? That's about terminology for one of the fields in this
thing, not about the name for the thing itself.
quoted
Or, IVE (Interrupt Vector Entry) ? which makes some sense.
This is was the former name in HW. I think we recycle it for KVM.
That's a terrible idea, which will make a confusing situation even
more confusing.
Let's use SOURCE_CONFIG and QUEUE_CONFIG. The KVM ioctls are very
similar to the hcalls anyhow.
Yes, I think that's a good idea.
Actually... AIUI the SET_CONFIG hcalls shouldn't be a fast path.
No indeed. I have move them to standard hcalls in the current version.
quoted
Can
we simplify things further by removing the hcall implementation from
the kernel entirely, and have qemu implement them by basically just
forwarding them to the appropriate SET_CONFIG ioctl()?
Yes. I think we could.
Great!
quoted
The hcalls H_INT_SET_SOURCE_CONFIG and H_INT_SET_QUEUE_CONFIG and
the KVM ioctls to set the EQ and the SOURCE configuration have a
lot in common. I need to look at how we can plug the KVM ioctl in
the hcalls under QEMU.
We will have to convert the returned error to respect the PAPR
specs or have the ioctls return H_* errors.
I don't think returning H_* values from a kernel call is a good idea.
Converting errors is kinda ugly, but I still think it's the better
option. Note that we already have something like this for the HPT
resizing hcalls.
ok.
quoted
quoted
Let's dig that idea. If we choose that path, QEMU will have an
up-to-date EAT and so we won't need to synchronize its state anymore
for migration.
I guess so, though I don't see that as essential.
quoted
H_INT_GET_SOURCE_CONFIG can be implemented in QEMU without any KVM
ioctl.
H_INT_GET_QUEUE_INFO could be implemented in QEMU. I need to check
how we return the address of the END ESB in sPAPR. We haven't paid
much attention to these pages because they are not used under Linux
and today the address is returned by OPAL.
H_INT_GET_QUEUE_CONFIG is a little more problematic because we need
to query into the XIVE HW the EQ index and toggle bit. OPAL support
is required for that. But we could reduce the KVM support to the
ioctl querying these EQ information.
Right, and we'd need an ioctl() like that for migration anyway, yes?
Yes. it is the same need.
quoted
quoted
H_INT_ESB could be entirely done under QEMU.
This one can actually happen on fairly hot paths, so I think doing
that in qemu probably isn't a good idea.
I agree It would nice to have some performance.
This hcall is used when LSIs are involved, which is not really a common
configuration. There are no OPAL calls involved. And we are duplicating
code at the KVM level to retrigger the interrupt when the level is still
asserted.
I will benchmark the two options before making a choice.
Ok.
--
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
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-08 07:48:30
quoted
With the dual mode, the interrupt mode
is negotiated at CAS time and when merged, the KVM device will be created
at reset. In case of failure, QEMU will abort.
I am not saying it is not possible but we will need some internal
infrastructure to handle dynamically the fall back to userspace
emulation.
Uh.. we do? I think in all cases we need to make the XICS vs. XIVE
decision first (i.e. what we present to the guest), then we should
decide how to implement it (userspace, KVM accelerated, impossible and
give up).
I am changing things with the addition of KM support for dual mode but
that might not be the right approach. Let's talk over it when you reach
the end of the QEMU patchset.
I will keep in mind that we should know exactly what KVM supports
before the machine starts. That is : not to abort QEMU if we can not
satisfy the interrupt mode chosen at CAS time. It might be possible
to fallback to XIVE emulated mode, I think that is where the problem
is but I haven't looked at it closely.
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-08 08:00:02
On 2/8/19 6:15 AM, David Gibson wrote:
On Thu, Feb 07, 2019 at 10:03:15AM +0100, Cédric Le Goater wrote:
quoted
On 2/7/19 3:49 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 08:21:10AM +0100, Cédric Le Goater wrote:
quoted
On 2/6/19 2:23 AM, David Gibson wrote:
quoted
On Tue, Feb 05, 2019 at 01:55:40PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:28 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 12:30:39PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 5:45 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
quoted
This will let the guest create a memory mapping to expose the ESB MMIO
regions used to control the interrupt sources, to trigger events, to
EOI or to turn off the sources.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 4 ++
arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Introducing a new FD for ESB and TIMA seems overkill. Can't you get
to both with an mmap() directly on the xive device fd? Using the
offset to distinguish which one to map, obviously.
The page offset would define some sort of user API. It seems feasible.
But I am not sure this would be practical in the future if we need to
tune the length.
Um.. why not? I mean, yes the XIVE supports rather a lot of
interrupts, but we have 64-bits of offset we can play with - we can
leave room for billions of ESB slots and still have room for billions
of VPs.
So the first 4 pages could be the TIMA pages and then would come
the pages for the interrupt ESBs. I think that we can have different
vm_fault handler for each mapping.
Um.. no, I'm saying you don't need to tightly pack them. So you could
have the ESB pages at 0, the TIMA at, say offset 2^60.
Well, we know that the TIMA is 4 pages wide and is "directly" related
with the KVM interrupt device. So being at offset 0 seems a good idea.
While the ESB segment is of a variable size depending on the number
of IRQs and it can come after I think.
quoted
quoted
I wonder how this will work out with pass-through. As Paul said in
a previous email, it would be better to let QEMU request a new
mapping to handle the ESB pages of the device being passed through.
I guess this is not a special case, just another offset and length.
Right, if we need multiple "chunks" of ESB pages we can given them
each their own terabyte or several. No need to be stingy with address
space.
You can not put them anywhere. They should map the same interrupt range
of ESB pages, overlapping with the underlying segment of IPI ESB pages.
I don't really follow what you're saying here.
What we want the guest to access in terms of ESB pages is something like
below, VMA0 being the initial mapping done by QEMU at offset 0x0, the IPI
ESB pages being populated on the demand with the loads and the stores from
the guest :
0x0 0x1100 0x1200 0x1300
ranges | CPU IPIs .. | VIO | PCI LSI | MSIs
+-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
VMA0 IPI ESB | | | | | | | | | | | | | | | | | | | | | | | | |
pages +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
A device is passed through and the driver requests MSIs.
We now want the guest to access the HW ESB pages for the requested IRQs
but still the initial IPI ESB pages for the others. Something like below :
0x0 0x1100 0x1200 0x1300
ranges | CPU IPIs .. | VIO | PCI LSI | MSIs
+-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
VMA0 IPI ESB | | | | | | | | | | | | | | | | | | | | | | | | |
pages +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
VMA1 PHB ESB +-------+
pages | | | | |
+-------+
Right, except of course VMA0 will be split into two pieces by
performing the mmap() over it.
quoted
The VMA1 is the result of a new mmap() being done at an offset depending on
the first IRQ number requested by the driver.
Right... that's one way we could do it. But the irq numbers are all
dynamically allocated here, so could we instead just put the
passthrough MSIs in a separate range?
Hmm, yes. These are still MSIs. I am not sure of the benefits. See below.
We'd still need a separate
mmap() for them, but we wouldn't have to deal with mapping over and
unmapping if the device is removed or whatever.
How would we handle multiples devices being hot-plugged, hot-unplugged
and hot-replugged ? The ESB pages would be populated the first time
they are touched and might not be the correct ones if a new device is
hot-plugged to the machine.
quoted
This is because the vm_fault handler uses the page offset to find the
associated KVM IRQ struct containing the addresses of the EOI and trigger
pages in the underlying hardware, which will be the PHB in case of a
passthrough device.
From there, the VMA1 mmap() pointer will be used to create a 'ram device'
memory region which will be mapped on top of the initial ESB memory region
in QEMU. This will override the initial IPI ESB pages with the PHB ESB pages
in the guest ESB address space.
Um.. what? If that qemu memory range is already mapped into the guest
we don't need to create new RAM devices or anything for the
overmapping. If we overmap in qemu that will just get carried into
the guest.
yes, that's the goal.
When the guest accesses the region, the vm_fault handler will be invoked
and the VMA will be populated with the ESB pages of the device being
passthrough. When the device is removed from the machine, we only need
to delete the region from QEMU and munmap() the VMA to clear the mappings.
The underlying pages will be the ones for the XIVE IC IPIs.
And the IRQ numbers can be safely recycled for another passthrough device.
quoted
That's the plan I have in mind as suggested by Paul if I understood it well.
The mechanics are more complex than the patch zapping the PTEs from the VMA
but it's also safer.
Well, yes, where "safer" means "has the possibility to be correct".
Well, the only problem with the kernel approach is keeping a pointer on
the VMA. If we could call find_vma(), it would be perfectly safe and much
more simpler.
C.
From: Paul Mackerras <hidden> Date: 2019-02-08 21:55:15
On Fri, Feb 08, 2019 at 08:58:14AM +0100, Cédric Le Goater wrote:
On 2/8/19 6:15 AM, David Gibson wrote:
quoted
On Thu, Feb 07, 2019 at 10:03:15AM +0100, Cédric Le Goater wrote:
quoted
That's the plan I have in mind as suggested by Paul if I understood it well.
The mechanics are more complex than the patch zapping the PTEs from the VMA
but it's also safer.
Well, yes, where "safer" means "has the possibility to be correct".
Well, the only problem with the kernel approach is keeping a pointer on
the VMA. If we could call find_vma(), it would be perfectly safe and much
more simpler.
You seem to be assuming that the kernel can easily work out a single
virtual address which will be the only place where a given set of
interrupt pages are mapped. But that is really not possible in the
general case, because userspace could have mapped the fd at many
different offsets in many different places.
QEMU doesn't do that; in QEMU, the mmaps are sufficiently limited that
it can work out a single virtual address that needs to be changed.
The way that QEMU should tell the kernel what that address is and what
the mapping should be changed to, is via the existing munmap()/mmap()
interface.
Paul.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-09 09:43:40
On 2/8/19 10:53 PM, Paul Mackerras wrote:
On Fri, Feb 08, 2019 at 08:58:14AM +0100, Cédric Le Goater wrote:
quoted
On 2/8/19 6:15 AM, David Gibson wrote:
quoted
On Thu, Feb 07, 2019 at 10:03:15AM +0100, Cédric Le Goater wrote:
quoted
That's the plan I have in mind as suggested by Paul if I understood it well.
The mechanics are more complex than the patch zapping the PTEs from the VMA
but it's also safer.
Well, yes, where "safer" means "has the possibility to be correct".
Well, the only problem with the kernel approach is keeping a pointer on
the VMA. If we could call find_vma(), it would be perfectly safe and much
more simpler.
You seem to be assuming that the kernel can easily work out a single
virtual address which will be the only place where a given set of
interrupt pages are mapped. But that is really not possible in the
general case, because userspace could have mapped the fd at many
different offsets in many different places.
QEMU doesn't do that; in QEMU, the mmaps are sufficiently limited that
it can work out a single virtual address that needs to be changed.
The way that QEMU should tell the kernel what that address is and what
the mapping should be changed to, is via the existing munmap()/mmap()
interface.
Yes. We agreed on that. QEMU should handle these mappings somewhere in
VFIO. It's me grumbling, that's all.
The discussion has moved to the mmap() interface of the KVM device. The
current proposal adds controls on the device creating fds to mmap() the
TIMA pages and the ESB pages. David is proposing to use directly the fd
of the KVM device to mmap() these pages with a different offset for each
set.
I think that should work pretty well, for passthrough also. The fault
handler should take care of populating the VMA(s) with the appropriate
pages.
We might support END notification one day, so we should have room for
these pages. And nested might require IRQ space extensions at L1.
something to keep in mind.
C.
From: David Gibson <hidden> Date: 2019-02-11 06:28:19
On Sat, Feb 09, 2019 at 10:41:38AM +0100, Cédric Le Goater wrote:
On 2/8/19 10:53 PM, Paul Mackerras wrote:
quoted
On Fri, Feb 08, 2019 at 08:58:14AM +0100, Cédric Le Goater wrote:
quoted
On 2/8/19 6:15 AM, David Gibson wrote:
quoted
On Thu, Feb 07, 2019 at 10:03:15AM +0100, Cédric Le Goater wrote:
quoted
That's the plan I have in mind as suggested by Paul if I understood it well.
The mechanics are more complex than the patch zapping the PTEs from the VMA
but it's also safer.
Well, yes, where "safer" means "has the possibility to be correct".
Well, the only problem with the kernel approach is keeping a pointer on
the VMA. If we could call find_vma(), it would be perfectly safe and much
more simpler.
You seem to be assuming that the kernel can easily work out a single
virtual address which will be the only place where a given set of
interrupt pages are mapped. But that is really not possible in the
general case, because userspace could have mapped the fd at many
different offsets in many different places.
QEMU doesn't do that; in QEMU, the mmaps are sufficiently limited that
it can work out a single virtual address that needs to be changed.
The way that QEMU should tell the kernel what that address is and what
the mapping should be changed to, is via the existing munmap()/mmap()
interface.
Yes. We agreed on that. QEMU should handle these mappings somewhere in
VFIO. It's me grumbling, that's all.
The discussion has moved to the mmap() interface of the KVM device. The
current proposal adds controls on the device creating fds to mmap() the
TIMA pages and the ESB pages. David is proposing to use directly the fd
of the KVM device to mmap() these pages with a different offset for each
set.
I think that should work pretty well, for passthrough also. The fault
handler should take care of populating the VMA(s) with the appropriate
pages.
We might support END notification one day, so we should have room for
these pages. And nested might require IRQ space extensions at L1.
something to keep in mind.
I had some more thoughts on this topic. I think there's been some
confusion because there are more ways of tackling this than I
previously realized:
1) All in kernel
The offset always maps directly to guest irq number and the kernel
somehow binds it either to an IPI or a host irq as necessary.
Cédric's original code attempts this, but the mechanism of keeping a
pointer to the VMA can't work.
But.. remapping the irqs should be sufficiently infrequent that it
might be ok to consider simply stepping through all the hosting
process's VMAs to do this.
2) Remapped in qemu (using memory regions)
I _think_ (in hindsight) was Cédric's been discussing as the
alternative in more recent posts.
Qemu maps the IPI pages at one place and the passthrough IRQ pages
somewhere else. The IPIs are mapped into the guest as one memory
region, then any passthrough IRQ pages are mapped over that using
overlapping memory regions.
I don't think this approach will work well, because it could require a
bunch of separate KVM memory slots, which are fairly scarce.
3) Remapped in qemu (using mmap())
This is the approach I (and I think Paul) have been suggested in
contrast to (1).
Qemu maps the IPI pages and maps those into the guest. When we need
to set up a passthrough IRQ, qemu mmap()s its pages directly over the
IPI pages, and it remains mapped into the guest with the same memory
region / memslot as the IPIs are already using. If the passthrough
device is removed we have to remap the IPI pages back into place.
4) Dedicated irq numbers
We never re-use regular guest irq numbers for passthrough irqs,
instead we put them somewhere else and keep those mapped to the
passthrough irq pages.
I was favouring this approach, but it does mean there will be a guest
visible difference between kernel_irqchip=on and off which isn't
great.
(1) is the most elegant _interface_, but as we've seen it's
problematic to implement. Looking at the for_all_vmas() approach
could be interesting, but otherwise option (3) might be the most
practical.
--
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2019-02-11 06:46:01
On Mon, 2019-02-11 at 13:38 +1100, David Gibson wrote:
1) All in kernel
The offset always maps directly to guest irq number and the kernel
somehow binds it either to an IPI or a host irq as necessary.
Cédric's original code attempts this, but the mechanism of keeping a
pointer to the VMA can't work.
Why do you need a pointer to the VMA anyway ? unmap_mapping_range()
doesn't need a VMA for the unmap part, and faults/mmaps have the VMA.
But.. remapping the irqs should be sufficiently infrequent that it
might be ok to consider simply stepping through all the hosting
process's VMAs to do this.
Which unmap_mapping_range() does for you as I explained previously. You
only need the address space. See how spufs does it (among others).
2) Remapped in qemu (using memory regions)
I _think_ (in hindsight) was Cédric's been discussing as the
alternative in more recent posts.
Qemu maps the IPI pages at one place and the passthrough IRQ pages
somewhere else. The IPIs are mapped into the guest as one memory
region, then any passthrough IRQ pages are mapped over that using
overlapping memory regions.
I don't think this approach will work well, because it could require a
bunch of separate KVM memory slots, which are fairly scarce.
3) Remapped in qemu (using mmap())
This is the approach I (and I think Paul) have been suggested in
contrast to (1).
Qemu maps the IPI pages and maps those into the guest. When we need
to set up a passthrough IRQ, qemu mmap()s its pages directly over the
IPI pages, and it remains mapped into the guest with the same memory
region / memslot as the IPIs are already using. If the passthrough
device is removed we have to remap the IPI pages back into place.
4) Dedicated irq numbers
We never re-use regular guest irq numbers for passthrough irqs,
instead we put them somewhere else and keep those mapped to the
passthrough irq pages.
I was favouring this approach, but it does mean there will be a guest
visible difference between kernel_irqchip=on and off which isn't
great.
(1) is the most elegant _interface_, but as we've seen it's
problematic to implement. Looking at the for_all_vmas() approach
could be interesting, but otherwise option (3) might be the most
practical.
--
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-12 22:18:46
On 2/11/19 7:42 AM, Benjamin Herrenschmidt wrote:
On Mon, 2019-02-11 at 13:38 +1100, David Gibson wrote:
quoted
1) All in kernel
The offset always maps directly to guest irq number and the kernel
somehow binds it either to an IPI or a host irq as necessary.
Cédric's original code attempts this, but the mechanism of keeping a
pointer to the VMA can't work.
Why do you need a pointer to the VMA anyway ? unmap_mapping_range()
doesn't need a VMA for the unmap part, and faults/mmaps have the VMA.
quoted
But.. remapping the irqs should be sufficiently infrequent that it
might be ok to consider simply stepping through all the hosting
process's VMAs to do this.
Which unmap_mapping_range() does for you as I explained previously. You
only need the address space. See how spufs does it (among others).
and the different CAPI drivers. This is much better and it works fine.
On the same topic, the XIVE IC on P10 will use IPI ESB pages for the
PHB interrupts sources. We will still need this kind of remapping but
the pages will be from the same controller.
Thanks,
C.
From: Cédric Le Goater <clg@kaod.org> Date: 2019-02-14 16:52:42
On 2/8/19 6:15 AM, David Gibson wrote:
On Thu, Feb 07, 2019 at 10:13:48AM +0100, Cédric Le Goater wrote:
quoted
On 2/7/19 3:48 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 08:07:36AM +0100, Cédric Le Goater wrote:
quoted
On 2/6/19 2:24 AM, David Gibson wrote:
quoted
On Wed, Feb 06, 2019 at 12:23:29PM +1100, David Gibson wrote:
quoted
On Tue, Feb 05, 2019 at 02:03:11PM +0100, Cédric Le Goater wrote:
quoted
On 2/5/19 6:32 AM, David Gibson wrote:
quoted
On Mon, Feb 04, 2019 at 05:07:28PM +0100, Cédric Le Goater wrote:
quoted
On 2/4/19 6:21 AM, David Gibson wrote:
quoted
On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
quoted
Theses are use to capure the XIVE EAS table of the KVM device, the
configuration of the source targets.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
@@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)return0;}+staticintkvmppc_xive_native_set_eas(structkvmppc_xive*xive,longirq,+u64addr)
I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
values, but rather essentially the "source config" in the terminology
of the PAPR hcalls. Which, yes, is basically implemented by setting
the EAS, but since it's the PAPR architected state that we need to
preserve across migration, I'd prefer to stick as close as we can to
the PAPR terminology.
But we don't have an equivalent name in the PAPR specs for the tuple
(prio, server). We could use the generic 'target' name may be ? even
if this is usually referring to a CPU number.
Um.. what? That's about terminology for one of the fields in this
thing, not about the name for the thing itself.
quoted
Or, IVE (Interrupt Vector Entry) ? which makes some sense.
This is was the former name in HW. I think we recycle it for KVM.
That's a terrible idea, which will make a confusing situation even
more confusing.
Let's use SOURCE_CONFIG and QUEUE_CONFIG. The KVM ioctls are very
similar to the hcalls anyhow.
Yes, I think that's a good idea.
Actually... AIUI the SET_CONFIG hcalls shouldn't be a fast path.
No indeed. I have move them to standard hcalls in the current version.
quoted
Can
we simplify things further by removing the hcall implementation from
the kernel entirely, and have qemu implement them by basically just
forwarding them to the appropriate SET_CONFIG ioctl()?
Yes. I think we could.
Great!
quoted
The hcalls H_INT_SET_SOURCE_CONFIG and H_INT_SET_QUEUE_CONFIG and
the KVM ioctls to set the EQ and the SOURCE configuration have a
lot in common. I need to look at how we can plug the KVM ioctl in
the hcalls under QEMU.
We will have to convert the returned error to respect the PAPR
specs or have the ioctls return H_* errors.
I don't think returning H_* values from a kernel call is a good idea.
Converting errors is kinda ugly, but I still think it's the better
option. Note that we already have something like this for the HPT
resizing hcalls.
ok.
quoted
quoted
Let's dig that idea. If we choose that path, QEMU will have an
up-to-date EAT and so we won't need to synchronize its state anymore
for migration.
I guess so, though I don't see that as essential.
quoted
H_INT_GET_SOURCE_CONFIG can be implemented in QEMU without any KVM
ioctl.
H_INT_GET_QUEUE_INFO could be implemented in QEMU. I need to check
how we return the address of the END ESB in sPAPR. We haven't paid
much attention to these pages because they are not used under Linux
and today the address is returned by OPAL.
H_INT_GET_QUEUE_CONFIG is a little more problematic because we need
to query into the XIVE HW the EQ index and toggle bit. OPAL support
is required for that. But we could reduce the KVM support to the
ioctl querying these EQ information.
Right, and we'd need an ioctl() like that for migration anyway, yes?
Yes. it is the same need.
quoted
quoted
H_INT_ESB could be entirely done under QEMU.
This one can actually happen on fairly hot paths, so I think doing
that in qemu probably isn't a good idea.
I agree It would nice to have some performance.
This hcall is used when LSIs are involved, which is not really a common
configuration. There are no OPAL calls involved. And we are duplicating
code at the KVM level to retrigger the interrupt when the level is still
asserted.
I will benchmark the two options before making a choice.
Ok.
Here are some iperf results for a 4 vCPUs guest running a 5.0.0 kernel
on a small initrd image. I didn't do any kind of tuning like CPU pinning.
So these are really rough figures :
kernel irqchip OFF ON ON (*)
rtl8139 (LSI) 1.19 1.24 1.23 Gbits/sec
VIRTIO 31.80 42.30 -- Gbits/sec
There is not much benefit in handling the H_INT_ESB hcall under KVM it seems.
I think we can leave it under QEMU.
C.