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

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

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@linux.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