| Message ID | 1303722395-10791-3-git-send-email-dbaryshkov@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 25 April 2011 11:06, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > Switch dscm1xxxx microdrive driver to use qdev infrastructure. > --- > hw/ide/microdrive.c | 49 +++++++++++++++++++++++++++++++++++++++---------- > hw/pcmcia.h | 2 +- > hw/spitz.c | 5 ++++- > hw/tosa.c | 5 ++++- > 4 files changed, 48 insertions(+), 13 deletions(-) > > diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c > index 9fbbf0e..7692603 100644 > --- a/hw/ide/microdrive.c > +++ b/hw/ide/microdrive.c > @@ -38,8 +38,8 @@ > > /* DSCM-1XXXX Microdrive hard disk with CF+ II / PCMCIA interface. */ > typedef struct { > - IDEBus bus; > PCMCIACardState card; > + IDEBus bus; > uint32_t attr_base; > uint32_t io_base; > > @@ -529,22 +529,51 @@ static int dscm1xxxx_detach(void *opaque) > return 0; > } > > -PCMCIACardState *dscm1xxxx_init(DriveInfo *bdrv) > +PCMCIACardState *dscm1xxxx_init(PCMCIASocket *socket, DriveInfo *bdrv) This looks like a regression that you have to pass the socket when creating a PCMCIA card. I consider it an advantage of the current code that pcmcia cards are hotswappable. Can we keep that with qdevification? Otherwise is there a gain from the qdevification? Cheers
Hello, On 5/16/11, andrzej zaborowski <balrogg@gmail.com> wrote: > On 25 April 2011 11:06, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > wrote: >> Switch dscm1xxxx microdrive driver to use qdev infrastructure. >> --- >> hw/ide/microdrive.c | 49 >> +++++++++++++++++++++++++++++++++++++++---------- >> hw/pcmcia.h | 2 +- >> hw/spitz.c | 5 ++++- >> hw/tosa.c | 5 ++++- >> 4 files changed, 48 insertions(+), 13 deletions(-) >> >> diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c >> index 9fbbf0e..7692603 100644 >> --- a/hw/ide/microdrive.c >> +++ b/hw/ide/microdrive.c >> @@ -38,8 +38,8 @@ >> >> /* DSCM-1XXXX Microdrive hard disk with CF+ II / PCMCIA interface. */ >> typedef struct { >> - IDEBus bus; >> PCMCIACardState card; >> + IDEBus bus; >> uint32_t attr_base; >> uint32_t io_base; >> >> @@ -529,22 +529,51 @@ static int dscm1xxxx_detach(void *opaque) >> return 0; >> } >> >> -PCMCIACardState *dscm1xxxx_init(DriveInfo *bdrv) >> +PCMCIACardState *dscm1xxxx_init(PCMCIASocket *socket, DriveInfo *bdrv) > > This looks like a regression that you have to pass the socket when > creating a PCMCIA card. I consider it an advantage of the current > code that pcmcia cards are hotswappable. Can we keep that with > qdevification? Otherwise is there a gain from the qdevification? Socket is required, as we have to know the QBus before creating the device on it. Reg. hotswap: my intent was to move reuse hotplug/hotswap from QDev layer. However I could not find a way to trigger device creation/removal at run time, so I was unable to even try to test this code path. How should I do that? Cards won´t be hotswitchable (you won´t be able to detach a card from one socket and then put it back to the other one, however PCMCIA code should be hotplugable (i.e. you should be able to unplug a card from socket and then plug another one in).
On 16 May 2011 06:54, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > Hello, > > On 5/16/11, andrzej zaborowski <balrogg@gmail.com> wrote: >> On 25 April 2011 11:06, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> >> wrote: >>> Switch dscm1xxxx microdrive driver to use qdev infrastructure. >>> --- >>> hw/ide/microdrive.c | 49 >>> +++++++++++++++++++++++++++++++++++++++---------- >>> hw/pcmcia.h | 2 +- >>> hw/spitz.c | 5 ++++- >>> hw/tosa.c | 5 ++++- >>> 4 files changed, 48 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c >>> index 9fbbf0e..7692603 100644 >>> --- a/hw/ide/microdrive.c >>> +++ b/hw/ide/microdrive.c >>> @@ -38,8 +38,8 @@ >>> >>> /* DSCM-1XXXX Microdrive hard disk with CF+ II / PCMCIA interface. */ >>> typedef struct { >>> - IDEBus bus; >>> PCMCIACardState card; >>> + IDEBus bus; >>> uint32_t attr_base; >>> uint32_t io_base; >>> >>> @@ -529,22 +529,51 @@ static int dscm1xxxx_detach(void *opaque) >>> return 0; >>> } >>> >>> -PCMCIACardState *dscm1xxxx_init(DriveInfo *bdrv) >>> +PCMCIACardState *dscm1xxxx_init(PCMCIASocket *socket, DriveInfo *bdrv) >> >> This looks like a regression that you have to pass the socket when >> creating a PCMCIA card. I consider it an advantage of the current >> code that pcmcia cards are hotswappable. Can we keep that with >> qdevification? Otherwise is there a gain from the qdevification? > > Socket is required, as we have to know the QBus before creating the > device on it. Let's skip the qbusification then. It seems that qbus is a wrong choice for pcmcia and there are no new features or bugs fixed by the conversion, it's code motion? I also don't see why the socket structure should be needed at the creation time of a PCI device for example, the BusInfo should be enough logically. Cheers
On 5/16/11, andrzej zaborowski <balrogg@gmail.com> wrote: > On 16 May 2011 06:54, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: >> Hello, >> >> On 5/16/11, andrzej zaborowski <balrogg@gmail.com> wrote: >>> On 25 April 2011 11:06, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> >>> wrote: >>>> Switch dscm1xxxx microdrive driver to use qdev infrastructure. >>>> --- >>>> hw/ide/microdrive.c | 49 >>>> +++++++++++++++++++++++++++++++++++++++---------- >>>> hw/pcmcia.h | 2 +- >>>> hw/spitz.c | 5 ++++- >>>> hw/tosa.c | 5 ++++- >>>> 4 files changed, 48 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c >>>> index 9fbbf0e..7692603 100644 >>>> --- a/hw/ide/microdrive.c >>>> +++ b/hw/ide/microdrive.c >>>> @@ -38,8 +38,8 @@ >>>> >>>> /* DSCM-1XXXX Microdrive hard disk with CF+ II / PCMCIA interface. */ >>>> typedef struct { >>>> - IDEBus bus; >>>> PCMCIACardState card; >>>> + IDEBus bus; >>>> uint32_t attr_base; >>>> uint32_t io_base; >>>> >>>> @@ -529,22 +529,51 @@ static int dscm1xxxx_detach(void *opaque) >>>> return 0; >>>> } >>>> >>>> -PCMCIACardState *dscm1xxxx_init(DriveInfo *bdrv) >>>> +PCMCIACardState *dscm1xxxx_init(PCMCIASocket *socket, DriveInfo *bdrv) >>> >>> This looks like a regression that you have to pass the socket when >>> creating a PCMCIA card. I consider it an advantage of the current >>> code that pcmcia cards are hotswappable. Can we keep that with >>> qdevification? Otherwise is there a gain from the qdevification? >> >> Socket is required, as we have to know the QBus before creating the >> device on it. > > Let's skip the qbusification then. It seems that qbus is a wrong > choice for pcmcia and there are no new features or bugs fixed by the > conversion, it's code motion? I also don't see why the socket > structure should be needed at the creation time of a PCI device for > example, the BusInfo should be enough logically. Major point for qbus'ification was ability to create PCMCIA devices from command line/via other management tools. This would also allow us e.g. to move microdrive driver to common ide parts, etc. For creation of a DeviceState via qdev_create you need BusState (which is a part of PCMCIASocket). Of course I can make one global QBus for all PCMCIA devices and make some artificial hacks to attach/detach cards to artificial sockets, but this seems like a hack.
On 16 May 2011 15:08, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > On 5/16/11, andrzej zaborowski <balrogg@gmail.com> wrote: >> On 16 May 2011 06:54, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: >>> Socket is required, as we have to know the QBus before creating the >>> device on it. >> >> Let's skip the qbusification then. It seems that qbus is a wrong >> choice for pcmcia and there are no new features or bugs fixed by the >> conversion, it's code motion? I also don't see why the socket >> structure should be needed at the creation time of a PCI device for >> example, the BusInfo should be enough logically. > > Major point for qbus'ification was ability to create PCMCIA devices from > command line/via other management tools. This would also allow us e.g. > to move microdrive driver to common ide parts, etc. That would be nice but it may be better to use separate command line switches / monitor commands for hotpluggable busses. > > For creation of a DeviceState via qdev_create you need BusState (which > is a part of PCMCIASocket). Of course I can make one global QBus for > all PCMCIA devices and make some artificial hacks to attach/detach cards > to artificial sockets, but this seems like a hack. I considered that for a moment too but it's uglier than current code and doesn't achieve what you want, because the command line has no provision for triggering attachment. A major problem with qdev I see now is that the creation and attachment of a device are one event instead of two, which is the case for pcmcia. So your patch tries to merge these two events. Cheers
On 2011-05-17 03:38, andrzej zaborowski wrote: > On 16 May 2011 15:08, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: >> On 5/16/11, andrzej zaborowski <balrogg@gmail.com> wrote: >>> On 16 May 2011 06:54, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: >>>> Socket is required, as we have to know the QBus before creating the >>>> device on it. >>> >>> Let's skip the qbusification then. It seems that qbus is a wrong >>> choice for pcmcia and there are no new features or bugs fixed by the >>> conversion, it's code motion? I also don't see why the socket >>> structure should be needed at the creation time of a PCI device for >>> example, the BusInfo should be enough logically. >> >> Major point for qbus'ification was ability to create PCMCIA devices from >> command line/via other management tools. This would also allow us e.g. >> to move microdrive driver to common ide parts, etc. > > That would be nice but it may be better to use separate command line > switches / monitor commands for hotpluggable busses. > >> >> For creation of a DeviceState via qdev_create you need BusState (which >> is a part of PCMCIASocket). Of course I can make one global QBus for >> all PCMCIA devices and make some artificial hacks to attach/detach cards >> to artificial sockets, but this seems like a hack. > > I considered that for a moment too but it's uglier than current code > and doesn't achieve what you want, because the command line has no > provision for triggering attachment. A major problem with qdev I see > now is that the creation and attachment of a device are one event > instead of two, which is the case for pcmcia. So your patch tries to > merge these two events. What is the point of allowing the existence of unattached pcmcia devices? I think there was similar discussion about usb to allow attach detach without delete, but IIRC that was finally rejected as there is no real benefit in avoiding full creation/destruction. Keep in mind that there may be a day where we finally obsolete support for non-qdev (or whatever it's name will be then) devices. Every device needs to be discoverable for QEMU in a generic way, e.g. to decide if there are devices attached that do not support save/restore or to explore its state. Rejecting a qdev conversion looks a bit strange from that perspective. Jan
On 17 May 2011 07:44, Jan Kiszka <jan.kiszka@web.de> wrote: > On 2011-05-17 03:38, andrzej zaborowski wrote: >> On 16 May 2011 15:08, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: >>> On 5/16/11, andrzej zaborowski <balrogg@gmail.com> wrote: >>>> On 16 May 2011 06:54, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: >>>>> Socket is required, as we have to know the QBus before creating the >>>>> device on it. >>>> >>>> Let's skip the qbusification then. It seems that qbus is a wrong >>>> choice for pcmcia and there are no new features or bugs fixed by the >>>> conversion, it's code motion? I also don't see why the socket >>>> structure should be needed at the creation time of a PCI device for >>>> example, the BusInfo should be enough logically. >>> >>> Major point for qbus'ification was ability to create PCMCIA devices from >>> command line/via other management tools. This would also allow us e.g. >>> to move microdrive driver to common ide parts, etc. >> >> That would be nice but it may be better to use separate command line >> switches / monitor commands for hotpluggable busses. >> >>> >>> For creation of a DeviceState via qdev_create you need BusState (which >>> is a part of PCMCIASocket). Of course I can make one global QBus for >>> all PCMCIA devices and make some artificial hacks to attach/detach cards >>> to artificial sockets, but this seems like a hack. >> >> I considered that for a moment too but it's uglier than current code >> and doesn't achieve what you want, because the command line has no >> provision for triggering attachment. A major problem with qdev I see >> now is that the creation and attachment of a device are one event >> instead of two, which is the case for pcmcia. So your patch tries to >> merge these two events. > > What is the point of allowing the existence of unattached pcmcia > devices? I think there was similar discussion about usb to allow attach > detach without delete, but IIRC that was finally rejected as there is no > real benefit in avoiding full creation/destruction. It's more about being able to detach and re-attach (in the same socket or another), migrate, savevm/loadvm separately from the machine although this possibility is not used now anyway. I just think it's logical for a hotpluggable bus that this be possible and it's wrong to require the socket structure when creating a device, although I'll ack/push the patches if that's a general opinion. > > Keep in mind that there may be a day where we finally obsolete support > for non-qdev (or whatever it's name will be then) devices. Not allowing non-qdev devices would be difficult to do because a "device" is just a set of memory mappings and it's a fuzzy term altogether (in SoCs especially). What I'd like to avoid is shuffling a piece of code into an api it does not fit just because there's a trend to use it, you can burn cycles endlessly reordering code with no new features/bugs fixed. Cheers
On 2011-05-17 13:08, andrzej zaborowski wrote: > On 17 May 2011 07:44, Jan Kiszka <jan.kiszka@web.de> wrote: >> On 2011-05-17 03:38, andrzej zaborowski wrote: >>> On 16 May 2011 15:08, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: >>>> On 5/16/11, andrzej zaborowski <balrogg@gmail.com> wrote: >>>>> On 16 May 2011 06:54, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: >>>>>> Socket is required, as we have to know the QBus before creating the >>>>>> device on it. >>>>> >>>>> Let's skip the qbusification then. It seems that qbus is a wrong >>>>> choice for pcmcia and there are no new features or bugs fixed by the >>>>> conversion, it's code motion? I also don't see why the socket >>>>> structure should be needed at the creation time of a PCI device for >>>>> example, the BusInfo should be enough logically. >>>> >>>> Major point for qbus'ification was ability to create PCMCIA devices from >>>> command line/via other management tools. This would also allow us e.g. >>>> to move microdrive driver to common ide parts, etc. >>> >>> That would be nice but it may be better to use separate command line >>> switches / monitor commands for hotpluggable busses. >>> >>>> >>>> For creation of a DeviceState via qdev_create you need BusState (which >>>> is a part of PCMCIASocket). Of course I can make one global QBus for >>>> all PCMCIA devices and make some artificial hacks to attach/detach cards >>>> to artificial sockets, but this seems like a hack. >>> >>> I considered that for a moment too but it's uglier than current code >>> and doesn't achieve what you want, because the command line has no >>> provision for triggering attachment. A major problem with qdev I see >>> now is that the creation and attachment of a device are one event >>> instead of two, which is the case for pcmcia. So your patch tries to >>> merge these two events. >> >> What is the point of allowing the existence of unattached pcmcia >> devices? I think there was similar discussion about usb to allow attach >> detach without delete, but IIRC that was finally rejected as there is no >> real benefit in avoiding full creation/destruction. > > It's more about being able to detach and re-attach (in the same socket > or another) I haven't looked at the details of this particular issue, but from 10000 meters I do not yet understand how qdev prevents this. Do we lack addressability via qdev for these sockets? Then that would have to be fixed. >, migrate, savevm/loadvm separately from the machine > although this possibility is not used now anyway. I just think it's > logical for a hotpluggable bus that this be possible and it's wrong to > require the socket structure when creating a device, although I'll > ack/push the patches if that's a general opinion. > >> >> Keep in mind that there may be a day where we finally obsolete support >> for non-qdev (or whatever it's name will be then) devices. > > Not allowing non-qdev devices would be difficult to do because a > "device" is just a set of memory mappings and it's a fuzzy term > altogether (in SoCs especially). Even in the SoC domain, I did not come across any set of "memory mappings" that could not reasonably be abstractable to a device, thus could be wrapped by qdev. That there is usually no chip containing such a device in reality does not mean you can't and shouldn't handle it as an abstract one, encapsulating separate functions in a more complex chip. > What I'd like to avoid is shuffling > a piece of code into an api it does not fit just because there's a > trend to use it, you can burn cycles endlessly reordering code with no > new features/bugs fixed. No longer having arbitrary, untraceable memory and io mappings but only well organized devices is a feature worth such shuffling. Usually that also offers the chance to clean up legacy code or complete half-done device models. We are good in inventing new APIs in QEMU, but so far we are not that successful getting rid of old ones. Jan
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c index 9fbbf0e..7692603 100644 --- a/hw/ide/microdrive.c +++ b/hw/ide/microdrive.c @@ -38,8 +38,8 @@ /* DSCM-1XXXX Microdrive hard disk with CF+ II / PCMCIA interface. */ typedef struct { - IDEBus bus; PCMCIACardState card; + IDEBus bus; uint32_t attr_base; uint32_t io_base; @@ -529,22 +529,51 @@ static int dscm1xxxx_detach(void *opaque) return 0; } -PCMCIACardState *dscm1xxxx_init(DriveInfo *bdrv) +PCMCIACardState *dscm1xxxx_init(PCMCIASocket *socket, DriveInfo *bdrv) +{ + DeviceState *dev; + MicroDriveState *md; + + dev = qdev_create(&socket->qbus, "dscm1xxxx"); + qdev_init_nofail(dev); + md = DO_UPCAST(MicroDriveState, card.dev, dev); + + ide_create_drive(&md->bus, 0, bdrv); + md->bus.ifs[0].drive_kind = IDE_CFATA; + md->bus.ifs[0].mdata_size = METADATA_SIZE; + md->bus.ifs[0].mdata_storage = (uint8_t *) qemu_mallocz(METADATA_SIZE); + + return &md->card; +} + +static int dscm1xxxx_initfn(PCMCIACardState *state) { - MicroDriveState *md = (MicroDriveState *) qemu_mallocz(sizeof(MicroDriveState)); + MicroDriveState *md; + md = DO_UPCAST(MicroDriveState, card, state); + md->card.state = md; md->card.attach = dscm1xxxx_attach; md->card.detach = dscm1xxxx_detach; md->card.cis = dscm1xxxx_cis; md->card.cis_len = sizeof(dscm1xxxx_cis); - ide_init2_with_non_qdev_drives(&md->bus, bdrv, NULL, - qemu_allocate_irqs(md_set_irq, md, 1)[0]); - md->bus.ifs[0].drive_kind = IDE_CFATA; - md->bus.ifs[0].mdata_size = METADATA_SIZE; - md->bus.ifs[0].mdata_storage = (uint8_t *) qemu_mallocz(METADATA_SIZE); + ide_bus_new(&md->bus, &state->dev, 0); + qdev_init_gpio_in(&state->dev, md_set_irq, 1); + ide_init2(&md->bus, qdev_get_gpio_in(&state->dev, 0)); - vmstate_register(NULL, -1, &vmstate_microdrive, md); + return 0; +} - return &md->card; +static PCMCIACardInfo dscm1xxxx_info = { + .qdev.name = "dscm1xxxx", + .qdev.desc = "QEMU CF MicroDrive emulation", + .init = dscm1xxxx_initfn, + .qdev.size = sizeof(MicroDriveState), + .qdev.vmsd = &vmstate_microdrive, +}; + +static void dscm1xxxx_register(void) +{ + pcmcia_card_register(&dscm1xxxx_info); } +device_init(dscm1xxxx_register); diff --git a/hw/pcmcia.h b/hw/pcmcia.h index 561d86c..c6b8100 100644 --- a/hw/pcmcia.h +++ b/hw/pcmcia.h @@ -64,4 +64,4 @@ void pcmcia_card_register(PCMCIACardInfo *info); DeviceState *pxa2xx_pcmcia_init(target_phys_addr_t base, uint8_t id); /* dscm1xxxx.c */ -PCMCIACardState *dscm1xxxx_init(DriveInfo *bdrv); +PCMCIACardState *dscm1xxxx_init(PCMCIASocket *socket, DriveInfo *bdrv); diff --git a/hw/spitz.c b/hw/spitz.c index ce19b5a..51cc08c 100644 --- a/hw/spitz.c +++ b/hw/spitz.c @@ -714,7 +714,10 @@ static void spitz_microdrive_attach(PXA2xxState *cpu, int slot) return; bs = dinfo->bdrv; if (bdrv_is_inserted(bs) && !bdrv_is_removable(bs)) { - md = dscm1xxxx_init(dinfo); + md = dscm1xxxx_init( + DO_UPCAST(PCMCIASocket, qbus, + qdev_get_child_bus(cpu->pcmcia[slot], "pcmcia")), + dinfo); pxa2xx_pcmcia_attach(cpu->pcmcia[slot], md); } } diff --git a/hw/tosa.c b/hw/tosa.c index 577b59f..f00555b 100644 --- a/hw/tosa.c +++ b/hw/tosa.c @@ -59,7 +59,10 @@ static void tosa_microdrive_attach(PXA2xxState *cpu) return; bs = dinfo->bdrv; if (bdrv_is_inserted(bs) && !bdrv_is_removable(bs)) { - md = dscm1xxxx_init(dinfo); + md = dscm1xxxx_init( + DO_UPCAST(PCMCIASocket, qbus, + qdev_get_child_bus(cpu->pcmcia[0], "pcmcia")), + dinfo); pxa2xx_pcmcia_attach(cpu->pcmcia[0], md); } }