[Qemu-devel] [PATCH] elf: Allow loading AArch64 ELF files
Treat EM_AARCH64 as a valid value when checking the ELF's machine-type
header.
Signed-off-by: Aaron Lindsay
---
include/hw/elf_ops.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 690f9238c8..f12faa90a1 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -381,6 +381,12 @@ static int glue(load_elf, SZ)(const char *name, int fd,
goto fail;
}
break;
+case EM_AARCH64:
+if (ehdr.e_machine != EM_AARCH64) {
+ret = ELF_LOAD_WRONG_ARCH;
+goto fail;
+}
+break;
default:
if (elf_machine != ehdr.e_machine) {
ret = ELF_LOAD_WRONG_ARCH;
--
2.17.1
Re: [Qemu-devel] [Qemu-arm] [PATCH] elf: Allow loading AArch64 ELF files
On Aug 12 16:02, Peter Maydell wrote:
> On Mon, 12 Aug 2019 at 15:46, Aaron Lindsay OS via Qemu-arm
> wrote:
> >
> > Treat EM_AARCH64 as a valid value when checking the ELF's machine-type
> > header.
> >
> > Signed-off-by: Aaron Lindsay
> > ---
> > include/hw/elf_ops.h | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> > index 690f9238c8..f12faa90a1 100644
> > --- a/include/hw/elf_ops.h
> > +++ b/include/hw/elf_ops.h
> > @@ -381,6 +381,12 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> > goto fail;
> > }
> > break;
> > +case EM_AARCH64:
> > +if (ehdr.e_machine != EM_AARCH64) {
> > +ret = ELF_LOAD_WRONG_ARCH;
> > +goto fail;
> > +}
> > +break;
> > default:
> > if (elf_machine != ehdr.e_machine) {
> > ret = ELF_LOAD_WRONG_ARCH;
> > --
> > 2.17.1
>
> What problem are we trying to solve here ? If I'm reading your patch
> correctly then it makes no difference to execution, because we're
> switching on 'elf_machine', and so this extra case is saying
> "if elf_machine is EM_AARCH64, insist that ehdr.e_machine
> is also EM_AARCH64", which is exactly what the default
> case would do anyway. The only reason to add extra cases in
> this switch is to handle the situation where a target's board/loader
> code says "I can handle elf files of type X" but actually this
> implicitly means it can handle both X and Y (so for EM_X86_64 we
> need to accept both EM_X86_64 and EM_386, for EM_PPC64 we need to
> accept both EM_PPC64 and EM_PPC, and so on). We don't have a
> case like that for aarch64/arm because the boot loader code has
> to deal with the 32 bit and 64 bit code separately anyway, so
> we can pass in the correct value depending on whether the CPU
> is 32-bit or 64-bit.
>
> The code in hw/arm/boot.c passes in an elf_machine value of
> EM_AARCH64 when it wants to load an AArch64 ELF file, so
> for that the default case is OK. The code in hw/core/generic-loader.c
> passes in 0 (EM_NONE) which will be handled by the check just
> before this switch statement, so the default case should
> work there too. Presumably there's some other code path
> for ELF file loading that doesn't work that you're trying to fix?
Please disregard this patch.
I'm sorry, upon closer inspection you are obviously correct... and I
have no earthly idea what happened here. I hit the "goto fail" in the
"default" case when debugging why I couldn't load an ELF on AArch64 last
week. I was in a hurry and saw that there were other architectures in
the switch/case and threw this code in there quickly without much
thought, and after re-compiling, it worked!
...But after your email this morning, I'm completely unable to reproduce
the failure case. I must have had another local issue which was
coincidentally resolved at the same time, unbeknownst to me.
-Aaron
Re: [Qemu-devel] [PATCH v4 04/54] target/arm: remove run time semihosting checks
On Jul 31 17:06, Alex Bennée wrote:
> Now we do all our checking and use a common EXCP_SEMIHOST for
> semihosting operations we can make helper code a lot simpler.
>
> Signed-off-by: Alex Bennée
>
> ---
> v2
> - fix re-base conflicts
> - hoist EXCP_SEMIHOST check
> - comment cleanups
> ---
> target/arm/helper.c | 90 +
> 1 file changed, 18 insertions(+), 72 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b74c23a9bc0..c5b90a83d36 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8259,86 +8259,30 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>new_el, env->pc, pstate_read(env));
> }
>
> -static inline bool check_for_semihosting(CPUState *cs)
> +/*
> + * Do semihosting call and set the appropriate return value. All the
> + * permission and validity checks have been done at translate time.
> + *
> + * We only see semihosting exceptions in TCG only as they are not
> + * trapped to the hypervisor in KVM.
> + */
> +static void handle_semihosting(CPUState *cs)
> {
> #ifdef CONFIG_TCG
> -/* Check whether this exception is a semihosting call; if so
> - * then handle it and return true; otherwise return false.
> - */
> ARMCPU *cpu = ARM_CPU(cs);
> CPUARMState *env = &cpu->env;
>
> if (is_a64(env)) {
> -if (cs->exception_index == EXCP_SEMIHOST) {
> -/* This is always the 64-bit semihosting exception.
> - * The "is this usermode" and "is semihosting enabled"
> - * checks have been done at translate time.
> - */
> -qemu_log_mask(CPU_LOG_INT,
> - "...handling as semihosting call 0x%" PRIx64 "\n",
> - env->xregs[0]);
> -env->xregs[0] = do_arm_semihosting(env);
> -return true;
> -}
> -return false;
> +qemu_log_mask(CPU_LOG_INT,
> + "...handling as semihosting call 0x%" PRIx64 "\n",
> + env->xregs[0]);
> +env->xregs[0] = do_arm_semihosting(env);
> } else {
> -uint32_t imm;
> -
> -/* Only intercept calls from privileged modes, to provide some
> - * semblance of security.
> - */
> -if (cs->exception_index != EXCP_SEMIHOST &&
> -(!semihosting_enabled() ||
> - ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_USR))) {
> -return false;
> -}
> -
> -switch (cs->exception_index) {
> -case EXCP_SEMIHOST:
> -/* This is always a semihosting call; the "is this usermode"
> - * and "is semihosting enabled" checks have been done at
> - * translate time.
> - */
> -break;
> -case EXCP_SWI:
> -/* Check for semihosting interrupt. */
> -if (env->thumb) {
> -imm = arm_lduw_code(env, env->regs[15] - 2, arm_sctlr_b(env))
> -& 0xff;
> -if (imm == 0xab) {
> -break;
> -}
> -} else {
> -imm = arm_ldl_code(env, env->regs[15] - 4, arm_sctlr_b(env))
> -& 0xff;
> -if (imm == 0x123456) {
> -break;
> -}
> -}
> -return false;
> -case EXCP_BKPT:
> -/* See if this is a semihosting syscall. */
> -if (env->thumb) {
> -imm = arm_lduw_code(env, env->regs[15], arm_sctlr_b(env))
> -& 0xff;
> -if (imm == 0xab) {
> -env->regs[15] += 2;
> -break;
> -}
> -}
> -return false;
> -default:
> -return false;
> -}
> -
> qemu_log_mask(CPU_LOG_INT,
>"...handling as semihosting call 0x%x\n",
>env->regs[0]);
> env->regs[0] = do_arm_semihosting(env);
> -return true;
> }
> -#else
> -return false;
> #endif
> }
>
> @@ -8371,11 +8315,13 @@ void arm_cpu_do_interrupt(CPUState *cs)
> return;
> }
>
> -/* Semihosting semantics depend on the register width of the
> - * code that caused the exception, not the target exception level,
> - * so must be handled here.
> +/*
> + * Semihosting semantics depend on the register width of the code
> + * that caused the exception, not the target exception level, so
> + * must be handled here.
> */
> -if (check_for_semihosting(cs)) {
> +if (cs->exception_index == EXCP_SEMIHOST) {
> +handle_semihosting(cs);
> return;
> }
Previously, this code would never return here if CONFIG_TCG was not
defined because check_for_semihosting() always returned false in that
case. Is it now true that `cs->exception_index` will never hold
Re: [Qemu-devel] [PATCH v4 24/54] plugins: implement helpers for resolving hwaddr
On Jul 31 17:06, Alex Bennée wrote:
> We need to keep a local per-cpu copy of the data as other threads may
> be running. We use a automatically growing array and re-use the space
> for subsequent queries.
[...]
> +bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
> + bool is_store, struct qemu_plugin_hwaddr *data)
> +{
> +CPUArchState *env = cpu->env_ptr;
> +CPUTLBEntry *tlbe = tlb_entry(env, mmu_idx, addr);
> +target_ulong tlb_addr = is_store ? tlb_addr_write(tlbe) :
> tlbe->addr_read;
> +
> +if (tlb_hit(tlb_addr, addr)) {
> +if (tlb_addr & TLB_MMIO) {
> +data->hostaddr = 0;
> +data->is_io = true;
> +/* XXX: lookup device */
> +} else {
> +data->hostaddr = addr + tlbe->addend;
> +data->is_io = false;
> +}
> +return true;
> +}
> +return false;
> +}
In what cases do you expect tlb_hit() should not evaluate to true here?
Will returns of false only be in error cases, or do you expect it can
occur during normal operation? In particular, I'm interested in ensuring
this is as reliable as possible, since some plugins may require physical
addresses.
> +struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
> + uint64_t vaddr)
> +{
> +CPUState *cpu = current_cpu;
> +unsigned int mmu_idx = info >> TRACE_MEM_MMU_SHIFT;
> +struct qemu_plugin_hwaddr *hwaddr;
> +
> +/* Ensure we have memory allocated for this work */
> +if (!hwaddr_refs) {
> +hwaddr_refs = g_array_sized_new(false, true,
> +sizeof(struct qemu_plugin_hwaddr),
> +cpu->cpu_index + 1);
> +} else if (cpu->cpu_index >= hwaddr_refs->len) {
> +hwaddr_refs = g_array_set_size(hwaddr_refs, cpu->cpu_index + 1);
> +}
Are there one or more race conditions with the allocations here? If so,
could they be solved by doing the allocations at plugin initialization
and when the number of online cpu's changes, instead of lazily?
> uint64_t qemu_plugin_hwaddr_to_raddr(const struct qemu_plugin_hwaddr *haddr)
I was at first confused about the utility of this function until I
(re-?)discovered you had to convert first to hwaddr and then raddr to
get a "true" physical address. Perhaps that could be added to a comment
here or in the API definition in the main plugin header file.
-Aaron
Re: [Qemu-devel] [PATCH v4 50/54] tests/plugin: add instruction execution breakdown
On Jul 31 17:07, Alex Bennée wrote: > + * Attempt to measure the amount of vectorisation that has been done > + * on some code by counting classes of instruction. This is very much > + * ARM specific. I suspect some of my plugins will also be architecture-specific. Does it make sense to have a plugin specify to QEMU which architectures or running modes (i.e. softmmu vs. linux user) it supports? Or alternatively to have QEMU expose this information to the plugin so that it can cleanly exit if its needs are not met? -Aaron
Re: [Qemu-devel] [PATCH v4 13/54] plugin: add user-facing API
One thing I would find useful is the ability to access register values during an execution-time callback. I think the easiest way to do that generically would be to expose them via the gdb functionality (like Pavel's earlier patchset did [1]), though that (currently) limits you to the general-purpose registers. Ideally it would be nice be able to access other registers (i.e. floating-point, or maybe even system registers), though those are more difficult to do generically. Perhaps if we added some sort of architectural-support checking for individual plugins like I mentioned in another response to this patchset, we could allow some limited architecture-specific functionality in this vein? I confess I haven't thought through all the ramifications of that yet, though. -Aaron [1] - See qemulib_read_register() at https://patchwork.ozlabs.org/patch/925393/
Re: [Qemu-devel] [PATCH v4 00/54] plugins for TCG
On Sep 06 20:52, Alex Bennée wrote: > > Markus Armbruster writes: > > Please advise why TCG plugins don't undermine the GPL. Any proposal to > > add a plugin interface needs to do that. > > I'm not sure what we can say about this apart from "ask your lawyer". > I'm certainly not proposing we add any sort of language about what > should and shouldn't be allowed to use the plugin interface. I find it > hard to see how anyone could argue code written to interface with the > plugin API couldn't be considered a derived work. I am not a lawyer, but I would not have expected software merely using a well-defined API to be considered a derivative work of the software defining it. Unless, of course, it is a derivative work of another plugin using the same interface in a way that is not necessitated by the structure of the API. What's your reasoning for why it would be a derivative work? Is your belief that the plugin API is complex enough that anything using it has to be a derivative work, or something else? That said, I'm not sure I understand in what way adding a plugin interface would undermine the GPL, so maybe I'm missing the point. -Aaron
Re: [Qemu-devel] [PATCH v4 13/54] plugin: add user-facing API
On Sep 06 20:31, Alex Bennée wrote: > Aaron Lindsay OS writes: > > > One thing I would find useful is the ability to access register values > > during an execution-time callback. I think the easiest way to do that > > generically would be to expose them via the gdb functionality (like > > Pavel's earlier patchset did [1]), though that (currently) limits you to > > the general-purpose registers. Ideally it would be nice be able to > > access other registers (i.e. floating-point, or maybe even system > > registers), though those are more difficult to do generically. > > ARM already has system register support via the gdbstub XML interface so > it's certainly doable. The trick is how we do that in a probable way > without leaking the gdb remote protocol into plugins (which is just very > ugly). What do you mean by "in a probable way"? I agree that simply exposing the gdb interface does not seem like a clean solution. > > Perhaps if we added some sort of architectural-support checking for > > individual plugins like I mentioned in another response to this > > patchset, we could allow some limited architecture-specific > > functionality in this vein? I confess I haven't thought through all the > > ramifications of that yet, though. > > I was wondering if exposing the Elf Type would be enough? It's portable > enough that plugins should be able to work with it without defining our > own architecture enumeration. I can't think of a reason that wouldn't work, assuming you're referring to exposing a value corresponding to the `e_machine` ELF header. -Aaron
Re: [Qemu-devel] [PATCH v3 19/50] tcg: let plugins instrument memory accesses
On Jun 14 18:11, Alex Bennée wrote: > From: "Emilio G. Cota" > > Here the trickiest feature is passing the host address to > memory callbacks that request it. Perhaps it would be more > appropriate to pass a "physical" address to plugins, but since > in QEMU host addr ~= guest physical, I'm going with that for > simplicity. How much more difficult would it be to get the true physical address (on the guest)? This is important enough to me that I would be willing to help if pointed in the right direction. -Aaron
Re: [Qemu-devel] [PATCH v3 19/50] tcg: let plugins instrument memory accesses
On Jun 28 18:11, Alex Bennée wrote: > Aaron Lindsay OS writes: > > On Jun 14 18:11, Alex Bennée wrote: > >> From: "Emilio G. Cota" > >> > >> Here the trickiest feature is passing the host address to > >> memory callbacks that request it. Perhaps it would be more > >> appropriate to pass a "physical" address to plugins, but since > >> in QEMU host addr ~= guest physical, I'm going with that for > >> simplicity. > > > > How much more difficult would it be to get the true physical address (on > > the guest)? > > Previously there was a helper that converted host address (i.e. where > QEMU actually stores that value) back to the physical address (ram > offset + ram base). However the code for calculating all of this is > pretty invasive and requires tweaks to all the softmmu TCG backends as > well as hooks into a slew of memory functions. > > I'm re-working this now so we just have the one memory callback and we > provide a helper function that can provide an opaque hwaddr struct which > can then be queried. To make sure I understand - you're implying that one such query will return the PA from the guest's perspective, right? > The catch is you can only call this helper during a > memory callback. Does this mean it will be difficult to get the physical address for the bytes containing the instruction encoding itself? > I'm not sure if having this restriction violates our > aim of not leaking implementation details to the plugin but it makes the > code simpler. Assuming that the purpose of "not leaking implementation details" is to allow the same plugin interface to work with other backend implementations in the future, isn't this probably fine? It may add an unnecessary limitation for another backend driving the same plugin interface, but I don't think it likely changes the structure of the interface itself. And that seems like the sort of restriction that could easily be dropped in the future while remaining backwards-compatible. > Internally what the helper does is simply re-query the SoftMMU TLB. As > the TLBs are per-CPU nothing else can have touched the TLB and the cache > should be hot so the cost of lookup should be minor. We could also > potentially expand the helpers so if you are interested in only IO > accesses we can do the full resolution and figure out what device we > just accessed. Oh, so you're already working on doing just what I asked about? > > This is important enough to me that I would be willing to help if > > pointed in the right direction. > > Well I'll certainly CC on the next series (hopefully posted Monday, > softfreeze starts Tuesday). I'll welcome any testing and review. Also if > you can tell us more about your use case that will help. Awesome, thanks! In terms of our use case - we use QEMU to drive studies to help us design the next generation of processors. As you can imagine, having the right physical addresses is important for some aspects of that. We're currently using a version of Pavel Dovgalyuk's earlier plugin patchset with some of our own patches/fixes on top, but it would obviously make our lives easier to work together to get this sort of infrastructure upstream! -Aaron
Re: [Qemu-devel] [PATCH v3 19/50] tcg: let plugins instrument memory accesses
On Jun 28 21:52, Alex Bennée wrote: > Aaron Lindsay OS writes: > > To make sure I understand - you're implying that one such query will > > return the PA from the guest's perspective, right? > > Yes - although it will be two queries: > > struct qemu_plugin_hwaddr *hw = qemu_plugin_get_hwaddr(info, vaddr); > > This does the actual lookup and stores enough information for the > further queries. > > uint64_t pa = qemu_plugin_hwaddr_to_raddr(hw); > > will return the physical address (assuming it's a RAM reference and not > some IO location). Sounds good, as long as we have a good way to either prevent or cleanly detect the failure mode for the IO accesses. > > In terms of our use case - we use QEMU to drive studies to help us > > design the next generation of processors. As you can imagine, having the > > right physical addresses is important for some aspects of that. We're > > currently using a version of Pavel Dovgalyuk's earlier plugin patchset > > with some of our own patches/fixes on top, but it would obviously make > > our lives easier to work together to get this sort of infrastructure > > upstream! > > Was this: > > Date: Tue, 05 Jun 2018 13:39:15 +0300 > Message-ID: > <152819515565.30857.16834004920507717324.stgit@pasha-ThinkPad-T60> > Subject: [Qemu-devel] [RFC PATCH v2 0/7] QEMU binary instrumentation > prototype Yes, that looks like the one. > What patches did you add on top? We added: - plugin support for linux-user mode (I sent that one upstream, I think) - memory tracing support and a VA->PA conversion helper - a way for a plugin to request getting a callback just before QEMU exits to clean up any internal state - a way for a plugin to reset any instrumentation decisions made in the past (essentially calls `tb_flush(cpu);` under the covers). We found this critical for plugins which undergo state changes during the course of their execution (i.e. watch for event X, then go into a more detailed profiling mode until you see event Y) - instrumentation at the TB granularity (in addition to the existing instruction-level support) - the ability for a plugin to trigger a checkpoint to be taken -Aaron
Re: [Qemu-devel] [PATCH v3 19/50] tcg: let plugins instrument memory accesses
On Jul 01 16:00, Alex Bennée wrote: > Aaron Lindsay OS writes: > > - a way for a plugin to reset any instrumentation decisions made in the > > past (essentially calls `tb_flush(cpu);` under the covers). We found > > this critical for plugins which undergo state changes during the > > course of their execution (i.e. watch for event X, then go into a more > > detailed profiling mode until you see event Y) > > check: > > /** > * qemu_plugin_reset() - Reset a plugin > * @id: this plugin's opaque ID > * @cb: callback to be called once the plugin has been reset > * > * Unregisters all callbacks for the plugin given by @id. > * > * Do NOT assume that the plugin has been reset once this function returns. > * Plugins are reset asynchronously, and therefore the given plugin receives > * callbacks until @cb is called. > */ > void qemu_plugin_reset(qemu_plugin_id_t id, qemu_plugin_simple_cb_t cb); Is this essentially synchronous for the current cpu, and only asynchronous for any other running cpus that didn't trigger the callback from which the call to qemu_plugin_reset() is being made? If not, could the state resetting be made synchronous for the current cpu (even if the callback doesn't happen until the others are complete)? This isn't absolutely critical, but it is often nice to begin capturing precisely when you mean to. > > - the ability for a plugin to trigger a checkpoint to be taken > > We don't have this at the moment. Pranith also mentioned it in his > review comments. I can see its use but I suspect it won't make the > initial implementation given the broader requirements of QEMU to do > checkpointing and how to cleanly expose that to plugins. Sure. Our patch works for us, but I know we're ignoring a few things that we can externally ensure won't happen while we're attempting a checkpoint (i.e. migration) that may have to be considered for something upstream. -Aaron
