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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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:
>>>> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
>>> grant_table *gt, grant_ref_t ref,
>>>>  }
>>>>  #endif
>>>>
>>>> +/* caller must hold write lock */
>>>> +static int gnttab_get_status_frame_mfn(struct domain *d,
>>>> +                                       unsigned long idx, mfn_t *mfn)
>>>> +{
>>>> +    struct grant_table *gt = d->grant_table;
>>> const?
>>>
>> IIRC that didn't work because gnttab_grow_table() modifies the content.
> But you don't pass gt to the function:
>
>>>> +    ASSERT(gt->gt_version == 2);
>>>> +
>>>> +    if ( idx >= nr_status_frames(gt) )
>>>> +    {
>>>> +        unsigned long nr = status_to_grant_frames(idx + 1);
>>>> +
>>>> +        if ( nr <= gt->max_grant_frames )
>>>> +            gnttab_grow_table(d, nr);
> ^^^
>
>>>> @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
>>> unsigned long idx, gfn_t gfn,
>>>>      return rc;
>>>>  }
>>>>
>>>> +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?

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.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel