Message ID | 1357681452-24963-1-git-send-email-lersek@redhat.com |
---|---|
State | New |
Headers | show |
On Tue, Jan 8, 2013 at 9:44 PM, Laszlo Ersek <lersek@redhat.com> wrote: > From <http://mjg59.dreamwidth.org/3561.html>: > > Traditional PCI config space access is achieved by writing a 32 bit > value to io port 0xcf8 to identify the bus, device, function and config > register. Port 0xcfc then contains the register in question. But if you > write the appropriate pair of magic values to 0xcf9, the machine will > reboot. Spectacular! And not standardised in any way (certainly not part > of the PCI spec), so different chipsets may have different requirements. > Booo. > > In the PIIX4 spec, IO port 0xcf9 is specified as the Reset Control > Register. Therefore let's handle single byte writes to offset 1 in the > [0xcf8, 0xcfb] range separately, and interpret the RCPU bit in the value. > (Based on "docs/memory.txt", overlapping regions seem to serve a different > purpose.) > > The SRST bit alone could be stateful. However we don't distinguish between > soft & hard reset, hence the SRST bit is neither checked nor saved. > > RHBZ reference: 890459 > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > hw/piix_pci.c | 22 +++++++++++++++++++++- > 1 files changed, 21 insertions(+), 1 deletions(-) > > diff --git a/hw/piix_pci.c b/hw/piix_pci.c > index 3d79c73..89d694c 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -31,6 +31,7 @@ > #include "qemu/range.h" > #include "xen.h" > #include "pam.h" > +#include "sysemu/sysemu.h" > > /* > * I440FX chipset data sheet. > @@ -180,11 +181,30 @@ static const VMStateDescription vmstate_i440fx = { > } > }; > > +static void i440fx_host_config_write(void *opaque, hwaddr addr, > + uint64_t val, unsigned len) > +{ > + if (addr == 1 && len == 1) { > + if (val & 4) { > + qemu_system_reset_request(); > + } > + return; > + } > + pci_host_conf_le_ops.write(opaque, addr, val, len); > +} > + > +static MemoryRegionOps i440fx_host_conf_ops = { > + .read = NULL, > + .write = i440fx_host_config_write, > + .endianness = DEVICE_LITTLE_ENDIAN > +}; > + > static int i440fx_pcihost_initfn(SysBusDevice *dev) > { > PCIHostState *s = PCI_HOST_BRIDGE(dev); > > - memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s, > + i440fx_host_conf_ops.read = pci_host_conf_le_ops.read; It would be cleaner to introduce a new memory region (without this copying) which passes 0xcf8 and 0xcfc to standard PCI host but catches accesses to 0xcf9. This may mean that pci_host_config_{read,write} will need to be exposed. > + memory_region_init_io(&s->conf_mem, &i440fx_host_conf_ops, s, > "pci-conf-idx", 4); > sysbus_add_io(dev, 0xcf8, &s->conf_mem); > sysbus_init_ioports(&s->busdev, 0xcf8, 4); > -- > 1.7.1 > >
Hi, On 01/09/13 22:01, Blue Swirl wrote: > On Tue, Jan 8, 2013 at 9:44 PM, Laszlo Ersek <lersek@redhat.com> wrote: >> +static void i440fx_host_config_write(void *opaque, hwaddr addr, >> + uint64_t val, unsigned len) >> +{ >> + if (addr == 1 && len == 1) { >> + if (val & 4) { >> + qemu_system_reset_request(); >> + } >> + return; >> + } >> + pci_host_conf_le_ops.write(opaque, addr, val, len); >> +} >> + >> +static MemoryRegionOps i440fx_host_conf_ops = { >> + .read = NULL, >> + .write = i440fx_host_config_write, >> + .endianness = DEVICE_LITTLE_ENDIAN >> +}; >> + >> static int i440fx_pcihost_initfn(SysBusDevice *dev) >> { >> PCIHostState *s = PCI_HOST_BRIDGE(dev); >> >> - memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s, >> + i440fx_host_conf_ops.read = pci_host_conf_le_ops.read; > > It would be cleaner to introduce a new memory region (without this > copying) which passes 0xcf8 and 0xcfc to standard PCI host but catches > accesses to 0xcf9. This may mean that pci_host_config_{read,write} > will need to be exposed. Do you mean: (1) introducing the new "i440fx_host_conf_ops" struct-of-funcptrs with detached functions (that is, duplicating the guts of pci_host_config_{read,write} and modifying them, and then registering s->conf_mem with this "i440fx_host_conf_ops"; or (2) leaving s->conf_mem as-is, and introducing a sub-region just for port 0xcf9, with higher visibility priority? (I don't feel confident about (2), and based on "docs/memory.txt" I thought that overlapping regions had not been invented for this purpose.) IOW, are you OK with the explicit offset + access-width based check, just organized differently, or are you proposing a one-byte-wide subregion? Thanks! Laszlo
Hi, Am 11.01.2013 10:30, schrieb Laszlo Ersek: > On 01/09/13 22:01, Blue Swirl wrote: >> On Tue, Jan 8, 2013 at 9:44 PM, Laszlo Ersek <lersek@redhat.com> wrote: > >>> +static void i440fx_host_config_write(void *opaque, hwaddr addr, >>> + uint64_t val, unsigned len) >>> +{ >>> + if (addr == 1 && len == 1) { >>> + if (val & 4) { >>> + qemu_system_reset_request(); >>> + } >>> + return; >>> + } >>> + pci_host_conf_le_ops.write(opaque, addr, val, len); >>> +} >>> + >>> +static MemoryRegionOps i440fx_host_conf_ops = { >>> + .read = NULL, >>> + .write = i440fx_host_config_write, >>> + .endianness = DEVICE_LITTLE_ENDIAN >>> +}; >>> + >>> static int i440fx_pcihost_initfn(SysBusDevice *dev) >>> { >>> PCIHostState *s = PCI_HOST_BRIDGE(dev); >>> >>> - memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s, >>> + i440fx_host_conf_ops.read = pci_host_conf_le_ops.read; >> >> It would be cleaner to introduce a new memory region (without this >> copying) which passes 0xcf8 and 0xcfc to standard PCI host but catches >> accesses to 0xcf9. This may mean that pci_host_config_{read,write} >> will need to be exposed. > > Do you mean: > > (1) introducing the new "i440fx_host_conf_ops" struct-of-funcptrs with > detached functions (that is, duplicating the guts of > pci_host_config_{read,write} and modifying them, and then registering > s->conf_mem with this "i440fx_host_conf_ops"; or > > (2) leaving s->conf_mem as-is, and introducing a sub-region just for > port 0xcf9, with higher visibility priority? > > (I don't feel confident about (2), and based on "docs/memory.txt" I > thought that overlapping regions had not been invented for this purpose.) > > IOW, are you OK with the explicit offset + access-width based check, > just organized differently, or are you proposing a one-byte-wide subregion? Another option: (3) leaving s->conf_mem as-is but implementing your own read function as well that forwards to pci_host_conf_le_ops.read() to avoid this unusual non-const MemoryRegionOps construct But I guess Blue meant (2), which should be slightly more performant. Regards, Andreas
diff --git a/hw/piix_pci.c b/hw/piix_pci.c index 3d79c73..89d694c 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -31,6 +31,7 @@ #include "qemu/range.h" #include "xen.h" #include "pam.h" +#include "sysemu/sysemu.h" /* * I440FX chipset data sheet. @@ -180,11 +181,30 @@ static const VMStateDescription vmstate_i440fx = { } }; +static void i440fx_host_config_write(void *opaque, hwaddr addr, + uint64_t val, unsigned len) +{ + if (addr == 1 && len == 1) { + if (val & 4) { + qemu_system_reset_request(); + } + return; + } + pci_host_conf_le_ops.write(opaque, addr, val, len); +} + +static MemoryRegionOps i440fx_host_conf_ops = { + .read = NULL, + .write = i440fx_host_config_write, + .endianness = DEVICE_LITTLE_ENDIAN +}; + static int i440fx_pcihost_initfn(SysBusDevice *dev) { PCIHostState *s = PCI_HOST_BRIDGE(dev); - memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s, + i440fx_host_conf_ops.read = pci_host_conf_le_ops.read; + memory_region_init_io(&s->conf_mem, &i440fx_host_conf_ops, s, "pci-conf-idx", 4); sysbus_add_io(dev, 0xcf8, &s->conf_mem); sysbus_init_ioports(&s->busdev, 0xcf8, 4);
From <http://mjg59.dreamwidth.org/3561.html>: Traditional PCI config space access is achieved by writing a 32 bit value to io port 0xcf8 to identify the bus, device, function and config register. Port 0xcfc then contains the register in question. But if you write the appropriate pair of magic values to 0xcf9, the machine will reboot. Spectacular! And not standardised in any way (certainly not part of the PCI spec), so different chipsets may have different requirements. Booo. In the PIIX4 spec, IO port 0xcf9 is specified as the Reset Control Register. Therefore let's handle single byte writes to offset 1 in the [0xcf8, 0xcfb] range separately, and interpret the RCPU bit in the value. (Based on "docs/memory.txt", overlapping regions seem to serve a different purpose.) The SRST bit alone could be stateful. However we don't distinguish between soft & hard reset, hence the SRST bit is neither checked nor saved. RHBZ reference: 890459 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/piix_pci.c | 22 +++++++++++++++++++++- 1 files changed, 21 insertions(+), 1 deletions(-)