Re: [Nouveau] [PATCH v2 1/7] PCI: Restore config space on runtime resume despite being unbound

2018-03-13 Thread Bjorn Helgaas 
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
> From: Rafael J. Wysocki 
> 
> We leave PCI devices not bound to a driver in D0 during runtime suspend.
> But they may have a parent which is bound and can be transitioned to
> D3cold at runtime.  Once the parent goes to D3cold, the unbound child
> may go to D3cold as well.  When the child comes out of D3cold, its BARs
> are uninitialized and thus inaccessible when a driver tries to probe.

There's no clear way to tell whether a BAR is uninitialized.  At
power-up, the writable bits will be zero, which is a valid BAR value.
If enabled in PCI_COMMAND, the BAR is accessible and may conflict with
other devices.

Possible alternate wording:

  When the child goes to D3cold, its internal state, including
  configuration of BARs, MSI, ASPM, MPS, etc., is lost.

> Moreover configuration done during enumeration, e.g. ASPM and MPS, will
> be lost.
> 
> One example are recent hybrid graphics laptops which cut power to the
> discrete GPU when the root port above it goes to ACPI power state D3.
> Users may provoke this by unbinding the GPU driver and allowing runtime
> PM on the GPU via sysfs:  The PM core will then treat the GPU as
> "suspended", which in turn allows the root port to runtime suspend,
> causing the power resources listed in its _PR3 object to be powered off.
> The GPU's BARs will be uninitialized when a driver later probes it.
> 
> Another example are hybrid graphics laptops where the GPU itself (rather
> than the root port) is capable of runtime suspending to D3cold.  If the
> GPU's integrated HDA controller is not bound and the GPU's driver
> decides to runtime suspend to D3cold, the HDA controller's BARs will be
> uninitialized when a driver later probes it.
> 
> Fix by saving and restoring config space over a runtime suspend cycle
> even if the device is not bound.
> 
> Cc: Bjorn Helgaas 
> Signed-off-by: Rafael J. Wysocki 
> [lukas: add commit message, bikeshed code comments for clarity]
> Signed-off-by: Lukas Wunner 

Acked-by: Bjorn Helgaas 

> ---
> Changes since v1:
> - Replace patch to use pci_save_state() / pci_restore_state()
>   for consistency between runtime PM code path of bound and unbound
>   devices. (Rafael, Bjorn)
> 
>  drivers/pci/pci-driver.c | 17 +++--
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3bed6beda051..6a67cdbd0e6a 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1224,11 +1224,14 @@ static int pci_pm_runtime_suspend(struct device *dev)
>   int error;
>  
>   /*
> -  * If pci_dev->driver is not set (unbound), the device should
> -  * always remain in D0 regardless of the runtime PM status
> +  * If pci_dev->driver is not set (unbound), we leave the device in D0,
> +  * but it may go to D3cold when the bridge above it runtime suspends.
> +  * Save its config space in case that happens.

Thanks for this clarification.

>*/
> - if (!pci_dev->driver)
> + if (!pci_dev->driver) {
> + pci_save_state(pci_dev);
>   return 0;
> + }
>  
>   if (!pm || !pm->runtime_suspend)
>   return -ENOSYS;
> @@ -1276,16 +1279,18 @@ static int pci_pm_runtime_resume(struct device *dev)
>   const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
>   /*
> -  * If pci_dev->driver is not set (unbound), the device should
> -  * always remain in D0 regardless of the runtime PM status
> +  * Restoring config space is necessary even if the device is not bound
> +  * to a driver because although we left it in D0, it may have gone to
> +  * D3cold when the bridge above it runtime suspended.
>*/
> + pci_restore_standard_config(pci_dev);
> +
>   if (!pci_dev->driver)
>   return 0;
>  
>   if (!pm || !pm->runtime_resume)
>   return -ENOSYS;
>  
> - pci_restore_standard_config(pci_dev);
>   pci_fixup_device(pci_fixup_resume_early, pci_dev);
>   pci_enable_wake(pci_dev, PCI_D0, false);
>   pci_fixup_device(pci_fixup_resume, pci_dev);
> -- 
> 2.15.1
> 
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [RFC PATCH 00/13] SVM (share virtual memory) with HMM in nouveau

2018-03-13 Thread Jerome Glisse 
On Mon, Mar 12, 2018 at 11:14:47PM -0700, John Hubbard wrote:
> On 03/12/2018 10:50 AM, Jerome Glisse wrote:

[...]

> Yes, on NVIDIA GPUs, the Host/FIFO unit is limited to 40-bit addresses, so
> things such as the following need to be below (1 << 40), and also accessible 
> to both CPU (user space) and GPU hardware. 
> -- command buffers (CPU user space driver fills them, GPU consumes them), 
> -- semaphores (here, a GPU-centric term, rather than OS-type: these are
>memory locations that, for example, the GPU hardware might write to, in
>order to indicate work completion; there are other uses as well), 
> -- a few other things most likely (this is not a complete list).
> 
> So what I'd tentatively expect that to translate into in the driver stack is, 
> approximately:
> 
> -- User space driver code mmap's an area below (1 << 40). It's hard to 
> avoid this,
>given that user space needs access to the area (for filling out command
>buffers and monitoring semaphores, that sort of thing). Then 
> suballocate
>from there using mmap's MAP_FIXED or (future-ish) MAP_FIXED_SAFE flags.
> 
>...glancing at the other fork of this thread, I think that is exactly 
> what
>Felix is saying, too. So that's good.
> 
> -- The user space program sits above the user space driver, and although 
> the
>program could, in theory, interfere with this mmap'd area, that would 
> be
>wrong in the same way that mucking around with malloc'd areas (outside 
> of
>malloc() itself) is wrong. So I don't see any particular need to do 
> much
>more than the above.

I am worried that rogue program (i am not worried about buggy program
if people shoot themself in the foot they should feel the pain) could
use that to abuse channel to do something harmful. I am not familiar
enough with the hardware to completely rule out such scenario.

I do believe hardware with userspace queue support have the necessary
boundary to keep thing secure as i would assume for those the hardware
engineers had to take security into consideration.

Note that in my patchset the code that monitor the special vma is small
something like 20lines of code that only get call if something happen
to the reserved area. So i believe it is worth having such thing, cost
is low for little extra peace of mind :)

Cheers,
Jérôme
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [Bug 105174] [GF108][Regression] Unable to handle NULL pointer dereference in nouveau_mem_host since kernel 4.15.3

2018-03-13 Thread bugzilla-daemon 
https://bugs.freedesktop.org/show_bug.cgi?id=105174

Gabriel M. Elder  changed:

   What|Removed |Added

   See Also||https://bugzilla.redhat.com
   ||/show_bug.cgi?id=1551401

-- 
You are receiving this mail because:
You are the assignee for the bug.___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [Bug 105174] [GF108][Regression] Unable to handle NULL pointer dereference in nouveau_mem_host since kernel 4.15.3

2018-03-13 Thread bugzilla-daemon 
https://bugs.freedesktop.org/show_bug.cgi?id=105174

Gabriel M. Elder  changed:

   What|Removed |Added

 CC||gabr...@tekgnowsys.com

--- Comment #12 from Gabriel M. Elder  ---
See also https://bugzilla.redhat.com/show_bug.cgi?id=1551401>https://bugzilla.redhat.com/show_bug.cgi?id=1551401

-- 
You are receiving this mail because:
You are the assignee for the bug.___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [RFC PATCH 00/13] SVM (share virtual memory) with HMM in nouveau

2018-03-13 Thread Jerome Glisse 
On Tue, Mar 13, 2018 at 06:29:40AM -0700, Matthew Wilcox wrote:
> On Mon, Mar 12, 2018 at 11:14:47PM -0700, John Hubbard wrote:
> > Yes, on NVIDIA GPUs, the Host/FIFO unit is limited to 40-bit addresses, so
> > things such as the following need to be below (1 << 40), and also 
> > accessible 
> > to both CPU (user space) and GPU hardware. 
> > -- command buffers (CPU user space driver fills them, GPU consumes 
> > them), 
> > -- semaphores (here, a GPU-centric term, rather than OS-type: these are
> >memory locations that, for example, the GPU hardware might write to, 
> > in
> >order to indicate work completion; there are other uses as well), 
> > -- a few other things most likely (this is not a complete list).
> 
> Is that a 40-bit virtual address limit or physical address limit?  I'm
> no longer sure who is addressing what memory through what mechanism ;-)
> 

Virtual address limit, those object get mapped into GPU page table but
the register/structure fields where you program those object's address
only are 32bits (the virtual address is shifted by 8bits for alignment).

Cheers,
Jérôme
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [RFC PATCH 00/13] SVM (share virtual memory) with HMM in nouveau

2018-03-13 Thread Jerome Glisse 
On Mon, Mar 12, 2018 at 02:28:42PM -0400, Felix Kuehling wrote:
> On 2018-03-10 10:01 AM, Christian König wrote:
> >> To accomodate those we need to
> >> create a "hole" inside the process address space. This patchset have
> >> a hack for that (patch 13 HACK FOR HMM AREA), it reserves a range of
> >> device file offset so that process can mmap this range with PROT_NONE
> >> to create a hole (process must make sure the hole is below 1 << 40).
> >> I feel un-easy of doing it this way but maybe it is ok with other
> >> folks.
> >
> > Well we have essentially the same problem with pre gfx9 AMD hardware.
> > Felix might have some advise how it was solved for HSA. 
> 
> For pre-gfx9 hardware we reserve address space in user mode using a big
> mmap PROT_NONE call at application start. Then we manage the address
> space in user mode and use MAP_FIXED to map buffers at specific
> addresses within the reserved range.
> 
> The big address space reservation causes issues for some debugging tools
> (clang-sanitizer was mentioned to me), so with gfx9 we're going to get
> rid of this address space reservation.

What do you need those mapping for ? What kind of object (pm4 packet
command buffer, GPU semaphore | fence, ...) ? Kernel private object ?
On nv we need it for the main command buffer ring which we do not want
to expose to application.

Thus for nv gpu we need kernel to monitor this PROT_NONE region to make
sure that i never got unmapped, resize, move ... this is me fearing a
rogue userspace that munmap and try to abuse some bug in SVM/GPU driver
to abuse object map behind those fix mapping.

Cheers,
Jérôme
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [RFC PATCH 00/13] SVM (share virtual memory) with HMM in nouveau

2018-03-13 Thread Daniel Vetter 
On Mon, Mar 12, 2018 at 01:50:58PM -0400, Jerome Glisse wrote:
> On Mon, Mar 12, 2018 at 06:30:09PM +0100, Daniel Vetter wrote:
> > On Sat, Mar 10, 2018 at 04:01:58PM +0100, Christian K??nig wrote:
> 
> [...]
> 
> > > > They are work underway to revamp nouveau channel creation with a new
> > > > userspace API. So we might want to delay upstreaming until this lands.
> > > > We can stil discuss one aspect specific to HMM here namely the issue
> > > > around GEM objects used for some specific part of the GPU. Some engine
> > > > inside the GPU (engine are a GPU block like the display block which
> > > > is responsible of scaning memory to send out a picture through some
> > > > connector for instance HDMI or DisplayPort) can only access memory
> > > > with virtual address below (1 << 40). To accomodate those we need to
> > > > create a "hole" inside the process address space. This patchset have
> > > > a hack for that (patch 13 HACK FOR HMM AREA), it reserves a range of
> > > > device file offset so that process can mmap this range with PROT_NONE
> > > > to create a hole (process must make sure the hole is below 1 << 40).
> > > > I feel un-easy of doing it this way but maybe it is ok with other
> > > > folks.
> > > 
> > > Well we have essentially the same problem with pre gfx9 AMD hardware. 
> > > Felix
> > > might have some advise how it was solved for HSA.
> > 
> > Couldn't we do an in-kernel address space for those special gpu blocks? As
> > long as it's display the kernel needs to manage it anyway, and adding a
> > 2nd mapping when you pin/unpin for scanout usage shouldn't really matter
> > (as long as you cache the mapping until the buffer gets thrown out of
> > vram). More-or-less what we do for i915 (where we have an entirely
> > separate address space for these things which is 4G on the latest chips).
> > -Daniel
> 
> We can not do an in-kernel address space for those. We already have an
> in kernel address space but it does not apply for the object considered
> here.
> 
> For NVidia (i believe this is the same for AMD AFAIK) the objects we
> are talking about are objects that must be in the same address space
> as the one against which process's shader/dma/... get executed.
> 
> For instance command buffer submited by userspace must be inside a
> GEM object mapped inside the GPU's process address against which the
> command are executed. My understanding is that the PFIFO (the engine
> on nv GPU that fetch commands) first context switch to address space
> associated with the channel and then starts fetching commands with
> all address being interpreted against the channel address space.
> 
> Hence why we need to reserve some range in the process virtual address
> space if we want to do SVM in a sane way. I mean we could just map
> buffer into GPU page table and then cross fingers and toes hopping that
> the process will never get any of its mmap overlapping those mapping :)

Ah, from the example I got the impression it's just the display engine
that has this restriction. CS/PFIFO having the same restriction is indeed
more fun.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [RFC PATCH 00/13] SVM (share virtual memory) with HMM in nouveau

2018-03-13 Thread John Hubbard 
On 03/12/2018 10:50 AM, Jerome Glisse wrote:
> On Mon, Mar 12, 2018 at 06:30:09PM +0100, Daniel Vetter wrote:
>> On Sat, Mar 10, 2018 at 04:01:58PM +0100, Christian K??nig wrote:
> 
> [...]
> 
 They are work underway to revamp nouveau channel creation with a new
 userspace API. So we might want to delay upstreaming until this lands.
 We can stil discuss one aspect specific to HMM here namely the issue
 around GEM objects used for some specific part of the GPU. Some engine
 inside the GPU (engine are a GPU block like the display block which
 is responsible of scaning memory to send out a picture through some
 connector for instance HDMI or DisplayPort) can only access memory
 with virtual address below (1 << 40). To accomodate those we need to
 create a "hole" inside the process address space. This patchset have
 a hack for that (patch 13 HACK FOR HMM AREA), it reserves a range of
 device file offset so that process can mmap this range with PROT_NONE
 to create a hole (process must make sure the hole is below 1 << 40).
 I feel un-easy of doing it this way but maybe it is ok with other
 folks.
>>>
>>> Well we have essentially the same problem with pre gfx9 AMD hardware. Felix
>>> might have some advise how it was solved for HSA.
>>
>> Couldn't we do an in-kernel address space for those special gpu blocks? As
>> long as it's display the kernel needs to manage it anyway, and adding a
>> 2nd mapping when you pin/unpin for scanout usage shouldn't really matter
>> (as long as you cache the mapping until the buffer gets thrown out of
>> vram). More-or-less what we do for i915 (where we have an entirely
>> separate address space for these things which is 4G on the latest chips).
>> -Daniel
> 
> We can not do an in-kernel address space for those. We already have an
> in kernel address space but it does not apply for the object considered
> here.
> 
> For NVidia (i believe this is the same for AMD AFAIK) the objects we
> are talking about are objects that must be in the same address space
> as the one against which process's shader/dma/... get executed.
> 
> For instance command buffer submited by userspace must be inside a
> GEM object mapped inside the GPU's process address against which the
> command are executed. My understanding is that the PFIFO (the engine
> on nv GPU that fetch commands) first context switch to address space
> associated with the channel and then starts fetching commands with
> all address being interpreted against the channel address space.
> 
> Hence why we need to reserve some range in the process virtual address
> space if we want to do SVM in a sane way. I mean we could just map
> buffer into GPU page table and then cross fingers and toes hopping that
> the process will never get any of its mmap overlapping those mapping :)
> 
> Cheers,
> Jérôme
> 

Hi Jerome and all,

Yes, on NVIDIA GPUs, the Host/FIFO unit is limited to 40-bit addresses, so
things such as the following need to be below (1 << 40), and also accessible 
to both CPU (user space) and GPU hardware. 
-- command buffers (CPU user space driver fills them, GPU consumes them), 
-- semaphores (here, a GPU-centric term, rather than OS-type: these are
   memory locations that, for example, the GPU hardware might write to, in
   order to indicate work completion; there are other uses as well), 
-- a few other things most likely (this is not a complete list).

So what I'd tentatively expect that to translate into in the driver stack is, 
approximately:

-- User space driver code mmap's an area below (1 << 40). It's hard to 
avoid this,
   given that user space needs access to the area (for filling out command
   buffers and monitoring semaphores, that sort of thing). Then suballocate
   from there using mmap's MAP_FIXED or (future-ish) MAP_FIXED_SAFE flags.

   ...glancing at the other fork of this thread, I think that is exactly 
what
   Felix is saying, too. So that's good.

-- The user space program sits above the user space driver, and although the
   program could, in theory, interfere with this mmap'd area, that would be
   wrong in the same way that mucking around with malloc'd areas (outside of
   malloc() itself) is wrong. So I don't see any particular need to do much
   more than the above.

thanks,
-- 
John Hubbard
NVIDIA
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau