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

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Wed, 15 May 2013 11:42:34 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 15.05.13 at 02:52, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > --- /dev/null
> > +
> > +static int pvh_grant_table_op(unsigned int cmd,
> > XEN_GUEST_HANDLE(void) uop,
> > +                              unsigned int count)
> > +{
> > +#ifndef NDEBUG
> 
> The whole function should be in such a conditional (if it's really
> needed, which I previously said I doubt). Without doing so, and

Temporary code, can we leave it for initial stages while PVH is being
tested and tried. It will help with debug a lot. Alternately, would you be
ok with something like the patch way below. I could do same for grant
and hypercalls too.

> with the way you have pvh_hypercall64_table[], build will fail
> for debug=n.

I know. I left it for you to implment it the way you wanted with memcopy
of the table, which I completely disagree with.

> > +    rc = do_vcpu_op(cmd, vcpuid, arg);
> > +
> > +    /* pvh boot vcpu setting context for bringing up smp vcpu */
> > +    if ( cmd == VCPUOP_initialise )
> > +        vmx_vmcs_enter(current);  
> 
> This is wrong in three ways - for one, you can't call a vmx function
> from here, then the operation also doesn't appear to belong here,

Ooops, sorry! This was in vmx_pvh.c file originally and got moved. Yeah, 
I struggled with this, I can't remember why I needed this. Let me go 
back and investigate this.

> and with pvh_hypercall64_table[] not even existing in non-debug
> builds this won't happen there at all (which is making very clear that
> these function overrides are plain wrong, as I had tried to tell you
> from the beginning).

I don't think they are *wrong*, they work just fine for HVM! They might be
a different approach than you might like, and I appreciate that. But I am 
thinking of being able to easily catch and debug things when the feature 
goes thru infancy.

thanks
Mukesh

--------------------------------------------------------------------------
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 21382eb..b1a33b5 100644
--- 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;
+
         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;
+
         rc = -EINVAL;
         if ( !is_pinned_vcpu(v) )
             break;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index a734755..68de4bb 100644
--- 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;
+        }
+
         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;
+        }
         if ( !guest_handle_is_null(arg) )
             return -EINVAL;