Re: [Xen-devel] [PATCH RFC v12 10/21] pvh: PVH access to hypercalls

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.09.13 at 18:25, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> Hypercalls where we now have unrestricted access:
> * memory_op
> * console_io
> * vcpu_op
> * mmuext_op
> 
> We also restrict PVH domain access to HVMOP_*_param to writing
> HVM_PARAM_CALLBACK_IRQ.
> 
> Most hvm_op functions require "is_hvm_domain()" and will default to
> -EINVAL; exceptions are HVMOP_get_time and HVMOP_xentrace.
> 
> Finally, we restrict setting IOPL permissions for a PVH domain.
> 
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

again with a couple of comments:

> @@ -3378,7 +3396,9 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>      if ( (eax & 0x80000000) && is_viridian_domain(curr->domain) )
>          return viridian_hypercall(regs);
>  
> -    if ( (eax >= NR_hypercalls) || !hvm_hypercall32_table[eax] )
> +    if ( (eax >= NR_hypercalls) ||
> +         (is_pvh_vcpu(curr) && !pvh_hypercall64_table[eax]) ||
> +         (is_hvm_vcpu(curr) && !hvm_hypercall32_table[eax]) )

This sort of gives the impression that the is_pv_() case isn't handled.
Realizing that PV can't make it here, doing this with the ?: operator
would seem more natural (matching the if/else further down around
doing the actual call).

> @@ -3393,16 +3413,20 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>                      regs->r10, regs->r8, regs->r9);
>  
>          curr->arch.hvm_vcpu.hcall_64bit = 1;
> -        regs->rax = hvm_hypercall64_table[eax](regs->rdi,
> -                                               regs->rsi,
> -                                               regs->rdx,
> -                                               regs->r10,
> -                                               regs->r8,
> -                                               regs->r9); 
> +        if ( is_pvh_vcpu(curr) )
> +            regs->rax = pvh_hypercall64_table[eax](regs->rdi, regs->rsi,
> +                                                   regs->rdx, regs->r10,
> +                                                   regs->r8, regs->r9);
> +        else
> +            regs->rax = hvm_hypercall64_table[eax](regs->rdi, regs->rsi,
> +                                                   regs->rdx, regs->r10,
> +                                                   regs->r8, regs->r9);
>          curr->arch.hvm_vcpu.hcall_64bit = 0;
>      }
>      else
>      {
> +        ASSERT(!is_pvh_vcpu(curr));   /* PVH 32bitfixme. */
> +

And this would then possibly better be

        ASSERT(is_hvm_vcpu(curr));   /* PVH 32bitfixme. */

> @@ -3827,7 +3851,12 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              return -ESRCH;
>  
>          rc = -EINVAL;
> -        if ( !is_hvm_domain(d) )
> +        if ( is_pv_domain(d) )

This would be a case where I would see better fit for
!has_hvm_container...(), if we already need to have that.

> +            goto param_fail;
> +
> +        if ( is_pvh_domain(d)
> +             && ( a.index != HVM_PARAM_CALLBACK_IRQ
> +                  ||  op != HVMOP_set_param ) )

Extra blanks inside the inner parentheses.

> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -519,6 +519,11 @@ ret_t do_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      case PHYSDEVOP_set_iopl: {
>          struct physdev_set_iopl set_iopl;
> +        
> +        ret = -EINVAL;
> +        if ( is_pvh_vcpu(current) )
> +            break;

Now this is a case where -ENOSYS would actually make sense: The
sub-hypercall is to be considered not implemented for PVH guests.

> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -293,6 +293,10 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>                  fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) |
>                               (1U << XENFEAT_highmem_assist) |
>                               (1U << XENFEAT_gnttab_map_avail_bits);
> +            else if ( is_pvh_vcpu(current) )
> +                fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
> +                             (1U << XENFEAT_supervisor_mode_kernel) |
> +                             (1U << XENFEAT_hvm_callback_vector);
>              else
>                  fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
>                               (1U << XENFEAT_hvm_callback_vector) |

This once again would better be a switch statement now that
we're dealing with an enumerated type.

Jan

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