Re: [PATCH v3 1/7] Drivers: hv: Introduce hv_hvcall_*() functions for hypercall arguments

[Mukesh R <mrathor@linux.microsoft.com>]

On 8/20/25 17:31, Mukesh R wrote:
> On 4/15/25 11:07, mhkelley58@gmail.com wrote:
>> From: Michael Kelley <mhklinux@outlook.com>
>>
>> Current code allocates the "hyperv_pcpu_input_arg", and in
>> some configurations, the "hyperv_pcpu_output_arg". Each is a 4 KiB
>> page of memory allocated per-vCPU. A hypercall call site disables
>> interrupts, then uses this memory to set up the input parameters for
>> the hypercall, read the output results after hypercall execution, and
>> re-enable interrupts. The open coding of these steps leads to
>> inconsistencies, and in some cases, violation of the generic
>> requirements for the hypercall input and output as described in the
>> Hyper-V Top Level Functional Spec (TLFS)[1].
>>
> <snip>
> 
>> The new functions are realized as a single inline function that
>> handles the most complex case, which is a hypercall with input
>> and output, both of which contain arrays. Simpler cases are mapped to
>> this most complex case with #define wrappers that provide zero or NULL
>> for some arguments. Several of the arguments to this new function
>> must be compile-time constants generated by "sizeof()"
>> expressions. As such, most of the code in the new function can be
>> evaluated by the compiler, with the result that the code paths are
>> no longer than with the current open coding. The one exception is
>> new code generated to zero the fixed-size portion of the input area
>> in cases where it is not currently done.
> 
> IMHO, this is unnecessary change that just obfuscates code. With status quo
> one has the advantage of seeing what exactly is going on, one can use the
> args any which way, change batch size any which way, and is thus flexible.
> With time these functions only get more complicated and error prone. The
> saving of ram is very minimal, this makes analyzing crash dumps harder,
> and in some cases like in your patch 3/7 disables unnecessarily in error case:
> 
> - if (count > HV_MAX_MODIFY_GPA_REP_COUNT) {
> -  pr_err("Hyper-V: GPA count:%d exceeds supported:%lu\n", count,
> -   HV_MAX_MODIFY_GPA_REP_COUNT);
> + local_irq_save(flags);      <<<<<<<
> ...
> 
> So, this is a nak from me. sorry.
> 

Furthermore, this makes us lose the ability to permanently map
input/output pages in the hypervisor. So, Wei kindly undo.

Thanks,
-Mukesh



> <snip>
> 
>> +/*
>> + * Allocate one page that is shared between input and output args, which is
>> + * sufficient for all current hypercalls. If a future hypercall requires
> 
> That is incorrect. We've iommu map hypercalls that will use up entire page
> for input. More coming as we implement ram withdrawl from the hypervisor.
> 
> Thanks,
> -Mukesh