From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49202) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQZOu-0000tv-L8 for qemu-devel@nongnu.org; Mon, 30 Sep 2013 05:04:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VQZOo-0000Bm-Kj for qemu-devel@nongnu.org; Mon, 30 Sep 2013 05:04:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2877) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQZOo-0000Be-Cb for qemu-devel@nongnu.org; Mon, 30 Sep 2013 05:04:14 -0400 Date: Mon, 30 Sep 2013 12:06:31 +0300 From: "Michael S. Tsirkin" Message-ID: <20130930090631.GC20291@redhat.com> References: <1378956318-23395-1-git-send-email-pingfank@linux.vnet.ibm.com> <1378956318-23395-3-git-send-email-pingfank@linux.vnet.ibm.com> <20130928195635.GC1051@redhat.com> <20130929041547.GA28242@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v5 2/5] hpet: entitle more irq pins for hpet List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liu ping fan Cc: Peter Maydell , Anthony Liguori , Jan Kiszka , qemu-devel@nongnu.org, Stefan Hajnoczi , Paolo Bonzini , Andreas =?iso-8859-1?Q?F=E4rber?= 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 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 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 > >> >> --- > >> >> 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 > >> >>