Re: [PATCH] drm/amdgpu: capture invalid hardware access v2

[Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>]

On 2021-03-09 1:51 p.m., Christian König wrote:
> Am 09.03.21 um 19:26 schrieb Andrey Grodzovsky:
> > On 2021-03-09 12:47 p.m., Christian König wrote:
> > > No it won't. Accessing the hardware without the lock is ok as long as 
> > > the write side isn't taken.
> >
> > Oh, forgot about the trylock part, sorry...
> >
> > > But that approach is illegal anyway because we suspend the hardware 
> > > without proper protection from concurrent access.
> >
> > For my understanding and from looking again at his steps related to this
> >
> > Step 0: atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) - [AG] protects from 
> > other TDR threads
> >
> > Step 1: cancel all delay works, stop drm schedule, complete all 
> > unreceived fences and so on. Call amdgpu_device_pre_asic_reset... e.t.c
> > - [AG] this is the HW suspend part
> >
> > Step 2: call down_write(&adev->reset_sem) to hold write lock, which will
> > block recovery thread until other threads release read locks.
> >
> > Is the problem here that HW is suspended while some other threads
> > that rely on the read side lock still access HW ?
>
> Exactly that, yes.
>
> > Mostly what I am
> > thinking about are IOCTls - we can't 'wait for them to complete' but
> > they might be accessing HW when we start suspend.
>
> Well, the bigger problem with the IOCTLs is that I can't seem to be able 
> to properly explain the dependency here.
>
> Maybe let me describe it from the other side once more to make it more 
> understandable:
>
> Core Linux memory management depends on waiting for dma_fences. Amdgpus 
> dma_fences in turn depend on the GPU scheduler and recovery/reset 
> functionality for proper functionality.
>
> So every call to kmalloc() or get_free_page() depends on the amdgpu GPU 
> recover/reset function and with it every lock taken in that function.
>
> Because of this you can't take any lock under which memory allocation 
> might happen while holding the reset lock.
>
> This is a hard and by now very well documented limitation and Dennis 
> changes here doesn't magically remove that.
>
> Because of this taking the reset lock or any other lock used in the GPU 
> reset functionality at the beginning of our IOCTLs will never work 
> correctly.
>
> What you need to do is to have all locks which depend on MM taken before 
> your take the reset lock. E.g. dma_resv, mmap_sem, VM locks etc etc.. As 
> well as not call functions like kmalloc, get_free_page, dma_fence_wait 
> etc etc
>
> I hope that somehow makes it more clear now. We should probably try to 
> merge Daniels patch set for teaching lockdep to complain about such 
> things sooner or later.

It is now more clear then in the previous discussion. One
thing that would be helpful is also to see the kernel documentation
about this limitation - can you maybe point in kernel DOC or even in
code to such documentation ?

Andrey


> With those patches merged lockdep starts to complain about the problems 
> as soon as you try to run a kernel with that enabled.
>
> Regards,
> Christian.
>
> > Andrey
> >
> >
> > > Christian.
> > >
> > > Am 09.03.21 um 17:40 schrieb Andrey Grodzovsky:
> > > > Because he takes the write side lock post amdgpu_pre_asic_reset - 
> > > > where HW suspend sequence happens (touching registers) - so i think 
> > > > it will assert.
> > > >
> > > > Andrey
> > > >
> > > > On 2021-03-09 7:56 a.m., Christian König wrote:
> > > > > Hi Dennis,
> > > > >
> > > > > why do you think that this will always assert in reset thread?
> > > > >
> > > > > In the reset thread while we are holding the reset lock write side 
> > > > > lockdep_assert_held() should be satisfied and not cause any splat 
> > > > > in the system log.
> > > > >
> > > > > Regards,
> > > > > Christian.
> > > > >
> > > > > Am 09.03.21 um 03:03 schrieb Li, Dennis:
> > > > > > [AMD Official Use Only - Internal Distribution Only]
> > > > > >
> > > > > > Hi, Christian,
> > > > > >         amdgpu_device_skip_hw_access will always assert in reset 
> > > > > > thread, which seems not a good idea.
> > > > > >
> > > > > > Best Regards
> > > > > > Dennis Li
> > > > > > -----Original Message-----
> > > > > > From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx>
> > > > > > Sent: Tuesday, March 9, 2021 2:07 AM
> > > > > > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > > Cc: Grodzovsky, Andrey <Andrey.Grodzovsky@xxxxxxx>; Li, Dennis 
> > > > > > <Dennis.Li@xxxxxxx>
> > > > > > Subject: [PATCH] drm/amdgpu: capture invalid hardware access v2
> > > > > >
> > > > > > From: Dennis Li <Dennis.Li@xxxxxxx>
> > > > > >
> > > > > > When recovery thread has begun GPU reset, there should be not 
> > > > > > other threads to access hardware, otherwise system randomly hang.
> > > > > >
> > > > > > v2 (chk): rewritten from scratch, use trylock and lockdep instead of
> > > > > >    hand wiring the logic.
> > > > > >
> > > > > > Signed-off-by: Dennis Li <Dennis.Li@xxxxxxx>
> > > > > > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > > > > > ---
> > > > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 74 
> > > > > > +++++++++++++++++-----
> > > > > >   1 file changed, 57 insertions(+), 17 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > index e247c3a2ec08..c990af6a43ca 100644
> > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > > > > @@ -326,6 +326,34 @@ void amdgpu_device_vram_access(struct 
> > > > > > amdgpu_device *adev, loff_t pos,
> > > > > >   /*
> > > > > >    * register access helper functions.
> > > > > >    */
> > > > > > +
> > > > > > +/* Check if hw access should be skipped because of hotplug or device
> > > > > > +error */ static bool amdgpu_device_skip_hw_access(struct 
> > > > > > amdgpu_device
> > > > > > +*adev) {
> > > > > > +if (adev->in_pci_err_recovery)
> > > > > > +return true;
> > > > > > +
> > > > > > +#ifdef CONFIG_LOCKDEP
> > > > > > +/*
> > > > > > + * This is a bit complicated to understand, so worth a comment. 
> > > > > > What we assert
> > > > > > + * here is that the GPU reset is not running on another thread in 
> > > > > > parallel.
> > > > > > + *
> > > > > > + * For this we trylock the read side of the reset semaphore, if 
> > > > > > that succeeds
> > > > > > + * we know that the reset is not running in paralell.
> > > > > > + *
> > > > > > + * If the trylock fails we assert that we are either already 
> > > > > > holding the read
> > > > > > + * side of the lock or are the reset thread itself and hold the 
> > > > > > write side of
> > > > > > + * the lock.
> > > > > > + */
> > > > > > +if (down_read_trylock(&adev->reset_sem))
> > > > > > +up_read(&adev->reset_sem);
> > > > > > +else
> > > > > > +lockdep_assert_held(&adev->reset_sem);
> > > > > > +#endif
> > > > > > +
> > > > > > +return false;
> > > > > > +}
> > > > > > +
> > > > > >   /**
> > > > > >    * amdgpu_device_rreg - read a memory mapped IO or indirect 
> > > > > > register
> > > > > >    *
> > > > > > @@ -340,7 +368,7 @@ uint32_t amdgpu_device_rreg(struct 
> > > > > > amdgpu_device *adev,  {
> > > > > >   uint32_t ret;
> > > > > >
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return 0;
> > > > > >
> > > > > >   if ((reg * 4) < adev->rmmio_size) {
> > > > > > @@ -377,7 +405,7 @@ uint32_t amdgpu_device_rreg(struct 
> > > > > > amdgpu_device *adev,
> > > > > >    */
> > > > > >   uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev, uint32_t 
> > > > > > offset)  {
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return 0;
> > > > > >
> > > > > >   if (offset < adev->rmmio_size)
> > > > > > @@ -402,7 +430,7 @@ uint8_t amdgpu_mm_rreg8(struct amdgpu_device 
> > > > > > *adev, uint32_t offset)
> > > > > >    */
> > > > > >   void amdgpu_mm_wreg8(struct amdgpu_device *adev, uint32_t 
> > > > > > offset, uint8_t value)  {
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return;
> > > > > >
> > > > > >   if (offset < adev->rmmio_size)
> > > > > > @@ -425,7 +453,7 @@ void amdgpu_device_wreg(struct amdgpu_device 
> > > > > > *adev,
> > > > > >   uint32_t reg, uint32_t v,
> > > > > >   uint32_t acc_flags)
> > > > > >   {
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return;
> > > > > >
> > > > > >   if ((reg * 4) < adev->rmmio_size) {
> > > > > > @@ -452,7 +480,7 @@ void amdgpu_device_wreg(struct amdgpu_device 
> > > > > > *adev,  void amdgpu_mm_wreg_mmio_rlc(struct amdgpu_device *adev,
> > > > > >        uint32_t reg, uint32_t v)
> > > > > >   {
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return;
> > > > > >
> > > > > >   if (amdgpu_sriov_fullaccess(adev) &&
> > > > > > @@ -475,7 +503,7 @@ void amdgpu_mm_wreg_mmio_rlc(struct 
> > > > > > amdgpu_device *adev,
> > > > > >    */
> > > > > >   u32 amdgpu_io_rreg(struct amdgpu_device *adev, u32 reg) {
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return 0;
> > > > > >
> > > > > >   if ((reg * 4) < adev->rio_mem_size)
> > > > > > @@ -497,7 +525,7 @@ u32 amdgpu_io_rreg(struct amdgpu_device *adev, 
> > > > > > u32 reg)
> > > > > >    */
> > > > > >   void amdgpu_io_wreg(struct amdgpu_device *adev, u32 reg, u32 v)  {
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return;
> > > > > >
> > > > > >   if ((reg * 4) < adev->rio_mem_size)
> > > > > > @@ -519,7 +547,7 @@ void amdgpu_io_wreg(struct amdgpu_device 
> > > > > > *adev, u32 reg, u32 v)
> > > > > >    */
> > > > > >   u32 amdgpu_mm_rdoorbell(struct amdgpu_device *adev, u32 index)  {
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return 0;
> > > > > >
> > > > > >   if (index < adev->doorbell.num_doorbells) { @@ -542,7 +570,7 @@ 
> > > > > > u32 amdgpu_mm_rdoorbell(struct amdgpu_device *adev, u32 index)
> > > > > >    */
> > > > > >   void amdgpu_mm_wdoorbell(struct amdgpu_device *adev, u32 index, 
> > > > > > u32 v)  {
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return;
> > > > > >
> > > > > >   if (index < adev->doorbell.num_doorbells) { @@ -563,7 +591,7 @@ 
> > > > > > void amdgpu_mm_wdoorbell(struct amdgpu_device *adev, u32 index, 
> > > > > > u32 v)
> > > > > >    */
> > > > > >   u64 amdgpu_mm_rdoorbell64(struct amdgpu_device *adev, u32 index)  {
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return 0;
> > > > > >
> > > > > >   if (index < adev->doorbell.num_doorbells) { @@ -586,7 +614,7 @@ 
> > > > > > u64 amdgpu_mm_rdoorbell64(struct amdgpu_device *adev, u32 index)
> > > > > >    */
> > > > > >   void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 
> > > > > > index, u64 v)  {
> > > > > > -if (adev->in_pci_err_recovery)
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > >   return;
> > > > > >
> > > > > >   if (index < adev->doorbell.num_doorbells) { @@ -610,10 +638,13 
> > > > > > @@ u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
> > > > > >   u32 pcie_index, u32 pcie_data,
> > > > > >   u32 reg_addr)
> > > > > >   {
> > > > > > -unsigned long flags;
> > > > > > -u32 r;
> > > > > >   void __iomem *pcie_index_offset;
> > > > > >   void __iomem *pcie_data_offset;
> > > > > > +unsigned long flags;
> > > > > > +u32 r;
> > > > > > +
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > > +return 0;
> > > > > >
> > > > > >   spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> > > > > >   pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 
> > > > > > 4; @@ -641,10 +672,13 @@ u64 amdgpu_device_indirect_rreg64(struct 
> > > > > > amdgpu_device *adev,
> > > > > >     u32 pcie_index, u32 pcie_data,
> > > > > >     u32 reg_addr)
> > > > > >   {
> > > > > > -unsigned long flags;
> > > > > > -u64 r;
> > > > > >   void __iomem *pcie_index_offset;
> > > > > >   void __iomem *pcie_data_offset;
> > > > > > +unsigned long flags;
> > > > > > +u64 r;
> > > > > > +
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > > +return 0;
> > > > > >
> > > > > >   spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> > > > > >   pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 
> > > > > > 4; @@ -677,9 +711,12 @@ void amdgpu_device_indirect_wreg(struct 
> > > > > > amdgpu_device *adev,
> > > > > >    u32 pcie_index, u32 pcie_data,
> > > > > >    u32 reg_addr, u32 reg_data)
> > > > > >   {
> > > > > > -unsigned long flags;
> > > > > >   void __iomem *pcie_index_offset;
> > > > > >   void __iomem *pcie_data_offset;
> > > > > > +unsigned long flags;
> > > > > > +
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > > +return;
> > > > > >
> > > > > >   spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> > > > > >   pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 
> > > > > > 4; @@ -706,9 +743,12 @@ void amdgpu_device_indirect_wreg64(struct 
> > > > > > amdgpu_device *adev,
> > > > > >      u32 pcie_index, u32 pcie_data,
> > > > > >      u32 reg_addr, u64 reg_data)
> > > > > >   {
> > > > > > -unsigned long flags;
> > > > > >   void __iomem *pcie_index_offset;
> > > > > >   void __iomem *pcie_data_offset;
> > > > > > +unsigned long flags;
> > > > > > +
> > > > > > +if (amdgpu_device_skip_hw_access(adev))
> > > > > > +return;
> > > > > >
> > > > > >   spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> > > > > >   pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> > > > > > --
> > > > > > 2.25.1
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx