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

[Sunil Muthuswamy]

> As the only usage of this function looks like
> if (!(hv_query_ext_cap() & HV_EXT_CAPABILITY_MEMORY_COLD_DISCARD_HINT))
> 
> I would've change the interface to
> 
> bool hv_query_ext_cap(u64 cap)
> 
> so the usage would look like
> 
> if (!(hv_query_ext_cap(HV_EXT_CAPABILITY_MEMORY_COLD_DISCARD_HINT))

Good idea. Will do in v2.

> >  	ms_hyperv.features = cpuid_eax(HYPERV_CPUID_FEATURES);
> > +	ms_hyperv.b_features = cpuid_ebx(HYPERV_CPUID_FEATURES);
> >  	ms_hyperv.misc_features = cpuid_edx(HYPERV_CPUID_FEATURES);
> >  	ms_hyperv.hints    = cpuid_eax(HYPERV_CPUID_ENLIGHTMENT_INFO);
> >
> > -	pr_info("Hyper-V: features 0x%x, hints 0x%x, misc 0x%x\n",
> > -		ms_hyperv.features, ms_hyperv.hints, ms_hyperv.misc_features);
> > +	pr_info("Hyper-V: features 0x%x, additional features: 0x%x, hints 0x%x, misc 0x%x\n",
> > +		ms_hyperv.features, ms_hyperv.b_features, ms_hyperv.hints,
> > +		ms_hyperv.misc_features);
> 
> HYPERV_CPUID_FEATURES(0x40000003) EAX and EBX correspond to Partition
> Privilege Flags (TLFS), I'd suggest to take the opportunity and rename
> this to something like 'privilege flags low=0x%x high=0x%x'.
> 
> Also, I don't quite like 'ms_hyperv.b_features' as I'll always have to
> look at what it's being assigned to understand what it holds. I'd even
> suggest to rename ms_hyperv.features to ms_hyperv.priv_low and
> ms_hyperv.b_features tp ms_hyperv.priv_high. Or maybe even better, pack
> them to the same 'u64 ms_hyperv.privileges'.

Good idea. I will make the change to rename this to 'priv_high' in v2. I like the idea of
combining 'features' & 'priv_high' to a u64, but that would be a cleanup change and a
separate patch.

> 
> Why is largepage always '1'?
I have responded to a similar question by Wei. Page reporting only supports huge pages
and, so does the Hyper-V hypervisor. Let's follow this there.

> 
> > +		range->page.additional_pages = (1ull << (order - 9)) - 1;
> > +		range->base_large_pfn = page_to_pfn(sg_page(sg)) >> 9;
> 
> What is '9'? Could you please define it through PAGE_*/HPAGE_* macro?
Yes, I will define a macro. Essentially, it is to get a count of 2M pages.

> Nit: you could've just used
> 
>         if (status & HV_HYPERCALL_RESULT_MASK != HV_STATUS_SUCCESS) {
Sure, coming in v2.

>         ...
> 
> > +		pr_err("Cold memory discard hypercall failed with status %llx\n",
> > +			status);
> > +		return -1;
> 
> -EFAULT or something like it maybe?
Coming in v2.

> > +#ifdef CONFIG_PAGE_REPORTING
> > +	if (enable_page_reporting() < 0)
> > +		goto probe_error;
> 
> Why? The hyperv-balloon driver itself may still be functional and you
> already set dm_device.pr_dev_info.report to NULL.
An error here would reflect an internal error and should not happen and
it was to make it easy to catch such errors, which are otherwise difficult
with just a print. But, the code should follow the general spirit. I will change
this in v2.

> 
> >  enum HV_GENERIC_SET_FORMAT {
> >  	HV_GENERIC_SET_SPARSE_4K,
> >  	HV_GENERIC_SET_ALL,
> > @@ -371,6 +379,12 @@ union hv_gpa_page_range {
> 
> There is a comment before this structure:
> 
> /* HvFlushGuestPhysicalAddressList hypercall */
> 
> which is now obsolete.

I will add that it also applies to 'HvExtCallMemoryHeatHint' hypercall also.

> 
> >  		u64 largepage:1;
> >  		u64 basepfn:52;
> >  	} page;
> > +	struct {
> > +		u64:12;
> 
> What is this unnamed member? Another 'reserved', 'pad'? Let's name it.

Sure, coming in v2.

> 
> > +		u64 page_size:1;
> > +		u64 reserved:8;
> > +		u64 base_large_pfn:43;
> > +	};
> 
> Please name this structure in the union.

Sure, coming in v2.

> > + */
> > +#define HV_MAX_GPA_PAGE_RANGES ((PAGE_SIZE - sizeof(u64)) / \
> > +				sizeof(union hv_gpa_page_range))
> > +
> 
> The name HV_MAX_GPA_PAGE_RANGES sounds too generic and I think this is
> specific to the HvExtCallMemoryHeatHint hypercall as other hypercalls
> may have a different header length.
> 

Good idea to rename. Coming in v2.

> > +/* HvExtCallMemoryHeatHint hypercall */
> > +#define HV_EXT_MEMORY_HEAT_HINT_TYPE_COLD_DISCARD	2
> > +struct hv_memory_hint {
> > +	u64 type:2;
> > +	u64 reserved:62;
> > +	union hv_gpa_page_range ranges[1];
> 
> Why '[1]' and not '[]'? If it was '[]' you could've used 'sizeof(struct
> hv_memory_hint)' in HV_MAX_GPA_PAGE_RANGES macro definition instead of
> 'sizeof(u64)'.
> 
Good idea, coming in v2.

- Sunil