diff mbox series

[3/3] gnttab: GTF_sub_page is a v2-only flag

Message ID 2bf46266-785d-0de3-5f61-48c3fd191a5c@suse.com (mailing list archive)
State New
Headers show
Series gnttab: misc fixes | expand

Commit Message

Jan Beulich Feb. 17, 2021, 10:46 a.m. UTC
Prior to its introduction, v1 entries weren't checked for this flag, and
the flag also has been meaningless for v1 entries. Therefore it also
shouldn't be checked. (The only consistent alternative would be to also
check for all currently undefined flags to be clear.)

Fixes: b545941b6638 ("Implement sub-page grant support")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Andrew Cooper Feb. 18, 2021, 2:22 p.m. UTC | #1
On 17/02/2021 10:46, Jan Beulich wrote:
> Prior to its introduction, v1 entries weren't checked for this flag, and
> the flag also has been meaningless for v1 entries. Therefore it also
> shouldn't be checked. (The only consistent alternative would be to also
> check for all currently undefined flags to be clear.)

We recently had a similar discussion for the stable libs.

Whatever we do, an unexpected corner case needs to break.  Checking for
all undefined flags up front is far cleaner - absolutely nothing good
can come for a guest which set GTF_sub_page with v1, and is expecting it
to work, and this way, we do all breaking in one go, rather than
breaking $N times in the future as new flags get added.

~Andrew
Jan Beulich Feb. 18, 2021, 3:02 p.m. UTC | #2
On 18.02.2021 15:22, Andrew Cooper wrote:
> On 17/02/2021 10:46, Jan Beulich wrote:
>> Prior to its introduction, v1 entries weren't checked for this flag, and
>> the flag also has been meaningless for v1 entries. Therefore it also
>> shouldn't be checked. (The only consistent alternative would be to also
>> check for all currently undefined flags to be clear.)
> 
> We recently had a similar discussion for the stable libs.
> 
> Whatever we do, an unexpected corner case needs to break.  Checking for
> all undefined flags up front is far cleaner - absolutely nothing good
> can come for a guest which set GTF_sub_page with v1, and is expecting it
> to work, and this way, we do all breaking in one go, rather than
> breaking $N times in the future as new flags get added.

Except that there doesn't need to be any breaking: v1 could continue
to ignore all originally undefined flags. v1 and v2 could continue to
ignore all flags presently undefined. See also Julien's question
about GTF_sub_page on a transitive grant entry. That's presently an
ignored setting as well (and, as an implementation detail, in fact
getting forced to be set, but with a range covering the entire page).

Retroactively starting to check (and reject) undefined flags isn't
going to be nice; nevertheless I wanted to put this up as an at
least theoretical alternative. Perhaps a topic for the next community
call, if I don't forget by the time an agenda page appears.

Jan
diff mbox series

Patch

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -762,13 +762,11 @@  static int _set_status_v1(const grant_en
                           struct domain *rd,
                           struct active_grant_entry *act,
                           int readonly,
-                          int mapflag,
-                          domid_t  ldomid)
+                          domid_t ldomid)
 {
     int rc = GNTST_okay;
     uint32_t *raw_shah = (uint32_t *)shah;
     union grant_combo scombo;
-    uint16_t mask = GTF_type_mask;
 
     /*
      * We bound the number of times we retry CMPXCHG on memory locations that
@@ -780,11 +778,6 @@  static int _set_status_v1(const grant_en
      */
     int retries = 0;
 
-    /* if this is a grant mapping operation we should ensure GTF_sub_page
-       is not set */
-    if ( mapflag )
-        mask |= GTF_sub_page;
-
     scombo.raw = ACCESS_ONCE(*raw_shah);
 
     /*
@@ -798,8 +791,9 @@  static int _set_status_v1(const grant_en
         union grant_combo prev, new;
 
         /* If not already pinned, check the grant domid and type. */
-        if ( !act->pin && (((scombo.flags & mask) != GTF_permit_access) ||
-                           (scombo.domid != ldomid)) )
+        if ( !act->pin &&
+             (((scombo.flags & GTF_type_mask) != GTF_permit_access) ||
+              (scombo.domid != ldomid)) )
             PIN_FAIL(done, GNTST_general_error,
                      "Bad flags (%x) or dom (%d); expected d%d\n",
                      scombo.flags, scombo.domid, ldomid);
@@ -916,7 +910,7 @@  static int _set_status(const grant_entry
 {
 
     if ( evaluate_nospec(rgt_version == 1) )
-        return _set_status_v1(shah, rd, act, readonly, mapflag, ldomid);
+        return _set_status_v1(shah, rd, act, readonly, ldomid);
     else
         return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid);
 }
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -175,7 +175,7 @@  typedef struct grant_entry_v1 grant_entr
  *                             mappings of the grant [GST]
  *  GTF_sub_page: Grant access to only a subrange of the page.  @domid
  *                will only be allowed to copy from the grant, and not
- *                map it. [GST]
+ *                map it. [GST, v2]
  */
 #define _GTF_readonly       (2)
 #define GTF_readonly        (1U<<_GTF_readonly)