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

[Peter Xu]

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 (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

Regards,

-- 
Peter Xu