* Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
From: Nicholas Piggin @ 2020-03-23 1:30 UTC (permalink / raw)
To: Haren Myneni; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584925071.9256.15311.camel@hbabu-laptop>
Haren Myneni's on March 23, 2020 10:57 am:
> On Mon, 2020-03-23 at 10:30 +1000, Nicholas Piggin wrote:
>> Haren Myneni's on March 19, 2020 4:13 pm:
>> >
>> > Kernel sets fault address and status in CRB for NX page fault on user
>> > space address after processing page fault. User space gets the signal
>> > and handles the fault mentioned in CRB by bringing the page in to
>> > memory and send NX request again.
>> >
>> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
>> > ---
>> > arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
>> > 1 file changed, 17 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
>> > index 9872f85..b233d1e 100644
>> > --- a/arch/powerpc/include/asm/icswx.h
>> > +++ b/arch/powerpc/include/asm/icswx.h
>>
>> "icswx" is not a thing anymore, after 6ff4d3e96652 ("powerpc: Remove old
>> unused icswx based coprocessor support"). I guess NX is reusing some
>> things from it, but it would be good to get rid of the cruft and re-name
>> this file and and relevant names.
>>
>> NX already uses this file, so I guesss that can happen after this series.
>
> But NX uses icswx on P8 and icswx.h has only NX specific macros right
> now.
Oh P8 in kernel is still using it? Ignore me then.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Alexey Kardashevskiy @ 2020-03-23 1:28 UTC (permalink / raw)
To: Christoph Hellwig, iommu
Cc: Greg Kroah-Hartman, Joerg Roedel, Robin Murphy, linux-kernel,
Aneesh Kumar K.V, linuxppc-dev, Lu Baolu
In-Reply-To: <20200320141640.366360-2-hch@lst.de>
On 21/03/2020 01:16, Christoph Hellwig wrote:
> Several IOMMU drivers have a bypass mode where they can use a direct
> mapping if the devices DMA mask is large enough. Add generic support
> to the core dma-mapping code to do that to switch those drivers to
> a common solution.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> include/linux/device.h | 6 ++++++
> include/linux/dma-mapping.h | 30 ++++++++++++++++++------------
> kernel/dma/mapping.c | 36 +++++++++++++++++++++++++++---------
> 3 files changed, 51 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 0cd7c647c16c..09be8bb2c4a6 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -525,6 +525,11 @@ struct dev_links_info {
> * sync_state() callback.
> * @dma_coherent: this particular device is dma coherent, even if the
> * architecture supports non-coherent devices.
> + * @dma_ops_bypass: If set to %true then the dma_ops are bypassed for the
> + * streaming DMA operations (->map_* / ->unmap_* / ->sync_*),
> + * and optionall (if the coherent mask is large enough) also
> + * for dma allocations. This flag is managed by the dma ops
> + * instance from ->dma_supported.
> *
> * At the lowest level, every device in a Linux system is represented by an
> * instance of struct device. The device structure contains the information
> @@ -625,6 +630,7 @@ struct device {
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> bool dma_coherent:1;
> #endif
> + bool dma_ops_bypass : 1;
> };
>
> static inline struct device *kobj_to_dev(struct kobject *kobj)
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 330ad58fbf4d..c3af0cf5e435 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -188,9 +188,15 @@ static inline int dma_mmap_from_global_coherent(struct vm_area_struct *vma,
> }
> #endif /* CONFIG_DMA_DECLARE_COHERENT */
>
> -static inline bool dma_is_direct(const struct dma_map_ops *ops)
> +/*
> + * Check if the devices uses a direct mapping for streaming DMA operations.
> + * This allows IOMMU drivers to set a bypass mode if the DMA mask is large
> + * enough.
> + */
> +static inline bool dma_map_direct(struct device *dev,
> + const struct dma_map_ops *ops)
> {
> - return likely(!ops);
> + return likely(!ops) || dev->dma_ops_bypass;
> }
>
> /*
> @@ -279,7 +285,7 @@ static inline dma_addr_t dma_map_page_attrs(struct device *dev,
> dma_addr_t addr;
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> addr = dma_direct_map_page(dev, page, offset, size, dir, attrs);
> else
> addr = ops->map_page(dev, page, offset, size, dir, attrs);
> @@ -294,7 +300,7 @@ static inline void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_unmap_page(dev, addr, size, dir, attrs);
> else if (ops->unmap_page)
> ops->unmap_page(dev, addr, size, dir, attrs);
> @@ -313,7 +319,7 @@ static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
> int ents;
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> ents = dma_direct_map_sg(dev, sg, nents, dir, attrs);
> else
> ents = ops->map_sg(dev, sg, nents, dir, attrs);
> @@ -331,7 +337,7 @@ static inline void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg
>
> BUG_ON(!valid_dma_direction(dir));
> debug_dma_unmap_sg(dev, sg, nents, dir);
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_unmap_sg(dev, sg, nents, dir, attrs);
> else if (ops->unmap_sg)
> ops->unmap_sg(dev, sg, nents, dir, attrs);
> @@ -352,7 +358,7 @@ static inline dma_addr_t dma_map_resource(struct device *dev,
> if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
> return DMA_MAPPING_ERROR;
>
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> addr = dma_direct_map_resource(dev, phys_addr, size, dir, attrs);
> else if (ops->map_resource)
> addr = ops->map_resource(dev, phys_addr, size, dir, attrs);
> @@ -368,7 +374,7 @@ static inline void dma_unmap_resource(struct device *dev, dma_addr_t addr,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (!dma_is_direct(ops) && ops->unmap_resource)
> + if (!dma_map_direct(dev, ops) && ops->unmap_resource)
> ops->unmap_resource(dev, addr, size, dir, attrs);
> debug_dma_unmap_resource(dev, addr, size, dir);
> }
> @@ -380,7 +386,7 @@ static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_sync_single_for_cpu(dev, addr, size, dir);
> else if (ops->sync_single_for_cpu)
> ops->sync_single_for_cpu(dev, addr, size, dir);
> @@ -394,7 +400,7 @@ static inline void dma_sync_single_for_device(struct device *dev,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_sync_single_for_device(dev, addr, size, dir);
> else if (ops->sync_single_for_device)
> ops->sync_single_for_device(dev, addr, size, dir);
> @@ -408,7 +414,7 @@ dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_sync_sg_for_cpu(dev, sg, nelems, dir);
> else if (ops->sync_sg_for_cpu)
> ops->sync_sg_for_cpu(dev, sg, nelems, dir);
> @@ -422,7 +428,7 @@ dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> BUG_ON(!valid_dma_direction(dir));
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> dma_direct_sync_sg_for_device(dev, sg, nelems, dir);
> else if (ops->sync_sg_for_device)
> ops->sync_sg_for_device(dev, sg, nelems, dir);
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 12ff766ec1fa..fdea45574345 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -105,6 +105,24 @@ void *dmam_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
> }
> EXPORT_SYMBOL(dmam_alloc_attrs);
>
> +static bool dma_alloc_direct(struct device *dev, const struct dma_map_ops *ops)
> +{
> + if (!ops)
> + return true;
> +
> + /*
> + * Allows IOMMU drivers to bypass dynamic translations if the DMA mask
> + * is large enough.
> + */
> + if (dev->dma_ops_bypass) {
> + if (min_not_zero(dev->coherent_dma_mask, dev->bus_dma_limit) >=
> + dma_direct_get_required_mask(dev))
> + return true;
> + }
Why not do this in dma_map_direct() as well?
Or simply have just one dma_map_direct()?
And one more general question - we need a way to use non-direct IOMMU
for RAM above certain limit.
Let's say we have a system with:
0 .. 0x1.0000.0000
0x100.0000.0000 .. 0x101.0000.0000
2x4G, each is 1TB aligned. And we can map directly only the first 4GB
(because of the maximum IOMMU table size) but not the other. And 1:1 on
that "pseries" is done with offset=0x0800.0000.0000.0000.
So we want to check every bus address against dev->bus_dma_limit, not
dev->coherent_dma_mask. In the example above I'd set bus_dma_limit to
0x0800.0001.0000.0000 and 1:1 mapping for the second 4GB would not be
tried. Does this sound reasonable? Thanks,
> +
> + return false;
> +}
> +
> /*
> * Create scatter-list for the already allocated DMA buffer.
> */
> @@ -138,7 +156,7 @@ int dma_get_sgtable_attrs(struct device *dev, struct sg_table *sgt,
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> return dma_direct_get_sgtable(dev, sgt, cpu_addr, dma_addr,
> size, attrs);
> if (!ops->get_sgtable)
> @@ -206,7 +224,7 @@ bool dma_can_mmap(struct device *dev)
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> return dma_direct_can_mmap(dev);
> return ops->mmap != NULL;
> }
> @@ -231,7 +249,7 @@ int dma_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> return dma_direct_mmap(dev, vma, cpu_addr, dma_addr, size,
> attrs);
> if (!ops->mmap)
> @@ -244,7 +262,7 @@ u64 dma_get_required_mask(struct device *dev)
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> return dma_direct_get_required_mask(dev);
> if (ops->get_required_mask)
> return ops->get_required_mask(dev);
> @@ -275,7 +293,7 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
> /* let the implementation decide on the zone to allocate from: */
> flag &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM);
>
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> cpu_addr = dma_direct_alloc(dev, size, dma_handle, flag, attrs);
> else if (ops->alloc)
> cpu_addr = ops->alloc(dev, size, dma_handle, flag, attrs);
> @@ -307,7 +325,7 @@ void dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
> return;
>
> debug_dma_free_coherent(dev, size, cpu_addr, dma_handle);
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> dma_direct_free(dev, size, cpu_addr, dma_handle, attrs);
> else if (ops->free)
> ops->free(dev, size, cpu_addr, dma_handle, attrs);
> @@ -318,7 +336,7 @@ int dma_supported(struct device *dev, u64 mask)
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - if (dma_is_direct(ops))
> + if (!ops)
> return dma_direct_supported(dev, mask);
> if (!ops->dma_supported)
> return 1;
> @@ -374,7 +392,7 @@ void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
>
> BUG_ON(!valid_dma_direction(dir));
>
> - if (dma_is_direct(ops))
> + if (dma_alloc_direct(dev, ops))
> arch_dma_cache_sync(dev, vaddr, size, dir);
> else if (ops->cache_sync)
> ops->cache_sync(dev, vaddr, size, dir);
> @@ -386,7 +404,7 @@ size_t dma_max_mapping_size(struct device *dev)
> const struct dma_map_ops *ops = get_dma_ops(dev);
> size_t size = SIZE_MAX;
>
> - if (dma_is_direct(ops))
> + if (dma_map_direct(dev, ops))
> size = dma_direct_max_mapping_size(dev);
> else if (ops && ops->max_mapping_size)
> size = ops->max_mapping_size(dev);
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address
From: Nicholas Piggin @ 2020-03-23 1:06 UTC (permalink / raw)
To: Haren Myneni, mpe; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584598473.9256.15248.camel@hbabu-laptop>
Haren Myneni's on March 19, 2020 4:14 pm:
>
> Alloc IRQ and get trigger port address for each VAS instance. Kernel
> register this IRQ per VAS instance and sets this port for each send
> window. NX interrupts the kernel when it sees page fault.
Again, should cc Cedric and Greg for XIVE / interrupt stuff. And
for patch 2/14.
The changelogs could use a bit of work. They're hard to read, and it can
be a bit hard to decipher "why".
Allocate a xive irq on each chip with a vas instance. The NX
coprocessor raises a host CPU interrupt via vas if it encounters a
page fault on an effective address. Subsequent patches register the
trigger port with the NX coprocessor, and create a vas fault handler
for this interrupt mapping.
Don't know if the technical details are correct, but something like that
in structure.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH V7 09/14] powerpc/vas: Update CSB and notify process for fault CRBs
From: Haren Myneni @ 2020-03-23 1:06 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584920780.45onmviqrl.astroid@bobo.none>
On Mon, 2020-03-23 at 10:06 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 18, 2020 5:27 am:
> > On Tue, 2020-03-17 at 16:28 +1100, Michael Ellerman wrote:
> >> Haren Myneni <haren@linux.ibm.com> writes:
> >> > For each fault CRB, update fault address in CRB (fault_storage_addr)
> >> > and translation error status in CSB so that user space can touch the
> >> > fault address and resend the request. If the user space passed invalid
> >> > CSB address send signal to process with SIGSEGV.
> >> >
> >> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> >> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> >> > ---
> >> > arch/powerpc/platforms/powernv/vas-fault.c | 114 +++++++++++++++++++++++++++++
> >> > 1 file changed, 114 insertions(+)
> >> >
> >> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> >> > index 1c6d5cc..751ce48 100644
> >> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
> >> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> >> > @@ -11,6 +11,7 @@
> >> > #include <linux/slab.h>
> >> > #include <linux/uaccess.h>
> >> > #include <linux/kthread.h>
> >> > +#include <linux/sched/signal.h>
> >> > #include <linux/mmu_context.h>
> >> > #include <asm/icswx.h>
> >> >
> >> > @@ -26,6 +27,118 @@
> >> > #define VAS_FAULT_WIN_FIFO_SIZE (4 << 20)
> >> >
> >> > /*
> >> > + * Update the CSB to indicate a translation error.
> >> > + *
> >> > + * If we are unable to update the CSB means copy_to_user failed due to
> >> > + * invalid csb_addr, send a signal to the process.
> >> > + *
> >> > + * Remaining settings in the CSB are based on wait_for_csb() of
> >> > + * NX-GZIP.
> >> > + */
> >> > +static void update_csb(struct vas_window *window,
> >> > + struct coprocessor_request_block *crb)
> >> > +{
> >> > + int rc;
> >> > + struct pid *pid;
> >> > + void __user *csb_addr;
> >> > + struct task_struct *tsk;
> >> > + struct kernel_siginfo info;
> >> > + struct coprocessor_status_block csb;
> >>
> >> csb is on the stack, and later copied to user, which is a risk for
> >> creating an infoleak.
> >>
> >> Also please use reverse Christmas tree layout for your variables.
> >>
> >> > +
> >> > + /*
> >> > + * NX user space windows can not be opened for task->mm=NULL
> >> > + * and faults will not be generated for kernel requests.
> >> > + */
> >> > + if (!window->mm || !window->user_win)
> >> > + return;
> >>
> >> If that's a should-never-happen condition then should it do a
> >> WARN_ON_ONCE() rather than silently returning?
> >
> > Will add WARN_ON
> >
> >>
> >> > + csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
> >> > +
> >> > + csb.cc = CSB_CC_TRANSLATION;
> >> > + csb.ce = CSB_CE_TERMINATION;
> >> > + csb.cs = 0;
> >> > + csb.count = 0;
> >> > +
> >> > + /*
> >> > + * NX operates and returns in BE format as defined CRB struct.
> >> > + * So return fault_storage_addr in BE as NX pastes in FIFO and
> >> > + * expects user space to convert to CPU format.
> >> > + */
> >> > + csb.address = crb->stamp.nx.fault_storage_addr;
> >> > + csb.flags = 0;
> >>
> >> I'm pretty sure this has initialised all the fields of csb.
> >>
> >> But, I'd still be much happier if you zeroed the whole struct to begin
> >> with, that way we know for sure we can't leak any uninitialised bytes to
> >> userspace. It's only 16 bytes so it shouldn't add any noticeable
> >> overhead.
> > Sure, will initialize csb
> >>
> >> > +
> >> > + pid = window->pid;
> >> > + tsk = get_pid_task(pid, PIDTYPE_PID);
> >> > + /*
> >> > + * Send window will be closed after processing all NX requests
> >> > + * and process exits after closing all windows. In multi-thread
> >> > + * applications, thread may not exists, but does not close FD
> >> > + * (means send window) upon exit. Parent thread (tgid) can use
> >> > + * and close the window later.
> >> > + * pid and mm references are taken when window is opened by
> >> > + * process (pid). So tgid is used only when child thread opens
> >> > + * a window and exits without closing it in multithread tasks.
> >> > + */
> >> > + if (!tsk) {
> >> > + pid = window->tgid;
> >> > + tsk = get_pid_task(pid, PIDTYPE_PID);
> >> > + /*
> >> > + * Parent thread will be closing window during its exit.
> >> > + * So should not get here.
> >> > + */
> >> > + if (!tsk)
> >> > + return;
> >>
> >> Similar question on WARN_ON_ONCE()
> > Yes, we can add WARN_ON
> >>
> >> > + }
> >> > +
> >> > + /* Return if the task is exiting. */
> >>
> >> Why? Just because it's no use? It's racy isn't it, so it can't be for
> >> correctness?
> > Yes process is exiting and no need to update CSB. We release the
> > task->usage refcount after copy_to_user().
> >
> >>
> >> > + if (tsk->flags & PF_EXITING) {
> >> > + put_task_struct(tsk);
> >> > + return;
> >> > + }
> >> > +
> >> > + use_mm(window->mm);
> >>
> >> There's no check that csb_addr is actually pointing into userspace, but
> >> copy_to_user() does it for you.
> >>
> >> > + rc = copy_to_user(csb_addr, &csb, sizeof(csb));
> >> > + /*
> >> > + * User space polls on csb.flags (first byte). So add barrier
> >> > + * then copy first byte with csb flags update.
> >> > + */
> >> > + smp_mb();
> >>
> >> You only need to order the stores above vs the store below to csb.flags.
> >> So you should only need an smp_wmb() here.
> > Sure, will add
> > if (!rc) {
> > csb.flags = CSB_V;
> > smp_mb();
> > rc = copy_to_user(csb_addr, &csb, sizeof(u8));
> > }
> >
> >>
> >> > + if (!rc) {
> >> > + csb.flags = CSB_V;
> >> > + rc = copy_to_user(csb_addr, &csb, sizeof(u8));
> >> > + }
> >> > + unuse_mm(window->mm);
> >> > + put_task_struct(tsk);
> >> > +
> >> > + /* Success */
> >> > + if (!rc)
> >> > + return;
> >> > +
> >> > + pr_debug("Invalid CSB address 0x%p signalling pid(%d)\n",
> >> > + csb_addr, pid_vnr(pid));
> >> > +
> >> > + clear_siginfo(&info);
> >> > + info.si_signo = SIGSEGV;
> >> > + info.si_errno = EFAULT;
> >> > + info.si_code = SEGV_MAPERR;
> >> > + info.si_addr = csb_addr;
> >> > +
> >> > + /*
> >> > + * process will be polling on csb.flags after request is sent to
> >> > + * NX. So generally CSB update should not fail except when an
> >> > + * application does not follow the process properly. So an error
> >> > + * message will be displayed and leave it to user space whether
> >> > + * to ignore or handle this signal.
> >> > + */
>
> The code would read a bit better if this comment goes at the start of
> this error handling process it describes (before the error message).
>
> And I feel a bit hypocritical complaining about readability, but it
> could possibly do with some work.
>
> /*
> * The application should have provided a valid mapping for the
> * csb, and not unmap it before the csb.flags update, so the
> * copy_to_user should not fail.
> *
> * If the application fails to follow this protocol, log a kernel
> * error and send a SEGV to the pid. This signal may be ignored,
> * so can't use force_sig_fault_to_task()
> */
>
> Something like tthat?
Thanks Nick, will update.
>
> >> > + rcu_read_lock();
> >> > + rc = kill_pid_info(SIGSEGV, &info, pid);
> >> > + rcu_read_unlock();
>
> ipc/mqueue.c says kill_pid_info doesn't need rcu_read_lock(). AFAIKS
> it's held around kill_pid_info in kernel/signal.c for the find_vpid().
I was following as in kill_proc_info(). Will remove rcu_read_lock().
>
> Thanks,
> Nick
>
> >>
> >> Shouldn't this be using force_sig_fault_to_task() or another helper,
> >> rather than open-coding?
> >
> > Applications or nxz library can ignore this signal based on si_addr or
> > take action like resend new request with valid csb_addr. Hence I did not
> > use force_sig_info_to_task().
>
> >
> >>
> >> > +
> >> > + pr_devel("%s(): pid %d kill_proc_info() rc %d\n", __func__,
> >> > + pid_vnr(pid), rc);
> >> > +}
> >> > +
> >> > +/*
> >> > * Process valid CRBs in fault FIFO.
> >> > */
> >> > irqreturn_t vas_fault_thread_fn(int irq, void *data)
> >> > @@ -111,6 +224,7 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
> >> > return IRQ_HANDLED;
> >> > }
> >> >
> >> > + update_csb(window, crb);
> >> > } while (true);
> >> > }
> >> >
> >> > --
> >> > 1.8.3.1
> >>
> >> cheers
> >
> >
> >
^ permalink raw reply
* Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
From: Haren Myneni @ 2020-03-23 0:57 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584923120.arc9bj6gmg.astroid@bobo.none>
On Mon, 2020-03-23 at 10:30 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 19, 2020 4:13 pm:
> >
> > Kernel sets fault address and status in CRB for NX page fault on user
> > space address after processing page fault. User space gets the signal
> > and handles the fault mentioned in CRB by bringing the page in to
> > memory and send NX request again.
> >
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> > arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
> > 1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
> > index 9872f85..b233d1e 100644
> > --- a/arch/powerpc/include/asm/icswx.h
> > +++ b/arch/powerpc/include/asm/icswx.h
>
> "icswx" is not a thing anymore, after 6ff4d3e96652 ("powerpc: Remove old
> unused icswx based coprocessor support"). I guess NX is reusing some
> things from it, but it would be good to get rid of the cruft and re-name
> this file and and relevant names.
>
> NX already uses this file, so I guesss that can happen after this series.
But NX uses icswx on P8 and icswx.h has only NX specific macros right
now.
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH v3 9/9] Documentation/powerpc: VAS API
From: Haren Myneni @ 2020-03-23 0:54 UTC (permalink / raw)
To: Daniel Axtens
Cc: mikey, herbert, npiggin, linux-crypto, sukadev, linuxppc-dev
In-Reply-To: <87blorw6wc.fsf@dja-thinkpad.axtens.net>
On Fri, 2020-03-20 at 23:24 +1100, Daniel Axtens wrote:
> Hi Haren,
>
> This is good documentation.
>
> > Power9 introduced Virtual Accelerator Switchboard (VAS) which allows
> > userspace to communicate with Nest Accelerator (NX) directly. But
> > kernel has to establish channel to NX for userspace. This document
> > describes user space API that application can use to establish
> > communication channel.
> >
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> > Documentation/powerpc/index.rst | 1 +
> > Documentation/powerpc/vas-api.rst | 246 ++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 247 insertions(+)
> > create mode 100644 Documentation/powerpc/vas-api.rst
> >
> > diff --git a/Documentation/powerpc/index.rst b/Documentation/powerpc/index.rst
> > index 0d45f0f..afe2d5e 100644
> > --- a/Documentation/powerpc/index.rst
> > +++ b/Documentation/powerpc/index.rst
> > @@ -30,6 +30,7 @@ powerpc
> > syscall64-abi
> > transactional_memory
> > ultravisor
> > + vas-api
> >
> > .. only:: subproject and html
> >
> > diff --git a/Documentation/powerpc/vas-api.rst b/Documentation/powerpc/vas-api.rst
> > new file mode 100644
> > index 0000000..13ce4e7
> > --- /dev/null
> > +++ b/Documentation/powerpc/vas-api.rst
> > @@ -0,0 +1,246 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +.. _VAS-API:
> > +
> > +===================================================
> > +Virtual Accelerator Switchboard (VAS) userspace API
> > +===================================================
> > +
> > +Introduction
> > +============
> > +
> > +Power9 processor introduced Virtual Accelerator Switchboard (VAS) which
> > +allows both userspace and kernel communicate to co-processor
> > +(hardware accelerator) referred to as the Nest Accelerator (NX). The NX
> > +unit comprises of one or more hardware engines or co-processor types
> > +such as 842 compression, GZIP compression and encryption. On power9,
> > +userspace applications will have access to only GZIP Compression engine
> > +which supports ZLIB and GZIP compression algorithms in the hardware.
> > +
> > +To communicate with NX, kernel has to establish a channel or window and
> > +then requests can be submitted directly without kernel involvement.
> > +Requests to the GZIP engine must be formatted as a co-processor Request
> > +Block (CRB) and these CRBs must be submitted to the NX using COPY/PASTE
> > +instructions to paste the CRB to hardware address that is associated with
> > +the engine's request queue.
> > +
> > +The GZIP engine provides two priority levels of requests: Normal and
> > +High. Only Normal requests are supported from userspace right now.
> > +
> > +This document explains userspace API that is used to interact with
> > +kernel to setup channel / window which can be used to send compression
> > +requests directly to NX accelerator.
> > +
> > +
> > +Overview
> > +========
> > +
> > +Application access to the GZIP engine is provided through
> > +/dev/crypto/nx-gzip device node implemented by the VAS/NX device driver.
> > +An application must open the /dev/crypto/nx-gzip device to obtain a file
> > +descriptor (fd). Then should issue VAS_TX_WIN_OPEN ioctl with this fd to
> > +establish connection to the engine. It means send window is opened on GZIP
> > +engine for this process. Once a connection is established, the application
> > +should use the mmap() system call to map the hardware address of engine's
> > +request queue into the application's virtual address space.
> > +
> > +The application can then submit one or more requests to the the engine by
> > +using copy/paste instructions and pasting the CRBs to the virtual address
> > +(aka paste_address) returned by mmap(). User space can close the
> > +established connection or send window by closing the file descriptior
> > +(close(fd)) or upon the process exit.
> > +
> > +Note that applications can send several requests with the same window or
> > +can establish multiple windows, but one window for each file descriptor.
> > +
> > +Following sections provide additional details and references about the
> > +individual steps.
> > +
> > +NX-GZIP Device Node
> > +===================
> > +
> > +There is one /dev/crypto/nx-gzip node in the system and it provides
> > +access to all GZIP engines in the system. The only valid operations on
> > +/dev/crypto/nx-gzip are:
> > +
> > + * open() the device for read and write.
> > + * issue VAS_TX_WIN_OPEN ioctl
> > + * mmap() the engine's request queue into application's virtual
> > + address space (i.e. get a paste_address for the co-processor
> > + engine).
> > + * close the device node.
> > +
> > +Other file operations on this device node are undefined.
> > +
> > +Note that the copy and paste operations go directly to the hardware and
> > +do not go through this device. Refer COPY/PASTE document for more
> > +details.
> > +
> > +Although a system may have several instances of the NX co-processor
> > +engines (typically, one per P9 chip) there is just one
> > +/dev/crypto/nx-gzip device node in the system. When the nx-gzip device
> > +node is opened, Kernel opens send window on a suitable instance of NX
> > +accelerator. It finds CPU on which the user process is executing and
> > +determine the NX instance for the corresponding chip on which this CPU
> > +belongs.
> > +
> > +Applications may chose a specific instance of the NX co-processor using
> > +the vas_id field in the VAS_TX_WIN_OPEN ioctl as detailed below.
> > +
> > +A userspace library libnxz is available here but still in development:
> > + https://github.com/abalib/power-gzip
> > +
> > +Applications that use inflate / deflate calls can link with libnxz
> > +instead of libz and use NX GZIP compression without any modification.
> > +
> > +Open /dev/crypto/nx-gzip
> > +========================
> > +
> > +The nx-gzip device should be opened for read and write. No special
> > +privileges are needed to open the device. Each window coreesponds to one
>
> s/coreesponds/corresponds/
>
> > +file descriptor. So if the userspace process needs multiple windows,
> > +several open calls have to be issued.
> > +
> > +See open(2) system call man pages for other details such as return values,
> > +error codes and restrictions.
> > +codes and restrictions.
>
> You have 'codes and restrictions' twice here.
>
> > +
> > +VAS_TX_WIN_OPEN ioctl
> > +=====================
> > +
> > +Applications should use the VAS_TX_WIN_OPEN ioctl as follows to establish
> > +a connection with NX co-processor engine:
> > +
> > + ::
> > + struct vas_tx_win_open_attr {
> > + __u32 version;
> > + __s16 vas_id; /* specific instance of vas or -1
> > + for default */
> > + __u16 reserved1;
> > + __u64 flags; /* For future use */
> > + __u64 reserved2[6];
> > + };
> > +
> > + version: The version field must be currently set to 1.
> > + vas_id: If '-1' is passed, kernel will make a best-effort attempt
> > + to assign an optimal instance of NX for the process. To
> > + select the specific VAS instance, refer
> > + "Discovery of available VAS engines" section below.
> > +
> > + flags, reserved1 and reserved2[6] fields are for future extension
> > + and must be set to 0.
> > +
> > + The attributes attr for the VAS_TX_WIN_OPEN ioctl are defined as
> > + follows:
> > + #define VAS_MAGIC 'v'
> > + #define VAS_TX_WIN_OPEN _IOW(VAS_MAGIC, 1,
> > + struct vas_tx_win_open_attr)
> > +
> > + struct vas_tx_win_open_attr attr;
> > + rc = ioctl(fd, VAS_TX_WIN_OPEN, &attr);
> > +
> > + The VAS_TX_WIN_OPEN ioctl returns 0 on success. On errors, it
> > + returns -1 and sets the errno variable to indicate the error.
> > +
> > + Error conditions:
> > + EINVAL fd does not refer to a valid VAS device.
> > + EINVAL Invalid vas ID
> > + EINVAL version is not set with proper value
> > + EEXIST Window is already opened for the given fd
> > + ENOMEM Memory is not available to allocate window
> > + ENOSPC System has too many active windows (connections)
> > + opened
> > + EINVAL reserved fields are not set to 0.
> > +
> > + See the ioctl(2) man page for more details, error codes and
> > + restrictions.
> > +
> > +mmap() NX-GZIP device
> > +=====================
> > +
> > +The mmap() system call for a NX-GZIP device fd returns a paste_address
> > +that the application can use to copy/paste its CRB to the hardware engines.
> > + ::
> > +
> > + paste_addr = mmap(addr, size, prot, flags, fd, offset);
> > +
> > + Only restrictions on mmap for a NX-GZIP device fd are:
> > + * size should be 4K page size
>
> Patch 3 seems to allow a 64k page if the system is compiled for 64k
> pages... Should it restrict it to 4K?
Yes restricted to 4K for NX-GZIP
^ permalink raw reply
* Re: [PATCH v8 03/14] powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
From: Nicholas Piggin @ 2020-03-23 0:30 UTC (permalink / raw)
To: Haren Myneni, mpe; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584598437.9256.15247.camel@hbabu-laptop>
Haren Myneni's on March 19, 2020 4:13 pm:
>
> Kernel sets fault address and status in CRB for NX page fault on user
> space address after processing page fault. User space gets the signal
> and handles the fault mentioned in CRB by bringing the page in to
> memory and send NX request again.
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
> arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
> index 9872f85..b233d1e 100644
> --- a/arch/powerpc/include/asm/icswx.h
> +++ b/arch/powerpc/include/asm/icswx.h
"icswx" is not a thing anymore, after 6ff4d3e96652 ("powerpc: Remove old
unused icswx based coprocessor support"). I guess NX is reusing some
things from it, but it would be good to get rid of the cruft and re-name
this file and and relevant names.
NX already uses this file, so I guesss that can happen after this series.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v8 01/14] powerpc/xive: Define xive_native_alloc_irq_on_chip()
From: Nicholas Piggin @ 2020-03-23 0:20 UTC (permalink / raw)
To: Haren Myneni, mpe
Cc: mikey, ajd, hch, oohall, Cédric Le Goater, sukadev,
linuxppc-dev, herbert
In-Reply-To: <1584598352.9256.15242.camel@hbabu-laptop>
Haren Myneni's on March 19, 2020 4:12 pm:
>
> This function allocates IRQ on a specific chip. VAS needs per chip
> IRQ allocation and will have IRQ handler per VAS instance.
Can't see a problem, but don't really know the XIVE code. Cédric seems
like an obvious omission from CC here.
Thanks,
Nick
>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
> arch/powerpc/include/asm/xive.h | 9 ++++++++-
> arch/powerpc/sysdev/xive/native.c | 6 +++---
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index 93f982db..d08ea11 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -5,6 +5,8 @@
> #ifndef _ASM_POWERPC_XIVE_H
> #define _ASM_POWERPC_XIVE_H
>
> +#include <asm/opal-api.h>
> +
> #define XIVE_INVALID_VP 0xffffffff
>
> #ifdef CONFIG_PPC_XIVE
> @@ -108,7 +110,6 @@ struct xive_q {
> int xive_native_populate_irq_data(u32 hw_irq,
> struct xive_irq_data *data);
> void xive_cleanup_irq_data(struct xive_irq_data *xd);
> -u32 xive_native_alloc_irq(void);
> void xive_native_free_irq(u32 irq);
> int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq);
>
> @@ -137,6 +138,12 @@ int xive_native_set_queue_state(u32 vp_id, uint32_t prio, u32 qtoggle,
> u32 qindex);
> int xive_native_get_vp_state(u32 vp_id, u64 *out_state);
> bool xive_native_has_queue_state_support(void);
> +extern u32 xive_native_alloc_irq_on_chip(u32 chip_id);
> +
> +static inline u32 xive_native_alloc_irq(void)
> +{
> + return xive_native_alloc_irq_on_chip(OPAL_XIVE_ANY_CHIP);
> +}
>
> #else
>
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 0ff6b73..14d4406 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -279,12 +279,12 @@ static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
> }
> #endif /* CONFIG_SMP */
>
> -u32 xive_native_alloc_irq(void)
> +u32 xive_native_alloc_irq_on_chip(u32 chip_id)
> {
> s64 rc;
>
> for (;;) {
> - rc = opal_xive_allocate_irq(OPAL_XIVE_ANY_CHIP);
> + rc = opal_xive_allocate_irq(chip_id);
> if (rc != OPAL_BUSY)
> break;
> msleep(OPAL_BUSY_DELAY_MS);
> @@ -293,7 +293,7 @@ u32 xive_native_alloc_irq(void)
> return 0;
> return rc;
> }
> -EXPORT_SYMBOL_GPL(xive_native_alloc_irq);
> +EXPORT_SYMBOL_GPL(xive_native_alloc_irq_on_chip);
>
> void xive_native_free_irq(u32 irq)
> {
> --
> 1.8.3.1
>
>
>
>
^ permalink raw reply
* Re: [PATCH V7 09/14] powerpc/vas: Update CSB and notify process for fault CRBs
From: Nicholas Piggin @ 2020-03-23 0:06 UTC (permalink / raw)
To: Haren Myneni, Michael Ellerman
Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584473263.9256.14791.camel@hbabu-laptop>
Haren Myneni's on March 18, 2020 5:27 am:
> On Tue, 2020-03-17 at 16:28 +1100, Michael Ellerman wrote:
>> Haren Myneni <haren@linux.ibm.com> writes:
>> > For each fault CRB, update fault address in CRB (fault_storage_addr)
>> > and translation error status in CSB so that user space can touch the
>> > fault address and resend the request. If the user space passed invalid
>> > CSB address send signal to process with SIGSEGV.
>> >
>> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
>> > ---
>> > arch/powerpc/platforms/powernv/vas-fault.c | 114 +++++++++++++++++++++++++++++
>> > 1 file changed, 114 insertions(+)
>> >
>> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
>> > index 1c6d5cc..751ce48 100644
>> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
>> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
>> > @@ -11,6 +11,7 @@
>> > #include <linux/slab.h>
>> > #include <linux/uaccess.h>
>> > #include <linux/kthread.h>
>> > +#include <linux/sched/signal.h>
>> > #include <linux/mmu_context.h>
>> > #include <asm/icswx.h>
>> >
>> > @@ -26,6 +27,118 @@
>> > #define VAS_FAULT_WIN_FIFO_SIZE (4 << 20)
>> >
>> > /*
>> > + * Update the CSB to indicate a translation error.
>> > + *
>> > + * If we are unable to update the CSB means copy_to_user failed due to
>> > + * invalid csb_addr, send a signal to the process.
>> > + *
>> > + * Remaining settings in the CSB are based on wait_for_csb() of
>> > + * NX-GZIP.
>> > + */
>> > +static void update_csb(struct vas_window *window,
>> > + struct coprocessor_request_block *crb)
>> > +{
>> > + int rc;
>> > + struct pid *pid;
>> > + void __user *csb_addr;
>> > + struct task_struct *tsk;
>> > + struct kernel_siginfo info;
>> > + struct coprocessor_status_block csb;
>>
>> csb is on the stack, and later copied to user, which is a risk for
>> creating an infoleak.
>>
>> Also please use reverse Christmas tree layout for your variables.
>>
>> > +
>> > + /*
>> > + * NX user space windows can not be opened for task->mm=NULL
>> > + * and faults will not be generated for kernel requests.
>> > + */
>> > + if (!window->mm || !window->user_win)
>> > + return;
>>
>> If that's a should-never-happen condition then should it do a
>> WARN_ON_ONCE() rather than silently returning?
>
> Will add WARN_ON
>
>>
>> > + csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
>> > +
>> > + csb.cc = CSB_CC_TRANSLATION;
>> > + csb.ce = CSB_CE_TERMINATION;
>> > + csb.cs = 0;
>> > + csb.count = 0;
>> > +
>> > + /*
>> > + * NX operates and returns in BE format as defined CRB struct.
>> > + * So return fault_storage_addr in BE as NX pastes in FIFO and
>> > + * expects user space to convert to CPU format.
>> > + */
>> > + csb.address = crb->stamp.nx.fault_storage_addr;
>> > + csb.flags = 0;
>>
>> I'm pretty sure this has initialised all the fields of csb.
>>
>> But, I'd still be much happier if you zeroed the whole struct to begin
>> with, that way we know for sure we can't leak any uninitialised bytes to
>> userspace. It's only 16 bytes so it shouldn't add any noticeable
>> overhead.
> Sure, will initialize csb
>>
>> > +
>> > + pid = window->pid;
>> > + tsk = get_pid_task(pid, PIDTYPE_PID);
>> > + /*
>> > + * Send window will be closed after processing all NX requests
>> > + * and process exits after closing all windows. In multi-thread
>> > + * applications, thread may not exists, but does not close FD
>> > + * (means send window) upon exit. Parent thread (tgid) can use
>> > + * and close the window later.
>> > + * pid and mm references are taken when window is opened by
>> > + * process (pid). So tgid is used only when child thread opens
>> > + * a window and exits without closing it in multithread tasks.
>> > + */
>> > + if (!tsk) {
>> > + pid = window->tgid;
>> > + tsk = get_pid_task(pid, PIDTYPE_PID);
>> > + /*
>> > + * Parent thread will be closing window during its exit.
>> > + * So should not get here.
>> > + */
>> > + if (!tsk)
>> > + return;
>>
>> Similar question on WARN_ON_ONCE()
> Yes, we can add WARN_ON
>>
>> > + }
>> > +
>> > + /* Return if the task is exiting. */
>>
>> Why? Just because it's no use? It's racy isn't it, so it can't be for
>> correctness?
> Yes process is exiting and no need to update CSB. We release the
> task->usage refcount after copy_to_user().
>
>>
>> > + if (tsk->flags & PF_EXITING) {
>> > + put_task_struct(tsk);
>> > + return;
>> > + }
>> > +
>> > + use_mm(window->mm);
>>
>> There's no check that csb_addr is actually pointing into userspace, but
>> copy_to_user() does it for you.
>>
>> > + rc = copy_to_user(csb_addr, &csb, sizeof(csb));
>> > + /*
>> > + * User space polls on csb.flags (first byte). So add barrier
>> > + * then copy first byte with csb flags update.
>> > + */
>> > + smp_mb();
>>
>> You only need to order the stores above vs the store below to csb.flags.
>> So you should only need an smp_wmb() here.
> Sure, will add
> if (!rc) {
> csb.flags = CSB_V;
> smp_mb();
> rc = copy_to_user(csb_addr, &csb, sizeof(u8));
> }
>
>>
>> > + if (!rc) {
>> > + csb.flags = CSB_V;
>> > + rc = copy_to_user(csb_addr, &csb, sizeof(u8));
>> > + }
>> > + unuse_mm(window->mm);
>> > + put_task_struct(tsk);
>> > +
>> > + /* Success */
>> > + if (!rc)
>> > + return;
>> > +
>> > + pr_debug("Invalid CSB address 0x%p signalling pid(%d)\n",
>> > + csb_addr, pid_vnr(pid));
>> > +
>> > + clear_siginfo(&info);
>> > + info.si_signo = SIGSEGV;
>> > + info.si_errno = EFAULT;
>> > + info.si_code = SEGV_MAPERR;
>> > + info.si_addr = csb_addr;
>> > +
>> > + /*
>> > + * process will be polling on csb.flags after request is sent to
>> > + * NX. So generally CSB update should not fail except when an
>> > + * application does not follow the process properly. So an error
>> > + * message will be displayed and leave it to user space whether
>> > + * to ignore or handle this signal.
>> > + */
The code would read a bit better if this comment goes at the start of
this error handling process it describes (before the error message).
And I feel a bit hypocritical complaining about readability, but it
could possibly do with some work.
/*
* The application should have provided a valid mapping for the
* csb, and not unmap it before the csb.flags update, so the
* copy_to_user should not fail.
*
* If the application fails to follow this protocol, log a kernel
* error and send a SEGV to the pid. This signal may be ignored,
* so can't use force_sig_fault_to_task()
*/
Something like tthat?
>> > + rcu_read_lock();
>> > + rc = kill_pid_info(SIGSEGV, &info, pid);
>> > + rcu_read_unlock();
ipc/mqueue.c says kill_pid_info doesn't need rcu_read_lock(). AFAIKS
it's held around kill_pid_info in kernel/signal.c for the find_vpid().
Thanks,
Nick
>>
>> Shouldn't this be using force_sig_fault_to_task() or another helper,
>> rather than open-coding?
>
> Applications or nxz library can ignore this signal based on si_addr or
> take action like resend new request with valid csb_addr. Hence I did not
> use force_sig_info_to_task().
>
>>
>> > +
>> > + pr_devel("%s(): pid %d kill_proc_info() rc %d\n", __func__,
>> > + pid_vnr(pid), rc);
>> > +}
>> > +
>> > +/*
>> > * Process valid CRBs in fault FIFO.
>> > */
>> > irqreturn_t vas_fault_thread_fn(int irq, void *data)
>> > @@ -111,6 +224,7 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
>> > return IRQ_HANDLED;
>> > }
>> >
>> > + update_csb(window, crb);
>> > } while (true);
>> > }
>> >
>> > --
>> > 1.8.3.1
>>
>> cheers
>
>
>
^ permalink raw reply
* Re: [PATCH 18/15] kvm: Replace vcpu->swait with rcuwait
From: Peter Zijlstra @ 2020-03-22 22:32 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: rdunlap, linux-pci, bigeasy, linux-kernel, joel, will, mingo,
arnd, Davidlohr Bueso, torvalds, paulmck, linuxppc-dev, rostedt,
bhelgaas, kurt.schwemmer, kvalo, balbi, gregkh, linux-usb,
linux-wireless, oleg, tglx, netdev, Paolo Bonzini, logang, davem
In-Reply-To: <20200322163317.mh4sygr7xcjptmjp@linux-p48b>
On Sun, Mar 22, 2020 at 09:33:17AM -0700, Davidlohr Bueso wrote:
> On Fri, 20 Mar 2020, Peter Zijlstra wrote:
>
> > On Fri, Mar 20, 2020 at 01:55:26AM -0700, Davidlohr Bueso wrote:
> > > - swait_event_interruptible_exclusive(*wq, ((!vcpu->arch.power_off) &&
> > > - (!vcpu->arch.pause)));
> > > + rcuwait_wait_event(*wait,
> > > + (!vcpu->arch.power_off) && (!vcpu->arch.pause),
> > > + TASK_INTERRUPTIBLE);
> >
> > > - for (;;) {
> > > - prepare_to_swait_exclusive(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
> > > -
> > > - if (kvm_vcpu_check_block(vcpu) < 0)
> > > - break;
> > > -
> > > - waited = true;
> > > - schedule();
> > > - }
> > > -
> > > - finish_swait(&vcpu->wq, &wait);
> > > + rcuwait_wait_event(&vcpu->wait,
> > > + (block_check = kvm_vcpu_check_block(vcpu)) < 0,
> > > + TASK_INTERRUPTIBLE);
> >
> > Are these yet more instances that really want to be TASK_IDLE ?
>
> Hmm probably as it makes sense for a blocked vcpu not to be contributing to
> the loadavg. So if this is the only reason to use interruptible, then yes we
> ought to change it.
>
> However, I'll make this a separate patch, given this (ab)use isn't as obvious
> as the PS3 case, which is a kthread and therefore signals are masked.
The thing that was a dead give-away was that the return value of the
interruptible wait wasn't used.
^ permalink raw reply
* Re: [PATCH v3 3/9] powerpc/vas: Add VAS user space API
From: Haren Myneni @ 2020-03-22 20:50 UTC (permalink / raw)
To: Daniel Axtens
Cc: mikey, herbert, npiggin, linux-crypto, sukadev, linuxppc-dev
In-Reply-To: <87fte3w775.fsf@dja-thinkpad.axtens.net>
On Fri, 2020-03-20 at 23:18 +1100, Daniel Axtens wrote:
> Haren Myneni <haren@linux.ibm.com> writes:
>
> > On power9, userspace can send GZIP compression requests directly to NX
> > once kernel establishes NX channel / window with VAS. This patch provides
> > user space API which allows user space to establish channel using open
> > VAS_TX_WIN_OPEN ioctl, mmap and close operations.
> >
> > Each window corresponds to file descriptor and application can open
> > multiple windows. After the window is opened, VAS_TX_WIN_OPEN icoctl to
> > open a window on specific VAS instance, mmap() system call to map
> > the hardware address of engine's request queue into the application's
> > virtual address space.
> >
> > Then the application can then submit one or more requests to the the
> > engine by using the copy/paste instructions and pasting the CRBs to
> > the virtual address (aka paste_address) returned by mmap().
> >
> > Only NX GZIP coprocessor type is supported right now and allow GZIP
> > engine access via /dev/crypto/nx-gzip device node.
> >
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> > arch/powerpc/include/asm/vas.h | 11 ++
> > arch/powerpc/platforms/powernv/Makefile | 2 +-
> > arch/powerpc/platforms/powernv/vas-api.c | 290 ++++++++++++++++++++++++++++
> > arch/powerpc/platforms/powernv/vas-window.c | 6 +-
> > arch/powerpc/platforms/powernv/vas.h | 2 +
> > 5 files changed, 307 insertions(+), 4 deletions(-)
> > create mode 100644 arch/powerpc/platforms/powernv/vas-api.c
> >
> > diff --git a/arch/powerpc/include/asm/vas.h b/arch/powerpc/include/asm/vas.h
> > index f93e6b0..e064953 100644
> > --- a/arch/powerpc/include/asm/vas.h
> > +++ b/arch/powerpc/include/asm/vas.h
> > @@ -163,4 +163,15 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
> > */
> > int vas_paste_crb(struct vas_window *win, int offset, bool re);
> >
> > +/*
> > + * Register / unregister coprocessor type to VAS API which will be exported
> > + * to user space. Applications can use this API to open / close window
> > + * which can be used to send / receive requests directly to cooprcessor.
> > + *
> > + * Only NX GZIP coprocessor type is supported now, but this API can be
> > + * used for others in future.
> > + */
> > +int vas_register_coproc_api(struct module *mod);
> > +void vas_unregister_coproc_api(void);
> > +
> > #endif /* __ASM_POWERPC_VAS_H */
> > diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> > index 395789f..fe3f0fb 100644
> > --- a/arch/powerpc/platforms/powernv/Makefile
> > +++ b/arch/powerpc/platforms/powernv/Makefile
> > @@ -17,7 +17,7 @@ obj-$(CONFIG_MEMORY_FAILURE) += opal-memory-errors.o
> > obj-$(CONFIG_OPAL_PRD) += opal-prd.o
> > obj-$(CONFIG_PERF_EVENTS) += opal-imc.o
> > obj-$(CONFIG_PPC_MEMTRACE) += memtrace.o
> > -obj-$(CONFIG_PPC_VAS) += vas.o vas-window.o vas-debug.o vas-fault.o
> > +obj-$(CONFIG_PPC_VAS) += vas.o vas-window.o vas-debug.o vas-fault.o vas-api.o
> > obj-$(CONFIG_OCXL_BASE) += ocxl.o
> > obj-$(CONFIG_SCOM_DEBUGFS) += opal-xscom.o
> > obj-$(CONFIG_PPC_SECURE_BOOT) += opal-secvar.o
> > diff --git a/arch/powerpc/platforms/powernv/vas-api.c b/arch/powerpc/platforms/powernv/vas-api.c
> > new file mode 100644
> > index 0000000..3473a4a
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/vas-api.c
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * VAS user space API for its accelerators (Only NX-GZIP is supported now)
> > + * Copyright (C) 2019 Haren Myneni, IBM Corp
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/cdev.h>
> > +#include <linux/fs.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <asm/vas.h>
> > +#include <uapi/asm/vas-api.h>
> > +#include "vas.h"
> > +
> > +/*
> > + * The driver creates the device node that can be used as follows:
> > + * For NX-GZIP
> > + *
> > + * fd = open("/dev/crypto/nx-gzip", O_RDWR);
> > + * rc = ioctl(fd, VAS_TX_WIN_OPEN, &attr);
> > + * paste_addr = mmap(NULL, PAGE_SIZE, prot, MAP_SHARED, fd, 0ULL).
> > + * vas_copy(&crb, 0, 1);
> > + * vas_paste(paste_addr, 0, 1);
> > + * close(fd) or exit process to close window.
> > + *
> > + * where "vas_copy" and "vas_paste" are defined in copy-paste.h.
> > + * copy/paste returns to the user space directly. So refer NX hardware
> > + * documententation for excat copy/paste usage and completion / error
> > + * conditions.
> > + */
> > +
> > +static char *coproc_dev_name = "nx-gzip";
> > +static atomic_t coproc_instid = ATOMIC_INIT(0);
> > +
> > +/*
> > + * Wrapper object for the nx-gzip device - there is just one instance of
> > + * this node for the whole system.
> > + */
> > +static struct coproc_dev {
> > + struct cdev cdev;
> > + struct device *device;
> > + char *name;
> > + dev_t devt;
> > + struct class *class;
> > +} coproc_device;
> > +
> > +/*
> > + * One instance per open of a nx-gzip device. Each coproc_instance is
> > + * associated with a VAS window after the caller issues
> > + * VAS_GZIP_TX_WIN_OPEN ioctl.
> > + */
> > +struct coproc_instance {
> > + int id;
> > + struct vas_window *txwin;
> > +};
> > +
> > +static char *coproc_devnode(struct device *dev, umode_t *mode)
> > +{
> > + return kasprintf(GFP_KERNEL, "crypto/%s", dev_name(dev));
> > +}
> > +
> > +static int coproc_open(struct inode *inode, struct file *fp)
> > +{
> > + struct coproc_instance *instance;
> > +
> > + instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> > + if (!instance)
> > + return -ENOMEM;
> > +
> > + instance->id = atomic_inc_return(&coproc_instid);
>
> I don't understand what this instance->id field does - I can't find any
> other uses of it in these series.
>
> I'm also not sure that this gives you a unique id - you increment it
> here and decrement it in coproc_release, but I'm not sure what prevents
> the same ID being given to multiple instances, e.g. the following
> sequence
>
> coproc_open(inode, file A) -> instance with id 0, coproc_instid = 1
> coproc_open(inode, file B) -> instance with id 1, coproc_instid = 2
> coproc_release(inode, file A) -> release id 0, coproc_instid = 1
> coproc_open(inode, file C) -> instance with id 1, coproc_instid = 2
>
> File B and C both have ID = 1, unless I'm misunderstanding something.
Thanks for your comments.
coproc_instid is not needed. Sorry My mistake, it was added in prototype
code, but forgot to remove. Added to determine how many windows are
active, but not needed now.
I will repost the patch with this change.
>
> > +
> > + fp->private_data = instance;
> > + return 0;
> > +}
> > +
> > +static int coproc_ioc_tx_win_open(struct file *fp, unsigned long arg)
> > +{
> > + int rc, vasid;
> > + struct vas_tx_win_attr txattr;
> > + struct vas_tx_win_open_attr uattr;
> > + void __user *uptr = (void __user *)arg;
> > + struct vas_window *txwin;
> > + struct coproc_instance *nxti = fp->private_data;
> > +
> > + if (!nxti)
> > + return -EINVAL;
> > +
> > + /*
> > + * One window for file descriptor
> > + */
> > + if (nxti->txwin)
> > + return -EEXIST;
> > +
> > + rc = copy_from_user(&uattr, uptr, sizeof(uattr));
> > + if (rc) {
> > + pr_err("%s(): copy_from_user() returns %d\n", __func__, rc);
> > + return -EFAULT;
> > + }
> > +
> > + if (uattr.version != 1) {
> > + pr_err("Invalid version\n");
> > + return -EINVAL;
> > + }
> > +
> > + vasid = uattr.vas_id;
> > +
> > + memset(&txattr, 0, sizeof(struct vas_tx_win_attr));
>
> You could define txattr with `struct vas_tx_win_attr txattr = {};` and
> avoid the explicit memset.
>
> > + vas_init_tx_win_attr(&txattr, VAS_COP_TYPE_GZIP);
> > +
> > + txattr.lpid = mfspr(SPRN_LPID);
> > + txattr.pidr = mfspr(SPRN_PID);
> > + txattr.user_win = true;
> > + txattr.rsvd_txbuf_count = false;
> > + txattr.pswid = false;
> > + /*
> > + * txattr.wcreds_max is set to VAS_WCREDS_DEFAULT (1024) in
> > + * vas-window.c, but can be changed specific to GZIP depends
> > + * on user space need.
> > + * If needed to set txattr.wcreds_max here.
> > + */
>
> Who could set this? You mention userspace need but it looks like the
> user cannot set this. Is this a message to future kernel developers?
set the default value in vax_tx_win_open() (vas_window.c) if it is not
set here. credits limit can set here if we decide to use non-default
value for user space windows. Not allowing user space to set this value.
Yes, this comment is for kernel developers.
I can remove this comment to remove the confusion.
>
> > +
> > + pr_devel("Pid %d: Opening txwin, PIDR %ld\n", txattr.pidr,
> > + mfspr(SPRN_PID));
> > +
> > + txwin = vas_tx_win_open(vasid, VAS_COP_TYPE_GZIP, &txattr);
> > + if (IS_ERR(txwin)) {
> > + pr_err("%s() vas_tx_win_open() failed, %ld\n", __func__,
> > + PTR_ERR(txwin));
> > + return PTR_ERR(txwin);
> > + }
> > +
> > + nxti->txwin = txwin;
> > +
> > + return 0;
> > +}
> > +
> > +static int coproc_release(struct inode *inode, struct file *fp)
> > +{
> > + struct coproc_instance *instance;
> > +
> > + instance = fp->private_data;
> > +
> > + if (instance && instance->txwin) {
> > + vas_win_close(instance->txwin);
> > + instance->txwin = NULL;
> > + }
> > +
> > + /*
> > + * We don't know here if user has other receive windows
> > + * open, so we can't really call clear_thread_tidr().
> > + * So, once the process calls set_thread_tidr(), the
> > + * TIDR value sticks around until process exits, resulting
> > + * in an extra copy in restore_sprs().
> > + */
> > +
> > + kfree(instance);
> > + fp->private_data = NULL;
> > + atomic_dec(&coproc_instid);
> > +
> > + return 0;
> > +}
> > +
> > +static int coproc_mmap(struct file *fp, struct vm_area_struct *vma)
> > +{
> > + int rc;
> > + pgprot_t prot;
> > + u64 paste_addr;
> > + unsigned long pfn;
> > + struct coproc_instance *instance = fp->private_data;
> > +
> > + if ((vma->vm_end - vma->vm_start) > PAGE_SIZE) {
> > + pr_debug("%s(): size 0x%zx, PAGE_SIZE 0x%zx\n", __func__,
> > + (vma->vm_end - vma->vm_start), PAGE_SIZE);
> > + return -EINVAL;
> > + }
> > +
> > + /* Ensure instance has an open send window */
> > + if (!instance->txwin) {
> > + pr_err("%s(): No send window open?\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + vas_win_paste_addr(instance->txwin, &paste_addr, NULL);
> > + pfn = paste_addr >> PAGE_SHIFT;
> > +
> > + /* flags, page_prot from cxl_mmap(), except we want cachable */
> > + vma->vm_flags |= VM_IO | VM_PFNMAP;
> > + vma->vm_page_prot = pgprot_cached(vma->vm_page_prot);
> > +
> > + prot = __pgprot(pgprot_val(vma->vm_page_prot) | _PAGE_DIRTY);
> > +
> > + rc = remap_pfn_range(vma, vma->vm_start, pfn + vma->vm_pgoff,
> > + vma->vm_end - vma->vm_start, prot);
> > +
> > + pr_devel("%s(): paste addr %llx at %lx, rc %d\n", __func__,
> > + paste_addr, vma->vm_start, rc);
> > +
> > + return rc;
> > +}
> > +
> > +static long coproc_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> > +{
> > + switch (cmd) {
> > + case VAS_TX_WIN_OPEN:
> > + return coproc_ioc_tx_win_open(fp, arg);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static struct file_operations coproc_fops = {
> > + .open = coproc_open,
> > + .release = coproc_release,
> > + .mmap = coproc_mmap,
> > + .unlocked_ioctl = coproc_ioctl,
> > +};
> > +
> > +/*
> > + * Supporting only nx-gzip coprocessor type now, but this API code
> > + * extended to other coprocessor types later.
> > + */
> > +int vas_register_coproc_api(struct module *mod)
> > +{
> > + int rc = -EINVAL;
> > + dev_t devno;
> > +
> > + rc = alloc_chrdev_region(&coproc_device.devt, 1, 1, "nx-gzip");
> > + if (rc) {
> > + pr_err("Unable to allocate coproc major number: %i\n", rc);
> > + return rc;
> > + }
> > +
> > + pr_devel("NX-GZIP device allocated, dev [%i,%i]\n",
> > + MAJOR(coproc_device.devt), MINOR(coproc_device.devt));
> > +
> > + coproc_device.class = class_create(mod, "nx-gzip");
> > + if (IS_ERR(coproc_device.class)) {
> > + rc = PTR_ERR(coproc_device.class);
> > + pr_err("Unable to create NX-GZIP class %d\n", rc);
> > + goto err_class;
> > + }
> > + coproc_device.class->devnode = coproc_devnode;
> > +
> > + coproc_fops.owner = mod;
> > + cdev_init(&coproc_device.cdev, &coproc_fops);
>
> Looking into this coproc_fops thing more:
>
> I find this API very confusing. The comment at the top of the function
> says it will be extended, but there's only one coproc_fops, so currently
> it can only be instantiated once and owned by one module. Much of the
> rest of that function is also very much based around the nx-gzip
> coprocessor.
>
> I'm not fully certain about how this should work, but I think probably
> it either needs to be fully generic or fully nx-gzip only for now. I
> would make it fully nx-gzip only and extend it later, but I'm not fussy.
We are adding only for NX-GZIP right now and mentioned nx-gzip. Most of
interfaces in coproc_fops such as open/mmap/close should be common for
any coprocs except ioctl or new ioctl cmd.
register_coproc_api creates device node and register fs API specific to
coprocessor. user space use fs API to establish communication channel
(using open, ioctl, mmap) to NX.
How about defining coproc_nxgzip_fs to remove this confusion.
>
> > +
> > + devno = MKDEV(MAJOR(coproc_device.devt), 0);
> > + rc = cdev_add(&coproc_device.cdev, devno, 1);
> > + if (rc) {
> > + pr_err("cdev_add() failed %d\n", rc);
> > + goto err_cdev;
> > + }
> > +
> > + coproc_device.device = device_create(coproc_device.class, NULL,
> > + devno, NULL, coproc_dev_name, MINOR(devno));
> > + if (IS_ERR(coproc_device.device)) {
> > + rc = PTR_ERR(coproc_device.device);
> > + pr_err("Unable to create coproc-%d %d\n", MINOR(devno), rc);
> > + goto err;
> > + }
> > +
> > + pr_devel("%s: Added dev [%d,%d]\n", __func__, MAJOR(devno),
> > + MINOR(devno));
> > +
> > + return 0;
> > +
> > +err:
> > + cdev_del(&coproc_device.cdev);
> > +err_cdev:
> > + class_destroy(coproc_device.class);
> > +err_class:
> > + unregister_chrdev_region(coproc_device.devt, 1);
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(vas_register_coproc_api);
> > +
> > +void vas_unregister_coproc_api(void)
> > +{
> > + dev_t devno;
> > +
> > + cdev_del(&coproc_device.cdev);
> > + devno = MKDEV(MAJOR(coproc_device.devt), 0);
> > + device_destroy(coproc_device.class, devno);
> > +
> > + class_destroy(coproc_device.class);
> > + unregister_chrdev_region(coproc_device.devt, 1);
> > +}
> > +EXPORT_SYMBOL_GPL(vas_unregister_coproc_api);
> > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> > index e9ab851..7484296 100644
> > --- a/arch/powerpc/platforms/powernv/vas-window.c
> > +++ b/arch/powerpc/platforms/powernv/vas-window.c
> > @@ -26,7 +26,7 @@
> > * Compute the paste address region for the window @window using the
> > * ->paste_base_addr and ->paste_win_id_shift we got from device tree.
> > */
> > -static void compute_paste_address(struct vas_window *window, u64 *addr, int *len)
> > +void vas_win_paste_addr(struct vas_window *window, u64 *addr, int *len)
> > {
> > int winid;
> > u64 base, shift;
> > @@ -80,7 +80,7 @@ static void *map_paste_region(struct vas_window *txwin)
> > goto free_name;
> >
> > txwin->paste_addr_name = name;
> > - compute_paste_address(txwin, &start, &len);
> > + vas_win_paste_addr(txwin, &start, &len);
> >
> > if (!request_mem_region(start, len, name)) {
> > pr_devel("%s(): request_mem_region(0x%llx, %d) failed\n",
> > @@ -138,7 +138,7 @@ static void unmap_paste_region(struct vas_window *window)
> > u64 busaddr_start;
> >
> > if (window->paste_kaddr) {
> > - compute_paste_address(window, &busaddr_start, &len);
> > + vas_win_paste_addr(window, &busaddr_start, &len);
> > unmap_region(window->paste_kaddr, busaddr_start, len);
> > window->paste_kaddr = NULL;
> > kfree(window->paste_addr_name);
> > diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
> > index 8c39a7d..a10abed 100644
> > --- a/arch/powerpc/platforms/powernv/vas.h
> > +++ b/arch/powerpc/platforms/powernv/vas.h
> > @@ -431,6 +431,8 @@ struct vas_winctx {
> > extern void vas_return_credit(struct vas_window *window, bool tx);
> > extern struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
> > uint32_t pswid);
> > +extern void vas_win_paste_addr(struct vas_window *window, u64 *addr,
> > + int *len);
> >
> > static inline int vas_window_pid(struct vas_window *window)
> > {
> > --
> > 1.8.3.1
^ permalink raw reply
* Re: [PATCH 18/15] kvm: Replace vcpu->swait with rcuwait
From: Davidlohr Bueso @ 2020-03-22 16:33 UTC (permalink / raw)
To: Peter Zijlstra
Cc: rdunlap, linux-pci, bigeasy, linux-kernel, joel, will, mingo,
arnd, Davidlohr Bueso, torvalds, paulmck, linuxppc-dev, rostedt,
bhelgaas, kurt.schwemmer, kvalo, balbi, gregkh, linux-usb,
linux-wireless, oleg, tglx, netdev, Paolo Bonzini, logang, davem
In-Reply-To: <20200320125455.GE20696@hirez.programming.kicks-ass.net>
On Fri, 20 Mar 2020, Peter Zijlstra wrote:
>On Fri, Mar 20, 2020 at 01:55:26AM -0700, Davidlohr Bueso wrote:
>> - swait_event_interruptible_exclusive(*wq, ((!vcpu->arch.power_off) &&
>> - (!vcpu->arch.pause)));
>> + rcuwait_wait_event(*wait,
>> + (!vcpu->arch.power_off) && (!vcpu->arch.pause),
>> + TASK_INTERRUPTIBLE);
>
>> - for (;;) {
>> - prepare_to_swait_exclusive(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
>> -
>> - if (kvm_vcpu_check_block(vcpu) < 0)
>> - break;
>> -
>> - waited = true;
>> - schedule();
>> - }
>> -
>> - finish_swait(&vcpu->wq, &wait);
>> + rcuwait_wait_event(&vcpu->wait,
>> + (block_check = kvm_vcpu_check_block(vcpu)) < 0,
>> + TASK_INTERRUPTIBLE);
>
>Are these yet more instances that really want to be TASK_IDLE ?
Hmm probably as it makes sense for a blocked vcpu not to be contributing to
the loadavg. So if this is the only reason to use interruptible, then yes we
ought to change it.
However, I'll make this a separate patch, given this (ab)use isn't as obvious
as the PS3 case, which is a kthread and therefore signals are masked.
Thanks,
Davidlohr
^ permalink raw reply
* Re: [PATCH v3] powerpc/kprobes: Ignore traps that happened in real mode
From: Naveen N. Rao @ 2020-03-22 15:54 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <f28a0219-abf1-07d4-b98b-b19db4af0f12@c-s.fr>
Christophe Leroy wrote:
> ping
>
>
> Le 18/02/2020 à 20:38, Christophe Leroy a écrit :
>> When a program check exception happens while MMU translation is
>> disabled, following Oops happens in kprobe_handler() in the following
>> code:
>
> Michael, we have several traps in assembly while MMU is still disabled
> (TRACE_IRQFLAGS, KUAP DEBUG, syscall from kernel, machine check in RTAS,
> ...).
> Without this fix, all of them trigger an Oops when CONFIG_KPROBE is set.
For this patch:
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
- Naveen
^ permalink raw reply
* Re: [PATCH] powerpc/64: ftrace don't trace real mode
From: Naveen N. Rao @ 2020-03-22 16:17 UTC (permalink / raw)
To: linuxppc-dev, Nicholas Piggin
In-Reply-To: <1584759479.7ueu8qvhgs.astroid@bobo.none>
Nicholas Piggin wrote:
> Naveen N. Rao's on March 21, 2020 4:39 am:
>> Hi Nick,
>>
>> Nicholas Piggin wrote:
>>> This warns and prevents tracing attempted in a real-mode context.
>>
>> Is this something you're seeing often? Last time we looked at this, KVM
>> was the biggest offender and we introduced paca->ftrace_enabled as a way
>> to disable ftrace while in KVM code.
>
> Not often but it has a tendancy to blow up the least tested code at the
> worst times :)
>
> Machine check is bad, I'm sure HMI too but I haven't tested that yet.
>
> I've fixed up most of it with annotations, this is obviously an extra
> safety not something to rely on like ftrace_enabled. Probably even the
> WARN_ON here is dangerous here, but I don't want to leave these bugs
> in there.
Ok, makes sense.
>
> Although the machine check and hmi code touch a fair bit of stuff and
> annotating is a bit fragile. It might actually be better if the
> paca->ftrace_enabled could be a nesting counter, then we could use it
> in machine checks too and avoid a lot of annotations.
I'm not too familiar with MC/HMI, but I suppose those aren't re-entrant?
If those have access to an emergency stack, can we save/restore
ftrace_enabled state across the handlers?
We're primarily disabling ftrace across idle/offline/KVM right now. I'm
not sure if nesting is useful there.
>
>> While this is cheap when handling ftrace_regs_caller() as done in this
>> patch, for simple function tracing (see below), we will have to grab the
>> MSR which will slow things down slightly.
>
> mfmsr is not too bad these days.
That's great!
Thanks,
Naveen
^ permalink raw reply
* [PATCH v3] powerpc/pseries: Handle UE event for memcpy_mcsafe
From: Ganesh Goudar @ 2020-03-22 16:05 UTC (permalink / raw)
To: mpe, linuxppc-dev; +Cc: mahesh, santosh, Ganesh Goudar
If we hit UE at an instruction with a fixup entry, flag to
ignore the event and set nip to continue execution at the
fixup entry.
For powernv these changes are already made by
commit 895e3dceeb97 ("powerpc/mce: Handle UE event for memcpy_mcsafe")
Reviewed-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
Reviewed-by: Santosh S <santosh@fossix.org>
Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
---
V2: Fixes a trivial checkpatch error in commit msg.
V3: Use proper subject prefix.
---
arch/powerpc/platforms/pseries/ras.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index 43710b69e09e..58e2483fbb1a 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c
@@ -10,6 +10,7 @@
#include <linux/fs.h>
#include <linux/reboot.h>
#include <linux/irq_work.h>
+#include <linux/extable.h>
#include <asm/machdep.h>
#include <asm/rtas.h>
@@ -505,6 +506,7 @@ static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
int initiator = rtas_error_initiator(errp);
int severity = rtas_error_severity(errp);
u8 error_type, err_sub_type;
+ const struct exception_table_entry *entry;
if (initiator == RTAS_INITIATOR_UNKNOWN)
mce_err.initiator = MCE_INITIATOR_UNKNOWN;
@@ -558,6 +560,12 @@ static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
switch (mce_log->error_type) {
case MC_ERROR_TYPE_UE:
mce_err.error_type = MCE_ERROR_TYPE_UE;
+ entry = search_kernel_exception_table(regs->nip);
+ if (entry) {
+ mce_err.ignore_event = true;
+ regs->nip = extable_fixup(entry);
+ disposition = RTAS_DISP_FULLY_RECOVERED;
+ }
switch (err_sub_type) {
case MC_ERROR_UE_IFETCH:
mce_err.u.ue_error_type = MCE_UE_ERROR_IFETCH;
--
2.17.2
^ permalink raw reply related
* Re: [patch V3 04/20] orinoco_usb: Use the regular completion interfaces
From: Kalle Valo @ 2020-03-22 14:42 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Randy Dunlap, linux-ia64, Peter Zijlstra, linux-pci,
Sebastian Siewior, tel.com, Oleg Nesterov, Guo Ren,
Joel Fernandes, Vincent Chen, Ingo Molnar, Jonathan Corbet,
Davidlohr Bueso, Paul E . McKenney, Brian Cain, linux-acpi,
linux-hexagon, Rafael J. Wysocki, linux-csky, Linus Torvalds,
Darren Hart, Zhang Rui, Len Brown, Fenghua Yu, Arnd Bergmann,
linux-pm, kbuild test robot, linuxppc-dev, Greentime Hu,
Bjorn Helgaas, Kurt Schwemmer, platform-driver-x86, Felipe Balbi,
Michal Simek, Tony Luck, Nick Hu, Geoff Levand,
Greg Kroah-Hartman, linux-usb, linux-wireless, LKML,
Davidlohr Bueso, netdev, Logan Gunthorpe, David S. Miller,
Andy Shevchenko
In-Reply-To: <20200321113241.150783464@linutronix.de>
Thomas Gleixner <tglx@linutronix.de> writes:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> The completion usage in this driver is interesting:
>
> - it uses a magic complete function which according to the comment was
> implemented by invoking complete() four times in a row because
> complete_all() was not exported at that time.
>
> - it uses an open coded wait/poll which checks completion:done. Only one wait
> side (device removal) uses the regular wait_for_completion() interface.
>
> The rationale behind this is to prevent that wait_for_completion() consumes
> completion::done which would prevent that all waiters are woken. This is not
> necessary with complete_all() as that sets completion::done to UINT_MAX which
> is left unmodified by the woken waiters.
>
> Replace the magic complete function with complete_all() and convert the
> open coded wait/poll to regular completion interfaces.
>
> This changes the wait to exclusive wait mode. But that does not make any
> difference because the wakers use complete_all() which ignores the
> exclusive mode.
>
> Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: linux-usb@vger.kernel.org
> ---
> V2: New patch to avoid conversion to swait functions later.
> ---
> drivers/net/wireless/intersil/orinoco/orinoco_usb.c | 21 ++++----------------
> 1 file changed, 5 insertions(+), 16 deletions(-)
I assume this is going via some other than wireless-drivers so:
Acked-by: Kalle Valo <kvalo@codeaurora.org>
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH v5 1/7] ASoC: dt-bindings: fsl_asrc: Add new property fsl, asrc-format
From: Shengjiu Wang @ 2020-03-22 10:47 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, Nicolin Chen, Mark Brown,
linuxppc-dev, linux-kernel
In-Reply-To: <20200320173213.GA9093@bogus>
On Sat, Mar 21, 2020 at 1:34 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Mar 09, 2020 at 02:19:44PM -0700, Nicolin Chen wrote:
> > On Mon, Mar 09, 2020 at 11:58:28AM +0800, Shengjiu Wang wrote:
> > > In order to support new EASRC and simplify the code structure,
> > > We decide to share the common structure between them. This bring
> > > a problem that EASRC accept format directly from devicetree, but
> > > ASRC accept width from devicetree.
> > >
> > > In order to align with new ESARC, we add new property fsl,asrc-format.
> > > The fsl,asrc-format can replace the fsl,asrc-width, then driver
> > > can accept format from devicetree, don't need to convert it to
> > > format through width.
> > >
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > ---
> > > Documentation/devicetree/bindings/sound/fsl,asrc.txt | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > index cb9a25165503..780455cf7f71 100644
> > > --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > @@ -51,6 +51,11 @@ Optional properties:
> > > will be in use as default. Otherwise, the big endian
> > > mode will be in use for all the device registers.
> > >
> > > + - fsl,asrc-format : Defines a mutual sample format used by DPCM Back
> > > + Ends, which can replace the fsl,asrc-width.
> > > + The value is SNDRV_PCM_FORMAT_S16_LE, or
> > > + SNDRV_PCM_FORMAT_S24_LE
> >
> > I am still holding the concern at the DT binding of this format,
> > as it uses values from ASoC header file instead of a dt-binding
> > header file -- not sure if we can do this. Let's wait for Rob's
> > comments.
>
> I assume those are an ABI as well, so it's okay to copy them unless we
> already have some format definitions for DT. But it does need to be copy
> in a header under include/dt-bindings/.
Thanks for reviewing. seems it is not a good time to add a new header
file in include/dt-bindings/ in this patch serial. I will drop this change
this time, that still using the "fsl,asrc-width".
best regards
wang shengjiu
^ permalink raw reply
* Re: [PATCH v3] powerpc/kprobes: Ignore traps that happened in real mode
From: Christophe Leroy @ 2020-03-22 10:00 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <424331e2006e7291a1bfe40e7f3fa58825f565e1.1582054578.git.christophe.leroy@c-s.fr>
ping
Le 18/02/2020 à 20:38, Christophe Leroy a écrit :
> When a program check exception happens while MMU translation is
> disabled, following Oops happens in kprobe_handler() in the following
> code:
Michael, we have several traps in assembly while MMU is still disabled
(TRACE_IRQFLAGS, KUAP DEBUG, syscall from kernel, machine check in RTAS,
...).
Without this fix, all of them trigger an Oops when CONFIG_KPROBE is set.
Christophe
>
> } else if (*addr != BREAKPOINT_INSTRUCTION) {
>
> [ 33.098554] BUG: Unable to handle kernel data access on read at 0x0000e268
> [ 33.105091] Faulting instruction address: 0xc000ec34
> [ 33.110010] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 33.115348] BE PAGE_SIZE=16K PREEMPT CMPC885
> [ 33.119540] Modules linked in:
> [ 33.122591] CPU: 0 PID: 429 Comm: cat Not tainted 5.6.0-rc1-s3k-dev-00824-g84195dc6c58a #3267
> [ 33.131005] NIP: c000ec34 LR: c000ecd8 CTR: c019cab8
> [ 33.136002] REGS: ca4d3b58 TRAP: 0300 Not tainted (5.6.0-rc1-s3k-dev-00824-g84195dc6c58a)
> [ 33.144324] MSR: 00001032 <ME,IR,DR,RI> CR: 2a4d3c52 XER: 00000000
> [ 33.150699] DAR: 0000e268 DSISR: c0000000
> [ 33.150699] GPR00: c000b09c ca4d3c10 c66d0620 00000000 ca4d3c60 00000000 00009032 00000000
> [ 33.150699] GPR08: 00020000 00000000 c087de44 c000afe0 c66d0ad0 100d3dd6 fffffff3 00000000
> [ 33.150699] GPR16: 00000000 00000041 00000000 ca4d3d70 00000000 00000000 0000416d 00000000
> [ 33.150699] GPR24: 00000004 c53b6128 00000000 0000e268 00000000 c07c0000 c07bb6fc ca4d3c60
> [ 33.188015] NIP [c000ec34] kprobe_handler+0x128/0x290
> [ 33.192989] LR [c000ecd8] kprobe_handler+0x1cc/0x290
> [ 33.197854] Call Trace:
> [ 33.200340] [ca4d3c30] [c000b09c] program_check_exception+0xbc/0x6fc
> [ 33.206590] [ca4d3c50] [c000e43c] ret_from_except_full+0x0/0x4
> [ 33.212392] --- interrupt: 700 at 0xe268
> [ 33.270401] Instruction dump:
> [ 33.273335] 913e0008 81220000 38600001 3929ffff 91220000 80010024 bb410008 7c0803a6
> [ 33.280992] 38210020 4e800020 38600000 4e800020 <813b0000> 6d2a7fe0 2f8a0008 419e0154
> [ 33.288841] ---[ end trace 5b9152d4cdadd06d ]---
>
> kprobe is not prepared to handle events in real mode and functions
> running in real mode should have been blacklisted, so kprobe_handler()
> can safely bail out telling 'this trap is not mine' for any trap that
> happened while in real-mode.
>
> If the trap happened with MSR_IR or MSR_DR cleared, return 0 immediately.
>
> Reported-by: Larry Finger <Larry.Finger@lwfinger.net>
> Fixes: 6cc89bad60a6 ("powerpc/kprobes: Invoke handlers directly")
> Cc: stable@vger.kernel.org
> Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>
> ---
> v3: Also bail out if MSR_DR is cleared.
>
> Resending v2 with a more appropriate name
>
> v2: bailing out instead of converting real-time address to virtual and continuing.
>
> The bug might have existed even before that commit from Naveen.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/kprobes.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 2d27ec4feee4..9b340af02c38 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -264,6 +264,9 @@ int kprobe_handler(struct pt_regs *regs)
> if (user_mode(regs))
> return 0;
>
> + if (!(regs->msr & MSR_IR) || !(regs->msr & MSR_DR))
> + return 0;
> +
> /*
> * We don't want to be preempted for the entire
> * duration of kprobe processing
>
^ permalink raw reply
* Re: [PATCH v5 6/7] ASoC: dt-bindings: fsl_easrc: Add document for EASRC
From: Shengjiu Wang @ 2020-03-22 8:02 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, Nicolin Chen, Mark Brown,
linuxppc-dev, linux-kernel
In-Reply-To: <20200320174812.GA27070@bogus>
On Sat, Mar 21, 2020 at 1:50 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Mar 09, 2020 at 11:58:33AM +0800, Shengjiu Wang wrote:
> > EASRC (Enhanced Asynchronous Sample Rate Converter) is a new
> > IP module found on i.MX8MN.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > .../devicetree/bindings/sound/fsl,easrc.yaml | 101 ++++++++++++++++++
> > 1 file changed, 101 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/sound/fsl,easrc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/fsl,easrc.yaml b/Documentation/devicetree/bindings/sound/fsl,easrc.yaml
> > new file mode 100644
> > index 000000000000..ff22f8056a63
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/fsl,easrc.yaml
> > @@ -0,0 +1,101 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/fsl,easrc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP Asynchronous Sample Rate Converter (ASRC) Controller
> > +
> > +maintainers:
> > + - Shengjiu Wang <shengjiu.wang@nxp.com>
> > +
> > +properties:
> > + $nodename:
> > + pattern: "^easrc@.*"
> > +
> > + compatible:
> > + const: fsl,imx8mn-easrc
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: Peripheral clock
> > +
> > + clock-names:
> > + items:
> > + - const: mem
> > +
> > + dmas:
> > + maxItems: 8
> > +
> > + dma-names:
> > + items:
> > + - const: ctx0_rx
> > + - const: ctx0_tx
> > + - const: ctx1_rx
> > + - const: ctx1_tx
> > + - const: ctx2_rx
> > + - const: ctx2_tx
> > + - const: ctx3_rx
> > + - const: ctx3_tx
> > +
> > + fsl,easrc-ram-script-name:
>
> 'firmware-name' is the established property name for this.
will use "firmware-name"
>
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/string
> > + - const: imx/easrc/easrc-imx8mn.bin
>
> Though if there's only 1 possible value, why does this need to be in DT?
>
> > + description: The coefficient table for the filters
>
> If the firmware is only 1 thing, then perhaps this should just be a DT
> property rather than a separate file. It depends on who owns/creates
> this file. If fixed for the platform, then DT is a good fit. If updated
> separately from DT and boot firmware, then keeping it separate makes
> sense.
>
The firmware is not fixed for the platform, it is updated separately from
DT. So we can keep it separately.
best regards
wang shengjiu
^ permalink raw reply
* Re: [patch V3 05/20] acpi: Remove header dependency
From: Rafael J. Wysocki @ 2020-03-22 7:02 UTC (permalink / raw)
To: Thomas Gleixner
Cc: open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:, linux-ia64,
Peter Zijlstra, Linux PCI, Sebastian Siewior, Oleg Nesterov,
Guo Ren, Joel Fernandes, Vincent Chen, Ingo Molnar,
Davidlohr Bueso, kbuild test robot, Brian Cain, Jonathan Corbet,
Paul E . McKenney, linux-hexagon, Rafael J. Wysocki, linux-csky,
ACPI Devel Maling List, Darren Hart, Zhang Rui, Len Brown,
Fenghua Yu, Randy Dunlap, Arnd Bergmann, Linux PM, linuxppc-dev,
Greentime Hu, Bjorn Helgaas, Kurt Schwemmer, Platform Driver,
Kalle Valo, Felipe Balbi, Michal Simek, Tony Luck, Nick Hu,
Geoff Levand, Greg Kroah-Hartman, Linus Torvalds,
open list:NETWORKING DRIVERS (WIRELESS), LKML, Davidlohr Bueso,
netdev, Logan Gunthorpe, David S. Miller, Andy Shevchenko
In-Reply-To: <20200321113241.246190285@linutronix.de>
On Sat, Mar 21, 2020 at 12:35 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> From: Peter Zijlstra <peterz@infradead.org>
>
> In order to avoid future header hell, remove the inclusion of
> proc_fs.h from acpi_bus.h. All it needs is a forward declaration of a
> struct.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Andy Shevchenko <andy@infradead.org>
> Cc: platform-driver-x86@vger.kernel.org
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Cc: linux-pm@vger.kernel.org
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-acpi@vger.kernel.org
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/platform/x86/dell-smo8800.c | 1 +
> drivers/platform/x86/wmi.c | 1 +
> drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c | 1 +
> include/acpi/acpi_bus.h | 2 +-
> 4 files changed, 4 insertions(+), 1 deletion(-)
>
> --- a/drivers/platform/x86/dell-smo8800.c
> +++ b/drivers/platform/x86/dell-smo8800.c
> @@ -16,6 +16,7 @@
> #include <linux/interrupt.h>
> #include <linux/miscdevice.h>
> #include <linux/uaccess.h>
> +#include <linux/fs.h>
>
> struct smo8800_device {
> u32 irq; /* acpi device irq */
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -29,6 +29,7 @@
> #include <linux/uaccess.h>
> #include <linux/uuid.h>
> #include <linux/wmi.h>
> +#include <linux/fs.h>
> #include <uapi/linux/wmi.h>
>
> ACPI_MODULE_NAME("wmi");
> --- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> +++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> @@ -19,6 +19,7 @@
> #include <linux/acpi.h>
> #include <linux/uaccess.h>
> #include <linux/miscdevice.h>
> +#include <linux/fs.h>
> #include "acpi_thermal_rel.h"
>
> static acpi_handle acpi_thermal_rel_handle;
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -80,7 +80,7 @@ bool acpi_dev_present(const char *hid, c
>
> #ifdef CONFIG_ACPI
>
> -#include <linux/proc_fs.h>
> +struct proc_dir_entry;
>
> #define ACPI_BUS_FILE_ROOT "acpi"
> extern struct proc_dir_entry *acpi_root_dir;
>
>
^ permalink raw reply
* Re: [PATCH] Documentation: Clarify better about the rwsem non-owner release issue
From: Kalle Valo @ 2020-03-22 6:51 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-doc, Peter Zijlstra, Sebastian Andrzej Siewior,
linux-kernel, netdev, Will Deacon, Thomas Gleixner,
Davidlohr Bueso, Arnd Bergmann, Jonathan Corbet, Ingo Molnar,
Ingo Molnar, Paul E . McKenney, Steven Rostedt, Bjorn Helgaas,
Kurt Schwemmer, linux-pci@vger.kernel.org Felipe Balbi,
".ozlabs.org", Logan Gunthorpe, Randy Dunlap,
linux-wireless, Oleg Nesterov, linuxppc-dev, Greg Kroah-Hartman,
Linus Torvalds, David S. Miller
In-Reply-To: <20200322021938.175736-1-joel@joelfernandes.org>
"Joel Fernandes (Google)" <joel@joelfernandes.org> writes:
> Reword and clarify better about the rwsem non-owner release issue.
>
> Link: https://lore.kernel.org/linux-pci/20200321212144.GA6475@google.com/
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
There's something wrong with your linux-pci and linux-usb addresses:
"linux-pci@vger.kernel.org Felipe Balbi" <balbi@kernel.org>,
"linux-usb@vger.kernel.org Kalle Valo" <kvalo@codeaurora.org>,
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* [PATCH] Documentation: Clarify better about the rwsem non-owner release issue
From: Joel Fernandes (Google) @ 2020-03-22 2:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, Peter Zijlstra, Sebastian Andrzej Siewior, netdev,
Joel Fernandes (Google), Will Deacon, Thomas Gleixner,
Davidlohr Bueso, Arnd Bergmann, Jonathan Corbet, Ingo Molnar,
Ingo Molnar, Paul E . McKenney, linuxppc-dev, Steven Rostedt,
Bjorn Helgaas, Kurt Schwemmer,
linux-usb@vger.kernel.org Kalle Valo,
linux-pci@vger.kernel.org Felipe Balbi, Logan Gunthorpe,
Randy Dunlap, linux-wireless, Oleg Nesterov, Greg Kroah-Hartman,
Linus Torvalds, David S. Miller
Reword and clarify better about the rwsem non-owner release issue.
Link: https://lore.kernel.org/linux-pci/20200321212144.GA6475@google.com/
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
Documentation/locking/locktypes.rst | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/locking/locktypes.rst b/Documentation/locking/locktypes.rst
index 6f2c0f5b041e..656dce72f11f 100644
--- a/Documentation/locking/locktypes.rst
+++ b/Documentation/locking/locktypes.rst
@@ -292,7 +292,7 @@ implementations to provide priority inheritance for all lock types except
the truly spinning ones. Priority inheritance on ownerless locks is
obviously impossible.
-For now the rwsem non-owner release excludes code which utilizes it from
-being used on PREEMPT_RT enabled kernels. In same cases this can be
-mitigated by disabling portions of the code, in other cases the complete
-functionality has to be disabled until a workable solution has been found.
+For now, a PREEMPT_RT kernel just disables code sections that perform a
+non-owner release of an rwsem. In some cases, parts of the code are disabled.
+In other cases, the complete functionality has to be disabled until a workable
+solution has been found.
--
2.25.1.696.g5e7596f4ac-goog
^ permalink raw reply related
* Re: [patch V2 08/15] Documentation: Add lock ordering and nesting documentation
From: Joel Fernandes @ 2020-03-22 1:36 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Randy Dunlap, Peter Zijlstra, linux-pci,
Sebastian Andrzej Siewior, Oleg Nesterov, Will Deacon,
Ingo Molnar, Davidlohr Bueso, Arnd Bergmann, Logan Gunthorpe,
Paul E . McKenney, linuxppc-dev, Steven Rostedt, Bjorn Helgaas,
Kurt Schwemmer, Kalle Valo, Felipe Balbi, Greg Kroah-Hartman,
linux-usb, linux-wireless, LKML, netdev, Linus Torvalds,
David S. Miller
In-Reply-To: <874kuhqsz3.fsf@nanos.tec.linutronix.de>
On Sat, Mar 21, 2020 at 10:49:04PM +0100, Thomas Gleixner wrote:
[...]
> >> +rwsems have grown interfaces which allow non owner release for special
> >> +purposes. This usage is problematic on PREEMPT_RT because PREEMPT_RT
> >> +substitutes all locking primitives except semaphores with RT-mutex based
> >> +implementations to provide priority inheritance for all lock types except
> >> +the truly spinning ones. Priority inheritance on ownerless locks is
> >> +obviously impossible.
> >> +
> >> +For now the rwsem non-owner release excludes code which utilizes it from
> >> +being used on PREEMPT_RT enabled kernels.
> >
> > I could not parse the last sentence here, but I think you meant "For now,
> > PREEMPT_RT enabled kernels disable code that perform a non-owner release of
> > an rwsem". Correct me if I'm wrong.
>
> Right, that's what I wanted to say :)
>
> Care to send a delta patch?
Absolutely, doing that now. :-)
thanks,
- Joel
^ permalink raw reply
* Re: [patch V2 08/15] Documentation: Add lock ordering and nesting documentation
From: Thomas Gleixner @ 2020-03-21 21:49 UTC (permalink / raw)
To: Joel Fernandes
Cc: Randy Dunlap, Peter Zijlstra, linux-pci,
Sebastian Andrzej Siewior, Oleg Nesterov, Will Deacon,
Ingo Molnar, Davidlohr Bueso, Arnd Bergmann, Logan Gunthorpe,
Paul E . McKenney, linuxppc-dev, Steven Rostedt, Bjorn Helgaas,
Kurt Schwemmer, Kalle Valo, Felipe Balbi, Greg Kroah-Hartman,
linux-usb, linux-wireless, LKML, netdev, Linus Torvalds,
David S. Miller
In-Reply-To: <20200321212144.GA6475@google.com>
Joel Fernandes <joel@joelfernandes.org> writes:
>> +rwlock_t
>> +========
>> +
>> +rwlock_t is a multiple readers and single writer lock mechanism.
>> +
>> +On a non PREEMPT_RT enabled kernel rwlock_t is implemented as a spinning
>> +lock and the suffix rules of spinlock_t apply accordingly. The
>> +implementation is fair and prevents writer starvation.
>>
>
> You mentioned writer starvation, but I think it would be good to also mention
> that rwlock_t on a non-PREEMPT_RT kernel also does not have _reader_
> starvation problem, since it uses queued implementation. This fact is worth
> mentioning here, since further below you explain that an rwlock in PREEMPT_RT
> does have reader starvation problem.
It's worth mentioning. But RT really has only write starvation not
reader starvation.
>> +rwlock_t and PREEMPT_RT
>> +-----------------------
>> +
>> +On a PREEMPT_RT enabled kernel rwlock_t is mapped to a separate
>> +implementation based on rt_mutex which changes the semantics:
>> +
>> + - Same changes as for spinlock_t
>> +
>> + - The implementation is not fair and can cause writer starvation under
>> + certain circumstances. The reason for this is that a writer cannot grant
>> + its priority to multiple readers. Readers which are blocked on a writer
>> + fully support the priority inheritance protocol.
>
> Is it hard to give priority to multiple readers because the number of readers
> to give priority to could be unbounded?
Yes, and it's horribly complex and racy. We had an implemetation years
ago which taught us not to try it again :)
>> +PREEMPT_RT also offers a local_lock mechanism to substitute the
>> +local_irq_disable/save() constructs in cases where a separation of the
>> +interrupt disabling and the locking is really unavoidable. This should be
>> +restricted to very rare cases.
>
> It would also be nice to mention where else local_lock() can be used, such as
> protecting per-cpu variables without disabling preemption. Could we add a
> section on protecting per-cpu data? (Happy to do that and send a patch if you
> prefer).
The local lock section will come soon when we post the local lock
patches again.
>> +rwsems have grown interfaces which allow non owner release for special
>> +purposes. This usage is problematic on PREEMPT_RT because PREEMPT_RT
>> +substitutes all locking primitives except semaphores with RT-mutex based
>> +implementations to provide priority inheritance for all lock types except
>> +the truly spinning ones. Priority inheritance on ownerless locks is
>> +obviously impossible.
>> +
>> +For now the rwsem non-owner release excludes code which utilizes it from
>> +being used on PREEMPT_RT enabled kernels.
>
> I could not parse the last sentence here, but I think you meant "For now,
> PREEMPT_RT enabled kernels disable code that perform a non-owner release of
> an rwsem". Correct me if I'm wrong.
Right, that's what I wanted to say :)
Care to send a delta patch?
Thanks!
tglx
^ permalink raw reply
* Re: [patch V2 08/15] Documentation: Add lock ordering and nesting documentation
From: Joel Fernandes @ 2020-03-21 21:21 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Randy Dunlap, Peter Zijlstra, linux-pci,
Sebastian Andrzej Siewior, Oleg Nesterov, Will Deacon,
Ingo Molnar, Davidlohr Bueso, Arnd Bergmann, Logan Gunthorpe,
Paul E . McKenney, linuxppc-dev, Steven Rostedt, Bjorn Helgaas,
Kurt Schwemmer, Kalle Valo, Felipe Balbi, Greg Kroah-Hartman,
linux-usb, linux-wireless, LKML, netdev, Linus Torvalds,
David S. Miller
In-Reply-To: <20200318204408.211530902@linutronix.de>
Hi Thomas,
Just a few comments:
[...]
> +rtmutex
> +=======
> +
> +RT-mutexes are mutexes with support for priority inheritance (PI).
> +
> +PI has limitations on non PREEMPT_RT enabled kernels due to preemption and
> +interrupt disabled sections.
> +
> +On a PREEMPT_RT enabled kernel most of these sections are fully
> +preemptible. This is possible because PREEMPT_RT forces most executions
> +into task context, especially interrupt handlers and soft interrupts, which
> +allows to substitute spinlock_t and rwlock_t with RT-mutex based
> +implementations.
> +
> +
> +raw_spinlock_t and spinlock_t
> +=============================
> +
> +raw_spinlock_t
> +--------------
> +
> +raw_spinlock_t is a strict spinning lock implementation regardless of the
> +kernel configuration including PREEMPT_RT enabled kernels.
> +
> +raw_spinlock_t is to be used only in real critical core code, low level
> +interrupt handling and places where protecting (hardware) state is required
> +to be safe against preemption and eventually interrupts.
> +
> +Another reason to use raw_spinlock_t is when the critical section is tiny
> +to avoid the overhead of spinlock_t on a PREEMPT_RT enabled kernel in the
> +contended case.
> +
> +spinlock_t
> +----------
> +
> +The semantics of spinlock_t change with the state of CONFIG_PREEMPT_RT.
> +
> +On a non PREEMPT_RT enabled kernel spinlock_t is mapped to raw_spinlock_t
> +and has exactly the same semantics.
> +
> +spinlock_t and PREEMPT_RT
> +-------------------------
> +
> +On a PREEMPT_RT enabled kernel spinlock_t is mapped to a separate
> +implementation based on rt_mutex which changes the semantics:
> +
> + - Preemption is not disabled
> +
> + - The hard interrupt related suffixes for spin_lock / spin_unlock
> + operations (_irq, _irqsave / _irqrestore) do not affect the CPUs
> + interrupt disabled state
> +
> + - The soft interrupt related suffix (_bh()) is still disabling the
> + execution of soft interrupts, but contrary to a non PREEMPT_RT enabled
> + kernel, which utilizes the preemption count, this is achieved by a per
> + CPU bottom half locking mechanism.
> +
> +All other semantics of spinlock_t are preserved:
> +
> + - Migration of tasks which hold a spinlock_t is prevented. On a non
> + PREEMPT_RT enabled kernel this is implicit due to preemption disable.
> + PREEMPT_RT has a separate mechanism to achieve this. This ensures that
> + pointers to per CPU variables stay valid even if the task is preempted.
> +
> + - Task state preservation. The task state is not affected when a lock is
> + contended and the task has to schedule out and wait for the lock to
> + become available. The lock wake up restores the task state unless there
> + was a regular (not lock related) wake up on the task. This ensures that
> + the task state rules are always correct independent of the kernel
> + configuration.
> +
> +rwlock_t
> +========
> +
> +rwlock_t is a multiple readers and single writer lock mechanism.
> +
> +On a non PREEMPT_RT enabled kernel rwlock_t is implemented as a spinning
> +lock and the suffix rules of spinlock_t apply accordingly. The
> +implementation is fair and prevents writer starvation.
>
You mentioned writer starvation, but I think it would be good to also mention
that rwlock_t on a non-PREEMPT_RT kernel also does not have _reader_
starvation problem, since it uses queued implementation. This fact is worth
mentioning here, since further below you explain that an rwlock in PREEMPT_RT
does have reader starvation problem.
> +rwlock_t and PREEMPT_RT
> +-----------------------
> +
> +On a PREEMPT_RT enabled kernel rwlock_t is mapped to a separate
> +implementation based on rt_mutex which changes the semantics:
> +
> + - Same changes as for spinlock_t
> +
> + - The implementation is not fair and can cause writer starvation under
> + certain circumstances. The reason for this is that a writer cannot grant
> + its priority to multiple readers. Readers which are blocked on a writer
> + fully support the priority inheritance protocol.
Is it hard to give priority to multiple readers because the number of readers
to give priority to could be unbounded?
> +
> +
> +PREEMPT_RT caveats
> +==================
> +
> +spinlock_t and rwlock_t
> +-----------------------
> +
> +The substitution of spinlock_t and rwlock_t on PREEMPT_RT enabled kernels
> +with RT-mutex based implementations has a few implications.
> +
> +On a non PREEMPT_RT enabled kernel the following code construct is
> +perfectly fine::
> +
> + local_irq_disable();
> + spin_lock(&lock);
> +
> +and fully equivalent to::
> +
> + spin_lock_irq(&lock);
> +
> +Same applies to rwlock_t and the _irqsave() suffix variant.
> +
> +On a PREEMPT_RT enabled kernel this breaks because the RT-mutex
> +substitution expects a fully preemptible context.
> +
> +The preferred solution is to use :c:func:`spin_lock_irq()` or
> +:c:func:`spin_lock_irqsave()` and their unlock counterparts.
> +
> +PREEMPT_RT also offers a local_lock mechanism to substitute the
> +local_irq_disable/save() constructs in cases where a separation of the
> +interrupt disabling and the locking is really unavoidable. This should be
> +restricted to very rare cases.
It would also be nice to mention where else local_lock() can be used, such as
protecting per-cpu variables without disabling preemption. Could we add a
section on protecting per-cpu data? (Happy to do that and send a patch if you
prefer).
> +raw_spinlock_t
> +--------------
> +
> +Locking of a raw_spinlock_t disables preemption and eventually interrupts.
> +Therefore code inside the critical region has to be careful to avoid calls
> +into code which takes a regular spinlock_t or rwlock_t. A prime example is
> +memory allocation.
> +
> +On a non PREEMPT_RT enabled kernel the following code construct is
> +perfectly fine code::
> +
> + raw_spin_lock(&lock);
> + p = kmalloc(sizeof(*p), GFP_ATOMIC);
> +
> +On a PREEMPT_RT enabled kernel this breaks because the memory allocator is
> +fully preemptible and therefore does not support allocations from truly
> +atomic contexts.
> +
> +Contrary to that the following code construct is perfectly fine on
> +PREEMPT_RT as spin_lock() does not disable preemption::
> +
> + spin_lock(&lock);
> + p = kmalloc(sizeof(*p), GFP_ATOMIC);
> +
> +Most places which use GFP_ATOMIC allocations are safe on PREEMPT_RT as the
> +execution is forced into thread context and the lock substitution is
> +ensuring preemptibility.
> +
> +
> +bit spinlocks
> +-------------
> +
> +Bit spinlocks are problematic for PREEMPT_RT as they cannot be easily
> +substituted by an RT-mutex based implementation for obvious reasons.
> +
> +The semantics of bit spinlocks are preserved on a PREEMPT_RT enabled kernel
> +and the caveats vs. raw_spinlock_t apply.
> +
> +Some bit spinlocks are substituted by regular spinlock_t for PREEMPT_RT but
> +this requires conditional (#ifdef'ed) code changes at the usage side while
> +the spinlock_t substitution is simply done by the compiler and the
> +conditionals are restricted to header files and core implementation of the
> +locking primitives and the usage sites do not require any changes.
> +
> +
> +Lock type nesting rules
> +=======================
> +
> +The most basic rules are:
> +
> + - Lock types of the same lock category (sleeping, spinning) can nest
> + arbitrarily as long as they respect the general lock ordering rules to
> + prevent deadlocks.
> +
> + - Sleeping lock types cannot nest inside spinning lock types.
> +
> + - Spinning lock types can nest inside sleeping lock types.
> +
> +These rules apply in general independent of CONFIG_PREEMPT_RT.
> +
> +As PREEMPT_RT changes the lock category of spinlock_t and rwlock_t from
> +spinning to sleeping this has obviously restrictions how they can nest with
> +raw_spinlock_t.
> +
> +This results in the following nest ordering:
> +
> + 1) Sleeping locks
> + 2) spinlock_t and rwlock_t
> + 3) raw_spinlock_t and bit spinlocks
> +
> +Lockdep is aware of these constraints to ensure that they are respected.
> +
> +
> +Owner semantics
> +===============
> +
> +Most lock types in the Linux kernel have strict owner semantics, i.e. the
> +context (task) which acquires a lock has to release it.
> +
> +There are two exceptions:
> +
> + - semaphores
> + - rwsems
> +
> +semaphores have no strict owner semantics for historical reasons. They are
> +often used for both serialization and waiting purposes. That's generally
> +discouraged and should be replaced by separate serialization and wait
> +mechanisms.
> +
> +rwsems have grown interfaces which allow non owner release for special
> +purposes. This usage is problematic on PREEMPT_RT because PREEMPT_RT
> +substitutes all locking primitives except semaphores with RT-mutex based
> +implementations to provide priority inheritance for all lock types except
> +the truly spinning ones. Priority inheritance on ownerless locks is
> +obviously impossible.
> +
> +For now the rwsem non-owner release excludes code which utilizes it from
> +being used on PREEMPT_RT enabled kernels.
I could not parse the last sentence here, but I think you meant "For now,
PREEMPT_RT enabled kernels disable code that perform a non-owner release of
an rwsem". Correct me if I'm wrong.
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
thanks,
- Joel
> In same cases this can be
> +mitigated by disabling portions of the code, in other cases the complete
> +functionality has to be disabled until a workable solution has been found.
>
^ permalink raw reply
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