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

[David Kiarie]

On Mon, Jul 11, 2016 at 10:41 AM, Peter Xu <address@hidden> wrote:
> On Mon, Jul 11, 2016 at 10:16:11AM +0300, David Kiarie wrote:
>> On Mon, Jul 11, 2016 at 9:49 AM, Peter Xu <address@hidden> wrote:
>> > On Mon, Jul 11, 2016 at 08:46:12AM +0300, David Kiarie wrote:
>> >> 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.
>>
>> Hi
>> >
>> > This patch can be dropped. I was just not sure whether it's the
>> > correct time to do that. Anyway, we may still need one more patch to
>> > cleanup this in the future, as I have mentioned in the previous email.
>>
>> I think there is a misunderstanding here.
>>
>> We (me and Jan) are basically asking did you plan to use "find_add_as"
>> somewhere and may be missed it ? Why does x86-iommu class need
>> "find_add_as" ?
>> The reason is I'm not able to receive IOAPIC
>> interrupts with AMD IOMMU basing my work on your code. We thought
>> you'd clarify on where "find_add_as" is used or how you plan to use
>> it.
>
> As mentioned in previous email, before Marcel's patches,
> vtd_host_dma_iommu() was named q35_host_dma_iommu().

Okay, that solves it - _before_ the adoption of '-device iommu' so
you're right, this is not needed anymore.

 At that time, I
> need "find_add_as" to let Q35 codes get rid of direct calls to VT-d
> (so that pc_q35.c will not need to include "intel_iommu.h" any more,
> instead, it should include "x86-iommu.h"). Also, that interface is
> prepared for future AMD as well. However, now AMD (you patches) are
> directly calling pci_setup_iommu(). I am not sure whether you were
> using it from the beginning, but IIUC as long as you are using
> pci_setup_iommu() interface, we should be able to avoid providing
> find_add_as any more. So I think this patch is indeed okay to be
> dropped... Please kindly correct me if I missed anything. :)
>
> The only reason that we keep this patch (as far as I can think of..)
> is that mst has done some testing on v11 and I'm not sure whether we'd
> better keep it untouched if we are going to merge it (fixing commit
> message does not count, right?). But I'd say I'm not familiar with how
> maintainers manage codes to be merged... Maybe different maintainers
> have their own flavor on this matter? I don't know. Anyway, these are
> only my wild guess.
>
> For the problem you have encountered with IOAPIC, do you think it's
> related to this patch? Have you tried to add some logs in e.g.
> ioapic_service() to see what's wrong in there?
>
> (Maybe we need some general trace logs in IOAPIC codes?...)
>
> -- peterx