Re: [PATCH v6 0/4] Make the mc146818 RTC device target independent

[Mark Cave-Ayland]

On 10/01/2023 09:53, Thomas Huth wrote:

> The basic idea of this patch set is to change hw/rtc/mc146818rtc.c into
> target independent code so that the file only has to be compiled once
> instead of multiple times (and that it can be used in a qemu-system-all
> binary once we get there).
>
> The first patch extracts some functions from the APIC code that will be
> required for linking when the mc146818rtc becomes target-independent.
>
> The second patch adds a new way for checking whether the "driftfix=slew"
> policy is available or not (since the corresponding #ifdefs in the
> mc146818rtc code will be removed).
>
> The third patch then removes the "#ifdef TARGET" switches and turns
> the mc146818rtc code into a target-independent file.
>
> The fourth patch just fixes a small cosmetic nit that I discovered along
> the way: On systems without mc146818, the "-rtc driftfix=slew" simply
> got ignored silently. We should at least emit a warning in this case.
>
> Changes since last iteration:
> - Dropped the approach of using a new "slew-tick-policy-available"
>    property that needs to be set by the machine code (and thus dropped
>    the clean-up patches from Bernhard from this series since they are
>    no longer required here now)
> - Use a new check in hw/core/qdev-properties-system.c instead
>    (see the second patch)
>
> Thomas Huth (4):
>    hw/intc: Extract the IRQ counting functions into a separate file
>    hw/core/qdev-properties-system: Allow the 'slew' policy only on x86
>    hw/rtc/mc146818rtc: Make the mc146818 RTC device target independent
>    softmmu/rtc: Emit warning when using driftfix=slew on systems without
>      mc146818
>
>   include/hw/i386/apic.h           |  2 --
>   include/hw/i386/apic_internal.h  |  1 -
>   include/hw/intc/kvm_irqcount.h   | 10 +++++++
>   include/hw/rtc/mc146818rtc.h     |  1 +
>   hw/core/qdev-properties-system.c | 28 +++++++++++++++++-
>   hw/i386/kvm/i8259.c              |  4 +--
>   hw/i386/kvm/ioapic.c             |  4 +--
>   hw/intc/apic.c                   |  3 +-
>   hw/intc/apic_common.c            | 30 ++-----------------
>   hw/intc/kvm_irqcount.c           | 49 ++++++++++++++++++++++++++++++++
>   hw/rtc/mc146818rtc.c             | 20 ++-----------
>   softmmu/rtc.c                    |  6 +++-
>   hw/intc/meson.build              |  6 ++++
>   hw/intc/trace-events             |  9 +++---
>   hw/rtc/meson.build               |  3 +-
>   15 files changed, 115 insertions(+), 61 deletions(-)
>   create mode 100644 include/hw/intc/kvm_irqcount.h
>   create mode 100644 hw/intc/kvm_irqcount.c

This looks much better than the previous approaches - thanks for working on this! 
Looks good to me, so:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.