Re: [qemu-s390x] [PATCH-for-4.2 v1 3/9] s390x/mmu: DAT translation rewrite

[David Hildenbrand]

On 12.08.19 09:20, Thomas Huth wrote:
> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>> Let's rewrite the DAT translation in a non-recursive way, similar to
>> arch/s390/kvm/gaccess.c:guest_translate() in KVM. This makes the
>> code much easier to read, compare and maintain.
>>
>> Use better names for the region/section/page table entries and for the
>> macros to extract relevant parts from virtual address. Introduce defines
>> for all defined bits, this will come in handy soon.
>>
>> All access exceptions now directly go via trigger_access_exception(),
>> at a central point. DAT protection checks are performed at a central
>> place.
>>
>> Also, we now catch and indicate invalid addresses of page tables. All
>> table entries are accessed via read_table_entry().
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  target/s390x/cpu.h        |  77 +++++---
>>  target/s390x/mem_helper.c |  13 +-
>>  target/s390x/mmu_helper.c | 360 +++++++++++++++++---------------------
>>  3 files changed, 229 insertions(+), 221 deletions(-)
>>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index c34992bb2e..1ff14250bd 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -554,26 +554,63 @@ QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>>  #define ASCE_TYPE_SEGMENT     0x00        /* segment table type             
>>   */
>>  #define ASCE_TABLE_LENGTH     0x03        /* region table length            
>>   */
>>  
>> -#define REGION_ENTRY_ORIGIN   (~0xfffULL) /* region/segment table origin    
>> */
>> -#define REGION_ENTRY_RO       0x200       /* region/segment protection bit  
>> */
>> -#define REGION_ENTRY_TF       0xc0        /* region/segment table offset    
>> */
>> -#define REGION_ENTRY_INV      0x20        /* invalid region table entry     
>> */
>> -#define REGION_ENTRY_TYPE_MASK 0x0c       /* region/segment table type mask 
>> */
>> -#define REGION_ENTRY_TYPE_R1  0x0c        /* region first table type        
>> */
>> -#define REGION_ENTRY_TYPE_R2  0x08        /* region second table type       
>> */
>> -#define REGION_ENTRY_TYPE_R3  0x04        /* region third table type        
>> */
>> -#define REGION_ENTRY_LENGTH   0x03        /* region third length            
>> */
>> -
>> -#define SEGMENT_ENTRY_ORIGIN  (~0x7ffULL) /* segment table origin        */
>> -#define SEGMENT_ENTRY_FC      0x400       /* format control              */
>> -#define SEGMENT_ENTRY_RO      0x200       /* page protection bit         */
>> -#define SEGMENT_ENTRY_INV     0x20        /* invalid segment table entry */
>> -
>> -#define VADDR_PX              0xff000     /* page index bits   */
>> -
>> -#define PAGE_RO               0x200       /* HW read-only bit  */
>> -#define PAGE_INVALID          0x400       /* HW invalid bit    */
>> -#define PAGE_RES0             0x800       /* bit must be zero  */
>> +#define REGION_ENTRY_ORIGIN         0xfffffffffffff000ULL
>> +#define REGION_ENTRY_P              0x0000000000000200ULL
>> +#define REGION_ENTRY_TF             0x00000000000000c0ULL
>> +#define REGION_ENTRY_I              0x0000000000000020ULL
>> +#define REGION_ENTRY_TT             0x000000000000000cULL
>> +#define REGION_ENTRY_TL             0x0000000000000003ULL
>> +
>> +#define REGION_ENTRY_TT_REGION1     0x000000000000000cULL
>> +#define REGION_ENTRY_TT_REGION2     0x0000000000000008ULL
>> +#define REGION_ENTRY_TT_REGION3     0x0000000000000004ULL
>> +
>> +#define REGION3_ENTRY_RFAA          0xffffffff80000000ULL
>> +#define REGION3_ENTRY_AV            0x0000000000010000ULL
>> +#define REGION3_ENTRY_ACC           0x000000000000f000ULL
>> +#define REGION3_ENTRY_F             0x0000000000000800ULL
>> +#define REGION3_ENTRY_FC            0x0000000000000400ULL
>> +#define REGION3_ENTRY_IEP           0x0000000000000100ULL
>> +#define REGION3_ENTRY_CR            0x0000000000000010ULL
>> +
>> +#define SEGMENT_ENTRY_ORIGIN        0xfffffffffffff800ULL
>> +#define SEGMENT_ENTRY_SFAA          0xfffffffffff80000ULL
>> +#define SEGMENT_ENTRY_AV            0x0000000000010000ULL
>> +#define SEGMENT_ENTRY_ACC           0x000000000000f000ULL
>> +#define SEGMENT_ENTRY_F             0x0000000000000800ULL
>> +#define SEGMENT_ENTRY_FC            0x0000000000000400ULL
>> +#define SEGMENT_ENTRY_P             0x0000000000000200ULL
>> +#define SEGMENT_ENTRY_IEP           0x0000000000000100ULL
>> +#define SEGMENT_ENTRY_I             0x0000000000000020ULL
>> +#define SEGMENT_ENTRY_CS            0x0000000000000010ULL
>> +#define SEGMENT_ENTRY_TT            0x000000000000000cULL
>> +
>> +#define SEGMENT_ENTRY_TT_REGION1    0x000000000000000cULL
>> +#define SEGMENT_ENTRY_TT_REGION2    0x0000000000000008ULL
>> +#define SEGMENT_ENTRY_TT_REGION3    0x0000000000000004ULL
>> +#define SEGMENT_ENTRY_TT_SEGMENT    0x0000000000000000ULL
>> +
>> +#define PAGE_ENTRY_0                0x0000000000000800ULL
>> +#define PAGE_ENTRY_I                0x0000000000000400ULL
>> +#define PAGE_ENTRY_P                0x0000000000000200ULL
>> +#define PAGE_ENTRY_IEP              0x0000000000000100ULL
>> +
>> +#define VADDR_REGION1_TX_MASK       0xffe0000000000000ULL
>> +#define VADDR_REGION2_TX_MASK       0x001ffc0000000000ULL
>> +#define VADDR_REGION3_TX_MASK       0x000003ff80000000ULL
>> +#define VADDR_SEGMENT_TX_MASK       0x000000007ff00000ULL
>> +#define VADDR_PAGE_TX_MASK          0x00000000000ff000ULL
>> +
>> +#define VADDR_REGION1_TX(vaddr)     (((vaddr) & VADDR_REGION1_TX_MASK) >> 
>> 53)
>> +#define VADDR_REGION2_TX(vaddr)     (((vaddr) & VADDR_REGION2_TX_MASK) >> 
>> 42)
>> +#define VADDR_REGION3_TX(vaddr)     (((vaddr) & VADDR_REGION3_TX_MASK) >> 
>> 31)
>> +#define VADDR_SEGMENT_TX(vaddr)     (((vaddr) & VADDR_SEGMENT_TX_MASK) >> 
>> 20)
>> +#define VADDR_PAGE_TX(vaddr)        (((vaddr) & VADDR_PAGE_TX_MASK) >> 12)
>> +
>> +#define VADDR_REGION1_TL(vaddr)     (((vaddr) & 0xc000000000000000ULL) >> 
>> 62)
>> +#define VADDR_REGION2_TL(vaddr)     (((vaddr) & 0x0018000000000000ULL) >> 
>> 51)
>> +#define VADDR_REGION3_TL(vaddr)     (((vaddr) & 0x0000030000000000ULL) >> 
>> 40)
>> +#define VADDR_SEGMENT_TL(vaddr)     (((vaddr) & 0x0000000060000000ULL) >> 
>> 29)
> 
> Ugh, this patch is quite big, and you're doing multiple things at once
> here, e.g. renaming macros from PAGE_INVALID to PAGE_ENTRY_I ... could

I could split out renaming the macros, however, besides a lot of work on
my side this won't really make a huge difference here.

> you maybe split this up in multiple patches instead? Also, is this
> complete rewrite really necessary, just to match your personal taste? I

I'm not gonna use lipstick on a pig ;) No honestly, the recursion is
just nasty and we can now easily compare the current code with
arch/s390/kvm/gaccess.c:guest_translate()

> think it would be easier to review if you'd just fix the current code
> instead if necessary...

No, won't do that.

Still thanks for having a look.


-- 

Thanks,

David / dhildenb