RE: [PATCH] drm/amdgpu: fix pm_load_smu_firmware for SR-IOV
Reviewed-by: Prike Liang
Thanks,
Prike
> -Original Message-
> From: Trigger Huang
> Sent: Tuesday, June 11, 2019 12:09 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Huang, Trigger ; Liang, Prike
>
> Subject: [PATCH] drm/amdgpu: fix pm_load_smu_firmware for SR-IOV
>
> For SR-IOV VF, powerplay may not be supported, in this case, error '-EINVAL'
> should not be returned.
>
> Signed-off-by: Trigger Huang
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 21b5be1..4276d63 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -2709,7 +2709,10 @@ int amdgpu_pm_load_smu_firmware(struct
> amdgpu_device *adev, uint32_t *smu_versio
> return r;
> }
> *smu_version = adev->pm.fw_version;
> + } else if (amdgpu_sriov_vf(adev)) {
> + r = 0;
> }
> +
> return r;
> }
>
> --
> 2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: fix pm_load_smu_firmware for SR-IOV
For SR-IOV VF, powerplay may not be supported, in this case,
error '-EINVAL' should not be returned.
Signed-off-by: Trigger Huang
---
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 21b5be1..4276d63 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -2709,7 +2709,10 @@ int amdgpu_pm_load_smu_firmware(struct amdgpu_device
*adev, uint32_t *smu_versio
return r;
}
*smu_version = adev->pm.fw_version;
+ } else if (amdgpu_sriov_vf(adev)) {
+ r = 0;
}
+
return r;
}
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm/amdkfd: Initialize HSA_CAP_ATS_PRESENT capability in topology codes
On 2019-06-10 13:48, Zeng, Oak wrote:
> Move HSA_CAP_ATS_PRESENT initialization logic from kfd iommu codes to
> kfd topology codes. This removes kfd_iommu_device_init's dependency
> on kfd_topology_add_device. Also remove duplicate code setting the
> same.
>
> Change-Id: I2bf3c000d4795b41afe1b39fe679677c1f86bfbe
> Signed-off-by: Oak Zeng
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_iommu.c| 10 +-
> drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 6 +-
> 2 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
> index 0149475..5f35df2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
> @@ -66,16 +66,8 @@ int kfd_iommu_device_init(struct kfd_dev *kfd)
>
> top_dev = kfd_topology_device_by_id(kfd->id);
>
> - /*
> - * Overwrite ATS capability according to needs_iommu_device to fix
> - * potential missing corresponding bit in CRAT of BIOS.
> - */
It would be good to preserve this comment by moving it to
kfd_topology_add_device.
> - if (!kfd->device_info->needs_iommu_device) {
> - top_dev->node_props.capability &= ~HSA_CAP_ATS_PRESENT;
> + if (!kfd->device_info->needs_iommu_device)
> return 0;
> - }
> -
> - top_dev->node_props.capability |= HSA_CAP_ATS_PRESENT;
>
> iommu_info.flags = 0;
> err = amd_iommu_device_info(kfd->pdev, &iommu_info);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index d241a86..1c184b3 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -1330,6 +1330,11 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>dev->gpu->device_info->asic_family);
> }
>
> + if (dev->gpu->device_info->needs_iommu_device)
> + dev->node_props.capability |= HSA_CAP_ATS_PRESENT;
> + else
> + dev->node_props.capability &= ~HSA_CAP_ATS_PRESENT;
> +
> /* Fix errors in CZ CRAT.
>* simd_count: Carrizo CRAT reports wrong simd_count, probably
>* because it doesn't consider masked out CUs
> @@ -1340,7 +1345,6 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
> dev->node_props.simd_count =
> cu_info.simd_per_cu * cu_info.cu_active_number;
> dev->node_props.max_waves_per_simd = 10;
> - dev->node_props.capability |= HSA_CAP_ATS_PRESENT;
There is a comment above the if-block that should be updated when you
remove this.
With these two comments fixed, the series is Reviewed-by: Felix Kuehling
Regards,
Felix
> }
>
> ctx = amdgpu_ras_get_context((struct amdgpu_device *)(dev->gpu->kgd));
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v16 16/16] selftests, arm64: add a selftest for passing tagged pointers to kernel
On 6/7/19 9:56 PM, Kees Cook wrote: On Mon, Jun 03, 2019 at 06:55:18PM +0200, Andrey Konovalov wrote: This patch is a part of a series that extends arm64 kernel ABI to allow to pass tagged user pointers (with the top byte set to something else other than 0x00) as syscall arguments. This patch adds a simple test, that calls the uname syscall with a tagged user pointer as an argument. Without the kernel accepting tagged user pointers the test fails with EFAULT. Signed-off-by: Andrey Konovalov I'm adding Shuah to CC in case she has some suggestions about the new selftest. Thanks Kees. Reviewed-by: Kees Cook -Kees Looks good to me. Acked-by: Shuah Khan thanks, -- Shuah ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v1 0/10] drm/amd: drop use of drmP.h
Hi Alex. > > Series is: > Reviewed-by: Alex Deucher > I'm fine to have this go through either drm-misc or my tree. Thanks, pushed to drm-misc-next. I ended up with a merge error in drm-tip that I dunno how to work with. Help would be appreciated. (I also wrote this on irc) It is getting late here, I hope someone has fixed it tomorrow morning. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/2] drm/amdkfd: Add device to topology after it is completely inited
We can't have devices that are not completely initialized in kfd topology.
Otherwise it is a race condition when user access not completely
initialized device. This also addresses a kfd_topology_add_device accessing
NULL dqm pointer issue.
Change-Id: I53a0fe498e6daaeb63aa8d78362df01955f14a2f
Signed-off-by: Oak Zeng
---
drivers/gpu/drm/amd/amdkfd/kfd_device.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index 9d1b026..ebac7d7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -603,11 +603,6 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
if (kfd->kfd2kgd->get_hive_id)
kfd->hive_id = kfd->kfd2kgd->get_hive_id(kfd->kgd);
- if (kfd_topology_add_device(kfd)) {
- dev_err(kfd_device, "Error adding device to topology\n");
- goto kfd_topology_add_device_error;
- }
-
if (kfd_interrupt_init(kfd)) {
dev_err(kfd_device, "Error initializing interrupts\n");
goto kfd_interrupt_error;
@@ -631,6 +626,11 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
kfd->dbgmgr = NULL;
+ if (kfd_topology_add_device(kfd)) {
+ dev_err(kfd_device, "Error adding device to topology\n");
+ goto kfd_topology_add_device_error;
+ }
+
kfd->init_complete = true;
dev_info(kfd_device, "added device %x:%x\n", kfd->pdev->vendor,
kfd->pdev->device);
@@ -640,14 +640,13 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
goto out;
+kfd_topology_add_device_error:
kfd_resume_error:
device_iommu_error:
device_queue_manager_uninit(kfd->dqm);
device_queue_manager_error:
kfd_interrupt_exit(kfd);
kfd_interrupt_error:
- kfd_topology_remove_device(kfd);
-kfd_topology_add_device_error:
kfd_doorbell_fini(kfd);
kfd_doorbell_error:
kfd_gtt_sa_fini(kfd);
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/2] drm/amdkfd: Initialize HSA_CAP_ATS_PRESENT capability in topology codes
Move HSA_CAP_ATS_PRESENT initialization logic from kfd iommu codes to
kfd topology codes. This removes kfd_iommu_device_init's dependency
on kfd_topology_add_device. Also remove duplicate code setting the
same.
Change-Id: I2bf3c000d4795b41afe1b39fe679677c1f86bfbe
Signed-off-by: Oak Zeng
---
drivers/gpu/drm/amd/amdkfd/kfd_iommu.c| 10 +-
drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 6 +-
2 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
index 0149475..5f35df2 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
@@ -66,16 +66,8 @@ int kfd_iommu_device_init(struct kfd_dev *kfd)
top_dev = kfd_topology_device_by_id(kfd->id);
- /*
-* Overwrite ATS capability according to needs_iommu_device to fix
-* potential missing corresponding bit in CRAT of BIOS.
-*/
- if (!kfd->device_info->needs_iommu_device) {
- top_dev->node_props.capability &= ~HSA_CAP_ATS_PRESENT;
+ if (!kfd->device_info->needs_iommu_device)
return 0;
- }
-
- top_dev->node_props.capability |= HSA_CAP_ATS_PRESENT;
iommu_info.flags = 0;
err = amd_iommu_device_info(kfd->pdev, &iommu_info);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index d241a86..1c184b3 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -1330,6 +1330,11 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
dev->gpu->device_info->asic_family);
}
+ if (dev->gpu->device_info->needs_iommu_device)
+ dev->node_props.capability |= HSA_CAP_ATS_PRESENT;
+ else
+ dev->node_props.capability &= ~HSA_CAP_ATS_PRESENT;
+
/* Fix errors in CZ CRAT.
* simd_count: Carrizo CRAT reports wrong simd_count, probably
* because it doesn't consider masked out CUs
@@ -1340,7 +1345,6 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
dev->node_props.simd_count =
cu_info.simd_per_cu * cu_info.cu_active_number;
dev->node_props.max_waves_per_simd = 10;
- dev->node_props.capability |= HSA_CAP_ATS_PRESENT;
}
ctx = amdgpu_ras_get_context((struct amdgpu_device *)(dev->gpu->kgd));
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
On Mon, Jun 10, 2019 at 07:53:30PM +0100, Catalin Marinas wrote:
> On Mon, Jun 10, 2019 at 11:07:03AM -0700, Kees Cook wrote:
> > On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote:
> > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > > index 3767fb21a5b8..fd191c5b92aa 100644
> > > --- a/arch/arm64/kernel/process.c
> > > +++ b/arch/arm64/kernel/process.c
> > > @@ -552,3 +552,18 @@ void arch_setup_new_exec(void)
> > >
> > > ptrauth_thread_init_user(current);
> > > }
> > > +
> > > +/*
> > > + * Enable the relaxed ABI allowing tagged user addresses into the kernel.
> > > + */
> > > +int untagged_uaddr_set_mode(unsigned long arg)
> > > +{
> > > + if (is_compat_task())
> > > + return -ENOTSUPP;
> > > + if (arg)
> > > + return -EINVAL;
> > > +
> > > + set_thread_flag(TIF_UNTAGGED_UADDR);
> > > +
> > > + return 0;
> > > +}
> >
> > I think this should be paired with a flag clearing in copy_thread(),
> > yes? (i.e. each binary needs to opt in)
>
> It indeed needs clearing though not in copy_thread() as that's used on
> clone/fork but rather in flush_thread(), called on the execve() path.
Ah! Yes, thanks.
> And a note to myself: I think PR_UNTAGGED_ADDR (not UADDR) looks better
> in a uapi header, the user doesn't differentiate between uaddr and
> kaddr.
Good point. I would agree. :)
--
Kees Cook
Re: [PATCH v1 0/7] drm/radeon: drop obsolete header files
Hi Alex. > > Series is: > Reviewed-by: Alex Deucher > > Feel free to take it through drm-misc if you want, otherwise, let me > know and I'll take it through my tree. Applied to drm-misc-next. I had accidently left a few empty lines that checkpatch complained about. I fixed these when I applied the patches. Will push it after iy passes my final pre-push build tests. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdkfd: Initialize dqm earlier
Hi Felix,
Kfd_iommu_device_init depends on kfd topology. I will do as you suggested
below. Also I will move the setting of HSA_CAP_ATS_PRESENT from
kfd_iommu_device_init to kfd_topology_add_device, to avoid the dependency.
Regards,
Oak
-Original Message-
From: Kuehling, Felix
Sent: Thursday, June 6, 2019 6:09 PM
To: Zeng, Oak ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdkfd: Initialize dqm earlier
On 2019-06-06 5:51 p.m., Zeng, Oak wrote:
> dqm is referenced in function kfd_toplogy_add_device.
> Move dqm initialization up to avoid NULL pointer reference.
This addresses a pretty unlikely race condition where someone looks at
/sys/kernel/debug/kfd/hqds during the device initialization.
We add devices do the topology before their initialization is successfully
completed. If it fails, we remove the device again. Having devices in the
topology that are not completely initialized yet seems to be the real issue. A
cleaner solution would move kfd_topoglogy_add_device to the end of
kgd2kfd_device_init, so that we only add a device to the topology after they
are successfully and completely initialized. Not sure if there are any
dependencies in the init sequence that would be broken by this, though.
Regards,
Felix
>
> Change-Id: Id6cb2541af129826b7621ceaa8e06e638c7bb122
> Signed-off-by: Oak Zeng
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_device.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 9d1b026..e7e24fe 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -603,6 +603,12 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
> if (kfd->kfd2kgd->get_hive_id)
> kfd->hive_id = kfd->kfd2kgd->get_hive_id(kfd->kgd);
>
> + kfd->dqm = device_queue_manager_init(kfd);
> + if (!kfd->dqm) {
> + dev_err(kfd_device, "Error initializing queue manager\n");
> + goto device_queue_manager_error;
> + }
> +
> if (kfd_topology_add_device(kfd)) {
> dev_err(kfd_device, "Error adding device to topology\n");
> goto kfd_topology_add_device_error; @@ -613,12 +619,6 @@ bool
> kgd2kfd_device_init(struct kfd_dev *kfd,
> goto kfd_interrupt_error;
> }
>
> - kfd->dqm = device_queue_manager_init(kfd);
> - if (!kfd->dqm) {
> - dev_err(kfd_device, "Error initializing queue manager\n");
> - goto device_queue_manager_error;
> - }
> -
> if (kfd_iommu_device_init(kfd)) {
> dev_err(kfd_device, "Error initializing iommuv2\n");
> goto device_iommu_error;
> @@ -642,12 +642,12 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>
> kfd_resume_error:
> device_iommu_error:
> - device_queue_manager_uninit(kfd->dqm);
> -device_queue_manager_error:
> kfd_interrupt_exit(kfd);
> kfd_interrupt_error:
> kfd_topology_remove_device(kfd);
> kfd_topology_add_device_error:
> + device_queue_manager_uninit(kfd->dqm);
> +device_queue_manager_error:
> kfd_doorbell_fini(kfd);
> kfd_doorbell_error:
> kfd_gtt_sa_fini(kfd);
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v1 0/10] drm/amd: drop use of drmP.h
On Sun, Jun 9, 2019 at 6:08 PM Sam Ravnborg wrote: > > This patcset drop all uses of drm_os_linux.h and > drmP.h in drm/amd/. > The patchset depends on the earlier series removing drmP.h > from drm/radeon. > https://lists.freedesktop.org/archives/dri-devel/2019-June/220969.html > > The only dependency os the patch to drm_debugfs.h: > https://lists.freedesktop.org/archives/dri-devel/2019-June/220971.html > > The removal was done in a number of steps, mainly to easy potential reviews > and to allow some parts to be applied if not everything are OK. > The patches are made on top of drm-misc-next. > > There is a single patch touching drm_print.h - this was needed > to prevent adding include of to a lot of files, > because it is required by one of the macros in drm_print.h. > As this patch only adds an include file, it should be straightforward to > apply. > > All patches are build tested with various configs and various architectures. > > In a few cases the include of header files was re-arranged, but in > general the changes are kept to a minimum. > When adding new include files the different blocks of include > failes are seperated by empty lines. > This account for some of the added lines. Series is: Reviewed-by: Alex Deucher I'm fine to have this go through either drm-misc or my tree. Alex > > Sam > > Sam Ravnborg (10): > drm: fix build errors with drm_print.h > drm/amd: drop dependencies on drm_os_linux.h > drm/amd: drop use of drmp.h in os_types.h > drm/amd: drop use of drmP.h in amdgpu.h > drm/amd: drop use of drmP.h in atom.h > drm/amd: drop use of drmP.h from all header files > drm/amd: drop use of drmP.h in powerplay/ > drm/amd: drop use of drmP.h in display/ > drm/amd: drop use of drmP.h in amdgpu/amdgpu* > drm/amd: drop use of drmP.h in remaining files > > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 1 + > drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c| 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c| 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c| 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 -- > drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c| 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++- > drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 7 +-- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 +++- > drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 - > drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 14 +++--- > drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c| 4 +++- > drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 5 - > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 4 +++- > drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_ioc32.c| 3 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 5 - > drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 5 - > drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 5 - > drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c| 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c | 2 +- >
Re: [PATCH v1 0/7] drm/radeon: drop obsolete header files
On Sat, Jun 8, 2019 at 4:03 AM Sam Ravnborg wrote: > > This patchset contains updates to two header files > in include/drm/. > The header files caused build errors due to missing dependencies. > Fixed this so others will not be hit by the same. > > The header file drm_os_linux.h is deprecated and should > no longer be used. > For radeon it was a simple 1:1 replacement of the > used macros + a adding missing include files. > > The remaining patches perpare for and remove the use > of drmP.h in the rest of the driver. > > The patches was all build tested on various architectures, > and as usual alpha resulted in a few extra build issues. > > The patches are made on top of drm-misc-next, but applies > clean to drm-next-5.3-wip branch of the agd5f git tree. Series is: Reviewed-by: Alex Deucher Feel free to take it through drm-misc if you want, otherwise, let me know and I'll take it through my tree. Alex > > Sam > > Sam Ravnborg (7): > drm: drm_crtc.h self-contained > drm: drm_debugfs.h self-contained > drm/radeon: drop dependency on drm_os_linux.h > drm/radeon: drop drmP.h from header files > drm/radeon: prepare header files for drmP.h removal > drm/radeon: drop use of drmP.h (1/2) > drm/radeon: drop use of drmP.h (2/2) > > drivers/gpu/drm/radeon/atom.c | 2 ++ > drivers/gpu/drm/radeon/atom.h | 1 - > drivers/gpu/drm/radeon/atombios_crtc.c | 7 +++-- > drivers/gpu/drm/radeon/atombios_dp.c| 2 +- > drivers/gpu/drm/radeon/atombios_encoders.c | 14 ++ > drivers/gpu/drm/radeon/atombios_i2c.c | 2 +- > drivers/gpu/drm/radeon/btc_dpm.c| 16 ++- > drivers/gpu/drm/radeon/btc_dpm.h| 3 +++ > drivers/gpu/drm/radeon/ci_dpm.c | 14 +- > drivers/gpu/drm/radeon/ci_dpm.h | 1 + > drivers/gpu/drm/radeon/ci_smc.c | 2 +- > drivers/gpu/drm/radeon/cik.c| 18 - > drivers/gpu/drm/radeon/cik_sdma.c | 6 ++--- > drivers/gpu/drm/radeon/clearstate_cayman.h | 2 ++ > drivers/gpu/drm/radeon/clearstate_ci.h | 2 ++ > drivers/gpu/drm/radeon/clearstate_si.h | 2 ++ > drivers/gpu/drm/radeon/cypress_dpm.c| 11 > drivers/gpu/drm/radeon/dce3_1_afmt.c| 2 +- > drivers/gpu/drm/radeon/dce6_afmt.c | 2 +- > drivers/gpu/drm/radeon/evergreen.c | 16 ++- > drivers/gpu/drm/radeon/evergreen_cs.c | 2 +- > drivers/gpu/drm/radeon/evergreen_dma.c | 2 +- > drivers/gpu/drm/radeon/evergreen_hdmi.c | 2 +- > drivers/gpu/drm/radeon/kv_dpm.c | 10 --- > drivers/gpu/drm/radeon/kv_smc.c | 2 +- > drivers/gpu/drm/radeon/ni.c | 17 +++- > drivers/gpu/drm/radeon/ni_dma.c | 2 +- > drivers/gpu/drm/radeon/ni_dpm.c | 16 ++- > drivers/gpu/drm/radeon/r100.c | 36 > ++--- > drivers/gpu/drm/radeon/r100_track.h | 2 ++ > drivers/gpu/drm/radeon/r200.c | 2 +- > drivers/gpu/drm/radeon/r300.c | 18 - > drivers/gpu/drm/radeon/r420.c | 16 +++ > drivers/gpu/drm/radeon/r520.c | 4 +-- > drivers/gpu/drm/radeon/r600.c | 18 - > drivers/gpu/drm/radeon/r600_cs.c| 2 +- > drivers/gpu/drm/radeon/r600_dma.c | 6 ++--- > drivers/gpu/drm/radeon/r600_dpm.c | 2 +- > drivers/gpu/drm/radeon/r600_dpm.h | 2 ++ > drivers/gpu/drm/radeon/r600_hdmi.c | 2 +- > drivers/gpu/drm/radeon/radeon_acpi.c| 13 + > drivers/gpu/drm/radeon/radeon_agp.c | 8 -- > drivers/gpu/drm/radeon/radeon_asic.c| 10 --- > drivers/gpu/drm/radeon/radeon_atombios.c| 5 +++- > drivers/gpu/drm/radeon/radeon_audio.c | 2 +- > drivers/gpu/drm/radeon/radeon_benchmark.c | 2 +- > drivers/gpu/drm/radeon/radeon_bios.c| 12 ++--- > drivers/gpu/drm/radeon/radeon_clocks.c | 9 --- > drivers/gpu/drm/radeon/radeon_combios.c | 5 +++- > drivers/gpu/drm/radeon/radeon_connectors.c | 2 +- > drivers/gpu/drm/radeon/radeon_cs.c | 10 +-- > drivers/gpu/drm/radeon/radeon_cursor.c | 4 ++- > drivers/gpu/drm/radeon/radeon_device.c | 18 - > drivers/gpu/drm/radeon/radeon_display.c | 21 +-- > drivers/gpu/drm/radeon/radeon_dp_auxch.c| 2 +- > drivers/gpu/drm/radeon/radeon_dp_mst.c | 6 +++-- > drivers/gpu/drm/radeon/radeon_drv.c | 19 - > drivers/gpu/drm/radeon/radeon_encoders.c| 5 +++- > drivers/gpu/drm/radeon/radeon_fb.c | 13 ---
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
On Mon, Jun 10, 2019 at 11:07:03AM -0700, Kees Cook wrote:
> On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote:
> > On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote:
> > > diff --git a/arch/arm64/include/asm/uaccess.h
> > > b/arch/arm64/include/asm/uaccess.h
> > > index e5d5f31c6d36..9164ecb5feca 100644
> > > --- a/arch/arm64/include/asm/uaccess.h
> > > +++ b/arch/arm64/include/asm/uaccess.h
> > > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void
> > > __user *addr, unsigned long si
> > > return ret;
> > > }
> > >
> > > -#define access_ok(addr, size)__range_ok(addr, size)
> > > +#define access_ok(addr, size)__range_ok(untagged_addr(addr), size)
[...]
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 3767fb21a5b8..fd191c5b92aa 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -552,3 +552,18 @@ void arch_setup_new_exec(void)
> >
> > ptrauth_thread_init_user(current);
> > }
> > +
> > +/*
> > + * Enable the relaxed ABI allowing tagged user addresses into the kernel.
> > + */
> > +int untagged_uaddr_set_mode(unsigned long arg)
> > +{
> > + if (is_compat_task())
> > + return -ENOTSUPP;
> > + if (arg)
> > + return -EINVAL;
> > +
> > + set_thread_flag(TIF_UNTAGGED_UADDR);
> > +
> > + return 0;
> > +}
>
> I think this should be paired with a flag clearing in copy_thread(),
> yes? (i.e. each binary needs to opt in)
It indeed needs clearing though not in copy_thread() as that's used on
clone/fork but rather in flush_thread(), called on the execve() path.
And a note to myself: I think PR_UNTAGGED_ADDR (not UADDR) looks better
in a uapi header, the user doesn't differentiate between uaddr and
kaddr.
--
Catalin
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote:
> On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote:
> > diff --git a/arch/arm64/include/asm/uaccess.h
> > b/arch/arm64/include/asm/uaccess.h
> > index e5d5f31c6d36..9164ecb5feca 100644
> > --- a/arch/arm64/include/asm/uaccess.h
> > +++ b/arch/arm64/include/asm/uaccess.h
> > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user
> > *addr, unsigned long si
> > return ret;
> > }
> >
> > -#define access_ok(addr, size) __range_ok(addr, size)
> > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size)
>
> I'm going to propose an opt-in method here (RFC for now). We can't have
> a check in untagged_addr() since this is already used throughout the
> kernel for both user and kernel addresses (khwasan) but we can add one
> in __range_ok(). The same prctl() option will be used for controlling
> the precise/imprecise mode of MTE later on. We can use a TIF_ flag here
> assuming that this will be called early on and any cloned thread will
> inherit this.
>
> Anyway, it's easier to paste some diff than explain but Vincenzo can
> fold them into his ABI patches that should really go together with
> these. I added a couple of MTE definitions for prctl() as an example,
> not used currently:
>
> --8<-
> diff --git a/arch/arm64/include/asm/processor.h
> b/arch/arm64/include/asm/processor.h
> index fcd0e691b1ea..2d4cb7e4edab 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -307,6 +307,10 @@ extern void __init minsigstksz_setup(void);
> /* PR_PAC_RESET_KEYS prctl */
> #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg)
>
> +/* PR_UNTAGGED_UADDR prctl */
> +int untagged_uaddr_set_mode(unsigned long arg);
> +#define SET_UNTAGGED_UADDR_MODE(arg) untagged_uaddr_set_mode(arg)
> +
> /*
> * For CONFIG_GCC_PLUGIN_STACKLEAK
> *
> diff --git a/arch/arm64/include/asm/thread_info.h
> b/arch/arm64/include/asm/thread_info.h
> index c285d1ce7186..89ce3c49 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -101,6 +101,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define TIF_SVE 23 /* Scalable Vector Extension in
> use */
> #define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */
> #define TIF_SSBD 25 /* Wants SSB mitigation */
> +#define TIF_UNTAGGED_UADDR 26
>
> #define _TIF_SIGPENDING (1 << TIF_SIGPENDING)
> #define _TIF_NEED_RESCHED(1 << TIF_NEED_RESCHED)
> @@ -116,6 +117,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define _TIF_FSCHECK (1 << TIF_FSCHECK)
> #define _TIF_32BIT (1 << TIF_32BIT)
> #define _TIF_SVE (1 << TIF_SVE)
> +#define _TIF_UNTAGGED_UADDR (1 << TIF_UNTAGGED_UADDR)
>
> #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
>_TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
> diff --git a/arch/arm64/include/asm/uaccess.h
> b/arch/arm64/include/asm/uaccess.h
> index 9164ecb5feca..54f5bbaebbc4 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -73,6 +73,9 @@ static inline unsigned long __range_ok(const void __user
> *addr, unsigned long si
> {
> unsigned long ret, limit = current_thread_info()->addr_limit;
>
> + if (test_thread_flag(TIF_UNTAGGED_UADDR))
> + addr = untagged_addr(addr);
> +
> __chk_user_ptr(addr);
> asm volatile(
> // A + B <= C + 1 for all A,B,C, in four easy steps:
> @@ -94,7 +97,7 @@ static inline unsigned long __range_ok(const void __user
> *addr, unsigned long si
> return ret;
> }
>
> -#define access_ok(addr, size)__range_ok(untagged_addr(addr), size)
> +#define access_ok(addr, size)__range_ok(addr, size)
> #define user_addr_maxget_fs
>
> #define _ASM_EXTABLE(from, to)
> \
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 3767fb21a5b8..fd191c5b92aa 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -552,3 +552,18 @@ void arch_setup_new_exec(void)
>
> ptrauth_thread_init_user(current);
> }
> +
> +/*
> + * Enable the relaxed ABI allowing tagged user addresses into the kernel.
> + */
> +int untagged_uaddr_set_mode(unsigned long arg)
> +{
> + if (is_compat_task())
> + return -ENOTSUPP;
> + if (arg)
> + return -EINVAL;
> +
> + set_thread_flag(TIF_UNTAGGED_UADDR);
> +
> + return 0;
> +}
I think this should be paired with a flag clearing in copy_thread(),
yes? (i.e. each binary needs to opt in)
--
Kees Cook
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote:
> diff --git a/arch/arm64/include/asm/uaccess.h
> b/arch/arm64/include/asm/uaccess.h
> index e5d5f31c6d36..9164ecb5feca 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user
> *addr, unsigned long si
> return ret;
> }
>
> -#define access_ok(addr, size)__range_ok(addr, size)
> +#define access_ok(addr, size)__range_ok(untagged_addr(addr), size)
I'm going to propose an opt-in method here (RFC for now). We can't have
a check in untagged_addr() since this is already used throughout the
kernel for both user and kernel addresses (khwasan) but we can add one
in __range_ok(). The same prctl() option will be used for controlling
the precise/imprecise mode of MTE later on. We can use a TIF_ flag here
assuming that this will be called early on and any cloned thread will
inherit this.
Anyway, it's easier to paste some diff than explain but Vincenzo can
fold them into his ABI patches that should really go together with
these. I added a couple of MTE definitions for prctl() as an example,
not used currently:
--8<-
diff --git a/arch/arm64/include/asm/processor.h
b/arch/arm64/include/asm/processor.h
index fcd0e691b1ea..2d4cb7e4edab 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -307,6 +307,10 @@ extern void __init minsigstksz_setup(void);
/* PR_PAC_RESET_KEYS prctl */
#define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg)
+/* PR_UNTAGGED_UADDR prctl */
+int untagged_uaddr_set_mode(unsigned long arg);
+#define SET_UNTAGGED_UADDR_MODE(arg) untagged_uaddr_set_mode(arg)
+
/*
* For CONFIG_GCC_PLUGIN_STACKLEAK
*
diff --git a/arch/arm64/include/asm/thread_info.h
b/arch/arm64/include/asm/thread_info.h
index c285d1ce7186..89ce3c49 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -101,6 +101,7 @@ void arch_release_task_struct(struct task_struct *tsk);
#define TIF_SVE23 /* Scalable Vector Extension in
use */
#define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */
#define TIF_SSBD 25 /* Wants SSB mitigation */
+#define TIF_UNTAGGED_UADDR 26
#define _TIF_SIGPENDING(1 << TIF_SIGPENDING)
#define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED)
@@ -116,6 +117,7 @@ void arch_release_task_struct(struct task_struct *tsk);
#define _TIF_FSCHECK (1 << TIF_FSCHECK)
#define _TIF_32BIT (1 << TIF_32BIT)
#define _TIF_SVE (1 << TIF_SVE)
+#define _TIF_UNTAGGED_UADDR(1 << TIF_UNTAGGED_UADDR)
#define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
_TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 9164ecb5feca..54f5bbaebbc4 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -73,6 +73,9 @@ static inline unsigned long __range_ok(const void __user
*addr, unsigned long si
{
unsigned long ret, limit = current_thread_info()->addr_limit;
+ if (test_thread_flag(TIF_UNTAGGED_UADDR))
+ addr = untagged_addr(addr);
+
__chk_user_ptr(addr);
asm volatile(
// A + B <= C + 1 for all A,B,C, in four easy steps:
@@ -94,7 +97,7 @@ static inline unsigned long __range_ok(const void __user
*addr, unsigned long si
return ret;
}
-#define access_ok(addr, size) __range_ok(untagged_addr(addr), size)
+#define access_ok(addr, size) __range_ok(addr, size)
#define user_addr_max get_fs
#define _ASM_EXTABLE(from, to) \
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 3767fb21a5b8..fd191c5b92aa 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -552,3 +552,18 @@ void arch_setup_new_exec(void)
ptrauth_thread_init_user(current);
}
+
+/*
+ * Enable the relaxed ABI allowing tagged user addresses into the kernel.
+ */
+int untagged_uaddr_set_mode(unsigned long arg)
+{
+ if (is_compat_task())
+ return -ENOTSUPP;
+ if (arg)
+ return -EINVAL;
+
+ set_thread_flag(TIF_UNTAGGED_UADDR);
+
+ return 0;
+}
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 094bb03b9cc2..4afd5e2980ee 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -229,4 +229,9 @@ struct prctl_mm_map {
# define PR_PAC_APDBKEY(1UL << 3)
# define PR_PAC_APGAKEY(1UL << 4)
+/* Untagged user addresses for arm64 */
+#define PR_UNTAGGED_UADDR 55
+# define PR_MTE_IMPRECISE_CHECK
[PATCH] drm/amdgpu: explicitly set mmGDS_VMID0_BASE to 0
Explicitly set mmGDS_VMID0_BASE to 0. Also update
GDS_VMID0_BASE/_SIZE with direct register writes.
Signed-off-by: James Zhu
---
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 33 +++--
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index ba36a28..78c79e9 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -305,6 +305,7 @@ static int gfx_v9_0_get_cu_info(struct amdgpu_device *adev,
static uint64_t gfx_v9_0_get_gpu_clock_counter(struct amdgpu_device *adev);
static void gfx_v9_0_select_se_sh(struct amdgpu_device *adev, u32 se_num, u32
sh_num, u32 instance);
static void gfx_v9_0_ring_emit_de_meta(struct amdgpu_ring *ring);
+static u64 gfx_v9_0_ring_get_rptr_compute(struct amdgpu_ring *ring);
static void gfx_v9_0_init_golden_registers(struct amdgpu_device *adev)
{
@@ -3630,25 +3631,20 @@ static const struct soc15_reg_entry
sec_ded_counter_registers[] = {
{ SOC15_REG_ENTRY(GC, 0, mmSQC_EDC_CNT3), 0, 4, 6},
};
-
static int gfx_v9_0_do_edc_gds_workarounds(struct amdgpu_device *adev)
{
struct amdgpu_ring *ring = &adev->gfx.compute_ring[0];
- int r;
+ int i, r;
- r = amdgpu_ring_alloc(ring, 17);
+ r = amdgpu_ring_alloc(ring, 7);
if (r) {
DRM_ERROR("amdgpu: GDS workarounds failed to lock ring %s
(%d).\n",
ring->name, r);
return r;
}
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, adev->gds.gds_size);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_BASE, 0x);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, adev->gds.gds_size);
amdgpu_ring_write(ring, PACKET3(PACKET3_DMA_DATA, 5));
amdgpu_ring_write(ring, (PACKET3_DMA_DATA_CP_SYNC |
@@ -3662,18 +3658,19 @@ static int gfx_v9_0_do_edc_gds_workarounds(struct
amdgpu_device *adev)
amdgpu_ring_write(ring, PACKET3_DMA_DATA_CMD_RAW_WAIT |
adev->gds.gds_size);
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, 0x0);
-
amdgpu_ring_commit(ring);
- return 0;
-}
+ for (i = 0; (i < adev->usec_timeout) &&
+ (ring->wptr != gfx_v9_0_ring_get_rptr_compute(ring));
i++)
+ DRM_UDELAY(1);
+
+ if (i >= adev->usec_timeout)
+ r = -ETIMEDOUT;
+
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, 0x);
+ return r;
+}
static int gfx_v9_0_do_edc_gpr_workarounds(struct amdgpu_device *adev)
{
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 11/11] mm/hmm: Remove confusing comment and logic from hmm_release
On Fri, Jun 07, 2019 at 02:37:07PM -0700, Ralph Campbell wrote: > > On 6/6/19 11:44 AM, Jason Gunthorpe wrote: > > From: Jason Gunthorpe > > > > hmm_release() is called exactly once per hmm. ops->release() cannot > > accidentally trigger any action that would recurse back onto > > hmm->mirrors_sem. > > > > This fixes a use after-free race of the form: > > > > CPU0 CPU1 > > hmm_release() > > up_write(&hmm->mirrors_sem); > > hmm_mirror_unregister(mirror) > >down_write(&hmm->mirrors_sem); > >up_write(&hmm->mirrors_sem); > >kfree(mirror) > > mirror->ops->release(mirror) > > > > The only user we have today for ops->release is an empty function, so this > > is unambiguously safe. > > > > As a consequence of plugging this race drivers are not allowed to > > register/unregister mirrors from within a release op. > > > > Signed-off-by: Jason Gunthorpe > > I agree with the analysis above but I'm not sure that release() will > always be an empty function. It might be more efficient to write back > all data migrated to a device "in one pass" instead of relying > on unmap_vmas() calling hmm_start_range_invalidate() per VMA. I think we have to focus on the *current* kernel - and we have two users of release, nouveau_svm.c is empty and amdgpu_mn.c does schedule_work() - so I believe we should go ahead with this simple solution to the actual race today that both of those will suffer from. If we find a need for a more complex version then it can be debated and justified with proper context... Ok? Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: drop the incorrect soft_reset for SRIOV
Reviewed-by: Alex Deucher
From: amd-gfx on behalf of Monk Liu
Sent: Monday, June 10, 2019 2:37 AM
To: amd-gfx@lists.freedesktop.org
Cc: Liu, Monk
Subject: [PATCH] drm/amdgpu: drop the incorrect soft_reset for SRIOV
It's incorrect to do soft reset for SRIOV, when GFX
hang the WREG would stuck there becuase it goes KIQ way.
the GPU reset counter is incorrect: always increase twice
for each timedout
Signed-off-by: Monk Liu
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 8f5026c..ff6976e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -399,7 +399,7 @@ bool amdgpu_ring_soft_recovery(struct amdgpu_ring *ring,
unsigned int vmid,
{
ktime_t deadline = ktime_add_us(ktime_get(), 1);
- if (!ring->funcs->soft_recovery || !fence)
+ if (amdgpu_sriov_vf(ring->adev) || !ring->funcs->soft_recovery ||
!fence)
return false;
atomic_inc(&ring->adev->gpu_reset_counter);
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/amdgpu: Bail out of BO node creation if not enough VRAM
Signed-off-by: Tom St Denis
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 8aea2f21b202..70b4a5a97ed2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -276,7 +276,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
struct drm_mm_node *nodes;
enum drm_mm_insert_mode mode;
unsigned long lpfn, num_nodes, pages_per_node, pages_left;
- uint64_t usage = 0, vis_usage = 0;
+ uint64_t vis_usage = 0;
unsigned i;
int r;
@@ -284,6 +284,13 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
if (!lpfn)
lpfn = man->size;
+ /* bail out quickly if there's likely not enough VRAM for this BO */
+ atomic64_add(mem->num_pages << PAGE_SHIFT, &mgr->usage);
+ if (atomic64_read(&mgr->usage) > adev->gmc.mc_vram_size) {
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, &mgr->usage);
+ return -ENOSPC;
+ }
+
if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
pages_per_node = ~0ul;
num_nodes = 1;
@@ -300,8 +307,10 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
nodes = kvmalloc_array((uint32_t)num_nodes, sizeof(*nodes),
GFP_KERNEL | __GFP_ZERO);
- if (!nodes)
+ if (!nodes) {
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, &mgr->usage);
return -ENOMEM;
+ }
mode = DRM_MM_INSERT_BEST;
if (place->flags & TTM_PL_FLAG_TOPDOWN)
@@ -321,7 +330,6 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
if (unlikely(r))
break;
- usage += nodes[i].size << PAGE_SHIFT;
vis_usage += amdgpu_vram_mgr_vis_size(adev, &nodes[i]);
amdgpu_vram_mgr_virt_start(mem, &nodes[i]);
pages_left -= pages;
@@ -341,14 +349,12 @@ static int amdgpu_vram_mgr_new(struct
ttm_mem_type_manager *man,
if (unlikely(r))
goto error;
- usage += nodes[i].size << PAGE_SHIFT;
vis_usage += amdgpu_vram_mgr_vis_size(adev, &nodes[i]);
amdgpu_vram_mgr_virt_start(mem, &nodes[i]);
pages_left -= pages;
}
spin_unlock(&mgr->lock);
- atomic64_add(usage, &mgr->usage);
atomic64_add(vis_usage, &mgr->vis_usage);
mem->mm_node = nodes;
@@ -359,6 +365,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
while (i--)
drm_mm_remove_node(&nodes[i]);
spin_unlock(&mgr->lock);
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, &mgr->usage);
kvfree(nodes);
return r == -ENOSPC ? 0 : r;
--
2.21.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v16 07/16] mm, arm64: untag user pointers in get_vaddr_frames
On Mon, Jun 03, 2019 at 06:55:09PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > get_vaddr_frames uses provided user pointers for vma lookups, which can > only by done with untagged pointers. Instead of locating and changing > all callers of this function, perform untagging in it. > > Signed-off-by: Andrey Konovalov Acked-by: Catalin Marinas
Re: [PATCH v16 05/16] arm64: untag user pointers passed to memory syscalls
On Mon, Jun 03, 2019 at 06:55:07PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > This patch allows tagged pointers to be passed to the following memory > syscalls: get_mempolicy, madvise, mbind, mincore, mlock, mlock2, mprotect, > mremap, msync, munlock. > > Signed-off-by: Andrey Konovalov I would add in the commit log (and possibly in the code with a comment) that mremap() and mmap() do not currently accept tagged hint addresses. Architectures may interpret the hint tag as a background colour for the corresponding vma. With this: Reviewed-by: Catalin Marinas -- Catalin
Re: [PATCH] drm/amdgpu: Add GDS clearing workaround in later init for gfx9
On 2019-06-08 8:50 a.m., Christian König wrote:
> Yeah, that would be a good idea as well.
>
> Additional to that I suggest to update GDS_VMID0_BASE/_SIZE with
> direct register writes instead of PM4 packets.
I can do so.
James
>
> Christian.
>
> Am 07.06.19 um 22:16 schrieb Shamis, Leonid:
>> James,
>>
>> Do you set GDS_VMID0_BASE to 0?.. I don't see it in your patch.
>>
>> Regards,
>> Leonid
>>
>> On 2019-06-07, 15:42, "Zhu, James" wrote:
>>
>> On 2019-06-07 3:12 p.m., Zhu, James wrote:
>> > On 2019-06-07 2:16 p.m., Alex Deucher wrote:
>> >> On Fri, Jun 7, 2019 at 12:38 PM Zhu, James
>> wrote:
>> >>> Since Hardware bug, GDS exist ECC error after cold boot up,
>> >>> adding GDS clearing workaround in later init for gfx9.
>> >>>
>> >>> Signed-off-by: James Zhu
>> >>> ---
>> >>> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 48
>> +++
>> >>> 1 file changed, 48 insertions(+)
>> >>>
>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> >>> index 76722fc..81f6ba8 100644
>> >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> >>> @@ -3634,6 +3634,50 @@ static const struct soc15_reg_entry
>> sec_ded_counter_registers[] = {
>> >>> { SOC15_REG_ENTRY(GC, 0, mmSQC_EDC_CNT3), 0, 4, 6},
>> >>> };
>> >>>
>> >>> +
>> >>> +static int gfx_v9_0_do_edc_gds_workarounds(struct
>> amdgpu_device *adev)
>> >>> +{
>> >>> + struct amdgpu_ring *ring = &adev->gfx.compute_ring[0];
>> >>> + int r;
>> >>> +
>> >>> + r = amdgpu_ring_alloc(ring, 17);
>> >>> + if (r) {
>> >>> + DRM_ERROR("amdgpu: GDS workarounds failed to
>> lock ring %s (%d).\n",
>> >>> + ring->name, r);
>> >>> + return r;
>> >>> + }
>> >>> +
>> >>> + amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA,
>> 3));
>> >>> + amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
>> >>> + WRITE_DATA_DST_SEL(0));
>> >>> + amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0,
>> mmGDS_VMID0_SIZE));
>> >>> + amdgpu_ring_write(ring, 0);
>> >>> + amdgpu_ring_write(ring, 0x1);
>> >> hardcoded size, please use the size from the driver.
>> >>
>> >>> +
>> >>> + amdgpu_ring_write(ring, PACKET3(PACKET3_DMA_DATA, 5));
>> >>> + amdgpu_ring_write(ring, (PACKET3_DMA_DATA_CP_SYNC |
>> >>> + PACKET3_DMA_DATA_DST_SEL(1) |
>> >>> + PACKET3_DMA_DATA_SRC_SEL(2) |
>> >>> + PACKET3_DMA_DATA_ENGINE(0)));
>> >>> + amdgpu_ring_write(ring, 0);
>> >>> + amdgpu_ring_write(ring, 0);
>> >>> + amdgpu_ring_write(ring, 0);
>> >>> + amdgpu_ring_write(ring, 0);
>> >>> + amdgpu_ring_write(ring,
>> PACKET3_DMA_DATA_CMD_RAW_WAIT | 0x1);
>> >> Instead of hardcoding the size, can you use the gds size from
>> the
>> >> driver (adev->gds.gds_size).
>> > Hi Alex,
>> >
>> > Do you mean adev->gds.mem.total_size?
>> >
>> > But I see below operation in gfx_v9_0_ngg_init.
>> >
>> > adev->gds.mem.total_size -=
>> >
>> > Or you want me to add gds_size in struct amdgpu_gds?
>> >
>> > James
>> Yeah, The amd-staging-drm-next branch has adev->gds.gds_size.
>> James
>> >
>> >> With that fixed:
>> >> Reviewed-by: Alex Deucher
>> >>
>> >>> +
>> >>> + amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA,
>> 3));
>> >>> + amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
>> >>> + WRITE_DATA_DST_SEL(0));
>> >>> + amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0,
>> mmGDS_VMID0_SIZE));
>> >>> + amdgpu_ring_write(ring, 0);
>> >>> + amdgpu_ring_write(ring, 0x0);
>> >>> +
>> >>> + amdgpu_ring_commit(ring);
>> >>> +
>> >>> + return 0;
>> >>> +}
>> >>> +
>> >>> +
>> >>> static int gfx_v9_0_do_edc_gpr_workarounds(struct
>> amdgpu_device *adev)
>> >>> {
>> >>> struct amdgpu_ring *ring =
>> &adev->gfx.compute_ring[0];
>> >>> @@ -3810,6 +3854,10 @@ static int
>> gfx_v9_0_ecc_late_init(void *handle)
>> >>> return 0;
>> >>> }
>> >>>
>> >>> + r = gfx_v9_0_do_edc_gds_workarounds(adev);
>> >>> + if (r)
>> >>> + return r;
>> >>> +
>> >>> /* requires IBs so do in late init after IB pool
>> is initialized */
>> >>> r = gfx_v9_0_do_edc_gpr_workarounds(adev);
>> >>> if (r)
>> >>> --
>> >>> 2.7.4
>> >>>
>> >>> ___
>> >>> amd-gfx mailing lis
Re: [PATCH] drm/amdgpu: Add GDS clearing workaround in later init for gfx9
On 2019-06-07 4:16 p.m., Shamis, Leonid wrote:
> James,
>
> Do you set GDS_VMID0_BASE to 0?.. I don't see it in your patch.
sure I will add it.
JAmes
>
> Regards,
> Leonid
>
> On 2019-06-07, 15:42, "Zhu, James" wrote:
>
>
> On 2019-06-07 3:12 p.m., Zhu, James wrote:
> > On 2019-06-07 2:16 p.m., Alex Deucher wrote:
> >> On Fri, Jun 7, 2019 at 12:38 PM Zhu, James wrote:
> >>> Since Hardware bug, GDS exist ECC error after cold boot up,
> >>> adding GDS clearing workaround in later init for gfx9.
> >>>
> >>> Signed-off-by: James Zhu
> >>> ---
> >>>drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 48
> +++
> >>>1 file changed, 48 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> >>> index 76722fc..81f6ba8 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> >>> @@ -3634,6 +3634,50 @@ static const struct soc15_reg_entry
> sec_ded_counter_registers[] = {
> >>> { SOC15_REG_ENTRY(GC, 0, mmSQC_EDC_CNT3), 0, 4, 6},
> >>>};
> >>>
> >>> +
> >>> +static int gfx_v9_0_do_edc_gds_workarounds(struct amdgpu_device
> *adev)
> >>> +{
> >>> + struct amdgpu_ring *ring = &adev->gfx.compute_ring[0];
> >>> + int r;
> >>> +
> >>> + r = amdgpu_ring_alloc(ring, 17);
> >>> + if (r) {
> >>> + DRM_ERROR("amdgpu: GDS workarounds failed to lock
> ring %s (%d).\n",
> >>> + ring->name, r);
> >>> + return r;
> >>> + }
> >>> +
> >>> + amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
> >>> + amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
> >>> + WRITE_DATA_DST_SEL(0));
> >>> + amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0,
> mmGDS_VMID0_SIZE));
> >>> + amdgpu_ring_write(ring, 0);
> >>> + amdgpu_ring_write(ring, 0x1);
> >> hardcoded size, please use the size from the driver.
> >>
> >>> +
> >>> + amdgpu_ring_write(ring, PACKET3(PACKET3_DMA_DATA, 5));
> >>> + amdgpu_ring_write(ring, (PACKET3_DMA_DATA_CP_SYNC |
> >>> + PACKET3_DMA_DATA_DST_SEL(1) |
> >>> + PACKET3_DMA_DATA_SRC_SEL(2) |
> >>> + PACKET3_DMA_DATA_ENGINE(0)));
> >>> + amdgpu_ring_write(ring, 0);
> >>> + amdgpu_ring_write(ring, 0);
> >>> + amdgpu_ring_write(ring, 0);
> >>> + amdgpu_ring_write(ring, 0);
> >>> + amdgpu_ring_write(ring, PACKET3_DMA_DATA_CMD_RAW_WAIT |
> 0x1);
> >> Instead of hardcoding the size, can you use the gds size from the
> >> driver (adev->gds.gds_size).
> > Hi Alex,
> >
> > Do you mean adev->gds.mem.total_size?
> >
> > But I see below operation in gfx_v9_0_ngg_init.
> >
> > adev->gds.mem.total_size -=
> >
> > Or you want me to add gds_size in struct amdgpu_gds?
> >
> > James
>
> Yeah, The amd-staging-drm-next branch has adev->gds.gds_size.
>
> James
>
> >
> >> With that fixed:
> >> Reviewed-by: Alex Deucher
> >>
> >>> +
> >>> + amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
> >>> + amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
> >>> + WRITE_DATA_DST_SEL(0));
> >>> + amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0,
> mmGDS_VMID0_SIZE));
> >>> + amdgpu_ring_write(ring, 0);
> >>> + amdgpu_ring_write(ring, 0x0);
> >>> +
> >>> + amdgpu_ring_commit(ring);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +
> >>>static int gfx_v9_0_do_edc_gpr_workarounds(struct amdgpu_device
> *adev)
> >>>{
> >>> struct amdgpu_ring *ring = &adev->gfx.compute_ring[0];
> >>> @@ -3810,6 +3854,10 @@ static int gfx_v9_0_ecc_late_init(void
> *handle)
> >>> return 0;
> >>> }
> >>>
> >>> + r = gfx_v9_0_do_edc_gds_workarounds(adev);
> >>> + if (r)
> >>> + return r;
> >>> +
> >>> /* requires IBs so do in late init after IB pool is
> initialized */
> >>> r = gfx_v9_0_do_edc_gpr_workarounds(adev);
> >>> if (r)
> >>> --
> >>> 2.7.4
> >>>
> >>> ___
> >>> amd-gfx mailing list
> >>> amd-gfx@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
___
amd-gfx mailing list
amd-gfx@lists.free
Re: [PATCH v2 hmm 10/11] mm/hmm: Do not use list*_rcu() for hmm->ranges
On Fri, Jun 7, 2019 at 12:15 AM Jason Gunthorpe wrote: > > From: Jason Gunthorpe > > This list is always read and written while holding hmm->lock so there is > no need for the confusing _rcu annotations. > > Signed-off-by: Jason Gunthorpe > Reviewed-by: Jérôme Glisse Acked-by: Souptick Joarder > --- > mm/hmm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/hmm.c b/mm/hmm.c > index c2fecb3ecb11e1..709d138dd49027 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -911,7 +911,7 @@ int hmm_range_register(struct hmm_range *range, > mutex_lock(&hmm->lock); > > range->hmm = hmm; > - list_add_rcu(&range->list, &hmm->ranges); > + list_add(&range->list, &hmm->ranges); > > /* > * If there are any concurrent notifiers we have to wait for them for > @@ -941,7 +941,7 @@ void hmm_range_unregister(struct hmm_range *range) > return; > > mutex_lock(&hmm->lock); > - list_del_rcu(&range->list); > + list_del(&range->list); > mutex_unlock(&hmm->lock); > > /* Drop reference taken by hmm_range_register() */ > -- > 2.21.0 > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 01/11] mm/hmm: fix use after free with struct hmm in the mmu notifiers
I still think sruct hmm should die. We already have a structure used for additional information for drivers having crazly tight integration into the VM, and it is called struct mmu_notifier_mm. We really need to reuse that intead of duplicating it badly. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 09/11] mm/hmm: Poison hmm_range during unregister
On Thu, Jun 06, 2019 at 03:44:36PM -0300, Jason Gunthorpe wrote: > From: Jason Gunthorpe > > Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON > and poison bytes to detect this condition. > > Signed-off-by: Jason Gunthorpe > Reviewed-by: Jérôme Glisse > --- > v2 > - Keep range start/end valid after unregistration (Jerome) > --- > mm/hmm.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/mm/hmm.c b/mm/hmm.c > index 6802de7080d172..c2fecb3ecb11e1 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range) > struct hmm *hmm = range->hmm; > > /* Sanity check this really should not happen. */ > - if (hmm == NULL || range->end <= range->start) > + if (WARN_ON(range->end <= range->start)) > return; > > mutex_lock(&hmm->lock); > @@ -948,7 +948,10 @@ void hmm_range_unregister(struct hmm_range *range) > range->valid = false; > mmput(hmm->mm); > hmm_put(hmm); > - range->hmm = NULL; > + > + /* The range is now invalid, leave it poisoned. */ > + range->valid = false; No need to set valid false again as you just did this 5 lines above. Reviewed-by: Ira Weiny > + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm)); > } > EXPORT_SYMBOL(hmm_range_unregister); > > -- > 2.21.0 > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [RFC] mm/hmm: pass mmu_notifier_range to sync_cpu_device_pagetables
On Sat, Jun 08, 2019 at 02:10:08AM -0700, Christoph Hellwig wrote: > On Fri, Jun 07, 2019 at 05:14:52PM -0700, Ralph Campbell wrote: > > HMM defines its own struct hmm_update which is passed to the > > sync_cpu_device_pagetables() callback function. This is > > sufficient when the only action is to invalidate. However, > > a device may want to know the reason for the invalidation and > > be able to see the new permissions on a range, update device access > > rights or range statistics. Since sync_cpu_device_pagetables() > > can be called from try_to_unmap(), the mmap_sem may not be held > > and find_vma() is not safe to be called. > > Pass the struct mmu_notifier_range to sync_cpu_device_pagetables() > > to allow the full invalidation information to be used. > > > > Signed-off-by: Ralph Campbell > > > > I'm sending this out now since we are updating many of the HMM APIs > > and I think it will be useful. > > This is the right thing to do. But the really right thing is to just > kill the hmm_mirror API entirely and move to mmu_notifiers. At least > for noveau this already is way simpler, although right now it defeats > Jasons patch to avoid allocating the struct hmm in the fault path. > But as said before that can be avoided by just killing struct hmm, > which for many reasons is the right thing to do anyway. > > I've got a series here, which is a bit broken (epecially the last > patch can't work as-is), but should explain where I'm trying to head: > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/hmm-mirror-simplification At least the current hmm approach does rely on the collision retry locking scheme in struct hmm/struct hmm_range for the pagefault side to work right. So, before we can apply patch one in this series we need to fix hmm_vma_fault() and all its varients. Otherwise the driver will be broken. I'm hoping to first define what this locking should be (see other emails to Ralph) then, ideally, see if we can extend mmu notifiers to get it directly withouth hmm stuff. Then we apply your patch one and the hmm ops wrapper dies. Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 12/11] mm/hmm: Fix error flows in hmm_invalidate_range_start
On 6/7/19 9:05 AM, Jason Gunthorpe wrote:
If the trylock on the hmm->mirrors_sem fails the function will return
without decrementing the notifiers that were previously incremented. Since
the caller will not call invalidate_range_end() on EAGAIN this will result
in notifiers becoming permanently incremented and deadlock.
If the sync_cpu_device_pagetables() required blocking the function will
not return EAGAIN even though the device continues to touch the
pages. This is a violation of the mmu notifier contract.
Switch, and rename, the ranges_lock to a spin lock so we can reliably
obtain it without blocking during error unwind.
The error unwind is necessary since the notifiers count must be held
incremented across the call to sync_cpu_device_pagetables() as we cannot
allow the range to become marked valid by a parallel
invalidate_start/end() pair while doing sync_cpu_device_pagetables().
Signed-off-by: Jason Gunthorpe
Reviewed-by: Ralph Campbell
---
include/linux/hmm.h | 2 +-
mm/hmm.c| 77 +++--
2 files changed, 48 insertions(+), 31 deletions(-)
I almost lost this patch - it is part of the series, hasn't been
posted before, and wasn't sent with the rest, sorry.
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index bf013e96525771..0fa8ea34ccef6d 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -86,7 +86,7 @@
struct hmm {
struct mm_struct*mm;
struct kref kref;
- struct mutexlock;
+ spinlock_t ranges_lock;
struct list_headranges;
struct list_headmirrors;
struct mmu_notifier mmu_notifier;
diff --git a/mm/hmm.c b/mm/hmm.c
index 4215edf737ef5b..10103a24e9b7b3 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -68,7 +68,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
init_rwsem(&hmm->mirrors_sem);
hmm->mmu_notifier.ops = NULL;
INIT_LIST_HEAD(&hmm->ranges);
- mutex_init(&hmm->lock);
+ spin_lock_init(&hmm->ranges_lock);
kref_init(&hmm->kref);
hmm->notifiers = 0;
hmm->mm = mm;
@@ -114,18 +114,19 @@ static void hmm_release(struct mmu_notifier *mn, struct
mm_struct *mm)
{
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
+ unsigned long flags;
/* Bail out if hmm is in the process of being freed */
if (!kref_get_unless_zero(&hmm->kref))
return;
- mutex_lock(&hmm->lock);
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
/*
* Since hmm_range_register() holds the mmget() lock hmm_release() is
* prevented as long as a range exists.
*/
WARN_ON(!list_empty(&hmm->ranges));
- mutex_unlock(&hmm->lock);
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
down_read(&hmm->mirrors_sem);
list_for_each_entry(mirror, &hmm->mirrors, list) {
@@ -141,6 +142,23 @@ static void hmm_release(struct mmu_notifier *mn, struct
mm_struct *mm)
hmm_put(hmm);
}
+static void notifiers_decrement(struct hmm *hmm)
+{
+ lockdep_assert_held(&hmm->ranges_lock);
+
+ hmm->notifiers--;
+ if (!hmm->notifiers) {
+ struct hmm_range *range;
+
+ list_for_each_entry(range, &hmm->ranges, list) {
+ if (range->valid)
+ continue;
+ range->valid = true;
+ }
This just effectively sets all ranges to valid.
I'm not sure that is best.
Shouldn't hmm_range_register() start with range.valid = true and
then hmm_invalidate_range_start() set affected ranges to false?
Then this becomes just wake_up_all() if --notifiers == 0 and
hmm_range_wait_until_valid() should wait for notifiers == 0.
Otherwise, range.valid doesn't really mean it's valid.
+ wake_up_all(&hmm->wq);
+ }
+}
+
static int hmm_invalidate_range_start(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
{
@@ -148,6 +166,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier
*mn,
struct hmm_mirror *mirror;
struct hmm_update update;
struct hmm_range *range;
+ unsigned long flags;
int ret = 0;
if (!kref_get_unless_zero(&hmm->kref))
@@ -158,12 +177,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier
*mn,
update.event = HMM_UPDATE_INVALIDATE;
update.blockable = mmu_notifier_range_blockable(nrange);
- if (mmu_notifier_range_blockable(nrange))
- mutex_lock(&hmm->lock);
- else if (!mutex_trylock(&hmm->lock)) {
- ret = -EAGAIN;
- goto out;
- }
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
hmm->notifiers++;
list_for_each_entry(range, &hmm->ranges, list) {
if (update.end < range->start || update.start >= rang
Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
On Fri, Jun 07, 2019 at 03:39:06PM -0700, Ralph Campbell wrote: > > > + range->hmm = hmm; > > > + kref_get(&hmm->kref); > > > /* Initialize range to track CPU page table updates. */ > > > mutex_lock(&hmm->lock); > > > > > I forgot to add that I think you can delete the duplicate > "range->hmm = hmm;" > here between the mutex_lock/unlock. Done, thanks Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
On Fri, Jun 07, 2019 at 03:13:00PM -0700, Ralph Campbell wrote:
> > Do you see a reason why the find_vma() ever needs to be before the
> > 'again' in my above example? range.vma does not need to be set for
> > range_register.
>
> Yes, for the GPU case, there can be many faults in an event queue
> and the goal is to try to handle more than one page at a time.
> The vma is needed to limit the amount of coalescing and checking
> for pages that could be speculatively migrated or mapped.
I'd need to see an algorithm sketch to see what you are thinking..
But, I guess a driver would have figure out a list of what virtual
pages it wants to fault under the mmap sem (maybe use find_vam, etc),
then drop mmap_sem, and start processing those pages for mirroring
under the hmm side.
ie they are two seperate unrelated tasks.
I looked at the hmm.rst again, and that reference algorithm is already
showing that that upon retry the mmap_sem is released:
take_lock(driver->update);
if (!range.valid) {
release_lock(driver->update);
up_read(&mm->mmap_sem);
goto again;
So a driver cannot assume that any VMA related work done under the
mmap before the hmm 'critical section' can carry into the hmm critical
section as the lock can be released upon retry and invalidate all that
data..
Forcing the hmm_range_start_and_lock() to acquire the mmap_sem is a
rough way to force the driver author to realize there are two locking
domains and lock protected information cannot cross between.
> OK, I understand.
> If you come up with a set of changes, I can try testing them.
Thanks, I make a sketch of the patch, I have to get back to it after
this series is done.
Jason
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 09/11] mm/hmm: Poison hmm_range during unregister
On 6/6/19 11:44 AM, Jason Gunthorpe wrote: From: Jason Gunthorpe Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON and poison bytes to detect this condition. Signed-off-by: Jason Gunthorpe Reviewed-by: Jérôme Glisse Reviewed-by: Ralph Campbell --- v2 - Keep range start/end valid after unregistration (Jerome) --- mm/hmm.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/mm/hmm.c b/mm/hmm.c index 6802de7080d172..c2fecb3ecb11e1 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range) struct hmm *hmm = range->hmm; /* Sanity check this really should not happen. */ - if (hmm == NULL || range->end <= range->start) + if (WARN_ON(range->end <= range->start)) return; WARN_ON() is definitely better than silent return but I wonder how useful it is since the caller shouldn't be modifying the hmm_range once it is registered. Other fields could be changed too... mutex_lock(&hmm->lock); @@ -948,7 +948,10 @@ void hmm_range_unregister(struct hmm_range *range) range->valid = false; mmput(hmm->mm); hmm_put(hmm); - range->hmm = NULL; + + /* The range is now invalid, leave it poisoned. */ + range->valid = false; + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm)); } EXPORT_SYMBOL(hmm_range_unregister); ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
On Fri, Jun 07, 2019 at 10:31:07AM -0300, Jason Gunthorpe wrote:
> The wait_event_timeout macro already tests the condition as its first
> action, so there is no reason to open code another version of this, all
> that does is skip the might_sleep() debugging in common cases, which is
> not helpful.
>
> Further, based on prior patches, we can now simplify the required condition
> test:
> - If range is valid memory then so is range->hmm
> - If hmm_release() has run then range->valid is set to false
>at the same time as dead, so no reason to check both.
> - A valid hmm has a valid hmm->mm.
>
> Allowing the return value of wait_event_timeout() (along with its internal
> barriers) to compute the result of the function.
>
> Signed-off-by: Jason Gunthorpe
Reviewed-by: Ira Weiny
> ---
> v3
> - Simplify the wait_event_timeout to not check valid
> ---
> include/linux/hmm.h | 13 ++---
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 1d97b6d62c5bcf..26e7c477490c4e 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -209,17 +209,8 @@ static inline unsigned long hmm_range_page_size(const
> struct hmm_range *range)
> static inline bool hmm_range_wait_until_valid(struct hmm_range *range,
> unsigned long timeout)
> {
> - /* Check if mm is dead ? */
> - if (range->hmm == NULL || range->hmm->dead || range->hmm->mm == NULL) {
> - range->valid = false;
> - return false;
> - }
> - if (range->valid)
> - return true;
> - wait_event_timeout(range->hmm->wq, range->valid || range->hmm->dead,
> -msecs_to_jiffies(timeout));
> - /* Return current valid status just in case we get lucky */
> - return range->valid;
> + return wait_event_timeout(range->hmm->wq, range->valid,
> + msecs_to_jiffies(timeout)) != 0;
> }
>
> /*
> --
> 2.21.0
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 01/11] mm/hmm: fix use after free with struct hmm in the mmu notifiers
On Sat, Jun 08, 2019 at 01:49:48AM -0700, Christoph Hellwig wrote: > I still think sruct hmm should die. We already have a structure used > for additional information for drivers having crazly tight integration > into the VM, and it is called struct mmu_notifier_mm. We really need > to reuse that intead of duplicating it badly. Probably. But at least in ODP we needed something very similar to 'struct hmm' to make our mmu notifier implementation work. The mmu notifier api really lends itself to having a per-mm structure in the driver to hold the 'struct mmu_notifier'.. I think I see other drivers are doing things like assuming that there is only one mm in their world (despite being FD based, so this is not really guarenteed) So, my first attempt would be an api something like: priv = mmu_notififer_attach_mm(ops, current->mm, sizeof(my_priv)) mmu_notifier_detach_mm(priv); ops->invalidate_start(struct mmu_notififer *mn): struct p *priv = mmu_notifier_priv(mn); Such that - There is only one priv per mm - All the srcu stuff is handled inside mmu notifier - It is reference counted, so ops can be attached multiple times to the same mm Then odp's per_mm, and struct hmm (if we keep it at all) is simply a 'priv' in the above. I was thinking of looking at this stuff next, once this series is done. Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 01/11] mm/hmm: fix use after free with struct hmm in the mmu notifiers
On 6/7/19 5:34 AM, Jason Gunthorpe wrote:
> On Thu, Jun 06, 2019 at 07:29:08PM -0700, John Hubbard wrote:
>> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
>>> From: Jason Gunthorpe
>> ...
>>> @@ -153,10 +158,14 @@ void hmm_mm_destroy(struct mm_struct *mm)
>>>
>>> static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
>>> {
>>> - struct hmm *hmm = mm_get_hmm(mm);
>>> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
>>> struct hmm_mirror *mirror;
>>> struct hmm_range *range;
>>>
>>> + /* hmm is in progress to free */
>>
>> Well, sometimes, yes. :)
>
> It think it is in all cases actually.. The only way we see a 0 kref
> and still reach this code path is if another thread has alreay setup
> the hmm_free in the call_srcu..
>
>> Maybe this wording is clearer (if we need any comment at all):
>
> I always find this hard.. This is a very standard pattern when working
> with RCU - however in my experience few people actually know the RCU
> patterns, and missing the _unless_zero is a common bug I find when
> looking at code.
>
> This is mm/ so I can drop it, what do you think?
>
I forgot to respond to this section, so catching up now:
I think we're talking about slightly different things. I was just
noting that the comment above the "if" statement was only accurate
if the branch is taken, which is why I recommended this combination
of comment and code:
/* Bail out if hmm is in the process of being freed */
if (!kref_get_unless_zero(&hmm->kref))
return;
As for the actual _unless_zero part, I think that's good to have.
And it's a good reminder if nothing else, even in mm/ code.
thanks,
--
John Hubbard
NVIDIA
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [RFC] mm/hmm: pass mmu_notifier_range to sync_cpu_device_pagetables
On Fri, Jun 07, 2019 at 05:14:52PM -0700, Ralph Campbell wrote: > HMM defines its own struct hmm_update which is passed to the > sync_cpu_device_pagetables() callback function. This is > sufficient when the only action is to invalidate. However, > a device may want to know the reason for the invalidation and > be able to see the new permissions on a range, update device access > rights or range statistics. Since sync_cpu_device_pagetables() > can be called from try_to_unmap(), the mmap_sem may not be held > and find_vma() is not safe to be called. > Pass the struct mmu_notifier_range to sync_cpu_device_pagetables() > to allow the full invalidation information to be used. > > Signed-off-by: Ralph Campbell > --- > > I'm sending this out now since we are updating many of the HMM APIs > and I think it will be useful. I agree with CH that struct hmm_update seems particularly pointless and we really should just use mmu_notifier_range directly. We need to find out from the DRM folks if we can merge this kind of stuff through hmm.git and then resolve any conflicts that might arise in DRM tree or in nouveau tree? But I would like to see this patch go in this cycle, thanks Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
On 6/7/19 6:31 AM, Jason Gunthorpe wrote:
> The wait_event_timeout macro already tests the condition as its first
> action, so there is no reason to open code another version of this, all
> that does is skip the might_sleep() debugging in common cases, which is
> not helpful.
>
> Further, based on prior patches, we can now simplify the required condition
> test:
> - If range is valid memory then so is range->hmm
> - If hmm_release() has run then range->valid is set to false
>at the same time as dead, so no reason to check both.
> - A valid hmm has a valid hmm->mm.
>
> Allowing the return value of wait_event_timeout() (along with its internal
> barriers) to compute the result of the function.
>
> Signed-off-by: Jason Gunthorpe
> ---
Reviewed-by: John Hubbard
thanks,
--
John Hubbard
NVIDIA
> v3
> - Simplify the wait_event_timeout to not check valid
> ---
> include/linux/hmm.h | 13 ++---
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 1d97b6d62c5bcf..26e7c477490c4e 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -209,17 +209,8 @@ static inline unsigned long hmm_range_page_size(const
> struct hmm_range *range)
> static inline bool hmm_range_wait_until_valid(struct hmm_range *range,
> unsigned long timeout)
> {
> - /* Check if mm is dead ? */
> - if (range->hmm == NULL || range->hmm->dead || range->hmm->mm == NULL) {
> - range->valid = false;
> - return false;
> - }
> - if (range->valid)
> - return true;
> - wait_event_timeout(range->hmm->wq, range->valid || range->hmm->dead,
> -msecs_to_jiffies(timeout));
> - /* Return current valid status just in case we get lucky */
> - return range->valid;
> + return wait_event_timeout(range->hmm->wq, range->valid,
> + msecs_to_jiffies(timeout)) != 0;
> }
>
> /*
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 12/11] mm/hmm: Fix error flows in hmm_invalidate_range_start
On Fri, Jun 07, 2019 at 04:52:58PM -0700, Ralph Campbell wrote:
> > @@ -141,6 +142,23 @@ static void hmm_release(struct mmu_notifier *mn,
> > struct mm_struct *mm)
> > hmm_put(hmm);
> > }
> > +static void notifiers_decrement(struct hmm *hmm)
> > +{
> > + lockdep_assert_held(&hmm->ranges_lock);
> > +
> > + hmm->notifiers--;
> > + if (!hmm->notifiers) {
> > + struct hmm_range *range;
> > +
> > + list_for_each_entry(range, &hmm->ranges, list) {
> > + if (range->valid)
> > + continue;
> > + range->valid = true;
> > + }
>
> This just effectively sets all ranges to valid.
> I'm not sure that is best.
This is a trade off, it would be much more expensive to have a precise
'valid = true' - instead this algorithm is precise about 'valid =
false' and lazy about 'valid = true' which is much less costly to
calculate.
> Shouldn't hmm_range_register() start with range.valid = true and
> then hmm_invalidate_range_start() set affected ranges to false?
It kind of does, expect when it doesn't, right? :)
> Then this becomes just wake_up_all() if --notifiers == 0 and
> hmm_range_wait_until_valid() should wait for notifiers == 0.
Almost.. but it is more tricky than that.
This scheme is a collision-retry algorithm. The pagefault side runs to
completion if no parallel invalidate start/end happens.
If a parallel invalidation happens then the pagefault retries.
Seeing notifiers == 0 means there is absolutely no current parallel
invalidation.
Seeing range->valid == true (under the device lock)
means this range doesn't intersect with a parallel invalidate.
So.. hmm_range_wait_until_valid() checks the per-range valid because
it doesn't want to sleep if *this range* is not involved in a parallel
invalidation - but once it becomes involved, then yes, valid == true
implies notifiers == 0.
It is easier/safer to use unlocked variable reads if there is only one
variable, thus the weird construction.
It is unclear to me if this micro optimization is really
worthwhile. It is very expensive to manage all this tracking, and no
other mmu notifier implementation really does something like
this. Eliminating the per-range tracking and using the notifier count
as a global lock would be much simpler...
> Otherwise, range.valid doesn't really mean it's valid.
Right, it doesn't really mean 'valid'
It is tracking possible colliding invalidates such that valid == true
(under the device lock) means that there was no colliding invalidate.
I still think this implementation doesn't quite work, as I described
here:
https://lore.kernel.org/linux-mm/20190527195829.gb18...@mellanox.com/
But the idea is basically sound and matches what other mmu notifier
users do, just using a seqcount like scheme, not a boolean.
Jason
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 04/11] mm/hmm: Simplify hmm_get_or_create and make it reliable
On Thu, Jun 06, 2019 at 03:44:31PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe
>
> As coded this function can false-fail in various racy situations. Make it
> reliable by running only under the write side of the mmap_sem and avoiding
> the false-failing compare/exchange pattern.
>
> Also make the locking very easy to understand by only ever reading or
> writing mm->hmm while holding the write side of the mmap_sem.
>
> Signed-off-by: Jason Gunthorpe
Reviewed-by: Ira Weiny
> ---
> v2:
> - Fix error unwind of mmgrab (Jerome)
> - Use hmm local instead of 2nd container_of (Jerome)
> ---
> mm/hmm.c | 80
> 1 file changed, 29 insertions(+), 51 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index cc7c26fda3300e..dc30edad9a8a02 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -40,16 +40,6 @@
> #if IS_ENABLED(CONFIG_HMM_MIRROR)
> static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
>
> -static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
> -{
> - struct hmm *hmm = READ_ONCE(mm->hmm);
> -
> - if (hmm && kref_get_unless_zero(&hmm->kref))
> - return hmm;
> -
> - return NULL;
> -}
> -
> /**
> * hmm_get_or_create - register HMM against an mm (HMM internal)
> *
> @@ -64,11 +54,20 @@ static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
> */
> static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> {
> - struct hmm *hmm = mm_get_hmm(mm);
> - bool cleanup = false;
> + struct hmm *hmm;
>
> - if (hmm)
> - return hmm;
> + lockdep_assert_held_exclusive(&mm->mmap_sem);
> +
> + if (mm->hmm) {
> + if (kref_get_unless_zero(&mm->hmm->kref))
> + return mm->hmm;
> + /*
> + * The hmm is being freed by some other CPU and is pending a
> + * RCU grace period, but this CPU can NULL now it since we
> + * have the mmap_sem.
> + */
> + mm->hmm = NULL;
> + }
>
> hmm = kmalloc(sizeof(*hmm), GFP_KERNEL);
> if (!hmm)
> @@ -83,57 +82,36 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> hmm->notifiers = 0;
> hmm->dead = false;
> hmm->mm = mm;
> - mmgrab(hmm->mm);
> -
> - spin_lock(&mm->page_table_lock);
> - if (!mm->hmm)
> - mm->hmm = hmm;
> - else
> - cleanup = true;
> - spin_unlock(&mm->page_table_lock);
>
> - if (cleanup)
> - goto error;
> -
> - /*
> - * We should only get here if hold the mmap_sem in write mode ie on
> - * registration of first mirror through hmm_mirror_register()
> - */
> hmm->mmu_notifier.ops = &hmm_mmu_notifier_ops;
> - if (__mmu_notifier_register(&hmm->mmu_notifier, mm))
> - goto error_mm;
> + if (__mmu_notifier_register(&hmm->mmu_notifier, mm)) {
> + kfree(hmm);
> + return NULL;
> + }
>
> + mmgrab(hmm->mm);
> + mm->hmm = hmm;
> return hmm;
> -
> -error_mm:
> - spin_lock(&mm->page_table_lock);
> - if (mm->hmm == hmm)
> - mm->hmm = NULL;
> - spin_unlock(&mm->page_table_lock);
> -error:
> - mmdrop(hmm->mm);
> - kfree(hmm);
> - return NULL;
> }
>
> static void hmm_free_rcu(struct rcu_head *rcu)
> {
> - kfree(container_of(rcu, struct hmm, rcu));
> + struct hmm *hmm = container_of(rcu, struct hmm, rcu);
> +
> + down_write(&hmm->mm->mmap_sem);
> + if (hmm->mm->hmm == hmm)
> + hmm->mm->hmm = NULL;
> + up_write(&hmm->mm->mmap_sem);
> + mmdrop(hmm->mm);
> +
> + kfree(hmm);
> }
>
> static void hmm_free(struct kref *kref)
> {
> struct hmm *hmm = container_of(kref, struct hmm, kref);
> - struct mm_struct *mm = hmm->mm;
> -
> - mmu_notifier_unregister_no_release(&hmm->mmu_notifier, mm);
>
> - spin_lock(&mm->page_table_lock);
> - if (mm->hmm == hmm)
> - mm->hmm = NULL;
> - spin_unlock(&mm->page_table_lock);
> -
> - mmdrop(hmm->mm);
> + mmu_notifier_unregister_no_release(&hmm->mmu_notifier, hmm->mm);
> mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
> }
>
> --
> 2.21.0
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
On 6/7/19 11:24 AM, Ralph Campbell wrote:
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
From: Jason Gunthorpe
Ralph observes that hmm_range_register() can only be called by a driver
while a mirror is registered. Make this clear in the API by passing in
the
mirror structure as a parameter.
This also simplifies understanding the lifetime model for struct hmm, as
the hmm pointer must be valid as part of a registered mirror so all we
need in hmm_register_range() is a simple kref_get.
Suggested-by: Ralph Campbell
Signed-off-by: Jason Gunthorpe
You might CC Ben for the nouveau part.
CC: Ben Skeggs
Reviewed-by: Ralph Campbell
---
v2
- Include the oneline patch to nouveau_svm.c
---
drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +-
include/linux/hmm.h | 7 ---
mm/hmm.c | 15 ++-
3 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
b/drivers/gpu/drm/nouveau/nouveau_svm.c
index 93ed43c413f0bb..8c92374afcf227 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -649,7 +649,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
range.values = nouveau_svm_pfn_values;
range.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT;
again:
- ret = hmm_vma_fault(&range, true);
+ ret = hmm_vma_fault(&svmm->mirror, &range, true);
if (ret == 0) {
mutex_lock(&svmm->mutex);
if (!hmm_vma_range_done(&range)) {
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 688c5ca7068795..2d519797cb134a 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -505,7 +505,7 @@ static inline bool hmm_mirror_mm_is_alive(struct
hmm_mirror *mirror)
* Please see Documentation/vm/hmm.rst for how to use the range API.
*/
int hmm_range_register(struct hmm_range *range,
- struct mm_struct *mm,
+ struct hmm_mirror *mirror,
unsigned long start,
unsigned long end,
unsigned page_shift);
@@ -541,7 +541,8 @@ static inline bool hmm_vma_range_done(struct
hmm_range *range)
}
/* This is a temporary helper to avoid merge conflict between trees. */
-static inline int hmm_vma_fault(struct hmm_range *range, bool block)
+static inline int hmm_vma_fault(struct hmm_mirror *mirror,
+ struct hmm_range *range, bool block)
{
long ret;
@@ -554,7 +555,7 @@ static inline int hmm_vma_fault(struct hmm_range
*range, bool block)
range->default_flags = 0;
range->pfn_flags_mask = -1UL;
- ret = hmm_range_register(range, range->vma->vm_mm,
+ ret = hmm_range_register(range, mirror,
range->start, range->end,
PAGE_SHIFT);
if (ret)
diff --git a/mm/hmm.c b/mm/hmm.c
index 547002f56a163d..8796447299023c 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -925,13 +925,13 @@ static void hmm_pfns_clear(struct hmm_range *range,
* Track updates to the CPU page table see include/linux/hmm.h
*/
int hmm_range_register(struct hmm_range *range,
- struct mm_struct *mm,
+ struct hmm_mirror *mirror,
unsigned long start,
unsigned long end,
unsigned page_shift)
{
unsigned long mask = ((1UL << page_shift) - 1UL);
- struct hmm *hmm;
+ struct hmm *hmm = mirror->hmm;
range->valid = false;
range->hmm = NULL;
@@ -945,15 +945,12 @@ int hmm_range_register(struct hmm_range *range,
range->start = start;
range->end = end;
- hmm = hmm_get_or_create(mm);
- if (!hmm)
- return -EFAULT;
-
/* Check if hmm_mm_destroy() was call. */
- if (hmm->mm == NULL || hmm->dead) {
- hmm_put(hmm);
+ if (hmm->mm == NULL || hmm->dead)
return -EFAULT;
- }
+
+ range->hmm = hmm;
+ kref_get(&hmm->kref);
/* Initialize range to track CPU page table updates. */
mutex_lock(&hmm->lock);
I forgot to add that I think you can delete the duplicate
"range->hmm = hmm;"
here between the mutex_lock/unlock.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
FYI, I very much disagree with the direction this is moving. struct hmm_mirror literally is a trivial duplication of the mmu_notifiers. All these drivers should just use the mmu_notifiers directly for the mirroring part instead of building a thing wrapper that adds nothing but helping to manage the lifetime of struct hmm, which shouldn't exist to start with. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [RFC] mm/hmm: pass mmu_notifier_range to sync_cpu_device_pagetables
On Fri, Jun 07, 2019 at 05:14:52PM -0700, Ralph Campbell wrote:
> HMM defines its own struct hmm_update which is passed to the
> sync_cpu_device_pagetables() callback function. This is
> sufficient when the only action is to invalidate. However,
> a device may want to know the reason for the invalidation and
> be able to see the new permissions on a range, update device access
> rights or range statistics. Since sync_cpu_device_pagetables()
> can be called from try_to_unmap(), the mmap_sem may not be held
> and find_vma() is not safe to be called.
> Pass the struct mmu_notifier_range to sync_cpu_device_pagetables()
> to allow the full invalidation information to be used.
>
> Signed-off-by: Ralph Campbell
I don't disagree with Christoph or Jason but since I've been trying to sort out
where hmm does and does not fit any chance to remove a custom structure is a
good simplification IMO. So...
Reviewed-by: Ira Weiny
> ---
>
> I'm sending this out now since we are updating many of the HMM APIs
> and I think it will be useful.
>
>
> drivers/gpu/drm/nouveau/nouveau_svm.c | 4 ++--
> include/linux/hmm.h | 27 ++-
> mm/hmm.c | 13 -
> 3 files changed, 8 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
> b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index 8c92374afcf2..c34b98fafe2f 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -252,13 +252,13 @@ nouveau_svmm_invalidate(struct nouveau_svmm *svmm, u64
> start, u64 limit)
>
> static int
> nouveau_svmm_sync_cpu_device_pagetables(struct hmm_mirror *mirror,
> - const struct hmm_update *update)
> + const struct mmu_notifier_range *update)
> {
> struct nouveau_svmm *svmm = container_of(mirror, typeof(*svmm), mirror);
> unsigned long start = update->start;
> unsigned long limit = update->end;
>
> - if (!update->blockable)
> + if (!mmu_notifier_range_blockable(update))
> return -EAGAIN;
>
> SVMM_DBG(svmm, "invalidate %016lx-%016lx", start, limit);
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 0fa8ea34ccef..07a2d38fde34 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -377,29 +377,6 @@ static inline uint64_t hmm_pfn_from_pfn(const struct
> hmm_range *range,
>
> struct hmm_mirror;
>
> -/*
> - * enum hmm_update_event - type of update
> - * @HMM_UPDATE_INVALIDATE: invalidate range (no indication as to why)
> - */
> -enum hmm_update_event {
> - HMM_UPDATE_INVALIDATE,
> -};
> -
> -/*
> - * struct hmm_update - HMM update information for callback
> - *
> - * @start: virtual start address of the range to update
> - * @end: virtual end address of the range to update
> - * @event: event triggering the update (what is happening)
> - * @blockable: can the callback block/sleep ?
> - */
> -struct hmm_update {
> - unsigned long start;
> - unsigned long end;
> - enum hmm_update_event event;
> - bool blockable;
> -};
> -
> /*
> * struct hmm_mirror_ops - HMM mirror device operations callback
> *
> @@ -420,7 +397,7 @@ struct hmm_mirror_ops {
> /* sync_cpu_device_pagetables() - synchronize page tables
>*
>* @mirror: pointer to struct hmm_mirror
> - * @update: update information (see struct hmm_update)
> + * @update: update information (see struct mmu_notifier_range)
>* Return: -EAGAIN if update.blockable false and callback need to
>* block, 0 otherwise.
>*
> @@ -434,7 +411,7 @@ struct hmm_mirror_ops {
>* synchronous call.
>*/
> int (*sync_cpu_device_pagetables)(struct hmm_mirror *mirror,
> - const struct hmm_update *update);
> + const struct mmu_notifier_range *update);
> };
>
> /*
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 9aad3550f2bb..b49a43712554 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -164,7 +164,6 @@ static int hmm_invalidate_range_start(struct mmu_notifier
> *mn,
> {
> struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> struct hmm_mirror *mirror;
> - struct hmm_update update;
> struct hmm_range *range;
> unsigned long flags;
> int ret = 0;
> @@ -173,15 +172,10 @@ static int hmm_invalidate_range_start(struct
> mmu_notifier *mn,
> if (!kref_get_unless_zero(&hmm->kref))
> return 0;
>
> - update.start = nrange->start;
> - update.end = nrange->end;
> - update.event = HMM_UPDATE_INVALIDATE;
> - update.blockable = mmu_notifier_range_blockable(nrange);
> -
> spin_lock_irqsave(&hmm->ranges_lock, flags);
> hmm->notifiers++;
> list_for_each_entry(range, &hmm->ranges, list) {
> - if (update.end < range->start || update.start >= range->end)
> +
Re: [PATCH v2 hmm 09/11] mm/hmm: Poison hmm_range during unregister
On Fri, Jun 07, 2019 at 01:46:30PM -0700, Ralph Campbell wrote: > > On 6/6/19 11:44 AM, Jason Gunthorpe wrote: > > From: Jason Gunthorpe > > > > Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON > > and poison bytes to detect this condition. > > > > Signed-off-by: Jason Gunthorpe > > Reviewed-by: Jérôme Glisse > > Reviewed-by: Ralph Campbell > > > v2 > > - Keep range start/end valid after unregistration (Jerome) > > mm/hmm.c | 7 +-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/mm/hmm.c b/mm/hmm.c > > index 6802de7080d172..c2fecb3ecb11e1 100644 > > +++ b/mm/hmm.c > > @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range) > > struct hmm *hmm = range->hmm; > > /* Sanity check this really should not happen. */ > > - if (hmm == NULL || range->end <= range->start) > > + if (WARN_ON(range->end <= range->start)) > > return; > > WARN_ON() is definitely better than silent return but I wonder how > useful it is since the caller shouldn't be modifying the hmm_range > once it is registered. Other fields could be changed too... I deleted the warn on (see the other thread), but I'm confused by your "shouldn't be modified" statement. The only thing that needs to be set and remain unchanged for register is the virtual start/end address. Everything else should be done once it is clear to proceed based on the collision-retry locking scheme this uses. Basically the range register only setups a 'detector' for colliding invalidations. The other stuff in the struct is just random temporary storage for the API. AFAICS at least.. Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 01/11] mm/hmm: fix use after free with struct hmm in the mmu notifiers
On 6/7/19 5:34 AM, Jason Gunthorpe wrote:
> On Thu, Jun 06, 2019 at 07:29:08PM -0700, John Hubbard wrote:
>> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
>>> From: Jason Gunthorpe
>> ...
>>> diff --git a/mm/hmm.c b/mm/hmm.c
>>> index 8e7403f081f44a..547002f56a163d 100644
>>> +++ b/mm/hmm.c
>> ...
>>> @@ -125,7 +130,7 @@ static void hmm_free(struct kref *kref)
>>> mm->hmm = NULL;
>>> spin_unlock(&mm->page_table_lock);
>>>
>>> - kfree(hmm);
>>> + mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
>>
>>
>> It occurred to me to wonder if it is best to use the MMU notifier's
>> instance of srcu, instead of creating a separate instance for HMM.
>
> It *has* to be the MMU notifier SRCU because we are synchornizing
> against the read side of that SRU inside the mmu notifier code, ie:
>
> int __mmu_notifier_invalidate_range_start(struct mmu_notifier_range *range)
> id = srcu_read_lock(&srcu);
> hlist_for_each_entry_rcu(mn, &range->mm->mmu_notifier_mm->list,
> hlist) {
> if (mn->ops->invalidate_range_start) {
>^
>
> Here 'mn' is really hmm (hmm = container_of(mn, struct hmm,
> mmu_notifier)), so we must protect the memory against free for the mmu
> notifier core.
>
> Thus we have no choice but to use its SRCU.
Ah right. It's embarassingly obvious when you say it out loud. :)
Thanks for explaining.
>
> CH also pointed out a more elegant solution, which is to get the write
> side of the mmap_sem during hmm_mirror_unregister - no notifier
> callback can be running in this case. Then we delete the kref, srcu
> and so forth.
>
> This is much clearer/saner/better, but.. requries the callers of
> hmm_mirror_unregister to be safe to get the mmap_sem write side.
>
> I think this is true, so maybe this patch should be switched, what do
> you think?
OK, your follow-up notes that we'll leave it as is, got it.
thanks,
--
John Hubbard
NVIDIA
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 07/11] mm/hmm: Use lockdep instead of comments
On Fri, Jun 7, 2019 at 12:15 AM Jason Gunthorpe wrote:
>
> From: Jason Gunthorpe
>
> So we can check locking at runtime.
Little more descriptive change log would be helpful.
Acked-by: Souptick Joarder
>
> Signed-off-by: Jason Gunthorpe
> Reviewed-by: Jérôme Glisse
> ---
> v2
> - Fix missing & in lockdeps (Jason)
> ---
> mm/hmm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index f67ba32983d9f1..c702cd72651b53 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -254,11 +254,11 @@ static const struct mmu_notifier_ops
> hmm_mmu_notifier_ops = {
> *
> * To start mirroring a process address space, the device driver must
> register
> * an HMM mirror struct.
> - *
> - * THE mm->mmap_sem MUST BE HELD IN WRITE MODE !
> */
> int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm)
> {
> + lockdep_assert_held_exclusive(&mm->mmap_sem);
> +
> /* Sanity check */
> if (!mm || !mirror || !mirror->ops)
> return -EINVAL;
> --
> 2.21.0
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [RFC] mm/hmm: pass mmu_notifier_range to sync_cpu_device_pagetables
On 6/8/19 4:41 AM, Jason Gunthorpe wrote: > On Sat, Jun 08, 2019 at 02:10:08AM -0700, Christoph Hellwig wrote: >> On Fri, Jun 07, 2019 at 05:14:52PM -0700, Ralph Campbell wrote: >>> HMM defines its own struct hmm_update which is passed to the >>> sync_cpu_device_pagetables() callback function. This is >>> sufficient when the only action is to invalidate. However, >>> a device may want to know the reason for the invalidation and >>> be able to see the new permissions on a range, update device access >>> rights or range statistics. Since sync_cpu_device_pagetables() >>> can be called from try_to_unmap(), the mmap_sem may not be held >>> and find_vma() is not safe to be called. >>> Pass the struct mmu_notifier_range to sync_cpu_device_pagetables() >>> to allow the full invalidation information to be used. >>> >>> Signed-off-by: Ralph Campbell >>> >>> I'm sending this out now since we are updating many of the HMM APIs >>> and I think it will be useful. >> >> This is the right thing to do. But the really right thing is to just >> kill the hmm_mirror API entirely and move to mmu_notifiers. At least >> for noveau this already is way simpler, although right now it defeats >> Jasons patch to avoid allocating the struct hmm in the fault path. >> But as said before that can be avoided by just killing struct hmm, >> which for many reasons is the right thing to do anyway. >> >> I've got a series here, which is a bit broken (epecially the last >> patch can't work as-is), but should explain where I'm trying to head: >> >> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/hmm-mirror-simplification > > At least the current hmm approach does rely on the collision retry > locking scheme in struct hmm/struct hmm_range for the pagefault side > to work right. > > So, before we can apply patch one in this series we need to fix > hmm_vma_fault() and all its varients. Otherwise the driver will be > broken. > > I'm hoping to first define what this locking should be (see other > emails to Ralph) then, ideally, see if we can extend mmu notifiers to > get it directly withouth hmm stuff. > > Then we apply your patch one and the hmm ops wrapper dies. > This all makes sense, and thanks for all this work to simplify and clarify HMM. It's going to make it a lot easier to work with, when the dust settles. thanks, -- John Hubbard NVIDIA ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 10/11] mm/hmm: Do not use list*_rcu() for hmm->ranges
On Thu, Jun 06, 2019 at 03:44:37PM -0300, Jason Gunthorpe wrote: > From: Jason Gunthorpe > > This list is always read and written while holding hmm->lock so there is > no need for the confusing _rcu annotations. > > Signed-off-by: Jason Gunthorpe > Reviewed-by: Jérôme Glisse Reviewed-by: Ira Weiny > --- > mm/hmm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/hmm.c b/mm/hmm.c > index c2fecb3ecb11e1..709d138dd49027 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -911,7 +911,7 @@ int hmm_range_register(struct hmm_range *range, > mutex_lock(&hmm->lock); > > range->hmm = hmm; > - list_add_rcu(&range->list, &hmm->ranges); > + list_add(&range->list, &hmm->ranges); > > /* >* If there are any concurrent notifiers we have to wait for them for > @@ -941,7 +941,7 @@ void hmm_range_unregister(struct hmm_range *range) > return; > > mutex_lock(&hmm->lock); > - list_del_rcu(&range->list); > + list_del(&range->list); > mutex_unlock(&hmm->lock); > > /* Drop reference taken by hmm_range_register() */ > -- > 2.21.0 > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 11/11] mm/hmm: Remove confusing comment and logic from hmm_release
On Fri, Jun 07, 2019 at 02:37:07PM -0700, Ralph Campbell wrote: > > On 6/6/19 11:44 AM, Jason Gunthorpe wrote: > > From: Jason Gunthorpe > > > > hmm_release() is called exactly once per hmm. ops->release() cannot > > accidentally trigger any action that would recurse back onto > > hmm->mirrors_sem. > > > > This fixes a use after-free race of the form: > > > > CPU0 CPU1 > > hmm_release() > > up_write(&hmm->mirrors_sem); > > hmm_mirror_unregister(mirror) > >down_write(&hmm->mirrors_sem); > >up_write(&hmm->mirrors_sem); > >kfree(mirror) > > mirror->ops->release(mirror) > > > > The only user we have today for ops->release is an empty function, so this > > is unambiguously safe. > > > > As a consequence of plugging this race drivers are not allowed to > > register/unregister mirrors from within a release op. > > > > Signed-off-by: Jason Gunthorpe > > I agree with the analysis above but I'm not sure that release() will > always be an empty function. It might be more efficient to write back > all data migrated to a device "in one pass" instead of relying > on unmap_vmas() calling hmm_start_range_invalidate() per VMA. Sure, but it should not be allowed to recurse back to hmm_mirror_unregister. > I think the bigger issue is potential deadlocks while calling > sync_cpu_device_pagetables() and tasks calling hmm_mirror_unregister(): > > Say you have three threads: > - Thread A is in try_to_unmap(), either without holding mmap_sem or with > mmap_sem held for read. > - Thread B has some unrelated driver calling hmm_mirror_unregister(). > This doesn't require mmap_sem. > - Thread C is about to call migrate_vma(). > > Thread AThread B Thread C > try_to_unmaphmm_mirror_unregistermigrate_vma > hmm_invalidate_range_start > down_read(mirrors_sem) > down_write(mirrors_sem) > // Blocked on A > device_lock > device_lock > // Blocked on C > migrate_vma() > hmm_invalidate_range_s > down_read(mirrors_sem) > // Blocked on B > // Deadlock Oh... you know I didn't know this about rwsems in linux that they have a fairness policy for writes to block future reads.. Still, at least as things are designed, the driver cannot hold a lock it obtains under sync_cpu_device_pagetables() and nest other things in that lock. It certainly can't recurse back into any mmu notifiers while holding that lock. (as you point out) The lock in sync_cpu_device_pagetables() needs to be very narrowly focused on updating device state only. So, my first reaction is that the driver in thread C is wrong, and needs a different locking scheme. I think you'd have to make a really good case that there is no alternative for a driver.. > Perhaps we should consider using SRCU for walking the mirror->list? It means the driver has to deal with races like in this patch description. At that point there is almost no reason to insert hmm here, just use mmu notifiers directly. Drivers won't get this right, it is too hard. Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 11/11] mm/hmm: Remove confusing comment and logic from hmm_release
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
From: Jason Gunthorpe
hmm_release() is called exactly once per hmm. ops->release() cannot
accidentally trigger any action that would recurse back onto
hmm->mirrors_sem.
This fixes a use after-free race of the form:
CPU0 CPU1
hmm_release()
up_write(&hmm->mirrors_sem);
hmm_mirror_unregister(mirror)
down_write(&hmm->mirrors_sem);
up_write(&hmm->mirrors_sem);
kfree(mirror)
mirror->ops->release(mirror)
The only user we have today for ops->release is an empty function, so this
is unambiguously safe.
As a consequence of plugging this race drivers are not allowed to
register/unregister mirrors from within a release op.
Signed-off-by: Jason Gunthorpe
I agree with the analysis above but I'm not sure that release() will
always be an empty function. It might be more efficient to write back
all data migrated to a device "in one pass" instead of relying
on unmap_vmas() calling hmm_start_range_invalidate() per VMA.
I think the bigger issue is potential deadlocks while calling
sync_cpu_device_pagetables() and tasks calling hmm_mirror_unregister():
Say you have three threads:
- Thread A is in try_to_unmap(), either without holding mmap_sem or with
mmap_sem held for read.
- Thread B has some unrelated driver calling hmm_mirror_unregister().
This doesn't require mmap_sem.
- Thread C is about to call migrate_vma().
Thread AThread B Thread C
try_to_unmaphmm_mirror_unregistermigrate_vma
-- --- --
hmm_invalidate_range_start
down_read(mirrors_sem)
down_write(mirrors_sem)
// Blocked on A
device_lock
device_lock
// Blocked on C
migrate_vma()
hmm_invalidate_range_s
down_read(mirrors_sem)
// Blocked on B
// Deadlock
Perhaps we should consider using SRCU for walking the mirror->list?
---
mm/hmm.c | 28 +---
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index 709d138dd49027..3a45dd3d778248 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -136,26 +136,16 @@ static void hmm_release(struct mmu_notifier *mn, struct
mm_struct *mm)
WARN_ON(!list_empty(&hmm->ranges));
mutex_unlock(&hmm->lock);
- down_write(&hmm->mirrors_sem);
- mirror = list_first_entry_or_null(&hmm->mirrors, struct hmm_mirror,
- list);
- while (mirror) {
- list_del_init(&mirror->list);
- if (mirror->ops->release) {
- /*
-* Drop mirrors_sem so the release callback can wait
-* on any pending work that might itself trigger a
-* mmu_notifier callback and thus would deadlock with
-* us.
-*/
- up_write(&hmm->mirrors_sem);
+ down_read(&hmm->mirrors_sem);
+ list_for_each_entry(mirror, &hmm->mirrors, list) {
+ /*
+* Note: The driver is not allowed to trigger
+* hmm_mirror_unregister() from this thread.
+*/
+ if (mirror->ops->release)
mirror->ops->release(mirror);
- down_write(&hmm->mirrors_sem);
- }
- mirror = list_first_entry_or_null(&hmm->mirrors,
- struct hmm_mirror, list);
}
- up_write(&hmm->mirrors_sem);
+ up_read(&hmm->mirrors_sem);
hmm_put(hmm);
}
@@ -287,7 +277,7 @@ void hmm_mirror_unregister(struct hmm_mirror *mirror)
struct hmm *hmm = mirror->hmm;
down_write(&hmm->mirrors_sem);
- list_del_init(&mirror->list);
+ list_del(&mirror->list);
up_write(&hmm->mirrors_sem);
hmm_put(hmm);
memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
On Thu, Jun 06, 2019 at 03:44:29PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe
>
> Ralph observes that hmm_range_register() can only be called by a driver
> while a mirror is registered. Make this clear in the API by passing in the
> mirror structure as a parameter.
>
> This also simplifies understanding the lifetime model for struct hmm, as
> the hmm pointer must be valid as part of a registered mirror so all we
> need in hmm_register_range() is a simple kref_get.
>
> Suggested-by: Ralph Campbell
> Signed-off-by: Jason Gunthorpe
> ---
> v2
> - Include the oneline patch to nouveau_svm.c
> ---
> drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +-
> include/linux/hmm.h | 7 ---
> mm/hmm.c | 15 ++-
> 3 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
> b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index 93ed43c413f0bb..8c92374afcf227 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -649,7 +649,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
> range.values = nouveau_svm_pfn_values;
> range.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT;
> again:
> - ret = hmm_vma_fault(&range, true);
> + ret = hmm_vma_fault(&svmm->mirror, &range, true);
> if (ret == 0) {
> mutex_lock(&svmm->mutex);
> if (!hmm_vma_range_done(&range)) {
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 688c5ca7068795..2d519797cb134a 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -505,7 +505,7 @@ static inline bool hmm_mirror_mm_is_alive(struct
> hmm_mirror *mirror)
> * Please see Documentation/vm/hmm.rst for how to use the range API.
> */
> int hmm_range_register(struct hmm_range *range,
> -struct mm_struct *mm,
> +struct hmm_mirror *mirror,
> unsigned long start,
> unsigned long end,
> unsigned page_shift);
> @@ -541,7 +541,8 @@ static inline bool hmm_vma_range_done(struct hmm_range
> *range)
> }
>
> /* This is a temporary helper to avoid merge conflict between trees. */
> -static inline int hmm_vma_fault(struct hmm_range *range, bool block)
> +static inline int hmm_vma_fault(struct hmm_mirror *mirror,
> + struct hmm_range *range, bool block)
> {
> long ret;
>
> @@ -554,7 +555,7 @@ static inline int hmm_vma_fault(struct hmm_range *range,
> bool block)
> range->default_flags = 0;
> range->pfn_flags_mask = -1UL;
>
> - ret = hmm_range_register(range, range->vma->vm_mm,
> + ret = hmm_range_register(range, mirror,
>range->start, range->end,
>PAGE_SHIFT);
> if (ret)
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 547002f56a163d..8796447299023c 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -925,13 +925,13 @@ static void hmm_pfns_clear(struct hmm_range *range,
> * Track updates to the CPU page table see include/linux/hmm.h
> */
> int hmm_range_register(struct hmm_range *range,
> -struct mm_struct *mm,
> +struct hmm_mirror *mirror,
> unsigned long start,
> unsigned long end,
> unsigned page_shift)
> {
> unsigned long mask = ((1UL << page_shift) - 1UL);
> - struct hmm *hmm;
> + struct hmm *hmm = mirror->hmm;
>
> range->valid = false;
> range->hmm = NULL;
> @@ -945,15 +945,12 @@ int hmm_range_register(struct hmm_range *range,
> range->start = start;
> range->end = end;
>
> - hmm = hmm_get_or_create(mm);
> - if (!hmm)
> - return -EFAULT;
> -
> /* Check if hmm_mm_destroy() was call. */
> - if (hmm->mm == NULL || hmm->dead) {
> - hmm_put(hmm);
> + if (hmm->mm == NULL || hmm->dead)
> return -EFAULT;
> - }
> +
> + range->hmm = hmm;
I don't think you need this assignment here. In the code below (right after
the mutext_lock()) it is set already. And looks like it remains that way after
the end of the series.
Reviewed-by: Ira Weiny
> + kref_get(&hmm->kref);
>
> /* Initialize range to track CPU page table updates. */
> mutex_lock(&hmm->lock);
> --
> 2.21.0
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 10/11] mm/hmm: Do not use list*_rcu() for hmm->ranges
On 6/6/19 11:44 AM, Jason Gunthorpe wrote: From: Jason Gunthorpe This list is always read and written while holding hmm->lock so there is no need for the confusing _rcu annotations. Signed-off-by: Jason Gunthorpe Reviewed-by: Jérôme Glisse Reviewed-by: Ralph Campbell --- mm/hmm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mm/hmm.c b/mm/hmm.c index c2fecb3ecb11e1..709d138dd49027 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -911,7 +911,7 @@ int hmm_range_register(struct hmm_range *range, mutex_lock(&hmm->lock); range->hmm = hmm; - list_add_rcu(&range->list, &hmm->ranges); + list_add(&range->list, &hmm->ranges); /* * If there are any concurrent notifiers we have to wait for them for @@ -941,7 +941,7 @@ void hmm_range_unregister(struct hmm_range *range) return; mutex_lock(&hmm->lock); - list_del_rcu(&range->list); + list_del(&range->list); mutex_unlock(&hmm->lock); /* Drop reference taken by hmm_range_register() */ ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 03/11] mm/hmm: Hold a mmgrab from hmm to mm
On Thu, Jun 06, 2019 at 03:44:30PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe
>
> So long a a struct hmm pointer exists, so should the struct mm it is
> linked too. Hold the mmgrab() as soon as a hmm is created, and mmdrop() it
> once the hmm refcount goes to zero.
>
> Since mmdrop() (ie a 0 kref on struct mm) is now impossible with a !NULL
> mm->hmm delete the hmm_hmm_destroy().
>
> Signed-off-by: Jason Gunthorpe
> Reviewed-by: Jérôme Glisse
Reviewed-by: Ira Weiny
> ---
> v2:
> - Fix error unwind paths in hmm_get_or_create (Jerome/Jason)
> ---
> include/linux/hmm.h | 3 ---
> kernel/fork.c | 1 -
> mm/hmm.c| 22 --
> 3 files changed, 4 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 2d519797cb134a..4ee3acabe5ed22 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -586,14 +586,11 @@ static inline int hmm_vma_fault(struct hmm_mirror
> *mirror,
> }
>
> /* Below are for HMM internal use only! Not to be used by device driver! */
> -void hmm_mm_destroy(struct mm_struct *mm);
> -
> static inline void hmm_mm_init(struct mm_struct *mm)
> {
> mm->hmm = NULL;
> }
> #else /* IS_ENABLED(CONFIG_HMM_MIRROR) */
> -static inline void hmm_mm_destroy(struct mm_struct *mm) {}
> static inline void hmm_mm_init(struct mm_struct *mm) {}
> #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b2b87d450b80b5..588c768ae72451 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm)
> WARN_ON_ONCE(mm == current->active_mm);
> mm_free_pgd(mm);
> destroy_context(mm);
> - hmm_mm_destroy(mm);
> mmu_notifier_mm_destroy(mm);
> check_mm(mm);
> put_user_ns(mm->user_ns);
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 8796447299023c..cc7c26fda3300e 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -82,6 +83,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> hmm->notifiers = 0;
> hmm->dead = false;
> hmm->mm = mm;
> + mmgrab(hmm->mm);
>
> spin_lock(&mm->page_table_lock);
> if (!mm->hmm)
> @@ -109,6 +111,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> mm->hmm = NULL;
> spin_unlock(&mm->page_table_lock);
> error:
> + mmdrop(hmm->mm);
> kfree(hmm);
> return NULL;
> }
> @@ -130,6 +133,7 @@ static void hmm_free(struct kref *kref)
> mm->hmm = NULL;
> spin_unlock(&mm->page_table_lock);
>
> + mmdrop(hmm->mm);
> mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
> }
>
> @@ -138,24 +142,6 @@ static inline void hmm_put(struct hmm *hmm)
> kref_put(&hmm->kref, hmm_free);
> }
>
> -void hmm_mm_destroy(struct mm_struct *mm)
> -{
> - struct hmm *hmm;
> -
> - spin_lock(&mm->page_table_lock);
> - hmm = mm_get_hmm(mm);
> - mm->hmm = NULL;
> - if (hmm) {
> - hmm->mm = NULL;
> - hmm->dead = true;
> - spin_unlock(&mm->page_table_lock);
> - hmm_put(hmm);
> - return;
> - }
> -
> - spin_unlock(&mm->page_table_lock);
> -}
> -
> static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> {
> struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> --
> 2.21.0
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[RFC] mm/hmm: pass mmu_notifier_range to sync_cpu_device_pagetables
HMM defines its own struct hmm_update which is passed to the
sync_cpu_device_pagetables() callback function. This is
sufficient when the only action is to invalidate. However,
a device may want to know the reason for the invalidation and
be able to see the new permissions on a range, update device access
rights or range statistics. Since sync_cpu_device_pagetables()
can be called from try_to_unmap(), the mmap_sem may not be held
and find_vma() is not safe to be called.
Pass the struct mmu_notifier_range to sync_cpu_device_pagetables()
to allow the full invalidation information to be used.
Signed-off-by: Ralph Campbell
---
I'm sending this out now since we are updating many of the HMM APIs
and I think it will be useful.
drivers/gpu/drm/nouveau/nouveau_svm.c | 4 ++--
include/linux/hmm.h | 27 ++-
mm/hmm.c | 13 -
3 files changed, 8 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
b/drivers/gpu/drm/nouveau/nouveau_svm.c
index 8c92374afcf2..c34b98fafe2f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -252,13 +252,13 @@ nouveau_svmm_invalidate(struct nouveau_svmm *svmm, u64
start, u64 limit)
static int
nouveau_svmm_sync_cpu_device_pagetables(struct hmm_mirror *mirror,
- const struct hmm_update *update)
+ const struct mmu_notifier_range *update)
{
struct nouveau_svmm *svmm = container_of(mirror, typeof(*svmm), mirror);
unsigned long start = update->start;
unsigned long limit = update->end;
- if (!update->blockable)
+ if (!mmu_notifier_range_blockable(update))
return -EAGAIN;
SVMM_DBG(svmm, "invalidate %016lx-%016lx", start, limit);
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 0fa8ea34ccef..07a2d38fde34 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -377,29 +377,6 @@ static inline uint64_t hmm_pfn_from_pfn(const struct
hmm_range *range,
struct hmm_mirror;
-/*
- * enum hmm_update_event - type of update
- * @HMM_UPDATE_INVALIDATE: invalidate range (no indication as to why)
- */
-enum hmm_update_event {
- HMM_UPDATE_INVALIDATE,
-};
-
-/*
- * struct hmm_update - HMM update information for callback
- *
- * @start: virtual start address of the range to update
- * @end: virtual end address of the range to update
- * @event: event triggering the update (what is happening)
- * @blockable: can the callback block/sleep ?
- */
-struct hmm_update {
- unsigned long start;
- unsigned long end;
- enum hmm_update_event event;
- bool blockable;
-};
-
/*
* struct hmm_mirror_ops - HMM mirror device operations callback
*
@@ -420,7 +397,7 @@ struct hmm_mirror_ops {
/* sync_cpu_device_pagetables() - synchronize page tables
*
* @mirror: pointer to struct hmm_mirror
-* @update: update information (see struct hmm_update)
+* @update: update information (see struct mmu_notifier_range)
* Return: -EAGAIN if update.blockable false and callback need to
* block, 0 otherwise.
*
@@ -434,7 +411,7 @@ struct hmm_mirror_ops {
* synchronous call.
*/
int (*sync_cpu_device_pagetables)(struct hmm_mirror *mirror,
- const struct hmm_update *update);
+ const struct mmu_notifier_range *update);
};
/*
diff --git a/mm/hmm.c b/mm/hmm.c
index 9aad3550f2bb..b49a43712554 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -164,7 +164,6 @@ static int hmm_invalidate_range_start(struct mmu_notifier
*mn,
{
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
- struct hmm_update update;
struct hmm_range *range;
unsigned long flags;
int ret = 0;
@@ -173,15 +172,10 @@ static int hmm_invalidate_range_start(struct mmu_notifier
*mn,
if (!kref_get_unless_zero(&hmm->kref))
return 0;
- update.start = nrange->start;
- update.end = nrange->end;
- update.event = HMM_UPDATE_INVALIDATE;
- update.blockable = mmu_notifier_range_blockable(nrange);
-
spin_lock_irqsave(&hmm->ranges_lock, flags);
hmm->notifiers++;
list_for_each_entry(range, &hmm->ranges, list) {
- if (update.end < range->start || update.start >= range->end)
+ if (nrange->end < range->start || nrange->start >= range->end)
continue;
range->valid = false;
@@ -198,9 +192,10 @@ static int hmm_invalidate_range_start(struct mmu_notifier
*mn,
list_for_each_entry(mirror, &hmm->mirrors, list) {
int rc;
- rc = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
+ rc = mirror->ops->sync_cpu_device_pa
Re: [PATCH v2 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
On 6/7/19 1:44 PM, Jason Gunthorpe wrote:
On Fri, Jun 07, 2019 at 01:21:12PM -0700, Ralph Campbell wrote:
What I want to get to is a pattern like this:
pagefault():
hmm_range_register(&range);
again:
/* On the slow path, if we appear to be live locked then we get
the write side of mmap_sem which will break the live lock,
otherwise this gets the read lock */
if (hmm_range_start_and_lock(&range))
goto err;
lockdep_assert_held(range->mm->mmap_sem);
// Optional: Avoid useless expensive work
if (hmm_range_needs_retry(&range))
goto again;
hmm_range_(touch vmas)
take_lock(driver->update);
if (hmm_range_end(&range) {
release_lock(driver->update);
goto again;
}
// Finish driver updates
release_lock(driver->update);
// Releases mmap_sem
hmm_range_unregister_and_unlock(&range);
What do you think?
Is it clear?
Jason
Are you talking about acquiring mmap_sem in hmm_range_start_and_lock()?
Usually, the fault code has to lock mmap_sem for read in order to
call find_vma() so it can set range.vma.
If HMM drops mmap_sem - which I don't think it should, just return an
error to tell the caller to drop mmap_sem and retry - the find_vma()
will need to be repeated as well.
Overall I don't think it makes a lot of sense to sleep for retry in
hmm_range_start_and_lock() while holding mmap_sem. It would be better
to drop that lock, sleep, then re-acquire it as part of the hmm logic.
The find_vma should be done inside the critical section created by
hmm_range_start_and_lock(), not before it. If we are retrying then we
already slept and the additional CPU cost to repeat the find_vma is
immaterial, IMHO?
Do you see a reason why the find_vma() ever needs to be before the
'again' in my above example? range.vma does not need to be set for
range_register.
Yes, for the GPU case, there can be many faults in an event queue
and the goal is to try to handle more than one page at a time.
The vma is needed to limit the amount of coalescing and checking
for pages that could be speculatively migrated or mapped.
I'm also not sure about acquiring the mmap_sem for write as way to
mitigate thrashing. It seems to me that if a device and a CPU are
both faulting on the same page,
One of the reasons to prefer this approach is that it means we don't
need to keep track of which ranges we are faulting, and if there is a
lot of *unrelated* fault activity (unlikely?) we can resolve it using
mmap sem instead of this elaborate ranges scheme and related
locking.
This would reduce the overall work in the page fault and
invalidate_start/end paths for the common uncontended cases.
some sort of backoff delay is needed to let one side or the other
make some progress.
What the write side of the mmap_sem would do is force the CPU and
device to cleanly take turns. Once the device pages are registered
under the write side the CPU will have to wait in invalidate_start for
the driver to complete a shootdown, then the whole thing starts all
over again.
It is certainly imaginable something could have a 'min life' timer for
a device mapping and hold mm invalidate_start, and device pagefault
for that min time to promote better sharing.
But, if we don't use the mmap_sem then we can livelock and the device
will see an unrecoverable error from the timeout which means we have
risk that under load the system will simply obscurely fail. This seems
unacceptable to me..
Particularly since for the ODP use case the issue is not trashing
migration as a GPU might have, but simple system stability under swap
load. We do not want the ODP pagefault to permanently fail due to
timeout if the VMA is still valid..
Jason
OK, I understand.
If you come up with a set of changes, I can try testing them.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
On Fri, Jun 07, 2019 at 01:21:12PM -0700, Ralph Campbell wrote:
> > What I want to get to is a pattern like this:
> >
> > pagefault():
> >
> > hmm_range_register(&range);
> > again:
> > /* On the slow path, if we appear to be live locked then we get
> >the write side of mmap_sem which will break the live lock,
> >otherwise this gets the read lock */
> > if (hmm_range_start_and_lock(&range))
> > goto err;
> >
> > lockdep_assert_held(range->mm->mmap_sem);
> >
> > // Optional: Avoid useless expensive work
> > if (hmm_range_needs_retry(&range))
> >goto again;
> > hmm_range_(touch vmas)
> >
> > take_lock(driver->update);
> > if (hmm_range_end(&range) {
> > release_lock(driver->update);
> > goto again;
> > }
> > // Finish driver updates
> > release_lock(driver->update);
> >
> > // Releases mmap_sem
> > hmm_range_unregister_and_unlock(&range);
> >
> > What do you think?
> >
> > Is it clear?
> >
> > Jason
> >
>
> Are you talking about acquiring mmap_sem in hmm_range_start_and_lock()?
> Usually, the fault code has to lock mmap_sem for read in order to
> call find_vma() so it can set range.vma.
> If HMM drops mmap_sem - which I don't think it should, just return an
> error to tell the caller to drop mmap_sem and retry - the find_vma()
> will need to be repeated as well.
Overall I don't think it makes a lot of sense to sleep for retry in
hmm_range_start_and_lock() while holding mmap_sem. It would be better
to drop that lock, sleep, then re-acquire it as part of the hmm logic.
The find_vma should be done inside the critical section created by
hmm_range_start_and_lock(), not before it. If we are retrying then we
already slept and the additional CPU cost to repeat the find_vma is
immaterial, IMHO?
Do you see a reason why the find_vma() ever needs to be before the
'again' in my above example? range.vma does not need to be set for
range_register.
> I'm also not sure about acquiring the mmap_sem for write as way to
> mitigate thrashing. It seems to me that if a device and a CPU are
> both faulting on the same page,
One of the reasons to prefer this approach is that it means we don't
need to keep track of which ranges we are faulting, and if there is a
lot of *unrelated* fault activity (unlikely?) we can resolve it using
mmap sem instead of this elaborate ranges scheme and related
locking.
This would reduce the overall work in the page fault and
invalidate_start/end paths for the common uncontended cases.
> some sort of backoff delay is needed to let one side or the other
> make some progress.
What the write side of the mmap_sem would do is force the CPU and
device to cleanly take turns. Once the device pages are registered
under the write side the CPU will have to wait in invalidate_start for
the driver to complete a shootdown, then the whole thing starts all
over again.
It is certainly imaginable something could have a 'min life' timer for
a device mapping and hold mm invalidate_start, and device pagefault
for that min time to promote better sharing.
But, if we don't use the mmap_sem then we can livelock and the device
will see an unrecoverable error from the timeout which means we have
risk that under load the system will simply obscurely fail. This seems
unacceptable to me..
Particularly since for the ODP use case the issue is not trashing
migration as a GPU might have, but simple system stability under swap
load. We do not want the ODP pagefault to permanently fail due to
timeout if the VMA is still valid..
Jason
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [RFC] mm/hmm: pass mmu_notifier_range to sync_cpu_device_pagetables
On Fri, Jun 07, 2019 at 05:14:52PM -0700, Ralph Campbell wrote: > HMM defines its own struct hmm_update which is passed to the > sync_cpu_device_pagetables() callback function. This is > sufficient when the only action is to invalidate. However, > a device may want to know the reason for the invalidation and > be able to see the new permissions on a range, update device access > rights or range statistics. Since sync_cpu_device_pagetables() > can be called from try_to_unmap(), the mmap_sem may not be held > and find_vma() is not safe to be called. > Pass the struct mmu_notifier_range to sync_cpu_device_pagetables() > to allow the full invalidation information to be used. > > Signed-off-by: Ralph Campbell > --- > > I'm sending this out now since we are updating many of the HMM APIs > and I think it will be useful. This is the right thing to do. But the really right thing is to just kill the hmm_mirror API entirely and move to mmu_notifiers. At least for noveau this already is way simpler, although right now it defeats Jasons patch to avoid allocating the struct hmm in the fault path. But as said before that can be avoided by just killing struct hmm, which for many reasons is the right thing to do anyway. I've got a series here, which is a bit broken (epecially the last patch can't work as-is), but should explain where I'm trying to head: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/hmm-mirror-simplification ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/display: Allow cursor async updates for framebuffer swaps
[Why]
We previously allowed framebuffer swaps as async updates for cursor
planes but had to disable them due to a bug in DRM with async update
handling and incorrect ref counting. The check to block framebuffer
swaps has been added to DRM for a while now, so this check is redundant.
The real fix that allows this to properly in DRM has also finally been
merged and is getting backported into stable branches, so dropping
this now seems to be the right time to do so.
[How]
Drop the redundant check for old_fb != new_fb.
With the proper fix in DRM, this should also fix some cursor stuttering
issues with xf86-video-amdgpu since it double buffers the cursor.
IGT tests that swap framebuffers (-varying-size for example) should
also pass again.
Cc: David Francis
Cc: Harry Wentland
Signed-off-by: Nicholas Kazlauskas
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 45f0d5b6c468..100b2c60ff1c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4315,20 +4315,10 @@ static int dm_plane_atomic_check(struct drm_plane
*plane,
static int dm_plane_atomic_async_check(struct drm_plane *plane,
struct drm_plane_state *new_plane_state)
{
- struct drm_plane_state *old_plane_state =
- drm_atomic_get_old_plane_state(new_plane_state->state, plane);
-
/* Only support async updates on cursor planes. */
if (plane->type != DRM_PLANE_TYPE_CURSOR)
return -EINVAL;
- /*
-* DRM calls prepare_fb and cleanup_fb on new_plane_state for
-* async commits so don't allow fb changes.
-*/
- if (old_plane_state->fb != new_plane_state->fb)
- return -EINVAL;
-
return 0;
}
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
