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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan


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