Message ID | 1431322734-12520-1-git-send-email-mrezanin@redhat.com |
---|---|
State | New |
Headers | show |
mrezanin@redhat.com writes: > From: Miroslav Rezanina <mrezanin@redhat.com> > > Disabling CONFIG_PARALLEL cause build failure as commit 07dc788 factored > out initialization to parallel_hds_isa_init function in hw/char/parallel.c > that is not build. > > Stub file is added to be able to disable CONFIG_PARALLEL. This file is used > in targets using parallel_hds_isa_init and provide empty definition of this > function. > > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> > > --- > hw/i386/Makefile.objs | 1 + > hw/mips/Makefile.objs | 2 ++ > hw/sparc64/Makefile.objs | 2 ++ > stubs/parallel-stub.c | 7 +++++++ Nitpick: the existing stub/*.c naming practice suggests stubs/parallel.c. > 4 files changed, 12 insertions(+) > create mode 100644 stubs/parallel-stub.c > > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs > index e058a39..2b7131a 100644 > --- a/hw/i386/Makefile.objs > +++ b/hw/i386/Makefile.objs > @@ -4,6 +4,7 @@ obj-y += pc.o pc_piix.o pc_q35.o > obj-y += pc_sysfw.o > obj-y += intel_iommu.o > obj-$(CONFIG_XEN) += ../xenpv/ xen/ > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > > obj-y += kvmvapic.o > obj-y += acpi-build.o Can we rely on the linker to pull parallel-stub.o from a suitable .a libqemustub.a when needed? > diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs > index 0a652f8..2e65305 100644 > --- a/hw/mips/Makefile.objs > +++ b/hw/mips/Makefile.objs > @@ -2,3 +2,5 @@ obj-y += mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o > obj-y += addr.o cputimer.o mips_int.o > obj-$(CONFIG_FULONG) += mips_fulong2e.o > obj-y += gt64xxx_pci.o > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > + > diff --git a/hw/sparc64/Makefile.objs b/hw/sparc64/Makefile.objs > index a84cfe3..7696611 100644 > --- a/hw/sparc64/Makefile.objs > +++ b/hw/sparc64/Makefile.objs > @@ -1 +1,3 @@ > obj-y += sun4u.o > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > + > diff --git a/stubs/parallel-stub.c b/stubs/parallel-stub.c > new file mode 100644 > index 0000000..949c1b2 > --- /dev/null > +++ b/stubs/parallel-stub.c > @@ -0,0 +1,7 @@ > +#include "qemu/typedefs.h" > +#include "hw/isa/isa.h" > +#include "hw/i386/pc.h" > + > +void parallel_hds_isa_init(ISABus *bus, int n) > +{ > +}
On Mon, May 11, 2015 at 08:46:19AM +0200, Markus Armbruster wrote: > mrezanin@redhat.com writes: > > > From: Miroslav Rezanina <mrezanin@redhat.com> > > > > Disabling CONFIG_PARALLEL cause build failure as commit 07dc788 factored > > out initialization to parallel_hds_isa_init function in hw/char/parallel.c > > that is not build. > > > > Stub file is added to be able to disable CONFIG_PARALLEL. This file is used > > in targets using parallel_hds_isa_init and provide empty definition of this > > function. > > > > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> > > > > --- > > hw/i386/Makefile.objs | 1 + > > hw/mips/Makefile.objs | 2 ++ > > hw/sparc64/Makefile.objs | 2 ++ > > stubs/parallel-stub.c | 7 +++++++ > > Nitpick: the existing stub/*.c naming practice suggests > stubs/parallel.c. Yeah...I forget to rename it after moving from repository root to stub directory. I originally have it in repository root as it is not included in libqemustub. So the naming can be treated as hint that something is different. However, I can rename it to follow stubs/* naming. > > > 4 files changed, 12 insertions(+) > > create mode 100644 stubs/parallel-stub.c > > > > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs > > index e058a39..2b7131a 100644 > > --- a/hw/i386/Makefile.objs > > +++ b/hw/i386/Makefile.objs > > @@ -4,6 +4,7 @@ obj-y += pc.o pc_piix.o pc_q35.o > > obj-y += pc_sysfw.o > > obj-y += intel_iommu.o > > obj-$(CONFIG_XEN) += ../xenpv/ xen/ > > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > > > > obj-y += kvmvapic.o > > obj-y += acpi-build.o > > Can we rely on the linker to pull parallel-stub.o from a suitable .a > libqemustub.a when needed? We do not have to as parallel-stub.o is not included in libqemustub.a. It is linked directly in case CONFIG_PARALLEL is not defined (for targets using parallel_hds_isa_init). Mirek > > > diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs > > index 0a652f8..2e65305 100644 > > --- a/hw/mips/Makefile.objs > > +++ b/hw/mips/Makefile.objs > > @@ -2,3 +2,5 @@ obj-y += mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o > > obj-y += addr.o cputimer.o mips_int.o > > obj-$(CONFIG_FULONG) += mips_fulong2e.o > > obj-y += gt64xxx_pci.o > > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > > + > > diff --git a/hw/sparc64/Makefile.objs b/hw/sparc64/Makefile.objs > > index a84cfe3..7696611 100644 > > --- a/hw/sparc64/Makefile.objs > > +++ b/hw/sparc64/Makefile.objs > > @@ -1 +1,3 @@ > > obj-y += sun4u.o > > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > > + > > diff --git a/stubs/parallel-stub.c b/stubs/parallel-stub.c > > new file mode 100644 > > index 0000000..949c1b2 > > --- /dev/null > > +++ b/stubs/parallel-stub.c > > @@ -0,0 +1,7 @@ > > +#include "qemu/typedefs.h" > > +#include "hw/isa/isa.h" > > +#include "hw/i386/pc.h" > > + > > +void parallel_hds_isa_init(ISABus *bus, int n) > > +{ > > +}
On 11/05/2015 07:38, mrezanin@redhat.com wrote: > From: Miroslav Rezanina <mrezanin@redhat.com> > > Disabling CONFIG_PARALLEL cause build failure as commit 07dc788 factored > out initialization to parallel_hds_isa_init function in hw/char/parallel.c > that is not build. > > Stub file is added to be able to disable CONFIG_PARALLEL. This file is used > in targets using parallel_hds_isa_init and provide empty definition of this > function. > > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> This patch will make "-parallel" a nop. The right thing to do is to fail startup whenever -parallel is passed and CONFIG_PARALLEL is disabled. You can move parallel_hds_isa_init and parallel_init to hw/isa/isa-bus.c, or to a new file hw/isa/isa-devices.c. Paolo > --- > hw/i386/Makefile.objs | 1 + > hw/mips/Makefile.objs | 2 ++ > hw/sparc64/Makefile.objs | 2 ++ > stubs/parallel-stub.c | 7 +++++++ > 4 files changed, 12 insertions(+) > create mode 100644 stubs/parallel-stub.c > > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs > index e058a39..2b7131a 100644 > --- a/hw/i386/Makefile.objs > +++ b/hw/i386/Makefile.objs > @@ -4,6 +4,7 @@ obj-y += pc.o pc_piix.o pc_q35.o > obj-y += pc_sysfw.o > obj-y += intel_iommu.o > obj-$(CONFIG_XEN) += ../xenpv/ xen/ > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > > obj-y += kvmvapic.o > obj-y += acpi-build.o > diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs > index 0a652f8..2e65305 100644 > --- a/hw/mips/Makefile.objs > +++ b/hw/mips/Makefile.objs > @@ -2,3 +2,5 @@ obj-y += mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o > obj-y += addr.o cputimer.o mips_int.o > obj-$(CONFIG_FULONG) += mips_fulong2e.o > obj-y += gt64xxx_pci.o > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > + > diff --git a/hw/sparc64/Makefile.objs b/hw/sparc64/Makefile.objs > index a84cfe3..7696611 100644 > --- a/hw/sparc64/Makefile.objs > +++ b/hw/sparc64/Makefile.objs > @@ -1 +1,3 @@ > obj-y += sun4u.o > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > + > diff --git a/stubs/parallel-stub.c b/stubs/parallel-stub.c > new file mode 100644 > index 0000000..949c1b2 > --- /dev/null > +++ b/stubs/parallel-stub.c > @@ -0,0 +1,7 @@ > +#include "qemu/typedefs.h" > +#include "hw/isa/isa.h" > +#include "hw/i386/pc.h" > + > +void parallel_hds_isa_init(ISABus *bus, int n) > +{ > +} >
On Mon, May 11, 2015 at 10:40:04AM +0200, Paolo Bonzini wrote: > > > On 11/05/2015 07:38, mrezanin@redhat.com wrote: > > From: Miroslav Rezanina <mrezanin@redhat.com> > > > > Disabling CONFIG_PARALLEL cause build failure as commit 07dc788 factored > > out initialization to parallel_hds_isa_init function in hw/char/parallel.c > > that is not build. > > > > Stub file is added to be able to disable CONFIG_PARALLEL. This file is used > > in targets using parallel_hds_isa_init and provide empty definition of this > > function. > > > > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> > > This patch will make "-parallel" a nop. The right thing to do is to > fail startup whenever -parallel is passed and CONFIG_PARALLEL is disabled. > This was original behavior before 07dc788. Intention of this patch is to make qemu buildable with CONFIG_PARALLEL disabled. > You can move parallel_hds_isa_init and parallel_init to > hw/isa/isa-bus.c, or to a new file hw/isa/isa-devices.c. > Moving functions will cause abort with "Unknown device" error. > Paolo > > > --- > > hw/i386/Makefile.objs | 1 + > > hw/mips/Makefile.objs | 2 ++ > > hw/sparc64/Makefile.objs | 2 ++ > > stubs/parallel-stub.c | 7 +++++++ > > 4 files changed, 12 insertions(+) > > create mode 100644 stubs/parallel-stub.c > > > > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs > > index e058a39..2b7131a 100644 > > --- a/hw/i386/Makefile.objs > > +++ b/hw/i386/Makefile.objs > > @@ -4,6 +4,7 @@ obj-y += pc.o pc_piix.o pc_q35.o > > obj-y += pc_sysfw.o > > obj-y += intel_iommu.o > > obj-$(CONFIG_XEN) += ../xenpv/ xen/ > > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > > > > obj-y += kvmvapic.o > > obj-y += acpi-build.o > > diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs > > index 0a652f8..2e65305 100644 > > --- a/hw/mips/Makefile.objs > > +++ b/hw/mips/Makefile.objs > > @@ -2,3 +2,5 @@ obj-y += mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o > > obj-y += addr.o cputimer.o mips_int.o > > obj-$(CONFIG_FULONG) += mips_fulong2e.o > > obj-y += gt64xxx_pci.o > > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > > + > > diff --git a/hw/sparc64/Makefile.objs b/hw/sparc64/Makefile.objs > > index a84cfe3..7696611 100644 > > --- a/hw/sparc64/Makefile.objs > > +++ b/hw/sparc64/Makefile.objs > > @@ -1 +1,3 @@ > > obj-y += sun4u.o > > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > > + > > diff --git a/stubs/parallel-stub.c b/stubs/parallel-stub.c > > new file mode 100644 > > index 0000000..949c1b2 > > --- /dev/null > > +++ b/stubs/parallel-stub.c > > @@ -0,0 +1,7 @@ > > +#include "qemu/typedefs.h" > > +#include "hw/isa/isa.h" > > +#include "hw/i386/pc.h" > > + > > +void parallel_hds_isa_init(ISABus *bus, int n) > > +{ > > +} > >
On 11/05/2015 11:36, Miroslav Rezanina wrote: >> > This patch will make "-parallel" a nop. The right thing to do is to >> > fail startup whenever -parallel is passed and CONFIG_PARALLEL is disabled. >> > > This was original behavior before 07dc788. Intention of this patch is to > make qemu buildable with CONFIG_PARALLEL disabled. Understood, but in the meanwhile Markus wrote commit 4bc6a3e (parallel: parallel_hds_isa_init() shouldn't fail, 2015-02-04), and you should preserve the logic of that commit. >> > You can move parallel_hds_isa_init and parallel_init to >> > hw/isa/isa-bus.c, or to a new file hw/isa/isa-devices.c. >> > > Moving functions will cause abort with "Unknown device" error. This is the right behavior that we want: exit QEMU, not go on silently without the parallel port. If you do not like the abort, you should revert commit 4bc6a3e, and make parallel_hds_isa_init check for failure of parallel_init. But for me it's okay to just let it abort. Paolo
mrezanin@redhat.com writes: > From: Miroslav Rezanina <mrezanin@redhat.com> > > Disabling CONFIG_PARALLEL cause build failure as commit 07dc788 factored > out initialization to parallel_hds_isa_init function in hw/char/parallel.c > that is not build. > > Stub file is added to be able to disable CONFIG_PARALLEL. This file is used > in targets using parallel_hds_isa_init and provide empty definition of this > function. > > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> > > --- > hw/i386/Makefile.objs | 1 + > hw/mips/Makefile.objs | 2 ++ > hw/sparc64/Makefile.objs | 2 ++ > stubs/parallel-stub.c | 7 +++++++ > 4 files changed, 12 insertions(+) > create mode 100644 stubs/parallel-stub.c > > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs > index e058a39..2b7131a 100644 > --- a/hw/i386/Makefile.objs > +++ b/hw/i386/Makefile.objs > @@ -4,6 +4,7 @@ obj-y += pc.o pc_piix.o pc_q35.o > obj-y += pc_sysfw.o > obj-y += intel_iommu.o > obj-$(CONFIG_XEN) += ../xenpv/ xen/ > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > > obj-y += kvmvapic.o > obj-y += acpi-build.o > diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs > index 0a652f8..2e65305 100644 > --- a/hw/mips/Makefile.objs > +++ b/hw/mips/Makefile.objs > @@ -2,3 +2,5 @@ obj-y += mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o > obj-y += addr.o cputimer.o mips_int.o > obj-$(CONFIG_FULONG) += mips_fulong2e.o > obj-y += gt64xxx_pci.o > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > + git-am complains "new blank line at EOF." > diff --git a/hw/sparc64/Makefile.objs b/hw/sparc64/Makefile.objs > index a84cfe3..7696611 100644 > --- a/hw/sparc64/Makefile.objs > +++ b/hw/sparc64/Makefile.objs > @@ -1 +1,3 @@ > obj-y += sun4u.o > +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o > + Likewise. > diff --git a/stubs/parallel-stub.c b/stubs/parallel-stub.c > new file mode 100644 > index 0000000..949c1b2 > --- /dev/null > +++ b/stubs/parallel-stub.c > @@ -0,0 +1,7 @@ > +#include "qemu/typedefs.h" > +#include "hw/isa/isa.h" > +#include "hw/i386/pc.h" > + > +void parallel_hds_isa_init(ISABus *bus, int n) > +{ > +} Fails to link if I disable CONFIG_PARALLEL in default-configs/mips-softmmu.mak: LINK mips-softmmu/qemu-system-mips hw/mips/mips_jazz.o: In function `mips_jazz_init': /home/armbru/work/qemu/hw/mips/mips_jazz.c:323: undefined reference to `parallel_mm_init' collect2: error: ld returned 1 exit status make[1]: *** [qemu-system-mips] Error 1 To fix that, you'd need to stub out parallel_mm_init(), too.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 11/05/2015 11:36, Miroslav Rezanina wrote: >>> > This patch will make "-parallel" a nop. The right thing to do is to >>> > fail startup whenever -parallel is passed and CONFIG_PARALLEL is disabled. >>> > >> This was original behavior before 07dc788. Intention of this patch is to >> make qemu buildable with CONFIG_PARALLEL disabled. > > Understood, but in the meanwhile Markus wrote commit 4bc6a3e (parallel: > parallel_hds_isa_init() shouldn't fail, 2015-02-04), and you should > preserve the logic of that commit. I have to admit didn't consider CONFIG_PARALLEL when I wrote the commit. >>> > You can move parallel_hds_isa_init and parallel_init to >>> > hw/isa/isa-bus.c, or to a new file hw/isa/isa-devices.c. >>> > >> Moving functions will cause abort with "Unknown device" error. > > This is the right behavior that we want: exit QEMU, not go on silently > without the parallel port. I agree silently ignoring command line options isn't nice, but it's unfortunately what QEMU has always done. In particular, -parallel is silently ignored with the vast majority of machine types. The few machine types that implement it silently ignore it only when they fail to create the device. I'm fine with changing -parallel to either create the device or fail. Seems outside the scope of this series, though. > If you do not like the abort, you should revert commit 4bc6a3e, and make > parallel_hds_isa_init check for failure of parallel_init. But for me > it's okay to just let it abort.
On 11/05/2015 17:52, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 11/05/2015 11:36, Miroslav Rezanina wrote: >>>>> This patch will make "-parallel" a nop. The right thing to do is to >>>>> fail startup whenever -parallel is passed and CONFIG_PARALLEL is disabled. >>>>> >>> This was original behavior before 07dc788. Intention of this patch is to >>> make qemu buildable with CONFIG_PARALLEL disabled. >> >> Understood, but in the meanwhile Markus wrote commit 4bc6a3e (parallel: >> parallel_hds_isa_init() shouldn't fail, 2015-02-04), and you should >> preserve the logic of that commit. > > I have to admit didn't consider CONFIG_PARALLEL when I wrote the commit. > >>>>> You can move parallel_hds_isa_init and parallel_init to >>>>> hw/isa/isa-bus.c, or to a new file hw/isa/isa-devices.c. >>>>> >>> Moving functions will cause abort with "Unknown device" error. >> >> This is the right behavior that we want: exit QEMU, not go on silently >> without the parallel port. > > I agree silently ignoring command line options isn't nice, but it's > unfortunately what QEMU has always done. > > In particular, -parallel is silently ignored with the vast majority of > machine types. The few machine types that implement it silently ignore > it only when they fail to create the device. Right. However, if I move a VM (that has a parallel port, which already puts us in a kind of reductio as absurdum) from a QEMU that has parallel ports to a QEMU that doesn't have them, _and the board does something about -parallel_, I think there should be a failure. This is because whoever compiled that QEMU is crippling a board, no matter what their reasons are. > I'm fine with changing -parallel to either create the device or fail. > Seems outside the scope of this series, though. Why? Your patch is _already_ trying to "create the device or fail", even if the failure mode isn't particularly clean. The thing that can be debated is whether to keep the abort or require a nicer check, and I'm not requiring it. Paolo >> If you do not like the abort, you should revert commit 4bc6a3e, and make >> parallel_hds_isa_init check for failure of parallel_init. But for me >> it's okay to just let it abort.
On 11/05/2015 17:09, Markus Armbruster wrote: > Fails to link if I disable CONFIG_PARALLEL in > default-configs/mips-softmmu.mak: > > LINK mips-softmmu/qemu-system-mips > hw/mips/mips_jazz.o: In function `mips_jazz_init': > /home/armbru/work/qemu/hw/mips/mips_jazz.c:323: undefined reference to `parallel_mm_init' > collect2: error: ld returned 1 exit status > make[1]: *** [qemu-system-mips] Error 1 > > To fix that, you'd need to stub out parallel_mm_init(), too. I would ignore that. parallel_mm_init isn't even qdev-ified, and qdevification would fix that problem too. Paolo
On 11 May 2015 at 16:52, Markus Armbruster <armbru@redhat.com> wrote: > I agree silently ignoring command line options isn't nice, but it's > unfortunately what QEMU has always done. Actually, we're inconsistent (who'd have guessed? :-)). For instance if CONFIG_CURSES isn't defined then we print an error if you try to use the curses option, similarly for iscsi, slirp related options, tpmdev, and others. But there are also many options where (as you note) we just silently ignore them, and even a few where we print a warning but continue to boot (eg some of the networking config options). -- PMM
diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs index e058a39..2b7131a 100644 --- a/hw/i386/Makefile.objs +++ b/hw/i386/Makefile.objs @@ -4,6 +4,7 @@ obj-y += pc.o pc_piix.o pc_q35.o obj-y += pc_sysfw.o obj-y += intel_iommu.o obj-$(CONFIG_XEN) += ../xenpv/ xen/ +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o obj-y += kvmvapic.o obj-y += acpi-build.o diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs index 0a652f8..2e65305 100644 --- a/hw/mips/Makefile.objs +++ b/hw/mips/Makefile.objs @@ -2,3 +2,5 @@ obj-y += mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o obj-y += addr.o cputimer.o mips_int.o obj-$(CONFIG_FULONG) += mips_fulong2e.o obj-y += gt64xxx_pci.o +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o + diff --git a/hw/sparc64/Makefile.objs b/hw/sparc64/Makefile.objs index a84cfe3..7696611 100644 --- a/hw/sparc64/Makefile.objs +++ b/hw/sparc64/Makefile.objs @@ -1 +1,3 @@ obj-y += sun4u.o +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o + diff --git a/stubs/parallel-stub.c b/stubs/parallel-stub.c new file mode 100644 index 0000000..949c1b2 --- /dev/null +++ b/stubs/parallel-stub.c @@ -0,0 +1,7 @@ +#include "qemu/typedefs.h" +#include "hw/isa/isa.h" +#include "hw/i386/pc.h" + +void parallel_hds_isa_init(ISABus *bus, int n) +{ +}