Re: [Xen-devel] [Qemu-devel] [PATCH RFC] acpi: don't build acpi tables for xen hvm guests

[Igor Mammedov <imammedo@xxxxxxxxxx>]

On Thu, 27 Oct 2016 12:29:58 +0100
Wei Liu <wei.liu2@xxxxxxxxxx> wrote:

> On Thu, Oct 27, 2016 at 01:26:49PM +0200, Igor Mammedov wrote:
> > On Thu, 27 Oct 2016 12:10:58 +0100
> > Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> >   
> > > On Thu, Oct 27, 2016 at 12:03:42PM +0100, Wei Liu wrote:  
> > > > Cc Sander
> > > > 
> > > > On Thu, Oct 27, 2016 at 12:54:02PM +0200, Igor Mammedov wrote:    
> > > > > On Wed, 26 Oct 2016 16:22:34 +0100
> > > > > Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> > > > >     
> > > > > > On Wed, Oct 26, 2016 at 05:09:52PM +0200, Igor Mammedov wrote:    
> > > > > > > On Tue, 25 Oct 2016 18:28:04 +0100
> > > > > > > Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> > > > > > >       
> > > > > > > > Xen's toolstack is in charge of building ACPI tables. Skip acpi 
> > > > > > > > table
> > > > > > > > building if running on Xen.
> > > > > > > > 
> > > > > > > > This issue is discovered due to direct kernel boot on Xen 
> > > > > > > > doesn't boot
> > > > > > > > anymore, because the new ACPI tables cause the guest to exceed 
> > > > > > > > its
> > > > > > > > memory allocation limit.
> > > > > > > > 
> > > > > > > > Reported-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>
> > > > > > > > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>      
> > > > > > > Question is:
> > > > > > > Why does xen guest get ACPI tables from QEMU instead of using
> > > > > > > Xen provided ones.
> > > > > > > Maybe it's firmware issue i.e. firmware side shouldn't load
> > > > > > > ACPI tables from QEMU provided fwcfg file and load Xen provided 
> > > > > > > instead.
> > > > > > >       
> > > > > > 
> > > > > > It hasn't come to the point that the guest is booted. QEMU exits 
> > > > > > when
> > > > > > trying to populate some pages for the guest, at which point the 
> > > > > > guest
> > > > > > has not yet been started.  In a sense, Xen guest doesn't get ACPI 
> > > > > > from
> > > > > > QEMU because it never gets to that point.
> > > > > > 
> > > > > > Direct kernel boot causes fw_cfg to be filled in. 
> > > > > > pcms->has_acpi_build
> > > > > > defaults to true and acpi_enabled is also true. These make all 
> > > > > > checks in
> > > > > > acpi_setup pass. QEMU proceeds to build and load ACPI tables (which 
> > > > > > are
> > > > > > never going to be used by Xen guests), causing the guest to exceeds 
> > > > > > its
> > > > > > limit.
> > > > > > 
> > > > > > Wei.    
> > > > > Would something like this fix issue for you?
> > > > > 
> > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > > index a54a468..61b6026 100644
> > > > > --- a/hw/i386/pc_piix.c
> > > > > +++ b/hw/i386/pc_piix.c
> > > > > @@ -1094,10 +1094,13 @@ DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
> > > > >  #ifdef CONFIG_XEN
> > > > >  static void xenfv_machine_options(MachineClass *m)
> > > > >  {
> > > > > +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> > > > > +
> > > > >      m->desc = "Xen Fully-virtualized PC";
> > > > >      m->max_cpus = HVM_MAX_VCPUS;
> > > > >      m->default_machine_opts = "accel=xen";
> > > > >      m->hot_add_cpu = pc_hot_add_cpu;
> > > > > +    pcmc->has_acpi_build = false;
> > > > >  }
> > > > >  
> > > > >  DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,
> > > > >     
> > > > 
> > > > Yes, it does.
> > > > 
> > > > (I have a similar patch in my queue already)
> > > >     
> > > 
> > > Oh, the reason I didn't send it out is because Eduardo suggested we
> > > should use a new field instead of setting has_acpi_build outside of
> > > PCMachineClass init function.
> > > 
> > > I have actually gotten around to investigate this option and what is
> > > needed to be done.  
> > static void xenfv_machine_options(MachineClass *m)                          
> >      
> > {                                                                           
> >      
> > ...                                              
> > }                                                                           
> >      
> >                                                                             
> >      
> > DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,                          
> >      
> >                   xenfv_machine_options);      
> > 
> > #define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \                
> >      
> >     static void pc_machine_##suffix##_class_init(ObjectClass *oc, void 
> > *data) \  
> >     { \                                                                     
> >      
> >         MachineClass *mc = MACHINE_CLASS(oc); \                             
> >      
> >         optsfn(mc); \                                                       
> >      
> >         mc->init = initfn; \                                                
> >      
> >     } \  
> > ...
> > 
> > So xenfv_machine_options() is a part of pc_machine_xenfv_class_init()  
> 
> Ah, so your (and mine) patch already fits the bill. Thanks for looking
> into this.
> 
> Are you going to submit a proper patch or do you want me to?
Please submit your version of patch.

> 
> Wei.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel