Re: [PATCH 2/6] mshv: Ignore second stats page map result failure

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

On 9/8/2025 10:22 AM, Easwar Hariharan wrote:
> On 9/8/2025 10:04 AM, Nuno Das Neves wrote:
>> On 9/5/2025 12:21 PM, Easwar Hariharan wrote:
>>> On 8/28/2025 5:43 PM, Nuno Das Neves wrote:
>>>> From: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com>
>>>>
>>>> Some versions of the hypervisor do not support HV_STATUS_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].
>>>
>>> What's the impact of this graceful fallback? It occurs to me that if a stats
>>> accumulator, in userspace perhaps, expected to get stats from the 2 pages,
>>> it'd get incorrect values.
>>>
>> This is going out of scope of this series a bit but I'll explain briefly.
>>
>> When we do add the code to expose these stats to userspace, the SELF and
>> PARENT pages won't be exposed separately, there is no duplication.
>>
>> For each stat counter in the page, we'll expose either the SELF or PARENT
>> value, depending on whether there is anything in that slot (whether it's zero
>> or not).
>>
>> Some stats are available via the SELF page, and some via the PARENT page, but
>> the counters in the page have the same layout. So some counters in the SELF
>> page will all stay zero while on the PARENT page they are updated, and vice
>> versa.
>>
>> I believe the hypervisor takes this strange approach for the purpose of
>> backward compatibility. Introducing L1VH created the need for this SELF/PARENT
>> distinction.
>>
>> Hope that makes some kind of sense...it will be clearer when we post the mshv
>> debugfs code itself.
>>
>> To put it another way, falling back to the SELF page won't cause any impact
>> to userspace because the distinction between the pages is all handled in the
>> driver, and we only read each stat value from either SELF or PARENT.
>>
>> Nuno
> 
> Thank you for that explanation, it sorta makes sense.
> 
> I think it'd be better if this patch is part of the series that exposes the stats
> to userspace, so that it can be reviewed in context with the rest of the code in
> the driver that manages the pick-and-choose of a stat value from the SELF/PARENT
> page.
> 
Good idea, I think I'll do that. Thanks!

> Unless there's an active problem now in the upstream kernel that this patch solves?
> i.e. are the versions of the hypervisor that don't support the PARENT stats
> page available in the wild?
> 
I thought there was, but on reflection, no it doesn't solve a problem that exists in
the code today.

Nuno

> Thanks,
> Easwar (he/him)