[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 10/10] p2m/ept: Enable PML in p2m-ept for log-dirty.





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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.