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

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

On Mon, 30 Sep 2013 07:56:30 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 28.09.13 at 01:03, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Fri, 27 Sep 2013 08:01:16 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
.......
> >> >> > @@ -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.

I beg to differ.... such nit picking is equally bogus IMHO. The
bit needs to be set once, putting it in vmx_vmenter_helper adds an
unnecessary slowdown IMO. 

> > 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 debugger would always read eflags, muck with only
the bits it needs to, leaving the resvd bit as is, then send it down.

> 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.

Having X86_EFLAGS_MBS makes sense.

> > 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.


Mukesh