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

From: Julien Grall <julien@xen.org>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
	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: Mon, 30 Dec 2019 13:45:06 +0000	[thread overview]
Message-ID: <7b5aa597-1cc9-a601-4eec-a6400a312cfc@xen.org> (raw)
In-Reply-To: <967aad91-812f-3178-e8f5-1b787127ee8d@citrix.com>

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?

> 
> 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've fixed the libxl code to check return values, but when the
> hypervisor has its
> "not-quite-an-XSA-because-the-guest-induced-damage-is-in-unsupported-subsystems"
> bugs fixed, these hypercalls will start failing.

Cheers,

-- 
Julien Grall

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