Re: [Qemu-devel] [PATCH v11 04/28] x86-iommu: q35: generalize find_add_as()

[David Kiarie]

On Mon, Jul 11, 2016 at 8:32 AM, Peter Xu <address@hidden> wrote:
> On Sat, Jul 09, 2016 at 10:14:48AM +0200, Jan Kiszka wrote:
>> On 2016-07-05 10:19, Peter Xu wrote:
>> > Remove VT-d calls in common q35 codes. Instead, we provide a general
>> > find_add_as() for x86-iommu type.
>> >
>> > Signed-off-by: Peter Xu <address@hidden>
>> > ---
>> >  hw/i386/intel_iommu.c         | 15 ++++++++-------
>> >  include/hw/i386/intel_iommu.h |  5 -----
>> >  include/hw/i386/x86-iommu.h   |  3 +++
>> >  3 files changed, 11 insertions(+), 12 deletions(-)
>>
>> You claim to remove something from "common q35 code", but I don't see
>> changes to it. Instead, the patch introduces a method that seems to
>> remain unused outside the implementing class (I just grep'ed your tree).
>> Anything missing?
>
> Right. The commit message lost its point after I did the rebase to
> Marcel's "-device intel_iommu" patches... Thanks for pointing it out.

I think Jan is mainly asking about where the method 'find_add_as()' is
being used. Unless I'm too missing something It doesn't seem to be
used anywhere outside the implementing class.

>
> Before the rebase, there is one q35_host_dma_iommu() in pc_q35.c, and
> originally this patch did remove something from q35. While in Marcel's
> commit (621d983a1f), q35_host_dma_iommu() is renamed to
> vtd_host_dma_iommu(), and it's put inside intel_iommu.c. After that,
> this commit message stopped making sense.
>
> So I think at least the commit message of this patch could be fixed
> into something like:
>
>    "Introduce common find_add_as() interface for x86-iommu."
>
> And if I now see this... A better solution is to provide a more common
> interface directly in x86-iommu.c to find address spaces, and let
> Intel/AMD IOMMUs share this functionality. After all, we are doing
> merely the same thing to maintain namespaces in both Intel/AMD IOMMUs
> (vtd_find_add_as() and bridge_host_amdvi()). So, do you (and mst?)
> think I should respin to a v12, or we can first fix commit message of
> this patch, then I post another patch basd on this series for a better
> cleanup?
>
> Thanks,
>
> -- peterx