From: Nuno Das Neves <nunodasne...@linux.microsoft.com> Sent: Friday, March 21,
2025 1:11 PM
>
> On 3/12/2025 11:19 PM, mhkelle...@gmail.com wrote:
> > From: Michael Kelley <mhkli...@outlook.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 <mhkli...@outlook.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.
Let me try again to introduce hv_hvcall_*() without changing struct
hv_input_post_message. If that struct isn't changed, then the
hv_ghcb_hypercall() arguments don't have to change.
I'd like to reach a point where hyperv_input_arg isn't referenced in any
open coding -- it should be referenced only internally in the hv_call_*()
functions and where it is allocated and deallocated. The arguments to
hv_hvcall_in_array() will be a slightly more complicated to prevent zero'ing
the entire payload area, but I don't think readability alone is justification
for open-coding the use of hyperv_input_arg.
Other reviewers -- please chime in on whether the "no open coding" goal
should be kept. I can drop that goal if that's the way folks prefer.
>
> > 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".
OK, right. That test would replace the WARN_ON_ONCE() based on 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.
OK.
>
> >
> > 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
