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

[Peter Xu]

On Fri, Dec 03, 2021 at 10:46:46AM +0000, David Woodhouse wrote:
> On Fri, 2021-12-03 at 15:38 +0800, Peter Xu wrote:
> > 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.
> 
> Bit zero is reserved. The *value* zero is just what you get when none
> of the bits are set.
> 
>       "A value of 1 in any of these bits indicates the corresponding
>       adjusted guest address width is supported.The adjusted guest
>       address widths corresponding to various bit positions within
>       this field are:
> 
>        • 0: Reserved
>        • 1: 39-bit AGAW (3-level page-table)
>        • 2: 48-bit AGAW (4-level page-table)
>        • 3: 57-bit AGAW (5-level page-table)
>        • 4: Reserved
> 
>       Software must ensure that the adjusted guest address width used
>       to set up the page tables is one of the supported guest address
>       widths reported in this field.
> 
>       Hardware implementations reporting second-level translation
>       support (SLTS) field as Clear also report this field as 0.

I see.

>  
> > 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.
> 
> No, we should set all bits that correspond to supported page table
> depths. And I believe we still do; there's no 'else' in the patch above
> so it will set *both* the 39-bit and 48-bit support bits if s->aw_bits
> is high enough to support both.

Yeah I missed that, sorry.

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu