[PATCH] drm/amdgpu: capture invalid hardware access v2
Daniel Vetter
daniel at ffwll.ch
Tue Mar 9 21:13:03 UTC 2021
On Tue, Mar 9, 2021 at 8:13 PM Andrey Grodzovsky
<andrey.grodzovsky at amd.com> wrote:
>
> 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 ?
For the annotations:
https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html?highlight=dma_fence#dma-fence-signalling-annotations
I need to brush up my drm/scheduler annotation patches, those should
hit all the important cases.
Wrt documenting the overall locking hierarchy, best place is probably:
https://cgit.freedesktop.org/drm/drm/tree/drivers/dma-buf/dma-resv.c#n96
Meaning there's no explicit (kerneldoc) documentation about the
nesting, but we at least try to teach lockdep about them directly.
Cheers, Daniel
> 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 at gmail.com>
> >>>>>> Sent: Tuesday, March 9, 2021 2:07 AM
> >>>>>> To: amd-gfx at lists.freedesktop.org
> >>>>>> Cc: Grodzovsky, Andrey <Andrey.Grodzovsky at amd.com>; Li, Dennis
> >>>>>> <Dennis.Li at amd.com>
> >>>>>> Subject: [PATCH] drm/amdgpu: capture invalid hardware access v2
> >>>>>>
> >>>>>> From: Dennis Li <Dennis.Li at amd.com>
> >>>>>>
> >>>>>> 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 at amd.com>
> >>>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
> >>>>>> ---
> >>>>>> 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
> >>>>>>
> >>>>>
> >>>
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the amd-gfx
mailing list