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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, 8 Apr 2011, Jan Kiszka wrote:

> [ Late comments, I know, sorry. Just happen to came across this. ]
>
> On 2011-03-29 20:27, anthony.perard@xxxxxxxxxx wrote:
> > From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> >  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.

I will do that.

> >  }
> >
> >  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?

I thought in a first place that was needed to avoid errors. But it works
also when we initialise other CPUs. But I prefere to keep it that way to
save memory and in the case where there is a thread for each cpu that
will also avoid to have many useless threads.

Also, I use -smp i to initialise the xen's structures related to the
vcpu.

> >      }
> >  }
> > @@ -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?

I'll do that, put the calculation in the caller, and change the
pc_memory_init prototypes.


Thanks for your review,
Regards,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel