[2/2] xen: cleanup unrealized flash devices

Commit Message

Paul Durrant June 24, 2020, 12:18 p.m. UTC

From: Paul Durrant <[e-mail]>

The generic pc_machine_initfn() calls pc_system_flash_create() which creates
'system.flash0' and 'system.flash1' devices. These devices are then realized
by pc_system_flash_map() which is called from pc_system_firmware_init() which
itself is called via pc_memory_init(). The latter however is not called when
xen_enable() is true and hence the following assertion fails:

qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
Assertion `dev->realized' failed

These flash devices are unneeded when using Xen so this patch avoids the
assertion by simply removing them using pc_system_flash_cleanup_unused().

Reported-by: Jason Andryuk <[e-mail]>
Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
Signed-off-by: Paul Durrant <[e-mail]>
Tested-by: Jason Andryuk <[e-mail]>
---
Cc: Paolo Bonzini <[e-mail]>
Cc: Richard Henderson <[e-mail]>
Cc: Eduardo Habkost <[e-mail]>
Cc: "Michael S. Tsirkin" <[e-mail]>
Cc: Marcel Apfelbaum <[e-mail]>
---
 hw/i386/pc_piix.c    | 9 ++++++---
 hw/i386/pc_sysfw.c   | 2 +-
 include/hw/i386/pc.h | 1 +
 3 files changed, 8 insertions(+), 4 deletions(-)

Comments

Anthony PERARD June 30, 2020, 3:08 p.m. UTC | #1

On Wed, Jun 24, 2020 at 01:18:41PM +0100, Paul Durrant wrote:
> From: Paul Durrant <[e-mail]>
> 
> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
> 'system.flash0' and 'system.flash1' devices. These devices are then realized
> by pc_system_flash_map() which is called from pc_system_firmware_init() which
> itself is called via pc_memory_init(). The latter however is not called when
> xen_enable() is true and hence the following assertion fails:
> 
> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> Assertion `dev->realized' failed
> 
> These flash devices are unneeded when using Xen so this patch avoids the
> assertion by simply removing them using pc_system_flash_cleanup_unused().
> 
> Reported-by: Jason Andryuk <[e-mail]>
> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> Signed-off-by: Paul Durrant <[e-mail]>
> Tested-by: Jason Andryuk <[e-mail]>

Reviewed-by: Anthony PERARD <[e-mail]>

I think I would add:

Fixes: dfe8c79c4468 ("qdev: Assert onboard devices all get realized properly")

as this is the first commit where the unrealized flash devices are an
issue.

Paul Durrant June 30, 2020, 3:17 p.m. UTC | #2

> -----Original Message-----
> From: Anthony PERARD <[e-mail]>
> Sent: 30 June 2020 16:09
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; [e-mail]; Eduardo Habkost <[e-mail]>;
> Michael S. Tsirkin <[e-mail]>; Paul Durrant <[e-mail]>; Jason Andryuk
> <[e-mail]>; Paolo Bonzini <[e-mail]>; Richard Henderson <[e-mail]>
> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> 
> On Wed, Jun 24, 2020 at 01:18:41PM +0100, Paul Durrant wrote:
> > From: Paul Durrant <[e-mail]>
> >
> > The generic pc_machine_initfn() calls pc_system_flash_create() which creates
> > 'system.flash0' and 'system.flash1' devices. These devices are then realized
> > by pc_system_flash_map() which is called from pc_system_firmware_init() which
> > itself is called via pc_memory_init(). The latter however is not called when
> > xen_enable() is true and hence the following assertion fails:
> >
> > qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > Assertion `dev->realized' failed
> >
> > These flash devices are unneeded when using Xen so this patch avoids the
> > assertion by simply removing them using pc_system_flash_cleanup_unused().
> >
> > Reported-by: Jason Andryuk <[e-mail]>
> > Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> > Signed-off-by: Paul Durrant <[e-mail]>
> > Tested-by: Jason Andryuk <[e-mail]>
> 
> Reviewed-by: Anthony PERARD <[e-mail]>
> 
> I think I would add:
> 
> Fixes: dfe8c79c4468 ("qdev: Assert onboard devices all get realized properly")
> 
> as this is the first commit where the unrealized flash devices are an
> issue.

OK.

  Paul

> 
> --
> Anthony PERARD

Philippe Mathieu-Daudé June 30, 2020, 3:25 p.m. UTC | #3

On 6/24/20 2:18 PM, Paul Durrant wrote:
> From: Paul Durrant <[e-mail]>
> 
> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
> 'system.flash0' and 'system.flash1' devices. These devices are then realized
> by pc_system_flash_map() which is called from pc_system_firmware_init() which
> itself is called via pc_memory_init(). The latter however is not called when
> xen_enable() is true and hence the following assertion fails:
> 
> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> Assertion `dev->realized' failed
> 
> These flash devices are unneeded when using Xen so this patch avoids the
> assertion by simply removing them using pc_system_flash_cleanup_unused().
> 
> Reported-by: Jason Andryuk <[e-mail]>
> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> Signed-off-by: Paul Durrant <[e-mail]>
> Tested-by: Jason Andryuk <[e-mail]>
> ---
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Richard Henderson <[e-mail]>
> Cc: Eduardo Habkost <[e-mail]>
> Cc: "Michael S. Tsirkin" <[e-mail]>
> Cc: Marcel Apfelbaum <[e-mail]>
> ---
>  hw/i386/pc_piix.c    | 9 ++++++---
>  hw/i386/pc_sysfw.c   | 2 +-
>  include/hw/i386/pc.h | 1 +
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1497d0e4ae..977d40afb8 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>      if (!xen_enabled()) {
>          pc_memory_init(pcms, system_memory,
>                         rom_memory, &ram_memory);
> -    } else if (machine->kernel_filename != NULL) {
> -        /* For xen HVM direct kernel boot, load linux here */
> -        xen_load_linux(pcms);
> +    } else {
> +        pc_system_flash_cleanup_unused(pcms);

TIL pc_system_flash_cleanup_unused().

What about restricting at the source?

-- >8 --
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1004,24 +1004,26 @@ void pc_memory_init(PCMachineState *pcms,
                                     &machine->device_memory->mr);
     }

-    /* Initialize PC system firmware */
-    pc_system_firmware_init(pcms, rom_memory);
-
-    option_rom_mr = g_malloc(sizeof(*option_rom_mr));
-    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
-                           &error_fatal);
-    if (pcmc->pci_enabled) {
-        memory_region_set_readonly(option_rom_mr, true);
-    }
-    memory_region_add_subregion_overlap(rom_memory,
-                                        PC_ROM_MIN_VGA,
-                                        option_rom_mr,
-                                        1);
-
     fw_cfg = fw_cfg_arch_create(machine,
                                 x86ms->boot_cpus, x86ms->apic_id_limit);

-    rom_set_fw(fw_cfg);
+    /* Initialize PC system firmware */
+    if (!xen_enabled()) {
+        pc_system_firmware_init(pcms, rom_memory);
+
+        option_rom_mr = g_malloc(sizeof(*option_rom_mr));
+        memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
+                               &error_fatal);
+        if (pcmc->pci_enabled) {
+            memory_region_set_readonly(option_rom_mr, true);
+        }
+        memory_region_add_subregion_overlap(rom_memory,
+                                            PC_ROM_MIN_VGA,
+                                            option_rom_mr,
+                                            1);
+
+        rom_set_fw(fw_cfg);
+    }

     if (pcmc->has_reserved_memory && machine->device_memory->base) {
         uint64_t *val = g_malloc(sizeof(*val));
---

> +        if (machine->kernel_filename != NULL) {
> +            /* For xen HVM direct kernel boot, load linux here */
> +            xen_load_linux(pcms);
> +        }
>      }
>  
>      gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index ec2a3b3e7e..0ff47a4b59 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
>      }
>  }
>  
> -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
>  {
>      char *prop_name;
>      int i;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index e6135c34d6..497f2b7ab7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
>  
>  /* pc_sysfw.c */
>  void pc_system_flash_create(PCMachineState *pcms);
> +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
>  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>  
>  /* acpi-build.c */
>

Paul Durrant June 30, 2020, 3:44 p.m. UTC | #4

> -----Original Message-----
> From: Philippe Mathieu-Daudé <[e-mail]>
> Sent: 30 June 2020 16:26
> To: Paul Durrant <[e-mail]>; [e-mail]; [e-mail]
> Cc: Eduardo Habkost <[e-mail]>; Michael S. Tsirkin <[e-mail]>; Paul Durrant
> <[e-mail]>; Jason Andryuk <[e-mail]>; Paolo Bonzini <[e-mail]>;
> Richard Henderson <[e-mail]>
> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> 
> On 6/24/20 2:18 PM, Paul Durrant wrote:
> > From: Paul Durrant <[e-mail]>
> >
> > The generic pc_machine_initfn() calls pc_system_flash_create() which creates
> > 'system.flash0' and 'system.flash1' devices. These devices are then realized
> > by pc_system_flash_map() which is called from pc_system_firmware_init() which
> > itself is called via pc_memory_init(). The latter however is not called when
> > xen_enable() is true and hence the following assertion fails:
> >
> > qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > Assertion `dev->realized' failed
> >
> > These flash devices are unneeded when using Xen so this patch avoids the
> > assertion by simply removing them using pc_system_flash_cleanup_unused().
> >
> > Reported-by: Jason Andryuk <[e-mail]>
> > Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> > Signed-off-by: Paul Durrant <[e-mail]>
> > Tested-by: Jason Andryuk <[e-mail]>
> > ---
> > Cc: Paolo Bonzini <[e-mail]>
> > Cc: Richard Henderson <[e-mail]>
> > Cc: Eduardo Habkost <[e-mail]>
> > Cc: "Michael S. Tsirkin" <[e-mail]>
> > Cc: Marcel Apfelbaum <[e-mail]>
> > ---
> >  hw/i386/pc_piix.c    | 9 ++++++---
> >  hw/i386/pc_sysfw.c   | 2 +-
> >  include/hw/i386/pc.h | 1 +
> >  3 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 1497d0e4ae..977d40afb8 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> >      if (!xen_enabled()) {
> >          pc_memory_init(pcms, system_memory,
> >                         rom_memory, &ram_memory);
> > -    } else if (machine->kernel_filename != NULL) {
> > -        /* For xen HVM direct kernel boot, load linux here */
> > -        xen_load_linux(pcms);
> > +    } else {
> > +        pc_system_flash_cleanup_unused(pcms);
> 
> TIL pc_system_flash_cleanup_unused().
> 
> What about restricting at the source?
>

And leave the devices in place? They are not relevant for Xen, so why not clean up?

  Paul
 
> -- >8 --
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1004,24 +1004,26 @@ void pc_memory_init(PCMachineState *pcms,
>                                      &machine->device_memory->mr);
>      }
> 
> -    /* Initialize PC system firmware */
> -    pc_system_firmware_init(pcms, rom_memory);
> -
> -    option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> -    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> -                           &error_fatal);
> -    if (pcmc->pci_enabled) {
> -        memory_region_set_readonly(option_rom_mr, true);
> -    }
> -    memory_region_add_subregion_overlap(rom_memory,
> -                                        PC_ROM_MIN_VGA,
> -                                        option_rom_mr,
> -                                        1);
> -
>      fw_cfg = fw_cfg_arch_create(machine,
>                                  x86ms->boot_cpus, x86ms->apic_id_limit);
> 
> -    rom_set_fw(fw_cfg);
> +    /* Initialize PC system firmware */
> +    if (!xen_enabled()) {
> +        pc_system_firmware_init(pcms, rom_memory);
> +
> +        option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> +        memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> +                               &error_fatal);
> +        if (pcmc->pci_enabled) {
> +            memory_region_set_readonly(option_rom_mr, true);
> +        }
> +        memory_region_add_subregion_overlap(rom_memory,
> +                                            PC_ROM_MIN_VGA,
> +                                            option_rom_mr,
> +                                            1);
> +
> +        rom_set_fw(fw_cfg);
> +    }
> 
>      if (pcmc->has_reserved_memory && machine->device_memory->base) {
>          uint64_t *val = g_malloc(sizeof(*val));
> ---
> 
> > +        if (machine->kernel_filename != NULL) {
> > +            /* For xen HVM direct kernel boot, load linux here */
> > +            xen_load_linux(pcms);
> > +        }
> >      }
> >
> >      gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
> > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > index ec2a3b3e7e..0ff47a4b59 100644
> > --- a/hw/i386/pc_sysfw.c
> > +++ b/hw/i386/pc_sysfw.c
> > @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
> >      }
> >  }
> >
> > -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> > +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> >  {
> >      char *prop_name;
> >      int i;
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index e6135c34d6..497f2b7ab7 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
> >
> >  /* pc_sysfw.c */
> >  void pc_system_flash_create(PCMachineState *pcms);
> > +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
> >  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
> >
> >  /* acpi-build.c */
> >

Philippe Mathieu-Daudé June 30, 2020, 5:27 p.m. UTC | #5

On 6/30/20 5:44 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé <[e-mail]>
>> Sent: 30 June 2020 16:26
>> To: Paul Durrant <[e-mail]>; [e-mail]; [e-mail]
>> Cc: Eduardo Habkost <[e-mail]>; Michael S. Tsirkin <[e-mail]>; Paul Durrant
>> <[e-mail]>; Jason Andryuk <[e-mail]>; Paolo Bonzini <[e-mail]>;
>> Richard Henderson <[e-mail]>
>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>
>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>> From: Paul Durrant <[e-mail]>
>>>
>>> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
>>> 'system.flash0' and 'system.flash1' devices. These devices are then realized
>>> by pc_system_flash_map() which is called from pc_system_firmware_init() which
>>> itself is called via pc_memory_init(). The latter however is not called when
>>> xen_enable() is true and hence the following assertion fails:
>>>
>>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
>>> Assertion `dev->realized' failed
>>>
>>> These flash devices are unneeded when using Xen so this patch avoids the
>>> assertion by simply removing them using pc_system_flash_cleanup_unused().
>>>
>>> Reported-by: Jason Andryuk <[e-mail]>
>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>>> Signed-off-by: Paul Durrant <[e-mail]>
>>> Tested-by: Jason Andryuk <[e-mail]>
>>> ---
>>> Cc: Paolo Bonzini <[e-mail]>
>>> Cc: Richard Henderson <[e-mail]>
>>> Cc: Eduardo Habkost <[e-mail]>
>>> Cc: "Michael S. Tsirkin" <[e-mail]>
>>> Cc: Marcel Apfelbaum <[e-mail]>
>>> ---
>>>  hw/i386/pc_piix.c    | 9 ++++++---
>>>  hw/i386/pc_sysfw.c   | 2 +-
>>>  include/hw/i386/pc.h | 1 +
>>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index 1497d0e4ae..977d40afb8 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>>>      if (!xen_enabled()) {
>>>          pc_memory_init(pcms, system_memory,
>>>                         rom_memory, &ram_memory);
>>> -    } else if (machine->kernel_filename != NULL) {
>>> -        /* For xen HVM direct kernel boot, load linux here */
>>> -        xen_load_linux(pcms);
>>> +    } else {
>>> +        pc_system_flash_cleanup_unused(pcms);
>>
>> TIL pc_system_flash_cleanup_unused().
>>
>> What about restricting at the source?
>>
> 
> And leave the devices in place? They are not relevant for Xen, so why not clean up?

No, I meant to not create them in the first place, instead of
create+destroy.

Anyway what you did works, so I don't have any problem.

Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>

> 
>   Paul
>  
>> -- >8 --
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1004,24 +1004,26 @@ void pc_memory_init(PCMachineState *pcms,
>>                                      &machine->device_memory->mr);
>>      }
>>
>> -    /* Initialize PC system firmware */
>> -    pc_system_firmware_init(pcms, rom_memory);
>> -
>> -    option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>> -    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
>> -                           &error_fatal);
>> -    if (pcmc->pci_enabled) {
>> -        memory_region_set_readonly(option_rom_mr, true);
>> -    }
>> -    memory_region_add_subregion_overlap(rom_memory,
>> -                                        PC_ROM_MIN_VGA,
>> -                                        option_rom_mr,
>> -                                        1);
>> -
>>      fw_cfg = fw_cfg_arch_create(machine,
>>                                  x86ms->boot_cpus, x86ms->apic_id_limit);
>>
>> -    rom_set_fw(fw_cfg);
>> +    /* Initialize PC system firmware */
>> +    if (!xen_enabled()) {
>> +        pc_system_firmware_init(pcms, rom_memory);
>> +
>> +        option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>> +        memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
>> +                               &error_fatal);
>> +        if (pcmc->pci_enabled) {
>> +            memory_region_set_readonly(option_rom_mr, true);
>> +        }
>> +        memory_region_add_subregion_overlap(rom_memory,
>> +                                            PC_ROM_MIN_VGA,
>> +                                            option_rom_mr,
>> +                                            1);
>> +
>> +        rom_set_fw(fw_cfg);
>> +    }
>>
>>      if (pcmc->has_reserved_memory && machine->device_memory->base) {
>>          uint64_t *val = g_malloc(sizeof(*val));
>> ---
>>
>>> +        if (machine->kernel_filename != NULL) {
>>> +            /* For xen HVM direct kernel boot, load linux here */
>>> +            xen_load_linux(pcms);
>>> +        }
>>>      }
>>>
>>>      gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
>>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>>> index ec2a3b3e7e..0ff47a4b59 100644
>>> --- a/hw/i386/pc_sysfw.c
>>> +++ b/hw/i386/pc_sysfw.c
>>> @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
>>>      }
>>>  }
>>>
>>> -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
>>> +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
>>>  {
>>>      char *prop_name;
>>>      int i;
>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>> index e6135c34d6..497f2b7ab7 100644
>>> --- a/include/hw/i386/pc.h
>>> +++ b/include/hw/i386/pc.h
>>> @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
>>>
>>>  /* pc_sysfw.c */
>>>  void pc_system_flash_create(PCMachineState *pcms);
>>> +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
>>>  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>>>
>>>  /* acpi-build.c */
>>>
> 
>

Markus Armbruster July 1, 2020, 5:56 a.m. UTC | #6

Anthony PERARD <[e-mail]> writes:

> On Wed, Jun 24, 2020 at 01:18:41PM +0100, Paul Durrant wrote:
>> From: Paul Durrant <[e-mail]>
>> 
>> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
>> 'system.flash0' and 'system.flash1' devices. These devices are then realized
>> by pc_system_flash_map() which is called from pc_system_firmware_init() which
>> itself is called via pc_memory_init(). The latter however is not called when
>> xen_enable() is true and hence the following assertion fails:
>> 
>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
>> Assertion `dev->realized' failed
>> 
>> These flash devices are unneeded when using Xen so this patch avoids the
>> assertion by simply removing them using pc_system_flash_cleanup_unused().
>> 
>> Reported-by: Jason Andryuk <[e-mail]>
>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>> Signed-off-by: Paul Durrant <[e-mail]>
>> Tested-by: Jason Andryuk <[e-mail]>
>
> Reviewed-by: Anthony PERARD <[e-mail]>
>
> I think I would add:
>
> Fixes: dfe8c79c4468 ("qdev: Assert onboard devices all get realized properly")
>
> as this is the first commit where the unrealized flash devices are an
> issue.

They were an issue before, but commit dfe8c79c4468 turned the minor
issue into a crash bug.  No objections.

Markus Armbruster July 1, 2020, 5:59 a.m. UTC | #7

Philippe Mathieu-Daudé <[e-mail]> writes:

> On 6/30/20 5:44 PM, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Philippe Mathieu-Daudé <[e-mail]>
>>> Sent: 30 June 2020 16:26
>>> To: Paul Durrant <[e-mail]>; [e-mail]; [e-mail]
>>> Cc: Eduardo Habkost <[e-mail]>; Michael S. Tsirkin <[e-mail]>; Paul Durrant
>>> <[e-mail]>; Jason Andryuk <[e-mail]>; Paolo Bonzini <[e-mail]>;
>>> Richard Henderson <[e-mail]>
>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>
>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>>> From: Paul Durrant <[e-mail]>
>>>>
>>>> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
>>>> 'system.flash0' and 'system.flash1' devices. These devices are then realized
>>>> by pc_system_flash_map() which is called from pc_system_firmware_init() which
>>>> itself is called via pc_memory_init(). The latter however is not called when
>>>> xen_enable() is true and hence the following assertion fails:
>>>>
>>>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
>>>> Assertion `dev->realized' failed
>>>>
>>>> These flash devices are unneeded when using Xen so this patch avoids the
>>>> assertion by simply removing them using pc_system_flash_cleanup_unused().
>>>>
>>>> Reported-by: Jason Andryuk <[e-mail]>
>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>>>> Signed-off-by: Paul Durrant <[e-mail]>
>>>> Tested-by: Jason Andryuk <[e-mail]>
>>>> ---
>>>> Cc: Paolo Bonzini <[e-mail]>
>>>> Cc: Richard Henderson <[e-mail]>
>>>> Cc: Eduardo Habkost <[e-mail]>
>>>> Cc: "Michael S. Tsirkin" <[e-mail]>
>>>> Cc: Marcel Apfelbaum <[e-mail]>
>>>> ---
>>>>  hw/i386/pc_piix.c    | 9 ++++++---
>>>>  hw/i386/pc_sysfw.c   | 2 +-
>>>>  include/hw/i386/pc.h | 1 +
>>>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>> index 1497d0e4ae..977d40afb8 100644
>>>> --- a/hw/i386/pc_piix.c
>>>> +++ b/hw/i386/pc_piix.c
>>>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>>>>      if (!xen_enabled()) {
>>>>          pc_memory_init(pcms, system_memory,
>>>>                         rom_memory, &ram_memory);
>>>> -    } else if (machine->kernel_filename != NULL) {
>>>> -        /* For xen HVM direct kernel boot, load linux here */
>>>> -        xen_load_linux(pcms);
>>>> +    } else {
>>>> +        pc_system_flash_cleanup_unused(pcms);
>>>
>>> TIL pc_system_flash_cleanup_unused().
>>>
>>> What about restricting at the source?
>>>
>> 
>> And leave the devices in place? They are not relevant for Xen, so why not clean up?
>
> No, I meant to not create them in the first place, instead of
> create+destroy.

Better.  Opinion, not demand :)

[...]

Paul Durrant July 1, 2020, 7:03 a.m. UTC | #8

> -----Original Message-----
> From: Philippe Mathieu-Daudé <[e-mail]>
> Sent: 30 June 2020 18:27
> To: [e-mail]; [e-mail]; [e-mail]
> Cc: 'Eduardo Habkost' <[e-mail]>; 'Michael S. Tsirkin' <[e-mail]>; 'Paul Durrant'
> <[e-mail]>; 'Jason Andryuk' <[e-mail]>; 'Paolo Bonzini' <[e-mail]>;
> 'Richard Henderson' <[e-mail]>
> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> 
> On 6/30/20 5:44 PM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Philippe Mathieu-Daudé <[e-mail]>
> >> Sent: 30 June 2020 16:26
> >> To: Paul Durrant <[e-mail]>; [e-mail]; [e-mail]
> >> Cc: Eduardo Habkost <[e-mail]>; Michael S. Tsirkin <[e-mail]>; Paul Durrant
> >> <[e-mail]>; Jason Andryuk <[e-mail]>; Paolo Bonzini <[e-mail]>;
> >> Richard Henderson <[e-mail]>
> >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >>
> >> On 6/24/20 2:18 PM, Paul Durrant wrote:
> >>> From: Paul Durrant <[e-mail]>
> >>>
> >>> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
> >>> 'system.flash0' and 'system.flash1' devices. These devices are then realized
> >>> by pc_system_flash_map() which is called from pc_system_firmware_init() which
> >>> itself is called via pc_memory_init(). The latter however is not called when
> >>> xen_enable() is true and hence the following assertion fails:
> >>>
> >>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> >>> Assertion `dev->realized' failed
> >>>
> >>> These flash devices are unneeded when using Xen so this patch avoids the
> >>> assertion by simply removing them using pc_system_flash_cleanup_unused().
> >>>
> >>> Reported-by: Jason Andryuk <[e-mail]>
> >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> >>> Signed-off-by: Paul Durrant <[e-mail]>
> >>> Tested-by: Jason Andryuk <[e-mail]>
> >>> ---
> >>> Cc: Paolo Bonzini <[e-mail]>
> >>> Cc: Richard Henderson <[e-mail]>
> >>> Cc: Eduardo Habkost <[e-mail]>
> >>> Cc: "Michael S. Tsirkin" <[e-mail]>
> >>> Cc: Marcel Apfelbaum <[e-mail]>
> >>> ---
> >>>  hw/i386/pc_piix.c    | 9 ++++++---
> >>>  hw/i386/pc_sysfw.c   | 2 +-
> >>>  include/hw/i386/pc.h | 1 +
> >>>  3 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>> index 1497d0e4ae..977d40afb8 100644
> >>> --- a/hw/i386/pc_piix.c
> >>> +++ b/hw/i386/pc_piix.c
> >>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> >>>      if (!xen_enabled()) {
> >>>          pc_memory_init(pcms, system_memory,
> >>>                         rom_memory, &ram_memory);
> >>> -    } else if (machine->kernel_filename != NULL) {
> >>> -        /* For xen HVM direct kernel boot, load linux here */
> >>> -        xen_load_linux(pcms);
> >>> +    } else {
> >>> +        pc_system_flash_cleanup_unused(pcms);
> >>
> >> TIL pc_system_flash_cleanup_unused().
> >>
> >> What about restricting at the source?
> >>
> >
> > And leave the devices in place? They are not relevant for Xen, so why not clean up?
> 
> No, I meant to not create them in the first place, instead of
> create+destroy.
> 
> Anyway what you did works, so I don't have any problem.

IIUC Jason originally tried restricting creation but encountered a problem because xen_enabled() would always return false at that point, because machine creation occurs before accelerators are initialized.

  Paul

> 
> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> 
> >
> >   Paul
> >
> >> -- >8 --
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1004,24 +1004,26 @@ void pc_memory_init(PCMachineState *pcms,
> >>                                      &machine->device_memory->mr);
> >>      }
> >>
> >> -    /* Initialize PC system firmware */
> >> -    pc_system_firmware_init(pcms, rom_memory);
> >> -
> >> -    option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> >> -    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> >> -                           &error_fatal);
> >> -    if (pcmc->pci_enabled) {
> >> -        memory_region_set_readonly(option_rom_mr, true);
> >> -    }
> >> -    memory_region_add_subregion_overlap(rom_memory,
> >> -                                        PC_ROM_MIN_VGA,
> >> -                                        option_rom_mr,
> >> -                                        1);
> >> -
> >>      fw_cfg = fw_cfg_arch_create(machine,
> >>                                  x86ms->boot_cpus, x86ms->apic_id_limit);
> >>
> >> -    rom_set_fw(fw_cfg);
> >> +    /* Initialize PC system firmware */
> >> +    if (!xen_enabled()) {
> >> +        pc_system_firmware_init(pcms, rom_memory);
> >> +
> >> +        option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> >> +        memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> >> +                               &error_fatal);
> >> +        if (pcmc->pci_enabled) {
> >> +            memory_region_set_readonly(option_rom_mr, true);
> >> +        }
> >> +        memory_region_add_subregion_overlap(rom_memory,
> >> +                                            PC_ROM_MIN_VGA,
> >> +                                            option_rom_mr,
> >> +                                            1);
> >> +
> >> +        rom_set_fw(fw_cfg);
> >> +    }
> >>
> >>      if (pcmc->has_reserved_memory && machine->device_memory->base) {
> >>          uint64_t *val = g_malloc(sizeof(*val));
> >> ---
> >>
> >>> +        if (machine->kernel_filename != NULL) {
> >>> +            /* For xen HVM direct kernel boot, load linux here */
> >>> +            xen_load_linux(pcms);
> >>> +        }
> >>>      }
> >>>
> >>>      gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
> >>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> >>> index ec2a3b3e7e..0ff47a4b59 100644
> >>> --- a/hw/i386/pc_sysfw.c
> >>> +++ b/hw/i386/pc_sysfw.c
> >>> @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
> >>>      }
> >>>  }
> >>>
> >>> -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> >>> +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> >>>  {
> >>>      char *prop_name;
> >>>      int i;
> >>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >>> index e6135c34d6..497f2b7ab7 100644
> >>> --- a/include/hw/i386/pc.h
> >>> +++ b/include/hw/i386/pc.h
> >>> @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
> >>>
> >>>  /* pc_sysfw.c */
> >>>  void pc_system_flash_create(PCMachineState *pcms);
> >>> +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
> >>>  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
> >>>
> >>>  /* acpi-build.c */
> >>>
> >
> >

Jason Andryuk July 1, 2020, 12:25 p.m. UTC | #9

On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant <[e-mail]> wrote:
>
> > -----Original Message-----
> > From: Philippe Mathieu-Daudé <[e-mail]>
> > Sent: 30 June 2020 18:27
> > To: [e-mail]; [e-mail]; [e-mail]
> > Cc: 'Eduardo Habkost' <[e-mail]>; 'Michael S. Tsirkin' <[e-mail]>; 'Paul Durrant'
> > <[e-mail]>; 'Jason Andryuk' <[e-mail]>; 'Paolo Bonzini' <[e-mail]>;
> > 'Richard Henderson' <[e-mail]>
> > Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >
> > On 6/30/20 5:44 PM, Paul Durrant wrote:
> > >> -----Original Message-----
> > >> From: Philippe Mathieu-Daudé <[e-mail]>
> > >> Sent: 30 June 2020 16:26
> > >> To: Paul Durrant <[e-mail]>; [e-mail]; [e-mail]
> > >> Cc: Eduardo Habkost <[e-mail]>; Michael S. Tsirkin <[e-mail]>; Paul Durrant
> > >> <[e-mail]>; Jason Andryuk <[e-mail]>; Paolo Bonzini <[e-mail]>;
> > >> Richard Henderson <[e-mail]>
> > >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> > >>
> > >> On 6/24/20 2:18 PM, Paul Durrant wrote:
> > >>> From: Paul Durrant <[e-mail]>
> > >>>
> > >>> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
> > >>> 'system.flash0' and 'system.flash1' devices. These devices are then realized
> > >>> by pc_system_flash_map() which is called from pc_system_firmware_init() which
> > >>> itself is called via pc_memory_init(). The latter however is not called when
> > >>> xen_enable() is true and hence the following assertion fails:
> > >>>
> > >>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > >>> Assertion `dev->realized' failed
> > >>>
> > >>> These flash devices are unneeded when using Xen so this patch avoids the
> > >>> assertion by simply removing them using pc_system_flash_cleanup_unused().
> > >>>
> > >>> Reported-by: Jason Andryuk <[e-mail]>
> > >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> > >>> Signed-off-by: Paul Durrant <[e-mail]>
> > >>> Tested-by: Jason Andryuk <[e-mail]>
> > >>> ---
> > >>> Cc: Paolo Bonzini <[e-mail]>
> > >>> Cc: Richard Henderson <[e-mail]>
> > >>> Cc: Eduardo Habkost <[e-mail]>
> > >>> Cc: "Michael S. Tsirkin" <[e-mail]>
> > >>> Cc: Marcel Apfelbaum <[e-mail]>
> > >>> ---
> > >>>  hw/i386/pc_piix.c    | 9 ++++++---
> > >>>  hw/i386/pc_sysfw.c   | 2 +-
> > >>>  include/hw/i386/pc.h | 1 +
> > >>>  3 files changed, 8 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > >>> index 1497d0e4ae..977d40afb8 100644
> > >>> --- a/hw/i386/pc_piix.c
> > >>> +++ b/hw/i386/pc_piix.c
> > >>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> > >>>      if (!xen_enabled()) {
> > >>>          pc_memory_init(pcms, system_memory,
> > >>>                         rom_memory, &ram_memory);
> > >>> -    } else if (machine->kernel_filename != NULL) {
> > >>> -        /* For xen HVM direct kernel boot, load linux here */
> > >>> -        xen_load_linux(pcms);
> > >>> +    } else {
> > >>> +        pc_system_flash_cleanup_unused(pcms);
> > >>
> > >> TIL pc_system_flash_cleanup_unused().
> > >>
> > >> What about restricting at the source?
> > >>
> > >
> > > And leave the devices in place? They are not relevant for Xen, so why not clean up?
> >
> > No, I meant to not create them in the first place, instead of
> > create+destroy.
> >
> > Anyway what you did works, so I don't have any problem.
>
> IIUC Jason originally tried restricting creation but encountered a problem because xen_enabled() would always return false at that point, because machine creation occurs before accelerators are initialized.

Correct.  Quoting my previous email:
"""
Removing the call to pc_system_flash_create() from pc_machine_initfn()
lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
there since accelerators have not been initialized yes, I guess?
"""

If you want to remove the creation in the first place, then I have two
questions.  Why does pc_system_flash_create()/pc_pflash_create() get
called so early creating the pflash devices?  Why aren't they just
created as needed in pc_system_flash_map()?

Regards,
Jason

>   Paul
>
> >
> > Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> >
> > >
> > >   Paul
> > >
> > >> -- >8 --
> > >> --- a/hw/i386/pc.c
> > >> +++ b/hw/i386/pc.c
> > >> @@ -1004,24 +1004,26 @@ void pc_memory_init(PCMachineState *pcms,
> > >>                                      &machine->device_memory->mr);
> > >>      }
> > >>
> > >> -    /* Initialize PC system firmware */
> > >> -    pc_system_firmware_init(pcms, rom_memory);
> > >> -
> > >> -    option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> > >> -    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> > >> -                           &error_fatal);
> > >> -    if (pcmc->pci_enabled) {
> > >> -        memory_region_set_readonly(option_rom_mr, true);
> > >> -    }
> > >> -    memory_region_add_subregion_overlap(rom_memory,
> > >> -                                        PC_ROM_MIN_VGA,
> > >> -                                        option_rom_mr,
> > >> -                                        1);
> > >> -
> > >>      fw_cfg = fw_cfg_arch_create(machine,
> > >>                                  x86ms->boot_cpus, x86ms->apic_id_limit);
> > >>
> > >> -    rom_set_fw(fw_cfg);
> > >> +    /* Initialize PC system firmware */
> > >> +    if (!xen_enabled()) {
> > >> +        pc_system_firmware_init(pcms, rom_memory);
> > >> +
> > >> +        option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> > >> +        memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> > >> +                               &error_fatal);
> > >> +        if (pcmc->pci_enabled) {
> > >> +            memory_region_set_readonly(option_rom_mr, true);
> > >> +        }
> > >> +        memory_region_add_subregion_overlap(rom_memory,
> > >> +                                            PC_ROM_MIN_VGA,
> > >> +                                            option_rom_mr,
> > >> +                                            1);
> > >> +
> > >> +        rom_set_fw(fw_cfg);
> > >> +    }
> > >>
> > >>      if (pcmc->has_reserved_memory && machine->device_memory->base) {
> > >>          uint64_t *val = g_malloc(sizeof(*val));
> > >> ---
> > >>
> > >>> +        if (machine->kernel_filename != NULL) {
> > >>> +            /* For xen HVM direct kernel boot, load linux here */
> > >>> +            xen_load_linux(pcms);
> > >>> +        }
> > >>>      }
> > >>>
> > >>>      gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
> > >>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > >>> index ec2a3b3e7e..0ff47a4b59 100644
> > >>> --- a/hw/i386/pc_sysfw.c
> > >>> +++ b/hw/i386/pc_sysfw.c
> > >>> @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
> > >>>      }
> > >>>  }
> > >>>
> > >>> -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> > >>> +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> > >>>  {
> > >>>      char *prop_name;
> > >>>      int i;
> > >>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > >>> index e6135c34d6..497f2b7ab7 100644
> > >>> --- a/include/hw/i386/pc.h
> > >>> +++ b/include/hw/i386/pc.h
> > >>> @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
> > >>>
> > >>>  /* pc_sysfw.c */
> > >>>  void pc_system_flash_create(PCMachineState *pcms);
> > >>> +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
> > >>>  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
> > >>>
> > >>>  /* acpi-build.c */
> > >>>
> > >
> > >
>
>

Philippe Mathieu-Daudé July 1, 2020, 12:40 p.m. UTC | #10

On 7/1/20 2:25 PM, Jason Andryuk wrote:
> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant <[e-mail]> wrote:
>>
>>> -----Original Message-----
>>> From: Philippe Mathieu-Daudé <[e-mail]>
>>> Sent: 30 June 2020 18:27
>>> To: [e-mail]; [e-mail]; [e-mail]
>>> Cc: 'Eduardo Habkost' <[e-mail]>; 'Michael S. Tsirkin' <[e-mail]>; 'Paul Durrant'
>>> <[e-mail]>; 'Jason Andryuk' <[e-mail]>; 'Paolo Bonzini' <[e-mail]>;
>>> 'Richard Henderson' <[e-mail]>
>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>
>>> On 6/30/20 5:44 PM, Paul Durrant wrote:
>>>>> -----Original Message-----
>>>>> From: Philippe Mathieu-Daudé <[e-mail]>
>>>>> Sent: 30 June 2020 16:26
>>>>> To: Paul Durrant <[e-mail]>; [e-mail]; [e-mail]
>>>>> Cc: Eduardo Habkost <[e-mail]>; Michael S. Tsirkin <[e-mail]>; Paul Durrant
>>>>> <[e-mail]>; Jason Andryuk <[e-mail]>; Paolo Bonzini <[e-mail]>;
>>>>> Richard Henderson <[e-mail]>
>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>>>
>>>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>>>>> From: Paul Durrant <[e-mail]>
>>>>>>
>>>>>> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
>>>>>> 'system.flash0' and 'system.flash1' devices. These devices are then realized
>>>>>> by pc_system_flash_map() which is called from pc_system_firmware_init() which
>>>>>> itself is called via pc_memory_init(). The latter however is not called when
>>>>>> xen_enable() is true and hence the following assertion fails:
>>>>>>
>>>>>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
>>>>>> Assertion `dev->realized' failed
>>>>>>
>>>>>> These flash devices are unneeded when using Xen so this patch avoids the
>>>>>> assertion by simply removing them using pc_system_flash_cleanup_unused().
>>>>>>
>>>>>> Reported-by: Jason Andryuk <[e-mail]>
>>>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>>>>>> Signed-off-by: Paul Durrant <[e-mail]>
>>>>>> Tested-by: Jason Andryuk <[e-mail]>
>>>>>> ---
>>>>>> Cc: Paolo Bonzini <[e-mail]>
>>>>>> Cc: Richard Henderson <[e-mail]>
>>>>>> Cc: Eduardo Habkost <[e-mail]>
>>>>>> Cc: "Michael S. Tsirkin" <[e-mail]>
>>>>>> Cc: Marcel Apfelbaum <[e-mail]>
>>>>>> ---
>>>>>>  hw/i386/pc_piix.c    | 9 ++++++---
>>>>>>  hw/i386/pc_sysfw.c   | 2 +-
>>>>>>  include/hw/i386/pc.h | 1 +
>>>>>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>>>> index 1497d0e4ae..977d40afb8 100644
>>>>>> --- a/hw/i386/pc_piix.c
>>>>>> +++ b/hw/i386/pc_piix.c
>>>>>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>>>>>>      if (!xen_enabled()) {
>>>>>>          pc_memory_init(pcms, system_memory,
>>>>>>                         rom_memory, &ram_memory);
>>>>>> -    } else if (machine->kernel_filename != NULL) {
>>>>>> -        /* For xen HVM direct kernel boot, load linux here */
>>>>>> -        xen_load_linux(pcms);
>>>>>> +    } else {
>>>>>> +        pc_system_flash_cleanup_unused(pcms);
>>>>>
>>>>> TIL pc_system_flash_cleanup_unused().
>>>>>
>>>>> What about restricting at the source?
>>>>>
>>>>
>>>> And leave the devices in place? They are not relevant for Xen, so why not clean up?
>>>
>>> No, I meant to not create them in the first place, instead of
>>> create+destroy.
>>>
>>> Anyway what you did works, so I don't have any problem.
>>
>> IIUC Jason originally tried restricting creation but encountered a problem because xen_enabled() would always return false at that point, because machine creation occurs before accelerators are initialized.
> 
> Correct.  Quoting my previous email:
> """
> Removing the call to pc_system_flash_create() from pc_machine_initfn()
> lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
> there since accelerators have not been initialized yes, I guess?

Ah, I missed that. You pointed at the bug here :)

I think pc_system_flash_create() shouldn't be called in init() but
realize().

> """
> 
> If you want to remove the creation in the first place, then I have two
> questions.  Why does pc_system_flash_create()/pc_pflash_create() get
> called so early creating the pflash devices?  Why aren't they just
> created as needed in pc_system_flash_map()?
> 
> Regards,
> Jason
> 
>>   Paul
>>
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>>>
>>>>
>>>>   Paul
>>>>
>>>>> -- >8 --
>>>>> --- a/hw/i386/pc.c
>>>>> +++ b/hw/i386/pc.c
>>>>> @@ -1004,24 +1004,26 @@ void pc_memory_init(PCMachineState *pcms,
>>>>>                                      &machine->device_memory->mr);
>>>>>      }
>>>>>
>>>>> -    /* Initialize PC system firmware */
>>>>> -    pc_system_firmware_init(pcms, rom_memory);
>>>>> -
>>>>> -    option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>>>>> -    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
>>>>> -                           &error_fatal);
>>>>> -    if (pcmc->pci_enabled) {
>>>>> -        memory_region_set_readonly(option_rom_mr, true);
>>>>> -    }
>>>>> -    memory_region_add_subregion_overlap(rom_memory,
>>>>> -                                        PC_ROM_MIN_VGA,
>>>>> -                                        option_rom_mr,
>>>>> -                                        1);
>>>>> -
>>>>>      fw_cfg = fw_cfg_arch_create(machine,
>>>>>                                  x86ms->boot_cpus, x86ms->apic_id_limit);
>>>>>
>>>>> -    rom_set_fw(fw_cfg);
>>>>> +    /* Initialize PC system firmware */
>>>>> +    if (!xen_enabled()) {
>>>>> +        pc_system_firmware_init(pcms, rom_memory);
>>>>> +
>>>>> +        option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>>>>> +        memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
>>>>> +                               &error_fatal);
>>>>> +        if (pcmc->pci_enabled) {
>>>>> +            memory_region_set_readonly(option_rom_mr, true);
>>>>> +        }
>>>>> +        memory_region_add_subregion_overlap(rom_memory,
>>>>> +                                            PC_ROM_MIN_VGA,
>>>>> +                                            option_rom_mr,
>>>>> +                                            1);
>>>>> +
>>>>> +        rom_set_fw(fw_cfg);
>>>>> +    }
>>>>>
>>>>>      if (pcmc->has_reserved_memory && machine->device_memory->base) {
>>>>>          uint64_t *val = g_malloc(sizeof(*val));
>>>>> ---
>>>>>
>>>>>> +        if (machine->kernel_filename != NULL) {
>>>>>> +            /* For xen HVM direct kernel boot, load linux here */
>>>>>> +            xen_load_linux(pcms);
>>>>>> +        }
>>>>>>      }
>>>>>>
>>>>>>      gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
>>>>>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>>>>>> index ec2a3b3e7e..0ff47a4b59 100644
>>>>>> --- a/hw/i386/pc_sysfw.c
>>>>>> +++ b/hw/i386/pc_sysfw.c
>>>>>> @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
>>>>>>      }
>>>>>>  }
>>>>>>
>>>>>> -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
>>>>>> +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
>>>>>>  {
>>>>>>      char *prop_name;
>>>>>>      int i;
>>>>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>>>>> index e6135c34d6..497f2b7ab7 100644
>>>>>> --- a/include/hw/i386/pc.h
>>>>>> +++ b/include/hw/i386/pc.h
>>>>>> @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
>>>>>>
>>>>>>  /* pc_sysfw.c */
>>>>>>  void pc_system_flash_create(PCMachineState *pcms);
>>>>>> +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
>>>>>>  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>>>>>>
>>>>>>  /* acpi-build.c */
>>>>>>
>>>>
>>>>
>>
>>
>

Philippe Mathieu-Daudé July 1, 2020, 12:55 p.m. UTC | #11

On 7/1/20 2:40 PM, Philippe Mathieu-Daudé wrote:
> On 7/1/20 2:25 PM, Jason Andryuk wrote:
>> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant <[e-mail]> wrote:
>>>
>>>> -----Original Message-----
>>>> From: Philippe Mathieu-Daudé <[e-mail]>
>>>> Sent: 30 June 2020 18:27
>>>> To: [e-mail]; [e-mail]; [e-mail]
>>>> Cc: 'Eduardo Habkost' <[e-mail]>; 'Michael S. Tsirkin' <[e-mail]>; 'Paul Durrant'
>>>> <[e-mail]>; 'Jason Andryuk' <[e-mail]>; 'Paolo Bonzini' <[e-mail]>;
>>>> 'Richard Henderson' <[e-mail]>
>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>>
>>>> On 6/30/20 5:44 PM, Paul Durrant wrote:
>>>>>> -----Original Message-----
>>>>>> From: Philippe Mathieu-Daudé <[e-mail]>
>>>>>> Sent: 30 June 2020 16:26
>>>>>> To: Paul Durrant <[e-mail]>; [e-mail]; [e-mail]
>>>>>> Cc: Eduardo Habkost <[e-mail]>; Michael S. Tsirkin <[e-mail]>; Paul Durrant
>>>>>> <[e-mail]>; Jason Andryuk <[e-mail]>; Paolo Bonzini <[e-mail]>;
>>>>>> Richard Henderson <[e-mail]>
>>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>>>>
>>>>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>>>>>> From: Paul Durrant <[e-mail]>
>>>>>>>
>>>>>>> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
>>>>>>> 'system.flash0' and 'system.flash1' devices. These devices are then realized
>>>>>>> by pc_system_flash_map() which is called from pc_system_firmware_init() which
>>>>>>> itself is called via pc_memory_init(). The latter however is not called when
>>>>>>> xen_enable() is true and hence the following assertion fails:
>>>>>>>
>>>>>>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
>>>>>>> Assertion `dev->realized' failed
>>>>>>>
>>>>>>> These flash devices are unneeded when using Xen so this patch avoids the
>>>>>>> assertion by simply removing them using pc_system_flash_cleanup_unused().
>>>>>>>
>>>>>>> Reported-by: Jason Andryuk <[e-mail]>
>>>>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>>>>>>> Signed-off-by: Paul Durrant <[e-mail]>
>>>>>>> Tested-by: Jason Andryuk <[e-mail]>
>>>>>>> ---
>>>>>>> Cc: Paolo Bonzini <[e-mail]>
>>>>>>> Cc: Richard Henderson <[e-mail]>
>>>>>>> Cc: Eduardo Habkost <[e-mail]>
>>>>>>> Cc: "Michael S. Tsirkin" <[e-mail]>
>>>>>>> Cc: Marcel Apfelbaum <[e-mail]>
>>>>>>> ---
>>>>>>>  hw/i386/pc_piix.c    | 9 ++++++---
>>>>>>>  hw/i386/pc_sysfw.c   | 2 +-
>>>>>>>  include/hw/i386/pc.h | 1 +
>>>>>>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>>>>> index 1497d0e4ae..977d40afb8 100644
>>>>>>> --- a/hw/i386/pc_piix.c
>>>>>>> +++ b/hw/i386/pc_piix.c
>>>>>>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>>>>>>>      if (!xen_enabled()) {
>>>>>>>          pc_memory_init(pcms, system_memory,
>>>>>>>                         rom_memory, &ram_memory);
>>>>>>> -    } else if (machine->kernel_filename != NULL) {
>>>>>>> -        /* For xen HVM direct kernel boot, load linux here */
>>>>>>> -        xen_load_linux(pcms);
>>>>>>> +    } else {
>>>>>>> +        pc_system_flash_cleanup_unused(pcms);
>>>>>>
>>>>>> TIL pc_system_flash_cleanup_unused().
>>>>>>
>>>>>> What about restricting at the source?
>>>>>>
>>>>>
>>>>> And leave the devices in place? They are not relevant for Xen, so why not clean up?
>>>>
>>>> No, I meant to not create them in the first place, instead of
>>>> create+destroy.
>>>>
>>>> Anyway what you did works, so I don't have any problem.
>>>
>>> IIUC Jason originally tried restricting creation but encountered a problem because xen_enabled() would always return false at that point, because machine creation occurs before accelerators are initialized.
>>
>> Correct.  Quoting my previous email:
>> """
>> Removing the call to pc_system_flash_create() from pc_machine_initfn()
>> lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
>> there since accelerators have not been initialized yes, I guess?
> 
> Ah, I missed that. You pointed at the bug here :)
> 
> I think pc_system_flash_create() shouldn't be called in init() but
> realize().

Hmm this is a MachineClass, not qdev, so no realize().

In softmmu/vl.c we have:

4152     configure_accelerators(argv[0]);
....
4327     if (!xen_enabled()) { // so xen_enable() is working
4328         /* On 32-bit hosts, QEMU is limited by virtual address space */
4329         if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
4330             error_report("at most 2047 MB RAM can be simulated");
4331             exit(1);
4332         }
4333     }
....
4348     machine_run_board_init(current_machine);

which calls in hw/core/machine.c:

1089 void machine_run_board_init(MachineState *machine)
1090 {
1091     MachineClass *machine_class = MACHINE_GET_CLASS(machine);
....
1138     machine_class->init(machine);
1139 }

         -> pc_machine_class_init()

This should come after:

         -> pc_machine_initfn()

            -> pc_system_flash_create(pcms)
> 
>> """
>>
>> If you want to remove the creation in the first place, then I have two
>> questions.  Why does pc_system_flash_create()/pc_pflash_create() get
>> called so early creating the pflash devices?  Why aren't they just
>> created as needed in pc_system_flash_map()?
>>
>> Regards,
>> Jason
>>
>>>   Paul
>>>
>>>>
>>>> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>>>>
>>>>>
>>>>>   Paul
>>>>>
>>>>>> -- >8 --
>>>>>> --- a/hw/i386/pc.c
>>>>>> +++ b/hw/i386/pc.c
>>>>>> @@ -1004,24 +1004,26 @@ void pc_memory_init(PCMachineState *pcms,
>>>>>>                                      &machine->device_memory->mr);
>>>>>>      }
>>>>>>
>>>>>> -    /* Initialize PC system firmware */
>>>>>> -    pc_system_firmware_init(pcms, rom_memory);
>>>>>> -
>>>>>> -    option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>>>>>> -    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
>>>>>> -                           &error_fatal);
>>>>>> -    if (pcmc->pci_enabled) {
>>>>>> -        memory_region_set_readonly(option_rom_mr, true);
>>>>>> -    }
>>>>>> -    memory_region_add_subregion_overlap(rom_memory,
>>>>>> -                                        PC_ROM_MIN_VGA,
>>>>>> -                                        option_rom_mr,
>>>>>> -                                        1);
>>>>>> -
>>>>>>      fw_cfg = fw_cfg_arch_create(machine,
>>>>>>                                  x86ms->boot_cpus, x86ms->apic_id_limit);
>>>>>>
>>>>>> -    rom_set_fw(fw_cfg);
>>>>>> +    /* Initialize PC system firmware */
>>>>>> +    if (!xen_enabled()) {
>>>>>> +        pc_system_firmware_init(pcms, rom_memory);
>>>>>> +
>>>>>> +        option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>>>>>> +        memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
>>>>>> +                               &error_fatal);
>>>>>> +        if (pcmc->pci_enabled) {
>>>>>> +            memory_region_set_readonly(option_rom_mr, true);
>>>>>> +        }
>>>>>> +        memory_region_add_subregion_overlap(rom_memory,
>>>>>> +                                            PC_ROM_MIN_VGA,
>>>>>> +                                            option_rom_mr,
>>>>>> +                                            1);
>>>>>> +
>>>>>> +        rom_set_fw(fw_cfg);
>>>>>> +    }
>>>>>>
>>>>>>      if (pcmc->has_reserved_memory && machine->device_memory->base) {
>>>>>>          uint64_t *val = g_malloc(sizeof(*val));
>>>>>> ---
>>>>>>
>>>>>>> +        if (machine->kernel_filename != NULL) {
>>>>>>> +            /* For xen HVM direct kernel boot, load linux here */
>>>>>>> +            xen_load_linux(pcms);
>>>>>>> +        }
>>>>>>>      }
>>>>>>>
>>>>>>>      gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
>>>>>>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>>>>>>> index ec2a3b3e7e..0ff47a4b59 100644
>>>>>>> --- a/hw/i386/pc_sysfw.c
>>>>>>> +++ b/hw/i386/pc_sysfw.c
>>>>>>> @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
>>>>>>>      }
>>>>>>>  }
>>>>>>>
>>>>>>> -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
>>>>>>> +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
>>>>>>>  {
>>>>>>>      char *prop_name;
>>>>>>>      int i;
>>>>>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>>>>>> index e6135c34d6..497f2b7ab7 100644
>>>>>>> --- a/include/hw/i386/pc.h
>>>>>>> +++ b/include/hw/i386/pc.h
>>>>>>> @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
>>>>>>>
>>>>>>>  /* pc_sysfw.c */
>>>>>>>  void pc_system_flash_create(PCMachineState *pcms);
>>>>>>> +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
>>>>>>>  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>>>>>>>
>>>>>>>  /* acpi-build.c */
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>>
>

Jason Andryuk July 1, 2020, 2:59 p.m. UTC | #12

On Wed, Jul 1, 2020 at 8:55 AM Philippe Mathieu-Daudé <[e-mail]> wrote:
>
> On 7/1/20 2:40 PM, Philippe Mathieu-Daudé wrote:
> > On 7/1/20 2:25 PM, Jason Andryuk wrote:
> >> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant <[e-mail]> wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: Philippe Mathieu-Daudé <[e-mail]>
> >>>> Sent: 30 June 2020 18:27
> >>>> To: [e-mail]; [e-mail]; [e-mail]
> >>>> Cc: 'Eduardo Habkost' <[e-mail]>; 'Michael S. Tsirkin' <[e-mail]>; 'Paul Durrant'
> >>>> <[e-mail]>; 'Jason Andryuk' <[e-mail]>; 'Paolo Bonzini' <[e-mail]>;
> >>>> 'Richard Henderson' <[e-mail]>
> >>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >>>>
> >>>> On 6/30/20 5:44 PM, Paul Durrant wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Philippe Mathieu-Daudé <[e-mail]>
> >>>>>> Sent: 30 June 2020 16:26
> >>>>>> To: Paul Durrant <[e-mail]>; [e-mail]; [e-mail]
> >>>>>> Cc: Eduardo Habkost <[e-mail]>; Michael S. Tsirkin <[e-mail]>; Paul Durrant
> >>>>>> <[e-mail]>; Jason Andryuk <[e-mail]>; Paolo Bonzini <[e-mail]>;
> >>>>>> Richard Henderson <[e-mail]>
> >>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >>>>>>
> >>>>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
> >>>>>>> From: Paul Durrant <[e-mail]>
> >>>>>>>
> >>>>>>> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
> >>>>>>> 'system.flash0' and 'system.flash1' devices. These devices are then realized
> >>>>>>> by pc_system_flash_map() which is called from pc_system_firmware_init() which
> >>>>>>> itself is called via pc_memory_init(). The latter however is not called when
> >>>>>>> xen_enable() is true and hence the following assertion fails:
> >>>>>>>
> >>>>>>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> >>>>>>> Assertion `dev->realized' failed
> >>>>>>>
> >>>>>>> These flash devices are unneeded when using Xen so this patch avoids the
> >>>>>>> assertion by simply removing them using pc_system_flash_cleanup_unused().
> >>>>>>>
> >>>>>>> Reported-by: Jason Andryuk <[e-mail]>
> >>>>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> >>>>>>> Signed-off-by: Paul Durrant <[e-mail]>
> >>>>>>> Tested-by: Jason Andryuk <[e-mail]>
> >>>>>>> ---
> >>>>>>> Cc: Paolo Bonzini <[e-mail]>
> >>>>>>> Cc: Richard Henderson <[e-mail]>
> >>>>>>> Cc: Eduardo Habkost <[e-mail]>
> >>>>>>> Cc: "Michael S. Tsirkin" <[e-mail]>
> >>>>>>> Cc: Marcel Apfelbaum <[e-mail]>
> >>>>>>> ---
> >>>>>>>  hw/i386/pc_piix.c    | 9 ++++++---
> >>>>>>>  hw/i386/pc_sysfw.c   | 2 +-
> >>>>>>>  include/hw/i386/pc.h | 1 +
> >>>>>>>  3 files changed, 8 insertions(+), 4 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>>>>>> index 1497d0e4ae..977d40afb8 100644
> >>>>>>> --- a/hw/i386/pc_piix.c
> >>>>>>> +++ b/hw/i386/pc_piix.c
> >>>>>>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> >>>>>>>      if (!xen_enabled()) {
> >>>>>>>          pc_memory_init(pcms, system_memory,
> >>>>>>>                         rom_memory, &ram_memory);
> >>>>>>> -    } else if (machine->kernel_filename != NULL) {
> >>>>>>> -        /* For xen HVM direct kernel boot, load linux here */
> >>>>>>> -        xen_load_linux(pcms);
> >>>>>>> +    } else {
> >>>>>>> +        pc_system_flash_cleanup_unused(pcms);
> >>>>>>
> >>>>>> TIL pc_system_flash_cleanup_unused().
> >>>>>>
> >>>>>> What about restricting at the source?
> >>>>>>
> >>>>>
> >>>>> And leave the devices in place? They are not relevant for Xen, so why not clean up?
> >>>>
> >>>> No, I meant to not create them in the first place, instead of
> >>>> create+destroy.
> >>>>
> >>>> Anyway what you did works, so I don't have any problem.
> >>>
> >>> IIUC Jason originally tried restricting creation but encountered a problem because xen_enabled() would always return false at that point, because machine creation occurs before accelerators are initialized.
> >>
> >> Correct.  Quoting my previous email:
> >> """
> >> Removing the call to pc_system_flash_create() from pc_machine_initfn()
> >> lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
> >> there since accelerators have not been initialized yes, I guess?
> >
> > Ah, I missed that. You pointed at the bug here :)
> >
> > I think pc_system_flash_create() shouldn't be called in init() but
> > realize().
>
> Hmm this is a MachineClass, not qdev, so no realize().
>
> In softmmu/vl.c we have:
>
> 4152     configure_accelerators(argv[0]);
> ....
> 4327     if (!xen_enabled()) { // so xen_enable() is working
> 4328         /* On 32-bit hosts, QEMU is limited by virtual address space */
> 4329         if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> 4330             error_report("at most 2047 MB RAM can be simulated");
> 4331             exit(1);
> 4332         }
> 4333     }
> ....
> 4348     machine_run_board_init(current_machine);
>
> which calls in hw/core/machine.c:
>
> 1089 void machine_run_board_init(MachineState *machine)
> 1090 {
> 1091     MachineClass *machine_class = MACHINE_GET_CLASS(machine);
> ....
> 1138     machine_class->init(machine);
> 1139 }
>
>          -> pc_machine_class_init()
>
> This should come after:
>
>          -> pc_machine_initfn()
>
>             -> pc_system_flash_create(pcms)

Sorry, I'm not following the flow you want.

The xen_enabled() call in vl.c may always fail.  Or at least in most
common Xen usage.  Xen HVMs are started with machine xenfv and don't
specify an accelerator.  You need to process the xenfv default machine
opts to process "accel=xen".  Actually, I don't know how the
accelerator initialization works, but xen_accel_class_init() needs to
be called to set `ac->allowed = &xen_allowed`.  xen_allowed is the
return value of xen_enabled() and the accelerator initialization must
toggle it.

Regards,
Jason

Philippe Mathieu-Daudé July 1, 2020, 3:10 p.m. UTC | #13

On 7/1/20 4:59 PM, Jason Andryuk wrote:
> On Wed, Jul 1, 2020 at 8:55 AM Philippe Mathieu-Daudé <[e-mail]> wrote:
>> On 7/1/20 2:40 PM, Philippe Mathieu-Daudé wrote:
>>> On 7/1/20 2:25 PM, Jason Andryuk wrote:
>>>> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant <[e-mail]> wrote:
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Philippe Mathieu-Daudé <[e-mail]>
>>>>>> Sent: 30 June 2020 18:27
>>>>>> To: [e-mail]; [e-mail]; [e-mail]
>>>>>> Cc: 'Eduardo Habkost' <[e-mail]>; 'Michael S. Tsirkin' <[e-mail]>; 'Paul Durrant'
>>>>>> <[e-mail]>; 'Jason Andryuk' <[e-mail]>; 'Paolo Bonzini' <[e-mail]>;
>>>>>> 'Richard Henderson' <[e-mail]>
>>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>>>>
>>>>>> On 6/30/20 5:44 PM, Paul Durrant wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Philippe Mathieu-Daudé <[e-mail]>
>>>>>>>> Sent: 30 June 2020 16:26
>>>>>>>> To: Paul Durrant <[e-mail]>; [e-mail]; [e-mail]
>>>>>>>> Cc: Eduardo Habkost <[e-mail]>; Michael S. Tsirkin <[e-mail]>; Paul Durrant
>>>>>>>> <[e-mail]>; Jason Andryuk <[e-mail]>; Paolo Bonzini <[e-mail]>;
>>>>>>>> Richard Henderson <[e-mail]>
>>>>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>>>>>>
>>>>>>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>>>>>>>> From: Paul Durrant <[e-mail]>
>>>>>>>>>
>>>>>>>>> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
>>>>>>>>> 'system.flash0' and 'system.flash1' devices. These devices are then realized
>>>>>>>>> by pc_system_flash_map() which is called from pc_system_firmware_init() which
>>>>>>>>> itself is called via pc_memory_init(). The latter however is not called when
>>>>>>>>> xen_enable() is true and hence the following assertion fails:
>>>>>>>>>
>>>>>>>>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
>>>>>>>>> Assertion `dev->realized' failed
>>>>>>>>>
>>>>>>>>> These flash devices are unneeded when using Xen so this patch avoids the
>>>>>>>>> assertion by simply removing them using pc_system_flash_cleanup_unused().
>>>>>>>>>
>>>>>>>>> Reported-by: Jason Andryuk <[e-mail]>
>>>>>>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>>>>>>>>> Signed-off-by: Paul Durrant <[e-mail]>
>>>>>>>>> Tested-by: Jason Andryuk <[e-mail]>
>>>>>>>>> ---
>>>>>>>>> Cc: Paolo Bonzini <[e-mail]>
>>>>>>>>> Cc: Richard Henderson <[e-mail]>
>>>>>>>>> Cc: Eduardo Habkost <[e-mail]>
>>>>>>>>> Cc: "Michael S. Tsirkin" <[e-mail]>
>>>>>>>>> Cc: Marcel Apfelbaum <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>  hw/i386/pc_piix.c    | 9 ++++++---
>>>>>>>>>  hw/i386/pc_sysfw.c   | 2 +-
>>>>>>>>>  include/hw/i386/pc.h | 1 +
>>>>>>>>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>>>>>>> index 1497d0e4ae..977d40afb8 100644
>>>>>>>>> --- a/hw/i386/pc_piix.c
>>>>>>>>> +++ b/hw/i386/pc_piix.c
>>>>>>>>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>>>>>>>>>      if (!xen_enabled()) {
>>>>>>>>>          pc_memory_init(pcms, system_memory,
>>>>>>>>>                         rom_memory, &ram_memory);
>>>>>>>>> -    } else if (machine->kernel_filename != NULL) {
>>>>>>>>> -        /* For xen HVM direct kernel boot, load linux here */
>>>>>>>>> -        xen_load_linux(pcms);
>>>>>>>>> +    } else {
>>>>>>>>> +        pc_system_flash_cleanup_unused(pcms);
>>>>>>>>
>>>>>>>> TIL pc_system_flash_cleanup_unused().
>>>>>>>>
>>>>>>>> What about restricting at the source?
>>>>>>>>
>>>>>>>
>>>>>>> And leave the devices in place? They are not relevant for Xen, so why not clean up?
>>>>>>
>>>>>> No, I meant to not create them in the first place, instead of
>>>>>> create+destroy.
>>>>>>
>>>>>> Anyway what you did works, so I don't have any problem.
>>>>>
>>>>> IIUC Jason originally tried restricting creation but encountered a problem because xen_enabled() would always return false at that point, because machine creation occurs before accelerators are initialized.
>>>>
>>>> Correct.  Quoting my previous email:
>>>> """
>>>> Removing the call to pc_system_flash_create() from pc_machine_initfn()
>>>> lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
>>>> there since accelerators have not been initialized yes, I guess?
>>>
>>> Ah, I missed that. You pointed at the bug here :)
>>>
>>> I think pc_system_flash_create() shouldn't be called in init() but
>>> realize().
>>
>> Hmm this is a MachineClass, not qdev, so no realize().
>>
>> In softmmu/vl.c we have:
>>
>> 4152     configure_accelerators(argv[0]);
>> ....
>> 4327     if (!xen_enabled()) { // so xen_enable() is working
>> 4328         /* On 32-bit hosts, QEMU is limited by virtual address space */
>> 4329         if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
>> 4330             error_report("at most 2047 MB RAM can be simulated");
>> 4331             exit(1);
>> 4332         }
>> 4333     }
>> ....
>> 4348     machine_run_board_init(current_machine);
>>
>> which calls in hw/core/machine.c:
>>
>> 1089 void machine_run_board_init(MachineState *machine)
>> 1090 {
>> 1091     MachineClass *machine_class = MACHINE_GET_CLASS(machine);
>> ....
>> 1138     machine_class->init(machine);
>> 1139 }
>>
>>          -> pc_machine_class_init()
>>
>> This should come after:
>>
>>          -> pc_machine_initfn()
>>
>>             -> pc_system_flash_create(pcms)
> 
> Sorry, I'm not following the flow you want.

Well, I was simply trying to understand what was wrong, and
what we should fix so you don't have to create flash devices
then destroy them when using Xen.

I already said Paul patch is OK and gave my R-b to it.

> 
> The xen_enabled() call in vl.c may always fail.  Or at least in most
> common Xen usage.  Xen HVMs are started with machine xenfv and don't
> specify an accelerator.  You need to process the xenfv default machine
> opts to process "accel=xen".  Actually, I don't know how the
> accelerator initialization works, but xen_accel_class_init() needs to
> be called to set `ac->allowed = &xen_allowed`.  xen_allowed is the
> return value of xen_enabled() and the accelerator initialization must
> toggle it.

Since you are happy how this current works, I'm also fine with it,
I won't investigate further.

Regards,

Phil.

Markus Armbruster July 2, 2020, 3:55 a.m. UTC | #14

Jason Andryuk <[e-mail]> writes:

> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant <[e-mail]> wrote:
>>
>> > -----Original Message-----
>> > From: Philippe Mathieu-Daudé <[e-mail]>
>> > Sent: 30 June 2020 18:27
>> > To: [e-mail]; [e-mail]; [e-mail]
>> > Cc: 'Eduardo Habkost' <[e-mail]>; 'Michael S. Tsirkin' <[e-mail]>; 'Paul Durrant'
>> > <[e-mail]>; 'Jason Andryuk' <[e-mail]>; 'Paolo Bonzini' <[e-mail]>;
>> > 'Richard Henderson' <[e-mail]>
>> > Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>> >
>> > On 6/30/20 5:44 PM, Paul Durrant wrote:
>> > >> -----Original Message-----
>> > >> From: Philippe Mathieu-Daudé <[e-mail]>
>> > >> Sent: 30 June 2020 16:26
>> > >> To: Paul Durrant <[e-mail]>; [e-mail]; [e-mail]
>> > >> Cc: Eduardo Habkost <[e-mail]>; Michael S. Tsirkin <[e-mail]>; Paul Durrant
>> > >> <[e-mail]>; Jason Andryuk <[e-mail]>; Paolo Bonzini <[e-mail]>;
>> > >> Richard Henderson <[e-mail]>
>> > >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>> > >>
>> > >> On 6/24/20 2:18 PM, Paul Durrant wrote:
>> > >>> From: Paul Durrant <[e-mail]>
>> > >>>
>> > >>> The generic pc_machine_initfn() calls pc_system_flash_create() which creates
>> > >>> 'system.flash0' and 'system.flash1' devices. These devices are then realized
>> > >>> by pc_system_flash_map() which is called from pc_system_firmware_init() which
>> > >>> itself is called via pc_memory_init(). The latter however is not called when
>> > >>> xen_enable() is true and hence the following assertion fails:
>> > >>>
>> > >>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
>> > >>> Assertion `dev->realized' failed
>> > >>>
>> > >>> These flash devices are unneeded when using Xen so this patch avoids the
>> > >>> assertion by simply removing them using pc_system_flash_cleanup_unused().
>> > >>>
>> > >>> Reported-by: Jason Andryuk <[e-mail]>
>> > >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>> > >>> Signed-off-by: Paul Durrant <[e-mail]>
>> > >>> Tested-by: Jason Andryuk <[e-mail]>
>> > >>> ---
>> > >>> Cc: Paolo Bonzini <[e-mail]>
>> > >>> Cc: Richard Henderson <[e-mail]>
>> > >>> Cc: Eduardo Habkost <[e-mail]>
>> > >>> Cc: "Michael S. Tsirkin" <[e-mail]>
>> > >>> Cc: Marcel Apfelbaum <[e-mail]>
>> > >>> ---
>> > >>>  hw/i386/pc_piix.c    | 9 ++++++---
>> > >>>  hw/i386/pc_sysfw.c   | 2 +-
>> > >>>  include/hw/i386/pc.h | 1 +
>> > >>>  3 files changed, 8 insertions(+), 4 deletions(-)
>> > >>>
>> > >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> > >>> index 1497d0e4ae..977d40afb8 100644
>> > >>> --- a/hw/i386/pc_piix.c
>> > >>> +++ b/hw/i386/pc_piix.c
>> > >>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>> > >>>      if (!xen_enabled()) {
>> > >>>          pc_memory_init(pcms, system_memory,
>> > >>>                         rom_memory, &ram_memory);
>> > >>> -    } else if (machine->kernel_filename != NULL) {
>> > >>> -        /* For xen HVM direct kernel boot, load linux here */
>> > >>> -        xen_load_linux(pcms);
>> > >>> +    } else {
>> > >>> +        pc_system_flash_cleanup_unused(pcms);
>> > >>
>> > >> TIL pc_system_flash_cleanup_unused().
>> > >>
>> > >> What about restricting at the source?
>> > >>
>> > >
>> > > And leave the devices in place? They are not relevant for Xen, so why not clean up?
>> >
>> > No, I meant to not create them in the first place, instead of
>> > create+destroy.
>> >
>> > Anyway what you did works, so I don't have any problem.
>>
>> IIUC Jason originally tried restricting creation but encountered a problem because xen_enabled() would always return false at that point, because machine creation occurs before accelerators are initialized.
>
> Correct.  Quoting my previous email:
> """
> Removing the call to pc_system_flash_create() from pc_machine_initfn()
> lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
> there since accelerators have not been initialized yes, I guess?
> """
>
> If you want to remove the creation in the first place, then I have two
> questions.  Why does pc_system_flash_create()/pc_pflash_create() get
> called so early creating the pflash devices?  Why aren't they just
> created as needed in pc_system_flash_map()?

commit ebc29e1beab02646702c8cb9a1d29b68f72ad503

    pc: Support firmware configuration with -blockdev

    [...]

    Properties need to be created in .instance_init() methods.  For PC
    machines, that's pc_machine_initfn().  To make alias properties work,
    we need to create the onboard flash devices there, too.  [...]

For context, read the entire commit message.  If you have questions
then, don't hesitate to ask them here.

Patch

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1497d0e4ae..977d40afb8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -186,9 +186,12 @@  static void pc_init1(MachineState *machine,
     if (!xen_enabled()) {
         pc_memory_init(pcms, system_memory,
                        rom_memory, &ram_memory);
-    } else if (machine->kernel_filename != NULL) {
-        /* For xen HVM direct kernel boot, load linux here */
-        xen_load_linux(pcms);
+    } else {
+        pc_system_flash_cleanup_unused(pcms);
+        if (machine->kernel_filename != NULL) {
+            /* For xen HVM direct kernel boot, load linux here */
+            xen_load_linux(pcms);
+        }
     }
 
     gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index ec2a3b3e7e..0ff47a4b59 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -108,7 +108,7 @@  void pc_system_flash_create(PCMachineState *pcms)
     }
 }
 
-static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
+void pc_system_flash_cleanup_unused(PCMachineState *pcms)
 {
     char *prop_name;
     int i;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index e6135c34d6..497f2b7ab7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -187,6 +187,7 @@  int cmos_get_fd_drive_type(FloppyDriveType fd0);
 
 /* pc_sysfw.c */
 void pc_system_flash_create(PCMachineState *pcms);
+void pc_system_flash_cleanup_unused(PCMachineState *pcms);
 void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 
 /* acpi-build.c */