Re: [Xen-devel] [PATCH v6 5/5] domain: use PGC_extra domheap page for shared_info

From: Jan Beulich <jbeulich@suse.com>
To: paul@xen.org
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Paul Durrant <pdurrant@amazon.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	xen-devel@lists.xenproject.org,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [Xen-devel] [PATCH v6 5/5] domain: use PGC_extra domheap page for shared_info
Date: Tue, 17 Mar 2020 14:14:06 +0100	[thread overview]
Message-ID: <bb08d849-bc56-336c-aedc-baca08aa1f9d@suse.com> (raw)
In-Reply-To: <20200310174917.1514-6-paul@xen.org>

On 10.03.2020 18:49, paul@xen.org wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> Currently shared_info is a shared xenheap page but shared xenheap pages
> complicate future plans for live-update of Xen so it is desirable to,
> where possible, not use them [1]. This patch therefore converts shared_info
> into a PGC_extra domheap page. This does entail freeing shared_info during
> domain_relinquish_resources() rather than domain_destroy() so care is
> needed to avoid de-referencing a NULL shared_info pointer hence some
> extra checks of 'is_dying' are needed.

If there's going to be agreement to follow this route, the implementation,
with a really minor cosmetic adjustment - see below -, looks okay to me.
Nevertheless I continue to dislike the implication from the extra care
that's now needed. As I think I have said before, I'd like to have at
least one other REST maintainer's opinion here.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -260,9 +260,12 @@ void dump_pageframe_info(struct domain *d)
>  
>      page_list_for_each ( page, &d->extra_page_list )
>      {
> -        printk("    ExtraPage %p: caf=%08lx, taf=%" PRtype_info "\n",
> +        const char *tag = mfn_eq(page_to_mfn(page), d->shared_info.mfn) ?
> +            "[SHARED INFO]" : "";

Please can this be " [SHARED INFO]" with ...

> +        printk("    ExtraPage %p: caf=%08lx, taf=%" PRtype_info " %s\n",

... the blank before the final %s dropped here, such that we won't
have a trailing blank in the output?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel