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

[Boqun Feng]

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

> +	__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);
> +}
[...]