From: Marc Zyngier <marc.zyngier@arm.com>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>,
Christoffer Dall <christoffer.dall@linaro.org>,
kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org
Subject: Re: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP
Date: Mon, 8 Feb 2016 16:45:44 +0000 [thread overview]
Message-ID: <56B8C638.2010402@arm.com> (raw)
In-Reply-To: <20160208155625.GQ6076@e104818-lin.cambridge.arm.com>
On 08/02/16 15:56, Catalin Marinas wrote:
> On Wed, Feb 03, 2016 at 06:00:14PM +0000, Marc Zyngier wrote:
>> @@ -76,6 +59,36 @@ static inline void decode_ctrl_reg(u32 reg,
>> #define ARM_KERNEL_STEP_ACTIVE 1
>> #define ARM_KERNEL_STEP_SUSPEND 2
>>
>> +#define DBG_HMC_HYP (1 << 13)
>> +#define DBG_SSC_HYP (3 << 14)
>> +
>> +static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
>> +{
>> + u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled;
>> +
>> + if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
>> + val |= DBG_HMC_HYP | DBG_SSC_HYP;
>> + else
>> + val |= ctrl.privilege << 1;
>> +
>> + return val;
>> +}
>> +
>> +static inline void decode_ctrl_reg(u32 reg,
>> + struct arch_hw_breakpoint_ctrl *ctrl)
>> +{
>> + ctrl->enabled = reg & 0x1;
>> + reg >>= 1;
>> + if (is_kernel_in_hyp_mode())
>> + ctrl->privilege = !!(reg & (DBG_HMC_HYP >> 1));
>
> I don't particularly like this part as it's not clear just by looking at
> the code that it, in fact, generates AARCH64_BREAKPOINT_EL1. I would
> make this clearer:
>
> if (is_kernel_in_hyp_mode() && (reg & (DBG_HMC_HYP >> 1)))
> ctrl->privilege = AARCH64_BREAKPOINT_EL1;
>
> Alternatively, you could define the PMC field value as:
>
> #define AARCH64_BREAKPOINT_EL2 0
>
> and change the privilege to EL1 after masking, something like:
>
> ctrl->privilege = reg & 0x3;
> if (ctrl->privilege == AARCH64_BREAKPOINT_EL2)
> ctrl->privilege = AARCH64_BREAKPOINT_EL1;
>
> BTW, do we need to check is_kernel_in_hyp_mode() when decoding? Is there
> anything else that could have set this SSC/HMC/PMC fields other than
> encode_ctrl_reg()?
I was being overzealous, and your solution is clearly better. I ended up with the following:
diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
index 9732908..c872b2f 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -18,6 +18,7 @@
#include <asm/cputype.h>
#include <asm/cpufeature.h>
+#include <asm/virt.h>
#ifdef __KERNEL__
@@ -35,24 +36,6 @@ struct arch_hw_breakpoint {
struct arch_hw_breakpoint_ctrl ctrl;
};
-static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
-{
- return (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) |
- ctrl.enabled;
-}
-
-static inline void decode_ctrl_reg(u32 reg,
- struct arch_hw_breakpoint_ctrl *ctrl)
-{
- ctrl->enabled = reg & 0x1;
- reg >>= 1;
- ctrl->privilege = reg & 0x3;
- reg >>= 2;
- ctrl->type = reg & 0x3;
- reg >>= 2;
- ctrl->len = reg & 0xff;
-}
-
/* Breakpoint */
#define ARM_BREAKPOINT_EXECUTE 0
@@ -62,6 +45,7 @@ static inline void decode_ctrl_reg(u32 reg,
#define AARCH64_ESR_ACCESS_MASK (1 << 6)
/* Privilege Levels */
+#define AARCH64_BREAKPOINT_EL2 0
#define AARCH64_BREAKPOINT_EL1 1
#define AARCH64_BREAKPOINT_EL0 2
@@ -76,6 +60,35 @@ static inline void decode_ctrl_reg(u32 reg,
#define ARM_KERNEL_STEP_ACTIVE 1
#define ARM_KERNEL_STEP_SUSPEND 2
+#define DBG_HMC_HYP (1 << 13)
+#define DBG_SSC_HYP (3 << 14)
+
+static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
+{
+ u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled;
+
+ if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
+ val |= DBG_HMC_HYP | DBG_SSC_HYP;
+ else
+ val |= ctrl.privilege << 1;
+
+ return val;
+}
+
+static inline void decode_ctrl_reg(u32 reg,
+ struct arch_hw_breakpoint_ctrl *ctrl)
+{
+ ctrl->enabled = reg & 0x1;
+ reg >>= 1;
+ ctrl->privilege = reg & 0x3;
+ if (ctrl->privilege == AARCH64_BREAKPOINT_EL2)
+ ctrl->privilege = AARCH64_BREAKPOINT_EL1;
+ reg >>= 2;
+ ctrl->type = reg & 0x3;
+ reg >>= 2;
+ ctrl->len = reg & 0xff;
+}
+
/*
* Limits.
* Changing these will require modifications to the register accessors.
Was that what you had in mind?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
next prev parent reply other threads:[~2016-02-08 16:45 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-03 17:59 [PATCH v3 00/23] arm64: Virtualization Host Extension support Marc Zyngier
2016-02-03 17:59 ` [PATCH v3 01/23] arm/arm64: KVM: Add hook for C-based stage2 init Marc Zyngier
2016-02-03 17:59 ` [PATCH v3 02/23] arm64: KVM: Switch to " Marc Zyngier
2016-02-03 17:59 ` [PATCH v3 03/23] arm/arm64: Add new is_kernel_in_hyp_mode predicate Marc Zyngier
2016-02-08 14:41 ` Catalin Marinas
2016-02-03 17:59 ` [PATCH v3 04/23] arm64: Allow the arch timer to use the HYP timer Marc Zyngier
2016-02-03 17:59 ` [PATCH v3 05/23] arm64: Add ARM64_HAS_VIRT_HOST_EXTN feature Marc Zyngier
2016-02-08 14:42 ` Catalin Marinas
2016-02-03 17:59 ` [PATCH v3 06/23] arm64: KVM: Skip HYP setup when already running in HYP Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 07/23] arm64: KVM: VHE: Patch out use of HVC Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 08/23] arm64: KVM: VHE: Patch out kern_hyp_va Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 09/23] arm64: KVM: VHE: Introduce unified system register accessors Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 10/23] arm64: KVM: VHE: Differenciate host/guest sysreg save/restore Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 11/23] arm64: KVM: VHE: Split save/restore of registers shared between guest and host Marc Zyngier
2016-02-04 19:15 ` Christoffer Dall
2016-02-03 18:00 ` [PATCH v3 12/23] arm64: KVM: VHE: Use unified system register accessors Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 13/23] arm64: KVM: VHE: Enable minimal sysreg save/restore Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 14/23] arm64: KVM: VHE: Make __fpsimd_enabled VHE aware Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 15/23] arm64: KVM: VHE: Implement VHE activate/deactivate_traps Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 16/23] arm64: KVM: VHE: Use unified sysreg accessors for timer Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 17/23] arm64: KVM: VHE: Add fpsimd enabling on guest access Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 18/23] arm64: KVM: VHE: Add alternative panic handling Marc Zyngier
2016-02-03 18:00 ` [PATCH v3 19/23] arm64: KVM: Move most of the fault decoding to C Marc Zyngier
2016-02-04 19:20 ` Christoffer Dall
2016-02-08 14:44 ` Catalin Marinas
2016-02-03 18:00 ` [PATCH v3 20/23] arm64: perf: Count EL2 events if the kernel is running in HYP Marc Zyngier
2016-02-04 19:23 ` Christoffer Dall
2016-02-05 9:02 ` Marc Zyngier
2016-02-08 14:48 ` Catalin Marinas
2016-02-03 18:00 ` [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if " Marc Zyngier
2016-02-08 15:56 ` Catalin Marinas
2016-02-08 16:45 ` Marc Zyngier [this message]
2016-02-08 16:52 ` Catalin Marinas
2016-02-03 18:00 ` [PATCH v3 22/23] arm64: VHE: Add support for running Linux in EL2 mode Marc Zyngier
2016-02-04 19:25 ` Christoffer Dall
2016-02-08 15:58 ` Catalin Marinas
2016-02-03 18:00 ` [PATCH v3 23/23] arm64: Panic when VHE and non VHE CPUs coexist Marc Zyngier
2016-02-04 19:25 ` Christoffer Dall
2016-02-08 16:04 ` Catalin Marinas
2016-02-08 16:24 ` Mark Rutland
2016-02-09 12:02 ` Catalin Marinas
2016-02-04 19:26 ` [PATCH v3 00/23] arm64: Virtualization Host Extension support Christoffer Dall
2016-02-05 8:56 ` Marc Zyngier
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=56B8C638.2010402@arm.com \
--to=marc.zyngier@arm.com \
--cc=catalin.marinas@arm.com \
--cc=christoffer.dall@linaro.org \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=will.deacon@arm.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).