[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] PIIX: reset the VM when the Reset Control R
From: |
Blue Swirl |
Subject: |
Re: [Qemu-devel] [RFC PATCH] PIIX: reset the VM when the Reset Control Register's RCPU bit gets set |
Date: |
Sat, 12 Jan 2013 12:13:34 +0000 |
On Fri, Jan 11, 2013 at 12:20 PM, Laszlo Ersek <address@hidden> wrote:
> On 01/09/13 22:01, Blue Swirl wrote:
>> On Tue, Jan 8, 2013 at 9:44 PM, Laszlo Ersek <address@hidden> wrote:
>
>>> 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.
>
> (3) Do you have a single region in mind that covers [ 0xcf8 ..0xcff ]
> contiguously? That would require exposing pci_host_data_{read,write}
> too, not only pci_host_config_{read,write}.
Actually I was thinking about a new region for 0xcf9 only, but perhaps
that is not possible without the higher priority and overlap. I'm also
not familiar with overlapping regions, could you try if it works?
>
> Plus, the config & data regions of I440FXState.parent_obj have distinct
> names now ("pci-conf-idx" and "pci-conf-data"); merging them (and
> inventing a new name) might be user-visible via the "info mtree" monitor
> command. (At least it would differ from many of the PCI host
> implementations in the tree.)
>
> What if I change v1 like this:
>
> --------*--------
> diff --git a/hw/pci/pci_host.h b/hw/pci/pci_host.h
> index 1845d4d..f5b6487 100644
> --- a/hw/pci/pci_host.h
> +++ b/hw/pci/pci_host.h
> @@ -53,6 +53,9 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev,
> uint32_t addr,
>
> void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
> uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
> +void pci_host_config_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned len);
> +uint64_t pci_host_config_read(void *opaque, hwaddr addr, unsigned len);
>
> extern const MemoryRegionOps pci_host_conf_le_ops;
> extern const MemoryRegionOps pci_host_conf_be_ops;
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 265c324..b0f359d 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -94,8 +94,8 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> return val;
> }
>
> -static void pci_host_config_write(void *opaque, hwaddr addr,
> - uint64_t val, unsigned len)
> +void pci_host_config_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned len)
> {
> PCIHostState *s = opaque;
>
> @@ -107,8 +107,7 @@ static void pci_host_config_write(void *opaque, hwaddr
> addr,
> s->config_reg = val;
> }
>
> -static uint64_t pci_host_config_read(void *opaque, hwaddr addr,
> - unsigned len)
> +uint64_t pci_host_config_read(void *opaque, hwaddr addr, unsigned len)
> {
> PCIHostState *s = opaque;
> uint32_t val = s->config_reg;
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 89d694c..168e20d 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -190,11 +190,11 @@ static void i440fx_host_config_write(void *opaque,
> hwaddr addr,
> }
> return;
> }
> - pci_host_conf_le_ops.write(opaque, addr, val, len);
> + pci_host_config_write(opaque, addr, val, len);
> }
>
> -static MemoryRegionOps i440fx_host_conf_ops = {
> - .read = NULL,
> +static const MemoryRegionOps i440fx_host_conf_ops = {
> + .read = pci_host_conf_read,
> .write = i440fx_host_config_write,
> .endianness = DEVICE_LITTLE_ENDIAN
> };
> @@ -203,7 +203,6 @@ static int i440fx_pcihost_initfn(SysBusDevice *dev)
> {
> PCIHostState *s = PCI_HOST_BRIDGE(dev);
>
> - 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);
> --------*--------
>
> I'm not sure at which depth you want me to stop duplicating the "base class":
> - call its functions via pci_host_conf_le_ops.FIELD (v1 rfc),
> - call its functions by their direct names (exposing them first, the
> above),
> - instead of reusing "pci_host_data_le_ops" for "pci-conf-data", create
> an "i440fx_host_DATA_ops" global as well:
> - pointing at pci_host_data_{read,write} (exposing them first),
> - pointing at private functions calling pci_host_data_{read,write}...
>
> Thanks,
> Laszlo