Re: [Xen-devel] [V10 PATCH 21/23] PVH xen: VMX support of PVH guest creation/destruction

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Sat, Aug 10, 2013 at 2:59 AM, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Fri, 9 Aug 2013 14:44:48 +0100
> George Dunlap <dunlapg@xxxxxxxxx> wrote:
>
>> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
>> <mukesh.rathor@xxxxxxxxxx> wrote:
>> > This patch implements the vmx portion of the guest create, ie
>> > vcpu and domain initialization. Some changes to support the destroy
>> > path.
>> >
>> > Change in V10:
>> >   - Don't call vmx_domain_initialise / vmx_domain_destroy for PVH.
>> >   - Do not set hvm_vcpu.guest_efer here in vmx.c.
>> >
>> > Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> > ---
>> >  xen/arch/x86/hvm/vmx/vmx.c |   28 ++++++++++++++++++++++++++++
>> >  1 files changed, 28 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> > index 80109c1..f6ea39a 100644
>> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> > @@ -1076,6 +1076,28 @@ static void vmx_update_host_cr3(struct vcpu
>> > *v) vmx_vmcs_exit(v);
>> >  }
>> >
>> > +/*
>> > + * PVH guest never causes CR3 write vmexit. This is called during
>> > the guest
>> > + * setup.
>> > + */
>> > +static void vmx_update_pvh_cr(struct vcpu *v, unsigned int cr)
>> > +{
>> > +    vmx_vmcs_enter(v);
>> > +    switch ( cr )
>> > +    {
>> > +    case 3:
>> > +        __vmwrite(GUEST_CR3, v->arch.hvm_vcpu.guest_cr[3]);
>> > +        hvm_asid_flush_vcpu(v);
>> > +        break;
>> > +
>> > +    default:
>> > +        printk(XENLOG_ERR
>> > +               "PVH: d%d v%d unexpected cr%d update at rip:%lx\n",
>> > +               v->domain->domain_id, v->vcpu_id, cr,
>> > __vmread(GUEST_RIP));
>> > +    }
>> > +    vmx_vmcs_exit(v);
>> > +}
>>
>> This function seems almost completely pointless.  In the case of CR3,
>> it basically does exactly what the function below does.  It avoids
>> maybe doing something pointless, like vmx_load_ptrs(), but that should
>> be harmless, right?
>
> Harmless, nonetheless pointless paying small penalty calling it. Such
> things are subjective and matter of personal opinions, and vary from
> maintainer to maintainer.... I don't care either way. If it helps this
> patch end it's misery, I'll make the change!

No, it's not pointless.  Having extra code means more instruction
cache misses, so it's as likely as not to slow things down rather than
speed things up.  And in any case, making the code easier to read and
maintain is absolutely worth an extra dozen cycles every time a domain
is created.

 -George

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