Re: [PATCH 04/18] arm/altp2m: Add altp2m init/teardown routines.

From: Julien Grall <julien.grall@arm.com>
To: Sergej Proskurin <proskurin@sec.in.tum.de>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH 04/18] arm/altp2m: Add altp2m init/teardown routines.
Date: Mon, 4 Jul 2016 22:35:27 +0100	[thread overview]
Message-ID: <a590c860-07c1-ff56-d83d-515aa82109ba@arm.com> (raw)
In-Reply-To: <9167f6e2-3f0d-797c-98cb-9ea046f97fb4@sec.in.tum.de>

Hello Sergej,

On 04/07/2016 22:19, Sergej Proskurin wrote:
> On 07/04/2016 07:44 PM, Julien Grall wrote:
>> On 04/07/16 17:56, Sergej Proskurin wrote:
>>> On 07/04/2016 06:43 PM, Andrew Cooper wrote:
>>>> On 04/07/16 17:40, Sergej Proskurin wrote:
>>>>>
>>>>>>>    {
>>>>>>> -    struct p2m_domain *p2m = &d->arch.p2m;
>>>>>>> +    int ret = 0;
>>>>>>> +
>>>>>>> +    spin_lock_init(&p2m->lock);
>>>>>>> +    INIT_PAGE_LIST_HEAD(&p2m->pages);
>>>>>>> +
>>>>>>> +    spin_lock(&p2m->lock);
>>>>>>> +
>>>>>>> +    p2m->domain = d;
>>>>>>> +    p2m->access_required = false;
>>>>>>> +    p2m->mem_access_enabled = false;
>>>>>>> +    p2m->default_access = p2m_access_rwx;
>>>>>>> +    p2m->p2m_class = p2m_host;
>>>>>>> +    p2m->root = NULL;
>>>>>>> +
>>>>>>> +    /* Adopt VMID of the associated domain */
>>>>>>> +    p2m->vmid = d->arch.p2m.vmid;
>>>>>> It looks like to me that re-using the same VMID will require more TLB
>>>>>> flush (such as when a VCPU is migrated to another physical CPU). So
>>>>>> could you explain why you decided to re-use the same VMID?
>>>>>>
>>>>> Please correct me if I am wrong, but I associate a VMID with an entire
>>>>> domain. Since, the altp2m view still belongs to the same domain
>>>>> (p2m_init_one is called only from p2m_init_altp2m), the code re-uses
>>>>> the
>>>>> old VMID.
>>>>
>>>> (I am not an ARM expert but) looking into VMIDs from the last time, they
>>>> are the TLB tag for the address space in use.
>>>>
>>>> Does ARM have shared TLBs between multiple cores?  If so, you must a
>>>> separate VMID, otherwise an ALT2PM used by one vcpu could cause a
>>>> separate vcpu with a different ALTP2M to reuse the wrong translation.
>>>>
>>>> ~Andrew
>>>>
>>>
>>> You're absolutely correct. However, on every VMENTRY Xen explicitly
>>> flushes the TLBs of the currently active domain (and with it, of the
>>> currently active (a)p2m table) and hence it should not result in an
>>> issue.
>>
>> VMENTRY is x86 not ARM. So are you sure you looked at the correct code?
>>
>> Regards,
>>
>
> This is true. I just use the term VMENTER for describing transitions to
> guests on both, x86 and ARM. In ./xen/arch/arm/domain.c the function
> ctxt_switch_to calls p2m_restore_state on every context switch, wich in
> turn loads the VTTBR associated to the domain and flushes the TLBs.

Really? I have this patch series applied on top of staging and there is 
no TLB flush instruction in p2m_restore_state nor p2m_load*.

Regards,

-- 
Julien Grall

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