From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kai Huang Subject: Re: [PATCH 10/10] p2m/ept: Enable PML in p2m-ept for log-dirty. Date: Fri, 10 Apr 2015 16:44:31 +0800 Message-ID: <55278D6F.7020200@linux.intel.com> References: <1427423754-11841-1-git-send-email-kai.huang@linux.intel.com> <1427423754-11841-11-git-send-email-kai.huang@linux.intel.com> <20150409122014.GH17031@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150409122014.GH17031@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: yang.z.zhang@intel.com, andrew.cooper3@citrix.com, kevin.tian@intel.com, jbeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 04/09/2015 08:20 PM, Tim Deegan wrote: > At 10:35 +0800 on 27 Mar (1427452554), Kai Huang wrote: >> @@ -118,6 +119,12 @@ static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_acces >> break; >> case p2m_ram_rw: >> entry->r = entry->w = entry->x = 1; >> + /* >> + * This is about to avoid unnecessary GPA logging in PML buffer, >> + * such as normal memory in partial log-dirty >> + */ >> + if ( vmx_domain_pml_enabled(p2m->domain) ) >> + entry->d = 1; > I think that both A and D should be set here, even when PML is not > supported, to avoid the MMU having to write them later. And indeed > not just for ram_rw, but for every present leaf type. I do agree we should also set A if PML is enabled for the domain, and looks there's no harm to set A bit for all present leaf types. Actually there should be no harm to set A bit even the leaf entry is not present.. But for D bit I think we should be more specific. For p2m types that are writable, we should set the D-bit to avoid unnecessary GPA logging, but for those are read-only, I think it's not reasonable to set D bit, as it's not possible for them to be dirty. > > You will still need to check whether the CPU supports the A/D bits, > since AIUI those are reserved bits otherwise. In my understanding, A/D bits actually are ignored if EPT A/D bit is not enabled in EPTP. Citing one statement in SDM for an example: "If bit 6 of EPTP is 1, accessed flag for EPT; indicates whether software has accessed the 4-KByte page referenced by this entry (see Section 28.2.4). Ignored if bit 6 of EPTP is 0". Therefore I think it's unnecessary to check whether CPU supports A/D bits prior to setting A/D bits. The checking is an overhead anyway. And it reminds me we can safely set A bit in middle level entry in splitting super page case, which you pointed out in patch 1 to enable A/D bits. Of course it's safer to operate A/D bits with whether cpu supports A/D bits checked, and we might better do it. Is this reasonable? Thanks, -Kai > >> break; >> case p2m_mmio_direct: >> entry->r = entry->x = 1; >> @@ -125,6 +132,26 @@ static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_acces >> entry->mfn); >> break; >> case p2m_ram_logdirty: >> + entry->r = entry->x = 1; >> + if ( vmx_domain_pml_enabled(p2m->domain) ) >> + { >> + /* >> + * In case of PML, we don't have to write protect 4K page, but >> + * only need to clear D-bit for it. Note we still need to write >> + * protect super page in order to split it to 4K pages in EPT >> + * violation. >> + */ >> + if ( !is_epte_superpage(entry) ) >> + { >> + entry->w = 1; >> + entry->d = 0; >> + } >> + else >> + entry->w = 0; >> + } >> + else >> + entry->w = 0; >> + break; > This can be folded into a neater form: > > if ( vmx_domain_pml_enabled(p2m->domain) > && !is_epte_superpage(entry) ) > { > /* > * In case of PML, we don't have to write protect 4K page, but > * only need to clear D-bit for it. Note we still need to write > * protect super page in order to split it to 4K pages in EPT > * violation. > */ > entry->w = 1; > entry->d = 0; > } > else > entry->w = 0; Sure. Thanks, -Kai > > Cheers, > > Tim.