[PATCH 00/11] assorted follow-ups to recent XSAs

[PATCH 00/11] assorted follow-ups to recent XSAs

[Jan Beulich]

01: public: adjust documentation following XSA-217
02: gnttab: remove redundant xenheap check from gnttab_transfer()
03: make steal_page() return a proper error value
04: domctl: restrict DOMCTL_set_target to HVM domains
05: evtchn: convert evtchn_port_is_*() to plain bool
06: ARM: simplify page type handling
07: x86: fold identical error paths in xenmem_add_to_physmap_one()
08: gnttab: remove host map in the event of a grant_map failure
09: gnttab: avoid spurious maptrack handle allocation failures
10: gnttab: limit mapkind()'s iteration count
11: gnttab: drop useless locking

Signed-off-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 01/11] public: adjust documentation following XSA-217

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -411,12 +411,13 @@ typedef struct gnttab_dump_table gnttab_
 DEFINE_XEN_GUEST_HANDLE(gnttab_dump_table_t);
 
 /*
- * GNTTABOP_transfer_grant_ref: Transfer <frame> to a foreign domain. The
- * foreign domain has previously registered its interest in the transfer via
- * <domid, ref>.
+ * GNTTABOP_transfer: Transfer <frame> to a foreign domain. The foreign domain
+ * has previously registered its interest in the transfer via <domid, ref>.
  *
  * Note that, even if the transfer fails, the specified page no longer belongs
  * to the calling domain *unless* the error is GNTST_bad_page.
+ *
+ * Note further that only PV guests can use this operation.
  */
 struct gnttab_transfer {
     /* IN parameters. */
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -102,6 +102,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_memory_reser
  * Returns zero on complete success, otherwise a negative error code.
  * On complete success then always @nr_exchanged == @in.nr_extents.
  * On partial success @nr_exchanged indicates how much work was done.
+ *
+ * Note that only PV guests can use this operation.
  */
 #define XENMEM_exchange             11
 struct xen_memory_exchange {




[-- Attachment #2: xsa217-doc.patch --]
[-- Type: text/plain, Size: 1394 bytes --]

public: adjust documentation following XSA-217

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -411,12 +411,13 @@ typedef struct gnttab_dump_table gnttab_
 DEFINE_XEN_GUEST_HANDLE(gnttab_dump_table_t);
 
 /*
- * GNTTABOP_transfer_grant_ref: Transfer <frame> to a foreign domain. The
- * foreign domain has previously registered its interest in the transfer via
- * <domid, ref>.
+ * GNTTABOP_transfer: Transfer <frame> to a foreign domain. The foreign domain
+ * has previously registered its interest in the transfer via <domid, ref>.
  *
  * Note that, even if the transfer fails, the specified page no longer belongs
  * to the calling domain *unless* the error is GNTST_bad_page.
+ *
+ * Note further that only PV guests can use this operation.
  */
 struct gnttab_transfer {
     /* IN parameters. */
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -102,6 +102,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_memory_reser
  * Returns zero on complete success, otherwise a negative error code.
  * On complete success then always @nr_exchanged == @in.nr_extents.
  * On partial success @nr_exchanged indicates how much work was done.
+ *
+ * Note that only PV guests can use this operation.
  */
 #define XENMEM_exchange             11
 struct xen_memory_exchange {

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 01/11] public: adjust documentation following XSA-217

[Andrew Cooper]

On 21/06/17 10:30, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 01/11] public: adjust documentation following XSA-217

[George Dunlap]

Jan,

When you have a long series like this, could you name the files in a way
that makes it easier for a shell script to get their order?  e.g.,
01-xsa217-doc.patch, 02-gnttab-xfer-xenheap.patch, &c?

Having to manually save-and-apply the name of each patch of an
eleven-patch series separately is fairly annoying.  If they started with
a number, I could save them all to the same directory and then use "for
patch in *.patch ; do..." to apply them.

(Using `git send-email` would also make things a lot easier...)

 -George

On 21/06/17 10:30, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/include/public/grant_table.h
> +++ b/xen/include/public/grant_table.h
> @@ -411,12 +411,13 @@ typedef struct gnttab_dump_table gnttab_
>  DEFINE_XEN_GUEST_HANDLE(gnttab_dump_table_t);
>  
>  /*
> - * GNTTABOP_transfer_grant_ref: Transfer <frame> to a foreign domain. The
> - * foreign domain has previously registered its interest in the transfer via
> - * <domid, ref>.
> + * GNTTABOP_transfer: Transfer <frame> to a foreign domain. The foreign domain
> + * has previously registered its interest in the transfer via <domid, ref>.
>   *
>   * Note that, even if the transfer fails, the specified page no longer belongs
>   * to the calling domain *unless* the error is GNTST_bad_page.
> + *
> + * Note further that only PV guests can use this operation.
>   */
>  struct gnttab_transfer {
>      /* IN parameters. */
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -102,6 +102,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_memory_reser
>   * Returns zero on complete success, otherwise a negative error code.
>   * On complete success then always @nr_exchanged == @in.nr_extents.
>   * On partial success @nr_exchanged indicates how much work was done.
> + *
> + * Note that only PV guests can use this operation.
>   */
>  #define XENMEM_exchange             11
>  struct xen_memory_exchange {
> 
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 01/11] public: adjust documentation following XSA-217

[Jan Beulich]

>>> On 21.06.17 at 17:44, <george.dunlap@citrix.com> wrote:
> When you have a long series like this, could you name the files in a way
> that makes it easier for a shell script to get their order?  e.g.,
> 01-xsa217-doc.patch, 02-gnttab-xfer-xenheap.patch, &c?
> 
> Having to manually save-and-apply the name of each patch of an
> eleven-patch series separately is fairly annoying.  If they started with
> a number, I could save them all to the same directory and then use "for
> patch in *.patch ; do..." to apply them.
> 
> (Using `git send-email` would also make things a lot easier...)

Well, that's kind of difficult without using git for development work.
I can try to remember to name patches suitably, but that's
another manual step for me then, and while I continue to attach
files I would have hoped that the mail bodies nowadays come
through uncorrupted (and hence I'd expect file names to be
chosen by your mail client based on subject, which includes
numbering - that's at least how mine behaves - and there being
no actual need to use the attachments).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 01/11] public: adjust documentation following XSA-217

[George Dunlap]

On 21/06/17 16:54, Jan Beulich wrote:
>>>> On 21.06.17 at 17:44, <george.dunlap@citrix.com> wrote:
>> When you have a long series like this, could you name the files in a way
>> that makes it easier for a shell script to get their order?  e.g.,
>> 01-xsa217-doc.patch, 02-gnttab-xfer-xenheap.patch, &c?
>>
>> Having to manually save-and-apply the name of each patch of an
>> eleven-patch series separately is fairly annoying.  If they started with
>> a number, I could save them all to the same directory and then use "for
>> patch in *.patch ; do..." to apply them.
>>
>> (Using `git send-email` would also make things a lot easier...)
> 
> Well, that's kind of difficult without using git for development work.
> I can try to remember to name patches suitably, but that's
> another manual step for me then

Well, it's either an extra manual step for you, or an extra manual step
for every person who reviews your code.  You don't need to justify to me
why you don't want to use git for development.  However, it's not fair
to externalize the cost of your development preferences to everybody else.

When it's just one or two patches I'm willing to make some
accommodation, but a series like this it's too much.

> , and while I continue to attach
> files I would have hoped that the mail bodies nowadays come
> through uncorrupted (and hence I'd expect file names to be
> chosen by your mail client based on subject, which includes
> numbering - that's at least how mine behaves - and there being
> no actual need to use the attachments).

I don't have a simple way of saving the text of the mail message without
saving the attachment in-line in the file; so without writing a special
tool just for your messages, I can't easily apply the patches into the
git tree (so I can see the change in-situ).

It looks like the mail bodies might actually be uncorrupted now; so it's
possible that if you send the mail without attachments anymore I might
be able to use the same workflow for you as I use for everyone else
(which presumably would extend to other people who might want to review
your patches).  If you send a test mail (or perhaps a series) I can give
it a try.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 01/11] public: adjust documentation following XSA-217

[Stefano Stabellini]

On Wed, 21 Jun 2017, George Dunlap wrote:
> On 21/06/17 16:54, Jan Beulich wrote:
> >>>> On 21.06.17 at 17:44, <george.dunlap@citrix.com> wrote:
> >> When you have a long series like this, could you name the files in a way
> >> that makes it easier for a shell script to get their order?  e.g.,
> >> 01-xsa217-doc.patch, 02-gnttab-xfer-xenheap.patch, &c?
> >>
> >> Having to manually save-and-apply the name of each patch of an
> >> eleven-patch series separately is fairly annoying.  If they started with
> >> a number, I could save them all to the same directory and then use "for
> >> patch in *.patch ; do..." to apply them.
> >>
> >> (Using `git send-email` would also make things a lot easier...)
> > 
> > Well, that's kind of difficult without using git for development work.
> > I can try to remember to name patches suitably, but that's
> > another manual step for me then
> 
> Well, it's either an extra manual step for you, or an extra manual step
> for every person who reviews your code.  You don't need to justify to me
> why you don't want to use git for development.  However, it's not fair
> to externalize the cost of your development preferences to everybody else.
> 
> When it's just one or two patches I'm willing to make some
> accommodation, but a series like this it's too much.

I don't use git either (I use guilt). My solution to this problem is to
`guilt push' all my patches, then export them back as patches from the
commits using `git format-patch'.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 01/11] public: adjust documentation following XSA-217

[Jan Beulich]

>>> On 21.06.17 at 18:53, <george.dunlap@citrix.com> wrote:
> On 21/06/17 16:54, Jan Beulich wrote:
>>>>> On 21.06.17 at 17:44, <george.dunlap@citrix.com> wrote:
>>> When you have a long series like this, could you name the files in a way
>>> that makes it easier for a shell script to get their order?  e.g.,
>>> 01-xsa217-doc.patch, 02-gnttab-xfer-xenheap.patch, &c?
>>>
>>> Having to manually save-and-apply the name of each patch of an
>>> eleven-patch series separately is fairly annoying.  If they started with
>>> a number, I could save them all to the same directory and then use "for
>>> patch in *.patch ; do..." to apply them.
>>>
>>> (Using `git send-email` would also make things a lot easier...)
>> 
>> Well, that's kind of difficult without using git for development work.
>> I can try to remember to name patches suitably, but that's
>> another manual step for me then
> 
> Well, it's either an extra manual step for you, or an extra manual step
> for every person who reviews your code.

That's a good point (which in fact I did think of while replying),
yet with one questionable aspect: Patch reviewing doesn't
generally mean patch application. Afaic, it's very rare that I
find it necessary to apply a patch in order to review it. Hence
I'm not convinced this is an extra manual step for everyone
looking at the patch.

>  You don't need to justify to me
> why you don't want to use git for development.  However, it's not fair
> to externalize the cost of your development preferences to everybody else.
> 
> When it's just one or two patches I'm willing to make some
> accommodation, but a series like this it's too much.

As said - I'll try to remember next time.

>> , and while I continue to attach
>> files I would have hoped that the mail bodies nowadays come
>> through uncorrupted (and hence I'd expect file names to be
>> chosen by your mail client based on subject, which includes
>> numbering - that's at least how mine behaves - and there being
>> no actual need to use the attachments).
> 
> I don't have a simple way of saving the text of the mail message without
> saving the attachment in-line in the file; so without writing a special
> tool just for your messages, I can't easily apply the patches into the
> git tree (so I can see the change in-situ).

Oh, okay. Mail programs seem to be even more different than so
far I did think they would be.

> It looks like the mail bodies might actually be uncorrupted now; so it's
> possible that if you send the mail without attachments anymore I might
> be able to use the same workflow for you as I use for everyone else
> (which presumably would extend to other people who might want to review
> your patches).  If you send a test mail (or perhaps a series) I can give
> it a try.

Perhaps best if I try to remember to do this next time round when
Cc-ing you on (not to small) a patch (I don't think it really needs to
be a series for that purpose). Not having to attach patches would,
in the end, remove one manual step for me.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 01/11] public: adjust documentation following XSA-217

[George Dunlap]

On 22/06/17 07:59, Jan Beulich wrote:
>>> , and while I continue to attach
>>> files I would have hoped that the mail bodies nowadays come
>>> through uncorrupted (and hence I'd expect file names to be
>>> chosen by your mail client based on subject, which includes
>>> numbering - that's at least how mine behaves - and there being
>>> no actual need to use the attachments).
>>
>> I don't have a simple way of saving the text of the mail message without
>> saving the attachment in-line in the file; so without writing a special
>> tool just for your messages, I can't easily apply the patches into the
>> git tree (so I can see the change in-situ).
> 
> Oh, okay. Mail programs seem to be even more different than so
> far I did think they would be.

Yes, I actually was surprised that I don't seem to be able to save the
messages without the attachments in my mailer.

>> It looks like the mail bodies might actually be uncorrupted now; so it's
>> possible that if you send the mail without attachments anymore I might
>> be able to use the same workflow for you as I use for everyone else
>> (which presumably would extend to other people who might want to review
>> your patches).  If you send a test mail (or perhaps a series) I can give
>> it a try.
> 
> Perhaps best if I try to remember to do this next time round when
> Cc-ing you on (not to small) a patch (I don't think it really needs to
> be a series for that purpose). Not having to attach patches would,
> in the end, remove one manual step for me.

Thanks.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 02/11] gnttab: remove redundant xenheap check from gnttab_transfer()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

The message isn't very useful, and the check is being done by
steal_page() anyway.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1843,15 +1843,6 @@ gnttab_transfer(
         }
 
         page = mfn_to_page(mfn);
-        if ( unlikely(is_xen_heap_page(page)) )
-        { 
-            put_gfn(d, gop.mfn);
-            gdprintk(XENLOG_INFO, "gnttab_transfer: xen frame %lx\n",
-                    (unsigned long)gop.mfn);
-            gop.status = GNTST_bad_page;
-            goto copyback;
-        }
-
         if ( steal_page(d, page, 0) < 0 )
         {
             put_gfn(d, gop.mfn);




[-- Attachment #2: gnttab-xfer-xenheap.patch --]
[-- Type: text/plain, Size: 752 bytes --]

gnttab: remove redundant xenheap check from gnttab_transfer()

The message isn't very useful, and the check is being done by
steal_page() anyway.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1843,15 +1843,6 @@ gnttab_transfer(
         }
 
         page = mfn_to_page(mfn);
-        if ( unlikely(is_xen_heap_page(page)) )
-        { 
-            put_gfn(d, gop.mfn);
-            gdprintk(XENLOG_INFO, "gnttab_transfer: xen frame %lx\n",
-                    (unsigned long)gop.mfn);
-            gop.status = GNTST_bad_page;
-            goto copyback;
-        }
-
         if ( steal_page(d, page, 0) < 0 )
         {
             put_gfn(d, gop.mfn);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 02/11] gnttab: remove redundant xenheap check from gnttab_transfer()

[Andrew Cooper]

On 21/06/17 10:31, Jan Beulich wrote:
> The message isn't very useful, and the check is being done by
> steal_page() anyway.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 03/11] make steal_page() return a proper error value

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4293 bytes --]

... and use it where suitable (the tmem caller doesn't propagate an
error code). While it doesn't matter as much, also make donate_page()
follow suit on x86 (on ARM it already returns -ENOSYS).

Also move their declarations to common code and add __must_check.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1090,7 +1090,7 @@ int donate_page(struct domain *d, struct
 int steal_page(
     struct domain *d, struct page_info *page, unsigned int memflags)
 {
-    return -1;
+    return -EOPNOTSUPP;
 }
 
 int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4424,7 +4424,7 @@ int donate_page(
              page_to_mfn(page), d->domain_id,
              owner ? owner->domain_id : DOMID_INVALID,
              page->count_info, page->u.inuse.type_info);
-    return -1;
+    return -EINVAL;
 }
 
 int steal_page(
@@ -4435,7 +4435,7 @@ int steal_page(
     const struct domain *owner = dom_xen;
 
     if ( paging_mode_external(d) )
-        return -1;
+        return -EOPNOTSUPP;
 
     spin_lock(&d->page_alloc_lock);
 
@@ -4490,7 +4490,7 @@ int steal_page(
              page_to_mfn(page), d->domain_id,
              owner ? owner->domain_id : DOMID_INVALID,
              page->count_info, page->u.inuse.type_info);
-    return -1;
+    return -EINVAL;
 }
 
 static int __do_update_va_mapping(
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1843,10 +1843,10 @@ gnttab_transfer(
         }
 
         page = mfn_to_page(mfn);
-        if ( steal_page(d, page, 0) < 0 )
+        if ( (rc = steal_page(d, page, 0)) < 0 )
         {
             put_gfn(d, gop.mfn);
-            gop.status = GNTST_bad_page;
+            gop.status = rc == -EINVAL ? GNTST_bad_page : GNTST_general_error;
             goto copyback;
         }
 
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -566,10 +566,10 @@ static long memory_exchange(XEN_GUEST_HA
 
                 page = mfn_to_page(mfn);
 
-                if ( unlikely(steal_page(d, page, MEMF_no_refcount)) )
+                rc = steal_page(d, page, MEMF_no_refcount);
+                if ( unlikely(rc) )
                 {
                     put_gfn(d, gmfn + k);
-                    rc = -EINVAL;
                     goto fail;
                 }
 
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -322,11 +322,6 @@ static inline int relinquish_shared_page
 /* Arch-specific portion of memory_op hypercall. */
 long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
 
-int steal_page(
-    struct domain *d, struct page_info *page, unsigned int memflags);
-int donate_page(
-    struct domain *d, struct page_info *page, unsigned int memflags);
-
 #define domain_set_alloc_bitsize(d) ((void)0)
 #define domain_clamp_alloc_bitsize(d, b) (b)
 
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -550,11 +550,6 @@ long subarch_memory_op(unsigned long cmd
 int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void));
 int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void));
 
-int steal_page(
-    struct domain *d, struct page_info *page, unsigned int memflags);
-int donate_page(
-    struct domain *d, struct page_info *page, unsigned int memflags);
-
 int map_ldt_shadow_page(unsigned int);
 
 #define NIL(type) ((type *)-sizeof(type))
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -567,8 +567,12 @@ int xenmem_add_to_physmap_one(struct dom
                               union xen_add_to_physmap_batch_extra extra,
                               unsigned long idx, gfn_t gfn);
 
-/* Returns 0 on success, or negative on error. */
+/* Return 0 on success, or negative on error. */
 int __must_check guest_remove_page(struct domain *d, unsigned long gmfn);
+int __must_check steal_page(struct domain *d, struct page_info *page,
+                            unsigned int memflags);
+int __must_check donate_page(struct domain *d, struct page_info *page,
+                             unsigned int memflags);
 
 #define RAM_TYPE_CONVENTIONAL 0x00000001
 #define RAM_TYPE_RESERVED     0x00000002



[-- Attachment #2: steal_page-retval.patch --]
[-- Type: text/plain, Size: 4338 bytes --]

make steal_page() return a proper error value

... and use it where suitable (the tmem caller doesn't propagate an
error code). While it doesn't matter as much, also make donate_page()
follow suit on x86 (on ARM it already returns -ENOSYS).

Also move their declarations to common code and add __must_check.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1090,7 +1090,7 @@ int donate_page(struct domain *d, struct
 int steal_page(
     struct domain *d, struct page_info *page, unsigned int memflags)
 {
-    return -1;
+    return -EOPNOTSUPP;
 }
 
 int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4424,7 +4424,7 @@ int donate_page(
              page_to_mfn(page), d->domain_id,
              owner ? owner->domain_id : DOMID_INVALID,
              page->count_info, page->u.inuse.type_info);
-    return -1;
+    return -EINVAL;
 }
 
 int steal_page(
@@ -4435,7 +4435,7 @@ int steal_page(
     const struct domain *owner = dom_xen;
 
     if ( paging_mode_external(d) )
-        return -1;
+        return -EOPNOTSUPP;
 
     spin_lock(&d->page_alloc_lock);
 
@@ -4490,7 +4490,7 @@ int steal_page(
              page_to_mfn(page), d->domain_id,
              owner ? owner->domain_id : DOMID_INVALID,
              page->count_info, page->u.inuse.type_info);
-    return -1;
+    return -EINVAL;
 }
 
 static int __do_update_va_mapping(
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1843,10 +1843,10 @@ gnttab_transfer(
         }
 
         page = mfn_to_page(mfn);
-        if ( steal_page(d, page, 0) < 0 )
+        if ( (rc = steal_page(d, page, 0)) < 0 )
         {
             put_gfn(d, gop.mfn);
-            gop.status = GNTST_bad_page;
+            gop.status = rc == -EINVAL ? GNTST_bad_page : GNTST_general_error;
             goto copyback;
         }
 
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -566,10 +566,10 @@ static long memory_exchange(XEN_GUEST_HA
 
                 page = mfn_to_page(mfn);
 
-                if ( unlikely(steal_page(d, page, MEMF_no_refcount)) )
+                rc = steal_page(d, page, MEMF_no_refcount);
+                if ( unlikely(rc) )
                 {
                     put_gfn(d, gmfn + k);
-                    rc = -EINVAL;
                     goto fail;
                 }
 
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -322,11 +322,6 @@ static inline int relinquish_shared_page
 /* Arch-specific portion of memory_op hypercall. */
 long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
 
-int steal_page(
-    struct domain *d, struct page_info *page, unsigned int memflags);
-int donate_page(
-    struct domain *d, struct page_info *page, unsigned int memflags);
-
 #define domain_set_alloc_bitsize(d) ((void)0)
 #define domain_clamp_alloc_bitsize(d, b) (b)
 
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -550,11 +550,6 @@ long subarch_memory_op(unsigned long cmd
 int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void));
 int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void));
 
-int steal_page(
-    struct domain *d, struct page_info *page, unsigned int memflags);
-int donate_page(
-    struct domain *d, struct page_info *page, unsigned int memflags);
-
 int map_ldt_shadow_page(unsigned int);
 
 #define NIL(type) ((type *)-sizeof(type))
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -567,8 +567,12 @@ int xenmem_add_to_physmap_one(struct dom
                               union xen_add_to_physmap_batch_extra extra,
                               unsigned long idx, gfn_t gfn);
 
-/* Returns 0 on success, or negative on error. */
+/* Return 0 on success, or negative on error. */
 int __must_check guest_remove_page(struct domain *d, unsigned long gmfn);
+int __must_check steal_page(struct domain *d, struct page_info *page,
+                            unsigned int memflags);
+int __must_check donate_page(struct domain *d, struct page_info *page,
+                             unsigned int memflags);
 
 #define RAM_TYPE_CONVENTIONAL 0x00000001
 #define RAM_TYPE_RESERVED     0x00000002

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 03/11] make steal_page() return a proper error value

[Andrew Cooper]

On 21/06/17 10:32, Jan Beulich wrote:
> ... and use it where suitable (the tmem caller doesn't propagate an
> error code). While it doesn't matter as much, also make donate_page()
> follow suit on x86 (on ARM it already returns -ENOSYS).
>
> Also move their declarations to common code and add __must_check.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 03/11] make steal_page() return a proper error value

[Julien Grall]

Hi,

On 21/06/17 10:32, Jan Beulich wrote:
> ... and use it where suitable (the tmem caller doesn't propagate an
> error code). While it doesn't matter as much, also make donate_page()
> follow suit on x86 (on ARM it already returns -ENOSYS).
>
> Also move their declarations to common code and add __must_check.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For the ARM bits:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 04/11] domctl: restrict DOMCTL_set_target to HVM domains

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

Both the XSA-217 fix and
lists.xenproject.org/archives/html/xen-devel/2017-04/msg02945.html
make this assumption, so let's enforce it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1071,7 +1071,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
             break;
         }
 
-        ret = xsm_set_target(XSM_HOOK, d, e);
+        ret = -EOPNOTSUPP;
+        if ( is_hvm_domain(e) )
+            ret = xsm_set_target(XSM_HOOK, d, e);
         if ( ret ) {
             put_domain(e);
             break;




[-- Attachment #2: domctl-set-target-HVM.patch --]
[-- Type: text/plain, Size: 633 bytes --]

domctl: restrict DOMCTL_set_target to HVM domains

Both the XSA-217 fix and
lists.xenproject.org/archives/html/xen-devel/2017-04/msg02945.html
make this assumption, so let's enforce it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1071,7 +1071,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
             break;
         }
 
-        ret = xsm_set_target(XSM_HOOK, d, e);
+        ret = -EOPNOTSUPP;
+        if ( is_hvm_domain(e) )
+            ret = xsm_set_target(XSM_HOOK, d, e);
         if ( ret ) {
             put_domain(e);
             break;

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 04/11] domctl: restrict DOMCTL_set_target to HVM domains

[Andrew Cooper]

On 21/06/17 10:33, Jan Beulich wrote:
> Both the XSA-217 fix and
> lists.xenproject.org/archives/html/xen-devel/2017-04/msg02945.html
> make this assumption, so let's enforce it.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper@citrix.com>, although...

>
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -1071,7 +1071,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>              break;
>          }
>  
> -        ret = xsm_set_target(XSM_HOOK, d, e);
> +        ret = -EOPNOTSUPP;
> +        if ( is_hvm_domain(e) )
> +            ret = xsm_set_target(XSM_HOOK, d, e);
>          if ( ret ) {

... do you mind fixing this style while you are here?

~Andrew

>              put_domain(e);
>              break;
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 05/11] evtchn: convert evtchn_port_is_*() to plain bool

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 5203 bytes --]

... at once reducing overall source size by combining some statements
and constifying a few pointers.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/event_2l.c
+++ b/xen/common/event_2l.c
@@ -61,7 +61,7 @@ static void evtchn_2l_unmask(struct doma
     }
 }
 
-static bool_t evtchn_2l_is_pending(struct domain *d, evtchn_port_t port)
+static bool evtchn_2l_is_pending(const struct domain *d, evtchn_port_t port)
 {
     unsigned int max_ports = BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d);
 
@@ -69,7 +69,7 @@ static bool_t evtchn_2l_is_pending(struc
     return port < max_ports && test_bit(port, &shared_info(d, evtchn_pending));
 }
 
-static bool_t evtchn_2l_is_masked(struct domain *d, evtchn_port_t port)
+static bool evtchn_2l_is_masked(const struct domain *d, evtchn_port_t port)
 {
     unsigned int max_ports = BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d);
 
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -19,7 +19,7 @@
 
 #include <public/event_channel.h>
 
-static inline event_word_t *evtchn_fifo_word_from_port(struct domain *d,
+static inline event_word_t *evtchn_fifo_word_from_port(const struct domain *d,
                                                        unsigned int port)
 {
     unsigned int p, w;
@@ -293,37 +293,25 @@ static void evtchn_fifo_unmask(struct do
         evtchn_fifo_set_pending(v, evtchn);
 }
 
-static bool_t evtchn_fifo_is_pending(struct domain *d, evtchn_port_t port)
+static bool evtchn_fifo_is_pending(const struct domain *d, evtchn_port_t port)
 {
-    event_word_t *word;
-
-    word = evtchn_fifo_word_from_port(d, port);
-    if ( unlikely(!word) )
-        return 0;
+    const event_word_t *word = evtchn_fifo_word_from_port(d, port);
 
-    return test_bit(EVTCHN_FIFO_PENDING, word);
+    return word && test_bit(EVTCHN_FIFO_PENDING, word);
 }
 
-static bool_t evtchn_fifo_is_masked(struct domain *d, evtchn_port_t port)
+static bool_t evtchn_fifo_is_masked(const struct domain *d, evtchn_port_t port)
 {
-    event_word_t *word;
+    const event_word_t *word = evtchn_fifo_word_from_port(d, port);
 
-    word = evtchn_fifo_word_from_port(d, port);
-    if ( unlikely(!word) )
-        return 1;
-
-    return test_bit(EVTCHN_FIFO_MASKED, word);
+    return !word || test_bit(EVTCHN_FIFO_MASKED, word);
 }
 
-static bool_t evtchn_fifo_is_busy(struct domain *d, evtchn_port_t port)
+static bool_t evtchn_fifo_is_busy(const struct domain *d, evtchn_port_t port)
 {
-    event_word_t *word;
-
-    word = evtchn_fifo_word_from_port(d, port);
-    if ( unlikely(!word) )
-        return 0;
+    const event_word_t *word = evtchn_fifo_word_from_port(d, port);
 
-    return test_bit(EVTCHN_FIFO_LINKED, word);
+    return word && test_bit(EVTCHN_FIFO_LINKED, word);
 }
 
 static int evtchn_fifo_set_priority(struct domain *d, struct evtchn *evtchn,
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -137,13 +137,13 @@ struct evtchn_port_ops {
     void (*set_pending)(struct vcpu *v, struct evtchn *evtchn);
     void (*clear_pending)(struct domain *d, struct evtchn *evtchn);
     void (*unmask)(struct domain *d, struct evtchn *evtchn);
-    bool_t (*is_pending)(struct domain *d, evtchn_port_t port);
-    bool_t (*is_masked)(struct domain *d, evtchn_port_t port);
+    bool (*is_pending)(const struct domain *d, evtchn_port_t port);
+    bool (*is_masked)(const struct domain *d, evtchn_port_t port);
     /*
      * Is the port unavailable because it's still being cleaned up
      * after being closed?
      */
-    bool_t (*is_busy)(struct domain *d, evtchn_port_t port);
+    bool (*is_busy)(const struct domain *d, evtchn_port_t port);
     int (*set_priority)(struct domain *d, struct evtchn *evtchn,
                         unsigned int priority);
     void (*print_state)(struct domain *d, const struct evtchn *evtchn);
@@ -174,23 +174,23 @@ static inline void evtchn_port_unmask(st
     d->evtchn_port_ops->unmask(d, evtchn);
 }
 
-static inline bool_t evtchn_port_is_pending(struct domain *d,
-                                            evtchn_port_t port)
+static inline bool evtchn_port_is_pending(const struct domain *d,
+                                          evtchn_port_t port)
 {
     return d->evtchn_port_ops->is_pending(d, port);
 }
 
-static inline bool_t evtchn_port_is_masked(struct domain *d,
-                                           evtchn_port_t port)
+static inline bool evtchn_port_is_masked(const struct domain *d,
+                                         evtchn_port_t port)
 {
     return d->evtchn_port_ops->is_masked(d, port);
 }
 
-static inline bool_t evtchn_port_is_busy(struct domain *d, evtchn_port_t port)
+static inline bool evtchn_port_is_busy(const struct domain *d,
+                                       evtchn_port_t port)
 {
-    if ( d->evtchn_port_ops->is_busy )
-        return d->evtchn_port_ops->is_busy(d, port);
-    return 0;
+    return d->evtchn_port_ops->is_busy &&
+           d->evtchn_port_ops->is_busy(d, port);
 }
 
 static inline int evtchn_port_set_priority(struct domain *d,



[-- Attachment #2: evtchn-port_is-bool.patch --]
[-- Type: text/plain, Size: 5251 bytes --]

evtchn: convert evtchn_port_is_*() to plain bool

... at once reducing overall source size by combining some statements
and constifying a few pointers.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/event_2l.c
+++ b/xen/common/event_2l.c
@@ -61,7 +61,7 @@ static void evtchn_2l_unmask(struct doma
     }
 }
 
-static bool_t evtchn_2l_is_pending(struct domain *d, evtchn_port_t port)
+static bool evtchn_2l_is_pending(const struct domain *d, evtchn_port_t port)
 {
     unsigned int max_ports = BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d);
 
@@ -69,7 +69,7 @@ static bool_t evtchn_2l_is_pending(struc
     return port < max_ports && test_bit(port, &shared_info(d, evtchn_pending));
 }
 
-static bool_t evtchn_2l_is_masked(struct domain *d, evtchn_port_t port)
+static bool evtchn_2l_is_masked(const struct domain *d, evtchn_port_t port)
 {
     unsigned int max_ports = BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d);
 
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -19,7 +19,7 @@
 
 #include <public/event_channel.h>
 
-static inline event_word_t *evtchn_fifo_word_from_port(struct domain *d,
+static inline event_word_t *evtchn_fifo_word_from_port(const struct domain *d,
                                                        unsigned int port)
 {
     unsigned int p, w;
@@ -293,37 +293,25 @@ static void evtchn_fifo_unmask(struct do
         evtchn_fifo_set_pending(v, evtchn);
 }
 
-static bool_t evtchn_fifo_is_pending(struct domain *d, evtchn_port_t port)
+static bool evtchn_fifo_is_pending(const struct domain *d, evtchn_port_t port)
 {
-    event_word_t *word;
-
-    word = evtchn_fifo_word_from_port(d, port);
-    if ( unlikely(!word) )
-        return 0;
+    const event_word_t *word = evtchn_fifo_word_from_port(d, port);
 
-    return test_bit(EVTCHN_FIFO_PENDING, word);
+    return word && test_bit(EVTCHN_FIFO_PENDING, word);
 }
 
-static bool_t evtchn_fifo_is_masked(struct domain *d, evtchn_port_t port)
+static bool_t evtchn_fifo_is_masked(const struct domain *d, evtchn_port_t port)
 {
-    event_word_t *word;
+    const event_word_t *word = evtchn_fifo_word_from_port(d, port);
 
-    word = evtchn_fifo_word_from_port(d, port);
-    if ( unlikely(!word) )
-        return 1;
-
-    return test_bit(EVTCHN_FIFO_MASKED, word);
+    return !word || test_bit(EVTCHN_FIFO_MASKED, word);
 }
 
-static bool_t evtchn_fifo_is_busy(struct domain *d, evtchn_port_t port)
+static bool_t evtchn_fifo_is_busy(const struct domain *d, evtchn_port_t port)
 {
-    event_word_t *word;
-
-    word = evtchn_fifo_word_from_port(d, port);
-    if ( unlikely(!word) )
-        return 0;
+    const event_word_t *word = evtchn_fifo_word_from_port(d, port);
 
-    return test_bit(EVTCHN_FIFO_LINKED, word);
+    return word && test_bit(EVTCHN_FIFO_LINKED, word);
 }
 
 static int evtchn_fifo_set_priority(struct domain *d, struct evtchn *evtchn,
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -137,13 +137,13 @@ struct evtchn_port_ops {
     void (*set_pending)(struct vcpu *v, struct evtchn *evtchn);
     void (*clear_pending)(struct domain *d, struct evtchn *evtchn);
     void (*unmask)(struct domain *d, struct evtchn *evtchn);
-    bool_t (*is_pending)(struct domain *d, evtchn_port_t port);
-    bool_t (*is_masked)(struct domain *d, evtchn_port_t port);
+    bool (*is_pending)(const struct domain *d, evtchn_port_t port);
+    bool (*is_masked)(const struct domain *d, evtchn_port_t port);
     /*
      * Is the port unavailable because it's still being cleaned up
      * after being closed?
      */
-    bool_t (*is_busy)(struct domain *d, evtchn_port_t port);
+    bool (*is_busy)(const struct domain *d, evtchn_port_t port);
     int (*set_priority)(struct domain *d, struct evtchn *evtchn,
                         unsigned int priority);
     void (*print_state)(struct domain *d, const struct evtchn *evtchn);
@@ -174,23 +174,23 @@ static inline void evtchn_port_unmask(st
     d->evtchn_port_ops->unmask(d, evtchn);
 }
 
-static inline bool_t evtchn_port_is_pending(struct domain *d,
-                                            evtchn_port_t port)
+static inline bool evtchn_port_is_pending(const struct domain *d,
+                                          evtchn_port_t port)
 {
     return d->evtchn_port_ops->is_pending(d, port);
 }
 
-static inline bool_t evtchn_port_is_masked(struct domain *d,
-                                           evtchn_port_t port)
+static inline bool evtchn_port_is_masked(const struct domain *d,
+                                         evtchn_port_t port)
 {
     return d->evtchn_port_ops->is_masked(d, port);
 }
 
-static inline bool_t evtchn_port_is_busy(struct domain *d, evtchn_port_t port)
+static inline bool evtchn_port_is_busy(const struct domain *d,
+                                       evtchn_port_t port)
 {
-    if ( d->evtchn_port_ops->is_busy )
-        return d->evtchn_port_ops->is_busy(d, port);
-    return 0;
+    return d->evtchn_port_ops->is_busy &&
+           d->evtchn_port_ops->is_busy(d, port);
 }
 
 static inline int evtchn_port_set_priority(struct domain *d,

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 05/11] evtchn: convert evtchn_port_is_*() to plain bool

[Andrew Cooper]

On 21/06/17 10:34, Jan Beulich wrote:
> ... at once reducing overall source size by combining some statements
> and constifying a few pointers.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Ah - I was planning to do precisely this.  I'm glad I hadn't started yet.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 06/11] ARM: simplify page type handling

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

There's no need to have anything here on ARM other than the distinction
between writable and non-writable pages (and even that could likely be
eliminated, but with a more intrusive change). Limit type to a single
bit and drop pinned and validated flags altogether.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note: Compile tested only.

--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1113,8 +1113,7 @@ void share_xen_page_with_guest(struct pa
     spin_lock(&d->page_alloc_lock);
 
     /* The incremented type count pins as writable or read-only. */
-    page->u.inuse.type_info  = (readonly ? PGT_none : PGT_writable_page);
-    page->u.inuse.type_info |= PGT_validated | 1;
+    page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page) | 1;
 
     page_set_owner(page, d);
     smp_wmb(); /* install valid domain ptr before updating refcnt. */
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -354,8 +354,10 @@ int guest_remove_page(struct domain *d,
 
     rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
 
+#ifdef _PGT_pinned
     if ( !rc && test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
         put_page_and_type(page);
+#endif
 
     /*
      * With the lack of an IOMMU on some platforms, domains with DMA-capable
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -77,20 +77,12 @@ struct page_info
 #define PG_shift(idx)   (BITS_PER_LONG - (idx))
 #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
 
-#define PGT_none          PG_mask(0, 4)  /* no special uses of this page   */
-#define PGT_writable_page PG_mask(7, 4)  /* has writable mappings?         */
-#define PGT_type_mask     PG_mask(15, 4) /* Bits 28-31 or 60-63.           */
-
- /* Owning guest has pinned this page to its current type? */
-#define _PGT_pinned       PG_shift(5)
-#define PGT_pinned        PG_mask(1, 5)
-
- /* Has this page been validated for use as its current type? */
-#define _PGT_validated    PG_shift(6)
-#define PGT_validated     PG_mask(1, 6)
+#define PGT_none          PG_mask(0, 1)  /* no special uses of this page   */
+#define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
+#define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
 
  /* Count of uses of this frame as its current type. */
-#define PGT_count_width   PG_shift(9)
+#define PGT_count_width   PG_shift(2)
 #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
 
  /* Cleared when the owning guest 'frees' this page. */




[-- Attachment #2: ARM-page-types.patch --]
[-- Type: text/plain, Size: 2591 bytes --]

ARM: simplify page type handling

There's no need to have anything here on ARM other than the distinction
between writable and non-writable pages (and even that could likely be
eliminated, but with a more intrusive change). Limit type to a single
bit and drop pinned and validated flags altogether.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note: Compile tested only.

--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1113,8 +1113,7 @@ void share_xen_page_with_guest(struct pa
     spin_lock(&d->page_alloc_lock);
 
     /* The incremented type count pins as writable or read-only. */
-    page->u.inuse.type_info  = (readonly ? PGT_none : PGT_writable_page);
-    page->u.inuse.type_info |= PGT_validated | 1;
+    page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page) | 1;
 
     page_set_owner(page, d);
     smp_wmb(); /* install valid domain ptr before updating refcnt. */
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -354,8 +354,10 @@ int guest_remove_page(struct domain *d,
 
     rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
 
+#ifdef _PGT_pinned
     if ( !rc && test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
         put_page_and_type(page);
+#endif
 
     /*
      * With the lack of an IOMMU on some platforms, domains with DMA-capable
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -77,20 +77,12 @@ struct page_info
 #define PG_shift(idx)   (BITS_PER_LONG - (idx))
 #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
 
-#define PGT_none          PG_mask(0, 4)  /* no special uses of this page   */
-#define PGT_writable_page PG_mask(7, 4)  /* has writable mappings?         */
-#define PGT_type_mask     PG_mask(15, 4) /* Bits 28-31 or 60-63.           */
-
- /* Owning guest has pinned this page to its current type? */
-#define _PGT_pinned       PG_shift(5)
-#define PGT_pinned        PG_mask(1, 5)
-
- /* Has this page been validated for use as its current type? */
-#define _PGT_validated    PG_shift(6)
-#define PGT_validated     PG_mask(1, 6)
+#define PGT_none          PG_mask(0, 1)  /* no special uses of this page   */
+#define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
+#define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
 
  /* Count of uses of this frame as its current type. */
-#define PGT_count_width   PG_shift(9)
+#define PGT_count_width   PG_shift(2)
 #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
 
  /* Cleared when the owning guest 'frees' this page. */

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 06/11] ARM: simplify page type handling

[Stefano Stabellini]

On Wed, 21 Jun 2017, Jan Beulich wrote:
> There's no need to have anything here on ARM other than the distinction
> between writable and non-writable pages (and even that could likely be
> eliminated, but with a more intrusive change). Limit type to a single
> bit and drop pinned and validated flags altogether.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Note: Compile tested only.
> 
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1113,8 +1113,7 @@ void share_xen_page_with_guest(struct pa
>      spin_lock(&d->page_alloc_lock);
>  
>      /* The incremented type count pins as writable or read-only. */
> -    page->u.inuse.type_info  = (readonly ? PGT_none : PGT_writable_page);
> -    page->u.inuse.type_info |= PGT_validated | 1;
> +    page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page) | 1;
>  
>      page_set_owner(page, d);
>      smp_wmb(); /* install valid domain ptr before updating refcnt. */
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -354,8 +354,10 @@ int guest_remove_page(struct domain *d,
>  
>      rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>  
> +#ifdef _PGT_pinned
>      if ( !rc && test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
>          put_page_and_type(page);
> +#endif
>  
>      /*
>       * With the lack of an IOMMU on some platforms, domains with DMA-capable
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -77,20 +77,12 @@ struct page_info
>  #define PG_shift(idx)   (BITS_PER_LONG - (idx))
>  #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
>  
> -#define PGT_none          PG_mask(0, 4)  /* no special uses of this page   */
> -#define PGT_writable_page PG_mask(7, 4)  /* has writable mappings?         */
> -#define PGT_type_mask     PG_mask(15, 4) /* Bits 28-31 or 60-63.           */
> -
> - /* Owning guest has pinned this page to its current type? */
> -#define _PGT_pinned       PG_shift(5)
> -#define PGT_pinned        PG_mask(1, 5)
> -
> - /* Has this page been validated for use as its current type? */
> -#define _PGT_validated    PG_shift(6)
> -#define PGT_validated     PG_mask(1, 6)
> +#define PGT_none          PG_mask(0, 1)  /* no special uses of this page   */
> +#define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
> +#define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
>  
>   /* Count of uses of this frame as its current type. */
> -#define PGT_count_width   PG_shift(9)
> +#define PGT_count_width   PG_shift(2)
>  #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
>  
>   /* Cleared when the owning guest 'frees' this page. */
> 
> 
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 07/11] x86: fold identical error paths in xenmem_add_to_physmap_one()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4899,11 +4899,8 @@ int xenmem_add_to_physmap_one(
 
     if ( !paging_mode_translate(d) || (mfn == 0) )
     {
-        if ( page )
-            put_page(page);
-        if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
-            put_gfn(d, gfn);
-        return -EINVAL;
+        rc = -EINVAL;
+        goto put_both;
     }
 
     /* Remove previously mapped page if it was present. */




[-- Attachment #2: x86-xatp1-error-paths.patch --]
[-- Type: text/plain, Size: 593 bytes --]

x86: fold identical error paths in xenmem_add_to_physmap_one()

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4899,11 +4899,8 @@ int xenmem_add_to_physmap_one(
 
     if ( !paging_mode_translate(d) || (mfn == 0) )
     {
-        if ( page )
-            put_page(page);
-        if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
-            put_gfn(d, gfn);
-        return -EINVAL;
+        rc = -EINVAL;
+        goto put_both;
     }
 
     /* Remove previously mapped page if it was present. */

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 07/11] x86: fold identical error paths in xenmem_add_to_physmap_one()

[Andrew Cooper]

On 21/06/17 10:36, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I think this function is another one which could benefit from explicitly
counting the number of references it collects, as per XSA-224.

>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4899,11 +4899,8 @@ int xenmem_add_to_physmap_one(
>  
>      if ( !paging_mode_translate(d) || (mfn == 0) )
>      {
> -        if ( page )
> -            put_page(page);
> -        if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
> -            put_gfn(d, gfn);
> -        return -EINVAL;
> +        rc = -EINVAL;
> +        goto put_both;
>      }
>  
>      /* Remove previously mapped page if it was present. */
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 08/11] gnttab: remove host map in the event of a grant_map failure

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

From: George Dunlap <george.dunlap@citrix.com>

The current code appropriately removes the reference and type counts
on failure, but leaves the mapping set up. As the only path which can
trigger this is failure from IOMMU manipulation, and as unprivileged
domains are being crashed in that case, this is not by itself a
security issue.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -764,6 +764,7 @@ __gnttab_map_grant_ref(
     u32            old_pin;
     u32            act_pin;
     unsigned int   cache_flags, refcnt = 0, typecnt = 0;
+    bool           host_map_created = false;
     struct active_grant_entry *act = NULL;
     struct grant_mapping *mt;
     grant_entry_header_t *shah;
@@ -923,6 +924,8 @@ __gnttab_map_grant_ref(
                                            cache_flags);
             if ( rc != GNTST_okay )
                 goto undo_out;
+
+            host_map_created = true;
         }
     }
     else if ( owner == rd || owner == dom_cow )
@@ -960,6 +963,8 @@ __gnttab_map_grant_ref(
             rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
             if ( rc != GNTST_okay )
                 goto undo_out;
+
+            host_map_created = true;
         }
     }
     else
@@ -1030,6 +1035,12 @@ __gnttab_map_grant_ref(
     return;
 
  undo_out:
+    if ( host_map_created )
+    {
+        replace_grant_host_mapping(op->host_addr, frame, 0, op->flags);
+        gnttab_flush_tlb(ld);
+    }
+
     while ( typecnt-- )
         put_page_type(pg);
 




[-- Attachment #2: gnttab-remove-host-map.patch --]
[-- Type: text/plain, Size: 1794 bytes --]

gnttab: remove host map in the event of a grant_map failure

From: George Dunlap <george.dunlap@citrix.com>

The current code appropriately removes the reference and type counts
on failure, but leaves the mapping set up. As the only path which can
trigger this is failure from IOMMU manipulation, and as unprivileged
domains are being crashed in that case, this is not by itself a
security issue.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -764,6 +764,7 @@ __gnttab_map_grant_ref(
     u32            old_pin;
     u32            act_pin;
     unsigned int   cache_flags, refcnt = 0, typecnt = 0;
+    bool           host_map_created = false;
     struct active_grant_entry *act = NULL;
     struct grant_mapping *mt;
     grant_entry_header_t *shah;
@@ -923,6 +924,8 @@ __gnttab_map_grant_ref(
                                            cache_flags);
             if ( rc != GNTST_okay )
                 goto undo_out;
+
+            host_map_created = true;
         }
     }
     else if ( owner == rd || owner == dom_cow )
@@ -960,6 +963,8 @@ __gnttab_map_grant_ref(
             rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
             if ( rc != GNTST_okay )
                 goto undo_out;
+
+            host_map_created = true;
         }
     }
     else
@@ -1030,6 +1035,12 @@ __gnttab_map_grant_ref(
     return;
 
  undo_out:
+    if ( host_map_created )
+    {
+        replace_grant_host_mapping(op->host_addr, frame, 0, op->flags);
+        gnttab_flush_tlb(ld);
+    }
+
     while ( typecnt-- )
         put_page_type(pg);
 

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]

When no memory is available in the hypervisor, rather than immediately
failing the request try to steal a handle from another vCPU.

Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -397,7 +397,7 @@ get_maptrack_handle(
     struct vcpu          *curr = current;
     unsigned int          i, head;
     grant_handle_t        handle;
-    struct grant_mapping *new_mt;
+    struct grant_mapping *new_mt = NULL;
 
     handle = __get_maptrack_handle(lgt, curr);
     if ( likely(handle != -1) )
@@ -408,8 +408,13 @@ get_maptrack_handle(
     /*
      * If we've run out of frames, try stealing an entry from another
      * VCPU (in case the guest isn't mapping across its VCPUs evenly).
+     * Also use this path in case we're out of memory, to avoid spurious
+     * failures.
      */
-    if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
+    if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
+        new_mt = alloc_xenheap_page();
+
+    if ( !new_mt )
     {
         /*
          * Can drop the lock since no other VCPU can be adding a new
@@ -432,12 +437,6 @@ get_maptrack_handle(
         return steal_maptrack_handle(lgt, curr);
     }
 
-    new_mt = alloc_xenheap_page();
-    if ( !new_mt )
-    {
-        spin_unlock(&lgt->maptrack_lock);
-        return -1;
-    }
     clear_page(new_mt);
 
     /*




[-- Attachment #2: gnttab-spurious-mt-handle-fail.patch --]
[-- Type: text/plain, Size: 1550 bytes --]

gnttab: avoid spurious maptrack handle allocation failures

When no memory is available in the hypervisor, rather than immediately
failing the request try to steal a handle from another vCPU.

Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -397,7 +397,7 @@ get_maptrack_handle(
     struct vcpu          *curr = current;
     unsigned int          i, head;
     grant_handle_t        handle;
-    struct grant_mapping *new_mt;
+    struct grant_mapping *new_mt = NULL;
 
     handle = __get_maptrack_handle(lgt, curr);
     if ( likely(handle != -1) )
@@ -408,8 +408,13 @@ get_maptrack_handle(
     /*
      * If we've run out of frames, try stealing an entry from another
      * VCPU (in case the guest isn't mapping across its VCPUs evenly).
+     * Also use this path in case we're out of memory, to avoid spurious
+     * failures.
      */
-    if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
+    if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
+        new_mt = alloc_xenheap_page();
+
+    if ( !new_mt )
     {
         /*
          * Can drop the lock since no other VCPU can be adding a new
@@ -432,12 +437,6 @@ get_maptrack_handle(
         return steal_maptrack_handle(lgt, curr);
     }
 
-    new_mt = alloc_xenheap_page();
-    if ( !new_mt )
-    {
-        spin_unlock(&lgt->maptrack_lock);
-        return -1;
-    }
     clear_page(new_mt);
 
     /*

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures

[Andrew Cooper]

On 21/06/17 10:37, Jan Beulich wrote:
> When no memory is available in the hypervisor, rather than immediately
> failing the request try to steal a handle from another vCPU.

"request, try"

>
> Reported-by: George Dunlap <george.dunlap@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -397,7 +397,7 @@ get_maptrack_handle(
>      struct vcpu          *curr = current;
>      unsigned int          i, head;
>      grant_handle_t        handle;
> -    struct grant_mapping *new_mt;
> +    struct grant_mapping *new_mt = NULL;
>  
>      handle = __get_maptrack_handle(lgt, curr);
>      if ( likely(handle != -1) )
> @@ -408,8 +408,13 @@ get_maptrack_handle(
>      /*
>       * If we've run out of frames, try stealing an entry from another
>       * VCPU (in case the guest isn't mapping across its VCPUs evenly).
> +     * Also use this path in case we're out of memory, to avoid spurious
> +     * failures.
>       */
> -    if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
> +    if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
> +        new_mt = alloc_xenheap_page();
> +
> +    if ( !new_mt )
>      {
>          /*
>           * Can drop the lock since no other VCPU can be adding a new

* frame once they've run out.
*/

It doesn't look like this comment is true any more, which brings the
locking correctness into question.

~Andrew

> @@ -432,12 +437,6 @@ get_maptrack_handle(
>          return steal_maptrack_handle(lgt, curr);
>      }
>  
> -    new_mt = alloc_xenheap_page();
> -    if ( !new_mt )
> -    {
> -        spin_unlock(&lgt->maptrack_lock);
> -        return -1;
> -    }
>      clear_page(new_mt);
>  
>      /*
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures

[Jan Beulich]

>>> On 21.06.17 at 14:02, <andrew.cooper3@citrix.com> wrote:
> On 21/06/17 10:37, Jan Beulich wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -397,7 +397,7 @@ get_maptrack_handle(
>>      struct vcpu          *curr = current;
>>      unsigned int          i, head;
>>      grant_handle_t        handle;
>> -    struct grant_mapping *new_mt;
>> +    struct grant_mapping *new_mt = NULL;
>>  
>>      handle = __get_maptrack_handle(lgt, curr);
>>      if ( likely(handle != -1) )
>> @@ -408,8 +408,13 @@ get_maptrack_handle(
>>      /*
>>       * If we've run out of frames, try stealing an entry from another
>>       * VCPU (in case the guest isn't mapping across its VCPUs evenly).
>> +     * Also use this path in case we're out of memory, to avoid spurious
>> +     * failures.
>>       */
>> -    if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
>> +    if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
>> +        new_mt = alloc_xenheap_page();
>> +
>> +    if ( !new_mt )
>>      {
>>          /*
>>           * Can drop the lock since no other VCPU can be adding a new
> 
> * frame once they've run out.
> */
> 
> It doesn't look like this comment is true any more, which brings the
> locking correctness into question.

Oh, indeed. I'll need to revive the locking change I had done here
and then dropped because we did realize we didn't need it for
XSA-218.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures

[Jan Beulich]

>>> On 21.06.17 at 14:02, <andrew.cooper3@citrix.com> wrote:
> On 21/06/17 10:37, Jan Beulich wrote:
>> @@ -408,8 +408,13 @@ get_maptrack_handle(
>>      /*
>>       * If we've run out of frames, try stealing an entry from another
>>       * VCPU (in case the guest isn't mapping across its VCPUs evenly).
>> +     * Also use this path in case we're out of memory, to avoid spurious
>> +     * failures.
>>       */
>> -    if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
>> +    if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
>> +        new_mt = alloc_xenheap_page();
>> +
>> +    if ( !new_mt )
>>      {
>>          /*
>>           * Can drop the lock since no other VCPU can be adding a new
> 
> * frame once they've run out.
> */
> 
> It doesn't look like this comment is true any more, which brings the
> locking correctness into question.

The whole function acts on current only, so races aren't possible
at all. The comment therefore is misleading, and I now think the
maptrack_lock is pointless altogether.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 10/11] gnttab: limit mapkind()'s iteration count

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

There's no need for the function to observe increases of the maptrack
table (which can occur as the maptrack lock isn't being held) - actual
population of maptrack entries is excluded while we're here (by way of
holding the respective grant table lock for writing, while code
populating entries acquires it for reading). Latch the limit ahead of
the loop, allowing for the barrier to move out, too.

Signed-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -712,7 +712,7 @@ static unsigned int mapkind(
     struct grant_table *lgt, const struct domain *rd, unsigned long mfn)
 {
     struct grant_mapping *map;
-    grant_handle_t handle;
+    grant_handle_t handle, limit = lgt->maptrack_limit;
     unsigned int kind = 0;
 
     /*
@@ -726,10 +726,10 @@ static unsigned int mapkind(
      */
     ASSERT(percpu_rw_is_write_locked(&rd->grant_table->lock));
 
-    for ( handle = 0; !(kind & MAPKIND_WRITE) &&
-                      handle < lgt->maptrack_limit; handle++ )
+    smp_rmb();
+
+    for ( handle = 0; !(kind & MAPKIND_WRITE) && handle < limit; handle++ )
     {
-        smp_rmb();
         map = &maptrack_entry(lgt, handle);
         if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ||
              map->domid != rd->domain_id )




[-- Attachment #2: gnttab-mapkind-limit.patch --]
[-- Type: text/plain, Size: 1386 bytes --]

gnttab: limit mapkind()'s iteration count

There's no need for the function to observe increases of the maptrack
table (which can occur as the maptrack lock isn't being held) - actual
population of maptrack entries is excluded while we're here (by way of
holding the respective grant table lock for writing, while code
populating entries acquires it for reading). Latch the limit ahead of
the loop, allowing for the barrier to move out, too.

Signed-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -712,7 +712,7 @@ static unsigned int mapkind(
     struct grant_table *lgt, const struct domain *rd, unsigned long mfn)
 {
     struct grant_mapping *map;
-    grant_handle_t handle;
+    grant_handle_t handle, limit = lgt->maptrack_limit;
     unsigned int kind = 0;
 
     /*
@@ -726,10 +726,10 @@ static unsigned int mapkind(
      */
     ASSERT(percpu_rw_is_write_locked(&rd->grant_table->lock));
 
-    for ( handle = 0; !(kind & MAPKIND_WRITE) &&
-                      handle < lgt->maptrack_limit; handle++ )
+    smp_rmb();
+
+    for ( handle = 0; !(kind & MAPKIND_WRITE) && handle < limit; handle++ )
     {
-        smp_rmb();
         map = &maptrack_entry(lgt, handle);
         if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ||
              map->domid != rd->domain_id )

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 10/11] gnttab: limit mapkind()'s iteration count

[Andrew Cooper]

On 21/06/17 10:38, Jan Beulich wrote:
> There's no need for the function to observe increases of the maptrack
> table (which can occur as the maptrack lock isn't being held) - actual
> population of maptrack entries is excluded while we're here (by way of
> holding the respective grant table lock for writing, while code
> populating entries acquires it for reading). Latch the limit ahead of
> the loop, allowing for the barrier to move out, too.
>
> Signed-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 11/11] gnttab: drop useless locking

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

Holding any lock while accessing the maptrack entry fields is
pointless, as these entries are protected by their associated active
entry lock (which is being acquired later, before re-validating the
fields read without holding the lock).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1122,19 +1122,14 @@ __gnttab_unmap_common(
     smp_rmb();
     map = &maptrack_entry(lgt, op->handle);
 
-    grant_read_lock(lgt);
-
     if ( unlikely(!read_atomic(&map->flags)) )
     {
-        grant_read_unlock(lgt);
         gdprintk(XENLOG_INFO, "Zero flags for handle %#x\n", op->handle);
         op->status = GNTST_bad_handle;
         return;
     }
 
     dom = map->domid;
-    grant_read_unlock(lgt);
-
     if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) )
     {
         /* This can happen when a grant is implicitly unmapped. */




[-- Attachment #2: gnttab-useless-locking.patch --]
[-- Type: text/plain, Size: 971 bytes --]

gnttab: drop useless locking

Holding any lock while accessing the maptrack entry fields is
pointless, as these entries are protected by their associated active
entry lock (which is being acquired later, before re-validating the
fields read without holding the lock).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1122,19 +1122,14 @@ __gnttab_unmap_common(
     smp_rmb();
     map = &maptrack_entry(lgt, op->handle);
 
-    grant_read_lock(lgt);
-
     if ( unlikely(!read_atomic(&map->flags)) )
     {
-        grant_read_unlock(lgt);
         gdprintk(XENLOG_INFO, "Zero flags for handle %#x\n", op->handle);
         op->status = GNTST_bad_handle;
         return;
     }
 
     dom = map->domid;
-    grant_read_unlock(lgt);
-
     if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) )
     {
         /* This can happen when a grant is implicitly unmapped. */

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Ping: [PATCH 11/11] gnttab: drop useless locking

[Jan Beulich]

>>> On 21.06.17 at 11:38, <JBeulich@suse.com> wrote:
> Holding any lock while accessing the maptrack entry fields is
> pointless, as these entries are protected by their associated active
> entry lock (which is being acquired later, before re-validating the
> fields read without holding the lock).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1122,19 +1122,14 @@ __gnttab_unmap_common(
>      smp_rmb();
>      map = &maptrack_entry(lgt, op->handle);
>  
> -    grant_read_lock(lgt);
> -
>      if ( unlikely(!read_atomic(&map->flags)) )
>      {
> -        grant_read_unlock(lgt);
>          gdprintk(XENLOG_INFO, "Zero flags for handle %#x\n", op->handle);
>          op->status = GNTST_bad_handle;
>          return;
>      }
>  
>      dom = map->domid;
> -    grant_read_unlock(lgt);
> -
>      if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) )
>      {
>          /* This can happen when a grant is implicitly unmapped. */




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel