Re: [PATCH 15/18] arm/altp2m: Add altp2m paging mechanism.

From: Sergej Proskurin <proskurin@sec.in.tum.de>
To: Julien Grall <julien.grall@arm.com>, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Tamas K Lengyel <tamas@tklengyel.com>
Subject: Re: [PATCH 15/18] arm/altp2m: Add altp2m paging mechanism.
Date: Wed, 6 Jul 2016 10:33:17 +0200	[thread overview]
Message-ID: <7ca75156-b98f-6157-8ea6-3099a2996f02@sec.in.tum.de> (raw)
In-Reply-To: <072110f2-4722-9d90-c441-e4e941f4468f@arm.com>

Hi Julien,

On 07/04/2016 10:53 PM, Julien Grall wrote:
> (CC Tamas)
>
> Hello Sergej,
>
> On 04/07/2016 12:45, Sergej Proskurin wrote:
>> This commit adds the function p2m_altp2m_lazy_copy implementing the
>> altp2m paging mechanism. The function p2m_altp2m_lazy_copy lazily copies
>> the hostp2m's mapping into the currently active altp2m view on 2nd stage
>> instruction or data access violations. Every altp2m violation generates
>> a vm_event.
>
> I have been working on clean up the abort path (see [1]). Please
> rebase your code on top of it.
>

I will do that, thank you.

>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>
> [...]
>
>> +/*
>> + * If the fault is for a not present entry:
>> + *     if the entry in the host p2m has a valid mfn, copy it and retry
>> + *     else indicate that outer handler should handle fault
>> + *
>> + * If the fault is for a present entry:
>> + *     indicate that outer handler should handle fault
>> + */
>> +bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
>> +                            unsigned long gva, struct npfec npfec,
>> +                            struct p2m_domain **ap2m)
>> +{
>> +    struct domain *d = v->domain;
>> +    struct p2m_domain *hp2m = p2m_get_hostp2m(v->domain);
>> +    p2m_type_t p2mt;
>> +    xenmem_access_t xma;
>> +    paddr_t maddr, mask = 0;
>> +    gfn_t gfn = _gfn(paddr_to_pfn(gpa));
>> +    unsigned int level;
>> +    unsigned long mattr;
>> +    int rc = 0;
>> +
>> +    static const p2m_access_t memaccess[] = {
>> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
>> +        ACCESS(n),
>> +        ACCESS(r),
>> +        ACCESS(w),
>> +        ACCESS(rw),
>> +        ACCESS(x),
>> +        ACCESS(rx),
>> +        ACCESS(wx),
>> +        ACCESS(rwx),
>> +        ACCESS(rx2rw),
>> +        ACCESS(n2rwx),
>> +#undef ACCESS
>> +    };
>> +
>> +    *ap2m = p2m_get_altp2m(v);
>> +    if ( *ap2m == NULL)
>> +        return 0;
>> +
>> +    /* Check if entry is part of the altp2m view */
>> +    spin_lock(&(*ap2m)->lock);
>> +    maddr = __p2m_lookup(*ap2m, gpa, NULL);
>> +    spin_unlock(&(*ap2m)->lock);
>> +    if ( maddr != INVALID_PADDR )
>> +        return 0;
>> +
>> +    /* Check if entry is part of the host p2m view */
>> +    spin_lock(&hp2m->lock);
>> +    maddr = __p2m_lookup(hp2m, gpa, &p2mt);
>> +    if ( maddr == INVALID_PADDR )
>> +        goto out;
>> +
>> +    rc = __p2m_get_mem_access(hp2m, gfn, &xma);
>> +    if ( rc )
>> +        goto out;
>> +
>> +    rc = p2m_get_gfn_level_and_attr(hp2m, gpa, &level, &mattr);
>> +    if ( rc )
>> +        goto out;
>
> Can we introduce a function which return the xma, mfn, order,
> attribute at once? It will avoid to browse the p2m 3 times which is
> really expensive on ARMv7 because the p2m is not mapped in the virtual
> address space of Xen.
>

I was already thinking of at least merging p2m_get_gfn_level_and_attr
with __p2m_lookup. But it would also make sense to introduce an entirely
new function, which does just that.I believe increasing the overhead of
__p2m_lookup would not be a good solution. Thank you.

>> +    spin_unlock(&hp2m->lock);
>> +
>> +    mask = level_masks[level];
>> +
>> +    rc = apply_p2m_changes(d, *ap2m, INSERT,
>> +                           pfn_to_paddr(gfn_x(gfn)) & mask,
>> +                           (pfn_to_paddr(gfn_x(gfn)) +
>> level_sizes[level]) & mask,
>> +                           maddr & mask, mattr, 0, p2mt,
>> +                           memaccess[xma]);
>
> The page associated to the MFN is not locked, so another thread could
> decide to remove the page from the domain and then the altp2m would
> contain an entry to something that does not belong to the domain
> anymore. Note that x86 is doing the same. So I am not sure why it is
> considered safe there...
>

If I understand you correctly, unlocking the hp2m->lock after calling
apply_p2m_changeswould already solve this issue, right? Thanks.

>> +    if ( rc )
>> +    {
>> +        gdprintk(XENLOG_ERR, "failed to set entry for %lx -> %lx p2m
>> %lx\n",
>> +                (unsigned long)pfn_to_paddr(gfn_x(gfn)), (unsigned
>> long)(maddr), (unsigned long)*ap2m);
>> +        domain_crash(hp2m->domain);
>> +    }
>> +
>> +    return 1;
>> +
>> +out:
>> +    spin_unlock(&hp2m->lock);
>> +    return 0;
>> +}
>> +
>>  static void p2m_init_altp2m_helper(struct domain *d, unsigned int i)
>>  {
>>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>
> [...]
>
>> @@ -2429,6 +2460,8 @@ static void do_trap_data_abort_guest(struct
>> cpu_user_regs *regs,
>>                                       const union hsr hsr)
>>  {
>>      const struct hsr_dabt dabt = hsr.dabt;
>> +    struct vcpu *v = current;
>> +    struct p2m_domain *p2m = NULL;
>>      int rc;
>>      mmio_info_t info;
>>
>> @@ -2449,6 +2482,12 @@ static void do_trap_data_abort_guest(struct
>> cpu_user_regs *regs,
>>          info.gpa = get_faulting_ipa();
>>      else
>>      {
>> +        /*
>> +         * When using altp2m, this flush is required to get rid of
>> old TLB
>> +         * entries and use the new, lazily copied, ap2m entries.
>> +         */
>> +        flush_tlb_local();
>
> Can you give more details why this flush is required?
>

Without the flush, the guest crashed to an unresolved data abort. To be
more precise, after the first lazy-copy of a mapping from the hostp2m to
the currently active altp2m view, the system crashed because it was not
able to find the new mapping in its altp2m table. The explicit flush
solved this issue quite nicely.

As I answer your question, I am starting to think that the crash was a
result of of a lack of a memory barrier because even with the old
(hostp2m's) TLBs present, the translation would not present an issue
(the mapping would be the same as the p2m entry is simply copied from
the hostp2m to the active altp2m view). Also, the guest would reuse the
old TLBs, the system would have used the access rights of the hostp2m,
and hence again be trapped by Xen.

I will try to solve this issue by means of a memory barrier, thank you.

>> +
>
> Regards,
>
> [1] https://lists.xen.org/archives/html/xen-devel/2016-06/msg02853.html
>

Cheers,
~Sergej

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