| 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 |
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
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
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 --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)
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(-)