[Nouveau] [PATCH v3] PCI: Use ioremap(), not phys_to_virt() for platform ROM
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(0x) - not-present page
*pde =
Oops: [#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: ECX: 3e8ed03c EDX: 0001
ESI: 0004 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: 06d0
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 updating all drivers which can access a platform
provided ROM. Instead of calling the helper function pci_platform_rom()
which uses phys_to_virt(), call ioremap() directly on the pdev->rom.
radeon_read_platform_bios() previously directly accessed an __iomem
pointer. Avoid this by calling memcpy_fromio() instead of kmemdup().
pci_platform_rom() now has no remaining callers, so remove it.
Signed-off-by: Mikel Rychliski
---
Tested on a MacPro 1,1 with a Radeon X1900 XT card and 32-bit kernel.
Changes in v3:
- Inline pci_platform_rom()
Changes in v2:
- Add iounmap() call in nouveau
- Update function comment for pci_platform_rom()
- Minor changes to commit messages
drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 31 +-
.../gpu/drm/nouveau/nvkm/subdev/bios/shadowpci.c | 17 ++--
drivers/gpu/drm/radeon/radeon_bios.c | 30 +
drivers/pci/rom.c | 17
include/linux/pci.h| 1 -
5 files changed, 52 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
index 50dff69a0f6e..b1172d93c99c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
@@ -192,30 +192,35 @@ static bool amdgpu_read_bios_from_rom(struct
amdgpu_device *adev)
static bool amdgpu_read_platform_bios(struct amdgpu_device *adev)
{
- uint8_t __iomem *bios;
- size_t size;
+ phys_addr_t rom = adev->pdev->rom;
+ size_t romlen = adev->pdev->romlen;
+ void __iomem *bios;
adev->bios = NULL;
- bios = pci_platform_rom(adev->pdev, );
- if (!bios) {
+ if (!rom || romlen == 0)
return false;
- }
- adev->bios = kzalloc(size, GFP_KERNEL);
- if (adev->bios == NULL)
+ adev->bios = kzalloc(romlen, GFP_KERNEL);
+ if (!adev->bios)
return false;
- memcpy_fromio(adev->bios, bios, size);
+ bios = ioremap(rom, romlen);
+ if (!bios)
+ goto free_bios;
- if (!check_atom_bios(adev->bios, size)) {
- kfree(adev->bios);
- return false;
- }
+ memcpy_fromio(adev->bios, bios, romlen);
+ iounmap(bios);
- adev->bios_size = size;
+ if (!check_atom_bios(adev->bios, romlen))
+ goto free_bios;
+
+ adev->bios_size = romlen;
return true;
+free_bios:
+ kfree(adev->bios);
+ return false;
}
#ifdef CONFIG_ACPI
diff --git
Re: [Nouveau] ensure device private pages have an owner v2
On Mon, Mar 16, 2020 at 08:32:12PM +0100, Christoph Hellwig wrote: > When acting on device private mappings a driver needs to know if the > device (or other entity in case of kvmppc) actually owns this private > mapping. This series adds an owner field and converts the migrate_vma > code over to check it. I looked into doing the same for > hmm_range_fault, but as far as I can tell that code has never been > wired up to actually work for device private memory, so instead of > trying to fix some unused code the second patch just remove the code. > We can add it back once we have a working and fully tested code, and > then should pass the expected owner in the hmm_range structure. > > Changes since v1: > - split out the pgmap->owner addition into a separate patch > - check pgmap->owner is set for device private mappings > - rename the dev_private_owner field in struct migrate_vma to src_owner > - refuse to migrate private pages if src_owner is not set > - keep the non-fault device private handling in hmm_range_fault I'm happy enough to take this, did you have plans for a v3? Thanks, Jason ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 1/9] drm/vblank: Add vblank works
On Tue, Mar 17, 2020 at 08:40:58PM -0400, Lyude Paul wrote:
> From: Ville Syrjälä
>
> Add some kind of vblank workers. The interface is similar to regular
> delayed works, and also allows for re-scheduling.
>
> Whatever hardware programming we do in the work must be fast
> (must at least complete during the vblank, sometimes during
> the first few scanlines of vblank), so we'll fire up a per-crtc
> high priority thread for this.
>
> [based off patches from Ville Syrjälä ,
> change below to signoff later]
>
> Cc: Ville Syrjälä
> Signed-off-by: Lyude Paul
Hm not really sold on the idea that we have should reinvent our own worker
infrastructure here. Imo a vblank_work should look like a delayed work,
i.e. using struct work_struct as the base class, and wrapping the vblank
thing around it (instead of the timer). That alos would allow drivers to
schedule works on their own work queues, allowing for easier flushing and
all that stuff.
Also if we do this I think we should try to follow the delayed work abi as
closely as possible (e.g. INIT_VBLANK_WORK, queue_vblank_work,
mod_vblank_work, ...). Delayed workers (whether timer or vblank) have a
bunch of edges cases where consistently would be really great to avoid
surprises and bugs.
-Daniel
> ---
> drivers/gpu/drm/drm_vblank.c | 322 +++
> include/drm/drm_vblank.h | 34
> 2 files changed, 356 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index da7b0b0c1090..06c796b6c381 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -25,7 +25,9 @@
> */
>
> #include
> +#include
> #include
> +#include
>
> #include
> #include
> @@ -91,6 +93,7 @@
> static bool
> drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> ktime_t *tvblank, bool in_vblank_irq);
> +static int drm_vblank_get(struct drm_device *dev, unsigned int pipe);
>
> static unsigned int drm_timestamp_precision = 20; /* Default to 20 usecs. */
>
> @@ -440,6 +443,9 @@ void drm_vblank_cleanup(struct drm_device *dev)
> drm_core_check_feature(dev, DRIVER_MODESET));
>
> del_timer_sync(>disable_timer);
> +
> + wake_up_all(>vblank_work.work_wait);
> + kthread_stop(vblank->vblank_work.thread);
> }
>
> kfree(dev->vblank);
> @@ -447,6 +453,108 @@ void drm_vblank_cleanup(struct drm_device *dev)
> dev->num_crtcs = 0;
> }
>
> +static int vblank_work_thread(void *data)
> +{
> + struct drm_vblank_crtc *vblank = data;
> +
> + while (!kthread_should_stop()) {
> + struct drm_vblank_work *work, *next;
> + LIST_HEAD(list);
> + u64 count;
> + int ret;
> +
> + spin_lock_irq(>dev->event_lock);
> +
> + ret = wait_event_interruptible_lock_irq(vblank->queue,
> + kthread_should_stop() ||
> +
> !list_empty(>vblank_work.work_list),
> +
> vblank->dev->event_lock);
> +
> + WARN_ON(ret && !kthread_should_stop() &&
> + list_empty(>vblank_work.irq_list) &&
> + list_empty(>vblank_work.work_list));
> +
> + list_for_each_entry_safe(work, next,
> + >vblank_work.work_list,
> + list) {
> + list_move_tail(>list, );
> + work->state = DRM_VBL_WORK_RUNNING;
> + }
> +
> + spin_unlock_irq(>dev->event_lock);
> +
> + if (list_empty())
> + continue;
> +
> + count = atomic64_read(>count);
> + list_for_each_entry(work, , list)
> + work->func(work, count);
> +
> + spin_lock_irq(>dev->event_lock);
> +
> + list_for_each_entry_safe(work, next, , list) {
> + if (work->reschedule) {
> + list_move_tail(>list,
> +>vblank_work.irq_list);
> + drm_vblank_get(vblank->dev, vblank->pipe);
> + work->reschedule = false;
> + work->state = DRM_VBL_WORK_WAITING;
> + } else {
> + list_del_init(>list);
> + work->cancel = false;
> + work->state = DRM_VBL_WORK_IDLE;
> + }
> + }
> +
> + spin_unlock_irq(>dev->event_lock);
> +
> + wake_up_all(>vblank_work.work_wait);
> + }
> +
> + return 0;
> +}
> +
> +static void vblank_work_init(struct drm_vblank_crtc *vblank)
> +{
> + struct sched_param param = {
> + .sched_priority = MAX_RT_PRIO
Re: [Nouveau] [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Tue, Mar 17, 2020 at 03:43:47PM -0700, Ralph Campbell wrote: >> Obviously no driver cared for that so far. Once we have test cases >> for that and thus testable code we can add code to fault it in from >> hmm_vma_handle_pte. >> > > I'm OK with the series. I think I would have been less confused if I looked at > patch 4 then 3. I guess I could just merge 3 and 4 if it is too confusing otherwise. ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH RESEND v2 2/2] PCI: Use ioremap(), not phys_to_virt() for platform ROM
On Tue, Mar 17, 2020 at 09:34:33PM -0400, Mikel Rychliski wrote: > I think the direct access to pdev->rom you suggest makes sense, especially > because users of the pci_platform_rom() are exposed to the fact that it just > calls ioremap(). > > I decided against wrapping the iounmap() with a pci_unmap_platform_rom(), but > I didn't apply the same consideration to the existing function. > > How about something like this (with pci_platform_rom() removed)? That looks sensible to me. ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 9/9] drm/nouveau/kms/nvd9-: Add CRC support
On Tue, Mar 17, 2020 at 08:41:06PM -0400, Lyude Paul wrote:
> + root = debugfs_create_dir("nv_crc", crtc->debugfs_entry);
> + if (IS_ERR(root))
> + return PTR_ERR(root);
No need to check this, just take the return value and move on.
> +
> + dent = debugfs_create_file("flip_threshold", 0644, root, head,
> +_crc_flip_threshold_fops);
> + if (IS_ERR(dent))
> + return PTR_ERR(dent);
No need to check this either, in fact this test is incorrect :(
Just make the call, and move on. See the loads of debugfs cleanups I
have been doing for examples.
thanks,
greg k-h
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
