Re: [PATCH 2 of 6] early PV on HVM

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 04/22/2010 08:15 AM, Stefano Stabellini wrote:
> From: Sheng Yang <sheng@linux.intel.com>
>
> this patch:
>
> - adds a xen_guest_init hook in setup.c right after kvm_guest_init;
>
> - initializes basic pv on hvm features in xen_guest_init;
>   

Looks mostly OK, except:

> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
> index ab04a1b..7d11957 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -55,6 +55,8 @@
>  #include <xen/events.h>
>  #include <xen/page.h>
>  
> +#include <xen/hvm.h>
> +
>  #include "xenbus_comms.h"
>  #include "xenbus_probe.h"
>  
> @@ -708,10 +710,19 @@ static int __init xenbus_probe_init(void)
>  		xen_store_interface = mfn_to_virt(xen_store_mfn);
>  	} else {
>  		xenstored_ready = 1;
> -		xen_store_evtchn = xen_start_info->store_evtchn;
> -		xen_store_mfn = xen_start_info->store_mfn;
> +		if (xen_hvm_domain()) {
> +			xen_store_evtchn =
> +				hvm_get_parameter(HVM_PARAM_STORE_EVTCHN);
> +			xen_store_mfn =
> +				hvm_get_parameter(HVM_PARAM_STORE_PFN);
> +			xen_store_interface =
> +				ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE);
>   

I think I'd prefer to see these wrap 80 cols than be line-broken like this.

> +		} else {
> +			xen_store_evtchn = xen_start_info->store_evtchn;
> +			xen_store_mfn = xen_start_info->store_mfn;
> +			xen_store_interface = mfn_to_virt(xen_store_mfn);
> +		}
>  	}
> -	xen_store_interface = mfn_to_virt(xen_store_mfn);
>  
>  	/* Initialize the interface to xenstore. */
>  	err = xs_init();
> diff --git a/include/xen/xen.h b/include/xen/xen.h
> index a164024..cb8c48b 100644
> --- a/include/xen/xen.h
> +++ b/include/xen/xen.h
> @@ -9,8 +9,10 @@ enum xen_domain_type {
>  
>  #ifdef CONFIG_XEN
>  extern enum xen_domain_type xen_domain_type;
> +extern void xen_guest_init(void);
>  #else
>  #define xen_domain_type		XEN_NATIVE
> +#define xen_guest_init() do { } while (0)
>  #endif
>   

This isn't an appropriate header for this.  Something under
arch/x86/include/asm would be better.

    J