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

[Christian König <christian.koenig@xxxxxxx>]

    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@xxxxxxxxxx:

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@kylinos.cnairlied@redhat.comkraxel@redhat.comairlied@linux.iedaniel@ffwll.chray.huang@amd.comvirtualization@lists.linux-foundation.orgspice-devel@lists.freedesktop.orgdri-devel@xxxxxxxxxxxxxxxxxxxxx                    

       

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@xxxxxxxxxx:
   

   
     

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@kylinos.cnairlied@redhat.comkraxel@redhat.comairlied@linux.iedaniel@ffwll.chray.huang@amd.comvirtualization@lists.linux-foundation.orgspice-devel@lists.freedesktop.orgdri-devel@xxxxxxxxxxxxxxxxxxxxx                             

              

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@xxxxxxxxxx:
                  

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@redhat.comkraxel@redhat.comairlied@linux.iedaniel@ffwll.chray.huang@amd.comvirtualization@lists.linux-foundation.orgspice-devel@lists.freedesktop.orgdri-devel@xxxxxxxxxxxxxxxxxxxxx                                            

                               

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;