Re: [PATCH v2 1/3] mshv: Ignore second stats page map result failure

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

On 12/8/2025 7:12 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, December 5, 2025 10:59 AM
>>
>> From: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com>
>>
>> Older versions of the hypervisor do not support HV_STATS_AREA_PARENT
>> and return HV_STATUS_INVALID_PARAMETER for the second stats page
>> mapping request.
>>
>> This results a failure in module init. Instead of failing, gracefully
>> fall back to populating stats_pages[HV_STATS_AREA_PARENT] with the
>> already-mapped stats_pages[HV_STATS_AREA_SELF].
> 
> This explains "what" this patch does. But could you add an explanation of "why"
> substituting SELF for the unavailable PARENT is the right thing to do? As a somewhat
> outside reviewer, I don't know enough about SELF vs. PARENT to immediately know
> why this substitution makes sense.
> 
I'll attempt to explain. I'm a little hindered by the fact that like many of the
root interfaces this is not well-documented, but this is my understanding:

The stats areas HV_STATS_AREA_SELF and HV_STATS_AREA_PARENT indicate the privilege
level of the data in the mapped stats page.

Both SELF and PARENT contain the same fields, but some fields that are 0 in the
SELF page may be nonzero in PARENT page, and vice-versa. So, to read all the fields
we need to map both pages if possible, and prioritize reading non-zero data from
each field, by checking both the SELF and PARENT pages.

I don't know if it's possible for a given field to have a different (nonzero) value
in both SELF and PARENT pages. I imagine in that case we'd want to prioritize the
PARENT value, but it may simply not be possible.

The API is designed in this way to be backward-compatible with older hypervisors
that didn't have a concept of SELF and PARENT. Hence on older hypervisors (detectable
via the error code), all we can do is map SELF and use it for everything.

> Also, does this patch affect the logic in mshv_vp_dispatch_thread_blocked() where
> a zero value for the SELF version of VpRootDispatchThreadBlocked is replaced by
> the PARENT value? But that logic seems to be in the reverse direction -- replacing
> a missing SELF value with the PARENT value -- whereas this patch is about replacing
> missing PARENT values with SELF values. So are there two separate PARENT vs. SELF
> issues overall? And after this patch is in place and PARENT values are replaced with
> SELF on older hypervisor versions, the logic in mshv_vp_dispatch_thread_blocked()
> then effectively becomes a no-op if the SELF value is zero, and the return value will
> be zero. Is that problem?
> 
This is the same issue, because we only care about any nonzero value in
mshv_vp_dispatch_thread_blocked(). It doesn't matter which page we check first in that
code, just that any nonzero value is returned as a boolean to indicate a blocked state.

The code in question could be rewritten:

return self_vp_cntrs[VpRootDispatchThreadBlocked] || parent_vp_cntrs[VpRootDispatchThreadBlocked];

>>
>> Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> Reviewed-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>> ---
>>  drivers/hv/mshv_root_hv_call.c | 41 ++++++++++++++++++++++++++++++----
>>  drivers/hv/mshv_root_main.c    |  3 +++
>>  2 files changed, 40 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
>> index 598eaff4ff29..b1770c7b500c 100644
>> --- a/drivers/hv/mshv_root_hv_call.c
>> +++ b/drivers/hv/mshv_root_hv_call.c
>> @@ -855,6 +855,24 @@ static int hv_call_map_stats_page2(enum
>> hv_stats_object_type type,
>>  	return ret;
>>  }
>>
>> +static int
>> +hv_stats_get_area_type(enum hv_stats_object_type type,
>> +		       const union hv_stats_object_identity *identity)
>> +{
>> +	switch (type) {
>> +	case HV_STATS_OBJECT_HYPERVISOR:
>> +		return identity->hv.stats_area_type;
>> +	case HV_STATS_OBJECT_LOGICAL_PROCESSOR:
>> +		return identity->lp.stats_area_type;
>> +	case HV_STATS_OBJECT_PARTITION:
>> +		return identity->partition.stats_area_type;
>> +	case HV_STATS_OBJECT_VP:
>> +		return identity->vp.stats_area_type;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>>  static int hv_call_map_stats_page(enum hv_stats_object_type type,
>>  				  const union hv_stats_object_identity *identity,
>>  				  void **addr)
>> @@ -863,7 +881,7 @@ static int hv_call_map_stats_page(enum hv_stats_object_type type,
>>  	struct hv_input_map_stats_page *input;
>>  	struct hv_output_map_stats_page *output;
>>  	u64 status, pfn;
>> -	int ret = 0;
>> +	int hv_status, ret = 0;
>>
>>  	do {
>>  		local_irq_save(flags);
>> @@ -878,11 +896,26 @@ static int hv_call_map_stats_page(enum hv_stats_object_type type,
>>  		pfn = output->map_location;
>>
>>  		local_irq_restore(flags);
>> -		if (hv_result(status) != HV_STATUS_INSUFFICIENT_MEMORY) {
>> -			ret = hv_result_to_errno(status);
>> +
>> +		hv_status = hv_result(status);
>> +		if (hv_status != HV_STATUS_INSUFFICIENT_MEMORY) {
>>  			if (hv_result_success(status))
>>  				break;
>> -			return ret;
>> +
>> +			/*
>> +			 * Older versions of the hypervisor do not support the
>> +			 * PARENT stats area. In this case return "success" but
>> +			 * set the page to NULL. The caller should check for
>> +			 * this case and instead just use the SELF area.
>> +			 */
>> +			if (hv_stats_get_area_type(type, identity) == HV_STATS_AREA_PARENT &&
>> +			    hv_status == HV_STATUS_INVALID_PARAMETER) {
>> +				*addr = NULL;
>> +				return 0;
>> +			}
>> +
>> +			hv_status_debug(status, "\n");
>> +			return hv_result_to_errno(status);
> 
> Does the hv_call_map_stats_page2() function need a similar fix? Or is there a linkage
> in hypervisor functionality where any hypervisor version that supports an overlay GPFN
> also supports the PARENT stats? If such a linkage is why hv_call_map_stats_page2()
> doesn't need a similar fix, please add a code comment to that effect in
> hv_call_map_stats_page2().
> 
Exactly; hv_call_map_stats_page2() is only available on hypervisors where the PARENT
page is also available. I'll add a comment.

>>  		}
>>
>>  		ret = hv_call_deposit_pages(NUMA_NO_NODE,
>> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
>> index bc15d6f6922f..f59a4ab47685 100644
>> --- a/drivers/hv/mshv_root_main.c
>> +++ b/drivers/hv/mshv_root_main.c
>> @@ -905,6 +905,9 @@ static int mshv_vp_stats_map(u64 partition_id, u32 vp_index,
>>  	if (err)
>>  		goto unmap_self;
>>
>> +	if (!stats_pages[HV_STATS_AREA_PARENT])
>> +		stats_pages[HV_STATS_AREA_PARENT] =
>> stats_pages[HV_STATS_AREA_SELF];
>> +
>>  	return 0;
>>
>>  unmap_self:
>> --
>> 2.34.1