[PATCH 0/4] Fix loading of radeon BIOS from 32-bit EFI

[PATCH 0/4] Fix loading of radeon BIOS from 32-bit EFI

[Mikel Rychliski]

This patch set fixes an opps when loading the radeon driver on old Macs
with a 32-bit EFI implementation.

Tested on a MacPro 1,1 with a Radeon X1900 XT card and 32-bit kernel.

Mikel Rychliski (4):
  drm/radeon: Stop directly referencing iomem
  PCI: Use ioremap, not phys_to_virt for platform rom
  drm/radeon: iounmap unused mapping
  drm/amdgpu: iounmap unused mapping

 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c |  1 +
 drivers/gpu/drm/radeon/radeon_bios.c     | 12 ++++++++----
 drivers/pci/rom.c                        |  5 ++---
 3 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.13.7

[PATCH 1/4] drm/radeon: Stop directly referencing iomem

[Mikel Rychliski]

pci_platform_rom returns an __iomem pointer which should not be accessed
directly. Change radeon_read_platform_bios to use memcpy_fromio instead of
calling kmemdup on the __iomem pointer.

Signed-off-by: Mikel Rychliski <mikel@mikelr.com>
---
 drivers/gpu/drm/radeon/radeon_bios.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
index c42f73fad3e3..c3ae4c92a115 100644
--- a/drivers/gpu/drm/radeon/radeon_bios.c
+++ b/drivers/gpu/drm/radeon/radeon_bios.c
@@ -118,11 +118,14 @@ static bool radeon_read_platform_bios(struct radeon_device *rdev)
 		return false;
 	}
 
-	if (size == 0 || bios[0] != 0x55 || bios[1] != 0xaa) {
+	rdev->bios = kzalloc(size, GFP_KERNEL);
+	if (!rdev->bios)
 		return false;
-	}
-	rdev->bios = kmemdup(bios, size, GFP_KERNEL);
-	if (rdev->bios == NULL) {
+
+	memcpy_fromio(rdev->bios, bios, size);
+
+	if (size == 0 || rdev->bios[0] != 0x55 || rdev->bios[1] != 0xaa) {
+		kfree(rdev->bios);
 		return false;
 	}
 
-- 
2.13.7

[PATCH 2/4] PCI: Use ioremap, not phys_to_virt for platform rom

[Mikel Rychliski]

On some EFI systems, the video BIOS is provided by the EFI firmware.  The
boot stub code stores the physical address of the ROM image in pdev->rom.
Currently we attempt to access this pointer using phys_to_virt, which
doesn't work with CONFIG_HIGHMEM.

On these systems, attempting to load the radeon module on a x86_32 kernel
can result in the following:

    BUG: unable to handle page fault for address: 3e8ed03c
    #PF: supervisor read access in kernel mode
    #PF: error_code(0x0000) - not-present page
    *pde = 00000000
    Oops: 0000 [#1] PREEMPT SMP
    CPU: 0 PID: 317 Comm: systemd-udevd Not tainted 5.6.0-rc3-next-20200228 #2
    Hardware name: Apple Computer, Inc. MacPro1,1/Mac-F4208DC8, BIOS     MP11.88Z.005C.B08.0707021221 07/02/07
    EIP: radeon_get_bios+0x5ed/0xe50 [radeon]
    Code: 00 00 84 c0 0f 85 12 fd ff ff c7 87 64 01 00 00 00 00 00 00 8b 47 08 8b 55 b0 e8 1e 83 e1 d6 85 c0 74 1a 8b 55 c0 85 d2 74 13 <80> 38 55 75 0e 80 78 01 aa 0f 84 a4 03 00 00 8d 74 26 00 68 dc 06
    EAX: 3e8ed03c EBX: 00000000 ECX: 3e8ed03c EDX: 00010000
    ESI: 00040000 EDI: eec04000 EBP: eef3fc60 ESP: eef3fbe0
    DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010206
    CR0: 80050033 CR2: 3e8ed03c CR3: 2ec77000 CR4: 000006d0
    Call Trace:
     ? register_client+0x34/0xe0
     ? register_client+0xab/0xe0
     r520_init+0x26/0x240 [radeon]
     radeon_device_init+0x533/0xa50 [radeon]
     radeon_driver_load_kms+0x80/0x220 [radeon]
     drm_dev_register+0xa7/0x180 [drm]
     radeon_pci_probe+0x10f/0x1a0 [radeon]
     pci_device_probe+0xd4/0x140
     really_probe+0x13d/0x3b0
     driver_probe_device+0x56/0xd0
     device_driver_attach+0x49/0x50
     __driver_attach+0x79/0x130
     ? device_driver_attach+0x50/0x50
     bus_for_each_dev+0x5b/0xa0
     driver_attach+0x19/0x20
     ? device_driver_attach+0x50/0x50
     bus_add_driver+0x117/0x1d0
     ? pci_bus_num_vf+0x20/0x20
     driver_register+0x66/0xb0
     ? 0xf80f4000
     __pci_register_driver+0x3d/0x40
     radeon_init+0x82/0x1000 [radeon]
     do_one_initcall+0x42/0x200
     ? kvfree+0x25/0x30
     ? __vunmap+0x206/0x230
     ? kmem_cache_alloc_trace+0x16f/0x220
     ? do_init_module+0x21/0x220
     do_init_module+0x50/0x220
     load_module+0x1f26/0x2200
     sys_init_module+0x12d/0x160
     do_fast_syscall_32+0x82/0x250
     entry_SYSENTER_32+0xa5/0xf8

Fix the issue by using ioremap instead of phys_to_virt in pci_platform_rom.

Signed-off-by: Mikel Rychliski <mikel@mikelr.com>
---
 drivers/pci/rom.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index 137bf0cee897..e352798eed0c 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -197,8 +197,7 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom)
 EXPORT_SYMBOL(pci_unmap_rom);
 
 /**
- * pci_platform_rom - provides a pointer to any ROM image provided by the
- * platform
+ * pci_platform_rom - ioremap the ROM image provided by the platform
  * @pdev: pointer to pci device struct
  * @size: pointer to receive size of pci window over ROM
  */
@@ -206,7 +205,7 @@ void __iomem *pci_platform_rom(struct pci_dev *pdev, size_t *size)
 {
 	if (pdev->rom && pdev->romlen) {
 		*size = pdev->romlen;
-		return phys_to_virt((phys_addr_t)pdev->rom);
+		return ioremap(pdev->rom, pdev->romlen);
 	}
 
 	return NULL;
-- 
2.13.7

[PATCH 3/4] drm/radeon: iounmap unused mapping

[Mikel Rychliski]

Now that pci_platform_rom creates a new mapping to access the ROM
image, we should remove this mapping after extracting the BIOS.

Signed-off-by: Mikel Rychliski <mikel@mikelr.com>
---
 drivers/gpu/drm/radeon/radeon_bios.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
index c3ae4c92a115..712b88d88957 100644
--- a/drivers/gpu/drm/radeon/radeon_bios.c
+++ b/drivers/gpu/drm/radeon/radeon_bios.c
@@ -123,6 +123,7 @@ static bool radeon_read_platform_bios(struct radeon_device *rdev)
 		return false;
 
 	memcpy_fromio(rdev->bios, bios, size);
+	iounmap(bios);
 
 	if (size == 0 || rdev->bios[0] != 0x55 || rdev->bios[1] != 0xaa) {
 		kfree(rdev->bios);
-- 
2.13.7

[PATCH 4/4] drm/amdgpu: iounmap unused mapping

[Mikel Rychliski]

Now that pci_platform_rom creates a new mapping to access the ROM
image, we should remove this mapping after extracting the BIOS.

Signed-off-by: Mikel Rychliski <mikel@mikelr.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
index 50dff69a0f6e..ea6a1fa98ad9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
@@ -207,6 +207,7 @@ static bool amdgpu_read_platform_bios(struct amdgpu_device *adev)
 		return false;
 
 	memcpy_fromio(adev->bios, bios, size);
+	iounmap(bios);
 
 	if (!check_atom_bios(adev->bios, size)) {
 		kfree(adev->bios);
-- 
2.13.7

Re: [PATCH 2/4] PCI: Use ioremap, not phys_to_virt for platform rom

[Bjorn Helgaas]

Cosmetics:

s/ioremap/ioremap()/ (also in commit log)
s/phys_to_virt/phys_to_virt()/ (also in commit log)
s/pci_platform_rom/pci_platform_rom()/ (commit log)
s/rom/ROM/

On Mon, Mar 02, 2020 at 10:34:55PM -0500, Mikel Rychliski wrote:
> On some EFI systems, the video BIOS is provided by the EFI firmware.  The
> boot stub code stores the physical address of the ROM image in pdev->rom.
> Currently we attempt to access this pointer using phys_to_virt, which
> doesn't work with CONFIG_HIGHMEM.
> 
> On these systems, attempting to load the radeon module on a x86_32 kernel
> can result in the following:
> 
>     BUG: unable to handle page fault for address: 3e8ed03c
>     #PF: supervisor read access in kernel mode
>     #PF: error_code(0x0000) - not-present page
>     *pde = 00000000
>     Oops: 0000 [#1] PREEMPT SMP
>     CPU: 0 PID: 317 Comm: systemd-udevd Not tainted 5.6.0-rc3-next-20200228 #2
>     Hardware name: Apple Computer, Inc. MacPro1,1/Mac-F4208DC8, BIOS     MP11.88Z.005C.B08.0707021221 07/02/07
>     EIP: radeon_get_bios+0x5ed/0xe50 [radeon]
>     Code: 00 00 84 c0 0f 85 12 fd ff ff c7 87 64 01 00 00 00 00 00 00 8b 47 08 8b 55 b0 e8 1e 83 e1 d6 85 c0 74 1a 8b 55 c0 85 d2 74 13 <80> 38 55 75 0e 80 78 01 aa 0f 84 a4 03 00 00 8d 74 26 00 68 dc 06
>     EAX: 3e8ed03c EBX: 00000000 ECX: 3e8ed03c EDX: 00010000
>     ESI: 00040000 EDI: eec04000 EBP: eef3fc60 ESP: eef3fbe0
>     DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010206
>     CR0: 80050033 CR2: 3e8ed03c CR3: 2ec77000 CR4: 000006d0
>     Call Trace:
>      ? register_client+0x34/0xe0
>      ? register_client+0xab/0xe0
>      r520_init+0x26/0x240 [radeon]
>      radeon_device_init+0x533/0xa50 [radeon]
>      radeon_driver_load_kms+0x80/0x220 [radeon]
>      drm_dev_register+0xa7/0x180 [drm]
>      radeon_pci_probe+0x10f/0x1a0 [radeon]
>      pci_device_probe+0xd4/0x140
>      really_probe+0x13d/0x3b0
>      driver_probe_device+0x56/0xd0
>      device_driver_attach+0x49/0x50
>      __driver_attach+0x79/0x130
>      ? device_driver_attach+0x50/0x50
>      bus_for_each_dev+0x5b/0xa0
>      driver_attach+0x19/0x20
>      ? device_driver_attach+0x50/0x50
>      bus_add_driver+0x117/0x1d0
>      ? pci_bus_num_vf+0x20/0x20
>      driver_register+0x66/0xb0
>      ? 0xf80f4000
>      __pci_register_driver+0x3d/0x40
>      radeon_init+0x82/0x1000 [radeon]
>      do_one_initcall+0x42/0x200
>      ? kvfree+0x25/0x30
>      ? __vunmap+0x206/0x230
>      ? kmem_cache_alloc_trace+0x16f/0x220
>      ? do_init_module+0x21/0x220
>      do_init_module+0x50/0x220
>      load_module+0x1f26/0x2200
>      sys_init_module+0x12d/0x160
>      do_fast_syscall_32+0x82/0x250
>      entry_SYSENTER_32+0xa5/0xf8
> 
> Fix the issue by using ioremap instead of phys_to_virt in pci_platform_rom.
> 
> Signed-off-by: Mikel Rychliski <mikel@mikelr.com>
> ---
>  drivers/pci/rom.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
> index 137bf0cee897..e352798eed0c 100644
> --- a/drivers/pci/rom.c
> +++ b/drivers/pci/rom.c
> @@ -197,8 +197,7 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom)
>  EXPORT_SYMBOL(pci_unmap_rom);
>  
>  /**
> - * pci_platform_rom - provides a pointer to any ROM image provided by the
> - * platform
> + * pci_platform_rom - ioremap the ROM image provided by the platform
>   * @pdev: pointer to pci device struct
>   * @size: pointer to receive size of pci window over ROM
>   */
> @@ -206,7 +205,7 @@ void __iomem *pci_platform_rom(struct pci_dev *pdev, size_t *size)
>  {
>  	if (pdev->rom && pdev->romlen) {
>  		*size = pdev->romlen;
> -		return phys_to_virt((phys_addr_t)pdev->rom);
> +		return ioremap(pdev->rom, pdev->romlen);

This changes the interface of pci_platform_rom() (the caller is now
responsible for doing an iounmap()).  That should be mentioned in the
function comment, and I think the subsequent patches should be
squashed into this one so the interface change and the caller changes
are done simultaneously.

Also, it looks like nvbios_platform.init() (platform_init()) needs a
similar change?

>  	}
>  
>  	return NULL;
> -- 
> 2.13.7
> 

Re: [PATCH 2/4] PCI: Use ioremap, not phys_to_virt for platform rom

[Mikel Rychliski]

On Tuesday, March 3, 2020 9:38:27 AM EST Bjorn Helgaas wrote:
> Cosmetics:
> 
> s/ioremap/ioremap()/ (also in commit log)
> s/phys_to_virt/phys_to_virt()/ (also in commit log)
> s/pci_platform_rom/pci_platform_rom()/ (commit log)
> s/rom/ROM/

> This changes the interface of pci_platform_rom() (the caller is now
> responsible for doing an iounmap()).  That should be mentioned in the
> function comment, and I think the subsequent patches should be
> squashed into this one so the interface change and the caller changes
> are done simultaneously.
> 
> Also, it looks like nvbios_platform.init() (platform_init()) needs a
> similar change?

Hi Bjorn,

Thank you for your comments. I'll make the suggested changes, add an iounmap() 
in the nouveau usage, and provide a new two-patch series.

Re: [PATCH 2/4] PCI: Use ioremap, not phys_to_virt for platform rom

[Christoph Hellwig]

Any reason drivers can't just use pci_map_rom insteadἅ which already
does the right thing?

Re: [PATCH 3/4] drm/radeon: iounmap unused mapping

[Christoph Hellwig]

On Mon, Mar 02, 2020 at 10:34:56PM -0500, Mikel Rychliski wrote:
> Now that pci_platform_rom creates a new mapping to access the ROM
> image, we should remove this mapping after extracting the BIOS.

This and the next patch really need to be folded into the previous
one to avoid regressions (assuming my other suggestion doesn't work
for some reason).