Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 08 August 2018 12:34
> To: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> 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.cooper3@xxxxxxxxxx> wrote:
> >> On 08/08/18 11:43, Jan Beulich wrote:
> >>>>>> On 08.08.18 at 12:38, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >>>>> Sent: 08 August 2018 11:30
> >>>>>
> >>>>>>>> On 08.08.18 at 11:00, <paul.durrant@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel