RE: [PATCH v4] x86/Hyper-V: Support for free page reporting

[Michael Kelley]

From: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx> Sent: Thursday, March 18, 2021 1:14 PM
> 
> Linux has support for free page reporting now (36e66c554b5c) for
> virtualized environment. On Hyper-V when virtually backed VMs are
> configured, Hyper-V will advertise cold memory discard capability,
> when supported. This patch adds the support to hook into the free
> page reporting infrastructure and leverage the Hyper-V cold memory
> discard hint hypercall to report/free these pages back to the host.
> 
> Signed-off-by: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx>
> Tested-by: Matheus Castello <matheus@xxxxxxxxxxxxxxx>
> ---
> In V2:
> - Addressed feedback comments
> - Added page reporting config option tied to hyper-v balloon config
> 
> In V3:
> - Addressed feedback from Vitaly
> 
> In V4:
> - Queried and cached the Hyper-V extended capability for the lifetime
>   of the VM
> - Addressed feedback from Michael Kelley.
> ---
>  arch/x86/hyperv/hv_init.c         | 45 +++++++++++++++-
>  arch/x86/kernel/cpu/mshyperv.c    |  9 ++--
>  drivers/hv/Kconfig                |  1 +
>  drivers/hv/hv_balloon.c           | 89 +++++++++++++++++++++++++++++++
>  include/asm-generic/hyperv-tlfs.h | 35 +++++++++++-
>  include/asm-generic/mshyperv.h    |  3 +-
>  6 files changed, 174 insertions(+), 8 deletions(-)
> 

[snip]

> +/* Bit mask of the extended capability to query: see HV_EXT_CAPABILITY_xxx */
> +bool hv_query_ext_cap(u64 cap_query)
> +{
> +	/*
> +	 * The address of the 'hv_extended_cap' variable will be used as an
> +	 * output parameter to the hypercall below and so it should be
> +	 * compatible with 'virt_to_phys'. Which means, it's address should be
> +	 * directly mapped. Use 'static' to keep it compatible; stack variables
> +	 * can be virtually mapped, making them imcompatible with
> +	 * 'virt_to_phys'.
> +	 * Hypercall input/output addresses should also be 8-byte aligned.
> +	 */
> +	static u64 hv_extended_cap __aligned(8);
> +	static bool hv_extended_cap_queried;
> +	u64 status;
> +
> +	/*
> +	 * Querying extended capabilities is an extended hypercall. Check if the
> +	 * partition supports extended hypercall, first.
> +	 */
> +	if (!(ms_hyperv.priv_high & HV_ENABLE_EXTENDED_HYPERCALLS))
> +		return false;
> +
> +	/* Extended capabilities do not change at runtime. */
> +	if (hv_extended_cap_queried)
> +		return hv_extended_cap & cap_query;
> +
> +	status = hv_do_hypercall(HV_EXT_CALL_QUERY_CAPABILITIES, NULL,
> +				 &hv_extended_cap);
> +	hv_extended_cap_queried = true;

What's the strategy for this flag in the unlikely event that the hypercall fails?
It doesn't seem right to have hv_query_ext_cap() fail, but leave the
static flag set to true.  Just move that line down to after the status check
has succeeded?

> +	status &= HV_HYPERCALL_RESULT_MASK;
> +	if (status != HV_STATUS_SUCCESS) {
> +		pr_err("Hyper-V: Extended query capabilities hypercall failed 0x%llx\n",
> +		       status);
> +		return false;
> +	}
> +
> +	return hv_extended_cap & cap_query;
> +}
> +EXPORT_SYMBOL_GPL(hv_query_ext_cap);

Other than the above about the flag when the hypercall fails,
everything else looks good.

Michael