diff mbox series

[1/2] ide: Make room for flags in PCIIDEState and add one for legacy IRQ routing

Message ID 775825dba26f6b36ab067f253e4ab5dc3a3d15dc.1583017348.git.balaton@eik.bme.hu (mailing list archive)
State New, archived
Headers show
Series Implement "non 100% native mode" in via-ide | expand

Commit Message

BALATON Zoltan Feb. 29, 2020, 11:02 p.m. UTC
We'll need a flag for implementing some device specific behaviour in
via-ide but we already have a currently CMD646 specific field that can
be repurposed for this and leave room for furhter flags if needed in
the future. This patch changes the "secondary" field to "flags" and
define the flags for CMD646 and via-ide and change CMD646 and its
users accordingly.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/alpha/dp264.c     |  2 +-
 hw/ide/cmd646.c      | 12 ++++++------
 hw/sparc64/sun4u.c   |  9 ++-------
 include/hw/ide.h     |  4 ++--
 include/hw/ide/pci.h |  7 ++++++-
 5 files changed, 17 insertions(+), 17 deletions(-)

Comments

Mark Cave-Ayland March 1, 2020, 11:32 a.m. UTC | #1
On 29/02/2020 23:02, BALATON Zoltan wrote:

> We'll need a flag for implementing some device specific behaviour in
> via-ide but we already have a currently CMD646 specific field that can
> be repurposed for this and leave room for furhter flags if needed in
> the future. This patch changes the "secondary" field to "flags" and
> define the flags for CMD646 and via-ide and change CMD646 and its
> users accordingly.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/alpha/dp264.c     |  2 +-
>  hw/ide/cmd646.c      | 12 ++++++------
>  hw/sparc64/sun4u.c   |  9 ++-------
>  include/hw/ide.h     |  4 ++--
>  include/hw/ide/pci.h |  7 ++++++-
>  5 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index 8d71a30617..e4075feaaf 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -102,7 +102,7 @@ static void clipper_init(MachineState *machine)
>          DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>          ide_drive_get(hd, ARRAY_SIZE(hd));
>  
> -        pci_cmd646_ide_init(pci_bus, hd, 0);
> +        pci_cmd646_ide_init(pci_bus, hd, -1, false);
>      }
>  
>      /* Load PALcode.  Given that this is not "real" cpu palcode,
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index 335c060673..0be650791f 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>      pci_conf[PCI_CLASS_PROG] = 0x8f;
>  
>      pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
> -    if (d->secondary) {
> +    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
>          /* XXX: if not enabled, really disable the seconday IDE controller */
>          pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
>      }
> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>      }
>  }
>  
> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> -                         int secondary_ide_enabled)
> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
> +                         bool secondary_ide_enabled)
>  {
>      PCIDevice *dev;
>  
> -    dev = pci_create(bus, -1, "cmd646-ide");
> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
> +    dev = pci_create(bus, devfn, "cmd646-ide");
> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>      qdev_init_nofail(&dev->qdev);
>  
>      pci_ide_create_devs(dev, hd_table);
>  }

Note that legacy init functions such as pci_cmd646_ide_init() should be removed where
possible, and in fact I posted a patch last week to completely remove it. This is
because using qdev directly allows each board to wire up the device as required,
rather than pushing it down into a set of init functions with different defaults.

Given that you're working in this area, I'd highly recommend doing the same for
via_ide_init() too.

>  static Property cmd646_ide_properties[] = {
> -    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
> +    DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index b7ac42f7a5..b64899300c 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -50,8 +50,7 @@
>  #include "hw/sparc/sparc64.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/sysbus.h"
> -#include "hw/ide.h"
> -#include "hw/ide/pci.h"
> +#include "hw/ide/internal.h"
>  #include "hw/loader.h"
>  #include "hw/fw-path-provider.h"
>  #include "elf.h"
> @@ -664,11 +663,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>      }
>  
>      ide_drive_get(hd, ARRAY_SIZE(hd));
> -
> -    pci_dev = pci_create(pci_busA, PCI_DEVFN(3, 0), "cmd646-ide");
> -    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
> -    qdev_init_nofail(&pci_dev->qdev);
> -    pci_ide_create_devs(pci_dev, hd);
> +    pci_cmd646_ide_init(pci_busA, hd, PCI_DEVFN(3, 0), true);
>  
>      /* Map NVRAM into I/O (ebus) space */
>      nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index 28d8a06439..d88c5ee695 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -12,8 +12,8 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>                          DriveInfo *hd0, DriveInfo *hd1);
>  
>  /* ide-pci.c */
> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> -                         int secondary_ide_enabled);
> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
> +                         bool secondary_ide_enabled);
>  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index a9f2c33e68..21075edf16 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -40,6 +40,11 @@ typedef struct BMDMAState {
>  #define TYPE_PCI_IDE "pci-ide"
>  #define PCI_IDE(obj) OBJECT_CHECK(PCIIDEState, (obj), TYPE_PCI_IDE)
>  
> +enum {
> +    PCI_IDE_SECONDARY, /* used only for cmd646 */
> +    PCI_IDE_LEGACY_IRQ
> +};
> +
>  typedef struct PCIIDEState {
>      /*< private >*/
>      PCIDevice parent_obj;
> @@ -47,7 +52,7 @@ typedef struct PCIIDEState {
>  
>      IDEBus bus[2];
>      BMDMAState bmdma[2];
> -    uint32_t secondary; /* used only for cmd646 */
> +    uint32_t flags;
>      MemoryRegion bmdma_bar;
>      MemoryRegion cmd_bar[2];
>      MemoryRegion data_bar[2];

ATB,

Mark.
BALATON Zoltan March 1, 2020, 3:27 p.m. UTC | #2
On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
> On 29/02/2020 23:02, BALATON Zoltan wrote:
>> We'll need a flag for implementing some device specific behaviour in
>> via-ide but we already have a currently CMD646 specific field that can
>> be repurposed for this and leave room for furhter flags if needed in
>> the future. This patch changes the "secondary" field to "flags" and
>> define the flags for CMD646 and via-ide and change CMD646 and its
>> users accordingly.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/alpha/dp264.c     |  2 +-
>>  hw/ide/cmd646.c      | 12 ++++++------
>>  hw/sparc64/sun4u.c   |  9 ++-------
>>  include/hw/ide.h     |  4 ++--
>>  include/hw/ide/pci.h |  7 ++++++-
>>  5 files changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
>> index 8d71a30617..e4075feaaf 100644
>> --- a/hw/alpha/dp264.c
>> +++ b/hw/alpha/dp264.c
>> @@ -102,7 +102,7 @@ static void clipper_init(MachineState *machine)
>>          DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>>          ide_drive_get(hd, ARRAY_SIZE(hd));
>>
>> -        pci_cmd646_ide_init(pci_bus, hd, 0);
>> +        pci_cmd646_ide_init(pci_bus, hd, -1, false);
>>      }
>>
>>      /* Load PALcode.  Given that this is not "real" cpu palcode,
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index 335c060673..0be650791f 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>>      pci_conf[PCI_CLASS_PROG] = 0x8f;
>>
>>      pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
>> -    if (d->secondary) {
>> +    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
>>          /* XXX: if not enabled, really disable the seconday IDE controller */
>>          pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
>>      }
>> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>>      }
>>  }
>>
>> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>> -                         int secondary_ide_enabled)
>> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>> +                         bool secondary_ide_enabled)
>>  {
>>      PCIDevice *dev;
>>
>> -    dev = pci_create(bus, -1, "cmd646-ide");
>> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
>> +    dev = pci_create(bus, devfn, "cmd646-ide");
>> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>>      qdev_init_nofail(&dev->qdev);
>>
>>      pci_ide_create_devs(dev, hd_table);
>>  }
>
> Note that legacy init functions such as pci_cmd646_ide_init() should be removed where
> possible, and in fact I posted a patch last week to completely remove it. This is
> because using qdev directly allows each board to wire up the device as required,
> rather than pushing it down into a set of init functions with different defaults.
>
> Given that you're working in this area, I'd highly recommend doing the same for
> via_ide_init() too.

I could do that, however these ide init functions seem to exist for piix, 
cmd646 and via-ide so that pci_ide_create_devs function is kept local to 
hw/ide. Nothing else called that func apart from sun4u so I've chosen this 
way to keep consistency (also keeps property type at one place instead of 
needing to change each board that sets property). If the consensus is that 
getting rid of these init funcs even if that means pci_ide_create_devs 
will not be local to ide any more I can go that way but would like to hear 
opinion of ide maintainer as well.

Regards,
BALATON Zoltan

>>  static Property cmd646_ide_properties[] = {
>> -    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
>> +    DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index b7ac42f7a5..b64899300c 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -50,8 +50,7 @@
>>  #include "hw/sparc/sparc64.h"
>>  #include "hw/nvram/fw_cfg.h"
>>  #include "hw/sysbus.h"
>> -#include "hw/ide.h"
>> -#include "hw/ide/pci.h"
>> +#include "hw/ide/internal.h"
>>  #include "hw/loader.h"
>>  #include "hw/fw-path-provider.h"
>>  #include "elf.h"
>> @@ -664,11 +663,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>>      }
>>
>>      ide_drive_get(hd, ARRAY_SIZE(hd));
>> -
>> -    pci_dev = pci_create(pci_busA, PCI_DEVFN(3, 0), "cmd646-ide");
>> -    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
>> -    qdev_init_nofail(&pci_dev->qdev);
>> -    pci_ide_create_devs(pci_dev, hd);
>> +    pci_cmd646_ide_init(pci_busA, hd, PCI_DEVFN(3, 0), true);
>>
>>      /* Map NVRAM into I/O (ebus) space */
>>      nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
>> diff --git a/include/hw/ide.h b/include/hw/ide.h
>> index 28d8a06439..d88c5ee695 100644
>> --- a/include/hw/ide.h
>> +++ b/include/hw/ide.h
>> @@ -12,8 +12,8 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>>                          DriveInfo *hd0, DriveInfo *hd1);
>>
>>  /* ide-pci.c */
>> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>> -                         int secondary_ide_enabled);
>> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>> +                         bool secondary_ide_enabled);
>>  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>>  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>>  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index a9f2c33e68..21075edf16 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -40,6 +40,11 @@ typedef struct BMDMAState {
>>  #define TYPE_PCI_IDE "pci-ide"
>>  #define PCI_IDE(obj) OBJECT_CHECK(PCIIDEState, (obj), TYPE_PCI_IDE)
>>
>> +enum {
>> +    PCI_IDE_SECONDARY, /* used only for cmd646 */
>> +    PCI_IDE_LEGACY_IRQ
>> +};
>> +
>>  typedef struct PCIIDEState {
>>      /*< private >*/
>>      PCIDevice parent_obj;
>> @@ -47,7 +52,7 @@ typedef struct PCIIDEState {
>>
>>      IDEBus bus[2];
>>      BMDMAState bmdma[2];
>> -    uint32_t secondary; /* used only for cmd646 */
>> +    uint32_t flags;
>>      MemoryRegion bmdma_bar;
>>      MemoryRegion cmd_bar[2];
>>      MemoryRegion data_bar[2];
>
> ATB,
>
> Mark.
>
>
Markus Armbruster March 2, 2020, 8:10 a.m. UTC | #3
BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
>> On 29/02/2020 23:02, BALATON Zoltan wrote:
>>> We'll need a flag for implementing some device specific behaviour in
>>> via-ide but we already have a currently CMD646 specific field that can
>>> be repurposed for this and leave room for furhter flags if needed in
>>> the future. This patch changes the "secondary" field to "flags" and
>>> define the flags for CMD646 and via-ide and change CMD646 and its
>>> users accordingly.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/alpha/dp264.c     |  2 +-
>>>  hw/ide/cmd646.c      | 12 ++++++------
>>>  hw/sparc64/sun4u.c   |  9 ++-------
>>>  include/hw/ide.h     |  4 ++--
>>>  include/hw/ide/pci.h |  7 ++++++-
>>>  5 files changed, 17 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
[...]
>>> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>>>      }
>>>  }
>>>
>>> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>>> -                         int secondary_ide_enabled)
>>> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>>> +                         bool secondary_ide_enabled)
>>>  {
>>>      PCIDevice *dev;
>>>
>>> -    dev = pci_create(bus, -1, "cmd646-ide");
>>> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
>>> +    dev = pci_create(bus, devfn, "cmd646-ide");
>>> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>>>      qdev_init_nofail(&dev->qdev);
>>>
>>>      pci_ide_create_devs(dev, hd_table);
>>>  }
>>
>> Note that legacy init functions such as pci_cmd646_ide_init() should be removed where
>> possible, and in fact I posted a patch last week to completely remove it. This is
>> because using qdev directly allows each board to wire up the device as required,
>> rather than pushing it down into a set of init functions with different defaults.
>>
>> Given that you're working in this area, I'd highly recommend doing the same for
>> via_ide_init() too.
>
> I could do that, however these ide init functions seem to exist for
> piix, cmd646 and via-ide so that pci_ide_create_devs function is kept
> local to hw/ide. Nothing else called that func apart from sun4u so
> I've chosen this way to keep consistency (also keeps property type at
> one place instead of needing to change each board that sets
> property). If the consensus is that getting rid of these init funcs
> even if that means pci_ide_create_devs will not be local to ide any
> more I can go that way but would like to hear opinion of ide
> maintainer as well.

I think Mark's point is that modelling a device and wiring up a device
model are separate things, and the latter belongs to the board.

pci_cmd646_ide_init() is a helper for boards.  Similar helpers exist
elsewhere.

In the oldest stratum of qdev code, such helpers were static inline
functions in the device model's .h.  That way, they're kind of separate
from the device model proper, in the .c, and kind of in the board code
where they belong, via inlining.  I've always considered that a terrible
idea; it's "kind of" as in "not really".  Over time, practice moved
first to putting the helpers into .c, then to open-coding the wiring
where it belongs: in the boards.

A few helper functions have survived, e.g. in hw/lm32/milkymist-hw.h,
and the IDE helpers we're discussing here.

Of course, when the code to wire up certain devices gets overly
repetitive, factoring out common code into helpers can make sense.  But
where to put them?  I can't see an obvious home for common board
helpers.  We tend to put these wiring helpers into a device model's .c
code for want of a better place.  Tolerable, I think.
Mark Cave-Ayland March 2, 2020, 7:13 p.m. UTC | #4
On 02/03/2020 08:10, Markus Armbruster wrote:

> BALATON Zoltan <balaton@eik.bme.hu> writes:
> 
>> On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
>>> On 29/02/2020 23:02, BALATON Zoltan wrote:
>>>> We'll need a flag for implementing some device specific behaviour in
>>>> via-ide but we already have a currently CMD646 specific field that can
>>>> be repurposed for this and leave room for furhter flags if needed in
>>>> the future. This patch changes the "secondary" field to "flags" and
>>>> define the flags for CMD646 and via-ide and change CMD646 and its
>>>> users accordingly.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/alpha/dp264.c     |  2 +-
>>>>  hw/ide/cmd646.c      | 12 ++++++------
>>>>  hw/sparc64/sun4u.c   |  9 ++-------
>>>>  include/hw/ide.h     |  4 ++--
>>>>  include/hw/ide/pci.h |  7 ++++++-
>>>>  5 files changed, 17 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> [...]
>>>> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>>>>      }
>>>>  }
>>>>
>>>> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>>>> -                         int secondary_ide_enabled)
>>>> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>>>> +                         bool secondary_ide_enabled)
>>>>  {
>>>>      PCIDevice *dev;
>>>>
>>>> -    dev = pci_create(bus, -1, "cmd646-ide");
>>>> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
>>>> +    dev = pci_create(bus, devfn, "cmd646-ide");
>>>> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>>>>      qdev_init_nofail(&dev->qdev);
>>>>
>>>>      pci_ide_create_devs(dev, hd_table);
>>>>  }
>>>
>>> Note that legacy init functions such as pci_cmd646_ide_init() should be removed where
>>> possible, and in fact I posted a patch last week to completely remove it. This is
>>> because using qdev directly allows each board to wire up the device as required,
>>> rather than pushing it down into a set of init functions with different defaults.
>>>
>>> Given that you're working in this area, I'd highly recommend doing the same for
>>> via_ide_init() too.
>>
>> I could do that, however these ide init functions seem to exist for
>> piix, cmd646 and via-ide so that pci_ide_create_devs function is kept
>> local to hw/ide. Nothing else called that func apart from sun4u so
>> I've chosen this way to keep consistency (also keeps property type at
>> one place instead of needing to change each board that sets
>> property). If the consensus is that getting rid of these init funcs
>> even if that means pci_ide_create_devs will not be local to ide any
>> more I can go that way but would like to hear opinion of ide
>> maintainer as well.
> 
> I think Mark's point is that modelling a device and wiring up a device
> model are separate things, and the latter belongs to the board.
> 
> pci_cmd646_ide_init() is a helper for boards.  Similar helpers exist
> elsewhere.
> 
> In the oldest stratum of qdev code, such helpers were static inline
> functions in the device model's .h.  That way, they're kind of separate
> from the device model proper, in the .c, and kind of in the board code
> where they belong, via inlining.  I've always considered that a terrible
> idea; it's "kind of" as in "not really".  Over time, practice moved
> first to putting the helpers into .c, then to open-coding the wiring
> where it belongs: in the boards.
> 
> A few helper functions have survived, e.g. in hw/lm32/milkymist-hw.h,
> and the IDE helpers we're discussing here.
> 
> Of course, when the code to wire up certain devices gets overly
> repetitive, factoring out common code into helpers can make sense.  But
> where to put them?  I can't see an obvious home for common board
> helpers.  We tend to put these wiring helpers into a device model's .c
> code for want of a better place.  Tolerable, I think.

Right, thanks for the more detailed explanation of what I was trying to say above.

As you say having helpers can definitely help avoid repetitive code, however there
was a case a few releases back when someone flipped a qdev property in a device
_init() helper function used to initialise one of the more common devices and it
broke several of the older machines.

So now I'm mainly of the opinion that if the helper is just instantiating a device,
setting qdev properties and then returning the device then you're better off moving
the initialisation into the board code to prevent problems like this occurring again
(and certainly this nudges us towards building machines from config files since all
the configuration/wiring is now done at board level).


ATB,

Mark.
BALATON Zoltan March 2, 2020, 11:26 p.m. UTC | #5
On Mon, 2 Mar 2020, Mark Cave-Ayland wrote:
> On 02/03/2020 08:10, Markus Armbruster wrote:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>> On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
>>>> On 29/02/2020 23:02, BALATON Zoltan wrote:
>>>>> We'll need a flag for implementing some device specific behaviour in
>>>>> via-ide but we already have a currently CMD646 specific field that can
>>>>> be repurposed for this and leave room for furhter flags if needed in
>>>>> the future. This patch changes the "secondary" field to "flags" and
>>>>> define the flags for CMD646 and via-ide and change CMD646 and its
>>>>> users accordingly.
>>>>>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>>  hw/alpha/dp264.c     |  2 +-
>>>>>  hw/ide/cmd646.c      | 12 ++++++------
>>>>>  hw/sparc64/sun4u.c   |  9 ++-------
>>>>>  include/hw/ide.h     |  4 ++--
>>>>>  include/hw/ide/pci.h |  7 ++++++-
>>>>>  5 files changed, 17 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
>> [...]
>>>>> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>>>>>      }
>>>>>  }
>>>>>
>>>>> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>>>>> -                         int secondary_ide_enabled)
>>>>> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>>>>> +                         bool secondary_ide_enabled)
>>>>>  {
>>>>>      PCIDevice *dev;
>>>>>
>>>>> -    dev = pci_create(bus, -1, "cmd646-ide");
>>>>> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
>>>>> +    dev = pci_create(bus, devfn, "cmd646-ide");
>>>>> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>>>>>      qdev_init_nofail(&dev->qdev);
>>>>>
>>>>>      pci_ide_create_devs(dev, hd_table);
>>>>>  }
>>>>
>>>> Note that legacy init functions such as pci_cmd646_ide_init() should be removed where
>>>> possible, and in fact I posted a patch last week to completely remove it. This is
>>>> because using qdev directly allows each board to wire up the device as required,
>>>> rather than pushing it down into a set of init functions with different defaults.
>>>>
>>>> Given that you're working in this area, I'd highly recommend doing the same for
>>>> via_ide_init() too.
>>>
>>> I could do that, however these ide init functions seem to exist for
>>> piix, cmd646 and via-ide so that pci_ide_create_devs function is kept
>>> local to hw/ide. Nothing else called that func apart from sun4u so
>>> I've chosen this way to keep consistency (also keeps property type at
>>> one place instead of needing to change each board that sets
>>> property). If the consensus is that getting rid of these init funcs
>>> even if that means pci_ide_create_devs will not be local to ide any
>>> more I can go that way but would like to hear opinion of ide
>>> maintainer as well.
>>
>> I think Mark's point is that modelling a device and wiring up a device
>> model are separate things, and the latter belongs to the board.
>>
>> pci_cmd646_ide_init() is a helper for boards.  Similar helpers exist
>> elsewhere.
>>
>> In the oldest stratum of qdev code, such helpers were static inline
>> functions in the device model's .h.  That way, they're kind of separate
>> from the device model proper, in the .c, and kind of in the board code
>> where they belong, via inlining.  I've always considered that a terrible
>> idea; it's "kind of" as in "not really".  Over time, practice moved
>> first to putting the helpers into .c, then to open-coding the wiring
>> where it belongs: in the boards.
>>
>> A few helper functions have survived, e.g. in hw/lm32/milkymist-hw.h,
>> and the IDE helpers we're discussing here.
>>
>> Of course, when the code to wire up certain devices gets overly
>> repetitive, factoring out common code into helpers can make sense.  But
>> where to put them?  I can't see an obvious home for common board
>> helpers.  We tend to put these wiring helpers into a device model's .c
>> code for want of a better place.  Tolerable, I think.
>
> Right, thanks for the more detailed explanation of what I was trying to say above.
>
> As you say having helpers can definitely help avoid repetitive code, however there
> was a case a few releases back when someone flipped a qdev property in a device
> _init() helper function used to initialise one of the more common devices and it
> broke several of the older machines.
>
> So now I'm mainly of the opinion that if the helper is just instantiating a device,
> setting qdev properties and then returning the device then you're better off moving
> the initialisation into the board code to prevent problems like this occurring again
> (and certainly this nudges us towards building machines from config files since all
> the configuration/wiring is now done at board level).

The conflicting interests here seem to be

1. Keeping pci_ide_create_devs() local to hw/ide
2. keeping things related to the board in board code and getting rid of 
init functions

I can't decide which of the above is more important or nicer but the patch 
I've proposed at least kept consistency with existing practice. If it's 
decided that board code should use pci_ide_create_devs directly instead we 
could either address that in a separate clean up series also getting rid 
of the piix init functions at the same time later or if Mark's series gets 
merged first I can rebase on that.

Regards,
BALATON Zoltan
diff mbox series

Patch

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 8d71a30617..e4075feaaf 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -102,7 +102,7 @@  static void clipper_init(MachineState *machine)
         DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
         ide_drive_get(hd, ARRAY_SIZE(hd));
 
-        pci_cmd646_ide_init(pci_bus, hd, 0);
+        pci_cmd646_ide_init(pci_bus, hd, -1, false);
     }
 
     /* Load PALcode.  Given that this is not "real" cpu palcode,
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 335c060673..0be650791f 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -256,7 +256,7 @@  static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
     pci_conf[PCI_CLASS_PROG] = 0x8f;
 
     pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
-    if (d->secondary) {
+    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
         /* XXX: if not enabled, really disable the seconday IDE controller */
         pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
     }
@@ -317,20 +317,20 @@  static void pci_cmd646_ide_exitfn(PCIDevice *dev)
     }
 }
 
-void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
-                         int secondary_ide_enabled)
+void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
+                         bool secondary_ide_enabled)
 {
     PCIDevice *dev;
 
-    dev = pci_create(bus, -1, "cmd646-ide");
-    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
+    dev = pci_create(bus, devfn, "cmd646-ide");
+    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
     qdev_init_nofail(&dev->qdev);
 
     pci_ide_create_devs(dev, hd_table);
 }
 
 static Property cmd646_ide_properties[] = {
-    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
+    DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index b7ac42f7a5..b64899300c 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -50,8 +50,7 @@ 
 #include "hw/sparc/sparc64.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/sysbus.h"
-#include "hw/ide.h"
-#include "hw/ide/pci.h"
+#include "hw/ide/internal.h"
 #include "hw/loader.h"
 #include "hw/fw-path-provider.h"
 #include "elf.h"
@@ -664,11 +663,7 @@  static void sun4uv_init(MemoryRegion *address_space_mem,
     }
 
     ide_drive_get(hd, ARRAY_SIZE(hd));
-
-    pci_dev = pci_create(pci_busA, PCI_DEVFN(3, 0), "cmd646-ide");
-    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
-    qdev_init_nofail(&pci_dev->qdev);
-    pci_ide_create_devs(pci_dev, hd);
+    pci_cmd646_ide_init(pci_busA, hd, PCI_DEVFN(3, 0), true);
 
     /* Map NVRAM into I/O (ebus) space */
     nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 28d8a06439..d88c5ee695 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -12,8 +12,8 @@  ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
                         DriveInfo *hd0, DriveInfo *hd1);
 
 /* ide-pci.c */
-void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
-                         int secondary_ide_enabled);
+void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
+                         bool secondary_ide_enabled);
 PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index a9f2c33e68..21075edf16 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -40,6 +40,11 @@  typedef struct BMDMAState {
 #define TYPE_PCI_IDE "pci-ide"
 #define PCI_IDE(obj) OBJECT_CHECK(PCIIDEState, (obj), TYPE_PCI_IDE)
 
+enum {
+    PCI_IDE_SECONDARY, /* used only for cmd646 */
+    PCI_IDE_LEGACY_IRQ
+};
+
 typedef struct PCIIDEState {
     /*< private >*/
     PCIDevice parent_obj;
@@ -47,7 +52,7 @@  typedef struct PCIIDEState {
 
     IDEBus bus[2];
     BMDMAState bmdma[2];
-    uint32_t secondary; /* used only for cmd646 */
+    uint32_t flags;
     MemoryRegion bmdma_bar;
     MemoryRegion cmd_bar[2];
     MemoryRegion data_bar[2];