Re: [PATCH 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU <czuzu@bitdefender.com>]

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

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 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>     +#if CONFIG_X86
>
>
> Wouldn't it be safe to include these headers on ARM as well?
>
>     +#include <public/vm_event.h>        /* for VM_EVENT_X86_CR3 */
>     +#include <asm/hvm/hvm.h>            /* 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.

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

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

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