* Re: [PATCH v3 06/17] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
From: David Gibson @ 2019-03-19 4:54 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: linuxppc-dev, Paul Mackerras, kvm, kvm-ppc
In-Reply-To: <8b3caeef-8750-b8db-d516-c722bc08c535@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 20680 bytes --]
On Mon, Mar 18, 2019 at 03:12:10PM +0100, Cédric Le Goater wrote:
> On 3/18/19 4:23 AM, David Gibson wrote:
> > On Fri, Mar 15, 2019 at 01:05:58PM +0100, Cédric Le Goater wrote:
> >> These controls will be used by the H_INT_SET_QUEUE_CONFIG and
> >> H_INT_GET_QUEUE_CONFIG hcalls from QEMU to configure the underlying
> >> Event Queue in the XIVE IC. They will also be used to restore the
> >> configuration of the XIVE EQs and to capture the internal run-time
> >> state of the EQs. Both 'get' and 'set' rely on an OPAL call to access
> >> the EQ toggle bit and EQ index which are updated by the XIVE IC when
> >> event notifications are enqueued in the EQ.
> >>
> >> The value of the guest physical address of the event queue is saved in
> >> the XIVE internal xive_q structure for later use. That is when
> >> migration needs to mark the EQ pages dirty to capture a consistent
> >> memory state of the VM.
> >>
> >> To be noted that H_INT_SET_QUEUE_CONFIG does not require the extra
> >> OPAL call setting the EQ toggle bit and EQ index to configure the EQ,
> >> but restoring the EQ state will.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>
> >> Changes since v2 :
> >>
> >> - fixed comments on the KVM device attribute definitions
> >> - fixed check on supported EQ size to restrict to 64K pages
> >> - checked kvm_eq.flags that need to be zero
> >> - removed the OPAL call when EQ qtoggle bit and index are zero.
> >>
> >> arch/powerpc/include/asm/xive.h | 2 +
> >> arch/powerpc/include/uapi/asm/kvm.h | 21 ++
> >> arch/powerpc/kvm/book3s_xive.h | 2 +
> >> arch/powerpc/kvm/book3s_xive.c | 15 +-
> >> arch/powerpc/kvm/book3s_xive_native.c | 232 +++++++++++++++++++++
> >> Documentation/virtual/kvm/devices/xive.txt | 31 +++
> >> 6 files changed, 297 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> >> index b579a943407b..46891f321606 100644
> >> --- a/arch/powerpc/include/asm/xive.h
> >> +++ b/arch/powerpc/include/asm/xive.h
> >> @@ -73,6 +73,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/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> >> index 12bb01baf0ae..1cd728c87d7c 100644
> >> --- a/arch/powerpc/include/uapi/asm/kvm.h
> >> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> >> @@ -679,6 +679,7 @@ struct kvm_ppc_cpu_char {
> >> #define KVM_DEV_XIVE_GRP_CTRL 1
> >> #define KVM_DEV_XIVE_GRP_SOURCE 2 /* 64-bit source identifier */
> >> #define KVM_DEV_XIVE_GRP_SOURCE_CONFIG 3 /* 64-bit source identifier */
> >> +#define KVM_DEV_XIVE_GRP_EQ_CONFIG 4 /* 64-bit EQ identifier */
> >>
> >> /* Layout of 64-bit XIVE source attribute values */
> >> #define KVM_XIVE_LEVEL_SENSITIVE (1ULL << 0)
> >> @@ -694,4 +695,24 @@ struct kvm_ppc_cpu_char {
> >> #define KVM_XIVE_SOURCE_EISN_SHIFT 33
> >> #define KVM_XIVE_SOURCE_EISN_MASK 0xfffffffe00000000ULL
> >>
> >> +/* Layout of 64-bit EQ identifier */
> >> +#define KVM_XIVE_EQ_PRIORITY_SHIFT 0
> >> +#define KVM_XIVE_EQ_PRIORITY_MASK 0x7
> >> +#define KVM_XIVE_EQ_SERVER_SHIFT 3
> >> +#define KVM_XIVE_EQ_SERVER_MASK 0xfffffff8ULL
> >> +
> >> +/* Layout of EQ configuration values (64 bytes) */
> >> +struct kvm_ppc_xive_eq {
> >> + __u32 flags;
> >> + __u32 qsize;
> >> + __u64 qpage;
> >> + __u32 qtoggle;
> >> + __u32 qindex;
> >> + __u8 pad[40];
> >> +};
> >> +
> >> +#define KVM_XIVE_EQ_FLAG_ENABLED 0x00000001
> >> +#define KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY 0x00000002
> >> +#define KVM_XIVE_EQ_FLAG_ESCALATE 0x00000004
> >> +
> >> #endif /* __LINUX_KVM_POWERPC_H */
> >> diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
> >> index ae26fe653d98..622f594d93e1 100644
> >> --- a/arch/powerpc/kvm/book3s_xive.h
> >> +++ b/arch/powerpc/kvm/book3s_xive.h
> >> @@ -272,6 +272,8 @@ struct kvmppc_xive_src_block *kvmppc_xive_create_src_block(
> >> struct kvmppc_xive *xive, int irq);
> >> void kvmppc_xive_free_sources(struct kvmppc_xive_src_block *sb);
> >> int kvmppc_xive_select_target(struct kvm *kvm, u32 *server, u8 prio);
> >> +int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
> >> + bool single_escalation);
> >>
> >> #endif /* CONFIG_KVM_XICS */
> >> #endif /* _KVM_PPC_BOOK3S_XICS_H */
> >> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> >> index e09f3addffe5..c1b7aa7dbc28 100644
> >> --- a/arch/powerpc/kvm/book3s_xive.c
> >> +++ b/arch/powerpc/kvm/book3s_xive.c
> >> @@ -166,7 +166,8 @@ static irqreturn_t xive_esc_irq(int irq, void *data)
> >> return IRQ_HANDLED;
> >> }
> >>
> >> -static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
> >> +int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
> >> + bool single_escalation)
> >> {
> >> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> >> struct xive_q *q = &xc->queues[prio];
> >> @@ -185,7 +186,7 @@ static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
> >> return -EIO;
> >> }
> >>
> >> - if (xc->xive->single_escalation)
> >> + if (single_escalation)
> >> name = kasprintf(GFP_KERNEL, "kvm-%d-%d",
> >> vcpu->kvm->arch.lpid, xc->server_num);
> >> else
> >> @@ -217,7 +218,7 @@ static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
> >> * interrupt, thus leaving it effectively masked after
> >> * it fires once.
> >> */
> >> - if (xc->xive->single_escalation) {
> >> + if (single_escalation) {
> >> struct irq_data *d = irq_get_irq_data(xc->esc_virq[prio]);
> >> struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
> >>
> >> @@ -291,7 +292,8 @@ static int xive_check_provisioning(struct kvm *kvm, u8 prio)
> >> continue;
> >> rc = xive_provision_queue(vcpu, prio);
> >> if (rc == 0 && !xive->single_escalation)
> >> - xive_attach_escalation(vcpu, prio);
> >> + kvmppc_xive_attach_escalation(vcpu, prio,
> >> + xive->single_escalation);
> >> if (rc)
> >> return rc;
> >> }
> >> @@ -1214,7 +1216,8 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> >> if (xive->qmap & (1 << i)) {
> >> r = xive_provision_queue(vcpu, i);
> >> if (r == 0 && !xive->single_escalation)
> >> - xive_attach_escalation(vcpu, i);
> >> + kvmppc_xive_attach_escalation(
> >> + vcpu, i, xive->single_escalation);
> >> if (r)
> >> goto bail;
> >> } else {
> >> @@ -1229,7 +1232,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> >> }
> >>
> >> /* If not done above, attach priority 0 escalation */
> >> - r = xive_attach_escalation(vcpu, 0);
> >> + r = kvmppc_xive_attach_escalation(vcpu, 0, xive->single_escalation);
> >> if (r)
> >> goto bail;
> >>
> >> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> >> index b841d339f674..42e824658a30 100644
> >> --- a/arch/powerpc/kvm/book3s_xive_native.c
> >> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> >> @@ -340,6 +340,226 @@ static int kvmppc_xive_native_set_source_config(struct kvmppc_xive *xive,
> >> priority, masked, eisn);
> >> }
> >>
> >> +static int xive_native_validate_queue_size(u32 qsize)
> >> +{
> >> + /*
> >> + * We only support 64K pages for the moment. This is also
> >> + * advertised in the DT property "ibm,xive-eq-sizes"
> >
> > IIUC, that won't work properly if you had a guest using 4kiB pages.
>
> > That's fine, but do we have somewhere that checks for that case and
> > throws a suitable error?
>
> Not in the device.
>
> So, we should check the current page_size of the guest ? Is there a way
> to do that simply from KVM ?
Not really. But I think I know where to make the necessary test, see
comment below..
> >> + */
> >> + switch (qsize) {
> >> + case 0: /* EQ reset */
> >> + case 16:
> >> + return 0;
> >> + case 12:
> >> + case 21:
> >> + case 24:
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> +}
> >> +
> >> +static int kvmppc_xive_native_set_queue_config(struct kvmppc_xive *xive,
> >> + long eq_idx, u64 addr)
> >> +{
> >> + struct kvm *kvm = xive->kvm;
> >> + struct kvm_vcpu *vcpu;
> >> + struct kvmppc_xive_vcpu *xc;
> >> + void __user *ubufp = (u64 __user *) addr;
> >
> > Nit: that should be (void __user *) on the right, shouldn't it?
>
> yes.
>
> >
> >> + u32 server;
> >> + u8 priority;
> >> + struct kvm_ppc_xive_eq kvm_eq;
> >> + int rc;
> >> + __be32 *qaddr = 0;
> >> + struct page *page;
> >> + struct xive_q *q;
> >> +
> >> + /*
> >> + * Demangle priority/server tuple from the EQ identifier
> >> + */
> >> + priority = (eq_idx & KVM_XIVE_EQ_PRIORITY_MASK) >>
> >> + KVM_XIVE_EQ_PRIORITY_SHIFT;
> >> + server = (eq_idx & KVM_XIVE_EQ_SERVER_MASK) >>
> >> + KVM_XIVE_EQ_SERVER_SHIFT;
> >> +
> >> + if (copy_from_user(&kvm_eq, ubufp, sizeof(kvm_eq)))
> >> + return -EFAULT;
> >> +
> >> + vcpu = kvmppc_xive_find_server(kvm, server);
> >> + if (!vcpu) {
> >> + pr_err("Can't find server %d\n", server);
> >> + return -ENOENT;
> >> + }
> >> + xc = vcpu->arch.xive_vcpu;
> >> +
> >> + if (priority != xive_prio_from_guest(priority)) {
> >> + pr_err("Trying to restore invalid queue %d for VCPU %d\n",
> >> + priority, server);
> >> + return -EINVAL;
> >> + }
> >> + q = &xc->queues[priority];
> >> +
> >> + pr_devel("%s VCPU %d priority %d fl:%x sz:%d addr:%llx g:%d idx:%d\n",
> >> + __func__, server, priority, kvm_eq.flags,
> >> + kvm_eq.qsize, kvm_eq.qpage, kvm_eq.qtoggle, kvm_eq.qindex);
> >> +
> >> + /*
> >> + * We can not tune the EQ configuration from user space. All
> >> + * is done in OPAL.
> >> + */
> >> + if (kvm_eq.flags != 0) {
> >> + pr_err("invalid flags %d\n", kvm_eq.flags);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + rc = xive_native_validate_queue_size(kvm_eq.qsize);
> >> + if (rc) {
> >> + pr_err("invalid queue size %d\n", kvm_eq.qsize);
> >> + return rc;
> >> + }
> >> +
> >> + /* reset queue and disable queueing */
> >> + if (!kvm_eq.qsize) {
> >> + q->guest_qpage = 0;
> >> + q->guest_qsize = 0;
> >> +
> >> + rc = xive_native_configure_queue(xc->vp_id, q, priority,
> >> + NULL, 0, true);
> >> + if (rc) {
> >> + pr_err("Failed to reset queue %d for VCPU %d: %d\n",
> >> + priority, xc->server_num, rc);
> >> + return rc;
> >> + }
> >> +
> >> + if (q->qpage) {
> >> + put_page(virt_to_page(q->qpage));
> >> + q->qpage = NULL;
> >> + }
> >> +
> >> + return 0;
> >> + }
> >> +
> >> +
> >> + page = gfn_to_page(kvm, gpa_to_gfn(kvm_eq.qpage));
> >> + if (is_error_page(page)) {
> >> + pr_warn("Couldn't get guest page for %llx!\n", kvm_eq.qpage);
> >> + return -EINVAL;
> >> + }
> >
> > Yeah.. for the case of a 4kiB page host (these days weird, but not
> > actually prohibited, AFAIK) you need to check that the qsize selected
> > actually fits within the page.
>
> Ah yes. sure.
I think the pagesize test belongs here. Rather than thinking about
the pagesize of the guest overall, you can check that this specific
page (possibly compound) is large enough to take the requested queue
size.
That should be enough to protect the host - it ensures that userspace
owns a suitable contiguous chunk of memory for the XIVE to write the
queue into.
It's possible there are weirder edge cases with a large page that's
not fully mapped into the guest - if necessary we can add tests for
that on the qemu side.
Oh.. it occurs to me that we might need to pin the queue page to make
sure it doesn't get swapped out or page-migrated while the XIVE holds
a pointer to it
> >> + qaddr = page_to_virt(page) + (kvm_eq.qpage & ~PAGE_MASK);
> >> +
> >> + /* Backup queue page guest address for migration */
> >
> > Hm.. KVM itself shouldn't generally need to know about migration.
> > IIUC these values won't change from what qemu set them to be, so it
> > should be able to store and migrate them without have to get them back
> > from the kernel.
>
> Euh. You are completely right. I don't know why I kept those around.
> >> + q->guest_qpage = kvm_eq.qpage;
> >> + q->guest_qsize = kvm_eq.qsize;
> >> +
> >> + rc = xive_native_configure_queue(xc->vp_id, q, priority,
> >> + (__be32 *) qaddr, kvm_eq.qsize, true);
> >> + if (rc) {
> >> + pr_err("Failed to configure queue %d for VCPU %d: %d\n",
> >> + priority, xc->server_num, rc);
> >> + put_page(page);
> >> + return rc;
> >> + }
> >> +
> >> + /*
> >> + * Only restore the queue state when needed. When doing the
> >> + * H_INT_SET_SOURCE_CONFIG hcall, it should not.
> >> + */
> >> + if (kvm_eq.qtoggle != 0 || kvm_eq.qindex != 0) {
> >> + rc = xive_native_set_queue_state(xc->vp_id, priority,
> >> + kvm_eq.qtoggle,
> >> + kvm_eq.qindex);
> >> + if (rc)
> >> + goto error;
> >> + }
> >> +
> >> + rc = kvmppc_xive_attach_escalation(vcpu, priority,
> >> + xive->single_escalation);
> >> +error:
> >> + if (rc)
> >> + kvmppc_xive_native_cleanup_queue(vcpu, priority);
> >> + return rc;
> >> +}
> >> +
> >> +static int kvmppc_xive_native_get_queue_config(struct kvmppc_xive *xive,
> >> + long eq_idx, u64 addr)
> >> +{
> >> + struct kvm *kvm = xive->kvm;
> >> + struct kvm_vcpu *vcpu;
> >> + struct kvmppc_xive_vcpu *xc;
> >> + struct xive_q *q;
> >> + void __user *ubufp = (u64 __user *) addr;
> >> + u32 server;
> >> + u8 priority;
> >> + struct kvm_ppc_xive_eq kvm_eq;
> >> + u64 qpage;
> >> + u64 qsize;
> >> + u64 qeoi_page;
> >> + u32 escalate_irq;
> >> + u64 qflags;
> >> + int rc;
> >> +
> >> + /*
> >> + * Demangle priority/server tuple from the EQ identifier
> >> + */
> >> + priority = (eq_idx & KVM_XIVE_EQ_PRIORITY_MASK) >>
> >> + KVM_XIVE_EQ_PRIORITY_SHIFT;
> >> + server = (eq_idx & KVM_XIVE_EQ_SERVER_MASK) >>
> >> + KVM_XIVE_EQ_SERVER_SHIFT;
> >> +
> >> + vcpu = kvmppc_xive_find_server(kvm, server);
> >> + if (!vcpu) {
> >> + pr_err("Can't find server %d\n", server);
> >> + return -ENOENT;
> >> + }
> >> + xc = vcpu->arch.xive_vcpu;
> >> +
> >> + if (priority != xive_prio_from_guest(priority)) {
> >> + pr_err("invalid priority for queue %d for VCPU %d\n",
> >> + priority, server);
> >> + return -EINVAL;
> >> + }
> >> + q = &xc->queues[priority];
> >> +
> >> + memset(&kvm_eq, 0, sizeof(kvm_eq));
> >> +
> >> + if (!q->qpage)
> >> + return 0;
> >> +
> >> + rc = xive_native_get_queue_info(xc->vp_id, priority, &qpage, &qsize,
> >> + &qeoi_page, &escalate_irq, &qflags);
> >> + if (rc)
> >> + return rc;
> >> +
> >> + /*
> >> + * Return some information on the EQ configuration in
> >> + * OPAL. This is purely informative for now as we can't really
> >> + * tune the EQ configuration from user space.
> >> + */
> >> + kvm_eq.flags = 0;
> >> + if (qflags & OPAL_XIVE_EQ_ENABLED)
> >> + kvm_eq.flags |= KVM_XIVE_EQ_FLAG_ENABLED;
> >> + if (qflags & OPAL_XIVE_EQ_ALWAYS_NOTIFY)
> >> + kvm_eq.flags |= KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY;
> >> + if (qflags & OPAL_XIVE_EQ_ESCALATE)
> >> + kvm_eq.flags |= KVM_XIVE_EQ_FLAG_ESCALATE;
> >
> > If there's not really anything it can do about it, does it make sense
> > to even expose this info to userspace?
>
> Hmm, good question.
>
> - KVM_XIVE_EQ_FLAG_ENABLED
> may be uselessly obvious.
What's it controlled by?
> - KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY
> means we do not use the END ESBs to coalesce the events at the END
> level. This flag is reflected by the XIVE_EQ_ALWAYS_NOTIFY option
> in the sPAPR specs. We don't support the END ESBs but we might one
> day.
Since the guest isn't currently permitted to set this, it should never
be set here either, no?
> - KVM_XIVE_EQ_FLAG_ESCALATE
> means the EQ is an escalation. QEMU doesn't really care for now
> but it's an important information I think.
Likewise this one, yes?
>
> I tried not to add too many of the END flags, only the relevant ones which
> could have an impact in the future modeling.
>
> I think KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY is important. I was setting it from
> QEMU in the hcall but as OPAL does the same blindly I removed it in
> v3.
So, I might have misinterpreted this a bit the first time around. Am
I correct in thinking that these bits all correspond to defined
options in the PAPR hcall - but that for now we don't allow guests to
set them (because we haven't implemented support so far).
>
> >> + kvm_eq.qsize = q->guest_qsize;
> >> + kvm_eq.qpage = q->guest_qpage;
> >
> >> + rc = xive_native_get_queue_state(xc->vp_id, priority, &kvm_eq.qtoggle,
> >> + &kvm_eq.qindex);
> >> + if (rc)
> >> + return rc;
> >> +
> >> + pr_devel("%s VCPU %d priority %d fl:%x sz:%d addr:%llx g:%d idx:%d\n",
> >> + __func__, server, priority, kvm_eq.flags,
> >> + kvm_eq.qsize, kvm_eq.qpage, kvm_eq.qtoggle, kvm_eq.qindex);
> >> +
> >> + if (copy_to_user(ubufp, &kvm_eq, sizeof(kvm_eq)))
> >> + return -EFAULT;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
> >> struct kvm_device_attr *attr)
> >> {
> >> @@ -354,6 +574,9 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
> >> case KVM_DEV_XIVE_GRP_SOURCE_CONFIG:
> >> return kvmppc_xive_native_set_source_config(xive, attr->attr,
> >> attr->addr);
> >> + case KVM_DEV_XIVE_GRP_EQ_CONFIG:
> >> + return kvmppc_xive_native_set_queue_config(xive, attr->attr,
> >> + attr->addr);
> >> }
> >> return -ENXIO;
> >> }
> >> @@ -361,6 +584,13 @@ 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_EQ_CONFIG:
> >> + return kvmppc_xive_native_get_queue_config(xive, attr->attr,
> >> + attr->addr);
> >> + }
> >> return -ENXIO;
> >> }
> >>
> >> @@ -376,6 +606,8 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,
> >> attr->attr < KVMPPC_XIVE_NR_IRQS)
> >> return 0;
> >> break;
> >> + case KVM_DEV_XIVE_GRP_EQ_CONFIG:
> >> + return 0;
> >> }
> >> return -ENXIO;
> >> }
> >> diff --git a/Documentation/virtual/kvm/devices/xive.txt b/Documentation/virtual/kvm/devices/xive.txt
> >> index 33c64b2cdbe8..a4de64f6e79c 100644
> >> --- a/Documentation/virtual/kvm/devices/xive.txt
> >> +++ b/Documentation/virtual/kvm/devices/xive.txt
> >> @@ -53,3 +53,34 @@ the legacy interrupt mode, referred as XICS (POWER7/8).
> >> -ENXIO: CPU event queues not configured or configuration of the
> >> underlying HW interrupt failed
> >> -EBUSY: No CPU available to serve interrupt
> >> +
> >> + 4. KVM_DEV_XIVE_GRP_EQ_CONFIG (read-write)
> >> + Configures an event queue of a CPU
> >> + Attributes:
> >> + EQ descriptor identifier (64-bit)
> >> + The EQ descriptor identifier is a tuple (server, priority) :
> >> + bits: | 63 .... 32 | 31 .. 3 | 2 .. 0
> >> + values: | unused | server | priority
> >> + The kvm_device_attr.addr points to :
> >> + struct kvm_ppc_xive_eq {
> >> + __u32 flags;
> >> + __u32 qsize;
> >> + __u64 qpage;
> >> + __u32 qtoggle;
> >> + __u32 qindex;
> >> + __u8 pad[40];
> >> + };
> >> + - flags: queue flags
> >> + - qsize: queue size (power of 2)
> >> + - qpage: real address of queue
> >> + - qtoggle: current queue toggle bit
> >> + - qindex: current queue index
> >> + - pad: reserved for future use
> >> + Errors:
> >> + -ENOENT: Invalid CPU number
> >> + -EINVAL: Invalid priority
> >> + -EINVAL: Invalid flags
> >> + -EINVAL: Invalid queue size
> >> + -EINVAL: Invalid queue address
> >> + -EFAULT: Invalid user pointer for attr->addr.
> >> + -EIO: Configuration of the underlying HW failed
> >
>
--
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 v3 17/17] KVM: PPC: Book3S HV: XIVE: clear the vCPU interrupt presenters
From: David Gibson @ 2019-03-19 5:37 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: linuxppc-dev, Paul Mackerras, kvm, kvm-ppc
In-Reply-To: <20190315120609.25910-18-clg@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 5652 bytes --]
On Fri, Mar 15, 2019 at 01:06:09PM +0100, Cédric Le Goater wrote:
> When the VM boots, the CAS negotiation process determines which
> interrupt mode to use and invokes a machine reset. At that time, the
> previous KVM interrupt device is 'destroyed' before the chosen one is
> created. Upon destruction, the vCPU interrupt presenters using the KVM
> device should be cleared first, the machine will reconnect them later
> to the new device after it is created.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>
> Changes since v2 :
>
> - removed comments on possible race in kvmppc_native_connect_vcpu()
> for the XIVE KVM device. This is still an issue in the
> XICS-over-XIVE device.
>
> arch/powerpc/kvm/book3s_xics.c | 19 +++++++++++++
> arch/powerpc/kvm/book3s_xive.c | 39 +++++++++++++++++++++++++--
> arch/powerpc/kvm/book3s_xive_native.c | 12 +++++++++
> 3 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c
> index f27ee57ab46e..81cdabf4295f 100644
> --- a/arch/powerpc/kvm/book3s_xics.c
> +++ b/arch/powerpc/kvm/book3s_xics.c
> @@ -1342,6 +1342,25 @@ static void kvmppc_xics_free(struct kvm_device *dev)
> struct kvmppc_xics *xics = dev->private;
> int i;
> struct kvm *kvm = xics->kvm;
> + struct kvm_vcpu *vcpu;
> +
> + /*
> + * When destroying the VM, the vCPUs are destroyed first and
> + * the vCPU list should be empty. If this is not the case,
> + * then we are simply destroying the device and we should
> + * clean up the vCPU interrupt presenters first.
> + */
> + if (atomic_read(&kvm->online_vcpus) != 0) {
> + /*
> + * call kick_all_cpus_sync() to ensure that all CPUs
> + * have executed any pending interrupts
> + */
> + if (is_kvmppc_hv_enabled(kvm))
> + kick_all_cpus_sync();
> +
> + kvm_for_each_vcpu(i, vcpu, kvm)
> + kvmppc_xics_free_icp(vcpu);
> + }
>
> debugfs_remove(xics->dentry);
>
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index 480a3fc6b9fd..cf6a4c6c5a28 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -1100,11 +1100,19 @@ void kvmppc_xive_disable_vcpu_interrupts(struct kvm_vcpu *vcpu)
> void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
> {
> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> - struct kvmppc_xive *xive = xc->xive;
> + struct kvmppc_xive *xive;
> int i;
>
> + if (!kvmppc_xics_enabled(vcpu))
> + return;
> +
> + if (!xc)
> + return;
> +
> pr_devel("cleanup_vcpu(cpu=%d)\n", xc->server_num);
>
> + xive = xc->xive;
> +
> /* Ensure no interrupt is still routed to that VP */
> xc->valid = false;
> kvmppc_xive_disable_vcpu_interrupts(vcpu);
> @@ -1141,6 +1149,10 @@ void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
> }
> /* Free the VP */
> kfree(xc);
> +
> + /* Cleanup the vcpu */
> + vcpu->arch.irq_type = KVMPPC_IRQ_DEFAULT;
> + vcpu->arch.xive_vcpu = NULL;
> }
>
> int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> @@ -1158,7 +1170,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> }
> if (xive->kvm != vcpu->kvm)
> return -EPERM;
> - if (vcpu->arch.irq_type)
> + if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT)
> return -EBUSY;
> if (kvmppc_xive_find_server(vcpu->kvm, cpu)) {
> pr_devel("Duplicate !\n");
> @@ -1828,8 +1840,31 @@ static void kvmppc_xive_free(struct kvm_device *dev)
> {
> struct kvmppc_xive *xive = dev->private;
> struct kvm *kvm = xive->kvm;
> + struct kvm_vcpu *vcpu;
> int i;
>
> + /*
> + * When destroying the VM, the vCPUs are destroyed first and
> + * the vCPU list should be empty. If this is not the case,
> + * then we are simply destroying the device and we should
> + * clean up the vCPU interrupt presenters first.
> + */
> + if (atomic_read(&kvm->online_vcpus) != 0) {
> + /*
> + * call kick_all_cpus_sync() to ensure that all CPUs
> + * have executed any pending interrupts
> + */
> + if (is_kvmppc_hv_enabled(kvm))
> + kick_all_cpus_sync();
> +
> + /*
> + * TODO: There is still a race window with the early
> + * checks in kvmppc_native_connect_vcpu()
> + */
> + kvm_for_each_vcpu(i, vcpu, kvm)
> + kvmppc_xive_cleanup_vcpu(vcpu);
> + }
> +
> debugfs_remove(xive->dentry);
>
> if (kvm)
> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> index 67a1bb26a4cc..8f7be5e23177 100644
> --- a/arch/powerpc/kvm/book3s_xive_native.c
> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> @@ -956,8 +956,20 @@ static void kvmppc_xive_native_free(struct kvm_device *dev)
> {
> struct kvmppc_xive *xive = dev->private;
> struct kvm *kvm = xive->kvm;
> + struct kvm_vcpu *vcpu;
> int i;
>
> + /*
> + * When destroying the VM, the vCPUs are destroyed first and
> + * the vCPU list should be empty. If this is not the case,
> + * then we are simply destroying the device and we should
> + * clean up the vCPU interrupt presenters first.
> + */
> + if (atomic_read(&kvm->online_vcpus) != 0) {
> + kvm_for_each_vcpu(i, vcpu, kvm)
> + kvmppc_xive_native_cleanup_vcpu(vcpu);
> + }
> +
> debugfs_remove(xive->dentry);
>
> pr_devel("Destroying xive native device\n");
--
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 v3 10/17] KVM: PPC: Book3S HV: XIVE: add get/set accessors for the VP XIVE state
From: David Gibson @ 2019-03-19 5:08 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: linuxppc-dev, Paul Mackerras, kvm, kvm-ppc
In-Reply-To: <20190315120609.25910-11-clg@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 2185 bytes --]
On Fri, Mar 15, 2019 at 01:06:02PM +0100, Cédric Le Goater wrote:
> The state of the thread interrupt management registers needs to be
> collected for migration. 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 underlying XIVE NVT structure and merges it in the
> KVM state.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
[snip]
> diff --git a/Documentation/virtual/kvm/devices/xive.txt b/Documentation/virtual/kvm/devices/xive.txt
> index e6a984592189..eb864d72ddc3 100644
> --- a/Documentation/virtual/kvm/devices/xive.txt
> +++ b/Documentation/virtual/kvm/devices/xive.txt
> @@ -104,6 +104,25 @@ the legacy interrupt mode, referred as XICS (POWER7/8).
> -ENOENT: Unknown source number
> -EINVAL: Not initialized source number
>
> +* VCPU state
> +
> + The XIVE IC maintains VP interrupt state in an internal structure
> + called the NVT. When a VP is not dispatched on a HW processor
> + thread, this structure can be updated by HW if the VP is the target
> + of an event notification.
> +
> + It is important for migration to capture the cached IPB from the NVT
> + as it synthesizes the priorities of the pending interrupts. We
> + capture a bit more to report debug information.
> +
> + KVM_REG_PPC_VP_STATE (4 * 64bits)
> + bits: | 63 .... 32 | 31 .... 0 |
> + values: | TIMA word0 | TIMA word1 |
> + bits: | 127 .......... 64 |
> + values: | unused |
> + bits: | 255 .......... 128 |
> + values: | unused |
> +
> * Migration:
>
> Saving the state of a VM using the XIVE native exploitation mode
Nit: looks like this doc needs to be updated now that only 2x 64-bit
values are returned.
--
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 v2 6/6] arm64: ptrace: add support for syscall emulation
From: Haibo Xu (Arm Technology China) @ 2019-03-19 3:26 UTC (permalink / raw)
To: Sudeep Holla, x86@kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Cc: Steve Capper, Catalin Marinas, jdike@addtoit.com, Will Deacon,
Oleg Nesterov, Bin Lu (Arm Technology China), Richard Weinberger,
Ingo Molnar, Paul Mackerras, Andy Lutomirski, Thomas Gleixner
In-Reply-To: <20190318104925.16600-7-sudeep.holla@arm.com>
On 2019/3/18 18:49, Sudeep Holla wrote:
> Add PTRACE_SYSEMU and PTRACE_SYSEMU_SINGLESTEP support on arm64.
> We can just make sure of the generic ptrace_syscall_enter hook to
> support PTRACE_SYSEMU. We don't need any special handling for
> PTRACE_SYSEMU_SINGLESTEP.
This looks good to me. But it'd be better to add the same logic to handle
PTRACE_SYSEMU_SINGLESTEP as that of x86 in case we may need enable the single
step trace function in the future.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> arch/arm64/include/asm/thread_info.h | 5 ++++-
> arch/arm64/kernel/ptrace.c | 3 +++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index eb3ef73e07cf..c285d1ce7186 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -75,6 +75,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> * TIF_SYSCALL_TRACE- syscall trace active
> * TIF_SYSCALL_TRACEPOINT - syscall tracepoint for ftrace
> * TIF_SYSCALL_AUDIT- syscall auditing
> + * TIF_SYSCALL_EMU - syscall emulation active
> * TIF_SECOMP- syscall secure computing
> * TIF_SIGPENDING- signal pending
> * TIF_NEED_RESCHED- rescheduling necessary
> @@ -91,6 +92,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define TIF_SYSCALL_AUDIT9
> #define TIF_SYSCALL_TRACEPOINT10
> #define TIF_SECCOMP11
> +#define TIF_SYSCALL_EMU12
> #define TIF_MEMDIE18/* is terminating due to OOM killer */
> #define TIF_FREEZE19
> #define TIF_RESTORE_SIGMASK20
> @@ -109,6 +111,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define _TIF_SYSCALL_AUDIT(1 << TIF_SYSCALL_AUDIT)
> #define _TIF_SYSCALL_TRACEPOINT(1 << TIF_SYSCALL_TRACEPOINT)
> #define _TIF_SECCOMP(1 << TIF_SECCOMP)
> +#define _TIF_SYSCALL_EMU(1 << TIF_SYSCALL_EMU)
> #define _TIF_UPROBE(1 << TIF_UPROBE)
> #define _TIF_FSCHECK(1 << TIF_FSCHECK)
> #define _TIF_32BIT(1 << TIF_32BIT)
> @@ -120,7 +123,7 @@ void arch_release_task_struct(struct task_struct *tsk);
>
> #define _TIF_SYSCALL_WORK(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
> _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \
> - _TIF_NOHZ)
> + _TIF_NOHZ | _TIF_SYSCALL_EMU)
>
> #define INIT_THREAD_INFO(tsk)\
> {\
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index b82e0a9b3da3..cf29275cd4d9 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -1819,6 +1819,9 @@ static void tracehook_report_syscall(struct pt_regs *regs,
>
> int syscall_trace_enter(struct pt_regs *regs)
> {
> +if (unlikely(ptrace_syscall_enter(regs)))
> +return -1;
> +
> if (test_thread_flag(TIF_SYSCALL_TRACE))
> tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply
* RE: [alsa-devel] [PATCH V4] ASoC: fsl_esai: fix channel swap issue when stream starts
From: S.j. Wang @ 2019-03-19 2:08 UTC (permalink / raw)
To: S.j. Wang, timur@kernel.org, nicoleotsuka@gmail.com,
Xiubo.Lee@gmail.com, festevam@gmail.com, broonie@kernel.org,
alsa-devel@alsa-project.org
Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <f05c95e5371336a8e3b0c395dc4cef70922f0caa.1551248985.git.shengjiu.wang@nxp.com>
Hi Mark
Can this patch be accepted? Or need I do any update?
Best regards
Wang shengjiu
>
> There is very low possibility ( < 0.1% ) that channel swap happened in
> beginning when multi output/input pin is enabled. The issue is that
> hardware can't send data to correct pin in the beginning with the normal
> enable flow.
>
> This is hardware issue, but there is no errata, the workaround flow is that:
> Each time playback/recording, firstly clear the xSMA/xSMB, then enable
> TE/RE, then enable xSMB and xSMA (xSMB must be enabled before xSMA).
> Which is to use the xSMA as the trigger start register, previously the xCR_TE
> or xCR_RE is the bit for starting.
>
> Fixes commit 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
> Cc: <stable@vger.kernel.org>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> sound/soc/fsl/fsl_esai.c | 47 +++++++++++++++++++++++++++++++++++++---
> -------
> 1 file changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> afe67c865330..3623aa9a6f2e 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -54,6 +54,8 @@ struct fsl_esai {
> u32 fifo_depth;
> u32 slot_width;
> u32 slots;
> + u32 tx_mask;
> + u32 rx_mask;
> u32 hck_rate[2];
> u32 sck_rate[2];
> bool hck_dir[2];
> @@ -361,21 +363,13 @@ static int fsl_esai_set_dai_tdm_slot(struct
> snd_soc_dai *dai, u32 tx_mask,
> regmap_update_bits(esai_priv->regmap, REG_ESAI_TCCR,
> ESAI_xCCR_xDC_MASK, ESAI_xCCR_xDC(slots));
>
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMA,
> - ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(tx_mask));
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMB,
> - ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(tx_mask));
> -
> regmap_update_bits(esai_priv->regmap, REG_ESAI_RCCR,
> ESAI_xCCR_xDC_MASK, ESAI_xCCR_xDC(slots));
>
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMA,
> - ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(rx_mask));
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMB,
> - ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(rx_mask));
> -
> esai_priv->slot_width = slot_width;
> esai_priv->slots = slots;
> + esai_priv->tx_mask = tx_mask;
> + esai_priv->rx_mask = rx_mask;
>
> return 0;
> }
> @@ -596,6 +590,7 @@ static int fsl_esai_trigger(struct snd_pcm_substream
> *substream, int cmd,
> bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> u8 i, channels = substream->runtime->channels;
> u32 pins = DIV_ROUND_UP(channels, esai_priv->slots);
> + u32 mask;
>
> switch (cmd) {
> case SNDRV_PCM_TRIGGER_START:
> @@ -608,15 +603,38 @@ static int fsl_esai_trigger(struct
> snd_pcm_substream *substream, int cmd,
> for (i = 0; tx && i < channels; i++)
> regmap_write(esai_priv->regmap, REG_ESAI_ETDR,
> 0x0);
>
> + /*
> + * When set the TE/RE in the end of enablement flow, there
> + * will be channel swap issue for multi data line case.
> + * In order to workaround this issue, we switch the bit
> + * enablement sequence to below sequence
> + * 1) clear the xSMB & xSMA: which is done in probe and
> + * stop state.
> + * 2) set TE/RE
> + * 3) set xSMB
> + * 4) set xSMA: xSMA is the last one in this flow, which
> + * will trigger esai to start.
> + */
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> tx ? ESAI_xCR_TE_MASK :
> ESAI_xCR_RE_MASK,
> tx ? ESAI_xCR_TE(pins) :
> ESAI_xCR_RE(pins));
> + mask = tx ? esai_priv->tx_mask : esai_priv->rx_mask;
> +
> + regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMB(tx),
> + ESAI_xSMB_xS_MASK,
> ESAI_xSMB_xS(mask));
> + regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMA(tx),
> + ESAI_xSMA_xS_MASK,
> ESAI_xSMA_xS(mask));
> +
> break;
> case SNDRV_PCM_TRIGGER_SUSPEND:
> case SNDRV_PCM_TRIGGER_STOP:
> case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> tx ? ESAI_xCR_TE_MASK :
> ESAI_xCR_RE_MASK, 0);
> + regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMA(tx),
> + ESAI_xSMA_xS_MASK, 0);
> + regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMB(tx),
> + ESAI_xSMB_xS_MASK, 0);
>
> /* Disable and reset FIFO */
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
> @@ -906,6 +924,15 @@ static int fsl_esai_probe(struct platform_device
> *pdev)
> return ret;
> }
>
> + esai_priv->tx_mask = 0xFFFFFFFF;
> + esai_priv->rx_mask = 0xFFFFFFFF;
> +
> + /* Clear the TSMA, TSMB, RSMA, RSMB */
> + regmap_write(esai_priv->regmap, REG_ESAI_TSMA, 0);
> + regmap_write(esai_priv->regmap, REG_ESAI_TSMB, 0);
> + regmap_write(esai_priv->regmap, REG_ESAI_RSMA, 0);
> + regmap_write(esai_priv->regmap, REG_ESAI_RSMB, 0);
> +
> ret = devm_snd_soc_register_component(&pdev->dev,
> &fsl_esai_component,
> &fsl_esai_dai, 1);
> if (ret) {
> --
> 1.9.1
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> ilman.alsa-project.org%2Fmailman%2Flistinfo%2Falsa-
> devel&data=02%7C01%7Cshengjiu.wang%40nxp.com%7Ce2940159f310
> 4473866a08d69c7d6c1e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C636868459834438771&sdata=WdeHv2JxOBqb0mQ%2B%2FrRISAt64
> JZ6ATY2u02IzzFDaEY%3D&reserved=0
^ permalink raw reply
* Re: [PULL REQUEST] powerpc generic command line
From: Michael Ellerman @ 2019-03-19 1:18 UTC (permalink / raw)
To: Daniel Walker, Andrew Morton; +Cc: Paul Mackerras, linuxppc-dev
In-Reply-To: <1551469472-53043-1-git-send-email-danielwa@cisco.com>
Hi Daniel,
Daniel Walker <danielwa@cisco.com> writes:
> Here are the generic command line changes for powerpc.
>
> These changes have been in linux-next for two cycles, with few problems reported.
> It's also been used at Cisco Systems, Inc. in production products for many many
> years with no problems.
>
> Please pull these changes.
Sorry I didn't reply to this earlier, have been busy with merge window
bugs and so on.
As I imagine you noticed, I didn't pull this. There are a few reasons.
Firstly you sent it a bit late, about a day before the 5.0 release, and
at 6am Saturday my time :) In future if you want me to merge something
please send a pull at least the ~Wednesday before the release.
Secondly I had no idea this code was even in linux-next. I'm not sure if
I was Cc'ed at some point when you added it, if so sorry I missed it,
but I get lots of email. If you're going to add changes to arch/powerpc
in your next tree I'd appreciate some notice, or preferably an explicit
ack.
The main reason I didn't merge it is that it's adding a bunch of code
outside of arch/powerpc, into files which I'm not the maintainer for,
and the patches doing so have no acks or reviews from anyone.
It's also adding a generic implementation with no indication that any
other arches are willing/able to use the generic implementation, which
begs the question whether it will actually used.
I appreciate it's hard to get these sort of cross architecture changes
into mainline, but I don't think this is the way to do it.
I'd suggest you post a patch series to linux-arch with the generic
changes and as many architecture conversions as you can manage, then get
some review/acks for the generic changes and chase arch maintainers for
some acks.
I realise you have posted the series before, it may require some
persistence. There were also quite a few comments from Christophe, so
replying to those would be a good place to start.
> The following changes since commit ccda4af0f4b92f7b4c308d3acc262f4a7e3affad:
>
> Linux 4.20-rc2 (2018-11-11 17:12:31 -0600)
>
> are available in the git repository at:
>
> https://github.com/daniel-walker/cisco-linux.git for-powerpc
>
> for you to fetch changes up to 5d4514a9c291ecf19b0626695161673d35e5d549:
>
> powerpc: convert config files to generic cmdline (2018-11-16 07:32:26 -0800)
>
> ----------------------------------------------------------------
> Daniel Walker (3):
> add generic builtin command line
> powerpc: convert to generic builtin command line
> powerpc: convert config files to generic cmdline
>
> arch/powerpc/Kconfig | 23 +--------
> arch/powerpc/configs/44x/fsp2_defconfig | 29 ++++++-----
> arch/powerpc/configs/44x/iss476-smp_defconfig | 24 ++++-----
> arch/powerpc/configs/44x/warp_defconfig | 12 ++---
> arch/powerpc/configs/holly_defconfig | 12 ++---
> arch/powerpc/configs/mvme5100_defconfig | 25 +++++-----
> arch/powerpc/configs/skiroot_defconfig | 48 +++++++++---------
> arch/powerpc/configs/storcenter_defconfig | 15 +++---
Also if you're updating defconfigs please don't include any unrelated
changes. Trimming the defconfigs can silently drop symbols and break
people's setups so needs to be done carefully.
It's safer to just sed the defconfig files directly, rather than running
savedefconfig on them.
cheers
> arch/powerpc/kernel/prom.c | 4 ++
> arch/powerpc/kernel/prom_init.c | 8 +--
> arch/powerpc/kernel/prom_init_check.sh | 2 +-
> include/linux/cmdline.h | 72 +++++++++++++++++++++++++++
> init/Kconfig | 69 +++++++++++++++++++++++++
> 13 files changed, 231 insertions(+), 112 deletions(-)
> create mode 100644 include/linux/cmdline.h
^ permalink raw reply
* Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode
From: Michael Ellerman @ 2019-03-19 0:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: leo.barbosa, Herbert Xu, Stephan Mueller, nayna, omosnacek,
Eric Biggers, marcelo.cerri, pfsmorigo,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, leitao,
linuxppc-dev, Daniel Axtens
In-Reply-To: <CAKv+Gu_w_d5AbkYu=fdK1ver97G4O_rqu9o9tNy_-zNaAOnkkA@mail.gmail.com>
Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:
> On Mon, 18 Mar 2019 at 09:41, Michael Ellerman <mpe@ellerman.id.au> wrote:
...
>>
>> I don't understand how the crypto core chooses which crypto_alg to use,
>> but I didn't expect enabling the tests to change it?
>
> This is not entirely unexpected. Based on the tests, algos that are
> found to be broken are disregarded for further use, and you should see
> a warning in the kernel log about this.
Ah right that makes sense then. I wasn't looking at the kernel log, just
rerunning the kcapi reproducer. Thanks for clarifying.
cheers
^ permalink raw reply
* Re: [PATCH v2 0/6] ptrace: consolidate PTRACE_SYSEMU handling and add support for arm64
From: Andy Lutomirski @ 2019-03-18 20:04 UTC (permalink / raw)
To: Sudeep Holla
Cc: Haibo Xu, Steve Capper, Catalin Marinas, Jeff Dike, X86 ML,
Will Deacon, LKML, Oleg Nesterov, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, Bin Lu,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20190318104925.16600-1-sudeep.holla@arm.com>
On Mon, Mar 18, 2019 at 3:49 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Hi,
>
> This patchset evolved from the discussion in the thread[0][1]. When we
> wanted to add PTRACE_SYSEMU support to ARM64, we thought instead of
> duplicating what other architectures like x86 and powerpc have done,
> let consolidate the existing support and move it to the core as there's
> nothing arch specific in it.
>
In the discussion from the first version, there was talk of some
testing. Can you put the test cases in question into v3?
--Andy
^ permalink raw reply
* Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU
From: Sudeep Holla @ 2019-03-18 17:40 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Bin Lu, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20190318173341.GD23521@redhat.com>
On Mon, Mar 18, 2019 at 06:33:41PM +0100, Oleg Nesterov wrote:
> On 03/18, Sudeep Holla wrote:
> >
> > On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> > >
> > > Again, to me this patch just makes the code look worse. Honestly, I don't
> > > think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
> > >
> >
> > Worse because we end up reading current_thread_info->flags twice ?
>
> Mostly because in my opinion ptrace_syscall_enter() buys nothing but makes
> the caller's code less readable/understandable.
>
> Sure, this is subjective.
>
Based on what we have in that function today, I tend to agree. Will and
Richard were in the opinion to consolidate SYSEMU handling(in the threads
pointed in my cover letter). If there's a better way to achieve the same
I am in for it. I have just tried to put something together based on
what I could think of.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU
From: Oleg Nesterov @ 2019-03-18 17:33 UTC (permalink / raw)
To: Sudeep Holla
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Bin Lu, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20190318172457.GD18196@e107155-lin>
On 03/18, Sudeep Holla wrote:
>
> On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> >
> > Again, to me this patch just makes the code look worse. Honestly, I don't
> > think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
> >
>
> Worse because we end up reading current_thread_info->flags twice ?
Mostly because in my opinion ptrace_syscall_enter() buys nothing but makes
the caller's code less readable/understandable.
Sure, this is subjective.
Oleg.
^ permalink raw reply
* Re: [PATCH v2 1/6] ptrace: move clearing of TIF_SYSCALL_EMU flag to core
From: Oleg Nesterov @ 2019-03-18 17:29 UTC (permalink / raw)
To: Sudeep Holla
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Bin Lu, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20190318104925.16600-2-sudeep.holla@arm.com>
On 03/18, Sudeep Holla wrote:
>
@@ -534,6 +534,10 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)
> /* Architecture-specific hardware disable .. */
> ptrace_disable(child);
>
> +#ifdef TIF_SYSCALL_EMU
> + clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
> +#endif
perhaps it makes sense to factor out clear_tsk_thread_flag(TIF_SYSCALL_EMU), but
then we should probably clear it along with TIF_SYSCALL_TRACE in __ptrace_unlink?
Oleg.
^ permalink raw reply
* Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU
From: Sudeep Holla @ 2019-03-18 17:24 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Bin Lu, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20190318172024.GB23521@redhat.com>
On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> On 03/18, Sudeep Holla wrote:
> >
> > --- a/arch/powerpc/kernel/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace.c
> > @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> >
> > user_exit();
> >
> > - flags = READ_ONCE(current_thread_info()->flags) &
> > - (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> > -
> > - if (flags) {
> > - int rc = tracehook_report_syscall_entry(regs);
> > + if (unlikely(ptrace_syscall_enter(regs))) {
> > + /*
> > + * A nonzero return code from tracehook_report_syscall_entry()
> > + * tells us to prevent the syscall execution, but we are not
> > + * going to execute it anyway.
> > + *
> > + * Returning -1 will skip the syscall execution. We want to
> > + * avoid clobbering any registers, so we don't goto the skip
> > + * label below.
> > + */
> > + return -1;
> > + }
> >
> > - if (unlikely(flags & _TIF_SYSCALL_EMU)) {
> > - /*
> > - * A nonzero return code from
> > - * tracehook_report_syscall_entry() tells us to prevent
> > - * the syscall execution, but we are not going to
> > - * execute it anyway.
> > - *
> > - * Returning -1 will skip the syscall execution. We want
> > - * to avoid clobbering any registers, so we don't goto
> > - * the skip label below.
> > - */
> > - return -1;
> > - }
> > + flags = READ_ONCE(current_thread_info()->flags) & _TIF_SYSCALL_TRACE;
>
> Why do we need READ_ONCE() with this change?
>
> And now that we change a single bit "flags" doesn't look like a good name.
>
> Again, to me this patch just makes the code look worse. Honestly, I don't
> think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
>
Worse because we end up reading current_thread_info->flags twice ?
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU
From: Oleg Nesterov @ 2019-03-18 17:20 UTC (permalink / raw)
To: Sudeep Holla
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Bin Lu, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20190318104925.16600-5-sudeep.holla@arm.com>
On 03/18, Sudeep Holla wrote:
>
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
>
> user_exit();
>
> - flags = READ_ONCE(current_thread_info()->flags) &
> - (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> -
> - if (flags) {
> - int rc = tracehook_report_syscall_entry(regs);
> + if (unlikely(ptrace_syscall_enter(regs))) {
> + /*
> + * A nonzero return code from tracehook_report_syscall_entry()
> + * tells us to prevent the syscall execution, but we are not
> + * going to execute it anyway.
> + *
> + * Returning -1 will skip the syscall execution. We want to
> + * avoid clobbering any registers, so we don't goto the skip
> + * label below.
> + */
> + return -1;
> + }
>
> - if (unlikely(flags & _TIF_SYSCALL_EMU)) {
> - /*
> - * A nonzero return code from
> - * tracehook_report_syscall_entry() tells us to prevent
> - * the syscall execution, but we are not going to
> - * execute it anyway.
> - *
> - * Returning -1 will skip the syscall execution. We want
> - * to avoid clobbering any registers, so we don't goto
> - * the skip label below.
> - */
> - return -1;
> - }
> + flags = READ_ONCE(current_thread_info()->flags) & _TIF_SYSCALL_TRACE;
Why do we need READ_ONCE() with this change?
And now that we change a single bit "flags" doesn't look like a good name.
Again, to me this patch just makes the code look worse. Honestly, I don't
think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
Oleg.
^ permalink raw reply
* Re: [PATCH v2 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook
From: Oleg Nesterov @ 2019-03-18 15:33 UTC (permalink / raw)
To: Sudeep Holla
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Bin Lu, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Borislav Petkov, Thomas Gleixner,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20190318104925.16600-4-sudeep.holla@arm.com>
On 03/18, Sudeep Holla wrote:
>
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -70,22 +70,16 @@ static long syscall_trace_enter(struct pt_regs *regs)
>
> struct thread_info *ti = current_thread_info();
> unsigned long ret = 0;
> - bool emulated = false;
> u32 work;
>
> if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
> BUG_ON(regs != task_pt_regs(current));
>
> - work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY;
> -
> - if (unlikely(work & _TIF_SYSCALL_EMU))
> - emulated = true;
> -
> - if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
> - tracehook_report_syscall_entry(regs))
> + if (unlikely(ptrace_syscall_enter(regs)))
> return -1L;
>
> - if (emulated)
> + work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY;
> + if ((work & _TIF_SYSCALL_TRACE) && tracehook_report_syscall_entry(regs))
> return -1L;
Well, I won't really argue, but to be honest I think this change doesn't make
the code better... With this patch tracehook_report_syscall_entry() has 2 callers,
to me this just adds some confusion.
I agree that the usage of emulated/_TIF_SYSCALL_EMU looks a bit overcomplicated,
I'd suggest a simple cleanup below.
And it seems that _TIF_WORK_SYSCALL_ENTRY needs some cleanups too... We don't need
"& _TIF_WORK_SYSCALL_ENTRY" in syscall_trace_enter, and _TIF_WORK_SYSCALL_ENTRY
should not include _TIF_NOHZ?
Oleg.
--- x/arch/x86/entry/common.c
+++ x/arch/x86/entry/common.c
@@ -70,23 +70,18 @@ static long syscall_trace_enter(struct pt_regs *regs)
struct thread_info *ti = current_thread_info();
unsigned long ret = 0;
- bool emulated = false;
u32 work;
if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
BUG_ON(regs != task_pt_regs(current));
- work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY;
+ work = READ_ONCE(ti->flags);
- if (unlikely(work & _TIF_SYSCALL_EMU))
- emulated = true;
-
- if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
- tracehook_report_syscall_entry(regs))
- return -1L;
-
- if (emulated)
- return -1L;
+ if (work & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
+ ret = tracehook_report_syscall_entry(regs);
+ if (ret || (work & _TIF_SYSCALL_EMU))
+ return -1L;
+ }
#ifdef CONFIG_SECCOMP
/*
^ permalink raw reply
* Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU
From: Sudeep Holla @ 2019-03-18 14:59 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Oleg Nesterov, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, Bin Lu,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20190318142618.GA27941@altlinux.org>
On Mon, Mar 18, 2019 at 05:26:18PM +0300, Dmitry V. Levin wrote:
> On Mon, Mar 18, 2019 at 10:49:23AM +0000, Sudeep Holla wrote:
> > Now that we have a new hook ptrace_syscall_enter that can be called from
> > syscall entry code and it handles PTRACE_SYSEMU in generic code, we
> > can do some cleanup using the same in do_syscall_trace_enter.
> >
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> > arch/powerpc/kernel/ptrace.c | 48 ++++++++++++++++--------------------
> > 1 file changed, 21 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> > index 2e2183b800a8..05579a5dcb12 100644
> > --- a/arch/powerpc/kernel/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace.c
> > @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> >
> > user_exit();
> >
> > - flags = READ_ONCE(current_thread_info()->flags) &
> > - (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> > -
> > - if (flags) {
> > - int rc = tracehook_report_syscall_entry(regs);
> > + if (unlikely(ptrace_syscall_enter(regs))) {
> > + /*
> > + * A nonzero return code from tracehook_report_syscall_entry()
> > + * tells us to prevent the syscall execution, but we are not
> > + * going to execute it anyway.
> > + *
> > + * Returning -1 will skip the syscall execution. We want to
> > + * avoid clobbering any registers, so we don't goto the skip
> > + * label below.
> > + */
> > + return -1;
> > + }
>
> This comment is out of sync with the changed code.
Still applicable indirectly as ptrace_syscall_enter just executes
tracehook_report_syscall_entry, but I agree needs rewording, will update.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH v2 2/6] ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling
From: Sudeep Holla @ 2019-03-18 14:57 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Oleg Nesterov, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, Bin Lu,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20190318144115.GC27941@altlinux.org>
On Mon, Mar 18, 2019 at 05:41:15PM +0300, Dmitry V. Levin wrote:
> On Mon, Mar 18, 2019 at 10:49:21AM +0000, Sudeep Holla wrote:
> > Currently each architecture handles PTRACE_SYSEMU in very similar way.
> > It's completely arch independent and can be handled in the code helping
> > to consolidate PTRACE_SYSEMU handling.
> >
> > Let's introduce a hook 'ptrace_syscall_enter' that arch specific syscall
> > entry code can call.
>
> Sorry if I'm late for the party, but the new name looks confusing.
> If all it does is related to TIF_SYSCALL_EMU, why does it have a generic
> name 'ptrace_syscall_enter' without any hint of being specific to
> TIF_SYSCALL_EMU?
>
Not at all late. Infact Haibo Xu pointed that out, I updated but somehow
missed to commit and lost those changes. I will rename as
ptrace_sysemu_syscall_enter
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH v3 06/17] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
From: Cédric Le Goater @ 2019-03-18 14:38 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev, Paul Mackerras, kvm, kvm-ppc
In-Reply-To: <8b3caeef-8750-b8db-d516-c722bc08c535@kaod.org>
[ ... ]
>>> + page = gfn_to_page(kvm, gpa_to_gfn(kvm_eq.qpage));
>>> + if (is_error_page(page)) {
>>> + pr_warn("Couldn't get guest page for %llx!\n", kvm_eq.qpage);
>>> + return -EINVAL;
>>> + }
>>
>> Yeah.. for the case of a 4kiB page host (these days weird, but not
>> actually prohibited, AFAIK) you need to check that the qsize selected
>> actually fits within the page.
>
> Ah yes. sure.
>
>>> + qaddr = page_to_virt(page) + (kvm_eq.qpage & ~PAGE_MASK);
>>> +
>>> + /* Backup queue page guest address for migration */
>>
>> Hm.. KVM itself shouldn't generally need to know about migration.
>> IIUC these values won't change from what qemu set them to be, so it
>> should be able to store and migrate them without have to get them back
>> from the kernel.
>
> Euh. You are completely right. I don't know why I kept those around.
No. I do need these values in patch 9 "KVM: PPC: Book3S HV: XIVE: add
a control to dirty the XIVE EQ pages" where the EQ pages are marked
dirty for migration:
+ /* Mark EQ page dirty for migration */
+ mark_page_dirty(vcpu->kvm, gpa_to_gfn(q->guest_qpage));
We could change the kvmppc_xive_native_vcpu_eq_sync() to work on a
EQ basis and not on a device basis. In this case, we could pass the
EQ guest address again. That would change a bit the save sequence.
C.
>>> + q->guest_qpage = kvm_eq.qpage;
>>> + q->guest_qsize = kvm_eq.qsize;
^ permalink raw reply
* Re: [PATCH v2 2/6] ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling
From: Sudeep Holla @ 2019-03-18 14:55 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Oleg Nesterov, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, Bin Lu,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20190318143147.GB27941@altlinux.org>
On Mon, Mar 18, 2019 at 05:31:47PM +0300, Dmitry V. Levin wrote:
> On Mon, Mar 18, 2019 at 10:49:21AM +0000, Sudeep Holla wrote:
> > Currently each architecture handles PTRACE_SYSEMU in very similar way.
> > It's completely arch independent and can be handled in the code helping
> > to consolidate PTRACE_SYSEMU handling.
> >
> > Let's introduce a hook 'ptrace_syscall_enter' that arch specific syscall
> > entry code can call.
> >
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> > include/linux/ptrace.h | 1 +
> > kernel/ptrace.c | 22 ++++++++++++++++++++++
> > 2 files changed, 23 insertions(+)
> >
> > diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> > index edb9b040c94c..e30f51e3363e 100644
> > --- a/include/linux/ptrace.h
> > +++ b/include/linux/ptrace.h
> > @@ -407,6 +407,7 @@ static inline void user_single_step_report(struct pt_regs *regs)
> > #define current_user_stack_pointer() user_stack_pointer(current_pt_regs())
> > #endif
> >
> > +extern long ptrace_syscall_enter(struct pt_regs *regs);
> > extern int task_current_syscall(struct task_struct *target, long *callno,
> > unsigned long args[6], unsigned int maxargs,
> > unsigned long *sp, unsigned long *pc);
> > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > index 4fa3b7f4c3c7..c9c505c483df 100644
> > --- a/kernel/ptrace.c
> > +++ b/kernel/ptrace.c
> > @@ -29,6 +29,7 @@
> > #include <linux/hw_breakpoint.h>
> > #include <linux/cn_proc.h>
> > #include <linux/compat.h>
> > +#include <linux/tracehook.h>
> >
> > /*
> > * Access another process' address space via ptrace.
> > @@ -557,6 +558,27 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)
> > return 0;
> > }
> >
> > +/*
> > + * Hook to check and report for PTRACE_SYSEMU, can be called from arch
> > + * arch syscall entry code
> > + */
> > +long ptrace_syscall_enter(struct pt_regs *regs)
> > +{
> > +#ifdef TIF_SYSCALL_EMU
> > + if (test_thread_flag(TIF_SYSCALL_EMU)) {
> > + if (tracehook_report_syscall_entry(regs))
> > + /*
> > + * We can ignore the return code here as we need
> > + * return -1 always for syscall emulation irrespective
> > + * of whether the tracehook report fails or succeed.
> > + */
> > + ;
>
> This is problematic as it causes build errors with -Werror=empty-body,
> see https://lore.kernel.org/lkml/20181218205305.26647-1-malat@debian.org/
>
Thanks for the pointer, will update.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH v3 06/17] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
From: Cédric Le Goater @ 2019-03-18 14:12 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev, Paul Mackerras, kvm, kvm-ppc
In-Reply-To: <20190318032348.GH6874@umbus.fritz.box>
On 3/18/19 4:23 AM, David Gibson wrote:
> On Fri, Mar 15, 2019 at 01:05:58PM +0100, Cédric Le Goater wrote:
>> These controls will be used by the H_INT_SET_QUEUE_CONFIG and
>> H_INT_GET_QUEUE_CONFIG hcalls from QEMU to configure the underlying
>> Event Queue in the XIVE IC. They will also be used to restore the
>> configuration of the XIVE EQs and to capture the internal run-time
>> state of the EQs. Both 'get' and 'set' rely on an OPAL call to access
>> the EQ toggle bit and EQ index which are updated by the XIVE IC when
>> event notifications are enqueued in the EQ.
>>
>> The value of the guest physical address of the event queue is saved in
>> the XIVE internal xive_q structure for later use. That is when
>> migration needs to mark the EQ pages dirty to capture a consistent
>> memory state of the VM.
>>
>> To be noted that H_INT_SET_QUEUE_CONFIG does not require the extra
>> OPAL call setting the EQ toggle bit and EQ index to configure the EQ,
>> but restoring the EQ state will.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>> Changes since v2 :
>>
>> - fixed comments on the KVM device attribute definitions
>> - fixed check on supported EQ size to restrict to 64K pages
>> - checked kvm_eq.flags that need to be zero
>> - removed the OPAL call when EQ qtoggle bit and index are zero.
>>
>> arch/powerpc/include/asm/xive.h | 2 +
>> arch/powerpc/include/uapi/asm/kvm.h | 21 ++
>> arch/powerpc/kvm/book3s_xive.h | 2 +
>> arch/powerpc/kvm/book3s_xive.c | 15 +-
>> arch/powerpc/kvm/book3s_xive_native.c | 232 +++++++++++++++++++++
>> Documentation/virtual/kvm/devices/xive.txt | 31 +++
>> 6 files changed, 297 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
>> index b579a943407b..46891f321606 100644
>> --- a/arch/powerpc/include/asm/xive.h
>> +++ b/arch/powerpc/include/asm/xive.h
>> @@ -73,6 +73,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/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>> index 12bb01baf0ae..1cd728c87d7c 100644
>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>> @@ -679,6 +679,7 @@ struct kvm_ppc_cpu_char {
>> #define KVM_DEV_XIVE_GRP_CTRL 1
>> #define KVM_DEV_XIVE_GRP_SOURCE 2 /* 64-bit source identifier */
>> #define KVM_DEV_XIVE_GRP_SOURCE_CONFIG 3 /* 64-bit source identifier */
>> +#define KVM_DEV_XIVE_GRP_EQ_CONFIG 4 /* 64-bit EQ identifier */
>>
>> /* Layout of 64-bit XIVE source attribute values */
>> #define KVM_XIVE_LEVEL_SENSITIVE (1ULL << 0)
>> @@ -694,4 +695,24 @@ struct kvm_ppc_cpu_char {
>> #define KVM_XIVE_SOURCE_EISN_SHIFT 33
>> #define KVM_XIVE_SOURCE_EISN_MASK 0xfffffffe00000000ULL
>>
>> +/* Layout of 64-bit EQ identifier */
>> +#define KVM_XIVE_EQ_PRIORITY_SHIFT 0
>> +#define KVM_XIVE_EQ_PRIORITY_MASK 0x7
>> +#define KVM_XIVE_EQ_SERVER_SHIFT 3
>> +#define KVM_XIVE_EQ_SERVER_MASK 0xfffffff8ULL
>> +
>> +/* Layout of EQ configuration values (64 bytes) */
>> +struct kvm_ppc_xive_eq {
>> + __u32 flags;
>> + __u32 qsize;
>> + __u64 qpage;
>> + __u32 qtoggle;
>> + __u32 qindex;
>> + __u8 pad[40];
>> +};
>> +
>> +#define KVM_XIVE_EQ_FLAG_ENABLED 0x00000001
>> +#define KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY 0x00000002
>> +#define KVM_XIVE_EQ_FLAG_ESCALATE 0x00000004
>> +
>> #endif /* __LINUX_KVM_POWERPC_H */
>> diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
>> index ae26fe653d98..622f594d93e1 100644
>> --- a/arch/powerpc/kvm/book3s_xive.h
>> +++ b/arch/powerpc/kvm/book3s_xive.h
>> @@ -272,6 +272,8 @@ struct kvmppc_xive_src_block *kvmppc_xive_create_src_block(
>> struct kvmppc_xive *xive, int irq);
>> void kvmppc_xive_free_sources(struct kvmppc_xive_src_block *sb);
>> int kvmppc_xive_select_target(struct kvm *kvm, u32 *server, u8 prio);
>> +int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
>> + bool single_escalation);
>>
>> #endif /* CONFIG_KVM_XICS */
>> #endif /* _KVM_PPC_BOOK3S_XICS_H */
>> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
>> index e09f3addffe5..c1b7aa7dbc28 100644
>> --- a/arch/powerpc/kvm/book3s_xive.c
>> +++ b/arch/powerpc/kvm/book3s_xive.c
>> @@ -166,7 +166,8 @@ static irqreturn_t xive_esc_irq(int irq, void *data)
>> return IRQ_HANDLED;
>> }
>>
>> -static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
>> +int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
>> + bool single_escalation)
>> {
>> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>> struct xive_q *q = &xc->queues[prio];
>> @@ -185,7 +186,7 @@ static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
>> return -EIO;
>> }
>>
>> - if (xc->xive->single_escalation)
>> + if (single_escalation)
>> name = kasprintf(GFP_KERNEL, "kvm-%d-%d",
>> vcpu->kvm->arch.lpid, xc->server_num);
>> else
>> @@ -217,7 +218,7 @@ static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
>> * interrupt, thus leaving it effectively masked after
>> * it fires once.
>> */
>> - if (xc->xive->single_escalation) {
>> + if (single_escalation) {
>> struct irq_data *d = irq_get_irq_data(xc->esc_virq[prio]);
>> struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
>>
>> @@ -291,7 +292,8 @@ static int xive_check_provisioning(struct kvm *kvm, u8 prio)
>> continue;
>> rc = xive_provision_queue(vcpu, prio);
>> if (rc == 0 && !xive->single_escalation)
>> - xive_attach_escalation(vcpu, prio);
>> + kvmppc_xive_attach_escalation(vcpu, prio,
>> + xive->single_escalation);
>> if (rc)
>> return rc;
>> }
>> @@ -1214,7 +1216,8 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
>> if (xive->qmap & (1 << i)) {
>> r = xive_provision_queue(vcpu, i);
>> if (r == 0 && !xive->single_escalation)
>> - xive_attach_escalation(vcpu, i);
>> + kvmppc_xive_attach_escalation(
>> + vcpu, i, xive->single_escalation);
>> if (r)
>> goto bail;
>> } else {
>> @@ -1229,7 +1232,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
>> }
>>
>> /* If not done above, attach priority 0 escalation */
>> - r = xive_attach_escalation(vcpu, 0);
>> + r = kvmppc_xive_attach_escalation(vcpu, 0, xive->single_escalation);
>> if (r)
>> goto bail;
>>
>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>> index b841d339f674..42e824658a30 100644
>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
>> @@ -340,6 +340,226 @@ static int kvmppc_xive_native_set_source_config(struct kvmppc_xive *xive,
>> priority, masked, eisn);
>> }
>>
>> +static int xive_native_validate_queue_size(u32 qsize)
>> +{
>> + /*
>> + * We only support 64K pages for the moment. This is also
>> + * advertised in the DT property "ibm,xive-eq-sizes"
>
> IIUC, that won't work properly if you had a guest using 4kiB pages.
> That's fine, but do we have somewhere that checks for that case and
> throws a suitable error?
Not in the device.
So, we should check the current page_size of the guest ? Is there a way
to do that simply from KVM ?
>> + */
>> + switch (qsize) {
>> + case 0: /* EQ reset */
>> + case 16:
>> + return 0;
>> + case 12:
>> + case 21:
>> + case 24:
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static int kvmppc_xive_native_set_queue_config(struct kvmppc_xive *xive,
>> + long eq_idx, u64 addr)
>> +{
>> + struct kvm *kvm = xive->kvm;
>> + struct kvm_vcpu *vcpu;
>> + struct kvmppc_xive_vcpu *xc;
>> + void __user *ubufp = (u64 __user *) addr;
>
> Nit: that should be (void __user *) on the right, shouldn't it?
yes.
>
>> + u32 server;
>> + u8 priority;
>> + struct kvm_ppc_xive_eq kvm_eq;
>> + int rc;
>> + __be32 *qaddr = 0;
>> + struct page *page;
>> + struct xive_q *q;
>> +
>> + /*
>> + * Demangle priority/server tuple from the EQ identifier
>> + */
>> + priority = (eq_idx & KVM_XIVE_EQ_PRIORITY_MASK) >>
>> + KVM_XIVE_EQ_PRIORITY_SHIFT;
>> + server = (eq_idx & KVM_XIVE_EQ_SERVER_MASK) >>
>> + KVM_XIVE_EQ_SERVER_SHIFT;
>> +
>> + if (copy_from_user(&kvm_eq, ubufp, sizeof(kvm_eq)))
>> + return -EFAULT;
>> +
>> + vcpu = kvmppc_xive_find_server(kvm, server);
>> + if (!vcpu) {
>> + pr_err("Can't find server %d\n", server);
>> + return -ENOENT;
>> + }
>> + xc = vcpu->arch.xive_vcpu;
>> +
>> + if (priority != xive_prio_from_guest(priority)) {
>> + pr_err("Trying to restore invalid queue %d for VCPU %d\n",
>> + priority, server);
>> + return -EINVAL;
>> + }
>> + q = &xc->queues[priority];
>> +
>> + pr_devel("%s VCPU %d priority %d fl:%x sz:%d addr:%llx g:%d idx:%d\n",
>> + __func__, server, priority, kvm_eq.flags,
>> + kvm_eq.qsize, kvm_eq.qpage, kvm_eq.qtoggle, kvm_eq.qindex);
>> +
>> + /*
>> + * We can not tune the EQ configuration from user space. All
>> + * is done in OPAL.
>> + */
>> + if (kvm_eq.flags != 0) {
>> + pr_err("invalid flags %d\n", kvm_eq.flags);
>> + return -EINVAL;
>> + }
>> +
>> + rc = xive_native_validate_queue_size(kvm_eq.qsize);
>> + if (rc) {
>> + pr_err("invalid queue size %d\n", kvm_eq.qsize);
>> + return rc;
>> + }
>> +
>> + /* reset queue and disable queueing */
>> + if (!kvm_eq.qsize) {
>> + q->guest_qpage = 0;
>> + q->guest_qsize = 0;
>> +
>> + rc = xive_native_configure_queue(xc->vp_id, q, priority,
>> + NULL, 0, true);
>> + if (rc) {
>> + pr_err("Failed to reset queue %d for VCPU %d: %d\n",
>> + priority, xc->server_num, rc);
>> + return rc;
>> + }
>> +
>> + if (q->qpage) {
>> + put_page(virt_to_page(q->qpage));
>> + q->qpage = NULL;
>> + }
>> +
>> + return 0;
>> + }
>> +
>> +
>> + page = gfn_to_page(kvm, gpa_to_gfn(kvm_eq.qpage));
>> + if (is_error_page(page)) {
>> + pr_warn("Couldn't get guest page for %llx!\n", kvm_eq.qpage);
>> + return -EINVAL;
>> + }
>
> Yeah.. for the case of a 4kiB page host (these days weird, but not
> actually prohibited, AFAIK) you need to check that the qsize selected
> actually fits within the page.
Ah yes. sure.
>> + qaddr = page_to_virt(page) + (kvm_eq.qpage & ~PAGE_MASK);
>> +
>> + /* Backup queue page guest address for migration */
>
> Hm.. KVM itself shouldn't generally need to know about migration.
> IIUC these values won't change from what qemu set them to be, so it
> should be able to store and migrate them without have to get them back
> from the kernel.
Euh. You are completely right. I don't know why I kept those around.
>> + q->guest_qpage = kvm_eq.qpage;
>> + q->guest_qsize = kvm_eq.qsize;
>> +
>> + rc = xive_native_configure_queue(xc->vp_id, q, priority,
>> + (__be32 *) qaddr, kvm_eq.qsize, true);
>> + if (rc) {
>> + pr_err("Failed to configure queue %d for VCPU %d: %d\n",
>> + priority, xc->server_num, rc);
>> + put_page(page);
>> + return rc;
>> + }
>> +
>> + /*
>> + * Only restore the queue state when needed. When doing the
>> + * H_INT_SET_SOURCE_CONFIG hcall, it should not.
>> + */
>> + if (kvm_eq.qtoggle != 0 || kvm_eq.qindex != 0) {
>> + rc = xive_native_set_queue_state(xc->vp_id, priority,
>> + kvm_eq.qtoggle,
>> + kvm_eq.qindex);
>> + if (rc)
>> + goto error;
>> + }
>> +
>> + rc = kvmppc_xive_attach_escalation(vcpu, priority,
>> + xive->single_escalation);
>> +error:
>> + if (rc)
>> + kvmppc_xive_native_cleanup_queue(vcpu, priority);
>> + return rc;
>> +}
>> +
>> +static int kvmppc_xive_native_get_queue_config(struct kvmppc_xive *xive,
>> + long eq_idx, u64 addr)
>> +{
>> + struct kvm *kvm = xive->kvm;
>> + struct kvm_vcpu *vcpu;
>> + struct kvmppc_xive_vcpu *xc;
>> + struct xive_q *q;
>> + void __user *ubufp = (u64 __user *) addr;
>> + u32 server;
>> + u8 priority;
>> + struct kvm_ppc_xive_eq kvm_eq;
>> + u64 qpage;
>> + u64 qsize;
>> + u64 qeoi_page;
>> + u32 escalate_irq;
>> + u64 qflags;
>> + int rc;
>> +
>> + /*
>> + * Demangle priority/server tuple from the EQ identifier
>> + */
>> + priority = (eq_idx & KVM_XIVE_EQ_PRIORITY_MASK) >>
>> + KVM_XIVE_EQ_PRIORITY_SHIFT;
>> + server = (eq_idx & KVM_XIVE_EQ_SERVER_MASK) >>
>> + KVM_XIVE_EQ_SERVER_SHIFT;
>> +
>> + vcpu = kvmppc_xive_find_server(kvm, server);
>> + if (!vcpu) {
>> + pr_err("Can't find server %d\n", server);
>> + return -ENOENT;
>> + }
>> + xc = vcpu->arch.xive_vcpu;
>> +
>> + if (priority != xive_prio_from_guest(priority)) {
>> + pr_err("invalid priority for queue %d for VCPU %d\n",
>> + priority, server);
>> + return -EINVAL;
>> + }
>> + q = &xc->queues[priority];
>> +
>> + memset(&kvm_eq, 0, sizeof(kvm_eq));
>> +
>> + if (!q->qpage)
>> + return 0;
>> +
>> + rc = xive_native_get_queue_info(xc->vp_id, priority, &qpage, &qsize,
>> + &qeoi_page, &escalate_irq, &qflags);
>> + if (rc)
>> + return rc;
>> +
>> + /*
>> + * Return some information on the EQ configuration in
>> + * OPAL. This is purely informative for now as we can't really
>> + * tune the EQ configuration from user space.
>> + */
>> + kvm_eq.flags = 0;
>> + if (qflags & OPAL_XIVE_EQ_ENABLED)
>> + kvm_eq.flags |= KVM_XIVE_EQ_FLAG_ENABLED;
>> + if (qflags & OPAL_XIVE_EQ_ALWAYS_NOTIFY)
>> + kvm_eq.flags |= KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY;
>> + if (qflags & OPAL_XIVE_EQ_ESCALATE)
>> + kvm_eq.flags |= KVM_XIVE_EQ_FLAG_ESCALATE;
>
> If there's not really anything it can do about it, does it make sense
> to even expose this info to userspace?
Hmm, good question.
- KVM_XIVE_EQ_FLAG_ENABLED
may be uselessly obvious.
- KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY
means we do not use the END ESBs to coalesce the events at the END
level. This flag is reflected by the XIVE_EQ_ALWAYS_NOTIFY option
in the sPAPR specs. We don't support the END ESBs but we might one
day.
- KVM_XIVE_EQ_FLAG_ESCALATE
means the EQ is an escalation. QEMU doesn't really care for now
but it's an important information I think.
I tried not to add too many of the END flags, only the relevant ones which
could have an impact in the future modeling.
I think KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY is important. I was setting it from
QEMU in the hcall but as OPAL does the same blindly I removed it in v3.
>> + kvm_eq.qsize = q->guest_qsize;
>> + kvm_eq.qpage = q->guest_qpage;
>
>> + rc = xive_native_get_queue_state(xc->vp_id, priority, &kvm_eq.qtoggle,
>> + &kvm_eq.qindex);
>> + if (rc)
>> + return rc;
>> +
>> + pr_devel("%s VCPU %d priority %d fl:%x sz:%d addr:%llx g:%d idx:%d\n",
>> + __func__, server, priority, kvm_eq.flags,
>> + kvm_eq.qsize, kvm_eq.qpage, kvm_eq.qtoggle, kvm_eq.qindex);
>> +
>> + if (copy_to_user(ubufp, &kvm_eq, sizeof(kvm_eq)))
>> + return -EFAULT;
>> +
>> + return 0;
>> +}
>> +
>> static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
>> struct kvm_device_attr *attr)
>> {
>> @@ -354,6 +574,9 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
>> case KVM_DEV_XIVE_GRP_SOURCE_CONFIG:
>> return kvmppc_xive_native_set_source_config(xive, attr->attr,
>> attr->addr);
>> + case KVM_DEV_XIVE_GRP_EQ_CONFIG:
>> + return kvmppc_xive_native_set_queue_config(xive, attr->attr,
>> + attr->addr);
>> }
>> return -ENXIO;
>> }
>> @@ -361,6 +584,13 @@ 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_EQ_CONFIG:
>> + return kvmppc_xive_native_get_queue_config(xive, attr->attr,
>> + attr->addr);
>> + }
>> return -ENXIO;
>> }
>>
>> @@ -376,6 +606,8 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,
>> attr->attr < KVMPPC_XIVE_NR_IRQS)
>> return 0;
>> break;
>> + case KVM_DEV_XIVE_GRP_EQ_CONFIG:
>> + return 0;
>> }
>> return -ENXIO;
>> }
>> diff --git a/Documentation/virtual/kvm/devices/xive.txt b/Documentation/virtual/kvm/devices/xive.txt
>> index 33c64b2cdbe8..a4de64f6e79c 100644
>> --- a/Documentation/virtual/kvm/devices/xive.txt
>> +++ b/Documentation/virtual/kvm/devices/xive.txt
>> @@ -53,3 +53,34 @@ the legacy interrupt mode, referred as XICS (POWER7/8).
>> -ENXIO: CPU event queues not configured or configuration of the
>> underlying HW interrupt failed
>> -EBUSY: No CPU available to serve interrupt
>> +
>> + 4. KVM_DEV_XIVE_GRP_EQ_CONFIG (read-write)
>> + Configures an event queue of a CPU
>> + Attributes:
>> + EQ descriptor identifier (64-bit)
>> + The EQ descriptor identifier is a tuple (server, priority) :
>> + bits: | 63 .... 32 | 31 .. 3 | 2 .. 0
>> + values: | unused | server | priority
>> + The kvm_device_attr.addr points to :
>> + struct kvm_ppc_xive_eq {
>> + __u32 flags;
>> + __u32 qsize;
>> + __u64 qpage;
>> + __u32 qtoggle;
>> + __u32 qindex;
>> + __u8 pad[40];
>> + };
>> + - flags: queue flags
>> + - qsize: queue size (power of 2)
>> + - qpage: real address of queue
>> + - qtoggle: current queue toggle bit
>> + - qindex: current queue index
>> + - pad: reserved for future use
>> + Errors:
>> + -ENOENT: Invalid CPU number
>> + -EINVAL: Invalid priority
>> + -EINVAL: Invalid flags
>> + -EINVAL: Invalid queue size
>> + -EINVAL: Invalid queue address
>> + -EFAULT: Invalid user pointer for attr->addr.
>> + -EIO: Configuration of the underlying HW failed
>
^ permalink raw reply
* Re: [PATCH v2 2/6] ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling
From: Dmitry V. Levin @ 2019-03-18 14:41 UTC (permalink / raw)
To: Sudeep Holla
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Oleg Nesterov, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, Bin Lu,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20190318104925.16600-3-sudeep.holla@arm.com>
[-- Attachment #1: Type: text/plain, Size: 601 bytes --]
On Mon, Mar 18, 2019 at 10:49:21AM +0000, Sudeep Holla wrote:
> Currently each architecture handles PTRACE_SYSEMU in very similar way.
> It's completely arch independent and can be handled in the code helping
> to consolidate PTRACE_SYSEMU handling.
>
> Let's introduce a hook 'ptrace_syscall_enter' that arch specific syscall
> entry code can call.
Sorry if I'm late for the party, but the new name looks confusing.
If all it does is related to TIF_SYSCALL_EMU, why does it have a generic
name 'ptrace_syscall_enter' without any hint of being specific to
TIF_SYSCALL_EMU?
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/6] ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling
From: Dmitry V. Levin @ 2019-03-18 14:31 UTC (permalink / raw)
To: Sudeep Holla
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Oleg Nesterov, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, Bin Lu,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20190318104925.16600-3-sudeep.holla@arm.com>
[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]
On Mon, Mar 18, 2019 at 10:49:21AM +0000, Sudeep Holla wrote:
> Currently each architecture handles PTRACE_SYSEMU in very similar way.
> It's completely arch independent and can be handled in the code helping
> to consolidate PTRACE_SYSEMU handling.
>
> Let's introduce a hook 'ptrace_syscall_enter' that arch specific syscall
> entry code can call.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> include/linux/ptrace.h | 1 +
> kernel/ptrace.c | 22 ++++++++++++++++++++++
> 2 files changed, 23 insertions(+)
>
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index edb9b040c94c..e30f51e3363e 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -407,6 +407,7 @@ static inline void user_single_step_report(struct pt_regs *regs)
> #define current_user_stack_pointer() user_stack_pointer(current_pt_regs())
> #endif
>
> +extern long ptrace_syscall_enter(struct pt_regs *regs);
> extern int task_current_syscall(struct task_struct *target, long *callno,
> unsigned long args[6], unsigned int maxargs,
> unsigned long *sp, unsigned long *pc);
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 4fa3b7f4c3c7..c9c505c483df 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -29,6 +29,7 @@
> #include <linux/hw_breakpoint.h>
> #include <linux/cn_proc.h>
> #include <linux/compat.h>
> +#include <linux/tracehook.h>
>
> /*
> * Access another process' address space via ptrace.
> @@ -557,6 +558,27 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)
> return 0;
> }
>
> +/*
> + * Hook to check and report for PTRACE_SYSEMU, can be called from arch
> + * arch syscall entry code
> + */
> +long ptrace_syscall_enter(struct pt_regs *regs)
> +{
> +#ifdef TIF_SYSCALL_EMU
> + if (test_thread_flag(TIF_SYSCALL_EMU)) {
> + if (tracehook_report_syscall_entry(regs))
> + /*
> + * We can ignore the return code here as we need
> + * return -1 always for syscall emulation irrespective
> + * of whether the tracehook report fails or succeed.
> + */
> + ;
This is problematic as it causes build errors with -Werror=empty-body,
see https://lore.kernel.org/lkml/20181218205305.26647-1-malat@debian.org/
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU
From: Dmitry V. Levin @ 2019-03-18 14:26 UTC (permalink / raw)
To: Sudeep Holla
Cc: Haibo Xu, Steve Capper, Catalin Marinas, jdike, x86, Will Deacon,
linux-kernel, Oleg Nesterov, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner, Bin Lu,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20190318104925.16600-5-sudeep.holla@arm.com>
[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]
On Mon, Mar 18, 2019 at 10:49:23AM +0000, Sudeep Holla wrote:
> Now that we have a new hook ptrace_syscall_enter that can be called from
> syscall entry code and it handles PTRACE_SYSEMU in generic code, we
> can do some cleanup using the same in do_syscall_trace_enter.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> arch/powerpc/kernel/ptrace.c | 48 ++++++++++++++++--------------------
> 1 file changed, 21 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 2e2183b800a8..05579a5dcb12 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
>
> user_exit();
>
> - flags = READ_ONCE(current_thread_info()->flags) &
> - (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> -
> - if (flags) {
> - int rc = tracehook_report_syscall_entry(regs);
> + if (unlikely(ptrace_syscall_enter(regs))) {
> + /*
> + * A nonzero return code from tracehook_report_syscall_entry()
> + * tells us to prevent the syscall execution, but we are not
> + * going to execute it anyway.
> + *
> + * Returning -1 will skip the syscall execution. We want to
> + * avoid clobbering any registers, so we don't goto the skip
> + * label below.
> + */
> + return -1;
> + }
This comment is out of sync with the changed code.
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [RFC v3] sched/topology: fix kernel crash when a CPU is hotplugged in a memoryless node
From: Peter Zijlstra @ 2019-03-18 11:26 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Laurent Vivier, linux-kernel, Michael Bringmann, Ingo Molnar,
Suravee Suthikulpanit, Nathan Fontenot, Borislav Petkov,
linuxppc-dev, David Gibson
In-Reply-To: <20190318104730.GA4450@linux.vnet.ibm.com>
On Mon, Mar 18, 2019 at 04:17:30PM +0530, Srikar Dronamraju wrote:
> > > node 0 (because firmware doesn't provide the distance information for
> > > memoryless/cpuless nodes):
> > >
> > > node 0 1 2 3
> > > 0: 10 40 10 10
> > > 1: 40 10 40 40
> > > 2: 10 40 10 10
> > > 3: 10 40 10 10
> >
> > *groan*... what does it do for things like percpu memory? ISTR the
> > per-cpu chunks are all allocated early too. Having them all use memory
> > out of node-0 would seem sub-optimal.
>
> In the specific failing case, there is only one node with memory; all other
> nodes are cpu only nodes.
>
> However in the generic case since its just a cpu hotplug ops, the memory
> allocated for per-cpu chunks allocated early would remain.
What do you do in the case where there's multiple nodes with memory, but
only one with CPUs on?
Do you then still allocate the per-cpu memory for the CPUs that will
appear on that second node on node0?
> > > We should have:
> > >
> > > node 0 1 2 3
> > > 0: 10 40 40 40
> > > 1: 40 10 40 40
> > > 2: 40 40 10 40
> > > 3: 40 40 40 10
> >
> > Can it happen that it introduces a new distance in the table? One that
> > hasn't been seen before? This example only has 10 and 40, but suppose
> > the new node lands at distance 20 (or 80); can such a thing happen?
> >
> > If not; why not?
>
> Yes distances can be 20, 40 or 80. There is nothing that makes the node
> distance to be 40 always.
This,
> > So you're relying on sched_domain_numa_masks_set/clear() to fix this up,
> > but that in turn relies on the sched_domain_numa_levels thing to stay
> > accurate.
> >
> > This all seems very fragile and unfortunate.
> >
>
> Any reasons why this is fragile?
breaks that patch. The code assumes all the numa distances are known at
boot. If you add distances later, it comes unstuck.
It's not like you're actually changing the interconnects around at
runtime. Node topology really should be known at boot time.
What I _think_ the x86 BIOS does is, for each empty socket, iterate as
many logical CPUs (non-present) as it finds on Socket-0 (or whatever
socket is the boot socket).
Those non-present CPUs are assigned to their respective nodes. And
if/when a physical CPU is placed on the socket and the CPUs onlined, it
all 'works' (see ACPI SRAT).
I'm not entirely sure what happens on x86 when it boots with say a
10-core part and you then fill an empty socket with a 20-core part, I
suspect we simply will not use more than 10, we'll not have space
reserved in the Linux cpumasks for them anyway.
^ permalink raw reply
* Re: [RFC v3] sched/topology: fix kernel crash when a CPU is hotplugged in a memoryless node
From: Srikar Dronamraju @ 2019-03-18 11:06 UTC (permalink / raw)
To: Laurent Vivier
Cc: Peter Zijlstra, linux-kernel, Michael Bringmann, Ingo Molnar,
Suravee Suthikulpanit, Nathan Fontenot, Borislav Petkov,
linuxppc-dev, David Gibson
In-Reply-To: <77b142fe-0886-1510-28bd-d432ea2c796a@redhat.com>
* Laurent Vivier <lvivier@redhat.com> [2019-03-15 12:12:45]:
>
> Another way to avoid the nodes overlapping for the offline nodes at
> startup is to ensure the default values don't define a distance that
> merge all offline nodes into node 0.
>
> A powerpc specific patch can workaround the kernel crash by doing this:
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 87f0dd0..3ba29bb 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -623,6 +623,7 @@ static int __init parse_numa_properties(void)
> struct device_node *memory;
> int default_nid = 0;
> unsigned long i;
> + int nid, dist;
>
> if (numa_enabled == 0) {
> printk(KERN_WARNING "NUMA disabled by user\n");
> @@ -636,6 +637,10 @@ static int __init parse_numa_properties(void)
>
> dbg("NUMA associativity depth for CPU/Memory: %d\n",
> min_common_depth);
>
> + for (nid = 0; nid < MAX_NUMNODES; nid ++)
> + for (dist = 0; dist < MAX_DISTANCE_REF_POINTS; dist++)
> + distance_lookup_table[nid][dist] = nid;
> +
The only reason, this would have worked in the specific case, is because we
are overriding the distance_lookup_table with a unique distance.
So node_distance for any other node other than itself will return max
distance which is 40 in this case. (since distance_ref_points_depth is 2)
I am not sure if this will work if the node distance between the two nodes
happens to be 20.
> /*
> * Even though we connect cpus to numa domains later in SMP
> * init, we need to know the node ids now. This is because
>
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* [PATCH v2 6/6] arm64: ptrace: add support for syscall emulation
From: Sudeep Holla @ 2019-03-18 10:49 UTC (permalink / raw)
To: x86, linux-arm-kernel, linux-kernel, linuxppc-dev
Cc: Haibo Xu, Steve Capper, jdike, Sudeep Holla, Will Deacon,
Oleg Nesterov, Bin Lu, Richard Weinberger, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, Catalin Marinas, Thomas Gleixner
In-Reply-To: <20190318104925.16600-1-sudeep.holla@arm.com>
Add PTRACE_SYSEMU and PTRACE_SYSEMU_SINGLESTEP support on arm64.
We can just make sure of the generic ptrace_syscall_enter hook to
support PTRACE_SYSEMU. We don't need any special handling for
PTRACE_SYSEMU_SINGLESTEP.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
arch/arm64/include/asm/thread_info.h | 5 ++++-
arch/arm64/kernel/ptrace.c | 3 +++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index eb3ef73e07cf..c285d1ce7186 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -75,6 +75,7 @@ void arch_release_task_struct(struct task_struct *tsk);
* TIF_SYSCALL_TRACE - syscall trace active
* TIF_SYSCALL_TRACEPOINT - syscall tracepoint for ftrace
* TIF_SYSCALL_AUDIT - syscall auditing
+ * TIF_SYSCALL_EMU - syscall emulation active
* TIF_SECOMP - syscall secure computing
* TIF_SIGPENDING - signal pending
* TIF_NEED_RESCHED - rescheduling necessary
@@ -91,6 +92,7 @@ void arch_release_task_struct(struct task_struct *tsk);
#define TIF_SYSCALL_AUDIT 9
#define TIF_SYSCALL_TRACEPOINT 10
#define TIF_SECCOMP 11
+#define TIF_SYSCALL_EMU 12
#define TIF_MEMDIE 18 /* is terminating due to OOM killer */
#define TIF_FREEZE 19
#define TIF_RESTORE_SIGMASK 20
@@ -109,6 +111,7 @@ void arch_release_task_struct(struct task_struct *tsk);
#define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT)
#define _TIF_SYSCALL_TRACEPOINT (1 << TIF_SYSCALL_TRACEPOINT)
#define _TIF_SECCOMP (1 << TIF_SECCOMP)
+#define _TIF_SYSCALL_EMU (1 << TIF_SYSCALL_EMU)
#define _TIF_UPROBE (1 << TIF_UPROBE)
#define _TIF_FSCHECK (1 << TIF_FSCHECK)
#define _TIF_32BIT (1 << TIF_32BIT)
@@ -120,7 +123,7 @@ void arch_release_task_struct(struct task_struct *tsk);
#define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
_TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \
- _TIF_NOHZ)
+ _TIF_NOHZ | _TIF_SYSCALL_EMU)
#define INIT_THREAD_INFO(tsk) \
{ \
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index b82e0a9b3da3..cf29275cd4d9 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1819,6 +1819,9 @@ static void tracehook_report_syscall(struct pt_regs *regs,
int syscall_trace_enter(struct pt_regs *regs)
{
+ if (unlikely(ptrace_syscall_enter(regs)))
+ return -1;
+
if (test_thread_flag(TIF_SYSCALL_TRACE))
tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
--
2.17.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