[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v14 11/17] pvh: Set up more PV stuff in set_info_guest



On 07/11/13 16:10, Jan Beulich wrote:
On 07.11.13 at 16:51, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
On 04/11/13 16:53, Jan Beulich wrote:
On 04.11.13 at 13:15, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
@@ -728,8 +740,21 @@ int arch_set_info_guest(
if ( has_hvm_container_vcpu(v) )
       {
-        hvm_set_info_guest(v);
-        goto out;
+        hvm_set_info_guest(v, compat ? 0 : c.nat->gs_base_kernel);
I'm afraid this isn't correct - so far gs_base_kernel didn't get used
for HVM guests, i.e. you're changing behavior here (even if only
in a - presumably - benign way).

+
+        if ( is_hvm_vcpu(v) || v->is_initialised )
+            goto out;
+
+        cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]);
I'd recommend against using this PV construct - the 32-bit
counterpart won't be correct to be used here once 32-bit
support gets added.
So the plan would be that once we support 32-bit, I'd just copy the code
from below:

      if ( !compat )
          cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]);
      else
          cr3_gfn = compat_cr3_to_pfn(c.cmp->ctrlreg[3]);

But since we know that compat is false here, it seems a bit silly to
have the if() statement.

But there should be a "PVH 32bitfixme" here -- is that enough for now?
Sure, but that wasn't my point. I was recommending against
*_cr3_to_pfn() altogether here. Just use the control register
value shifted right by 12 bits.

Oh, right, I see.


@@ -1426,6 +1426,11 @@ static void vmx_set_info_guest(struct vcpu *v)
           __vmwrite(GUEST_INTERRUPTIBILITY_INFO, intr_shadow);
       }
+ /* PVH 32bitfixme */
+    if ( is_pvh_vcpu(v) )
+        __vmwrite(GUEST_GS_BASE, gs_base_kernel);
Oh, I see, you suppress this here. I'd really suggest adjusting the
caller, then you don't need to do anything here afaict.
What do you mean "adjusting the caller"?  What we want for HVM guests is
for this field to be entirely left alone, isn't it?

If we set GUEST_GS_BASE unconditionally here, the only way to effect "no
change" is to read it and pass in the existing value, which seems kind
of pointless.
Oh, right, I didn't pay attention to the calling path also being
used for HVM, and us not wanting to write zero here in that
case. Or maybe I did, but concluded that the code can be used
only for initial state setup, in which case writing zero would be
benign.

I guess we could do that... but since we're talking about changing the interface here anyway, it's kind of nitpicking at this point.

 -George

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.