Re: [Qemu-devel] [PATCH v7 4/4] PC: differentiate hpet's interrupt capability on piix and q35

[liu ping fan]

On Thu, Oct 17, 2013 at 1:44 PM, Michael S. Tsirkin <address@hidden> wrote:
> On Thu, Oct 17, 2013 at 11:16:05AM +0800, Liu Ping Fan wrote:
>> For pc-piix-*, hpet's intcap is always hard coded as IRQ2.
>> For q35, if it is pc-q35-1.7 and earlier, we use IRQ2 for compat
>> reason, otherwise IRQ2, IRQ8, and IRQ16~23 are allowed.
>>
>> Signed-off-by: Liu Ping Fan <address@hidden>
>> ---
>>  hw/i386/pc.c         | 20 +++++++++++++++-----
>>  hw/i386/pc_piix.c    |  7 ++++++-
>>  hw/i386/pc_q35.c     |  6 +++++-
>>  include/hw/i386/pc.h | 11 ++++++++++-
>>  4 files changed, 36 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 72cc850..3e98ff0 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1219,7 +1219,8 @@ static const MemoryRegionOps ioportF0_io_ops = {
>>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>>                            ISADevice **rtc_state,
>>                            ISADevice **floppy,
>> -                          bool no_vmport)
>> +                          bool no_vmport,
>> +                          bool hpet_irqs)
>>  {
>>      int i;
>>      DriveInfo *fd[MAX_FD];
>> @@ -1249,10 +1250,19 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq 
>> *gsi,
>>          /* In order to set property, here not using 
>> sysbus_try_create_simple */
>>          hpet = qdev_try_create(NULL, "hpet");
>>          if (hpet) {
>> -            /* tmp fix. For compat, hard code to IRQ2 until we have correct
>> -             * compat property and differentiate pc-iix with pc-q35
>> -             */
>> -            qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
>> +            /* For pc-piix-*, hpet's intcap is always IRQ2. */
>> +            if (!hpet_irqs) {
>
> So for piix the property is ignored even if set.
>
Yes.
>> +                qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
>> +            } else {
>> +                /* For pc-q35-1.7 and earlier, use IRQ2 for compat.
>
> Now you lost me. It's still 4 for 1.7?
> You only target 1.8 then?
>
Yes, since the compat for guest. And start this fix with 1.8

>> +                 * Otherwise, use IRQ16~23, IRQ8 and IRQ2.
>> +                 */
>> +                uint8_t compat = object_property_get_int(OBJECT(hpet),
>> +                        HPET_INTCAP, NULL);
>> +                if (!compat) {
>> +                    qdev_prop_set_uint32(hpet, HPET_INTCAP, 0xff0104);
>> +                }
>> +            }
>>              qdev_init_nofail(hpet);
>>              sysbus_mmio_map(SYS_BUS_DEVICE(hpet), 0, HPET_BASE);
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index c6042c7..a45ce11 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -180,7 +180,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>      pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
>>
>>      /* init basic PC hardware */
>> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
>> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled(),
>> +        false);
>>
>>      pc_nic_init(isa_bus, pci_bus);
>>
>> @@ -346,6 +347,10 @@ static QEMUMachine pc_i440fx_machine_v1_7 = {
>>      .alias = "pc",
>>      .init = pc_init_pci,
>>      .is_default = 1,
>> +    .compat_props = (GlobalProperty[]) {
>> +        PC_COMPAT_1_7,
>
> So you add this property for PIIX but it's then ignored?
> That's kind of ugly.

Yes a little ugly, but PIIX and ich9 diverge for hpet's hardware.

> Not sure how to fix this. Paolo?
>
>> +        { /* end of list */ }
>> +    },
>>  };
>>
>>  #define PC_I440FX_1_6_MACHINE_OPTIONS PC_I440FX_MACHINE_OPTIONS
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index ca84e1c..124ecc1 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -181,7 +181,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>      pc_register_ferr_irq(gsi[13]);
>>
>>      /* init basic PC hardware */
>> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false);
>> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false, true);
>>
>>      /* connect pm stuff to lpc */
>>      ich9_lpc_pm_init(lpc);
>> @@ -270,6 +270,10 @@ static QEMUMachine pc_q35_machine_v1_7 = {
>>      .name = "pc-q35-1.7",
>>      .alias = "q35",
>>      .init = pc_q35_init,
>> +    .compat_props = (GlobalProperty[]) {
>> +        PC_COMPAT_1_7,
>> +        { /* end of list */ }
>> +    },
>>  };
>>
>>  #define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 1361a27..bfeccf2 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -136,7 +136,8 @@ DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus 
>> *pci_bus);
>>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>>                            ISADevice **rtc_state,
>>                            ISADevice **floppy,
>> -                          bool no_vmport);
>> +                          bool no_vmport,
>> +                          bool hpet_irqs);
>>  void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd);
>>  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>                    const char *boot_device,
>> @@ -227,7 +228,15 @@ void pvpanic_init(ISABus *bus);
>>
>>  int e820_add_entry(uint64_t, uint64_t, uint32_t);
>>
>> +#define PC_COMPAT_1_7 \
>> +        {\
>> +            .driver   = "hpet",\
>> +            .property = HPET_INTCAP,\
>> +            .value    = stringify(4),\
>> +        }
>> +
>>  #define PC_COMPAT_1_6 \
>> +        PC_COMPAT_1_7, \
>>          {\
>>              .driver   = "e1000",\
>>              .property = "mitigation",\
>> --
>> 1.8.1.4