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

From: Julien Grall <julien.grall@arm.com>
To: Sergej Proskurin <proskurin@sec.in.tum.de>,
	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 15:26:20 +0100	[thread overview]
Message-ID: <577D150C.1040703@arm.com> (raw)
In-Reply-To: <7ca75156-b98f-6157-8ea6-3099a2996f02@sec.in.tum.de>

On 06/07/16 09:33, Sergej Proskurin wrote:
> On 07/04/2016 10:53 PM, Julien Grall wrote:
>> 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.

How the overhead of __p2m_lookup will be increased? level, mattr and 
type is already available in p2m_lookup. For the mem access, as you pass 
a pointer you can retrieve the access only when the pointer is not NULL.

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

That would solve it. However, you will add a lot of contention on hp2m 
when each vCPU is using a different empty view.

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

This explicit flush likely means there is another issue in the code. 
This flush should not be necessary.

Can you give more details about the exact data abort problem? Is it 
because the translation VA -> IPA is failing?

>
> 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 am sorry but I don't understand what you are trying to say.

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

Can you details where you think there is a lack of memory barrier?

Regards,

-- 
Julien Grall

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