Re: [Xen-devel] [PATCH 08/18] PVH xen: tools changes to create PVH domain

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-08-28 at 18:51 -0700, Mukesh Rathor wrote:
> On Thu, 1 Aug 2013 09:01:31 +0100
> Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 
> > On Wed, 2013-07-31 at 19:02 -0700, Mukesh Rathor wrote:
> > > On Wed, 31 Jul 2013 13:00:57 +0100
> > > Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> > > 
> > > > On Tue, 2013-07-30 at 16:47 -0700, Mukesh Rathor wrote:
> > > > > On Mon, 17 Jun 2013 12:11:34 +0100
> > > > > Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> ...
> > I think it is valid for a user to both request PVH mode and request
> > some features of their own which they want to enable for this guest.
> > The code should be set up to deal with  this, even if that currently
> > means rejecting any user request for a feature not in the PVH set
> > (although I'd prefer to see a stronger rationale for rejecting a
> > requested feature than that).
> > 
> > >  I can juse use the existing 
> > > xc_interface_core.flags? (would like to rename it to xc_flags so
> > > one can easily find its usages please :)).
> > 
> > Pointless churn. You've made this argument countless times about
> > various and been told no by several different people, please just
> > drop it.
> > 
> > >  So:
> > > 
> > > xc_dom_allocate:
> > > 
> > >     if (xch->flags & PVH)
> > >     {
> > >         if (features)
> > >         {
> > >             error
> > >             return NULL;
> > 
> > No, please handle the case of the user asking for features.
> > 
> > At the very least if they only ask for things in the set you are
> > forcing then it is fine.
> > 
> > >         }
> > >         features =
> > > writable_descriptor_tables|auto_translated_physmap"
> > > "|supervisor_mode_kernel|hvm_callback_vector;
> > 
> > Don't do this. Instead, drop this whole if and add below:
> > 
> > >     }
> > >     if ( features )
> > >         elf_xen_parse_features(features, dom->f_requested, NULL);
> > 
> >     if ( xch->flags & PVH )
> >             elf_xen_parse_features("writable_desc...|etc",
> > dom->f_requested, dom->f_required)
> 
> Hmm.. the problem I am running here now is setting of PVH flag in 
> xch->flags from libxl? struct xch seems to be private to libxc.

xch is the libxc handle used by all the api calls, so it can't be
private to libxc. There is an xch inside the libxl ctx, use either
ctx->xch or CTX->xch depending on whether you have a ctx or a gc in the
function in question.

Actually, xch->flags & PVH is not the right place. xch is a handle onto
an open libxc instance, it is not per-domain, so adding PVH to
xch->flags is wrong. Not sure how I missed that initially.

I think you need to add the flag to the dom->flags in libxl__build_pv. I
don't think anything before the existing setting of that field needs to
know if the guest is PVH or not. The calls between xc_dom_allocate and
there are xc_dom_(kernel|ramdisk)_(file|mem) which are just setting up
internal state and not touching the guest yet. If I'm wrong about that
then I think the block setting all of those dom->fields can be moved up.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel