1. Patchew
  2. QEMU
  3. [PATCH 00/13] PPC440 devices misc clean up
  4. View patch

[PATCH 12/13] ppc440_pcix: Don't use iomem for regs

BALATON Zoltan posted 13 patches 2 years, 7 months ago
Maintainers: BALATON Zoltan <balaton@eik.bme.hu>
There is a newer version of this series
[PATCH 12/13] ppc440_pcix: Don't use iomem for regs
Posted by BALATON Zoltan 2 years, 7 months ago 
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);
 
     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);
 }
 
-- 
2.30.9
Re: [PATCH 12/13] ppc440_pcix: Don't use iomem for regs
Posted by Philippe Mathieu-Daudé 2 years, 7 months ago 
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?

>       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);
>   }
>
Re: [PATCH 12/13] ppc440_pcix: Don't use iomem for regs
Posted by BALATON Zoltan 2 years, 7 months ago 
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.

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);
>>   }
>> 
>
>
>
Re: [PATCH 12/13] ppc440_pcix: Don't use iomem for regs
Posted by Philippe Mathieu-Daudé 2 years, 7 months ago 
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.

> 
> 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);
>>>   }
>>>
>>
>>
>>
Re: [PATCH 12/13] ppc440_pcix: Don't use iomem for regs
Posted by BALATON Zoltan 2 years, 7 months ago 
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);
>>>>   }
>>>> 
>>> 
>>> 
>>> 
>
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.