On 8/19/19 1:58 PM, David Hildenbrand wrote:
> On 19.08.19 13:40, 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.
>>
>> Ok, I just had another look at this patch, and even if I don't like the
>> complete rewrite just for the sake of it, the new code looks ok to me.
>>
>> [...]
>>> + switch (asce & ASCE_TYPE_MASK) {
>>> + case ASCE_TYPE_REGION1:
>>> + if (read_table_entry(gaddr, &entry)) {
>>> + return PGM_ADDRESSING;
>>> + }
>>> + if (entry & REGION_ENTRY_I) {
>>> + return PGM_REG_FIRST_TRANS;
>>> + }
>>> + if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
>>> + return PGM_TRANS_SPEC;
>>> + }
>>> + if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>> + VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>> + return PGM_REG_SEC_TRANS;
>>> + }
>>> + if (edat1 && (entry & REGION_ENTRY_P)) {
>>> + *flags &= ~PAGE_WRITE;
>>> + }
>>> + gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) *
>>> 8;
>>> + /* FALL THROUGH */
>>> + case ASCE_TYPE_REGION2:
>>> + if (read_table_entry(gaddr, &entry)) {
>>> + return PGM_ADDRESSING;
>>> + }
>>> + if (entry & REGION_ENTRY_I) {
>>> + return PGM_REG_SEC_TRANS;
>>> + }
>>> + if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
>>> + return PGM_TRANS_SPEC;
>>> + }
>>> + if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>> + VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>> + return PGM_REG_THIRD_TRANS;
>>> + }
>>> + if (edat1 && (entry & REGION_ENTRY_P)) {
>>> + *flags &= ~PAGE_WRITE;
>>> + }
>>> + gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) *
>>> 8;
>>> + /* FALL THROUGH */
>>> + case ASCE_TYPE_REGION3:
>>> + if (read_table_entry(gaddr, &entry)) {
>>> + return PGM_ADDRESSING;
>>> + }
>>> + if (entry & REGION_ENTRY_I) {
>>> + return PGM_REG_THIRD_TRANS;
>>> + }
>>> + if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
>>> + return PGM_TRANS_SPEC;
>>> + }
>>> + if (edat1 && (entry & REGION_ENTRY_P)) {
>>> + *flags &= ~PAGE_WRITE;
>>> + }
>>> + if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>> + VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>> + return PGM_SEGMENT_TRANS;
>>> + }
>>> + gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_SEGMENT_TX(vaddr) *
>>> 8;
>>> + /* FALL THROUGH */
>>
>> If you don't like recursion, maybe you could at least use a for-loop for
>> the region tables? ... the code is really quite repetitive here... just
>> my 0.02 €.
>
> I'll split this patch further up once I have time. With the unrolling I
> am not yet quite sure - the tables tend to get more different with every
> new HW release (especially, see the other patches in this series, like
> edat2 and IEP) and we want our branch predictor to produce sane
> predictions (especially when processing consecutive pages).
Ok, I just also saw in the next patch that region 3 already needs some
special handling for EDAT-2, so never mind, it's likely best to keep it
unrolled.
Thomas
