Re: [PATCH v4 1/2] xen/arm: vpl011: emulate non-SBSA registers as WI/RAZ

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 01/12/2022 09:52, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 01/12/2022 09:42, Michal Orzel wrote:
>> On 01/12/2022 09:03, Jiamei Xie wrote:
>>>
>>>
>>> When the guest kernel enables DMA engine with "CONFIG_DMA_ENGINE=y",
>>> Linux SBSA PL011 driver will access PL011 DMACR register in some
>>> functions. As chapter "B Generic UART" in "ARM Server Base System
>>> Architecture"[1] documentation describes, SBSA UART doesn't support
>>> DMA. In current code, when the kernel tries to access DMACR register,
>>> Xen will inject a data abort:
>>> Unhandled fault at 0xffffffc00944d048
>>> Mem abort info:
>>>    ESR = 0x96000000
>>>    EC = 0x25: DABT (current EL), IL = 32 bits
>>>    SET = 0, FnV = 0
>>>    EA = 0, S1PTW = 0
>>>    FSC = 0x00: ttbr address size fault
>>> Data abort info:
>>>    ISV = 0, ISS = 0x00000000
>>>    CM = 0, WnR = 0
>>> swapper pgtable: 4k pages, 39-bit VAs, pgdp=0000000020e2e000
>>> [ffffffc00944d048] pgd=100000003ffff803, p4d=100000003ffff803, 
>>> pud=100000003ffff803, pmd=100000003fffa803, pte=006800009c090f13
>>> Internal error: ttbr address size fault: 96000000 [#1] PREEMPT SMP
>>> ...
>>> Call trace:
>>>   pl011_stop_rx+0x70/0x80
>>>   tty_port_shutdown+0x7c/0xb4
>>>   tty_port_close+0x60/0xcc
>>>   uart_close+0x34/0x8c
>>>   tty_release+0x144/0x4c0
>>>   __fput+0x78/0x220
>>>   ____fput+0x1c/0x30
>>>   task_work_run+0x88/0xc0
>>>   do_notify_resume+0x8d0/0x123c
>>>   el0_svc+0xa8/0xc0
>>>   el0t_64_sync_handler+0xa4/0x130
>>>   el0t_64_sync+0x1a0/0x1a4
>>> Code: b9000083 b901f001 794038a0 8b000042 (b9000041)
>>> ---[ end trace 83dd93df15c3216f ]---
>>> note: bootlogd[132] exited with preempt_count 1
>>> /etc/rcS.d/S07bootlogd: line 47: 132 Segmentation fault start-stop-daemon
>>>
>>> As discussed in [2], this commit makes the access to non-SBSA registers
>>> RAZ/WI as an improvement.
>>>
>>> [1] 
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeveloper.arm.com%2Fdocumentation%2Fden0094%2Fc%2F%3Flang%3Den&data=05%7C01%7Cmichal.orzel%40amd.com%7C84567fc1ef8b4a0b314e08dad3796005%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638054815513527676%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5Sa0xLx%2Bje0bxXssHrNz7tnV8QmbTD0FvjeenlsLGaw%3D&reserved=0
>>> [2] 
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fxen-devel%2Falpine.DEB.2.22.394.2211161552420.4020%40ubuntu-linux-20-04-desktop%2F&data=05%7C01%7Cmichal.orzel%40amd.com%7C84567fc1ef8b4a0b314e08dad3796005%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638054815513527676%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=AjAaBk1N2amRLW4CKrAlqqbAvxi1SUWZwZDpOf6%2FgfU%3D&reserved=0
>>>
>>> Signed-off-by: Jiamei Xie <jiamei.xie@xxxxxxx>
>> The patch looks good, so:
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>>
>> However, because your series is about vpl011 refinement, I spotted two things
>> (this does not necessarily needs to be done by you).
>>
>>> ---
>>> v3 -> v4
>>> - remove the size check for unknown registers in the SBSA UART
>>> - remove lock in read_as_zero
>>> v2 -> v3
>>> - emulate non-SBSA registers as WI/RAZ in default case
>>> - update commit message
>>> v1 -> v2
>>> - print a message using XENLOG_G_DEBUG when it's write-ignore
>>> ---
>>>   xen/arch/arm/vpl011.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>> index 43522d48fd..f4a5621fab 100644
>>> --- a/xen/arch/arm/vpl011.c
>>> +++ b/xen/arch/arm/vpl011.c
>>> @@ -414,11 +414,15 @@ static int vpl011_mmio_read(struct vcpu *v,
>>>       default:
>>>           gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
>> This is an emulated UART MMIO handler, so instead XENLOG_ERR, we should use 
>> XENLOG_G_ERR
>> to indicate gust error and not Xen error.
> 
> That's already the case because the code is using gprintk() which will
> append XENLOG_GUEST to the log level and also print the current vCPU.
Ups. I did not notice g prefix in front of printk by accident.
So we're all good in this case.

> 
> Otherwise this would have been considered a security issue because
> XENLOG_ERR is not ratelimited the same way as XENLOG_G_ERR by default.
> 
> Cheers,
> 
> --
> Julien Grall

~Michal