linux-kernel - Re: [PATCH v3 5/5] mshv: Introduce new hypercall to map stats page for L1VH partitions

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] [day] [month] [year] [list]

Message-ID: <43d27c68-f92f-4236-830d-9f28a1b52931@linux.microsoft.com>
Date: Fri, 19 Sep 2025 15:44:56 -0700
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
Cc: linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
 prapal@...ux.microsoft.com, easwar.hariharan@...ux.microsoft.com,
 tiala@...rosoft.com, anirudh@...rudhrb.com,
 paekkaladevi@...ux.microsoft.com, kys@...rosoft.com, haiyangz@...rosoft.com,
 wei.liu@...nel.org, decui@...rosoft.com,
 Jinank Jain <jinankjain@...ux.microsoft.com>
Subject: Re: [PATCH v3 5/5] mshv: Introduce new hypercall to map stats page
 for L1VH partitions

On 9/18/2025 12:53 PM, Stanislav Kinsburskii wrote:
> On Tue, Sep 16, 2025 at 04:44:22PM -0700, Nuno Das Neves wrote:
>> From: Jinank Jain <jinankjain@...ux.microsoft.com>
>>
> 
> <snip>
> 
>> +static int hv_call_map_stats_page2(enum hv_stats_object_type type,
>> +				   const union hv_stats_object_identity *identity,
>> +				   u64 map_location)
>> +{
>> +	unsigned long flags;
>> +	struct hv_input_map_stats_page2 *input;
>> +	u64 status;
>> +	int ret;
>> +
>> +	if (!map_location || !mshv_use_overlay_gpfn())
>> +		return -EINVAL;
>> +
>> +	do {
>> +		local_irq_save(flags);
>> +		input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> +
>> +		memset(input, 0, sizeof(*input));
>> +		input->type = type;
>> +		input->identity = *identity;
>> +		input->map_location = map_location;
>> +
>> +		status = hv_do_hypercall(HVCALL_MAP_STATS_PAGE2, input, NULL);
>> +
>> +		local_irq_restore(flags);
>> +		if (hv_result(status) != HV_STATUS_INSUFFICIENT_MEMORY) {
>> +			if (hv_result_success(status))
>> +				break;
>> +			hv_status_debug(status, "\n");
> 
> It looks more natural to check for success first and break the loop, and
> only then handle errors.
> Maybe even set ret for both success and error messages and break and
> handle only the unsufficient memory status.
> 

Something like this?

	local_irq_restore(flags);

	ret = hv_result_to_errno(status);

	if (!ret)
		break;

	if (hv_result(status) != HV_STATUS_INSUFFICIENT_MEMORY) {
		hv_status_debug(status, "\n");
		break;
	}

	ret = hv_call_deposit_pages(NUMA_NO_NODE,
				    hv_current_partition_id, 1);

>> @@ -865,6 +931,19 @@ int hv_call_unmap_stat_page(enum hv_stats_object_type type,
>>  	return hv_result_to_errno(status);
>>  }
>>  
>> +int hv_unmap_stats_page(enum hv_stats_object_type type, void *page_addr,
>> +			const union hv_stats_object_identity *identity)
>> +{
> 
> Should this function be type of void?
> 

The return type is consistent with the other hypercall helpers. It's true that
in practice we don't ever check if the unmap succeeded. I think it's fine as-is.

> Thanks,
> Stanislav