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 <c...@kaod.org>
>> ---
>> arch/powerpc/include/uapi/asm/kvm.h | 4 ++
>> arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
>> 2 files changed, 101 insertions(+)
>>
>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h
>> b/arch/powerpc/include/uapi/asm/kvm.h
>> index 8c876c166ef2..6bb61ba141c2 100644
>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>> @@ -675,4 +675,8 @@ struct kvm_ppc_cpu_char {
>> #define KVM_XICS_PRESENTED (1ULL << 43)
>> #define KVM_XICS_QUEUED (1ULL << 44)
>>
>> +/* POWER9 XIVE Native Interrupt Controller */
>> +#define KVM_DEV_XIVE_GRP_CTRL 1
>> +#define KVM_DEV_XIVE_GET_ESB_FD 1
>> +
>> #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.
