Re: [Qemu-devel] [PATCH RFC qom-cpu-next] apic: QOM'ify APICCommonState casts

[Andreas Färber]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 28.04.2013 15:03, schrieb Jan Kiszka:
> On 2013-04-28 13:22, Andreas Färber wrote:
>> Necessary to change the name of ICCDevice's parent object field.
>> 
>> Signed-off-by: Andreas Färber <address@hidden> --- Could any of
>> the APIC experts please review whether I'm touching any hot
>> path? Not sure if this was a mix of pre- and post-QOM code or
>> intentional... Thanks.
> 
> How "hot" does it have to be before we notice the type check
> overhead? Did anyone already measure this, given that they are
> getting very common now?

Heh, if I had a conclusive benchmark I wouldn't ask. ;)

In general init, reset and MMIO were considered cold paths in this
regard. Timer and interrupt code paths by contrast I've usually tried
to spare.

> But none of the touched functions are used during normal operation
> when the KVM APIC is active. With emulated APIC, they are
> frequently used, of course.

Where in doubt, we could just use (APICCommonState *) or a macro
hiding that.

Andreas

>> hw/i386/kvm/apic.c       |  4 ++-- hw/intc/apic.c           | 20
>> +++++++++++--------- hw/intc/apic_common.c    | 10 +++++----- 
>> include/hw/cpu/icc_bus.h |  2 +- 4 files changed, 19
>> insertions(+), 17 deletions(-)
>> 
>> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c index
>> 8f80425..0fe31ae 100644 --- a/hw/i386/kvm/apic.c +++
>> b/hw/i386/kvm/apic.c @@ -27,7 +27,7 @@ static inline uint32_t
>> kvm_apic_get_reg(struct kvm_lapic_state *kapic,
>> 
>> void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state
>> *kapic) { -    APICCommonState *s = DO_UPCAST(APICCommonState,
>> busdev.qdev, d); +    APICCommonState *s = APIC_COMMON(d); int
>> i;
>> 
>> memset(kapic, 0, sizeof(*kapic)); @@ -53,7 +53,7 @@ void
>> kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state
>> *kapic)
>> 
>> void kvm_get_apic_state(DeviceState *d, struct kvm_lapic_state
>> *kapic) { -    APICCommonState *s = DO_UPCAST(APICCommonState,
>> busdev.qdev, d); +    APICCommonState *s = APIC_COMMON(d); int i,
>> v;
>> 
>> s->id = kvm_apic_get_reg(kapic, 0x2) >> 24; diff --git
>> a/hw/intc/apic.c b/hw/intc/apic.c index 756dff0..f171620 100644 
>> --- a/hw/intc/apic.c +++ b/hw/intc/apic.c @@ -173,7 +173,7 @@
>> static void apic_local_deliver(APICCommonState *s, int vector)
>> 
>> void apic_deliver_pic_intr(DeviceState *d, int level) { -
>> APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d); 
>> +    APICCommonState *s = APIC_COMMON(d);
>> 
>> if (level) { apic_local_deliver(s, APIC_LVT_LINT0); @@ -484,7
>> +484,7 @@ static void apic_startup(APICCommonState *s, int
>> vector_num)
>> 
>> void apic_sipi(DeviceState *d) { -    APICCommonState *s =
>> DO_UPCAST(APICCommonState, busdev.qdev, d); +    APICCommonState
>> *s = APIC_COMMON(d);
>> 
>> cpu_reset_interrupt(CPU(s->cpu), CPU_INTERRUPT_SIPI);
>> 
>> @@ -498,7 +498,7 @@ static void apic_deliver(DeviceState *d,
>> uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode, uint8_t
>> vector_num, uint8_t trigger_mode) { -    APICCommonState *s =
>> DO_UPCAST(APICCommonState, busdev.qdev, d); +    APICCommonState
>> *s = APIC_COMMON(d); uint32_t deliver_bitmask[MAX_APIC_WORDS]; 
>> int dest_shorthand = (s->icr[0] >> 18) & 3; APICCommonState
>> *apic_iter; @@ -544,16 +544,18 @@ static void
>> apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
>> 
>> static bool apic_check_pic(APICCommonState *s) { -    if
>> (!apic_accept_pic_intr(&s->busdev.qdev) ||
>> !pic_get_output(isa_pic)) { +    DeviceState *dev = DEVICE(s); + 
>> +    if (!apic_accept_pic_intr(dev) || !pic_get_output(isa_pic))
>> { return false; } -    apic_deliver_pic_intr(&s->busdev.qdev,
>> 1); +    apic_deliver_pic_intr(dev, 1); return true; }
>> 
>> int apic_get_interrupt(DeviceState *d) { -    APICCommonState *s
>> = DO_UPCAST(APICCommonState, busdev.qdev, d); +
>> APICCommonState *s = APIC_COMMON(d); int intno;
>> 
>> /* if the APIC is installed or enabled, we let the 8259 handle
>> the @@ -587,7 +589,7 @@ int apic_get_interrupt(DeviceState *d)
>> 
>> int apic_accept_pic_intr(DeviceState *d) { -    APICCommonState
>> *s = DO_UPCAST(APICCommonState, busdev.qdev, d); +
>> APICCommonState *s = APIC_COMMON(d); uint32_t lvt0;
>> 
>> if (!s) @@ -666,7 +668,7 @@ static uint32_t apic_mem_readl(void
>> *opaque, hwaddr addr) if (!d) { return 0; } -    s =
>> DO_UPCAST(APICCommonState, busdev.qdev, d); +    s =
>> APIC_COMMON(d);
>> 
>> index = (addr >> 4) & 0xff; switch(index) { @@ -769,7 +771,7 @@
>> static void apic_mem_writel(void *opaque, hwaddr addr, uint32_t
>> val) if (!d) { return; } -    s = DO_UPCAST(APICCommonState,
>> busdev.qdev, d); +    s = APIC_COMMON(d);
>> 
>> trace_apic_mem_writel(addr, val);
>> 
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c index
>> b03e904..0087a14 100644 --- a/hw/intc/apic_common.c +++
>> b/hw/intc/apic_common.c @@ -82,7 +82,7 @@ uint8_t
>> cpu_get_apic_tpr(DeviceState *d)
>> 
>> void apic_enable_tpr_access_reporting(DeviceState *d, bool
>> enable) { -    APICCommonState *s = DO_UPCAST(APICCommonState,
>> busdev.qdev, d); +    APICCommonState *s = APIC_COMMON(d); 
>> APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
>> 
>> apic_report_tpr_access = enable; @@ -93,7 +93,7 @@ void
>> apic_enable_tpr_access_reporting(DeviceState *d, bool enable)
>> 
>> void apic_enable_vapic(DeviceState *d, hwaddr paddr) { -
>> APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d); 
>> +    APICCommonState *s = APIC_COMMON(d); APICCommonClass *info =
>> APIC_COMMON_GET_CLASS(s);
>> 
>> s->vapic_paddr = paddr; @@ -103,7 +103,7 @@ void
>> apic_enable_vapic(DeviceState *d, hwaddr paddr) void
>> apic_handle_tpr_access_report(DeviceState *d, target_ulong ip, 
>> TPRAccess access) { -    APICCommonState *s =
>> DO_UPCAST(APICCommonState, busdev.qdev, d); +    APICCommonState
>> *s = APIC_COMMON(d);
>> 
>> vapic_report_tpr_access(s->vapic, CPU(s->cpu), ip, access); } @@
>> -172,7 +172,7 @@ bool apic_next_timer(APICCommonState *s, int64_t
>> current_time)
>> 
>> void apic_init_reset(DeviceState *d) { -    APICCommonState *s =
>> DO_UPCAST(APICCommonState, busdev.qdev, d); +    APICCommonState
>> *s = APIC_COMMON(d); int i;
>> 
>> if (!s) { @@ -215,7 +215,7 @@ void apic_designate_bsp(DeviceState
>> *d)
>> 
>> static void apic_reset_common(DeviceState *d) { -
>> APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d); 
>> +    APICCommonState *s = APIC_COMMON(d); APICCommonClass *info =
>> APIC_COMMON_GET_CLASS(s); bool bsp;
>> 
>> diff --git a/include/hw/cpu/icc_bus.h b/include/hw/cpu/icc_bus.h 
>> index b550070..f2c8a50 100644 --- a/include/hw/cpu/icc_bus.h +++
>> b/include/hw/cpu/icc_bus.h @@ -51,7 +51,7 @@ typedef struct
>> ICCBus { */ typedef struct ICCDevice { /*< private >*/ -
>> DeviceState qdev; +    DeviceState parent_obj; /*< public >*/ }
>> ICCDevice;
>> 
>> 
> 
> 


- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIcBAEBAgAGBQJRfSnoAAoJEPou0S0+fgE/NcgP/iJ99zuWVy+OG13Zr+gpGUu7
gL+GjPjKAo1QYTGBEtpB1g8veyXaVbDWCHzkGsitQRtIRqCJnwQcORcWes395J9N
ffy/fWUSwdzOdv4DP+hPoy8RaFdxKWr4JqUsoSLERM7vefQ86xieWvE7RW6Xcy+r
SDE4Qm67WWyGN5c875deF1/9LlT08vv0KCXzaqMLdm5y17c5td7h82JNNlYfYF9S
QeyPYOtWvEFR4uUaLqgoMbWpMjvobjt5G/tbSss1wfds9fiBM4nU+Blk51eF2N0v
VeNCO5NyNnQyiNR7znwYYnljNvg7Web9ITenCQGPj0eUDdAyb1kBwqCDZo5giqp4
ifvTVi4LAq0vCmBArKDeHpi0xZXPXDLm2QREdO80Qwnt9y2fieV3mMMTXl1X6vSp
R0dmGSbomgEoigQMXSHOdiz4M6CHphpUicG8/05op9Sjf5DVkfaDt7isvvzKifNL
K+vFogG3DLRyL6iw10H2B9A7e4mY/b6VCw+XvzhZa5iiZ7qa/ZVEt84uJQJNXxnv
Wd0rR5nlCdUJBq8IbDyJm80P+bnzgKU+Wfa4wOzxeDEWLJj4bRPdZd+I9mbFEgkf
rtseRM+vjooQhyTE6l/uphX94kzoOAVtwJefo9GXhhduZ26fwVepVsqH4q1Bie+G
xu50SmEmd2N4aw9VaRF9
=qCIU
-----END PGP SIGNATURE-----