[v3] xen/arm: refine vpl011

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

Posted by Jiamei Xie 3 years, 2 months ago

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@arm.com>
---
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 | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 43522d48fd..1bf803fc1f 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -414,11 +414,19 @@ static int vpl011_mmio_read(struct vcpu *v,
     default:
         gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
                 dabt.reg, vpl011_reg);
-        return 0;
+        goto read_as_zero;
     }
 
     return 1;
 
+read_as_zero:
+    if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+    VPL011_LOCK(d, flags);
+    *r = 0;
+    VPL011_UNLOCK(d, flags);
+    return 1;
+
 bad_width:
     gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
             dabt.size, dabt.reg, vpl011_reg);
@@ -486,10 +494,11 @@ 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:
+    if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
     return 1;
 
 bad_width:
-- 
2.25.1

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

Posted by Julien Grall 3 years, 2 months ago

Hi,

On 29/11/2022 03:39, 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@arm.com>
> ---
> 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 | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 43522d48fd..1bf803fc1f 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -414,11 +414,19 @@ static int vpl011_mmio_read(struct vcpu *v,
>       default:
>           gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
>                   dabt.reg, vpl011_reg);
> -        return 0;
> +        goto read_as_zero;
>       }
>   
>       return 1;
>   
> +read_as_zero:
> +    if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;

We don't know the registers and therefore I don't think we should check 
the size.

> +
> +    VPL011_LOCK(d, flags);
> +    *r = 0;
> +    VPL011_UNLOCK(d, flags);
There is no need to lock/unlock here.

> +    return 1;
> +
>   bad_width:
>       gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
>               dabt.size, dabt.reg, vpl011_reg);
> @@ -486,10 +494,11 @@ 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:
> +    if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;

Same as for the read_as_zero, the size is unknown and shouldn't be checked.

Cheers,

-- 
Julien Grall

[PATCH v3 1/2] xen/arm: vpl011: emulate non-SBSA registers as WI/RAZ
[PATCH v3 2/2] xen/arm: vpl011: drop redundancy in mmio_write/read