Re: [Qemu-devel] [RFC PATCH] PIIX: reset the VM when the Reset Control Register's RCPU bit gets set

[Blue Swirl]

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