[Qemu-devel] [PATCH v2] pc: drop external DSDT loading

[Qemu-devel] [PATCH v2] pc: drop external DSDT loading

[Anthony Liguori]

This breaks migration and is unneeded with modern SeaBIOS.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - Still load external DSDT for q35
---
 hw/i386/pc_piix.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 95c45b8..311574a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -103,7 +103,6 @@ static void pc_init1(MemoryRegion *system_memory,
                               OBJECT(icc_bridge), NULL);
 
     pc_cpus_init(cpu_model, icc_bridge);
-    pc_acpi_init("acpi-dsdt.aml");
 
     if (kvm_enabled() && kvmclock_enabled) {
         kvmclock_create();
-- 
1.8.0

Re: [Qemu-devel] [PATCH v2] pc: drop external DSDT loading

[Andreas Färber]

Am 12.08.2013 16:01, schrieb Anthony Liguori:
> This breaks migration and is unneeded with modern SeaBIOS.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> v1 -> v2
>  - Still load external DSDT for q35

Care to extend $subject with "for i440fx" then before applying? :)

Regards,
Andreas

> ---
>  hw/i386/pc_piix.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 95c45b8..311574a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -103,7 +103,6 @@ static void pc_init1(MemoryRegion *system_memory,
>                                OBJECT(icc_bridge), NULL);
>  
>      pc_cpus_init(cpu_model, icc_bridge);
> -    pc_acpi_init("acpi-dsdt.aml");
>  
>      if (kvm_enabled() && kvmclock_enabled) {
>          kvmclock_create();
> 


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

Re: [Qemu-devel] [PATCH v2] pc: drop external DSDT loading

[Michael S. Tsirkin]

On Mon, Aug 12, 2013 at 09:01:44AM -0500, Anthony Liguori wrote:
> This breaks migration and is unneeded with modern SeaBIOS.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Hmm don't we want to keep it around for machine types
1.4.0 and 1.5.0?

By the way, copy stable as well?
Loading it unconditonally is a cross
version migration bug that we probably want to fix
on stable branch - disabling for 1.3.0 and older.

> ---
> v1 -> v2
>  - Still load external DSDT for q35
> ---
>  hw/i386/pc_piix.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 95c45b8..311574a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -103,7 +103,6 @@ static void pc_init1(MemoryRegion *system_memory,
>                                OBJECT(icc_bridge), NULL);
>  
>      pc_cpus_init(cpu_model, icc_bridge);
> -    pc_acpi_init("acpi-dsdt.aml");
>  
>      if (kvm_enabled() && kvmclock_enabled) {
>          kvmclock_create();
> -- 
> 1.8.0

Re: [Qemu-devel] [PATCH v2] pc: drop external DSDT loading

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Aug 12, 2013 at 09:01:44AM -0500, Anthony Liguori wrote:
>> This breaks migration and is unneeded with modern SeaBIOS.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> Hmm don't we want to keep it around for machine types
> 1.4.0 and 1.5.0?

Hrm, why?

Regards,

Anthony Liguori

>
> By the way, copy stable as well?
> Loading it unconditonally is a cross
> version migration bug that we probably want to fix
> on stable branch - disabling for 1.3.0 and older.
>
>> ---
>> v1 -> v2
>>  - Still load external DSDT for q35
>> ---
>>  hw/i386/pc_piix.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 95c45b8..311574a 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -103,7 +103,6 @@ static void pc_init1(MemoryRegion *system_memory,
>>                                OBJECT(icc_bridge), NULL);
>>  
>>      pc_cpus_init(cpu_model, icc_bridge);
>> -    pc_acpi_init("acpi-dsdt.aml");
>>  
>>      if (kvm_enabled() && kvmclock_enabled) {
>>          kvmclock_create();
>> -- 
>> 1.8.0

Re: [Qemu-devel] [PATCH v2] pc: drop external DSDT loading

[Michael S. Tsirkin]

On Mon, Aug 12, 2013 at 10:20:41AM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Aug 12, 2013 at 09:01:44AM -0500, Anthony Liguori wrote:
> >> This breaks migration and is unneeded with modern SeaBIOS.
> >> 
> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >
> > Hmm don't we want to keep it around for machine types
> > 1.4.0 and 1.5.0?
> 
> Hrm, why?

Well this modifies the contents of ROM which
is loaded from FW CFG and isn't migrated, correct?
If guest is migrated from 1.6 to 1.5
while it is loading the ROM,
it will likely get a corrupted table.


> Regards,
> 
> Anthony Liguori
> 
> >
> > By the way, copy stable as well?
> > Loading it unconditonally is a cross
> > version migration bug that we probably want to fix
> > on stable branch - disabling for 1.3.0 and older.
> >
> >> ---
> >> v1 -> v2
> >>  - Still load external DSDT for q35
> >> ---
> >>  hw/i386/pc_piix.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >> 
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index 95c45b8..311574a 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -103,7 +103,6 @@ static void pc_init1(MemoryRegion *system_memory,
> >>                                OBJECT(icc_bridge), NULL);
> >>  
> >>      pc_cpus_init(cpu_model, icc_bridge);
> >> -    pc_acpi_init("acpi-dsdt.aml");
> >>  
> >>      if (kvm_enabled() && kvmclock_enabled) {
> >>          kvmclock_create();
> >> -- 
> >> 1.8.0

Re: [Qemu-devel] [PATCH v2] pc: drop external DSDT loading

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Aug 12, 2013 at 10:20:41AM -0500, Anthony Liguori wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Mon, Aug 12, 2013 at 09:01:44AM -0500, Anthony Liguori wrote:
>> >> This breaks migration and is unneeded with modern SeaBIOS.
>> >> 
>> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> >
>> > Hmm don't we want to keep it around for machine types
>> > 1.4.0 and 1.5.0?
>> 
>> Hrm, why?
>
> Well this modifies the contents of ROM which
> is loaded from FW CFG and isn't migrated, correct?
> If guest is migrated from 1.6 to 1.5
> while it is loading the ROM,
> it will likely get a corrupted table.

No, if you migrate from 1.6 to 1.5 the ROM is untouched.  ROMs aren't
migrated so it doesn't really matter.

If you migrate from 1.5 while reading the ROM contents, then badness can
ensue but that's already the case without this patch.

Regards,

Anthony Liguori

>
>
>> Regards,
>> 
>> Anthony Liguori
>> 
>> >
>> > By the way, copy stable as well?
>> > Loading it unconditonally is a cross
>> > version migration bug that we probably want to fix
>> > on stable branch - disabling for 1.3.0 and older.
>> >
>> >> ---
>> >> v1 -> v2
>> >>  - Still load external DSDT for q35
>> >> ---
>> >>  hw/i386/pc_piix.c | 1 -
>> >>  1 file changed, 1 deletion(-)
>> >> 
>> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> >> index 95c45b8..311574a 100644
>> >> --- a/hw/i386/pc_piix.c
>> >> +++ b/hw/i386/pc_piix.c
>> >> @@ -103,7 +103,6 @@ static void pc_init1(MemoryRegion *system_memory,
>> >>                                OBJECT(icc_bridge), NULL);
>> >>  
>> >>      pc_cpus_init(cpu_model, icc_bridge);
>> >> -    pc_acpi_init("acpi-dsdt.aml");
>> >>  
>> >>      if (kvm_enabled() && kvmclock_enabled) {
>> >>          kvmclock_create();
>> >> -- 
>> >> 1.8.0

Re: [Qemu-devel] [PATCH v2] pc: drop external DSDT loading

[Michael S. Tsirkin]

On Mon, Aug 12, 2013 at 02:59:24PM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Aug 12, 2013 at 10:20:41AM -0500, Anthony Liguori wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > On Mon, Aug 12, 2013 at 09:01:44AM -0500, Anthony Liguori wrote:
> >> >> This breaks migration and is unneeded with modern SeaBIOS.
> >> >> 
> >> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >> >
> >> > Hmm don't we want to keep it around for machine types
> >> > 1.4.0 and 1.5.0?
> >> 
> >> Hrm, why?
> >
> > Well this modifies the contents of ROM which
> > is loaded from FW CFG and isn't migrated, correct?
> > If guest is migrated from 1.6 to 1.5
> > while it is loading the ROM,
> > it will likely get a corrupted table.
> 
> No, if you migrate from 1.6 to 1.5 the ROM is untouched.  ROMs aren't
> migrated so it doesn't really matter.

Clarifying this on IRC, what was meant here is that
-acpi is uncommon so bios typically checks and sees nothing
in the ACPI FW CFG entry, and this check is atomic.
So this works, by luck.

> 
> If you migrate from 1.5 while reading the ROM contents, then badness can
> ensue but that's already the case without this patch.
> 
> Regards,
> 
> Anthony Liguori

Well there's a difference between can and will :)
But yes, some future version will likely break it anyway.


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> >
> >
> >> Regards,
> >> 
> >> Anthony Liguori
> >> 
> >> >
> >> > By the way, copy stable as well?
> >> > Loading it unconditonally is a cross
> >> > version migration bug that we probably want to fix
> >> > on stable branch - disabling for 1.3.0 and older.
> >> >
> >> >> ---
> >> >> v1 -> v2
> >> >>  - Still load external DSDT for q35
> >> >> ---
> >> >>  hw/i386/pc_piix.c | 1 -
> >> >>  1 file changed, 1 deletion(-)
> >> >> 
> >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> >> index 95c45b8..311574a 100644
> >> >> --- a/hw/i386/pc_piix.c
> >> >> +++ b/hw/i386/pc_piix.c
> >> >> @@ -103,7 +103,6 @@ static void pc_init1(MemoryRegion *system_memory,
> >> >>                                OBJECT(icc_bridge), NULL);
> >> >>  
> >> >>      pc_cpus_init(cpu_model, icc_bridge);
> >> >> -    pc_acpi_init("acpi-dsdt.aml");
> >> >>  
> >> >>      if (kvm_enabled() && kvmclock_enabled) {
> >> >>          kvmclock_create();
> >> >> -- 
> >> >> 1.8.0

Re: [Qemu-devel] [PATCH v2] pc: drop external DSDT loading

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori