On Wed, Jul 1, 2020 at 8:55 AM Philippe Mathieu-Daudé <[email protected]> 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 <[email protected]> wrote: > >>> > >>>> -----Original Message----- > >>>> From: Philippe Mathieu-Daudé <[email protected]> > >>>> Sent: 30 June 2020 18:27 > >>>> To: [email protected]; [email protected]; [email protected] > >>>> Cc: 'Eduardo Habkost' <[email protected]>; 'Michael S. Tsirkin' > >>>> <[email protected]>; 'Paul Durrant' > >>>> <[email protected]>; 'Jason Andryuk' <[email protected]>; 'Paolo > >>>> Bonzini' <[email protected]>; > >>>> 'Richard Henderson' <[email protected]> > >>>> 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é <[email protected]> > >>>>>> Sent: 30 June 2020 16:26 > >>>>>> To: Paul Durrant <[email protected]>; [email protected]; > >>>>>> [email protected] > >>>>>> Cc: Eduardo Habkost <[email protected]>; Michael S. Tsirkin > >>>>>> <[email protected]>; Paul Durrant > >>>>>> <[email protected]>; Jason Andryuk <[email protected]>; Paolo > >>>>>> Bonzini <[email protected]>; > >>>>>> Richard Henderson <[email protected]> > >>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices > >>>>>> > >>>>>> On 6/24/20 2:18 PM, Paul Durrant wrote: > >>>>>>> From: Paul Durrant <[email protected]> > >>>>>>> > >>>>>>> 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 <[email protected]> > >>>>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with > >>>>>>> -blockdev") > >>>>>>> Signed-off-by: Paul Durrant <[email protected]> > >>>>>>> Tested-by: Jason Andryuk <[email protected]> > >>>>>>> --- > >>>>>>> Cc: Paolo Bonzini <[email protected]> > >>>>>>> Cc: Richard Henderson <[email protected]> > >>>>>>> Cc: Eduardo Habkost <[email protected]> > >>>>>>> Cc: "Michael S. Tsirkin" <[email protected]> > >>>>>>> Cc: Marcel Apfelbaum <[email protected]> > >>>>>>> --- > >>>>>>> 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
