Re: [PATCH 05/16] x86/monitor: relocate code more appropriately

From: Corneliu ZUZU <czuzu@bitdefender.com>
To: "Tian, Kevin" <kevin.tian@intel.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	"Nakajima, Jun" <jun.nakajima@intel.com>
Subject: Re: [PATCH 05/16] x86/monitor: relocate code more appropriately
Date: Fri, 15 Jul 2016 14:41:40 +0300	[thread overview]
Message-ID: <e3df1498-bd88-d446-7887-8b4d2bc1f7cf@bitdefender.com> (raw)
In-Reply-To: <00126a1b-4664-80fe-7c7e-58a539e10071@bitdefender.com>

On 7/12/2016 11:07 AM, Corneliu ZUZU wrote:
> On 7/12/2016 10:45 AM, Tian, Kevin wrote:
>>> From: Corneliu ZUZU [mailto:czuzu@bitdefender.com]
>>> Sent: Monday, July 11, 2016 2:19 PM
>>>> +static inline
>>>> +void monitor_ctrlreg_adjust_traps(struct domain *d, unsigned int 
>>>> index)
>>>> +{
>>>> +    /* For now, VMX only. */
>>>> +    ASSERT(cpu_has_vmx);
>>>> +
>>>> +    /* Other CRs than CR3 are always trapped. */
>>>> +    if ( VM_EVENT_X86_CR3 == index )
>>>> +        vmx_vm_event_update_cr3_traps(d);
>>>       [Kevin wrote]:
>>>
>>>     Please add above into a hvm function instead of directly calling
>>>     vmx in common file. (other arch can have it unimplemented).
>>>     Then you don't need change this common code again later when
>>>     other archs are added
>>>
>>> ---
>>>
>>>
>>> This is not common code, it's in arch/x86/monitor.c (?) and currently,
>>> as the above ASSERT indicates, only VMX is supported. If in the future
>>> support for SVM for example will be added, then the hvm move you 
>>> suggest
>>> must be done (Jan also suggested this).
>>> Or, I only now realize, if you guys prefer doing this now I could add a
>>> vm_event_update_cr3_traps field in hvm_function_table, but BUG() in the
>>> SVM one..
>>>
>> The latter is desired. Instead of BUG, it makes more sense to return
>> error on an arch which doesn't register the callback.
>>
>> Thanks
>> Kevin
>
> Will do.
>
> Thanks,
> Zuzu C.

I've decided to discard separating CR3 load-exiting handling (i.e. 
discard vmx_vm_event_update_cr3_traps) entirely since I find that it's 
complicated to have to handle the bit from 2 different places 
(vmx_update_guest_cr and arch_monitor_domctl_event).

Normally such a situation is resolved by counting the number of 
subscribers to the resource (in this case counting the number of 
'entities' that want to CR3 load-exiting enabled - i.e. just as we have 
a vCPU pause_count to count entities that want the vCPU to be paused), 
but it's just a single bit of a lot more and I don't think the overhead 
is worth.

Let me know if you disagree and I'm open to suggestions, if you guys 
have any.

Thanks,
Corneliu.

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