2016-07-02 1:08 GMT+08:00 Ralf Ramsauer <[email protected]>:
>
>
> 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
OK, I will take a try.
>> +
>> + 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?
I am already thinking additional functions, on my way.
>>
>>
>> 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.
--
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.
