RE: [PATCH v2 1/2] hyperv: Move hv_current_partition_id to arch-generic code

[Michael Kelley]

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