diff mbox series

[v2,1/3] x86/mm: Introduce altp2m_get_gfn_type_access

Message ID 20190405132521.6630-1-aisaila@bitdefender.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/3] x86/mm: Introduce altp2m_get_gfn_type_access | expand

Commit Message

Isaila Alexandru April 5, 2019, 1:25 p.m. UTC
This patch moves common code from p2m_set_altp2m_mem_access() and
p2m_change_altp2m_gfn() into one function

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/mm/mem_access.c | 30 +++++++++++------------------
 xen/arch/x86/mm/p2m.c        | 37 ++++++++++++++----------------------
 xen/include/asm-x86/p2m.h    | 23 ++++++++++++++++++++++
 3 files changed, 48 insertions(+), 42 deletions(-)

Comments

Tamas K Lengyel April 5, 2019, 3:04 p.m. UTC | #1
On Fri, Apr 5, 2019 at 7:25 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
> This patch moves common code from p2m_set_altp2m_mem_access() and
> p2m_change_altp2m_gfn() into one function
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
>  xen/arch/x86/mm/mem_access.c | 30 +++++++++++------------------
>  xen/arch/x86/mm/p2m.c        | 37 ++++++++++++++----------------------
>  xen/include/asm-x86/p2m.h    | 23 ++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 56c06a4fc6..608f748a57 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -265,31 +265,23 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>      unsigned int page_order;
>      unsigned long gfn_l = gfn_x(gfn);
>      int rc;
> +    bool found_in_hostp2m;
>
> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &found_in_hostp2m);
>
> -    /* Check host p2m if no valid entry in alternate */
>      if ( !mfn_valid(mfn) )
> -    {
> +        return -ESRCH;
>
> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> +    /* If this is a superpage, copy that first */
> +    if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
> +    {
> +        unsigned long mask = ~((1UL << page_order) - 1);
> +        gfn_t gfn2 = _gfn(gfn_l & mask);
> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>
> -        rc = -ESRCH;
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> +        if ( rc )
>              return rc;
> -
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            unsigned long mask = ~((1UL << page_order) - 1);
> -            gfn_t gfn2 = _gfn(gfn_l & mask);
> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> -
> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> -            if ( rc )
> -                return rc;
> -        }
>      }
>
>      /*
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b9bbb8f485..b2a5c0c42e 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      mfn_t mfn;
>      unsigned int page_order;
>      int rc = -EINVAL;
> +    bool found_in_hostp2m;
>
>      if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>          return rc;
> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>      p2m_lock(hp2m);
>      p2m_lock(ap2m);
>
> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &found_in_hostp2m);
>
>      if ( gfn_eq(new_gfn, INVALID_GFN) )
>      {
> @@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>
>      /* Check host p2m if no valid entry in alternate */
>      if ( !mfn_valid(mfn) )
> -    {
> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> -                                    P2M_ALLOC, &page_order, 0);
> -
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> -            goto out;
> +         goto out;
>
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            gfn_t gfn;
> -            unsigned long mask;
> +    /* If this is a superpage, copy that first */
> +    if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
> +    {
> +        gfn_t gfn;
> +        unsigned long mask;
>
> -            mask = ~((1UL << page_order) - 1);
> -            gfn = _gfn(gfn_x(old_gfn) & mask);
> -            mfn = _mfn(mfn_x(mfn) & mask);
> +        mask = ~((1UL << page_order) - 1);
> +        gfn = _gfn(gfn_x(old_gfn) & mask);
> +        mfn = _mfn(mfn_x(mfn) & mask);
>
> -            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> -                goto out;
> -        }
> +        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> +            goto out;
>      }
>
> -    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
> +    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
>
>      if ( !mfn_valid(mfn) )
> -        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
> -
> -    /* Note: currently it is not safe to remap to a shared entry */
> -    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
>          goto out;
>
>      if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 2801a8ccca..42068b4aed 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -448,6 +448,29 @@ static inline mfn_t __nonnull(3) get_gfn_type(
>      return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
>  }
>
> +static inline mfn_t altp2m_get_gfn_type_access(
> +    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
> +    unsigned int *page_order, bool *found_in_hostp2m)

I don't like this. The way it's implemented is very convoluted and not
consistent.

> +{
> +    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
> +
> +    *found_in_hostp2m = false;

Just because an entry is in the altp2m now means it's not found in the
hostp2m? Doesn't make sense.

> +
> +    /* Check host p2m if no valid entry in alternate */
> +    if ( !mfn_valid(mfn) )
> +    {
> +        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
> +                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
> +        *found_in_hostp2m = true;

Now it's found in the hostp2m, even if the mfn is invalid? Again,
doesn't make sense.

> +
> +        /* Note: currently it is not safe to remap to a shared entry */
> +        if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
> +            return INVALID_MFN;

This function is supposed to just get the type and access.. but
instead it does sanity checks on the results that the caller is
probably in a better position to judge. Again, inconsistent with the
function's name and seemingly intended use.

Tamas
Isaila Alexandru April 8, 2019, 8:13 a.m. UTC | #2
On 05.04.2019 18:04, Tamas K Lengyel wrote:
> On Fri, Apr 5, 2019 at 7:25 AM Alexandru Stefan ISAILA
> <aisaila@bitdefender.com> wrote:
>>
>> This patch moves common code from p2m_set_altp2m_mem_access() and
>> p2m_change_altp2m_gfn() into one function
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>> ---
>>   xen/arch/x86/mm/mem_access.c | 30 +++++++++++------------------
>>   xen/arch/x86/mm/p2m.c        | 37 ++++++++++++++----------------------
>>   xen/include/asm-x86/p2m.h    | 23 ++++++++++++++++++++++
>>   3 files changed, 48 insertions(+), 42 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 56c06a4fc6..608f748a57 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -265,31 +265,23 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>       unsigned int page_order;
>>       unsigned long gfn_l = gfn_x(gfn);
>>       int rc;
>> +    bool found_in_hostp2m;
>>
>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &found_in_hostp2m);
>>
>> -    /* Check host p2m if no valid entry in alternate */
>>       if ( !mfn_valid(mfn) )
>> -    {
>> +        return -ESRCH;
>>
>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>> +    /* If this is a superpage, copy that first */
>> +    if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
>> +    {
>> +        unsigned long mask = ~((1UL << page_order) - 1);
>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>
>> -        rc = -ESRCH;
>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> +        if ( rc )
>>               return rc;
>> -
>> -        /* If this is a superpage, copy that first */
>> -        if ( page_order != PAGE_ORDER_4K )
>> -        {
>> -            unsigned long mask = ~((1UL << page_order) - 1);
>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>> -
>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> -            if ( rc )
>> -                return rc;
>> -        }
>>       }
>>
>>       /*
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index b9bbb8f485..b2a5c0c42e 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>       mfn_t mfn;
>>       unsigned int page_order;
>>       int rc = -EINVAL;
>> +    bool found_in_hostp2m;
>>
>>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>           return rc;
>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>       p2m_lock(hp2m);
>>       p2m_lock(ap2m);
>>
>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &found_in_hostp2m);
>>
>>       if ( gfn_eq(new_gfn, INVALID_GFN) )
>>       {
>> @@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>
>>       /* Check host p2m if no valid entry in alternate */
>>       if ( !mfn_valid(mfn) )
>> -    {
>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>> -                                    P2M_ALLOC, &page_order, 0);
>> -
>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> -            goto out;
>> +         goto out;
>>
>> -        /* If this is a superpage, copy that first */
>> -        if ( page_order != PAGE_ORDER_4K )
>> -        {
>> -            gfn_t gfn;
>> -            unsigned long mask;
>> +    /* If this is a superpage, copy that first */
>> +    if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
>> +    {
>> +        gfn_t gfn;
>> +        unsigned long mask;
>>
>> -            mask = ~((1UL << page_order) - 1);
>> -            gfn = _gfn(gfn_x(old_gfn) & mask);
>> -            mfn = _mfn(mfn_x(mfn) & mask);
>> +        mask = ~((1UL << page_order) - 1);
>> +        gfn = _gfn(gfn_x(old_gfn) & mask);
>> +        mfn = _mfn(mfn_x(mfn) & mask);
>>
>> -            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
>> -                goto out;
>> -        }
>> +        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
>> +            goto out;
>>       }
>>
>> -    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
>>
>>       if ( !mfn_valid(mfn) )
>> -        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
>> -
>> -    /* Note: currently it is not safe to remap to a shared entry */
>> -    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
>>           goto out;
>>
>>       if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> index 2801a8ccca..42068b4aed 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -448,6 +448,29 @@ static inline mfn_t __nonnull(3) get_gfn_type(
>>       return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
>>   }
>>
>> +static inline mfn_t altp2m_get_gfn_type_access(
>> +    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
>> +    unsigned int *page_order, bool *found_in_hostp2m)
> 
> I don't like this. The way it's implemented is very convoluted and not
> consistent.
> 
>> +{
>> +    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
>> +
>> +    *found_in_hostp2m = false;
> 
> Just because an entry is in the altp2m now means it's not found in the
> hostp2m? Doesn't make sense.

The name of the var is not a good one I must agree. I tried to 
incorporate as much as the common code in one function.

The found_in_hostp2m var is used by the caller in the page_order != 
PAGE_ORDER_4K case.

> 
>> +
>> +    /* Check host p2m if no valid entry in alternate */
>> +    if ( !mfn_valid(mfn) )
>> +    {
>> +        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
>> +                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
>> +        *found_in_hostp2m = true;
> 
> Now it's found in the hostp2m, even if the mfn is invalid? Again,
> doesn't make sense.

I agree that the true comes here early and it can get true only if mfn 
is valid.

> 
>> +
>> +        /* Note: currently it is not safe to remap to a shared entry */
>> +        if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
>> +            return INVALID_MFN;
> 
> This function is supposed to just get the type and access.. but
> instead it does sanity checks on the results that the caller is
> probably in a better position to judge. Again, inconsistent with the
> function's name and seemingly intended use.
> 

Ok I will get the check out of the function and let the caller.

About the found_in_hostp2m var, should it get a different name?

Regards,
Alex
Tamas K Lengyel April 8, 2019, 3:40 p.m. UTC | #3
On Mon, Apr 8, 2019 at 2:13 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
>
> On 05.04.2019 18:04, Tamas K Lengyel wrote:
> > On Fri, Apr 5, 2019 at 7:25 AM Alexandru Stefan ISAILA
> > <aisaila@bitdefender.com> wrote:
> >>
> >> This patch moves common code from p2m_set_altp2m_mem_access() and
> >> p2m_change_altp2m_gfn() into one function
> >>
> >> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> >> ---
> >>   xen/arch/x86/mm/mem_access.c | 30 +++++++++++------------------
> >>   xen/arch/x86/mm/p2m.c        | 37 ++++++++++++++----------------------
> >>   xen/include/asm-x86/p2m.h    | 23 ++++++++++++++++++++++
> >>   3 files changed, 48 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> >> index 56c06a4fc6..608f748a57 100644
> >> --- a/xen/arch/x86/mm/mem_access.c
> >> +++ b/xen/arch/x86/mm/mem_access.c
> >> @@ -265,31 +265,23 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
> >>       unsigned int page_order;
> >>       unsigned long gfn_l = gfn_x(gfn);
> >>       int rc;
> >> +    bool found_in_hostp2m;
> >>
> >> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> >> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &found_in_hostp2m);
> >>
> >> -    /* Check host p2m if no valid entry in alternate */
> >>       if ( !mfn_valid(mfn) )
> >> -    {
> >> +        return -ESRCH;
> >>
> >> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> >> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> >> +    /* If this is a superpage, copy that first */
> >> +    if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
> >> +    {
> >> +        unsigned long mask = ~((1UL << page_order) - 1);
> >> +        gfn_t gfn2 = _gfn(gfn_l & mask);
> >> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >>
> >> -        rc = -ESRCH;
> >> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> >> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> >> +        if ( rc )
> >>               return rc;
> >> -
> >> -        /* If this is a superpage, copy that first */
> >> -        if ( page_order != PAGE_ORDER_4K )
> >> -        {
> >> -            unsigned long mask = ~((1UL << page_order) - 1);
> >> -            gfn_t gfn2 = _gfn(gfn_l & mask);
> >> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> >> -
> >> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> >> -            if ( rc )
> >> -                return rc;
> >> -        }
> >>       }
> >>
> >>       /*
> >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> >> index b9bbb8f485..b2a5c0c42e 100644
> >> --- a/xen/arch/x86/mm/p2m.c
> >> +++ b/xen/arch/x86/mm/p2m.c
> >> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>       mfn_t mfn;
> >>       unsigned int page_order;
> >>       int rc = -EINVAL;
> >> +    bool found_in_hostp2m;
> >>
> >>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> >>           return rc;
> >> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>       p2m_lock(hp2m);
> >>       p2m_lock(ap2m);
> >>
> >> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
> >> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &found_in_hostp2m);
> >>
> >>       if ( gfn_eq(new_gfn, INVALID_GFN) )
> >>       {
> >> @@ -2648,35 +2649,25 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> >>
> >>       /* Check host p2m if no valid entry in alternate */
> >>       if ( !mfn_valid(mfn) )
> >> -    {
> >> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> >> -                                    P2M_ALLOC, &page_order, 0);
> >> -
> >> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> >> -            goto out;
> >> +         goto out;
> >>
> >> -        /* If this is a superpage, copy that first */
> >> -        if ( page_order != PAGE_ORDER_4K )
> >> -        {
> >> -            gfn_t gfn;
> >> -            unsigned long mask;
> >> +    /* If this is a superpage, copy that first */
> >> +    if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
> >> +    {
> >> +        gfn_t gfn;
> >> +        unsigned long mask;
> >>
> >> -            mask = ~((1UL << page_order) - 1);
> >> -            gfn = _gfn(gfn_x(old_gfn) & mask);
> >> -            mfn = _mfn(mfn_x(mfn) & mask);
> >> +        mask = ~((1UL << page_order) - 1);
> >> +        gfn = _gfn(gfn_x(old_gfn) & mask);
> >> +        mfn = _mfn(mfn_x(mfn) & mask);
> >>
> >> -            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> >> -                goto out;
> >> -        }
> >> +        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> >> +            goto out;
> >>       }
> >>
> >> -    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
> >> +    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
> >>
> >>       if ( !mfn_valid(mfn) )
> >> -        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
> >> -
> >> -    /* Note: currently it is not safe to remap to a shared entry */
> >> -    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
> >>           goto out;
> >>
> >>       if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
> >> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> >> index 2801a8ccca..42068b4aed 100644
> >> --- a/xen/include/asm-x86/p2m.h
> >> +++ b/xen/include/asm-x86/p2m.h
> >> @@ -448,6 +448,29 @@ static inline mfn_t __nonnull(3) get_gfn_type(
> >>       return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
> >>   }
> >>
> >> +static inline mfn_t altp2m_get_gfn_type_access(
> >> +    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
> >> +    unsigned int *page_order, bool *found_in_hostp2m)
> >
> > I don't like this. The way it's implemented is very convoluted and not
> > consistent.
> >
> >> +{
> >> +    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
> >> +
> >> +    *found_in_hostp2m = false;
> >
> > Just because an entry is in the altp2m now means it's not found in the
> > hostp2m? Doesn't make sense.
>
> The name of the var is not a good one I must agree. I tried to
> incorporate as much as the common code in one function.
>
> The found_in_hostp2m var is used by the caller in the page_order !=
> PAGE_ORDER_4K case.
>
> >
> >> +
> >> +    /* Check host p2m if no valid entry in alternate */
> >> +    if ( !mfn_valid(mfn) )
> >> +    {
> >> +        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
> >> +                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
> >> +        *found_in_hostp2m = true;
> >
> > Now it's found in the hostp2m, even if the mfn is invalid? Again,
> > doesn't make sense.
>
> I agree that the true comes here early and it can get true only if mfn
> is valid.
>
> >
> >> +
> >> +        /* Note: currently it is not safe to remap to a shared entry */
> >> +        if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
> >> +            return INVALID_MFN;
> >
> > This function is supposed to just get the type and access.. but
> > instead it does sanity checks on the results that the caller is
> > probably in a better position to judge. Again, inconsistent with the
> > function's name and seemingly intended use.
> >
>
> Ok I will get the check out of the function and let the caller.
>
> About the found_in_hostp2m var, should it get a different name?

It should reflect what it actually contains, which right now looks
like more like "copied_from_hostp2m".

Tamas
diff mbox series

Patch

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 56c06a4fc6..608f748a57 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -265,31 +265,23 @@  int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
     unsigned int page_order;
     unsigned long gfn_l = gfn_x(gfn);
     int rc;
+    bool found_in_hostp2m;
 
-    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &found_in_hostp2m);
 
-    /* Check host p2m if no valid entry in alternate */
     if ( !mfn_valid(mfn) )
-    {
+        return -ESRCH;
 
-        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
-                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
+    /* If this is a superpage, copy that first */
+    if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
+    {
+        unsigned long mask = ~((1UL << page_order) - 1);
+        gfn_t gfn2 = _gfn(gfn_l & mask);
+        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
 
-        rc = -ESRCH;
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
+        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
+        if ( rc )
             return rc;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            unsigned long mask = ~((1UL << page_order) - 1);
-            gfn_t gfn2 = _gfn(gfn_l & mask);
-            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
-
-            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
-            if ( rc )
-                return rc;
-        }
     }
 
     /*
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9bbb8f485..b2a5c0c42e 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2626,6 +2626,7 @@  int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     mfn_t mfn;
     unsigned int page_order;
     int rc = -EINVAL;
+    bool found_in_hostp2m;
 
     if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
         return rc;
@@ -2636,7 +2637,7 @@  int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     p2m_lock(hp2m);
     p2m_lock(ap2m);
 
-    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &found_in_hostp2m);
 
     if ( gfn_eq(new_gfn, INVALID_GFN) )
     {
@@ -2648,35 +2649,25 @@  int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
 
     /* Check host p2m if no valid entry in alternate */
     if ( !mfn_valid(mfn) )
-    {
-        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
-                                    P2M_ALLOC, &page_order, 0);
-
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            goto out;
+         goto out;
 
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            gfn_t gfn;
-            unsigned long mask;
+    /* If this is a superpage, copy that first */
+    if ( page_order != PAGE_ORDER_4K && found_in_hostp2m )
+    {
+        gfn_t gfn;
+        unsigned long mask;
 
-            mask = ~((1UL << page_order) - 1);
-            gfn = _gfn(gfn_x(old_gfn) & mask);
-            mfn = _mfn(mfn_x(mfn) & mask);
+        mask = ~((1UL << page_order) - 1);
+        gfn = _gfn(gfn_x(old_gfn) & mask);
+        mfn = _mfn(mfn_x(mfn) & mask);
 
-            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
-                goto out;
-        }
+        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
+            goto out;
     }
 
-    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
+    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &found_in_hostp2m);
 
     if ( !mfn_valid(mfn) )
-        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
-
-    /* Note: currently it is not safe to remap to a shared entry */
-    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
         goto out;
 
     if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 2801a8ccca..42068b4aed 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -448,6 +448,29 @@  static inline mfn_t __nonnull(3) get_gfn_type(
     return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
 }
 
+static inline mfn_t altp2m_get_gfn_type_access(
+    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
+    unsigned int *page_order, bool *found_in_hostp2m)
+{
+    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
+
+    *found_in_hostp2m = false;
+
+    /* Check host p2m if no valid entry in alternate */
+    if ( !mfn_valid(mfn) )
+    {
+        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a,
+                                    P2M_ALLOC | P2M_UNSHARE, page_order, false);
+        *found_in_hostp2m = true;
+
+        /* Note: currently it is not safe to remap to a shared entry */
+        if ( !mfn_valid(mfn) || *t != p2m_ram_rw )
+            return INVALID_MFN;
+    }
+
+    return mfn;
+}
+
 /* Syntactic sugar: most callers will use one of these. */
 #define get_gfn(d, g, t)         get_gfn_type((d), (g), (t), P2M_ALLOC)
 #define get_gfn_query(d, g, t)   get_gfn_type((d), (g), (t), 0)