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

[Philippe Mathieu-Daudé]

On Tue, Mar 9, 2021 at 6:03 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> +Gerd
>
> On 3/9/21 5:00 PM, Dr. David Alan Gilbert wrote:
> > * David Gibson (david@gibson.dropbear.id.au) 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.
> >
> > Except 'vt82c686b-usb-uhci' is a generic PCI device that anyone can
> > instantiate, so it's not actually Fuloong specific.
>
> I tend to see this as a bug, as this is a function specific to the
> southbridge chipset and isn't meant to be used apart...
>
> If this isn't a feature but really a bug, a simple way to clean this
> is to make struct UHCIInfo and usb_uhci_common_realize() public, and
> type_register "vt82c686b-usb-uhci" elsewhere.
>
> Gerd would that work with you?

Or is it too late, we can not make this device kconfig-selectable now?