Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()
On 03/03/2020 13:25, Jan Beulich wrote: On 04.02.2020 14:06, Julien Grall wrote: From: Julien Grall Unlike shadow_enable(), hap_enable() can only be called once during domain creation and with the mode equal to mode equal to PG_external | PG_translate | PG_refcounts. If it were called twice, then we might have something interesting problem as the p2m tables would be re-allocated (and therefore all the mappings would be lost). Add code to sanity check the mode and that the function is only called once. Take the opportunity to an if checking that PG_translate is set. Signed-off-by: Julien Grall Acked-by: Jan Beulich preferably with the duplicate words on the second line of the description dropped. I will remove it on commit. I keep the check != 0 because this is consistent with the rest of the file. If we want to omit comparison against 0, then this should be in a separate patches converting the file. I disagree, but not enough to make the ack dependent upon the adjustment. --- a/xen/arch/x86/mm/hap/hap.c +++ b/xen/arch/x86/mm/hap/hap.c @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode) unsigned int i; int rv = 0; +if ( mode != (PG_external | PG_translate | PG_refcounts) ) +return -EINVAL; + +/* The function can only be called once */ +if ( d->arch.paging.mode != 0 ) +return -EEXIST; I think if such a comment gets added, it should be unambiguous. The function can be called once per domain, not just once in total. I will replace with "The function can only be called one per domain". Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()
On 03/03/2020 13:27, Jan Beulich wrote: On 03.03.2020 13:21, Julien Grall wrote: Ping again. To be honest, with the recent maintainer change it would probably have been helpful if you had simply re-sent the patch. Well, I don't think you can expect a contributor to resend a patch because the maintainers has changed. It would have been nicer if there was an handover with the list of pending patches. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()
On 03.03.2020 13:21, Julien Grall wrote: > Ping again. To be honest, with the recent maintainer change it would probably have been helpful if you had simply re-sent the patch. I did see it back then, but had no comments to make and didn't have the authority to ack it, so simply dropped it from my mailbox seeing that you would be able to commit it yourself eventually. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()
On 04.02.2020 14:06, Julien Grall wrote: > From: Julien Grall > > Unlike shadow_enable(), hap_enable() can only be called once during > domain creation and with the mode equal to mode equal to > PG_external | PG_translate | PG_refcounts. > > If it were called twice, then we might have something interesting > problem as the p2m tables would be re-allocated (and therefore all the > mappings would be lost). > > Add code to sanity check the mode and that the function is only called > once. Take the opportunity to an if checking that PG_translate is set. > > Signed-off-by: Julien Grall Acked-by: Jan Beulich preferably with the duplicate words on the second line of the description dropped. > I keep the check != 0 because this is consistent with the rest of the > file. If we want to omit comparison against 0, then this should be in a > separate patches converting the file. I disagree, but not enough to make the ack dependent upon the adjustment. > --- a/xen/arch/x86/mm/hap/hap.c > +++ b/xen/arch/x86/mm/hap/hap.c > @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode) > unsigned int i; > int rv = 0; > > +if ( mode != (PG_external | PG_translate | PG_refcounts) ) > +return -EINVAL; > + > +/* The function can only be called once */ > +if ( d->arch.paging.mode != 0 ) > +return -EEXIST; I think if such a comment gets added, it should be unambiguous. The function can be called once per domain, not just once in total. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()
Ping again.
On 13/02/2020 12:44, Julien Grall wrote:
Hi,
Gentle ping.
Cheers,
On 04/02/2020 14:06, Julien Grall wrote:
From: Julien Grall
Unlike shadow_enable(), hap_enable() can only be called once during
domain creation and with the mode equal to mode equal to
PG_external | PG_translate | PG_refcounts.
If it were called twice, then we might have something interesting
problem as the p2m tables would be re-allocated (and therefore all the
mappings would be lost).
Add code to sanity check the mode and that the function is only called
once. Take the opportunity to an if checking that PG_translate is set.
Signed-off-by: Julien Grall
---
It is not entirely clear when PG_translate was enforced.
I keep the check != 0 because this is consistent with the rest of the
file. If we want to omit comparison against 0, then this should be in a
separate patches converting the file.
Changes in v2:
- Fix typoes in the commit message
- Use -EEXIST instead of -EINVAL
---
xen/arch/x86/mm/hap/hap.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 31362a31b6..4974bd13d4 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
unsigned int i;
int rv = 0;
+ if ( mode != (PG_external | PG_translate | PG_refcounts) )
+ return -EINVAL;
+
+ /* The function can only be called once */
+ if ( d->arch.paging.mode != 0 )
+ return -EEXIST;
+
domain_pause(d);
old_pages = d->arch.paging.hap.total_pages;
@@ -465,13 +472,10 @@ int hap_enable(struct domain *d, u32 mode)
d->arch.paging.alloc_page = hap_alloc_p2m_page;
d->arch.paging.free_page = hap_free_p2m_page;
- /* allocate P2m table */
- if ( mode & PG_translate )
- {
- rv = p2m_alloc_table(p2m_get_hostp2m(d));
- if ( rv != 0 )
- goto out;
- }
+ /* allocate P2M table */
+ rv = p2m_alloc_table(p2m_get_hostp2m(d));
+ if ( rv != 0 )
+ goto out;
for ( i = 0; i < MAX_NESTEDP2M; i++ )
{
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()
Hi,
Gentle ping.
Cheers,
On 04/02/2020 14:06, Julien Grall wrote:
From: Julien Grall
Unlike shadow_enable(), hap_enable() can only be called once during
domain creation and with the mode equal to mode equal to
PG_external | PG_translate | PG_refcounts.
If it were called twice, then we might have something interesting
problem as the p2m tables would be re-allocated (and therefore all the
mappings would be lost).
Add code to sanity check the mode and that the function is only called
once. Take the opportunity to an if checking that PG_translate is set.
Signed-off-by: Julien Grall
---
It is not entirely clear when PG_translate was enforced.
I keep the check != 0 because this is consistent with the rest of the
file. If we want to omit comparison against 0, then this should be in a
separate patches converting the file.
Changes in v2:
- Fix typoes in the commit message
- Use -EEXIST instead of -EINVAL
---
xen/arch/x86/mm/hap/hap.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 31362a31b6..4974bd13d4 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
unsigned int i;
int rv = 0;
+if ( mode != (PG_external | PG_translate | PG_refcounts) )
+return -EINVAL;
+
+/* The function can only be called once */
+if ( d->arch.paging.mode != 0 )
+return -EEXIST;
+
domain_pause(d);
old_pages = d->arch.paging.hap.total_pages;
@@ -465,13 +472,10 @@ int hap_enable(struct domain *d, u32 mode)
d->arch.paging.alloc_page = hap_alloc_p2m_page;
d->arch.paging.free_page = hap_free_p2m_page;
-/* allocate P2m table */
-if ( mode & PG_translate )
-{
-rv = p2m_alloc_table(p2m_get_hostp2m(d));
-if ( rv != 0 )
-goto out;
-}
+/* allocate P2M table */
+rv = p2m_alloc_table(p2m_get_hostp2m(d));
+if ( rv != 0 )
+goto out;
for ( i = 0; i < MAX_NESTEDP2M; i++ )
{
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()
On 04/02/2020 13:16, Durrant, Paul wrote: -Original Message- From: Xen-devel On Behalf Of Julien Grall Sent: 04 February 2020 13:06 To: xen-devel@lists.xenproject.org Cc: jul...@xen.org; Wei Liu ; George Dunlap ; Andrew Cooper ; Grall, Julien ; Jan Beulich ; Roger Pau Monné Subject: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable() From: Julien Grall Unlike shadow_enable(), hap_enable() can only be called once during domain creation and with the mode equal to mode equal to PG_external | PG_translate | PG_refcounts. If it were called twice, then we might have something interesting problem as the p2m tables would be re-allocated (and therefore all the mappings would be lost). There are two tests in p2m_alloc_tabl2: whether the domain has memory allocated, and whether the domain already has a p2m. Can these now be dropped? I don't think so. They are still necessary for the shadow page-tables. AFAICT, they are enabled via a DOMCTL. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()
> -Original Message-
> From: Xen-devel On Behalf Of
> Julien Grall
> Sent: 04 February 2020 13:06
> To: xen-devel@lists.xenproject.org
> Cc: jul...@xen.org; Wei Liu ; George Dunlap
> ; Andrew Cooper ;
> Grall, Julien ; Jan Beulich ; Roger
> Pau Monné
> Subject: [Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden
> hap_enable()
>
> From: Julien Grall
>
> Unlike shadow_enable(), hap_enable() can only be called once during
> domain creation and with the mode equal to mode equal to
> PG_external | PG_translate | PG_refcounts.
>
> If it were called twice, then we might have something interesting
> problem as the p2m tables would be re-allocated (and therefore all the
> mappings would be lost).
There are two tests in p2m_alloc_tabl2: whether the domain has memory
allocated, and whether the domain already has a p2m. Can these now be dropped?
Paul
>
> Add code to sanity check the mode and that the function is only called
> once. Take the opportunity to an if checking that PG_translate is set.
>
> Signed-off-by: Julien Grall
>
> ---
>
> It is not entirely clear when PG_translate was enforced.
>
> I keep the check != 0 because this is consistent with the rest of the
> file. If we want to omit comparison against 0, then this should be in a
> separate patches converting the file.
>
> Changes in v2:
> - Fix typoes in the commit message
> - Use -EEXIST instead of -EINVAL
> ---
> xen/arch/x86/mm/hap/hap.c | 18 +++---
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 31362a31b6..4974bd13d4 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
> unsigned int i;
> int rv = 0;
>
> +if ( mode != (PG_external | PG_translate | PG_refcounts) )
> +return -EINVAL;
> +
> +/* The function can only be called once */
> +if ( d->arch.paging.mode != 0 )
> +return -EEXIST;
> +
> domain_pause(d);
>
> old_pages = d->arch.paging.hap.total_pages;
> @@ -465,13 +472,10 @@ int hap_enable(struct domain *d, u32 mode)
> d->arch.paging.alloc_page = hap_alloc_p2m_page;
> d->arch.paging.free_page = hap_free_p2m_page;
>
> -/* allocate P2m table */
> -if ( mode & PG_translate )
> -{
> -rv = p2m_alloc_table(p2m_get_hostp2m(d));
> -if ( rv != 0 )
> -goto out;
> -}
> +/* allocate P2M table */
> +rv = p2m_alloc_table(p2m_get_hostp2m(d));
> +if ( rv != 0 )
> +goto out;
>
> for ( i = 0; i < MAX_NESTEDP2M; i++ )
> {
> --
> 2.17.1
>
>
> ___
> 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
[Xen-devel] [PATCH v2 2/2] xen/x86: hap: Clean-up and harden hap_enable()
From: Julien Grall
Unlike shadow_enable(), hap_enable() can only be called once during
domain creation and with the mode equal to mode equal to
PG_external | PG_translate | PG_refcounts.
If it were called twice, then we might have something interesting
problem as the p2m tables would be re-allocated (and therefore all the
mappings would be lost).
Add code to sanity check the mode and that the function is only called
once. Take the opportunity to an if checking that PG_translate is set.
Signed-off-by: Julien Grall
---
It is not entirely clear when PG_translate was enforced.
I keep the check != 0 because this is consistent with the rest of the
file. If we want to omit comparison against 0, then this should be in a
separate patches converting the file.
Changes in v2:
- Fix typoes in the commit message
- Use -EEXIST instead of -EINVAL
---
xen/arch/x86/mm/hap/hap.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 31362a31b6..4974bd13d4 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
unsigned int i;
int rv = 0;
+if ( mode != (PG_external | PG_translate | PG_refcounts) )
+return -EINVAL;
+
+/* The function can only be called once */
+if ( d->arch.paging.mode != 0 )
+return -EEXIST;
+
domain_pause(d);
old_pages = d->arch.paging.hap.total_pages;
@@ -465,13 +472,10 @@ int hap_enable(struct domain *d, u32 mode)
d->arch.paging.alloc_page = hap_alloc_p2m_page;
d->arch.paging.free_page = hap_free_p2m_page;
-/* allocate P2m table */
-if ( mode & PG_translate )
-{
-rv = p2m_alloc_table(p2m_get_hostp2m(d));
-if ( rv != 0 )
-goto out;
-}
+/* allocate P2M table */
+rv = p2m_alloc_table(p2m_get_hostp2m(d));
+if ( rv != 0 )
+goto out;
for ( i = 0; i < MAX_NESTEDP2M; i++ )
{
--
2.17.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
