Re: [Qemu-devel] [PATCH v3 3/3] hpet: entitle more irq pins for hpet

[Andreas Färber]

Am 30.08.2013 09:53, schrieb Liu Ping Fan:
> On q35 machine, IRQ2/8 can be reserved for hpet timer 0/1. And pin 16~23
> of ioapic can be dynamically assigned to hpet as guest chooses.
> 
> Signed-off-by: Liu Ping Fan <address@hidden>
> ---
>  hw/i386/pc.c    |  8 +++++++-
>  hw/timer/hpet.c | 12 ++++++++++--
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 09c10ac..bb23d99 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1217,6 +1217,12 @@ static const MemoryRegionOps ioportF0_io_ops = {
>      },
>  };
>  
> +static void hpet_intcap_set(DeviceState *dev)
> +{
> +    /* For guest bug compatibility, only IRQ2 is reserved for hpet on q35 */
> +    qdev_prop_set_uint32(dev, "intcap", 0x4);
> +}
> +
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>                            ISADevice **rtc_state,
>                            ISADevice **floppy,
> @@ -1247,7 +1253,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq 
> *gsi,
>       * when the HPET wants to take over. Thus we have to disable the latter.
>       */
>      if (!no_hpet && (!kvm_irqchip_in_kernel() || kvm_has_pit_state2())) {
> -        hpet = sysbus_try_create_simple("hpet", NULL, HPET_BASE, NULL);
> +        hpet = sysbus_try_create_simple("hpet", hpet_intcap_set, HPET_BASE, 
> NULL);
>  
>          if (hpet) {
>              for (i = 0; i < GSI_NUM_PINS; i++) {

As PMM has said, this is unnecessary. Just use qdev_create(),
qdev_set_prop_*(), qdev_init_nofail(), sysbus_mmio_map(). (This code
seems not much QOM'ified yet.)

> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 1139448..2e19ff5 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -25,6 +25,7 @@
>   */
>  
>  #include "hw/hw.h"
> +#include "hw/boards.h"
>  #include "hw/i386/pc.h"
>  #include "ui/console.h"
>  #include "qemu/timer.h"
> @@ -42,6 +43,11 @@
>  
>  #define HPET_MSI_SUPPORT        0
>  
> +/* only IRQ2 allowed for pc-1.6 and former */
> +#define HPET_TN_INT_CAP_PC (0x4ULL << 32)
> +/* Hpet can use non-legacy IRQ16~23, and an IRQ2 ,IRQ8 */
> +#define HPET_TN_INT_CAP_DEFAULT 0xff0104ULL
> +
>  #define TYPE_HPET "hpet"
>  #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
>  
> @@ -73,6 +79,7 @@ typedef struct HPETState {
>      uint8_t rtc_irq_level;
>      qemu_irq pit_enabled;
>      uint8_t num_timers;
> +    uint32_t intcap;
>      HPETTimer timer[HPET_MAX_TIMERS];
>  
>      /* Memory-mapped, software visible registers */
> @@ -663,8 +670,8 @@ static void hpet_reset(DeviceState *d)
>          if (s->flags & (1 << HPET_MSI_SUPPORT)) {
>              timer->config |= HPET_TN_FSB_CAP;
>          }
> -        /* advertise availability of ioapic inti2 */
> -        timer->config |=  0x00000004ULL << 32;
> +        /* advertise availability of ioapic int */
> +        timer->config |=  (uint64_t)s->intcap << 32;
>          timer->period = 0ULL;
>          timer->wrap_flag = 0;
>      }
> @@ -753,6 +760,7 @@ static void hpet_realize(DeviceState *dev, Error **errp)
>  static Property hpet_device_properties[] = {
>      DEFINE_PROP_UINT8("timers", HPETState, num_timers, HPET_MIN_TIMERS),
>      DEFINE_PROP_BIT("msi", HPETState, flags, HPET_MSI_SUPPORT, false),
> +    DEFINE_PROP_UINT32("intcap", HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),

What is "intcap"? It sounds like capabilities? In that case
DEFINE_PROP_BIT() might be a more appropriate way to model individually
tweakable properties? Either way, the property name could probably use
some love for clarity - there is no explanation for users.

Regards,
Andreas

>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg