xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] fix assertion failures when using Xen
@ 2020-06-24 12:18 Paul Durrant
  2020-06-24 12:18 ` [PATCH 1/2] xen: fix legacy 'xen-sysdev' and 'xen-backend' bus types Paul Durrant
  2020-06-24 12:18 ` [PATCH 2/2] xen: cleanup unrealized flash devices Paul Durrant
  0 siblings, 2 replies; 18+ messages in thread
From: Paul Durrant @ 2020-06-24 12:18 UTC (permalink / raw)
  To: xen-devel, qemu-devel; +Cc: Paul Durrant

From: Paul Durrant <pdurrant@amazon.com>

Paul Durrant (2):
  xen: fix legacy 'xen-sysdev' and 'xen-backend' bus types
  xen: cleanup unrealized flash devices

 hw/i386/pc_piix.c           | 9 ++++++---
 hw/i386/pc_sysfw.c          | 2 +-
 hw/xen/xen-legacy-backend.c | 4 ++--
 include/hw/i386/pc.h        | 1 +
 4 files changed, 10 insertions(+), 6 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/2] xen: fix legacy 'xen-sysdev' and 'xen-backend' bus types
  2020-06-24 12:18 [PATCH 0/2] fix assertion failures when using Xen Paul Durrant
@ 2020-06-24 12:18 ` Paul Durrant
  2020-06-30 15:19   ` Philippe Mathieu-Daudé
  2020-06-24 12:18 ` [PATCH 2/2] xen: cleanup unrealized flash devices Paul Durrant
  1 sibling, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2020-06-24 12:18 UTC (permalink / raw)
  To: xen-devel, qemu-devel
  Cc: Anthony Perard, Paul Durrant, Stefano Stabellini, Jason Andryuk

From: Paul Durrant <pdurrant@amazon.com>

'xen-sysdev' plugs into the 'System' bus type, not 'xen-sysbus. That bus type
is what 'xen-backend' plugs into.
'xen-sysdev' is drived form 'sys-bus-device' so the bus type need not be
overridden. 'xen-backend' is derived from 'device', which plugs into the
generic 'bus' type, so its bus type should be overridden to 'xen-sysbus'.

Without this patch, the following assertion will fail:

qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
`dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
failed.

Reported-by: Jason Andryuk <jandryuk@gmail.com>
Fixes: 81cb05732efb ("qdev: Assert devices are plugged into a bus that can take them")
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
 hw/xen/xen-legacy-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 2335ee2e65..c5c75c0064 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -789,11 +789,12 @@ static void xendev_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     /* xen-backend devices can be plugged/unplugged dynamically */
     dc->user_creatable = true;
+    dc->bus_type = TYPE_XENSYSBUS;
 }
 
 static const TypeInfo xendev_type_info = {
     .name          = TYPE_XENBACKEND,
-    .parent        = TYPE_XENSYSDEV,
+    .parent        = TYPE_DEVICE,
     .class_init    = xendev_class_init,
     .instance_size = sizeof(struct XenLegacyDevice),
 };
@@ -824,7 +825,6 @@ static void xen_sysdev_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     device_class_set_props(dc, xen_sysdev_properties);
-    dc->bus_type = TYPE_XENSYSBUS;
 }
 
 static const TypeInfo xensysdev_info = {
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-06-24 12:18 [PATCH 0/2] fix assertion failures when using Xen Paul Durrant
  2020-06-24 12:18 ` [PATCH 1/2] xen: fix legacy 'xen-sysdev' and 'xen-backend' bus types Paul Durrant
@ 2020-06-24 12:18 ` Paul Durrant
  2020-06-30 15:08   ` Anthony PERARD
  2020-06-30 15:25   ` Philippe Mathieu-Daudé
  1 sibling, 2 replies; 18+ messages in thread
From: Paul Durrant @ 2020-06-24 12:18 UTC (permalink / raw)
  To: xen-devel, qemu-devel
  Cc: Eduardo Habkost, Michael S. Tsirkin, Paul Durrant, Jason Andryuk,
	Marcel Apfelbaum, Paolo Bonzini, Richard Henderson

From: Paul Durrant <pdurrant@amazon.com>

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 <jandryuk@gmail.com>
Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Tested-by: Jason Andryuk <jandryuk@gmail.com>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
---
 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);
+        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 */
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-06-24 12:18 ` [PATCH 2/2] xen: cleanup unrealized flash devices Paul Durrant
@ 2020-06-30 15:08   ` Anthony PERARD
  2020-06-30 15:17     ` Paul Durrant
  2020-07-01  5:56     ` Markus Armbruster
  2020-06-30 15:25   ` Philippe Mathieu-Daudé
  1 sibling, 2 replies; 18+ messages in thread
From: Anthony PERARD @ 2020-06-30 15:08 UTC (permalink / raw)
  To: Paul Durrant
  Cc: Eduardo Habkost, Michael S. Tsirkin, Paul Durrant, Jason Andryuk,
	qemu-devel, Paolo Bonzini, xen-devel, Richard Henderson

On Wed, Jun 24, 2020 at 01:18:41PM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> 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 <jandryuk@gmail.com>
> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Tested-by: Jason Andryuk <jandryuk@gmail.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

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.

-- 
Anthony PERARD


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-06-30 15:08   ` Anthony PERARD
@ 2020-06-30 15:17     ` Paul Durrant
  2020-07-01  5:56     ` Markus Armbruster
  1 sibling, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2020-06-30 15:17 UTC (permalink / raw)
  To: 'Anthony PERARD'
  Cc: 'Eduardo Habkost', 'Michael S. Tsirkin',
	'Paul Durrant', 'Jason Andryuk',
	qemu-devel, 'Paolo Bonzini',
	xen-devel, 'Richard Henderson'

> -----Original Message-----
> From: Anthony PERARD <anthony.perard@citrix.com>
> Sent: 30 June 2020 16:09
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; Eduardo Habkost <ehabkost@redhat.com>;
> Michael S. Tsirkin <mst@redhat.com>; Paul Durrant <pdurrant@amazon.com>; Jason Andryuk
> <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>
> 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 <pdurrant@amazon.com>
> >
> > 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 <jandryuk@gmail.com>
> > Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > Tested-by: Jason Andryuk <jandryuk@gmail.com>
> 
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> 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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] xen: fix legacy 'xen-sysdev' and 'xen-backend' bus types
  2020-06-24 12:18 ` [PATCH 1/2] xen: fix legacy 'xen-sysdev' and 'xen-backend' bus types Paul Durrant
@ 2020-06-30 15:19   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-30 15:19 UTC (permalink / raw)
  To: Paul Durrant, xen-devel, qemu-devel
  Cc: Anthony Perard, Paul Durrant, Stefano Stabellini, Jason Andryuk

On 6/24/20 2:18 PM, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> 'xen-sysdev' plugs into the 'System' bus type, not 'xen-sysbus. That bus type
> is what 'xen-backend' plugs into.
> 'xen-sysdev' is drived form 'sys-bus-device' so the bus type need not be
> overridden. 'xen-backend' is derived from 'device', which plugs into the
> generic 'bus' type, so its bus type should be overridden to 'xen-sysbus'.
> 
> Without this patch, the following assertion will fail:
> 
> qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
> `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
> failed.
> 
> Reported-by: Jason Andryuk <jandryuk@gmail.com>
> Fixes: 81cb05732efb ("qdev: Assert devices are plugged into a bus that can take them")
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> ---
>  hw/xen/xen-legacy-backend.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index 2335ee2e65..c5c75c0064 100644
> --- a/hw/xen/xen-legacy-backend.c
> +++ b/hw/xen/xen-legacy-backend.c
> @@ -789,11 +789,12 @@ static void xendev_class_init(ObjectClass *klass, void *data)
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      /* xen-backend devices can be plugged/unplugged dynamically */
>      dc->user_creatable = true;
> +    dc->bus_type = TYPE_XENSYSBUS;
>  }
>  
>  static const TypeInfo xendev_type_info = {
>      .name          = TYPE_XENBACKEND,
> -    .parent        = TYPE_XENSYSDEV,
> +    .parent        = TYPE_DEVICE,
>      .class_init    = xendev_class_init,
>      .instance_size = sizeof(struct XenLegacyDevice),
>  };
> @@ -824,7 +825,6 @@ static void xen_sysdev_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      device_class_set_props(dc, xen_sysdev_properties);
> -    dc->bus_type = TYPE_XENSYSBUS;
>  }
>  
>  static const TypeInfo xensysdev_info = {
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-06-24 12:18 ` [PATCH 2/2] xen: cleanup unrealized flash devices Paul Durrant
  2020-06-30 15:08   ` Anthony PERARD
@ 2020-06-30 15:25   ` Philippe Mathieu-Daudé
  2020-06-30 15:44     ` Paul Durrant
  1 sibling, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-30 15:25 UTC (permalink / raw)
  To: Paul Durrant, xen-devel, qemu-devel
  Cc: Eduardo Habkost, Jason Andryuk, Paul Durrant, Michael S. Tsirkin,
	Paolo Bonzini, Richard Henderson

On 6/24/20 2:18 PM, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> 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 <jandryuk@gmail.com>
> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Tested-by: Jason Andryuk <jandryuk@gmail.com>
> ---
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> ---
>  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 */
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-06-30 15:25   ` Philippe Mathieu-Daudé
@ 2020-06-30 15:44     ` Paul Durrant
  2020-06-30 17:27       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2020-06-30 15:44 UTC (permalink / raw)
  To: 'Philippe Mathieu-Daudé', xen-devel, qemu-devel
  Cc: 'Eduardo Habkost', 'Jason Andryuk',
	'Paul Durrant', 'Michael S. Tsirkin',
	'Paolo Bonzini', 'Richard Henderson'

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> Sent: 30 June 2020 16:26
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Paul Durrant
> <pdurrant@amazon.com>; Jason Andryuk <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
> Richard Henderson <rth@twiddle.net>
> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> 
> On 6/24/20 2:18 PM, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > 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 <jandryuk@gmail.com>
> > Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > Tested-by: Jason Andryuk <jandryuk@gmail.com>
> > ---
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > ---
> >  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 */
> >




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-06-30 15:44     ` Paul Durrant
@ 2020-06-30 17:27       ` Philippe Mathieu-Daudé
  2020-07-01  5:59         ` Markus Armbruster
  2020-07-01  7:03         ` Paul Durrant
  0 siblings, 2 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-30 17:27 UTC (permalink / raw)
  To: paul, xen-devel, qemu-devel
  Cc: 'Eduardo Habkost', 'Jason Andryuk',
	'Paul Durrant', 'Michael S. Tsirkin',
	'Paolo Bonzini', 'Richard Henderson'

On 6/30/20 5:44 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Sent: 30 June 2020 16:26
>> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
>> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Paul Durrant
>> <pdurrant@amazon.com>; Jason Andryuk <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
>> Richard Henderson <rth@twiddle.net>
>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>
>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>> From: Paul Durrant <pdurrant@amazon.com>
>>>
>>> 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 <jandryuk@gmail.com>
>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>>> Tested-by: Jason Andryuk <jandryuk@gmail.com>
>>> ---
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Richard Henderson <rth@twiddle.net>
>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>>> ---
>>>  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é <philmd@redhat.com>

> 
>   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 */
>>>
> 
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-06-30 15:08   ` Anthony PERARD
  2020-06-30 15:17     ` Paul Durrant
@ 2020-07-01  5:56     ` Markus Armbruster
  1 sibling, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2020-07-01  5:56 UTC (permalink / raw)
  To: Anthony PERARD
  Cc: Eduardo Habkost, Michael S. Tsirkin, Paul Durrant, Jason Andryuk,
	qemu-devel, Paul Durrant, xen-devel, Paolo Bonzini,
	Richard Henderson

Anthony PERARD <anthony.perard@citrix.com> writes:

> On Wed, Jun 24, 2020 at 01:18:41PM +0100, Paul Durrant wrote:
>> From: Paul Durrant <pdurrant@amazon.com>
>> 
>> 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 <jandryuk@gmail.com>
>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>> Tested-by: Jason Andryuk <jandryuk@gmail.com>
>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
>
> 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.



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-06-30 17:27       ` Philippe Mathieu-Daudé
@ 2020-07-01  5:59         ` Markus Armbruster
  2020-07-01  7:03         ` Paul Durrant
  1 sibling, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2020-07-01  5:59 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: 'Eduardo Habkost', 'Jason Andryuk',
	'Paul Durrant',
	paul, qemu-devel, 'Michael S. Tsirkin',
	'Paolo Bonzini', xen-devel, 'Richard Henderson'

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 6/30/20 5:44 PM, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Sent: 30 June 2020 16:26
>>> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
>>> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Paul Durrant
>>> <pdurrant@amazon.com>; Jason Andryuk <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
>>> Richard Henderson <rth@twiddle.net>
>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>
>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>>> From: Paul Durrant <pdurrant@amazon.com>
>>>>
>>>> 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 <jandryuk@gmail.com>
>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>>>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>>>> Tested-by: Jason Andryuk <jandryuk@gmail.com>
>>>> ---
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Richard Henderson <rth@twiddle.net>
>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>>>> ---
>>>>  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 :)

[...]



^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-06-30 17:27       ` Philippe Mathieu-Daudé
  2020-07-01  5:59         ` Markus Armbruster
@ 2020-07-01  7:03         ` Paul Durrant
  2020-07-01 12:25           ` Jason Andryuk
  1 sibling, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2020-07-01  7:03 UTC (permalink / raw)
  To: 'Philippe Mathieu-Daudé', xen-devel, qemu-devel
  Cc: 'Eduardo Habkost', 'Jason Andryuk',
	'Paul Durrant', 'Michael S. Tsirkin',
	'Paolo Bonzini', 'Richard Henderson'

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> Sent: 30 June 2020 18:27
> To: paul@xen.org; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
> Cc: 'Eduardo Habkost' <ehabkost@redhat.com>; 'Michael S. Tsirkin' <mst@redhat.com>; 'Paul Durrant'
> <pdurrant@amazon.com>; 'Jason Andryuk' <jandryuk@gmail.com>; 'Paolo Bonzini' <pbonzini@redhat.com>;
> 'Richard Henderson' <rth@twiddle.net>
> 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é <philmd@redhat.com>
> >> Sent: 30 June 2020 16:26
> >> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
> >> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Paul Durrant
> >> <pdurrant@amazon.com>; Jason Andryuk <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
> >> Richard Henderson <rth@twiddle.net>
> >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >>
> >> On 6/24/20 2:18 PM, Paul Durrant wrote:
> >>> From: Paul Durrant <pdurrant@amazon.com>
> >>>
> >>> 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 <jandryuk@gmail.com>
> >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> >>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> >>> Tested-by: Jason Andryuk <jandryuk@gmail.com>
> >>> ---
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: Richard Henderson <rth@twiddle.net>
> >>> Cc: Eduardo Habkost <ehabkost@redhat.com>
> >>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> >>> ---
> >>>  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é <philmd@redhat.com>
> 
> >
> >   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 */
> >>>
> >
> >




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-07-01  7:03         ` Paul Durrant
@ 2020-07-01 12:25           ` Jason Andryuk
  2020-07-01 12:40             ` Philippe Mathieu-Daudé
  2020-07-02  3:55             ` Markus Armbruster
  0 siblings, 2 replies; 18+ messages in thread
From: Jason Andryuk @ 2020-07-01 12:25 UTC (permalink / raw)
  To: Paul Durrant
  Cc: Eduardo Habkost, Michael S. Tsirkin, Paul Durrant, QEMU,
	Paolo Bonzini, xen-devel, Philippe Mathieu-Daudé,
	Richard Henderson

On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant <xadimgnik@gmail.com> wrote:
>
> > -----Original Message-----
> > From: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Sent: 30 June 2020 18:27
> > To: paul@xen.org; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
> > Cc: 'Eduardo Habkost' <ehabkost@redhat.com>; 'Michael S. Tsirkin' <mst@redhat.com>; 'Paul Durrant'
> > <pdurrant@amazon.com>; 'Jason Andryuk' <jandryuk@gmail.com>; 'Paolo Bonzini' <pbonzini@redhat.com>;
> > 'Richard Henderson' <rth@twiddle.net>
> > 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é <philmd@redhat.com>
> > >> Sent: 30 June 2020 16:26
> > >> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
> > >> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Paul Durrant
> > >> <pdurrant@amazon.com>; Jason Andryuk <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
> > >> Richard Henderson <rth@twiddle.net>
> > >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> > >>
> > >> On 6/24/20 2:18 PM, Paul Durrant wrote:
> > >>> From: Paul Durrant <pdurrant@amazon.com>
> > >>>
> > >>> 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 <jandryuk@gmail.com>
> > >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> > >>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > >>> Tested-by: Jason Andryuk <jandryuk@gmail.com>
> > >>> ---
> > >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> > >>> Cc: Richard Henderson <rth@twiddle.net>
> > >>> Cc: Eduardo Habkost <ehabkost@redhat.com>
> > >>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > >>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > >>> ---
> > >>>  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é <philmd@redhat.com>
> >
> > >
> > >   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 */
> > >>>
> > >
> > >
>
>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-07-01 12:25           ` Jason Andryuk
@ 2020-07-01 12:40             ` Philippe Mathieu-Daudé
  2020-07-01 12:55               ` Philippe Mathieu-Daudé
  2020-07-02  3:55             ` Markus Armbruster
  1 sibling, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-01 12:40 UTC (permalink / raw)
  To: Jason Andryuk, Paul Durrant
  Cc: Eduardo Habkost, Michael S. Tsirkin, Paul Durrant, QEMU,
	Paolo Bonzini, xen-devel, Richard Henderson

On 7/1/20 2:25 PM, Jason Andryuk wrote:
> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant <xadimgnik@gmail.com> wrote:
>>
>>> -----Original Message-----
>>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Sent: 30 June 2020 18:27
>>> To: paul@xen.org; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
>>> Cc: 'Eduardo Habkost' <ehabkost@redhat.com>; 'Michael S. Tsirkin' <mst@redhat.com>; 'Paul Durrant'
>>> <pdurrant@amazon.com>; 'Jason Andryuk' <jandryuk@gmail.com>; 'Paolo Bonzini' <pbonzini@redhat.com>;
>>> 'Richard Henderson' <rth@twiddle.net>
>>> 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é <philmd@redhat.com>
>>>>> Sent: 30 June 2020 16:26
>>>>> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Paul Durrant
>>>>> <pdurrant@amazon.com>; Jason Andryuk <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
>>>>> Richard Henderson <rth@twiddle.net>
>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>>>
>>>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>>>>> From: Paul Durrant <pdurrant@amazon.com>
>>>>>>
>>>>>> 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 <jandryuk@gmail.com>
>>>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>>>>>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>>>>>> Tested-by: Jason Andryuk <jandryuk@gmail.com>
>>>>>> ---
>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>> Cc: Richard Henderson <rth@twiddle.net>
>>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>>>>>> ---
>>>>>>  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é <philmd@redhat.com>
>>>
>>>>
>>>>   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 */
>>>>>>
>>>>
>>>>
>>
>>
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-07-01 12:40             ` Philippe Mathieu-Daudé
@ 2020-07-01 12:55               ` Philippe Mathieu-Daudé
  2020-07-01 14:59                 ` Jason Andryuk
  0 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-01 12:55 UTC (permalink / raw)
  To: Jason Andryuk, Paul Durrant
  Cc: Eduardo Habkost, Michael S. Tsirkin, Paul Durrant, QEMU,
	Paolo Bonzini, xen-devel, Richard Henderson

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 <xadimgnik@gmail.com> wrote:
>>>
>>>> -----Original Message-----
>>>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> Sent: 30 June 2020 18:27
>>>> To: paul@xen.org; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
>>>> Cc: 'Eduardo Habkost' <ehabkost@redhat.com>; 'Michael S. Tsirkin' <mst@redhat.com>; 'Paul Durrant'
>>>> <pdurrant@amazon.com>; 'Jason Andryuk' <jandryuk@gmail.com>; 'Paolo Bonzini' <pbonzini@redhat.com>;
>>>> 'Richard Henderson' <rth@twiddle.net>
>>>> 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é <philmd@redhat.com>
>>>>>> Sent: 30 June 2020 16:26
>>>>>> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
>>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Paul Durrant
>>>>>> <pdurrant@amazon.com>; Jason Andryuk <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
>>>>>> Richard Henderson <rth@twiddle.net>
>>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>>>>
>>>>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>>>>>> From: Paul Durrant <pdurrant@amazon.com>
>>>>>>>
>>>>>>> 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 <jandryuk@gmail.com>
>>>>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>>>>>>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>>>>>>> Tested-by: Jason Andryuk <jandryuk@gmail.com>
>>>>>>> ---
>>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>> Cc: Richard Henderson <rth@twiddle.net>
>>>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>>>>>>> ---
>>>>>>>  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é <philmd@redhat.com>
>>>>
>>>>>
>>>>>   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 */
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>>
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-07-01 12:55               ` Philippe Mathieu-Daudé
@ 2020-07-01 14:59                 ` Jason Andryuk
  2020-07-01 15:10                   ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 18+ messages in thread
From: Jason Andryuk @ 2020-07-01 14:59 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Eduardo Habkost, Michael S. Tsirkin, Paul Durrant, Paul Durrant,
	QEMU, Paolo Bonzini, xen-devel, Richard Henderson

On Wed, Jul 1, 2020 at 8:55 AM Philippe Mathieu-Daudé <philmd@redhat.com> 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 <xadimgnik@gmail.com> wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>>> Sent: 30 June 2020 18:27
> >>>> To: paul@xen.org; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
> >>>> Cc: 'Eduardo Habkost' <ehabkost@redhat.com>; 'Michael S. Tsirkin' <mst@redhat.com>; 'Paul Durrant'
> >>>> <pdurrant@amazon.com>; 'Jason Andryuk' <jandryuk@gmail.com>; 'Paolo Bonzini' <pbonzini@redhat.com>;
> >>>> 'Richard Henderson' <rth@twiddle.net>
> >>>> 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é <philmd@redhat.com>
> >>>>>> Sent: 30 June 2020 16:26
> >>>>>> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
> >>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Paul Durrant
> >>>>>> <pdurrant@amazon.com>; Jason Andryuk <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
> >>>>>> Richard Henderson <rth@twiddle.net>
> >>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >>>>>>
> >>>>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
> >>>>>>> From: Paul Durrant <pdurrant@amazon.com>
> >>>>>>>
> >>>>>>> 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 <jandryuk@gmail.com>
> >>>>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
> >>>>>>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> >>>>>>> Tested-by: Jason Andryuk <jandryuk@gmail.com>
> >>>>>>> ---
> >>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>>>>>> Cc: Richard Henderson <rth@twiddle.net>
> >>>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
> >>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >>>>>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> >>>>>>> ---
> >>>>>>>  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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-07-01 14:59                 ` Jason Andryuk
@ 2020-07-01 15:10                   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-01 15:10 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Eduardo Habkost, Michael S. Tsirkin, Paul Durrant, Paul Durrant,
	QEMU, Paolo Bonzini, xen-devel, Richard Henderson

On 7/1/20 4:59 PM, Jason Andryuk wrote:
> On Wed, Jul 1, 2020 at 8:55 AM Philippe Mathieu-Daudé <philmd@redhat.com> 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 <xadimgnik@gmail.com> wrote:
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>>> Sent: 30 June 2020 18:27
>>>>>> To: paul@xen.org; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
>>>>>> Cc: 'Eduardo Habkost' <ehabkost@redhat.com>; 'Michael S. Tsirkin' <mst@redhat.com>; 'Paul Durrant'
>>>>>> <pdurrant@amazon.com>; 'Jason Andryuk' <jandryuk@gmail.com>; 'Paolo Bonzini' <pbonzini@redhat.com>;
>>>>>> 'Richard Henderson' <rth@twiddle.net>
>>>>>> 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é <philmd@redhat.com>
>>>>>>>> Sent: 30 June 2020 16:26
>>>>>>>> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
>>>>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Paul Durrant
>>>>>>>> <pdurrant@amazon.com>; Jason Andryuk <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
>>>>>>>> Richard Henderson <rth@twiddle.net>
>>>>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>>>>>>>>
>>>>>>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
>>>>>>>>> From: Paul Durrant <pdurrant@amazon.com>
>>>>>>>>>
>>>>>>>>> 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 <jandryuk@gmail.com>
>>>>>>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>>>>>>>>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>>>>>>>>> Tested-by: Jason Andryuk <jandryuk@gmail.com>
>>>>>>>>> ---
>>>>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>>>> Cc: Richard Henderson <rth@twiddle.net>
>>>>>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>>>>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>>>>>>>>> ---
>>>>>>>>>  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.



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] xen: cleanup unrealized flash devices
  2020-07-01 12:25           ` Jason Andryuk
  2020-07-01 12:40             ` Philippe Mathieu-Daudé
@ 2020-07-02  3:55             ` Markus Armbruster
  1 sibling, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2020-07-02  3:55 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Eduardo Habkost, Michael S. Tsirkin, Paul Durrant, Paul Durrant,
	QEMU, xen-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
	Richard Henderson

Jason Andryuk <jandryuk@gmail.com> writes:

> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant <xadimgnik@gmail.com> wrote:
>>
>> > -----Original Message-----
>> > From: Philippe Mathieu-Daudé <philmd@redhat.com>
>> > Sent: 30 June 2020 18:27
>> > To: paul@xen.org; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
>> > Cc: 'Eduardo Habkost' <ehabkost@redhat.com>; 'Michael S. Tsirkin' <mst@redhat.com>; 'Paul Durrant'
>> > <pdurrant@amazon.com>; 'Jason Andryuk' <jandryuk@gmail.com>; 'Paolo Bonzini' <pbonzini@redhat.com>;
>> > 'Richard Henderson' <rth@twiddle.net>
>> > 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é <philmd@redhat.com>
>> > >> Sent: 30 June 2020 16:26
>> > >> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
>> > >> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Paul Durrant
>> > >> <pdurrant@amazon.com>; Jason Andryuk <jandryuk@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
>> > >> Richard Henderson <rth@twiddle.net>
>> > >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
>> > >>
>> > >> On 6/24/20 2:18 PM, Paul Durrant wrote:
>> > >>> From: Paul Durrant <pdurrant@amazon.com>
>> > >>>
>> > >>> 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 <jandryuk@gmail.com>
>> > >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
>> > >>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>> > >>> Tested-by: Jason Andryuk <jandryuk@gmail.com>
>> > >>> ---
>> > >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> > >>> Cc: Richard Henderson <rth@twiddle.net>
>> > >>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> > >>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> > >>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> > >>> ---
>> > >>>  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.



^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-07-02  3:56 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-24 12:18 [PATCH 0/2] fix assertion failures when using Xen Paul Durrant
2020-06-24 12:18 ` [PATCH 1/2] xen: fix legacy 'xen-sysdev' and 'xen-backend' bus types Paul Durrant
2020-06-30 15:19   ` Philippe Mathieu-Daudé
2020-06-24 12:18 ` [PATCH 2/2] xen: cleanup unrealized flash devices Paul Durrant
2020-06-30 15:08   ` Anthony PERARD
2020-06-30 15:17     ` Paul Durrant
2020-07-01  5:56     ` Markus Armbruster
2020-06-30 15:25   ` Philippe Mathieu-Daudé
2020-06-30 15:44     ` Paul Durrant
2020-06-30 17:27       ` Philippe Mathieu-Daudé
2020-07-01  5:59         ` Markus Armbruster
2020-07-01  7:03         ` Paul Durrant
2020-07-01 12:25           ` Jason Andryuk
2020-07-01 12:40             ` Philippe Mathieu-Daudé
2020-07-01 12:55               ` Philippe Mathieu-Daudé
2020-07-01 14:59                 ` Jason Andryuk
2020-07-01 15:10                   ` Philippe Mathieu-Daudé
2020-07-02  3:55             ` Markus Armbruster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).