| Message ID | AANLkTinReKqGNOHY9OWEn17JYA5ezm5dbtvZB2Ca2rs+@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 2011-02-13 22:10, Blue Swirl wrote: > Convert to qdev. Don't expose PITState. > ... > diff --git a/hw/pc.h b/hw/pc.h > index 60f8c42..feb8a7a 100644 > --- a/hw/pc.h > +++ b/hw/pc.h > @@ -82,14 +82,23 @@ void isa_irq_handler(void *opaque, int n, int level); > > #define PIT_FREQ 1193182 > > -typedef struct PITState PITState; > +static inline ISADevice *pit_init(int base, int irq) > +{ > + ISADevice *dev; > + > + dev = isa_create("isa-pit"); > + qdev_prop_set_uint32(&dev->qdev, "iobase", base); > + qdev_prop_set_uint32(&dev->qdev, "irq", irq); > + qdev_init_nofail(&dev->qdev); > + > + return dev; > +} This should be moved to i8254.c. We cannot compile the PIT out, and the above contains no board information. So I see no reason for this inlining. Jan
On Sun, Mar 6, 2011 at 5:10 PM, Jan Kiszka <jan.kiszka@web.de> wrote: > On 2011-02-13 22:10, Blue Swirl wrote: >> Convert to qdev. Don't expose PITState. >> > > ... > >> diff --git a/hw/pc.h b/hw/pc.h >> index 60f8c42..feb8a7a 100644 >> --- a/hw/pc.h >> +++ b/hw/pc.h >> @@ -82,14 +82,23 @@ void isa_irq_handler(void *opaque, int n, int level); >> >> #define PIT_FREQ 1193182 >> >> -typedef struct PITState PITState; >> +static inline ISADevice *pit_init(int base, int irq) >> +{ >> + ISADevice *dev; >> + >> + dev = isa_create("isa-pit"); >> + qdev_prop_set_uint32(&dev->qdev, "iobase", base); >> + qdev_prop_set_uint32(&dev->qdev, "irq", irq); >> + qdev_init_nofail(&dev->qdev); >> + >> + return dev; >> +} > > This should be moved to i8254.c. We cannot compile the PIT out, and the > above contains no board information. So I see no reason for this inlining. I see no reason for moving this back. In fact, if this was only used by PC, it could be moved to pc.c.
On 2011-03-06 16:35, Blue Swirl wrote: > On Sun, Mar 6, 2011 at 5:10 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >> On 2011-02-13 22:10, Blue Swirl wrote: >>> Convert to qdev. Don't expose PITState. >>> >> >> ... >> >>> diff --git a/hw/pc.h b/hw/pc.h >>> index 60f8c42..feb8a7a 100644 >>> --- a/hw/pc.h >>> +++ b/hw/pc.h >>> @@ -82,14 +82,23 @@ void isa_irq_handler(void *opaque, int n, int level); >>> >>> #define PIT_FREQ 1193182 >>> >>> -typedef struct PITState PITState; >>> +static inline ISADevice *pit_init(int base, int irq) >>> +{ >>> + ISADevice *dev; >>> + >>> + dev = isa_create("isa-pit"); >>> + qdev_prop_set_uint32(&dev->qdev, "iobase", base); >>> + qdev_prop_set_uint32(&dev->qdev, "irq", irq); >>> + qdev_init_nofail(&dev->qdev); >>> + >>> + return dev; >>> +} >> >> This should be moved to i8254.c. We cannot compile the PIT out, and the >> above contains no board information. So I see no reason for this inlining. > > I see no reason for moving this back. In fact, if this was only used > by PC, it could be moved to pc.c. To my understanding, it's a factory helper for the PIT, avoiding boilerplate code at the creator site. And if we get >1 users, this cleanup would definitely pay off. Jan
On Sun, Mar 6, 2011 at 5:39 PM, Jan Kiszka <jan.kiszka@web.de> wrote: > On 2011-03-06 16:35, Blue Swirl wrote: >> On Sun, Mar 6, 2011 at 5:10 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >>> On 2011-02-13 22:10, Blue Swirl wrote: >>>> Convert to qdev. Don't expose PITState. >>>> >>> >>> ... >>> >>>> diff --git a/hw/pc.h b/hw/pc.h >>>> index 60f8c42..feb8a7a 100644 >>>> --- a/hw/pc.h >>>> +++ b/hw/pc.h >>>> @@ -82,14 +82,23 @@ void isa_irq_handler(void *opaque, int n, int level); >>>> >>>> #define PIT_FREQ 1193182 >>>> >>>> -typedef struct PITState PITState; >>>> +static inline ISADevice *pit_init(int base, int irq) >>>> +{ >>>> + ISADevice *dev; >>>> + >>>> + dev = isa_create("isa-pit"); >>>> + qdev_prop_set_uint32(&dev->qdev, "iobase", base); >>>> + qdev_prop_set_uint32(&dev->qdev, "irq", irq); >>>> + qdev_init_nofail(&dev->qdev); >>>> + >>>> + return dev; >>>> +} >>> >>> This should be moved to i8254.c. We cannot compile the PIT out, and the >>> above contains no board information. So I see no reason for this inlining. >> >> I see no reason for moving this back. In fact, if this was only used >> by PC, it could be moved to pc.c. > > To my understanding, it's a factory helper for the PIT, avoiding > boilerplate code at the creator site. And if we get >1 users, this > cleanup would definitely pay off. It should be possible to leave PIT out, with small changes to pcspk (which obviously has a hard dependency to PIT) and HPET. Then this makes even more sense.
On 2011-03-06 17:06, Blue Swirl wrote: > On Sun, Mar 6, 2011 at 5:39 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >> On 2011-03-06 16:35, Blue Swirl wrote: >>> On Sun, Mar 6, 2011 at 5:10 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >>>> On 2011-02-13 22:10, Blue Swirl wrote: >>>>> Convert to qdev. Don't expose PITState. >>>>> >>>> >>>> ... >>>> >>>>> diff --git a/hw/pc.h b/hw/pc.h >>>>> index 60f8c42..feb8a7a 100644 >>>>> --- a/hw/pc.h >>>>> +++ b/hw/pc.h >>>>> @@ -82,14 +82,23 @@ void isa_irq_handler(void *opaque, int n, int level); >>>>> >>>>> #define PIT_FREQ 1193182 >>>>> >>>>> -typedef struct PITState PITState; >>>>> +static inline ISADevice *pit_init(int base, int irq) >>>>> +{ >>>>> + ISADevice *dev; >>>>> + >>>>> + dev = isa_create("isa-pit"); >>>>> + qdev_prop_set_uint32(&dev->qdev, "iobase", base); >>>>> + qdev_prop_set_uint32(&dev->qdev, "irq", irq); >>>>> + qdev_init_nofail(&dev->qdev); >>>>> + >>>>> + return dev; >>>>> +} >>>> >>>> This should be moved to i8254.c. We cannot compile the PIT out, and the >>>> above contains no board information. So I see no reason for this inlining. >>> >>> I see no reason for moving this back. In fact, if this was only used >>> by PC, it could be moved to pc.c. >> >> To my understanding, it's a factory helper for the PIT, avoiding >> boilerplate code at the creator site. And if we get >1 users, this >> cleanup would definitely pay off. > > It should be possible to leave PIT out, with small changes to pcspk > (which obviously has a hard dependency to PIT) and HPET. Then this > makes even more sense. A PC without a PIT makes _no_ sense. Jan
On 02/13/2011 03:10 PM, Blue Swirl wrote: > Convert to qdev. Don't expose PITState. > > Signed-off-by: Blue Swirl<blauwirbel@gmail.com> > --- > hw/i8254.c | 61 +++++++++++++++++++++++++++++++++++++-------------- > hw/mips_fulong2e.c | 4 +- > hw/mips_jazz.c | 4 +- > hw/mips_malta.c | 4 +- > hw/mips_r4k.c | 4 +- > hw/pc.c | 5 +-- > hw/pc.h | 25 ++++++++++++++------ > hw/pcspk.c | 4 +- > hw/ppc_prep.c | 4 +- > 9 files changed, 75 insertions(+), 40 deletions(-) > > diff --git a/hw/i8254.c b/hw/i8254.c > index 06b225c..680caab 100644 > --- a/hw/i8254.c > +++ b/hw/i8254.c > @@ -53,9 +53,12 @@ typedef struct PITChannelState { > qemu_irq irq; > } PITChannelState; > > -struct PITState { > +typedef struct PITState { > + ISADevice dev; > The PIT is not an ISA device. Modelling it as such is worse than leaving it unmodelled. > -PITState *pit_init(int base, qemu_irq irq); > -void pit_set_gate(PITState *pit, int channel, int val); > -int pit_get_gate(PITState *pit, int channel); > -int pit_get_initial_count(PITState *pit, int channel); > -int pit_get_mode(PITState *pit, int channel); > -int pit_get_out(PITState *pit, int channel, int64_t current_time); > +void pit_set_gate(ISADevice *dev, int channel, int val); > +int pit_get_gate(ISADevice *dev, int channel); > +int pit_get_initial_count(ISADevice *dev, int channel); > +int pit_get_mode(ISADevice *dev, int channel); > +int pit_get_out(ISADevice *dev, int channel, int64_t current_time); > Making these functions take an ISADevice hurts type safety. They should take a PITState. Regards, Anthony Liguori
On Sun, Mar 6, 2011 at 6:31 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 02/13/2011 03:10 PM, Blue Swirl wrote: >> >> Convert to qdev. Don't expose PITState. >> >> Signed-off-by: Blue Swirl<blauwirbel@gmail.com> >> --- >> hw/i8254.c | 61 >> +++++++++++++++++++++++++++++++++++++-------------- >> hw/mips_fulong2e.c | 4 +- >> hw/mips_jazz.c | 4 +- >> hw/mips_malta.c | 4 +- >> hw/mips_r4k.c | 4 +- >> hw/pc.c | 5 +-- >> hw/pc.h | 25 ++++++++++++++------ >> hw/pcspk.c | 4 +- >> hw/ppc_prep.c | 4 +- >> 9 files changed, 75 insertions(+), 40 deletions(-) >> >> diff --git a/hw/i8254.c b/hw/i8254.c >> index 06b225c..680caab 100644 >> --- a/hw/i8254.c >> +++ b/hw/i8254.c >> @@ -53,9 +53,12 @@ typedef struct PITChannelState { >> qemu_irq irq; >> } PITChannelState; >> >> -struct PITState { >> +typedef struct PITState { >> + ISADevice dev; >> > > The PIT is not an ISA device. Modelling it as such is worse than leaving it > unmodelled. No. These days, PIT is part of Super I/O chip, which is accessed via LPC bus. LPC is from software point of view equal to ISA. Therefore, in absence of LPC, ISA is correct. >> -PITState *pit_init(int base, qemu_irq irq); >> -void pit_set_gate(PITState *pit, int channel, int val); >> -int pit_get_gate(PITState *pit, int channel); >> -int pit_get_initial_count(PITState *pit, int channel); >> -int pit_get_mode(PITState *pit, int channel); >> -int pit_get_out(PITState *pit, int channel, int64_t current_time); >> +void pit_set_gate(ISADevice *dev, int channel, int val); >> +int pit_get_gate(ISADevice *dev, int channel); >> +int pit_get_initial_count(ISADevice *dev, int channel); >> +int pit_get_mode(ISADevice *dev, int channel); >> +int pit_get_out(ISADevice *dev, int channel, int64_t current_time); >> > > Making these functions take an ISADevice hurts type safety. They should > take a PITState. These functions should be removed anyway. For example, pcspk probably should be merged with i825, or a signal like interface could be used.
On Sun, Mar 6, 2011 at 6:47 PM, Blue Swirl <blauwirbel@gmail.com> wrote: > On Sun, Mar 6, 2011 at 6:31 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: >> On 02/13/2011 03:10 PM, Blue Swirl wrote: >>> >>> Convert to qdev. Don't expose PITState. >>> >>> Signed-off-by: Blue Swirl<blauwirbel@gmail.com> >>> --- >>> hw/i8254.c | 61 >>> +++++++++++++++++++++++++++++++++++++-------------- >>> hw/mips_fulong2e.c | 4 +- >>> hw/mips_jazz.c | 4 +- >>> hw/mips_malta.c | 4 +- >>> hw/mips_r4k.c | 4 +- >>> hw/pc.c | 5 +-- >>> hw/pc.h | 25 ++++++++++++++------ >>> hw/pcspk.c | 4 +- >>> hw/ppc_prep.c | 4 +- >>> 9 files changed, 75 insertions(+), 40 deletions(-) >>> >>> diff --git a/hw/i8254.c b/hw/i8254.c >>> index 06b225c..680caab 100644 >>> --- a/hw/i8254.c >>> +++ b/hw/i8254.c >>> @@ -53,9 +53,12 @@ typedef struct PITChannelState { >>> qemu_irq irq; >>> } PITChannelState; >>> >>> -struct PITState { >>> +typedef struct PITState { >>> + ISADevice dev; >>> >> >> The PIT is not an ISA device. Modelling it as such is worse than leaving it >> unmodelled. > > No. These days, PIT is part of Super I/O chip, which is accessed via > LPC bus. LPC is from software point of view equal to ISA. Therefore, > in absence of LPC, ISA is correct. Actually, I could not find any pure Super I/O chips (without south bridge functionality) which also contained PIT and other legacy devices, instead most south bridges (with or without a Super I/O) implement these. Then PIT could be a SysBusDevice, though SysBus does not know anything about I/O ports. I think ISADevice is not totally wrong.
On 03/06/2011 10:47 AM, Blue Swirl wrote: >> The PIT is not an ISA device. Modelling it as such is worse than leaving it >> unmodelled. >> > No. These days, PIT is part of Super I/O chip, which is accessed via > LPC bus. LPC is from software point of view equal to ISA. Therefore, > in absence of LPC, ISA is correct. > In the system we model, the PIT is part of the PIIX3. The right way to model it is as a DeviceState that's no_user=1 and created as part of the initialized of PIIX3 (for the PC at least). LPC is still an expansion bus and it's primarily used for discrete components like a TPM. For components that are all part of a Super I/O chip, there really just isn't a bus in the middle. >>> -PITState *pit_init(int base, qemu_irq irq); >>> -void pit_set_gate(PITState *pit, int channel, int val); >>> -int pit_get_gate(PITState *pit, int channel); >>> -int pit_get_initial_count(PITState *pit, int channel); >>> -int pit_get_mode(PITState *pit, int channel); >>> -int pit_get_out(PITState *pit, int channel, int64_t current_time); >>> +void pit_set_gate(ISADevice *dev, int channel, int val); >>> +int pit_get_gate(ISADevice *dev, int channel); >>> +int pit_get_initial_count(ISADevice *dev, int channel); >>> +int pit_get_mode(ISADevice *dev, int channel); >>> +int pit_get_out(ISADevice *dev, int channel, int64_t current_time); >>> >>> >> Making these functions take an ISADevice hurts type safety. They should >> take a PITState. >> > These functions should be removed anyway. For example, pcspk probably > should be merged with i825, or a signal like interface could be used. > It's tough to do that without doing a real simulation. The PC speaker uses one of the PIT channel outputs to drive its tone output. But we don't want to do something like use a qemu_irq with a timer programmed to generate the right frequency wave to drive the tone. So having the PC speaker hold a reference to the PIT and providing a high level method interface makes sense to me. Regards, Anthony Liguori
On 2011-03-06 18:47, Anthony Liguori wrote: > On 03/06/2011 10:47 AM, Blue Swirl wrote: >>> The PIT is not an ISA device. Modelling it as such is worse than >>> leaving it >>> unmodelled. >>> >> No. These days, PIT is part of Super I/O chip, which is accessed via >> LPC bus. LPC is from software point of view equal to ISA. Therefore, >> in absence of LPC, ISA is correct. >> > > In the system we model, the PIT is part of the PIIX3. The right way to > model it is as a DeviceState that's no_user=1 and created as part of the > initialized of PIIX3 (for the PC at least). > > LPC is still an expansion bus and it's primarily used for discrete > components like a TPM. For components that are all part of a Super I/O > chip, there really just isn't a bus in the middle. There surely is some bus (or even multiple), just not external an one. Most of the currently ISA-attached devices are chipset internal. They belong to the PIIX3, so they need to be attached to some bus that is owned by this device. If that is its ISA bus or a separate one for internal devices - really, this looks like an academic discussion to me. Jan
On 03/06/2011 12:06 PM, Jan Kiszka wrote: >> In the system we model, the PIT is part of the PIIX3. The right way to >> model it is as a DeviceState that's no_user=1 and created as part of the >> initialized of PIIX3 (for the PC at least). >> >> LPC is still an expansion bus and it's primarily used for discrete >> components like a TPM. For components that are all part of a Super I/O >> chip, there really just isn't a bus in the middle. >> > There surely is some bus (or even multiple), just not external an one. > It almost doesn't matter. It would look like: I8254 is-a DeviceState SuperIO has-a I8254 And the has-a relationship might be some custom bus mechanism (even if it's purely a VHDL or software concept). But in terms of modelling, we make I8254 a DeviceState because we don't care what bus it sits on. > Most of the currently ISA-attached devices are chipset internal. Yeah, and making them ISA devices was the wrong thing to do. This is all going to have to be redone in the not too distant future. An is-a relationship only makes sense when the device is naturally represented as the parent object. > They > belong to the PIIX3, so they need to be attached to some bus that is > owned by this device. If that is its ISA bus or a separate one for > internal devices - really, this looks like an academic discussion to me. > It's far from academic as this is user-visible and visible via the command line. Regards, Anthony Liguori > Jan > >
On Sun, Mar 6, 2011 at 10:45 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 03/06/2011 12:06 PM, Jan Kiszka wrote: >>> >>> In the system we model, the PIT is part of the PIIX3. The right way to >>> model it is as a DeviceState that's no_user=1 and created as part of the >>> initialized of PIIX3 (for the PC at least). >>> >>> LPC is still an expansion bus and it's primarily used for discrete >>> components like a TPM. For components that are all part of a Super I/O >>> chip, there really just isn't a bus in the middle. >>> >> >> There surely is some bus (or even multiple), just not external an one. >> > > It almost doesn't matter. It would look like: > > I8254 is-a DeviceState > > SuperIO has-a I8254 > > And the has-a relationship might be some custom bus mechanism (even if it's > purely a VHDL or software concept). > > But in terms of modelling, we make I8254 a DeviceState because we don't care > what bus it sits on. Southbridge (+Super I/O) provides an ISA bus, partially internal and external (LPC). i8254 is one of those devices, so calling it an ISADevice is not completely wrong. >> Most of the currently ISA-attached devices are chipset internal. > > Yeah, and making them ISA devices was the wrong thing to do. This is all > going to have to be redone in the not too distant future. I don't think the recent discussion about device models was very conclusive. Maybe patches would clarify. > An is-a relationship only makes sense when the device is naturally > represented as the parent object. > >> They >> belong to the PIIX3, so they need to be attached to some bus that is >> owned by this device. If that is its ISA bus or a separate one for >> internal devices - really, this looks like an academic discussion to me. >> > > It's far from academic as this is user-visible and visible via the command > line. Currently the device tree looks like this (extracted from 'info qtree' output): bus: main-system-bus type System dev: i440FX-pcihost, id "" irq 0 bus: pci.0 type PCI dev: PIIX3, id "" class ISA bridge, addr 00:01.0, pci id 8086:7000 (sub 1af4:1100) bus: isa.0 type ISA dev: isa-pit, id "" dev-prop: irq = 0 dev-prop: iobase = 0x40 I don't think this is completely wrong. 'isa-pit' device is in fact part of PIIX3 and that is not visible. The tree should probably reflect that somehow (for example, name the device PIIX3-isa-pit).
On 2011-03-06 21:45, Anthony Liguori wrote: > On 03/06/2011 12:06 PM, Jan Kiszka wrote: >>> In the system we model, the PIT is part of the PIIX3. The right way to >>> model it is as a DeviceState that's no_user=1 and created as part of the >>> initialized of PIIX3 (for the PC at least). >>> >>> LPC is still an expansion bus and it's primarily used for discrete >>> components like a TPM. For components that are all part of a Super I/O >>> chip, there really just isn't a bus in the middle. >>> >> There surely is some bus (or even multiple), just not external an one. >> > > It almost doesn't matter. It would look like: > > I8254 is-a DeviceState > > SuperIO has-a I8254 > > And the has-a relationship might be some custom bus mechanism (even if > it's purely a VHDL or software concept). > > But in terms of modelling, we make I8254 a DeviceState because we don't > care what bus it sits on. > >> Most of the currently ISA-attached devices are chipset internal. > > Yeah, and making them ISA devices was the wrong thing to do. This is > all going to have to be redone in the not too distant future. > > An is-a relationship only makes sense when the device is naturally > represented as the parent object. > >> They >> belong to the PIIX3, so they need to be attached to some bus that is >> owned by this device. If that is its ISA bus or a separate one for >> internal devices - really, this looks like an academic discussion to me. >> > > It's far from academic as this is user-visible and visible via the > command line. I thought it was stated before that there is no guarantee on the internal structure of our device tree as the user may explore it (as long as it's stable for the guest). Regarding command line: What are your worries here? The user can't mess with built-in devices. I still think we have more important things to improve than these cosmetic issues. Jan
On 03/06/2011 03:18 PM, Jan Kiszka wrote: >> It's far from academic as this is user-visible and visible via the >> command line. >> > I thought it was stated before that there is no guarantee on the > internal structure of our device tree as the user may explore it (as > long as it's stable for the guest). We have a lot of "unstable" interfaces that folks yell about every time it changes (like the -help output). Providing a bad external interface and justifying by saying its unstable is just asking for pain later. > Regarding command line: What are > your worries here? The user can't mess with built-in devices. > -global still applies to no_user devices. > I still think we have more important things to improve than these > cosmetic issues. > What does converting this device to qdev actually add other than an interface that we're not going to be able to support long term? Regards, Anthony Liguori > Jan > >
On 2011-03-07 01:32, Anthony Liguori wrote: > On 03/06/2011 03:18 PM, Jan Kiszka wrote: >>> It's far from academic as this is user-visible and visible via the >>> command line. >>> >> I thought it was stated before that there is no guarantee on the >> internal structure of our device tree as the user may explore it (as >> long as it's stable for the guest). > > We have a lot of "unstable" interfaces that folks yell about every time > it changes (like the -help output). Providing a bad external interface > and justifying by saying its unstable is just asking for pain later. This is really a "harmless", mostly read-only interface we are discussing here. > >> Regarding command line: What are >> your worries here? The user can't mess with built-in devices. >> > > -global still applies to no_user devices. Already tried "-global isa-pit.iobase=0x4711"? -global changes property defaults, not directly their values. > >> I still think we have more important things to improve than these >> cosmetic issues. >> > > What does converting this device to qdev actually add other than an > interface that we're not going to be able to support long term? This device is probably no big deal. But generally those conversions help to clean up or at least uncover twisted dependencies between devices. They are surely a step in the right direction as everything that follows qdev will have to improve it evolutionary (we can't effort a third device model in qemu), so will be able to build on top. The major issues I see when looking at more complex devices, specifically in the x86 world: - IRQ/GPIO binding should become a generic qdev service instead of a sysbus bonus (and its interface should be improved). - Buses may benefit from generic IRQ management as well. - We need a generic connector between devices, maybe multi-bus binding (e.g. to bind the IOAPIC both to the sysbus and the inter-processor bus), maybe PROP_TYPE_BUS that could be set via qtree path or alternatively a plain pointer. That should obsolete PROP_TYPE_PTR. And there are likely thousand things in the qdev model that could be made simpler, more handy to use. Jan
On 03/07/2011 01:58 AM, Jan Kiszka wrote: > On 2011-03-07 01:32, Anthony Liguori wrote: > >> On 03/06/2011 03:18 PM, Jan Kiszka wrote: >> >>>> It's far from academic as this is user-visible and visible via the >>>> command line. >>>> >>>> >>> I thought it was stated before that there is no guarantee on the >>> internal structure of our device tree as the user may explore it (as >>> long as it's stable for the guest). >>> >> We have a lot of "unstable" interfaces that folks yell about every time >> it changes (like the -help output). Providing a bad external interface >> and justifying by saying its unstable is just asking for pain later. >> > This is really a "harmless", mostly read-only interface we are > discussing here. > > >> >>> Regarding command line: What are >>> your worries here? The user can't mess with built-in devices. >>> >>> >> -global still applies to no_user devices. >> > Already tried "-global isa-pit.iobase=0x4711"? -global changes property > defaults, not directly their values. > And as soon as we do time drift catch-up, it's going to be an important interface to work with. >>> I still think we have more important things to improve than these >>> cosmetic issues. >>> >>> >> What does converting this device to qdev actually add other than an >> interface that we're not going to be able to support long term? >> > This device is probably no big deal. But generally those conversions > help to clean up or at least uncover twisted dependencies between > devices. They are surely a step in the right direction as everything > that follows qdev will have to improve it evolutionary (we can't effort > a third device model in qemu), so will be able to build on top. > We can convert it to a DeviceState, and then have it hang off of Sysbus (but created under PIIX3). It's less pretty from a tree perspective but at least the modelling is correct. We should model based on logic relationships, not how we want things to look in info qtree. > The major issues I see when looking at more complex devices, > specifically in the x86 world: > - IRQ/GPIO binding should become a generic qdev service instead of a > sysbus bonus (and its interface should be improved). > - Buses may benefit from generic IRQ management as well. > - We need a generic connector between devices, maybe multi-bus binding > (e.g. to bind the IOAPIC both to the sysbus and the inter-processor > bus), maybe PROP_TYPE_BUS that could be set via qtree path or > alternatively a plain pointer. That should obsolete PROP_TYPE_PTR. > > And there are likely thousand things in the qdev model that could be > made simpler, more handy to use. > My biggest concern moving forward is that the more legacy baggage we accumulate with the current qdev implementation, the tougher its going to be to improve it down the road. Regards, Anthony Liguori > Jan > >
On 2011-03-07 15:57, Anthony Liguori wrote: > On 03/07/2011 01:58 AM, Jan Kiszka wrote: >> On 2011-03-07 01:32, Anthony Liguori wrote: >> >>> On 03/06/2011 03:18 PM, Jan Kiszka wrote: >>> >>>>> It's far from academic as this is user-visible and visible via the >>>>> command line. >>>>> >>>>> >>>> I thought it was stated before that there is no guarantee on the >>>> internal structure of our device tree as the user may explore it (as >>>> long as it's stable for the guest). >>>> >>> We have a lot of "unstable" interfaces that folks yell about every time >>> it changes (like the -help output). Providing a bad external interface >>> and justifying by saying its unstable is just asking for pain later. >>> >> This is really a "harmless", mostly read-only interface we are >> discussing here. >> >> >>> >>>> Regarding command line: What are >>>> your worries here? The user can't mess with built-in devices. >>>> >>>> >>> -global still applies to no_user devices. >>> >> Already tried "-global isa-pit.iobase=0x4711"? -global changes property >> defaults, not directly their values. >> > > And as soon as we do time drift catch-up, it's going to be an important > interface to work with. /me confused. How does this relate to the original topic? > >>>> I still think we have more important things to improve than these >>>> cosmetic issues. >>>> >>>> >>> What does converting this device to qdev actually add other than an >>> interface that we're not going to be able to support long term? >>> >> This device is probably no big deal. But generally those conversions >> help to clean up or at least uncover twisted dependencies between >> devices. They are surely a step in the right direction as everything >> that follows qdev will have to improve it evolutionary (we can't effort >> a third device model in qemu), so will be able to build on top. >> > > We can convert it to a DeviceState, and then have it hang off of Sysbus > (but created under PIIX3). It's less pretty from a tree perspective but > at least the modelling is correct. > > We should model based on logic relationships, not how we want things to > look in info qtree. That looks fairly odd to me compared to other ISA/LPC-attached devices. Without better alternatives, consistency of the existing model is more important IMO. > >> The major issues I see when looking at more complex devices, >> specifically in the x86 world: >> - IRQ/GPIO binding should become a generic qdev service instead of a >> sysbus bonus (and its interface should be improved). >> - Buses may benefit from generic IRQ management as well. >> - We need a generic connector between devices, maybe multi-bus binding >> (e.g. to bind the IOAPIC both to the sysbus and the inter-processor >> bus), maybe PROP_TYPE_BUS that could be set via qtree path or >> alternatively a plain pointer. That should obsolete PROP_TYPE_PTR. >> >> And there are likely thousand things in the qdev model that could be >> made simpler, more handy to use. >> > > My biggest concern moving forward is that the more legacy baggage we > accumulate with the current qdev implementation, the tougher its going > to be to improve it down the road. Sticking with open-coded devices until some magic, perfect, easily introduceable qdev 2.0 arrives is worse than continuing the cleanup work and make all devices more regular and explorable. That's even more true as this conversion comes with no relevant legacy baggage IMHO. Moreover, every conversion that is done now (as the trivial ones are mostly through) gives valuable input for necessary improvements of qdev. Jan
diff --git a/hw/i8254.c b/hw/i8254.c index 06b225c..680caab 100644 --- a/hw/i8254.c +++ b/hw/i8254.c @@ -53,9 +53,12 @@ typedef struct PITChannelState { qemu_irq irq; } PITChannelState; -struct PITState { +typedef struct PITState { + ISADevice dev; + uint32_t irq; + uint32_t iobase; PITChannelState channels[3]; -}; +} PITState; static PITState pit_state; @@ -119,8 +122,9 @@ static int pit_get_out1(PITChannelState *s, int64_t current_time) return out; } -int pit_get_out(PITState *pit, int channel, int64_t current_time) +int pit_get_out(ISADevice *dev, int channel, int64_t current_time) { + PITState *pit = DO_UPCAST(PITState, dev, dev); PITChannelState *s = &pit->channels[channel]; return pit_get_out1(s, current_time); } @@ -179,8 +183,9 @@ static int64_t pit_get_next_transition_time(PITChannelState *s, } /* val must be 0 or 1 */ -void pit_set_gate(PITState *pit, int channel, int val) +void pit_set_gate(ISADevice *dev, int channel, int val) { + PITState *pit = DO_UPCAST(PITState, dev, dev); PITChannelState *s = &pit->channels[channel]; switch(s->mode) { @@ -210,20 +215,23 @@ void pit_set_gate(PITState *pit, int channel, int val) s->gate = val; } -int pit_get_gate(PITState *pit, int channel) +int pit_get_gate(ISADevice *dev, int channel) { + PITState *pit = DO_UPCAST(PITState, dev, dev); PITChannelState *s = &pit->channels[channel]; return s->gate; } -int pit_get_initial_count(PITState *pit, int channel) +int pit_get_initial_count(ISADevice *dev, int channel) { + PITState *pit = DO_UPCAST(PITState, dev, dev); PITChannelState *s = &pit->channels[channel]; return s->count; } -int pit_get_mode(PITState *pit, int channel) +int pit_get_mode(ISADevice *dev, int channel) { + PITState *pit = DO_UPCAST(PITState, dev, dev); PITChannelState *s = &pit->channels[channel]; return s->mode; } @@ -462,9 +470,9 @@ static const VMStateDescription vmstate_pit = { } }; -static void pit_reset(void *opaque) +static void pit_reset(DeviceState *dev) { - PITState *pit = opaque; + PITState *pit = container_of(dev, PITState, dev.qdev); PITChannelState *s; int i; @@ -498,20 +506,39 @@ void hpet_pit_enable(void) pit_load_count(s, 0); } -PITState *pit_init(int base, qemu_irq irq) +static int pit_initfn(ISADevice *dev) { - PITState *pit = &pit_state; + PITState *pit = DO_UPCAST(PITState, dev, dev); PITChannelState *s; s = &pit->channels[0]; /* the timer 0 is connected to an IRQ */ s->irq_timer = qemu_new_timer(vm_clock, pit_irq_timer, s); - s->irq = irq; + s->irq = isa_reserve_irq(pit->irq); - vmstate_register(NULL, base, &vmstate_pit, pit); - qemu_register_reset(pit_reset, pit); - register_ioport_write(base, 4, 1, pit_ioport_write, pit); - register_ioport_read(base, 3, 1, pit_ioport_read, pit); + register_ioport_write(pit->iobase, 4, 1, pit_ioport_write, pit); + register_ioport_read(pit->iobase, 3, 1, pit_ioport_read, pit); + isa_init_ioport(dev, pit->iobase); - return pit; + return 0; +} + +static ISADeviceInfo pit_info = { + .qdev.name = "isa-pit", + .qdev.size = sizeof(PITState), + .qdev.vmsd = &vmstate_pit, + .qdev.reset = pit_reset, + .qdev.no_user = 1, + .init = pit_initfn, + .qdev.props = (Property[]) { + DEFINE_PROP_UINT32("irq", PITState, irq, -1), + DEFINE_PROP_HEX32("iobase", PITState, iobase, -1), + DEFINE_PROP_END_OF_LIST(), + }, +}; + +static void pit_register(void) +{ + isa_qdev_register(&pit_info); } +device_init(pit_register) diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c index 2783ed5..f5ae639 100644 --- a/hw/mips_fulong2e.c +++ b/hw/mips_fulong2e.c @@ -67,7 +67,7 @@ #define FULONG2E_ATI_SLOT 6 #define FULONG2E_RTL8139_SLOT 7 -static PITState *pit; +static ISADevice *pit; static struct _loaderparams { int ram_size; @@ -369,7 +369,7 @@ static void mips_fulong2e_init(ram_addr_t ram_size, const char *boot_device, qdev_init_nofail(eeprom); /* init other devices */ - pit = pit_init(0x40, isa_reserve_irq(0)); + pit = pit_init(0x40, 0); cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); DMA_init(0, cpu_exit_irq); diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c index 85eba5a..a100394 100644 --- a/hw/mips_jazz.c +++ b/hw/mips_jazz.c @@ -115,7 +115,7 @@ void mips_jazz_init (ram_addr_t ram_size, void* rc4030_opaque; int s_rtc, s_dma_dummy; NICInfo *nd; - PITState *pit; + ISADevice *pit; DriveInfo *fds[MAX_FD]; qemu_irq esp_reset, dma_enable; qemu_irq *cpu_exit_irq; @@ -181,7 +181,7 @@ void mips_jazz_init (ram_addr_t ram_size, isa_bus_irqs(i8259); cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); DMA_init(0, cpu_exit_irq); - pit = pit_init(0x40, i8259[0]); + pit = pit_init(0x40, 0); pcspk_init(pit); /* ISA IO space at 0x90000000 */ diff --git a/hw/mips_malta.c b/hw/mips_malta.c index 930c51c..ca6c6d7 100644 --- a/hw/mips_malta.c +++ b/hw/mips_malta.c @@ -68,7 +68,7 @@ typedef struct { SerialState *uart; } MaltaFPGAState; -static PITState *pit; +static ISADevice *pit; static struct _loaderparams { int ram_size; @@ -930,7 +930,7 @@ void mips_malta_init (ram_addr_t ram_size, qdev_prop_set_ptr(eeprom, "data", eeprom_buf + (i * 256)); qdev_init_nofail(eeprom); } - pit = pit_init(0x40, isa_reserve_irq(0)); + pit = pit_init(0x40, 0); cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); DMA_init(0, cpu_exit_irq); diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c index fb34dcf..8feb461 100644 --- a/hw/mips_r4k.c +++ b/hw/mips_r4k.c @@ -30,7 +30,7 @@ static const int ide_iobase[2] = { 0x1f0, 0x170 }; static const int ide_iobase2[2] = { 0x3f6, 0x376 }; static const int ide_irq[2] = { 14, 15 }; -static PITState *pit; /* PIT i8254 */ +static ISADevice *pit; /* PIT i8254 */ /* i8254 PIT is attached to the IRQ0 at PIC i8259 */ @@ -274,7 +274,7 @@ void mips_r4k_init (ram_addr_t ram_size, isa_mmio_init(0x14000000, 0x00010000); isa_mem_base = 0x10000000; - pit = pit_init(0x40, i8259[0]); + pit = pit_init(0x40, 0); for(i = 0; i < MAX_SERIAL_PORTS; i++) { if (serial_hds[i]) { diff --git a/hw/pc.c b/hw/pc.c index 36641b3..3d7e036 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -1104,10 +1104,9 @@ void pc_basic_device_init(qemu_irq *isa_irq, { int i; DriveInfo *fd[MAX_FD]; - PITState *pit; qemu_irq rtc_irq = NULL; qemu_irq *a20_line; - ISADevice *i8042, *port92, *vmmouse; + ISADevice *i8042, *port92, *vmmouse, *pit; qemu_irq *cpu_exit_irq; register_ioport_write(0x80, 1, 1, ioport80_write, NULL); @@ -1128,7 +1127,7 @@ void pc_basic_device_init(qemu_irq *isa_irq, qemu_register_boot_set(pc_boot_set, *rtc_state); - pit = pit_init(0x40, isa_reserve_irq(0)); + pit = pit_init(0x40, 0); pcspk_init(pit); for(i = 0; i < MAX_SERIAL_PORTS; i++) { diff --git a/hw/pc.h b/hw/pc.h index 60f8c42..feb8a7a 100644 --- a/hw/pc.h +++ b/hw/pc.h @@ -82,14 +82,23 @@ void isa_irq_handler(void *opaque, int n, int level); #define PIT_FREQ 1193182 -typedef struct PITState PITState; +static inline ISADevice *pit_init(int base, int irq) +{ + ISADevice *dev; + + dev = isa_create("isa-pit"); + qdev_prop_set_uint32(&dev->qdev, "iobase", base); + qdev_prop_set_uint32(&dev->qdev, "irq", irq); + qdev_init_nofail(&dev->qdev); + + return dev; +} -PITState *pit_init(int base, qemu_irq irq); -void pit_set_gate(PITState *pit, int channel, int val); -int pit_get_gate(PITState *pit, int channel); -int pit_get_initial_count(PITState *pit, int channel); -int pit_get_mode(PITState *pit, int channel); -int pit_get_out(PITState *pit, int channel, int64_t current_time); +void pit_set_gate(ISADevice *dev, int channel, int val); +int pit_get_gate(ISADevice *dev, int channel); +int pit_get_initial_count(ISADevice *dev, int channel); +int pit_get_mode(ISADevice *dev, int channel); +int pit_get_out(ISADevice *dev, int channel, int64_t current_time); void hpet_pit_disable(void); void hpet_pit_enable(void); @@ -159,7 +168,7 @@ void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr); extern int no_hpet; /* pcspk.c */ -void pcspk_init(PITState *); +void pcspk_init(ISADevice *pit); int pcspk_audio_init(qemu_irq *pic); /* piix_pci.c */ diff --git a/hw/pcspk.c b/hw/pcspk.c index 26a0ecb..5f02908 100644 --- a/hw/pcspk.c +++ b/hw/pcspk.c @@ -37,7 +37,7 @@ typedef struct { uint8_t sample_buf[PCSPK_BUF_LEN]; QEMUSoundCard card; SWVoiceOut *voice; - PITState *pit; + ISADevice *pit; unsigned int pit_count; unsigned int samples; unsigned int play_pos; @@ -137,7 +137,7 @@ static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val) } } -void pcspk_init(PITState *pit) +void pcspk_init(ISADevice *pit) { PCSpkState *s = &pcspk_state; diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c index 6c1499a..6c9826b 100644 --- a/hw/ppc_prep.c +++ b/hw/ppc_prep.c @@ -82,7 +82,7 @@ static const int ide_irq[2] = { 13, 13 }; static uint32_t ne2000_io[NE2000_NB_MAX] = { 0x300, 0x320, 0x340, 0x360, 0x280, 0x380 }; static int ne2000_irq[NE2000_NB_MAX] = { 9, 10, 11, 3, 4, 5 }; -//static PITState *pit; +//static ISADevice *pit; /* ISA IO ports bridge */
Convert to qdev. Don't expose PITState. Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- hw/i8254.c | 61 +++++++++++++++++++++++++++++++++++++-------------- hw/mips_fulong2e.c | 4 +- hw/mips_jazz.c | 4 +- hw/mips_malta.c | 4 +- hw/mips_r4k.c | 4 +- hw/pc.c | 5 +-- hw/pc.h | 25 ++++++++++++++------ hw/pcspk.c | 4 +- hw/ppc_prep.c | 4 +- 9 files changed, 75 insertions(+), 40 deletions(-) #define PPC_IO_BASE 0x80000000 @@ -662,7 +662,7 @@ static void ppc_prep_init (ram_addr_t ram_size, /* init basic PC hardware */ pci_vga_init(pci_bus); // openpic = openpic_init(0x00000000, 0xF0000000, 1); - // pit = pit_init(0x40, i8259[0]); + // pit = pit_init(0x40, 0); rtc_init(2000, NULL); if (serial_hds[0])