Re: [PATCH 10/35] drm/i915/gvt: Change _PAGE_DIRTY to _PAGE_DIRTY_BITS

On 2/11/22 1:39 AM, Edgecombe, Rick P wrote:

CC intel-gfx@lists.freedesktop.org

Thread: 
https://lore.kernel.org/lkml/a5bb32b8-8bd7-ac98-5c4c-5af604ac8256@intel.com/ (local)

On Wed, 2022-02-09 at 08:58 -0800, Dave Hansen wrote:

quoted

On 1/30/22 13:18, Rick Edgecombe wrote:

quoted

diff --git a/drivers/gpu/drm/i915/gvt/gtt.c

b/drivers/gpu/drm/i915/gvt/gtt.c
index 99d1781fa5f0..75ce4e823902 100644

--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c

@@ -1210,7 +1210,7 @@ static int split_2MB_gtt_entry(struct

intel_vgpu *vgpu,
 	}
 
 	/* Clear dirty field. */
-	se->val64 &= ~_PAGE_DIRTY;
+	se->val64 &= ~_PAGE_DIRTY_BITS;
 
 	ops->clear_pse(se);
 	ops->clear_ips(se);

Are these x86 CPU page table values?  I see ->val64 being used like
this:

        e->val64 &= ~GEN8_PAGE_PRESENT;
and
	se.val64 |= GEN8_PAGE_PRESENT | GEN8_PAGE_RW;

where we also have:

#define GEN8_PAGE_PRESENT               BIT_ULL(0)
#define GEN8_PAGE_RW                    BIT_ULL(1)

Which tells me that these are probably *close* to the CPU's page
tables.
 But, I honestly don't know which format they are.  I don't know if
_PAGE_COW is still a software bit in that format or not.

Either way, I don't think we should be messing with i915 device page
tables.

Or, are these somehow magically shared with the CPU in some way I
don't
know about?

[ If these are device-only page tables, it would probably be nice to
  stop using _PAGE_FOO for them.  It would avoid confusion like this.
]

The two Reviewed-by tags are giving me pause, but as far as I can tell
this should not be setting _PAGE_DIRTY_BITS. This code seems to be
shadowing guest page tables, and the change would clear the COW
software bit in the guest page tables. So, yes, I think this should be
dropped.

Hi:

According to the PRM https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-lkf-vol06-memory_views.pdf p.28,
the GPU page table is IA-like and there will be scenarios when IA and
gpu sharing a page table. That's why they are sharing part of the
definitions. But the dirty bits will be ignored in the HW which GVT-g
supports. The code should copy the bits from the guest PDPE 2M entry
and then cleans some unused bits. So the _PAGE_DIRTY_ is misused here.

I would suggest you can remove that line in your patch and I will clean
this function after your patches got merged.

Thanks,
Zhi.