RE: [PATCH v2] xen/arm: vpl011: Make access to DMACR write-ignore

[Jiamei Xie <Jiamei.Xie@xxxxxxx>]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@xxxxxxx>
> Sent: Tuesday, November 22, 2022 11:17 PM
> To: Julien Grall <julien@xxxxxxx>; Jiamei Xie <Jiamei.Xie@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v2] xen/arm: vpl011: Make access to DMACR write-
> ignore
> 
> Hi,
> 
> On 22/11/2022 13:25, Julien Grall wrote:
> >
> >
> > Hi,
> >
> > On 22/11/2022 05:46, 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 DMACR register
> >> write-ignore as an improvement.
> >
> > Didn't we agree to emulate all non-SBSA registers as WI? IOW, the
> > default case should contain a 'goto write_ignore' rather return 0.
> + we also agreed on emulating the reads to non spec compliant registers as
> RAZ.

Thanks for reminding me of this. I'll  emulating the reads to non spec 
compliant registers as
RAZ in patch v3.

Best wishes
Jiamei Xie

> 
> >
> >>
> >> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeve
> loper.arm.com%2Fdocumentation%2Fden0094%2Fc%2F%3Flang%3Den&am
> p;data=05%7C01%7Cmichal.orzel%40amd.com%7C1065702b4fd2457cdbf80
> 8dacc84b45a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6380
> 47167600786580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAi
> LCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&
> amp;sdata=W1dbakw6lkGkv4ydElIi%2Ba7uT7e7Pt5dB3vDtYpP%2FqQ%3D&a
> mp;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&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7C1065702
> b4fd2457cdbf808dacc84b45a%7C3dd8961fe4884e608e11a82d994e183d%7C
> 0%7C0%7C638047167600786580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C30
> 00%7C%7C%7C&amp;sdata=O4zxuI3HqRA1bdraGcVVY8vV0HGbqOI3nFa%2F
> ciC1cGQ%3D&amp;reserved=0
> >>
> >> Signed-off-by: Jiamei Xie <jiamei.xie@xxxxxxx>
> >> ---
> >>   xen/arch/arm/vpl011.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> >> index 43522d48fd..e97fe3ebe7 100644
> >> --- a/xen/arch/arm/vpl011.c
> >> +++ b/xen/arch/arm/vpl011.c
> >> @@ -463,6 +463,10 @@ static int vpl011_mmio_write(struct vcpu *v,
> >>       case FR:
> >>       case RIS:
> >>       case MIS:
> >> +    case DMACR:
> >> +        printk(XENLOG_G_DEBUG
> >> +               "vpl011: WI on register offset %#08x\n",
> >> +               vpl011_reg);
> >
> > IMHO, this message should be printed just after the write_ignore label.
> >
> >>           goto write_ignore;
> >>
> >>       case IMSC:
> >
> > Cheers,
> >
> > --
> > Julien Grall
> >
> 
> ~Michal