Re: [PATCH 11/20] PVH xen: introduce pvh.c

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.05.13 at 03:42, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1129,6 +1129,10 @@ arch_do_vcpu_op(
>          struct domain *d = v->domain;
>          struct vcpu_register_vcpu_info info;
>  
> +        rc = -ENOSYS;
> +        if ( is_pvh_vcpu(current) )
> +            break;
> +

Assuming this is meant to be temporary - yes, this _might_ be
acceptable (if accompanied by a proper comment). But then again
registering vCPU info is a pretty basic interface (which recently
got even moved into common code iirc), so I'm having a hard time
seeing why you need to suppress it rather than make it work from
the beginning.

>          rc = -EFAULT;
>          if ( copy_from_guest(&info, arg, 1) )
>              break;
> @@ -1169,6 +1173,10 @@ arch_do_vcpu_op(
>      {
>          struct vcpu_get_physid cpu_id;
>  
> +        rc = -ENOSYS;
> +        if ( is_pvh_vcpu(current) )
> +            break;
> +

Similarly here - what's wrong with this for PVH?

>          rc = -EINVAL;
>          if ( !is_pinned_vcpu(v) )
>              break;
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -968,6 +968,12 @@ long do_vcpu_op(int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>      }
>  
>      case VCPUOP_down:
> +        if ( is_pvh_vcpu(current) )
> +        {
> +            rc = -ENOSYS;
> +            break;
> +        }

I can see that this may indeed require some special cases to be
taken care of. But adding a comment is then the minimum
requirement. And the increasing amount of "PVH fixme"s is
worrying in their own right - once again, I went through a similar
exercise with 32-on-64 support, and didn't have a need to post
patches for public review (with the underlying implication of them
being in a mergeable state) with this many issues left open.

> +
>          if ( !test_and_set_bit(_VPF_down, &v->pause_flags) )
>              vcpu_sleep_nosync(v);
>          break;
> @@ -1039,6 +1045,11 @@ long do_vcpu_op(int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>  #ifdef VCPU_TRAP_NMI
>      case VCPUOP_send_nmi:
> +        if ( is_pvh_vcpu(current) )
> +        {
> +            rc = -ENOSYS;
> +            break;
> +        }

This one may even have to remain, assuming PVH would send NMIs
via LAPIC ICR? But then using !is_pv_domain() would seem the right
thing here. Otoh, allowing as many PV operations as possible along
with the respective HVM counterparts may be desirable for easing
the kernel side transition?

Jan

>          if ( !guest_handle_is_null(arg) )
>              return -EINVAL;
>