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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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