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())
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.
