| Message ID | bf9400cc8e4ddd3129aa5678de4d3cf38384805f.1610223397.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | vt82c686b clean ups and vt8231 emulation | expand |
Hi Zoltan, On 1/9/21 9:16 PM, BALATON Zoltan wrote: > Currently the ISA devices that are part of the VIA south bridge, > superio chip are wired up by board code. Move creation of these ISA > devices to the VIA ISA bridge model so that board code does not need > to access ISA bus. This also allows vt82c686b-superio to be made > internal to vt82c686 which allows implementing its configuration via > registers in subseqent commits. Is this patch dependent of the VT82C686B_PM changes or can it be applied before them? > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/isa/vt82c686.c | 20 ++++++++++++++++++++ > hw/mips/fuloong2e.c | 29 +++++------------------------ > 2 files changed, 25 insertions(+), 24 deletions(-) > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 58c0bba1d0..5df9be8ff4 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -16,6 +16,11 @@ > #include "hw/qdev-properties.h" > #include "hw/isa/isa.h" > #include "hw/isa/superio.h" > +#include "hw/intc/i8259.h" > +#include "hw/irq.h" > +#include "hw/dma/i8257.h" > +#include "hw/timer/i8254.h" > +#include "hw/rtc/mc146818rtc.h" > #include "migration/vmstate.h" > #include "hw/isa/apm.h" > #include "hw/acpi/acpi.h" > @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA) > > struct VT82C686BISAState { > PCIDevice dev; > + qemu_irq cpu_intr; > SuperIOConfig superio_cfg; > }; > > +static void via_isa_request_i8259_irq(void *opaque, int irq, int level) > +{ > + VT82C686BISAState *s = opaque; > + qemu_set_irq(s->cpu_intr, level); > +} > + > static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, > uint32_t val, int len) > { > @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp) > VT82C686BISAState *s = VT82C686B_ISA(d); > DeviceState *dev = DEVICE(d); > ISABus *isa_bus; > + qemu_irq *isa_irq; > int i; > > + qdev_init_gpio_out(dev, &s->cpu_intr, 1); Why not use the SysBus API? > + isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); > isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d), > &error_fatal); Isn't it get_system_memory() -> pci_address_space(d)? > + isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq)); > + i8254_pit_init(isa_bus, 0x40, 0, NULL); > + i8257_dma_init(isa_bus, 0); > + isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO); > + mc146818_rtc_init(isa_bus, 2000, NULL); > > for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) { > if (i < PCI_COMMAND || i >= PCI_REVISION_ID) { > diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c > index fbdd6122b3..0fc3288556 100644 > --- a/hw/mips/fuloong2e.c > +++ b/hw/mips/fuloong2e.c > @@ -25,9 +25,6 @@ > #include "qapi/error.h" > #include "cpu.h" > #include "hw/clock.h" > -#include "hw/intc/i8259.h" > -#include "hw/dma/i8257.h" > -#include "hw/isa/superio.h" > #include "net/net.h" > #include "hw/boards.h" > #include "hw/i2c/smbus_eeprom.h" > @@ -38,13 +35,13 @@ > #include "qemu/log.h" > #include "hw/loader.h" > #include "hw/ide/pci.h" > +#include "hw/qdev-properties.h" > #include "elf.h" > #include "hw/isa/vt82c686.h" > -#include "hw/rtc/mc146818rtc.h" > -#include "hw/timer/i8254.h" > #include "exec/address-spaces.h" > #include "sysemu/qtest.h" > #include "sysemu/reset.h" > +#include "sysemu/sysemu.h" > #include "qemu/error-report.h" > > #define ENVP_PADDR 0x2000 > @@ -224,26 +221,13 @@ static void main_cpu_reset(void *opaque) > } > > static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc, > - I2CBus **i2c_bus, ISABus **p_isa_bus) > + I2CBus **i2c_bus) > { > - qemu_irq *i8259; > - ISABus *isa_bus; > PCIDevice *dev; > > dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true, > TYPE_VT82C686B_ISA); > - isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0")); > - assert(isa_bus); > - *p_isa_bus = isa_bus; > - /* Interrupt controller */ > - /* The 8259 -> IP5 */ > - i8259 = i8259_init(isa_bus, intc); > - isa_bus_irqs(isa_bus, i8259); > - /* init other devices */ > - i8254_pit_init(isa_bus, 0x40, 0, NULL); > - i8257_dma_init(isa_bus, 0); > - /* Super I/O */ > - isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO); > + qdev_connect_gpio_out(DEVICE(dev), 0, intc); > > dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide"); > pci_ide_create_devs(dev); > @@ -290,7 +274,6 @@ static void mips_fuloong2e_init(MachineState *machine) > uint64_t kernel_entry; > PCIDevice *pci_dev; > PCIBus *pci_bus; > - ISABus *isa_bus; > I2CBus *smbus; > Clock *cpuclk; > MIPSCPU *cpu; > @@ -357,7 +340,7 @@ static void mips_fuloong2e_init(MachineState *machine) > > /* South bridge -> IP5 */ > vt82c686b_southbridge_init(pci_bus, FULOONG2E_VIA_SLOT, env->irq[5], > - &smbus, &isa_bus); > + &smbus); > > /* GPU */ > if (vga_interface_type != VGA_NONE) { > @@ -372,8 +355,6 @@ static void mips_fuloong2e_init(MachineState *machine) > spd_data = spd_data_generate(DDR, machine->ram_size); > smbus_eeprom_init_one(smbus, 0x50, spd_data); > > - mc146818_rtc_init(isa_bus, 2000, NULL); > - > /* Network card: RTL8139D */ > network_init(pci_bus); > } >
On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: > Hi Zoltan, > > On 1/9/21 9:16 PM, BALATON Zoltan wrote: >> Currently the ISA devices that are part of the VIA south bridge, >> superio chip are wired up by board code. Move creation of these ISA >> devices to the VIA ISA bridge model so that board code does not need >> to access ISA bus. This also allows vt82c686b-superio to be made >> internal to vt82c686 which allows implementing its configuration via >> registers in subseqent commits. > > Is this patch dependent of the VT82C686B_PM changes > or can it be applied before them? I don't know but why would that be better? I thought it's clearer to clean up pm related parts first before moving more stuff to this file so that's why this patch comes after (and also because that's the order I did it). >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/isa/vt82c686.c | 20 ++++++++++++++++++++ >> hw/mips/fuloong2e.c | 29 +++++------------------------ >> 2 files changed, 25 insertions(+), 24 deletions(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 58c0bba1d0..5df9be8ff4 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -16,6 +16,11 @@ >> #include "hw/qdev-properties.h" >> #include "hw/isa/isa.h" >> #include "hw/isa/superio.h" >> +#include "hw/intc/i8259.h" >> +#include "hw/irq.h" >> +#include "hw/dma/i8257.h" >> +#include "hw/timer/i8254.h" >> +#include "hw/rtc/mc146818rtc.h" >> #include "migration/vmstate.h" >> #include "hw/isa/apm.h" >> #include "hw/acpi/acpi.h" >> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA) >> >> struct VT82C686BISAState { >> PCIDevice dev; >> + qemu_irq cpu_intr; >> SuperIOConfig superio_cfg; >> }; >> >> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level) >> +{ >> + VT82C686BISAState *s = opaque; >> + qemu_set_irq(s->cpu_intr, level); >> +} >> + >> static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, >> uint32_t val, int len) >> { >> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp) >> VT82C686BISAState *s = VT82C686B_ISA(d); >> DeviceState *dev = DEVICE(d); >> ISABus *isa_bus; >> + qemu_irq *isa_irq; >> int i; >> >> + qdev_init_gpio_out(dev, &s->cpu_intr, 1); > > Why not use the SysBus API? How? This is a PCIDevice not a SysBusDevice. >> + isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >> isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d), >> &error_fatal); > > Isn't it get_system_memory() -> pci_address_space(d)? I don't really know. Most other places that create an isa bus seem to also use get_system_memory(), only piix4 uses pci_address_space(dev) so I thought if those others are OK this should be too. Regards, BALATON Zoltan >> + isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq)); >> + i8254_pit_init(isa_bus, 0x40, 0, NULL); >> + i8257_dma_init(isa_bus, 0); >> + isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO); >> + mc146818_rtc_init(isa_bus, 2000, NULL); >> >> for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) { >> if (i < PCI_COMMAND || i >= PCI_REVISION_ID) { >> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c >> index fbdd6122b3..0fc3288556 100644 >> --- a/hw/mips/fuloong2e.c >> +++ b/hw/mips/fuloong2e.c >> @@ -25,9 +25,6 @@ >> #include "qapi/error.h" >> #include "cpu.h" >> #include "hw/clock.h" >> -#include "hw/intc/i8259.h" >> -#include "hw/dma/i8257.h" >> -#include "hw/isa/superio.h" >> #include "net/net.h" >> #include "hw/boards.h" >> #include "hw/i2c/smbus_eeprom.h" >> @@ -38,13 +35,13 @@ >> #include "qemu/log.h" >> #include "hw/loader.h" >> #include "hw/ide/pci.h" >> +#include "hw/qdev-properties.h" >> #include "elf.h" >> #include "hw/isa/vt82c686.h" >> -#include "hw/rtc/mc146818rtc.h" >> -#include "hw/timer/i8254.h" >> #include "exec/address-spaces.h" >> #include "sysemu/qtest.h" >> #include "sysemu/reset.h" >> +#include "sysemu/sysemu.h" >> #include "qemu/error-report.h" >> >> #define ENVP_PADDR 0x2000 >> @@ -224,26 +221,13 @@ static void main_cpu_reset(void *opaque) >> } >> >> static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc, >> - I2CBus **i2c_bus, ISABus **p_isa_bus) >> + I2CBus **i2c_bus) >> { >> - qemu_irq *i8259; >> - ISABus *isa_bus; >> PCIDevice *dev; >> >> dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true, >> TYPE_VT82C686B_ISA); >> - isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0")); >> - assert(isa_bus); >> - *p_isa_bus = isa_bus; >> - /* Interrupt controller */ >> - /* The 8259 -> IP5 */ >> - i8259 = i8259_init(isa_bus, intc); >> - isa_bus_irqs(isa_bus, i8259); >> - /* init other devices */ >> - i8254_pit_init(isa_bus, 0x40, 0, NULL); >> - i8257_dma_init(isa_bus, 0); >> - /* Super I/O */ >> - isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO); >> + qdev_connect_gpio_out(DEVICE(dev), 0, intc); >> >> dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide"); >> pci_ide_create_devs(dev); >> @@ -290,7 +274,6 @@ static void mips_fuloong2e_init(MachineState *machine) >> uint64_t kernel_entry; >> PCIDevice *pci_dev; >> PCIBus *pci_bus; >> - ISABus *isa_bus; >> I2CBus *smbus; >> Clock *cpuclk; >> MIPSCPU *cpu; >> @@ -357,7 +340,7 @@ static void mips_fuloong2e_init(MachineState *machine) >> >> /* South bridge -> IP5 */ >> vt82c686b_southbridge_init(pci_bus, FULOONG2E_VIA_SLOT, env->irq[5], >> - &smbus, &isa_bus); >> + &smbus); >> >> /* GPU */ >> if (vga_interface_type != VGA_NONE) { >> @@ -372,8 +355,6 @@ static void mips_fuloong2e_init(MachineState *machine) >> spd_data = spd_data_generate(DDR, machine->ram_size); >> smbus_eeprom_init_one(smbus, 0x50, spd_data); >> >> - mc146818_rtc_init(isa_bus, 2000, NULL); >> - >> /* Network card: RTL8139D */ >> network_init(pci_bus); >> } >> > >
+PCI experts On 1/10/21 1:43 AM, BALATON Zoltan wrote: > On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >> Hi Zoltan, >> >> On 1/9/21 9:16 PM, BALATON Zoltan wrote: >>> Currently the ISA devices that are part of the VIA south bridge, >>> superio chip are wired up by board code. Move creation of these ISA >>> devices to the VIA ISA bridge model so that board code does not need >>> to access ISA bus. This also allows vt82c686b-superio to be made >>> internal to vt82c686 which allows implementing its configuration via >>> registers in subseqent commits. >> >> Is this patch dependent of the VT82C686B_PM changes >> or can it be applied before them? > > I don't know but why would that be better? I thought it's clearer to > clean up pm related parts first before moving more stuff to this file so > that's why this patch comes after (and also because that's the order I > did it). Not any better, but easier for me to get your patches integrated, as I'm reviewing your patches slowly. Finding other reviewers would certainly help. >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> hw/isa/vt82c686.c | 20 ++++++++++++++++++++ >>> hw/mips/fuloong2e.c | 29 +++++------------------------ >>> 2 files changed, 25 insertions(+), 24 deletions(-) >>> >>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>> index 58c0bba1d0..5df9be8ff4 100644 >>> --- a/hw/isa/vt82c686.c >>> +++ b/hw/isa/vt82c686.c >>> @@ -16,6 +16,11 @@ >>> #include "hw/qdev-properties.h" >>> #include "hw/isa/isa.h" >>> #include "hw/isa/superio.h" >>> +#include "hw/intc/i8259.h" >>> +#include "hw/irq.h" >>> +#include "hw/dma/i8257.h" >>> +#include "hw/timer/i8254.h" >>> +#include "hw/rtc/mc146818rtc.h" >>> #include "migration/vmstate.h" >>> #include "hw/isa/apm.h" >>> #include "hw/acpi/acpi.h" >>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, >>> VT82C686B_ISA) >>> >>> struct VT82C686BISAState { >>> PCIDevice dev; >>> + qemu_irq cpu_intr; >>> SuperIOConfig superio_cfg; >>> }; >>> >>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level) >>> +{ >>> + VT82C686BISAState *s = opaque; >>> + qemu_set_irq(s->cpu_intr, level); >>> +} >>> + >>> static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, >>> uint32_t val, int len) >>> { >>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, >>> Error **errp) >>> VT82C686BISAState *s = VT82C686B_ISA(d); >>> DeviceState *dev = DEVICE(d); >>> ISABus *isa_bus; >>> + qemu_irq *isa_irq; >>> int i; >>> >>> + qdev_init_gpio_out(dev, &s->cpu_intr, 1); >> >> Why not use the SysBus API? > > How? This is a PCIDevice not a SysBusDevice. Indeed :) >>> + isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >>> isa_bus = isa_bus_new(dev, get_system_memory(), >>> pci_address_space_io(d), >>> &error_fatal); >> >> Isn't it get_system_memory() -> pci_address_space(d)? > > I don't really know. Most other places that create an isa bus seem to > also use get_system_memory(), only piix4 uses pci_address_space(dev) so > I thought if those others are OK this should be too. I'm not a PCI expert but my understanding is PCI device functions are restricted to the PCI bus address space. The host bridge may map this space within the host. QEMU might be using get_system_memory() because for some host bridge the mapping is not implemented so it was easier this way? Regards, Phil.
On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: > +PCI experts > > On 1/10/21 1:43 AM, BALATON Zoltan wrote: >> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>> Hi Zoltan, >>> >>> On 1/9/21 9:16 PM, BALATON Zoltan wrote: >>>> Currently the ISA devices that are part of the VIA south bridge, >>>> superio chip are wired up by board code. Move creation of these ISA >>>> devices to the VIA ISA bridge model so that board code does not need >>>> to access ISA bus. This also allows vt82c686b-superio to be made >>>> internal to vt82c686 which allows implementing its configuration via >>>> registers in subseqent commits. >>> >>> Is this patch dependent of the VT82C686B_PM changes >>> or can it be applied before them? >> >> I don't know but why would that be better? I thought it's clearer to >> clean up pm related parts first before moving more stuff to this file so >> that's why this patch comes after (and also because that's the order I >> did it). > > Not any better, but easier for me to get your patches integrated, > as I'm reviewing your patches slowly. Finding other reviewers > would certainly help. No problem, I'll wait for your review. Merging parts of the series does not help much because the whole series is needed for vt8231 which is prerequisite for pegasos2 so eventually all of these are needed so it does not matter if this one patch gets in earlier or later. Not sure who could help with review. Maybe Jiaxun or Huacai as this is used by fuloong2e so they might be interested and could have info on this chip. Most of these patches just cleaning up the vt82c686b and adding some missing features so these can be reused by the vt8231 model in last 3 patches (which is very similar to 686b only some reg addresses and ids seem to be different for what we are concerned). >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> --- >>>> hw/isa/vt82c686.c | 20 ++++++++++++++++++++ >>>> hw/mips/fuloong2e.c | 29 +++++------------------------ >>>> 2 files changed, 25 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>> index 58c0bba1d0..5df9be8ff4 100644 >>>> --- a/hw/isa/vt82c686.c >>>> +++ b/hw/isa/vt82c686.c >>>> @@ -16,6 +16,11 @@ >>>> #include "hw/qdev-properties.h" >>>> #include "hw/isa/isa.h" >>>> #include "hw/isa/superio.h" >>>> +#include "hw/intc/i8259.h" >>>> +#include "hw/irq.h" >>>> +#include "hw/dma/i8257.h" >>>> +#include "hw/timer/i8254.h" >>>> +#include "hw/rtc/mc146818rtc.h" >>>> #include "migration/vmstate.h" >>>> #include "hw/isa/apm.h" >>>> #include "hw/acpi/acpi.h" >>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, >>>> VT82C686B_ISA) >>>> >>>> struct VT82C686BISAState { >>>> PCIDevice dev; >>>> + qemu_irq cpu_intr; >>>> SuperIOConfig superio_cfg; >>>> }; >>>> >>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level) >>>> +{ >>>> + VT82C686BISAState *s = opaque; >>>> + qemu_set_irq(s->cpu_intr, level); >>>> +} >>>> + >>>> static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, >>>> uint32_t val, int len) >>>> { >>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, >>>> Error **errp) >>>> VT82C686BISAState *s = VT82C686B_ISA(d); >>>> DeviceState *dev = DEVICE(d); >>>> ISABus *isa_bus; >>>> + qemu_irq *isa_irq; >>>> int i; >>>> >>>> + qdev_init_gpio_out(dev, &s->cpu_intr, 1); >>> >>> Why not use the SysBus API? >> >> How? This is a PCIDevice not a SysBusDevice. > > Indeed :) > >>>> + isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >>>> isa_bus = isa_bus_new(dev, get_system_memory(), >>>> pci_address_space_io(d), >>>> &error_fatal); >>> >>> Isn't it get_system_memory() -> pci_address_space(d)? >> >> I don't really know. Most other places that create an isa bus seem to >> also use get_system_memory(), only piix4 uses pci_address_space(dev) so >> I thought if those others are OK this should be too. > > I'm not a PCI expert but my understanding is PCI device functions are > restricted to the PCI bus address space. The host bridge may map this > space within the host. > > QEMU might be using get_system_memory() because for some host bridge > the mapping is not implemented so it was easier this way? Maybe, also one less indirection which if not really needed is a good thing for performance so unless it's found to be needed to use another address space here I'm happy with this as it matches what other similar devices do and it seems to work. Maybe a separate address space is only really needed if we have an iommu? Regards, BALATON Zoltan
On Mon, Jan 11, 2021, at 3:25 AM, BALATON Zoltan wrote: > On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: > > +PCI experts > > > > On 1/10/21 1:43 AM, BALATON Zoltan wrote: > >> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: [...] > > I'm not a PCI expert but my understanding is PCI device functions are > > restricted to the PCI bus address space. The host bridge may map this > > space within the host. > > > > QEMU might be using get_system_memory() because for some host bridge > > the mapping is not implemented so it was easier this way? > > Maybe, also one less indirection which if not really needed is a good > thing for performance so unless it's found to be needed to use another > address space here I'm happy with this as it matches what other similar > devices do and it seems to work. Maybe a separate address space is only > really needed if we have an iommu? Hi Zoltan, It is possible for bonito to remap PCI address space so maybe it's essential for bonito. Appreciate for your work. I'm going to help with reviewing as well. > > Regards, > BALATON Zoltan
On Mon, 11 Jan 2021, Jiaxun Yang wrote: > On Mon, Jan 11, 2021, at 3:25 AM, BALATON Zoltan wrote: >> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>> +PCI experts >>> >>> On 1/10/21 1:43 AM, BALATON Zoltan wrote: >>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: > > [...] > >>> I'm not a PCI expert but my understanding is PCI device functions are >>> restricted to the PCI bus address space. The host bridge may map this >>> space within the host. >>> >>> QEMU might be using get_system_memory() because for some host bridge >>> the mapping is not implemented so it was easier this way? >> >> Maybe, also one less indirection which if not really needed is a good >> thing for performance so unless it's found to be needed to use another >> address space here I'm happy with this as it matches what other similar >> devices do and it seems to work. Maybe a separate address space is only >> really needed if we have an iommu? > > Hi Zoltan, > > It is possible for bonito to remap PCI address space so maybe it's essential for bonito. I'm still not sure it's needed or how that would work. Maybe while it's possible to remap these, all guests just map these one-to-one (otherwise they may need to do some address translation which is much better avoided) so in practice we don't need to emulate this. If we use a different memory region maybe we also need some additional iommu code somewhere to connect the two but I'm not sure, I haven't tried since most other isa bridges do the same way using system_memory and this seems to work. I'm also a bit concerned about additional overhead which we could avoid if possible. Just to model something that's not really used I don't think it's worth the additional complexity and possible performance loss unless it's found to be needed to get some guests work. > Appreciate for your work. I'm going to help with reviewing as well. Thanks, I hope to get these in now before the freeze so testing and reviewing is really appreciated. Regards, BALATON Zoltan
On 1/11/21 11:28 AM, BALATON Zoltan wrote: > On Mon, 11 Jan 2021, Jiaxun Yang wrote: >> On Mon, Jan 11, 2021, at 3:25 AM, BALATON Zoltan wrote: >>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>>> +PCI experts >>>> >>>> On 1/10/21 1:43 AM, BALATON Zoltan wrote: >>>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >> >> [...] >> >>>> I'm not a PCI expert but my understanding is PCI device functions are >>>> restricted to the PCI bus address space. The host bridge may map this >>>> space within the host. >>>> >>>> QEMU might be using get_system_memory() because for some host bridge >>>> the mapping is not implemented so it was easier this way? >>> >>> Maybe, also one less indirection which if not really needed is a good >>> thing for performance so unless it's found to be needed to use another >>> address space here I'm happy with this as it matches what other similar >>> devices do and it seems to work. Maybe a separate address space is only >>> really needed if we have an iommu? >> >> Hi Zoltan, >> >> It is possible for bonito to remap PCI address space so maybe it's >> essential for bonito. > > I'm still not sure it's needed or how that would work. Maybe while it's > possible to remap these, all guests just map these one-to-one (otherwise > they may need to do some address translation which is much better > avoided) so in practice we don't need to emulate this. If we use a > different memory region maybe we also need some additional iommu code > somewhere to connect the two but I'm not sure, I haven't tried since > most other isa bridges do the same way using system_memory and this > seems to work. I'm also a bit concerned about additional overhead which > we could avoid if possible. Just to model something that's not really > used I don't think it's worth the additional complexity and possible > performance loss unless it's found to be needed to get some guests work. Fine. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On Sun, 10 Jan 2021, BALATON Zoltan wrote: > On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >> +PCI experts >> >> On 1/10/21 1:43 AM, BALATON Zoltan wrote: >>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>>> Hi Zoltan, >>>> >>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote: >>>>> Currently the ISA devices that are part of the VIA south bridge, >>>>> superio chip are wired up by board code. Move creation of these ISA >>>>> devices to the VIA ISA bridge model so that board code does not need >>>>> to access ISA bus. This also allows vt82c686b-superio to be made >>>>> internal to vt82c686 which allows implementing its configuration via >>>>> registers in subseqent commits. >>>> >>>> Is this patch dependent of the VT82C686B_PM changes >>>> or can it be applied before them? >>> >>> I don't know but why would that be better? I thought it's clearer to >>> clean up pm related parts first before moving more stuff to this file so >>> that's why this patch comes after (and also because that's the order I >>> did it). >> >> Not any better, but easier for me to get your patches integrated, >> as I'm reviewing your patches slowly. Finding other reviewers >> would certainly help. > > No problem, I'll wait for your review. Merging parts of the series does not > help much because the whole series is needed for vt8231 which is prerequisite > for pegasos2 so eventually all of these are needed so it does not matter if > this one patch gets in earlier or later. > > Not sure who could help with review. Maybe Jiaxun or Huacai as this is used > by fuloong2e so they might be interested and could have info on this chip. > Most of these patches just cleaning up the vt82c686b and adding some missing > features so these can be reused by the vt8231 model in last 3 patches (which > is very similar to 686b only some reg addresses and ids seem to be different > for what we are concerned). Ping? There are still a few patches needing review: http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512 Jiaxun, Hiacai, or anybody else could you please help reviewing or testing if this works with fuloong2e? Thank you, BALATON Zoltan >>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>> --- >>>>> hw/isa/vt82c686.c | 20 ++++++++++++++++++++ >>>>> hw/mips/fuloong2e.c | 29 +++++------------------------ >>>>> 2 files changed, 25 insertions(+), 24 deletions(-) >>>>> >>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>>> index 58c0bba1d0..5df9be8ff4 100644 >>>>> --- a/hw/isa/vt82c686.c >>>>> +++ b/hw/isa/vt82c686.c >>>>> @@ -16,6 +16,11 @@ >>>>> #include "hw/qdev-properties.h" >>>>> #include "hw/isa/isa.h" >>>>> #include "hw/isa/superio.h" >>>>> +#include "hw/intc/i8259.h" >>>>> +#include "hw/irq.h" >>>>> +#include "hw/dma/i8257.h" >>>>> +#include "hw/timer/i8254.h" >>>>> +#include "hw/rtc/mc146818rtc.h" >>>>> #include "migration/vmstate.h" >>>>> #include "hw/isa/apm.h" >>>>> #include "hw/acpi/acpi.h" >>>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, >>>>> VT82C686B_ISA) >>>>> >>>>> struct VT82C686BISAState { >>>>> PCIDevice dev; >>>>> + qemu_irq cpu_intr; >>>>> SuperIOConfig superio_cfg; >>>>> }; >>>>> >>>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level) >>>>> +{ >>>>> + VT82C686BISAState *s = opaque; >>>>> + qemu_set_irq(s->cpu_intr, level); >>>>> +} >>>>> + >>>>> static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, >>>>> uint32_t val, int len) >>>>> { >>>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, >>>>> Error **errp) >>>>> VT82C686BISAState *s = VT82C686B_ISA(d); >>>>> DeviceState *dev = DEVICE(d); >>>>> ISABus *isa_bus; >>>>> + qemu_irq *isa_irq; >>>>> int i; >>>>> >>>>> + qdev_init_gpio_out(dev, &s->cpu_intr, 1); >>>> >>>> Why not use the SysBus API? >>> >>> How? This is a PCIDevice not a SysBusDevice. >> >> Indeed :) >> >>>>> + isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >>>>> isa_bus = isa_bus_new(dev, get_system_memory(), >>>>> pci_address_space_io(d), >>>>> &error_fatal); >>>> >>>> Isn't it get_system_memory() -> pci_address_space(d)? >>> >>> I don't really know. Most other places that create an isa bus seem to >>> also use get_system_memory(), only piix4 uses pci_address_space(dev) so >>> I thought if those others are OK this should be too. >> >> I'm not a PCI expert but my understanding is PCI device functions are >> restricted to the PCI bus address space. The host bridge may map this >> space within the host. >> >> QEMU might be using get_system_memory() because for some host bridge >> the mapping is not implemented so it was easier this way? > > Maybe, also one less indirection which if not really needed is a good thing > for performance so unless it's found to be needed to use another address > space here I'm happy with this as it matches what other similar devices do > and it seems to work. Maybe a separate address space is only really needed if > we have an iommu? > > Regards, > BALATON Zoltan
在 2021/2/2 上午4:04, BALATON Zoltan 写道: > On Sun, 10 Jan 2021, BALATON Zoltan wrote: >> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>> +PCI experts >>> >>> On 1/10/21 1:43 AM, BALATON Zoltan wrote: >>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>>>> Hi Zoltan, >>>>> >>>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote: >>>>>> Currently the ISA devices that are part of the VIA south bridge, >>>>>> superio chip are wired up by board code. Move creation of these ISA >>>>>> devices to the VIA ISA bridge model so that board code does not need >>>>>> to access ISA bus. This also allows vt82c686b-superio to be made >>>>>> internal to vt82c686 which allows implementing its configuration via >>>>>> registers in subseqent commits. >>>>> >>>>> Is this patch dependent of the VT82C686B_PM changes >>>>> or can it be applied before them? >>>> >>>> I don't know but why would that be better? I thought it's clearer to >>>> clean up pm related parts first before moving more stuff to this >>>> file so >>>> that's why this patch comes after (and also because that's the order I >>>> did it). >>> >>> Not any better, but easier for me to get your patches integrated, >>> as I'm reviewing your patches slowly. Finding other reviewers >>> would certainly help. >> >> No problem, I'll wait for your review. Merging parts of the series >> does not help much because the whole series is needed for vt8231 >> which is prerequisite for pegasos2 so eventually all of these are >> needed so it does not matter if this one patch gets in earlier or later. >> >> Not sure who could help with review. Maybe Jiaxun or Huacai as this >> is used by fuloong2e so they might be interested and could have info >> on this chip. Most of these patches just cleaning up the vt82c686b >> and adding some missing features so these can be reused by the vt8231 >> model in last 3 patches (which is very similar to 686b only some reg >> addresses and ids seem to be different for what we are concerned). > > Ping? There are still a few patches needing review: > > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512 > > Jiaxun, Hiacai, or anybody else could you please help reviewing or > testing if this works with fuloong2e? Tested the series against Fuloong2E PMON. Fuloong's kernel doesn't have much to do with VIA ISA. Which patch is pending for test or review? Thanks. - Jiaxun > > Thank you, > BALATON Zoltan > >>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>>> --- >>>>>> hw/isa/vt82c686.c | 20 ++++++++++++++++++++ >>>>>> hw/mips/fuloong2e.c | 29 +++++------------------------ >>>>>> 2 files changed, 25 insertions(+), 24 deletions(-) >>>>>> >>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>>>> index 58c0bba1d0..5df9be8ff4 100644 >>>>>> --- a/hw/isa/vt82c686.c >>>>>> +++ b/hw/isa/vt82c686.c >>>>>> @@ -16,6 +16,11 @@ >>>>>> #include "hw/qdev-properties.h" >>>>>> #include "hw/isa/isa.h" >>>>>> #include "hw/isa/superio.h" >>>>>> +#include "hw/intc/i8259.h" >>>>>> +#include "hw/irq.h" >>>>>> +#include "hw/dma/i8257.h" >>>>>> +#include "hw/timer/i8254.h" >>>>>> +#include "hw/rtc/mc146818rtc.h" >>>>>> #include "migration/vmstate.h" >>>>>> #include "hw/isa/apm.h" >>>>>> #include "hw/acpi/acpi.h" >>>>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, >>>>>> VT82C686B_ISA) >>>>>> >>>>>> struct VT82C686BISAState { >>>>>> PCIDevice dev; >>>>>> + qemu_irq cpu_intr; >>>>>> SuperIOConfig superio_cfg; >>>>>> }; >>>>>> >>>>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int >>>>>> level) >>>>>> +{ >>>>>> + VT82C686BISAState *s = opaque; >>>>>> + qemu_set_irq(s->cpu_intr, level); >>>>>> +} >>>>>> + >>>>>> static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, >>>>>> uint32_t val, int len) >>>>>> { >>>>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, >>>>>> Error **errp) >>>>>> VT82C686BISAState *s = VT82C686B_ISA(d); >>>>>> DeviceState *dev = DEVICE(d); >>>>>> ISABus *isa_bus; >>>>>> + qemu_irq *isa_irq; >>>>>> int i; >>>>>> >>>>>> + qdev_init_gpio_out(dev, &s->cpu_intr, 1); >>>>> >>>>> Why not use the SysBus API? >>>> >>>> How? This is a PCIDevice not a SysBusDevice. >>> >>> Indeed :) >>> >>>>>> + isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >>>>>> isa_bus = isa_bus_new(dev, get_system_memory(), >>>>>> pci_address_space_io(d), >>>>>> &error_fatal); >>>>> >>>>> Isn't it get_system_memory() -> pci_address_space(d)? >>>> >>>> I don't really know. Most other places that create an isa bus seem to >>>> also use get_system_memory(), only piix4 uses >>>> pci_address_space(dev) so >>>> I thought if those others are OK this should be too. >>> >>> I'm not a PCI expert but my understanding is PCI device functions are >>> restricted to the PCI bus address space. The host bridge may map this >>> space within the host. >>> >>> QEMU might be using get_system_memory() because for some host bridge >>> the mapping is not implemented so it was easier this way? >> >> Maybe, also one less indirection which if not really needed is a good >> thing for performance so unless it's found to be needed to use >> another address space here I'm happy with this as it matches what >> other similar devices do and it seems to work. Maybe a separate >> address space is only really needed if we have an iommu? >> >> Regards, >> BALATON Zoltan
On Thu, 4 Feb 2021, Jiaxun Yang wrote: > 在 2021/2/2 上午4:04, BALATON Zoltan 写道: >> On Sun, 10 Jan 2021, BALATON Zoltan wrote: >>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>>> +PCI experts >>>> >>>> On 1/10/21 1:43 AM, BALATON Zoltan wrote: >>>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>>>>> Hi Zoltan, >>>>>> >>>>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote: >>>>>>> Currently the ISA devices that are part of the VIA south bridge, >>>>>>> superio chip are wired up by board code. Move creation of these ISA >>>>>>> devices to the VIA ISA bridge model so that board code does not need >>>>>>> to access ISA bus. This also allows vt82c686b-superio to be made >>>>>>> internal to vt82c686 which allows implementing its configuration via >>>>>>> registers in subseqent commits. >>>>>> >>>>>> Is this patch dependent of the VT82C686B_PM changes >>>>>> or can it be applied before them? >>>>> >>>>> I don't know but why would that be better? I thought it's clearer to >>>>> clean up pm related parts first before moving more stuff to this file so >>>>> that's why this patch comes after (and also because that's the order I >>>>> did it). >>>> >>>> Not any better, but easier for me to get your patches integrated, >>>> as I'm reviewing your patches slowly. Finding other reviewers >>>> would certainly help. >>> >>> No problem, I'll wait for your review. Merging parts of the series does >>> not help much because the whole series is needed for vt8231 which is >>> prerequisite for pegasos2 so eventually all of these are needed so it does >>> not matter if this one patch gets in earlier or later. >>> >>> Not sure who could help with review. Maybe Jiaxun or Huacai as this is >>> used by fuloong2e so they might be interested and could have info on this >>> chip. Most of these patches just cleaning up the vt82c686b and adding some >>> missing features so these can be reused by the vt8231 model in last 3 >>> patches (which is very similar to 686b only some reg addresses and ids >>> seem to be different for what we are concerned). >> >> Ping? There are still a few patches needing review: >> >> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512 >> >> Jiaxun, Hiacai, or anybody else could you please help reviewing or testing >> if this works with fuloong2e? > > Tested the series against Fuloong2E PMON. Fuloong's kernel doesn't have much > to do with > VIA ISA. > > Which patch is pending for test or review? Thank you. In the above patchwork link there are numbers next to patches showing the number of tags, those which don't have any need review, mostly the last few. Unfortunately patchwork seems to be down at the moment from here. Pathes that don't have review yet are: vt82c686: Fix up power management io base and config and those starting with vt82c686: Fix superio_cfg_{read,write}() functions and after that. The patchew link for the series is here: https://patchew.org/QEMU/cover.1610223396.git.balaton@eik.bme.hu/ Regards, BALATON Zoltan
On Mon, 1 Feb 2021, BALATON Zoltan wrote: > On Sun, 10 Jan 2021, BALATON Zoltan wrote: >> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>> +PCI experts >>> >>> On 1/10/21 1:43 AM, BALATON Zoltan wrote: >>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>>>> Hi Zoltan, >>>>> >>>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote: >>>>>> Currently the ISA devices that are part of the VIA south bridge, >>>>>> superio chip are wired up by board code. Move creation of these ISA >>>>>> devices to the VIA ISA bridge model so that board code does not need >>>>>> to access ISA bus. This also allows vt82c686b-superio to be made >>>>>> internal to vt82c686 which allows implementing its configuration via >>>>>> registers in subseqent commits. >>>>> >>>>> Is this patch dependent of the VT82C686B_PM changes >>>>> or can it be applied before them? >>>> >>>> I don't know but why would that be better? I thought it's clearer to >>>> clean up pm related parts first before moving more stuff to this file so >>>> that's why this patch comes after (and also because that's the order I >>>> did it). >>> >>> Not any better, but easier for me to get your patches integrated, >>> as I'm reviewing your patches slowly. Finding other reviewers >>> would certainly help. >> >> No problem, I'll wait for your review. Merging parts of the series does not >> help much because the whole series is needed for vt8231 which is >> prerequisite for pegasos2 so eventually all of these are needed so it does >> not matter if this one patch gets in earlier or later. >> >> Not sure who could help with review. Maybe Jiaxun or Huacai as this is used >> by fuloong2e so they might be interested and could have info on this chip. >> Most of these patches just cleaning up the vt82c686b and adding some >> missing features so these can be reused by the vt8231 model in last 3 >> patches (which is very similar to 686b only some reg addresses and ids seem >> to be different for what we are concerned). > > Ping? There are still a few patches needing review: > > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512 > > Jiaxun, Hiacai, or anybody else could you please help reviewing or testing if > this works with fuloong2e? Ping^2 > Thank you, > BALATON Zoltan > >>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>>> --- >>>>>> hw/isa/vt82c686.c | 20 ++++++++++++++++++++ >>>>>> hw/mips/fuloong2e.c | 29 +++++------------------------ >>>>>> 2 files changed, 25 insertions(+), 24 deletions(-) >>>>>> >>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>>>> index 58c0bba1d0..5df9be8ff4 100644 >>>>>> --- a/hw/isa/vt82c686.c >>>>>> +++ b/hw/isa/vt82c686.c >>>>>> @@ -16,6 +16,11 @@ >>>>>> #include "hw/qdev-properties.h" >>>>>> #include "hw/isa/isa.h" >>>>>> #include "hw/isa/superio.h" >>>>>> +#include "hw/intc/i8259.h" >>>>>> +#include "hw/irq.h" >>>>>> +#include "hw/dma/i8257.h" >>>>>> +#include "hw/timer/i8254.h" >>>>>> +#include "hw/rtc/mc146818rtc.h" >>>>>> #include "migration/vmstate.h" >>>>>> #include "hw/isa/apm.h" >>>>>> #include "hw/acpi/acpi.h" >>>>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, >>>>>> VT82C686B_ISA) >>>>>> >>>>>> struct VT82C686BISAState { >>>>>> PCIDevice dev; >>>>>> + qemu_irq cpu_intr; >>>>>> SuperIOConfig superio_cfg; >>>>>> }; >>>>>> >>>>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int >>>>>> level) >>>>>> +{ >>>>>> + VT82C686BISAState *s = opaque; >>>>>> + qemu_set_irq(s->cpu_intr, level); >>>>>> +} >>>>>> + >>>>>> static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, >>>>>> uint32_t val, int len) >>>>>> { >>>>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, >>>>>> Error **errp) >>>>>> VT82C686BISAState *s = VT82C686B_ISA(d); >>>>>> DeviceState *dev = DEVICE(d); >>>>>> ISABus *isa_bus; >>>>>> + qemu_irq *isa_irq; >>>>>> int i; >>>>>> >>>>>> + qdev_init_gpio_out(dev, &s->cpu_intr, 1); >>>>> >>>>> Why not use the SysBus API? >>>> >>>> How? This is a PCIDevice not a SysBusDevice. >>> >>> Indeed :) >>> >>>>>> + isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >>>>>> isa_bus = isa_bus_new(dev, get_system_memory(), >>>>>> pci_address_space_io(d), >>>>>> &error_fatal); >>>>> >>>>> Isn't it get_system_memory() -> pci_address_space(d)? >>>> >>>> I don't really know. Most other places that create an isa bus seem to >>>> also use get_system_memory(), only piix4 uses pci_address_space(dev) so >>>> I thought if those others are OK this should be too. >>> >>> I'm not a PCI expert but my understanding is PCI device functions are >>> restricted to the PCI bus address space. The host bridge may map this >>> space within the host. >>> >>> QEMU might be using get_system_memory() because for some host bridge >>> the mapping is not implemented so it was easier this way? >> >> Maybe, also one less indirection which if not really needed is a good thing >> for performance so unless it's found to be needed to use another address >> space here I'm happy with this as it matches what other similar devices do >> and it seems to work. Maybe a separate address space is only really needed >> if we have an iommu? >> >> Regards, >> BALATON Zoltan
On Tue, 9 Feb 2021, BALATON Zoltan wrote: > On Mon, 1 Feb 2021, BALATON Zoltan wrote: >> On Sun, 10 Jan 2021, BALATON Zoltan wrote: >>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>>> +PCI experts >>>> >>>> On 1/10/21 1:43 AM, BALATON Zoltan wrote: >>>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: >>>>>> Hi Zoltan, >>>>>> >>>>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote: >>>>>>> Currently the ISA devices that are part of the VIA south bridge, >>>>>>> superio chip are wired up by board code. Move creation of these ISA >>>>>>> devices to the VIA ISA bridge model so that board code does not need >>>>>>> to access ISA bus. This also allows vt82c686b-superio to be made >>>>>>> internal to vt82c686 which allows implementing its configuration via >>>>>>> registers in subseqent commits. >>>>>> >>>>>> Is this patch dependent of the VT82C686B_PM changes >>>>>> or can it be applied before them? >>>>> >>>>> I don't know but why would that be better? I thought it's clearer to >>>>> clean up pm related parts first before moving more stuff to this file so >>>>> that's why this patch comes after (and also because that's the order I >>>>> did it). >>>> >>>> Not any better, but easier for me to get your patches integrated, >>>> as I'm reviewing your patches slowly. Finding other reviewers >>>> would certainly help. >>> >>> No problem, I'll wait for your review. Merging parts of the series does >>> not help much because the whole series is needed for vt8231 which is >>> prerequisite for pegasos2 so eventually all of these are needed so it does >>> not matter if this one patch gets in earlier or later. >>> >>> Not sure who could help with review. Maybe Jiaxun or Huacai as this is >>> used by fuloong2e so they might be interested and could have info on this >>> chip. Most of these patches just cleaning up the vt82c686b and adding some >>> missing features so these can be reused by the vt8231 model in last 3 >>> patches (which is very similar to 686b only some reg addresses and ids >>> seem to be different for what we are concerned). >> >> Ping? There are still a few patches needing review: >> >> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512 >> >> Jiaxun, Hiacai, or anybody else could you please help reviewing or testing >> if this works with fuloong2e? > > Ping^2 Ping^3 >> Thank you, >> BALATON Zoltan >> >>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>>>> --- >>>>>>> hw/isa/vt82c686.c | 20 ++++++++++++++++++++ >>>>>>> hw/mips/fuloong2e.c | 29 +++++------------------------ >>>>>>> 2 files changed, 25 insertions(+), 24 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>>>>> index 58c0bba1d0..5df9be8ff4 100644 >>>>>>> --- a/hw/isa/vt82c686.c >>>>>>> +++ b/hw/isa/vt82c686.c >>>>>>> @@ -16,6 +16,11 @@ >>>>>>> #include "hw/qdev-properties.h" >>>>>>> #include "hw/isa/isa.h" >>>>>>> #include "hw/isa/superio.h" >>>>>>> +#include "hw/intc/i8259.h" >>>>>>> +#include "hw/irq.h" >>>>>>> +#include "hw/dma/i8257.h" >>>>>>> +#include "hw/timer/i8254.h" >>>>>>> +#include "hw/rtc/mc146818rtc.h" >>>>>>> #include "migration/vmstate.h" >>>>>>> #include "hw/isa/apm.h" >>>>>>> #include "hw/acpi/acpi.h" >>>>>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, >>>>>>> VT82C686B_ISA) >>>>>>> >>>>>>> struct VT82C686BISAState { >>>>>>> PCIDevice dev; >>>>>>> + qemu_irq cpu_intr; >>>>>>> SuperIOConfig superio_cfg; >>>>>>> }; >>>>>>> >>>>>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int >>>>>>> level) >>>>>>> +{ >>>>>>> + VT82C686BISAState *s = opaque; >>>>>>> + qemu_set_irq(s->cpu_intr, level); >>>>>>> +} >>>>>>> + >>>>>>> static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, >>>>>>> uint32_t val, int len) >>>>>>> { >>>>>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, >>>>>>> Error **errp) >>>>>>> VT82C686BISAState *s = VT82C686B_ISA(d); >>>>>>> DeviceState *dev = DEVICE(d); >>>>>>> ISABus *isa_bus; >>>>>>> + qemu_irq *isa_irq; >>>>>>> int i; >>>>>>> >>>>>>> + qdev_init_gpio_out(dev, &s->cpu_intr, 1); >>>>>> >>>>>> Why not use the SysBus API? >>>>> >>>>> How? This is a PCIDevice not a SysBusDevice. >>>> >>>> Indeed :) >>>> >>>>>>> + isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >>>>>>> isa_bus = isa_bus_new(dev, get_system_memory(), >>>>>>> pci_address_space_io(d), >>>>>>> &error_fatal); >>>>>> >>>>>> Isn't it get_system_memory() -> pci_address_space(d)? >>>>> >>>>> I don't really know. Most other places that create an isa bus seem to >>>>> also use get_system_memory(), only piix4 uses pci_address_space(dev) so >>>>> I thought if those others are OK this should be too. >>>> >>>> I'm not a PCI expert but my understanding is PCI device functions are >>>> restricted to the PCI bus address space. The host bridge may map this >>>> space within the host. >>>> >>>> QEMU might be using get_system_memory() because for some host bridge >>>> the mapping is not implemented so it was easier this way? >>> >>> Maybe, also one less indirection which if not really needed is a good >>> thing for performance so unless it's found to be needed to use another >>> address space here I'm happy with this as it matches what other similar >>> devices do and it seems to work. Maybe a separate address space is only >>> really needed if we have an iommu? >>> >>> Regards, >>> BALATON Zoltan
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 58c0bba1d0..5df9be8ff4 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -16,6 +16,11 @@ #include "hw/qdev-properties.h" #include "hw/isa/isa.h" #include "hw/isa/superio.h" +#include "hw/intc/i8259.h" +#include "hw/irq.h" +#include "hw/dma/i8257.h" +#include "hw/timer/i8254.h" +#include "hw/rtc/mc146818rtc.h" #include "migration/vmstate.h" #include "hw/isa/apm.h" #include "hw/acpi/acpi.h" @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA) struct VT82C686BISAState { PCIDevice dev; + qemu_irq cpu_intr; SuperIOConfig superio_cfg; }; +static void via_isa_request_i8259_irq(void *opaque, int irq, int level) +{ + VT82C686BISAState *s = opaque; + qemu_set_irq(s->cpu_intr, level); +} + static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len) { @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp) VT82C686BISAState *s = VT82C686B_ISA(d); DeviceState *dev = DEVICE(d); ISABus *isa_bus; + qemu_irq *isa_irq; int i; + qdev_init_gpio_out(dev, &s->cpu_intr, 1); + isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d), &error_fatal); + isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq)); + i8254_pit_init(isa_bus, 0x40, 0, NULL); + i8257_dma_init(isa_bus, 0); + isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO); + mc146818_rtc_init(isa_bus, 2000, NULL); for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) { if (i < PCI_COMMAND || i >= PCI_REVISION_ID) { diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c index fbdd6122b3..0fc3288556 100644 --- a/hw/mips/fuloong2e.c +++ b/hw/mips/fuloong2e.c @@ -25,9 +25,6 @@ #include "qapi/error.h" #include "cpu.h" #include "hw/clock.h" -#include "hw/intc/i8259.h" -#include "hw/dma/i8257.h" -#include "hw/isa/superio.h" #include "net/net.h" #include "hw/boards.h" #include "hw/i2c/smbus_eeprom.h" @@ -38,13 +35,13 @@ #include "qemu/log.h" #include "hw/loader.h" #include "hw/ide/pci.h" +#include "hw/qdev-properties.h" #include "elf.h" #include "hw/isa/vt82c686.h" -#include "hw/rtc/mc146818rtc.h" -#include "hw/timer/i8254.h" #include "exec/address-spaces.h" #include "sysemu/qtest.h" #include "sysemu/reset.h" +#include "sysemu/sysemu.h" #include "qemu/error-report.h" #define ENVP_PADDR 0x2000 @@ -224,26 +221,13 @@ static void main_cpu_reset(void *opaque) } static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc, - I2CBus **i2c_bus, ISABus **p_isa_bus) + I2CBus **i2c_bus) { - qemu_irq *i8259; - ISABus *isa_bus; PCIDevice *dev; dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true, TYPE_VT82C686B_ISA); - isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0")); - assert(isa_bus); - *p_isa_bus = isa_bus; - /* Interrupt controller */ - /* The 8259 -> IP5 */ - i8259 = i8259_init(isa_bus, intc); - isa_bus_irqs(isa_bus, i8259); - /* init other devices */ - i8254_pit_init(isa_bus, 0x40, 0, NULL); - i8257_dma_init(isa_bus, 0); - /* Super I/O */ - isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO); + qdev_connect_gpio_out(DEVICE(dev), 0, intc); dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide"); pci_ide_create_devs(dev); @@ -290,7 +274,6 @@ static void mips_fuloong2e_init(MachineState *machine) uint64_t kernel_entry; PCIDevice *pci_dev; PCIBus *pci_bus; - ISABus *isa_bus; I2CBus *smbus; Clock *cpuclk; MIPSCPU *cpu; @@ -357,7 +340,7 @@ static void mips_fuloong2e_init(MachineState *machine) /* South bridge -> IP5 */ vt82c686b_southbridge_init(pci_bus, FULOONG2E_VIA_SLOT, env->irq[5], - &smbus, &isa_bus); + &smbus); /* GPU */ if (vga_interface_type != VGA_NONE) { @@ -372,8 +355,6 @@ static void mips_fuloong2e_init(MachineState *machine) spd_data = spd_data_generate(DDR, machine->ram_size); smbus_eeprom_init_one(smbus, 0x50, spd_data); - mc146818_rtc_init(isa_bus, 2000, NULL); - /* Network card: RTL8139D */ network_init(pci_bus); }
Currently the ISA devices that are part of the VIA south bridge, superio chip are wired up by board code. Move creation of these ISA devices to the VIA ISA bridge model so that board code does not need to access ISA bus. This also allows vt82c686b-superio to be made internal to vt82c686 which allows implementing its configuration via registers in subseqent commits. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/isa/vt82c686.c | 20 ++++++++++++++++++++ hw/mips/fuloong2e.c | 29 +++++------------------------ 2 files changed, 25 insertions(+), 24 deletions(-)