From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corneliu ZUZU Subject: Re: [PATCH 3/7] xen/vm-events: Move monitor_domctl to common-side. Date: Mon, 8 Feb 2016 20:43:02 +0200 Message-ID: <56B8E1B6.8060507@bitdefender.com> References: <1454950682-9459-1-git-send-email-czuzu@bitdefender.com> <1454950682-9459-4-git-send-email-czuzu@bitdefender.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4906044950345792348==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: Kevin Tian , Keir Fraser , Ian Campbell , Razvan Cojocaru , Jun Nakajima , Andrew Cooper , Xen-devel , Stefano Stabellini , Jan Beulich List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============4906044950345792348== Content-Type: multipart/alternative; boundary="------------080404030003020701090803" This is a multi-part message in MIME format. --------------080404030003020701090803 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 2/8/2016 8:15 PM, Tamas K Lengyel wrote: > On Mon, Feb 8, 2016 at 9:57 AM, Corneliu ZUZU > wrote: > > +#if CONFIG_X86 > > > Wouldn't it be safe to include these headers on ARM as well? > > +#include /* for VM_EVENT_X86_CR3 */ > +#include /* for hvm_update_guest_cr, > ... */ > +#endif > It wouldn't break anything, but they are only necessary on X86, so why include them? > + > + if ( unlikely(mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE && > + mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE) ) > + { > > > Please make a switch on mop->op instead where the default case is to > call arch_monitor_domctl. It would be a bit easier to follow. > > + if ( XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES == mop->op ) > + { > + mop->event = arch_monitor_get_capabilities(d); > + return 0; > + } > + > Noted, good point. > > "proly"->probably? > > + /* The monitor op is proly handled on the arch-side. */ > + if ( likely(arch_monitor_domctl_op(d, mop, &rc)) ) > + return rc; > Yep, will "proly" change that, heh, just kidding. Noted :D. > Empty line not needed. (*) Noted. > > So I don't particularly like this #if check here. IMHO this should be > done in arch-specific function that you call from here that is just > empty for ARM. It could be a static inline function as it's rather small. > > +#if CONFIG_X86 > > + if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index ) > + { > + struct vcpu *v; > + /* Latches new CR3 mask through CR0 code. */ > + for_each_vcpu ( d, v ) > + hvm_update_guest_cr(v, 0); > + } > +#endif > I agree, but I was actually hoping to approach that in a future ARM-patch I'm going to send after this one. On ARM, that part of code (where hypervisor CR trapping is enabled based on write_ctrlreg_enabled bits) is done in another place (on the scheduling tail). I wanted to approach removing that #ifdef and finding maybe a solution to do that similarly for X86. Would it be ok to leave this for that discussion? It would be simpler to illustrate what I have in mind. > > Looks good otherwise. > > Thanks, > Tamas Thanks, Corneliu. --------------080404030003020701090803 Content-Type: text/html; charset=utf-8 Content-Length: 7745 Content-Transfer-Encoding: quoted-printable
On 2/8/2016 8:15 PM, Tamas K Lengyel wrote:
On Mon, Feb 8, 2016 at 9:57 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
+#if CONFIG_X86

Wouldn't it be safe to include these headers on ARM as well=3F
=C2=A0
+#include <public/vm_event.h>=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* for VM_EVENT_X86_CR3 */
+#include <asm/hvm/hvm.h>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* for hvm_update_guest_cr, ... */
+#endif

It wouldn't break anything, but they are only necessary on X86, so why include them=3F

+
+=C2=A0 =C2=A0 if ( unlikely(mop->op !=3D XEN_DOMCTL_MONITOR_OP_ENABLE &&
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mop->op !=3D XEN_DOMCTL_MONITOR_OP_DISABLE) )
+=C2=A0 =C2=A0 {

Please make a switch on mop->op instead where the default case is to call arch_monitor_domctl. It would be a bit easier to follow.

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES =3D=3D mop->op )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mop->event =3D arch_monitor_get_capabilities(d);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 0;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
+

Noted, good point.


"proly"->probably=3F
=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* The monitor op is proly handled on the arch-side. */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( likely(arch_monitor_domctl_op(d, mop, &rc)) )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return rc;

Yep, will "proly" change that, heh, just kidding. Noted :D.

=C2=A0
Empty line not needed.=C2=A0 (*)

Noted.


So I don't particularly like this #if check here. IMHO this should be done in arch-specific function that you call from here that is just empty for ARM. It could be a static inline function as it's rather small.
=C2=A0
+#if CONFIG_X86
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( VM_EVENT_X86_CR3 =3D=3D mop->u.mov_to_cr.index )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct vcpu *v;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Latches new CR3 mask through CR0 code. */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for_each_vcpu ( d, v )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hvm_update_guest_cr(v, 0);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
+#endif

I agree, but I was actually hoping to approach that in a future ARM-patch I'm going to send after this one.
On ARM, that part of code (where hypervisor CR trapping is enabled based on write_ctrlreg_enabled bits)
is done in another place (on the scheduling tail). I wanted to approach removing that #ifdef and finding maybe
a solution to do that similarly for X86. Would it be ok to leave this for that discussion=3F It would be simpler to illustrate
what I have in mind.


Looks good otherwise.

Thanks,
Tamas

Thanks,
Corneliu.
--------------080404030003020701090803-- --===============4906044950345792348== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4906044950345792348==--