* Re: [PATCH 05/19] KVM: PPC: Book3S HV: add a new KVM device for the XIVE native exploitation mode
From: David Gibson @ 2019-02-05 5:26 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <a4355ddf-462e-57d0-c225-7cd7d02a5482@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 19688 bytes --]
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:
> > On Mon, Jan 07, 2019 at 07:43:17PM +0100, Cédric Le Goater wrote:
> >> 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
> >>
> >> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> >> index 0f98f00da2ea..c522e8274ad9 100644
> >> --- a/arch/powerpc/include/asm/kvm_host.h
> >> +++ b/arch/powerpc/include/asm/kvm_host.h
> >> @@ -220,6 +220,7 @@ extern struct kvm_device_ops kvm_xics_ops;
> >> struct kvmppc_xive;
> >> struct kvmppc_xive_vcpu;
> >> extern struct kvm_device_ops kvm_xive_ops;
> >> +extern struct kvm_device_ops kvm_xive_native_ops;
> >>
> >> struct kvmppc_passthru_irqmap;
> >>
> >> @@ -446,6 +447,7 @@ struct kvmppc_passthru_irqmap {
> >> #define KVMPPC_IRQ_DEFAULT 0
> >> #define KVMPPC_IRQ_MPIC 1
> >> #define KVMPPC_IRQ_XICS 2 /* Includes a XIVE option */
> >> +#define KVMPPC_IRQ_XIVE 3 /* XIVE native exploitation mode */
> >>
> >> #define MMIO_HPTE_CACHE_SIZE 4
> >>
> >> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> >> index eb0d79f0ca45..1bb313f238fe 100644
> >> --- a/arch/powerpc/include/asm/kvm_ppc.h
> >> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> >> @@ -591,6 +591,18 @@ extern int kvmppc_xive_set_icp(struct kvm_vcpu *vcpu, u64 icpval);
> >> extern int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq,
> >> int level, bool line_status);
> >> extern void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu);
> >> +
> >> +static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
> >> +{
> >> + return vcpu->arch.irq_type == KVMPPC_IRQ_XIVE;
> >> +}
> >> +
> >> +extern int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
> >> + struct kvm_vcpu *vcpu, u32 cpu);
> >> +extern void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu);
> >> +extern void kvmppc_xive_native_init_module(void);
> >> +extern void kvmppc_xive_native_exit_module(void);
> >> +
> >> #else
> >> static inline int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
> >> u32 priority) { return -1; }
> >> @@ -614,6 +626,15 @@ static inline int kvmppc_xive_set_icp(struct kvm_vcpu *vcpu, u64 icpval) { retur
> >> static inline int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq,
> >> int level, bool line_status) { return -ENODEV; }
> >> static inline void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu) { }
> >> +
> >> +static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
> >> + { return 0; }
> >> +static inline int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
> >> + struct kvm_vcpu *vcpu, u32 cpu) { return -EBUSY; }
> >> +static inline void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu) { }
> >> +static inline void kvmppc_xive_native_init_module(void) { }
> >> +static inline void kvmppc_xive_native_exit_module(void) { }
> >> +
> >> #endif /* CONFIG_KVM_XIVE */
> >>
> >> /*
> >> diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
> >> index 10c4aa5cd010..5f22415520b4 100644
> >> --- a/arch/powerpc/kvm/book3s_xive.h
> >> +++ b/arch/powerpc/kvm/book3s_xive.h
> >> @@ -12,6 +12,9 @@
> >> #ifdef CONFIG_KVM_XICS
> >> #include "book3s_xics.h"
> >>
> >> +#define KVMPPC_XIVE_FIRST_IRQ 0
> >> +#define KVMPPC_XIVE_NR_IRQS KVMPPC_XICS_NR_IRQS
> >> +
> >> /*
> >> * State for one guest irq source.
> >> *
> >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> >> index 6d4ea4b6c922..52bf74a1616e 100644
> >> --- a/include/uapi/linux/kvm.h
> >> +++ b/include/uapi/linux/kvm.h
> >> @@ -988,6 +988,7 @@ struct kvm_ppc_resize_hpt {
> >> #define KVM_CAP_ARM_VM_IPA_SIZE 165
> >> #define KVM_CAP_MANUAL_DIRTY_LOG_PROTECT 166
> >> #define KVM_CAP_HYPERV_CPUID 167
> >> +#define KVM_CAP_PPC_IRQ_XIVE 168
> >>
> >> #ifdef KVM_CAP_IRQ_ROUTING
> >>
> >> @@ -1211,6 +1212,8 @@ enum kvm_device_type {
> >> #define KVM_DEV_TYPE_ARM_VGIC_V3 KVM_DEV_TYPE_ARM_VGIC_V3
> >> KVM_DEV_TYPE_ARM_VGIC_ITS,
> >> #define KVM_DEV_TYPE_ARM_VGIC_ITS KVM_DEV_TYPE_ARM_VGIC_ITS
> >> + KVM_DEV_TYPE_XIVE,
> >> +#define KVM_DEV_TYPE_XIVE KVM_DEV_TYPE_XIVE
> >> KVM_DEV_TYPE_MAX,
> >> };
> >>
> >> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> >> index bd1a677dd9e4..de7eed191107 100644
> >> --- a/arch/powerpc/kvm/book3s.c
> >> +++ b/arch/powerpc/kvm/book3s.c
> >> @@ -1039,7 +1039,10 @@ static int kvmppc_book3s_init(void)
> >> #ifdef CONFIG_KVM_XIVE
> >> if (xive_enabled()) {
> >> kvmppc_xive_init_module();
> >> + kvmppc_xive_native_init_module();
> >> kvm_register_device_ops(&kvm_xive_ops, KVM_DEV_TYPE_XICS);
> >> + kvm_register_device_ops(&kvm_xive_native_ops,
> >> + KVM_DEV_TYPE_XIVE);
> >> } else
> >> #endif
> >> kvm_register_device_ops(&kvm_xics_ops, KVM_DEV_TYPE_XICS);
> >> @@ -1050,8 +1053,10 @@ static int kvmppc_book3s_init(void)
> >> static void kvmppc_book3s_exit(void)
> >> {
> >> #ifdef CONFIG_KVM_XICS
> >> - if (xive_enabled())
> >> + if (xive_enabled()) {
> >> kvmppc_xive_exit_module();
> >> + kvmppc_xive_native_exit_module();
> >> + }
> >> #endif
> >> #ifdef CONFIG_KVM_BOOK3S_32_HANDLER
> >> kvmppc_book3s_exit_pr();
> >> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> >> new file mode 100644
> >> index 000000000000..115143e76c45
> >> --- /dev/null
> >> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> >> @@ -0,0 +1,332 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2017-2019, IBM Corporation.
> >> + */
> >> +
> >> +#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"
> >> +
> >> +static void xive_native_cleanup_queue(struct kvm_vcpu *vcpu, int prio)
> >> +{
> >> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> >> + struct xive_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;
> >> + }
> >> +}
> >> +
> >> +void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu)
> >> +{
> >> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> >> + int i;
> >> +
> >> + 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;
> >> +}
> >> +
> >> +int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
> >> + struct kvm_vcpu *vcpu, u32 cpu)
> >
> > 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.
> >> +{
> >> + 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.
>
>
> >
> >> + 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)
> >> +{
> >> + ;
> >> +}
> >> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> >> index b90a7d154180..01d526e15e9d 100644
> >> --- a/arch/powerpc/kvm/powerpc.c
> >> +++ b/arch/powerpc/kvm/powerpc.c
> >> @@ -566,6 +566,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >> case KVM_CAP_PPC_ENABLE_HCALL:
> >> #ifdef CONFIG_KVM_XICS
> >> case KVM_CAP_IRQ_XICS:
> >> +#endif
> >> +#ifdef CONFIG_KVM_XIVE
> >> + case KVM_CAP_PPC_IRQ_XIVE:
> >> #endif
> >> case KVM_CAP_PPC_GET_CPU_CHAR:
> >> r = 1;
> >> @@ -753,6 +756,9 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
> >> else
> >> kvmppc_xics_free_icp(vcpu);
> >> break;
> >> + case KVMPPC_IRQ_XIVE:
> >> + kvmppc_xive_native_cleanup_vcpu(vcpu);
> >> + break;
> >> }
> >>
> >> kvmppc_core_vcpu_free(vcpu);
> >> @@ -1941,6 +1947,30 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
> >> break;
> >> }
> >> #endif /* CONFIG_KVM_XICS */
> >> +#ifdef CONFIG_KVM_XIVE
> >> + case KVM_CAP_PPC_IRQ_XIVE: {
> >> + struct fd f;
> >> + struct kvm_device *dev;
> >> +
> >> + r = -EBADF;
> >> + f = fdget(cap->args[0]);
> >> + if (!f.file)
> >> + break;
> >> +
> >> + r = -ENXIO;
> >> + if (!xive_enabled())
> >> + break;
> >> +
> >> + r = -EPERM;
> >> + dev = kvm_device_from_filp(f.file);
> >> + if (dev)
> >> + r = kvmppc_xive_native_connect_vcpu(dev, vcpu,
> >> + cap->args[1]);
> >> +
> >> + fdput(f);
> >> + break;
> >> + }
> >> +#endif /* CONFIG_KVM_XIVE */
> >> #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> >> case KVM_CAP_PPC_FWNMI:
> >> r = -EINVAL;
> >> diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
> >> index 64f1135e7732..806cbe488410 100644
> >> --- a/arch/powerpc/kvm/Makefile
> >> +++ b/arch/powerpc/kvm/Makefile
> >> @@ -99,7 +99,7 @@ endif
> >> kvm-book3s_64-objs-$(CONFIG_KVM_XICS) += \
> >> book3s_xics.o
> >>
> >> -kvm-book3s_64-objs-$(CONFIG_KVM_XIVE) += book3s_xive.o
> >> +kvm-book3s_64-objs-$(CONFIG_KVM_XIVE) += book3s_xive.o book3s_xive_native.o
> >> kvm-book3s_64-objs-$(CONFIG_SPAPR_TCE_IOMMU) += book3s_64_vio.o
> >>
> >> kvm-book3s_64-module-objs := \
> >
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 14/19] KVM: PPC: Book3S HV: add a control to make the XIVE EQ pages dirty
From: David Gibson @ 2019-02-05 5:30 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <c5c3f60a-dfb0-4641-901d-a41f412d80fc@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]
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:
> > 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?
>
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 06/19] KVM: PPC: Book3S HV: add a GET_ESB_FD control to the XIVE native device
From: David Gibson @ 2019-02-05 5:28 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <69791b73-f93e-6957-92e8-5b8620b87731@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 6190 bytes --]
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:
> > On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
> >> 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(+)
> >>
> >> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> >> index 8c876c166ef2..6bb61ba141c2 100644
> >> --- a/arch/powerpc/include/uapi/asm/kvm.h
> >> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> >> @@ -675,4 +675,8 @@ struct kvm_ppc_cpu_char {
> >> #define KVM_XICS_PRESENTED (1ULL << 43)
> >> #define KVM_XICS_QUEUED (1ULL << 44)
> >>
> >> +/* POWER9 XIVE Native Interrupt Controller */
> >> +#define KVM_DEV_XIVE_GRP_CTRL 1
> >> +#define KVM_DEV_XIVE_GET_ESB_FD 1
> >
> > 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.
>
> >> #endif /* __LINUX_KVM_POWERPC_H */
> >> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> >> index 115143e76c45..e20081f0c8d4 100644
> >> --- a/arch/powerpc/kvm/book3s_xive_native.c
> >> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> >> @@ -153,6 +153,85 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
> >> return rc;
> >> }
> >>
> >> +static int xive_native_esb_fault(struct vm_fault *vmf)
> >> +{
> >> + struct vm_area_struct *vma = vmf->vma;
> >> + struct kvmppc_xive *xive = vma->vm_file->private_data;
> >> + struct kvmppc_xive_src_block *sb;
> >> + struct kvmppc_xive_irq_state *state;
> >> + struct xive_irq_data *xd;
> >> + u32 hw_num;
> >> + u16 src;
> >> + u64 page;
> >> + unsigned long irq;
> >> +
> >> + /*
> >> + * Linux/KVM uses a two pages ESB setting, one for trigger and
> >> + * one for EOI
> >> + */
> >> + irq = vmf->pgoff / 2;
> >> +
> >> + sb = kvmppc_xive_find_source(xive, irq, &src);
> >> + if (!sb) {
> >> + pr_err("%s: source %lx not found !\n", __func__, irq);
> >> + return VM_FAULT_SIGBUS;
> >> + }
> >> +
> >> + state = &sb->irq_state[src];
> >> + kvmppc_xive_select_irq(state, &hw_num, &xd);
> >> +
> >> + arch_spin_lock(&sb->lock);
> >> +
> >> + /*
> >> + * first/even page is for trigger
> >> + * second/odd page is for EOI and management.
> >> + */
> >> + 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);
> >> + return VM_FAULT_SIGBUS;
> >> + }
> >> +
> >> + vmf_insert_pfn(vma, vmf->address, page >> PAGE_SHIFT);
> >> + return VM_FAULT_NOPAGE;
> >> +}
> >> +
> >> +static const struct vm_operations_struct xive_native_esb_vmops = {
> >> + .fault = xive_native_esb_fault,
> >> +};
> >> +
> >> +static int xive_native_esb_mmap(struct file *file, struct vm_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;
> >> + return 0;
> >> +}
> >> +
> >> +static const struct file_operations xive_native_esb_fops = {
> >> + .mmap = xive_native_esb_mmap,
> >> +};
> >> +
> >> +static int kvmppc_xive_native_get_esb_fd(struct kvmppc_xive *xive, u64 addr)
> >> +{
> >> + u64 __user *ubufp = (u64 __user *) addr;
> >> + int ret;
> >> +
> >> + ret = anon_inode_getfd("[xive-esb]", &xive_native_esb_fops, xive,
> >> + O_RDWR | O_CLOEXEC);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return put_user(ret, ubufp);
> >> +}
> >> +
> >> static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
> >> struct kvm_device_attr *attr)
> >> {
> >> @@ -162,12 +241,30 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
> >> static int kvmppc_xive_native_get_attr(struct kvm_device *dev,
> >> struct kvm_device_attr *attr)
> >> {
> >> + struct kvmppc_xive *xive = dev->private;
> >> +
> >> + switch (attr->group) {
> >> + case KVM_DEV_XIVE_GRP_CTRL:
> >> + switch (attr->attr) {
> >> + case KVM_DEV_XIVE_GET_ESB_FD:
> >> + return kvmppc_xive_native_get_esb_fd(xive, attr->addr);
> >> + }
> >> + break;
> >> + }
> >> return -ENXIO;
> >> }
> >>
> >> static int kvmppc_xive_native_has_attr(struct kvm_device *dev,
> >> struct kvm_device_attr *attr)
> >> {
> >> + switch (attr->group) {
> >> + case KVM_DEV_XIVE_GRP_CTRL:
> >> + switch (attr->attr) {
> >> + case KVM_DEV_XIVE_GET_ESB_FD:
> >> + return 0;
> >> + }
> >> + 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 17/19] KVM: PPC: Book3S HV: add get/set accessors for the VP XIVE state
From: David Gibson @ 2019-02-05 5:33 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <5ebeb37b-623b-9b82-5628-fd6d78382064@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 3259 bytes --]
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:
> > On Mon, Jan 07, 2019 at 08:10:04PM +0100, Cédric Le Goater wrote:
> >> 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(+)
> >>
> >> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> >> index 4cc897039485..49c488af168c 100644
> >> --- a/arch/powerpc/include/asm/kvm_ppc.h
> >> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> >> @@ -270,6 +270,7 @@ union kvmppc_one_reg {
> >> u64 addr;
> >> u64 length;
> >> } vpaval;
> >> + u64 xive_timaval[4];
> >> };
> >>
> >> struct kvmppc_ops {
> >> @@ -603,6 +604,8 @@ extern void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu);
> >> extern void kvmppc_xive_native_init_module(void);
> >> extern void kvmppc_xive_native_exit_module(void);
> >> extern int kvmppc_xive_native_hcall(struct kvm_vcpu *vcpu, u32 cmd);
> >> +extern int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val);
> >> +extern int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val);
> >>
> >> #else
> >> static inline int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
> >> @@ -637,6 +640,8 @@ static inline void kvmppc_xive_native_init_module(void) { }
> >> static inline void kvmppc_xive_native_exit_module(void) { }
> >> static inline int kvmppc_xive_native_hcall(struct kvm_vcpu *vcpu, u32 cmd)
> >> { return 0; }
> >> +static inline int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val) { return 0; }
> >> +static inline int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val) { return -ENOENT; }
> >
> > 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 09/19] KVM: PPC: Book3S HV: add a SET_SOURCE control to the XIVE native device
From: David Gibson @ 2019-02-05 5:35 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <40c8dc36-0d54-1e66-d766-5580c19ae8fd@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]
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:
> > On Mon, Jan 07, 2019 at 07:43:21PM +0100, Cédric Le Goater wrote:
[snip]
> >> + 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/powernv/npu: Remove redundant change_pte() hook
From: Alistair Popple @ 2019-02-05 3:52 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Alexey Kardashevskiy, Mark Hairgrove, linux-kernel, Peter Xu,
Jerome Glisse, Paul Mackerras, linuxppc-dev, Jason Wang,
David Gibson
In-Reply-To: <20190131171106.GD19324@redhat.com>
On Thursday, 31 January 2019 12:11:06 PM AEDT Andrea Arcangeli wrote:
> On Thu, Jan 31, 2019 at 06:30:22PM +0800, Peter Xu wrote:
> > The change_pte() notifier was designed to use as a quick path to
> > update secondary MMU PTEs on write permission changes or PFN changes.
> > For KVM, it could reduce the vm-exits when vcpu faults on the pages
> > that was touched up by KSM. It's not used to do cache invalidations,
> > for example, if we see the notifier will be called before the real PTE
> > update after all (please see set_pte_at_notify that set_pte_at was
> > called later).
Thanks for the fixup. I didn't realise that invalidate_range() always gets
called but I now see that is the case so this change looks good to me as well.
Reviewed-by: Alistair Popple <alistair@popple.id.au>
> > All the necessary cache invalidation should all be done in
> > invalidate_range() already.
> >
> > CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > CC: Paul Mackerras <paulus@samba.org>
> > CC: Michael Ellerman <mpe@ellerman.id.au>
> > CC: Alistair Popple <alistair@popple.id.au>
> > CC: Alexey Kardashevskiy <aik@ozlabs.ru>
> > CC: Mark Hairgrove <mhairgrove@nvidia.com>
> > CC: Balbir Singh <bsingharora@gmail.com>
> > CC: David Gibson <david@gibson.dropbear.id.au>
> > CC: Andrea Arcangeli <aarcange@redhat.com>
> > CC: Jerome Glisse <jglisse@redhat.com>
> > CC: Jason Wang <jasowang@redhat.com>
> > CC: linuxppc-dev@lists.ozlabs.org
> > CC: linux-kernel@vger.kernel.org
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >
> > arch/powerpc/platforms/powernv/npu-dma.c | 10 ----------
> > 1 file changed, 10 deletions(-)
>
> Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>
>
> It doesn't make sense to implement change_pte as an invalidate,
> change_pte is not compulsory to implement so if one wants to have
> invalidates only, change_pte method shouldn't be implemented in the
> first place and the common code will guarantee to invoke the range
> invalidates instead.
>
> Currently the whole change_pte optimization is effectively disabled as
> noted in past discussions with Jerome (because of the range
> invalidates that always surrounds it), so we need to revisit the whole
> change_pte logic and decide it to re-enable it or to drop it as a
> whole, but in the meantime it's good to cleanup spots like below that
> should leave change_pte alone.
>
> There are several examples of mmu_notifiers_ops in the kernel that
> don't implement change_pte, in fact it's the majority. Of all mmu
> notifier users, only nv_nmmu_notifier_ops, intel_mmuops_change and
> kvm_mmu_notifier_ops implements change_pte and as Peter found out by
> source review nv_nmmu_notifier_ops, intel_mmuops_change are wrong
> about it and should stop implementing it as an invalidate.
>
> In short change_pte is only implemented correctly from KVM which can
> really updates the spte and flushes the TLB but the spte update
> remains and could avoid a vmexit if we figure out how to re-enable the
> optimization safely (the TLB fill after change_pte in KVM EPT/shadow
> secondary MMU will be looked up by the CPU in hardware).
>
> If change_pte is implemented, it should update the mapping like KVM
> does and not do an invalidate.
>
> Thanks,
> Andrea
>
> > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c
> > b/arch/powerpc/platforms/powernv/npu-dma.c index
> > 3f58c7dbd581..c003b29d870e 100644
> > --- a/arch/powerpc/platforms/powernv/npu-dma.c
> > +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> > @@ -917,15 +917,6 @@ static void pnv_npu2_mn_release(struct mmu_notifier
> > *mn,>
> > mmio_invalidate(npu_context, 0, ~0UL);
> >
> > }
> >
> > -static void pnv_npu2_mn_change_pte(struct mmu_notifier *mn,
> > - struct mm_struct *mm,
> > - unsigned long address,
> > - pte_t pte)
> > -{
> > - struct npu_context *npu_context = mn_to_npu_context(mn);
> > - mmio_invalidate(npu_context, address, PAGE_SIZE);
> > -}
> > -
> >
> > static void pnv_npu2_mn_invalidate_range(struct mmu_notifier *mn,
> >
> > struct mm_struct *mm,
> > unsigned long start, unsigned long end)
> >
> > @@ -936,7 +927,6 @@ static void pnv_npu2_mn_invalidate_range(struct
> > mmu_notifier *mn,>
> > static const struct mmu_notifier_ops nv_nmmu_notifier_ops = {
> >
> > .release = pnv_npu2_mn_release,
> >
> > - .change_pte = pnv_npu2_mn_change_pte,
> >
> > .invalidate_range = pnv_npu2_mn_invalidate_range,
> >
> > };
^ permalink raw reply
* Re: [PATCH] scsi: cxlflash: Prevent deadlock when adapter probe fails
From: Martin K. Petersen @ 2019-02-05 3:24 UTC (permalink / raw)
To: Vaibhav Jain
Cc: Uma Krishnan, linux-scsi, stable, Manoj N . Kumar,
Frederic Barrat, linuxppc-dev, Matthew R . Ochs
In-Reply-To: <20190130122651.19255-1-vaibhav@linux.ibm.com>
Vaibhav,
> Presently when an error is encountered during probe of the cxlflash
> adapter, a deadlock is seen with cpu thread stuck inside
> cxlflash_remove(). Below is the trace of the deadlock as logged by
> khungtaskd:
Applied to 5.0/scsi-fixes, thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH 12/17] powerpc/64s/exception: unwind exception-64s.h macros
From: kbuild test robot @ 2019-02-04 23:32 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, kbuild-all, Nicholas Piggin
In-Reply-To: <20190204141828.13043-13-npiggin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]
Hi Nicholas,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.0-rc4 next-20190204]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Nicholas-Piggin/powerpc-64s-tidy-and-gasify-exception-handler-code-round-1/20190205-020038
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-ps3_defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o:(__ftr_alt_97+0xcc): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o: in function `exc_real_0xe60_hmi_exception':
>> (.head.text.real_vectors+0xd7c): undefined reference to `hanndler'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o: in function `exc_virt_0x4300_data_access':
(.head.text.virt_vectors+0x354): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o: in function `exc_virt_0x4380_data_access_slb':
(.head.text.virt_vectors+0x3d4): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o: in function `exc_virt_0x4400_instruction_access':
(.head.text.virt_vectors+0x454): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o: in function `exc_virt_0x4480_instruction_access_slb':
(.head.text.virt_vectors+0x4d4): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o: in function `exc_virt_0x4500_hardware_interrupt':
(.head.text.virt_vectors+0x564): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o:(.head.text.virt_vectors+0x654): more undefined references to `label' follow
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 15103 bytes --]
^ permalink raw reply
* Re: [PATCH v2 10/21] memblock: refactor internal allocation functions
From: Stephen Rothwell @ 2019-02-04 23:08 UTC (permalink / raw)
To: Michael Ellerman
Cc: linux-mm, Andrew Morton, linuxppc-dev, linux-kernel,
Mike Rapoport
In-Reply-To: <878sywndr6.fsf@concordia.ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 3509 bytes --]
Hi all,
On Mon, 04 Feb 2019 19:45:17 +1100 Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Mike Rapoport <rppt@linux.ibm.com> writes:
> > On Sun, Feb 03, 2019 at 08:39:20PM +1100, Michael Ellerman wrote:
> >> Mike Rapoport <rppt@linux.ibm.com> writes:
> >> > Currently, memblock has several internal functions with overlapping
> >> > functionality. They all call memblock_find_in_range_node() to find free
> >> > memory and then reserve the allocated range and mark it with kmemleak.
> >> > However, there is difference in the allocation constraints and in fallback
> >> > strategies.
> ...
> >>
> >> This is causing problems on some of my machines.
> ...
> >>
> >> On some of my other systems it does that, and then panics because it
> >> can't allocate anything at all:
> >>
> >> [ 0.000000] numa: NODE_DATA [mem 0x7ffcaee80-0x7ffcb3fff]
> >> [ 0.000000] numa: NODE_DATA [mem 0x7ffc99d00-0x7ffc9ee7f]
> >> [ 0.000000] numa: NODE_DATA(1) on node 0
> >> [ 0.000000] Kernel panic - not syncing: Cannot allocate 20864 bytes for node 16 data
> >> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc4-gccN-next-20190201-gdc4c899 #1
> >> [ 0.000000] Call Trace:
> >> [ 0.000000] [c0000000011cfca0] [c000000000c11044] dump_stack+0xe8/0x164 (unreliable)
> >> [ 0.000000] [c0000000011cfcf0] [c0000000000fdd6c] panic+0x17c/0x3e0
> >> [ 0.000000] [c0000000011cfd90] [c000000000f61bc8] initmem_init+0x128/0x260
> >> [ 0.000000] [c0000000011cfe60] [c000000000f57940] setup_arch+0x398/0x418
> >> [ 0.000000] [c0000000011cfee0] [c000000000f50a94] start_kernel+0xa0/0x684
> >> [ 0.000000] [c0000000011cff90] [c00000000000af70] start_here_common+0x1c/0x52c
> >> [ 0.000000] Rebooting in 180 seconds..
> >>
> >>
> >> So there's something going wrong there, I haven't had time to dig into
> >> it though (Sunday night here).
> >
> > Yeah, I've misplaced 'nid' and 'MEMBLOCK_ALLOC_ACCESSIBLE' in
> > memblock_phys_alloc_try_nid() :(
> >
> > Can you please check if the below patch fixes the issue on your systems?
>
> Yes it does, thanks.
>
> Tested-by: Michael Ellerman <mpe@ellerman.id.au>
>
> cheers
>
>
> > From 5875b7440e985ce551e6da3cb28aa8e9af697e10 Mon Sep 17 00:00:00 2001
> > From: Mike Rapoport <rppt@linux.ibm.com>
> > Date: Sun, 3 Feb 2019 13:35:42 +0200
> > Subject: [PATCH] memblock: fix parameter order in
> > memblock_phys_alloc_try_nid()
> >
> > The refactoring of internal memblock allocation functions used wrong order
> > of parameters in memblock_alloc_range_nid() call from
> > memblock_phys_alloc_try_nid().
> > Fix it.
> >
> > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > ---
> > mm/memblock.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index e047933..0151a5b 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -1402,8 +1402,8 @@ phys_addr_t __init memblock_phys_alloc_range(phys_addr_t size,
> >
> > phys_addr_t __init memblock_phys_alloc_try_nid(phys_addr_t size, phys_addr_t align, int nid)
> > {
> > - return memblock_alloc_range_nid(size, align, 0, nid,
> > - MEMBLOCK_ALLOC_ACCESSIBLE);
> > + return memblock_alloc_range_nid(size, align, 0,
> > + MEMBLOCK_ALLOC_ACCESSIBLE, nid);
> > }
> >
> > /**
> > --
> > 2.7.4
I have applied that patch to the akpm tree in linux-next from today.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 13/19] KVM: PPC: Book3S HV: add a SYNC control for the XIVE native migration
From: Cédric Le Goater @ 2019-02-04 15:39 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190204051739.GF1927@umbus.fritz.box>
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:
>> 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.
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> arch/powerpc/include/uapi/asm/kvm.h | 1 +
>> arch/powerpc/kvm/book3s_xive_native.c | 32 +++++++++++++++++++++++++++
>> 2 files changed, 33 insertions(+)
>>
>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>> index 6fc9660c5aec..f3b859223b80 100644
>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>> @@ -681,6 +681,7 @@ struct kvm_ppc_cpu_char {
>> #define KVM_DEV_XIVE_GET_TIMA_FD 2
>> #define KVM_DEV_XIVE_VC_BASE 3
>> #define KVM_DEV_XIVE_GRP_SOURCES 2 /* 64-bit source attributes */
>> +#define KVM_DEV_XIVE_GRP_SYNC 3 /* 64-bit source attributes */
>>
>> /* Layout of 64-bit XIVE source attribute values */
>> #define KVM_XIVE_LEVEL_SENSITIVE (1ULL << 0)
>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>> index 4ca75aade069..a8052867afc1 100644
>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
>> @@ -459,6 +459,35 @@ static int kvmppc_xive_native_set_source(struct kvmppc_xive *xive, long irq,
>> return 0;
>> }
>>
>> +static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)
>> +{
>> + struct kvmppc_xive_src_block *sb;
>> + struct kvmppc_xive_irq_state *state;
>> + struct xive_irq_data *xd;
>> + u32 hw_num;
>> + u16 src;
>> +
>> + 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);
>> + return 0;
>> +}
>> +
>> static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
>> struct kvm_device_attr *attr)
>> {
>> @@ -474,6 +503,8 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
>> case KVM_DEV_XIVE_GRP_SOURCES:
>> return kvmppc_xive_native_set_source(xive, attr->attr,
>> attr->addr);
>> + case KVM_DEV_XIVE_GRP_SYNC:
>> + return kvmppc_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;
>> case KVM_DEV_XIVE_GRP_SOURCES:
>> + case KVM_DEV_XIVE_GRP_SYNC:
>> if (attr->attr >= KVMPPC_XIVE_FIRST_IRQ &&
>> attr->attr < KVMPPC_XIVE_NR_IRQS)
>> return 0;
>
^ permalink raw reply
* Re: [PATCH 02/17] powerpc/64s/exception: remove H concatenation for EXC_HV variants
From: kbuild test robot @ 2019-02-04 21:55 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, kbuild-all, Nicholas Piggin
In-Reply-To: <20190204141828.13043-3-npiggin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]
Hi Nicholas,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.0-rc4 next-20190204]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Nicholas-Piggin/powerpc-64s-tidy-and-gasify-exception-handler-code-round-1/20190205-020038
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-ps3_defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
>> powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o:(__ftr_alt_97+0x104): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o: in function `exc_virt_0x4300_data_access':
>> (.head.text.virt_vectors+0x354): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o: in function `exc_virt_0x4380_data_access_slb':
(.head.text.virt_vectors+0x3d4): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o: in function `exc_virt_0x4400_instruction_access':
(.head.text.virt_vectors+0x454): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o: in function `exc_virt_0x4480_instruction_access_slb':
(.head.text.virt_vectors+0x4d4): undefined reference to `label'
powerpc64-linux-gnu-ld: arch/powerpc/kernel/head_64.o:(.head.text.virt_vectors+0x564): more undefined references to `label' follow
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 15103 bytes --]
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Michael S. Tsirkin @ 2019-02-04 21:38 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Jean-Philippe Brucker, Jason Wang, Alexey Kardashevskiy, Ram Pai,
linux-kernel, virtualization, iommu, linuxppc-dev,
Christoph Hellwig, David Gibson
In-Reply-To: <875ztzxvw2.fsf@morokweng.localdomain>
On Mon, Feb 04, 2019 at 04:15:41PM -0200, Thiago Jung Bauermann wrote:
>
> Christoph Hellwig <hch@lst.de> writes:
>
> > On Tue, Jan 29, 2019 at 09:36:08PM -0500, Michael S. Tsirkin wrote:
> >> This has been discussed ad nauseum. virtio is all about compatibility.
> >> Losing a couple of lines of code isn't worth breaking working setups.
> >> People that want "just use DMA API no tricks" now have the option.
> >> Setting a flag in a feature bit map is literally a single line
> >> of code in the hypervisor. So stop pushing for breaking working
> >> legacy setups and just fix it in the right place.
> >
> > I agree with the legacy aspect. What I am missing is an extremely
> > strong wording that says you SHOULD always set this flag for new
> > hosts, including an explanation why.
>
> My understanding of ACCESS_PLATFORM is that it means "this device will
> behave in all aspects like a regular device attached to this bus".
Not really. Look it up in the spec:
VIRTIO_F_ACCESS_PLATFORM(33) This feature indicates that the device can be used on a platform
where device access to data in memory is limited and/or translated. E.g. this is the case if the device
can be located behind an IOMMU that translates bus addresses from the device into physical addresses
in memory, if the device can be limited to only access certain memory addresses or if special commands
such as a cache flush can be needed to synchronise data in memory with the device. Whether accesses
are actually limited or translated is described by platform-specific means. If this feature bit is set to 0,
then the device has same access to memory addresses supplied to it as the driver has. In particular, the
device will always use physical addresses matching addresses used by the driver (typically meaning
physical addresses used by the CPU) and not translated further, and can access any address supplied
to it by the driver. When clear, this overrides any platform-specific description of whether device access
is limited or translated in any way, e.g. whether an IOMMU may be present.
> Is
> that it? Therefore it should be set because it's the sane thing to do?
It's the sane thing to do unless you want the very specific thing that
having it clear means, which is just have it be another CPU.
It was designed to make, when set, as many guests as we can work
correctly, and it seems to be successful in doing exactly that.
Unfortunately there could be legacy guests that do work correctly but
become slow. Whether trying to somehow work around that
can paint us into a corner where things again don't
work for some people is a question worth discussing.
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH] soc: fsl: dpio: Use after free in dpaa2_dpio_remove()
From: Li Yang @ 2019-02-04 20:59 UTC (permalink / raw)
To: Dan Carpenter
Cc: linuxppc-dev, Roy Pledge, kernel-janitors,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Ioana Ciornei
In-Reply-To: <20190204141005.GA915@kadam>
On Mon, Feb 4, 2019 at 8:12 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> The dpaa2_io_down(priv->io) call frees "priv->io" so I've shifted the
> code around a little bit to avoid the use after free.
>
> Fixes: 991e873223e9 ("soc: fsl: dpio: use a cpumask to identify which cpus are unused")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied. Thanks.
> ---
> drivers/soc/fsl/dpio/dpio-driver.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/soc/fsl/dpio/dpio-driver.c b/drivers/soc/fsl/dpio/dpio-driver.c
> index 2d4af32a0dec..a28799b62d53 100644
> --- a/drivers/soc/fsl/dpio/dpio-driver.c
> +++ b/drivers/soc/fsl/dpio/dpio-driver.c
> @@ -220,12 +220,12 @@ static int dpaa2_dpio_remove(struct fsl_mc_device *dpio_dev)
>
> dev = &dpio_dev->dev;
> priv = dev_get_drvdata(dev);
> + cpu = dpaa2_io_get_cpu(priv->io);
>
> dpaa2_io_down(priv->io);
>
> dpio_teardown_irqs(dpio_dev);
>
> - cpu = dpaa2_io_get_cpu(priv->io);
> cpumask_set_cpu(cpu, cpus_unused_mask);
>
> err = dpio_open(dpio_dev->mc_io, 0, dpio_dev->obj_desc.id,
> --
> 2.17.1
>
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Michael S. Tsirkin @ 2019-02-04 20:23 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Jean-Philippe Brucker, Jason Wang, Alexey Kardashevskiy, Ram Pai,
linux-kernel, virtualization, iommu, linuxppc-dev,
Christoph Hellwig, David Gibson
In-Reply-To: <877eefxvyb.fsf@morokweng.localdomain>
On Mon, Feb 04, 2019 at 04:14:20PM -0200, Thiago Jung Bauermann wrote:
>
> Hello Michael,
>
> Michael S. Tsirkin <mst@redhat.com> writes:
>
> > On Tue, Jan 29, 2019 at 03:42:44PM -0200, Thiago Jung Bauermann wrote:
> >>
> >> Fixing address of powerpc mailing list.
> >>
> >> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
> >>
> >> > Hello,
> >> >
> >> > With Christoph's rework of the DMA API that recently landed, the patch
> >> > below is the only change needed in virtio to make it work in a POWER
> >> > secure guest under the ultravisor.
> >> >
> >> > The other change we need (making sure the device's dma_map_ops is NULL
> >> > so that the dma-direct/swiotlb code is used) can be made in
> >> > powerpc-specific code.
> >> >
> >> > Of course, I also have patches (soon to be posted as RFC) which hook up
> >> > <linux/mem_encrypt.h> to the powerpc secure guest support code.
> >> >
> >> > What do you think?
> >> >
> >> > From d0629a36a75c678b4a72b853f8f7f8c17eedd6b3 Mon Sep 17 00:00:00 2001
> >> > From: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> >> > Date: Thu, 24 Jan 2019 22:08:02 -0200
> >> > Subject: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
> >> >
> >> > The host can't access the guest memory when it's encrypted, so using
> >> > regular memory pages for the ring isn't an option. Go through the DMA API.
> >> >
> >> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> >
> > Well I think this will come back to bite us (witness xen which is now
> > reworking precisely this path - but at least they aren't to blame, xen
> > came before ACCESS_PLATFORM).
> >
> > I also still think the right thing would have been to set
> > ACCESS_PLATFORM for all systems where device can't access all memory.
>
> I understand. The problem with that approach for us is that because we
> don't know which guests will become secure guests and which will remain
> regular guests, QEMU would need to offer ACCESS_PLATFORM to all guests.
>
> And the problem with that is that for QEMU on POWER, having
> ACCESS_PLATFORM turned off means that it can bypass the IOMMU for the
> device (which makes sense considering that the name of the flag was
> IOMMU_PLATFORM). And we need that for regular guests to avoid
> performance degradation.
You don't really, ACCESS_PLATFORM means just that, platform decides.
> So while ACCESS_PLATFORM solves our problems for secure guests, we can't
> turn it on by default because we can't affect legacy systems. Doing so
> would penalize existing systems that can access all memory. They would
> all have to unnecessarily go through address translations, and take a
> performance hit.
So as step one, you just give hypervisor admin an option to run legacy
systems faster by blocking secure mode. I don't see why that is
so terrible.
But as step two, assuming you use above step one to make legacy
guests go fast - maybe there is a point in detecting
such a hypervisor and doing something smarter with it.
By all means let's have a discussion around this but that is no longer
"to make it work" as the commit log says it's more a performance
optimization.
> The semantics of ACCESS_PLATFORM assume that the hypervisor/QEMU knows
> in advance - right when the VM is instantiated - that it will not have
> access to all guest memory.
Not quite. It just means that hypervisor can live with not having
access to all memory. If platform wants to give it access
to all memory that is quite all right.
> Unfortunately that assumption is subtly
> broken on our secure-platform. The hypervisor/QEMU realizes that the
> platform is going secure only *after the VM is instantiated*. It's the
> kernel running in the VM that determines that it wants to switch the
> platform to secure-mode.
ACCESS_PLATFORM is there so guests can detect legacy hypervisors
which always assumed it's another CPU.
> Another way of looking at this issue which also explains our reluctance
> is that the only difference between a secure guest and a regular guest
> (at least regarding virtio) is that the former uses swiotlb while the
> latter doens't.
But swiotlb is just one implementation. It's a guest internal thing. The
issue is that memory isn't host accessible. Yes linux does not use that
info too much right now but it already begins to seep out of the
abstraction. For example as you are doing data copies you should maybe
calculate the packet checksum just as well. Not something DMA API will
let you know right now, but that's because any bounce buffer users so
far weren't terribly fast anyway - it was all for 16 bit hardware and
such.
> And from the device's point of view they're
> indistinguishable. It can't tell one guest that is using swiotlb from
> one that isn't. And that implies that secure guest vs regular guest
> isn't a virtio interface issue, it's "guest internal affairs". So
> there's no reason to reflect that in the feature flags.
So don't. The way not to reflect that in the feature flags is
to set ACCESS_PLATFORM. Then you say *I don't care let platform device*.
Without ACCESS_PLATFORM
virtio has a very specific opinion about the security of the
device, and that opinion is that device is part of the guest
supervisor security domain.
> That said, we still would like to arrive at a proper design for this
> rather than add yet another hack if we can avoid it. So here's another
> proposal: considering that the dma-direct code (in kernel/dma/direct.c)
> automatically uses swiotlb when necessary (thanks to Christoph's recent
> DMA work), would it be ok to replace virtio's own direct-memory code
> that is used in the !ACCESS_PLATFORM case with the dma-direct code? That
> way we'll get swiotlb even with !ACCESS_PLATFORM, and virtio will get a
> code cleanup (replace open-coded stuff with calls to existing
> infrastructure).
Let's say I have some doubts that there's an API that
matches what virtio with its bag of legacy compatibility exactly.
But taking a step back you seem to keep looking at it at the code level.
And I think that's not necessarily right. If ACCESS_PLATFORM isn't what you
are looking for then maybe you need another feature bit.
But you/we need to figure out what it means first.
> > But I also think I don't have the energy to argue about power secure
> > guest anymore. So be it for power secure guest since the involved
> > engineers disagree with me. Hey I've been wrong in the past ;).
>
> Yeah, it's been a difficult discussion. Thanks for still engaging!
> I honestly thought that this patch was a good solution (if the guest has
> encrypted memory it means that the DMA API needs to be used), but I can
> see where you are coming from. As I said, we'd like to arrive at a good
> solution if possible.
>
> > But the name "sev_active" makes me scared because at least AMD guys who
> > were doing the sensible thing and setting ACCESS_PLATFORM
>
> My understanding is, AMD guest-platform knows in advance that their
> guest will run in secure mode and hence sets the flag at the time of VM
> instantiation. Unfortunately we dont have that luxury on our platforms.
Well you do have that luxury. It looks like that there are existing
guests that already acknowledge ACCESS_PLATFORM and you are not happy
with how that path is slow. So you are trying to optimize for
them by clearing ACCESS_PLATFORM and then you have lost ability
to invoke DMA API.
For example if there was another flag just like ACCESS_PLATFORM
just not yet used by anyone, you would be all fine using that right?
Is there any justification to doing that beyond someone putting
out slow code in the past?
> > (unless I'm
> > wrong? I reemember distinctly that's so) will likely be affected too.
> > We don't want that.
> >
> > So let's find a way to make sure it's just power secure guest for now
> > pls.
>
> Yes, my understanding is that they turn ACCESS_PLATFORM on. And because
> of that, IIUC this patch wouldn't affect them because in their platform
> vring_use_dma_api() returns true earlier in the
> "if !virtio_has_iommu_quirk(vdev)" condition.
Let's just say I don't think we should assume how the specific hypervisor
behaves. It seems to follow the spec and so should Linux.
> > I also think we should add a dma_api near features under virtio_device
> > such that these hacks can move off data path.
>
> Sorry, I don't understand this.
I mean we can set a flag within struct virtio_device instead
of poking at features checking xen etc etc.
> > By the way could you please respond about virtio-iommu and
> > why there's no support for ACCESS_PLATFORM on power?
>
> There is support for ACCESS_PLATFORM on POWER. We don't enable it
> because it causes a performance hit.
For legacy guests.
> > I have Cc'd you on these discussions.
>
> I'm having a look at the spec and the patches, but to be honest I'm not
> the best powerpc guy for this. I'll see if I can get others to have a
> look.
>
> > Thanks!
>
> Thanks as well!
>
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: soc: fsl: Document Qixis FPGA usage
From: Li Yang @ 2019-02-04 20:12 UTC (permalink / raw)
To: Pankaj Bansal
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring, linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190204143449.25040-2-pankaj.bansal@nxp.com>
Please include device tree binding mailing list and maintainers for
binding patches(cc'ed now).
On Mon, Feb 4, 2019 at 3:15 AM Pankaj Bansal <pankaj.bansal@nxp.com> wrote:
>
> an FPGA-based system controller, called “Qixis”, which
> manages several critical system features, including:
> • Reset sequencing
> • Power supply configuration
> • Board configuration
> • hardware configuration
>
> The qixis registers are accessible over one or more system-specific
> interfaces, typically I2C, JTAG or an embedded processor.
>
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>
> Notes:
> V3:
> - Added boardname based compatible field in bindings
> - Added bindings for MMIO based FPGA
> V2:
> - No change
>
> .../bindings/soc/fsl/qixis_ctrl.txt | 53 ++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/soc/fsl/qixis_ctrl.txt b/Documentation/devicetree/bindings/soc/fsl/qixis_ctrl.txt
> new file mode 100644
> index 000000000000..5d510df14be8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/fsl/qixis_ctrl.txt
> @@ -0,0 +1,53 @@
> +* QIXIS FPGA block
> +
> +an FPGA-based system controller, called “Qixis”, which
> +manages several critical system features, including:
> +• Configuration switch monitoring
> +• Power on/off sequencing
> +• Reset sequencing
> +• Power supply configuration
> +• Board configuration
> +• hardware configuration
> +• Background power data collection (DCM)
> +• Fault monitoring
> +• RCW bypass SRAM (replace flash RCW with internal RCW) (NOR only)
> +• Dedicated functional validation blocks (POSt/IRS, triggered event, and so on)
> +• I2C master for remote board control even with no DUT available
> +
> +The qixis registers are accessible over one or more system-specific interfaces,
> +typically I2C, JTAG or an embedded processor.
> +
> +FPGA connected to I2C:
> +Required properties:
> +
> + - compatible: should be a board-specific string followed by a string
> + indicating the type of FPGA. Example:
> + "fsl,<board>-fpga", "fsl,fpga-qixis-i2c"
> + - reg : i2c address of the qixis device.
> +
> +Example (LX2160A-QDS):
> + /* The FPGA node */
> + fpga@66 {
> + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
> + reg = <0x66>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + }
> +
> +* Freescale on-board FPGA
> +
> +This is the memory-mapped registers for on board FPGA.
> +
> +Required properties:
> +- compatible: should be a board-specific string followed by a string
> + indicating the type of FPGA. Example:
> + "fsl,<board>-fpga", "fsl,fpga-qixis"
> +- reg: should contain the address and the length of the FPGA register set.
> +
> +Example (LS2080A-RDB):
> +
> + cpld@3,0 {
> + compatible = "fsl,ls2080ardb-fpga", "fsl,fpga-qixis";
> + reg = <0x3 0 0x10000>;
> + };
> +
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 09/19] KVM: PPC: Book3S HV: add a SET_SOURCE control to the XIVE native device
From: Cédric Le Goater @ 2019-02-04 19:07 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190204045751.GD1927@umbus.fritz.box>
On 2/4/19 5:57 AM, David Gibson wrote:
> On Mon, Jan 07, 2019 at 07:43:21PM +0100, Cédric Le Goater wrote:
>> Interrupt sources are simply created at the OPAL level and then
>> MASKED. KVM only needs to know about their type: LSI or MSI.
>
> This commit message isn't very illuminating.
There is room for improvement certainly.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> arch/powerpc/include/uapi/asm/kvm.h | 5 +
>> arch/powerpc/kvm/book3s_xive_native.c | 98 +++++++++++++++++++
>> .../powerpc/kvm/book3s_xive_native_template.c | 27 +++++
>> 3 files changed, 130 insertions(+)
>> create mode 100644 arch/powerpc/kvm/book3s_xive_native_template.c
>>
>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>> index 8b78b12aa118..6fc9660c5aec 100644
>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>> @@ -680,5 +680,10 @@ struct kvm_ppc_cpu_char {
>> #define KVM_DEV_XIVE_GET_ESB_FD 1
>> #define KVM_DEV_XIVE_GET_TIMA_FD 2
>> #define KVM_DEV_XIVE_VC_BASE 3
>> +#define KVM_DEV_XIVE_GRP_SOURCES 2 /* 64-bit source attributes */
>> +
>> +/* Layout of 64-bit XIVE source attribute values */
>> +#define KVM_XIVE_LEVEL_SENSITIVE (1ULL << 0)
>> +#define KVM_XIVE_LEVEL_ASSERTED (1ULL << 1)
>>
>> #endif /* __LINUX_KVM_POWERPC_H */
>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>> index 29a62914de55..2518640d4a58 100644
>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
>> @@ -31,6 +31,24 @@
>>
>> #include "book3s_xive.h"
>>
>> +/*
>> + * We still instantiate them here because we use some of the
>> + * generated utility functions as well in this file.
>
> And this comment is downright cryptic.
I have removed this part now that the hcalls are not done under
real mode anymore.
>
>> + */
>> +#define XIVE_RUNTIME_CHECKS
>> +#define X_PFX xive_vm_
>> +#define X_STATIC static
>> +#define X_STAT_PFX stat_vm_
>> +#define __x_tima xive_tima
>> +#define __x_eoi_page(xd) ((void __iomem *)((xd)->eoi_mmio))
>> +#define __x_trig_page(xd) ((void __iomem *)((xd)->trig_mmio))
>> +#define __x_writeb __raw_writeb
>> +#define __x_readw __raw_readw
>> +#define __x_readq __raw_readq
>> +#define __x_writeq __raw_writeq
>> +
>> +#include "book3s_xive_native_template.c"
>> +
>> static void xive_native_cleanup_queue(struct kvm_vcpu *vcpu, int prio)
>> {
>> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>> @@ -305,6 +323,78 @@ static int kvmppc_xive_native_get_tima_fd(struct kvmppc_xive *xive, u64 addr)
>> return put_user(ret, ubufp);
>> }
>>
>> +static int kvmppc_xive_native_set_source(struct kvmppc_xive *xive, long irq,
>> + u64 addr)
>> +{
>> + struct kvmppc_xive_src_block *sb;
>> + struct kvmppc_xive_irq_state *state;
>> + u64 __user *ubufp = (u64 __user *) addr;
>> + u64 val;
>> + u16 idx;
>> +
>> + pr_devel("%s irq=0x%lx\n", __func__, irq);
>> +
>> + if (irq < KVMPPC_XIVE_FIRST_IRQ || irq >= KVMPPC_XIVE_NR_IRQS)
>> + return -ENOENT;
>> +
>> + sb = kvmppc_xive_find_source(xive, irq, &idx);
>> + if (!sb) {
>> + pr_debug("No source, creating source block...\n");
>
> Doesn't this need to be protected by some lock?
>
>> + 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.
>
>> + 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)
>> {
>> @@ -317,6 +407,9 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
>> return kvmppc_xive_native_set_vc_base(xive, attr->addr);
>> }
>> break;
>> + case KVM_DEV_XIVE_GRP_SOURCES:
>> + return kvmppc_xive_native_set_source(xive, attr->attr,
>> + attr->addr);
>> }
>> return -ENXIO;
>> }
>> @@ -353,6 +446,11 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,
>> return 0;
>> }
>> break;
>> + case KVM_DEV_XIVE_GRP_SOURCES:
>> + if (attr->attr >= KVMPPC_XIVE_FIRST_IRQ &&
>> + attr->attr < KVMPPC_XIVE_NR_IRQS)
>> + return 0;
>> + break;
>> }
>> return -ENXIO;
>> }
>> diff --git a/arch/powerpc/kvm/book3s_xive_native_template.c b/arch/powerpc/kvm/book3s_xive_native_template.c
>> new file mode 100644
>> index 000000000000..e7260da4a596
>> --- /dev/null
>> +++ b/arch/powerpc/kvm/book3s_xive_native_template.c
>> @@ -0,0 +1,27 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2017-2019, IBM Corporation.
>> + */
>> +
>> +/* File to be included by other .c files */
>> +
>> +#define XGLUE(a, b) a##b
>> +#define GLUE(a, b) XGLUE(a, b)
>> +
>> +/*
>> + * TODO: introduce a common template file with the XIVE native layer
>> + * and the XICS-on-XIVE glue for the utility functions
>> + */
>> +static u8 GLUE(X_PFX, esb_load)(struct xive_irq_data *xd, u32 offset)
>> +{
>> + u64 val;
>> +
>> + 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;
>> +}
>
^ permalink raw reply
* Re: [PATCH 17/19] KVM: PPC: Book3S HV: add get/set accessors for the VP XIVE state
From: Cédric Le Goater @ 2019-02-04 18:57 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190204052646.GJ1927@umbus.fritz.box>
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:
>> 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(+)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
>> index 4cc897039485..49c488af168c 100644
>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>> @@ -270,6 +270,7 @@ union kvmppc_one_reg {
>> u64 addr;
>> u64 length;
>> } vpaval;
>> + u64 xive_timaval[4];
>> };
>>
>> struct kvmppc_ops {
>> @@ -603,6 +604,8 @@ extern void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu);
>> extern void kvmppc_xive_native_init_module(void);
>> extern void kvmppc_xive_native_exit_module(void);
>> extern int kvmppc_xive_native_hcall(struct kvm_vcpu *vcpu, u32 cmd);
>> +extern int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val);
>> +extern int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val);
>>
>> #else
>> static inline int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
>> @@ -637,6 +640,8 @@ static inline void kvmppc_xive_native_init_module(void) { }
>> static inline void kvmppc_xive_native_exit_module(void) { }
>> static inline int kvmppc_xive_native_hcall(struct kvm_vcpu *vcpu, u32 cmd)
>> { return 0; }
>> +static inline int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val) { return 0; }
>> +static inline int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val) { return -ENOENT; }
>
> 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.
>
>> #endif /* CONFIG_KVM_XIVE */
>>
>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>> index 95302558ce10..3c958c39a782 100644
>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>> @@ -480,6 +480,8 @@ struct kvm_ppc_cpu_char {
>> #define KVM_REG_PPC_ICP_PPRI_SHIFT 16 /* pending irq priority */
>> #define KVM_REG_PPC_ICP_PPRI_MASK 0xff
>>
>> +#define KVM_REG_PPC_VP_STATE (KVM_REG_PPC | KVM_REG_SIZE_U256 | 0x8d)
>> +
>> /* Device control API: PPC-specific devices */
>> #define KVM_DEV_MPIC_GRP_MISC 1
>> #define KVM_DEV_MPIC_BASE_ADDR 0 /* 64-bit */
>> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
>> index de7eed191107..5ad658077a35 100644
>> --- a/arch/powerpc/kvm/book3s.c
>> +++ b/arch/powerpc/kvm/book3s.c
>> @@ -641,6 +641,18 @@ int kvmppc_get_one_reg(struct kvm_vcpu *vcpu, u64 id,
>> *val = get_reg_val(id, kvmppc_xics_get_icp(vcpu));
>> break;
>> #endif /* CONFIG_KVM_XICS */
>> +#ifdef CONFIG_KVM_XIVE
>> + case KVM_REG_PPC_VP_STATE:
>> + if (!vcpu->arch.xive_vcpu) {
>> + r = -ENXIO;
>> + break;
>> + }
>> + if (xive_enabled())
>> + r = kvmppc_xive_native_get_vp(vcpu, val);
>> + else
>> + r = -ENXIO;
>> + break;
>> +#endif /* CONFIG_KVM_XIVE */
>> case KVM_REG_PPC_FSCR:
>> *val = get_reg_val(id, vcpu->arch.fscr);
>> break;
>> @@ -714,6 +726,18 @@ int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id,
>> r = kvmppc_xics_set_icp(vcpu, set_reg_val(id, *val));
>> break;
>> #endif /* CONFIG_KVM_XICS */
>> +#ifdef CONFIG_KVM_XIVE
>> + case KVM_REG_PPC_VP_STATE:
>> + if (!vcpu->arch.xive_vcpu) {
>> + r = -ENXIO;
>> + break;
>> + }
>> + if (xive_enabled())
>> + r = kvmppc_xive_native_set_vp(vcpu, val);
>> + else
>> + r = -ENXIO;
>> + break;
>> +#endif /* CONFIG_KVM_XIVE */
>> case KVM_REG_PPC_FSCR:
>> vcpu->arch.fscr = set_reg_val(id, *val);
>> break;
>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>> index f4eb71eafc57..1aefb366df0b 100644
>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>> +++ b/arch/powerpc/kvm/book3s_xive_native.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)
>> +
>> +int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val)
>> +{
>> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>> + u64 opal_state;
>> + int rc;
>> +
>> + 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;
>> +
>> + /*
>> + * Return the OS CAM line to print out the VP identifier in
>> + * the QEMU monitor. This is not restored.
>> + */
>> + 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)
>> + return rc;
>> +
>> + /*
>> + * Capture the backup of IPB register in the NVT structure and
>> + * merge it in our KVM VP state.
>> + *
>> + * TODO: P10 support.
>> + */
>> + 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);
>> +
>> + return 0;
>> +}
>> +
>> +int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val)
>> +{
>> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>> + struct kvmppc_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];
>> +
>> + /*
>> + * There is no need to restore the XIVE internal state (IPB
>> + * stored in the NVT) as the IPB register was merged in KVM VP
>> + * state.
>> + */
>> + return 0;
>> +}
>> +
>> static int kvmppc_xive_native_set_source(struct kvmppc_xive *xive, long irq,
>> u64 addr)
>> {
>
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Thiago Jung Bauermann @ 2019-02-04 18:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Michael S. Tsirkin, Jean-Philippe Brucker, Jason Wang,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
iommu, linuxppc-dev, David Gibson
In-Reply-To: <20190130074427.GA29516@lst.de>
Christoph Hellwig <hch@lst.de> writes:
> On Tue, Jan 29, 2019 at 09:36:08PM -0500, Michael S. Tsirkin wrote:
>> This has been discussed ad nauseum. virtio is all about compatibility.
>> Losing a couple of lines of code isn't worth breaking working setups.
>> People that want "just use DMA API no tricks" now have the option.
>> Setting a flag in a feature bit map is literally a single line
>> of code in the hypervisor. So stop pushing for breaking working
>> legacy setups and just fix it in the right place.
>
> I agree with the legacy aspect. What I am missing is an extremely
> strong wording that says you SHOULD always set this flag for new
> hosts, including an explanation why.
My understanding of ACCESS_PLATFORM is that it means "this device will
behave in all aspects like a regular device attached to this bus". Is
that it? Therefore it should be set because it's the sane thing to do?
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Thiago Jung Bauermann @ 2019-02-04 18:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jean-Philippe Brucker, Jason Wang, Alexey Kardashevskiy, Ram Pai,
linux-kernel, virtualization, iommu, linuxppc-dev,
Christoph Hellwig, David Gibson
In-Reply-To: <20190129134750-mutt-send-email-mst@kernel.org>
Hello Michael,
Michael S. Tsirkin <mst@redhat.com> writes:
> On Tue, Jan 29, 2019 at 03:42:44PM -0200, Thiago Jung Bauermann wrote:
>>
>> Fixing address of powerpc mailing list.
>>
>> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>>
>> > Hello,
>> >
>> > With Christoph's rework of the DMA API that recently landed, the patch
>> > below is the only change needed in virtio to make it work in a POWER
>> > secure guest under the ultravisor.
>> >
>> > The other change we need (making sure the device's dma_map_ops is NULL
>> > so that the dma-direct/swiotlb code is used) can be made in
>> > powerpc-specific code.
>> >
>> > Of course, I also have patches (soon to be posted as RFC) which hook up
>> > <linux/mem_encrypt.h> to the powerpc secure guest support code.
>> >
>> > What do you think?
>> >
>> > From d0629a36a75c678b4a72b853f8f7f8c17eedd6b3 Mon Sep 17 00:00:00 2001
>> > From: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> > Date: Thu, 24 Jan 2019 22:08:02 -0200
>> > Subject: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
>> >
>> > The host can't access the guest memory when it's encrypted, so using
>> > regular memory pages for the ring isn't an option. Go through the DMA API.
>> >
>> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>
> Well I think this will come back to bite us (witness xen which is now
> reworking precisely this path - but at least they aren't to blame, xen
> came before ACCESS_PLATFORM).
>
> I also still think the right thing would have been to set
> ACCESS_PLATFORM for all systems where device can't access all memory.
I understand. The problem with that approach for us is that because we
don't know which guests will become secure guests and which will remain
regular guests, QEMU would need to offer ACCESS_PLATFORM to all guests.
And the problem with that is that for QEMU on POWER, having
ACCESS_PLATFORM turned off means that it can bypass the IOMMU for the
device (which makes sense considering that the name of the flag was
IOMMU_PLATFORM). And we need that for regular guests to avoid
performance degradation.
So while ACCESS_PLATFORM solves our problems for secure guests, we can't
turn it on by default because we can't affect legacy systems. Doing so
would penalize existing systems that can access all memory. They would
all have to unnecessarily go through address translations, and take a
performance hit.
The semantics of ACCESS_PLATFORM assume that the hypervisor/QEMU knows
in advance - right when the VM is instantiated - that it will not have
access to all guest memory. Unfortunately that assumption is subtly
broken on our secure-platform. The hypervisor/QEMU realizes that the
platform is going secure only *after the VM is instantiated*. It's the
kernel running in the VM that determines that it wants to switch the
platform to secure-mode.
Another way of looking at this issue which also explains our reluctance
is that the only difference between a secure guest and a regular guest
(at least regarding virtio) is that the former uses swiotlb while the
latter doens't. And from the device's point of view they're
indistinguishable. It can't tell one guest that is using swiotlb from
one that isn't. And that implies that secure guest vs regular guest
isn't a virtio interface issue, it's "guest internal affairs". So
there's no reason to reflect that in the feature flags.
That said, we still would like to arrive at a proper design for this
rather than add yet another hack if we can avoid it. So here's another
proposal: considering that the dma-direct code (in kernel/dma/direct.c)
automatically uses swiotlb when necessary (thanks to Christoph's recent
DMA work), would it be ok to replace virtio's own direct-memory code
that is used in the !ACCESS_PLATFORM case with the dma-direct code? That
way we'll get swiotlb even with !ACCESS_PLATFORM, and virtio will get a
code cleanup (replace open-coded stuff with calls to existing
infrastructure).
> But I also think I don't have the energy to argue about power secure
> guest anymore. So be it for power secure guest since the involved
> engineers disagree with me. Hey I've been wrong in the past ;).
Yeah, it's been a difficult discussion. Thanks for still engaging!
I honestly thought that this patch was a good solution (if the guest has
encrypted memory it means that the DMA API needs to be used), but I can
see where you are coming from. As I said, we'd like to arrive at a good
solution if possible.
> But the name "sev_active" makes me scared because at least AMD guys who
> were doing the sensible thing and setting ACCESS_PLATFORM
My understanding is, AMD guest-platform knows in advance that their
guest will run in secure mode and hence sets the flag at the time of VM
instantiation. Unfortunately we dont have that luxury on our platforms.
> (unless I'm
> wrong? I reemember distinctly that's so) will likely be affected too.
> We don't want that.
>
> So let's find a way to make sure it's just power secure guest for now
> pls.
Yes, my understanding is that they turn ACCESS_PLATFORM on. And because
of that, IIUC this patch wouldn't affect them because in their platform
vring_use_dma_api() returns true earlier in the
"if !virtio_has_iommu_quirk(vdev)" condition.
> I also think we should add a dma_api near features under virtio_device
> such that these hacks can move off data path.
Sorry, I don't understand this.
> By the way could you please respond about virtio-iommu and
> why there's no support for ACCESS_PLATFORM on power?
There is support for ACCESS_PLATFORM on POWER. We don't enable it
because it causes a performance hit.
> I have Cc'd you on these discussions.
I'm having a look at the spec and the patches, but to be honest I'm not
the best powerpc guy for this. I'll see if I can get others to have a
look.
> Thanks!
Thanks as well!
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH 15/19] KVM: PPC: Book3S HV: add get/set accessors for the source configuration
From: Cédric Le Goater @ 2019-02-04 16:07 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190204052148.GH1927@umbus.fritz.box>
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:
>> 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(+)
>>
>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>> index 1a8740629acf..faf024f39858 100644
>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>> @@ -683,9 +683,20 @@ struct kvm_ppc_cpu_char {
>> #define KVM_DEV_XIVE_SAVE_EQ_PAGES 4
>> #define KVM_DEV_XIVE_GRP_SOURCES 2 /* 64-bit source attributes */
>> #define KVM_DEV_XIVE_GRP_SYNC 3 /* 64-bit source attributes */
>> +#define KVM_DEV_XIVE_GRP_EAS 4 /* 64-bit eas attributes */
>>
>> /* Layout of 64-bit XIVE source attribute values */
>> #define KVM_XIVE_LEVEL_SENSITIVE (1ULL << 0)
>> #define KVM_XIVE_LEVEL_ASSERTED (1ULL << 1)
>>
>> +/* Layout of 64-bit eas attribute values */
>> +#define KVM_XIVE_EAS_PRIORITY_SHIFT 0
>> +#define KVM_XIVE_EAS_PRIORITY_MASK 0x7
>> +#define KVM_XIVE_EAS_SERVER_SHIFT 3
>> +#define KVM_XIVE_EAS_SERVER_MASK 0xfffffff8ULL
>> +#define KVM_XIVE_EAS_MASK_SHIFT 32
>> +#define KVM_XIVE_EAS_MASK_MASK 0x100000000ULL
>> +#define KVM_XIVE_EAS_EISN_SHIFT 33
>> +#define KVM_XIVE_EAS_EISN_MASK 0xfffffffe00000000ULL
>> +
>> #endif /* __LINUX_KVM_POWERPC_H */
>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>> index f2de1bcf3b35..0468b605baa7 100644
>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
>> @@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)
>> return 0;
>> }
>>
>> +static int kvmppc_xive_native_set_eas(struct kvmppc_xive *xive, long irq,
>> + u64 addr)
>
> 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.
>
>> +{
>> + struct kvmppc_xive_src_block *sb;
>> + struct kvmppc_xive_irq_state *state;
>> + u64 __user *ubufp = (u64 __user *) addr;
>> + u16 src;
>> + u64 kvm_eas;
>> + u32 server;
>> + u8 priority;
>> + u32 eisn;
>> +
>> + sb = kvmppc_xive_find_source(xive, irq, &src);
>> + if (!sb)
>> + return -ENOENT;
>> +
>> + state = &sb->irq_state[src];
>> +
>> + if (!state->valid)
>> + return -EINVAL;
>> +
>> + if (get_user(kvm_eas, ubufp))
>> + return -EFAULT;
>> +
>> + pr_devel("%s irq=0x%lx eas=%016llx\n", __func__, irq, kvm_eas);
>> +
>> + priority = (kvm_eas & KVM_XIVE_EAS_PRIORITY_MASK) >>
>> + KVM_XIVE_EAS_PRIORITY_SHIFT;
>> + server = (kvm_eas & KVM_XIVE_EAS_SERVER_MASK) >>
>> + KVM_XIVE_EAS_SERVER_SHIFT;
>> + eisn = (kvm_eas & KVM_XIVE_EAS_EISN_MASK) >> KVM_XIVE_EAS_EISN_SHIFT;
>> +
>> + if (priority != xive_prio_from_guest(priority)) {
>> + pr_err("invalid priority for queue %d for VCPU %d\n",
>> + priority, server);
>> + return -EINVAL;
>> + }
>> +
>> + return kvmppc_xive_native_set_source_config(xive, sb, state, server,
>> + priority, eisn);
>> +}
>> +
>> +static int kvmppc_xive_native_get_eas(struct kvmppc_xive *xive, long irq,
>> + u64 addr)
>> +{
>> + struct kvmppc_xive_src_block *sb;
>> + struct kvmppc_xive_irq_state *state;
>> + u64 __user *ubufp = (u64 __user *) addr;
>> + u16 src;
>> + u64 kvm_eas;
>> +
>> + sb = kvmppc_xive_find_source(xive, irq, &src);
>> + if (!sb)
>> + return -ENOENT;
>> +
>> + state = &sb->irq_state[src];
>> +
>> + if (!state->valid)
>> + return -EINVAL;
>> +
>> + arch_spin_lock(&sb->lock);
>> +
>> + if (state->act_priority == MASKED)
>> + kvm_eas = KVM_XIVE_EAS_MASK_MASK;
>> + else {
>> + kvm_eas = (state->act_priority << KVM_XIVE_EAS_PRIORITY_SHIFT) &
>> + KVM_XIVE_EAS_PRIORITY_MASK;
>> + kvm_eas |= (state->act_server << KVM_XIVE_EAS_SERVER_SHIFT) &
>> + KVM_XIVE_EAS_SERVER_MASK;
>> + kvm_eas |= ((u64) state->eisn << KVM_XIVE_EAS_EISN_SHIFT) &
>> + KVM_XIVE_EAS_EISN_MASK;
>> + }
>> + arch_spin_unlock(&sb->lock);
>> +
>> + pr_devel("%s irq=0x%lx eas=%016llx\n", __func__, irq, kvm_eas);
>> +
>> + if (put_user(kvm_eas, ubufp))
>> + return -EFAULT;
>> +
>> + return 0;
>> +}
>> +
>> static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
>> struct kvm_device_attr *attr)
>> {
>> @@ -544,6 +626,8 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
>> attr->addr);
>> case KVM_DEV_XIVE_GRP_SYNC:
>> return kvmppc_xive_native_sync(xive, attr->attr, attr->addr);
>> + case KVM_DEV_XIVE_GRP_EAS:
>> + return kvmppc_xive_native_set_eas(xive, attr->attr, attr->addr);
>> }
>> return -ENXIO;
>> }
>> @@ -564,6 +648,8 @@ static int kvmppc_xive_native_get_attr(struct kvm_device *dev,
>> return kvmppc_xive_native_get_vc_base(xive, attr->addr);
>> }
>> break;
>> + case KVM_DEV_XIVE_GRP_EAS:
>> + return kvmppc_xive_native_get_eas(xive, attr->attr, attr->addr);
>> }
>> return -ENXIO;
>> }
>> @@ -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;
>
^ permalink raw reply
* Re: [PATCH 12/19] KVM: PPC: Book3S HV: record guest queue page address
From: Cédric Le Goater @ 2019-02-04 15:37 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190204051548.GE1927@umbus.fritz.box>
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:
>> 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.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> arch/powerpc/include/asm/xive.h | 2 ++
>> arch/powerpc/kvm/book3s_xive_native.c | 4 ++++
>> 2 files changed, 6 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
>> index 7a7aa22d8258..e90c3c5d9533 100644
>> --- a/arch/powerpc/include/asm/xive.h
>> +++ b/arch/powerpc/include/asm/xive.h
>> @@ -74,6 +74,8 @@ struct xive_q {
>> u32 esc_irq;
>> atomic_t count;
>> atomic_t pending_count;
>> + u64 guest_qpage;
>> + u32 guest_qsize;
>> };
>>
>> /* Global enable flags for the XIVE support */
>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>> index 35d806740c3a..4ca75aade069 100644
>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
>> @@ -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) {
>
^ permalink raw reply
* [PATCH RFC v3 06/21] PCI: Pause the devices with movable BARs during rescan
From: Sergey Miroshnichenko @ 2019-02-04 15:35 UTC (permalink / raw)
To: linux-pci
Cc: Sam Bobroff, Sergey Miroshnichenko, linux, Lukas Wunner,
Bjorn Helgaas, Oliver, Rajat Jain, linuxppc-dev
In-Reply-To: <20190204153601.7576-1-s.miroshnichenko@yadro.com>
Drivers indicate their support of movable BARs by implementing the
new rescan_prepare() and rescan_done() hooks in the struct pci_driver.
All device's activity must be stopped during a rescan, and iounmap()
+ioremap() must be applied to every used BAR.
Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
drivers/pci/probe.c | 51 +++++++++++++++++++++++++++++++++++++++++++--
include/linux/pci.h | 2 ++
2 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index bbc12934f041..e18d07996cf3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3172,6 +3172,38 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
return max;
}
+static void pci_bus_rescan_prepare(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ struct pci_bus *child = dev->subordinate;
+
+ if (child) {
+ pci_bus_rescan_prepare(child);
+ } else if (dev->driver &&
+ dev->driver->rescan_prepare) {
+ dev->driver->rescan_prepare(dev);
+ }
+ }
+}
+
+static void pci_bus_rescan_done(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ struct pci_bus *child = dev->subordinate;
+
+ if (child) {
+ pci_bus_rescan_done(child);
+ } else if (dev->driver &&
+ dev->driver->rescan_done) {
+ dev->driver->rescan_done(dev);
+ }
+ }
+}
+
/**
* pci_rescan_bus - Scan a PCI bus for devices
* @bus: PCI bus to scan
@@ -3185,8 +3217,23 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)
{
unsigned int max;
- max = pci_scan_child_bus(bus);
- pci_assign_unassigned_bus_resources(bus);
+ if (pci_movable_bars_enabled()) {
+ struct pci_bus *root = bus;
+
+ while (!pci_is_root_bus(root))
+ root = root->parent;
+
+ pci_bus_rescan_prepare(root);
+
+ max = pci_scan_child_bus(root);
+ pci_assign_unassigned_root_bus_resources(root);
+
+ pci_bus_rescan_done(root);
+ } else {
+ max = pci_scan_child_bus(bus);
+ pci_assign_unassigned_bus_resources(bus);
+ }
+
pci_bus_add_devices(bus);
return max;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ba0b1d0ea2d2..5cd534b6631b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -777,6 +777,8 @@ struct pci_driver {
int (*resume)(struct pci_dev *dev); /* Device woken up */
void (*shutdown)(struct pci_dev *dev);
int (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */
+ void (*rescan_prepare)(struct pci_dev *dev);
+ void (*rescan_done)(struct pci_dev *dev);
const struct pci_error_handlers *err_handler;
const struct attribute_group **groups;
struct device_driver driver;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 14/19] KVM: PPC: Book3S HV: add a control to make the XIVE EQ pages dirty
From: Cédric Le Goater @ 2019-02-04 15:46 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190204051833.GG1927@umbus.fritz.box>
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:
>> 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.
>
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> arch/powerpc/include/uapi/asm/kvm.h | 1 +
>> arch/powerpc/kvm/book3s_xive_native.c | 40 +++++++++++++++++++++++++++
>> 2 files changed, 41 insertions(+)
>>
>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>> index f3b859223b80..1a8740629acf 100644
>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>> @@ -680,6 +680,7 @@ struct kvm_ppc_cpu_char {
>> #define KVM_DEV_XIVE_GET_ESB_FD 1
>> #define KVM_DEV_XIVE_GET_TIMA_FD 2
>> #define KVM_DEV_XIVE_VC_BASE 3
>> +#define KVM_DEV_XIVE_SAVE_EQ_PAGES 4
>> #define KVM_DEV_XIVE_GRP_SOURCES 2 /* 64-bit source attributes */
>> #define KVM_DEV_XIVE_GRP_SYNC 3 /* 64-bit source attributes */
>>
>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>> index a8052867afc1..f2de1bcf3b35 100644
>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
>> @@ -373,6 +373,43 @@ static int kvmppc_xive_native_get_tima_fd(struct kvmppc_xive *xive, u64 addr)
>> return put_user(ret, ubufp);
>> }
>>
>> +static int kvmppc_xive_native_vcpu_save_eq_pages(struct kvm_vcpu *vcpu)
>> +{
>> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>> + unsigned int prio;
>> +
>> + if (!xc)
>> + return -ENOENT;
>> +
>> + for (prio = 0; prio < KVMPPC_XIVE_Q_COUNT; prio++) {
>> + struct xive_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));
>> + }
>> + return 0;
>> +}
>> +
>> +static int kvmppc_xive_native_save_eq_pages(struct kvmppc_xive *xive)
>> +{
>> + struct kvm *kvm = xive->kvm;
>> + struct kvm_vcpu *vcpu;
>> + unsigned int i;
>> +
>> + 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);
>> +
>> + return 0;
>> +}
>> +
>> static int xive_native_validate_queue_size(u32 qsize)
>> {
>> switch (qsize) {
>> @@ -498,6 +535,8 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
>> switch (attr->attr) {
>> case KVM_DEV_XIVE_VC_BASE:
>> return kvmppc_xive_native_set_vc_base(xive, attr->addr);
>> + case KVM_DEV_XIVE_SAVE_EQ_PAGES:
>> + return kvmppc_xive_native_save_eq_pages(xive);
>> }
>> break;
>> case KVM_DEV_XIVE_GRP_SOURCES:
>> @@ -538,6 +577,7 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,
>> case KVM_DEV_XIVE_GET_ESB_FD:
>> case KVM_DEV_XIVE_GET_TIMA_FD:
>> case KVM_DEV_XIVE_VC_BASE:
>> + case KVM_DEV_XIVE_SAVE_EQ_PAGES:
>> return 0;
>> }
>> break;
>
^ permalink raw reply
* [PATCH RFC v3 02/21] PCI: Fix race condition in pci_enable/disable_device()
From: Sergey Miroshnichenko @ 2019-02-04 15:35 UTC (permalink / raw)
To: linux-pci
Cc: Sam Bobroff, Sergey Miroshnichenko, linux, Bjorn Helgaas, Oliver,
Rajat Jain, linuxppc-dev
In-Reply-To: <20190204153601.7576-1-s.miroshnichenko@yadro.com>
CPU0 CPU1
pci_enable_device_mem() pci_enable_device_mem()
pci_enable_bridge() pci_enable_bridge()
pci_is_enabled()
return false;
atomic_inc_return(enable_cnt)
Start actual enabling the bridge
... pci_is_enabled()
... return true;
... Start memory requests <-- FAIL
...
Set the PCI_COMMAND_MEMORY bit <-- Must wait for this
This patch protects the pci_enable/disable_device() and pci_enable_bridge()
with mutexes.
Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
drivers/pci/pci.c | 26 ++++++++++++++++++++++----
drivers/pci/probe.c | 1 +
include/linux/pci.h | 1 +
3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e1fc93c9eea1..3a83e05f8363 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1571,6 +1571,8 @@ static void pci_enable_bridge(struct pci_dev *dev)
struct pci_dev *bridge;
int retval;
+ mutex_lock(&dev->enable_mutex);
+
bridge = pci_upstream_bridge(dev);
if (bridge)
pci_enable_bridge(bridge);
@@ -1578,6 +1580,7 @@ static void pci_enable_bridge(struct pci_dev *dev)
if (pci_is_enabled(dev)) {
if (!dev->is_busmaster)
pci_set_master(dev);
+ mutex_unlock(&dev->enable_mutex);
return;
}
@@ -1586,11 +1589,14 @@ static void pci_enable_bridge(struct pci_dev *dev)
pci_err(dev, "Error enabling bridge (%d), continuing\n",
retval);
pci_set_master(dev);
+ mutex_unlock(&dev->enable_mutex);
}
static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
{
struct pci_dev *bridge;
+ /* Enable-locking of bridges is performed within the pci_enable_bridge() */
+ bool need_lock = !dev->subordinate;
int err;
int i, bars = 0;
@@ -1606,8 +1612,13 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
}
- if (atomic_inc_return(&dev->enable_cnt) > 1)
+ if (need_lock)
+ mutex_lock(&dev->enable_mutex);
+ if (pci_is_enabled(dev)) {
+ if (need_lock)
+ mutex_unlock(&dev->enable_mutex);
return 0; /* already enabled */
+ }
bridge = pci_upstream_bridge(dev);
if (bridge)
@@ -1622,8 +1633,10 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
bars |= (1 << i);
err = do_pci_enable_device(dev, bars);
- if (err < 0)
- atomic_dec(&dev->enable_cnt);
+ if (err >= 0)
+ atomic_inc(&dev->enable_cnt);
+ if (need_lock)
+ mutex_unlock(&dev->enable_mutex);
return err;
}
@@ -1866,15 +1879,20 @@ void pci_disable_device(struct pci_dev *dev)
if (dr)
dr->enabled = 0;
+ mutex_lock(&dev->enable_mutex);
dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= 0,
"disabling already-disabled device");
- if (atomic_dec_return(&dev->enable_cnt) != 0)
+ if (atomic_dec_return(&dev->enable_cnt) != 0) {
+ mutex_unlock(&dev->enable_mutex);
return;
+ }
do_pci_disable_device(dev);
dev->is_busmaster = 0;
+
+ mutex_unlock(&dev->enable_mutex);
}
EXPORT_SYMBOL(pci_disable_device);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 257b9f6f2ebb..bbc12934f041 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2191,6 +2191,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
INIT_LIST_HEAD(&dev->bus_list);
dev->dev.type = &pci_dev_type;
dev->bus = pci_bus_get(bus);
+ mutex_init(&dev->enable_mutex);
return dev;
}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 65f1d8c2f082..28fecfdd598d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -416,6 +416,7 @@ struct pci_dev {
unsigned int no_vf_scan:1; /* Don't scan for VFs after IOV enablement */
pci_dev_flags_t dev_flags;
atomic_t enable_cnt; /* pci_enable_device has been called */
+ struct mutex enable_mutex;
u32 saved_config_space[16]; /* Config space saved at suspend time */
struct hlist_head saved_cap_space;
--
2.20.1
^ permalink raw reply related
* [PATCH RFC v3 05/21] PCI: hotplug: Add a flag for the movable BARs feature
From: Sergey Miroshnichenko @ 2019-02-04 15:35 UTC (permalink / raw)
To: linux-pci
Cc: Sam Bobroff, Sergey Miroshnichenko, linux, Lukas Wunner,
Bjorn Helgaas, Oliver, Rajat Jain, linuxppc-dev
In-Reply-To: <20190204153601.7576-1-s.miroshnichenko@yadro.com>
If a new PCIe device has been hot-plugged between the two active ones
without big enough gap between their BARs, these BARs should be moved
if their drivers support this feature. The drivers should be notified
and paused during the procedure:
1) dev 8 (new)
|
v
.. | dev 3 | dev 3 | dev 5 | dev 7 |
.. | BAR 0 | BAR 1 | BAR 0 | BAR 0 |
2) dev 8
|
v
.. | dev 3 | dev 3 | --> --> | dev 5 | dev 7 |
.. | BAR 0 | BAR 1 | --> --> | BAR 0 | BAR 0 |
3)
.. | dev 3 | dev 3 | dev 8 | dev 8 | dev 5 | dev 7 |
.. | BAR 0 | BAR 1 | BAR 0 | BAR 1 | BAR 0 | BAR 0 |
Thus, prior reservation of memory regions by BIOS/bootloader/firmware
is not required anymore for the PCIe hotplug.
The PCI_MOVABLE_BARS flag is set by the platform is this feature is
supported and tested, but can be overridden by the following command
line option:
pcie_movable_bars={ off | force }
Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
.../admin-guide/kernel-parameters.txt | 7 ++++++
drivers/pci/pci.c | 24 +++++++++++++++++++
include/linux/pci.h | 2 ++
3 files changed, 33 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index b799bcf67d7b..2165c4b5aea6 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3387,6 +3387,13 @@
nomsi Do not use MSI for native PCIe PME signaling (this makes
all PCIe root ports use INTx for all services).
+ pcie_movable_bars=[PCIE]
+ Override the movable BARs support detection:
+ off
+ Disable even if supported by the platform
+ force
+ Enable even if not explicitly declared as supported
+
pcmv= [HW,PCMCIA] BadgePAD 4
pd_ignore_unused
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 55cf18389c15..096413f9ee67 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -139,6 +139,30 @@ static int __init pcie_port_pm_setup(char *str)
}
__setup("pcie_port_pm=", pcie_port_pm_setup);
+static bool pcie_movable_bars_off;
+static bool pcie_movable_bars_force;
+static int __init pcie_movable_bars_setup(char *str)
+{
+ if (!strcmp(str, "off"))
+ pcie_movable_bars_off = true;
+ else if (!strcmp(str, "force"))
+ pcie_movable_bars_force = true;
+ return 1;
+}
+__setup("pcie_movable_bars=", pcie_movable_bars_setup);
+
+bool pci_movable_bars_enabled(void)
+{
+ if (pcie_movable_bars_off)
+ return false;
+
+ if (pcie_movable_bars_force)
+ return true;
+
+ return pci_has_flag(PCI_MOVABLE_BARS);
+}
+EXPORT_SYMBOL(pci_movable_bars_enabled);
+
/* Time to wait after a reset for device to become responsive */
#define PCIE_RESET_READY_POLL_MS 60000
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 28fecfdd598d..ba0b1d0ea2d2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -863,6 +863,7 @@ enum {
PCI_ENABLE_PROC_DOMAINS = 0x00000010, /* Enable domains in /proc */
PCI_COMPAT_DOMAIN_0 = 0x00000020, /* ... except domain 0 */
PCI_SCAN_ALL_PCIE_DEVS = 0x00000040, /* Scan all, not just dev 0 */
+ PCI_MOVABLE_BARS = 0x00000080, /* Runtime BAR reassign after hotplug */
};
/* These external functions are only available when PCI support is enabled */
@@ -1342,6 +1343,7 @@ unsigned char pci_bus_max_busnr(struct pci_bus *bus);
void pci_setup_bridge(struct pci_bus *bus);
resource_size_t pcibios_window_alignment(struct pci_bus *bus,
unsigned long type);
+bool pci_movable_bars_enabled(void);
#define PCI_VGA_STATE_CHANGE_BRIDGE (1 << 0)
#define PCI_VGA_STATE_CHANGE_DECODES (1 << 1)
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox