* [Qemu-devel] [PATCH 0/2] x86_iommu: Fix segfault when starting on non-PCI machines
@ 2017-09-15 11:13 Mohammed Gamal
2017-09-15 11:13 ` [Qemu-devel] [PATCH 1/2] x86_iommu: Move machine check to x86_iommu_realize() Mohammed Gamal
2017-09-15 11:13 ` [Qemu-devel] [PATCH 2/2] x86_iommu: check if machine has PCI bus Mohammed Gamal
0 siblings, 2 replies; 6+ messages in thread
From: Mohammed Gamal @ 2017-09-15 11:13 UTC (permalink / raw)
To: qemu-devel; +Cc: ehabkost, mst, thuth, peterx, Mohammed Gamal
Starting qemu with
qemu-system-x86_64 -S -M isapc -device {amd|intel}-iommu
leads to a segfault. The code assume PCI bus is present and
tries to access the bus structure without checking.
The patch series moves the error checks from vtd_realize()
and amdvi_realize() to the generic x86_iommu_realize() and
adds a check for PCI bus presence.
Mohammed Gamal (2):
x86_iommu: Move machine check to x86_iommu_realize()
x86_iommu: check if machine has PCI bus
hw/i386/amd_iommu.c | 10 +---------
hw/i386/intel_iommu.c | 10 +---------
hw/i386/x86-iommu.c | 13 +++++++++++++
3 files changed, 15 insertions(+), 18 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 1/2] x86_iommu: Move machine check to x86_iommu_realize() 2017-09-15 11:13 [Qemu-devel] [PATCH 0/2] x86_iommu: Fix segfault when starting on non-PCI machines Mohammed Gamal @ 2017-09-15 11:13 ` Mohammed Gamal 2017-09-15 13:52 ` Peter Xu 2017-09-15 11:13 ` [Qemu-devel] [PATCH 2/2] x86_iommu: check if machine has PCI bus Mohammed Gamal 1 sibling, 1 reply; 6+ messages in thread From: Mohammed Gamal @ 2017-09-15 11:13 UTC (permalink / raw) To: qemu-devel; +Cc: ehabkost, mst, thuth, peterx, Mohammed Gamal Instead of having the same error checks in vtd_realize() and amdvi_realize(), move that over to the generic x86_iommu_realize(). Signed-off-by: Mohammed Gamal <mgamal@redhat.com> --- hw/i386/amd_iommu.c | 10 +--------- hw/i386/intel_iommu.c | 10 +--------- hw/i386/x86-iommu.c | 13 +++++++++++++ 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c index 334938a..839f01f 100644 --- a/hw/i386/amd_iommu.c +++ b/hw/i386/amd_iommu.c @@ -1141,18 +1141,10 @@ static void amdvi_realize(DeviceState *dev, Error **err) AMDVIState *s = AMD_IOMMU_DEVICE(dev); X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); MachineState *ms = MACHINE(qdev_get_machine()); - MachineClass *mc = MACHINE_GET_CLASS(ms); PCMachineState *pcms = PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); - PCIBus *bus; + PCIBus *bus = pcms->bus; - if (!pcms) { - error_setg(err, "Machine-type '%s' not supported by amd-iommu", - mc->name); - return; - } - - bus = pcms->bus; s->iotlb = g_hash_table_new_full(amdvi_uint64_hash, amdvi_uint64_equal, g_free, g_free); diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 3a5bb0b..aa01812 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3027,20 +3027,12 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) static void vtd_realize(DeviceState *dev, Error **errp) { MachineState *ms = MACHINE(qdev_get_machine()); - MachineClass *mc = MACHINE_GET_CLASS(ms); PCMachineState *pcms = PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); - PCIBus *bus; + PCIBus *bus = pcms->bus; IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); - if (!pcms) { - error_setg(errp, "Machine-type '%s' not supported by intel-iommu", - mc->name); - return; - } - - bus = pcms->bus; x86_iommu->type = TYPE_INTEL; if (!vtd_decide_config(s, errp)) { diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c index 293caf8..4d17e1f 100644 --- a/hw/i386/x86-iommu.c +++ b/hw/i386/x86-iommu.c @@ -21,6 +21,8 @@ #include "hw/sysbus.h" #include "hw/boards.h" #include "hw/i386/x86-iommu.h" +#include "hw/i386/pc.h" +#include "qapi/error.h" #include "qemu/error-report.h" #include "trace.h" @@ -80,7 +82,18 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp) { X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); X86IOMMUClass *x86_class = X86_IOMMU_GET_CLASS(dev); + MachineState *ms = MACHINE(qdev_get_machine()); + MachineClass *mc = MACHINE_GET_CLASS(ms); + PCMachineState *pcms = + PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); QLIST_INIT(&x86_iommu->iec_notifiers); + + if (!pcms) { + error_setg(errp, "Machine-type '%s' not supported by iommu", + mc->name); + return; + } + if (x86_class->realize) { x86_class->realize(dev, errp); } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] x86_iommu: Move machine check to x86_iommu_realize() 2017-09-15 11:13 ` [Qemu-devel] [PATCH 1/2] x86_iommu: Move machine check to x86_iommu_realize() Mohammed Gamal @ 2017-09-15 13:52 ` Peter Xu 2017-09-15 13:55 ` Peter Xu 0 siblings, 1 reply; 6+ messages in thread From: Peter Xu @ 2017-09-15 13:52 UTC (permalink / raw) To: Mohammed Gamal; +Cc: qemu-devel, ehabkost, mst, thuth On Fri, Sep 15, 2017 at 01:13:45PM +0200, Mohammed Gamal wrote: > Instead of having the same error checks in vtd_realize() > and amdvi_realize(), move that over to the generic > x86_iommu_realize(). > > Signed-off-by: Mohammed Gamal <mgamal@redhat.com> > --- > hw/i386/amd_iommu.c | 10 +--------- > hw/i386/intel_iommu.c | 10 +--------- > hw/i386/x86-iommu.c | 13 +++++++++++++ > 3 files changed, 15 insertions(+), 18 deletions(-) > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c > index 334938a..839f01f 100644 > --- a/hw/i386/amd_iommu.c > +++ b/hw/i386/amd_iommu.c > @@ -1141,18 +1141,10 @@ static void amdvi_realize(DeviceState *dev, Error **err) > AMDVIState *s = AMD_IOMMU_DEVICE(dev); > X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); > MachineState *ms = MACHINE(qdev_get_machine()); > - MachineClass *mc = MACHINE_GET_CLASS(ms); > PCMachineState *pcms = > PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); > - PCIBus *bus; > + PCIBus *bus = pcms->bus; > > - if (!pcms) { > - error_setg(err, "Machine-type '%s' not supported by amd-iommu", > - mc->name); > - return; > - } > - > - bus = pcms->bus; > s->iotlb = g_hash_table_new_full(amdvi_uint64_hash, > amdvi_uint64_equal, g_free, g_free); > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 3a5bb0b..aa01812 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -3027,20 +3027,12 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) > static void vtd_realize(DeviceState *dev, Error **errp) > { > MachineState *ms = MACHINE(qdev_get_machine()); > - MachineClass *mc = MACHINE_GET_CLASS(ms); > PCMachineState *pcms = > PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); > - PCIBus *bus; > + PCIBus *bus = pcms->bus; > IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); > X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); > > - if (!pcms) { > - error_setg(errp, "Machine-type '%s' not supported by intel-iommu", > - mc->name); > - return; > - } > - > - bus = pcms->bus; > x86_iommu->type = TYPE_INTEL; > > if (!vtd_decide_config(s, errp)) { > diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c > index 293caf8..4d17e1f 100644 > --- a/hw/i386/x86-iommu.c > +++ b/hw/i386/x86-iommu.c > @@ -21,6 +21,8 @@ > #include "hw/sysbus.h" > #include "hw/boards.h" > #include "hw/i386/x86-iommu.h" > +#include "hw/i386/pc.h" > +#include "qapi/error.h" > #include "qemu/error-report.h" > #include "trace.h" > > @@ -80,7 +82,18 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp) > { > X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); > X86IOMMUClass *x86_class = X86_IOMMU_GET_CLASS(dev); > + MachineState *ms = MACHINE(qdev_get_machine()); > + MachineClass *mc = MACHINE_GET_CLASS(ms); > + PCMachineState *pcms = > + PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); > QLIST_INIT(&x86_iommu->iec_notifiers); > + > + if (!pcms) { > + error_setg(errp, "Machine-type '%s' not supported by iommu", When moving it, maybe also fix the English? :) "Machine-type '%s' does not support IOMMU." Otherwise: Reviewed-by: Peter Xu <peterx@redhat.com> Thanks, > + mc->name); > + return; > + } > + > if (x86_class->realize) { > x86_class->realize(dev, errp); > } > -- > 1.8.3.1 > -- Peter Xu ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] x86_iommu: Move machine check to x86_iommu_realize() 2017-09-15 13:52 ` Peter Xu @ 2017-09-15 13:55 ` Peter Xu 0 siblings, 0 replies; 6+ messages in thread From: Peter Xu @ 2017-09-15 13:55 UTC (permalink / raw) To: Mohammed Gamal; +Cc: qemu-devel, ehabkost, mst, thuth On Fri, Sep 15, 2017 at 09:52:23PM +0800, Peter Xu wrote: > On Fri, Sep 15, 2017 at 01:13:45PM +0200, Mohammed Gamal wrote: > > Instead of having the same error checks in vtd_realize() > > and amdvi_realize(), move that over to the generic > > x86_iommu_realize(). > > > > Signed-off-by: Mohammed Gamal <mgamal@redhat.com> > > --- > > hw/i386/amd_iommu.c | 10 +--------- > > hw/i386/intel_iommu.c | 10 +--------- > > hw/i386/x86-iommu.c | 13 +++++++++++++ > > 3 files changed, 15 insertions(+), 18 deletions(-) > > > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c > > index 334938a..839f01f 100644 > > --- a/hw/i386/amd_iommu.c > > +++ b/hw/i386/amd_iommu.c > > @@ -1141,18 +1141,10 @@ static void amdvi_realize(DeviceState *dev, Error **err) > > AMDVIState *s = AMD_IOMMU_DEVICE(dev); > > X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); > > MachineState *ms = MACHINE(qdev_get_machine()); > > - MachineClass *mc = MACHINE_GET_CLASS(ms); > > PCMachineState *pcms = > > PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); > > - PCIBus *bus; > > + PCIBus *bus = pcms->bus; > > > > - if (!pcms) { > > - error_setg(err, "Machine-type '%s' not supported by amd-iommu", > > - mc->name); > > - return; > > - } > > - > > - bus = pcms->bus; > > s->iotlb = g_hash_table_new_full(amdvi_uint64_hash, > > amdvi_uint64_equal, g_free, g_free); > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index 3a5bb0b..aa01812 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -3027,20 +3027,12 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) > > static void vtd_realize(DeviceState *dev, Error **errp) > > { > > MachineState *ms = MACHINE(qdev_get_machine()); > > - MachineClass *mc = MACHINE_GET_CLASS(ms); > > PCMachineState *pcms = > > PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); > > - PCIBus *bus; > > + PCIBus *bus = pcms->bus; > > IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); > > X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); > > > > - if (!pcms) { > > - error_setg(errp, "Machine-type '%s' not supported by intel-iommu", > > - mc->name); > > - return; > > - } > > - > > - bus = pcms->bus; > > x86_iommu->type = TYPE_INTEL; > > > > if (!vtd_decide_config(s, errp)) { > > diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c > > index 293caf8..4d17e1f 100644 > > --- a/hw/i386/x86-iommu.c > > +++ b/hw/i386/x86-iommu.c > > @@ -21,6 +21,8 @@ > > #include "hw/sysbus.h" > > #include "hw/boards.h" > > #include "hw/i386/x86-iommu.h" > > +#include "hw/i386/pc.h" > > +#include "qapi/error.h" > > #include "qemu/error-report.h" > > #include "trace.h" > > > > @@ -80,7 +82,18 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp) > > { > > X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); > > X86IOMMUClass *x86_class = X86_IOMMU_GET_CLASS(dev); > > + MachineState *ms = MACHINE(qdev_get_machine()); > > + MachineClass *mc = MACHINE_GET_CLASS(ms); > > + PCMachineState *pcms = > > + PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); > > QLIST_INIT(&x86_iommu->iec_notifiers); > > + > > + if (!pcms) { > > + error_setg(errp, "Machine-type '%s' not supported by iommu", > > When moving it, maybe also fix the English? :) > > "Machine-type '%s' does not support IOMMU." I was wrong. I changed the correct thing into error. E.g., ppc64 obviously supports IOMMU... Please ignore my comment and just pick my r-b. Sorry for the noise. > > Otherwise: > > Reviewed-by: Peter Xu <peterx@redhat.com> > > Thanks, > > > + mc->name); > > + return; > > + } > > + > > if (x86_class->realize) { > > x86_class->realize(dev, errp); > > } > > -- > > 1.8.3.1 > > > > -- > Peter Xu -- Peter Xu ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] x86_iommu: check if machine has PCI bus 2017-09-15 11:13 [Qemu-devel] [PATCH 0/2] x86_iommu: Fix segfault when starting on non-PCI machines Mohammed Gamal 2017-09-15 11:13 ` [Qemu-devel] [PATCH 1/2] x86_iommu: Move machine check to x86_iommu_realize() Mohammed Gamal @ 2017-09-15 11:13 ` Mohammed Gamal 2017-09-15 13:52 ` Peter Xu 1 sibling, 1 reply; 6+ messages in thread From: Mohammed Gamal @ 2017-09-15 11:13 UTC (permalink / raw) To: qemu-devel; +Cc: ehabkost, mst, thuth, peterx, Mohammed Gamal Starting qemu with qemu-system-x86_64 -S -M isapc -device {amd|intel}-iommu leads to a segfault. The code assume PCI bus is present and tries to access the bus structure without checking. Since Intel VT-d and AMDVI should only work with PCI, add a check for PCI bus and return error if not present. Signed-off-by: Mohammed Gamal <mgamal@redhat.com> --- hw/i386/x86-iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c index 4d17e1f..afd8cd9 100644 --- a/hw/i386/x86-iommu.c +++ b/hw/i386/x86-iommu.c @@ -88,7 +88,7 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp) PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); QLIST_INIT(&x86_iommu->iec_notifiers); - if (!pcms) { + if (!pcms || !pcms->bus) { error_setg(errp, "Machine-type '%s' not supported by iommu", mc->name); return; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] x86_iommu: check if machine has PCI bus 2017-09-15 11:13 ` [Qemu-devel] [PATCH 2/2] x86_iommu: check if machine has PCI bus Mohammed Gamal @ 2017-09-15 13:52 ` Peter Xu 0 siblings, 0 replies; 6+ messages in thread From: Peter Xu @ 2017-09-15 13:52 UTC (permalink / raw) To: Mohammed Gamal; +Cc: qemu-devel, ehabkost, mst, thuth On Fri, Sep 15, 2017 at 01:13:46PM +0200, Mohammed Gamal wrote: > Starting qemu with > qemu-system-x86_64 -S -M isapc -device {amd|intel}-iommu > leads to a segfault. The code assume PCI bus is present and > tries to access the bus structure without checking. > > Since Intel VT-d and AMDVI should only work with PCI, add a > check for PCI bus and return error if not present. > > Signed-off-by: Mohammed Gamal <mgamal@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com> > --- > hw/i386/x86-iommu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c > index 4d17e1f..afd8cd9 100644 > --- a/hw/i386/x86-iommu.c > +++ b/hw/i386/x86-iommu.c > @@ -88,7 +88,7 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp) > PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE)); > QLIST_INIT(&x86_iommu->iec_notifiers); > > - if (!pcms) { > + if (!pcms || !pcms->bus) { > error_setg(errp, "Machine-type '%s' not supported by iommu", > mc->name); > return; > -- > 1.8.3.1 > -- Peter Xu ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-15 13:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-15 11:13 [Qemu-devel] [PATCH 0/2] x86_iommu: Fix segfault when starting on non-PCI machines Mohammed Gamal 2017-09-15 11:13 ` [Qemu-devel] [PATCH 1/2] x86_iommu: Move machine check to x86_iommu_realize() Mohammed Gamal 2017-09-15 13:52 ` Peter Xu 2017-09-15 13:55 ` Peter Xu 2017-09-15 11:13 ` [Qemu-devel] [PATCH 2/2] x86_iommu: check if machine has PCI bus Mohammed Gamal 2017-09-15 13:52 ` Peter Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).