Re: [Qemu-devel] [Xen-devel] [XEN][RFC PATCH V2 15/17] xl: support spawn/destroy on multiple device model

[Ian Campbell]

On Fri, 2012-08-24 at 14:51 +0100, Julien Grall wrote:
> >> @@ -1044,7 +1044,8 @@ int libxl__wait_for_device_model(libxl__gc *gc,
> >>                                    void *check_callback_userdata)
> >>   {
> >>       char *path;
> >> -    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", 
> >> domid);
> >> +    path = libxl__sprintf(gc, "/local/domain/0/dms/%u/%u/state",
> >> +                          domid, dmid);
> >>      
> > Isn't this control path shared with qemu? I'm not sure we can just
> > change it like that? We need to at least retain compatibility with
> > pre-disag qemus.
> >
> >    
> Indeed, as we have multiple QEMUs for a same domain, we need
> to have one control path by QEMU.
> 
> Pre-disag QEMUs cannot work with my changes inside the Xen.
> Xen will not forward by default ioreq if there is no ioreq server.

We might need to consider making disagg an opt in feature, with the
default being to have as we do today.

> >> +     * PCI device number. Before 3, we have IDE, ISA, SouthBridge and
> >> +     * XEN PCI. Theses devices will be emulate in each QEMU, but only
> >> +     * one QEMU (the one which emulates default device) will register
> >> +     * these devices through Xen PCI hypercall.
> >> +     */
> >> +    static unsigned int bdf = 3;
> >>      
> > Do you mean const rather than static?
> >
> >    
> No static. With QEMU disaggregation, the toolstack allocate
> BDF incrementaly. QEMU is unable to know if a BDF is already
> allocated in another QEMU.

This is broken if the toolstack is building multiple domains, since the
bdf will be preserved across each of them.

You need to put this in some sort of data structure specific to this
particular iteration of the builder code. We must surely have something
suitable close to hand in this function. libxl__domain_build_state
perhaps?

A static variable in a library is almost always a mistake.

> > Isn't this baking in some implementation detail from the current qemu
> > version? What happens if it changes?
> >    
> 
> I don't have another way for the moment. I would be happy,
> if someone have a good solution.

Could we at least make the assignments of the 3 prior BDFs explicit on
the command line too?

> >>       dm_args = flexarray_make(16, 1);
> >>       if (!dm_args)
> >> @@ -348,11 +389,12 @@ static char ** 
> >> libxl__build_device_model_args_new(libxl__gc *gc,
> >>                         "-xen-domid",
> >>                         libxl__sprintf(gc, "%d", guest_domid), NULL);
> >>
> >> +    flexarray_append(dm_args, "-nodefaults");
> >>      
> > Does this not cause a change in behaviour other than what you've
> > accounted for here?
> >    
>   By default QEMU emulates VGA card, and a network card. This options,
> disabled it  and avoid to add "-net none".
> I added it after a discussion on my first patch series.
> https://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04767.html

OK. 

> >> @@ -528,65 +583,69 @@ static char ** 
> >> libxl__build_device_model_args_new(libxl__gc *gc,
> >>           abort();
> >>       }
> >>
> >> -    ram_size = libxl__sizekb_to_mb(b_info->max_memkb - 
> >> b_info->video_memkb);
> >> +    // Allocate ram space of 32Mo per previous device model to store rom
> >>      
> > What is this about?
> >
> > (also that Mo looks a bit odd in among all these mb's)
> >
> >    
> It's space for ROM allocation, like vga, rtl8139 roms ...
> Each QEMU can load ROM and memory, but the memory
> allocator consider that it's alone. It starts to allocate
> ROM space from the end of memory RAM.
> 
> It's a solution suggest by Stefano, it's avoid modification
> in QEMU. As we don't know the number of ROM and their
> size per QEMU, we chose a space of 32 Mo to be sure, but in
> fine most of time memory is not allocated.

"32Mo per previous device model" is the bit which struck me as odd. That
means the first device model uses 32Mo, the second 64Mo, the third 96Mo
etc?

Aren't we already modifying qemu quite substantially to implement this
functionality anyway? so why are we trying to avoid it in this one
corner? Especially at the cost of doing something which on the face of
it looks quite strange!

Isn't space for the ROMs allocated by SeaBIOS as part of enumerating the
PCI bus anyway? Or is this a different per-ROM allocation?

Ian.