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

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

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:
> > 
> 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)
> 
> I'm surprised that this features are only requested for PVH and not
> required (more on this below).
> 
> I see PVH is a flag, are you not intending to handle it as a tristate
> i.e.:
>       PVH = no  : do not use PVH ever
>       PVH = yes : either enable PVH or fail
>         PVH = default : libxc decides if PVH is possible, based on 
>               kernels supported feature set.

Wasn't planning on it, since the original plan was to just have this
temporary while in experimental phase. The prev discussion was to just 
boot in PVH mode if the kernel elf supports it. 

> > Acutally, wait! Looking at code more, I think I found the place we
> > need to put the check.  In xc_dom_parse_elf_kernel:
> > 
> > After:
> >     if ( elf_xen_feature_get(XENFEAT_dom0, dom->parms.f_required) )
> >     {
> >         xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: Kernel does
> > not" " support unprivileged (DomU) operation", __FUNCTION__);
> >         rc = -EINVAL;
> >         goto out;
> >     }
> > Add:
> >     if (dom->pvh)
> >     {
> >         if ( !elf_xen_feature_get(XENFEAT_hvm_callback_vector, 
> >                                  dom->parms.f_supported)   ||
> >              !elf_xen_feature_get(XENFEAT_auto_translated_physmap
> >                                  dom->parms.f_supported)   ||
> >              ...
> >         {
> >             xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: Kernel
> > does not support PVH"......
> >             rc = -EINVAL;
> >             goto out;
> >         }
> >     }
> 
> This is the sort of thing I was imagining, it's obviously going to
> need to be more complex if you want to support optional PVH though.

Ok. With limited time I've to work on PVH right now, I'll hack up a 
patch with binary pvh and post it.

thanks
mukesh

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