Re: [PATCH v3] hw/rtc/mc146818rtc: Make this rtc device target independent

[Thomas Huth]

On 12/12/2022 14.39, Mark Cave-Ayland wrote:
> On 12/12/2022 07:56, Thomas Huth wrote:
>
> > The only reason for this code being target dependent is the apic-related
> > code in rtc_policy_slew_deliver_irq(). Since these apic functions are rather
> > simple, we can easily move them into a new, separate file (apic_irqcount.c)
> > which will always be compiled and linked if either APIC or the mc146818 
> > device
> > are required. This way we can get rid of the #ifdef TARGET_I386 switches in
> > mc146818rtc.c and declare it in the softmmu_ss instead of specific_ss, so
> > that the code only gets compiled once for all targets.
> >
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   v3: Move TYPE_APIC_COMMON from apic_internal.h to apic.h and use it
> >
> >   include/hw/i386/apic.h          |  2 ++
> >   include/hw/i386/apic_internal.h |  2 --
> >   include/hw/rtc/mc146818rtc.h    |  1 +
> >   hw/intc/apic_common.c           | 27 -----------------
> >   hw/intc/apic_irqcount.c         | 53 +++++++++++++++++++++++++++++++++
> >   hw/rtc/mc146818rtc.c            | 25 +++++-----------
> >   hw/intc/meson.build             |  6 +++-
> >   hw/rtc/meson.build              |  3 +-
> >   8 files changed, 69 insertions(+), 50 deletions(-)
> >   create mode 100644 hw/intc/apic_irqcount.c
> >
> > diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
> > index da1d2fe155..24069fb961 100644
> > --- a/include/hw/i386/apic.h
> > +++ b/include/hw/i386/apic.h
> > @@ -1,6 +1,7 @@
> >   #ifndef APIC_H
> >   #define APIC_H
> > +#define TYPE_APIC_COMMON "apic-common"
>
> Ah sorry, I should have been more specific here: what I was suggesting was 
> to move the entire QOM type information into apic.h as per the normal 
> convention, as opposed to just the #define. At first glance that would 
> involve lines 128-190 in apic_internal.h which would also bring in 
> APICCommonClass and APICCommonState - possibly the change may warrant its 
> own commit.

At least APICCommonState is target specific since it uses "X86CPU" ... so 
moving that to apic.h would be very counterproductive here.

Anyway, moving those structs is certainly way more than what is required for 
this patch, so if we decide to move anything else related to the APIC, it 
should be done in a separate patch later.

 Thomas