[RFC,v1,2/4] exec.c: Remove static allocation of sub_section of sub_page
diff mbox

Message ID 1465808915-4887-3-git-send-email-vijayak@caviumnetworks.com
State New
Headers show

Commit Message

vijayak@caviumnetworks.com June 13, 2016, 9:08 a.m. UTC
From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Allocate sub_section dynamically. Remove dependency
on TARGET_PAGE_SIZE to make run-time page size detection
for arm platforms.

Signed-off-by: Vijaya Kumar K <vijayak@cavium.com>
---
 exec.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Peter Maydell June 13, 2016, 9:32 a.m. UTC | #1
On 13 June 2016 at 10:08,  <vijayak@caviumnetworks.com> wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> Allocate sub_section dynamically. Remove dependency
> on TARGET_PAGE_SIZE to make run-time page size detection
> for arm platforms.
>
> Signed-off-by: Vijaya Kumar K <vijayak@cavium.com>
> ---
>  exec.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index a9d465b..e803a41 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -154,7 +154,7 @@ typedef struct subpage_t {
>      MemoryRegion iomem;
>      AddressSpace *as;
>      hwaddr base;
> -    uint16_t sub_section[TARGET_PAGE_SIZE];
> +    uint16_t *sub_section;
>  } subpage_t;
>
>  #define PHYS_SECTION_UNASSIGNED 0
> @@ -1151,6 +1151,7 @@ static void phys_section_destroy(MemoryRegion *mr)
>      if (have_sub_page) {
>          subpage_t *subpage = container_of(mr, subpage_t, iomem);
>          object_unref(OBJECT(&subpage->iomem));
> +        g_free(subpage->sub_section);
>          g_free(subpage);
>      }
>  }
> @@ -2272,7 +2273,7 @@ static subpage_t *subpage_init(AddressSpace *as, hwaddr base)
>      subpage_t *mmio;
>
>      mmio = g_malloc0(sizeof(subpage_t));
> -
> +    mmio->sub_section = g_malloc0(TARGET_PAGE_SIZE * sizeof(uint16_t));

You can write this as
   = g_new0(uint16_t, TARGET_PAGE_SIZE);

thanks
-- PMM
Paolo Bonzini June 13, 2016, 9:52 a.m. UTC | #2
On 13/06/2016 11:08, vijayak@caviumnetworks.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Allocate sub_section dynamically. Remove dependency
> on TARGET_PAGE_SIZE to make run-time page size detection
> for arm platforms.
> 
> Signed-off-by: Vijaya Kumar K <vijayak@cavium.com>
> ---
>  exec.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index a9d465b..e803a41 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -154,7 +154,7 @@ typedef struct subpage_t {
>      MemoryRegion iomem;
>      AddressSpace *as;
>      hwaddr base;
> -    uint16_t sub_section[TARGET_PAGE_SIZE];
> +    uint16_t *sub_section;

Please make this a flexible array member instead, so that you can avoid
the extra pointer dereference.

Thanks,

Paolo

>  } subpage_t;
>  
>  #define PHYS_SECTION_UNASSIGNED 0
> @@ -1151,6 +1151,7 @@ static void phys_section_destroy(MemoryRegion *mr)
>      if (have_sub_page) {
>          subpage_t *subpage = container_of(mr, subpage_t, iomem);
>          object_unref(OBJECT(&subpage->iomem));
> +        g_free(subpage->sub_section);
>          g_free(subpage);
>      }
>  }
> @@ -2272,7 +2273,7 @@ static subpage_t *subpage_init(AddressSpace *as, hwaddr base)
>      subpage_t *mmio;
>  
>      mmio = g_malloc0(sizeof(subpage_t));
> -
> +    mmio->sub_section = g_malloc0(TARGET_PAGE_SIZE * sizeof(uint16_t));
>      mmio->as = as;
>      mmio->base = base;
>      memory_region_init_io(&mmio->iomem, NULL, &subpage_ops, mmio,
>
Vijay Kilari June 17, 2016, 10:14 a.m. UTC | #3
Hi Paolo,

On Mon, Jun 13, 2016 at 3:22 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 13/06/2016 11:08, vijayak@caviumnetworks.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> Allocate sub_section dynamically. Remove dependency
>> on TARGET_PAGE_SIZE to make run-time page size detection
>> for arm platforms.
>>
>> Signed-off-by: Vijaya Kumar K <vijayak@cavium.com>
>> ---
>>  exec.c |    5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index a9d465b..e803a41 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -154,7 +154,7 @@ typedef struct subpage_t {
>>      MemoryRegion iomem;
>>      AddressSpace *as;
>>      hwaddr base;
>> -    uint16_t sub_section[TARGET_PAGE_SIZE];
>> +    uint16_t *sub_section;
>
> Please make this a flexible array member instead, so that you can avoid
> the extra pointer dereference.

What do you mean by flexible array member?. please give more info.
Peter Maydell June 17, 2016, 10:29 a.m. UTC | #4
On 17 June 2016 at 11:14, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> Hi Paolo,
>
> On Mon, Jun 13, 2016 at 3:22 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> diff --git a/exec.c b/exec.c
>>> index a9d465b..e803a41 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -154,7 +154,7 @@ typedef struct subpage_t {
>>>      MemoryRegion iomem;
>>>      AddressSpace *as;
>>>      hwaddr base;
>>> -    uint16_t sub_section[TARGET_PAGE_SIZE];
>>> +    uint16_t *sub_section;
>>
>> Please make this a flexible array member instead, so that you can avoid
>> the extra pointer dereference.
>
> What do you mean by flexible array member?. please give more info.

https://en.wikipedia.org/wiki/Flexible_array_member

thanks
-- PMM

Patch
diff mbox

diff --git a/exec.c b/exec.c
index a9d465b..e803a41 100644
--- a/exec.c
+++ b/exec.c
@@ -154,7 +154,7 @@  typedef struct subpage_t {
     MemoryRegion iomem;
     AddressSpace *as;
     hwaddr base;
-    uint16_t sub_section[TARGET_PAGE_SIZE];
+    uint16_t *sub_section;
 } subpage_t;
 
 #define PHYS_SECTION_UNASSIGNED 0
@@ -1151,6 +1151,7 @@  static void phys_section_destroy(MemoryRegion *mr)
     if (have_sub_page) {
         subpage_t *subpage = container_of(mr, subpage_t, iomem);
         object_unref(OBJECT(&subpage->iomem));
+        g_free(subpage->sub_section);
         g_free(subpage);
     }
 }
@@ -2272,7 +2273,7 @@  static subpage_t *subpage_init(AddressSpace *as, hwaddr base)
     subpage_t *mmio;
 
     mmio = g_malloc0(sizeof(subpage_t));
-
+    mmio->sub_section = g_malloc0(TARGET_PAGE_SIZE * sizeof(uint16_t));
     mmio->as = as;
     mmio->base = base;
     memory_region_init_io(&mmio->iomem, NULL, &subpage_ops, mmio,