* [Qemu-devel] [PATCH v3 0/2] x86_iommu: Fix segfault when starting on non-PCI machines
@ 2017-09-18 10:14 Mohammed Gamal
2017-09-18 10:14 ` [Qemu-devel] [PATCH v3 1/2] x86_iommu: Move machine check to x86_iommu_realize() Mohammed Gamal
2017-09-18 10:14 ` [Qemu-devel] [PATCH v3 2/2] x86_iommu: check if machine has PCI bus Mohammed Gamal
0 siblings, 2 replies; 5+ messages in thread
From: Mohammed Gamal @ 2017-09-18 10:14 UTC (permalink / raw)
To: qemu-devel; +Cc: ehabkost, mst, thuth, peterx, pbonzini, 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.
v2 --> v3:
* Use PC_MACHINE macro directly
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 | 12 ++++++++++++
3 files changed, 14 insertions(+), 18 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread* [Qemu-devel] [PATCH v3 1/2] x86_iommu: Move machine check to x86_iommu_realize() 2017-09-18 10:14 [Qemu-devel] [PATCH v3 0/2] x86_iommu: Fix segfault when starting on non-PCI machines Mohammed Gamal @ 2017-09-18 10:14 ` Mohammed Gamal 2017-09-18 13:13 ` Eduardo Habkost 2017-09-18 10:14 ` [Qemu-devel] [PATCH v3 2/2] x86_iommu: check if machine has PCI bus Mohammed Gamal 1 sibling, 1 reply; 5+ messages in thread From: Mohammed Gamal @ 2017-09-18 10:14 UTC (permalink / raw) To: qemu-devel; +Cc: ehabkost, mst, thuth, peterx, pbonzini, 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 | 12 ++++++++++++ 3 files changed, 14 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..d43b08a 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,17 @@ 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(ms); 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] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] x86_iommu: Move machine check to x86_iommu_realize() 2017-09-18 10:14 ` [Qemu-devel] [PATCH v3 1/2] x86_iommu: Move machine check to x86_iommu_realize() Mohammed Gamal @ 2017-09-18 13:13 ` Eduardo Habkost 2017-09-18 13:47 ` Mohammed Gamal 0 siblings, 1 reply; 5+ messages in thread From: Eduardo Habkost @ 2017-09-18 13:13 UTC (permalink / raw) To: Mohammed Gamal; +Cc: qemu-devel, mst, thuth, peterx, pbonzini On Mon, Sep 18, 2017 at 12:14:08PM +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 | 12 ++++++++++++ > 3 files changed, 14 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)); This is where you shouldn't use object_dynamic_cast() because the code now expects pcms to never be NULL... > - 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)); Same as above. > - 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..d43b08a 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,17 @@ 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(ms); And here is where you really need object_dynamic_cast(), otherwise the "if (!pcms)" check below will be useless and you'll get this: $ qemu-system-x86_64 -machine none -device intel-iommu qemu/hw/i386/x86-iommu.c:87:x86_iommu_realize: Object 0x53d7d02060 is not an instance of type generic-pc-machine Aborted (core dumped) > 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 > -- Eduardo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] x86_iommu: Move machine check to x86_iommu_realize() 2017-09-18 13:13 ` Eduardo Habkost @ 2017-09-18 13:47 ` Mohammed Gamal 0 siblings, 0 replies; 5+ messages in thread From: Mohammed Gamal @ 2017-09-18 13:47 UTC (permalink / raw) To: Eduardo Habkost; +Cc: pbonzini, thuth, qemu-devel, peterx, mst On Mon, 2017-09-18 at 10:13 -0300, Eduardo Habkost wrote: > On Mon, Sep 18, 2017 at 12:14:08PM +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 | 12 ++++++++++++ > > 3 files changed, 14 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)); > > This is where you shouldn't use object_dynamic_cast() because the > code now expects pcms to never be NULL... > > > - 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)); > > Same as above. > > > - 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..d43b08a 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,17 @@ 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(ms); > > And here is where you really need object_dynamic_cast(), > otherwise the "if (!pcms)" check below will be useless and you'll > get this: > > $ qemu-system-x86_64 -machine none -device intel-iommu > qemu/hw/i386/x86-iommu.c:87:x86_iommu_realize: Object 0x53d7d02060 is not an instance of type generic-pc-machine > Aborted (core dumped) This is what happens when you miss your daily Caffeine dose :) I thought the object_dynamic_cast call was redundant and thus needs to be removed when moving the code. I see the correct sequence you mean now. Since the check is done in the calling x86_iommu_realize(), the check won't be need in the callees. I will send the fixed version shortly. > > > 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 [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH v3 2/2] x86_iommu: check if machine has PCI bus 2017-09-18 10:14 [Qemu-devel] [PATCH v3 0/2] x86_iommu: Fix segfault when starting on non-PCI machines Mohammed Gamal 2017-09-18 10:14 ` [Qemu-devel] [PATCH v3 1/2] x86_iommu: Move machine check to x86_iommu_realize() Mohammed Gamal @ 2017-09-18 10:14 ` Mohammed Gamal 1 sibling, 0 replies; 5+ messages in thread From: Mohammed Gamal @ 2017-09-18 10:14 UTC (permalink / raw) To: qemu-devel; +Cc: ehabkost, mst, thuth, peterx, pbonzini, 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 d43b08a..00f70bb 100644 --- a/hw/i386/x86-iommu.c +++ b/hw/i386/x86-iommu.c @@ -87,7 +87,7 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp) PCMachineState *pcms = PC_MACHINE(ms); 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] 5+ messages in thread
end of thread, other threads:[~2017-09-18 13:48 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-18 10:14 [Qemu-devel] [PATCH v3 0/2] x86_iommu: Fix segfault when starting on non-PCI machines Mohammed Gamal 2017-09-18 10:14 ` [Qemu-devel] [PATCH v3 1/2] x86_iommu: Move machine check to x86_iommu_realize() Mohammed Gamal 2017-09-18 13:13 ` Eduardo Habkost 2017-09-18 13:47 ` Mohammed Gamal 2017-09-18 10:14 ` [Qemu-devel] [PATCH v3 2/2] x86_iommu: check if machine has PCI bus Mohammed Gamal
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).