From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Subject: Re: [PATCH 06/19] KVM: PPC: Book3S HV: add a GET_ESB_FD control to the XIVE native device Date: Wed, 23 Jan 2019 17:48:17 +0100 Message-ID: <0659e143-d43d-75e8-4dbb-c8dea9a08369@kaod.org> References: <20190107184331.8429-1-clg@kaod.org> <20190107184331.8429-7-clg@kaod.org> <20190122050952.GD15124@blackberry> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, David Gibson To: Paul Mackerras Return-path: In-Reply-To: <20190122050952.GD15124@blackberry> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Id: kvm.vger.kernel.org On 1/22/19 6:09 AM, Paul Mackerras wrote: > On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote: >> This will let the guest create a memory mapping to expose the ESB MMIO >> regions used to control the interrupt sources, to trigger events, to >> EOI or to turn off the sources. >> >> Signed-off-by: Cédric Le Goater >> --- >> arch/powerpc/include/uapi/asm/kvm.h | 4 ++ >> arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++ >> 2 files changed, 101 insertions(+) >> >> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h >> index 8c876c166ef2..6bb61ba141c2 100644 >> --- a/arch/powerpc/include/uapi/asm/kvm.h >> +++ b/arch/powerpc/include/uapi/asm/kvm.h >> @@ -675,4 +675,8 @@ struct kvm_ppc_cpu_char { >> #define KVM_XICS_PRESENTED (1ULL << 43) >> #define KVM_XICS_QUEUED (1ULL << 44) >> >> +/* POWER9 XIVE Native Interrupt Controller */ >> +#define KVM_DEV_XIVE_GRP_CTRL 1 >> +#define KVM_DEV_XIVE_GET_ESB_FD 1 >> + >> #endif /* __LINUX_KVM_POWERPC_H */ >> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c >> index 115143e76c45..e20081f0c8d4 100644 >> --- a/arch/powerpc/kvm/book3s_xive_native.c >> +++ b/arch/powerpc/kvm/book3s_xive_native.c >> @@ -153,6 +153,85 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev, >> return rc; >> } >> >> +static int xive_native_esb_fault(struct vm_fault *vmf) >> +{ >> + struct vm_area_struct *vma = vmf->vma; >> + struct kvmppc_xive *xive = vma->vm_file->private_data; >> + struct kvmppc_xive_src_block *sb; >> + struct kvmppc_xive_irq_state *state; >> + struct xive_irq_data *xd; >> + u32 hw_num; >> + u16 src; >> + u64 page; >> + unsigned long irq; >> + >> + /* >> + * Linux/KVM uses a two pages ESB setting, one for trigger and >> + * one for EOI >> + */ >> + irq = vmf->pgoff / 2; >> + >> + sb = kvmppc_xive_find_source(xive, irq, &src); >> + if (!sb) { >> + pr_err("%s: source %lx not found !\n", __func__, irq); > > In general it's a bad idea to have a printk that userspace can trigger > at will without any rate-limiting. Is there a real reason why this > printk is needed (and can't be pr_devel)? yes. It should. The SIGBUS is enough to know what's going on. > >> + return VM_FAULT_SIGBUS; >> + } >> + >> + state = &sb->irq_state[src]; >> + kvmppc_xive_select_irq(state, &hw_num, &xd); >> + >> + arch_spin_lock(&sb->lock); >> + >> + /* >> + * first/even page is for trigger >> + * second/odd page is for EOI and management. >> + */ >> + page = vmf->pgoff % 2 ? xd->eoi_page : xd->trig_page; >> + arch_spin_unlock(&sb->lock); >> + >> + if (!page) { >> + pr_err("%s: acessing invalid ESB page for source %lx !\n", >> + __func__, irq); > > Does this represent a exceptional condition that userspace can't just > trigger at will (i.e. it implies the presence of a kernel bug)? If > not then the same comment as above applies. Not having an ESB page (trigger or EOI) implies that the xive_irq_data for the source is bogus. This probably deserves a WARN(). Thanks, C. From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Date: Wed, 23 Jan 2019 16:48:17 +0000 Subject: Re: [PATCH 06/19] KVM: PPC: Book3S HV: add a GET_ESB_FD control to the XIVE native device Message-Id: <0659e143-d43d-75e8-4dbb-c8dea9a08369@kaod.org> List-Id: References: <20190107184331.8429-1-clg@kaod.org> <20190107184331.8429-7-clg@kaod.org> <20190122050952.GD15124@blackberry> In-Reply-To: <20190122050952.GD15124@blackberry> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: Paul Mackerras Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, David Gibson On 1/22/19 6:09 AM, Paul Mackerras wrote: > On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote: >> This will let the guest create a memory mapping to expose the ESB MMIO >> regions used to control the interrupt sources, to trigger events, to >> EOI or to turn off the sources. >> >> Signed-off-by: Cédric Le Goater >> --- >> arch/powerpc/include/uapi/asm/kvm.h | 4 ++ >> arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++ >> 2 files changed, 101 insertions(+) >> >> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h >> index 8c876c166ef2..6bb61ba141c2 100644 >> --- a/arch/powerpc/include/uapi/asm/kvm.h >> +++ b/arch/powerpc/include/uapi/asm/kvm.h >> @@ -675,4 +675,8 @@ struct kvm_ppc_cpu_char { >> #define KVM_XICS_PRESENTED (1ULL << 43) >> #define KVM_XICS_QUEUED (1ULL << 44) >> >> +/* POWER9 XIVE Native Interrupt Controller */ >> +#define KVM_DEV_XIVE_GRP_CTRL 1 >> +#define KVM_DEV_XIVE_GET_ESB_FD 1 >> + >> #endif /* __LINUX_KVM_POWERPC_H */ >> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c >> index 115143e76c45..e20081f0c8d4 100644 >> --- a/arch/powerpc/kvm/book3s_xive_native.c >> +++ b/arch/powerpc/kvm/book3s_xive_native.c >> @@ -153,6 +153,85 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev, >> return rc; >> } >> >> +static int xive_native_esb_fault(struct vm_fault *vmf) >> +{ >> + struct vm_area_struct *vma = vmf->vma; >> + struct kvmppc_xive *xive = vma->vm_file->private_data; >> + struct kvmppc_xive_src_block *sb; >> + struct kvmppc_xive_irq_state *state; >> + struct xive_irq_data *xd; >> + u32 hw_num; >> + u16 src; >> + u64 page; >> + unsigned long irq; >> + >> + /* >> + * Linux/KVM uses a two pages ESB setting, one for trigger and >> + * one for EOI >> + */ >> + irq = vmf->pgoff / 2; >> + >> + sb = kvmppc_xive_find_source(xive, irq, &src); >> + if (!sb) { >> + pr_err("%s: source %lx not found !\n", __func__, irq); > > In general it's a bad idea to have a printk that userspace can trigger > at will without any rate-limiting. Is there a real reason why this > printk is needed (and can't be pr_devel)? yes. It should. The SIGBUS is enough to know what's going on. > >> + return VM_FAULT_SIGBUS; >> + } >> + >> + state = &sb->irq_state[src]; >> + kvmppc_xive_select_irq(state, &hw_num, &xd); >> + >> + arch_spin_lock(&sb->lock); >> + >> + /* >> + * first/even page is for trigger >> + * second/odd page is for EOI and management. >> + */ >> + page = vmf->pgoff % 2 ? xd->eoi_page : xd->trig_page; >> + arch_spin_unlock(&sb->lock); >> + >> + if (!page) { >> + pr_err("%s: acessing invalid ESB page for source %lx !\n", >> + __func__, irq); > > Does this represent a exceptional condition that userspace can't just > trigger at will (i.e. it implies the presence of a kernel bug)? If > not then the same comment as above applies. Not having an ESB page (trigger or EOI) implies that the xive_irq_data for the source is bogus. This probably deserves a WARN(). Thanks, C.