Re: [Xen-devel] [PATCH 12/17] x86: connect guest creation with CONFIG_PV

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Oct 12, 2018 at 09:09:28AM -0600, Jan Beulich wrote:
> >>> On 04.10.18 at 17:43, <wei.liu2@xxxxxxxxxx> wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -322,17 +322,34 @@ struct domain *domain_create(domid_t domid,
> >      }
> >  
> >      /* Sort out our idea of is_{pv,hvm}_domain(). */
> > -    if ( config && (config->flags & XEN_DOMCTL_CDF_hvm_guest) )
> > +    if ( config )
> >      {
> > +        ASSERT(!is_system_domain(d));
> 
> This and the other ASSERT() are redundant with what's earlier in
> the function. Do we really need them?

No, not really.

> 
> > +        if ( config->flags & XEN_DOMCTL_CDF_hvm_guest)
> > +        {
> >  #ifdef CONFIG_HVM
> > -        d->guest_type = guest_type_hvm;
> > +            d->guest_type = guest_type_hvm;
> > +#else
> > +            err = -EINVAL;
> > +            goto fail;
> > +#endif
> > +        }
> > +        else
> > +        {
> > +#ifdef CONFIG_PV
> > +        d->guest_type = guest_type_pv;
> >  #else
> >          err = -EINVAL;
> >          goto fail;
> >  #endif
> > +        }
> >      }
> >      else
> > +    {
> > +        /* The type of system domain shouldn't matter. */
> > +        ASSERT(is_system_domain(d));
> >          d->guest_type = guest_type_pv;
> > +    }
> 
> I'm afraid this comment may cause ambiguity. I think we had (and
> perhaps still have) a number of places where we assume that in
> particular the idle domain is a PV one. So I'd like to ask to eithr
> extend the comment to explain reality, or to drop it.

Yes the idle domain uses PV context switch routines, but that's the
extend of its PV-ness, which has nothing to do with guest interfaces.
The system still functions even if I change the type of system domains
to guest_type_foobar AFAICT.

But I think at least the idle domain should be called out explicitly. I
will change the comment to say:

   At least the idle domain should be treated as PV domain because it
   uses PV context switch functions. To err on the safe side, leave all
   system domains to be guest_type_pv.

Wei.

> 
> Jan
> 
> 

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