From: Michal Orzel <michal.orzel@amd.com>
Currently, if user enables HVC_DCC config option in Linux, it invokes access
to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, DBGDTRTXINT on
arm32). As these registers are not emulated, Xen injects an undefined
exception to the guest and Linux crashes.
To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
(these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as TXfull.
Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
"If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> hvc_dcc_check(),
and returns -ENODEV in case TXfull bit is still set after writing a test
character. This way we prevent the guest from making use of HVC DCC as a
console.
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from
v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving the OS any
indication that the RX buffer is full and is waiting to be read.
2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at EL0 only.
3. Fixed the commit message and inline code comments.
v2 :- 1. Split the patch into two (separate patches for arm64 and arm32).
2. Removed the "fail" label.
3. Fixed the commit message.
v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
partial_emulation_enabled is true or not.
2. If partial_emulation_enabled is false, then access to HSR_SYSREG_DBGDTR_EL0,
HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.
xen/arch/arm/arm64/vsysreg.c | 28 ++++++++++++++++++++++++----
xen/arch/arm/include/asm/arm64/hsr.h | 3 +++
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index b5d54c569b..94f0a6c384 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs,
*
* Unhandled:
* MDCCINT_EL1
- * DBGDTR_EL0
- * DBGDTRRX_EL0
- * DBGDTRTX_EL0
* OSDTRRX_EL1
* OSDTRTX_EL1
* OSECCR_EL1
@@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs,
return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
case HSR_SYSREG_MDCCSR_EL0:
/*
+ * Xen doesn't expose a real (or emulated) Debug Communications Channel
+ * (DCC) to a domain. Yet the Arm ARM implies this is not an optional
+ * feature. So some domains may start to probe it. For instance, the
+ * HVC_DCC driver in Linux (since f377775dc083 and at least up to v6.7),
+ * will try to write some characters and check if the transmit buffer
+ * has emptied.
+ *
+ * By setting TX status bit (only if partial emulation is enabled) to
+ * indicate the transmit buffer is full, we would hint the OS that the
+ * DCC is probably not working.
+ *
+ * Bit 29: TX full
+ *
* Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that
* register as RAZ/WI above. So RO at both EL0 and EL1.
*/
- return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
+ return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 0,
+ partial_emulation ? (1U << 29) : 0);
+
+ case HSR_SYSREG_DBGDTR_EL0:
+ /* DBGDTR[TR]X_EL0 share the same encoding */
+ case HSR_SYSREG_DBGDTRTX_EL0:
+ if ( !partial_emulation )
+ goto fail;
+ return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
+
HSR_SYSREG_DBG_CASES(DBGBVR):
HSR_SYSREG_DBG_CASES(DBGBCR):
HSR_SYSREG_DBG_CASES(DBGWVR):
@@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs,
* And all other unknown registers.
*/
default:
+ fail:
{
const struct hsr_sysreg sysreg = hsr.sysreg;
diff --git a/xen/arch/arm/include/asm/arm64/hsr.h b/xen/arch/arm/include/asm/arm64/hsr.h
index e691d41c17..1495ccddea 100644
--- a/xen/arch/arm/include/asm/arm64/hsr.h
+++ b/xen/arch/arm/include/asm/arm64/hsr.h
@@ -47,6 +47,9 @@
#define HSR_SYSREG_OSDLR_EL1 HSR_SYSREG(2,0,c1,c3,4)
#define HSR_SYSREG_DBGPRCR_EL1 HSR_SYSREG(2,0,c1,c4,4)
#define HSR_SYSREG_MDCCSR_EL0 HSR_SYSREG(2,3,c0,c1,0)
+#define HSR_SYSREG_DBGDTR_EL0 HSR_SYSREG(2,3,c0,c4,0)
+#define HSR_SYSREG_DBGDTRTX_EL0 HSR_SYSREG(2,3,c0,c5,0)
+#define HSR_SYSREG_DBGDTRRX_EL0 HSR_SYSREG(2,3,c0,c5,0)
#define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
#define HSR_SYSREG_DBGBCRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,5)
--
2.25.1Hi Ayan,
On 31/01/2024 12:10, Ayan Kumar Halder wrote:
> From: Michal Orzel <michal.orzel@amd.com>
>
> Currently, if user enables HVC_DCC config option in Linux, it invokes access
> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, DBGDTRTXINT on
> arm32). As these registers are not emulated, Xen injects an undefined
> exception to the guest and Linux crashes.
>
> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as TXfull.
>
> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
>
> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> hvc_dcc_check(),
> and returns -ENODEV in case TXfull bit is still set after writing a test
> character. This way we prevent the guest from making use of HVC DCC as a
> console.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from
>
> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving the OS any
> indication that the RX buffer is full and is waiting to be read.
>
> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at EL0 only.
>
> 3. Fixed the commit message and inline code comments.
>
> v2 :- 1. Split the patch into two (separate patches for arm64 and arm32).
> 2. Removed the "fail" label.
> 3. Fixed the commit message.
>
> v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
> partial_emulation_enabled is true or not.
>
> 2. If partial_emulation_enabled is false, then access to HSR_SYSREG_DBGDTR_EL0,
> HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.
>
> xen/arch/arm/arm64/vsysreg.c | 28 ++++++++++++++++++++++++----
> xen/arch/arm/include/asm/arm64/hsr.h | 3 +++
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index b5d54c569b..94f0a6c384 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs,
> *
> * Unhandled:
> * MDCCINT_EL1
> - * DBGDTR_EL0
> - * DBGDTRRX_EL0
> - * DBGDTRTX_EL0
> * OSDTRRX_EL1
> * OSDTRTX_EL1
> * OSECCR_EL1
> @@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs,
> return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> case HSR_SYSREG_MDCCSR_EL0:
> /*
> + * Xen doesn't expose a real (or emulated) Debug Communications Channel
> + * (DCC) to a domain. Yet the Arm ARM implies this is not an optional
> + * feature. So some domains may start to probe it. For instance, the
> + * HVC_DCC driver in Linux (since f377775dc083 and at least up to v6.7),
> + * will try to write some characters and check if the transmit buffer
> + * has emptied.
> + *
> + * By setting TX status bit (only if partial emulation is enabled) to
> + * indicate the transmit buffer is full, we would hint the OS that the
> + * DCC is probably not working.
> + *
> + * Bit 29: TX full
> + *
> * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that
> * register as RAZ/WI above. So RO at both EL0 and EL1.
The sentence "we emulate that register as ..." seems to be stale?
> */
> - return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 0,
> + partial_emulation ? (1U << 29) : 0);
> +
> + case HSR_SYSREG_DBGDTR_EL0:
> + /* DBGDTR[TR]X_EL0 share the same encoding */
> + case HSR_SYSREG_DBGDTRTX_EL0:
> + if ( !partial_emulation )
> + goto fail;
> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
AFAICT, all the emulation helpers have an explanation why we are using
them. But here this is not the case. Can you add one?
> +
> HSR_SYSREG_DBG_CASES(DBGBVR):
> HSR_SYSREG_DBG_CASES(DBGBCR):
> HSR_SYSREG_DBG_CASES(DBGWVR):
> @@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs,
> * And all other unknown registers.
> */
> default:
> + fail:
AFAICT, this would violate MISRA 15.3 [1]. We didn't seem to have yet
(?) accepted the rule, but I don't see we would not given I feel this is
similar to what Rule 16.2 is trying to prevent and we accepted it.
I think case, I move all the code within default outside. And then call
"goto fail" from the default label.
> {
> const struct hsr_sysreg sysreg = hsr.sysreg;
>
> diff --git a/xen/arch/arm/include/asm/arm64/hsr.h b/xen/arch/arm/include/asm/arm64/hsr.h
> index e691d41c17..1495ccddea 100644
> --- a/xen/arch/arm/include/asm/arm64/hsr.h
> +++ b/xen/arch/arm/include/asm/arm64/hsr.h
> @@ -47,6 +47,9 @@
> #define HSR_SYSREG_OSDLR_EL1 HSR_SYSREG(2,0,c1,c3,4)
> #define HSR_SYSREG_DBGPRCR_EL1 HSR_SYSREG(2,0,c1,c4,4)
> #define HSR_SYSREG_MDCCSR_EL0 HSR_SYSREG(2,3,c0,c1,0)
> +#define HSR_SYSREG_DBGDTR_EL0 HSR_SYSREG(2,3,c0,c4,0)
> +#define HSR_SYSREG_DBGDTRTX_EL0 HSR_SYSREG(2,3,c0,c5,0)
> +#define HSR_SYSREG_DBGDTRRX_EL0 HSR_SYSREG(2,3,c0,c5,0)
>
> #define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
> #define HSR_SYSREG_DBGBCRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,5)
Cheers,
[1]
https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_15_03.c
[2]
https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_16_02.c
--
Julien Grall
On 06/02/2024 19:05, Julien Grall wrote:
> Hi Ayan,
Hi Julien/Michal,
>
> On 31/01/2024 12:10, Ayan Kumar Halder wrote:
>> From: Michal Orzel <michal.orzel@amd.com>
>>
>> Currently, if user enables HVC_DCC config option in Linux, it invokes
>> access
>> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64,
>> DBGDTRTXINT on
>> arm32). As these registers are not emulated, Xen injects an undefined
>> exception to the guest and Linux crashes.
>>
>> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
>> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as
>> TXfull.
>>
>> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
>>
>> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
>> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
>> hvc_dcc_check(),
>> and returns -ENODEV in case TXfull bit is still set after writing a test
>> character. This way we prevent the guest from making use of HVC DCC as a
>> console.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> Changes from
>>
>> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving
>> the OS any
>> indication that the RX buffer is full and is waiting to be read.
>>
>> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at
>> EL0 only.
>>
>> 3. Fixed the commit message and inline code comments.
>>
>> v2 :- 1. Split the patch into two (separate patches for arm64 and
>> arm32).
>> 2. Removed the "fail" label.
>> 3. Fixed the commit message.
>>
>> v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
>> partial_emulation_enabled is true or not.
>>
>> 2. If partial_emulation_enabled is false, then access to
>> HSR_SYSREG_DBGDTR_EL0,
>> HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.
>>
>> xen/arch/arm/arm64/vsysreg.c | 28 ++++++++++++++++++++++++----
>> xen/arch/arm/include/asm/arm64/hsr.h | 3 +++
>> 2 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>> index b5d54c569b..94f0a6c384 100644
>> --- a/xen/arch/arm/arm64/vsysreg.c
>> +++ b/xen/arch/arm/arm64/vsysreg.c
>> @@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs,
>> *
>> * Unhandled:
>> * MDCCINT_EL1
>> - * DBGDTR_EL0
>> - * DBGDTRRX_EL0
>> - * DBGDTRTX_EL0
>> * OSDTRRX_EL1
>> * OSDTRTX_EL1
>> * OSECCR_EL1
>> @@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs,
>> return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>> case HSR_SYSREG_MDCCSR_EL0:
>> /*
>> + * Xen doesn't expose a real (or emulated) Debug
>> Communications Channel
>> + * (DCC) to a domain. Yet the Arm ARM implies this is not an
>> optional
>> + * feature. So some domains may start to probe it. For
>> instance, the
>> + * HVC_DCC driver in Linux (since f377775dc083 and at least
>> up to v6.7),
>> + * will try to write some characters and check if the
>> transmit buffer
>> + * has emptied.
>> + *
>> + * By setting TX status bit (only if partial emulation is
>> enabled) to
>> + * indicate the transmit buffer is full, we would hint the
>> OS that the
>> + * DCC is probably not working.
>> + *
>> + * Bit 29: TX full
>> + *
>> * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We
>> emulate that
>> * register as RAZ/WI above. So RO at both EL0 and EL1.
>
> The sentence "we emulate that register as ..." seems to be stale?
>
>> */
>> - return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
>> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read,
>> hsr, 0,
>> + partial_emulation ? (1U << 29) : 0);
>> +
>> + case HSR_SYSREG_DBGDTR_EL0:
>> + /* DBGDTR[TR]X_EL0 share the same encoding */
>> + case HSR_SYSREG_DBGDTRTX_EL0:
>> + if ( !partial_emulation )
>> + goto fail;
>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
>
> AFAICT, all the emulation helpers have an explanation why we are using
> them. But here this is not the case. Can you add one?
This and..
>
>> +
>> HSR_SYSREG_DBG_CASES(DBGBVR):
>> HSR_SYSREG_DBG_CASES(DBGBCR):
>> HSR_SYSREG_DBG_CASES(DBGWVR):
>> @@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>> * And all other unknown registers.
>> */
>> default:
>> + fail:
>
> AFAICT, this would violate MISRA 15.3 [1]. We didn't seem to have yet
> (?) accepted the rule, but I don't see we would not given I feel this
> is similar to what Rule 16.2 is trying to prevent and we accepted it.
>
> I think case, I move all the code within default outside. And then
> call "goto fail" from the default label.
I am not sure if I have interpreted this correctly.
Is it ok if you can take a look at the attached patch and let me know if
the explaination and the code change looks sane ?
- Ayan
>
>> {
>> const struct hsr_sysreg sysreg = hsr.sysreg;
>> diff --git a/xen/arch/arm/include/asm/arm64/hsr.h
>> b/xen/arch/arm/include/asm/arm64/hsr.h
>> index e691d41c17..1495ccddea 100644
>> --- a/xen/arch/arm/include/asm/arm64/hsr.h
>> +++ b/xen/arch/arm/include/asm/arm64/hsr.h
>> @@ -47,6 +47,9 @@
>> #define HSR_SYSREG_OSDLR_EL1 HSR_SYSREG(2,0,c1,c3,4)
>> #define HSR_SYSREG_DBGPRCR_EL1 HSR_SYSREG(2,0,c1,c4,4)
>> #define HSR_SYSREG_MDCCSR_EL0 HSR_SYSREG(2,3,c0,c1,0)
>> +#define HSR_SYSREG_DBGDTR_EL0 HSR_SYSREG(2,3,c0,c4,0)
>> +#define HSR_SYSREG_DBGDTRTX_EL0 HSR_SYSREG(2,3,c0,c5,0)
>> +#define HSR_SYSREG_DBGDTRRX_EL0 HSR_SYSREG(2,3,c0,c5,0)
>> #define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
>> #define HSR_SYSREG_DBGBCRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,5)
>
> Cheers,
>
> [1]
> https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_15_03.c
> [2]
> https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_16_02.c
>From 769efc903eed18839641a003aa63ce64de27bb5f Mon Sep 17 00:00:00 2001
From: Michal Orzel <michal.orzel@amd.com>
Date: Wed, 3 Jan 2024 14:44:22 +0000
Subject: [XEN v5 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer
Registers
To: xen-devel@lists.xenproject.org
Cc: sstabellini@kernel.org,
stefano.stabellini@amd.com,
julien@xen.org,
Volodymyr_Babchuk@epam.com,
bertrand.marquis@arm.com,
michal.orzel@amd.com
Currently, if user enables HVC_DCC config option in Linux, it invokes access
to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, DBGDTRTXINT on
arm32). As these registers are not emulated, Xen injects an undefined
exception to the guest and Linux crashes.
To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
(these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as TXfull.
Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
"If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> hvc_dcc_check(),
and returns -ENODEV in case TXfull bit is still set after writing a test
character. This way we prevent the guest from making use of HVC DCC as a
console.
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
xen/arch/arm/arm64/vsysreg.c | 69 +++++++++++++++++++---------
xen/arch/arm/include/asm/arm64/hsr.h | 3 ++
2 files changed, 50 insertions(+), 22 deletions(-)
diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index b5d54c569b..813b40425d 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs,
*
* Unhandled:
* MDCCINT_EL1
- * DBGDTR_EL0
- * DBGDTRRX_EL0
- * DBGDTRTX_EL0
* OSDTRRX_EL1
* OSDTRTX_EL1
* OSECCR_EL1
@@ -173,10 +170,40 @@ void do_sysreg(struct cpu_user_regs *regs,
return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
case HSR_SYSREG_MDCCSR_EL0:
/*
+ * Xen doesn't expose a real (or emulated) Debug Communications Channel
+ * (DCC) to a domain. Yet the Arm ARM implies this is not an optional
+ * feature. So some domains may start to probe it. For instance, the
+ * HVC_DCC driver in Linux (since f377775dc083 and at least up to v6.7),
+ * will try to write some characters and check if the transmit buffer
+ * has emptied.
+ *
+ * By setting TX status bit (only if partial emulation is enabled) to
+ * indicate the transmit buffer is full, we would hint the OS that the
+ * DCC is probably not working.
+ *
+ * Bit 29: TX full
+ *
* Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that
- * register as RAZ/WI above. So RO at both EL0 and EL1.
+ * register as RO at EL0 and above.
*/
- return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
+ return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 0,
+ partial_emulation ? (1U << 29) : 0);
+
+ case HSR_SYSREG_DBGDTR_EL0:
+ /* DBGDTR[TR]X_EL0 share the same encoding */
+ case HSR_SYSREG_DBGDTRTX_EL0:
+ /*
+ * As stated before, Xen does not support full emulation of Debug
+ * Communications Channel (DCC). Thus, if Xen does not support partial
+ * emulation and access to DBGDTRTX is trapped, it will inject an undef
+ * exception. Otherwise, Xen emulates this as RAZ/WI as it emulates the
+ * TX status as full. Thus, guests are not expected to use DBGDTRTX for
+ * reading/writing.
+ */
+ if ( !partial_emulation )
+ goto fail;
+ return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
+
HSR_SYSREG_DBG_CASES(DBGBVR):
HSR_SYSREG_DBG_CASES(DBGBCR):
HSR_SYSREG_DBG_CASES(DBGWVR):
@@ -394,23 +421,21 @@ void do_sysreg(struct cpu_user_regs *regs,
* And all other unknown registers.
*/
default:
- {
- const struct hsr_sysreg sysreg = hsr.sysreg;
-
- gdprintk(XENLOG_ERR,
- "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
- sysreg.read ? "mrs" : "msr",
- sysreg.op0, sysreg.op1,
- sysreg.crn, sysreg.crm,
- sysreg.op2,
- sysreg.read ? "=>" : "<=",
- sysreg.reg, regs->pc);
- gdprintk(XENLOG_ERR,
- "unhandled 64-bit sysreg access %#"PRIregister"\n",
- hsr.bits & HSR_SYSREG_REGS_MASK);
- inject_undef_exception(regs, hsr);
- return;
- }
+ goto fail;
+ fail:
+
+ const struct hsr_sysreg sysreg = hsr.sysreg;
+
+ gdprintk(XENLOG_ERR,
+ "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
+ sysreg.read ? "mrs" : "msr", sysreg.op0, sysreg.op1, sysreg.crn,
+ sysreg.crm, sysreg.op2, sysreg.read ? "=>" : "<=", sysreg.reg,
+ regs->pc);
+ gdprintk(XENLOG_ERR,
+ "unhandled 64-bit sysreg access %#"PRIregister"\n",
+ hsr.bits & HSR_SYSREG_REGS_MASK);
+ inject_undef_exception(regs, hsr);
+ return;
}
regs->pc += 4;
diff --git a/xen/arch/arm/include/asm/arm64/hsr.h b/xen/arch/arm/include/asm/arm64/hsr.h
index e691d41c17..1495ccddea 100644
--- a/xen/arch/arm/include/asm/arm64/hsr.h
+++ b/xen/arch/arm/include/asm/arm64/hsr.h
@@ -47,6 +47,9 @@
#define HSR_SYSREG_OSDLR_EL1 HSR_SYSREG(2,0,c1,c3,4)
#define HSR_SYSREG_DBGPRCR_EL1 HSR_SYSREG(2,0,c1,c4,4)
#define HSR_SYSREG_MDCCSR_EL0 HSR_SYSREG(2,3,c0,c1,0)
+#define HSR_SYSREG_DBGDTR_EL0 HSR_SYSREG(2,3,c0,c4,0)
+#define HSR_SYSREG_DBGDTRTX_EL0 HSR_SYSREG(2,3,c0,c5,0)
+#define HSR_SYSREG_DBGDTRRX_EL0 HSR_SYSREG(2,3,c0,c5,0)
#define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
#define HSR_SYSREG_DBGBCRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,5)
--
2.25.1
Hi Ayan,
On 19/02/2024 15:45, Ayan Kumar Halder wrote:
>
> On 06/02/2024 19:05, Julien Grall wrote:
>> Hi Ayan,
> Hi Julien/Michal,
>>
>> On 31/01/2024 12:10, Ayan Kumar Halder wrote:
>>> From: Michal Orzel <michal.orzel@amd.com>
>>>
>>> Currently, if user enables HVC_DCC config option in Linux, it invokes
>>> access
>>> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64,
>>> DBGDTRTXINT on
>>> arm32). As these registers are not emulated, Xen injects an undefined
>>> exception to the guest and Linux crashes.
>>>
>>> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0
>>> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as
>>> TXfull.
>>>
>>> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0
>>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN".
>>>
>>> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before
>>> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
>>> hvc_dcc_check(),
>>> and returns -ENODEV in case TXfull bit is still set after writing a test
>>> character. This way we prevent the guest from making use of HVC DCC as a
>>> console.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>> Changes from
>>>
>>> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving
>>> the OS any
>>> indication that the RX buffer is full and is waiting to be read.
>>>
>>> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at
>>> EL0 only.
>>>
>>> 3. Fixed the commit message and inline code comments.
>>>
>>> v2 :- 1. Split the patch into two (separate patches for arm64 and
>>> arm32).
>>> 2. Removed the "fail" label.
>>> 3. Fixed the commit message.
>>>
>>> v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether
>>> partial_emulation_enabled is true or not.
>>>
>>> 2. If partial_emulation_enabled is false, then access to
>>> HSR_SYSREG_DBGDTR_EL0,
>>> HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception.
>>>
>>> xen/arch/arm/arm64/vsysreg.c | 28 ++++++++++++++++++++++++----
>>> xen/arch/arm/include/asm/arm64/hsr.h | 3 +++
>>> 2 files changed, 27 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>>> index b5d54c569b..94f0a6c384 100644
>>> --- a/xen/arch/arm/arm64/vsysreg.c
>>> +++ b/xen/arch/arm/arm64/vsysreg.c
>>> @@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs,
>>> *
>>> * Unhandled:
>>> * MDCCINT_EL1
>>> - * DBGDTR_EL0
>>> - * DBGDTRRX_EL0
>>> - * DBGDTRTX_EL0
>>> * OSDTRRX_EL1
>>> * OSDTRTX_EL1
>>> * OSECCR_EL1
>>> @@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs,
>>> return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>> case HSR_SYSREG_MDCCSR_EL0:
>>> /*
>>> + * Xen doesn't expose a real (or emulated) Debug
>>> Communications Channel
>>> + * (DCC) to a domain. Yet the Arm ARM implies this is not an
>>> optional
>>> + * feature. So some domains may start to probe it. For
>>> instance, the
>>> + * HVC_DCC driver in Linux (since f377775dc083 and at least
>>> up to v6.7),
>>> + * will try to write some characters and check if the
>>> transmit buffer
>>> + * has emptied.
>>> + *
>>> + * By setting TX status bit (only if partial emulation is
>>> enabled) to
>>> + * indicate the transmit buffer is full, we would hint the
>>> OS that the
>>> + * DCC is probably not working.
>>> + *
>>> + * Bit 29: TX full
>>> + *
>>> * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We
>>> emulate that
>>> * register as RAZ/WI above. So RO at both EL0 and EL1.
>>
>> The sentence "we emulate that register as ..." seems to be stale?
I can see that you tried to handle Julien remark here. But I disagree. This statement
is not stale. It explains that because MDSCR_EL1 is RAZ/WI, MDCCSR_EL0 is RO at both
EL0 and EL1. This patch does not change this behavior.
>>
>>> */
>>> - return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
>>> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read,
>>> hsr, 0,
>>> + partial_emulation ? (1U << 29) : 0);
>>> +
>>> + case HSR_SYSREG_DBGDTR_EL0:
>>> + /* DBGDTR[TR]X_EL0 share the same encoding */
>>> + case HSR_SYSREG_DBGDTRTX_EL0:
>>> + if ( !partial_emulation )
>>> + goto fail;
>>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0);
>>
>> AFAICT, all the emulation helpers have an explanation why we are using
>> them. But here this is not the case. Can you add one?
> This and..
>>
>>> +
>>> HSR_SYSREG_DBG_CASES(DBGBVR):
>>> HSR_SYSREG_DBG_CASES(DBGBCR):
>>> HSR_SYSREG_DBG_CASES(DBGWVR):
>>> @@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>>> * And all other unknown registers.
>>> */
>>> default:
>>> + fail:
>>
>> AFAICT, this would violate MISRA 15.3 [1]. We didn't seem to have yet
>> (?) accepted the rule, but I don't see we would not given I feel this
>> is similar to what Rule 16.2 is trying to prevent and we accepted it.
>>
>> I think case, I move all the code within default outside. And then
>> call "goto fail" from the default label.
>
> I am not sure if I have interpreted this correctly.
>
> Is it ok if you can take a look at the attached patch and let me know if
> the explaination and the code change looks sane ?
Looking at the attached patch and fail handling, I don't think it is what Julien meant.
In the default case you should jump to fail that would be defined outside of switch clause.
Something like:
default:
goto fail;
}
regs->pc += 4;
return;
fail:
gdprintk...
When it comes to explanation for HSR_SYSREG_DBGDTRTX_EL0, I will let Julien to provide a comment he believes is right.
To me, it feels strange to repeat almost the same information as for MDCCSR_EL0.
~Michal
Hi Michal, On 19/02/2024 15:43, Michal Orzel wrote: > On 19/02/2024 15:45, Ayan Kumar Halder wrote: >> >> On 06/02/2024 19:05, Julien Grall wrote: >>> Hi Ayan, >> Hi Julien/Michal, >>> >>> On 31/01/2024 12:10, Ayan Kumar Halder wrote: >>>> From: Michal Orzel <michal.orzel@amd.com> >>>> >>>> Currently, if user enables HVC_DCC config option in Linux, it invokes >>>> access >>>> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, >>>> DBGDTRTXINT on >>>> arm32). As these registers are not emulated, Xen injects an undefined >>>> exception to the guest and Linux crashes. >>>> >>>> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0 >>>> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as >>>> TXfull. >>>> >>>> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0 >>>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN". >>>> >>>> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before >>>> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> >>>> hvc_dcc_check(), >>>> and returns -ENODEV in case TXfull bit is still set after writing a test >>>> character. This way we prevent the guest from making use of HVC DCC as a >>>> console. >>>> >>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>>> --- >>>> Changes from >>>> >>>> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving >>>> the OS any >>>> indication that the RX buffer is full and is waiting to be read. >>>> >>>> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at >>>> EL0 only. >>>> >>>> 3. Fixed the commit message and inline code comments. >>>> >>>> v2 :- 1. Split the patch into two (separate patches for arm64 and >>>> arm32). >>>> 2. Removed the "fail" label. >>>> 3. Fixed the commit message. >>>> >>>> v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether >>>> partial_emulation_enabled is true or not. >>>> >>>> 2. If partial_emulation_enabled is false, then access to >>>> HSR_SYSREG_DBGDTR_EL0, >>>> HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception. >>>> >>>> xen/arch/arm/arm64/vsysreg.c | 28 ++++++++++++++++++++++++---- >>>> xen/arch/arm/include/asm/arm64/hsr.h | 3 +++ >>>> 2 files changed, 27 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c >>>> index b5d54c569b..94f0a6c384 100644 >>>> --- a/xen/arch/arm/arm64/vsysreg.c >>>> +++ b/xen/arch/arm/arm64/vsysreg.c >>>> @@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs, >>>> * >>>> * Unhandled: >>>> * MDCCINT_EL1 >>>> - * DBGDTR_EL0 >>>> - * DBGDTRRX_EL0 >>>> - * DBGDTRTX_EL0 >>>> * OSDTRRX_EL1 >>>> * OSDTRTX_EL1 >>>> * OSECCR_EL1 >>>> @@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs, >>>> return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); >>>> case HSR_SYSREG_MDCCSR_EL0: >>>> /* >>>> + * Xen doesn't expose a real (or emulated) Debug >>>> Communications Channel >>>> + * (DCC) to a domain. Yet the Arm ARM implies this is not an >>>> optional >>>> + * feature. So some domains may start to probe it. For >>>> instance, the >>>> + * HVC_DCC driver in Linux (since f377775dc083 and at least >>>> up to v6.7), >>>> + * will try to write some characters and check if the >>>> transmit buffer >>>> + * has emptied. >>>> + * >>>> + * By setting TX status bit (only if partial emulation is >>>> enabled) to >>>> + * indicate the transmit buffer is full, we would hint the >>>> OS that the >>>> + * DCC is probably not working. >>>> + * >>>> + * Bit 29: TX full >>>> + * >>>> * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We >>>> emulate that >>>> * register as RAZ/WI above. So RO at both EL0 and EL1. >>> >>> The sentence "we emulate that register as ..." seems to be stale? > I can see that you tried to handle Julien remark here. But I disagree. This statement > is not stale. It explains that because MDSCR_EL1 is RAZ/WI, MDCCSR_EL0 is RO at both > EL0 and EL1. This patch does not change this behavior. Indeed. I misread the comment. So what I wrote can be ignored here. > >>> >>>> */ >>>> - return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); >>>> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read, >>>> hsr, 0, >>>> + partial_emulation ? (1U << 29) : 0); >>>> + >>>> + case HSR_SYSREG_DBGDTR_EL0: >>>> + /* DBGDTR[TR]X_EL0 share the same encoding */ >>>> + case HSR_SYSREG_DBGDTRTX_EL0: >>>> + if ( !partial_emulation ) >>>> + goto fail; >>>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0); >>> >>> AFAICT, all the emulation helpers have an explanation why we are using >>> them. But here this is not the case. Can you add one? >> This and.. >>> >>>> + >>>> HSR_SYSREG_DBG_CASES(DBGBVR): >>>> HSR_SYSREG_DBG_CASES(DBGBCR): >>>> HSR_SYSREG_DBG_CASES(DBGWVR): >>>> @@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs, >>>> * And all other unknown registers. >>>> */ >>>> default: >>>> + fail: >>> >>> AFAICT, this would violate MISRA 15.3 [1]. We didn't seem to have yet >>> (?) accepted the rule, but I don't see we would not given I feel this >>> is similar to what Rule 16.2 is trying to prevent and we accepted it. >>> >>> I think case, I move all the code within default outside. And then >>> call "goto fail" from the default label. >> >> I am not sure if I have interpreted this correctly. >> >> Is it ok if you can take a look at the attached patch and let me know if >> the explaination and the code change looks sane ? > Looking at the attached patch and fail handling, I don't think it is what Julien meant. > In the default case you should jump to fail that would be defined outside of switch clause. > > Something like: > default: > goto fail; > } > > regs->pc += 4; > return; > > fail: > gdprintk... +1. > > When it comes to explanation for HSR_SYSREG_DBGDTRTX_EL0, I will let Julien to provide a comment he believes is right. > To me, it feels strange to repeat almost the same information as for MDCCSR_EL0. It is not clear to me whether you are objecting of adding a comment or whether you would be ok with a comment that is not duplicating. If the latter, you could move the first paragraph outside of the case and then have a register specific explanation of the implementation. Cheers, -- Julien Grall
Hi Julien, On 19/02/2024 19:48, Julien Grall wrote: > > > Hi Michal, > > On 19/02/2024 15:43, Michal Orzel wrote: > >> On 19/02/2024 15:45, Ayan Kumar Halder wrote: >>> >>> On 06/02/2024 19:05, Julien Grall wrote: >>>> Hi Ayan, >>> Hi Julien/Michal, >>>> >>>> On 31/01/2024 12:10, Ayan Kumar Halder wrote: >>>>> From: Michal Orzel <michal.orzel@amd.com> >>>>> >>>>> Currently, if user enables HVC_DCC config option in Linux, it invokes >>>>> access >>>>> to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, >>>>> DBGDTRTXINT on >>>>> arm32). As these registers are not emulated, Xen injects an undefined >>>>> exception to the guest and Linux crashes. >>>>> >>>>> To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0 >>>>> (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as >>>>> TXfull. >>>>> >>>>> Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0 >>>>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN". >>>>> >>>>> Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before >>>>> using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> >>>>> hvc_dcc_check(), >>>>> and returns -ENODEV in case TXfull bit is still set after writing a test >>>>> character. This way we prevent the guest from making use of HVC DCC as a >>>>> console. >>>>> >>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>>>> --- >>>>> Changes from >>>>> >>>>> v1 :- 1. DBGDTR_EL0 does not emulate RXfull. This is to avoid giving >>>>> the OS any >>>>> indication that the RX buffer is full and is waiting to be read. >>>>> >>>>> 2. In Arm32, DBGOSLSR is emulated. Also DBGDTRTXINT is emulated at >>>>> EL0 only. >>>>> >>>>> 3. Fixed the commit message and inline code comments. >>>>> >>>>> v2 :- 1. Split the patch into two (separate patches for arm64 and >>>>> arm32). >>>>> 2. Removed the "fail" label. >>>>> 3. Fixed the commit message. >>>>> >>>>> v3 :- 1. "HSR_SYSREG_MDCCSR_EL0" emulation differs based on whether >>>>> partial_emulation_enabled is true or not. >>>>> >>>>> 2. If partial_emulation_enabled is false, then access to >>>>> HSR_SYSREG_DBGDTR_EL0, >>>>> HSR_SYSREG_DBGDTRTX_EL0 would lead to undefined exception. >>>>> >>>>> xen/arch/arm/arm64/vsysreg.c | 28 ++++++++++++++++++++++++---- >>>>> xen/arch/arm/include/asm/arm64/hsr.h | 3 +++ >>>>> 2 files changed, 27 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c >>>>> index b5d54c569b..94f0a6c384 100644 >>>>> --- a/xen/arch/arm/arm64/vsysreg.c >>>>> +++ b/xen/arch/arm/arm64/vsysreg.c >>>>> @@ -159,9 +159,6 @@ void do_sysreg(struct cpu_user_regs *regs, >>>>> * >>>>> * Unhandled: >>>>> * MDCCINT_EL1 >>>>> - * DBGDTR_EL0 >>>>> - * DBGDTRRX_EL0 >>>>> - * DBGDTRTX_EL0 >>>>> * OSDTRRX_EL1 >>>>> * OSDTRTX_EL1 >>>>> * OSECCR_EL1 >>>>> @@ -173,10 +170,32 @@ void do_sysreg(struct cpu_user_regs *regs, >>>>> return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); >>>>> case HSR_SYSREG_MDCCSR_EL0: >>>>> /* >>>>> + * Xen doesn't expose a real (or emulated) Debug >>>>> Communications Channel >>>>> + * (DCC) to a domain. Yet the Arm ARM implies this is not an >>>>> optional >>>>> + * feature. So some domains may start to probe it. For >>>>> instance, the >>>>> + * HVC_DCC driver in Linux (since f377775dc083 and at least >>>>> up to v6.7), >>>>> + * will try to write some characters and check if the >>>>> transmit buffer >>>>> + * has emptied. >>>>> + * >>>>> + * By setting TX status bit (only if partial emulation is >>>>> enabled) to >>>>> + * indicate the transmit buffer is full, we would hint the >>>>> OS that the >>>>> + * DCC is probably not working. >>>>> + * >>>>> + * Bit 29: TX full >>>>> + * >>>>> * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We >>>>> emulate that >>>>> * register as RAZ/WI above. So RO at both EL0 and EL1. >>>> >>>> The sentence "we emulate that register as ..." seems to be stale? >> I can see that you tried to handle Julien remark here. But I disagree. This statement >> is not stale. It explains that because MDSCR_EL1 is RAZ/WI, MDCCSR_EL0 is RO at both >> EL0 and EL1. This patch does not change this behavior. > > Indeed. I misread the comment. So what I wrote can be ignored here. > >> >>>> >>>>> */ >>>>> - return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); >>>>> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read, >>>>> hsr, 0, >>>>> + partial_emulation ? (1U << 29) : 0); >>>>> + >>>>> + case HSR_SYSREG_DBGDTR_EL0: >>>>> + /* DBGDTR[TR]X_EL0 share the same encoding */ >>>>> + case HSR_SYSREG_DBGDTRTX_EL0: >>>>> + if ( !partial_emulation ) >>>>> + goto fail; >>>>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 0); >>>> >>>> AFAICT, all the emulation helpers have an explanation why we are using >>>> them. But here this is not the case. Can you add one? >>> This and.. >>>> >>>>> + >>>>> HSR_SYSREG_DBG_CASES(DBGBVR): >>>>> HSR_SYSREG_DBG_CASES(DBGBCR): >>>>> HSR_SYSREG_DBG_CASES(DBGWVR): >>>>> @@ -394,6 +413,7 @@ void do_sysreg(struct cpu_user_regs *regs, >>>>> * And all other unknown registers. >>>>> */ >>>>> default: >>>>> + fail: >>>> >>>> AFAICT, this would violate MISRA 15.3 [1]. We didn't seem to have yet >>>> (?) accepted the rule, but I don't see we would not given I feel this >>>> is similar to what Rule 16.2 is trying to prevent and we accepted it. >>>> >>>> I think case, I move all the code within default outside. And then >>>> call "goto fail" from the default label. >>> >>> I am not sure if I have interpreted this correctly. >>> >>> Is it ok if you can take a look at the attached patch and let me know if >>> the explaination and the code change looks sane ? >> Looking at the attached patch and fail handling, I don't think it is what Julien meant. >> In the default case you should jump to fail that would be defined outside of switch clause. >> >> Something like: >> default: >> goto fail; >> } >> >> regs->pc += 4; >> return; >> >> fail: >> gdprintk... > > +1. > >> >> When it comes to explanation for HSR_SYSREG_DBGDTRTX_EL0, I will let Julien to provide a comment he believes is right. >> To me, it feels strange to repeat almost the same information as for MDCCSR_EL0. > > It is not clear to me whether you are objecting of adding a comment or > whether you would be ok with a comment that is not duplicating. Adding a comment is always welcome. I was against duplication. @Ayan: Move a paragraph starting with "Xen doesn't expose" above case HSR_SYSREG_MDCCSR_EL0 and leave rest as is. For HSR_SYSREG_DBGDTRTX_EL0, add sth like: Emulate as RAZ/WI (only if partial emulation is enabled) to prevent injecting undefined exception. Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that register as RAZ/WI. ~Michal
Hi Ayan, On 31/01/2024 13:10, Ayan Kumar Halder wrote: > From: Michal Orzel <michal.orzel@amd.com> > > Currently, if user enables HVC_DCC config option in Linux, it invokes access > to debug data transfer registers (i.e. DBGDTRTX_EL0 on arm64, DBGDTRTXINT on > arm32). As these registers are not emulated, Xen injects an undefined > exception to the guest and Linux crashes. > > To prevent this crash, introduce a partial emulation of DBGDTR[TR]X_EL0 > (these registers share the same encoding) as RAZ/WI and MDCCSR_EL0 as TXfull. > > Refer ARM DDI 0487J.a ID042523, D19.3.8, DBGDTRTX_EL0 > "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN". > > Thus, any OS is expected to read MDCCSR_EL0 and check for TXfull before > using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() ---> hvc_dcc_check(), > and returns -ENODEV in case TXfull bit is still set after writing a test > character. This way we prevent the guest from making use of HVC DCC as a > console. > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
© 2016 - 2026 Red Hat, Inc.