2016-07-02 1:08 GMT+08:00 Ralf Ramsauer <[email protected]>:
>
> On 07/01/16 05:57, Xuguo Wang wrote:
>> Some comments of code are redundant, since the codes are
>> self-explanatory, so omit them.
> This message explains the changes were made from V4 to V5, but not the
> commit message that should actually appear in the repository.
>>
>> Signed-off-by: Xuguo Wang <[email protected]>
>> ---
>> tools/jailhouse-config-create | 180
>> +++++++++++++++++++++++++++++++++---------
>> 1 file changed, 143 insertions(+), 37 deletions(-)
>>
>> diff --git a/tools/jailhouse-config-create b/tools/jailhouse-config-create
>> index 4e61abc..9fe7f96 100755
>> --- a/tools/jailhouse-config-create
>> +++ b/tools/jailhouse-config-create
>> @@ -368,22 +368,30 @@ class PCIPCIBridge(PCIDevice):
>> return (secondbus, subordinate)
>>
>>
>> -class MemRegion:
>> - def __init__(self, start, stop, typestr, comments=None):
>> +class IORegion:
>> + def __init__(self, start, stop, typestr):
>> self.start = start
>> self.stop = stop
>> self.typestr = typestr
>> +
>> +
>> +# Each IOMemRegion represents one of the /proc/iomem entries.
>> +# It extends IORegion by the typestr property.
> Does it? IORegion already has the 'typestr' member. It extends IORegion
> by the 'comments' variable.
>> +class IOMemRegion(IORegion):
>> + def __init__(self, start, stop, typestr, comments=None):
>> +
>> if comments is None:
>> self.comments = []
>> else:
>> self.comments = comments
> These lines already changed in next, your patch should apply against
> next. Sorry that I didn't mention that earlier, otherwise Jan only gets
> conflicts when merging.
>> + IORegion.__init__(self, start, stop, typestr)
>>
>> def __str__(self):
>> - return 'MemRegion: %08x-%08x : %s' % \
>> + return 'IOMemRegion: %08x-%08x : %s' % \
>> (self.start, self.stop, self.typestr)
>>
>> def size(self):
>> - # round up to full PAGE_SIZE
>> + # round up to PAGE_SIZE
>> return int((self.stop - self.start + 0xfff) / 0x1000) * 0x1000
>>
>> def flagstr(self, p=''):
>> @@ -400,6 +408,19 @@ class MemRegion:
>> return 'JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE'
>>
>>
>> +# Each IOPortRegion represents one of the /proc/ioports entries.
>> +# It extends IORegion typestr priority.
> Really? IOPortRegion extends IORegion by adding the 'value' member variable.
>> +# the value == 0 means permit, 1 means intercept.
>> +class IOPortRegion(IORegion):
>> + def __init__(self, start, stop, typestr, value=0):
>> + self.value = value
>> + IORegion.__init__(self, start, stop, typestr)
>> +
>> + def size(self):
>> + # the range is from start to stop including self.start
>> + return self.stop - self.start + 1
>> +
>> +
>> class IOAPIC:
>> def __init__(self, id, address, gsi_base, iommu=0, bdf=0):
>> self.id = id
>> @@ -416,7 +437,11 @@ class IOAPIC:
>> return (self.iommu << 16) | self.bdf
>>
>>
>> -class IOMemRegionTree:
>> +class IORegionTree:
>> + # This regex matches entries in /proc/ioports, like
> ... in /proc/ioports or /proc/iomem ...
> We use this class and its parse_line static method for both, ioports and
> iomem. Mention /proc/iomem to avoid misunderstandings.
>> + # ' 0020-0021 : pic1'
>> + IOEntry_Regex = re.compile('( *)([0-9a-f]*)-([0-9a-f]*) : (\w?.*)')
>> +
>> def __init__(self, region, level):
>> self.region = region
>> self.level = level
>> @@ -434,6 +459,27 @@ class IOMemRegionTree:
>> s += str(c)
>> return s
>>
>> + @staticmethod
>> + def parse_line(regionclass, line):
>> + match = IORegionTree.IOEntry_Regex.match(line)
>> + if not match:
>> + raise ValueError('invalid entry')
> Let the error message be more verbose:
> 'Invalid entry: %s' % line
>
> But take care, I just realised, that 'line' has a trailing newline
> character at the moment, I have a further comment on how to avoid
> trailing '\n's later.
>> +
>> + # blank level
>> + level = int(match.group(1).count(' ') / 2) + 1
>> + # start address
>> + start = match.group(2)
>> + # end address
>> + stop = match.group(3)
>> + # comments
>> + typestr = match.group(4)
>> + return level, regionclass(int(start, 16), int(stop, 16), typestr)
>> +
>> +
>> +class IOMemRegionTree(IORegionTree):
>> + def __init__(self, region, level):
>> + IORegionTree.__init__(self, region, level)
>> +
>> def regions_split_by_kernel(self):
>> kernel = [x for x in self.children if
>> x.region.typestr.startswith('Kernel ')]
>> @@ -457,38 +503,30 @@ class IOMemRegionTree:
>>
>> # before Kernel if any
>> if (r.start < kernel_start):
>> - before_kernel = MemRegion(r.start, kernel_start - 1, s)
>> + before_kernel = IOMemRegion(r.start, kernel_start - 1, s)
>>
>> - kernel_region = MemRegion(kernel_start, kernel_stop, "Kernel")
>> + kernel_region = IOMemRegion(kernel_start, kernel_stop, "Kernel")
>>
>> # after Kernel if any
>> if (r.stop > kernel_stop):
>> - after_kernel = MemRegion(kernel_stop + 1, r.stop, s)
>> + after_kernel = IOMemRegion(kernel_stop + 1, r.stop, s)
>>
>> return [before_kernel, kernel_region, after_kernel]
>>
>> @staticmethod
>> - def parse_iomem_line(line):
>> - a = line.split(':', 1)
>> - level = int(a[0].count(' ') / 2) + 1
>> - region = a[0].split('-', 1)
>> - a[1] = a[1].strip()
>> - return level, MemRegion(int(region[0], 16), int(region[1], 16),
>> a[1])
>> -
>> - @staticmethod
>> def parse_iomem_file():
>> root = IOMemRegionTree(None, 0)
>> f = input_open('/proc/iomem')
>> lastlevel = 0
>> lastnode = root
>> for line in f:
> for line in f.read().splitlines()
> will avoid trailing newline characters.
>> - (level, r) = IOMemRegionTree.parse_iomem_line(line)
>> + level, r = IOMemRegionTree.parse_line(IOMemRegion, line)
>> t = IOMemRegionTree(r, level)
>> - if (t.level > lastlevel):
>> + if t.level > lastlevel:
>> t.parent = lastnode
>> - if (t.level == lastlevel):
>> + if t.level == lastlevel:
>> t.parent = lastnode.parent
>> - if (t.level < lastlevel):
>> + if t.level < lastlevel:
> Why do you start to fix PEP8 issues of old code fragments? That's not
> part of your actual commit. I think we should move non-functional
> cosmetic changes of existing code to separate commits.
>
> Nevertheless - I'm using the pycharm IDE, and it complains that on
> master(!!) there are
> 30 warnings
> 95 weak warnings
> 242 typos
> :-D
>
> Most of those warnings are redundant parentheses and in deed, the
> absolute number of PEP8 warning decreases after applying your three
> patches (though they introduce new ones as well). Anyway, we probably
> should move PEP8, cosmetic changes and syntactic sugar to separate
> commits and then fix it all at once and not just pick out some parts.
>> p = lastnode.parent
>> while(t.level < p.level):
> Like this one...
>> p = p.parent
>> @@ -510,11 +548,11 @@ class IOMemRegionTree:
>> r = tree.region
>> s = r.typestr
>>
>> - if (s.find('HPET') >= 0):
>> + if s.find('HPET') >= 0:
>> regions.append(r)
>>
>> # if the tree continues recurse further down ...
>> - if (len(tree.children) > 0):
>> + if len(tree.children) > 0:
>> regions.extend(IOMemRegionTree.find_hpet_regions(tree))
>>
>> return regions
>> @@ -543,12 +581,12 @@ class IOMemRegionTree:
>> continue
>>
>> # generally blacklisted, unless we find an HPET behind it
>> - if (s == 'reserved'):
>> + if s == 'reserved':
>> regions.extend(IOMemRegionTree.find_hpet_regions(tree))
>> continue
>>
>> # if the tree continues recurse further down ...
>> - if (len(tree.children) > 0):
>> + if len(tree.children) > 0:
>> regions.extend(IOMemRegionTree.parse_iomem_tree(tree))
>> continue
>>
>> @@ -558,6 +596,72 @@ class IOMemRegionTree:
>> return regions
>>
>>
>> +class IOPortRegionTree(IORegionTree):
>> + def __init__(self, region, level):
>> + IORegionTree.__init__(self, region, level)
>> +
>> + @staticmethod
>> + def parse_ioport_file():
>> + root = IOPortRegionTree(None, 0)
>> + f = input_open('/proc/ioports')
>> + lastlevel = 0
>> + lastnode = root
>> + for line in f:
> Same here, use f.read().splitlines() to get rid of trailing \n's.
>> + level, r = IOPortRegionTree.parse_line(IOPortRegion, line)
>> + t = IOPortRegionTree(r, level)
>> + if t.level > lastlevel:
>> + t.parent = lastnode
>> + if t.level == lastlevel:
>> + t.parent = lastnode.parent
>> + if t.level < lastlevel:
>> + p = lastnode.parent
>> + while(t.level < p.level):
> Here you even introduce new PEP8 warnings
>> + p = p.parent
>> + t.parent = p.parent
>> +
>> + t.parent.children.append(t)
>> + lastnode = t
>> + lastlevel = t.level
>> + f.close()
>> +
>> + return root
>> +
>> + # recurse down the tree
>> + @staticmethod
> I don't understand why this method has to be static. There are more
> (syntactic) advantages if it is not static:
>
> You're calling this static function from two different locations: from
> parse_ioports() and recursively from parse_ioports_tree. In any case,
> you already have an IOPortRegionTree object. Why not using it?
>> + def parse_ioport_tree(tree):
> s/tree/self/
>> + regions = []
>> +
>> + for tree in tree.children:
>> + r = tree.region
>> + c = r.typestr
>> +
>> + # intercept keyboard and PCI conf1
>> + if (c is not None and (c == 'keyboard' or c == 'PCI conf1')):
> Here again, new pep8 warnings...
> Even if there are already tons of pep8 issues in current code, try to
> not introduce new ones :-)
>> + r.value = IOPortRegionTree.get_first_ioport_byte(
>> + r.start, r.stop)
>> +
>> + # if the tree continues recurse further down ...
>> + if len(tree.children) > 0:
>> + regions.extend(IOPortRegionTree.parse_ioport_tree(tree))
> And here you get the reward of un-staticfying the function:
> regions.extend(tree.parse_ioport_tree())
What do you mean of un-staticfying the function? I can't understand
what you say..
>
> There's a follow-up comment on this in 2/3
>> + continue
>> +
>> + # add all remaining leaves
>> + regions.append(r)
>> +
>> + return regions
>> +
>> + # get a byte of ioport region mask from start_permitted, end
>> + # at the last_permitted bit.
>> + @staticmethod
>> + def get_first_ioport_byte(start_permitted, last_permitted):
>> + byte_head = start_permitted % 8
>> + byte_tail = last_permitted % 8
>> + byte = (1 << byte_head) - 1
>> + byte |= (~((1 << (byte_tail + 1)) - 1) & 0xff)
>> + byte &= 0xff
>> + return int(byte)
>> +
>> +
>> class IOMMUConfig(object):
>> def __init__(self, props):
>> self.base_addr = props['base_addr']
>> @@ -577,7 +681,7 @@ def parse_iomem(pcidevices):
>> regions = IOMemRegionTree.parse_iomem_tree(
>> IOMemRegionTree.parse_iomem_file())
>>
>> - rom_region = MemRegion(0xc0000, 0xdffff, 'ROMs')
>> + rom_region = IOMemRegion(0xc0000, 0xdffff, 'ROMs')
>> add_rom_region = False
>>
>> ret = []
>> @@ -588,12 +692,12 @@ def parse_iomem(pcidevices):
>> for d in pcidevices:
>> if d.msix_address >= r.start and d.msix_address <= r.stop:
>> if d.msix_address > r.start:
>> - head_r = MemRegion(r.start, d.msix_address - 1,
>> - r.typestr, r.comments)
>> + head_r = IOMemRegion(r.start, d.msix_address - 1,
>> + r.typestr, r.comments)
>> ret.append(head_r)
>> if d.msix_address + d.msix_region_size < r.stop:
>> - tail_r = MemRegion(d.msix_address + d.msix_region_size,
>> - r.stop, r.typestr, r.comments)
>> + tail_r = IOMemRegion(d.msix_address +
>> d.msix_region_size,
>> + r.stop, r.typestr, r.comments)
>> ret.append(tail_r)
>> append_r = False
>> break
>> @@ -667,11 +771,12 @@ def alloc_mem(regions, size):
>> r.stop + 1 >= mem[0] + mem[1]
>> ):
>> if r.start < mem[0]:
>> - head_r = MemRegion(r.start, mem[0] - 1, r.typestr,
>> r.comments)
>> + head_r = IOMemRegion(r.start, mem[0] - 1,
>> + r.typestr, r.comments)
>> regions.insert(regions.index(r), head_r)
>> if r.stop + 1 > mem[0] + mem[1]:
>> - tail_r = MemRegion(mem[0] + mem[1], r.stop, r.typestr,
>> - r.comments)
>> + tail_r = IOMemRegion(mem[0] + mem[1], r.stop,
>> + r.typestr, r.comments)
>> regions.insert(regions.index(r), tail_r)
>> regions.remove(r)
>> return mem
>> @@ -826,7 +931,7 @@ def parse_dmar(pcidevices, ioapics, dmar_regions):
>> comments.append('DMAR parser could not decode device
>> path')
>> offset += scope_len
>>
>> - reg = MemRegion(base, limit, 'ACPI DMAR RMRR', comments)
>> + reg = IOMemRegion(base, limit, 'ACPI DMAR RMRR', comments)
>> regions.append(reg)
>>
>> f.seek(struct_len - offset, os.SEEK_CUR)
>> @@ -1003,7 +1108,8 @@ def parse_ivrs(pcidevices, ioapics):
>> 'regions. The memory at 0x%x will be mapped
>> accessible '
>> 'to all devices.' % mem_addr)
>>
>> - regions.append(MemRegion(mem_addr, mem_len, 'ACPI IVRS',
>> comment))
>> + regions.append(IOMemRegion(mem_addr, mem_len,
>> + 'ACPI IVRS', comment))
>> elif type == 0x40:
>> raise RuntimeError(
>> 'You board uses IVRS Rev. 2 feature Jailhouse doesn\'t '
>> @@ -1135,9 +1241,9 @@ elif (total > ourmem[1]):
>>
>> hvmem[0] = ourmem[0]
>>
>> -inmatereg = MemRegion(ourmem[0] + hvmem[1],
>> - ourmem[0] + hvmem[1] + inmatemem - 1,
>> - 'JAILHOUSE Inmate Memory')
>> +inmatereg = IOMemRegion(ourmem[0] + hvmem[1],
>> + ourmem[0] + hvmem[1] + inmatemem - 1,
>> + 'JAILHOUSE Inmate Memory')
>> regions.append(inmatereg)
>>
>> cpucount = count_cpus()
>>
>
> --
> 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.
