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

[Peter Xu]

On Thu, Mar 28, 2019 at 11:56:40AM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (address@hidden) 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 <address@hidden>
> > ---
> >  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