[Qemu-devel] [PATCH] pc: disable acpi info for isapc and old pc machine

[Qemu-devel] [PATCH] pc: disable acpi info for isapc and old pc machine

[Michael S. Tsirkin]

Disable acpi build for isapc and no_kvmclock machine
types (used by xen), since acpi build currently expects pci.

Reported-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/pc_piix.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 24a98cb..4fdb7b6 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -309,6 +309,7 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
 static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
+    has_acpi_build = false;
     disable_kvm_pv_eoi();
     enable_compat_apic_id_mode();
     pc_init1(args, 1, 0);
@@ -317,6 +318,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
 static void pc_init_isa(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
+    has_acpi_build = false;
     if (!args->cpu_model) {
         args->cpu_model = "486";
     }
-- 
MST

Re: [Qemu-devel] [PATCH] pc: disable acpi info for isapc and old pc machine

[Paolo Bonzini]

Il 04/11/2013 11:46, Michael S. Tsirkin ha scritto:
> Disable acpi build for isapc and no_kvmclock machine
> types (used by xen), since acpi build currently expects pci.

Xen is now using "-M pc", so it's okay to remove it from the legacy "-M
xenfv".

> Reported-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/pc_piix.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 24a98cb..4fdb7b6 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -309,6 +309,7 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
>  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
> +    has_acpi_build = false;
>      disable_kvm_pv_eoi();
>      enable_compat_apic_id_mode();
>      pc_init1(args, 1, 0);
> @@ -317,6 +318,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>  static void pc_init_isa(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
> +    has_acpi_build = false;
>      if (!args->cpu_model) {
>          args->cpu_model = "486";
>      }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] pc: disable acpi info for isapc and old pc machine

[Paolo Bonzini]

Il 04/11/2013 11:46, Michael S. Tsirkin ha scritto:
> Disable acpi build for isapc and no_kvmclock machine
> types (used by xen), since acpi build currently expects pci.
> 
> Reported-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Wait, Xen is not using pc_init_pci_no_kvmclock.  But that's not a
problem, we can expose ACPI info via fw_cfg and then Xen hvmloader can
decide whether to use it or not.  Right now it doesn't, maybe later on
it should.

If Xen is broken, we need to patch pc_xen_hvm_init, but Andreas only
tested isapc not xenfv.  So unless someone reports otherwise, there is
no need to exclude ACPI info from "-M xenpv".

So my first review was wrong, but I still agree with the patch.

Paolo

Re: [Qemu-devel] [PATCH] pc: disable acpi info for isapc and old pc machine

[Michael S. Tsirkin]

On Mon, Nov 04, 2013 at 03:13:50PM +0100, Paolo Bonzini wrote:
> Il 04/11/2013 11:46, Michael S. Tsirkin ha scritto:
> > Disable acpi build for isapc and no_kvmclock machine
> > types (used by xen), since acpi build currently expects pci.
> > 
> > Reported-by: Andreas Färber <afaerber@suse.de>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Wait, Xen is not using pc_init_pci_no_kvmclock.  But that's not a
> problem, we can expose ACPI info via fw_cfg and then Xen hvmloader can
> decide whether to use it or not.  Right now it doesn't, maybe later on
> it should.
> 
> If Xen is broken, we need to patch pc_xen_hvm_init, but Andreas only
> tested isapc not xenfv.  So unless someone reports otherwise, there is
> no need to exclude ACPI info from "-M xenpv".
> 
> So my first review was wrong, but I still agree with the patch.
> 
> Paolo

I really went by the comment
/* PC init function for pc-0.10 to pc-0.13, and reused by xenfv */
static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)

Clearly pc-0.13 and older should not have acpi :)

-- 
MST

Re: [Qemu-devel] [PATCH] pc: disable acpi info for isapc and old pc machine

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

On Mon, 4 Nov 2013, Paolo Bonzini wrote:
> Il 04/11/2013 11:46, Michael S. Tsirkin ha scritto:
> > Disable acpi build for isapc and no_kvmclock machine
> > types (used by xen), since acpi build currently expects pci.
> 
> Xen is now using "-M pc", so it's okay to remove it from the legacy "-M
> xenfv".

that is correct

> > Reported-by: Andreas Färber <afaerber@suse.de>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/i386/pc_piix.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 24a98cb..4fdb7b6 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -309,6 +309,7 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
> >  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
> >  {
> >      has_pci_info = false;
> > +    has_acpi_build = false;
> >      disable_kvm_pv_eoi();
> >      enable_compat_apic_id_mode();
> >      pc_init1(args, 1, 0);
> > @@ -317,6 +318,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
> >  static void pc_init_isa(QEMUMachineInitArgs *args)
> >  {
> >      has_pci_info = false;
> > +    has_acpi_build = false;
> >      if (!args->cpu_model) {
> >          args->cpu_model = "486";
> >      }
> > 
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 

Re: [Qemu-devel] [PATCH] pc: disable acpi info for isapc and old pc machine

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

On Mon, 4 Nov 2013, Stefano Stabellini wrote:
> On Mon, 4 Nov 2013, Paolo Bonzini wrote:
> > Il 04/11/2013 11:46, Michael S. Tsirkin ha scritto:
> > > Disable acpi build for isapc and no_kvmclock machine
> > > types (used by xen), since acpi build currently expects pci.
> > 
> > Xen is now using "-M pc", so it's okay to remove it from the legacy "-M
> > xenfv".
> 
> that is correct

Partially correct: even xenfv is not using the no_kvmclock init function
anymore. So it is certainly OK from Xen POV to remove acpi builds for
it, but the commit message is wrong.


> > > Reported-by: Andreas Färber <afaerber@suse.de>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  hw/i386/pc_piix.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index 24a98cb..4fdb7b6 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -309,6 +309,7 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
> > >  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
> > >  {
> > >      has_pci_info = false;
> > > +    has_acpi_build = false;
> > >      disable_kvm_pv_eoi();
> > >      enable_compat_apic_id_mode();
> > >      pc_init1(args, 1, 0);
> > > @@ -317,6 +318,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
> > >  static void pc_init_isa(QEMUMachineInitArgs *args)
> > >  {
> > >      has_pci_info = false;
> > > +    has_acpi_build = false;
> > >      if (!args->cpu_model) {
> > >          args->cpu_model = "486";
> > >      }
> > > 
> > 
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > 

Re: [Qemu-devel] [PATCH] pc: disable acpi info for isapc and old pc machine

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

On Mon, 4 Nov 2013, Michael S. Tsirkin wrote:
> On Mon, Nov 04, 2013 at 03:13:50PM +0100, Paolo Bonzini wrote:
> > Il 04/11/2013 11:46, Michael S. Tsirkin ha scritto:
> > > Disable acpi build for isapc and no_kvmclock machine
> > > types (used by xen), since acpi build currently expects pci.
> > > 
> > > Reported-by: Andreas Färber <afaerber@suse.de>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > Wait, Xen is not using pc_init_pci_no_kvmclock.  But that's not a
> > problem, we can expose ACPI info via fw_cfg and then Xen hvmloader can
> > decide whether to use it or not.  Right now it doesn't, maybe later on
> > it should.
> > 
> > If Xen is broken, we need to patch pc_xen_hvm_init, but Andreas only
> > tested isapc not xenfv.  So unless someone reports otherwise, there is
> > no need to exclude ACPI info from "-M xenpv".
> > 
> > So my first review was wrong, but I still agree with the patch.
> > 
> > Paolo
> 
> I really went by the comment
> /* PC init function for pc-0.10 to pc-0.13, and reused by xenfv */
> static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
> 
> Clearly pc-0.13 and older should not have acpi :)
 
That's fine, it's just that the comment is outdated.
Since you are at it you could remove the "and reused by xenfv" :-)

Re: [Qemu-devel] [PATCH] pc: disable acpi info for isapc and old pc machine

[Andreas Färber]

Am 04.11.2013 11:46, schrieb Michael S. Tsirkin:
> Disable acpi build for isapc and no_kvmclock machine
> types (used by xen), since acpi build currently expects pci.
> 
> Reported-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/pc_piix.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 24a98cb..4fdb7b6 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -309,6 +309,7 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
>  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
> +    has_acpi_build = false;
>      disable_kvm_pv_eoi();
>      enable_compat_apic_id_mode();
>      pc_init1(args, 1, 0);

> @@ -317,6 +318,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>  static void pc_init_isa(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
> +    has_acpi_build = false;
>      if (!args->cpu_model) {
>          args->cpu_model = "486";
>      }

FWIW I forgot to reply yesterday that it solves my isapc qom-test
failure, too

Tested-by: Andreas Färber <afaerber@suse.de>

Thanks!

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg