Re: [PATCH v5 4/8] vt82c686: Introduce abstract TYPE_VIA_ISA and base vt82c686b_isa on it

[David Gibson]

On Tue, Mar 09, 2021 at 10:12:24AM +0100, Philippe Mathieu-Daudé wrote:
> On 3/5/21 2:02 AM, David Gibson wrote:
> > On Thu, Mar 04, 2021 at 11:42:10PM +0100, Philippe Mathieu-Daudé wrote:
> >> On 3/4/21 9:16 PM, BALATON Zoltan wrote:
> >>> On Thu, 4 Mar 2021, Philippe Mathieu-Daudé wrote:
> >>>> On 3/2/21 10:11 PM, BALATON Zoltan wrote:
> >>>>> To allow reusing ISA bridge emulation for vt8231_isa move the device
> >>>>> state of vt82c686b_isa emulation in an abstract via_isa class.
> >>>>>
> >>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >>>>> ---
> >>>>>  hw/isa/vt82c686.c        | 70 ++++++++++++++++++++++------------------
> >>>>>  include/hw/pci/pci_ids.h |  2 +-
> >>>>>  2 files changed, 40 insertions(+), 32 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> >>>>> index 72234bc4d1..5137f97f37 100644
> >>>>> --- a/hw/isa/vt82c686.c
> >>>>> +++ b/hw/isa/vt82c686.c
> >>>>> @@ -609,24 +609,48 @@ static const TypeInfo vt8231_superio_info = {
> >>>>>  };
> >>>>>
> >>>>>
> >>>>> -OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)
> >>>>> +#define TYPE_VIA_ISA "via-isa"
> >>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
> >>>>>
> >>>>> -struct VT82C686BISAState {
> >>>>> +struct ViaISAState {
> >>>>>      PCIDevice dev;
> >>>>>      qemu_irq cpu_intr;
> >>>>>      ViaSuperIOState *via_sio;
> >>>>>  };
> >>>>>
> >>>>> +static const VMStateDescription vmstate_via = {
> >>>>> +    .name = "via-isa",
> >>>>
> >>>> You changed the migration stream name, so I think we have
> >>>> a problem with migration... No clue how to do that properly.
> >>>
> >>> I don't think these machines support migration or state description of
> >>> vt86c686b was not missing something before these patches that would make
> >>> it not work anyway so I did not worry about this too much. I doubt
> >>> anybody wants to migrate a fuloong2e machine so this should not be a
> >>> problem in practice but maybe you can mention it in the release notes if
> >>> you think that would be necessary.
> >>
> >> Maybe just add in the description:
> >>
> >>  This change breaks migration back compatibility, but
> >>  this is not an issue for the Fuloong2E machine.
> > 
> > Hrm.  If migration was never supported, why is there a vmstate
> > description there at all though?
> > 
> > That said, I don't think breaking compat is a problem: that's only an
> > issue where we actually have versioned machine types, which covers
> > only pc, pseries, arm virt and a very few others.  I don't think this
> > device was used on any of them.
> 
> OK. Can you provide a formal Ack-by tag then? :)

Not really - this device isn't within the things I maintain, and I
don't really understand it.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature