Re: [Xen-devel] [PATCH 14/18] PVH xen: Checks, asserts, and limitations for PVH

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -578,6 +578,9 @@ int32_t hvm_set_mem_pinned_cacheattr(
>  {
>      struct hvm_mem_pinned_cacheattr_range *range;
>  
> +    /* A PVH guest writes to MSR_IA32_CR_PAT natively. */
> +    ASSERT(!is_pvh_domain(d));

This can't be an assert, or did I overlook you preventing the
function to be called for PVH guests.

The comment would then be wrong too, as there is a path
leading here from a domctl (i.e. unaffected by how the guest
itself would access the MSR).

> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -475,6 +475,13 @@ ret_t do_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      case PHYSDEVOP_set_iopl: {
>          struct physdev_set_iopl set_iopl;
> +
> +        if ( is_pvh_vcpu(current) )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
>          ret = -EFAULT;
>          if ( copy_from_guest(&set_iopl, arg, 1) != 0 )
>              break;
> @@ -488,6 +495,12 @@ ret_t do_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      case PHYSDEVOP_set_iobitmap: {
>          struct physdev_set_iobitmap set_iobitmap;
> +
> +        if ( is_pvh_vcpu(current) )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
>          ret = -EFAULT;
>          if ( copy_from_guest(&set_iobitmap, arg, 1) != 0 )
>              break;

I would really like these two to have better distinguishable error
codes (e.g. -EPERM).

> @@ -3325,6 +3327,9 @@ void do_device_not_available(struct cpu_user_regs *regs)
>  
>      BUG_ON(!guest_mode(regs));
>  
> +    /* PVH should not get here. (ctrlreg is not implemented). */
> +    ASSERT(!is_pvh_vcpu(curr));

I think this is right, but the comment is confusing/misleading.

> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -440,6 +440,8 @@ static long register_guest_callback(struct 
> callback_register *reg)
>      long ret = 0;
>      struct vcpu *v = current;
>  
> +    ASSERT(!is_pvh_vcpu(v));
> +

For one, I don't think there has been anything so far making
clear that this is unreachable for PVH.

And then it is inconsistent to do this here, but not also in
unregister_guest_callback().

Jan


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