Re: [Xen-devel] [PATCH v3] x86/mm: Suppresses vm_events caused by page-walks

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 02/28/2018 03:56 PM, Jan Beulich wrote:
>>>> On 28.02.18 at 14:41, <JBeulich@xxxxxxxx> wrote:
>>>>> On 28.02.18 at 14:25, <aisaila@xxxxxxxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -137,6 +137,23 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
>>>      return violation;
>>>  }
>>>  
>>> +static void p2m_set_ad_bits(struct vcpu *v, paddr_t ga)
>>> +{
>>> +    struct hvm_hw_cpu ctxt;
>>> +    uint32_t pfec = 0;
>>> +
>>> +    hvm_funcs.save_cpu_ctxt(v, &ctxt);
>>> +
>>> +    if ( guest_cpu_user_regs()->eip == v->arch.pg_dirty.eip
>>> +         && ga == v->arch.pg_dirty.gla )
>>> +        pfec = PFEC_write_access;
>>> +
>>> +    paging_ga_to_gfn_cr3(v, ctxt.cr3, ga, &pfec, NULL);
>>> +
>>> +    v->arch.pg_dirty.eip = guest_cpu_user_regs()->eip;
>>> +    v->arch.pg_dirty.gla = ga;
>>> +}
>>
>> This being the only user of v->arch.pg_dirty, why is the new
>> sub-structure made part of struct arch_vcpu instead of
>> struct arch_vm_event (or some other sub-structure referenced
>> by pointer, such that struct arch_vcpu wouldn't grow in size?
>> And even without that, this is HVM-specific, so would at best
>> belong into the HVM sub-structure.
>>
>> The PFEC_write_access logic is completely unclear to me, despite
>> the attempt to describe this in the description. I'm pretty sure you
>> want a code comment here.

The thinking here is this: if we've ended up in p2m_mem_access_check()
with npfec.kind != npfec_kind_with_gla, then that's an EPT fault caused
by a page walk, that can be performed "manually" once Xen tries to set
both the A and the D bits.

So when it tries to set the A bit, we mark the attempt by storing the
RIP/GLA pair, so that when the function is called again for the same
values we know that that's an attempt to set the D bit, and we act on
that (so that we don't have to lift the page restrictions on the fault
page).

If there's a cleaner way to achieve this it would be great.

>> What if the first pass through this function is with guest EIP zero
>> and gla also zero?

Is that possible? If it really is, we could use another default value,
for example ~0UL (INVALID_GFN?).

> I should have added here: If the vCPU is paused, there surely
> should be a cheaper way. If the vCPU is not paused, the value
> read is stale by the time you use it.

The VCPU is paused. We'll look at a lighter way to extract CR3.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel