linux-kernel - Re: [PATCH 10/35] drm/i915/gvt: Change _PAGE_DIRTY to _PAGE_DIRTY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <37652390-1645-8801-05de-a83a7dcfba7a@intel.com>
Date:   Fri, 11 Feb 2022 07:13:26 +0000
From:   "Wang, Zhi A" <zhi.a.wang@...el.com>
To:     "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "bsingharora@...il.com" <bsingharora@...il.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "Syromiatnikov, Eugene" <esyr@...hat.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "Eranian, Stephane" <eranian@...gle.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "nadav.amit@...il.com" <nadav.amit@...il.com>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "kcc@...gle.com" <kcc@...gle.com>, "bp@...en8.de" <bp@...en8.de>,
        "oleg@...hat.com" <oleg@...hat.com>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "Yang, Weijiang" <weijiang.yang@...el.com>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "pavel@....cz" <pavel@....cz>, "arnd@...db.de" <arnd@...db.de>,
        "Moreira, Joao" <joao.moreira@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "Dave.Martin@....com" <Dave.Martin@....com>,
        "john.allen@....com" <john.allen@....com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "gorcunov@...il.com" <gorcunov@...il.com>
CC:     "Yu, Yu-cheng" <yu-cheng.yu@...el.com>,
        "daniel@...ll.ch" <daniel@...ll.ch>,
        "zhenyuw@...ux.intel.com" <zhenyuw@...ux.intel.com>,
        "airlied@...ux.ie" <airlied@...ux.ie>,
        "joonas.lahtinen@...ux.intel.com" <joonas.lahtinen@...ux.intel.com>,
        "jani.nikula@...ux.intel.com" <jani.nikula@...ux.intel.com>,
        "Vivi, Rodrigo" <rodrigo.vivi@...el.com>
Subject: 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@...ts.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.