Re: [Xen-devel] Re: [PATCH] xen: reduce severity of message about using v1 grant tables.

[Steven Smith <steven.smith@xxxxxxxxxx>]

> Oh, I see what you meant... in the proper resume case (as opposed to the
> cancelled suspend/checkpoint case I was thinking of) there should be no
> grant tables in use at this point so most devices should, in theory, be
> able to reconnect using v1 grants, any drivers which require v2 grant
> tables need to check for them in their resume hook as well as at start
> of day.
> 
> Unfortunately frontend devices tear down their grant entries after the
> resume rather than before the suspend (I presume this has to do with
> faster checkpointing?) which means they could be trying to clear an
> entry of the wrong layout, leading the unbounded badness that the
> comment refers to.
Actually, I think that'd be okay.  Drivers should be clearing grant
table entries through the gnttab_end_* functions, which always check
grant_table_version and do the right thing.
gnttab_grant_foreign_access_ref_{subpage,trans} would BUG(), but that
could be turned into an error return easily enough.  Handling it
everywhere would be a pain, though.

The only other potential subtlety is handling a suspend while some
other vcpu is doing grant table operations, but I think the
stop_machine() is sufficient protection against that.

So I think a downgrade might actually be safe (despite what I said
before).  It can only happen if you migrate from a new Xen to an older
one, which I didn't think we supported anyway, but it could be made to
work.

> I think the choices are basically:
>       * Always latch to either v1 or v2 at start of day, if we can't get
>         the version we want then panic (this is a stronger restriction
>         than the current code which will try to upgrade to v2 on resume)
Yeah, that probably counts as a bug in the current code.  If we boot
on a Xen which only supports v1 tabled then we should probably stick
with v1 until we shut down.

panic()ing when v2 isn't available sounds like overkill, though.

Would something like this work?


Allow downgrades from v2 grant tables to v1.

Completely untested, beyond checking that it actually compiles.

Signed-off-by: Steven Smith <sos22@xxxxxxxxxx>

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index fd619b4..fa971e2 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -210,36 +210,40 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned 
long frame,
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access);
 
-void gnttab_grant_foreign_access_ref_subpage(grant_ref_t ref, domid_t domid,
-                                             unsigned long frame, int flags,
-                                             unsigned page_off,
-                                             unsigned length)
+int gnttab_grant_foreign_access_ref_subpage(grant_ref_t ref, domid_t domid,
+                                           unsigned long frame, int flags,
+                                           unsigned page_off,
+                                           unsigned length)
 {
        BUG_ON(flags & (GTF_accept_transfer | GTF_reading |
                        GTF_writing | GTF_sub_page | GTF_permit_access));
-       BUG_ON(grant_table_version == 1);
+       if (grant_table_version == 1)
+               return -EOPNOTSUPP;
        shared.v2[ref].sub_page.frame = frame;
        shared.v2[ref].sub_page.page_off = page_off;
        shared.v2[ref].sub_page.length = length;
        shared.v2[ref].hdr.domid = domid;
        wmb();
        shared.v2[ref].hdr.flags = GTF_permit_access | GTF_sub_page | flags;
+       return 0;
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_ref_subpage);
 
-void gnttab_grant_foreign_access_ref_trans(grant_ref_t ref, domid_t domid,
-                                          int flags,
-                                          domid_t trans_domid,
-                                          grant_ref_t trans_gref)
+int gnttab_grant_foreign_access_ref_trans(grant_ref_t ref, domid_t domid,
+                                         int flags,
+                                         domid_t trans_domid,
+                                         grant_ref_t trans_gref)
 {
        BUG_ON(flags & (GTF_accept_transfer | GTF_reading |
                        GTF_writing | GTF_sub_page | GTF_permit_access));
-       BUG_ON(grant_table_version == 1);
+       if (grant_table_version == 1)
+               return -EOPNOTSUPP;
        shared.v2[ref].transitive.trans_domid = trans_domid;
        shared.v2[ref].transitive.gref = trans_gref;
        shared.v2[ref].hdr.domid = domid;
        wmb();
        shared.v2[ref].hdr.flags = GTF_permit_access | GTF_transitive | flags;
+       return 0;
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_ref_trans);
 
@@ -588,25 +592,40 @@ static inline unsigned max_nr_grant_status_frames(void)
        return nr_status_frames(max_nr_grant_frames());
 }
 
+/* Called whenever there's some possibility that the hypervisor
+   disagrees with us about what grant table version we're using
+   (i.e. start of day and following a resume). */
 static void gnttab_request_version(void)
 {
        int rc;
        struct gnttab_set_version gsv;
 
-       gsv.version = 2;
-       rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
-       if (rc == 0) {
-               grant_table_version = 2;
-       } else {
-               if (grant_table_version == 2) {
-                       /* If we've already used version 2 features,
-                          but then suddenly discover that they're not
-                          available (e.g. migrating to an older
-                          version of Xen), almost unbounded badness
-                          can happen. */
-                       panic("we need grant tables version 2, but only version 
1 is available");
+       if (grant_table_version == 0 || grant_table_version == 2) {
+               /* Try to get v2 grant tables. */
+               gsv.version = 2;
+               rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
+               if (rc == 0) {
+                       /* Success */
+                       grant_table_version = 2;
+               } else {
+                       if (grant_table_version == 2) {
+                               /* We've seen v2 features before, but
+                                  they're no longer available.
+                                  That's not disastrous, but it is
+                                  pretty bad. */
+                               printk(KERN_WARNING "Downgrading from v2 grant 
tables to v1; expect some device misbehaviour.\n");
+                       }
+                       grant_table_version = 1;
                }
-               grant_table_version = 1;
+       }
+       if (grant_table_version == 1) {
+               /* This shouldn't be necessary (Xen should default to
+                  v1, and we've not asked for anything else), but do
+                  it anyway for sanity. */
+               gsv.version = 1;
+               /* If this fails, it probably just means the
+                  hypervisor only supports v1, so we're fine. */
+               (void)HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
        }
 }
 
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index cf316d7..85f2d52 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -65,14 +65,14 @@ int gnttab_resume(void);
 int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
                                int flags);
 
-void gnttab_grant_foreign_access_ref_subpage(grant_ref_t ref, domid_t domid,
-                                            unsigned long frame, int flags,
-                                            unsigned page_off,
-                                            unsigned length);
-void gnttab_grant_foreign_access_ref_trans(grant_ref_t ref, domid_t domid,
-                                          int flags,
-                                          domid_t trans_domid,
-                                          grant_ref_t trans_gref);
+int gnttab_grant_foreign_access_ref_subpage(grant_ref_t ref, domid_t domid,
+                                           unsigned long frame, int flags,
+                                           unsigned page_off,
+                                           unsigned length);
+int gnttab_grant_foreign_access_ref_trans(grant_ref_t ref, domid_t domid,
+                                         int flags,
+                                         domid_t trans_domid,
+                                         grant_ref_t trans_gref);
 
 /*
  * Are sub-page grants available on this version of Xen?  Returns 1 if

Attachment: signature.asc
Description: Digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel