linux-kernel - Re: [PATCH 1/3] Drivers: hv: Rename a few memory region related functions for clarity

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: <6165c48c-a71e-4aa0-99d3-2ff8158ddd4a@linux.microsoft.com>
Date: Fri, 26 Sep 2025 10:14:25 -0700
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>,
 kys@...rosoft.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
 decui@...rosoft.com
Cc: linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] Drivers: hv: Rename a few memory region related
 functions for clarity

On 9/24/2025 2:31 PM, Stanislav Kinsburskii wrote:
> A cleanup and precursor patch.
> 
This line doesn't add much, I think you can remove it.

> Rename "mshv_partition_mem_region_map" to "mshv_handle_pinned_region",
> "mshv_region_populate" to "mshv_pin_region" and
> "mshv_region_populate_pages" to "mshv_region_pin_pages"
> to better reflect its purpose of handling pinned memory regions.
> 
> Update the "mshv_handle_pinned_region" function's documentation to provide
> detailed information about its behavior and return values.
> 
> Also drop the check for range as pinned, as this function is static and
> all the memory regions are pinned anyway.
> 
> Signed-off-by: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
> ---
>  drivers/hv/mshv_root_main.c |   41 ++++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index a1c8c3bc79bf1..5ed6bce334417 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -1137,8 +1137,8 @@ mshv_region_evict(struct mshv_mem_region *region)
>  }
>  
>  static int
> -mshv_region_populate_pages(struct mshv_mem_region *region,
> -			   u64 page_offset, u64 page_count)
> +mshv_region_pin_pages(struct mshv_mem_region *region,
> +		      u64 page_offset, u64 page_count)
>  {
>  	u64 done_count, nr_pages;
>  	struct page **pages;
> @@ -1164,14 +1164,9 @@ mshv_region_populate_pages(struct mshv_mem_region *region,
>  		 * with the FOLL_LONGTERM flag does a large temporary
>  		 * allocation of contiguous memory.
>  		 */
> -		if (region->flags.range_pinned)
> -			ret = pin_user_pages_fast(userspace_addr,
> -						  nr_pages,
> -						  FOLL_WRITE | FOLL_LONGTERM,
> -						  pages);
> -		else
> -			ret = -EOPNOTSUPP;
> -
> +		ret = pin_user_pages_fast(userspace_addr, nr_pages,
> +					  FOLL_WRITE | FOLL_LONGTERM,
> +					  pages);
>  		if (ret < 0)
>  			goto release_pages;
>  	}
> @@ -1187,9 +1182,9 @@ mshv_region_populate_pages(struct mshv_mem_region *region,
>  }
>  
>  static int
> -mshv_region_populate(struct mshv_mem_region *region)
> +mshv_region_pin(struct mshv_mem_region *region)
>  {
> -	return mshv_region_populate_pages(region, 0, region->nr_pages);
> +	return mshv_region_pin_pages(region, 0, region->nr_pages);
>  }
Do we ever partially pin a region? Maybe we don't need a function called
mshv_region_pin_pages() and we just have mshv_region_pin() instead.

>  
>  static struct mshv_mem_region *
> @@ -1264,17 +1259,25 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
>  	return 0;
>  }
>  
> -/*
> - * Map guest ram. if snp, make sure to release that from the host first
> - * Side Effects: In case of failure, pages are unpinned when feasible.
> +/**
> + * mshv_handle_pinned_region - Handle pinned memory regions
> + * @region: Pointer to the memory region structure
> + *
> + * This function processes memory regions that are explicitly marked as pinned.
> + * Pinned regions are preallocated, mapped upfront, and do not rely on fault-based
> + * population. The function ensures the region is properly populated, handles
> + * encryption requirements for SNP partitions if applicable, maps the region,
> + * and performs necessary sharing or eviction operations based on the mapping
> + * result.
> + *
> + * Return: 0 on success, negative error code on failure.
>   */
> -static int
> -mshv_partition_mem_region_map(struct mshv_mem_region *region)
> +static int mshv_handle_pinned_region(struct mshv_mem_region *region)

Why the verb "handle"? It doesn't provide any information on what the function does,
when it might be called etc. Maybe mshv_init_pinned_region() ?

>  {
>  	struct mshv_partition *partition = region->partition;
>  	int ret;
>  
> -	ret = mshv_region_populate(region);
> +	ret = mshv_region_pin(region);
>  	if (ret) {
>  		pt_err(partition, "Failed to populate memory region: %d\n",
>  		       ret);
> @@ -1368,7 +1371,7 @@ mshv_map_user_memory(struct mshv_partition *partition,
>  		ret = hv_call_map_mmio_pages(partition->pt_id, mem.guest_pfn,
>  					     mmio_pfn, HVPFN_DOWN(mem.size));
>  	else
> -		ret = mshv_partition_mem_region_map(region);
> +		ret = mshv_handle_pinned_region(region);
>  
>  	if (ret)
>  		goto errout;
> 
>