1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/2] intel_iommu: misc scalable mode fixes for 4.0
  4. View patch

[Qemu-devel] [PATCH 2/2] intel_iommu: Drop extended root field

Peter Xu posted 2 patches 6 years, 10 months ago
Maintainers: Eduardo Habkost <ehabkost@redhat.com>, Richard Henderson <rth@twiddle.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
[Qemu-devel] [PATCH 2/2] intel_iommu: Drop extended root field
Posted by Peter Xu 6 years, 10 months ago 
VTD_RTADDR_RTT is dropped even by the VT-d spec, so QEMU should
probably do the same thing (after all we never really implemented it).
Since we've had a field for that in the migration stream, to keep
compatibility we need to fill the hole up.

Please refer to VT-d spec 10.4.6.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c          | 6 ++----
 hw/i386/intel_iommu_internal.h | 1 -
 hw/i386/trace-events           | 2 +-
 include/hw/i386/intel_iommu.h  | 1 -
 4 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 11ece40ed0..91be1cf239 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1718,12 +1718,11 @@ error:
 static void vtd_root_table_setup(IntelIOMMUState *s)
 {
     s->root = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
-    s->root_extended = s->root & VTD_RTADDR_RTT;
     s->root &= VTD_RTADDR_ADDR_MASK(s->aw_bits);
 
     vtd_update_scalable_state(s);
 
-    trace_vtd_reg_dmar_root(s->root, s->root_extended);
+    trace_vtd_reg_dmar_root(s->root, s->root_scalable);
 }
 
 static void vtd_iec_notify_all(IntelIOMMUState *s, bool global,
@@ -2982,7 +2981,7 @@ static const VMStateDescription vtd_vmstate = {
         VMSTATE_UINT16(next_frcd_reg, IntelIOMMUState),
         VMSTATE_UINT8_ARRAY(csr, IntelIOMMUState, DMAR_REG_SIZE),
         VMSTATE_UINT8(iq_last_desc_type, IntelIOMMUState),
-        VMSTATE_BOOL(root_extended, IntelIOMMUState),
+        VMSTATE_UNUSED(sizeof(bool)),
         VMSTATE_BOOL(dmar_enabled, IntelIOMMUState),
         VMSTATE_BOOL(qi_enabled, IntelIOMMUState),
         VMSTATE_BOOL(intr_enabled, IntelIOMMUState),
@@ -3493,7 +3492,6 @@ static void vtd_init(IntelIOMMUState *s)
     memset(s->womask, 0, DMAR_REG_SIZE);
 
     s->root = 0;
-    s->root_extended = false;
     s->root_scalable = false;
     s->dmar_enabled = false;
     s->intr_enabled = false;
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 1160618177..c1235a7063 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -171,7 +171,6 @@
 #define VTD_CCMD_FM(val)            (((val) >> 32) & 3ULL)
 
 /* RTADDR_REG */
-#define VTD_RTADDR_RTT              (1ULL << 11)
 #define VTD_RTADDR_SMT              (1ULL << 10)
 #define VTD_RTADDR_ADDR_MASK(aw)    (VTD_HAW_MASK(aw) ^ 0xfffULL)
 
diff --git a/hw/i386/trace-events b/hw/i386/trace-events
index 83f8369778..c8bc464bc5 100644
--- a/hw/i386/trace-events
+++ b/hw/i386/trace-events
@@ -45,7 +45,7 @@ vtd_pt_enable_fast_path(uint16_t sid, bool success) "sid 0x%"PRIu16" %d"
 vtd_irq_generate(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data 0x%"PRIx64
 vtd_reg_read(uint64_t addr, uint64_t size) "addr 0x%"PRIx64" size 0x%"PRIx64
 vtd_reg_write(uint64_t addr, uint64_t size, uint64_t val) "addr 0x%"PRIx64" size 0x%"PRIx64" value 0x%"PRIx64
-vtd_reg_dmar_root(uint64_t addr, bool extended) "addr 0x%"PRIx64" extended %d"
+vtd_reg_dmar_root(uint64_t addr, bool scalable) "addr 0x%"PRIx64" scalable %d"
 vtd_reg_ir_root(uint64_t addr, uint32_t size) "addr 0x%"PRIx64" size 0x%"PRIx32
 vtd_reg_write_gcmd(uint32_t status, uint32_t val) "status 0x%"PRIx32" value 0x%"PRIx32
 vtd_reg_write_fectl(uint32_t value) "value 0x%"PRIx32
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index c11e3d5b34..199a813eee 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -231,7 +231,6 @@ struct IntelIOMMUState {
     bool scalable_mode;             /* RO - is Scalable Mode supported? */
 
     dma_addr_t root;                /* Current root table pointer */
-    bool root_extended;             /* Type of root table (extended or not) */
     bool root_scalable;             /* Type of root table (scalable or not) */
     bool dmar_enabled;              /* Set if DMA remapping is enabled */
 
-- 
2.17.1
Re: [Qemu-devel] [PATCH 2/2] intel_iommu: Drop extended root field
Posted by Dr. David Alan Gilbert 6 years, 10 months ago 
* Peter Xu (peterx@redhat.com) wrote:
> VTD_RTADDR_RTT is dropped even by the VT-d spec, so QEMU should
> probably do the same thing (after all we never really implemented it).
> Since we've had a field for that in the migration stream, to keep
> compatibility we need to fill the hole up.
> 
> Please refer to VT-d spec 10.4.6.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c          | 6 ++----
>  hw/i386/intel_iommu_internal.h | 1 -
>  hw/i386/trace-events           | 2 +-
>  include/hw/i386/intel_iommu.h  | 1 -
>  4 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 11ece40ed0..91be1cf239 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1718,12 +1718,11 @@ error:
>  static void vtd_root_table_setup(IntelIOMMUState *s)
>  {
>      s->root = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
> -    s->root_extended = s->root & VTD_RTADDR_RTT;
>      s->root &= VTD_RTADDR_ADDR_MASK(s->aw_bits);
>  
>      vtd_update_scalable_state(s);
>  
> -    trace_vtd_reg_dmar_root(s->root, s->root_extended);
> +    trace_vtd_reg_dmar_root(s->root, s->root_scalable);
>  }
>  
>  static void vtd_iec_notify_all(IntelIOMMUState *s, bool global,
> @@ -2982,7 +2981,7 @@ static const VMStateDescription vtd_vmstate = {
>          VMSTATE_UINT16(next_frcd_reg, IntelIOMMUState),
>          VMSTATE_UINT8_ARRAY(csr, IntelIOMMUState, DMAR_REG_SIZE),
>          VMSTATE_UINT8(iq_last_desc_type, IntelIOMMUState),
> -        VMSTATE_BOOL(root_extended, IntelIOMMUState),
> +        VMSTATE_UNUSED(sizeof(bool)),

I'm not sure that's right; a VMSTATE_BOOL uses get_bool/put_bool that
always writes a single byte, so probably a 
   VMSTATE_UNUSED(1 /* Was a bool */);

may be safer?

Dave

>          VMSTATE_BOOL(dmar_enabled, IntelIOMMUState),
>          VMSTATE_BOOL(qi_enabled, IntelIOMMUState),
>          VMSTATE_BOOL(intr_enabled, IntelIOMMUState),
> @@ -3493,7 +3492,6 @@ static void vtd_init(IntelIOMMUState *s)
>      memset(s->womask, 0, DMAR_REG_SIZE);
>  
>      s->root = 0;
> -    s->root_extended = false;
>      s->root_scalable = false;
>      s->dmar_enabled = false;
>      s->intr_enabled = false;
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 1160618177..c1235a7063 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -171,7 +171,6 @@
>  #define VTD_CCMD_FM(val)            (((val) >> 32) & 3ULL)
>  
>  /* RTADDR_REG */
> -#define VTD_RTADDR_RTT              (1ULL << 11)
>  #define VTD_RTADDR_SMT              (1ULL << 10)
>  #define VTD_RTADDR_ADDR_MASK(aw)    (VTD_HAW_MASK(aw) ^ 0xfffULL)
>  
> diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> index 83f8369778..c8bc464bc5 100644
> --- a/hw/i386/trace-events
> +++ b/hw/i386/trace-events
> @@ -45,7 +45,7 @@ vtd_pt_enable_fast_path(uint16_t sid, bool success) "sid 0x%"PRIu16" %d"
>  vtd_irq_generate(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data 0x%"PRIx64
>  vtd_reg_read(uint64_t addr, uint64_t size) "addr 0x%"PRIx64" size 0x%"PRIx64
>  vtd_reg_write(uint64_t addr, uint64_t size, uint64_t val) "addr 0x%"PRIx64" size 0x%"PRIx64" value 0x%"PRIx64
> -vtd_reg_dmar_root(uint64_t addr, bool extended) "addr 0x%"PRIx64" extended %d"
> +vtd_reg_dmar_root(uint64_t addr, bool scalable) "addr 0x%"PRIx64" scalable %d"
>  vtd_reg_ir_root(uint64_t addr, uint32_t size) "addr 0x%"PRIx64" size 0x%"PRIx32
>  vtd_reg_write_gcmd(uint32_t status, uint32_t val) "status 0x%"PRIx32" value 0x%"PRIx32
>  vtd_reg_write_fectl(uint32_t value) "value 0x%"PRIx32
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index c11e3d5b34..199a813eee 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -231,7 +231,6 @@ struct IntelIOMMUState {
>      bool scalable_mode;             /* RO - is Scalable Mode supported? */
>  
>      dma_addr_t root;                /* Current root table pointer */
> -    bool root_extended;             /* Type of root table (extended or not) */
>      bool root_scalable;             /* Type of root table (scalable or not) */
>      bool dmar_enabled;              /* Set if DMA remapping is enabled */
>  
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH 2/2] intel_iommu: Drop extended root field
Posted by Peter Xu 6 years, 10 months ago 
On Thu, Mar 28, 2019 at 11:56:40AM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > VTD_RTADDR_RTT is dropped even by the VT-d spec, so QEMU should
> > probably do the same thing (after all we never really implemented it).
> > Since we've had a field for that in the migration stream, to keep
> > compatibility we need to fill the hole up.
> > 
> > Please refer to VT-d spec 10.4.6.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/i386/intel_iommu.c          | 6 ++----
> >  hw/i386/intel_iommu_internal.h | 1 -
> >  hw/i386/trace-events           | 2 +-
> >  include/hw/i386/intel_iommu.h  | 1 -
> >  4 files changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 11ece40ed0..91be1cf239 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -1718,12 +1718,11 @@ error:
> >  static void vtd_root_table_setup(IntelIOMMUState *s)
> >  {
> >      s->root = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
> > -    s->root_extended = s->root & VTD_RTADDR_RTT;
> >      s->root &= VTD_RTADDR_ADDR_MASK(s->aw_bits);
> >  
> >      vtd_update_scalable_state(s);
> >  
> > -    trace_vtd_reg_dmar_root(s->root, s->root_extended);
> > +    trace_vtd_reg_dmar_root(s->root, s->root_scalable);
> >  }
> >  
> >  static void vtd_iec_notify_all(IntelIOMMUState *s, bool global,
> > @@ -2982,7 +2981,7 @@ static const VMStateDescription vtd_vmstate = {
> >          VMSTATE_UINT16(next_frcd_reg, IntelIOMMUState),
> >          VMSTATE_UINT8_ARRAY(csr, IntelIOMMUState, DMAR_REG_SIZE),
> >          VMSTATE_UINT8(iq_last_desc_type, IntelIOMMUState),
> > -        VMSTATE_BOOL(root_extended, IntelIOMMUState),
> > +        VMSTATE_UNUSED(sizeof(bool)),
> 
> I'm not sure that's right; a VMSTATE_BOOL uses get_bool/put_bool that
> always writes a single byte, so probably a 
>    VMSTATE_UNUSED(1 /* Was a bool */);
> 
> may be safer?

Probably true. I am sure it's 1 byte on x86_64 but indeed I don't know
all the rest of archs...  Will repost.

Also, since you mentioned about it, I noticed that we have a similar
case where VMSTATE_UNUSED is used in vmstate_ppc_cpu with type that
may have different size with different host/compilers:

        VMSTATE_UNUSED(sizeof(target_ulong)), /* was _EQUAL(env.spr[SPR_PVR]) */

Would that be problematic too?  CCing Alexey and David for this.

Maybe we should comment on VMSTATE_BOOL about the fact (because it
seems error prone)? And maybe also on VMSTATE_UNUSED too.

-- 
Peter Xu
Re: [Qemu-devel] [PATCH 2/2] intel_iommu: Drop extended root field
Posted by Peter Xu 6 years, 10 months ago 
I didn't really CC David and Alexey, I'm doing it again...

On Fri, Mar 29, 2019 at 12:55:38PM +0800, Peter Xu wrote:
> On Thu, Mar 28, 2019 at 11:56:40AM +0000, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > VTD_RTADDR_RTT is dropped even by the VT-d spec, so QEMU should
> > > probably do the same thing (after all we never really implemented it).
> > > Since we've had a field for that in the migration stream, to keep
> > > compatibility we need to fill the hole up.
> > > 
> > > Please refer to VT-d spec 10.4.6.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  hw/i386/intel_iommu.c          | 6 ++----
> > >  hw/i386/intel_iommu_internal.h | 1 -
> > >  hw/i386/trace-events           | 2 +-
> > >  include/hw/i386/intel_iommu.h  | 1 -
> > >  4 files changed, 3 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index 11ece40ed0..91be1cf239 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -1718,12 +1718,11 @@ error:
> > >  static void vtd_root_table_setup(IntelIOMMUState *s)
> > >  {
> > >      s->root = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
> > > -    s->root_extended = s->root & VTD_RTADDR_RTT;
> > >      s->root &= VTD_RTADDR_ADDR_MASK(s->aw_bits);
> > >  
> > >      vtd_update_scalable_state(s);
> > >  
> > > -    trace_vtd_reg_dmar_root(s->root, s->root_extended);
> > > +    trace_vtd_reg_dmar_root(s->root, s->root_scalable);
> > >  }
> > >  
> > >  static void vtd_iec_notify_all(IntelIOMMUState *s, bool global,
> > > @@ -2982,7 +2981,7 @@ static const VMStateDescription vtd_vmstate = {
> > >          VMSTATE_UINT16(next_frcd_reg, IntelIOMMUState),
> > >          VMSTATE_UINT8_ARRAY(csr, IntelIOMMUState, DMAR_REG_SIZE),
> > >          VMSTATE_UINT8(iq_last_desc_type, IntelIOMMUState),
> > > -        VMSTATE_BOOL(root_extended, IntelIOMMUState),
> > > +        VMSTATE_UNUSED(sizeof(bool)),
> > 
> > I'm not sure that's right; a VMSTATE_BOOL uses get_bool/put_bool that
> > always writes a single byte, so probably a 
> >    VMSTATE_UNUSED(1 /* Was a bool */);
> > 
> > may be safer?
> 
> Probably true. I am sure it's 1 byte on x86_64 but indeed I don't know
> all the rest of archs...  Will repost.
> 
> Also, since you mentioned about it, I noticed that we have a similar
> case where VMSTATE_UNUSED is used in vmstate_ppc_cpu with type that
> may have different size with different host/compilers:
> 
>         VMSTATE_UNUSED(sizeof(target_ulong)), /* was _EQUAL(env.spr[SPR_PVR]) */
> 
> Would that be problematic too?  CCing Alexey and David for this.
> 
> Maybe we should comment on VMSTATE_BOOL about the fact (because it
> seems error prone)? And maybe also on VMSTATE_UNUSED too.
> 
> -- 
> Peter Xu

Regards,

-- 
Peter Xu
Re: [Qemu-devel] [PATCH 2/2] intel_iommu: Drop extended root field
Posted by Dr. David Alan Gilbert 6 years, 10 months ago 
* Peter Xu (peterx@redhat.com) wrote:
> On Thu, Mar 28, 2019 at 11:56:40AM +0000, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > VTD_RTADDR_RTT is dropped even by the VT-d spec, so QEMU should
> > > probably do the same thing (after all we never really implemented it).
> > > Since we've had a field for that in the migration stream, to keep
> > > compatibility we need to fill the hole up.
> > > 
> > > Please refer to VT-d spec 10.4.6.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  hw/i386/intel_iommu.c          | 6 ++----
> > >  hw/i386/intel_iommu_internal.h | 1 -
> > >  hw/i386/trace-events           | 2 +-
> > >  include/hw/i386/intel_iommu.h  | 1 -
> > >  4 files changed, 3 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index 11ece40ed0..91be1cf239 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -1718,12 +1718,11 @@ error:
> > >  static void vtd_root_table_setup(IntelIOMMUState *s)
> > >  {
> > >      s->root = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
> > > -    s->root_extended = s->root & VTD_RTADDR_RTT;
> > >      s->root &= VTD_RTADDR_ADDR_MASK(s->aw_bits);
> > >  
> > >      vtd_update_scalable_state(s);
> > >  
> > > -    trace_vtd_reg_dmar_root(s->root, s->root_extended);
> > > +    trace_vtd_reg_dmar_root(s->root, s->root_scalable);
> > >  }
> > >  
> > >  static void vtd_iec_notify_all(IntelIOMMUState *s, bool global,
> > > @@ -2982,7 +2981,7 @@ static const VMStateDescription vtd_vmstate = {
> > >          VMSTATE_UINT16(next_frcd_reg, IntelIOMMUState),
> > >          VMSTATE_UINT8_ARRAY(csr, IntelIOMMUState, DMAR_REG_SIZE),
> > >          VMSTATE_UINT8(iq_last_desc_type, IntelIOMMUState),
> > > -        VMSTATE_BOOL(root_extended, IntelIOMMUState),
> > > +        VMSTATE_UNUSED(sizeof(bool)),
> > 
> > I'm not sure that's right; a VMSTATE_BOOL uses get_bool/put_bool that
> > always writes a single byte, so probably a 
> >    VMSTATE_UNUSED(1 /* Was a bool */);
> > 
> > may be safer?
> 
> Probably true. I am sure it's 1 byte on x86_64 but indeed I don't know
> all the rest of archs...  Will repost.
> 
> Also, since you mentioned about it, I noticed that we have a similar
> case where VMSTATE_UNUSED is used in vmstate_ppc_cpu with type that
> may have different size with different host/compilers:
> 
>         VMSTATE_UNUSED(sizeof(target_ulong)), /* was _EQUAL(env.spr[SPR_PVR]) */
> 
> Would that be problematic too?  CCing Alexey and David for this.

The change was:
-        VMSTATE_UINTTL_EQUAL(env.spr[SPR_PVR], PowerPCCPU),
+        VMSTATE_UNUSED(sizeof(target_ulong)), /* was _EQUAL(env.spr[SPR_PVR]) */

so it was always a target-long rather than a portable value; so that's
fine I think.

> Maybe we should comment on VMSTATE_BOOL about the fact (because it
> seems error prone)? And maybe also on VMSTATE_UNUSED too.

A comment wouldn't hurt; espeically on UNUSED I think.

Dave

> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH 2/2] intel_iommu: Drop extended root field
Posted by Peter Xu 6 years, 10 months ago 
On Fri, Mar 29, 2019 at 09:12:06AM +0000, Dr. David Alan Gilbert wrote:
> The change was:
> -        VMSTATE_UINTTL_EQUAL(env.spr[SPR_PVR], PowerPCCPU),
> +        VMSTATE_UNUSED(sizeof(target_ulong)), /* was _EQUAL(env.spr[SPR_PVR]) */
> 
> so it was always a target-long rather than a portable value; so that's
> fine I think.

Oh yes, I obviously misread on "target".  Sorry.

-- 
Peter Xu

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.