|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 21/25] arm/altp2m: Add HVMOP_altp2m_change_gfn.
Hi Julien,
On 08/04/2016 04:04 PM, Julien Grall wrote:
> Hello Sergej,
>
> On 01/08/16 18:10, Sergej Proskurin wrote:
>> This commit adds the functionality to change mfn mappings for specified
>> gfn's in altp2m views. This mechanism can be used within the context of
>> VMI, e.g., to establish stealthy debugging.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> xen/arch/arm/altp2m.c | 116
>> +++++++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/hvm.c | 7 ++-
>> xen/arch/arm/p2m.c | 14 ++++++
>> xen/include/asm-arm/altp2m.h | 6 +++
>> xen/include/asm-arm/p2m.h | 4 ++
>> 5 files changed, 146 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
>> index 78fc1d5..db86c14 100644
>> --- a/xen/arch/arm/altp2m.c
>> +++ b/xen/arch/arm/altp2m.c
>> @@ -294,6 +294,122 @@ out:
>> altp2m_unlock(d);
>> }
>>
>> +int altp2m_change_gfn(struct domain *d,
>> + unsigned int idx,
>> + gfn_t old_gfn,
>> + gfn_t new_gfn)
>> +{
>> + struct p2m_domain *hp2m, *ap2m;
>> + paddr_t old_gpa = pfn_to_paddr(gfn_x(old_gfn));
>> + mfn_t mfn;
>> + xenmem_access_t xma;
>> + p2m_type_t p2mt;
>> + unsigned int level;
>> + int rc = -EINVAL;
>> +
>> + 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
>> + };
>> +
>> + if ( idx >= MAX_ALTP2M || d->arch.altp2m_vttbr[idx] ==
>> INVALID_VTTBR )
>
> The second check is not safe. Another operation could destroy the
> altp2m at the same time, but because of memory ordering this thread
> may still see altp2m_vttbr as valid.
>
Ok, I will move the alp2m_lock further up right before the check. Thank you.
>> + return rc;
>> +
>> + hp2m = p2m_get_hostp2m(d);
>> + ap2m = d->arch.altp2m_p2m[idx];
>> +
>> + altp2m_lock(d);
>> +
>> + /*
>> + * Flip mem_access_enabled to true when a permission is set, as
>> to prevent
>> + * allocating or inserting super-pages.
>> + */
>> + ap2m->mem_access_enabled = true;
>
> Can you give more details about why you need this?
>
Similar to altp2m_set_mem_access, if we remap a page that is part of a
super page in the hostp2m, we first map the superpage in form of 512
pages into the ap2m and then change only one page. So, we set
mem_access_enabled to true to shatter the superpage on the ap2m side.
>> +
>> + mfn = p2m_lookup_attr(ap2m, old_gfn, &p2mt, &level, NULL, NULL);
>> +
>> + /* Check whether the page needs to be reset. */
>> + if ( gfn_eq(new_gfn, INVALID_GFN) )
>> + {
>> + /* If mfn is mapped by old_gpa, remove old_gpa from the
>> altp2m table. */
>> + if ( !mfn_eq(mfn, INVALID_MFN) )
>> + {
>> + rc = remove_altp2m_entry(d, ap2m, old_gpa,
>> pfn_to_paddr(mfn_x(mfn)), level);
>
> remove_altp2m_entry should take a gfn and mfn in parameter and not an
> address. The latter is a call for misusage of the API.
>
Ok. This will also remove the need for level_sizes/level_masks in the
associated function.
>> + if ( rc )
>> + {
>> + rc = -EINVAL;
>> + goto out;
>> + }
>> + }
>> +
>> + rc = 0;
>> + goto out;
>> + }
>> +
>> + /* Check host p2m if no valid entry in altp2m present. */
>> + if ( mfn_eq(mfn, INVALID_MFN) )
>> + {
>> + mfn = p2m_lookup_attr(hp2m, old_gfn, &p2mt, &level, NULL,
>> &xma);
>> + if ( mfn_eq(mfn, INVALID_MFN) || (p2mt != p2m_ram_rw) )
>
> Please add a comment to explain why the second check.
>
Ok, I will. It has the same reason as in patch #19: It is not sufficient
so simply check for invalid MFN's as the type might be invalid. Also,
the x86 implementation did not allow to remap a gfn to a shared page.
>> + {
>> + rc = -EINVAL;
>> + goto out;
>> + }
>> +
>> + /* If this is a superpage, copy that first. */
>> + if ( level != 3 )
>> + {
>> + rc = modify_altp2m_entry(d, ap2m, old_gpa,
>> pfn_to_paddr(mfn_x(mfn)),
>> + level, p2mt, memaccess[xma]);
>> + if ( rc )
>> + {
>> + rc = -EINVAL;
>> + goto out;
>> + }
>> + }
>> + }
>> +
>> + mfn = p2m_lookup_attr(ap2m, new_gfn, &p2mt, &level, NULL, &xma);
>> +
>> + /* If new_gfn is not part of altp2m, get the mapping information
>> from hp2m */
>> + if ( mfn_eq(mfn, INVALID_MFN) )
>> + mfn = p2m_lookup_attr(hp2m, new_gfn, &p2mt, &level, NULL,
>> &xma);
>> +
>> + if ( mfn_eq(mfn, INVALID_MFN) || (p2mt != p2m_ram_rw) )
>
> Please add a comment to explain why the second check.
>
Same reason as above.
>> + {
>> + rc = -EINVAL;
>> + goto out;
>> + }
>> +
>> + /* Set mem access attributes - currently supporting only one
>> (4K) page. */
>> + level = 3;
>> + rc = modify_altp2m_entry(d, ap2m, old_gpa,
>> pfn_to_paddr(mfn_x(mfn)),
>
> modify_altp2m_entry should take a gfn and mfn in parameter and not an
> address. The latter is a call for misusage of the API.
>
Ok.
>> + level, p2mt, memaccess[xma]);
>> + if ( rc )
>> + {
>> + rc = -EINVAL;
>> + goto out;
>> + }
>> +
>> + rc = 0;
>> +
>> +out:
>> + altp2m_unlock(d);
>> +
>> + return rc;
>> +}
>> +
>> +
>> static void altp2m_vcpu_reset(struct vcpu *v)
>> {
>> struct altp2mvcpu *av = &vcpu_altp2m(v);
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index 00a244a..38b32de 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -142,7 +142,12 @@ static int
>> do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
>> break;
>>
>> case HVMOP_altp2m_change_gfn:
>> - rc = -EOPNOTSUPP;
>> + if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>> + rc = -EINVAL;
>> + else
>> + rc = altp2m_change_gfn(d, a.u.change_gfn.view,
>> + _gfn(a.u.change_gfn.old_gfn),
>> + _gfn(a.u.change_gfn.new_gfn));
>> break;
>> }
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index bee8be7..2f4751b 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1321,6 +1321,20 @@ void guest_physmap_remove_page(struct domain *d,
>> p2m_remove_mapping(d, p2m_get_hostp2m(d), gfn, (1 <<
>> page_order), mfn);
>> }
>>
>> +int remove_altp2m_entry(struct domain *d, struct p2m_domain *ap2m,
>> + paddr_t gpa, paddr_t maddr, unsigned int level)
>
> The interface should take mfn_t and gfn_t in parameter and not address.
>
Ok.
>> +{
>> + paddr_t size = level_sizes[level];
>> + paddr_t mask = level_masks[level];
>> + gfn_t gfn = _gfn(paddr_to_pfn(gpa & mask));
>> + mfn_t mfn = _mfn(paddr_to_pfn(maddr & mask));
>> + unsigned long nr = paddr_to_pfn(size);
>> +
>> + ASSERT(p2m_is_altp2m(ap2m));
>> +
>> + return p2m_remove_mapping(d, ap2m, gfn, nr, mfn);
>> +}
>> +
>> int modify_altp2m_entry(struct domain *d, struct p2m_domain *ap2m,
>> paddr_t gpa, paddr_t maddr, unsigned int level,
>> p2m_type_t t, p2m_access_t a)
>> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
>> index 8bfbc6a..64fbff7 100644
>> --- a/xen/include/asm-arm/altp2m.h
>> +++ b/xen/include/asm-arm/altp2m.h
>> @@ -99,4 +99,10 @@ void altp2m_propagate_change(struct domain *d,
>> p2m_type_t p2mt,
>> p2m_access_t p2ma);
>>
>> +/* Change a gfn->mfn mapping */
>> +int altp2m_change_gfn(struct domain *d,
>> + unsigned int idx,
>> + gfn_t old_gfn,
>> + gfn_t new_gfn);
>> +
>> #endif /* __ASM_ARM_ALTP2M_H */
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 16e33ca..8433d66 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -182,6 +182,10 @@ mfn_t p2m_lookup_attr(struct p2m_domain *p2m,
>> gfn_t gfn, p2m_type_t *t,
>> unsigned int *level, unsigned int *mattr,
>> xenmem_access_t *xma);
>>
>> +/* Remove an altp2m view's entry. */
>> +int remove_altp2m_entry(struct domain *d, struct p2m_domain *p2m,
>> + paddr_t gpa, paddr_t maddr, unsigned int
>> level);
>> +
>> /* Modify an altp2m view's entry or its attributes. */
>> int modify_altp2m_entry(struct domain *d, struct p2m_domain *p2m,
>> paddr_t gpa, paddr_t maddr, unsigned int level,
>>
>
Best regards,
~Sergej
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |