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:     Wei Liu <wei.liu@kernel.org>,
        Alexander Duyck <alexander.h.duyck@linux.intel.com>
CC:     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>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [EXTERNAL] Re: [PATCH] x86/Hyper-V: Support for free page
 reporting
Thread-Topic: [EXTERNAL] Re: [PATCH] x86/Hyper-V: Support for free page
 reporting
Thread-Index: AdYuDAEiPLOc+vnoR1KnBaBInLPPAQAeU/0AAHOA1cA=
Date:   Fri, 22 May 2020 16:40:15 +0000
Message-ID: <SN4PR2101MB08804F99992085C64982C821C0B40@SN4PR2101MB0880.namprd21.prod.outlook.com>
References: <SN4PR2101MB0880BB5C9780A854B2609992C0B90@SN4PR2101MB0880.namprd21.prod.outlook.com>
 <20200520090158.4x4lkbssm7ncirn7@liuwe-devbox-debian-v2.j3c5onc20sse1dnehy4noqpfcg.zx.internal.cloudapp.net>
In-Reply-To: <20200520090158.4x4lkbssm7ncirn7@liuwe-devbox-debian-v2.j3c5onc20sse1dnehy4noqpfcg.zx.internal.cloudapp.net>
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: d3366da3-becf-49c2-4852-08d7fe6ece99
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 May 2020 16:40:16.0815
 (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: GBWO7d9tkVVe0N4FCNojO+gNHRNxdPCKpfgLmql75m/yelCnEG8lBJA5/6ZPmSHax2acvdKac6NqOsSUFUBurMvxB+zs6R2bcGkIgnNEVpU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN4PR2101MB0813
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > +	if (hv_do_hypercall(HV_EXT_CALL_QUERY_CAPABILITIES, NULL, cap) =3D=3D
> > +	    HV_STATUS_SUCCESS)
>=20
> You're using the input page as the output parameter. Ideally we should
> introduce hyperv_pcpu_output_arg page, but that would waste one page per
> cpu just for this one call.
>=20
> So for now I think this setup is fine, but I would like to add the
> following comment.
>=20
>     /*
>      * Repurpose the input_arg page 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 page once we have more places that require output.
>      */

Sounds good. Will add it in v2.

> > +#ifdef CONFIG_PAGE_REPORTING
> > +static int hv_free_page_report(struct page_reporting_dev_info *pr_dev_=
info,
> > +		    struct scatterlist *sgl, unsigned int nents)
> > +{
> > +	unsigned long flags;
> > +	struct hv_memory_hint *hint;
> > +	int i;
> > +	u64 status;
> > +	struct scatterlist *sg;
> > +
> > +	WARN_ON(nents > HV_MAX_GPA_PAGE_RANGES);
>=20
> Should we return -ENOSPC here?

This is more of an assert because PAGE_REPORTING_CAPACITY is set to 32 and =
has
already been checked that it is < HV_MAX_GPA_PAGE_RANGES in enable_page_rep=
orting.

> > +	hint->type =3D HV_EXT_MEMORY_HEAT_HINT_TYPE_COLD_DISCARD;
> > +	hint->reserved =3D 0;
> > +	for (i =3D 0, sg =3D sgl; sg; sg =3D sg_next(sg), i++) {
> > +		int order;
> > +		union hv_gpa_page_range *range;
> > +
>=20
> Unfortunately I can't find the semantics of this hypercall in TLFS 6, so
> I have a few questions here.

This structure is not specific to this hypercall.

>=20
> > +		order =3D get_order(sg->length);
> > +		range =3D &hint->ranges[i];
> > +		range->address_space =3D 0;
>=20
> I guess this means all address spaces?

'address_space' is being used here just as a zero initializer for the union=
. I think the use
of 'address_space' in the definition of hv_gpa_page_range is not appropriat=
e. This struct is
defined in the TLFS 6.0 with the same name, if you want to look it up.

>=20
> > +		range->page.largepage =3D 1;
>=20
> What effect does this have? What if the page is a 4k page?

Page reporting infrastructure doesn't report 4k pages today, but only huge =
pages (see=20
PAGE_REPORTING_MIN_ORDER in page_reporting.h). Additionally, the Hyper-V hy=
pervisor
only supports reporting of 2M pages and above. The current code assumes tha=
t the minimum
order will be 9 i.e 2M pages and above.
If we feel that this could change in the future, or an implementation detai=
l that should be
protected against, I can add some checks in hv_balloon.c. But, in that case=
, the ballon driver
should be able to query the page reporting min order somehow, which it is c=
urrently, since it is
private.
Alexander, do you have any suggestions or feedback here?

>=20
> > +		range->page.additional_pages =3D (1ull << (order - 9)) - 1;
>=20
> What is 9 here? Is there a macro name *ORDER that you can use?

It is to determine the count of 2M pages. I will define a macro.

>=20
> Wei.