On 07/03/2016 03:31 AM, charles king wrote: > 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.. This change is necessary if you remove the @staticmethod of parse_ioport_tree(). > >> >> 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
-- Ralf Ramsauer GPG: 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.
