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

[Kai Huang <kaih.linux@xxxxxxxxx>]

On Fri, Apr 10, 2015 at 5:46 PM, Tim Deegan <tim@xxxxxxx> wrote:
> At 16:44 +0800 on 10 Apr (1428684271), Kai Huang wrote:
>>
>>
>> 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.
>
> Fair enough.
>
>> > 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".
>
> I see.  I think I was confused by the field being called "reserved" in
> the struct.  I've just checked an older copy of the SDM and those bits
> used to be "Ignored", so we can just set them on all hardware.  Good!

So to confirm, you intend to  update EPT A/D bits directly without
prior checking whether cpu supports A/D bits?

Thanks,
-Kai
>
> Tim.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



-- 
Thanks,
-Kai

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