> -----Original Message----- > From: Andrew Cooper > Sent: 09 August 2018 12:14 > To: Paul Durrant <paul.durr...@citrix.com>; Xen-devel <xen- > de...@lists.xen.org> > Cc: Jan Beulich <jbeul...@suse.com>; Stefano Stabellini > <sstabell...@kernel.org>; Julien Grall <julien.gr...@arm.com>; Wei Liu > <wei.l...@citrix.com>; Roger Pau Monne <roger....@citrix.com>; George > Dunlap <george.dun...@citrix.com> > Subject: Re: [PATCH] common/gnttab: Explicitly default to gnttab v1 during > domain creation > > On 09/08/18 11:41, Paul Durrant wrote: > >> -----Original Message----- > >> From: Andrew Cooper [mailto:andrew.coop...@citrix.com] > >> Sent: 09 August 2018 11:32 > >> To: Xen-devel <xen-de...@lists.xen.org> > >> Cc: Andrew Cooper <andrew.coop...@citrix.com>; Jan Beulich > >> <jbeul...@suse.com>; Paul Durrant <paul.durr...@citrix.com>; Stefano > >> Stabellini <sstabell...@kernel.org>; Julien Grall <julien.gr...@arm.com>; > Wei > >> Liu <wei.l...@citrix.com>; Roger Pau Monne <roger....@citrix.com>; > >> George Dunlap <george.dun...@citrix.com> > >> Subject: [PATCH] common/gnttab: Explicitly default to gnttab v1 during > >> domain creation > >> > >> For reasons which appear to be exclusively down to poor review of the > grant > >> table v2 code, a grant table's version field was wasn't initialised during > >> creation. > >> > >> A number of problems (including XSAs) have occurred in the past trying > >> trying > >> to use a grant table which hasn't been properly set up, and various areas > of > >> the code cope with v0 by defaulting to v1. > >> > >> In particular, the toolstack using GNTTABOP_setup_table to be able to fill > in > >> the store/console grants has a side effect of switching to v1. > >> > >> In hindsight however, this "fixup if we see 0" is a very poor, with a > >> substantial degree of risk. Explicitly default to grant table v1 during > >> domain create, and let the rest of the code work safely in the knowledge > >> that > >> the version is sensibly set. > >> > >> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> > >> --- > >> CC: Jan Beulich <jbeul...@suse.com> > >> CC: Paul Durrant <paul.durr...@citrix.com> > >> CC: Stefano Stabellini <sstabell...@kernel.org> > >> CC: Julien Grall <julien.gr...@arm.com> > >> CC: Wei Liu <wei.l...@citrix.com> > >> CC: Roger Pau Monné <roger....@citrix.com> > >> CC: George Dunlap <george.dun...@eu.citrix.com> > >> > >> This probably wants backporting, so I chose not to integrate it into my > series > >> which rearranges large chunks of DOMCTL_createdomain > >> --- > >> xen/common/grant_table.c | 40 +++++++--------------------------------- > >> 1 file changed, 7 insertions(+), 33 deletions(-) > >> > >> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > >> index d9ec711..8bae656 100644 > >> --- a/xen/common/grant_table.c > >> +++ b/xen/common/grant_table.c > >> @@ -49,10 +49,7 @@ struct grant_table { > >> percpu_rwlock_t lock; > >> /* Lock protecting the maptrack limit */ > >> spinlock_t maptrack_lock; > >> - /* > >> - * The defined versions are 1 and 2. Set to 0 if we don't know > >> - * what version to use yet. > >> - */ > > Given that only the guest can set the version by hypercall, it might be > worth comment somewhere saying that grant table always start in v1 format > and then get morphed if the guest sets v2. I guess it might be inferred that a > v1 table is immutable (for those who don't look too hard). > > Really? I don't think that's a reasonable inference to make, given > GNTTABOP_set_version as a public interface. >
If you think it is sufficiently evident then fine, but the comment you're removing implies that 0 -> 1 and 0 -> 2 are valid transitions but maybe 1 -> 2 is not (otherwise why would you have 0, right?) Paul > ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
