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
|
Home