All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Graf <agraf@csgraf.de>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Eduardo Habkost" <ehabkost@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Cameron Esfahani" <dirty@apple.com>,
	"Roman Bolshakov" <r.bolshakov@yadro.com>,
	qemu-arm <qemu-arm@nongnu.org>, "Frank Yang" <lfy@google.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Peter Collingbourne" <pcc@google.com>
Subject: Re: [PATCH v8 13/19] hvf: Add Apple Silicon support
Date: Sun, 27 Jun 2021 22:40:19 +0200	[thread overview]
Message-ID: <30c30532-c84a-051d-825e-9da6da0b4dcf@csgraf.de> (raw)
In-Reply-To: <CAFEAcA_M6SB8f4X1z=P4+pZ0Q-H3vdhvgu8ZsQLxZFro5TuiVQ@mail.gmail.com>

Hi Peter,

On 15.06.21 12:21, Peter Maydell wrote:
> On Wed, 19 May 2021 at 21:23, Alexander Graf <agraf@csgraf.de> wrote:
>> With Apple Silicon available to the masses, it's a good time to add support
>> for driving its virtualization extensions from QEMU.
>>
>> This patch adds all necessary architecture specific code to get basic VMs
>> working. It's still pretty raw, but definitely functional.
>>
>> Known limitations:
>>
>>   - Vtimer acknowledgement is hacky
>>   - Should implement more sysregs and fault on invalid ones then
>>   - WFI handling is missing, need to marry it with vtimer
>>
>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
>> @@ -446,11 +454,17 @@ static void hvf_start_vcpu_thread(CPUState *cpu)
>>                         cpu, QEMU_THREAD_JOINABLE);
>>  }
>>
>> +__attribute__((weak)) void hvf_kick_vcpu_thread(CPUState *cpu)
>> +{
>> +    cpus_kick_thread(cpu);
>> +}
> Why is this marked 'weak' ? If there's a reason for it then
> it ought to have a comment describing the reason. If we can avoid
> it then we should do so -- past experience is that 'weak' refs
> are rather non-portable, though at least this one is in a
> host-OS-specific file.


Mostly because I wanted to keep the kick function in the generic file
for the generic case. ARM is special in that it requires different kick
mechanisms depending on which context we're in (in-vcpu or in-QEMU).

However, I agree that with 2 architectures, there's not really a
"default". I'm happy to move it into the x86 specific file.


>
>> +static void hvf_raise_exception(CPUARMState *env, uint32_t excp,
>> +                                uint32_t syndrome)
>> +{
>> +    unsigned int new_el = 1;
>> +    unsigned int old_mode = pstate_read(env);
>> +    unsigned int new_mode = aarch64_pstate_mode(new_el, true);
>> +    target_ulong addr = env->cp15.vbar_el[new_el];
>> +
>> +    env->cp15.esr_el[new_el] = syndrome;
>> +    aarch64_save_sp(env, arm_current_el(env));
>> +    env->elr_el[new_el] = env->pc;
>> +    env->banked_spsr[aarch64_banked_spsr_index(new_el)] = old_mode;
>> +    pstate_write(env, PSTATE_DAIF | new_mode);
>> +    aarch64_restore_sp(env, new_el);
>> +    env->pc = addr;
>> +}
> KVM does "raise an exception" by calling arm_cpu_do_interrupt()
> to do the "set ESR_ELx, save SPSR, etc etc" work (see eg
> kvm_arm_handle_debug()". Does that not work here ?


It works like a charm. I mostly did the dance because I was under the
impression you wanted to avoid me calling into any TCG code. And to me
arm_cpu_do_interrupt() seemed like TCG code. I'm absolutely happy to
change it though. Leaving things to generic code is good IMHO :).


>
>> +
>> +static uint64_t hvf_sysreg_read(CPUState *cpu, uint32_t reg)
>> +{
>> +    ARMCPU *arm_cpu = ARM_CPU(cpu);
>> +    uint64_t val = 0;
>> +
>> +    switch (reg) {
>> +    case SYSREG_CNTPCT_EL0:
>> +        val = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) /
>> +              gt_cntfrq_period_ns(arm_cpu);
>> +        break;
> Does hvf handle the "EL0 access which should be denied because
> CNTKCTL_EL1.EL0PCTEN is 0" case for us, or should we have
> an access-check here ?


A quick test where I tried to access it in a VM in EL0 shows that either
the CPU or HVF already generates the trap. So no check needed.


>
>> +    case SYSREG_PMCCNTR_EL0:
>> +        val = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> This is supposed to be a cycle counter, not a timestamp...


At 1Ghz cycle frequency, what's the difference? Or are you concerned
about the lack of overflow and PMCR logic?


>
>> +        break;
>> +    default:
>> +        trace_hvf_unhandled_sysreg_read(reg,
>> +                                        (reg >> 20) & 0x3,
>> +                                        (reg >> 14) & 0x7,
>> +                                        (reg >> 10) & 0xf,
>> +                                        (reg >> 1) & 0xf,
>> +                                        (reg >> 17) & 0x7);
>> +        break;
>> +    }
>> +
>> +    return val;
>> +}
>> +
>> +static void hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
>> +{
>> +    switch (reg) {
>> +    case SYSREG_CNTPCT_EL0:
>> +        break;
> CNTPCT_EL0 is read-only (ie writes should fault) but this
> makes it writes-ignored, doesn't it ?


It does indeed, let me fix that.


>
>> +    default:
>> +        trace_hvf_unhandled_sysreg_write(reg,
>> +                                         (reg >> 20) & 0x3,
>> +                                         (reg >> 14) & 0x7,
>> +                                         (reg >> 10) & 0xf,
>> +                                         (reg >> 1) & 0xf,
>> +                                         (reg >> 17) & 0x7);
>> +        break;
>> +    }
>> +}
>> +    switch (ec) {
>> +    case EC_DATAABORT: {
>> +        bool isv = syndrome & ARM_EL_ISV;
>> +        bool iswrite = (syndrome >> 6) & 1;
>> +        bool s1ptw = (syndrome >> 7) & 1;
>> +        uint32_t sas = (syndrome >> 22) & 3;
>> +        uint32_t len = 1 << sas;
>> +        uint32_t srt = (syndrome >> 16) & 0x1f;
>> +        uint64_t val = 0;
>> +
>> +        trace_hvf_data_abort(env->pc, hvf_exit->exception.virtual_address,
>> +                             hvf_exit->exception.physical_address, isv,
>> +                             iswrite, s1ptw, len, srt);
>> +
>> +        assert(isv);
> This seems dubious -- won't we just crash if the guest does
> a data access to a device or to unmapped memory with an insn that
> doesn't set ISV ? With KVM we feed this back to the guest as an
> external data abort (see the KVM_EXIT_ARM_NISV handling).


Hm, I can't say I'm a big fan of that behavior: It makes enabling OSs
that are not properly adapted for non-ISV behavior harder, because
understanding in-guest exception flows is usually harder than looking at
undesired code paths from the outside. And I highly doubt you'll find a
properly functional guest environment that gets external data aborts on
non-enlightened MMIO access.

But if you feel strongly about it, I'm happy to imitate the behavior of
today's KVM_EXIT_ARM_NISV handler.


>
>> +
>> +        if (iswrite) {
>> +            val = hvf_get_reg(cpu, srt);
>> +            address_space_write(&address_space_memory,
>> +                                hvf_exit->exception.physical_address,
>> +                                MEMTXATTRS_UNSPECIFIED, &val, len);
>> +        } else {
>> +            address_space_read(&address_space_memory,
>> +                               hvf_exit->exception.physical_address,
>> +                               MEMTXATTRS_UNSPECIFIED, &val, len);
>> +            hvf_set_reg(cpu, srt, val);
>> +        }
>> +
>> +        advance_pc = true;
>> +        break;
>> +    }
>> +    case EC_SYSTEMREGISTERTRAP: {
>> +        bool isread = (syndrome >> 0) & 1;
>> +        uint32_t rt = (syndrome >> 5) & 0x1f;
>> +        uint32_t reg = syndrome & SYSREG_MASK;
>> +        uint64_t val = 0;
>> +
>> +        if (isread) {
>> +            val = hvf_sysreg_read(cpu, reg);
>> +            trace_hvf_sysreg_read(reg,
>> +                                  (reg >> 20) & 0x3,
>> +                                  (reg >> 14) & 0x7,
>> +                                  (reg >> 10) & 0xf,
>> +                                  (reg >> 1) & 0xf,
>> +                                  (reg >> 17) & 0x7,
>> +                                  val);
>> +            hvf_set_reg(cpu, rt, val);
>> +        } else {
>> +            val = hvf_get_reg(cpu, rt);
>> +            trace_hvf_sysreg_write(reg,
>> +                                   (reg >> 20) & 0x3,
>> +                                   (reg >> 14) & 0x7,
>> +                                   (reg >> 10) & 0xf,
>> +                                   (reg >> 1) & 0xf,
>> +                                   (reg >> 17) & 0x7,
>> +                                   val);
>> +            hvf_sysreg_write(cpu, reg, val);
>> +        }
> This needs support for "this really was a bogus system register
> access, feed the guest an exception".


Yup. Added.


Thanks!

Alex




  reply	other threads:[~2021-06-27 20:41 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-19 20:22 [PATCH v8 00/19] hvf: Implement Apple Silicon Support Alexander Graf
2021-05-19 20:22 ` [PATCH v8 01/19] hvf: Move assert_hvf_ok() into common directory Alexander Graf
2021-05-27 10:00   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 02/19] hvf: Move vcpu thread functions " Alexander Graf
2021-05-27 10:01   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 03/19] hvf: Move cpu " Alexander Graf
2021-05-27 10:02   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 04/19] hvf: Move hvf internal definitions into common header Alexander Graf
2021-05-27 10:04   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 05/19] hvf: Make hvf_set_phys_mem() static Alexander Graf
2021-05-27 10:06   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 06/19] hvf: Remove use of hv_uvaddr_t and hv_gpaddr_t Alexander Graf
2021-05-27 10:07   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 07/19] hvf: Split out common code on vcpu init and destroy Alexander Graf
2021-05-27 10:09   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 08/19] hvf: Use cpu_synchronize_state() Alexander Graf
2021-05-27 10:15   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 09/19] hvf: Make synchronize functions static Alexander Graf
2021-05-27 10:15   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 10/19] hvf: Remove hvf-accel-ops.h Alexander Graf
2021-05-27 10:16   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 11/19] hvf: Introduce hvf vcpu struct Alexander Graf
2021-05-27 10:18   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 12/19] hvf: Simplify post reset/init/loadvm hooks Alexander Graf
2021-05-27 10:20   ` Sergio Lopez
2021-05-19 20:22 ` [PATCH v8 13/19] hvf: Add Apple Silicon support Alexander Graf
2021-05-27 10:55   ` Sergio Lopez
2021-06-15 10:21   ` Peter Maydell
2021-06-27 20:40     ` Alexander Graf [this message]
2021-05-19 20:22 ` [PATCH v8 14/19] arm/hvf: Add a WFI handler Alexander Graf
2021-05-27 10:53   ` Sergio Lopez
2021-06-15 10:38   ` Peter Maydell
2021-05-19 20:22 ` [PATCH v8 15/19] hvf: arm: Implement -cpu host Alexander Graf
2021-05-27 11:18   ` Sergio Lopez
2021-06-15 10:56   ` Peter Maydell
2021-09-12 20:23     ` Alexander Graf
2021-09-13  8:28       ` Peter Maydell
2021-09-13 10:46         ` Alexander Graf
2021-09-13 10:52           ` Peter Maydell
2021-09-13 11:46             ` Alexander Graf
2021-09-13 11:48               ` Peter Maydell
2021-09-13 11:55                 ` Alexander Graf
2021-05-19 20:22 ` [PATCH v8 16/19] hvf: arm: Implement PSCI handling Alexander Graf
2021-05-27 11:20   ` Sergio Lopez
2021-06-15 12:54   ` Peter Maydell
2021-09-12 20:36     ` Alexander Graf
2021-09-12 21:20       ` Richard Henderson
2021-09-12 21:37         ` Alexander Graf
2021-09-12 21:40           ` Richard Henderson
2021-09-13 10:06             ` Alexander Graf
2021-09-13 10:30               ` Philippe Mathieu-Daudé
2021-05-19 20:22 ` [PATCH v8 17/19] arm: Add Hypervisor.framework build target Alexander Graf
2021-05-27 11:20   ` Sergio Lopez
2021-06-15 10:59   ` Peter Maydell
2021-05-19 20:22 ` [PATCH v8 18/19] arm: Enable Windows 10 trusted SMCCC boot call Alexander Graf
2021-05-27 11:21   ` Sergio Lopez
2021-06-15 11:02   ` Peter Maydell
2021-06-27 21:12     ` Alexander Graf
2021-05-19 20:22 ` [PATCH v8 19/19] hvf: arm: Handle Windows 10 SMC call Alexander Graf
2021-05-27 11:22   ` Sergio Lopez
2021-06-15  9:31   ` Peter Maydell
2021-06-27 21:07     ` Alexander Graf
2021-05-19 20:45 ` [PATCH v8 00/19] hvf: Implement Apple Silicon Support no-reply
2021-06-03 13:43 ` Peter Maydell
2021-06-03 13:53   ` Alexander Graf
2021-06-15 12:54   ` Peter Maydell

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=30c30532-c84a-051d-825e-9da6da0b4dcf@csgraf.de \
    --to=agraf@csgraf.de \
    --cc=dirty@apple.com \
    --cc=ehabkost@redhat.com \
    --cc=lfy@google.com \
    --cc=pbonzini@redhat.com \
    --cc=pcc@google.com \
    --cc=peter.maydell@linaro.org \
    --cc=philmd@redhat.com \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=r.bolshakov@yadro.com \
    --cc=richard.henderson@linaro.org \
    --subject='Re: [PATCH v8 13/19] hvf: Add Apple Silicon support' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.