Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Jason Andryuk <jandryuk@gmail.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Paul Durrant <pdurrant@amazon.com>, Paul Durrant <paul@xen.org>,
	QEMU <qemu-devel@nongnu.org>, Paolo Bonzini <pbonzini@redhat.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
Date: Wed, 1 Jul 2020 10:59:56 -0400
Message-ID: <CAKf6xpuiQBhvChwfikaLd4tvKVUn3oo88wbsMVw7P11ehV-EYQ@mail.gmail.com> (raw)
In-Reply-To: <5c00f6a5-3f86-258e-999f-956eef825d14@redhat.com>

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


  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2020-07-01 15:10                   ` Philippe Mathieu-Daudé
2020-07-02  3:55             ` Markus Armbruster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKf6xpuiQBhvChwfikaLd4tvKVUn3oo88wbsMVw7P11ehV-EYQ@mail.gmail.com \
    --to=jandryuk@gmail.com \
    --cc=ehabkost@redhat.com \
    --cc=mst@redhat.com \
    --cc=paul@xen.org \
    --cc=pbonzini@redhat.com \
    --cc=pdurrant@amazon.com \
    --cc=philmd@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=rth@twiddle.net \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git