From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> Sent: Tuesday, January 28, 2025 4:46 PM
>
> On 1/28/2025 10:45 AM, Michael Kelley wrote:
> > From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> Sent: Wednesday, January 22, 2025 5:48 PM
> >>
> >> Move hv_current_partition_id and hv_get_partition_id() to hv_common.c.
> >> These aren't specific to x86_64 and will be needed by common code.
> >>
> >> Set hv_current_partition_id to HV_PARTITION_ID_SELF by default.
> >>
> >> Use a stack variable for the output of the hypercall. This allows moving
> >> the call of hv_get_partition_id() to hv_common_init() before the percpu
> >> pages are initialized.
> >>
> >> Remove the BUG()s. Failing to get the id need not crash the machine.
> >>
> >> Signed-off-by: Nuno Das Neves <nudasnev@xxxxxxxxxxxxx>
> >> ---
> >> arch/x86/hyperv/hv_init.c | 26 --------------------------
> >> arch/x86/include/asm/mshyperv.h | 2 --
> >> drivers/hv/hv_common.c | 23 +++++++++++++++++++++++
> >> include/asm-generic/mshyperv.h | 1 +
> >> 4 files changed, 24 insertions(+), 28 deletions(-)
[snip]
> >> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> >> index af5d1dc451f6..1da19b64ef16 100644
> >> --- a/drivers/hv/hv_common.c
> >> +++ b/drivers/hv/hv_common.c
> >> @@ -31,6 +31,9 @@
> >> #include <hyperv/hvhdk.h>
> >> #include <asm/mshyperv.h>
> >>
> >> +u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
> >> +EXPORT_SYMBOL_GPL(hv_current_partition_id);
> >> +
> >> /*
> >> * hv_root_partition, ms_hyperv and hv_nested are defined here with other
> >> * Hyper-V specific globals so they are shared across all architectures and are
> >> @@ -283,6 +286,23 @@ static inline bool hv_output_page_exists(void)
> >> return hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE);
> >> }
> >>
> >> +static void __init hv_get_partition_id(void)
> >> +{
> >> + /*
> >> + * Note in this case the output can be on the stack because it is just
> >> + * a single u64 and hence won't cross a page boundary.
> >> + */
> >> + struct hv_get_partition_id output;
> >
> > It's unfortunate that the structure name "hv_get_partition_id" is also
> > the name of this function. Could the structure name be changed to
> > follow the pattern of having "output" in the name, like other hypercall
> > parameters? It's not a blocker if it can't be changed. I was just surprised
> > to search for "hv_get_partition_id" and find both uses.
> >
>
> hv_output_get_partition_id is really the "correct" name from the Hyper-V code,
> so it makes sense to just change it to that in this patch.
>
> > Also, see the comment at the beginning of hv_query_ext_cap() regarding
> > using a local stack variable as hypercall input or output. The comment
> > originated here [1]. At that time, I didn't investigate Sunil's assertion any
> > further, and I'm still unsure whether it is really true. But perhaps for
> > consistency and safety we should follow what it says.
> >
> > [1] https://lore.kernel.org/linux-hyperv/SN4PR2101MB0880DB0606A5A0B72AD244B4C06A9@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> >
> Hmm, from some cursory research it does look like stack variables can't be
> used with virt_to_phys().
>
> I thought about just using &hv_current_partition directly - I *think* that
> will work - but in the end I think it's just simpler to just move calls so the
> percpu output page can be used as normal. That may save some additional
> back-and-forth as well as explanatory comments in the code.
>
> I will also add a check for hv_output_page_exists() here, as a precaution in
> case the HV_ACCESS_PARTITION_ID privilege ever becomes decoupled from
> that (as it stands, I believe that permission is only for the root
> partition, but you never know).
Or just use the hyperv_pcpu_input_page, even though the use here is
for output. Then you don't have to worry about whether the output
page exists. FWIW, I'm working on a set of changes that encapsulates
the assignment of the per-cpu hypercall argument pages, and it does
away with the distinction between the input and output pages. But
that will come sometime after this patch.
Michael
>
> >> + u64 status;
> >> +
> >> + status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, &output);
> >> + if (!hv_result_success(status)) {
> >> + pr_err("Hyper-V: failed to get partition ID: %#lx\n", status);
> >> + return;
> >> + }
> >> + hv_current_partition_id = output.partition_id;
> >> +}
> >> +
> >> int __init hv_common_init(void)
> >> {
> >> int i;
> >> @@ -298,6 +318,9 @@ int __init hv_common_init(void)
> >> if (hv_is_isolation_supported())
> >> sysctl_record_panic_msg = 0;
> >>
> >> + if (ms_hyperv.priv_high & HV_ACCESS_PARTITION_ID)
> >> + hv_get_partition_id();
> >
> > I don't see how this works. On the x86 side, hv_common_init()
> > is called before the guest ID is set and the hypercall page is setup.
> > So the hypercall in hv_get_partition_id() should fail.
> >
>
> Oh, I tried to get too clever. I will put it back where it was and
> add it on the arm64 side to hyperv_init() after the per-cpu init as
> I mentioned above.
>
> Thanks for the comments,
> Nuno
