diff mbox series

[2/4] intel_iommu: Support IR-only mode without DMA translation

Message ID 20220314142544.150555-2-dwmw2@infradead.org (mailing list archive)
State New, archived
Headers show
Series [1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement | expand

Commit Message

David Woodhouse March 14, 2022, 2:25 p.m. UTC
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>
Reviewed-by: Peter Xu <peterx@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/intel_iommu.c         | 14 ++++++++++----
 include/hw/i386/intel_iommu.h |  1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

Comments

Michael S. Tsirkin March 14, 2022, 3:24 p.m. UTC | #1
On Mon, Mar 14, 2022 at 02:25:42PM +0000, David Woodhouse 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>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Acked-by: Jason Wang <jasowang@redhat.com>

this is borderline like a feature, but ...

> ---
>  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 32471a44cb..948c653e74 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2214,7 +2214,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);
>      }
> @@ -3122,6 +3122,7 @@ static Property vtd_properties[] = {
>      DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
>      DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false),
>      DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> +    DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -3627,12 +3628,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;
> +            }
>      }
>      s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
>


... this looks like you are actually fixing aw_bits < VTD_HOST_AW_39BIT,
right? So maybe this patch is ok like this since it also fixes a
bug. Pls add this to commit log though.

  
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 3b5ac869db..d898be85ce 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -267,6 +267,7 @@ struct IntelIOMMUState {
>      bool buggy_eim;                 /* Force buggy EIM unless eim=off */
>      uint8_t aw_bits;                /* Host/IOVA address width (in bits) */
>      bool dma_drain;                 /* Whether DMA r/w draining enabled */
> +    bool dma_translation;           /* Whether DMA translation supported */
>  
>      /*
>       * Protects IOMMU states in general.  Currently it protects the
> -- 
> 2.33.1
David Woodhouse March 14, 2022, 3:45 p.m. UTC | #2
On Mon, 2022-03-14 at 11:24 -0400, Michael S. Tsirkin wrote:
> On Mon, Mar 14, 2022 at 02:25:42PM +0000, David Woodhouse 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>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> 
> this is borderline like a feature, but ...

It's the opposite of a feature — it's turning the feature *off* ;)

> > 
> > @@ -3627,12 +3628,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;
> > +            }
> >      }
> >      s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
> > 
> 
> 
> ... this looks like you are actually fixing aw_bits < VTD_HOST_AW_39BIT,
> right? So maybe this patch is ok like this since it also fixes a
> bug. Pls add this to commit log though.

Nah, aw_bits cannot be < VTD_HOST_AW_39BIT. We explicitly check in
vtd_decide_config(), and only 39 or 48 bits are supported,
corresponding to 3-level or 4-level page tables.

The only time we'd want to *not* advertise 39-bit support is if we
aren't advertising DMA translation at all. Which is the (anti-)feature
introduced by this patch.
David Woodhouse March 14, 2022, 4:01 p.m. UTC | #3
On Mon, 2022-03-14 at 14:25 +0000, David Woodhouse 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

I suppose we could update that commit message a little. Since I first
posted this and the IRE bugfix in October 2020, the Linux commit it
references was released in the v5.10 kernel.
Michael S. Tsirkin March 14, 2022, 10:27 p.m. UTC | #4
On Mon, Mar 14, 2022 at 03:45:47PM +0000, David Woodhouse wrote:
> On Mon, 2022-03-14 at 11:24 -0400, Michael S. Tsirkin wrote:
> > On Mon, Mar 14, 2022 at 02:25:42PM +0000, David Woodhouse 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>
> > > Reviewed-by: Peter Xu <peterx@redhat.com>
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> > 
> > this is borderline like a feature, but ...
> 
> It's the opposite of a feature — it's turning the feature *off* ;)

Right. Still - do you believe it's appropriate in soft freeze
and if yes why?

> > > 
> > > @@ -3627,12 +3628,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;
> > > +            }
> > >      }
> > >      s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
> > > 
> > 
> > 
> > ... this looks like you are actually fixing aw_bits < VTD_HOST_AW_39BIT,
> > right? So maybe this patch is ok like this since it also fixes a
> > bug. Pls add this to commit log though.
> 
> Nah, aw_bits cannot be < VTD_HOST_AW_39BIT. We explicitly check in
> vtd_decide_config(), and only 39 or 48 bits are supported,
> corresponding to 3-level or 4-level page tables.

Oh right. So not a bugfix then.

> The only time we'd want to *not* advertise 39-bit support is if we
> aren't advertising DMA translation at all. Which is the (anti-)feature
> introduced by this patch.
>
David Woodhouse March 16, 2022, 9:34 a.m. UTC | #5
On Mon, 2022-03-14 at 18:27 -0400, Michael S. Tsirkin wrote:
> On Mon, Mar 14, 2022 at 03:45:47PM +0000, David Woodhouse wrote:
> > It's the opposite of a feature — it's turning the feature *off* ;)
> 
> Right. Still - do you believe it's appropriate in soft freeze
> and if yes why?
> 

Not sure I care very much. I've reposted it a couple of times, with the
bug fixes that go along with it, since it was first posted in October
2020. I confess I don't keep track very much of the freeze status; I'm
only posting the series again this time because Igor posted a related
patch.

But all it's doing is giving us a way to *disable* functionality; it's
not adding new functionality. I suppose someone who cares might make an
argument that that's not so egregious a 'feature' to slip in the middle
of a set of bug fixes that have been outstanding for so long.
diff mbox series

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 32471a44cb..948c653e74 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2214,7 +2214,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);
     }
@@ -3122,6 +3122,7 @@  static Property vtd_properties[] = {
     DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
     DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false),
     DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
+    DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -3627,12 +3628,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;
+            }
     }
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 3b5ac869db..d898be85ce 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -267,6 +267,7 @@  struct IntelIOMMUState {
     bool buggy_eim;                 /* Force buggy EIM unless eim=off */
     uint8_t aw_bits;                /* Host/IOVA address width (in bits) */
     bool dma_drain;                 /* Whether DMA r/w draining enabled */
+    bool dma_translation;           /* Whether DMA translation supported */
 
     /*
      * Protects IOMMU states in general.  Currently it protects the