Re: [RFC 0 PATCH 3/3] PVH dom0: construct_dom0 changes

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Thu, 26 Sep 2013 09:02:41 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 25.09.13 at 23:03, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > @@ -307,6 +308,136 @@ static void __init
.....
> 

> 
> > +    sav_guest_l4 = *l4tab;
> > +
> > +    /* Give guest a clean slate to start with */
> > +    clear_page(l4start);
> > +    *l4tab = sav_guest_l4;
> > +    BUG_ON(!l4e_get_pfn(sav_guest_l4));
> 
> This assumes that the initial mapping is covered by a single L4
> entry. While true for Linux, this is not generic, and hence needs
> fixing (the problem continues further down).

True. How about I just create the opposite of init_guest_l4_table,
ie, clear_guest_l4_table and just clear out xen entries?

> 
> > +    {
> > +        if ( l3e_get_pfn(*l3tab) == 0 )
> 
> Is that really a good check? Shouldn't that rather look at
> _PAGE_PRESENT?

Ok, I will change that.

> Hardly worth the comment considering the variable name.
> 
> > @@ -868,6 +1016,9 @@ int __init construct_dom0(
> >                                      L1_PROT : COMPAT_L1_PROT));
> >          l1tab++;
> >  
> > +        if ( is_pvh_domain(d) )
> > +            continue;
> > +
> >          page = mfn_to_page(mfn);
> >          if ( (page->u.inuse.type_info == 0) &&
> >               !get_page_and_type(page, d, PGT_writable_page) )
> 
> So why is the remaining part of this loop not applicable to PVH?

My bad, looks like it should be. I'll remove it. BTW, looking at it again
I realized we don't really need to set type_info to PGT* for PVH, but it's 
harmless I guess. Should I just leave it or condition them for PV only?

> >          (void)alloc_vcpu(d, i, cpu);
> >      }
> >  
> > +    /*
> > +     * pvh: we temporarily disable paging mode so that we can
> > build cr3 needed
> > +     * to run on dom0's page tables.
> > +     */
> > +    if ( is_pvh_domain(d) )
> > +    {
> > +        save_pvh_pg_mode = d->arch.paging.mode;
> > +        d->arch.paging.mode = 0;
> > +    }
> 
> Does this really need to be in a conditional?

Not really, wasn't sure what you'd prefer. I'll remove conditions.

> > @@ -1089,11 +1262,18 @@ int __init construct_dom0(
> >      regs->eip = parms.virt_entry;
> >      regs->esp = vstack_end;
> >      regs->esi = vstartinfo_start;
> > -    regs->eflags = X86_EFLAGS_IF;
> > +    regs->eflags = X86_EFLAGS_IF | 0x2;
> 
> Unrelated change?

Nop, we need to make sure the resvd bit is set in eflags otherwise
it won't vmenter (invalid guest state). Should be harmless for PV,
right? Not sure where it does it for PV before actually scheduling it..

thanks
Mukesh