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.or...@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.or...@amd.com>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.hal...@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
