Re: [PATCH 05/12] vt82c686: Make vt82c686b-pm an abstract base class and add vt8231-pm based on it

[BALATON Zoltan]

On Thu, 7 Jan 2021, Igor Mammedov wrote:
> On Thu, 7 Jan 2021 11:38:21 +0100 (CET)
> BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> > On Thu, 7 Jan 2021, Philippe Mathieu-Daudé wrote:
> > > Hi Zoltan,
> > >
> > > On 1/6/21 10:13 PM, BALATON Zoltan wrote:
> > > > The vt82c686b-pm model can be shared between VT82C686B and VT8231. The
> > > > only difference between the two is the device id in what we emulate so
> > > > make an abstract via-pm model by renaming appropriately and add types
> > > > for vt82c686b-pm and vt8231-pm based on it.
> > > >
> > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > ---
> > > >  hw/isa/vt82c686.c         | 87 ++++++++++++++++++++++++++-------------
> > > >  include/hw/isa/vt82c686.h |  1 +
> > > >  2 files changed, 59 insertions(+), 29 deletions(-)
> > > ...
> > >
> > > > +typedef struct via_pm_init_info {
> > > > +    uint16_t device_id;
> > > > +} ViaPMInitInfo;
> > > > +
> > > >  static void via_pm_class_init(ObjectClass *klass, void *data)
> > > >  {
> > > >      DeviceClass *dc = DEVICE_CLASS(klass);
> > > >      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > > > +    ViaPMInitInfo *info = data;
> > > >
> > > > -    k->realize = vt82c686b_pm_realize;
> > > > +    k->realize = via_pm_realize;
> > > >      k->config_write = pm_write_config;
> > > >      k->vendor_id = PCI_VENDOR_ID_VIA;
> > > > -    k->device_id = PCI_DEVICE_ID_VIA_ACPI;
> > > > +    k->device_id = info->device_id;
> > > >      k->class_id = PCI_CLASS_BRIDGE_OTHER;
> > > >      k->revision = 0x40;
> > > > -    dc->reset = vt82c686b_pm_reset;
> > > > -    dc->desc = "PM";
> > > > +    dc->reset = via_pm_reset;
> > >
> > > > +    /* Reason: part of VIA south bridge, does not exist stand alone */
> > > > +    dc->user_creatable = false;
> > > >      dc->vmsd = &vmstate_acpi;
> > > > -    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> > >
> > > Please do this change in a previous patch.
> >
> > OK, done.
> >
> > > >  }
> > > >
> > > >  static const TypeInfo via_pm_info = {
> > > > -    .name          = TYPE_VT82C686B_PM,
> > > > +    .name          = TYPE_VIA_PM,
> > > >      .parent        = TYPE_PCI_DEVICE,
> > > > -    .instance_size = sizeof(VT686PMState),
> > > > -    .class_init    = via_pm_class_init,
> > > > +    .instance_size = sizeof(ViaPMState),
> > > > +    .abstract      = true,
> > > >      .interfaces = (InterfaceInfo[]) {
> > > >          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> > > >          { },
> > > >      },
> > > >  };
> > > >
> > > > +static const ViaPMInitInfo vt82c686b_pm_init_info = {
> > > > +    .device_id = PCI_DEVICE_ID_VIA_ACPI,
> > > > +};
> > > > +
> > > > +static const TypeInfo vt82c686b_pm_info = {
> > > > +    .name          = TYPE_VT82C686B_PM,
> > > > +    .parent        = TYPE_VIA_PM,
> > > > +    .class_init    = via_pm_class_init,
> > > > +    .class_data    = (void *)&vt82c686b_pm_init_info,
> > >
> > > Igor said new code should avoid using .class_data:
> > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg678305.html
> > > Can you convert to "leaf class"? Then this patch is good to go.
> >
> > That says for machines it is not advised (and Igor generally prefers init
> > funcs everywhere) but this is a device model. Is it still not allowed to
> > use class_data here? I think this is shorter this way than with an init
> > function but I may try to convert if absolutely necessary.
>
> For this simple case class_init would be cleaner as it doesn't need casting 
> (void*).

Cast is only needed because of this:

../hw/isa/vt82c686.c:240:22: error: initialization discards ‘const’ qualifier 
from pointer target type [-Werror=discarded-qualifiers]
     .class_data    = &vt82c686b_pm_init_info,
                      ^
../hw/isa/vt82c686.c:251:22: error: initialization discards ‘const’ qualifier 
from pointer target type [-Werror=discarded-qualifiers]
     .class_data    = &vt8231_pm_init_info,
                      ^

Could be avoided by removing const from declaration, i.e.

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 604ab4a55e..14e9a4bf76 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -229,7 +229,7 @@ static const TypeInfo via_pm_info = {
     },
 };

-static const ViaPMInitInfo vt82c686b_pm_init_info = {
+static ViaPMInitInfo vt82c686b_pm_init_info = {
     .device_id = PCI_DEVICE_ID_VIA_ACPI,
 };

@@ -237,10 +237,10 @@ static const TypeInfo vt82c686b_pm_info = {
     .name          = TYPE_VT82C686B_PM,
     .parent        = TYPE_VIA_PM,
     .class_init    = via_pm_class_init,
-    .class_data    = (void *)&vt82c686b_pm_init_info,
+    .class_data    = &vt82c686b_pm_init_info,
 };

-static const ViaPMInitInfo vt8231_pm_init_info = {
+static ViaPMInitInfo vt8231_pm_init_info = {
     .device_id = 0x8235,
 };

@@ -248,7 +248,7 @@ static const TypeInfo vt8231_pm_info = {
     .name          = TYPE_VT8231_PM,
     .parent        = TYPE_VIA_PM,
     .class_init    = via_pm_class_init,
-    .class_data    = (void *)&vt8231_pm_init_info,
+    .class_data    = &vt8231_pm_init_info,
 };


Is that any better?

> But I'm fine with either approaches here.

In that case I'd just leave it as it is now unless you say otherwise.

> > > A trivial example of conversion is commit f0eeb4b6154
> > > ("hw/arm/raspi: Avoid using TypeInfo::class_data pointer").
> > >
> > > > +};
> > > > +
> > > > +static const ViaPMInitInfo vt8231_pm_init_info = {
> > > > +    .device_id = 0x8235,
>
> Is it possible to replace magic number with a human readable macro?

Yes, I'll need to add these IDs where the other constants are defined in 
include/hw/pci/pci_ids.h but I did not want to touch that file until now 
as I tried to localise changes only to hw/isa/vt82c686.c but I can move 
these to there as new constants.

Regards,
BALATON Zoltan

> > > > +};
> > > > +
> > > > +static const TypeInfo vt8231_pm_info = {
> > > > +    .name          = TYPE_VT8231_PM,
> > > > +    .parent        = TYPE_VIA_PM,
> > > > +    .class_init    = via_pm_class_init,
> > > > +    .class_data    = (void *)&vt8231_pm_init_info,
> > > > +};