Re: [PATCH 1/3] hw/misc/pvpanic: split-out generic and bus dependent code

[Peter Maydell]

On Fri, 18 Dec 2020 at 13:36, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> To ease the PCI device addition in next patches, split the code as follows:
> - generic code (read/write/setup) is being kept in pvpanic.c
> - ISA dependent code moved to pvpanic-isa.c
>
> Also, rename:
> - ISA_PVPANIC_DEVICE -> PVPANIC_ISA_DEVICE.
> - TYPE_PVPANIC -> TYPE_PVPANIC_ISA.
> - MemoryRegion io -> mr.
> - pvpanic_ioport_* in pvpanic_*.
>
> Update the build system with the new files and config structure.
>
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  hw/i386/Kconfig           |  1 -
>  hw/misc/Kconfig           |  7 +++-
>  hw/misc/meson.build       |  3 +-
>  hw/misc/pvpanic-isa.c     | 95 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  hw/misc/pvpanic.c         | 85 +++---------------------------------------
>  include/hw/misc/pvpanic.h | 23 +++++++++++-
>  tests/qtest/meson.build   |  2 +-
>  7 files changed, 131 insertions(+), 85 deletions(-)
>  create mode 100644 hw/misc/pvpanic-isa.c

Hi; couple of minor review issues below but mostly this looks good.

> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index eea059f..994fcaa 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -14,7 +14,6 @@ config PC
>      imply ISA_DEBUG
>      imply PARALLEL
>      imply PCI_DEVICES
> -    imply PVPANIC
>      imply QXL
>      imply SEV
>      imply SGA

Why is it ok for this imply line to just go away rather
than changing to "imply PVPANIC_ISA" ? I think the answer is
the "default y" in the Kconfig file below, but really that
is changing behaviour (making PVPANIC available on all
guests with ISA, not just PC) -- so it ought to be done
in a separate patch, so this one can be purely refactoring
with no user-visible changes.

> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index cf18ac0..b58e6fd 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -121,9 +121,14 @@ config IOTKIT_SYSCTL
>  config IOTKIT_SYSINFO
>      bool
>
> -config PVPANIC
> +config PVPANIC_COMMON
>      bool
> +
> +config PVPANIC_ISA
> +    bool
> +    default y
>      depends on ISA_BUS
> +    select PVPANIC_COMMON
>
>  config AUX
>      bool



> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 6a67c53..d7b5a9e 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -33,7 +33,7 @@ qtests_i386 = \
>    (config_host.has_key('CONFIG_LINUX') and                                   
>                \
>     config_all_devices.has_key('CONFIG_ISA_IPMI_BT') ? ['ipmi-bt-test'] : []) 
> +              \
>    (config_all_devices.has_key('CONFIG_WDT_IB700') ? ['wdt_ib700-test'] : []) 
> +              \
> -  (config_all_devices.has_key('CONFIG_PVPANIC') ? ['pvpanic-test'] : []) +   
>                \
> +  (config_all_devices.has_key('CONFIG_PVPANIC_COMMON') ? ['pvpanic-test'] : 
> []) +           \
>    (config_all_devices.has_key('CONFIG_HDA') ? ['intel-hda-test'] : []) +     
>                \
>    (config_all_devices.has_key('CONFIG_I82801B11') ? ['i82801b11-test'] : []) 
> +             \
>    (config_all_devices.has_key('CONFIG_IOH3420') ? ['ioh3420-test'] : []) +   
>                \

The pvpanic-test test only tests the ISA device, so I think it
either needs to be only built for CONFIG_PVPANIC_ISA, or
the tests themselves within the pvpanic-test.c file would need to
be updated to somehow skip if the QEMU under test didn't have
the ISA pvpanic device. The former seems easier.

While I'm talking about tests, it would be nice to have a
basic test of the new pvpanic-pci device too. If you put that
in its own pvpanic-pci-test.c file then you can make that one
be dependent on CONFIG_PVPANIC_PCI.

thanks
-- PMM