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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 12/12/22 14:48, Thomas Huth wrote:
> 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" ...

Replace it by ArchCPU ;)
> 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.

I concur.