Re: [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

[Corneliu ZUZU <czuzu@bitdefender.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3581 bytes --]

On 3/7/2016 2:07 PM, Corneliu ZUZU wrote:
> On 3/7/2016 11:45 AM, Tamas K Lengyel wrote:
>>
>>
>> On Mon, Mar 7, 2016 at 10:31 AM, Corneliu ZUZU <czuzu@bitdefender.com 
>> <mailto:czuzu@bitdefender.com>> wrote:
>>
>>     On 3/7/2016 11:12 AM, Tamas K Lengyel wrote:
>>>     EPT is not really required for CR3 monitoring, it just has been
>>>     the case that vm_events have been only implemented for
>>>     hap-enabled domains.
>>
>>     I suppose this is not valid for vm-events in their entirety,
>>     right? I mean it seems to me that @ least for monitor vm-events
>>     VMX is enough.
>>
>>
>> Yes. OTOH I don't think you can find any CPUs on the market today 
>> that support VMX but have no EPT so this hasn't really caused any 
>> issues for anyone using vm_events, but technically yes VMX is enough 
>> for these events.
>>
>>>     AFAIK for non-hap case CR3 needs to be trapped unconditionally, yes.
>>>
>>>         If the former is true, shouldn't we do a check like this in
>>>         vm_event_monitor_get_capabilities instead?
>>>
>>>
>>>     Yes, it should now, this code was just written before
>>>     vm_event_monitor_get_capabilities was introduced and we haven't
>>>     gotten around converting this check to it.
>>
>>     Is there any reason why monitor vm-events in their current state
>>     wouldn't work on non-hap domains?
>>     If they would work, shouldn't we instead simply move the
>>     monitor.write_ctrlreg_enabled part out of the if (
>>     paging_mode_hap(...) ) ?
>>
>>
>> Yeap, that sounds like the right place to have that check.
>>
>> Tamas
>
> Good, with that out of the way, one more issue to solve. What I'm 
> actually trying to do is to move that part of the code to the 
> scheduling tail - i.e. enabling/disabling CPU_BASED_CR3_LOAD_EXITING 
> only when we actually enter the vcpu.
> To do this I also need to know exactly in what cases 
> CPU_BASED_CR3_LOAD_EXITING can/is enabled, besides the already 
> mentioned case when a domain's paging is disabled.
>
> I'm searching through the codebase right now but it's a bit dizzying, 
> can someone provide some feedback on this matter?
>
> Thanks,
> Corneliu.

Ok, by searching for places v->arch.hvm_vmx.exec_control is set, it 
seems that vmx_update_guest_cr is the only place where CR3 load-exiting 
is set/unset.

It also seems that in non-hap case CPU_BASED_CR3_LOAD_EXITING is indeed 
unconditionally enabled, i.e. @ vmx_vcpu_initialise -> vmx_create_vmcs 
-> construct_vmcs:

     v->arch.hvm_vmx.secondary_exec_control = vmx_secondary_exec_control;

     /* Disable VPID for now: we decide when to enable it on VMENTER. */
     v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VPID;

     if ( paging_mode_hap(d) )
     {
         v->arch.hvm_vmx.exec_control &= ~(CPU_BASED_INVLPG_EXITING |
                                           CPU_BASED_CR3_LOAD_EXITING |
CPU_BASED_CR3_STORE_EXITING);
     }

Can somebody else confirm this, just to be sure?

Tamas, if this were true, that would mean that we can move that part to 
the scheduling tail, and we could write there smth like this pseudocode:

     /* if ! hap => CR3 writes unconditionally trap */
     if (paging_mode_hap) return;
     if  (monitor.write_ctrlreg_enabled for CR3) and 
(CPU_BASED_CR3_LOAD_EXITING currently disabled)
         enable CPU_BASED_CR3_LOAD_EXITING;
    else if (NOT monitor.write_ctrlreg_enabled for CR3) and 
(CPU_BASED_CR3_LOAD_EXITING currently enabled) and (paging is enabled)
         disable CPU_BASED_CR3_LOAD_EXITING;

Would that be suitable?

Thanks,
Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 7410 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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