diff mbox series

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

Message ID 20220130211838.8382-11-rick.p.edgecombe@intel.com (mailing list archive)
State New
Headers show
Series Shadow stacks for userspace | expand

Commit Message

Rick Edgecombe Jan. 30, 2022, 9:18 p.m. UTC 
From: Yu-cheng Yu <yu-cheng.yu@intel.com>

After the introduction of _PAGE_COW, a modified page's PTE can have either
_PAGE_DIRTY or _PAGE_COW.  Change _PAGE_DIRTY to _PAGE_DIRTY_BITS.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dave Hansen Feb. 9, 2022, 4:58 p.m. UTC | #1 
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. ]
Rick Edgecombe Feb. 11, 2022, 1:39 a.m. UTC | #2 
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.
Wang, Zhi A Feb. 11, 2022, 7:13 a.m. UTC | #3 
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/
> 
> 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.
> 

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.
Rick Edgecombe Feb. 12, 2022, 1:45 a.m. UTC | #4 
On Fri, 2022-02-11 at 07:13 +0000, Wang, Zhi A wrote:
> I would suggest you can remove that line in your patch and I will
> clean
> this function after your patches got merged.

Thanks!
diff mbox series

Patch

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);