[RFC] PIIX: reset the VM when the Reset Control Register's RCPU bit gets set

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 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}.

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:

--------*--------
--------*--------

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

On Fri, Jan 11, 2013 at 12:20 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 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 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

On 01/12/13 13:13, Blue Swirl wrote:
> On Fri, Jan 11, 2013 at 12:20 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> 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 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?

I'll come through as my own detractor, but I don't feel confident
experimenting with this. It might work, but that would not be convincing
enough; I don't want to violate any (undocumented) rules of the memory
region framework.

The region with name "pci-conf-idx" is registered like this, for
[0xcf8..0xcfb]:

i440fx_pcihost_initfn()
  memory_region_init_io()
    mr->terminates = true;
  sysbus_add_io()
    memory_region_add_subregion()
      subregion->may_overlap = false;
      memory_region_add_subregion_common()

The points that speak against turning 0xcf9 into a subregion are:
- in the memory_region_init_io() primitive (it says terminates=true,
apparently affecting the traversal in render_memory_region()), and
- in the memory_region_add_subregion() primitive (which says
may_overlap=false, which doesn't look very promising, because 0xcf9
would do exactly that).

I guess I could try to implement the subregion by open-coding the
memory_region_init_io() and sysbus_add_io() functions and overriding as
needed, but it seems like a mess. (Which is of course caused by this
impossible hack of PIIX4 that places the reset control register inside
the PCI config index register! The IO port 0xcf9 actually has double
purpose, and it doesn't depend on any kind of "mapping".)

Annotating "memory.c", most of it seems to have come from Avi. Can
someone else closely familiar with this code advise me? I maintain that
0xcf9 it's going to be a hack, and that my RFC (or the followup to that)
keeps the damage reasonably contained.

Thanks!
Laszlo

On 01/14/13 18:05, Laszlo Ersek wrote:

> I guess I could try to implement the subregion by open-coding the
> memory_region_init_io() and sysbus_add_io() functions and overriding as
> needed, but it seems like a mess. (Which is of course caused by this
> impossible hack of PIIX4 that places the reset control register inside
> the PCI config index register! The IO port 0xcf9 actually has double
> purpose, and it doesn't depend on any kind of "mapping".)

After looking again at the docs, I'm now trying to add a generic region
first (a container), and then, as its children, pci-conf-idx and rcr as
two sibling regions. The docs suggest siblings can overlap.

Thanks
Laszlo