| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bae5bb62-d480-46fd-837c-9267c0a30fae@linux.microsoft.com>
Date: Fri, 21 Mar 2025 13:11:22 -0700
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: mhklinux@...look.com, kys@...rosoft.com, haiyangz@...rosoft.com,
wei.liu@...nel.org, decui@...rosoft.com, tglx@...utronix.de,
mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
lpieralisi@...nel.org, kw@...ux.com, manivannan.sadhasivam@...aro.org,
robh@...nel.org, bhelgaas@...gle.com, arnd@...db.de
Cc: x86@...nel.org, linux-hyperv@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
linux-arch@...r.kernel.org
Subject: Re: [PATCH v2 4/6] Drivers: hv: Use hv_hvcall_*() to set up hypercall
arguments
On 3/12/2025 11:19 PM, mhkelley58@...il.com wrote:
> From: Michael Kelley <mhklinux@...look.com>
>
> Update hypercall call sites to use the new hv_hvcall_*() functions
> to set up hypercall arguments. Since these functions zero the
> fixed portion of input memory, remove now redundant zero'ing of
> input fields.
>
> hv_post_message() requires additional updates. The payload area is
> treated as an array to avoid wasting cycles on zero'ing it and
> then overwriting with memcpy(). To allow treatment as an array,
> the corresponding payload[] field is updated to have zero size.
>
I'd prefer to leave the payload field as a fixed-sized array.
Changing it to a flexible array makes it look like that input is
for a variable-sized or rep hypercall, and it makes the surrounding
code in hv_post_message() more complex and inscrutable as a result.
I suggest leaving hv_post_message() alone, except for changing
hyperv_pcpu_input_arg -> hyperv_pcpu_arg, and perhaps a comment
explaining why hv_hvcall_input() isn't used there.
> Signed-off-by: Michael Kelley <mhklinux@...look.com>
> ---
> drivers/hv/hv.c | 9 ++++++---
> drivers/hv/hv_balloon.c | 4 ++--
> drivers/hv/hv_common.c | 2 +-
> drivers/hv/hv_proc.c | 8 ++++----
> drivers/hv/hyperv_vmbus.h | 2 +-
> 5 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index a38f84548bc2..e2dcbc816fc5 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -66,7 +66,8 @@ int hv_post_message(union hv_connection_id connection_id,
> if (hv_isolation_type_tdx() && ms_hyperv.paravisor_present)
> aligned_msg = this_cpu_ptr(hv_context.cpu_context)->post_msg_page;
> else
> - aligned_msg = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + hv_hvcall_in_array(&aligned_msg, sizeof(*aligned_msg),
> + sizeof(aligned_msg->payload[0]));
>
> aligned_msg->connectionid = connection_id;
> aligned_msg->reserved = 0;
> @@ -80,8 +81,10 @@ int hv_post_message(union hv_connection_id connection_id,
> virt_to_phys(aligned_msg), 0);
> else if (hv_isolation_type_snp())
> status = hv_ghcb_hypercall(HVCALL_POST_MESSAGE,
> - aligned_msg, NULL,
> - sizeof(*aligned_msg));
> + aligned_msg,
> + NULL,
> + struct_size(aligned_msg, payload,
> + HV_MESSAGE_PAYLOAD_QWORD_COUNT));
See my comment above, I'd prefer to leave this function mostly
alone to maintain readability.
> else
> status = HV_STATUS_INVALID_PARAMETER;
> } else {
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index fec2f18679e3..2def8b8794ee 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -1582,14 +1582,14 @@ static int hv_free_page_report(struct page_reporting_dev_info *pr_dev_info,
> WARN_ON_ONCE(nents > HV_MEMORY_HINT_MAX_GPA_PAGE_RANGES);
> WARN_ON_ONCE(sgl->length < (HV_HYP_PAGE_SIZE << page_reporting_order));
> local_irq_save(flags);
> - hint = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +
> + hv_hvcall_in_array(&hint, sizeof(*hint), sizeof(hint->ranges[0]));
We should ensure the returned batch size is large enough for
"nents".
> if (!hint) {
> local_irq_restore(flags);
> return -ENOSPC;
> }
>
> hint->heat_type = HV_EXTMEM_HEAT_HINT_COLD_DISCARD;
> - hint->reserved = 0;
> for_each_sg(sgl, sg, nents, i) {
> union hv_gpa_page_range *range;
>
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 9804adb4cc56..a6b1cdfbc8d4 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -293,7 +293,7 @@ void __init hv_get_partition_id(void)
> u64 status, pt_id;
>
> local_irq_save(flags);
> - output = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + hv_hvcall_inout(NULL, 0, &output, sizeof(*output));
> status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, &output);
> pt_id = output->partition_id;
> local_irq_restore(flags);
> diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
> index 2fae18e4f7d2..5c580ee1c23f 100644
> --- a/drivers/hv/hv_proc.c
> +++ b/drivers/hv/hv_proc.c
> @@ -73,7 +73,8 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
>
> local_irq_save(flags);
>
> - input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + hv_hvcall_in_array(&input_page, sizeof(*input_page),
> + sizeof(input_page->gpa_page_list[0]));
We should ensure the returned batch size is large enough.
>
> input_page->partition_id = partition_id;
>
> @@ -124,9 +125,8 @@ int hv_call_add_logical_proc(int node, u32 lp_index, u32 apic_id)
> do {
> local_irq_save(flags);
>
> - input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> /* We don't do anything with the output right now */
> - output = *this_cpu_ptr(hyperv_pcpu_output_arg);
> + hv_hvcall_inout(&input, sizeof(*input), &output, sizeof(*output));
>
> input->lp_index = lp_index;
> input->apic_id = apic_id;
> @@ -167,7 +167,7 @@ int hv_call_create_vp(int node, u64 partition_id, u32 vp_index, u32 flags)
> do {
> local_irq_save(irq_flags);
>
> - input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + hv_hvcall_in(&input, sizeof(*input));
>
> input->partition_id = partition_id;
> input->vp_index = vp_index;
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 29780f3a7478..44b5e8330d9d 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -101,7 +101,7 @@ struct hv_input_post_message {
> u32 reserved;
> u32 message_type;
> u32 payload_size;
> - u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT];
> + u64 payload[];
See my comment above, I'd prefer to keep this how it is.
> };
>
>
Thanks
Nuno
Powered by blists - more mailing lists