| Message ID | 0657d1ec504d2f58de5835f4d67625b583005a09.1573783477.git.sbobroff@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] drm/radeon drm/amdgpu: fix bad DMA from INTERRUPT_CNTL2 | expand |
On Thu, Nov 14, 2019 at 9:05 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote: > > The INTERRUPT_CNTL2 register expects a valid DMA address, but is > currently set with a GPU MC address. This can cause problems on > systems that detect the resulting DMA read from an invalid address > (found on a Power8 guest). > > Instead, use the DMA address of the dummy page because it will always > be safe. > > Fixes: d8f60cfc9345 ("drm/radeon/kms: Add support for interrupts on r6xx/r7xx chips (v3)") > Fixes: 25a857fbe973 ("drm/radeon/kms: add support for interrupts on SI") > Fixes: a59781bbe528 ("drm/radeon: add support for interrupts on CIK (v5)") > Fixes: 27ae10641e9c ("drm/amdgpu: add interupt handler implementation for si v3") > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> Can you split this into two patches, one for radeon and one for amdgpu? I'll apply them. Thanks! Alex > --- > A couple of notes: > - Initial discussion: > https://lists.freedesktop.org/archives/dri-devel/2019-November/244090.html > - I have only tested the case that uses r600_irq_init(), but they are all very > similar. > - I've included a fixes tag for each change, but I don't know if that's the > right thing to do in this case (please feel free to fix them on commit or > whatever). > > Cheers, > Sam. > > drivers/gpu/drm/amd/amdgpu/si_ih.c | 3 ++- > drivers/gpu/drm/radeon/cik.c | 4 ++-- > drivers/gpu/drm/radeon/r600.c | 4 ++-- > drivers/gpu/drm/radeon/si.c | 4 ++-- > 4 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/si_ih.c b/drivers/gpu/drm/amd/amdgpu/si_ih.c > index 57bb5f9e08b2..88ae27a5a03d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/si_ih.c > +++ b/drivers/gpu/drm/amd/amdgpu/si_ih.c > @@ -64,7 +64,8 @@ static int si_ih_irq_init(struct amdgpu_device *adev) > u32 interrupt_cntl, ih_cntl, ih_rb_cntl; > > si_ih_disable_interrupts(adev); > - WREG32(INTERRUPT_CNTL2, adev->irq.ih.gpu_addr >> 8); > + /* set dummy read address to dummy page address */ > + WREG32(INTERRUPT_CNTL2, adev->dummy_page_addr >> 8); > interrupt_cntl = RREG32(INTERRUPT_CNTL); > interrupt_cntl &= ~IH_DUMMY_RD_OVERRIDE; > interrupt_cntl &= ~IH_REQ_NONSNOOP_EN; > diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c > index 62eab82a64f9..897442754fd0 100644 > --- a/drivers/gpu/drm/radeon/cik.c > +++ b/drivers/gpu/drm/radeon/cik.c > @@ -6969,8 +6969,8 @@ static int cik_irq_init(struct radeon_device *rdev) > } > > /* setup interrupt control */ > - /* XXX this should actually be a bus address, not an MC address. same on older asics */ > - WREG32(INTERRUPT_CNTL2, rdev->ih.gpu_addr >> 8); > + /* set dummy read address to dummy page address */ > + WREG32(INTERRUPT_CNTL2, rdev->dummy_page.addr >> 8); > interrupt_cntl = RREG32(INTERRUPT_CNTL); > /* IH_DUMMY_RD_OVERRIDE=0 - dummy read disabled with msi, enabled without msi > * IH_DUMMY_RD_OVERRIDE=1 - dummy read controlled by IH_DUMMY_RD_EN > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c > index e937cc01910d..033bc466a862 100644 > --- a/drivers/gpu/drm/radeon/r600.c > +++ b/drivers/gpu/drm/radeon/r600.c > @@ -3696,8 +3696,8 @@ int r600_irq_init(struct radeon_device *rdev) > } > > /* setup interrupt control */ > - /* set dummy read address to ring address */ > - WREG32(INTERRUPT_CNTL2, rdev->ih.gpu_addr >> 8); > + /* set dummy read address to dummy page address */ > + WREG32(INTERRUPT_CNTL2, rdev->dummy_page.addr >> 8); > interrupt_cntl = RREG32(INTERRUPT_CNTL); > /* IH_DUMMY_RD_OVERRIDE=0 - dummy read disabled with msi, enabled without msi > * IH_DUMMY_RD_OVERRIDE=1 - dummy read controlled by IH_DUMMY_RD_EN > diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c > index 05894d198a79..1d8efb0eefdb 100644 > --- a/drivers/gpu/drm/radeon/si.c > +++ b/drivers/gpu/drm/radeon/si.c > @@ -5997,8 +5997,8 @@ static int si_irq_init(struct radeon_device *rdev) > } > > /* setup interrupt control */ > - /* set dummy read address to ring address */ > - WREG32(INTERRUPT_CNTL2, rdev->ih.gpu_addr >> 8); > + /* set dummy read address to dummy page address */ > + WREG32(INTERRUPT_CNTL2, rdev->dummy_page.addr >> 8); > interrupt_cntl = RREG32(INTERRUPT_CNTL); > /* IH_DUMMY_RD_OVERRIDE=0 - dummy read disabled with msi, enabled without msi > * IH_DUMMY_RD_OVERRIDE=1 - dummy read controlled by IH_DUMMY_RD_EN > -- > 2.22.0.216.g00a2a96fc9 >
On Fri, Nov 15, 2019 at 09:58:18AM -0500, Alex Deucher wrote: > On Thu, Nov 14, 2019 at 9:05 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote: > > > > The INTERRUPT_CNTL2 register expects a valid DMA address, but is > > currently set with a GPU MC address. This can cause problems on > > systems that detect the resulting DMA read from an invalid address > > (found on a Power8 guest). > > > > Instead, use the DMA address of the dummy page because it will always > > be safe. > > > > Fixes: d8f60cfc9345 ("drm/radeon/kms: Add support for interrupts on r6xx/r7xx chips (v3)") > > Fixes: 25a857fbe973 ("drm/radeon/kms: add support for interrupts on SI") > > Fixes: a59781bbe528 ("drm/radeon: add support for interrupts on CIK (v5)") > > Fixes: 27ae10641e9c ("drm/amdgpu: add interupt handler implementation for si v3") > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > > Can you split this into two patches, one for radeon and one for > amdgpu? I'll apply them. > > Thanks! > > Alex No problem at all, I'll post it as v2. Cheers, Sam. > > > --- > > A couple of notes: > > - Initial discussion: > > https://lists.freedesktop.org/archives/dri-devel/2019-November/244090.html > > - I have only tested the case that uses r600_irq_init(), but they are all very > > similar. > > - I've included a fixes tag for each change, but I don't know if that's the > > right thing to do in this case (please feel free to fix them on commit or > > whatever). > > > > Cheers, > > Sam. > > > > drivers/gpu/drm/amd/amdgpu/si_ih.c | 3 ++- > > drivers/gpu/drm/radeon/cik.c | 4 ++-- > > drivers/gpu/drm/radeon/r600.c | 4 ++-- > > drivers/gpu/drm/radeon/si.c | 4 ++-- > > 4 files changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/si_ih.c b/drivers/gpu/drm/amd/amdgpu/si_ih.c > > index 57bb5f9e08b2..88ae27a5a03d 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/si_ih.c > > +++ b/drivers/gpu/drm/amd/amdgpu/si_ih.c > > @@ -64,7 +64,8 @@ static int si_ih_irq_init(struct amdgpu_device *adev) > > u32 interrupt_cntl, ih_cntl, ih_rb_cntl; > > > > si_ih_disable_interrupts(adev); > > - WREG32(INTERRUPT_CNTL2, adev->irq.ih.gpu_addr >> 8); > > + /* set dummy read address to dummy page address */ > > + WREG32(INTERRUPT_CNTL2, adev->dummy_page_addr >> 8); > > interrupt_cntl = RREG32(INTERRUPT_CNTL); > > interrupt_cntl &= ~IH_DUMMY_RD_OVERRIDE; > > interrupt_cntl &= ~IH_REQ_NONSNOOP_EN; > > diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c > > index 62eab82a64f9..897442754fd0 100644 > > --- a/drivers/gpu/drm/radeon/cik.c > > +++ b/drivers/gpu/drm/radeon/cik.c > > @@ -6969,8 +6969,8 @@ static int cik_irq_init(struct radeon_device *rdev) > > } > > > > /* setup interrupt control */ > > - /* XXX this should actually be a bus address, not an MC address. same on older asics */ > > - WREG32(INTERRUPT_CNTL2, rdev->ih.gpu_addr >> 8); > > + /* set dummy read address to dummy page address */ > > + WREG32(INTERRUPT_CNTL2, rdev->dummy_page.addr >> 8); > > interrupt_cntl = RREG32(INTERRUPT_CNTL); > > /* IH_DUMMY_RD_OVERRIDE=0 - dummy read disabled with msi, enabled without msi > > * IH_DUMMY_RD_OVERRIDE=1 - dummy read controlled by IH_DUMMY_RD_EN > > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c > > index e937cc01910d..033bc466a862 100644 > > --- a/drivers/gpu/drm/radeon/r600.c > > +++ b/drivers/gpu/drm/radeon/r600.c > > @@ -3696,8 +3696,8 @@ int r600_irq_init(struct radeon_device *rdev) > > } > > > > /* setup interrupt control */ > > - /* set dummy read address to ring address */ > > - WREG32(INTERRUPT_CNTL2, rdev->ih.gpu_addr >> 8); > > + /* set dummy read address to dummy page address */ > > + WREG32(INTERRUPT_CNTL2, rdev->dummy_page.addr >> 8); > > interrupt_cntl = RREG32(INTERRUPT_CNTL); > > /* IH_DUMMY_RD_OVERRIDE=0 - dummy read disabled with msi, enabled without msi > > * IH_DUMMY_RD_OVERRIDE=1 - dummy read controlled by IH_DUMMY_RD_EN > > diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c > > index 05894d198a79..1d8efb0eefdb 100644 > > --- a/drivers/gpu/drm/radeon/si.c > > +++ b/drivers/gpu/drm/radeon/si.c > > @@ -5997,8 +5997,8 @@ static int si_irq_init(struct radeon_device *rdev) > > } > > > > /* setup interrupt control */ > > - /* set dummy read address to ring address */ > > - WREG32(INTERRUPT_CNTL2, rdev->ih.gpu_addr >> 8); > > + /* set dummy read address to dummy page address */ > > + WREG32(INTERRUPT_CNTL2, rdev->dummy_page.addr >> 8); > > interrupt_cntl = RREG32(INTERRUPT_CNTL); > > /* IH_DUMMY_RD_OVERRIDE=0 - dummy read disabled with msi, enabled without msi > > * IH_DUMMY_RD_OVERRIDE=1 - dummy read controlled by IH_DUMMY_RD_EN > > -- > > 2.22.0.216.g00a2a96fc9 > >
diff --git a/drivers/gpu/drm/amd/amdgpu/si_ih.c b/drivers/gpu/drm/amd/amdgpu/si_ih.c index 57bb5f9e08b2..88ae27a5a03d 100644 --- a/drivers/gpu/drm/amd/amdgpu/si_ih.c +++ b/drivers/gpu/drm/amd/amdgpu/si_ih.c @@ -64,7 +64,8 @@ static int si_ih_irq_init(struct amdgpu_device *adev) u32 interrupt_cntl, ih_cntl, ih_rb_cntl; si_ih_disable_interrupts(adev); - WREG32(INTERRUPT_CNTL2, adev->irq.ih.gpu_addr >> 8); + /* set dummy read address to dummy page address */ + WREG32(INTERRUPT_CNTL2, adev->dummy_page_addr >> 8); interrupt_cntl = RREG32(INTERRUPT_CNTL); interrupt_cntl &= ~IH_DUMMY_RD_OVERRIDE; interrupt_cntl &= ~IH_REQ_NONSNOOP_EN; diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c index 62eab82a64f9..897442754fd0 100644 --- a/drivers/gpu/drm/radeon/cik.c +++ b/drivers/gpu/drm/radeon/cik.c @@ -6969,8 +6969,8 @@ static int cik_irq_init(struct radeon_device *rdev) } /* setup interrupt control */ - /* XXX this should actually be a bus address, not an MC address. same on older asics */ - WREG32(INTERRUPT_CNTL2, rdev->ih.gpu_addr >> 8); + /* set dummy read address to dummy page address */ + WREG32(INTERRUPT_CNTL2, rdev->dummy_page.addr >> 8); interrupt_cntl = RREG32(INTERRUPT_CNTL); /* IH_DUMMY_RD_OVERRIDE=0 - dummy read disabled with msi, enabled without msi * IH_DUMMY_RD_OVERRIDE=1 - dummy read controlled by IH_DUMMY_RD_EN diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index e937cc01910d..033bc466a862 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -3696,8 +3696,8 @@ int r600_irq_init(struct radeon_device *rdev) } /* setup interrupt control */ - /* set dummy read address to ring address */ - WREG32(INTERRUPT_CNTL2, rdev->ih.gpu_addr >> 8); + /* set dummy read address to dummy page address */ + WREG32(INTERRUPT_CNTL2, rdev->dummy_page.addr >> 8); interrupt_cntl = RREG32(INTERRUPT_CNTL); /* IH_DUMMY_RD_OVERRIDE=0 - dummy read disabled with msi, enabled without msi * IH_DUMMY_RD_OVERRIDE=1 - dummy read controlled by IH_DUMMY_RD_EN diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c index 05894d198a79..1d8efb0eefdb 100644 --- a/drivers/gpu/drm/radeon/si.c +++ b/drivers/gpu/drm/radeon/si.c @@ -5997,8 +5997,8 @@ static int si_irq_init(struct radeon_device *rdev) } /* setup interrupt control */ - /* set dummy read address to ring address */ - WREG32(INTERRUPT_CNTL2, rdev->ih.gpu_addr >> 8); + /* set dummy read address to dummy page address */ + WREG32(INTERRUPT_CNTL2, rdev->dummy_page.addr >> 8); interrupt_cntl = RREG32(INTERRUPT_CNTL); /* IH_DUMMY_RD_OVERRIDE=0 - dummy read disabled with msi, enabled without msi * IH_DUMMY_RD_OVERRIDE=1 - dummy read controlled by IH_DUMMY_RD_EN
The INTERRUPT_CNTL2 register expects a valid DMA address, but is currently set with a GPU MC address. This can cause problems on systems that detect the resulting DMA read from an invalid address (found on a Power8 guest). Instead, use the DMA address of the dummy page because it will always be safe. Fixes: d8f60cfc9345 ("drm/radeon/kms: Add support for interrupts on r6xx/r7xx chips (v3)") Fixes: 25a857fbe973 ("drm/radeon/kms: add support for interrupts on SI") Fixes: a59781bbe528 ("drm/radeon: add support for interrupts on CIK (v5)") Fixes: 27ae10641e9c ("drm/amdgpu: add interupt handler implementation for si v3") Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> --- A couple of notes: - Initial discussion: https://lists.freedesktop.org/archives/dri-devel/2019-November/244090.html - I have only tested the case that uses r600_irq_init(), but they are all very similar. - I've included a fixes tag for each change, but I don't know if that's the right thing to do in this case (please feel free to fix them on commit or whatever). Cheers, Sam. drivers/gpu/drm/amd/amdgpu/si_ih.c | 3 ++- drivers/gpu/drm/radeon/cik.c | 4 ++-- drivers/gpu/drm/radeon/r600.c | 4 ++-- drivers/gpu/drm/radeon/si.c | 4 ++-- 4 files changed, 8 insertions(+), 7 deletions(-)