Re: [PATCH v2 14/25] arm/altp2m: Make get_page_from_gva ready for altp2m.

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,


On 08/04/2016 01:59 PM, Julien Grall wrote:
> Hello Sergej,
>
> On 01/08/16 18:10, Sergej Proskurin wrote:
>> The function get_page_from_gva uses ARM's hardware support to translate
>> gva's to machine addresses. This function is used, among others, for
>> memory regulation purposes, e.g, within the context of memory
>> ballooning.
>> To ensure correct behavior while altp2m is in use, we use the host's p2m
>> table for the associated gva to ma translation. This is required at this
>> point, as altp2m lazily copies pages from the host's p2m and even might
>> be flushed because of changes to the host's p2m (as it is done within
>> the context of memory ballooning).
>
> I was expecting to see some change in
> p2m_mem_access_check_and_get_page. Is there any reason to not fix it?
>
>

I did not yet encounter any issues with
p2m_mem_access_check_and_get_page. According to ARM ARM, ATS1C** (see
gva_to_ipa_par) translates VA to IPA in non-secure privilege levels (as
it is the the case here). Thus, the 2nd level translation represented by
the (alt)p2m is not really considered at this point and hence make an
extension obsolete.

Or did you have anything else in mind?

>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/arm/p2m.c | 31 +++++++++++++++++++++++++++++--
>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index bcad51f..784f8da 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1614,7 +1614,7 @@ struct page_info *get_page_from_gva(struct vcpu
>> *v, vaddr_t va,
>>                                      unsigned long flags)
>>  {
>>      struct domain *d = v->domain;
>> -    struct p2m_domain *p2m = &d->arch.p2m;
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>
> This is more a clean-up than necessary. I would prefer to see a patch
> modifying all "&d->arch.p2m" by p2m_get_hostp2m in one go.
>

I will add a patch, which will do just that. Thank you.

>>      struct page_info *page = NULL;
>>      paddr_t maddr = 0;
>>      int rc;
>> @@ -1628,7 +1628,34 @@ struct page_info *get_page_from_gva(struct
>> vcpu *v, vaddr_t va,
>>
>>      p2m_read_lock(p2m);
>>
>> -    rc = gvirt_to_maddr(va, &maddr, flags);
>> +    /*
>> +     * If altp2m is active, we still read the gva from the hostp2m,
>> as it
>
> s/still/need to/
>

Ok.

>> +     * contains all valid mappings while the currently active altp2m
>> view might
>> +     * not have the required gva mapping yet.
>> +     */
>> +    if ( unlikely(altp2m_active(d)) )
>> +    {
>> +        unsigned long irq_flags = 0;
>> +        uint64_t ovttbr = READ_SYSREG64(VTTBR_EL2);
>> +
>> +        if ( ovttbr != p2m->vttbr.vttbr )
>> +        {
>> +            local_irq_save(irq_flags);
>> +            WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2);
>> +            isb();
>> +        }
>> +
>> +        rc = gvirt_to_maddr(va, &maddr, flags);
>> +
>> +        if ( ovttbr != p2m->vttbr.vttbr )
>> +        {
>> +            WRITE_SYSREG64(ovttbr, VTTBR_EL2);
>> +            isb();
>> +            local_irq_restore(irq_flags);
>> +        }
>
> The pattern is very similar to what p2m_flush_tlb does. Can we get
> macro/helpers to avoid duplicate code?
>

Sure thing :)

>> +    }
>> +    else
>> +        rc = gvirt_to_maddr(va, &maddr, flags);
>>
>>      if ( rc )
>>          goto err;
>>
>

Best regards,
~Sergej


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