Re: [Xen-devel] [PATCH v3 16/24] xen/passthrough: Introduce iommu_construct

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.01.15 at 18:11, <julien.grall@xxxxxxxxxx> wrote:
> On 20/01/15 16:40, Jan Beulich wrote:
>>>>> On 20.01.15 at 15:28, <julien.grall@xxxxxxxxxx> wrote:
>>> On 19/01/15 17:02, Jan Beulich wrote:
>>>>>>> On 13.01.15 at 15:25, <julien.grall@xxxxxxxxxx> wrote:
>>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>>> @@ -41,6 +41,10 @@ int iommu_assign_dt_device(struct domain *d, struct 
> dt_device_node *dev)
>>>>>      if ( !list_empty(&dev->domain_list) )
>>>>>          goto fail;
>>>>>  
>>>>> +    rc = iommu_construct(d);
>>>>> +    if ( rc )
>>>>> +        goto fail;
>>>>
>>>> Considering that the only (current) caller of this it domain_build.c I'm
>>>> afraid you're going to get into trouble if you get back -ERESTART
>>>> here. Note that on x86 Dom0 setup works via iommu_hwdom_init(),
>>>> which deals with the preemption needs (at that point in time) by
>>>> calling process_pending_softirqs() every once in a while.
>>>
>>> iommu_hwdom_init is also called for ARM (it's part of the common domain
>>> creation code). So, I don't see any issue here as we match the same
>>> behavior as PCI.
>>>
>>> FWIW, on the previous version you asked to check the need_iommu(d) in
>>> iommu_construct. For DOM0 it will return 0 and therefore never return
>>> -ERESTART.
>> 
>> Quoting the function:
>> 
>> +int iommu_construct(struct domain *d)
>> +{
>> +    int rc = 0;
>> +
>> +    if ( need_iommu(d) > 0 )
>> +        return 0;
>> +
>> +    if ( !iommu_use_hap_pt(d) )
>> +    {
>> +        rc = arch_iommu_populate_page_table(d);
>> +        if ( rc )
>> +            return rc;
>> +    }
>> +
>> +    d->need_iommu = 1;
>> +
>> +    return rc;
>> +}
> 
>> If need_iommu() returns 0 for Dom0, then the early return won't get
>> used. Hence I don't follow your comment above. And if what you say
>> there was correct, then I don't understand why you add the call
>> quoted at the very top in the first place (again taking into consideration
>> that - afaict - the only [current] caller is in domain_build.c).
> 
> I don't understand what is the issue in the device tree use case. As I
> said, assign_device in the pci do exactly the same things.

Sure, but it's not being called for Dom0, but only out of the domctl
handler.

> While this function is currently only used for DOM0, this will be used
> in a later patch for guest non-PCI passthrough.

Okay, but you shouldn't break (or alter in [seemingly] benign ways) the
Dom0 case imo.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel