Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Sunil Muthuswamy <sunilmut@microsoft.com>
To: vkuznets <vkuznets@redhat.com>
Cc: "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Wei Liu <liuwe@microsoft.com>,
	Michael Kelley <mikelley@microsoft.com>,
	Tianyu Lan <Tianyu.Lan@microsoft.com>
Subject: RE: [EXTERNAL] Re: [PATCH] x86/Hyper-V: Support for free page reporting
Date: Fri, 22 May 2020 17:02:41 +0000
Message-ID: <SN4PR2101MB0880E7DD03EF18680A2A88B8C0B40@SN4PR2101MB0880.namprd21.prod.outlook.com> (raw)
In-Reply-To: <87ftbvt21h.fsf@vitty.brq.redhat.com>

> 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

  parent reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-19 18:37 Sunil Muthuswamy
2020-05-20  8:59 ` Vitaly Kuznetsov
2020-05-20  9:05   ` Wei Liu
2020-05-22 17:02   ` Sunil Muthuswamy [this message]
2020-05-20  9:01 ` Wei Liu
2020-05-22 16:40   ` [EXTERNAL] " Sunil Muthuswamy
2020-05-22 20:11     ` Alexander Duyck
2020-05-23  0:40     ` Michael Kelley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=SN4PR2101MB0880E7DD03EF18680A2A88B8C0B40@SN4PR2101MB0880.namprd21.prod.outlook.com \
    --to=sunilmut@microsoft.com \
    --cc=Tianyu.Lan@microsoft.com \
    --cc=haiyangz@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=liuwe@microsoft.com \
    --cc=mikelley@microsoft.com \
    --cc=sthemmin@microsoft.com \
    --cc=vkuznets@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git