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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 21/01/15 14:13, Jan Beulich wrote:
>>>> On 21.01.15 at 13:13, <julien.grall@xxxxxxxxxx> wrote:
>> Hi Jan,
>>
>> On 21/01/15 10:48, Jan Beulich wrote:
>>>>>> On 21.01.15 at 11:37, <julien.grall@xxxxxxxxxx> wrote:
>>>> On 21/01/2015 10:23, Jan Beulich wrote:
>>>>>>>> On 20.01.15 at 18:11, <julien.grall@xxxxxxxxxx> wrote:
>>>>>> 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.
>>>>
>>>> As iommu_hwdom_init is initialized correctly the IOMMU for DOM0, 
>>>> iommu_construct is a no-op.
>>>>
>>>> Would an if ( need_iommu(d) ) will be more clear? Maybe we an assert 
>>>> (!is_hardware_domain(d)).
>>>
>>> Just think this through properly: iommu_hwdom_init() may leave
>>> Dom0's ->need_iommu at 0 or 1 (depending on iommu_dom0_strict).
>>> And iommu_construct() specifically is a nop only when ->need_iommu
>>> is positive (x86's arch_iommu_populate_page_table() sets it to a
>>> negative value to indicate "being set up", and I wonder how ARM
>>> gets away without doing so).
>>
>> iommu_dom0_strict is always set to 1 when IOMMU is used on ARM (see
>> check_hwdom_reqs).
>>
>> Futhermore, we always share the page table with the processor, so we
>> never need to populate the page table.
> 
> That's all far from obvious looking at the patch at hand. And you
> can then only hope that these two "always" will always remain to
> be that way.

Hence the suggested ASSERT on a previous mail. I could also add a
comment in the code explaining the ASSERT().

Regards,

-- 
Julien Grall

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