qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Andreas Färber" <afaerber@suse.de>
To: James Hogan <james.hogan@imgtec.com>, qemu-devel@nongnu.org
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [Qemu-devel] [PATCH] hw/mips: malta: Don't boot from flash with KVM T&E
Date: Fri, 20 Jun 2014 15:17:42 +0200	[thread overview]
Message-ID: <53A43476.7060901@suse.de> (raw)
In-Reply-To: <1403264879-13247-1-git-send-email-james.hogan@imgtec.com>

Hi,

Am 20.06.2014 13:47, schrieb James Hogan:
> In KVM trap & emulate (T&E) mode the flash reset region at 0xbfc00000
> isn't executable, which is why the minimal kernel bootloader is loaded
> and executed from the last 1MB of DRAM instead.
> 
> Therefore if no kernel is provided on the command line and KVM is
> enabled, exit with an error since booting from flash will fail.
> 
> Reported-by: Aurelien Jarno <aurelien@aurel32.net>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/mips/mips_malta.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Previous commits have used just "mips_malta:", so suggest to do the same
for consistency - or clean hw/mips/ dir up by renaming the file to just
malta.c and use "mips/malta:".

http://git.qemu-project.org/?p=qemu.git;a=history;f=hw/mips/mips_malta.c;h=f4a7d47129526f6762eccc47251c8cf3cd27f928;hb=HEAD

That said,

> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 8bc5392b4223..91b0ce566111 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1052,6 +1052,12 @@ void mips_malta_init(MachineState *machine)
>                               bootloader_run_addr, kernel_entry);
>          }
>      } else {
> +        /* The flash region isn't executable from a KVM T&E guest */
> +        if (kvm_enabled()) {
> +            error_report("KVM enabled but no -kernel argument was specified. "
> +                         "Booting from flash is not supported with KVM T&E.");
> +            exit(1);
> +        }
>          /* Load firmware from flash. */
>          if (!dinfo) {
>              /* Load a BIOS image. */

Reviewed-by: Andreas Färber <afaerber@suse.de>

Are users expected to know/understand "KVM T&E"? You don't test for that
T&E mode here, so you could either shorten the message to just "KVM" or
replace the check with a more specific one.

Since qtest does not use KVM on MIPS today, exiting without -kernel
should be fine, but may need updating if we ever get to that.

Regards,
Andreas

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

  reply	other threads:[~2014-06-20 13:17 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-06-20 11:47 [Qemu-devel] [PATCH] hw/mips: malta: Don't boot from flash with KVM T&E James Hogan
2014-06-20 13:17 ` Andreas Färber [this message]
2014-06-20 13:35   ` James Hogan

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=53A43476.7060901@suse.de \
    --to=afaerber@suse.de \
    --cc=aurelien@aurel32.net \
    --cc=james.hogan@imgtec.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).