Re: [Xen-devel] [PATCH v4 13/31] xen/x86: allow disabling the emulated local apic

[Roger Pau Monnà <roger.pau@xxxxxxxxxx>]

El 07/08/15 a les 18.11, Boris Ostrovsky ha escrit:
> On 08/07/2015 11:41 AM, Roger Pau Monnà wrote:
>> El 07/08/15 a les 16.09, Boris Ostrovsky ha escrit:
>>> On 08/07/2015 06:17 AM, Roger Pau Monne wrote:
>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>> index a0a97e7..5acb246 100644
>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>> @@ -1027,6 +1027,20 @@ static int construct_vmcs(struct vcpu *v)
>>>>            ASSERT(!(v->arch.hvm_vmx.exec_control &
>>>> CPU_BASED_RDTSC_EXITING));
>>>>        }
>>>>    +    if ( !has_vlapic(d) )
>>>> +    {
>>>> +        /* Disable virtual apics, TPR */
>>>> +        v->arch.hvm_vmx.secondary_exec_control &=
>>>> +            ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES
>>>> +              | SECONDARY_EXEC_APIC_REGISTER_VIRT
>>>> +              | SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
>>>> +        v->arch.hvm_vmx.exec_control &= ~CPU_BASED_TPR_SHADOW;
>>>> +
>>>> +        /* In turn, disable posted interrupts. */
>>>> +        __vmwrite(PIN_BASED_VM_EXEC_CONTROL,
>>>> +                  vmx_pin_based_exec_control &
>>>> ~PIN_BASED_POSTED_INTERRUPT);
>>>> +    }
>>>> +
>>>>        vmx_update_cpu_exec_control(v);
>>> This is the same code as the one used right above, in 'if (
>>> is_pvh_domain(d) )' clause. Can you combine the two?
>> No, it's not the same code. The PVH code disables unrestricted guest and
>> sets the entry of the VM to be in long mode, which is not true for
>> HVMlite.
> 
> Right, but the first part of that 'if' statement is the same as the one
> you are adding (including the comments). So I was suggesting
> 
>     if ( is_pvh_domain(d) || !has_vlapic(d))
>     {
>         /* Disable virtual apics, TPR */
>         v->arch.hvm_vmx.secondary_exec_control &=
>             ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES
>               | SECONDARY_EXEC_APIC_REGISTER_VIRT
>               | SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
>         v->arch.hvm_vmx.exec_control &= ~CPU_BASED_TPR_SHADOW;
> 
>         /* In turn, disable posted interrupts. */
>         __vmwrite(PIN_BASED_VM_EXEC_CONTROL,
>                   vmx_pin_based_exec_control &
> ~PIN_BASED_POSTED_INTERRUPT);
>    }
> 
>     if ( is_pvh_domain(d) )
>     {
>         /* Unrestricted guest (real mode for EPT) */
>         v->arch.hvm_vmx.secondary_exec_control &=
>             ~SECONDARY_EXEC_UNRESTRICTED_GUEST;
> 
>         /* Start in 64-bit mode. PVH 32bitfixme. */
>         vmentry_ctl |= VM_ENTRY_IA32E_MODE;       /* GUEST_EFER.LME/LMA
> ignored */
> 
>         ASSERT(v->arch.hvm_vmx.exec_control &
> CPU_BASED_ACTIVATE_SECONDARY_CONTROLS);
>         ASSERT(v->arch.hvm_vmx.exec_control &
> CPU_BASED_ACTIVATE_MSR_BITMAP);
>         ASSERT(!(v->arch.hvm_vmx.exec_control & CPU_BASED_RDTSC_EXITING));
>     }

I did it as a separate condition so that when the time comes we can
completely rip the is_pvh_domain case out without having to touch this
code, but I don't have any preference TBH. I can do it that way in a
further revision if that seems preferable.

Roger.

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