| Message ID | 20210315204226.3481044-6-laurent@vivier.eu |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/5] hw/char: add goldfish-tty | expand |
Hi Laurent, +Paolo / Thomas On 3/15/21 9:42 PM, Laurent Vivier wrote: > The machine is based on Goldfish interfaces defined by Google > for Android simulator. It uses Goldfish-rtc (timer and RTC), > Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty). > > The machine is created with 128 virtio-mmio bus, and they can > be used to use serial console, GPU, disk, NIC, HID, ... > > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Message-Id: <20210312214145.2936082-6-laurent@vivier.eu> > --- > default-configs/devices/m68k-softmmu.mak | 1 + > .../standard-headers/asm-m68k/bootinfo-virt.h | 18 + > hw/m68k/virt.c | 313 ++++++++++++++++++ > MAINTAINERS | 13 + > hw/m68k/Kconfig | 9 + > hw/m68k/meson.build | 1 + > 6 files changed, 355 insertions(+) > create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h > create mode 100644 hw/m68k/virt.c > diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig > index 60d7bcfb8f2b..f839f8a03064 100644 > --- a/hw/m68k/Kconfig > +++ b/hw/m68k/Kconfig > @@ -23,3 +23,12 @@ config Q800 > select ESP > select DP8393X > select OR_IRQ > + > +config M68K_VIRT > + bool > + select M68K_IRQC > + select VIRT_CTRL > + select GOLDFISH_PIC > + select GOLDFISH_TTY > + select GOLDFISH_RTC > + select VIRTIO_MMIO I had this error on gitlab: (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio: 'virtio-blk-pci' is not a valid device model name job: check-system-fedora https://gitlab.com/philmd/qemu/-/jobs/1106469724 I bisected locally to this commit. check-system-fedora uses build-system-fedora: build-system-fedora: CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs --enable-fdt=system --enable-slirp=system --enable-capstone=system I'm confused because the machine provides a VIRTIO bus via MMIO: config VIRTIO_MMIO bool select VIRTIO I remember I tested your machine with virtio-blk-device. config VIRTIO_BLK bool default y depends on VIRTIO Ah, this is virtio-blk-pci, which has: virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk-pci.c')) virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss) And VIRTIO_PCI isn't selected... Are the tests incorrect then? libqos isn't restricted to PCI: tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h" tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */ tests/qtest/libqos/virtio-blk.c:33: if (!g_strcmp0(interface, "virtio-blk")) { tests/qtest/libqos/virtio-blk.c:40: fprintf(stderr, "%s not present in virtio-blk-device\n", interface); tests/qtest/libqos/virtio-blk.c:109: /* virtio-blk-device */ tests/qtest/libqos/virtio-blk.c:111: qos_node_create_driver("virtio-blk-device", virtio_blk_device_create); tests/qtest/libqos/virtio-blk.c:112: qos_node_consumes("virtio-blk-device", "virtio-bus", &opts); tests/qtest/libqos/virtio-blk.c:113: qos_node_produces("virtio-blk-device", "virtio-blk"); But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should use a generic virtio-blk-device instead, hoping it get plugged correctly to the virtio bus...
Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit : > Hi Laurent, > > +Paolo / Thomas > > On 3/15/21 9:42 PM, Laurent Vivier wrote: >> The machine is based on Goldfish interfaces defined by Google >> for Android simulator. It uses Goldfish-rtc (timer and RTC), >> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty). >> >> The machine is created with 128 virtio-mmio bus, and they can >> be used to use serial console, GPU, disk, NIC, HID, ... >> >> Signed-off-by: Laurent Vivier <laurent@vivier.eu> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu> >> --- >> default-configs/devices/m68k-softmmu.mak | 1 + >> .../standard-headers/asm-m68k/bootinfo-virt.h | 18 + >> hw/m68k/virt.c | 313 ++++++++++++++++++ >> MAINTAINERS | 13 + >> hw/m68k/Kconfig | 9 + >> hw/m68k/meson.build | 1 + >> 6 files changed, 355 insertions(+) >> create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h >> create mode 100644 hw/m68k/virt.c > >> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig >> index 60d7bcfb8f2b..f839f8a03064 100644 >> --- a/hw/m68k/Kconfig >> +++ b/hw/m68k/Kconfig >> @@ -23,3 +23,12 @@ config Q800 >> select ESP >> select DP8393X >> select OR_IRQ >> + >> +config M68K_VIRT >> + bool >> + select M68K_IRQC >> + select VIRT_CTRL >> + select GOLDFISH_PIC >> + select GOLDFISH_TTY >> + select GOLDFISH_RTC >> + select VIRTIO_MMIO > > I had this error on gitlab: > > (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio: > 'virtio-blk-pci' is not a valid device model name > job: check-system-fedora > https://gitlab.com/philmd/qemu/-/jobs/1106469724 > > I bisected locally to this commit. > > check-system-fedora uses build-system-fedora: > > build-system-fedora: > CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs > --enable-fdt=system --enable-slirp=system > --enable-capstone=system > > I'm confused because the machine provides a VIRTIO bus > via MMIO: > > config VIRTIO_MMIO > bool > select VIRTIO > > I remember I tested your machine with virtio-blk-device. > > config VIRTIO_BLK > bool > default y > depends on VIRTIO > > Ah, this is virtio-blk-pci, which has: > > virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: > files('virtio-blk-pci.c')) > virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss) > > And VIRTIO_PCI isn't selected... This machine doesn't have virtio-pci, but only virtio-mmio buses. > Are the tests incorrect then? > > libqos isn't restricted to PCI: > > tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h" > tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */ > tests/qtest/libqos/virtio-blk.c:33: if (!g_strcmp0(interface, > "virtio-blk")) { > tests/qtest/libqos/virtio-blk.c:40: fprintf(stderr, "%s not present > in virtio-blk-device\n", interface); > tests/qtest/libqos/virtio-blk.c:109: /* virtio-blk-device */ > tests/qtest/libqos/virtio-blk.c:111: > qos_node_create_driver("virtio-blk-device", virtio_blk_device_create); > tests/qtest/libqos/virtio-blk.c:112: > qos_node_consumes("virtio-blk-device", "virtio-bus", &opts); > tests/qtest/libqos/virtio-blk.c:113: > qos_node_produces("virtio-blk-device", "virtio-blk"); > > But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should > use a generic virtio-blk-device instead, hoping it get plugged correctly > to the virtio bus... Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly in the first free ones. I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus. Why is it executed for now? Thanks, Laurent
On 3/18/21 10:52 AM, Laurent Vivier wrote: > Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit : >> Hi Laurent, >> >> +Paolo / Thomas >> >> On 3/15/21 9:42 PM, Laurent Vivier wrote: >>> The machine is based on Goldfish interfaces defined by Google >>> for Android simulator. It uses Goldfish-rtc (timer and RTC), >>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty). >>> >>> The machine is created with 128 virtio-mmio bus, and they can >>> be used to use serial console, GPU, disk, NIC, HID, ... >>> >>> Signed-off-by: Laurent Vivier <laurent@vivier.eu> >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu> >>> --- >>> default-configs/devices/m68k-softmmu.mak | 1 + >>> .../standard-headers/asm-m68k/bootinfo-virt.h | 18 + >>> hw/m68k/virt.c | 313 ++++++++++++++++++ >>> MAINTAINERS | 13 + >>> hw/m68k/Kconfig | 9 + >>> hw/m68k/meson.build | 1 + >>> 6 files changed, 355 insertions(+) >>> create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h >>> create mode 100644 hw/m68k/virt.c >> >>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig >>> index 60d7bcfb8f2b..f839f8a03064 100644 >>> --- a/hw/m68k/Kconfig >>> +++ b/hw/m68k/Kconfig >>> @@ -23,3 +23,12 @@ config Q800 >>> select ESP >>> select DP8393X >>> select OR_IRQ >>> + >>> +config M68K_VIRT >>> + bool >>> + select M68K_IRQC >>> + select VIRT_CTRL >>> + select GOLDFISH_PIC >>> + select GOLDFISH_TTY >>> + select GOLDFISH_RTC >>> + select VIRTIO_MMIO >> >> I had this error on gitlab: >> >> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio: >> 'virtio-blk-pci' is not a valid device model name >> job: check-system-fedora >> https://gitlab.com/philmd/qemu/-/jobs/1106469724 >> >> I bisected locally to this commit. >> >> check-system-fedora uses build-system-fedora: >> >> build-system-fedora: >> CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs >> --enable-fdt=system --enable-slirp=system >> --enable-capstone=system >> >> I'm confused because the machine provides a VIRTIO bus >> via MMIO: >> >> config VIRTIO_MMIO >> bool >> select VIRTIO >> >> I remember I tested your machine with virtio-blk-device. >> >> config VIRTIO_BLK >> bool >> default y >> depends on VIRTIO >> >> Ah, this is virtio-blk-pci, which has: >> >> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: >> files('virtio-blk-pci.c')) >> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss) >> >> And VIRTIO_PCI isn't selected... > > This machine doesn't have virtio-pci, but only virtio-mmio buses. Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config for this machine". So the problem isn't the m68k-virt machine addition, but it shows another problem elsewhere. >> Are the tests incorrect then? >> >> libqos isn't restricted to PCI: >> >> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h" >> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */ >> tests/qtest/libqos/virtio-blk.c:33: if (!g_strcmp0(interface, >> "virtio-blk")) { >> tests/qtest/libqos/virtio-blk.c:40: fprintf(stderr, "%s not present >> in virtio-blk-device\n", interface); >> tests/qtest/libqos/virtio-blk.c:109: /* virtio-blk-device */ >> tests/qtest/libqos/virtio-blk.c:111: >> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create); >> tests/qtest/libqos/virtio-blk.c:112: >> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts); >> tests/qtest/libqos/virtio-blk.c:113: >> qos_node_produces("virtio-blk-device", "virtio-blk"); >> >> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should >> use a generic virtio-blk-device instead, hoping it get plugged correctly >> to the virtio bus... > > Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly > in the first free ones. > > I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus. > > Why is it executed for now? This is probably the problem root cause. Possible fix: -->8 -- diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 66ee9fbf450..d7f3fad51c1 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -217,13 +217,17 @@ 'emc141x-test.c', 'usb-hcd-ohci-test.c', 'virtio-test.c', - 'virtio-blk-test.c', - 'virtio-net-test.c', - 'virtio-rng-test.c', - 'virtio-scsi-test.c', 'virtio-serial-test.c', 'vmxnet3-test.c', ) +if config_all_devices.has_key('CONFIG_VIRTIO_PCI') + qos_test_ss.add( + 'virtio-blk-test.c', + 'virtio-net-test.c', + 'virtio-rng-test.c', + 'virtio-scsi-test.c', + ) +endif if have_virtfs qos_test_ss.add(files('virtio-9p-test.c')) endif --- I'll test that locally but not on Gitlab. Thanks, Phil.
Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit : > On 3/18/21 10:52 AM, Laurent Vivier wrote: >> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit : >>> Hi Laurent, >>> >>> +Paolo / Thomas >>> >>> On 3/15/21 9:42 PM, Laurent Vivier wrote: >>>> The machine is based on Goldfish interfaces defined by Google >>>> for Android simulator. It uses Goldfish-rtc (timer and RTC), >>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty). >>>> >>>> The machine is created with 128 virtio-mmio bus, and they can >>>> be used to use serial console, GPU, disk, NIC, HID, ... >>>> >>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu> >>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu> >>>> --- >>>> default-configs/devices/m68k-softmmu.mak | 1 + >>>> .../standard-headers/asm-m68k/bootinfo-virt.h | 18 + >>>> hw/m68k/virt.c | 313 ++++++++++++++++++ >>>> MAINTAINERS | 13 + >>>> hw/m68k/Kconfig | 9 + >>>> hw/m68k/meson.build | 1 + >>>> 6 files changed, 355 insertions(+) >>>> create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h >>>> create mode 100644 hw/m68k/virt.c >>> >>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig >>>> index 60d7bcfb8f2b..f839f8a03064 100644 >>>> --- a/hw/m68k/Kconfig >>>> +++ b/hw/m68k/Kconfig >>>> @@ -23,3 +23,12 @@ config Q800 >>>> select ESP >>>> select DP8393X >>>> select OR_IRQ >>>> + >>>> +config M68K_VIRT >>>> + bool >>>> + select M68K_IRQC >>>> + select VIRT_CTRL >>>> + select GOLDFISH_PIC >>>> + select GOLDFISH_TTY >>>> + select GOLDFISH_RTC >>>> + select VIRTIO_MMIO >>> >>> I had this error on gitlab: >>> >>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio: >>> 'virtio-blk-pci' is not a valid device model name >>> job: check-system-fedora >>> https://gitlab.com/philmd/qemu/-/jobs/1106469724 >>> >>> I bisected locally to this commit. >>> >>> check-system-fedora uses build-system-fedora: >>> >>> build-system-fedora: >>> CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs >>> --enable-fdt=system --enable-slirp=system >>> --enable-capstone=system >>> >>> I'm confused because the machine provides a VIRTIO bus >>> via MMIO: >>> >>> config VIRTIO_MMIO >>> bool >>> select VIRTIO >>> >>> I remember I tested your machine with virtio-blk-device. >>> >>> config VIRTIO_BLK >>> bool >>> default y >>> depends on VIRTIO >>> >>> Ah, this is virtio-blk-pci, which has: >>> >>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: >>> files('virtio-blk-pci.c')) >>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss) >>> >>> And VIRTIO_PCI isn't selected... >> >> This machine doesn't have virtio-pci, but only virtio-mmio buses. > > Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config > for this machine". So the problem isn't the m68k-virt machine addition, > but it shows another problem elsewhere. > >>> Are the tests incorrect then? >>> >>> libqos isn't restricted to PCI: >>> >>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h" >>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */ >>> tests/qtest/libqos/virtio-blk.c:33: if (!g_strcmp0(interface, >>> "virtio-blk")) { >>> tests/qtest/libqos/virtio-blk.c:40: fprintf(stderr, "%s not present >>> in virtio-blk-device\n", interface); >>> tests/qtest/libqos/virtio-blk.c:109: /* virtio-blk-device */ >>> tests/qtest/libqos/virtio-blk.c:111: >>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create); >>> tests/qtest/libqos/virtio-blk.c:112: >>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts); >>> tests/qtest/libqos/virtio-blk.c:113: >>> qos_node_produces("virtio-blk-device", "virtio-blk"); >>> >>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should >>> use a generic virtio-blk-device instead, hoping it get plugged correctly >>> to the virtio bus... >> >> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly >> in the first free ones. >> >> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus. >> >> Why is it executed for now? > > This is probably the problem root cause. > > Possible fix: > > -->8 -- > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build > index 66ee9fbf450..d7f3fad51c1 100644 > --- a/tests/qtest/meson.build > +++ b/tests/qtest/meson.build > @@ -217,13 +217,17 @@ > 'emc141x-test.c', > 'usb-hcd-ohci-test.c', > 'virtio-test.c', > - 'virtio-blk-test.c', > - 'virtio-net-test.c', > - 'virtio-rng-test.c', > - 'virtio-scsi-test.c', > 'virtio-serial-test.c', > 'vmxnet3-test.c', > ) > +if config_all_devices.has_key('CONFIG_VIRTIO_PCI') > + qos_test_ss.add( > + 'virtio-blk-test.c', > + 'virtio-net-test.c', > + 'virtio-rng-test.c', > + 'virtio-scsi-test.c', > + ) > +endif > if have_virtfs > qos_test_ss.add(files('virtio-9p-test.c')) > endif > --- > > I'll test that locally but not on Gitlab. > This also removes the virtio-devices test, I think we should keep the files, but in the files to disable the PCI part when it is not available. Thanks, Laurent
On 18/03/21 11:06, Laurent Vivier wrote: > This also removes the virtio-devices test, I think we should keep the > files, but in the files to disable the PCI part when it is not > available. I think we should just shuffle the targets in the gitlab YAML to bypass the issue. Paolo
On Thu, 18 Mar 2021 at 10:37, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 18/03/21 11:06, Laurent Vivier wrote: > > This also removes the virtio-devices test, I think we should keep the > > files, but in the files to disable the PCI part when it is not > > available. > > I think we should just shuffle the targets in the gitlab YAML to bypass > the issue. Then we'll hit it again later. I'm pretty sure this isn't the first time we've run into "some test makes dubious assumptions"... -- PMM
On 18/03/21 11:40, Peter Maydell wrote: > On Thu, 18 Mar 2021 at 10:37, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 18/03/21 11:06, Laurent Vivier wrote: >>> This also removes the virtio-devices test, I think we should keep the >>> files, but in the files to disable the PCI part when it is not >>> available. >> >> I think we should just shuffle the targets in the gitlab YAML to bypass >> the issue. > > Then we'll hit it again later. I'm pretty sure this isn't the > first time we've run into "some test makes dubious assumptions"... We can both fix qemu-iotests and CI configuration, but m68k is certainly not the culprit here. And we are going to make more assumptions over time, not fewer, in order to keep the CI time at bay. Paolo
On Thu, 18 Mar 2021 at 10:45, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 18/03/21 11:40, Peter Maydell wrote: > > On Thu, 18 Mar 2021 at 10:37, Paolo Bonzini <pbonzini@redhat.com> wrote: > >> > >> On 18/03/21 11:06, Laurent Vivier wrote: > >>> This also removes the virtio-devices test, I think we should keep the > >>> files, but in the files to disable the PCI part when it is not > >>> available. > >> > >> I think we should just shuffle the targets in the gitlab YAML to bypass > >> the issue. > > > > Then we'll hit it again later. I'm pretty sure this isn't the > > first time we've run into "some test makes dubious assumptions"... > > We can both fix qemu-iotests and CI configuration, but m68k is certainly > not the culprit here. And we are going to make more assumptions over > time, not fewer, in order to keep the CI time at bay. I don't see why CI time is relevant to whether the test says "I require X,Y,Z, so don't run me on configs without those" or whether it just randomly assumes X,Y,Z are always present or that if it says "I require W" than W must imply X,Y,Z... thanks -- PMM
On 3/18/21 12:10 PM, Peter Maydell wrote: > On Thu, 18 Mar 2021 at 10:45, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 18/03/21 11:40, Peter Maydell wrote: >>> On Thu, 18 Mar 2021 at 10:37, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> >>>> On 18/03/21 11:06, Laurent Vivier wrote: >>>>> This also removes the virtio-devices test, I think we should keep the >>>>> files, but in the files to disable the PCI part when it is not >>>>> available. >>>> >>>> I think we should just shuffle the targets in the gitlab YAML to bypass >>>> the issue. >>> >>> Then we'll hit it again later. I'm pretty sure this isn't the >>> first time we've run into "some test makes dubious assumptions"... >> >> We can both fix qemu-iotests and CI configuration, but m68k is certainly >> not the culprit here. And we are going to make more assumptions over >> time, not fewer, in order to keep the CI time at bay. > > I don't see why CI time is relevant to whether the test says > "I require X,Y,Z, so don't run me on configs without those" > or whether it just randomly assumes X,Y,Z are always present > or that if it says "I require W" than W must imply X,Y,Z... Recently we changed a bit our view and are trying to have smarter tests. In particular building target/device agnostic tests, and have the test queries the QEMU binary what features/devices are available before running. This will take some time before we get there, unlikely for the 6.0 release. For short term, Paolo's "shuffle gitlab YAML" suggestion is simpler.
On 3/18/21 11:06 AM, Laurent Vivier wrote: > Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit : >> On 3/18/21 10:52 AM, Laurent Vivier wrote: >>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit : >>>> Hi Laurent, >>>> >>>> +Paolo / Thomas >>>> >>>> On 3/15/21 9:42 PM, Laurent Vivier wrote: >>>>> The machine is based on Goldfish interfaces defined by Google >>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC), >>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty). >>>>> >>>>> The machine is created with 128 virtio-mmio bus, and they can >>>>> be used to use serial console, GPU, disk, NIC, HID, ... >>>>> >>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu> >>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu> >>>>> --- >>>>> default-configs/devices/m68k-softmmu.mak | 1 + >>>>> .../standard-headers/asm-m68k/bootinfo-virt.h | 18 + >>>>> hw/m68k/virt.c | 313 ++++++++++++++++++ >>>>> MAINTAINERS | 13 + >>>>> hw/m68k/Kconfig | 9 + >>>>> hw/m68k/meson.build | 1 + >>>>> 6 files changed, 355 insertions(+) >>>>> create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h >>>>> create mode 100644 hw/m68k/virt.c >>>> >>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig >>>>> index 60d7bcfb8f2b..f839f8a03064 100644 >>>>> --- a/hw/m68k/Kconfig >>>>> +++ b/hw/m68k/Kconfig >>>>> @@ -23,3 +23,12 @@ config Q800 >>>>> select ESP >>>>> select DP8393X >>>>> select OR_IRQ >>>>> + >>>>> +config M68K_VIRT >>>>> + bool >>>>> + select M68K_IRQC >>>>> + select VIRT_CTRL >>>>> + select GOLDFISH_PIC >>>>> + select GOLDFISH_TTY >>>>> + select GOLDFISH_RTC >>>>> + select VIRTIO_MMIO >>>> >>>> I had this error on gitlab: >>>> >>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio: >>>> 'virtio-blk-pci' is not a valid device model name >>>> job: check-system-fedora >>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724 >>>> >>>> I bisected locally to this commit. >>>> >>>> check-system-fedora uses build-system-fedora: >>>> >>>> build-system-fedora: >>>> CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs >>>> --enable-fdt=system --enable-slirp=system >>>> --enable-capstone=system >>>> >>>> I'm confused because the machine provides a VIRTIO bus >>>> via MMIO: >>>> >>>> config VIRTIO_MMIO >>>> bool >>>> select VIRTIO >>>> >>>> I remember I tested your machine with virtio-blk-device. >>>> >>>> config VIRTIO_BLK >>>> bool >>>> default y >>>> depends on VIRTIO >>>> >>>> Ah, this is virtio-blk-pci, which has: >>>> >>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: >>>> files('virtio-blk-pci.c')) >>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss) >>>> >>>> And VIRTIO_PCI isn't selected... >>> >>> This machine doesn't have virtio-pci, but only virtio-mmio buses. >> >> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config >> for this machine". So the problem isn't the m68k-virt machine addition, >> but it shows another problem elsewhere. >> >>>> Are the tests incorrect then? >>>> >>>> libqos isn't restricted to PCI: >>>> >>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h" >>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */ >>>> tests/qtest/libqos/virtio-blk.c:33: if (!g_strcmp0(interface, >>>> "virtio-blk")) { >>>> tests/qtest/libqos/virtio-blk.c:40: fprintf(stderr, "%s not present >>>> in virtio-blk-device\n", interface); >>>> tests/qtest/libqos/virtio-blk.c:109: /* virtio-blk-device */ >>>> tests/qtest/libqos/virtio-blk.c:111: >>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create); >>>> tests/qtest/libqos/virtio-blk.c:112: >>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts); >>>> tests/qtest/libqos/virtio-blk.c:113: >>>> qos_node_produces("virtio-blk-device", "virtio-blk"); >>>> >>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should >>>> use a generic virtio-blk-device instead, hoping it get plugged correctly >>>> to the virtio bus... >>> >>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly >>> in the first free ones. >>> >>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus. >>> >>> Why is it executed for now? >> >> This is probably the problem root cause. >> >> Possible fix: >> >> -->8 -- >> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build >> index 66ee9fbf450..d7f3fad51c1 100644 >> --- a/tests/qtest/meson.build >> +++ b/tests/qtest/meson.build >> @@ -217,13 +217,17 @@ >> 'emc141x-test.c', >> 'usb-hcd-ohci-test.c', >> 'virtio-test.c', >> - 'virtio-blk-test.c', >> - 'virtio-net-test.c', >> - 'virtio-rng-test.c', >> - 'virtio-scsi-test.c', >> 'virtio-serial-test.c', >> 'vmxnet3-test.c', >> ) >> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI') >> + qos_test_ss.add( >> + 'virtio-blk-test.c', >> + 'virtio-net-test.c', >> + 'virtio-rng-test.c', >> + 'virtio-scsi-test.c', >> + ) >> +endif >> if have_virtfs >> qos_test_ss.add(files('virtio-9p-test.c')) >> endif >> --- >> >> I'll test that locally but not on Gitlab. This approach doesn't work for the iotests. > This also removes the virtio-devices test, I think we should keep the files, but in the files to > disable the PCI part when it is not available. I don't understand how the virtio devices are created, it seems there is an alias to generic virtio hw that map to the arch virtio bus. I was not obvious to understand why start the virt machine with "-device virtio-blk" returns "'virtio-blk-pci' is not a valid device model name" at first, then I figured out the qdev_alias_table array. Maybe you need to complete it for your arch? I've been using that: -- >8 -- diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 8dc656becca..b326bd76c2a 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -65,8 +65,10 @@ static const QDevAlias qdev_alias_table[] = { { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X }, { "virtio-balloon-pci", "virtio-balloon", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, + { "virtio-blk-device", "virtio-blk", QEMU_ARCH_M68K }, { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X }, - { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, + { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL + & ~(QEMU_ARCH_S390X | QEMU_ARCH_M68K) }, { "virtio-gpu-ccw", "virtio-gpu", QEMU_ARCH_S390X }, { "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X }, @@ -84,8 +86,10 @@ static const QDevAlias qdev_alias_table[] = { { "virtio-rng-pci", "virtio-rng", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X }, { "virtio-scsi-pci", "virtio-scsi", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, + { "virtio-serial-device", "virtio-serial", QEMU_ARCH_M68K }, { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X }, - { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, + { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL + & ~(QEMU_ARCH_S390X | QEMU_ARCH_M68K)}, { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X }, { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, { } --- But this looks ugly, I don't think it should work that way (because a machine could provide virtio buses over multiple transport, mmio and pci...). I'll ignore this problem and send my pull request with a red CI as others seem to do. Regards, Phil.
Le 18/03/2021 à 16:36, Philippe Mathieu-Daudé a écrit : > On 3/18/21 11:06 AM, Laurent Vivier wrote: >> Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit : >>> On 3/18/21 10:52 AM, Laurent Vivier wrote: >>>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit : >>>>> Hi Laurent, >>>>> >>>>> +Paolo / Thomas >>>>> >>>>> On 3/15/21 9:42 PM, Laurent Vivier wrote: >>>>>> The machine is based on Goldfish interfaces defined by Google >>>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC), >>>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty). >>>>>> >>>>>> The machine is created with 128 virtio-mmio bus, and they can >>>>>> be used to use serial console, GPU, disk, NIC, HID, ... >>>>>> >>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu> >>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu> >>>>>> --- >>>>>> default-configs/devices/m68k-softmmu.mak | 1 + >>>>>> .../standard-headers/asm-m68k/bootinfo-virt.h | 18 + >>>>>> hw/m68k/virt.c | 313 ++++++++++++++++++ >>>>>> MAINTAINERS | 13 + >>>>>> hw/m68k/Kconfig | 9 + >>>>>> hw/m68k/meson.build | 1 + >>>>>> 6 files changed, 355 insertions(+) >>>>>> create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h >>>>>> create mode 100644 hw/m68k/virt.c >>>>> >>>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig >>>>>> index 60d7bcfb8f2b..f839f8a03064 100644 >>>>>> --- a/hw/m68k/Kconfig >>>>>> +++ b/hw/m68k/Kconfig >>>>>> @@ -23,3 +23,12 @@ config Q800 >>>>>> select ESP >>>>>> select DP8393X >>>>>> select OR_IRQ >>>>>> + >>>>>> +config M68K_VIRT >>>>>> + bool >>>>>> + select M68K_IRQC >>>>>> + select VIRT_CTRL >>>>>> + select GOLDFISH_PIC >>>>>> + select GOLDFISH_TTY >>>>>> + select GOLDFISH_RTC >>>>>> + select VIRTIO_MMIO >>>>> >>>>> I had this error on gitlab: >>>>> >>>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio: >>>>> 'virtio-blk-pci' is not a valid device model name >>>>> job: check-system-fedora >>>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724 >>>>> >>>>> I bisected locally to this commit. >>>>> >>>>> check-system-fedora uses build-system-fedora: >>>>> >>>>> build-system-fedora: >>>>> CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs >>>>> --enable-fdt=system --enable-slirp=system >>>>> --enable-capstone=system >>>>> >>>>> I'm confused because the machine provides a VIRTIO bus >>>>> via MMIO: >>>>> >>>>> config VIRTIO_MMIO >>>>> bool >>>>> select VIRTIO >>>>> >>>>> I remember I tested your machine with virtio-blk-device. >>>>> >>>>> config VIRTIO_BLK >>>>> bool >>>>> default y >>>>> depends on VIRTIO >>>>> >>>>> Ah, this is virtio-blk-pci, which has: >>>>> >>>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: >>>>> files('virtio-blk-pci.c')) >>>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss) >>>>> >>>>> And VIRTIO_PCI isn't selected... >>>> >>>> This machine doesn't have virtio-pci, but only virtio-mmio buses. >>> >>> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config >>> for this machine". So the problem isn't the m68k-virt machine addition, >>> but it shows another problem elsewhere. >>> >>>>> Are the tests incorrect then? >>>>> >>>>> libqos isn't restricted to PCI: >>>>> >>>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h" >>>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */ >>>>> tests/qtest/libqos/virtio-blk.c:33: if (!g_strcmp0(interface, >>>>> "virtio-blk")) { >>>>> tests/qtest/libqos/virtio-blk.c:40: fprintf(stderr, "%s not present >>>>> in virtio-blk-device\n", interface); >>>>> tests/qtest/libqos/virtio-blk.c:109: /* virtio-blk-device */ >>>>> tests/qtest/libqos/virtio-blk.c:111: >>>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create); >>>>> tests/qtest/libqos/virtio-blk.c:112: >>>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts); >>>>> tests/qtest/libqos/virtio-blk.c:113: >>>>> qos_node_produces("virtio-blk-device", "virtio-blk"); >>>>> >>>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should >>>>> use a generic virtio-blk-device instead, hoping it get plugged correctly >>>>> to the virtio bus... >>>> >>>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly >>>> in the first free ones. >>>> >>>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus. >>>> >>>> Why is it executed for now? >>> >>> This is probably the problem root cause. >>> >>> Possible fix: >>> >>> -->8 -- >>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build >>> index 66ee9fbf450..d7f3fad51c1 100644 >>> --- a/tests/qtest/meson.build >>> +++ b/tests/qtest/meson.build >>> @@ -217,13 +217,17 @@ >>> 'emc141x-test.c', >>> 'usb-hcd-ohci-test.c', >>> 'virtio-test.c', >>> - 'virtio-blk-test.c', >>> - 'virtio-net-test.c', >>> - 'virtio-rng-test.c', >>> - 'virtio-scsi-test.c', >>> 'virtio-serial-test.c', >>> 'vmxnet3-test.c', >>> ) >>> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI') >>> + qos_test_ss.add( >>> + 'virtio-blk-test.c', >>> + 'virtio-net-test.c', >>> + 'virtio-rng-test.c', >>> + 'virtio-scsi-test.c', >>> + ) >>> +endif >>> if have_virtfs >>> qos_test_ss.add(files('virtio-9p-test.c')) >>> endif >>> --- >>> >>> I'll test that locally but not on Gitlab. > > This approach doesn't work for the iotests. > >> This also removes the virtio-devices test, I think we should keep the files, but in the files to >> disable the PCI part when it is not available. > I don't understand how the virtio devices are created, it seems there > is an alias to generic virtio hw that map to the arch virtio bus. > > I was not obvious to understand why start the virt machine with > "-device virtio-blk" returns "'virtio-blk-pci' is not a valid device > model name" at first, then I figured out the qdev_alias_table array. > > Maybe you need to complete it for your arch? I've been using that: > > -- >8 -- > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > index 8dc656becca..b326bd76c2a 100644 > --- a/softmmu/qdev-monitor.c > +++ b/softmmu/qdev-monitor.c > @@ -65,8 +65,10 @@ static const QDevAlias qdev_alias_table[] = { > { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X }, > { "virtio-balloon-pci", "virtio-balloon", > QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, > + { "virtio-blk-device", "virtio-blk", QEMU_ARCH_M68K }, > { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X }, > - { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, > + { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL > + & ~(QEMU_ARCH_S390X | > QEMU_ARCH_M68K) }, > { "virtio-gpu-ccw", "virtio-gpu", QEMU_ARCH_S390X }, > { "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, > { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X }, > @@ -84,8 +86,10 @@ static const QDevAlias qdev_alias_table[] = { > { "virtio-rng-pci", "virtio-rng", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, > { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X }, > { "virtio-scsi-pci", "virtio-scsi", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, > + { "virtio-serial-device", "virtio-serial", QEMU_ARCH_M68K }, > { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X }, > - { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL & > ~QEMU_ARCH_S390X }, > + { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL > + & ~(QEMU_ARCH_S390X | > QEMU_ARCH_M68K)}, > { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X }, > { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL & > ~QEMU_ARCH_S390X }, > { } > --- > > But this looks ugly, I don't think it should work that way (because > a machine could provide virtio buses over multiple transport, mmio > and pci...). IMHO, this looks like the solution. The alias is to define the prefered way, on PCI it's the -pci one otherwise it is the -device one. Thanks, Laurent
Le 18/03/2021 à 16:51, Laurent Vivier a écrit : > Le 18/03/2021 à 16:36, Philippe Mathieu-Daudé a écrit : >> On 3/18/21 11:06 AM, Laurent Vivier wrote: >>> Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit : >>>> On 3/18/21 10:52 AM, Laurent Vivier wrote: >>>>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit : >>>>>> Hi Laurent, >>>>>> >>>>>> +Paolo / Thomas >>>>>> >>>>>> On 3/15/21 9:42 PM, Laurent Vivier wrote: >>>>>>> The machine is based on Goldfish interfaces defined by Google >>>>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC), >>>>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty). >>>>>>> >>>>>>> The machine is created with 128 virtio-mmio bus, and they can >>>>>>> be used to use serial console, GPU, disk, NIC, HID, ... >>>>>>> >>>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu> >>>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu> >>>>>>> --- >>>>>>> default-configs/devices/m68k-softmmu.mak | 1 + >>>>>>> .../standard-headers/asm-m68k/bootinfo-virt.h | 18 + >>>>>>> hw/m68k/virt.c | 313 ++++++++++++++++++ >>>>>>> MAINTAINERS | 13 + >>>>>>> hw/m68k/Kconfig | 9 + >>>>>>> hw/m68k/meson.build | 1 + >>>>>>> 6 files changed, 355 insertions(+) >>>>>>> create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h >>>>>>> create mode 100644 hw/m68k/virt.c >>>>>> >>>>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig >>>>>>> index 60d7bcfb8f2b..f839f8a03064 100644 >>>>>>> --- a/hw/m68k/Kconfig >>>>>>> +++ b/hw/m68k/Kconfig >>>>>>> @@ -23,3 +23,12 @@ config Q800 >>>>>>> select ESP >>>>>>> select DP8393X >>>>>>> select OR_IRQ >>>>>>> + >>>>>>> +config M68K_VIRT >>>>>>> + bool >>>>>>> + select M68K_IRQC >>>>>>> + select VIRT_CTRL >>>>>>> + select GOLDFISH_PIC >>>>>>> + select GOLDFISH_TTY >>>>>>> + select GOLDFISH_RTC >>>>>>> + select VIRTIO_MMIO >>>>>> >>>>>> I had this error on gitlab: >>>>>> >>>>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio: >>>>>> 'virtio-blk-pci' is not a valid device model name >>>>>> job: check-system-fedora >>>>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724 >>>>>> >>>>>> I bisected locally to this commit. >>>>>> >>>>>> check-system-fedora uses build-system-fedora: >>>>>> >>>>>> build-system-fedora: >>>>>> CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs >>>>>> --enable-fdt=system --enable-slirp=system >>>>>> --enable-capstone=system >>>>>> >>>>>> I'm confused because the machine provides a VIRTIO bus >>>>>> via MMIO: >>>>>> >>>>>> config VIRTIO_MMIO >>>>>> bool >>>>>> select VIRTIO >>>>>> >>>>>> I remember I tested your machine with virtio-blk-device. >>>>>> >>>>>> config VIRTIO_BLK >>>>>> bool >>>>>> default y >>>>>> depends on VIRTIO >>>>>> >>>>>> Ah, this is virtio-blk-pci, which has: >>>>>> >>>>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: >>>>>> files('virtio-blk-pci.c')) >>>>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss) >>>>>> >>>>>> And VIRTIO_PCI isn't selected... >>>>> >>>>> This machine doesn't have virtio-pci, but only virtio-mmio buses. >>>> >>>> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config >>>> for this machine". So the problem isn't the m68k-virt machine addition, >>>> but it shows another problem elsewhere. >>>> >>>>>> Are the tests incorrect then? >>>>>> >>>>>> libqos isn't restricted to PCI: >>>>>> >>>>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h" >>>>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */ >>>>>> tests/qtest/libqos/virtio-blk.c:33: if (!g_strcmp0(interface, >>>>>> "virtio-blk")) { >>>>>> tests/qtest/libqos/virtio-blk.c:40: fprintf(stderr, "%s not present >>>>>> in virtio-blk-device\n", interface); >>>>>> tests/qtest/libqos/virtio-blk.c:109: /* virtio-blk-device */ >>>>>> tests/qtest/libqos/virtio-blk.c:111: >>>>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create); >>>>>> tests/qtest/libqos/virtio-blk.c:112: >>>>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts); >>>>>> tests/qtest/libqos/virtio-blk.c:113: >>>>>> qos_node_produces("virtio-blk-device", "virtio-blk"); >>>>>> >>>>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should >>>>>> use a generic virtio-blk-device instead, hoping it get plugged correctly >>>>>> to the virtio bus... >>>>> >>>>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly >>>>> in the first free ones. >>>>> >>>>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus. >>>>> >>>>> Why is it executed for now? >>>> >>>> This is probably the problem root cause. >>>> >>>> Possible fix: >>>> >>>> -->8 -- >>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build >>>> index 66ee9fbf450..d7f3fad51c1 100644 >>>> --- a/tests/qtest/meson.build >>>> +++ b/tests/qtest/meson.build >>>> @@ -217,13 +217,17 @@ >>>> 'emc141x-test.c', >>>> 'usb-hcd-ohci-test.c', >>>> 'virtio-test.c', >>>> - 'virtio-blk-test.c', >>>> - 'virtio-net-test.c', >>>> - 'virtio-rng-test.c', >>>> - 'virtio-scsi-test.c', >>>> 'virtio-serial-test.c', >>>> 'vmxnet3-test.c', >>>> ) >>>> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI') >>>> + qos_test_ss.add( >>>> + 'virtio-blk-test.c', >>>> + 'virtio-net-test.c', >>>> + 'virtio-rng-test.c', >>>> + 'virtio-scsi-test.c', >>>> + ) >>>> +endif >>>> if have_virtfs >>>> qos_test_ss.add(files('virtio-9p-test.c')) >>>> endif >>>> --- >>>> >>>> I'll test that locally but not on Gitlab. >> >> This approach doesn't work for the iotests. >> >>> This also removes the virtio-devices test, I think we should keep the files, but in the files to >>> disable the PCI part when it is not available. >> I don't understand how the virtio devices are created, it seems there >> is an alias to generic virtio hw that map to the arch virtio bus. >> >> I was not obvious to understand why start the virt machine with >> "-device virtio-blk" returns "'virtio-blk-pci' is not a valid device >> model name" at first, then I figured out the qdev_alias_table array. >> >> Maybe you need to complete it for your arch? I've been using that: >> >> -- >8 -- >> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >> index 8dc656becca..b326bd76c2a 100644 >> --- a/softmmu/qdev-monitor.c >> +++ b/softmmu/qdev-monitor.c >> @@ -65,8 +65,10 @@ static const QDevAlias qdev_alias_table[] = { >> { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X }, >> { "virtio-balloon-pci", "virtio-balloon", >> QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >> + { "virtio-blk-device", "virtio-blk", QEMU_ARCH_M68K }, >> { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X }, >> - { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >> + { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL >> + & ~(QEMU_ARCH_S390X | >> QEMU_ARCH_M68K) }, >> { "virtio-gpu-ccw", "virtio-gpu", QEMU_ARCH_S390X }, >> { "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >> { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X }, >> @@ -84,8 +86,10 @@ static const QDevAlias qdev_alias_table[] = { >> { "virtio-rng-pci", "virtio-rng", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >> { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X }, >> { "virtio-scsi-pci", "virtio-scsi", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >> + { "virtio-serial-device", "virtio-serial", QEMU_ARCH_M68K }, >> { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X }, >> - { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL & >> ~QEMU_ARCH_S390X }, >> + { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL >> + & ~(QEMU_ARCH_S390X | >> QEMU_ARCH_M68K)}, >> { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X }, >> { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL & >> ~QEMU_ARCH_S390X }, >> { } >> --- >> >> But this looks ugly, I don't think it should work that way (because >> a machine could provide virtio buses over multiple transport, mmio >> and pci...). > > IMHO, this looks like the solution. > > The alias is to define the prefered way, on PCI it's the -pci one otherwise it is the -device one. See: commit 5f629d943cb0b11c37a891cf4f40a9166aee6f53 Author: Alexander Graf <agraf@csgraf.de> Date: Fri May 18 02:36:26 2012 +0200 s390x: fix s390 virtio aliases Some of the virtio devices have the same frontend name, but actually implement different devices behind the scenes through aliases. The indicator which device type to use is the architecture. On s390, we want s390 virtio devices. On everything else, we want PCI devices. Reflect this in the alias selection code. This way we fix commands like -device virtio-blk on s390x which with this patch applied select the correct virtio-blk-s390 device rather than virtio-blk-pci. Reported-by: Christian Borntraeger <borntraeger@de.ibm.com> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> Signed-off-by: Alexander Graf <agraf@suse.de> Thanks, Laurent
On 3/18/21 4:56 PM, Laurent Vivier wrote: > Le 18/03/2021 à 16:51, Laurent Vivier a écrit : >> Le 18/03/2021 à 16:36, Philippe Mathieu-Daudé a écrit : >>> On 3/18/21 11:06 AM, Laurent Vivier wrote: >>>> Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit : >>>>> On 3/18/21 10:52 AM, Laurent Vivier wrote: >>>>>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit : >>>>>>> Hi Laurent, >>>>>>> >>>>>>> +Paolo / Thomas >>>>>>> >>>>>>> On 3/15/21 9:42 PM, Laurent Vivier wrote: >>>>>>>> The machine is based on Goldfish interfaces defined by Google >>>>>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC), >>>>>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty). >>>>>>>> >>>>>>>> The machine is created with 128 virtio-mmio bus, and they can >>>>>>>> be used to use serial console, GPU, disk, NIC, HID, ... >>>>>>>> >>>>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu> >>>>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>>>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu> >>>>>>>> --- >>>>>>>> default-configs/devices/m68k-softmmu.mak | 1 + >>>>>>>> .../standard-headers/asm-m68k/bootinfo-virt.h | 18 + >>>>>>>> hw/m68k/virt.c | 313 ++++++++++++++++++ >>>>>>>> MAINTAINERS | 13 + >>>>>>>> hw/m68k/Kconfig | 9 + >>>>>>>> hw/m68k/meson.build | 1 + >>>>>>>> 6 files changed, 355 insertions(+) >>>>>>>> create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h >>>>>>>> create mode 100644 hw/m68k/virt.c >>>>>>> >>>>>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig >>>>>>>> index 60d7bcfb8f2b..f839f8a03064 100644 >>>>>>>> --- a/hw/m68k/Kconfig >>>>>>>> +++ b/hw/m68k/Kconfig >>>>>>>> @@ -23,3 +23,12 @@ config Q800 >>>>>>>> select ESP >>>>>>>> select DP8393X >>>>>>>> select OR_IRQ >>>>>>>> + >>>>>>>> +config M68K_VIRT >>>>>>>> + bool >>>>>>>> + select M68K_IRQC >>>>>>>> + select VIRT_CTRL >>>>>>>> + select GOLDFISH_PIC >>>>>>>> + select GOLDFISH_TTY >>>>>>>> + select GOLDFISH_RTC >>>>>>>> + select VIRTIO_MMIO >>>>>>> >>>>>>> I had this error on gitlab: >>>>>>> >>>>>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio: >>>>>>> 'virtio-blk-pci' is not a valid device model name >>>>>>> job: check-system-fedora >>>>>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724 >>>>>>> >>>>>>> I bisected locally to this commit. >>>>>>> >>>>>>> check-system-fedora uses build-system-fedora: >>>>>>> >>>>>>> build-system-fedora: >>>>>>> CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs >>>>>>> --enable-fdt=system --enable-slirp=system >>>>>>> --enable-capstone=system >>>>>>> >>>>>>> I'm confused because the machine provides a VIRTIO bus >>>>>>> via MMIO: >>>>>>> >>>>>>> config VIRTIO_MMIO >>>>>>> bool >>>>>>> select VIRTIO >>>>>>> >>>>>>> I remember I tested your machine with virtio-blk-device. >>>>>>> >>>>>>> config VIRTIO_BLK >>>>>>> bool >>>>>>> default y >>>>>>> depends on VIRTIO >>>>>>> >>>>>>> Ah, this is virtio-blk-pci, which has: >>>>>>> >>>>>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: >>>>>>> files('virtio-blk-pci.c')) >>>>>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss) >>>>>>> >>>>>>> And VIRTIO_PCI isn't selected... >>>>>> >>>>>> This machine doesn't have virtio-pci, but only virtio-mmio buses. >>>>> >>>>> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config >>>>> for this machine". So the problem isn't the m68k-virt machine addition, >>>>> but it shows another problem elsewhere. >>>>> >>>>>>> Are the tests incorrect then? >>>>>>> >>>>>>> libqos isn't restricted to PCI: >>>>>>> >>>>>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h" >>>>>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */ >>>>>>> tests/qtest/libqos/virtio-blk.c:33: if (!g_strcmp0(interface, >>>>>>> "virtio-blk")) { >>>>>>> tests/qtest/libqos/virtio-blk.c:40: fprintf(stderr, "%s not present >>>>>>> in virtio-blk-device\n", interface); >>>>>>> tests/qtest/libqos/virtio-blk.c:109: /* virtio-blk-device */ >>>>>>> tests/qtest/libqos/virtio-blk.c:111: >>>>>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create); >>>>>>> tests/qtest/libqos/virtio-blk.c:112: >>>>>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts); >>>>>>> tests/qtest/libqos/virtio-blk.c:113: >>>>>>> qos_node_produces("virtio-blk-device", "virtio-blk"); >>>>>>> >>>>>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should >>>>>>> use a generic virtio-blk-device instead, hoping it get plugged correctly >>>>>>> to the virtio bus... >>>>>> >>>>>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly >>>>>> in the first free ones. >>>>>> >>>>>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus. >>>>>> >>>>>> Why is it executed for now? >>>>> >>>>> This is probably the problem root cause. >>>>> >>>>> Possible fix: >>>>> >>>>> -->8 -- >>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build >>>>> index 66ee9fbf450..d7f3fad51c1 100644 >>>>> --- a/tests/qtest/meson.build >>>>> +++ b/tests/qtest/meson.build >>>>> @@ -217,13 +217,17 @@ >>>>> 'emc141x-test.c', >>>>> 'usb-hcd-ohci-test.c', >>>>> 'virtio-test.c', >>>>> - 'virtio-blk-test.c', >>>>> - 'virtio-net-test.c', >>>>> - 'virtio-rng-test.c', >>>>> - 'virtio-scsi-test.c', >>>>> 'virtio-serial-test.c', >>>>> 'vmxnet3-test.c', >>>>> ) >>>>> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI') >>>>> + qos_test_ss.add( >>>>> + 'virtio-blk-test.c', >>>>> + 'virtio-net-test.c', >>>>> + 'virtio-rng-test.c', >>>>> + 'virtio-scsi-test.c', >>>>> + ) >>>>> +endif >>>>> if have_virtfs >>>>> qos_test_ss.add(files('virtio-9p-test.c')) >>>>> endif >>>>> --- >>>>> >>>>> I'll test that locally but not on Gitlab. >>> >>> This approach doesn't work for the iotests. >>> >>>> This also removes the virtio-devices test, I think we should keep the files, but in the files to >>>> disable the PCI part when it is not available. >>> I don't understand how the virtio devices are created, it seems there >>> is an alias to generic virtio hw that map to the arch virtio bus. >>> >>> I was not obvious to understand why start the virt machine with >>> "-device virtio-blk" returns "'virtio-blk-pci' is not a valid device >>> model name" at first, then I figured out the qdev_alias_table array. >>> >>> Maybe you need to complete it for your arch? I've been using that: >>> >>> -- >8 -- >>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>> index 8dc656becca..b326bd76c2a 100644 >>> --- a/softmmu/qdev-monitor.c >>> +++ b/softmmu/qdev-monitor.c >>> @@ -65,8 +65,10 @@ static const QDevAlias qdev_alias_table[] = { >>> { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X }, >>> { "virtio-balloon-pci", "virtio-balloon", >>> QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >>> + { "virtio-blk-device", "virtio-blk", QEMU_ARCH_M68K }, >>> { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X }, >>> - { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >>> + { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL >>> + & ~(QEMU_ARCH_S390X | >>> QEMU_ARCH_M68K) }, >>> { "virtio-gpu-ccw", "virtio-gpu", QEMU_ARCH_S390X }, >>> { "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >>> { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X }, >>> @@ -84,8 +86,10 @@ static const QDevAlias qdev_alias_table[] = { >>> { "virtio-rng-pci", "virtio-rng", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >>> { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X }, >>> { "virtio-scsi-pci", "virtio-scsi", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >>> + { "virtio-serial-device", "virtio-serial", QEMU_ARCH_M68K }, >>> { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X }, >>> - { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL & >>> ~QEMU_ARCH_S390X }, >>> + { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL >>> + & ~(QEMU_ARCH_S390X | >>> QEMU_ARCH_M68K)}, >>> { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X }, >>> { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL & >>> ~QEMU_ARCH_S390X }, >>> { } >>> --- >>> >>> But this looks ugly, I don't think it should work that way (because >>> a machine could provide virtio buses over multiple transport, mmio >>> and pci...). >> >> IMHO, this looks like the solution. >> >> The alias is to define the prefered way, on PCI it's the -pci one otherwise it is the -device one. > > See: > > commit 5f629d943cb0b11c37a891cf4f40a9166aee6f53 > Author: Alexander Graf <agraf@csgraf.de> > Date: Fri May 18 02:36:26 2012 +0200 > > s390x: fix s390 virtio aliases > > Some of the virtio devices have the same frontend name, but actually > implement different devices behind the scenes through aliases. > > The indicator which device type to use is the architecture. On s390, we > want s390 virtio devices. On everything else, we want PCI devices. > > Reflect this in the alias selection code. This way we fix commands like > -device virtio-blk on s390x which with this patch applied select the > correct virtio-blk-s390 device rather than virtio-blk-pci. > > Reported-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > Signed-off-by: Alexander Graf <agraf@suse.de> So now than MMIO is available, we hit the "everything else" limit :) The other function I had to modify is in tests/qemu-iotests/iotests.py: def get_virtio_scsi_device(): if qemu_default_machine == 's390-ccw-virtio': return 'virtio-scsi-ccw' return 'virtio-scsi-pci' But Max said there is no interest in testing the block devices here (here = archs providing virtio via MMIO such ARM/m68k).
On 18/03/21 16:36, Philippe Mathieu-Daudé wrote: > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build > index 66ee9fbf450..d7f3fad51c1 100644 > --- a/tests/qtest/meson.build > +++ b/tests/qtest/meson.build > @@ -217,13 +217,17 @@ > 'emc141x-test.c', > 'usb-hcd-ohci-test.c', > 'virtio-test.c', > - 'virtio-blk-test.c', > - 'virtio-net-test.c', > - 'virtio-rng-test.c', > - 'virtio-scsi-test.c', > 'virtio-serial-test.c', > 'vmxnet3-test.c', > ) > +if config_all_devices.has_key('CONFIG_VIRTIO_PCI') > + qos_test_ss.add( > + 'virtio-blk-test.c', > + 'virtio-net-test.c', > + 'virtio-rng-test.c', > + 'virtio-scsi-test.c', > + ) > +endif > if have_virtfs > qos_test_ss.add(files('virtio-9p-test.c')) > endif I don't understand, what would this be trying to do? (And besides, why would it work? The CI failure is in qemu-iotests that has no connection to qtest at all). > Maybe you need to complete it for your arch? I've been using that: Instead of completing it, you can drop your arch from virtio-*-pci, so: > + { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL > + & ~(QEMU_ARCH_S390X | > QEMU_ARCH_M68K) }, but do not add m68k anywhere. Paolo
On 18.03.21 17:25, Philippe Mathieu-Daudé wrote: > On 3/18/21 4:56 PM, Laurent Vivier wrote: >> Le 18/03/2021 à 16:51, Laurent Vivier a écrit : >>> Le 18/03/2021 à 16:36, Philippe Mathieu-Daudé a écrit : >>>> On 3/18/21 11:06 AM, Laurent Vivier wrote: >>>>> Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit : >>>>>> On 3/18/21 10:52 AM, Laurent Vivier wrote: >>>>>>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit : >>>>>>>> Hi Laurent, >>>>>>>> >>>>>>>> +Paolo / Thomas >>>>>>>> >>>>>>>> On 3/15/21 9:42 PM, Laurent Vivier wrote: >>>>>>>>> The machine is based on Goldfish interfaces defined by Google >>>>>>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC), >>>>>>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty). >>>>>>>>> >>>>>>>>> The machine is created with 128 virtio-mmio bus, and they can >>>>>>>>> be used to use serial console, GPU, disk, NIC, HID, ... >>>>>>>>> >>>>>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu> >>>>>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>>>>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>>>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu> >>>>>>>>> --- >>>>>>>>> default-configs/devices/m68k-softmmu.mak | 1 + >>>>>>>>> .../standard-headers/asm-m68k/bootinfo-virt.h | 18 + >>>>>>>>> hw/m68k/virt.c | 313 ++++++++++++++++++ >>>>>>>>> MAINTAINERS | 13 + >>>>>>>>> hw/m68k/Kconfig | 9 + >>>>>>>>> hw/m68k/meson.build | 1 + >>>>>>>>> 6 files changed, 355 insertions(+) >>>>>>>>> create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h >>>>>>>>> create mode 100644 hw/m68k/virt.c >>>>>>>> >>>>>>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig >>>>>>>>> index 60d7bcfb8f2b..f839f8a03064 100644 >>>>>>>>> --- a/hw/m68k/Kconfig >>>>>>>>> +++ b/hw/m68k/Kconfig >>>>>>>>> @@ -23,3 +23,12 @@ config Q800 >>>>>>>>> select ESP >>>>>>>>> select DP8393X >>>>>>>>> select OR_IRQ >>>>>>>>> + >>>>>>>>> +config M68K_VIRT >>>>>>>>> + bool >>>>>>>>> + select M68K_IRQC >>>>>>>>> + select VIRT_CTRL >>>>>>>>> + select GOLDFISH_PIC >>>>>>>>> + select GOLDFISH_TTY >>>>>>>>> + select GOLDFISH_RTC >>>>>>>>> + select VIRTIO_MMIO >>>>>>>> >>>>>>>> I had this error on gitlab: >>>>>>>> >>>>>>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio: >>>>>>>> 'virtio-blk-pci' is not a valid device model name >>>>>>>> job: check-system-fedora >>>>>>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724 >>>>>>>> >>>>>>>> I bisected locally to this commit. >>>>>>>> >>>>>>>> check-system-fedora uses build-system-fedora: >>>>>>>> >>>>>>>> build-system-fedora: >>>>>>>> CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs >>>>>>>> --enable-fdt=system --enable-slirp=system >>>>>>>> --enable-capstone=system >>>>>>>> >>>>>>>> I'm confused because the machine provides a VIRTIO bus >>>>>>>> via MMIO: >>>>>>>> >>>>>>>> config VIRTIO_MMIO >>>>>>>> bool >>>>>>>> select VIRTIO >>>>>>>> >>>>>>>> I remember I tested your machine with virtio-blk-device. >>>>>>>> >>>>>>>> config VIRTIO_BLK >>>>>>>> bool >>>>>>>> default y >>>>>>>> depends on VIRTIO >>>>>>>> >>>>>>>> Ah, this is virtio-blk-pci, which has: >>>>>>>> >>>>>>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: >>>>>>>> files('virtio-blk-pci.c')) >>>>>>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss) >>>>>>>> >>>>>>>> And VIRTIO_PCI isn't selected... >>>>>>> >>>>>>> This machine doesn't have virtio-pci, but only virtio-mmio buses. >>>>>> >>>>>> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config >>>>>> for this machine". So the problem isn't the m68k-virt machine addition, >>>>>> but it shows another problem elsewhere. >>>>>> >>>>>>>> Are the tests incorrect then? >>>>>>>> >>>>>>>> libqos isn't restricted to PCI: >>>>>>>> >>>>>>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h" >>>>>>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */ >>>>>>>> tests/qtest/libqos/virtio-blk.c:33: if (!g_strcmp0(interface, >>>>>>>> "virtio-blk")) { >>>>>>>> tests/qtest/libqos/virtio-blk.c:40: fprintf(stderr, "%s not present >>>>>>>> in virtio-blk-device\n", interface); >>>>>>>> tests/qtest/libqos/virtio-blk.c:109: /* virtio-blk-device */ >>>>>>>> tests/qtest/libqos/virtio-blk.c:111: >>>>>>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create); >>>>>>>> tests/qtest/libqos/virtio-blk.c:112: >>>>>>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts); >>>>>>>> tests/qtest/libqos/virtio-blk.c:113: >>>>>>>> qos_node_produces("virtio-blk-device", "virtio-blk"); >>>>>>>> >>>>>>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should >>>>>>>> use a generic virtio-blk-device instead, hoping it get plugged correctly >>>>>>>> to the virtio bus... >>>>>>> >>>>>>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly >>>>>>> in the first free ones. >>>>>>> >>>>>>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus. >>>>>>> >>>>>>> Why is it executed for now? >>>>>> >>>>>> This is probably the problem root cause. >>>>>> >>>>>> Possible fix: >>>>>> >>>>>> -->8 -- >>>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build >>>>>> index 66ee9fbf450..d7f3fad51c1 100644 >>>>>> --- a/tests/qtest/meson.build >>>>>> +++ b/tests/qtest/meson.build >>>>>> @@ -217,13 +217,17 @@ >>>>>> 'emc141x-test.c', >>>>>> 'usb-hcd-ohci-test.c', >>>>>> 'virtio-test.c', >>>>>> - 'virtio-blk-test.c', >>>>>> - 'virtio-net-test.c', >>>>>> - 'virtio-rng-test.c', >>>>>> - 'virtio-scsi-test.c', >>>>>> 'virtio-serial-test.c', >>>>>> 'vmxnet3-test.c', >>>>>> ) >>>>>> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI') >>>>>> + qos_test_ss.add( >>>>>> + 'virtio-blk-test.c', >>>>>> + 'virtio-net-test.c', >>>>>> + 'virtio-rng-test.c', >>>>>> + 'virtio-scsi-test.c', >>>>>> + ) >>>>>> +endif >>>>>> if have_virtfs >>>>>> qos_test_ss.add(files('virtio-9p-test.c')) >>>>>> endif >>>>>> --- >>>>>> >>>>>> I'll test that locally but not on Gitlab. >>>> >>>> This approach doesn't work for the iotests. >>>> >>>>> This also removes the virtio-devices test, I think we should keep the files, but in the files to >>>>> disable the PCI part when it is not available. >>>> I don't understand how the virtio devices are created, it seems there >>>> is an alias to generic virtio hw that map to the arch virtio bus. >>>> >>>> I was not obvious to understand why start the virt machine with >>>> "-device virtio-blk" returns "'virtio-blk-pci' is not a valid device >>>> model name" at first, then I figured out the qdev_alias_table array. >>>> >>>> Maybe you need to complete it for your arch? I've been using that: >>>> >>>> -- >8 -- >>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>>> index 8dc656becca..b326bd76c2a 100644 >>>> --- a/softmmu/qdev-monitor.c >>>> +++ b/softmmu/qdev-monitor.c >>>> @@ -65,8 +65,10 @@ static const QDevAlias qdev_alias_table[] = { >>>> { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X }, >>>> { "virtio-balloon-pci", "virtio-balloon", >>>> QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >>>> + { "virtio-blk-device", "virtio-blk", QEMU_ARCH_M68K }, >>>> { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X }, >>>> - { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >>>> + { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL >>>> + & ~(QEMU_ARCH_S390X | >>>> QEMU_ARCH_M68K) }, >>>> { "virtio-gpu-ccw", "virtio-gpu", QEMU_ARCH_S390X }, >>>> { "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >>>> { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X }, >>>> @@ -84,8 +86,10 @@ static const QDevAlias qdev_alias_table[] = { >>>> { "virtio-rng-pci", "virtio-rng", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >>>> { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X }, >>>> { "virtio-scsi-pci", "virtio-scsi", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X }, >>>> + { "virtio-serial-device", "virtio-serial", QEMU_ARCH_M68K }, >>>> { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X }, >>>> - { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL & >>>> ~QEMU_ARCH_S390X }, >>>> + { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL >>>> + & ~(QEMU_ARCH_S390X | >>>> QEMU_ARCH_M68K)}, >>>> { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X }, >>>> { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL & >>>> ~QEMU_ARCH_S390X }, >>>> { } >>>> --- >>>> >>>> But this looks ugly, I don't think it should work that way (because >>>> a machine could provide virtio buses over multiple transport, mmio >>>> and pci...). >>> >>> IMHO, this looks like the solution. >>> >>> The alias is to define the prefered way, on PCI it's the -pci one otherwise it is the -device one. >> >> See: >> >> commit 5f629d943cb0b11c37a891cf4f40a9166aee6f53 >> Author: Alexander Graf <agraf@csgraf.de> >> Date: Fri May 18 02:36:26 2012 +0200 >> >> s390x: fix s390 virtio aliases >> >> Some of the virtio devices have the same frontend name, but actually >> implement different devices behind the scenes through aliases. >> >> The indicator which device type to use is the architecture. On s390, we >> want s390 virtio devices. On everything else, we want PCI devices. >> >> Reflect this in the alias selection code. This way we fix commands like >> -device virtio-blk on s390x which with this patch applied select the >> correct virtio-blk-s390 device rather than virtio-blk-pci. >> >> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >> Signed-off-by: Alexander Graf <agraf@suse.de> > > So now than MMIO is available, we hit the "everything else" limit :) > > The other function I had to modify is in tests/qemu-iotests/iotests.py: > > def get_virtio_scsi_device(): > if qemu_default_machine == 's390-ccw-virtio': > return 'virtio-scsi-ccw' > return 'virtio-scsi-pci' > > But Max said there is no interest in testing the block devices here > (here = archs providing virtio via MMIO such ARM/m68k). (To elaborate on my perspective) I’m not exactly sure what you mean by this, i.e. whether you mean that we don’t want to test at all on that target, or that we don’t want to test specific devices. The former is not entirely true (but in practice kind of), the latter I think is true. I think we’d like to be able to provide developers the ability to run the iotests whatever target they’re working on, except where it’s just too complicated to do and nobody cares. The obvious problem is that often nobody cares, so there’s a low threshold to disabling stuff. (On s390, there was some pain, but people did care, so that’s a counter-story.) The threshold is especially low if it’s just to silence CI, obviously. The thing to note is that at least most iotests are not there to test the guest device, but rather the block layer; and the block layer is pretty much the same for every target, so there isn’t a big incentive for the block layer developers to have it run it on different targets. (I can’t rule out the possibility of a target-specific bug in how the block layer is used, which the iotests might reveal, but I don’t remember something like that to have happened so far.) From that it follows that I don’t see much use in testing specific devices either. Say there’s a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine for the iotests. I see little value in testing virtio-mmio as well. (Perhaps I’m short-sighted, though.) Max
On 18/03/2021 18.28, Max Reitz wrote: [...] > From that it follows that I don’t see much use in testing specific devices > either. Say there’s a platform that provides both virtio-pci and > virtio-mmio, the default (say virtio-pci) is fine for the iotests. I see > little value in testing virtio-mmio as well. (Perhaps I’m short-sighted, > though.) That's a fair point. But still, if someone compiled QEMU only with a target that only provided virtio-mmio, the iotests should not fail when running "make check". To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests only run with x86, aarch64, s390x and ppc64 ? Thomas
On 19.03.21 07:32, Thomas Huth wrote: > On 18/03/2021 18.28, Max Reitz wrote: > [...] >> From that it follows that I don’t see much use in testing specific >> devices either. Say there’s a platform that provides both virtio-pci >> and virtio-mmio, the default (say virtio-pci) is fine for the iotests. >> I see little value in testing virtio-mmio as well. (Perhaps I’m >> short-sighted, though.) > > That's a fair point. But still, if someone compiled QEMU only with a > target that only provided virtio-mmio, the iotests should not fail when > running "make check". > To avoid that we continue playing whack-a-mole here in the future, maybe > it would be better to restrict the iotests to the "main" targets only, > e.g. modify check-block.sh so that the tests only run with x86, aarch64, > s390x and ppc64 ? Right, that would certainly be the simplest solution. Max
On 19/03/21 10:20, Max Reitz wrote: > On 19.03.21 07:32, Thomas Huth wrote: >> On 18/03/2021 18.28, Max Reitz wrote: >> [...] >>> From that it follows that I don’t see much use in testing specific >>> devices either. Say there’s a platform that provides both virtio-pci >>> and virtio-mmio, the default (say virtio-pci) is fine for the >>> iotests. I see little value in testing virtio-mmio as well. (Perhaps >>> I’m short-sighted, though.) >> >> That's a fair point. But still, if someone compiled QEMU only with a >> target that only provided virtio-mmio, the iotests should not fail >> when running "make check". >> To avoid that we continue playing whack-a-mole here in the future, >> maybe it would be better to restrict the iotests to the "main" targets >> only, e.g. modify check-block.sh so that the tests only run with x86, >> aarch64, s390x and ppc64 ? > > Right, that would certainly be the simplest solution. It would also make the patches that Laurent sent this morning unnecessary, and avoid the use of aliases in the tests (so that it's clear what is tested). Paolo
Le 19/03/2021 à 10:20, Max Reitz a écrit : > On 19.03.21 07:32, Thomas Huth wrote: >> On 18/03/2021 18.28, Max Reitz wrote: >> [...] >>> From that it follows that I don’t see much use in testing specific devices either. Say there’s >>> a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine >>> for the iotests. I see little value in testing virtio-mmio as well. (Perhaps I’m short-sighted, >>> though.) >> >> That's a fair point. But still, if someone compiled QEMU only with a target that only provided >> virtio-mmio, the iotests should not fail when running "make check". >> To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to >> restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests only >> run with x86, aarch64, s390x and ppc64 ? > > Right, that would certainly be the simplest solution. > The problem with that is we can't run the tests if target-list doesn't contain one of these targets. Thanks, Laurent
Le 19/03/2021 à 10:29, Paolo Bonzini a écrit : > On 19/03/21 10:20, Max Reitz wrote: >> On 19.03.21 07:32, Thomas Huth wrote: >>> On 18/03/2021 18.28, Max Reitz wrote: >>> [...] >>>> From that it follows that I don’t see much use in testing specific devices either. Say there’s >>>> a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine >>>> for the iotests. I see little value in testing virtio-mmio as well. (Perhaps I’m short-sighted, >>>> though.) >>> >>> That's a fair point. But still, if someone compiled QEMU only with a target that only provided >>> virtio-mmio, the iotests should not fail when running "make check". >>> To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to >>> restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests >>> only run with x86, aarch64, s390x and ppc64 ? >> >> Right, that would certainly be the simplest solution. > > It would also make the patches that Laurent sent this morning unnecessary, and avoid the use of > aliases in the tests (so that it's clear what is tested). > We don't test the virtio frontend, but the blockdev backend, so we don't care what we use here. Aliases simplify the code... Thanks, Laurent
On 19.03.21 11:50, Laurent Vivier wrote: > Le 19/03/2021 à 10:20, Max Reitz a écrit : >> On 19.03.21 07:32, Thomas Huth wrote: >>> On 18/03/2021 18.28, Max Reitz wrote: >>> [...] >>>> From that it follows that I don’t see much use in testing specific devices either. Say there’s >>>> a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine >>>> for the iotests. I see little value in testing virtio-mmio as well. (Perhaps I’m short-sighted, >>>> though.) >>> >>> That's a fair point. But still, if someone compiled QEMU only with a target that only provided >>> virtio-mmio, the iotests should not fail when running "make check". >>> To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to >>> restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests only >>> run with x86, aarch64, s390x and ppc64 ? >> >> Right, that would certainly be the simplest solution. >> > > The problem with that is we can't run the tests if target-list doesn't contain one of these targets. Yes, but is that really a problem? Max
On 19/03/2021 11.50, Laurent Vivier wrote: > Le 19/03/2021 à 10:20, Max Reitz a écrit : >> On 19.03.21 07:32, Thomas Huth wrote: >>> On 18/03/2021 18.28, Max Reitz wrote: >>> [...] >>>> From that it follows that I don’t see much use in testing specific devices either. Say there’s >>>> a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine >>>> for the iotests. I see little value in testing virtio-mmio as well. (Perhaps I’m short-sighted, >>>> though.) >>> >>> That's a fair point. But still, if someone compiled QEMU only with a target that only provided >>> virtio-mmio, the iotests should not fail when running "make check". >>> To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to >>> restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests only >>> run with x86, aarch64, s390x and ppc64 ? >> >> Right, that would certainly be the simplest solution. >> > > The problem with that is we can't run the tests if target-list doesn't contain one of these targets. The iotests are skipped in quite a bunch of cases already anyway, e.g. when GNU sed or bash are not available in the right version. So I think it would be also ok to skip them in builds without one of the major architectures. Anyway, since your patches are already ready, I think we also could simply go with those this time, and reconsider tweaking tests/check-block.sh the next time we run into this issue. Thomas
On 19.03.21 11:51, Max Reitz wrote: > On 19.03.21 11:50, Laurent Vivier wrote: >> Le 19/03/2021 à 10:20, Max Reitz a écrit : >>> On 19.03.21 07:32, Thomas Huth wrote: >>>> On 18/03/2021 18.28, Max Reitz wrote: >>>> [...] >>>>> From that it follows that I don’t see much use in testing >>>>> specific devices either. Say there’s >>>>> a platform that provides both virtio-pci and virtio-mmio, the >>>>> default (say virtio-pci) is fine >>>>> for the iotests. I see little value in testing virtio-mmio as >>>>> well. (Perhaps I’m short-sighted, >>>>> though.) >>>> >>>> That's a fair point. But still, if someone compiled QEMU only with a >>>> target that only provided >>>> virtio-mmio, the iotests should not fail when running "make check". >>>> To avoid that we continue playing whack-a-mole here in the future, >>>> maybe it would be better to >>>> restrict the iotests to the "main" targets only, e.g. modify >>>> check-block.sh so that the tests only >>>> run with x86, aarch64, s390x and ppc64 ? >>> >>> Right, that would certainly be the simplest solution. >>> >> >> The problem with that is we can't run the tests if target-list doesn't >> contain one of these targets. > > Yes, but is that really a problem? I should add: The thing is, I wouldn’t really call it a problem. But still, as I said before somewhere in this thread, in theory we want to allow running the tests with every configuration. It’s just that it’s a tradeoff between how much it helps and how much work it is to make them work. (I gave s390 as an example, where effort was undertaken to make the iotests work.) You’ve sent patches, so it seems you’re willing to invest the work. Sounds good to me, as long as we know it won’t rot. Max
On 19/03/21 11:51, Laurent Vivier wrote: >> It would also make the patches that Laurent sent this morning unnecessary, and avoid the use of >> aliases in the tests (so that it's clear what is tested). > > We don't test the virtio frontend, but the blockdev backend, so we don't care what we use here. Sort of, see for example the iothreads which, with your patch, would not be covered anymore on s390. > Aliases simplify the code... Aliases are not deprecated but... let's say despised, because of the special casing they add. But yes, the code simplification from your patch is hard to brush away. So I agree, since you have already mostly written the patches let's just complete them. Paolo
diff --git a/default-configs/devices/m68k-softmmu.mak b/default-configs/devices/m68k-softmmu.mak index 6629fd2aa330..7f8619e42786 100644 --- a/default-configs/devices/m68k-softmmu.mak +++ b/default-configs/devices/m68k-softmmu.mak @@ -8,3 +8,4 @@ CONFIG_AN5206=y CONFIG_MCF5208=y CONFIG_NEXTCUBE=y CONFIG_Q800=y +CONFIG_M68K_VIRT=y diff --git a/include/standard-headers/asm-m68k/bootinfo-virt.h b/include/standard-headers/asm-m68k/bootinfo-virt.h new file mode 100644 index 000000000000..81be1e092497 --- /dev/null +++ b/include/standard-headers/asm-m68k/bootinfo-virt.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +/* +** asm/bootinfo-virt.h -- Virtual-m68k-specific boot information definitions +*/ + +#ifndef _UAPI_ASM_M68K_BOOTINFO_VIRT_H +#define _UAPI_ASM_M68K_BOOTINFO_VIRT_H + +#define BI_VIRT_QEMU_VERSION 0x8000 +#define BI_VIRT_GF_PIC_BASE 0x8001 +#define BI_VIRT_GF_RTC_BASE 0x8002 +#define BI_VIRT_GF_TTY_BASE 0x8003 +#define BI_VIRT_VIRTIO_BASE 0x8004 +#define BI_VIRT_CTRL_BASE 0x8005 + +#define VIRT_BOOTI_VERSION MK_BI_VERSION(2, 0) + +#endif /* _UAPI_ASM_M68K_BOOTINFO_MAC_H */ diff --git a/hw/m68k/virt.c b/hw/m68k/virt.c new file mode 100644 index 000000000000..e9a5d4c69b97 --- /dev/null +++ b/hw/m68k/virt.c @@ -0,0 +1,313 @@ +/* + * SPDX-License-Identifer: GPL-2.0-or-later + * + * QEMU Vitual M68K Machine + * + * (c) 2020 Laurent Vivier <laurent@vivier.eu> + * + */ + +#include "qemu/osdep.h" +#include "qemu/units.h" +#include "qemu-common.h" +#include "sysemu/sysemu.h" +#include "cpu.h" +#include "hw/hw.h" +#include "hw/boards.h" +#include "hw/irq.h" +#include "hw/qdev-properties.h" +#include "elf.h" +#include "hw/loader.h" +#include "ui/console.h" +#include "exec/address-spaces.h" +#include "hw/sysbus.h" +#include "standard-headers/asm-m68k/bootinfo.h" +#include "standard-headers/asm-m68k/bootinfo-virt.h" +#include "bootinfo.h" +#include "net/net.h" +#include "qapi/error.h" +#include "sysemu/qtest.h" +#include "sysemu/runstate.h" +#include "sysemu/reset.h" + +#include "hw/intc/m68k_irqc.h" +#include "hw/misc/virt_ctrl.h" +#include "hw/char/goldfish_tty.h" +#include "hw/rtc/goldfish_rtc.h" +#include "hw/intc/goldfish_pic.h" +#include "hw/virtio/virtio-mmio.h" +#include "hw/virtio/virtio-blk.h" + +/* + * 6 goldfish-pic for CPU IRQ #1 to IRQ #6 + * CPU IRQ #1 -> PIC #1 + * IRQ #1 to IRQ #31 -> unused + * IRQ #32 -> goldfish-tty + * CPU IRQ #2 -> PIC #2 + * IRQ #1 to IRQ #32 -> virtio-mmio from 1 to 32 + * CPU IRQ #3 -> PIC #3 + * IRQ #1 to IRQ #32 -> virtio-mmio from 33 to 64 + * CPU IRQ #4 -> PIC #4 + * IRQ #1 to IRQ #32 -> virtio-mmio from 65 to 96 + * CPU IRQ #5 -> PIC #5 + * IRQ #1 to IRQ #32 -> virtio-mmio from 97 to 128 + * CPU IRQ #6 -> PIC #6 + * IRQ #1 -> goldfish-rtc + * IRQ #2 to IRQ #32 -> unused + * CPU IRQ #7 -> NMI + */ + +#define PIC_IRQ_BASE(num) (8 + (num - 1) * 32) +#define PIC_IRQ(num, irq) (PIC_IRQ_BASE(num) + irq - 1) +#define PIC_GPIO(pic_irq) (qdev_get_gpio_in(pic_dev[(pic_irq - 8) / 32], \ + (pic_irq - 8) % 32)) + +#define VIRT_GF_PIC_MMIO_BASE 0xff000000 /* MMIO: 0xff000000 - 0xff005fff */ +#define VIRT_GF_PIC_IRQ_BASE 1 /* IRQ: #1 -> #6 */ +#define VIRT_GF_PIC_NB 6 + +/* 2 goldfish-rtc (and timer) */ +#define VIRT_GF_RTC_MMIO_BASE 0xff006000 /* MMIO: 0xff006000 - 0xff007fff */ +#define VIRT_GF_RTC_IRQ_BASE PIC_IRQ(6, 1) /* PIC: #6, IRQ: #1 */ +#define VIRT_GF_RTC_NB 2 + +/* 1 goldfish-tty */ +#define VIRT_GF_TTY_MMIO_BASE 0xff008000 /* MMIO: 0xff008000 - 0xff008fff */ +#define VIRT_GF_TTY_IRQ_BASE PIC_IRQ(1, 32) /* PIC: #1, IRQ: #32 */ + +/* 1 virt-ctrl */ +#define VIRT_CTRL_MMIO_BASE 0xff009000 /* MMIO: 0xff009000 - 0xff009fff */ +#define VIRT_CTRL_IRQ_BASE PIC_IRQ(1, 1) /* PIC: #1, IRQ: #1 */ + +/* + * virtio-mmio size is 0x200 bytes + * we use 4 goldfish-pic to attach them, + * we can attach 32 virtio devices / goldfish-pic + * -> we can manage 32 * 4 = 128 virtio devices + */ +#define VIRT_VIRTIO_MMIO_BASE 0xff010000 /* MMIO: 0xff010000 - 0xff01ffff */ +#define VIRT_VIRTIO_IRQ_BASE PIC_IRQ(2, 1) /* PIC: 2, 3, 4, 5, IRQ: ALL */ + +static void main_cpu_reset(void *opaque) +{ + M68kCPU *cpu = opaque; + CPUState *cs = CPU(cpu); + + cpu_reset(cs); + cpu->env.aregs[7] = ldl_phys(cs->as, 0); + cpu->env.pc = ldl_phys(cs->as, 4); +} + +static void virt_init(MachineState *machine) +{ + M68kCPU *cpu = NULL; + int32_t kernel_size; + uint64_t elf_entry; + ram_addr_t initrd_base; + int32_t initrd_size; + ram_addr_t ram_size = machine->ram_size; + const char *kernel_filename = machine->kernel_filename; + const char *initrd_filename = machine->initrd_filename; + const char *kernel_cmdline = machine->kernel_cmdline; + hwaddr parameters_base; + DeviceState *dev; + DeviceState *irqc_dev; + DeviceState *pic_dev[VIRT_GF_PIC_NB]; + SysBusDevice *sysbus; + hwaddr io_base; + int i; + + if (ram_size > 3399672 * KiB) { + /* + * The physical memory can be up to 4 GiB - 16 MiB, but linux + * kernel crashes after this limit (~ 3.2 GiB) + */ + error_report("Too much memory for this machine: %" PRId64 " KiB, " + "maximum 3399672 KiB", ram_size / KiB); + exit(1); + } + + /* init CPUs */ + cpu = M68K_CPU(cpu_create(machine->cpu_type)); + qemu_register_reset(main_cpu_reset, cpu); + + /* RAM */ + memory_region_add_subregion(get_system_memory(), 0, machine->ram); + + /* IRQ Controller */ + + irqc_dev = qdev_new(TYPE_M68K_IRQC); + sysbus_realize_and_unref(SYS_BUS_DEVICE(irqc_dev), &error_fatal); + + /* + * 6 goldfish-pic + * + * map: 0xff000000 - 0xff006fff = 28 KiB + * IRQ: #1 (lower priority) -> #6 (higher priority) + * + */ + io_base = VIRT_GF_PIC_MMIO_BASE; + for (i = 0; i < VIRT_GF_PIC_NB; i++) { + pic_dev[i] = qdev_new(TYPE_GOLDFISH_PIC); + sysbus = SYS_BUS_DEVICE(pic_dev[i]); + qdev_prop_set_uint8(pic_dev[i], "index", i); + sysbus_realize_and_unref(sysbus, &error_fatal); + + sysbus_mmio_map(sysbus, 0, io_base); + sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(irqc_dev, i)); + + io_base += 0x1000; + } + + /* goldfish-rtc */ + io_base = VIRT_GF_RTC_MMIO_BASE; + for (i = 0; i < VIRT_GF_RTC_NB; i++) { + dev = qdev_new(TYPE_GOLDFISH_RTC); + sysbus = SYS_BUS_DEVICE(dev); + sysbus_realize_and_unref(sysbus, &error_fatal); + sysbus_mmio_map(sysbus, 0, io_base); + sysbus_connect_irq(sysbus, 0, PIC_GPIO(VIRT_GF_RTC_IRQ_BASE + i)); + + io_base += 0x1000; + } + + /* goldfish-tty */ + dev = qdev_new(TYPE_GOLDFISH_TTY); + sysbus = SYS_BUS_DEVICE(dev); + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); + sysbus_realize_and_unref(sysbus, &error_fatal); + sysbus_mmio_map(sysbus, 0, VIRT_GF_TTY_MMIO_BASE); + sysbus_connect_irq(sysbus, 0, PIC_GPIO(VIRT_GF_TTY_IRQ_BASE)); + + /* virt controller */ + dev = qdev_new(TYPE_VIRT_CTRL); + sysbus = SYS_BUS_DEVICE(dev); + sysbus_realize_and_unref(sysbus, &error_fatal); + sysbus_mmio_map(sysbus, 0, VIRT_CTRL_MMIO_BASE); + sysbus_connect_irq(sysbus, 0, PIC_GPIO(VIRT_CTRL_IRQ_BASE)); + + /* virtio-mmio */ + io_base = VIRT_VIRTIO_MMIO_BASE; + for (i = 0; i < 128; i++) { + dev = qdev_new(TYPE_VIRTIO_MMIO); + qdev_prop_set_bit(dev, "force-legacy", false); + sysbus = SYS_BUS_DEVICE(dev); + sysbus_realize_and_unref(sysbus, &error_fatal); + sysbus_connect_irq(sysbus, 0, PIC_GPIO(VIRT_VIRTIO_IRQ_BASE + i)); + sysbus_mmio_map(sysbus, 0, io_base); + io_base += 0x200; + } + + if (kernel_filename) { + CPUState *cs = CPU(cpu); + uint64_t high; + + kernel_size = load_elf(kernel_filename, NULL, NULL, NULL, + &elf_entry, NULL, &high, NULL, 1, + EM_68K, 0, 0); + if (kernel_size < 0) { + error_report("could not load kernel '%s'", kernel_filename); + exit(1); + } + stl_phys(cs->as, 4, elf_entry); /* reset initial PC */ + parameters_base = (high + 1) & ~1; + + BOOTINFO1(cs->as, parameters_base, BI_MACHTYPE, MACH_VIRT); + BOOTINFO1(cs->as, parameters_base, BI_FPUTYPE, FPU_68040); + BOOTINFO1(cs->as, parameters_base, BI_MMUTYPE, MMU_68040); + BOOTINFO1(cs->as, parameters_base, BI_CPUTYPE, CPU_68040); + BOOTINFO2(cs->as, parameters_base, BI_MEMCHUNK, 0, ram_size); + + BOOTINFO1(cs->as, parameters_base, BI_VIRT_QEMU_VERSION, + ((QEMU_VERSION_MAJOR << 24) | (QEMU_VERSION_MINOR << 16) | + (QEMU_VERSION_MICRO << 8))); + BOOTINFO2(cs->as, parameters_base, BI_VIRT_GF_PIC_BASE, + VIRT_GF_PIC_MMIO_BASE, VIRT_GF_PIC_IRQ_BASE); + BOOTINFO2(cs->as, parameters_base, BI_VIRT_GF_RTC_BASE, + VIRT_GF_RTC_MMIO_BASE, VIRT_GF_RTC_IRQ_BASE); + BOOTINFO2(cs->as, parameters_base, BI_VIRT_GF_TTY_BASE, + VIRT_GF_TTY_MMIO_BASE, VIRT_GF_TTY_IRQ_BASE); + BOOTINFO2(cs->as, parameters_base, BI_VIRT_CTRL_BASE, + VIRT_CTRL_MMIO_BASE, VIRT_CTRL_IRQ_BASE); + BOOTINFO2(cs->as, parameters_base, BI_VIRT_VIRTIO_BASE, + VIRT_VIRTIO_MMIO_BASE, VIRT_VIRTIO_IRQ_BASE); + + if (kernel_cmdline) { + BOOTINFOSTR(cs->as, parameters_base, BI_COMMAND_LINE, + kernel_cmdline); + } + + /* load initrd */ + if (initrd_filename) { + initrd_size = get_image_size(initrd_filename); + if (initrd_size < 0) { + error_report("could not load initial ram disk '%s'", + initrd_filename); + exit(1); + } + + initrd_base = (ram_size - initrd_size) & TARGET_PAGE_MASK; + load_image_targphys(initrd_filename, initrd_base, + ram_size - initrd_base); + BOOTINFO2(cs->as, parameters_base, BI_RAMDISK, initrd_base, + initrd_size); + } else { + initrd_base = 0; + initrd_size = 0; + } + BOOTINFO0(cs->as, parameters_base, BI_LAST); + } +} + +static void virt_machine_class_init(ObjectClass *oc, void *data) +{ + MachineClass *mc = MACHINE_CLASS(oc); + mc->desc = "QEMU M68K Virtual Machine"; + mc->init = virt_init; + mc->default_cpu_type = M68K_CPU_TYPE_NAME("m68040"); + mc->max_cpus = 1; + mc->no_floppy = 1; + mc->no_parallel = 1; + mc->default_ram_id = "m68k_virt.ram"; +} + +static const TypeInfo virt_machine_info = { + .name = MACHINE_TYPE_NAME("virt"), + .parent = TYPE_MACHINE, + .abstract = true, + .class_init = virt_machine_class_init, +}; + +static void virt_machine_register_types(void) +{ + type_register_static(&virt_machine_info); +} + +type_init(virt_machine_register_types) + +#define DEFINE_VIRT_MACHINE(major, minor, latest) \ + static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ + void *data) \ + { \ + MachineClass *mc = MACHINE_CLASS(oc); \ + virt_machine_##major##_##minor##_options(mc); \ + mc->desc = "QEMU " # major "." # minor " M68K Virtual Machine"; \ + if (latest) { \ + mc->alias = "virt"; \ + } \ + } \ + static const TypeInfo machvirt_##major##_##minor##_info = { \ + .name = MACHINE_TYPE_NAME("virt-" # major "." # minor), \ + .parent = MACHINE_TYPE_NAME("virt"), \ + .class_init = virt_##major##_##minor##_class_init, \ + }; \ + static void machvirt_machine_##major##_##minor##_init(void) \ + { \ + type_register_static(&machvirt_##major##_##minor##_info); \ + } \ + type_init(machvirt_machine_##major##_##minor##_init); + +static void virt_machine_6_0_options(MachineClass *mc) +{ +} +DEFINE_VIRT_MACHINE(6, 0, true) diff --git a/MAINTAINERS b/MAINTAINERS index 5ca3c9f851f9..682e4ed48152 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1130,6 +1130,19 @@ F: include/hw/nubus/* F: include/hw/display/macfb.h F: include/hw/block/swim.h +virt +M: Laurent Vivier <laurent@vivier.eu> +S: Maintained +F: hw/m68k/virt.c +F: hw/char/goldfish_tty.c +F: hw/intc/goldfish_pic.c +F: hw/intc/m68k_irqc.c +F: hw/misc/virt_ctrl.c +F: include/hw/char/goldfish_tty.h +F: include/hw/intc/goldfish_pic.h +F: include/hw/intc/m68k_irqc.h +F: include/hw/misc/virt_ctrl.h + MicroBlaze Machines ------------------- petalogix_s3adsp1800 diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig index 60d7bcfb8f2b..f839f8a03064 100644 --- a/hw/m68k/Kconfig +++ b/hw/m68k/Kconfig @@ -23,3 +23,12 @@ config Q800 select ESP select DP8393X select OR_IRQ + +config M68K_VIRT + bool + select M68K_IRQC + select VIRT_CTRL + select GOLDFISH_PIC + select GOLDFISH_TTY + select GOLDFISH_RTC + select VIRTIO_MMIO diff --git a/hw/m68k/meson.build b/hw/m68k/meson.build index ca0044c652d3..31248641d301 100644 --- a/hw/m68k/meson.build +++ b/hw/m68k/meson.build @@ -3,5 +3,6 @@ m68k_ss.add(when: 'CONFIG_AN5206', if_true: files('an5206.c', 'mcf5206.c')) m68k_ss.add(when: 'CONFIG_MCF5208', if_true: files('mcf5208.c', 'mcf_intc.c')) m68k_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('next-kbd.c', 'next-cube.c')) m68k_ss.add(when: 'CONFIG_Q800', if_true: files('q800.c')) +m68k_ss.add(when: 'CONFIG_M68K_VIRT', if_true: files('virt.c')) hw_arch += {'m68k': m68k_ss}