On 12/8/2025 7:12 AM, Michael Kelley wrote: > From: Nuno Das Neves <[email protected]> Sent: Friday, > December 5, 2025 10:59 AM >> >> From: Purna Pavan Chandra Aekkaladevi <[email protected]> >> >> 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 >> <[email protected]> >> Signed-off-by: Nuno Das Neves <[email protected]> >> Reviewed-by: Stanislav Kinsburskii <[email protected]> >> --- >> 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
