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

[Michal Orzel <michal.orzel@xxxxxxx>]

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.

> 
>>
>> [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%7C1065702b4fd2457cdbf808dacc84b45a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638047167600786580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=W1dbakw6lkGkv4ydElIi%2Ba7uT7e7Pt5dB3vDtYpP%2FqQ%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%7C1065702b4fd2457cdbf808dacc84b45a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638047167600786580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=O4zxuI3HqRA1bdraGcVVY8vV0HGbqOI3nFa%2FciC1cGQ%3D&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