Virtualization - Mar 2022 - Re: 回复: Re: 回复: Re: 回复: Re: [PATCH v1 1/2] drm/qxl: replace ioremap by ioremap_cache on arm64

Hi Cong,

when I understand Robin correctly all mapping (host, guest, kernel, 
userspace etc..) must have the same caching attributes unless you use 
the S2FWB feature introduced with Armv8.4.

If you don't follow those rules you usually run into coherency issues or 
even worse system hangs. So you not only need to adjust the kernel 
mappings, but also the function for userspace mappings to follow those 
rules.

Additional to that I think I read Robin correctly that mapping the 
resource write combined should be sufficient to solve your problem. So I 
suggest to use that instead.

On the other hand if you manage to fix this completely for arm64 by 
updating all the places to use cached mappings I'm fine with it as well. 
It's then up to Dave to decide if that's something we want because QXL 
is intentionally emulating a hardware device as far as I know.

Regards,
Christian.

Am 24.03.22 um 08:04 schrieb liucong2 at kylinos.cn:
>
> Hi Christian,
>
>
> Can you help explain more detail under what circumstances userspace 
> mapping
>
> will be problematic, you mean cached mappings also need adjustment in
>
> function ttm_prot_from_caching?
>
>
> Regards,
>
> Cong.
>
>
>
>
> *????*Re: ??: Re: ??: Re: [PATCH v1 1/2] drm/qxl: replace ioremap 
> by ioremap_cache on arm64
> *????*2022-03-23 18:17
> *????*Christian K?nig
> *????*liucong2 at kylinos.cnairlied@redhat.comkraxel at
redhat.comairlied@linux.iedaniel at ffwll.chray.huang@amd.comvirtualization at
lists.linux-foundation.orgspice-devel@lists.freedesktop.orgdri-devel at
lists.freedesktop.org
>
>
> Hi Cong,
>
> ? ?yes I've seen that, but that is still not sufficient.
>
> ? ?You need to update the check in ttm_module.c as well or otherwise ? 
> ?your userspace mapping might not work correctly either.
>
> ? ?Regards,
> ? ?Christian.
>
> Am 23.03.22 um 11:00 schrieb liucong2 at kylinos.cn:
>>
>> Hi Christian,
>>
>>
>> another commit fix the case in ttm. I send two patches at the ? ? ? 
>> ?same time, but seems I miss
>>
>> ?'--cover-letter' when run format-patch or some other bad ? ? ?
>> ?operation.
>>
>> ----
>>
>>
>> Regards,
>>
>> Cong.
>>
>>
>>
>>
>> *????*Re: ??: Re: [PATCH v1 1/2] drm/qxl: replace ? ? ? ? ? 
>> ?ioremap by ioremap_cache on arm64
>> *????*2022-03-23 17:34
>> *????*Christian K?nig
>> *????*liucong2 at kylinos.cnairlied@redhat.comkraxel at
redhat.comairlied@linux.iedaniel at ffwll.chray.huang@amd.comvirtualization at
lists.linux-foundation.orgspice-devel@lists.freedesktop.orgdri-devel at
lists.freedesktop.org
>>
>>
>> Hi Cong,
>>
>> ? ? ? ? ? ? ?? ?well than Dave must decide what to do here.
>>
>> ? ? ? ? ? ? ?? ?When QXL emulates a device it should also not use ? ? 
>> ? ? ? ? ?memory accesses ? ?which won't work on a physical device.
>>
>> ? ? ? ? ? ? ?? ?BTW: Your patch is really buggy, it misses the cases 
>> in ? ? ? ? ? ? ?ttm_module.c
>>
>> ? ? ? ? ? ? ?? ?Regards,
>> ? ? ? ? ? ? ?? ?Christian.
>>
>> Am 23.03.22 um 09:48 schrieb liucong2 at kylinos.cn:
>>>
>>> Hi Christian,
>>>
>>>
>>> according to 'Arm Architecture Reference Manual ? ? ? ? ? ? ? ?
>>> ?Armv8,for ?Armv8-A
>>>
>>> architecture profile' pdf E2.6.5
>>>
>>>
>>> E2.6.5 Generation of Alignment faults by load/store ? ? ? ? ? ? ? ?
>>> ?multiple ?accesses to
>>>
>>> ?Device memory
>>>
>>>
>>> When ? ? ? ? ? ? ? ? ?FEAT_LSMAOC is ?implemented and the value of 
>>> the ? ? ? ?applicable nTLSMD
>>>
>>> field is 0, any ? ? ? ? ? ? ? ? ?memory ?access by an AArch32 Load 
>>> Multiple or ? ? ? ? ? ?Store
>>>
>>> Multiple ? ? ? ? ? ? ? ? ?instruction to ? ? ? ?an address that the
>>> stage 1 ?translation
>>>
>>> assigns as ? ? ? ? ? ? ? ? ?Device-nGRE, ?Device-nGnRE, or 
>>> Device-nGnRnE ? ? ?generates
>>>
>>> an Alignment ? ? ? ? ? ? ? ? ?fault.
>>>
>>>
>>> so it seems not just?some ARM boards doesn't allow ? ? ? ? ? ?
? ?
>>> ?unaligned ? ?access to MMIO
>>>
>>> space, all pci memory mapped as?Device-nGnRE in arm64 ?cannot ? ? ?
>>> ?support
>>>
>>> unaligned access. and qxl is a device?simulated by ? ? ? ? ? ? ? ? 
>>> ?qemu, it ? ? ? ?should be able to access
>>>
>>> to MMIO space in a more flexible way(PROT_NORMAL) ? ? ? ? ? ? ? ? 
>>> ?than the real ? ? ? ?physical
>>>
>>> graphics card.
>>>
>>>
>>> ----
>>>
>>>
>>>
>>> Cong.
>>>
>>>
>>>
>>>
>>>
>>> *????*Re: [PATCH v1 ? ? ? ? ? ? ? ? ? ?1/2] ?drm/qxl: replace 
>>> ioremap by ? ? ?ioremap_cache on arm64
>>> *????*2022-03-23 ?15:15
>>> *????*Christian ?K?nig
>>> *????*CongLiuairlied at redhat.comkraxel@redhat.comairlied at
linux.iedaniel@ffwll.chray.huang at
amd.comvirtualization@lists.linux-foundation.orgspice-devel at
lists.freedesktop.orgdri-devel@lists.freedesktop.org
>>>
>>>
>>>
>>> Am 22.03.22 um 10:34 schrieb Cong Liu:
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> qxl use ioremap to map ram_header and
rom,
>>> ?in the arm64 ? ? ? ?implementation,
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> the device is mapped as DEVICE_nGnRE,
it
>>> ?can not support ? ? ? ?unaligned
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> access.
>>>
>>> ? ? ? ? ? ? ? ? ?? ? ? ?Well that some ARM boards doesn't allow
>>> ?unaligned access to MMIO ? ? ? ?space
>>> ? ? ? ? ? ? ? ? ?? ? ? ?is a well known bug of those ARM boards.
>>>
>>> ? ? ? ? ? ? ? ? ?? ? ? ?So as far as I know this is a hardware bug 
>>> you ?are trying to ? ? ? ?workaround
>>> ? ? ? ? ? ? ? ? ?? ? ? ?here and I'm not 100% sure that this is
>>> ?correct.
>>>
>>> ? ? ? ? ? ? ? ? ?? ? ? ?Christian.
>>>
>>> ? ? ? ? ? ? ? ? ?? ? ? ?>
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> ? ?6.620515] pc :
setup_hw_slot+0x24/0x60
>>> ?[qxl]
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.620961] lr :
setup_slot+0x34/0xf0
>>> ?[qxl]
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.621376] sp : ffff800012b73760
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.621701] x29: ffff800012b73760
x28:
>>> ?0000000000000001 ? ? ? ?x27: 0000000010000000
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.622400] x26: 0000000000000001
x25:
>>> ?0000000004000000 ? ? ? ?x24: ffffcf376848c000
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.623099] x23: ffff0000c4087400
x22:
>>> ?ffffcf3718e17140 ? ? ? ?x21: 0000000000000000
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.623823] x20: ffff0000c4086000
x19:
>>> ?ffff0000c40870b0 ? ? ? ?x18: 0000000000000014
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.624519] x17: 000000004d3605ab
x16:
>>> ?00000000bb3b6129 ? ? ? ?x15: 000000006e771809
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.625214] x14: 0000000000000001
x13:
>>> ?007473696c5f7974 ? ? ? ?x12: 696e696666615f65
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.625909] x11: 00000000d543656a
x10:
>>> ?0000000000000000 ? ? ? ?x9 : ffffcf3718e085a4
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.626616] x8 : 00000000006c7871
x7 :
>>> ?000000000000000a ? ? ? ?x6 : 0000000000000017
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.627343] x5 : 0000000000001400
x4 :
>>> ?ffff800011f63400 ? ? ? ?x3 : 0000000014000000
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.628047] x2 : 0000000000000000
x1 :
>>> ?ffff0000c40870b0 ? ? ? ?x0 : ffff0000c4086000
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.628751] Call trace:
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.628994]
?setup_hw_slot+0x24/0x60 ?[qxl]
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.629404] ?setup_slot+0x34/0xf0
[qxl]
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.629790] 
>>> ??qxl_device_init+0x6f0/0x7f0 [qxl]
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.630235]
?qxl_pci_probe+0xdc/0x1d0
>>> ?[qxl]
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.630654]
?local_pci_probe+0x48/0xb8
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.631027]
??pci_device_probe+0x194/0x208
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.631464]
?really_probe+0xd0/0x458
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.631818] 
>>> ??__driver_probe_device+0x124/0x1c0
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.632256] 
>>> ??driver_probe_device+0x48/0x130
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.632669]
?__driver_attach+0xc4/0x1a8
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.633049]
?bus_for_each_dev+0x78/0xd0
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.633437]
?driver_attach+0x2c/0x38
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.633789]
?bus_add_driver+0x154/0x248
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.634168]
?driver_register+0x6c/0x128
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.635205] 
>>> ??__pci_register_driver+0x4c/0x58
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.635628] ?qxl_init+0x48/0x1000
[qxl]
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.636013]
?do_one_initcall+0x50/0x240
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.636390]
?do_init_module+0x60/0x238
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.636768]
?load_module+0x2458/0x2900
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.637136] 
>>> ??__do_sys_finit_module+0xbc/0x128
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.637561] 
>>> ??__arm64_sys_finit_module+0x28/0x38
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.638004]
?invoke_syscall+0x74/0xf0
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.638366] 
>>> ??el0_svc_common.constprop.0+0x58/0x1a8
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.638836] ?do_el0_svc+0x2c/0x90
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.639216] ?el0_svc+0x40/0x190
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.639526] 
>>> ??el0t_64_sync_handler+0xb0/0xb8
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.639934]
?el0t_64_sync+0x1a4/0x1a8
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.640294] Code: 910003fd f9484804
>>> ?f9400c23 8b050084 ? ? ? ?(f809c083)
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> [ ? ?6.640889] ---[ end trace 
>>> ?95615d89b7c87f95 ]---
>>> ? ? ? ? ? ? ? ? ?? ? ? ?>
>>> ? ? ? ? ? ? ? ? ?? ? ? ?> Signed-off-by: Cong Liu
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> ---
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> drivers/gpu/drm/qxl/qxl_kms.c | 10
? ?
>>> ? ? ? ?++++++++++
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> ? 1 file changed, 10 insertions(+)
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?>
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> diff --git 
>>> ?a/drivers/gpu/drm/qxl/qxl_kms.c ??b/drivers/gpu/drm/qxl/qxl_kms.c
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> index 4dc5ad13f12c..0e61ac04d8ad ?
?100644
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> ---
a/drivers/gpu/drm/qxl/qxl_kms.c
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> +++
b/drivers/gpu/drm/qxl/qxl_kms.c
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> @@ -165,7 +165,11 @@ int 
>>> ?qxl_device_init(struct ?qxl_device *qdev,
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> (int)qdev->surfaceram_size / ?
? ? ?
>>> ?1024,
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> ? (sb == 4) ? "64bit" :
"32bit");
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?>
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> +#ifdef CONFIG_ARM64
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> + qdev->rom = 
>>> ?ioremap_cache(qdev->rom_base, ? ? ? ? ? ? ? ? ?
??qdev->rom_size);
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> +#else
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> qdev->rom =
?ioremap(qdev->rom_base, ?
>>> ? ? ? ? ? ??qdev->rom_size);
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> +#endif
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> ? if (!qdev->rom) {
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> pr_err("Unable to ioremap
ROM\n");
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> ? r = -ENOMEM;
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> @@ -183,9 +187,15 @@ int 
>>> ?qxl_device_init(struct ?qxl_device *qdev,
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> ? goto rom_unmap;
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> ? }
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?>
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> +#ifdef CONFIG_ARM64
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> + qdev->ram_header = 
>>> ?ioremap_cache(qdev->vram_base ?+
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> +
?qdev->rom->ram_header_offset,
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> + sizeof(*qdev->ram_header));
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> +#else
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> qdev->ram_header = 
>>> ?ioremap(qdev->vram_base +
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?>
?qdev->rom->ram_header_offset,
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> sizeof(*qdev->ram_header));
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> +#endif
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> ? if (!qdev->ram_header) {
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> DRM_ERROR("Unable to ioremap
RAM ? ? ?
>>> ? ?header\n");
>>> ? ? ? ? ? ? ? ? ? ?? ? ? ? ?> ? r = -ENOMEM;
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20220324/1782aa6c/attachment-0001.html>

On Thu, Mar 24, 2022 at 10:20:40AM +0100, Christian K?nig
wrote:
> Hi Cong,
> 
> when I understand Robin correctly all mapping (host, guest, kernel,
> userspace etc..) must have the same caching attributes unless you use the
> S2FWB feature introduced with Armv8.4.
> 
> If you don't follow those rules you usually run into coherency issues
or
> even worse system hangs. So you not only need to adjust the kernel
mappings,
> but also the function for userspace mappings to follow those rules.
That matches my understanding.

For qxl specifically: when using the xork qxl driver getting the
userspace mappings right is essential because userspace will write qxl
command buffers then.  When using the xorg modesetting driver or wayland
the worst thing happening would be display corruption because userspace
will only map dumb bo's for pixel data.

I'm wondering though why you are keen on getting qxl work instead of
just using virtio-gpu?

take care,
  Gerd