On Tue, 4 Jul 2023, Philippe Mathieu-Daudé wrote:
> On 4/7/23 11:37, BALATON Zoltan wrote:
>> On Tue, 4 Jul 2023, Philippe Mathieu-Daudé wrote:
>>> On 4/7/23 00:02, BALATON Zoltan wrote:
>>>> The iomem memory region is better used for the PCI IO space but
>>>> currently used for registers. Stop using it for that to allow this to
>>>> be cleaned up in the next patch.
>>>> 
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>   hw/ppc/ppc440_pcix.c | 7 ++++---
>>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
>>>> index adfecf1e76..ee2dc44f67 100644
>>>> --- a/hw/ppc/ppc440_pcix.c
>>>> +++ b/hw/ppc/ppc440_pcix.c
>>>> @@ -484,6 +484,7 @@ static void ppc440_pcix_realize(DeviceState *dev, 
>>>> Error **errp)
>>>>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>>       PPC440PCIXState *s;
>>>>       PCIHostState *h;
>>>> +    MemoryRegion *regs = g_new(MemoryRegion, 1);
>>> 
>>> Why not hold it within PPC440PCIXState?
>> 
>> Because it's never needed after this function.
>
> But we can't free() it because it has to stay valid as long as
> PPC440PCIXState is in use. So it seems to belong there.

OK, moved it to PPC440PCIXState.

Regards,
BALATON Zoltan

>>>>       h = PCI_HOST_BRIDGE(dev);
>>>>       s = PPC440_PCIX_HOST(dev);
>>>> @@ -507,11 +508,11 @@ static void ppc440_pcix_realize(DeviceState *dev, 
>>>> Error **errp)
>>>>                             h, "pci-conf-idx", 4);
>>>>       memory_region_init_io(&h->data_mem, OBJECT(s), 
>>>> &pci_host_data_le_ops,
>>>>                             h, "pci-conf-data", 4);
>>>> -    memory_region_init_io(&s->iomem, OBJECT(s), &pci_reg_ops, s,
>>>> -                          "pci.reg", PPC440_REG_SIZE);
>>>> +    memory_region_init_io(regs, OBJECT(s), &pci_reg_ops, s, "pci-reg",
>>>> +                          PPC440_REG_SIZE);
>>>>       memory_region_add_subregion(&s->container, PCIC0_CFGADDR, 
>>>> &h->conf_mem);
>>>>       memory_region_add_subregion(&s->container, PCIC0_CFGDATA, 
>>>> &h->data_mem);
>>>> -    memory_region_add_subregion(&s->container, PPC440_REG_BASE, 
>>>> &s->iomem);
>>>> +    memory_region_add_subregion(&s->container, PPC440_REG_BASE, regs);
>>>>       sysbus_init_mmio(sbd, &s->container);
>>>>   }
>>>> 
>>> 
>>> 
>>> 
>
>
>