| Message ID | 07df96112b78673ca191f9a4ffa17bf3a11160f3.1614719482.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Pegasos2 emulation | expand |
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. Otherwise the rest LGTM. > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_PCI_DEVICE(dev, ViaISAState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const TypeInfo via_isa_info = { > + .name = TYPE_VIA_ISA, > + .parent = TYPE_PCI_DEVICE, > + .instance_size = sizeof(ViaISAState), > + .abstract = true, > + .interfaces = (InterfaceInfo[]) { > + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, > + { }, > + }, > +}; > + > static void via_isa_request_i8259_irq(void *opaque, int irq, int level) > { > - VT82C686BISAState *s = opaque; > + ViaISAState *s = opaque; > qemu_set_irq(s->cpu_intr, level); > } > > +/* TYPE_VT82C686B_ISA */ > + > static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, > uint32_t val, int len) > { > - VT82C686BISAState *s = VT82C686B_ISA(d); > + ViaISAState *s = VIA_ISA(d); > > trace_via_isa_write(addr, val, len); > pci_default_write_config(d, addr, val, len); > @@ -636,19 +660,9 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, > } > } > > -static const VMStateDescription vmstate_via = { > - .name = "vt82c686b", > - .version_id = 1, > - .minimum_version_id = 1, > - .fields = (VMStateField[]) { > - VMSTATE_PCI_DEVICE(dev, VT82C686BISAState), > - VMSTATE_END_OF_LIST() > - } > -}; > - > static void vt82c686b_isa_reset(DeviceState *dev) > { > - VT82C686BISAState *s = VT82C686B_ISA(dev); > + ViaISAState *s = VIA_ISA(dev); > uint8_t *pci_conf = s->dev.config; > > pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0); > @@ -668,7 +682,7 @@ static void vt82c686b_isa_reset(DeviceState *dev) > > static void vt82c686b_realize(PCIDevice *d, Error **errp) > { > - VT82C686BISAState *s = VT82C686B_ISA(d); > + ViaISAState *s = VIA_ISA(d); > DeviceState *dev = DEVICE(d); > ISABus *isa_bus; > qemu_irq *isa_irq; > @@ -692,7 +706,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp) > } > } > > -static void via_class_init(ObjectClass *klass, void *data) > +static void vt82c686b_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > @@ -700,28 +714,21 @@ static void via_class_init(ObjectClass *klass, void *data) > k->realize = vt82c686b_realize; > k->config_write = vt82c686b_write_config; > k->vendor_id = PCI_VENDOR_ID_VIA; > - k->device_id = PCI_DEVICE_ID_VIA_ISA_BRIDGE; > + k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA; > k->class_id = PCI_CLASS_BRIDGE_ISA; > k->revision = 0x40; > dc->reset = vt82c686b_isa_reset; > dc->desc = "ISA bridge"; > dc->vmsd = &vmstate_via; > - /* > - * Reason: part of VIA VT82C686 southbridge, needs to be wired up, > - * e.g. by mips_fuloong2e_init() > - */ > + /* Reason: part of VIA VT82C686 southbridge, needs to be wired up */ > dc->user_creatable = false; > } > > -static const TypeInfo via_info = { > +static const TypeInfo vt82c686b_isa_info = { > .name = TYPE_VT82C686B_ISA, > - .parent = TYPE_PCI_DEVICE, > - .instance_size = sizeof(VT82C686BISAState), > - .class_init = via_class_init, > - .interfaces = (InterfaceInfo[]) { > - { INTERFACE_CONVENTIONAL_PCI_DEVICE }, > - { }, > - }, > + .parent = TYPE_VIA_ISA, > + .instance_size = sizeof(ViaISAState), > + .class_init = vt82c686b_class_init, > }; > > > @@ -733,7 +740,8 @@ static void vt82c686b_register_types(void) > type_register_static(&via_superio_info); > type_register_static(&vt82c686b_superio_info); > type_register_static(&vt8231_superio_info); > - type_register_static(&via_info); > + type_register_static(&via_isa_info); > + type_register_static(&vt82c686b_isa_info); > } > > type_init(vt82c686b_register_types) > diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h > index ea28dcc850..aa3f67eaa4 100644 > --- a/include/hw/pci/pci_ids.h > +++ b/include/hw/pci/pci_ids.h > @@ -204,7 +204,7 @@ > #define PCI_VENDOR_ID_XILINX 0x10ee > > #define PCI_VENDOR_ID_VIA 0x1106 > -#define PCI_DEVICE_ID_VIA_ISA_BRIDGE 0x0686 > +#define PCI_DEVICE_ID_VIA_82C686B_ISA 0x0686 > #define PCI_DEVICE_ID_VIA_IDE 0x0571 > #define PCI_DEVICE_ID_VIA_UHCI 0x3038 > #define PCI_DEVICE_ID_VIA_82C686B_PM 0x3057 >
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. Regards, BALATON Zoltan > Otherwise the rest LGTM. > >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_PCI_DEVICE(dev, ViaISAState), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> +static const TypeInfo via_isa_info = { >> + .name = TYPE_VIA_ISA, >> + .parent = TYPE_PCI_DEVICE, >> + .instance_size = sizeof(ViaISAState), >> + .abstract = true, >> + .interfaces = (InterfaceInfo[]) { >> + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, >> + { }, >> + }, >> +}; >> + >> static void via_isa_request_i8259_irq(void *opaque, int irq, int level) >> { >> - VT82C686BISAState *s = opaque; >> + ViaISAState *s = opaque; >> qemu_set_irq(s->cpu_intr, level); >> } >> >> +/* TYPE_VT82C686B_ISA */ >> + >> static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, >> uint32_t val, int len) >> { >> - VT82C686BISAState *s = VT82C686B_ISA(d); >> + ViaISAState *s = VIA_ISA(d); >> >> trace_via_isa_write(addr, val, len); >> pci_default_write_config(d, addr, val, len); >> @@ -636,19 +660,9 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, >> } >> } >> >> -static const VMStateDescription vmstate_via = { >> - .name = "vt82c686b", >> - .version_id = 1, >> - .minimum_version_id = 1, >> - .fields = (VMStateField[]) { >> - VMSTATE_PCI_DEVICE(dev, VT82C686BISAState), >> - VMSTATE_END_OF_LIST() >> - } >> -}; >> - >> static void vt82c686b_isa_reset(DeviceState *dev) >> { >> - VT82C686BISAState *s = VT82C686B_ISA(dev); >> + ViaISAState *s = VIA_ISA(dev); >> uint8_t *pci_conf = s->dev.config; >> >> pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0); >> @@ -668,7 +682,7 @@ static void vt82c686b_isa_reset(DeviceState *dev) >> >> static void vt82c686b_realize(PCIDevice *d, Error **errp) >> { >> - VT82C686BISAState *s = VT82C686B_ISA(d); >> + ViaISAState *s = VIA_ISA(d); >> DeviceState *dev = DEVICE(d); >> ISABus *isa_bus; >> qemu_irq *isa_irq; >> @@ -692,7 +706,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp) >> } >> } >> >> -static void via_class_init(ObjectClass *klass, void *data) >> +static void vt82c686b_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> @@ -700,28 +714,21 @@ static void via_class_init(ObjectClass *klass, void *data) >> k->realize = vt82c686b_realize; >> k->config_write = vt82c686b_write_config; >> k->vendor_id = PCI_VENDOR_ID_VIA; >> - k->device_id = PCI_DEVICE_ID_VIA_ISA_BRIDGE; >> + k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA; >> k->class_id = PCI_CLASS_BRIDGE_ISA; >> k->revision = 0x40; >> dc->reset = vt82c686b_isa_reset; >> dc->desc = "ISA bridge"; >> dc->vmsd = &vmstate_via; >> - /* >> - * Reason: part of VIA VT82C686 southbridge, needs to be wired up, >> - * e.g. by mips_fuloong2e_init() >> - */ >> + /* Reason: part of VIA VT82C686 southbridge, needs to be wired up */ >> dc->user_creatable = false; >> } >> >> -static const TypeInfo via_info = { >> +static const TypeInfo vt82c686b_isa_info = { >> .name = TYPE_VT82C686B_ISA, >> - .parent = TYPE_PCI_DEVICE, >> - .instance_size = sizeof(VT82C686BISAState), >> - .class_init = via_class_init, >> - .interfaces = (InterfaceInfo[]) { >> - { INTERFACE_CONVENTIONAL_PCI_DEVICE }, >> - { }, >> - }, >> + .parent = TYPE_VIA_ISA, >> + .instance_size = sizeof(ViaISAState), >> + .class_init = vt82c686b_class_init, >> }; >> >> >> @@ -733,7 +740,8 @@ static void vt82c686b_register_types(void) >> type_register_static(&via_superio_info); >> type_register_static(&vt82c686b_superio_info); >> type_register_static(&vt8231_superio_info); >> - type_register_static(&via_info); >> + type_register_static(&via_isa_info); >> + type_register_static(&vt82c686b_isa_info); >> } >> >> type_init(vt82c686b_register_types) >> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h >> index ea28dcc850..aa3f67eaa4 100644 >> --- a/include/hw/pci/pci_ids.h >> +++ b/include/hw/pci/pci_ids.h >> @@ -204,7 +204,7 @@ >> #define PCI_VENDOR_ID_XILINX 0x10ee >> >> #define PCI_VENDOR_ID_VIA 0x1106 >> -#define PCI_DEVICE_ID_VIA_ISA_BRIDGE 0x0686 >> +#define PCI_DEVICE_ID_VIA_82C686B_ISA 0x0686 >> #define PCI_DEVICE_ID_VIA_IDE 0x0571 >> #define PCI_DEVICE_ID_VIA_UHCI 0x3038 >> #define PCI_DEVICE_ID_VIA_82C686B_PM 0x3057 >> > >
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. ?
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.
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? :)
* 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. Dave > -- > 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
On Tue, 9 Mar 2021, 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. This is the isa bridge part of vt82c686b which should not be used anywhere else than fuloong2e and should not change the usb part. That the usb-uhci part of this chip appears in the PCI device list may be a bug as that's not much useful without the rest of the superio chip but fixing that is beyond this patch I think. There's no reason one would want to use vt82c686-usb device on a machine without the vt82c686 chip instead or another uhci device matching the hardware of that machine. I've updated the commit message as Philippe suggested. Regards, BALATON Zoltan > Dave > >> -- >> 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 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > >
+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?
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?
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.
> >> 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... Strictly speaking this is true for all other uhci devices too, they are piix / q35 specific. > 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? Fine with me. But given that all usb_uhci_vt82c686b_realize() does is tweak pci config space a bit we can IMHO also live with just fine the current state. Your choice. take care, Gerd
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", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_PCI_DEVICE(dev, ViaISAState), + VMSTATE_END_OF_LIST() + } +}; + +static const TypeInfo via_isa_info = { + .name = TYPE_VIA_ISA, + .parent = TYPE_PCI_DEVICE, + .instance_size = sizeof(ViaISAState), + .abstract = true, + .interfaces = (InterfaceInfo[]) { + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, + { }, + }, +}; + static void via_isa_request_i8259_irq(void *opaque, int irq, int level) { - VT82C686BISAState *s = opaque; + ViaISAState *s = opaque; qemu_set_irq(s->cpu_intr, level); } +/* TYPE_VT82C686B_ISA */ + static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len) { - VT82C686BISAState *s = VT82C686B_ISA(d); + ViaISAState *s = VIA_ISA(d); trace_via_isa_write(addr, val, len); pci_default_write_config(d, addr, val, len); @@ -636,19 +660,9 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, } } -static const VMStateDescription vmstate_via = { - .name = "vt82c686b", - .version_id = 1, - .minimum_version_id = 1, - .fields = (VMStateField[]) { - VMSTATE_PCI_DEVICE(dev, VT82C686BISAState), - VMSTATE_END_OF_LIST() - } -}; - static void vt82c686b_isa_reset(DeviceState *dev) { - VT82C686BISAState *s = VT82C686B_ISA(dev); + ViaISAState *s = VIA_ISA(dev); uint8_t *pci_conf = s->dev.config; pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0); @@ -668,7 +682,7 @@ static void vt82c686b_isa_reset(DeviceState *dev) static void vt82c686b_realize(PCIDevice *d, Error **errp) { - VT82C686BISAState *s = VT82C686B_ISA(d); + ViaISAState *s = VIA_ISA(d); DeviceState *dev = DEVICE(d); ISABus *isa_bus; qemu_irq *isa_irq; @@ -692,7 +706,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp) } } -static void via_class_init(ObjectClass *klass, void *data) +static void vt82c686b_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); @@ -700,28 +714,21 @@ static void via_class_init(ObjectClass *klass, void *data) k->realize = vt82c686b_realize; k->config_write = vt82c686b_write_config; k->vendor_id = PCI_VENDOR_ID_VIA; - k->device_id = PCI_DEVICE_ID_VIA_ISA_BRIDGE; + k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA; k->class_id = PCI_CLASS_BRIDGE_ISA; k->revision = 0x40; dc->reset = vt82c686b_isa_reset; dc->desc = "ISA bridge"; dc->vmsd = &vmstate_via; - /* - * Reason: part of VIA VT82C686 southbridge, needs to be wired up, - * e.g. by mips_fuloong2e_init() - */ + /* Reason: part of VIA VT82C686 southbridge, needs to be wired up */ dc->user_creatable = false; } -static const TypeInfo via_info = { +static const TypeInfo vt82c686b_isa_info = { .name = TYPE_VT82C686B_ISA, - .parent = TYPE_PCI_DEVICE, - .instance_size = sizeof(VT82C686BISAState), - .class_init = via_class_init, - .interfaces = (InterfaceInfo[]) { - { INTERFACE_CONVENTIONAL_PCI_DEVICE }, - { }, - }, + .parent = TYPE_VIA_ISA, + .instance_size = sizeof(ViaISAState), + .class_init = vt82c686b_class_init, }; @@ -733,7 +740,8 @@ static void vt82c686b_register_types(void) type_register_static(&via_superio_info); type_register_static(&vt82c686b_superio_info); type_register_static(&vt8231_superio_info); - type_register_static(&via_info); + type_register_static(&via_isa_info); + type_register_static(&vt82c686b_isa_info); } type_init(vt82c686b_register_types) diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h index ea28dcc850..aa3f67eaa4 100644 --- a/include/hw/pci/pci_ids.h +++ b/include/hw/pci/pci_ids.h @@ -204,7 +204,7 @@ #define PCI_VENDOR_ID_XILINX 0x10ee #define PCI_VENDOR_ID_VIA 0x1106 -#define PCI_DEVICE_ID_VIA_ISA_BRIDGE 0x0686 +#define PCI_DEVICE_ID_VIA_82C686B_ISA 0x0686 #define PCI_DEVICE_ID_VIA_IDE 0x0571 #define PCI_DEVICE_ID_VIA_UHCI 0x3038 #define PCI_DEVICE_ID_VIA_82C686B_PM 0x3057
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(-)