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

Edgecombe, Rick P
2022-02-11T01:39:09Z

CC intel-gfx@lists.freedesktop.org

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

On Wed, 2022-02-09 at 08:58 -0800, Dave Hansen wrote:
> On 1/30/22 13:18, Rick Edgecombe wrote:
> >
> > 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.