Re: [XEN v4 2/3] xen/arm: arm64: Add emulation of Debug Data Transfer Registers

[Michal Orzel <michal.orzel@xxxxxxx>]

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@xxxxxxx>
>>>>>
>>>>> 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@xxxxxxx>
>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
>>>>> ---
>>>>> 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