[Qemu-devel] Re: [PATCH V12 05/17] xen: Add xenfv machine

[Jan Kiszka <jan.kiszka@siemens.com>]

[ Late comments, I know, sorry. Just happen to came across this. ]

On 2011-03-29 20:27, anthony.perard@citrix.com wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> Introduce the Xen FV (Fully Virtualized) machine to Qemu, some more Xen
> specific call will be added in further patches.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hw/pc.c      |   19 +++++++++++++++++--
>  hw/pc_piix.c |   17 +++++++++++++++++
>  hw/xen.h     |    4 ++++
>  3 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 6939c04..d7732d4 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -41,6 +41,7 @@
>  #include "sysemu.h"
>  #include "blockdev.h"
>  #include "ui/qemu-spice.h"
> +#include "xen.h"
>  
>  /* output Bochs bios info messages */
>  //#define DEBUG_BIOS
> @@ -918,7 +919,11 @@ static void pc_cpu_reset(void *opaque)
>      CPUState *env = opaque;
>  
>      cpu_reset(env);
> -    env->halted = !cpu_is_bsp(env);
> +    if (!xen_enabled()) {
> +        env->halted = !cpu_is_bsp(env);
> +    } else {
> +        env->halted = 1;
> +    }

Not a fault of your patch, but pc_cpu_reset should not exist in the
first place. Setting env->halted should be done in i386's cpu_reset.

I think Xen would be better off with installing a custom VCPU reset
handler and overwrite halted according to its own needs. KVM is doing
the same. Then we could clean up pc_cpu_reset without bothering Xen.

>  }
>  
>  static CPUState *pc_new_cpu(const char *cpu_model)
> @@ -952,7 +957,12 @@ void pc_cpus_init(const char *cpu_model)
>  #endif
>      }
>  
> -    for(i = 0; i < smp_cpus; i++) {
> +    if (!xen_enabled()) {
> +        for(i = 0; i < smp_cpus; i++) {
> +            pc_new_cpu(cpu_model);
> +        }
> +    } else {
> +        /* Xen require only one Qemu VCPU */
>          pc_new_cpu(cpu_model);

This looks a bit fishy. What is the semantic of -smp 2 or more in Xen
mode? If that is an invalid/unused configuration option, catch that and
reject it instead of installing this workaround. If it has a valid
semantic, please elaborate why you need to restrict the number of
instantiated cpus. Just to optimize memory usage?

>      }
>  }
> @@ -980,6 +990,11 @@ void pc_memory_init(ram_addr_t ram_size,
>      *above_4g_mem_size_p = above_4g_mem_size;
>      *below_4g_mem_size_p = below_4g_mem_size;
>  
> +    if (xen_enabled()) {
> +        /* Nothing to do for Xen */
> +        return;
> +    }
> +

This looks fragile /wrt potential future changes of pc_memory_init.
Can't those bits Xen is interested in, ie. the above/below_4g_mem_size
calculation, be moved into a separate function or even to the caller
(should be trivial enough, the interface of pc_memory_init is clumsy in
this regard anyway) so that you can simply skip pc_memory_init when in
Xen mode?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux