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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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)
>          BUG();
>  
>      /* 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().

> @@ -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.

I'm intending to adjust this once I get to committing the first so
many patches from this series. (And in the unlikely case I might
need to defer committing until another rev of the series, this
means Reviewed-by as long as the above gets fixed.)

Jan


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