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 <[email protected]>
>>
>
> <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
