> -----Original Message----- > From: Andrew Cooper > Sent: 08 August 2018 12:34 > To: Jan Beulich <jbeul...@suse.com> > Cc: George Dunlap <george.dun...@citrix.com>; Ian Jackson > <ian.jack...@citrix.com>; Paul Durrant <paul.durr...@citrix.com>; Wei Liu > <wei.l...@citrix.com>; Stefano Stabellini <sstabell...@kernel.org>; xen- > devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk > <konrad.w...@oracle.com>; Tim (Xen.org) <t...@xen.org> > Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type: > XENMEM_resource_grant_table > > On 08/08/18 11:55, Jan Beulich wrote: > >>>> On 08.08.18 at 12:46, <andrew.coop...@citrix.com> wrote: > >> On 08/08/18 11:43, Jan Beulich wrote: > >>>>>> On 08.08.18 at 12:38, <paul.durr...@citrix.com> wrote: > >>>>> From: Jan Beulich [mailto:jbeul...@suse.com] > >>>>> Sent: 08 August 2018 11:30 > >>>>> > >>>>>>>> On 08.08.18 at 11:00, <paul.durr...@citrix.com> wrote: > >>>>>> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx, > >>>>>> + mfn_t *mfn) > >>>>>> +{ > >>>>>> + struct grant_table *gt = d->grant_table; > >>>>>> + int rc; > >>>>>> + > >>>>>> + grant_write_lock(gt); > >>>>>> + > >>>>>> + if ( gt->gt_version == 0 ) > >>>>>> + gt->gt_version = 1; > >>>>> Since you've moved this here instead of dropping it, what > requirement > >>>>> have you found for this to be set (other than the ASSERT() you put in > >>>>> gnttab_get_shared_frame_mfn()? > >>>>> > >>>> The code in patch #2 is executed before the grant table version is set. I > >>>> could alternatively have libxl explicitly set the version to 1 before > >>>> trying > >>>> to seed the table. > >>> But that's not my point. What's wrong with leaving it at zero? > >> On a tangent, why does a gnttab version of 0 exist at all? Why don't we > >> explicitly initialise it to 1 in the hypervisor? > > Fair question, which unfortunately I can't answer. > > > >> We've had plenty of XSAs to do with mishandling of a gnttab version of > >> 0. Why not fix the problem at its source, and simplify the gnttab code > >> while we are at it. > > I don't mind, unless a reason for the seemingly strange behavior can be > > found. > > gt_version only came in with grant table v2, so the toolstack never > previously set a version. That ended up with cases where dom0 tries to > map the store/cons grants before the guest driver has chosen a version. > > I expect its like this because grant table v2 was a giant pile of poorly > reviewed hacks, rather than for any better reason. > > If noone else gets to it, I'll fold it into my series to mess thoroughly > with parameter handling in DOMCTL_createdomain.
If that's going to take a while then I can add the explicit version set in patch #2. Paul > > ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel