Re: [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation

[Peter Xu]

On Thu, Dec 02, 2021 at 11:49:25AM +0800, Jason Wang wrote:
> On Thu, Dec 2, 2021 at 4:55 AM David Woodhouse <dwmw2@infradead.org> wrote:
> >
> > From: David Woodhouse <dwmw@amazon.co.uk>
> >
> > By setting none of the SAGAW bits we can indicate to a guest that DMA
> > translation isn't supported. Tested by booting Windows 10, as well as
> > Linux guests with the fix at https://git.kernel.org/torvalds/c/c40aaaac10
> >
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >  hw/i386/intel_iommu.c         | 14 ++++++++++----
> >  include/hw/i386/intel_iommu.h |  1 +
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 294499ee20..ffc852d110 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2202,7 +2202,7 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s)
> >      uint32_t changed = status ^ val;
> >
> >      trace_vtd_reg_write_gcmd(status, val);
> > -    if (changed & VTD_GCMD_TE) {
> > +    if ((changed & VTD_GCMD_TE) && s->dma_translation) {
> >          /* Translation enable/disable */
> >          vtd_handle_gcmd_te(s, val & VTD_GCMD_TE);
> >      }
> > @@ -3100,6 +3100,7 @@ static Property vtd_properties[] = {
> >      DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
> >      DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, 
> > FALSE),
> >      DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> > +    DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, 
> > true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > @@ -3605,12 +3606,17 @@ static void vtd_init(IntelIOMMUState *s)
> >      s->next_frcd_reg = 0;
> >      s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
> >               VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS |
> > -             VTD_CAP_SAGAW_39bit | VTD_CAP_MGAW(s->aw_bits);
> > +             VTD_CAP_MGAW(s->aw_bits);
> >      if (s->dma_drain) {
> >          s->cap |= VTD_CAP_DRAIN;
> >      }
> > -    if (s->aw_bits == VTD_HOST_AW_48BIT) {
> > -        s->cap |= VTD_CAP_SAGAW_48bit;
> > +    if (s->dma_translation) {
> > +            if (s->aw_bits >= VTD_HOST_AW_39BIT) {
> > +                    s->cap |= VTD_CAP_SAGAW_39bit;
> > +            }
> > +            if (s->aw_bits >= VTD_HOST_AW_48BIT) {
> > +                    s->cap |= VTD_CAP_SAGAW_48bit;
> > +            }
> >      }
> 
> Just wonder if this is the hardware behaviour as I see 0 is reserved
> for SAGAW in vtd 3.3 spec.

Yes I have the same question.  But if latest Linux & Windows work fine then it
seems ok if we have explicit use scenario with enabling IR only.

In that case commenting with some details would be more than welcomed, e.g.
mentioning Linux commit c40aaaac1018 ("iommu/vt-d: Gracefully handle DMAR units
with no supported address widths", 2020-10-07) as comments above the code.

One more question: SAGAW is a bitmask, is it intended to only set 1 bit out of
the mask?  I'm afraid it could break guest OS that only support 39 bits when
the qemu cmdline has aw-bits=48 specified.

Thanks,

-- 
Peter Xu