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: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.

~Andrew

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