RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities

[Michael Kelley]

From: Boqun Feng <boqun.feng@xxxxxxxxx> Sent: Tuesday, February 23, 2021 6:37 PM
> 
> On Thu, Feb 18, 2021 at 03:16:29PM -0800, Michael Kelley wrote:
> [...]
> > +
> > +/*
> > + * Get the value of a single VP register.  One version
> > + * returns just 64 bits and another returns the full 128 bits.
> > + * The two versions are separate to avoid complicating the
> > + * calling sequence for the more frequently used 64 bit version.
> > + */
> > +
> > +void __hv_get_vpreg_128(u32 msr,
> > +			struct hv_get_vp_registers_input  *input,
> > +			struct hv_get_vp_registers_output *res)
> > +{
> > +	u64	status;
> > +
> > +	input->header.partitionid = HV_PARTITION_ID_SELF;
> > +	input->header.vpindex = HV_VP_INDEX_SELF;
> > +	input->header.inputvtl = 0;
> > +	input->element[0].name0 = msr;
> > +	input->element[0].name1 = 0;
> > +
> > +
> > +	status = hv_do_hypercall(
> > +		HVCALL_GET_VP_REGISTERS | HV_HYPERCALL_REP_COMP_1,
> > +		input, res);
> > +
> > +	/*
> > +	 * Something is fundamentally broken in the hypervisor if
> > +	 * getting a VP register fails. There's really no way to
> > +	 * continue as a guest VM, so panic.
> > +	 */
> > +	BUG_ON((status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS);
> > +}
> > +
> > +u64 hv_get_vpreg(u32 msr)
> > +{
> > +	struct hv_get_vp_registers_input	*input;
> > +	struct hv_get_vp_registers_output	*output;
> > +	u64					result;
> > +
> > +	/*
> > +	 * Allocate a power of 2 size so alignment to that size is
> > +	 * guaranteed, since the hypercall input and output areas
> > +	 * must not cross a page boundary.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_ATOMIC);
> > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > +
> 
> Do we need to BUG_ON(!input || !output)? Or we expect the page fault
> (for input being NULL) or the failure of hypercall (for output being
> NULL) to tell us the allocation failed?
> 
> Hmm.. think a bit more on this, maybe we'd better retry the allocation
> if it failed. Because say we are under memory pressusre, and only have
> memory enough for doing one hvcall, and one thread allocates that memory
> but gets preempted by another thread trying to do another hvcall:
> 
> 	<thread 1>
> 	hv_get_vpreg():
> 	  input = kzalloc(...);
> 	  output = kmalloc(...);
> 	<preempted and switch to thread 2>
> 	hv_get_vpreg():
> 	  intput = kzalloc(...); // allocation fails, but actually if
> 	                         // we wait for thread 1 to finish its
> 				 // hvcall, we can get enough memory.
> 
> , in this case, if thread 2 retried, it might get the enough memory,
> therefore there is no need to BUG_ON() on allocation failure. That said,
> I don't think this is likely to happen, and there may be better
> solutions for this, so maybe we can keep it as it is (assuming that
> memory allocation for hvcall never fails) and improve later.
> 
> Regards,
> Boqun

Having to do these memory allocations in order to make a
hypercall results in a lot of messiness.  I've just gone back to
try again at doing hv_get_vpreg() and hv_get_vpreg_128()
as "fast" hypercalls that pass inputs (and outputs) in registers
like hv_set_vpreg().  I have it working now, with some tweaks
to arm_smccc_1_1_hvc() to allow outputs in a wider range
of registers than just X0 thru X3.  This wider range of registers
is allowed by the SMCCC version 1.2 and 1.3 specs, so hopefully
is acceptable.  I'll send out a new version using this "fast"
hypercall approach that completely avoids all these memory
allocation problems.

Michael

> 
> > +	__hv_get_vpreg_128(msr, input, output);
> > +
> > +	result = output->as64.low;
> > +	kfree(input);
> > +	kfree(output);
> > +	return result;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_get_vpreg);
> > +
> > +void hv_get_vpreg_128(u32 msr, struct hv_get_vp_registers_output *res)
> > +{
> > +	struct hv_get_vp_registers_input	*input;
> > +	struct hv_get_vp_registers_output	*output;
> > +
> > +	/*
> > +	 * Allocate a power of 2 size so alignment to that size is
> > +	 * guaranteed, since the hypercall input and output areas
> > +	 * must not cross a page boundary.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_ATOMIC);
> > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > +
> > +	__hv_get_vpreg_128(msr, input, output);
> > +
> > +	res->as64.low = output->as64.low;
> > +	res->as64.high = output->as64.high;
> > +	kfree(input);
> > +	kfree(output);
> > +}
> [...]