On 07/01/16 05:57, Xuguo Wang wrote:
> The function of the parse_ports has many redundant codes,
> and the logic is not clearly, so refactor this function
> and add some comments.
>
> Signed-off-by: Xuguo Wang <[email protected]>
> ---
> tools/jailhouse-config-create | 227
> ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 218 insertions(+), 9 deletions(-)
>
> diff --git a/tools/jailhouse-config-create b/tools/jailhouse-config-create
> index 9fe7f96..1197113 100755
> --- a/tools/jailhouse-config-create
> +++ b/tools/jailhouse-config-create
> @@ -1132,13 +1132,219 @@ def parse_ivrs(pcidevices, ioapics):
>
> def parse_ioports():
> pm_timer_base = None
> - f = input_open('/proc/ioports')
> - for line in f:
> - if line.endswith('ACPI PM_TMR\n'):
> - pm_timer_base = int(line.split('-')[0], 16)
> - break
> - f.close()
> - return pm_timer_base
> + bitmap_regions = []
> + regions = IOPortRegionTree.parse_ioport_tree(
> + IOPortRegionTree.parse_ioport_file())
to follow up with remarks in 1/3, this can be changed to
regions = IOPortRegionTree.parse_ioport_file().parse_ioport_tree()
which is in my opinion more straight forward than nesting the call of
static methods
> +
> + for region in regions:
> + # parse the ACPI PM_TMR
> + if region.typestr == 'ACPI PM_TMR':
> + pm_timer_base = region.start
> + continue
> +
> + # region PCI conf1 must intercept
> + if region.typestr == 'PCI conf1':
> + region.value = 0xff
> + if region.size() > 8:
> + region.value = -1
> + bitmap_regions.append(region)
> + continue
> +
> + byte_size = region.size()/8
> +
> + # region head bit and tail bit are both not aligned
> + if region.start % 8 != 0 and (region.stop % 8 + 1) % 8 != 0:
> + round_up = (region.start % 8)
> + round_down = 7 - (region.stop % 8)
> +
> + if byte_size <= 1:
> + value_head = IOPortRegionTree.get_first_ioport_byte(
> + region.start, region.stop)
> + else:
> + value_head = IOPortRegionTree.get_first_ioport_byte(
> + region.start, region.start + (7 - round_up))
> + value_tail = IOPortRegionTree.get_first_ioport_byte(
> + region.stop - (region.stop % 8), region.stop)
> +
> + # adjust the region address to make the region aligned
> + region.start -= round_up
> + region.stop += round_down
> +
> + # the region overlap with the previous region,
> + # so have to modify value and typestr
> + if region.start - round_up < bitmap_regions[-1].stop:
> + region.value = bitmap_regions[-1].value & value_head
> + region.typestr += ', ' + bitmap_regions[-1].typestr
> + bitmap_regions.pop()
> + else:
> + region.value = value_head
> +
> + # append the region according to the region overlap with the
> + # previous region or not
> + if byte_size <= 1:
> + bitmap_regions.append(region)
> + elif byte_size == 2 \
> + or (byte_size > 3 and
> + region.start - round_up < bitmap_regions[-1].stop):
> + bitmap_regions.append(IOPortRegion(
> + region.start,
> + region.start + (7 - round_up),
> + region.typestr,
> + region.value))
> + bitmap_regions.append(IOPortRegion(
> + region.stop - (region.stop % 8),
> + region.stop,
> + region.typestr,
> + value_tail))
> + else:
> + bitmap_regions.append(IOPortRegion(
> + region.start,
> + region.start + (7 - round_up),
> + region.typestr,
> + region.value))
> + bitmap_regions.append(IOPortRegion(
> + region.start + (7 - round_up) + 1,
> + region.stop - (region.stop % 8) - 1,
> + region.typestr,
> + 0))
> + bitmap_regions.append(IOPortRegion(
> + region.stop - (region.stop % 8),
> + region.stop,
> + region.typestr,
> + value_tail))
> +
> + # region head bit is not aligned but tail
> + elif region.start % 8 != 0 \
> + and (region.stop % 8 + 1) % 8 == 0:
> + round_up = (region.start % 8)
> +
> + if byte_size <= 1:
> + value_head = IOPortRegionTree.get_first_ioport_byte(
> + region.start, region.stop)
> + else:
> + value_head = IOPortRegionTree.get_first_ioport_byte(
> + region.start, region.start + (7 - round_up))
> + value_tail = IOPortRegionTree.get_first_ioport_byte(
> + region.stop - (region.stop % 8), region.stop)
> +
> + # adjust the region address to make the region aligned
> + region.start -= round_up
> +
> + # the region overlap with the previous region,
> + # so have to modify value and typestr
> + if region.start - round_up < bitmap_regions[-1].stop:
> + region.value = bitmap_regions[-1].value & value_head
> + region.typestr += ', ' + bitmap_regions[-1].typestr
> + bitmap_regions.pop()
> + else:
> + region.value = value_head
> +
> + # append the region according to the region overlap with the
> + # previous region or not
> + if byte_size <= 1:
> + bitmap_regions.append(region)
> + elif byte_size == 2 \
> + or (byte_size > 3 and
> + region.start - round_up < bitmap_regions[-1].stop):
> + region.value = value_head
> + bitmap_regions.append(IOPortRegion(
> + region.start,
> + region.start + (7 - round_up),
> + region.typestr,
> + region.value))
> + bitmap_regions.append(IOPortRegion(
> + region.stop - (region.stop % 8),
> + region.stop,
> + region.typestr,
> + value_tail))
> + else:
> + bitmap_regions.append(IOPortRegion(
> + region.start,
> + region.start + (7 - round_up),
> + region.typestr,
> + value_head))
> + bitmap_regions.append(IOPortRegion(
> + region.start + (7 - round_up) + 1,
> + region.stop,
> + region.typestr,
> + 0))
> +
> + # region head bit is aligned but tail bit not
> + elif region.start % 8 == 0 \
> + and (region.stop % 8 + 1) % 8 != 0:
> + round_down = 7 - (region.stop % 8)
> +
> + if byte_size <= 1:
> + value_head = IOPortRegionTree.get_first_ioport_byte(
> + region.start, region.stop)
> + region.value = value_head
> + else:
> + value_tail = IOPortRegionTree.get_first_ioport_byte(
> + region.stop - (region.stop % 8), region.stop)
> +
> + # adjust the region address to make the region aligned
> + region.stop += round_down
> +
> + # append the region according to the region overlap with the
> + # previous region or not
> + if byte_size <= 1:
> + bitmap_regions.append(region)
> + elif byte_size == 2:
> + bitmap_regions.append(IOPortRegion(
> + region.start,
> + region.start + 7,
> + region.typestr,
> + 0))
> + bitmap_regions.append(IOPortRegion(
> + region.stop - (region.stop % 8),
> + region.stop,
> + region.typestr,
> + value_tail))
> + else:
> + bitmap_regions.append(IOPortRegion(
> + region.start,
> + region.stop - (region.stop % 8) - 1,
> + region.typestr,
> + 0))
> + bitmap_regions.append(IOPortRegion(
> + region.stop - (region.stop % 8),
> + region.stop,
> + region.typestr,
> + value_tail))
> +
> + # region head bit and tail bit are both aligned
> + else:
> + region.value = 0
> + bitmap_regions.append(region)
> +
> + # replenishment the region hole
> + for region in bitmap_regions:
> + i = bitmap_regions.index(region)
> + if region.start != 0 \
> + and region.stop != 0xffff \
> + and region != bitmap_regions[-1]:
> + if region.stop + 1 != bitmap_regions[i + 1].start:
> + size = bitmap_regions[i + 1].start - region.stop - 1
> + value = 0xff
> + if size > 8:
> + value = -1
> + bitmap_regions.insert(i + 1,
> + IOPortRegion(
> + region.stop + 1,
> + bitmap_regions[i + 1].start - 1,
> + '',
> + value))
> +
> + # pad with the 0xff if the last address is not 0xffff
> + if bitmap_regions[-1].stop != 0xffff:
> + start = bitmap_regions[-1].stop + 1
> + stop = 0xffff
> + value = 0xff
> + if stop - start - 1 > 8:
> + value = -1
> + bitmap_regions.append(IOPortRegion(start, stop, '', value))
> +
> + return pm_timer_base, bitmap_regions
Wow, this function is really huge (213 sloc).
After collapsing it, I identified four major parts:
1. get regions
2. Iterate over regions to fill bitmap_regions
3. Iterate over bitmap_regions again (maybe we can consolidate step 2
and 3?)
4. Check last region and pad if necessary
What do you think, wouldn't it be easier to understand if we introduce
additional functions that help to understand the idea behind your code?
>
>
> class MMConfig:
> @@ -1248,7 +1454,8 @@ regions.append(inmatereg)
>
> cpucount = count_cpus()
>
> -pm_timer_base = parse_ioports()
> +(pm_timer_base, pm_bitmap_regions) = parse_ioports()
PEP8: redundant parantheses
> +pm_bitmap_size = int(pm_bitmap_regions[-1].stop/8 + 1)
>
>
> f = open(options.file, 'w')
> @@ -1266,7 +1473,9 @@ kwargs = {
> 'irqchips': ioapics,
> 'pm_timer_base': pm_timer_base,
> 'mmconfig': mmconfig,
> - 'iommu_units': iommu_units
> + 'iommu_units': iommu_units,
> + 'pm_bitmap_size': pm_bitmap_size,
> + 'pm_bitmap_regions': pm_bitmap_regions
> }
>
> f.write(tmpl.render(**kwargs))
>
--
Ralf Ramsauer
PGP: 0x8F10049B
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
