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

Re: [Xen-devel] [PATCH v15 05/19] pvh prep: Introduce pv guest type and has_hvm_container macros

On 11/12/2013 01:34 PM, Jan Beulich wrote:
On 11.11.13 at 15:57, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -85,7 +85,7 @@ void restore_rest_processor_state(void)

      /* Maybe load the debug registers. */
-    BUG_ON(is_hvm_vcpu(curr));
+    BUG_ON(!is_pv_vcpu(curr));
      if ( !is_idle_vcpu(curr) && curr->arch.debugreg[7] )
          write_debugreg(0, curr->arch.debugreg[0]);

This is certainly fine for now, but I guess in the long run (i.e. when
supporting Dom0) this will need changing from BUG_ON() to
integrating the condition with the if().

Yes -- the BUG_ON() was to make sure someone thinks carefully about what the right thing is to do here. I suppose there should have been a fixme here as well, however.

@@ -1246,8 +1246,7 @@ void arch_get_info_guest(struct vcpu *v, 
vcpu_guest_context_u c)
      bool_t compat = is_pv_32on64_domain(v->domain);
  #define c(fld) (!compat ? (c.nat->fld) : (c.cmp->fld))

-    if ( is_hvm_vcpu(v) )
-        memset(c.nat, 0, sizeof(*c.nat));
+    memset(c.nat, 0, sizeof(*c.nat));

Removing the conditional is both dangerous and unmotivated:
Dangerous because you now assume that the space allocated is
always covering the (larger) native size (this currently happens to
be that way in the only caller, but isn't guaranteed). And
unmotivated because switching just the conditional (as done
throughout the rest of the patch) would have sufficed here.

Indeed, I can't think of why I didn't change it to "!is_pv_vcpu()" here.


Xen-devel mailing list



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