Re: [Xen-devel] [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.08.14 at 15:12, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>                      rc = -EINVAL;
>                  break;
>              case HVM_PARAM_VIRIDIAN:
> -                if ( a.value > 1 )
> -                    rc = -EINVAL;
> +                /* This should only ever be set once by the tools and read 
> by the guest. */
> +                rc = -EPERM;
> +                if ( curr_d == d )
> +                    break;
> +
> +                rc = -EPERM;
> +                if ( d->arch.hvm_domain.params[a.index] &&
> +                     a.value != d->arch.hvm_domain.params[a.index] )

I was actually expecting this to succeed if the value is the same as the
currently stored one, rather than fail when they're different. The
subtle difference is that this would allow the tools to set the value to
zero when it currently is zero, which the check below would otherwise
refuse afaict.

Apart from that, if the code was left as is the second of the -EPERM
assignments would be redundant.

> +                    break;
> +                

There is still a line with only blanks here.

> +                rc = -EINVAL;
> +                if ( a.value & ~HVMPV_feature_mask ||

We commonly parenthesize such expressions when combined with
&& or ||.

Jan


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