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

[Julien Grall]

On 08/24/2012 03:09 PM, Ian Campbell wrote:
> 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.
>    
When you told feature, it's only for libxl or even for Xen ?
In case of libxl, if 'device_models' options is not specified
we used only one QEMU. So there is compatibility with
previous configuration file.
In case of Xen, it's hard to have a compatibility. We can
still spawn only one QEMU, but ioreq handling will not
send an io request if no device models registered it.
There is no more default QEMU.

> > > > +     * 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?
>
>    
Will be fix in the next patch version.
> 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?
>    
I don't understand your question. Theses 3 priors BDFs can't
be modify via QEMU command line (or I don't know how).
> > > > @@ -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?
>    
That means:
    - first QEMU can allocate ROM after ram_size + 0
    - second after ram_size + 32 mo
    - ...

It's a hack to avoid modification in QEMU memory allocator
(find_ram_offset exec.c in QEMU).

> 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!
>
>    
It's not possible to made it in QEMU, otherwise QEMU need to
be spawn one by one. Indeed, the next QEMU need to know
what is the last 'address' used by the previous QEMU.

I made a modification in this way, but it was abandoned. Indeed,
it required XenStore.

> 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?
>    
It's the rom allocated via pci_add_option_rom in QEMU.
QEMU seems to store ROM in memory and then SeaBIOS
will copy it, in the right place.

--
Julien