Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sunil Muthuswamy <sunilmut@microsoft.com>
To:     vkuznets <vkuznets@redhat.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Matheus Castello <matheus@castello.eng.br>,
        KY Srinivasan <kys@microsoft.com>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        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>,
        Wei Liu <wei.liu@kernel.org>
Subject: RE: [EXTERNAL] Re: [PATCH v2] x86/Hyper-V: Support for free page
 reporting
Thread-Topic: [EXTERNAL] Re: [PATCH v2] x86/Hyper-V: Support for free page
 reporting
Thread-Index: AdbjiOckXy2XABo0QQC5vn/n9MZDigApuVgAABBfCaA=
Date:   Wed, 6 Jan 2021 23:04:01 +0000
Message-ID: <SN4PR2101MB088078C9B0C7A6F41E7FC919C0D09@SN4PR2101MB0880.namprd21.prod.outlook.com>
References: <SN4PR2101MB08800F9A2F6961A05DAEAE4BC0D19@SN4PR2101MB0880.namprd21.prod.outlook.com>
 <87v9cagpor.fsf@vitty.brq.redhat.com>
In-Reply-To: <87v9cagpor.fsf@vitty.brq.redhat.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 6c992b2e-6d64-4f99-25a6-08d8b2975b31
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jan 2021 23:04:01.1156
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: s8Zkv6w4DXH4MVuCbcHsUWzCJE3vjGOGI/QrNv8WTvUFHwsEAnZ8asdza6EFNPazLLFTspmVro0NcSpwlEaEhHIT0+IK6OjqiBdQm7eTIlk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR2101MB0896
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > +// Bit mask of the extended capability to query: see HV_EXT_CAPABILITY=
_xxx
>=20
> Please don't use '//' comments in Linux (here and below)
Will fix in v3.

> > +	/*
> > +	 * Repurpose the input page arg to accept output from Hyper-V for
> > +	 * now because this is the only call that needs output from the
> > +	 * hypervisor. It should be fixed properly by introducing an
> > +	 * output arg once we have more places that require output.
> > +	 */
>=20
> I remember there was a patch from Wei (realter to running Linux as root
> partition) doing the job, we can probably merge it early to avoid this
> re-purposing.
I would prefer getting this in right now and we can update this once Wei's
patches gets merged.

> > -	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, privilege flags high: 0x%x, hints 0x=
%x, misc 0x%x\n",
> > +		ms_hyperv.features, ms_hyperv.priv_high, ms_hyperv.hints,
> > +		ms_hyperv.misc_features);
>=20
> Even if we would like to avoid the curn and keep 'ms_hyperv.features',
> we could've reported this as
>=20
> "privilege flags low:%0x%x high:0x%x" to avoid the misleading 'features'.
>=20
Sure, will change it in v3.

> > +	WARN_ON(nents > HV_MEMORY_HINT_MAX_GPA_PAGE_RANGES);
>=20
> WARN_ON_ONCE() maybe?
Sure, coming in v3.

> > +	for (i =3D 0, sg =3D sgl; sg; sg =3D sg_next(sg), i++) {
>=20
> This looks like an open-coded for_each_sg() version.
Thanks, will change in v3.

> > +	BUILD_BUG_ON(pageblock_order < HV_MIN_PAGE_REPORTING_ORDER);
> > +	if (!hv_query_ext_cap(HV_EXT_CAPABILITY_MEMORY_COLD_DISCARD_HINT)) {
> > +		pr_info("Cold memory discard hint not supported by Hyper-V\n");
> > +		return;
> > +	}
>=20
> Which Hyper-V versions/Azure instance types don't support it? In case
> there's something fairly modern on the list, I'd suggest to drop this
> pr_info (or convert it to pr_debug) to not mislead users into thinking
> there's something wrong. In case they don't see 'Cold memory discard
> hint enabled' they know it is unsupported.
>=20
This is not available in Hypervisor spec 5.0 and was added in 6.0. I don't =
see any problems
with making it 'pr_debug'

> > +#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[];
> > +};
>=20
> Other similar structures use '__packed' but I remember there was some
> disagreement if this is needed or not when everything is properly padded
> (or doesn't require padding like in this case).
>=20
There are other places where we don't use '__packed' where everything is
properly aligned. But, I will add it as I don't see a problem with it.

- Sunil