[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.