olofk / ipyxact

Python-based IP-XACT parser
MIT License
120 stars 46 forks source link

Fields not having a reset mask are still parsed with a mask of all zeros #41

Open whiteboxdv opened 8 months ago

whiteboxdv commented 8 months ago

The IP-XACT standard allows for an optional mask element inside a field reset element. This is used to define which bits of the field have a known reset value. The absence of the mask element makes this N/A, i.e. it is equivalent to a mask of the same size as the register field and consisting of all 1's.

When the latter situation occurs, the ipyxact parser (v0.3.2) still evaluates the mask as all 0's. This violates the standard and makes impossible to discriminate between a mask of all 0's and an absent mask (i.e. N/A or all 1's).

For instance, consider the ipxact.xml sample below:

<ipxact:component xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:ipxact="http://www.accellera.org/XMLSchema/IPXACT/1685-2014" xsi:schemaLocation="http://www.accellera.org/XMLSchema/IPXACT/1685-2014/index.xsd">
    <ipxact:vendor>some_vendor</ipxact:vendor>
    <ipxact:library>some_library</ipxact:library>
    <ipxact:name>some_component</ipxact:name>
    <ipxact:version>1.0</ipxact:version>
    <ipxact:memoryMaps>
        <ipxact:memoryMap>
            <ipxact:name>some_memory_map</ipxact:name>
            <ipxact:addressBlock>
                <ipxact:name>some_address_block</ipxact:name>
                <ipxact:baseAddress>0x0</ipxact:baseAddress>
                <ipxact:range>1</ipxact:range>
                <ipxact:width>32</ipxact:width>
                <ipxact:register>
                    <ipxact:name>some_register</ipxact:name>
                    <ipxact:addressOffset>0x0</ipxact:addressOffset>
                    <ipxact:size>32</ipxact:size>
                    <ipxact:access>read-write</ipxact:access>
                    <ipxact:field>
                        <ipxact:name>without_mask</ipxact:name>
                        <ipxact:bitOffset>0</ipxact:bitOffset>
                        <ipxact:resets>
                            <ipxact:reset>
                                <ipxact:value>0xff</ipxact:value> <!-- No mask here, i.e. should be N/A or all 1's -->
                            </ipxact:reset>
                        </ipxact:resets>
                        <ipxact:bitWidth>8</ipxact:bitWidth>
                        <ipxact:access>read-write</ipxact:access>
                    </ipxact:field>
                    <ipxact:field>
                        <ipxact:name>with_zero_mask</ipxact:name>
                        <ipxact:bitOffset>8</ipxact:bitOffset>
                        <ipxact:resets>
                            <ipxact:reset>
                                <ipxact:value>0xff</ipxact:value>
                                <ipxact:mask>0x0</ipxact:mask> <!-- This mask is all 0's -->
                            </ipxact:reset>
                        </ipxact:resets>
                        <ipxact:bitWidth>8</ipxact:bitWidth>
                        <ipxact:access>read-write</ipxact:access>
                    </ipxact:field>
                    <ipxact:field>
                        <ipxact:name>with_nonzero_mask</ipxact:name>
                        <ipxact:bitOffset>16</ipxact:bitOffset>
                        <ipxact:resets>
                            <ipxact:reset>
                                <ipxact:value>0xff</ipxact:value>
                                <ipxact:mask>0xff</ipxact:mask> <!-- This mask is all 1's -->
                            </ipxact:reset>
                        </ipxact:resets>
                        <ipxact:bitWidth>8</ipxact:bitWidth>
                        <ipxact:access>read-write</ipxact:access>
                    </ipxact:field>
                </ipxact:register>
            </ipxact:addressBlock>
            <ipxact:addressUnitBits>8</ipxact:addressUnitBits>
        </ipxact:memoryMap>
    </ipxact:memoryMaps>
</ipxact:component>

When parsed:

from ipyxact.ipyxact import Component

component = Component()
component.load("ipxact.xml")

register = component.memoryMaps.memoryMap[0].addressBlock[0].register[0]
fields = register.field
print(f'Printing fields from register {register.name}:')

for f in fields:
    _reset = f.resets.reset.value
    _mask = f.resets.reset.mask
    print(f'    Field {f.name} has reset value of x{_reset:X} and mask of x{_mask:X} --> overall reset value: x{_reset & _mask:X}')

It yields:

Printing fields from register some_register:
    Field without_mask has reset value of xFF and mask of x0 --> overall reset value: x0
    Field with_zero_mask has reset value of xFF and mask of x0 --> overall reset value: x0
    Field with_nonzero_mask has reset value of xFF and mask of xFF --> overall reset value: xFF

As you can see, fields without_mask and with_zero_mask are both evaluated with a mask of all 0's and there's no way to tell the former doesn't have a mask defined in the first place. I believe the above considerations also applies to register resets as per IP-XACT 2009, even though I haven't tested it.

I suspect this behavior is due to:

The preferred behavior for me would be not to create a mask attribute inside the reset class if there's no mask in the IP-XACT, but it may involve a re-structuring on how the parser works.

A sub-optimal solution could be creating another integer type that defaults to all 1's and use it for the mask. However, note that in this case there's no easy way to dynamically adjust the number of bits that should be 1 w.r.t. the field/register size, so a worst-case guess is needed (a bit of an hack).

olofk commented 7 months ago

I see the problem, and we should fix it somehow, but I would like to make sure that the solution works with the new API that was introduced in ipyxact 0.3.0, which doesn't depend on the awkward inline yaml solution that I came up with first.

I tried porting your example to the new API, but I'm not sure I understand what the result is supposed to be and I also don't remember how this parse_uint thing is supposed to work so I just copied the function here. Not sure if it's of any help, but here it is

import ipyxact.ipxact2014 as ipxact

def parse_uint(expr):
    import re

    base = 10

    horrible_regex_hack = re.compile(r"\('h([0-9A-Fa-f]+)\) / \$pow\(2,(\d+)\) % \$pow\(2,(\d+)\)")
    horrible_regex_match = horrible_regex_hack.match(expr)

    if len(expr) > 2 and expr[0:2] == '0x':
        # handle non-standard SystemVerilog (but commonly-used) syntax
        expr = expr[2:]
        base = 16
    #Super-dirty hack to handle a common pattern used by XSLT upgrade transforms
    elif horrible_regex_match:
        _g = horrible_regex_match.groups()
        expr = str(int(int(_g[0], 16) / 2**int(_g[1]) % 2**int(_g[2])))

    elif "'" in expr:
        sep = expr.find("'")
        # ignore any bit size specified before ' because size is handled by other IP-XACT properties
        if expr[sep+1] in ["h", "H"]:
            base = 16
        elif expr[sep+1] in ["d", "D"]:
            base = 10
        elif expr[sep+1] in ["o", "O"]:
            base = 8
        elif expr[sep+1] in ["b", "B"]:
            base = 2
        else:
            raise ValueError("Could not convert expression to an integer: {}".format(self.valueOf_))
        expr = expr[sep+2:]

    return int(expr.replace('_', ''), base)

component = ipxact.parse("ipxact.xml", True)
register = component.memoryMaps.memoryMap[0].addressBlock[0].register[0]
fields = register.field
print(f'Printing fields from register {register.name}:')

for f in fields:
    for reset in f.resets.reset:
        _reset = parse_uint(reset.value.valueOf_)
        if reset.mask:
            _mask = parse_uint(reset.mask.valueOf_)
        else:
            _mask = 0
        print(f'    Field {f.name} has reset value of x{_reset:X} and mask of x{_mask:X} --> overall reset value: x{_reset & _mask:X}')
whiteboxdv commented 6 months ago

Sorry for the delay in response and Happy New Year.

I wasn't aware of the API change but I see this now. For what is worth I didn't personally think the original YAML solution was awkward (actually I found it rather elegant), but I understand the benefit of generating the API directly from the XML schema.

Tried it again using the new API as suggested above:

import ipyxact.ipxact2014 as ipxact

component = ipxact.parse("ipxact.xml", silence=True)

register = component.memoryMaps.memoryMap[0].addressBlock[0].register[0]
fields = register.field
print(f'Printing fields from register {register.name}:')

for f in fields:            
    _reset = ipxact.unsignedPositiveIntExpression.parse_uint(f.resets.reset[0].value)
    _mask = None
    _mask_string = str(_mask)
    _overall_reset = _reset

    if f.resets.reset[0].mask is not None:
        _mask = ipxact.unsignedPositiveIntExpression.parse_uint(f.resets.reset[0].mask)
        _mask_string = f"{_mask:X}"
        _overall_reset &= _mask

    print(f"    Field {f.name} has reset value of x{_reset:X} and mask of x{_mask_string} --> overall reset value: x{_overall_reset:X}")

This yields:

Printing fields from register some_register:
    Field without_mask has reset value of xFF and mask of xNone --> overall reset value: xFF
    Field with_zero_mask has reset value of xFF and mask of x0 --> overall reset value: x0
    Field with_nonzero_mask has reset value of xFF and mask of xFF --> overall reset value: xFF

Which looks correct. The user can check the mask against None and understand if that is non-existing (hence the overall reset is simply the reset field) or existing and equal to some value. You can argue generating the API from the XML schema makes the parser adhering to the standard "by construction".

The problem still remains using the old API. It's up to you whether that is worth fixing (as I was saying above a workaround might involve few changes to ipyxact_yaml.py and ipyxact.py) or it is finally time to deprecate it for good. However, note that there might still be people around using it.

olofk commented 6 months ago

Good to hear it's working with the new API! And for context, I thought my original idea was pretty neat too, but in addition to only covering a small subset of the standard, it also gave rise to some subtle bugs that turned out to be hard to fix. I would happily apply a contributed fix for the old API, as you're probably right that it's still widely used. I don't have the time to work on it myself though.