Re: [RFC for-4.5 08/12] xen/passthrough: iommu: Split generic IOMMU code

[Julien Grall <julien.grall@linaro.org>]

Hello Jan,

Thanks for the review.

On 10/02/14 08:22, Jan Beulich wrote:
>>>> On 07.02.14 at 18:43, Julien Grall <julien.grall@linaro.org> wrote:
>> The generic IOMMU framework code (xen/drivers/passthrough/iommu.c) contains
>> functions specific to x86 and PCI.
>>
>> Split the framework in 3 distincts files:
>>      - iommu.c: contains generic functions shared between x86 and ARM
>>                 (when it will be supported)
>>      - iommu_pci.c: contains specific functions for PCI passthrough
>>      - iommu_x86.c: contains specific functions for x86
>>
>> iommu_pci.c will be only compiled when PCI is supported by the architecture
>> (eg. HAS_PCI is defined).
>>
>> This patch is mostly code movement in new files.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Cc: Xiantao Zhang <xiantao.zhang@intel.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> ---
>>   xen/drivers/passthrough/Makefile    |    6 +-
>>   xen/drivers/passthrough/iommu.c     |  473 +----------------------------------
>>   xen/drivers/passthrough/iommu_pci.c |  468 ++++++++++++++++++++++++++++++++++
>
> There's xen/drivers/passthrough/pci.c already - any reason not to
> move the code there?

I didn't specific about moving the code directly in passthrough/pci.c. I 
will do on the next version.

>
>>   xen/drivers/passthrough/iommu_x86.c |   65 +++++
>
> Same here for xen/drivers/passthrough/x86/.
>
>> @@ -696,125 +344,6 @@ void iommu_crash_shutdown(void)
>>       iommu_enabled = iommu_intremap = 0;
>>   }
>>
>> -int iommu_do_domctl(
>> -    struct xen_domctl *domctl, struct domain *d,
>> -    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>
> The function itself should probably not be moved out. Either the
> PCI-specific pieces of it should be made conditional, or a
> descendant function be created. Since (afaict) you'll need all of
> the domctl-s (with different arguments) too for pass-through on
> ARM - what's your plan for them?

PCI passthrough will be supported on ARM in the future. But we will also 
have to support passthrough (via the device tree). I think, we will add 
new DOMCTL for that. Didn't really think about that know.

I will add a descendant function to handle PCI DOMCTL.

>
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -1784,31 +1784,31 @@ static int intel_iommu_unmap_page(struct domain *d,
>> unsigned long gfn)
>>
>>   void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
>>                        int order, int present)
>> -{
>> -    struct acpi_drhd_unit *drhd;
>> -    struct iommu *iommu = NULL;
>> -    struct hvm_iommu *hd = domain_hvm_iommu(d);
>> -    int flush_dev_iotlb;
>> -    int iommu_domid;
>> +    {
>> +        struct acpi_drhd_unit *drhd;
>> +        struct iommu *iommu = NULL;
>> +        struct hvm_iommu *hd = domain_hvm_iommu(d);
>> +        int flush_dev_iotlb;
>> +        int iommu_domid;
>>
>> -    iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
>> +        iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
>>
>> -    for_each_drhd_unit ( drhd )
>> -    {
>> -        iommu = drhd->iommu;
>> -        if ( !test_bit(iommu->index, &hd->iommu_bitmap) )
>> -            continue;
>> +        for_each_drhd_unit ( drhd )
>> +        {
>> +            iommu = drhd->iommu;
>> +            if ( !test_bit(iommu->index, &hd->iommu_bitmap) )
>> +                continue;
>>
>> -        flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
>> -        iommu_domid= domain_iommu_domid(d, iommu);
>> -        if ( iommu_domid == -1 )
>> -            continue;
>> -        if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
>> -                                   (paddr_t)gfn << PAGE_SHIFT_4K,
>> -                                   order, !present, flush_dev_iotlb) )
>> -            iommu_flush_write_buffer(iommu);
>> +            flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
>> +            iommu_domid= domain_iommu_domid(d, iommu);
>> +            if ( iommu_domid == -1 )
>> +                continue;
>> +            if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
>> +                                       (paddr_t)gfn << PAGE_SHIFT_4K,
>> +                                       order, !present, flush_dev_iotlb) )
>> +                iommu_flush_write_buffer(iommu);
>> +        }
>>       }
>> -}
>
> What are these changes to indentation about? Are you
> deliberately breaking common rules here, or is this some sort of
> unintentional leftover?

It's an error when I have created this patch. I will remove it.

Regards,

-- 
Julien Grall