Re: [Xen-devel] [PATCH v2 09/15] xen/passthrough: iommu: Introduce arch specific code

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 02/24/2014 01:16 PM, Jan Beulich wrote:
>>>> On 24.02.14 at 13:57, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
>>>>  struct hvm_iommu {
>>>> -    u64 pgd_maddr;                 /* io page directory machine address */
>>>> -    spinlock_t mapping_lock;       /* io page table lock */
>>>> -    int agaw;     /* adjusted guest address width, 0 is level 2 30-bit */
>>>> -    struct list_head g2m_ioport_list;  /* guest to machine ioport mapping 
>>>> */
>>>> -    u64 iommu_bitmap;              /* bitmap of iommu(s) that the domain 
>>>> uses */
>>>> -    struct list_head mapped_rmrrs;
>>>> -
>>>> -    /* amd iommu support */
>>>> -    int domain_id;
>>>
>>> At the very least this field doesn't look all that architecture specific,
>>> even if it might only be used on x86/AMD right now.
>>
>> On ARM, each IOMMU will have it's own private data stored in arch.priv.
>> I don't think domain_id will be used as the driver can directly use
>> d->domain_id.
>>
>> I gave a look on AMD IOMMU drivers, and in a same function they mixed
>> d->domain_id and domain_hvm_iommu(d)->arch.domain_id. The latter one has
>> been initialized to d->domain_id in amd_iommu_domain_init.
>>
>> I think, we can even remove this field for x86...
> 
> As Andrew suggested too.
> 
>>>> -    int paging_mode;
>>>
>>> The same might go for this one.
>>
>> There is only one paging mode on ARM.
> 
> But please make your changes here not in the spirit of fitting in ARM,
> but to make the code reasonable architecture clean. Which means
> that fields having a purpose outside of x86 should remain common,
> instead of making x86-specific everything that ARM (currently) has
> no use for.

I didn't want to repeat that I said for the previous field. ARM doesn't
have a generic IOMMU driver (or two) as x86. Basically every vendor can
decide to design it's own IOMMU.

To avoid to have a big structure with some fields used only by one
driver, every IOMMU ARM drivers as its own private structure.

IHMO, I think every fields stored in the generic hvm_iommu structure
should be initialized by generic code, not by a specific driver.

-- 
Julien Grall

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