| Message ID | 20220726092248.128336-3-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | dump: Add arch section and s390x PV dump | expand |
On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> wrote: > > The iteration over the memblocks is hard to understand so it's about > time to clean it up. Instead of manually grabbing the next memblock we > can use QTAILQ_FOREACH to iterate over all memblocks. > > Additionally we move the calculation of the offset and length out by > using the dump_get_memblock_*() functions. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > dump/dump.c | 37 +++++++++++++++++++++++++++++++++++++ > include/sysemu/dump.h | 5 +++++ > 2 files changed, 42 insertions(+) > > diff --git a/dump/dump.c b/dump/dump.c > index 0ed7cf9c7b..0fd7c76c1e 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -591,6 +591,43 @@ static void dump_begin(DumpState *s, Error **errp) > write_elf_notes(s, errp); > } > > +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t filter_area_start, > + int64_t filter_area_length) > +{ > + int64_t size, left, right; > + > + /* No filter, return full size */ > + if (!filter_area_length) { > + return block->target_end - block->target_start; > + } > + > + /* calculate the overlapped region. */ > + left = MAX(filter_area_start, block->target_start); > + right = MIN(filter_area_start + filter_area_length, block->target_end); > + size = right - left; > + size = size > 0 ? size : 0; > + > + return size; > +} > + > +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t filter_area_start, > + int64_t filter_area_length) > +{ > + if (filter_area_length) { > + /* return -1 if the block is not within filter area */ > + if (block->target_start >= filter_area_start + filter_area_length || > + block->target_end <= filter_area_start) { > + return -1; > + } > + > + if (filter_area_start > block->target_start) { > + return filter_area_start - block->target_start; > + } > + } > + > + return 0; > +} > + > static int get_next_block(DumpState *s, GuestPhysBlock *block) > { > while (1) { > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index ffc2ea1072..6ce3c24197 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -203,4 +203,9 @@ typedef struct DumpState { > uint16_t cpu_to_dump16(DumpState *s, uint16_t val); > uint32_t cpu_to_dump32(DumpState *s, uint32_t val); > uint64_t cpu_to_dump64(DumpState *s, uint64_t val); > + > +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t filter_area_start, > + int64_t filter_area_length); > +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t filter_area_start, > + int64_t filter_area_length); The functions don't need to be exported. You probably need to introduce them back with their usage, to avoid some compiler warning. If you can't split the introduction and related refactoring, then let's have a single patch. Thanks > #endif > -- > 2.34.1 >
On 7/26/22 13:35, Marc-André Lureau wrote: > On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> wrote: >> >> The iteration over the memblocks is hard to understand so it's about >> time to clean it up. Instead of manually grabbing the next memblock we >> can use QTAILQ_FOREACH to iterate over all memblocks. >> >> Additionally we move the calculation of the offset and length out by >> using the dump_get_memblock_*() functions. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> dump/dump.c | 37 +++++++++++++++++++++++++++++++++++++ >> include/sysemu/dump.h | 5 +++++ >> 2 files changed, 42 insertions(+) >> >> diff --git a/dump/dump.c b/dump/dump.c >> index 0ed7cf9c7b..0fd7c76c1e 100644 >> --- a/dump/dump.c >> +++ b/dump/dump.c >> @@ -591,6 +591,43 @@ static void dump_begin(DumpState *s, Error **errp) >> write_elf_notes(s, errp); >> } >> >> +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t filter_area_start, >> + int64_t filter_area_length) >> +{ >> + int64_t size, left, right; >> + >> + /* No filter, return full size */ >> + if (!filter_area_length) { >> + return block->target_end - block->target_start; >> + } >> + >> + /* calculate the overlapped region. */ >> + left = MAX(filter_area_start, block->target_start); >> + right = MIN(filter_area_start + filter_area_length, block->target_end); >> + size = right - left; >> + size = size > 0 ? size : 0; >> + >> + return size; >> +} >> + >> +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t filter_area_start, >> + int64_t filter_area_length) >> +{ >> + if (filter_area_length) { >> + /* return -1 if the block is not within filter area */ >> + if (block->target_start >= filter_area_start + filter_area_length || >> + block->target_end <= filter_area_start) { >> + return -1; >> + } >> + >> + if (filter_area_start > block->target_start) { >> + return filter_area_start - block->target_start; >> + } >> + } >> + >> + return 0; >> +} >> + >> static int get_next_block(DumpState *s, GuestPhysBlock *block) >> { >> while (1) { >> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h >> index ffc2ea1072..6ce3c24197 100644 >> --- a/include/sysemu/dump.h >> +++ b/include/sysemu/dump.h >> @@ -203,4 +203,9 @@ typedef struct DumpState { >> uint16_t cpu_to_dump16(DumpState *s, uint16_t val); >> uint32_t cpu_to_dump32(DumpState *s, uint32_t val); >> uint64_t cpu_to_dump64(DumpState *s, uint64_t val); >> + >> +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t filter_area_start, >> + int64_t filter_area_length); >> +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t filter_area_start, >> + int64_t filter_area_length); > > The functions don't need to be exported. You probably need to > introduce them back with their usage, to avoid some compiler warning. Right, I'll add them in the last s390 dump patch and make them static > If you can't split the introduction and related refactoring, then > let's have a single patch. So squashing this with the next one but leave the other refactoring patches (dump_calculate_size() and get_start_block()) as they are? > > Thanks > >> #endif >> -- >> 2.34.1 >> > >
On Tue, Jul 26, 2022 at 5:05 PM Janosch Frank <frankja@linux.ibm.com> wrote: > On 7/26/22 13:35, Marc-André Lureau wrote: > > On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> > wrote: > >> > >> The iteration over the memblocks is hard to understand so it's about > >> time to clean it up. Instead of manually grabbing the next memblock we > >> can use QTAILQ_FOREACH to iterate over all memblocks. > >> > >> Additionally we move the calculation of the offset and length out by > >> using the dump_get_memblock_*() functions. > >> > >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > >> --- > >> dump/dump.c | 37 +++++++++++++++++++++++++++++++++++++ > >> include/sysemu/dump.h | 5 +++++ > >> 2 files changed, 42 insertions(+) > >> > >> diff --git a/dump/dump.c b/dump/dump.c > >> index 0ed7cf9c7b..0fd7c76c1e 100644 > >> --- a/dump/dump.c > >> +++ b/dump/dump.c > >> @@ -591,6 +591,43 @@ static void dump_begin(DumpState *s, Error **errp) > >> write_elf_notes(s, errp); > >> } > >> > >> +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t > filter_area_start, > >> + int64_t filter_area_length) > >> +{ > >> + int64_t size, left, right; > >> + > >> + /* No filter, return full size */ > >> + if (!filter_area_length) { > >> + return block->target_end - block->target_start; > >> + } > >> + > >> + /* calculate the overlapped region. */ > >> + left = MAX(filter_area_start, block->target_start); > >> + right = MIN(filter_area_start + filter_area_length, > block->target_end); > >> + size = right - left; > >> + size = size > 0 ? size : 0; > >> + > >> + return size; > >> +} > >> + > >> +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t > filter_area_start, > >> + int64_t filter_area_length) > >> +{ > >> + if (filter_area_length) { > >> + /* return -1 if the block is not within filter area */ > >> + if (block->target_start >= filter_area_start + > filter_area_length || > >> + block->target_end <= filter_area_start) { > >> + return -1; > >> + } > >> + > >> + if (filter_area_start > block->target_start) { > >> + return filter_area_start - block->target_start; > >> + } > >> + } > >> + > >> + return 0; > >> +} > >> + > >> static int get_next_block(DumpState *s, GuestPhysBlock *block) > >> { > >> while (1) { > >> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > >> index ffc2ea1072..6ce3c24197 100644 > >> --- a/include/sysemu/dump.h > >> +++ b/include/sysemu/dump.h > >> @@ -203,4 +203,9 @@ typedef struct DumpState { > >> uint16_t cpu_to_dump16(DumpState *s, uint16_t val); > >> uint32_t cpu_to_dump32(DumpState *s, uint32_t val); > >> uint64_t cpu_to_dump64(DumpState *s, uint64_t val); > >> + > >> +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t > filter_area_start, > >> + int64_t filter_area_length); > >> +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t > filter_area_start, > >> + int64_t filter_area_length); > > > > The functions don't need to be exported. You probably need to > > introduce them back with their usage, to avoid some compiler warning. > > Right, I'll add them in the last s390 dump patch and make them static > > > If you can't split the introduction and related refactoring, then > > let's have a single patch. > > So squashing this with the next one but leave the other refactoring > patches (dump_calculate_size() and get_start_block()) as they are? > > Right, if you can't split it further.
On 7/26/22 11:22, Janosch Frank wrote: > The iteration over the memblocks is hard to understand so it's about > time to clean it up. Instead of manually grabbing the next memblock we > can use QTAILQ_FOREACH to iterate over all memblocks. This got out of sync with the patch, didn't it? With that addressed: Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> See minor stuff below. > > Additionally we move the calculation of the offset and length out by > using the dump_get_memblock_*() functions. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > dump/dump.c | 37 +++++++++++++++++++++++++++++++++++++ > include/sysemu/dump.h | 5 +++++ > 2 files changed, 42 insertions(+) > > diff --git a/dump/dump.c b/dump/dump.c > index 0ed7cf9c7b..0fd7c76c1e 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -591,6 +591,43 @@ static void dump_begin(DumpState *s, Error **errp) > write_elf_notes(s, errp); > } > > +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t filter_area_start, > + int64_t filter_area_length)> +{ > + int64_t size, left, right; I assume the int64_t everywhere are because DumpState.begin and .length are int64_t, which is itself because the numbers are coming from a command? There isn't any reason to have negative numbers for that command, is there? Since the block->target_* are unsigned we'd get problems with negative numbers. Ideally the the values should be checked up the stack and unsigned used in this function, IMO, but it's not a big deal either. > + > + /* No filter, return full size */ > + if (!filter_area_length) { > + return block->target_end - block->target_start; > + } > + > + /* calculate the overlapped region. */ > + left = MAX(filter_area_start, block->target_start); > + right = MIN(filter_area_start + filter_area_length, block->target_end); > + size = right - left; > + size = size > 0 ? size : 0; > + > + return size; > +} > + > +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t filter_area_start, > + int64_t filter_area_length) > +{ > + if (filter_area_length) { > + /* return -1 if the block is not within filter area */ > + if (block->target_start >= filter_area_start + filter_area_length || > + block->target_end <= filter_area_start) { > + return -1; > + } > + > + if (filter_area_start > block->target_start) { > + return filter_area_start - block->target_start; > + } > + } > + > + return 0; > +} > + > static int get_next_block(DumpState *s, GuestPhysBlock *block) > { > while (1) { > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index ffc2ea1072..6ce3c24197 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -203,4 +203,9 @@ typedef struct DumpState { > uint16_t cpu_to_dump16(DumpState *s, uint16_t val); > uint32_t cpu_to_dump32(DumpState *s, uint32_t val); > uint64_t cpu_to_dump64(DumpState *s, uint64_t val); > + > +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t filter_area_start, > + int64_t filter_area_length); > +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t filter_area_start, > + int64_t filter_area_length); I don't love the names of the functions, maybe dump_filtered_block_size, dump_filtered_block_offset? > #endif
diff --git a/dump/dump.c b/dump/dump.c index 0ed7cf9c7b..0fd7c76c1e 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -591,6 +591,43 @@ static void dump_begin(DumpState *s, Error **errp) write_elf_notes(s, errp); } +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t filter_area_start, + int64_t filter_area_length) +{ + int64_t size, left, right; + + /* No filter, return full size */ + if (!filter_area_length) { + return block->target_end - block->target_start; + } + + /* calculate the overlapped region. */ + left = MAX(filter_area_start, block->target_start); + right = MIN(filter_area_start + filter_area_length, block->target_end); + size = right - left; + size = size > 0 ? size : 0; + + return size; +} + +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t filter_area_start, + int64_t filter_area_length) +{ + if (filter_area_length) { + /* return -1 if the block is not within filter area */ + if (block->target_start >= filter_area_start + filter_area_length || + block->target_end <= filter_area_start) { + return -1; + } + + if (filter_area_start > block->target_start) { + return filter_area_start - block->target_start; + } + } + + return 0; +} + static int get_next_block(DumpState *s, GuestPhysBlock *block) { while (1) { diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h index ffc2ea1072..6ce3c24197 100644 --- a/include/sysemu/dump.h +++ b/include/sysemu/dump.h @@ -203,4 +203,9 @@ typedef struct DumpState { uint16_t cpu_to_dump16(DumpState *s, uint16_t val); uint32_t cpu_to_dump32(DumpState *s, uint32_t val); uint64_t cpu_to_dump64(DumpState *s, uint64_t val); + +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t filter_area_start, + int64_t filter_area_length); +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t filter_area_start, + int64_t filter_area_length); #endif
The iteration over the memblocks is hard to understand so it's about time to clean it up. Instead of manually grabbing the next memblock we can use QTAILQ_FOREACH to iterate over all memblocks. Additionally we move the calculation of the offset and length out by using the dump_get_memblock_*() functions. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- dump/dump.c | 37 +++++++++++++++++++++++++++++++++++++ include/sysemu/dump.h | 5 +++++ 2 files changed, 42 insertions(+)