diff mbox

[v3,3/6] target-arm: add hvc and smc exception emulation handling infrastructure

Message ID 1409919897-360-4-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Sept. 5, 2014, 12:24 p.m. UTC 
From: Rob Herring <rob.herring@linaro.org>

Add the infrastructure to handle and emulate hvc and smc exceptions.
This will enable emulation of things such as PSCI calls. This commit
does not change the behavior and will exit with unknown exception.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 target-arm/cpu-qom.h       |  3 +++
 target-arm/cpu.h           |  2 ++
 target-arm/helper-a64.c    | 16 ++++++++++++++++
 target-arm/helper.c        | 23 +++++++++++++++++++++++
 target-arm/internals.h     | 20 ++++++++++++++++++++
 target-arm/translate-a64.c | 26 +++++++++++++++++---------
 target-arm/translate.c     | 24 +++++++++++++++++-------
 7 files changed, 98 insertions(+), 16 deletions(-)

Comments

Peter Maydell Sept. 9, 2014, 5:45 p.m. UTC | #1 
On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> Add the infrastructure to handle and emulate hvc and smc exceptions.
> This will enable emulation of things such as PSCI calls. This commit
> does not change the behavior and will exit with unknown exception.

This generally looks ok except that it has nasty interactions
with singlestep debug. For SVC, HVC and SMC instructions that
do something, we want to advance the singlestep state machine
using gen_ss_advance(), so that singlestep of one of these
insns works correctly. For UNDEF instruction patterns we don't
want to advance the state machine, so that we don't treat the
insn we didn't execute like we single-stepped it. Unfortunately
this patch means that SMC and HVC are now "sometimes this
does something but sometimes we act like it UNDEFed"...

This is my suggestion for the best compromise between
"theoretical perfect fidelity to the architecture" and
"not too painful to implement":
at translate time, do:

  if (psci enabled via HVC || EL2 implemented) {
      gen_ss_advance(s);
      gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
  } else {
      unallocated_encoding();
  }
and ditto for SMC.

Then in the exception handler have the same code as
now (with fall through to UNDEF if PSCI doesn't
recognise the op). That corresponds to "EL3 firmware
implements PSCI and handles unrecognised ops by
feeding the guest an UNDEF", which nobody would
expect to singlestep cleanly either.

> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7871,9 +7871,14 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
>          case 7:
>          {
>              int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4);
> -            /* SMC instruction (op1 == 3)
> -               and undefined instructions (op1 == 0 || op1 == 2)
> -               will trap */
> +            /* HVC and SMC instructions */
> +            if (op1 == 2) {
> +                gen_exception_insn(s, 0, EXCP_HVC, syn_aa32_hvc(imm16));
> +                break;
> +            } else if (op1 == 3) {
> +                gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
> +                break;
> +            }
>              if (op1 != 1) {
>                  goto illegal_op;
>              }
> @@ -9709,10 +9714,15 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>                      goto illegal_op;
>
>                  if (insn & (1 << 26)) {
> -                    /* Secure monitor call (v6Z) */
> -                    qemu_log_mask(LOG_UNIMP,
> -                                  "arm: unimplemented secure monitor call\n");
> -                    goto illegal_op; /* not implemented.  */
> +                    if (!(insn & (1 << 20))) {
> +                        /* Hypervisor call (v7) */
> +                        uint32_t imm16 = extract32(insn, 16, 4) << 12;
> +                        imm16 |= extract32(insn, 0, 12);
> +                        gen_exception_insn(s, 0, EXCP_HVC, syn_aa32_hvc(imm16));
> +                    } else {
> +                        /* Secure monitor call (v6+) */
> +                        gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
> +                    }
>                  } else {
>                      op = (insn >> 20) & 7;
>                      switch (op) {

I'm pretty sure you can't do the A32/T32 HVC/SMC
like this, because of oddball cases like SMC in
an IT block. Look at the way we handle SWI by
setting s->is_jmp and then doing the actual
gen_exception() work in the tail end of the
main loop. (It might be possible to do HVC
the way you have it since HVC in an IT block
is UNPREDICTABLE, but let's do it all the same
way...)

thanks
-- PMM
Ard Biesheuvel Sept. 9, 2014, 9:51 p.m. UTC | #2 
On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> From: Rob Herring <rob.herring@linaro.org>
>>
>> Add the infrastructure to handle and emulate hvc and smc exceptions.
>> This will enable emulation of things such as PSCI calls. This commit
>> does not change the behavior and will exit with unknown exception.
>
> This generally looks ok except that it has nasty interactions
> with singlestep debug. For SVC, HVC and SMC instructions that
> do something, we want to advance the singlestep state machine
> using gen_ss_advance(), so that singlestep of one of these
> insns works correctly. For UNDEF instruction patterns we don't
> want to advance the state machine, so that we don't treat the
> insn we didn't execute like we single-stepped it. Unfortunately
> this patch means that SMC and HVC are now "sometimes this
> does something but sometimes we act like it UNDEFed"...
>
> This is my suggestion for the best compromise between
> "theoretical perfect fidelity to the architecture" and
> "not too painful to implement":
> at translate time, do:
>
>   if (psci enabled via HVC || EL2 implemented) {
>       gen_ss_advance(s);
>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
>   } else {
>       unallocated_encoding();
>   }
> and ditto for SMC.
>

OK, so does that mean I need to add fields to DisasContext for these
functions to inspect at the translation stage, and copy the
PSCI_METHOD_[SVC|HVC|NONE] values in it?
Or is there another way to get at that information at that stage?

> Then in the exception handler have the same code as
> now (with fall through to UNDEF if PSCI doesn't
> recognise the op). That corresponds to "EL3 firmware
> implements PSCI and handles unrecognised ops by
> feeding the guest an UNDEF", which nobody would
> expect to singlestep cleanly either.
>

ok

>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -7871,9 +7871,14 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
>>          case 7:
>>          {
>>              int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4);
>> -            /* SMC instruction (op1 == 3)
>> -               and undefined instructions (op1 == 0 || op1 == 2)
>> -               will trap */
>> +            /* HVC and SMC instructions */
>> +            if (op1 == 2) {
>> +                gen_exception_insn(s, 0, EXCP_HVC, syn_aa32_hvc(imm16));
>> +                break;
>> +            } else if (op1 == 3) {
>> +                gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
>> +                break;
>> +            }
>>              if (op1 != 1) {
>>                  goto illegal_op;
>>              }
>> @@ -9709,10 +9714,15 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>>                      goto illegal_op;
>>
>>                  if (insn & (1 << 26)) {
>> -                    /* Secure monitor call (v6Z) */
>> -                    qemu_log_mask(LOG_UNIMP,
>> -                                  "arm: unimplemented secure monitor call\n");
>> -                    goto illegal_op; /* not implemented.  */
>> +                    if (!(insn & (1 << 20))) {
>> +                        /* Hypervisor call (v7) */
>> +                        uint32_t imm16 = extract32(insn, 16, 4) << 12;
>> +                        imm16 |= extract32(insn, 0, 12);
>> +                        gen_exception_insn(s, 0, EXCP_HVC, syn_aa32_hvc(imm16));
>> +                    } else {
>> +                        /* Secure monitor call (v6+) */
>> +                        gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
>> +                    }
>>                  } else {
>>                      op = (insn >> 20) & 7;
>>                      switch (op) {
>
> I'm pretty sure you can't do the A32/T32 HVC/SMC
> like this, because of oddball cases like SMC in
> an IT block. Look at the way we handle SWI by
> setting s->is_jmp and then doing the actual
> gen_exception() work in the tail end of the
> main loop. (It might be possible to do HVC
> the way you have it since HVC in an IT block
> is UNPREDICTABLE, but let's do it all the same
> way...)
>

Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
SMC and HVC respectively.
(I don't suppose there is any point in adding TZ and VIRT feature bits
for this atm)
Peter Maydell Sept. 9, 2014, 9:59 p.m. UTC | #3 
On 9 September 2014 22:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This is my suggestion for the best compromise between
>> "theoretical perfect fidelity to the architecture" and
>> "not too painful to implement":
>> at translate time, do:
>>
>>   if (psci enabled via HVC || EL2 implemented) {
>>       gen_ss_advance(s);
>>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
>>   } else {
>>       unallocated_encoding();
>>   }
>> and ditto for SMC.
>>
>
> OK, so does that mean I need to add fields to DisasContext for these
> functions to inspect at the translation stage, and copy the
> PSCI_METHOD_[SVC|HVC|NONE] values in it?

You only need one field in DisasContext, but yes.
(The idea of DisasContext and not giving most of translate-a64.c
access to the CPU object is that it makes it hard to accidentally
access stuff in the CPU object that's not valid to depend on at
translate time, because it's an easy to spot and easy to review
change if something new gets added. PSCI method type is
OK because it's constant for the life of the simulation.)

> Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
> SMC and HVC respectively.
> (I don't suppose there is any point in adding TZ and VIRT feature bits
> for this atm)

We already have ARM_FEATURE_EL2 and ARM_FEATURE_EL3,
actually. You should probably look at Edgar's patchset on list which
adds proper SMC/HVC support -- that has failed review on a
few of the early patches but the middle of the set includes
some which also change this area:
http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02865.html
http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02866.html
I don't want this patchset to depend on that one but you
might find the shape of the code useful. (It doesn't do anything
in the 32 bit code, though.)

thanks
-- PMM
Ard Biesheuvel Sept. 9, 2014, 10:21 p.m. UTC | #4 
On 9 September 2014 23:59, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 September 2014 22:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> This is my suggestion for the best compromise between
>>> "theoretical perfect fidelity to the architecture" and
>>> "not too painful to implement":
>>> at translate time, do:
>>>
>>>   if (psci enabled via HVC || EL2 implemented) {
>>>       gen_ss_advance(s);
>>>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
>>>   } else {
>>>       unallocated_encoding();
>>>   }
>>> and ditto for SMC.
>>>
>>
>> OK, so does that mean I need to add fields to DisasContext for these
>> functions to inspect at the translation stage, and copy the
>> PSCI_METHOD_[SVC|HVC|NONE] values in it?
>
> You only need one field in DisasContext, but yes.
> (The idea of DisasContext and not giving most of translate-a64.c
> access to the CPU object is that it makes it hard to accidentally
> access stuff in the CPU object that's not valid to depend on at
> translate time, because it's an easy to spot and easy to review
> change if something new gets added. PSCI method type is
> OK because it's constant for the life of the simulation.)
>

OK

>> Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
>> SMC and HVC respectively.
>> (I don't suppose there is any point in adding TZ and VIRT feature bits
>> for this atm)
>
> We already have ARM_FEATURE_EL2 and ARM_FEATURE_EL3,
> actually. You should probably look at Edgar's patchset on list which
> adds proper SMC/HVC support -- that has failed review on a
> few of the early patches but the middle of the set includes
> some which also change this area:
> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02865.html
> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02866.html
> I don't want this patchset to depend on that one but you
> might find the shape of the code useful. (It doesn't do anything
> in the 32 bit code, though.)
>

I had a peek, and there is indeed substantial overlap, but not in an
incompatible way afaict
Setting ARM_FEATURE_EL[2|3] and then only emulate some HVC/SMC calls
under some circumstances is probably not the right way to go.
In fact, I think they are mutually exclusive, i.e., it probably makes
little sense to fully emulate EL2 /and/ use the HVC conduit for PSCI
at the same time.
Greg Bellows Sept. 10, 2014, 3:42 p.m. UTC | #5 
We also have v4 of the TZ patches which do provide 32-bit EL3 support, but
not EL2.  Maybe good to align with this code as well.

http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg07347.html


On 9 September 2014 16:59, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 9 September 2014 22:51, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> wrote:
> > On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >> This is my suggestion for the best compromise between
> >> "theoretical perfect fidelity to the architecture" and
> >> "not too painful to implement":
> >> at translate time, do:
> >>
> >>   if (psci enabled via HVC || EL2 implemented) {
> >>       gen_ss_advance(s);
> >>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
> >>   } else {
> >>       unallocated_encoding();
> >>   }
> >> and ditto for SMC.
> >>
> >
> > OK, so does that mean I need to add fields to DisasContext for these
> > functions to inspect at the translation stage, and copy the
> > PSCI_METHOD_[SVC|HVC|NONE] values in it?
>
> You only need one field in DisasContext, but yes.
> (The idea of DisasContext and not giving most of translate-a64.c
> access to the CPU object is that it makes it hard to accidentally
> access stuff in the CPU object that's not valid to depend on at
> translate time, because it's an easy to spot and easy to review
> change if something new gets added. PSCI method type is
> OK because it's constant for the life of the simulation.)
>
> > Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
> > SMC and HVC respectively.
> > (I don't suppose there is any point in adding TZ and VIRT feature bits
> > for this atm)
>
> We already have ARM_FEATURE_EL2 and ARM_FEATURE_EL3,
> actually. You should probably look at Edgar's patchset on list which
> adds proper SMC/HVC support -- that has failed review on a
> few of the early patches but the middle of the set includes
> some which also change this area:
> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02865.html
> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02866.html
> I don't want this patchset to depend on that one but you
> might find the shape of the code useful. (It doesn't do anything
> in the 32 bit code, though.)
>
> thanks
> -- PMM
>
>
Ard Biesheuvel Sept. 10, 2014, 4:13 p.m. UTC | #6 
On 10 September 2014 17:42, Greg Bellows <greg.bellows@linaro.org> wrote:
> We also have v4 of the TZ patches which do provide 32-bit EL3 support, but
> not EL2.  Maybe good to align with this code as well.
>
> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg07347.html
>

As far as I can tell, there is very little overlap, only patch 8/33
contains some stuff that we added as well.
I expect PSCI handling and emulation of hvc and/or smc to be mutually
exclusive, nothing we won't be able to handle with a couple of
conditionals.

My apologies for not aligning with you beforehand, I just adopted some
patches from Rob that I needed for reset and poweroff under UEFI, and
I had no idea there was so much in flight already.
Peter Maydell Sept. 10, 2014, 4:17 p.m. UTC | #7 
On 10 September 2014 17:13, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> As far as I can tell, there is very little overlap, only patch 8/33
> contains some stuff that we added as well.
> I expect PSCI handling and emulation of hvc and/or smc to be mutually
> exclusive, nothing we won't be able to handle with a couple of
> conditionals.

Agreed.

> My apologies for not aligning with you beforehand, I just adopted some
> patches from Rob that I needed for reset and poweroff under UEFI, and
> I had no idea there was so much in flight already.

The other stuff needs to come together into something coherent
anyway; I expect your changes to land first.

-- PMM
Greg Bellows Sept. 10, 2014, 4:29 p.m. UTC | #8 
No problem, just wanted to point it out in case it helps.

Greg

On 10 September 2014 11:13, Ard Biesheuvel <ard.biesheuvel@linaro.org>
wrote:

> On 10 September 2014 17:42, Greg Bellows <greg.bellows@linaro.org> wrote:
> > We also have v4 of the TZ patches which do provide 32-bit EL3 support,
> but
> > not EL2.  Maybe good to align with this code as well.
> >
> > http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg07347.html
> >
>
> As far as I can tell, there is very little overlap, only patch 8/33
> contains some stuff that we added as well.
> I expect PSCI handling and emulation of hvc and/or smc to be mutually
> exclusive, nothing we won't be able to handle with a couple of
> conditionals.
>
> My apologies for not aligning with you beforehand, I just adopted some
> patches from Rob that I needed for reset and poweroff under UEFI, and
> I had no idea there was so much in flight already.
>
> --
> Ard.
>
> >
> > On 9 September 2014 16:59, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >>
> >> On 9 September 2014 22:51, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> wrote:
> >> > On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org>
> >> > wrote:
> >> >> This is my suggestion for the best compromise between
> >> >> "theoretical perfect fidelity to the architecture" and
> >> >> "not too painful to implement":
> >> >> at translate time, do:
> >> >>
> >> >>   if (psci enabled via HVC || EL2 implemented) {
> >> >>       gen_ss_advance(s);
> >> >>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
> >> >>   } else {
> >> >>       unallocated_encoding();
> >> >>   }
> >> >> and ditto for SMC.
> >> >>
> >> >
> >> > OK, so does that mean I need to add fields to DisasContext for these
> >> > functions to inspect at the translation stage, and copy the
> >> > PSCI_METHOD_[SVC|HVC|NONE] values in it?
> >>
> >> You only need one field in DisasContext, but yes.
> >> (The idea of DisasContext and not giving most of translate-a64.c
> >> access to the CPU object is that it makes it hard to accidentally
> >> access stuff in the CPU object that's not valid to depend on at
> >> translate time, because it's an easy to spot and easy to review
> >> change if something new gets added. PSCI method type is
> >> OK because it's constant for the life of the simulation.)
> >>
> >> > Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
> >> > SMC and HVC respectively.
> >> > (I don't suppose there is any point in adding TZ and VIRT feature bits
> >> > for this atm)
> >>
> >> We already have ARM_FEATURE_EL2 and ARM_FEATURE_EL3,
> >> actually. You should probably look at Edgar's patchset on list which
> >> adds proper SMC/HVC support -- that has failed review on a
> >> few of the early patches but the middle of the set includes
> >> some which also change this area:
> >> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02865.html
> >> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02866.html
> >> I don't want this patchset to depend on that one but you
> >> might find the shape of the code useful. (It doesn't do anything
> >> in the 32 bit code, though.)
> >>
> >> thanks
> >> -- PMM
> >>
> >
>
diff mbox

Patch

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index eae0a7b9c908..104cc67e82d2 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -192,6 +192,9 @@  extern const struct VMStateDescription vmstate_arm_cpu;
 void register_cp_regs_for_features(ARMCPU *cpu);
 void init_cpreg_list(ARMCPU *cpu);
 
+bool arm_cpu_do_hvc(CPUState *cs);
+bool arm_cpu_do_smc(CPUState *cs);
+
 void arm_cpu_do_interrupt(CPUState *cpu);
 void arm_v7m_cpu_do_interrupt(CPUState *cpu);
 
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 51bedc826299..d235929f4c12 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -51,6 +51,8 @@ 
 #define EXCP_EXCEPTION_EXIT  8   /* Return from v7M exception.  */
 #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
 #define EXCP_STREX          10
+#define EXCP_HVC            11
+#define EXCP_SMC            12
 
 #define ARMV7M_EXCP_RESET   1
 #define ARMV7M_EXCP_NMI     2
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 89b913ee9396..1f8072ab141b 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -485,6 +485,22 @@  void aarch64_cpu_do_interrupt(CPUState *cs)
     case EXCP_FIQ:
         addr += 0x100;
         break;
+    case EXCP_HVC:
+        if (arm_cpu_do_hvc(cs)) {
+            return;
+        }
+        /* Treat as unallocated encoding */
+        qemu_log_mask(LOG_GUEST_ERROR, "HVC not implemented on this CPU\n");
+        env->exception.syndrome = syn_uncategorized();
+        break;
+    case EXCP_SMC:
+        if (arm_cpu_do_smc(cs)) {
+            return;
+        }
+        /* Treat as unallocated encoding */
+        qemu_log_mask(LOG_GUEST_ERROR, "SMC not implemented on this CPU\n");
+        env->exception.syndrome = syn_uncategorized();
+        break;
     default:
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 2b95f33872cb..440ee07a2ff8 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3497,6 +3497,16 @@  void arm_v7m_cpu_do_interrupt(CPUState *cs)
     env->thumb = addr & 1;
 }
 
+bool arm_cpu_do_hvc(CPUState *cs)
+{
+    return false;
+}
+
+bool arm_cpu_do_smc(CPUState *cs)
+{
+    return false;
+}
+
 /* Handle a CPU exception.  */
 void arm_cpu_do_interrupt(CPUState *cs)
 {
@@ -3513,6 +3523,19 @@  void arm_cpu_do_interrupt(CPUState *cs)
 
     /* TODO: Vectored interrupt controller.  */
     switch (cs->exception_index) {
+    case EXCP_HVC:
+        if (arm_cpu_do_hvc(cs)) {
+            return;
+        }
+        qemu_log_mask(LOG_GUEST_ERROR, "HVC not implemented on this CPU\n");
+        goto hvc_unallocated;
+    case EXCP_SMC:
+        if (arm_cpu_do_smc(cs)) {
+            return;
+        }
+        qemu_log_mask(LOG_GUEST_ERROR, "SMC not implemented on this CPU\n");
+    hvc_unallocated:
+        /* Fall through -- treat as unallocated encoding */
     case EXCP_UDEF:
         new_mode = ARM_CPU_MODE_UND;
         addr = 0x04;
diff --git a/target-arm/internals.h b/target-arm/internals.h
index 53c2e3cf3e7e..caab98e6b508 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -210,6 +210,26 @@  static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
         | (is_thumb ? 0 : ARM_EL_IL);
 }
 
+static inline uint32_t syn_aa64_hvc(uint32_t imm16)
+{
+    return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
+}
+
+static inline uint32_t syn_aa32_hvc(uint32_t imm16)
+{
+    return (EC_AA32_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
+}
+
+static inline uint32_t syn_aa64_smc(uint32_t imm16)
+{
+    return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
+}
+
+static inline uint32_t syn_aa32_smc(void)
+{
+    return (EC_AA32_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL;
+}
+
 static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
 {
     return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 8e66b6c97282..65e35e3aaec0 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1473,20 +1473,28 @@  static void disas_exc(DisasContext *s, uint32_t insn)
 
     switch (opc) {
     case 0:
-        /* SVC, HVC, SMC; since we don't support the Virtualization
-         * or TrustZone extensions these all UNDEF except SVC.
-         */
-        if (op2_ll != 1) {
-            unallocated_encoding(s);
-            break;
-        }
         /* For SVC, HVC and SMC we advance the single-step state
          * machine before taking the exception. This is architecturally
          * mandated, to ensure that single-stepping a system call
          * instruction works properly.
          */
-        gen_ss_advance(s);
-        gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
+        switch (op2_ll) {
+        case 1:
+            gen_ss_advance(s);
+            gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
+            break;
+        case 2:
+            gen_ss_advance(s);
+            gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
+            break;
+        case 3:
+            gen_ss_advance(s);
+            gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16));
+            break;
+        default:
+            unallocated_encoding(s);
+            break;
+        }
         break;
     case 1:
         if (op2_ll != 0) {
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 2c0b1deaea81..a4545ed2bc40 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -7871,9 +7871,14 @@  static void disas_arm_insn(CPUARMState * env, DisasContext *s)
         case 7:
         {
             int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4);
-            /* SMC instruction (op1 == 3)
-               and undefined instructions (op1 == 0 || op1 == 2)
-               will trap */
+            /* HVC and SMC instructions */
+            if (op1 == 2) {
+                gen_exception_insn(s, 0, EXCP_HVC, syn_aa32_hvc(imm16));
+                break;
+            } else if (op1 == 3) {
+                gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
+                break;
+            }
             if (op1 != 1) {
                 goto illegal_op;
             }
@@ -9709,10 +9714,15 @@  static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                     goto illegal_op;
 
                 if (insn & (1 << 26)) {
-                    /* Secure monitor call (v6Z) */
-                    qemu_log_mask(LOG_UNIMP,
-                                  "arm: unimplemented secure monitor call\n");
-                    goto illegal_op; /* not implemented.  */
+                    if (!(insn & (1 << 20))) {
+                        /* Hypervisor call (v7) */
+                        uint32_t imm16 = extract32(insn, 16, 4) << 12;
+                        imm16 |= extract32(insn, 0, 12);
+                        gen_exception_insn(s, 0, EXCP_HVC, syn_aa32_hvc(imm16));
+                    } else {
+                        /* Secure monitor call (v6+) */
+                        gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
+                    }
                 } else {
                     op = (insn >> 20) & 7;
                     switch (op) {