| Message ID | 20210819142039.2825366-5-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory: Introduce address_space_create(), re-use &address_space_memory | expand |
On Thu, 19 Aug 2021 at 15:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Replace g_malloc0() + address_space_init() by address_space_create(). > Release the resource in DeviceUnrealize(). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/dma/xlnx-zdma.c | 15 +++++++++------ > hw/dma/xlnx_csu_dma.c | 9 ++------- > 2 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c > index fa38a556341..9f6b3fa47c6 100644 > --- a/hw/dma/xlnx-zdma.c > +++ b/hw/dma/xlnx-zdma.c > @@ -777,15 +777,17 @@ static void zdma_realize(DeviceState *dev, Error **errp) > }; > } > > - if (s->dma_mr) { > - s->dma_as = g_malloc0(sizeof(AddressSpace)); > - address_space_init(s->dma_as, s->dma_mr, NULL); > - } else { > - s->dma_as = &address_space_memory; > - } > + s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL); > s->attr = MEMTXATTRS_UNSPECIFIED; > } Why are these devices doing a heap allocation rather than having an AddressSpace whatever field in their device struct ? thanks -- PMM
On 8/19/21 4:22 PM, Peter Maydell wrote: > On Thu, 19 Aug 2021 at 15:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> Replace g_malloc0() + address_space_init() by address_space_create(). >> Release the resource in DeviceUnrealize(). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/dma/xlnx-zdma.c | 15 +++++++++------ >> hw/dma/xlnx_csu_dma.c | 9 ++------- >> 2 files changed, 11 insertions(+), 13 deletions(-) >> >> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c >> index fa38a556341..9f6b3fa47c6 100644 >> --- a/hw/dma/xlnx-zdma.c >> +++ b/hw/dma/xlnx-zdma.c >> @@ -777,15 +777,17 @@ static void zdma_realize(DeviceState *dev, Error **errp) >> }; >> } >> >> - if (s->dma_mr) { >> - s->dma_as = g_malloc0(sizeof(AddressSpace)); >> - address_space_init(s->dma_as, s->dma_mr, NULL); >> - } else { >> - s->dma_as = &address_space_memory; >> - } >> + s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL); >> s->attr = MEMTXATTRS_UNSPECIFIED; >> } > > Why are these devices doing a heap allocation rather than > having an AddressSpace whatever field in their device struct ? To easily use &address_space_memory if 'memory' link is NULL?
On Thu, 19 Aug 2021 at 15:32, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 8/19/21 4:22 PM, Peter Maydell wrote: > > On Thu, 19 Aug 2021 at 15:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> > >> Replace g_malloc0() + address_space_init() by address_space_create(). > >> Release the resource in DeviceUnrealize(). > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> hw/dma/xlnx-zdma.c | 15 +++++++++------ > >> hw/dma/xlnx_csu_dma.c | 9 ++------- > >> 2 files changed, 11 insertions(+), 13 deletions(-) > >> > >> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c > >> index fa38a556341..9f6b3fa47c6 100644 > >> --- a/hw/dma/xlnx-zdma.c > >> +++ b/hw/dma/xlnx-zdma.c > >> @@ -777,15 +777,17 @@ static void zdma_realize(DeviceState *dev, Error **errp) > >> }; > >> } > >> > >> - if (s->dma_mr) { > >> - s->dma_as = g_malloc0(sizeof(AddressSpace)); > >> - address_space_init(s->dma_as, s->dma_mr, NULL); > >> - } else { > >> - s->dma_as = &address_space_memory; > >> - } > >> + s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL); > >> s->attr = MEMTXATTRS_UNSPECIFIED; > >> } > > > > Why are these devices doing a heap allocation rather than > > having an AddressSpace whatever field in their device struct ? > > To easily use &address_space_memory if 'memory' link is NULL? They could do that with AddressSpace our_as; AddressSpace *dma_as; and set dma_as to &s->our_as or &address_space_memory. Or we could fix the two boards which create these devices to always pass in an MR to use for DMA and drop the conditionality. -- PMM
On 8/19/21 4:38 PM, Peter Maydell wrote: > On Thu, 19 Aug 2021 at 15:32, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> On 8/19/21 4:22 PM, Peter Maydell wrote: >>> On Thu, 19 Aug 2021 at 15:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >>>> >>>> Replace g_malloc0() + address_space_init() by address_space_create(). >>>> Release the resource in DeviceUnrealize(). >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> hw/dma/xlnx-zdma.c | 15 +++++++++------ >>>> hw/dma/xlnx_csu_dma.c | 9 ++------- >>>> 2 files changed, 11 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c >>>> index fa38a556341..9f6b3fa47c6 100644 >>>> --- a/hw/dma/xlnx-zdma.c >>>> +++ b/hw/dma/xlnx-zdma.c >>>> @@ -777,15 +777,17 @@ static void zdma_realize(DeviceState *dev, Error **errp) >>>> }; >>>> } >>>> >>>> - if (s->dma_mr) { >>>> - s->dma_as = g_malloc0(sizeof(AddressSpace)); >>>> - address_space_init(s->dma_as, s->dma_mr, NULL); >>>> - } else { >>>> - s->dma_as = &address_space_memory; >>>> - } >>>> + s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL); >>>> s->attr = MEMTXATTRS_UNSPECIFIED; >>>> } >>> >>> Why are these devices doing a heap allocation rather than >>> having an AddressSpace whatever field in their device struct ? >> >> To easily use &address_space_memory if 'memory' link is NULL? > > They could do that with > AddressSpace our_as; > AddressSpace *dma_as; > > and set dma_as to &s->our_as or &address_space_memory. > > Or we could fix the two boards which create these devices to always > pass in an MR to use for DMA and drop the conditionality. Clever, thanks.
diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c index fa38a556341..9f6b3fa47c6 100644 --- a/hw/dma/xlnx-zdma.c +++ b/hw/dma/xlnx-zdma.c @@ -777,15 +777,17 @@ static void zdma_realize(DeviceState *dev, Error **errp) }; } - if (s->dma_mr) { - s->dma_as = g_malloc0(sizeof(AddressSpace)); - address_space_init(s->dma_as, s->dma_mr, NULL); - } else { - s->dma_as = &address_space_memory; - } + s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL); s->attr = MEMTXATTRS_UNSPECIFIED; } +static void zdma_unrealize(DeviceState *dev) +{ + XlnxZDMA *s = XLNX_ZDMA(dev); + + address_space_destroy(s->dma_as); +} + static void zdma_init(Object *obj) { XlnxZDMA *s = XLNX_ZDMA(obj); @@ -827,6 +829,7 @@ static void zdma_class_init(ObjectClass *klass, void *data) dc->reset = zdma_reset; dc->realize = zdma_realize; + dc->unrealize = zdma_unrealize; device_class_set_props(dc, zdma_props); dc->vmsd = &vmstate_zdma; } diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c index 0c1c19cab5a..730c0f999ce 100644 --- a/hw/dma/xlnx_csu_dma.c +++ b/hw/dma/xlnx_csu_dma.c @@ -648,13 +648,7 @@ static void xlnx_csu_dma_realize(DeviceState *dev, Error **errp) s->src_timer = ptimer_init(xlnx_csu_dma_src_timeout_hit, s, PTIMER_POLICY_DEFAULT); - if (s->dma_mr) { - s->dma_as = g_malloc0(sizeof(AddressSpace)); - address_space_init(s->dma_as, s->dma_mr, NULL); - } else { - s->dma_as = &address_space_memory; - } - + s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL); s->attr = MEMTXATTRS_UNSPECIFIED; s->r_size_last_word = 0; @@ -665,6 +659,7 @@ static void xlnx_csu_dma_unrealize(DeviceState *dev) XlnxCSUDMA *s = XLNX_CSU_DMA(dev); ptimer_free(s->src_timer); + address_space_destroy(s->dma_as); } static const VMStateDescription vmstate_xlnx_csu_dma = {
Replace g_malloc0() + address_space_init() by address_space_create(). Release the resource in DeviceUnrealize(). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/dma/xlnx-zdma.c | 15 +++++++++------ hw/dma/xlnx_csu_dma.c | 9 ++------- 2 files changed, 11 insertions(+), 13 deletions(-)