| Message ID | 20190328104933.13465-3-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | intel_iommu: misc scalable mode fixes for 4.0 | expand |
* 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
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.
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 (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
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.
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 */
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(-)