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

[Kai Huang <kai.huang@linux.intel.com>]

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.