Make the mc146818 RTC device target independent

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

Posted by Thomas Huth 1 year, 3 months ago

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

-- 
2.31.1

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

Posted by Philippe Mathieu-Daudé 1 year, 3 months ago

On 10/1/23 10: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).

> 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

I was happy enough with each iterations, but the changes requested over
each iter was worth having a v6, it is now very clean.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

Posted by Philippe Mathieu-Daudé 1 year, 3 months ago

Hi Michael,

On 10/1/23 10: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).

> 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

I am queuing this series via mips-next tree if you don't object
(all patches reviewed multiple times, in case there is an issue,
we are early enough in the release cycle to fix it).

Regards,

Phil.

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

Posted by Mark Cave-Ayland 1 year, 3 months ago

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.