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

[Peter Xu]

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