Message ID | 1423064635-19045-2-git-send-email-marcel@redhat.com |
---|---|
State | New |
Headers | show |
Marcel Apfelbaum <marcel@redhat.com> writes: > Fixes a QEMU crash when passing iommu parameter in command line. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > hw/core/machine.c | 5 +++++ > hw/pci-host/q35.c | 2 +- > include/hw/boards.h | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index fbd91be..096eb10 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine) > return machine->usb; > } > > +bool machine_iommu(MachineState *machine) > +{ > + return machine->iommu; > +} > + > static const TypeInfo machine_info = { > .name = TYPE_MACHINE, > .parent = TYPE_OBJECT, What does this buy us over a straight current_machine->iommu? Same for the other wrapper functions. [...]
On 02/04/2015 06:47 PM, Markus Armbruster wrote: > Marcel Apfelbaum <marcel@redhat.com> writes: > >> Fixes a QEMU crash when passing iommu parameter in command line. >> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> hw/core/machine.c | 5 +++++ >> hw/pci-host/q35.c | 2 +- >> include/hw/boards.h | 1 + >> 3 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index fbd91be..096eb10 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine) >> return machine->usb; >> } >> >> +bool machine_iommu(MachineState *machine) >> +{ >> + return machine->iommu; >> +} >> + >> static const TypeInfo machine_info = { >> .name = TYPE_MACHINE, >> .parent = TYPE_OBJECT, > > What does this buy us over a straight current_machine->iommu? Following QOM guidelines/conventions all object fields are private to machine files only. The *only* ways that they can be exposed are: 1. A wrapper 2. object_property_get... I chose the wrapper because the other variant would be really ugly and should be used only on a generic code. This is the real reason I used this, but pure theoretical speaking using wrappers will give us the opportunity to change machine_iommu implementation without the need to change the call sites. To wrap it up, I just followed previous comments I received on QOM handling. Thanks, Marcel > > Same for the other wrapper functions. > > [...] >
Marcel Apfelbaum <marcel@redhat.com> writes: > On 02/04/2015 06:47 PM, Markus Armbruster wrote: >> Marcel Apfelbaum <marcel@redhat.com> writes: >> >>> Fixes a QEMU crash when passing iommu parameter in command line. >>> >>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>> --- >>> hw/core/machine.c | 5 +++++ >>> hw/pci-host/q35.c | 2 +- >>> include/hw/boards.h | 1 + >>> 3 files changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/core/machine.c b/hw/core/machine.c >>> index fbd91be..096eb10 100644 >>> --- a/hw/core/machine.c >>> +++ b/hw/core/machine.c >>> @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine) >>> return machine->usb; >>> } >>> >>> +bool machine_iommu(MachineState *machine) >>> +{ >>> + return machine->iommu; >>> +} >>> + >>> static const TypeInfo machine_info = { >>> .name = TYPE_MACHINE, >>> .parent = TYPE_OBJECT, >> >> What does this buy us over a straight current_machine->iommu? > Following QOM guidelines/conventions all object fields are private > to machine files only. The *only* ways that they can be exposed are: > 1. A wrapper > 2. object_property_get... > > I chose the wrapper because the other variant would be really > ugly and should be used only on a generic code. > > This is the real reason I used this, but pure theoretical speaking > using wrappers will give us the opportunity to change machine_iommu > implementation without the need to change the call sites. So we pay the notational and cognitive overhead of wrappers now and going forward to maybe some day save us fixing up a couple of machine->iommu instances. A theory that makes sense for interfacing complex machinery with non-trivial implementation details becomes silly when applied to a couple of straightforward configuration flags. As Doug McIlroy observed, "KISS became MICAHI: make it complicated and hide it." > To wrap it up, I just followed previous comments I received on QOM handling. In other words, it's somebody else's fault. I'll shut up now ;) [...]
On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote: > Fixes a QEMU crash when passing iommu parameter in command line. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> Please amend commit message: Running x86_64-softmmu/qemu-system-x86_64 -machine pc,iommu=on -enable-kvm leads to crash: qemu-system-x86_64: qemu/util/qemu-option.c:387: qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed. Aborted (core dumped) This happens because the commit e79d5a6 ("machine: remove qemu_machine_opts global list") removed the global option descriptions and moved them to MachineState's QOM properties. Fix this by querying machine properties through designated wrappers. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > hw/core/machine.c | 5 +++++ > hw/pci-host/q35.c | 2 +- > include/hw/boards.h | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index fbd91be..096eb10 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine) > return machine->usb; > } > > +bool machine_iommu(MachineState *machine) > +{ > + return machine->iommu; > +} > + > static const TypeInfo machine_info = { > .name = TYPE_MACHINE, > .parent = TYPE_OBJECT, > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index b20bad8..dfd8bbf 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -415,7 +415,7 @@ static int mch_init(PCIDevice *d) > PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE); > } > /* Intel IOMMU (VT-d) */ > - if (qemu_opt_get_bool(qemu_get_machine_opts(), "iommu", false)) { > + if (machine_iommu(current_machine)) { > mch_init_dmar(mch); > } > return 0; > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 3ddc449..a12f041 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -66,6 +66,7 @@ MachineClass *find_default_machine(void); > extern MachineState *current_machine; > > bool machine_usb(MachineState *machine); > +bool machine_iommu(MachineState *machine); > > /** > * MachineClass: >
diff --git a/hw/core/machine.c b/hw/core/machine.c index fbd91be..096eb10 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine) return machine->usb; } +bool machine_iommu(MachineState *machine) +{ + return machine->iommu; +} + static const TypeInfo machine_info = { .name = TYPE_MACHINE, .parent = TYPE_OBJECT, diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index b20bad8..dfd8bbf 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -415,7 +415,7 @@ static int mch_init(PCIDevice *d) PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE); } /* Intel IOMMU (VT-d) */ - if (qemu_opt_get_bool(qemu_get_machine_opts(), "iommu", false)) { + if (machine_iommu(current_machine)) { mch_init_dmar(mch); } return 0; diff --git a/include/hw/boards.h b/include/hw/boards.h index 3ddc449..a12f041 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -66,6 +66,7 @@ MachineClass *find_default_machine(void); extern MachineState *current_machine; bool machine_usb(MachineState *machine); +bool machine_iommu(MachineState *machine); /** * MachineClass:
Fixes a QEMU crash when passing iommu parameter in command line. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- hw/core/machine.c | 5 +++++ hw/pci-host/q35.c | 2 +- include/hw/boards.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-)