[PATCH umr] Skip ahead if PDE entry is actually a PTE entry. (v2)

[PATCH umr] Skip ahead if PDE entry is actually a PTE entry. (v2)

[Tom St Denis]

Signed-off-by: Tom St Denis <tom.stdenis@amd.com>

(v2) Don't print out PDE entries with PTE bit set
---
 src/lib/read_vram.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/lib/read_vram.c b/src/lib/read_vram.c
index 0df48dadec12..51823d71021e 100644
--- a/src/lib/read_vram.c
+++ b/src/lib/read_vram.c
@@ -509,7 +509,7 @@ static int umr_access_vram_ai(struct umr_asic *asic, uint32_t vmid,
 				pde_fields.system        = (pde_entry >> 1) & 1;
 				pde_fields.cache         = (pde_entry >> 2) & 1;
 				pde_fields.pte           = (pde_entry >> 54) & 1;
-				if (memcmp(&pde_fields, &pde_array[pde_cnt], sizeof pde_fields) && asic->options.verbose)
+				if (!pde_fields.pte && memcmp(&pde_fields, &pde_array[pde_cnt], sizeof pde_fields) && asic->options.verbose)
 					fprintf(stderr, "[VERBOSE]: %s PDE%d=0x%016llx, VA=0x%012llx, PBA==0x%012llx, V=%d, S=%d, C=%d, P=%d\n",
 							&indentation[12-pde_cnt*3],
 							pde_cnt,
@@ -522,6 +522,11 @@ static int umr_access_vram_ai(struct umr_asic *asic, uint32_t vmid,
 							(int)pde_fields.pte);
 				memcpy(&pde_array[pde_cnt++], &pde_fields, sizeof pde_fields);
 
+				if (pde_fields.pte) {
+					pte_entry = pde_entry;
+					goto pde_is_pte;
+				}
+
 				if (!pde_fields.system)
 					pde_fields.pte_base_addr -= vm_fb_offset;
 
@@ -539,6 +544,7 @@ static int umr_access_vram_ai(struct umr_asic *asic, uint32_t vmid,
 				return -1;
 
 			// decode PTE values
+pde_is_pte:
 			pte_fields.page_base_addr = pte_entry & 0xFFFFFFFFFF000ULL;
 			pte_fields.fragment       = (pte_entry >> 7)  & 0x1F;
 			pte_fields.system         = (pte_entry >> 1) & 1;
-- 
2.12.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH umr] Skip ahead if PDE entry is actually a PTE entry. (v2)

[Christian König]

Am 04.11.2017 um 18:15 schrieb Tom St Denis:
> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>

Still not perfect, but good enough for now. Patch is Tested-by: 
Christian König <christian.koenig@amd.com>.

I think you need to rework the VM walking a bit, cause we need to 
support the T bit as well in the future and your code make a few 
assumptions which doesn't allow that.

Regards,
Christian.

>
> (v2) Don't print out PDE entries with PTE bit set
> ---
>   src/lib/read_vram.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/lib/read_vram.c b/src/lib/read_vram.c
> index 0df48dadec12..51823d71021e 100644
> --- a/src/lib/read_vram.c
> +++ b/src/lib/read_vram.c
> @@ -509,7 +509,7 @@ static int umr_access_vram_ai(struct umr_asic *asic, uint32_t vmid,
>   				pde_fields.system        = (pde_entry >> 1) & 1;
>   				pde_fields.cache         = (pde_entry >> 2) & 1;
>   				pde_fields.pte           = (pde_entry >> 54) & 1;
> -				if (memcmp(&pde_fields, &pde_array[pde_cnt], sizeof pde_fields) && asic->options.verbose)
> +				if (!pde_fields.pte && memcmp(&pde_fields, &pde_array[pde_cnt], sizeof pde_fields) && asic->options.verbose)
>   					fprintf(stderr, "[VERBOSE]: %s PDE%d=0x%016llx, VA=0x%012llx, PBA==0x%012llx, V=%d, S=%d, C=%d, P=%d\n",
>   							&indentation[12-pde_cnt*3],
>   							pde_cnt,
> @@ -522,6 +522,11 @@ static int umr_access_vram_ai(struct umr_asic *asic, uint32_t vmid,
>   							(int)pde_fields.pte);
>   				memcpy(&pde_array[pde_cnt++], &pde_fields, sizeof pde_fields);
>   
> +				if (pde_fields.pte) {
> +					pte_entry = pde_entry;
> +					goto pde_is_pte;
> +				}
> +
>   				if (!pde_fields.system)
>   					pde_fields.pte_base_addr -= vm_fb_offset;
>   
> @@ -539,6 +544,7 @@ static int umr_access_vram_ai(struct umr_asic *asic, uint32_t vmid,
>   				return -1;
>   
>   			// decode PTE values
> +pde_is_pte:
>   			pte_fields.page_base_addr = pte_entry & 0xFFFFFFFFFF000ULL;
>   			pte_fields.fragment       = (pte_entry >> 7)  & 0x1F;
>   			pte_fields.system         = (pte_entry >> 1) & 1;


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH umr] Skip ahead if PDE entry is actually a PTE entry. (v2)

[Tom St Denis]

On 06/11/17 05:01 AM, Christian König wrote:
> Am 04.11.2017 um 18:15 schrieb Tom St Denis:
>> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
> 
> Still not perfect, but good enough for now. Patch is Tested-by: 
> Christian König <christian.koenig@amd.com>.
> 
> I think you need to rework the VM walking a bit, cause we need to 
> support the T bit as well in the future and your code make a few 
> assumptions which doesn't allow that.

Doesn't the T bit imply V=0 which means the page isn't backed by memory. 
  Not much umr could do about that other than to print out the T bit.

Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH umr] Skip ahead if PDE entry is actually a PTE entry. (v2)

[Christian König]

Am 06.11.2017 um 19:28 schrieb Tom St Denis:
> On 06/11/17 05:01 AM, Christian König wrote:
>> Am 04.11.2017 um 18:15 schrieb Tom St Denis:
>>> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
>>
>> Still not perfect, but good enough for now. Patch is Tested-by: 
>> Christian König <christian.koenig@amd.com>.
>>
>> I think you need to rework the VM walking a bit, cause we need to 
>> support the T bit as well in the future and your code make a few 
>> assumptions which doesn't allow that.
>
> Doesn't the T bit imply V=0 which means the page isn't backed by 
> memory.  Not much umr could do about that other than to print out the 
> T bit.

No, the T bit means translate further. In other words it is the counter 
part of the P bit and means that a PTE should be handled as a PDE.

But for this to have meaning you also need to handle the fragment size 
as well (Now I have you totally confused, haven't I? :).

Cheers,
Christian.

>
> Tom
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH umr] Skip ahead if PDE entry is actually a PTE entry. (v2)

[Tom St Denis]

On 06/11/17 01:34 PM, Christian König wrote:
> Am 06.11.2017 um 19:28 schrieb Tom St Denis:
>> On 06/11/17 05:01 AM, Christian König wrote:
>>> Am 04.11.2017 um 18:15 schrieb Tom St Denis:
>>>> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
>>>
>>> Still not perfect, but good enough for now. Patch is Tested-by: 
>>> Christian König <christian.koenig@amd.com>.
>>>
>>> I think you need to rework the VM walking a bit, cause we need to 
>>> support the T bit as well in the future and your code make a few 
>>> assumptions which doesn't allow that.
>>
>> Doesn't the T bit imply V=0 which means the page isn't backed by 
>> memory.  Not much umr could do about that other than to print out the 
>> T bit.
> 
> No, the T bit means translate further. In other words it is the counter 
> part of the P bit and means that a PTE should be handled as a PDE.
> 
> But for this to have meaning you also need to handle the fragment size 
> as well (Now I have you totally confused, haven't I? :).


Yes :-)

I thought fragment size was more for hinting to the cache controller and 
not actually part of the VM decoding.

Also the PI docs say T => "Tiled (PRT)" and from what I gather that just 
means the page is valid but might not be backed so instead of raising a 
page fault you raise a new fault that the application (?) handles 
accordingly.

There's an 'F' bit that is labeled "translate further".

Reading section 8 of said document seems to indicate you're confusing 
bits F and T or my doc is wildly out of date (or we're talking about 
different IP revisions)

Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH umr] Skip ahead if PDE entry is actually a PTE entry. (v2)

[Christian König]

Am 06.11.2017 um 19:39 schrieb Tom St Denis:
> On 06/11/17 01:34 PM, Christian König wrote:
>> Am 06.11.2017 um 19:28 schrieb Tom St Denis:
>>> On 06/11/17 05:01 AM, Christian König wrote:
>>>> Am 04.11.2017 um 18:15 schrieb Tom St Denis:
>>>>> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
>>>>
>>>> Still not perfect, but good enough for now. Patch is Tested-by: 
>>>> Christian König <christian.koenig@amd.com>.
>>>>
>>>> I think you need to rework the VM walking a bit, cause we need to 
>>>> support the T bit as well in the future and your code make a few 
>>>> assumptions which doesn't allow that.
>>>
>>> Doesn't the T bit imply V=0 which means the page isn't backed by 
>>> memory.  Not much umr could do about that other than to print out 
>>> the T bit.
>>
>> No, the T bit means translate further. In other words it is the 
>> counter part of the P bit and means that a PTE should be handled as a 
>> PDE.
>>
>> But for this to have meaning you also need to handle the fragment 
>> size as well (Now I have you totally confused, haven't I? :).
>
>
> Yes :-)
>
> I thought fragment size was more for hinting to the cache controller 
> and not actually part of the VM decoding.

Correct, our hardware devs are unfortunately using the same name for two 
different things.

The fragment size in the PTE means what you described above and controls 
the L1 TLB settings.

The fragment size in the PDE controls how big the PTE entries will be.

So with the default fragment size of zero in the PDE we have 512 PTEs 
resulting in a 4K page table.

> Also the PI docs say T => "Tiled (PRT)" and from what I gather that 
> just means the page is valid but might not be backed so instead of 
> raising a page fault you raise a new fault that the application (?) 
> handles accordingly.
>
> There's an 'F' bit that is labeled "translate further".
>
> Reading section 8 of said document seems to indicate you're confusing 
> bits F and T or my doc is wildly out of date (or we're talking about 
> different IP revisions)

Sorry my fault. I was just confusing the F and T bits.

Christian.

>
> Tom


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx