Re: [Qemu-devel] [PATCH 2/6] x86_iommu/amd: Prepare for interrupt remap support

[Peter Xu]

On Thu, Sep 13, 2018 at 03:15:27PM +0700, Suravee Suthikulpanit wrote:
> Brijesh / Peter,
> 
> On 9/13/18 10:15 AM, Peter Xu wrote:
> > On Wed, Sep 12, 2018 at 01:59:06PM -0500, Brijesh Singh wrote:
> > 
> > [...]
> > 
> > > > >        }
> > > > >        return &iommu_as[devfn]->as;
> > > > >    }
> > > > > @@ -1172,6 +1274,10 @@ static void amdvi_realize(DeviceState *dev, 
> > > > > Error **err)
> > > > >            return;
> > > > >        }
> > > > > +    /* Pseudo address space under root PCI bus. */
> > > > > +    pcms->ioapic_as = amdvi_host_dma_iommu(bus, s, 
> > > > > AMDVI_SB_IOAPIC_ID);
> > > > > +    s->intr_enabled = x86_iommu->intr_supported;
> > > > 
> > > > So does this mean that AMD IR cannot be disabled if declared support?
> > > > For VT-d, IR needs to be explicitly enabled otherwise disabled (even
> > > > supported).
> > > > 
> > > 
> > > 
> > > Yes, once its declared as supported then it can not disabled. Its
> > > upto the guest OS to decide whether it want to use the intr remapping
> > > feature by parsing the IVRS. This also brings question, should we
> > > just enable it by default because its guest OS decision whether it
> > > wants to use it or not.
> > 
> > It's by default off?  I mean:
> > 
> >      DEFINE_PROP_BOOL("intremap", X86IOMMUState, intr_supported, false),
> > 
> > Then it's good to me, thanks.  You could add a comment there
> > explicitly mentioning that "IR will be enabled as long as declared
> > support for AMD" since it might confuse some of the people like me...
> > but it's optional.
> > 
> > Regards,
> > 
> 
> Actually, there are two separate knobs here:
> 
> * This option basically means the interrupt remapping support is 
> available/unavailable,
>   which should default to available unless specified to unavailable at the 
> QEMU command line.
>   For example, the AMD IOMMU v1 does not have interrupt remapping support, 
> which means
>   the interrupt remapping bit fields in DTE (e.g. IV, IntCtl, EIntPass, 
> INITPass, NMIPass,
>   Lint0Pass, Lint1Pass) are ignored.
> 
> * If interrupt remapping support is available, the guest OS can decide to 
> enable or disable the feature
>   using DTE bit fileds (e.g. Linux option intremap=off would disable 
> interrupt remapping
>   by not setting DTE[IV] bit). For Linux AMD IOMMU driver, the default is to 
> enable the interrupt remapping.
> 
> In facts, we should not need this option. However, if you prefer to keep this 
> option,
> we probably should rename this to "intremap_sup", in which if the default 
> value should be 1.

I see, thanks.  For me I don't yet see a reason for that intremap_sup
since AFAIU that's exactly what current QEMU's intremap parameter do.

I think Intel is having the similar knobs:

* The "intremap" in QEMU decides whether the hardware we emulate
  supports interrupt remapping, and

* The "intremap" in the guest decides whether the guest kernel will
  use the interrupt remapping feature

Though IMHO the only difference is that Intel has another global knob
to turn that on/off in the GCMD register (VT-d spec 10.4.4, GCMD bit
25) depending on whether the 2nd knob is on, while for AMD it's just
per-device rather than per-iommu.

Regards,

-- 
Peter Xu