Re: [Xen-devel] [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.05.15 at 19:12, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
> that does that.

But you should also say a word on why this is needed, since things
worked fine so far without, and enabling the functions to run
outside of their own vCPU context is not immediately obviously
correct.

> -int hvm_set_cr0(unsigned long value)
> +int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>  {
> -    struct vcpu *v = current;

This change is covered by neither the title nor the description, but
considering it's you who sends this likely is the meat of the change.
However, considering that the three calls you add to
arch_set_info_guest() pass this in as zero, I even more wonder why
what the title says is needed in the first place.

I further wonder whether you wouldn't want an event if and only
if v == current (in which case the flag parameter could be dropped).

> @@ -3328,12 +3330,11 @@ int hvm_set_cr3(unsigned long value)
>      return X86EMUL_UNHANDLEABLE;
>  }
>  
> -int hvm_set_cr4(unsigned long value)
> +int hvm_set_cr4(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>  {
> -    struct vcpu *v = current;
>      unsigned long old_cr;
>  
> -    if ( value & hvm_cr4_guest_reserved_bits(v, 0) )
> +    if ( value & hvm_cr4_guest_reserved_bits(v, with_vm_event ? 0 : 1) )

Why does this depend on with_vm_event? And if indeed correct,
please simplify to just !with_vm_event.

> @@ -3359,7 +3360,8 @@ int hvm_set_cr4(unsigned long value)
>          goto gpf;
>      }
>  
> -    hvm_update_cr(v, 4, value);
> +    if ( with_vm_event )
> +        hvm_update_cr(v, 4, value);

Again, why? Such non-obvious conditionals need comments, or
people wanting to make future changes in these areas won't know
how to correctly change such code.

Jan


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