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

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 02/01/2020 17:40, Andrew Cooper wrote:
> 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.
I am happy with "They should not survive". If the altp2m work is 
upstreamed first then we can have the discussion whether we should carry 
"legacy" interface if this wasn't reworked beforehand.
> > > 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.
Sorry to hear that. I have put the work in my tracking list for Arm.

Cheers,

--
Julien Grall

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