| Message ID | 20190424142718.14721-1-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] x86/mm: Make change_type_range return error | expand |
On Wed, Apr 24, 2019 at 02:27:32PM +0000, Alexandru Stefan ISAILA wrote: > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index 9e81a30cc4..27697d5a77 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1028,7 +1028,7 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn_l, > } > > /* Modify the p2m type of [start, end_exclusive) from ot to nt. */ > -static void change_type_range(struct p2m_domain *p2m, > +static int change_type_range(struct p2m_domain *p2m, > unsigned long start, unsigned long end_exclusive, > p2m_type_t ot, p2m_type_t nt) > { > @@ -1053,15 +1053,11 @@ static void change_type_range(struct p2m_domain *p2m, > * This should be revisited later, but for now post a warning. > */ > if ( unlikely(end > host_max_pfn) ) > - { > - printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n", > - d->domain_id); > - end = invalidate_end = host_max_pfn; > - } > + return -EINVAL; > > /* If the requested range is out of scope, return doing nothing. */ > if ( start > end ) > - return; > + return 0; Since you are already changing the behavior of the function above this should also return EINVAL IMO. > > if ( p2m_is_altp2m(p2m) ) > invalidate_end = min(invalidate_end, max_pfn); > @@ -1115,13 +1111,16 @@ static void change_type_range(struct p2m_domain *p2m, > rc, d->domain_id); > domain_crash(d); > } > + > + return 0; Here you should use 'return rc;', or else you are dropping the error code from the calls from the rangeset functions. It would be worth to consider where the domain_crash paths in the function could be followed by a return rc, so that you don't lose the error code from the call to change_entry_type_range. > } > > -void p2m_change_type_range(struct domain *d, > - unsigned long start, unsigned long end, > - p2m_type_t ot, p2m_type_t nt) > +int p2m_change_type_range(struct domain *d, > + unsigned long start, unsigned long end, > + p2m_type_t ot, p2m_type_t nt) > { > struct p2m_domain *hostp2m = p2m_get_hostp2m(d); > + int rc = 0; I don't think you need to initialize rc, it will be unconditionally initialized by the call to change_type_range below. Thanks, Roger.
On 4/24/19 5:46 PM, Roger Pau Monné wrote: > On Wed, Apr 24, 2019 at 02:27:32PM +0000, Alexandru Stefan ISAILA wrote: >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >> index 9e81a30cc4..27697d5a77 100644 >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -1028,7 +1028,7 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn_l, >> } >> >> /* Modify the p2m type of [start, end_exclusive) from ot to nt. */ >> -static void change_type_range(struct p2m_domain *p2m, >> +static int change_type_range(struct p2m_domain *p2m, >> unsigned long start, unsigned long end_exclusive, >> p2m_type_t ot, p2m_type_t nt) >> { >> @@ -1053,15 +1053,11 @@ static void change_type_range(struct p2m_domain *p2m, >> * This should be revisited later, but for now post a warning. >> */ >> if ( unlikely(end > host_max_pfn) ) >> - { >> - printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n", >> - d->domain_id); >> - end = invalidate_end = host_max_pfn; >> - } >> + return -EINVAL; >> >> /* If the requested range is out of scope, return doing nothing. */ >> if ( start > end ) >> - return; >> + return 0; > > Since you are already changing the behavior of the function above this > should also return EINVAL IMO. My fault, I suggested 0 there. :) Thanks, Razvan
>>> On 24.04.19 at 16:46, <roger.pau@citrix.com> wrote: > On Wed, Apr 24, 2019 at 02:27:32PM +0000, Alexandru Stefan ISAILA wrote: >> @@ -1053,15 +1053,11 @@ static void change_type_range(struct p2m_domain *p2m, >> * This should be revisited later, but for now post a warning. >> */ >> if ( unlikely(end > host_max_pfn) ) >> - { >> - printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n", >> - d->domain_id); >> - end = invalidate_end = host_max_pfn; >> - } >> + return -EINVAL; >> >> /* If the requested range is out of scope, return doing nothing. */ >> if ( start > end ) >> - return; >> + return 0; > > Since you are already changing the behavior of the function above this > should also return EINVAL IMO. I don't think I agree. Quite the other way around: In the latter case it's simply an empty range that gets requested, which is a no-op (and hence no reason to fail). Avoiding empty ranges in the callers may result in less readable code there. Even in the former case I don't think returning an error is appropriate, the more that the comment there says this is probably not the right behavior. I think it would be better to leave this alone until we have settled on what the right behavior here is. It is an issue anyway that a change is made without saying why the new behavior preferable over the current one. In any event the comment there would become stale with the removal of the printk(). Jan
On 25.04.2019 15:54, Jan Beulich wrote: >>>> On 24.04.19 at 16:46, <roger.pau@citrix.com> wrote: >> On Wed, Apr 24, 2019 at 02:27:32PM +0000, Alexandru Stefan ISAILA wrote: >>> @@ -1053,15 +1053,11 @@ static void change_type_range(struct p2m_domain *p2m, >>> * This should be revisited later, but for now post a warning. >>> */ >>> if ( unlikely(end > host_max_pfn) ) >>> - { >>> - printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n", >>> - d->domain_id); >>> - end = invalidate_end = host_max_pfn; >>> - } >>> + return -EINVAL; >>> >>> /* If the requested range is out of scope, return doing nothing. */ >>> if ( start > end ) >>> - return; >>> + return 0; >> >> Since you are already changing the behavior of the function above this >> should also return EINVAL IMO. > > I don't think I agree. Quite the other way around: In the latter > case it's simply an empty range that gets requested, which is a > no-op (and hence no reason to fail). Avoiding empty ranges in > the callers may result in less readable code there. > > Even in the former case I don't think returning an error is > appropriate, the more that the comment there says this is > probably not the right behavior. I think it would be better to > leave this alone until we have settled on what the right > behavior here is. I guess George may have a say here to clarify the right behavior. > It is an issue anyway that a change is > made without saying why the new behavior preferable over > the current one. So is there a way to continue with this? > > In any event the comment there would become stale with the > removal of the printk(). I will change the comment. Alex
>>> On 03.05.19 at 09:53, <aisaila@bitdefender.com> wrote: > On 25.04.2019 15:54, Jan Beulich wrote: >> It is an issue anyway that a change is >> made without saying why the new behavior preferable over >> the current one. > > So is there a way to continue with this? Why not - I've not said I'm against, I've just asked for an improved description. Of course, if it turns out the change is done "just in case", I'm not sure I see much value. But as you say, it's first and foremost George to judge. Jan
Hi George, Did you have time to look at this patch? Regards, Alex On 03.05.2019 11:04, Jan Beulich wrote: >>>> On 03.05.19 at 09:53, <aisaila@bitdefender.com> wrote: >> On 25.04.2019 15:54, Jan Beulich wrote: >>> It is an issue anyway that a change is >>> made without saying why the new behavior preferable over >>> the current one. >> >> So is there a way to continue with this? > > Why not - I've not said I'm against, I've just asked for an improved > description. Of course, if it turns out the change is done "just in > case", I'm not sure I see much value. But as you say, it's first and > foremost George to judge. > > Jan > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel >
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c index 412a442b6a..b113c3154b 100644 --- a/xen/arch/x86/mm/hap/hap.c +++ b/xen/arch/x86/mm/hap/hap.c @@ -108,15 +108,19 @@ int hap_track_dirty_vram(struct domain *d, paging_unlock(d); if ( oend > ostart ) - p2m_change_type_range(d, ostart, oend, - p2m_ram_logdirty, p2m_ram_rw); + rc = p2m_change_type_range(d, ostart, oend, + p2m_ram_logdirty, p2m_ram_rw); + if ( rc ) + goto out; /* * Switch vram to log dirty mode, either by setting l1e entries of * P2M table to be read-only, or via hardware-assisted log-dirty. */ - p2m_change_type_range(d, begin_pfn, begin_pfn + nr, - p2m_ram_rw, p2m_ram_logdirty); + rc = p2m_change_type_range(d, begin_pfn, begin_pfn + nr, + p2m_ram_rw, p2m_ram_logdirty); + if ( rc ) + goto out; flush_tlb_mask(d->dirty_cpumask); diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 9e81a30cc4..27697d5a77 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1028,7 +1028,7 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn_l, } /* Modify the p2m type of [start, end_exclusive) from ot to nt. */ -static void change_type_range(struct p2m_domain *p2m, +static int change_type_range(struct p2m_domain *p2m, unsigned long start, unsigned long end_exclusive, p2m_type_t ot, p2m_type_t nt) { @@ -1053,15 +1053,11 @@ static void change_type_range(struct p2m_domain *p2m, * This should be revisited later, but for now post a warning. */ if ( unlikely(end > host_max_pfn) ) - { - printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n", - d->domain_id); - end = invalidate_end = host_max_pfn; - } + return -EINVAL; /* If the requested range is out of scope, return doing nothing. */ if ( start > end ) - return; + return 0; if ( p2m_is_altp2m(p2m) ) invalidate_end = min(invalidate_end, max_pfn); @@ -1115,13 +1111,16 @@ static void change_type_range(struct p2m_domain *p2m, rc, d->domain_id); domain_crash(d); } + + return 0; } -void p2m_change_type_range(struct domain *d, - unsigned long start, unsigned long end, - p2m_type_t ot, p2m_type_t nt) +int p2m_change_type_range(struct domain *d, + unsigned long start, unsigned long end, + p2m_type_t ot, p2m_type_t nt) { struct p2m_domain *hostp2m = p2m_get_hostp2m(d); + int rc = 0; ASSERT(ot != nt); ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt)); @@ -1129,7 +1128,10 @@ void p2m_change_type_range(struct domain *d, p2m_lock(hostp2m); hostp2m->defer_nested_flush = 1; - change_type_range(hostp2m, start, end, ot, nt); + rc = change_type_range(hostp2m, start, end, ot, nt); + + if ( rc ) + goto out; #ifdef CONFIG_HVM if ( unlikely(altp2m_active(d)) ) @@ -1142,8 +1144,11 @@ void p2m_change_type_range(struct domain *d, struct p2m_domain *altp2m = d->arch.altp2m_p2m[i]; p2m_lock(altp2m); - change_type_range(altp2m, start, end, ot, nt); + rc = change_type_range(altp2m, start, end, ot, nt); p2m_unlock(altp2m); + + if ( rc ) + goto out; } } #endif @@ -1151,7 +1156,10 @@ void p2m_change_type_range(struct domain *d, if ( nestedhvm_enabled(d) ) p2m_flush_nestedp2m(d); +out: p2m_unlock(hostp2m); + + return rc; } /* diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 2801a8ccca..a118812025 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -613,9 +613,9 @@ void p2m_change_entry_type_global(struct domain *d, p2m_type_t ot, p2m_type_t nt); /* Change types across a range of p2m entries (start ... end-1) */ -void p2m_change_type_range(struct domain *d, - unsigned long start, unsigned long end, - p2m_type_t ot, p2m_type_t nt); +int p2m_change_type_range(struct domain *d, + unsigned long start, unsigned long end, + p2m_type_t ot, p2m_type_t nt); /* Compare-exchange the type of a single p2m entry */ int p2m_change_type_one(struct domain *d, unsigned long gfn,
At this moment change_type_range() prints a warning in case end > host_max_pfn. While this is unlikely to happen the function should return a error and propagate it to the caller, hap_track_dirty_vram() This patch makes change_type_range() return -EINVAL or 0 if all is ok. Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> --- xen/arch/x86/mm/hap/hap.c | 12 ++++++++---- xen/arch/x86/mm/p2m.c | 32 ++++++++++++++++++++------------ xen/include/asm-x86/p2m.h | 6 +++--- 3 files changed, 31 insertions(+), 19 deletions(-)