From: Nuno Das Neves <nunodasneves@linux.microsoft.com>
To: Michael Kelley <mhklinux@outlook.com>,
"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"prapal@linux.microsoft.com" <prapal@linux.microsoft.com>,
"easwar.hariharan@linux.microsoft.com"
<easwar.hariharan@linux.microsoft.com>,
"tiala@microsoft.com" <tiala@microsoft.com>,
"anirudh@anirudhrb.com" <anirudh@anirudhrb.com>,
"paekkaladevi@linux.microsoft.com"
<paekkaladevi@linux.microsoft.com>,
"skinsburskii@linux.microsoft.com"
<skinsburskii@linux.microsoft.com>
Cc: "kys@microsoft.com" <kys@microsoft.com>,
"haiyangz@microsoft.com" <haiyangz@microsoft.com>,
"wei.liu@kernel.org" <wei.liu@kernel.org>,
"decui@microsoft.com" <decui@microsoft.com>,
Jinank Jain <jinankjain@linux.microsoft.com>
Subject: Re: [PATCH v4 5/5] mshv: Introduce new hypercall to map stats page for L1VH partitions
Date: Wed, 1 Oct 2025 16:03:19 -0700 [thread overview]
Message-ID: <33868cb1-4cb1-40bb-a773-8f15ec75f679@linux.microsoft.com> (raw)
In-Reply-To: <SN6PR02MB41571CB74AAAB29181443EE9D41BA@SN6PR02MB4157.namprd02.prod.outlook.com>
On 9/29/2025 10:57 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, September 26, 2025 9:23 AM
>>
>> From: Jinank Jain <jinankjain@linux.microsoft.com>
>>
>> Introduce HVCALL_MAP_STATS_PAGE2 which provides a map location (GPFN)
>> to map the stats to. This hypercall is required for L1VH partitions,
>> depending on the hypervisor version. This uses the same check as the
>> state page map location; mshv_use_overlay_gpfn().
>>
>> Add mshv_map_vp_state_page() helpers to use this new hypercall or the
>> old one depending on availability.
>>
>> For unmapping, the original HVCALL_UNMAP_STATS_PAGE works for both
>> cases.
>>
>> Signed-off-by: Jinank Jain <jinankjain@linux.microsoft.com>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> Reviewed-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
>> ---
>> drivers/hv/mshv_root.h | 10 ++--
>> drivers/hv/mshv_root_hv_call.c | 93 ++++++++++++++++++++++++++++++++--
>> drivers/hv/mshv_root_main.c | 22 ++++----
>> include/hyperv/hvgdk_mini.h | 1 +
>> include/hyperv/hvhdk_mini.h | 7 +++
>> 5 files changed, 113 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/hv/mshv_root.h b/drivers/hv/mshv_root.h
>> index dbe2d1d0b22f..0dfccfbe6123 100644
>> --- a/drivers/hv/mshv_root.h
>> +++ b/drivers/hv/mshv_root.h
>> @@ -297,11 +297,11 @@ int hv_call_connect_port(u64 port_partition_id, union hv_port_id port_id,
>> int hv_call_disconnect_port(u64 connection_partition_id,
>> union hv_connection_id connection_id);
>> int hv_call_notify_port_ring_empty(u32 sint_index);
>> -int hv_call_map_stat_page(enum hv_stats_object_type type,
>> - const union hv_stats_object_identity *identity,
>> - void **addr);
>> -int hv_call_unmap_stat_page(enum hv_stats_object_type type,
>> - const union hv_stats_object_identity *identity);
>> +int hv_map_stats_page(enum hv_stats_object_type type,
>> + const union hv_stats_object_identity *identity,
>> + void **addr);
>> +int hv_unmap_stats_page(enum hv_stats_object_type type, void *page_addr,
>> + const union hv_stats_object_identity *identity);
>> int hv_call_modify_spa_host_access(u64 partition_id, struct page **pages,
>> u64 page_struct_count, u32 host_access,
>> u32 flags, u8 acquire);
>> diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
>> index 98c6278ff151..5a805b3dec0b 100644
>> --- a/drivers/hv/mshv_root_hv_call.c
>> +++ b/drivers/hv/mshv_root_hv_call.c
>> @@ -804,9 +804,51 @@ hv_call_notify_port_ring_empty(u32 sint_index)
>> return hv_result_to_errno(status);
>> }
>>
>> -int hv_call_map_stat_page(enum hv_stats_object_type type,
>> - const union hv_stats_object_identity *identity,
>> - void **addr)
>> +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);
>> +
>> + ret = hv_result_to_errno(status);
>> +
>> + if (!ret)
>> + break;
>
> This logic is incorrect. If the hypercall returns
> HV_STATUS_INSUFFICIENT_MEMORY, then errno -ENOMEM is immediately
> returned to the caller without any opportunity to do hv_call_deposit_pages().
>
The loop breaks if (!ret), i.e. on success. Maybe you misread it as `if (ret)`?
>> +
>> + 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);
>> + } while (!ret);
>> +
>> + return ret;
>> +}
>> +
>> +static int hv_call_map_stats_page(enum hv_stats_object_type type,
>> + const union hv_stats_object_identity *identity,
>> + void **addr)
>> {
>> unsigned long flags;
>> struct hv_input_map_stats_page *input;
>> @@ -845,8 +887,36 @@ int hv_call_map_stat_page(enum hv_stats_object_type type,
>> return ret;
>> }
>>
>> -int hv_call_unmap_stat_page(enum hv_stats_object_type type,
>> - const union hv_stats_object_identity *identity)
>> +int hv_map_stats_page(enum hv_stats_object_type type,
>> + const union hv_stats_object_identity *identity,
>> + void **addr)
>> +{
>> + int ret;
>> + struct page *allocated_page = NULL;
>> +
>> + if (!addr)
>> + return -EINVAL;
>> +
>> + if (mshv_use_overlay_gpfn()) {
>> + allocated_page = alloc_page(GFP_KERNEL);
>> + if (!allocated_page)
>> + return -ENOMEM;
>> +
>> + ret = hv_call_map_stats_page2(type, identity,
>> + page_to_pfn(allocated_page));
>
> This should use page_to_hvpfn() per my comments in Patch 4 of this series.
>
I could change it, but as mentioned in my reply to Patch 4 I'm not sure it's
worth doing without addressing the whole issue which is a much bigger ask.
>> + *addr = page_address(allocated_page);
>> + } else {
>> + ret = hv_call_map_stats_page(type, identity, addr);
>> + }
>> +
>> + if (ret && allocated_page)
>> + __free_page(allocated_page);
>
> Might want to do *addr = NULL after freeing the page so that the caller
> can't erroneously reference the free page. Again, the current caller doesn't
> do that, so current code isn't broken.
>
Yep, I can add it to provide a little more robustness against bugs in callers.
>> +
>> + return ret;
>> +}
>> +
>> +static int hv_call_unmap_stats_page(enum hv_stats_object_type type,
>> + const union hv_stats_object_identity *identity)
>> {
>> unsigned long flags;
>> struct hv_input_unmap_stats_page *input;
>> @@ -865,6 +935,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)
>> +{
>> + int ret;
>> +
>> + ret = hv_call_unmap_stats_page(type, identity);
>> +
>> + if (mshv_use_overlay_gpfn() && page_addr)
>> + __free_page(virt_to_page(page_addr));
>> +
>> + return ret;
>> +}
>> +
>> int hv_call_modify_spa_host_access(u64 partition_id, struct page **pages,
>> u64 page_struct_count, u32 host_access,
>> u32 flags, u8 acquire)
>> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
>> index 2d0ad17acde6..71a8ab5db3b8 100644
>> --- a/drivers/hv/mshv_root_main.c
>> +++ b/drivers/hv/mshv_root_main.c
>> @@ -841,7 +841,8 @@ mshv_vp_release(struct inode *inode, struct file *filp)
>> return 0;
>> }
>>
>> -static void mshv_vp_stats_unmap(u64 partition_id, u32 vp_index)
>> +static void mshv_vp_stats_unmap(u64 partition_id, u32 vp_index,
>> + void *stats_pages[])
>> {
>> union hv_stats_object_identity identity = {
>> .vp.partition_id = partition_id,
>> @@ -849,10 +850,10 @@ static void mshv_vp_stats_unmap(u64 partition_id, u32 vp_index)
>> };
>>
>> identity.vp.stats_area_type = HV_STATS_AREA_SELF;
>> - hv_call_unmap_stat_page(HV_STATS_OBJECT_VP, &identity);
>> + hv_unmap_stats_page(HV_STATS_OBJECT_VP, NULL, &identity);
>>
>> identity.vp.stats_area_type = HV_STATS_AREA_PARENT;
>> - hv_call_unmap_stat_page(HV_STATS_OBJECT_VP, &identity);
>> + hv_unmap_stats_page(HV_STATS_OBJECT_VP, NULL, &identity);
>> }
>>
>> static int mshv_vp_stats_map(u64 partition_id, u32 vp_index,
>> @@ -865,14 +866,14 @@ static int mshv_vp_stats_map(u64 partition_id, u32 vp_index,
>> int err;
>>
>> identity.vp.stats_area_type = HV_STATS_AREA_SELF;
>> - err = hv_call_map_stat_page(HV_STATS_OBJECT_VP, &identity,
>> - &stats_pages[HV_STATS_AREA_SELF]);
>> + err = hv_map_stats_page(HV_STATS_OBJECT_VP, &identity,
>> + &stats_pages[HV_STATS_AREA_SELF]);
>> if (err)
>> return err;
>>
>> identity.vp.stats_area_type = HV_STATS_AREA_PARENT;
>> - err = hv_call_map_stat_page(HV_STATS_OBJECT_VP, &identity,
>> - &stats_pages[HV_STATS_AREA_PARENT]);
>> + err = hv_map_stats_page(HV_STATS_OBJECT_VP, &identity,
>> + &stats_pages[HV_STATS_AREA_PARENT]);
>> if (err)
>> goto unmap_self;
>>
>> @@ -880,7 +881,7 @@ static int mshv_vp_stats_map(u64 partition_id, u32 vp_index,
>>
>> unmap_self:
>> identity.vp.stats_area_type = HV_STATS_AREA_SELF;
>> - hv_call_unmap_stat_page(HV_STATS_OBJECT_VP, &identity);
>> + hv_unmap_stats_page(HV_STATS_OBJECT_VP, NULL, &identity);
>> return err;
>> }
>>
>> @@ -988,7 +989,7 @@ mshv_partition_ioctl_create_vp(struct mshv_partition *partition,
>> kfree(vp);
>> unmap_stats_pages:
>> if (hv_scheduler_type == HV_SCHEDULER_TYPE_ROOT)
>> - mshv_vp_stats_unmap(partition->pt_id, args.vp_index);
>> + mshv_vp_stats_unmap(partition->pt_id, args.vp_index, stats_pages);
>> unmap_ghcb_page:
>> if (mshv_partition_encrypted(partition) && is_ghcb_mapping_available())
>> hv_unmap_vp_state_page(partition->pt_id, args.vp_index,
>> @@ -1740,7 +1741,8 @@ static void destroy_partition(struct mshv_partition *partition)
>> continue;
>>
>> if (hv_scheduler_type == HV_SCHEDULER_TYPE_ROOT)
>> - mshv_vp_stats_unmap(partition->pt_id, vp->vp_index);
>> + mshv_vp_stats_unmap(partition->pt_id, vp->vp_index,
>> + (void **)vp->vp_stats_pages);
>>
>> if (vp->vp_register_page) {
>> (void)hv_unmap_vp_state_page(partition->pt_id,
>> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
>> index ff4325fb623a..f66565106d21 100644
>> --- a/include/hyperv/hvgdk_mini.h
>> +++ b/include/hyperv/hvgdk_mini.h
>> @@ -493,6 +493,7 @@ union hv_vp_assist_msr_contents { /*
>> HV_REGISTER_VP_ASSIST_PAGE */
>> #define HVCALL_GET_PARTITION_PROPERTY_EX 0x0101
>> #define HVCALL_MMIO_READ 0x0106
>> #define HVCALL_MMIO_WRITE 0x0107
>> +#define HVCALL_MAP_STATS_PAGE2 0x0131
>>
>> /* HV_HYPERCALL_INPUT */
>> #define HV_HYPERCALL_RESULT_MASK GENMASK_ULL(15, 0)
>> diff --git a/include/hyperv/hvhdk_mini.h b/include/hyperv/hvhdk_mini.h
>> index bf2ce27dfcc5..064bf735cab6 100644
>> --- a/include/hyperv/hvhdk_mini.h
>> +++ b/include/hyperv/hvhdk_mini.h
>> @@ -177,6 +177,13 @@ struct hv_input_map_stats_page {
>> union hv_stats_object_identity identity;
>> } __packed;
>>
>> +struct hv_input_map_stats_page2 {
>> + u32 type; /* enum hv_stats_object_type */
>> + u32 padding;
>> + union hv_stats_object_identity identity;
>> + u64 map_location;
>> +} __packed;
>> +
>> struct hv_output_map_stats_page {
>> u64 map_location;
>> } __packed;
>> --
>> 2.34.1
>>
next prev parent reply other threads:[~2025-10-01 23:03 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-26 16:23 [PATCH v4 0/5] mshv: Fixes for stats and vp state page mappings Nuno Das Neves
2025-09-26 16:23 ` [PATCH v4 1/5] mshv: Only map vp->vp_stats_pages if on root scheduler Nuno Das Neves
2025-09-26 16:23 ` [PATCH v4 2/5] mshv: Add the HVCALL_GET_PARTITION_PROPERTY_EX hypercall Nuno Das Neves
2025-09-29 5:31 ` Tianyu Lan
2025-09-29 17:55 ` Michael Kelley
2025-10-01 22:32 ` Nuno Das Neves
2025-10-02 0:01 ` Michael Kelley
2025-09-26 16:23 ` [PATCH v4 3/5] mshv: Get the vmm capabilities offered by the hypervisor Nuno Das Neves
2025-09-26 16:23 ` [PATCH v4 4/5] mshv: Allocate vp state page for HVCALL_MAP_VP_STATE_PAGE on L1VH Nuno Das Neves
2025-09-29 5:31 ` Tianyu Lan
2025-09-29 17:56 ` Michael Kelley
2025-10-01 22:56 ` Nuno Das Neves
2025-10-02 0:02 ` Michael Kelley
2025-10-04 15:25 ` Michael Kelley
2025-10-10 19:08 ` Nuno Das Neves
2025-09-26 16:23 ` [PATCH v4 5/5] mshv: Introduce new hypercall to map stats page for L1VH partitions Nuno Das Neves
2025-09-29 17:57 ` Michael Kelley
2025-10-01 23:03 ` Nuno Das Neves [this message]
2025-10-02 0:16 ` Michael Kelley
2025-09-26 23:12 ` [PATCH v4 0/5] mshv: Fixes for stats and vp state page mappings Stanislav Kinsburskii
2025-09-29 18:19 ` Nuno Das Neves
2025-09-30 23:14 ` Wei Liu
2025-10-03 16:31 ` Stanislav Kinsburskii
2025-10-03 17:30 ` Wei Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=33868cb1-4cb1-40bb-a773-8f15ec75f679@linux.microsoft.com \
--to=nunodasneves@linux.microsoft.com \
--cc=anirudh@anirudhrb.com \
--cc=decui@microsoft.com \
--cc=easwar.hariharan@linux.microsoft.com \
--cc=haiyangz@microsoft.com \
--cc=jinankjain@linux.microsoft.com \
--cc=kys@microsoft.com \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mhklinux@outlook.com \
--cc=paekkaladevi@linux.microsoft.com \
--cc=prapal@linux.microsoft.com \
--cc=skinsburskii@linux.microsoft.com \
--cc=tiala@microsoft.com \
--cc=wei.liu@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox