| Message ID | 1378956318-23395-3-git-send-email-pingfank@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote: > On PC, 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. > (Will enable them after introducing pc 1.6 compat) > > Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > --- > hw/timer/hpet.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c > index 8429eb3..46903b9 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,12 @@ > > #define HPET_MSI_SUPPORT 0 > > +/* For bug compat, using only IRQ2. Soon it will be fixed as > + * 0xff0104ULL, i.e using IRQ16~23, IRQ8 and IRQ2 So users are expected to stick a bitmask of legal pins here? I think that's a bit too much rope to give to users. Don't you think? > after > + * introducing pc-1.6 compat. > + */ > +#define HPET_TN_INT_CAP_DEFAULT 0x4ULL > + > #define TYPE_HPET "hpet" > #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET) > > @@ -73,6 +80,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 +671,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 +761,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), > DEFINE_PROP_END_OF_LIST(), > }; > > -- > 1.8.1.4 >
On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote: >> On PC, 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. >> (Will enable them after introducing pc 1.6 compat) >> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >> --- >> hw/timer/hpet.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c >> index 8429eb3..46903b9 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,12 @@ >> >> #define HPET_MSI_SUPPORT 0 >> >> +/* For bug compat, using only IRQ2. Soon it will be fixed as >> + * 0xff0104ULL, i.e using IRQ16~23, IRQ8 and IRQ2 > > So users are expected to stick a bitmask of legal > pins here? > I think that's a bit too much rope to give to users. > Don't you think? > Sorry, not understand your meaning exactly. But the scene will be: guest kernel polls the ability bitmask, and pick up one pin which is not occupied or can be shared with the level-trigger and low-active. So is it rope? Thanks and regards, Pingfan >> after >> + * introducing pc-1.6 compat. >> + */ >> +#define HPET_TN_INT_CAP_DEFAULT 0x4ULL >> + >> #define TYPE_HPET "hpet" >> #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET) >> >> @@ -73,6 +80,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 +671,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 +761,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), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> -- >> 1.8.1.4 >>
On Sun, Sep 29, 2013 at 11:49:41AM +0800, liu ping fan wrote: > On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote: > >> On PC, 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. > >> (Will enable them after introducing pc 1.6 compat) > >> > >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > >> --- > >> hw/timer/hpet.c | 13 +++++++++++-- > >> 1 file changed, 11 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c > >> index 8429eb3..46903b9 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,12 @@ > >> > >> #define HPET_MSI_SUPPORT 0 > >> > >> +/* For bug compat, using only IRQ2. Soon it will be fixed as > >> + * 0xff0104ULL, i.e using IRQ16~23, IRQ8 and IRQ2 > > > > So users are expected to stick a bitmask of legal > > pins here? > > I think that's a bit too much rope to give to users. > > Don't you think? > > > Sorry, not understand your meaning exactly. But the scene will be: > guest kernel polls the ability bitmask, and pick up one pin which is > not occupied or can be shared with the level-trigger and low-active. > So is it rope? I merely say that it's better to make this a bool or bit property. UINT32 is too much flexibility imho. > Thanks and regards, > Pingfan > >> after > >> + * introducing pc-1.6 compat. > >> + */ > >> +#define HPET_TN_INT_CAP_DEFAULT 0x4ULL > >> + > >> #define TYPE_HPET "hpet" > >> #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET) > >> > >> @@ -73,6 +80,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 +671,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 +761,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), > >> DEFINE_PROP_END_OF_LIST(), > >> }; > >> > >> -- > >> 1.8.1.4 > >>
On Sun, Sep 29, 2013 at 12:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Sun, Sep 29, 2013 at 11:49:41AM +0800, liu ping fan wrote: >> On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote: >> >> On PC, 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. >> >> (Will enable them after introducing pc 1.6 compat) >> >> >> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >> >> --- >> >> hw/timer/hpet.c | 13 +++++++++++-- >> >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c >> >> index 8429eb3..46903b9 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,12 @@ >> >> >> >> #define HPET_MSI_SUPPORT 0 >> >> >> >> +/* For bug compat, using only IRQ2. Soon it will be fixed as >> >> + * 0xff0104ULL, i.e using IRQ16~23, IRQ8 and IRQ2 >> > >> > So users are expected to stick a bitmask of legal >> > pins here? >> > I think that's a bit too much rope to give to users. >> > Don't you think? >> > >> Sorry, not understand your meaning exactly. But the scene will be: >> guest kernel polls the ability bitmask, and pick up one pin which is >> not occupied or can be shared with the level-trigger and low-active. >> So is it rope? > > I merely say that it's better to make this a bool or bit property. > UINT32 is too much flexibility imho. > The interrupt capability is export to guest by register Tn_INT_ROUTE_CAP[63:32]. So it is useless to make them as a bit property. Do you think so? Regards Pingfan config register in hpet con >> Thanks and regards, >> Pingfan >> >> after >> >> + * introducing pc-1.6 compat. >> >> + */ >> >> +#define HPET_TN_INT_CAP_DEFAULT 0x4ULL >> >> + >> >> #define TYPE_HPET "hpet" >> >> #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET) >> >> >> >> @@ -73,6 +80,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 +671,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 +761,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), >> >> DEFINE_PROP_END_OF_LIST(), >> >> }; >> >> >> >> -- >> >> 1.8.1.4 >> >>
On Mon, Sep 30, 2013 at 04:02:29PM +0800, liu ping fan wrote: > On Sun, Sep 29, 2013 at 12:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Sun, Sep 29, 2013 at 11:49:41AM +0800, liu ping fan wrote: > >> On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote: > >> >> On PC, 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. > >> >> (Will enable them after introducing pc 1.6 compat) > >> >> > >> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > >> >> --- > >> >> hw/timer/hpet.c | 13 +++++++++++-- > >> >> 1 file changed, 11 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c > >> >> index 8429eb3..46903b9 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,12 @@ > >> >> > >> >> #define HPET_MSI_SUPPORT 0 > >> >> > >> >> +/* For bug compat, using only IRQ2. Soon it will be fixed as > >> >> + * 0xff0104ULL, i.e using IRQ16~23, IRQ8 and IRQ2 > >> > > >> > So users are expected to stick a bitmask of legal > >> > pins here? > >> > I think that's a bit too much rope to give to users. > >> > Don't you think? > >> > > >> Sorry, not understand your meaning exactly. But the scene will be: > >> guest kernel polls the ability bitmask, and pick up one pin which is > >> not occupied or can be shared with the level-trigger and low-active. > >> So is it rope? > > > > I merely say that it's better to make this a bool or bit property. > > UINT32 is too much flexibility imho. > > > The interrupt capability is export to guest by register > Tn_INT_ROUTE_CAP[63:32]. So it is useless to make them as a bit > property. Do you think so? > > Regards > Pingfan I think we merely need to support two modes: - qemu 1.6 and older compatible - compatible to actual hardware Why would we let users configure an arbitrary configuration which isn't compatible to either old qemu or real hardware? > config register in hpet con > >> Thanks and regards, > >> Pingfan > >> >> after > >> >> + * introducing pc-1.6 compat. > >> >> + */ > >> >> +#define HPET_TN_INT_CAP_DEFAULT 0x4ULL > >> >> + > >> >> #define TYPE_HPET "hpet" > >> >> #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET) > >> >> > >> >> @@ -73,6 +80,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 +671,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 +761,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), > >> >> DEFINE_PROP_END_OF_LIST(), > >> >> }; > >> >> > >> >> -- > >> >> 1.8.1.4 > >> >>
Il 30/09/2013 11:06, Michael S. Tsirkin ha scritto: > On Mon, Sep 30, 2013 at 04:02:29PM +0800, liu ping fan wrote: >> On Sun, Sep 29, 2013 at 12:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> On Sun, Sep 29, 2013 at 11:49:41AM +0800, liu ping fan wrote: >>>> On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >>>>> On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote: >>>>>> On PC, 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. >>>>>> (Will enable them after introducing pc 1.6 compat) >>>>>> >>>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >>>>>> --- >>>>>> hw/timer/hpet.c | 13 +++++++++++-- >>>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c >>>>>> index 8429eb3..46903b9 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,12 @@ >>>>>> >>>>>> #define HPET_MSI_SUPPORT 0 >>>>>> >>>>>> +/* For bug compat, using only IRQ2. Soon it will be fixed as >>>>>> + * 0xff0104ULL, i.e using IRQ16~23, IRQ8 and IRQ2 >>>>> >>>>> So users are expected to stick a bitmask of legal >>>>> pins here? >>>>> I think that's a bit too much rope to give to users. >>>>> Don't you think? >>>>> >>>> Sorry, not understand your meaning exactly. But the scene will be: >>>> guest kernel polls the ability bitmask, and pick up one pin which is >>>> not occupied or can be shared with the level-trigger and low-active. >>>> So is it rope? >>> >>> I merely say that it's better to make this a bool or bit property. >>> UINT32 is too much flexibility imho. >>> >> The interrupt capability is export to guest by register >> Tn_INT_ROUTE_CAP[63:32]. So it is useless to make them as a bit >> property. Do you think so? >> >> Regards >> Pingfan > > I think we merely need to support two modes: > - qemu 1.6 and older compatible > - compatible to actual hardware > > Why would we let users configure an arbitrary > configuration which isn't compatible to either > old qemu or real hardware? The actual setting depends on the chipset. For example, the "real" PIIX4 is the same as the QEMU PIIX4, only Q35 uses the new value. If in the future we had a chipset with more than 24 GSIs, you would have a third possibility. Paolo
On Mon, Sep 30, 2013 at 11:06:48AM +0200, Paolo Bonzini wrote: > Il 30/09/2013 11:06, Michael S. Tsirkin ha scritto: > > On Mon, Sep 30, 2013 at 04:02:29PM +0800, liu ping fan wrote: > >> On Sun, Sep 29, 2013 at 12:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >>> On Sun, Sep 29, 2013 at 11:49:41AM +0800, liu ping fan wrote: > >>>> On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > >>>>> On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote: > >>>>>> On PC, 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. > >>>>>> (Will enable them after introducing pc 1.6 compat) > >>>>>> > >>>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > >>>>>> --- > >>>>>> hw/timer/hpet.c | 13 +++++++++++-- > >>>>>> 1 file changed, 11 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c > >>>>>> index 8429eb3..46903b9 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,12 @@ > >>>>>> > >>>>>> #define HPET_MSI_SUPPORT 0 > >>>>>> > >>>>>> +/* For bug compat, using only IRQ2. Soon it will be fixed as > >>>>>> + * 0xff0104ULL, i.e using IRQ16~23, IRQ8 and IRQ2 > >>>>> > >>>>> So users are expected to stick a bitmask of legal > >>>>> pins here? > >>>>> I think that's a bit too much rope to give to users. > >>>>> Don't you think? > >>>>> > >>>> Sorry, not understand your meaning exactly. But the scene will be: > >>>> guest kernel polls the ability bitmask, and pick up one pin which is > >>>> not occupied or can be shared with the level-trigger and low-active. > >>>> So is it rope? > >>> > >>> I merely say that it's better to make this a bool or bit property. > >>> UINT32 is too much flexibility imho. > >>> > >> The interrupt capability is export to guest by register > >> Tn_INT_ROUTE_CAP[63:32]. So it is useless to make them as a bit > >> property. Do you think so? > >> > >> Regards > >> Pingfan > > > > I think we merely need to support two modes: > > - qemu 1.6 and older compatible > > - compatible to actual hardware > > > > Why would we let users configure an arbitrary > > configuration which isn't compatible to either > > old qemu or real hardware? > > The actual setting depends on the chipset. For example, the "real" > PIIX4 is the same as the QEMU PIIX4, only Q35 uses the new value. > > If in the future we had a chipset with more than 24 GSIs, you would have > a third possibility. > > Paolo I was really only talking about q35 here. I thought it's ugly that users can control intcap directly. Can object_set_property be used after qdev_try_create? PIIX has another issue: the default value in hpet is really Q35 specific, that's also kind of ugly, isn't it?
Il 30/09/2013 11:30, Michael S. Tsirkin ha scritto: > I was really only talking about q35 here. > I thought it's ugly that users can control intcap > directly. Can object_set_property be used after > qdev_try_create? Yes, after that and before qdev_init. This is how Ping Fan is doing PIIX right now. > PIIX has another issue: > the default value in hpet is really Q35 specific, > that's also kind of ugly, isn't it? Yeah, perhaps it is. So are you suggesting the default to be 4 (GSI2 only)? Paolo
On Mon, Sep 30, 2013 at 05:48:03PM +0200, Paolo Bonzini wrote: > Il 30/09/2013 11:30, Michael S. Tsirkin ha scritto: > > I was really only talking about q35 here. > > I thought it's ugly that users can control intcap > > directly. Can object_set_property be used after > > qdev_try_create? > > Yes, after that and before qdev_init. This is how Ping Fan is doing > PIIX right now. > > > PIIX has another issue: > > the default value in hpet is really Q35 specific, > > that's also kind of ugly, isn't it? > > Yeah, perhaps it is. So are you suggesting the default to be 4 (GSI2 only)? > > Paolo I suggest it fails unless caller set the property.
On Mon, Sep 30, 2013 at 11:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Sep 30, 2013 at 05:48:03PM +0200, Paolo Bonzini wrote: >> Il 30/09/2013 11:30, Michael S. Tsirkin ha scritto: >> > I was really only talking about q35 here. >> > I thought it's ugly that users can control intcap >> > directly. Can object_set_property be used after >> > qdev_try_create? >> >> Yes, after that and before qdev_init. This is how Ping Fan is doing >> PIIX right now. >> >> > PIIX has another issue: >> > the default value in hpet is really Q35 specific, >> > that's also kind of ugly, isn't it? >> >> Yeah, perhaps it is. So are you suggesting the default to be 4 (GSI2 only)? >> >> Paolo > > I suggest it fails unless caller set the property. > Sorry, out of office for a long time, and did not keep up with this thread in time. When letting the caller set the intcap, we should consider the compatibility of q35. For pc-q35-1.7 or later, the caller should set the property, otherwise not. But how can the caller tell that it runs on q35-1.7? The essential problem is that "set the property" will always overwrite the property which is set up by compatible mechanism. So it is hard to implement without breaking the current mechanism. Do you think so? Thanks and regards, Ping fan
On Wed, Oct 09, 2013 at 11:27:24AM +0800, liu ping fan wrote: > On Mon, Sep 30, 2013 at 11:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Sep 30, 2013 at 05:48:03PM +0200, Paolo Bonzini wrote: > >> Il 30/09/2013 11:30, Michael S. Tsirkin ha scritto: > >> > I was really only talking about q35 here. > >> > I thought it's ugly that users can control intcap > >> > directly. Can object_set_property be used after > >> > qdev_try_create? > >> > >> Yes, after that and before qdev_init. This is how Ping Fan is doing > >> PIIX right now. > >> > >> > PIIX has another issue: > >> > the default value in hpet is really Q35 specific, > >> > that's also kind of ugly, isn't it? > >> > >> Yeah, perhaps it is. So are you suggesting the default to be 4 (GSI2 only)? > >> > >> Paolo > > > > I suggest it fails unless caller set the property. > > > Sorry, out of office for a long time, and did not keep up with this > thread in time. > When letting the caller set the intcap, we should consider the > compatibility of q35. For pc-q35-1.7 or later, the caller should set > the property, otherwise not. Set it always - just set it to a compatible value for 1.6. > But how can the caller tell that it runs > on q35-1.7? > The essential problem is that "set the property" will always overwrite > the property which is set up by compatible mechanism. So it is hard to > implement without breaking the current mechanism. Do you think so? > > Thanks and regards, > Ping fan Not that hard. Fail init if it wasn't set.
Il 09/10/2013 09:24, Michael S. Tsirkin ha scritto: >> > Sorry, out of office for a long time, and did not keep up with this >> > thread in time. >> > When letting the caller set the intcap, we should consider the >> > compatibility of q35. For pc-q35-1.7 or later, the caller should set >> > the property, otherwise not. > Set it always - just set it to a compatible value for 1.6. > So you're suggesting skipping the mechanism we have for compatibility properties, and instead using a global variable or something like that? Paolo
On Wed, Oct 09, 2013 at 09:41:29AM +0200, Paolo Bonzini wrote: > Il 09/10/2013 09:24, Michael S. Tsirkin ha scritto: > >> > Sorry, out of office for a long time, and did not keep up with this > >> > thread in time. > >> > When letting the caller set the intcap, we should consider the > >> > compatibility of q35. For pc-q35-1.7 or later, the caller should set > >> > the property, otherwise not. > > Set it always - just set it to a compatible value for 1.6. > > > > So you're suggesting skipping the mechanism we have for compatibility > properties, and instead using a global variable or something like that? > > Paolo That's one option. Or we can use global properties - just set for 1.7 too. All this is not ideal: properties really need to integrate better with QOM, and using global properties for compat always was a hack.
Il 09/10/2013 10:01, Michael S. Tsirkin ha scritto: > > So you're suggesting skipping the mechanism we have for compatibility > > properties, and instead using a global variable or something like that? > > That's one option. > Or we can use global properties - just set for 1.7 too. That's better. Paolo
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index 8429eb3..46903b9 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,12 @@ #define HPET_MSI_SUPPORT 0 +/* For bug compat, using only IRQ2. Soon it will be fixed as + * 0xff0104ULL, i.e using IRQ16~23, IRQ8 and IRQ2 after + * introducing pc-1.6 compat. + */ +#define HPET_TN_INT_CAP_DEFAULT 0x4ULL + #define TYPE_HPET "hpet" #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET) @@ -73,6 +80,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 +671,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 +761,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), DEFINE_PROP_END_OF_LIST(), };
On PC, 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. (Will enable them after introducing pc 1.6 compat) Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> --- hw/timer/hpet.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)