linux-kernel - Re: [RFC PATCH] mm/hmm, mm/migrate_device: Allow p2p access and p2p migration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241015121759.GG3394334@nvidia.com>
Date: Tue, 15 Oct 2024 09:17:59 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Thomas Hellström <thomas.hellstrom@...ux.intel.com>
Cc: intel-xe@...ts.freedesktop.org, Matthew Brost <matthew.brost@...el.com>,
	Simona Vetter <simona.vetter@...ll.ch>,
	DRI-devel <dri-devel@...ts.freedesktop.org>,
	Linux Memory Management List <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] mm/hmm, mm/migrate_device: Allow p2p access and p2p
 migration

On Tue, Oct 15, 2024 at 01:13:22PM +0200, Thomas Hellström wrote:
> Introduce a way for hmm_range_fault() and migrate_vma_setup() to identify
> foreign devices with fast interconnect and thereby allow
> both direct access over the interconnect and p2p migration.
> 
> The need for a callback arises because without it, the p2p ability would
> need to be static and determined at dev_pagemap creation time. With
> a callback it can be determined dynamically, and in the migrate case
> the callback could separate out local device pages.

> +static bool hmm_allow_devmem(struct hmm_range *range, struct page *page)
> +{
> +	if (likely(page->pgmap->owner == range->dev_private_owner))
> +		return true;
> +	if (likely(!range->p2p))
> +		return false;
> +	return range->p2p->ops->p2p_allow(range->p2p, page);
> +}
> +
>  static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  			      unsigned long end, pmd_t *pmdp, pte_t *ptep,
>  			      unsigned long *hmm_pfn)
> @@ -248,8 +258,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  		 * just report the PFN.
>  		 */
>  		if (is_device_private_entry(entry) &&
> -		    pfn_swap_entry_to_page(entry)->pgmap->owner ==
> -		    range->dev_private_owner) {
> +		    hmm_allow_devmem(range, pfn_swap_entry_to_page(entry))) {
>  			cpu_flags = HMM_PFN_VALID;
>  			if (is_writable_device_private_entry(entry))
>  				cpu_flags |= HMM_PFN_WRITE;

This is really misnamed and took me a while to get it.

It has nothing to do with kernel P2P, you are just allowing more
selective filtering of dev_private_owner. You should focus on that in
the naming, not p2p. ie allow_dev_private()

P2P is stuff that is dealing with MEMORY_DEVICE_PCI_P2PDMA.

This is just allowing more instances of the same driver to co-ordinate
their device private memory handle, for whatever purpose.

Otherwise I don't see a particular problem, though we have talked
about widening the matching for device_private more broadly using some
kind of grouping tag or something like that instead of a callback. You
may consider that as an alternative

I would also probably try to have less indirection, you can embedd the
hmm_range struct inside a caller private data struct and use that
instead if inventing a whole new struct and pointer.

Jason