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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.18 at 11:00, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> 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.

"Cleaner" is a secondary goal. A correct way would be desirable
for starters. I don't see what would prevent coming back here a
second time because something somewhere having returned
X86EMUL_RETRY, causing the insn to simply be restarted. This
_may_ be possible to address by suitably flushing the two values
under certain conditions.

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

Except that we're talking about a GLA here, and ~0UL is a
valid linear address. Some non-canonical address _may_ be
suitable, but I'm not sure.

Jan


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