> -----Original Message----- > From: Andrew Cooper > Sent: 09 August 2018 12:14 > To: Paul Durrant <[email protected]>; Xen-devel <xen- > [email protected]> > Cc: Jan Beulich <[email protected]>; Stefano Stabellini > <[email protected]>; Julien Grall <[email protected]>; Wei Liu > <[email protected]>; Roger Pau Monne <[email protected]>; George > Dunlap <[email protected]> > 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:[email protected]] > >> Sent: 09 August 2018 11:32 > >> To: Xen-devel <[email protected]> > >> Cc: Andrew Cooper <[email protected]>; Jan Beulich > >> <[email protected]>; Paul Durrant <[email protected]>; Stefano > >> Stabellini <[email protected]>; Julien Grall <[email protected]>; > Wei > >> Liu <[email protected]>; Roger Pau Monne <[email protected]>; > >> George Dunlap <[email protected]> > >> 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 <[email protected]> > >> --- > >> CC: Jan Beulich <[email protected]> > >> CC: Paul Durrant <[email protected]> > >> CC: Stefano Stabellini <[email protected]> > >> CC: Julien Grall <[email protected]> > >> CC: Wei Liu <[email protected]> > >> CC: Roger Pau Monné <[email protected]> > >> CC: George Dunlap <[email protected]> > >> > >> 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 [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
