* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.