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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.
> 
> Why is the function argument named ga instead of gla (thus
> matching the structure field)?
> 
> As to using eip (and naming the new structure field eip): Do you
> really mean only the low 32 bits of rip?
> 
> What if the first pass through this function is with guest EIP zero
> and gla also zero?
> 
> And finally, using the context save function seems pretty heavy
> for something that cares about exactly a single item in the
> structure. It would help if you explained in the description why
> no lighter weight alternative would work.

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.

Jan


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