Re: Remapping BOs and registers post BAR movements [was - Re: Question about supporting AMD eGPU hot plug case]

[Christian König <christian.koenig@xxxxxxx>]

Hi Andrey,

Am 03.03.21 um 17:05 schrieb Andrey Grodzovsky:
> Ping
>
> Andrey
>
> On 2021-03-02 12:48 p.m., Andrey Grodzovsky wrote:
> > Adding PCI mailing list per Bjorn's advise (sorry, skipped the 
> > address last time).
> >
> > On 2021-03-02 12:22 p.m., Christian König wrote:
> > > Hi Andrey,
> > >
> > > > Can you elaborate more why you need such a lock on a global level 
> > > > and not
> > > > enough per driver (per device more specifically) local lock 
> > > > acquired in
> > > > rescan_preapare (before BARs shuffling starts) and released in 
> > > > rescan_done
> > > > (after BARs shuffling done) ? 
> > >
> > > because this adds inter device dependencies.
> > >
> > > E.g. when we have device A, B and C and each is taking it's own lock 
> > > we end up with 3 locks.
> > >
> > > If those device have inter dependencies on their own, for example 
> > > because of DMA-buf we will end up in quite a locking mess.
> > >
> > > Apart from that if we don't always keep the order A, B, C but 
> > > instead end up with A, C, B lockdep will certainly start to complain.
> > >
> > > Regards,
> > > Christian.
> >
> > But as far as I understand those locks are agnostic to each other - 
> > each lock is taken only by it's respective device and not by others.

Well they should be agnostic to each other, but are they really? I mean 
we could have inter device lock dependencies because of DMA-buf or good 
knows what.

> > So I don't see deadlock danger here but, I do suspect lockdep might 
> > throw a false warning...

Yeah and disabling or ignoring the lockdep warning could cause problems 
because we don't see issues caused by inter device lock dependencies any 
more.

I just have the feeling that this would cause a lot of problems.

Christian.

> > Andrey
> >
> > > Am 02.03.21 um 18:16 schrieb Andrey Grodzovsky:
> > > > Per Bjorn's advise adding PCI mailing list.
> > > >
> > > > Christian, please reply on top of this mail and not on top the 
> > > > earlier, original one.
> > > >
> > > > Andrey
> > > >
> > > > On 2021-03-02 11:49 a.m., Bjorn Helgaas wrote:
> > > > > Please add linux-pci@xxxxxxxxxxxxxxx to the cc list.  There's a 
> > > > > lot of
> > > > > useful context in this thread and it needs to be archived.
> > > > > On Tue, Mar 02, 2021 at 11:37:25AM -0500, Andrey Grodzovsky wrote:
> > > > > > Adding Sergei and Bjorn since you are suggesting some PCI related 
> > > > > > changes.
> > > > > >
> > > > > > On 2021-03-02 6:28 a.m., Christian König wrote:
> > > > > > > Yeah, I'm thinking about that as well for quite a while now.
> > > > > > >
> > > > > > > I'm not 100% sure what Sergei does in his patch set will work 
> > > > > > > for us. He
> > > > > > > basically tells the driver to stop any processing then shuffle 
> > > > > > > around
> > > > > > > the BARs and then tells him to start again.
> > > > > > >
> > > > > > > Because of the necessity to reprogram bridges there isn't much 
> > > > > > > other way
> > > > > > > how to do this, but we would need something like taking a lock,
> > > > > > > returning and dropping the lock later on. If you do this for 
> > > > > > > multiple
> > > > > > > drivers at the same time this results in a locking nightmare.
> > > > > > >
> > > > > > > What we need instead is a upper level PCI layer rw-lock which we 
> > > > > > > can
> > > > > > > take on the read side in the driver, similar to our internal 
> > > > > > > reset lock.
> > > > > >
> > > > > >
> > > > > > Can you elaborate more why you need such a lock on a global level 
> > > > > > and not
> > > > > > enough per driver (per device more specifically) local lock 
> > > > > > acquired in
> > > > > > rescan_preapare (before BARs shuffling starts) and released in 
> > > > > > rescan_done
> > > > > > (after BARs shuffling done) ?
> > > > > >
> > > > > > Andrey
> > > > > >
> > > > > >
> > > > > > > Wiring up changing the mapping is the smaller problem in that 
> > > > > > > picture.
> > > > > > >
> > > > > > > Christian.
> > > > > > >
> > > > > > > Am 01.03.21 um 22:14 schrieb Andrey Grodzovsky:
> > > > > > > > Hi, I started looking into actual implementation for this, as
> > > > > > > > reference point I am using Sege's NVME implementations (see 
> > > > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FYADRO-KNS%2Flinux%2Fcommit%2F58c375df086502538706ee23e8ef7c8bb5a4178f&data=04%7C01%7Candrey.grodzovsky%40amd.com%7C44bc68a68feb4b71836c08d8dd9b306a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637503005898302093%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=nuTptAjGewG2T7tXOQr2h%2Bdr42lyg3m1%2Bfv0mJzd8tw%3D&reserved=0) 
> > > > > > > >
> > > > > > > > and our own amdgpu_device_resize_fb_bar. Our use case being more
> > > > > > > > complicated then NVME's arises some questions or thoughts:
> > > > > > > >
> > > > > > > > Before the PCI core will move the BARs (VRAM and registers) we 
> > > > > > > > have
> > > > > > > > to stop the HW and SW and then unmap all MMIO mapped entitles 
> > > > > > > > in the
> > > > > > > > driver. This includes, registers of all types and VRAM placed BOs
> > > > > > > > with CPU visibility. My concern is how to prevent accesses to all
> > > > > > > > MMIO ranges between iounmap in rescan_preapare and ioremap in
> > > > > > > > rescan_done. For register accesses we are again back to same issue
> > > > > > > > we had with device unplug and GPU reset we are discussing with 
> > > > > > > > Denis
> > > > > > > > now. So here at least as first solution just to confirm the 
> > > > > > > > feature
> > > > > > > > works I can introduce low level (in register accessors) RW locking
> > > > > > > > to avoid access until all register access driver pointers are
> > > > > > > > remapped post BARs move. What more concerns me is how to - In
> > > > > > > > rescan_preapare: collect all CPU accessible BOs placed in VRAM (i
> > > > > > > > can use amdgpu specific list like I currently use for device
> > > > > > > > unplug), lock them to prevent any further use of them, unpin them
> > > > > > > > (and unmap). In rescan_preapare: remap them back and unlock 
> > > > > > > > them for
> > > > > > > > further use. Also, there is a difference between kernel BOs where
> > > > > > > > indeed iounmap, ioremap should be enough and user space mapped BOs
> > > > > > > > where it seems like in hot unplug, we need in rescan_preapare to
> > > > > > > > only unamp them, in between drop new page faults with 
> > > > > > > > VM_FAULT_RETRY
> > > > > > > > and post rescan_done allow page faults to go through to refill all
> > > > > > > > the user page tables with updated MMIO addresses.
> > > > > > > >
> > > > > > > > Let me know your thoughts.
> > > > > > > >
> > > > > > > > Andrey
> > > > > > > >
> > > > > > > > On 2021-02-26 4:03 p.m., Andrey Grodzovsky wrote:
> > > > > > > > > On 2021-02-26 1:44 a.m., Sergei Miroshnichenko wrote:
> > > > > > > > > > On Thu, 2021-02-25 at 13:28 -0500, Andrey Grodzovsky wrote:
> > > > > > > > > > > On 2021-02-25 2:00 a.m., Sergei Miroshnichenko wrote:
> > > > > > > > > > > > On Wed, 2021-02-24 at 17:51 -0500, Andrey Grodzovsky wrote:
> > > > > > > > > > > > > On 2021-02-24 1:23 p.m., Sergei Miroshnichenko wrote:
> > > > > > > > > > > > > > ...
> > > > > > > > > > > > > Are you saying that even without hot-plugging, while both 
> > > > > > > > > > > > > nvme
> > > > > > > > > > > > > and
> > > > > > > > > > > > > AMD
> > > > > > > > > > > > > card are present
> > > > > > > > > > > > > right from boot, you still get BARs moving and MMIO ranges
> > > > > > > > > > > > > reassigned
> > > > > > > > > > > > > for NVME BARs
> > > > > > > > > > > > > just because amdgpu driver will start resize of AMD card 
> > > > > > > > > > > > > BARs and
> > > > > > > > > > > > > this
> > > > > > > > > > > > > will trigger NVMEs BARs move to
> > > > > > > > > > > > > allow AMD card BARs to cover full range of VIDEO RAM ?
> > > > > > > > > > > > Yes. Unconditionally, because it is unknown beforehand if 
> > > > > > > > > > > > NVMe's
> > > > > > > > > > > > BAR
> > > > > > > > > > > > movement will help. In this particular case BAR movement is 
> > > > > > > > > > > > not
> > > > > > > > > > > > needed,
> > > > > > > > > > > > but is done anyway.
> > > > > > > > > > > >
> > > > > > > > > > > > BARs are not moved one by one, but the kernel releases all the
> > > > > > > > > > > > releasable ones, and then recalculates a new BAR layout to 
> > > > > > > > > > > > fit them
> > > > > > > > > > > > all. Kernel's algorithm is different from BIOS's, so NVME has
> > > > > > > > > > > > appeared
> > > > > > > > > > > > at a new place.
> > > > > > > > > > > >
> > > > > > > > > > > > This is triggered by following:
> > > > > > > > > > > > - at boot, if BIOS had assigned not every BAR;
> > > > > > > > > > > > - during pci_resize_resource();
> > > > > > > > > > > > - during pci_rescan_bus() -- after a pciehp event or a 
> > > > > > > > > > > > manual via
> > > > > > > > > > > > sysfs.
> > > > > > > > > > > By manual via sysfs you mean something like this - 'echo 1 >
> > > > > > > > > > > /sys/bus/pci/drivers/amdgpu/0000\:0c\:00.0/remove && echo 1 >
> > > > > > > > > > > /sys/bus/pci/rescan ' ? I am looking into how most reliably 
> > > > > > > > > > > trigger
> > > > > > > > > > > PCI
> > > > > > > > > > > code to call my callbacks even without having external PCI 
> > > > > > > > > > > cage for
> > > > > > > > > > > GPU
> > > > > > > > > > > (will take me some time to get it).
> > > > > > > > > > Yeah, this is our way to go when a device can't be physically 
> > > > > > > > > > removed
> > > > > > > > > > or unpowered remotely. With just a bit shorter path:
> > > > > > > > > >
> > > > > > > > > >     sudo sh -c 'echo 1 > 
> > > > > > > > > > /sys/bus/pci/devices/0000\:0c\:00.0/remove'
> > > > > > > > > >     sudo sh -c 'echo 1 > /sys/bus/pci/rescan'
> > > > > > > > > >
> > > > > > > > > > Or, just a second command (rescan) is enough: a BAR movement 
> > > > > > > > > > attempt
> > > > > > > > > > will be triggered even if there were no changes in PCI topology.
> > > > > > > > > >
> > > > > > > > > > Serge
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Ok, able to trigger stub callbacks call after doing rescan from
> > > > > > > > > sysfs. Also I see BARs movement
> > > > > > > > >
> > > > > > > > > [ 1860.968036] amdgpu 0000:09:00.0: BAR 0: assigned [mem
> > > > > > > > > 0xc0000000-0xcfffffff 64bit pref]
> > > > > > > > > [ 1860.968050] amdgpu 0000:09:00.0: BAR 0 updated: 0xe0000000 ->
> > > > > > > > > 0xc0000000
> > > > > > > > > [ 1860.968054] amdgpu 0000:09:00.0: BAR 2: assigned [mem
> > > > > > > > > 0xd0000000-0xd01fffff 64bit pref]
> > > > > > > > > [ 1860.968068] amdgpu 0000:09:00.0: BAR 2 updated: 0xf0000000 ->
> > > > > > > > > 0xd0000000
> > > > > > > > > [ 1860.968071] amdgpu 0000:09:00.0: BAR 5: assigned [mem
> > > > > > > > > 0xf0100000-0xf013ffff]
> > > > > > > > > [ 1860.968078] amdgpu 0000:09:00.0: BAR 5 updated: 0xfce00000 ->
> > > > > > > > > 0xf0100000
> > > > > > > > >
> > > > > > > > > As expected, after the move the device becomes unresponsive as
> > > > > > > > > unmap/ioremap wasn't done.
> > > > > > > > >
> > > > > > > > > Andrey