Re: [PATCH 05/13] hw/ide: Extract pci_ide_class_init()

[BALATON Zoltan]

On Sun, 23 Apr 2023, Bernhard Beschow wrote:
> Am 23. April 2023 17:41:33 UTC schrieb "Philippe Mathieu-Daudé" 
> <philmd@linaro.org>:
> > On 22/4/23 17:07, Bernhard Beschow wrote:
> > > Resolves redundant code in every PCI IDE device model.
> > > ---
> > >   include/hw/ide/pci.h |  1 -
> > >   hw/ide/cmd646.c      | 15 ---------------
> > >   hw/ide/pci.c         | 25 ++++++++++++++++++++++++-
> > >   hw/ide/piix.c        | 19 -------------------
> > >   hw/ide/sii3112.c     |  3 ++-
> > >   hw/ide/via.c         | 15 ---------------
> > >   6 files changed, 26 insertions(+), 52 deletions(-)
> >
> >
> > > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
> > > index 5dd3d03c29..0af897a9ef 100644
> > > --- a/hw/ide/sii3112.c
> > > +++ b/hw/ide/sii3112.c
> > > @@ -301,9 +301,10 @@ static void sii3112_pci_class_init(ObjectClass *klass, 
> > > void *data)
> > >       pd->class_id = PCI_CLASS_STORAGE_RAID;
> > >       pd->revision = 1;
> > >       pd->realize = sii3112_pci_realize;
> > > +    pd->exit = NULL;
> > >       dc->reset = sii3112_reset;
> > > +    dc->vmsd = NULL;
> > >       dc->desc = "SiI3112A SATA controller";
> >
> > The SiI3112A doesn't have these regions?
>
> Yeah, it ignores a lot of stuff in the base class. This gets changed in 
> the last part of this series though. This seems why there is no exit 
> method. Furthermore -- probably due to additional custom fields -- there 
> is no migration description.

Probably there's no state descriptor because I did not bother to implement 
it back then when I did not even know how that worked. I've considered 
extending this to a 4 port version before adding migration/save support 
but that did not happen. This is only used on sam460ex by default and 
likely nobody wants to migrate that anyway.

However why do you need to explicitly set these to NULL? Aren't those 
structs allocated 0 filled so you'd only need to set non-NULL members.

As for ignoting stuff in the base class, this isn't really a PCI IDE 
controller. It's a SATA controller that for compatibility with older 
drivers looks a lot like an IDE controller but handles only one device per 
channel and has some additional SATA stuff that we mostly don't model. 
This way I could reuse code that was already there but still had some 
duplication that you're now resolving.

Regards,
BALATON Zoltan