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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.09.13 at 01:03, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Fri, 27 Sep 2013 08:01:16 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 27.09.13 at 03:55, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> wrote:
>> > On Thu, 26 Sep 2013 09:02:41 +0100 "Jan Beulich"
>> > <JBeulich@xxxxxxxx> wrote:
>> >> > @@ -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?
>> 
>> No, I'm pretty certain you want them marked writable. If nothing
>> else then for forward compatibility with an eventual change
>> needing to mark certain pages R/O. But I could also imaging the
>> page sharing code to look at this attribute of a page (but I say
>> this without knowing that code at all).
> 
> Sorry, I meant PGT_l*_page_table settings for type_info. Yes, we do 
> need the PGT_writable_page settings.
> 
>> >> > @@ -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..
>> 
>> PV doesn't set this anywhere - the hardware doesn't allow the
>> flag to be cleared (writes are ignored). If VMENTER is picky
>> about this, the GUEST_RFLAGS write at the end of
>> vmx_vmenter_helper() should be doing this instead of having to
>> do it here (and obviously in some other place for DomU creation).
> 
> For domU we set it in arch_set_info_guest.

Which is bogus too. 15910:ec3b23d8d544 ("hvm: Always keep
canonical copy of RIP/RSP/RFLAGS in guest_cpu_user_regs()") did
this adjustment without really explaining why it can't be done
centrally in just the two places copying regs->eflags into the
VMCS/VMCB spot.

> vmx_vmenter_helper gets
> called on every vmentry, we just need this setting once.

Would a debugger update guest state via arch_set_info_guest()?
I doubt it. It would imo be a desirable up front cleanup patch to
move this bogus thing out of arch_set_info_guest() into
vmx_vmenter_helper() (and whatever SVM equivalent, should
SVM too be incapable of dealing with the flag being clear). See
how e.g. hvm_load_cpu_ctxt() already sets the flag? It's really
like being done almost at random...

The only place where it gets legitimately enforced outside of
the vmx_vmenter_helper() is in the x86 emulator code.

And if we'd have such a cleanup patch, doing away with the literal
2 in favor of a proper symbolic (e.g. X86_EFLAGS_MBS) should
probably be done at once.

> So I think this is the best place. Do you want me to if it:
> 
> regs->eflags = X86_EFLAGS_IF;
> if ( pvh )
>     regs->eflags |= 0x2.

No, that would be pointless.

Jan


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