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

[Julien Grall <julien@xxxxxxx>]

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://developer.arm.com/documentation/den0094/c/?lang=en
> > [2] 
> > https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2211161552420.4020@ubuntu-linux-20-04-desktop/
> >
> > 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.
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