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(-)
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(-)
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,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> 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
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 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.