Re: [PATCH V2] cxl: Export library to support IBM XSL

[christophe lombard <clombard@linux.vnet.ibm.com>]

Le 15/06/2017 à 14:36, Frederic Barrat a écrit :
> Salut Christophe,
>
> A few comments below, nothing major...
>
> Le 14/06/2017 à 15:29, Christophe Lombard a écrit :
>> This patch exports a in-kernel 'library' API which can be called by
>> other drivers to help interacting with an IBM XSL on a POWER9 system.
>>
>> The XSL (Translation Service Layer) is a stripped down version of the
>> PSL (Power Service Layer) used in some cards such as the Mellanox CX5.
>> Like the PSL, it implements the CAIA architecture, but has a number
>> of differences, mostly in it's implementation dependent registers.
>>
>> The XSL also uses a special DMA cxl mode, which uses a slightly
>> different init sequence for the CAPP and PHB.
>>
>> Changelog[v2]
>>   - Rebase to latest upstream.
>>   - Return -EFAULT in case of NULL pointer in cxllib_handle_fault().
>>   - Reverse parameters when copro_handle_mm_fault() is called.
>>
>
>
> The change log shouldn't be part of the commit message, but below the 
> next "---"
>
>

sure.

>> +++ b/drivers/misc/cxl/cxllib.c
>> @@ -0,0 +1,241 @@
>> +/*
>> + * Copyright 2016 IBM Corp.
>
>
> Year need to be updated
>
>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + */
>> +
>> +#include <linux/hugetlb.h>
>> +#include <linux/sched/mm.h>
>> +#include "cxl.h"
>> +#include <misc/cxllib.h>
>> +#include <asm/pnv-pci.h>
>
> Ordering of the #include is messy:
> #include <linux/hugetlb.h>
> #include <linux/sched/mm.h>
> #include <asm/pnv-pci.h>
> #include <misc/cxllib.h>
> #include "cxl.h"
>
>
>
>> +int cxllib_set_device_dma(struct pci_dev *dev, unsigned long flags)
>> +{
>> +    int rc;
>> +
>> +    if (flags)
>> +        return -EINVAL;
>> +
>> +    rc = dma_set_mask(&dev->dev, DMA_BIT_MASK(64));
>> +    return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(cxllib_set_device_dma);
>
>
> A comment in cxllib_set_device_dma() would help:
>     /*
>      * When switching the PHB to capi mode, the TVT#1 entry for
>      * the Partitionable Endpoint is set in bypass mode, like
>      * in PCI mode.
>      * Configure the device dma to use TVT#1, which is done
>      * by calling dma_set_mask() with a mask large enough.
>      */
>
>
>
>> +
>> +int cxllib_get_PE_attributes(struct task_struct *task,
>> +         unsigned long translation_mode, struct cxllib_pe_attributes 
>> *attr)
>> +{
>> +    struct mm_struct *mm = NULL;
>> +
>> +    if (translation_mode != CXL_TRANSLATED_MODE &&
>> +        translation_mode != CXL_REAL_MODE)
>> +        return -EINVAL;
>> +
>> +    attr->sr = cxl_calculate_sr(false /* master */,
>> +                task == NULL /* kernel ctx */,
>> +                translation_mode == CXL_REAL_MODE,
>> +                true /* p9 */);
>> +    attr->lpid = mfspr(SPRN_LPID);
>> +    if (task) {
>> +        mm = get_task_mm(task);
>> +        if (mm == NULL)
>> +            return -EINVAL;
>> +        /*
>> +         * Caller is keeping a reference on mm_users for as long
>> +         * as XSL uses the memory context
>> +         */
>> +        attr->pid = mm->context.id;
>> +        mmput(mm);
>> +    } else {
>> +        attr->pid = 0;
>> +    }
>> +    attr->tid = 0;
>
>
> We'll need to remember to patch that function as welll (attr->tid) 
> when we add support for as_notify (even though Mellanox is not 
> expected to use as_notify in capi mode, just pci I believe)
>
>

yep.


>
>> +int cxllib_handle_fault(struct mm_struct *mm, u64 addr, u64 size, 
>> u64 flags)
>> +{
>> +    int rc;
>> +    u64 dar;
>> +    struct vm_area_struct *vma = NULL;
>> +    unsigned long page_size;
>> +
>> +    if (mm == NULL)
>> +        return -EFAULT;
>> +
>> +    down_read(&mm->mmap_sem);
>> +
>> +    for (dar = addr; dar < addr + size; dar += page_size) {
>> +        if (!vma || dar < vma->vm_start || dar > vma->vm_end) {
>> +            vma = find_vma(mm, addr);
>> +            if (!vma) {
>> +                pr_err("Can't find vma for addr %016llx\n", addr);
>> +                rc = -EFAULT;
>> +                goto out;
>> +            }
>> +            /* get the size of the pages allocated */
>> +            page_size = vma_kernel_pagesize(vma);
>> +        }
>> +
>> +        rc = cxl_handle_page_fault(true, mm, flags, dar);
>
>
> Why do we pass "true" for kernel parameter?
> Actually do we even need a kernel input parameter for 
> cxl_handle_page_fault() ? It seems that we can infer it based on mm. 
> If NULL, then we are in kernel space.
>
>

you are right. Previously, the test was based on the field ctx->kernel. 
This explains the kernel parameter.

>> diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
>> index c79e39b..9db63f3 100644
>> --- a/drivers/misc/cxl/fault.c
>> +++ b/drivers/misc/cxl/fault.c
>> @@ -132,18 +132,16 @@ static int cxl_handle_segment_miss(struct 
>> cxl_context *ctx,
>>       return IRQ_HANDLED;
>>   }
>>
>> -static void cxl_handle_page_fault(struct cxl_context *ctx,
>> -                  struct mm_struct *mm, u64 dsisr, u64 dar)
>> +int cxl_handle_page_fault(bool kernel_context,
>> +              struct mm_struct *mm, u64 dsisr, u64 dar)
>>   {
>>       unsigned flt = 0;
>>       int result;
>>       unsigned long access, flags, inv_flags = 0;
>>
>> -    trace_cxl_pte_miss(ctx, dsisr, dar);
>> -
>>       if ((result = copro_handle_mm_fault(mm, dar, dsisr, &flt))) {
>>           pr_devel("copro_handle_mm_fault failed: %#x\n", result);
>> -        return cxl_ack_ae(ctx);
>> +        return result;
>>       }
>>
>>       if (!radix_enabled()) {
>> @@ -156,7 +154,7 @@ static void cxl_handle_page_fault(struct 
>> cxl_context *ctx,
>>               access |= _PAGE_WRITE;
>>
>>           access |= _PAGE_PRIVILEGED;
>> -        if ((!ctx->kernel) || (REGION_ID(dar) == USER_REGION_ID))
>> +        if (!kernel_context || (REGION_ID(dar) == USER_REGION_ID))
>>               access &= ~_PAGE_PRIVILEGED;
>>
>>           if (dsisr & DSISR_NOHPTE)
>> @@ -166,8 +164,7 @@ static void cxl_handle_page_fault(struct 
>> cxl_context *ctx,
>>           hash_page_mm(mm, dar, access, 0x300, inv_flags);
>>           local_irq_restore(flags);
>>       }
>> -    pr_devel("Page fault successfully handled for pe: %i!\n", ctx->pe);
>> -    cxl_ops->ack_irq(ctx, CXL_PSL_TFC_An_R, 0);
>> +    return 0;
>>   }
>>
>>   /*
>> @@ -261,9 +258,15 @@ void cxl_handle_fault(struct work_struct 
>> *fault_work)
>>
>>       if (cxl_is_segment_miss(ctx, dsisr))
>>           cxl_handle_segment_miss(ctx, mm, dar);
>> -    else if (cxl_is_page_fault(ctx, dsisr))
>> -        cxl_handle_page_fault(ctx, mm, dsisr, dar);
>> -    else
>> +    else if (cxl_is_page_fault(ctx, dsisr)) {
>> +        trace_cxl_pte_miss(ctx, dsisr, dar);
>> +        if (cxl_handle_page_fault(ctx->kernel, mm, dsisr, dar)) {
>> +            cxl_ack_ae(ctx);
>> +        } else {
>> +            pr_devel("Page fault successfully handled for pe: 
>> %i!\n", ctx->pe);
>> +            cxl_ops->ack_irq(ctx, CXL_PSL_TFC_An_R, 0);
>> +        }
>
>
> Could we have that code in a wrapper before calling 
> cxl_handle_page_fault()? It would keep the code cleaner and in line 
> with what we do for cxl_handle_segment_miss().
>

We can try to to that.


>
>
>> +++ b/include/misc/cxllib.h
>> @@ -0,0 +1,132 @@
>> +/*
>> + * Copyright 2016 IBM Corp.
>
>
> Year update.
>
>
>> +/*
>> + * Get the Process Element structure for the given thread
>> + *
>> + * Input:
>> + *    pid: points the struct pid for the given thread (i.e. linux pid)
>> + *    translation_mode: whether addresses should be translated
>> + */
>> +struct cxllib_pe_attributes {
>> +    u64 sr;
>> +    u32 lpid;
>> +    u32 tid;
>> +    u32 pid;
>> +};
>> +#define CXL_TRANSLATED_MODE 0
>> +#define CXL_REAL_MODE 1
>> +
>> +int cxllib_get_PE_attributes(struct task_struct *task,
>> +         unsigned long translation_mode, struct cxllib_pe_attributes 
>> *attr);
>
>
> Description in comment no longer matches reality.
> /*
>  * Get the Process Element structure for the given thread
>  *
>  * Input:
>  *    task: task_struct for the context of the translation
>  *    translation_mode: whether addresses should be translated
>  * Output:
>  *    attr: attributes to fill up the Process Element structure from CAIA
>  */
>
>
> Thanks,
>
>   Fred