Re: [PATCH v2 20/25] arm/altp2m: Add altp2m paging mechanism.

[Julien Grall <julien.grall@arm.com>]

On 06/08/2016 13:51, Sergej Proskurin wrote:
> Hi Julien,

Hello Sergej,

> On 08/04/2016 03:50 PM, Julien Grall wrote:
>> Hello Sergej,
>>
>> On 01/08/16 18:10, Sergej Proskurin wrote:
>>> This commit adds the function "altp2m_lazy_copy" implementing the altp2m
>>> paging mechanism. The function "altp2m_lazy_copy" lazily copies the
>>> hostp2m's mapping into the currently active altp2m view on 2nd stage
>>> translation violations on instruction or data access. Every altp2m
>>> violation generates a vm_event.
>>
>> I think you want to "translation fault" and not "violation". The
>> latter looks more a permission issue whilst it is not the case here.
>>
>
> The implemented paging mechanism also covers permission issues, which is
> the reason why I chose the term violation. By this, I mean that the
> implementation covers traps to Xen occured due to 2nd stage permission
> violations (as altp2m view's might have set different access permissions
> to trap on). Also, the implementation covers translation faults due to
> not present entries in the active altp2m view, as well.

FSC_FLT_TRANS can only happen for a translation fault. And you don't 
modify FSC_FLT_PERM (except move coding around as usual...). So why are 
you talking about permission violations?

>
>> However I am not sure what you mean by "every altp2m violation
>> generates a vm_event". Do you mean the userspace will be aware of it?
>>
>
> No. Every time, the altp2m's configuration lets the guest trap into Xen
> due to a lack of memory access permissions (e.g., on execution of a rw
> page), we fill the associated fields in the req buffer in
> mem_access_check so that the management domain receives the information
> required to understand what kind of altp2m violation just happened.
> Based on this information, it might decide what to do next (perform
> additional checks or simply change the altp2m view to continue guest
> execution).

You will receive a FSC_FLT_PERM in this case and not a FSC_FLT_TRANS.

[...]

>>> +                        struct npfec npfec,
>>> +                        struct p2m_domain **ap2m)
>>
>> Why do you need the parameter ap2m? None of the callers make use of it
>> except setting it.
>>
>
> True. Another leftover from the x86 implementation. I will change that.
>
>>> +{
>>> +    struct domain *d = v->domain;
>>> +    struct p2m_domain *hp2m = p2m_get_hostp2m(v->domain);
>>
>> p2m_get_hostp2m(d);
>>
>
> Thanks.
>
>>> +    p2m_type_t p2mt;
>>> +    xenmem_access_t xma;
>>> +    gfn_t gfn = _gfn(paddr_to_pfn(gpa));
>>> +    mfn_t mfn;
>>> +    unsigned int level;
>>> +    int rc = 0;
>>
>> Please use true/false rather than 0/1. Also this should be bool_t.
>>
>
> Ok.
>
>>> +
>>> +    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 = altp2m_get_altp2m(v);
>>> +    if ( *ap2m == NULL)
>>> +        return 0;
>>> +
>>> +    /* Check if entry is part of the altp2m view */
>>> +    mfn = p2m_lookup_attr(*ap2m, gfn, NULL, NULL, NULL, NULL);
>>> +    if ( !mfn_eq(mfn, INVALID_MFN) )
>>> +        goto out;
>>> +
>>> +    /* Check if entry is part of the host p2m view */
>>> +    mfn = p2m_lookup_attr(hp2m, gfn, &p2mt, &level, NULL, &xma);
>>> +    if ( mfn_eq(mfn, INVALID_MFN) )
>>> +        goto out;
>>
>> This is quite racy. The page could be removed from the host p2m by the
>> time you have added it in the altp2m because you have no lock.
>>
>
> In the last patch series, I proposed to lock the hp2m->lock
> (write_lock), which is not a good solution at this point, as it would
> potentially create lots of contention on hp2m. Also, we would need to
> export __p2m_lookup or remove the lock from p2m_lookup_attr, which is
> not a good solution either.

The P2M lock has been converted to a read-write lock. So it will not 
create contention as multiple read can be done concurrently.

You should be more concerned about security than contention. The former 
may lead to exploit or corrupting Xen. The latter will only impact 
performance.

>
> The addition of the altp2m_lock could help in this case: If a page would
> be removed from the hostp2m before we added it in the altp2m view, the
> propagate function would need to wait until lazy_copy would finish and
> eventually remove it from the altp2m view. But on the other hand, it
> would highly decrease the performance on a multi core system.
>
> If I understand that correctly, a better solution would be to use a
> p2m_read_lock(hp2m) as we would still allow reading but not writing (in
> case the hp2m gets entries removed in apply_p2m_changes). That is, I
> would set it right before p2m_lookup_attr(hp2m, ...) and release it
> right after modify_altp2m_entry. This solution would not present a
> bottleneck on the lazy_copy mechanism, and simultaneously prevent hp2m
> from changing. What do you think?

That's the solution I would like to see and the only safe one.

>
>>> +
>>> +    rc = modify_altp2m_entry(d, *ap2m, gpa,
>>> pfn_to_paddr(mfn_x(mfn)), level,
>>> +                             p2mt, memaccess[xma]);
>>
>> Please avoid to mix bool and int even though today we have implicitly
>> conversion.
>>
>
> Ok.
>
>>> +    if ( rc )
>>> +    {
>>> +        gdprintk(XENLOG_ERR, "failed to set entry for %lx -> %lx p2m
>>> %lx\n",
>>> +                (unsigned long)gpa, (unsigned
>>> long)(paddr_to_pfn(mfn_x(mfn))),
>>
>> By using (unsigned long) you will truncate the address on ARM32
>> because we are able to support up to 40 bits address.
>>
>> Also why do you print the full address? The guest physical address may
>> not be page-aligned so it will confuse the user.
>>
>
> x86 leftover. I will change that.

It is not in the x86 code....

>>> +                (unsigned long)*ap2m);
>>
>> It does not seem really helpful to print the pointer here. You will
>> not be able to exploit it when reading the log. Also this should be
>> printed with "%p" and not using a cast.
>>
>
> Another x86 leftover. I will change that.
>
>>> +        domain_crash(hp2m->domain);
>>> +    }
>>> +
>>> +    rc = 1;
>>> +
>>> +out:
>>> +    return rc;
>>> +}
>>> +
>>>  static inline void altp2m_reset(struct p2m_domain *p2m)
>>>  {
>>>      read_lock(&p2m->lock);
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 31810e6..bee8be7 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -1812,6 +1812,12 @@ void __init setup_virt_paging(void)
>>>      smp_call_function(setup_virt_paging_one, (void *)val, 1);
>>>  }
>>>
>>> +void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>>> +{
>>> +    if ( altp2m_active(v->domain) )
>>> +        altp2m_switch_vcpu_altp2m_by_id(v, idx);
>>> +}
>>> +
>>
>> I am not sure why this function lives here.
>>
>
> This function is used in ./xen/common/vm_event.c. Since the function is
> used from a common place and already named p2m_* I did not want to pull
> it out of p2m.c (and have a p2m_* file/function prefix in altp2m.c).
> However, I could move the function to altp2m.c rename it globally (also
> in the x86 implementation). Or, I could simply move it to altp2m despite
> the name. What would you prefer?

It seems that the altp2m functions on x86 will be move from p2m.c, 
altp2m.c. So you may want to rename the function here.

[...]

>>> +             * currently active altp2m view.
>>> +             */
>>> +            if ( altp2m_lazy_copy(v, gpa, gva, npfec, &p2m) )
>>> +                return;
>>> +
>>> +            rc = p2m_mem_access_check(gpa, gva, npfec);
>>
>> Why do you call p2m_mem_access_check here? If you are here it is for a
>> translation fault which you handle via altp2m_lazy_copy.
>>
>
> Right. I have experienced that the test systems jump into the case
> FSC_FLT_TRANS, right after I have lazily-copied the page into the
> associated altp2m view. Not sure what the issue might be here.

I don't understand. What do you mean?

>
>>> +
>>> +            /* Trap was triggered by mem_access, work here is done */
>>> +            if ( !rc )
>>> +                return;
>>> +        }
>>> +
>>> +        break;
>>> +    }
>>>      case FSC_FLT_PERM:
>>>      {
>>> -        paddr_t gpa;
>>>          const struct npfec npfec = {
>>>              .insn_fetch = 1,
>>>              .gla_valid = 1,
>>>              .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt :
>>> npfec_kind_with_gla
>>>          };
>>>
>>> -        if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
>>> -            gpa = get_faulting_ipa(gva);
>>> -        else
>>> -        {
>>> -            /*
>>> -             * Flush the TLB to make sure the DTLB is clear before
>>> -             * doing GVA->IPA translation. If we got here because of
>>> -             * an entry only present in the ITLB, this translation may
>>> -             * still be inaccurate.
>>> -             */
>>> -            flush_tlb_local();
>>> -
>>> -            rc = gva_to_ipa(gva, &gpa, GV2M_READ);
>>> -            if ( rc == -EFAULT )
>>> -                return; /* Try again */
>>> -        }
>>> -
>>>          rc = p2m_mem_access_check(gpa, gva, npfec);
>>>
>>>          /* Trap was triggered by mem_access, work here is done */
>>> @@ -2451,6 +2482,8 @@ static void do_trap_data_abort_guest(struct
>>> cpu_user_regs *regs,
>>>      int rc;
>>>      mmio_info_t info;
>>>      uint8_t fsc = hsr.dabt.dfsc & ~FSC_LL_MASK;
>>> +    struct vcpu *v = current;
>>> +    struct p2m_domain *p2m = NULL;
>>>
>>>      info.dabt = dabt;
>>>  #ifdef CONFIG_ARM_32
>>> @@ -2459,7 +2492,7 @@ static void do_trap_data_abort_guest(struct
>>> cpu_user_regs *regs,
>>>      info.gva = READ_SYSREG64(FAR_EL2);
>>>  #endif
>>>
>>> -    if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
>>> +    if ( hpfar_is_valid(hsr.dabt.s1ptw, fsc) )
>>>          info.gpa = get_faulting_ipa(info.gva);
>>>      else
>>>      {
>>> @@ -2470,23 +2503,31 @@ static void do_trap_data_abort_guest(struct
>>> cpu_user_regs *regs,
>>>
>>>      switch ( fsc )
>>>      {
>>> -    case FSC_FLT_PERM:
>>> +    case FSC_FLT_TRANS:
>>>      {
>>> -        const struct npfec npfec = {
>>> -            .read_access = !dabt.write,
>>> -            .write_access = dabt.write,
>>> -            .gla_valid = 1,
>>> -            .kind = dabt.s1ptw ? npfec_kind_in_gpt :
>>> npfec_kind_with_gla
>>> -        };
>>> +        if ( altp2m_active(current->domain) )
>>
>> I would much prefer to this checking altp2m only if the MMIO was not
>> emulated (so moving the code afterwards). This will avoid to add
>> overhead when access the virtual interrupt controller.
>
> I am not sure whether I understood your request. Could you be more
> specific please? What excatly shall be moved where?

With this patch, the translation fault will do:
	1) Check altp2m
	2) Emulate MMIO

So if altp2m is enabled, you will add overhead for any MMIO access (such 
as the virtual interrupt controller).

I would much prefer to see 2) then 1).

[...]

>>> +
>>> +            /* Trap was triggered by mem_access, work here is done */
>>> +            if ( !rc )
>>> +                return;
>>> +        }
>>>
>>> -        /* Trap was triggered by mem_access, work here is done */
>>> -        if ( !rc )
>>> -            return;
>>> -        break;
>>> -    }
>>> -    case FSC_FLT_TRANS:
>>>          if ( dabt.s1ptw )
>>>              goto bad_data_abort;
>>>
>>> @@ -2515,6 +2556,23 @@ static void do_trap_data_abort_guest(struct
>>> cpu_user_regs *regs,
>>>              return;
>>>          }
>>>          break;
>>> +    }
>>> +    case FSC_FLT_PERM:
>>> +    {
>>> +        const struct npfec npfec = {
>>> +            .read_access = !dabt.write,
>>> +            .write_access = dabt.write,
>>> +            .gla_valid = 1,
>>> +            .kind = dabt.s1ptw ? npfec_kind_in_gpt :
>>> npfec_kind_with_gla
>>> +        };
>>> +
>>> +        rc = p2m_mem_access_check(info.gpa, info.gva, npfec);
>>> +
>>> +        /* Trap was triggered by mem_access, work here is done */
>>> +        if ( !rc )
>>> +            return;
>>> +        break;
>>> +    }
>>
>> Why did you move the case handling FSC_FLT_PERM?
>>
>
> I really important reason: I moved it simply because int(FSC_FLT_TRANS)
> < int(FSC_FLT_PERM). I can move it back if you like.

Again, you should not make a such change in the same patch introduce new 
code. It took me a while to understand that you only move code.

This series is already difficult enough to review. So please don't have 
extra work by mixing code movement and new code in the same patch.


Regards,

-- 
Julien Grall

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