Hi Jiamei, 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....@arm.com> The patch looks good, so: Reviewed-by: Michal Orzel <michal.or...@amd.com>
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. > dabt.reg, vpl011_reg); > - return 0; > + goto read_as_zero; > } > > return 1; This return statement is unreachable. > > +read_as_zero: > + *r = 0; > + return 1; > + > bad_width: > gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n", > dabt.size, dabt.reg, vpl011_reg); > @@ -486,7 +490,7 @@ static int vpl011_mmio_write(struct vcpu *v, > default: > gprintk(XENLOG_ERR, "vpl011: unhandled write r%d offset %#08x\n", > dabt.reg, vpl011_reg); > - return 0; > + goto write_ignore; > } > > write_ignore: > -- > 2.25.1 > > ~Michal
