Re: [Xen-devel] [PATCH] tools/libxl: Reposition build_pre() logic between architectures

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Julien Grall <julien@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Jan Beulich" <JBeulich@suse.com>,
	"Anthony Perard" <anthony.perard@citrix.com>,
	"Ian Jackson" <Ian.Jackson@citrix.com>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH] tools/libxl: Reposition build_pre() logic between architectures
Date: Thu, 2 Jan 2020 17:40:35 +0000	[thread overview]
Message-ID: <3139d26f-62e1-6afe-822a-75fb2144ca5c@citrix.com> (raw)
In-Reply-To: <7b5aa597-1cc9-a601-4eec-a6400a312cfc@xen.org>

On 30/12/2019 13:45, Julien Grall wrote:
> Hi,
>
> On 30/12/2019 13:38, Andrew Cooper wrote:
>> On 30/12/2019 13:15, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> On 27/12/2019 13:45, Andrew Cooper wrote:
>>>> The call to xc_domain_disable_migrate() is made only from x86,
>>>> while its
>>>> handling in Xen is common.  Move it to the libxl__build_pre().
>>>>
>>>> hvm_set_conf_params(), hvm_set_viridian_features(),
>>>> hvm_set_mca_capabilities(), and the altp2m logic is all in common
>>>> code (parts ifdef'd) but despite this, is all actually x86 specific.
>>>
>>> While altp2m is only supported on x86, the concept is not
>>> x86-specific. I am actually aware of people using altp2m on Arm,
>>> althought the support is not upstream yet.
>>>
>>>>
>>>> Move it into x86 specific code, and fold all of the
>>>> xc_hvm_param_set() calls
>>>> together into hvm_set_conf_params() in a far more coherent way.
>>>>
>>>> Finally - ensure that all hypercalls have their return values checked.
>>>>
>>>> No practical change in constructed domains.  Fewer useless hypercalls
>>>> now to
>>>> construct an ARM guest.
>>>
>>> I think it would be best to keep anything that we know can be used on
>>> arm (or new architecture) in common code. I am thinking about
>>> "nestedhvm" and "altp2m".
>>
>> Neither of those options are going to survive in this form.
>
> Oh, it wasn't clear from the commit message. Would you mind to add a
> sentence in the commit message about it?

Well - they aren't going to survive long-term in this form.  Both need
to become domain_create parameters.

Whether or not they actually get changed before someone tries
upstreaming the ARM Altp2m work is a different matter, if that affects
your answer.

>
>>
>> Also, the checks can't stay in common code.  Currently, Xen doesn't
>> reject bad parameters, and the toolstack doesn't check return values.
>> Frankly, neither of these bugs should ever have got through code review,
>> seeing as we were doing rather better code review by the time the ARM
>> port came about.
>
> The HVM_PARAM is not great on Arm :(. It would be nice to get this
> fixed properly.

I looked back at my patch series doing just this, and despite being a
year old, I'm still sufficiently irritated at the nitpicking and
inability to read/interpret CODING_STYLE that I don't feel like wasting
any more of my time right now.

~Andrew

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