Re: [RFC PATCH] xen/arm: Add emulation of Debug Data Transfer Registers

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 04/12/2023 20:55, Julien Grall wrote:
> 
> 
> On 04/12/2023 13:02, Ayan Kumar Halder wrote:
>>
>> On 04/12/2023 10:31, Julien Grall wrote:
>>> Hi Ayan,
>> Hi Julien,
>>>
>>> On 01/12/2023 18:50, Ayan Kumar Halder wrote:
>>>> Currently if user enables HVC_DCC config option in Linux, it invokes
>>>> access to debug data transfer registers (ie DBGDTRTX_EL0 on arm64,
>>>> DBGDTRTXINT on arm32). As these registers are not emulated, Xen injects
>>>> an undefined exception to the guest. And Linux crashes.
>>>
>>> I am missing some data points here to be able to say whether I would
>>> be ok with emulating the registers. So some questions:
>>>   * As you wrote below, HVC_DCC will return -ENODEV after this
>>> emulation. So may I ask what's the use case to enable it? (e.g. is
>>> there a distro turning this on?)
>>
>> I am not aware of any distro using (or not using) this feature. This
>> issue came to light during our internal testing, when HVC_DCC was
>> enabled to use the debug console. When Linux runs without Xen, then we
>> could observe the logs on the debug console. When Linux was running as a
>> VM, it crashed.
>>
>> My intention here was to do the bare minimum emulation so that the crash
>> could be avoided.
> This reminds me a bit the discussion around "xen/arm64: Decode ldr/str
> post increment operations". I don't want Xen to contain half-backed
> emulation just to please an OS in certain configuration that doesn't
> seem to be often used.
> 
> Also, AFAICT, KVM is in the same situation...
Well, KVM is not in the same situation. It emulates all DCC regs as RAZ/WI, so 
there
will be no fault on an attempt to access the DCC.

In general, I think that if a register is not optional and does not depend on 
other register
to be checked first (e.g. a feature/control register we emulate), which implies 
that it is fully ok for a guest to
access it directly - we should at least try to do something not to crash a 
guest.

I agree that this feature is not widely used. In fact I can only find it 
implemented in Linux and U-BOOT
and the issue I found in DBGDSCRINT (no access from EL0, even though we emulate 
REXT.UDCCdis as 0) only
proves that. At the same time, it does not cost us much to add this trivial 
support.

> 
> Given this is internal testing, have you considered to ask them to
> disable HVC_DCC?
> 
>>
>>>  * Linux is writing to the registers unconditionally, but is the spec
>>> mandating the implementation of the registers? (I couldn't find either
>>> way)
>>
>>  From ARM DDI 0487I.a ID081822, H1.2, External debug,
>>
>> "The Debug Communications Channel, DCC, passes data between the PE and
>> the debugger:
>>
>> — The DCC includes the data transfer registers, DTRRX and DTRTX, and
>> associated flow-control flags.
>>
>> — Although the DCC is an essential part of Debug state operation, it can
>> also be used in Non-debug state."
>>
>>  From this I infer that the spec mandates the implementation of these
>> registers. IOW, these registers should always be present in any Arm
>> compliant SoC.
>>
>>>  * When was this check introduced in Linux? Did it ever changed?
>>>
>> This check was introduced in the following commit
>>
>> "commit f377775dc083506e2fd7739d8615971c46b5246e
>> Author: Rob Herring <rob.herring@xxxxxxxxxxx>
>> Date:   Tue Sep 24 21:05:58 2013 -0500
>>
>>      TTY: hvc_dcc: probe for a JTAG connection before registering
>>
>>      Enabling the ARM DCC console and using without a JTAG connection will
>>      simply hang the system. Since distros like to turn on all options,
>> this
>>      is a reoccurring problem to debug. We can do better by checking if
>>      anything is attached and handling characters. There is no way to probe
>>      this, so send a newline and check that it is handled.
>> "
> 
> I think this is the part I was missing from the commit message. I have
> proposed some wording below.
> 
>>
>> As of today, this check hasn't changed.
>>
>>>>
>>>> We wish to avoid this crash by adding a "partial" emulation. DBGDTR_EL0
>>>> is emulated as TXfull | RXfull.
>>>
>>> Skimming through the Arm Arm, I see that TXfull and Rxfull indicates
>>> that both buffers are full but it doesn't explicitly say this means
>>> the feature is not available.
>> We are not saying that the feature is not available. Rather ...
>>>
>>> I understand this is what Linux checks, but if we want to partially
>>> emulate the registers in Xen, then I'd like us to make sure this is
>>> backed by the Arm Arm rather than based on Linux implementation (which
>>> can change at any point).
>>>
>>>> Refer ARM DDI 0487I.a ID081822, D17.3.8, DBGDTRTX_EL0
>>>> "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN"
>>>> Also D17.3.7 DBGDTRRX_EL0,
>>>> " If RXfull is set to 1, return the last value written to DTRRX."
>>>>
>>>> Thus, any OS is expected to read DBGDTR_EL0 and check for TXfull
>>>> before using DBGDTRTX_EL0. Linux does it via hvc_dcc_init() --->
>>>> hvc_dcc_check() , it returns -ENODEV. In this way, we are preventing
>>>> the guest to be aborted.
>>>
>>> See above, what guarantees us that Linux will not change this behavior
>>> in the future?
>>
>> If I understand "If TXfull is set to 1, set DTRRX and DTRTX to UNKNOWN"
>> correctly, it seems that Arm ARM expects OS to check for TXfull.
>>
>> If the condition is true, it should return some error.
>>
>> Let me know if I misunderstood this.
> 
> You understand the Arm spec correcly. I think we are disagreeing on the
> wording and whether we should accept basic emulation (see above).
> 
> I would like more opinion on that.
> 
> [...]
> 
>>>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>>>> index 39aeda9dab..3f1276f96e 100644
>>>> --- a/xen/arch/arm/vcpreg.c
>>>> +++ b/xen/arch/arm/vcpreg.c
>>>> @@ -548,20 +548,37 @@ void do_cp14_32(struct cpu_user_regs *regs,
>>>> const union hsr hsr)
>>>>           break;
>>>>       }
>>>>   -    case HSR_CPREG32(DBGDSCRINT):
>>>> +    case HSR_CPREG32(DBGDSCREXT):
>>>> +    {
>>>>           /*
>>>> -         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
>>>> -         * is set to 0, which we emulated below.
>>>> +         * Bit 29: TX full, bit 30: RX full
>>>> +         * Given that we emulate DCC registers as RAZ/WI, doing the
>>>> same for
>>>> +         * DBGDSCRint would cause a guest to continue using the DCC
>>>> despite no
>>>> +         * real effect. Setting the TX/RX status bits should result
>>>> in a probe
>>>> +         * fail (based on Linux behavior).
>>> If you want to mention Linux then you need to be a bit more specific
>>> because Linux can change at any point. So you at least want to specify
>>> the Linux version and place in the code.
>>>
>>> So this doesn't get stale as soon as the HVC_DCC driver changes.
>>
>> (based on Linux behavior since f377775dc083).
> 
> Base on the discussion above, I would like to suggest the following:
> 
> 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 to indicate the transmit buffer is
> full. This we would hint the OS that the DCC is probably not working.
> 
> This would give enough information for the reader to know what's going
> and how you emulate.
> 
> Also, while writing the proposed comment, I wonder why we need to set
> RX? Wouldn't this potentially indicate to the OS that there are some
> data to read?
You're right. No need for that.

~Michal