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

[Kai Huang <kai.huang@xxxxxxxxxxxxxxx>]

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