[3/3] Support up to N external memory models

[umarcor]

NOT READY FOR MERGE

Ref #465. Close #462.

This PR allows to use integer_vector_access_t (memory buffers) defined in external C functions, along with the default internal model. The API for the testbench is as follows:

• In the python script:
  • Do nothing, to use extmemory_pkg-novhpi.vhd, which declares read_byte and write_byte in VHDL, so no C files are required and cannot be used.
  • Use ui.set_objects([c_obj]) to add pre-built object and to automatically use extmemory_pkg-vhpi.vhd, which declares read_byte and write_byte as external. C definitions of the functions/procedures is required. The definition can be empty if external memory models are not to be used.
• In the VHDL tb:
  • constant memory : memory_t := new_memory;: default memory model.
  • constant memory : memory_t := new_memory(n, length);: where n is any integer /=0, will use the external memory identified by the number (n).
    • If n<0 calls to the external memory model are executed through VHPI functions read_byte and write_byte.
    • If n>0, a foreign array is mapped to a VHDL access type through function get_addr. [NOT IMPLEMENTED YET].

NOTE: It is compulsory to provide argument length (in bytes), when an extermal memory model is used.

The API for the C sources is:

void write_byte ( uint8_t id, uint32_t i, uint8_t v );
uint8_t read_byte ( uint8_t id, uint32_t i );
uintptr_t get_addr ( uint8_t id );

Issues with the current implementation:

• When a function/procedure is decorated with attribute foreign it is compulsory to provide a definition in C, either if only the default internal model is to be used (see ghdl/ghdl#793). Therefore, currently a stubs.c file is used unless the user provides a custom implementation.
  • It is not acceptable for all the designs to require a stubs.c file, which will be useless for most of the users. Moreover, this is currently only supported by GHDL (and not for mcode backend).
  • An alternative is to conditionally comment the two lines that declare the functions/procedures as foreign. @kraigher, what do you think? Should I move those two functions/procedures to a separate package to make it easier to change it? Or is it possible to process the file at compile time?

Currently, the usage of VHPI is a project level option, i.e. all the tests will require a C definition file if a single one requires it, even if external memory models are not used in others (see ghdl/ghdl#793). Ideally, it would be possible to compile VUnit without VHPI, execute the tests that do not require VHPI; then compile again with VHPI and execute the tests that do require VHPI. All of it in the same python script.

• ATM, this PR changes axi_dma so the tests can only be executed on simulators with VHPI support. It would be desirable to provide two sets of test runs, one without VHPI and another one with VHPI.

When an external memory is used, data is already available before anything is written in the tesbench. The length is fixed for now, and explicitly set. By the same token, permissions are unset/non-existent. This produces multiple errors in check_address. Therefore, some of the checks are skipped for external memories. They are still used for the default memory model, in case multiple models are used at the same time.

deallocate, reallocate and resize are not implemented yet.

There are two types of external memory models. To implement the case when n>0, where the base address of an external buffer is assigned to an access type in VHDL, integer_vector would need to be of type array (natural range <>) of character;. However, it is of type integer.

[umarcor]

@kraigher, I think this is ready for a preliminary review.

[kraigher]

Ok I will try to have a look this weekend. You have kept me busy discussion your other enhancements.

[umarcor]

Ok I will try to have a look this weekend. You have kept me busy discussion your other enhancements.

It's ok. Nonetheless, this is not ready for merge. I'd just be glad to hear any thoughts about where I am heading.

[kraigher]

It seems you have reformatted many files making it harder to review.

[kraigher]

Also I do not think the integer_vector_ptr should be changed at all. It it has nothing to do with the memory model except that it is being used as a storage medium.

[kraigher]

The integer_vector_ptr is not even used to store bytes but integers. It is a general purpose data structure that has no benefit of reading or writing bytes in an external object file. It is in the memory model that this flexibility must be added.

[kraigher]

In the simplest case just add a natural range 0 to 1 to the memory_t where 0 means use integer_vector_ptr and 1 means use external read_byte/write_byte calls.

[umarcor]

It seems you have reformatted many files making it harder to review.

You can check the first 11 commits: https://github.com/VUnit/vunit/pull/470/files/b6bca4993ad78c66618767f76a60739372b3e086. That's before I switched to integer_vector_ptr and before I switched the format of function signatures.

Also I do not think the integer_vector_ptr should be changed at all. It it has nothing to do with the memory model except that it is being used as a storage medium.

The memory model is exclusive to verification components. After I implemented it for the memory model only (first 11 commits), I realized that:

• Most of the changes did only affect p_data, and the procedures/functions defined for it.
• Having an externally defined array can be useful to share data between the testbench and the software app, without requiring VCs.
• VCs require to use VHDL2008, but the external feature is possible with earlier versions of the standard. Implementing it in integer_vector_ptr allows to use the feature with VHDL 1993 too.

With my latest proposal, integer_vector_ptr can be used in six different contexts:

• Default memory model.
• Memory model bind to helper functions defined in C.
• Memory model bind to an array defined in C, through an VHDL access type.
• General purpose internal array.
• General purpose array bind to helper functions.
• General purpose array bind to an array defined in C, through an VHDL access type.

Helper C functions (read_byte, write_byte) are useful to implement communication with external processes, be it through pipes, sockets, gRPC, etc. Binding the array through an access type reduces the overhead, as assignments and reads are direct.

For example, in #465 an array is defined in C and pkg_c.vhd is used to bind helper functions. Since VCs are AXI Streams, no memory model is used explicitly in the testbench. However, the code in pkg_c.vhd is equivalent to extmem_pkg (renamed to ext_pkg in the latest commits). So, implementing the external feature in integer_vector_ptr allows the user not to provide pkg_c, but to use new_integer_vector_ptr directly.

The integer_vector_ptr is not even used to store bytes but integers. It is a general purpose data structure that has no benefit of reading or writing bytes in an external object file. It is in the memory model that this flexibility must be added.

Yet, I did not see where in the memory model is a array of bytes/characters defined. It seems that a integer type is used to store data. This is because memory.p_data is of type integer_vector_ptr.

In the simplest case just add a natural range 0 to 1 to the memory_t where 0 means use integer_vector_ptr and 1 means use external read_byte/write_byte calls.

That was the first approach. But instead of a natural range 0 to 1, I used field p_id of type integer. -1 meant 'use integer_vector_ptr', and /=-1 meant 'use external read_byte/write_byte' calls. Note that:

• A boolean field is not enough. We need to pass a parameter to the C functions to tell which memory (among multiple possibly mapped buffers) we want to access, along with the address and the value.
  • Now, I use a integer type to represent three models: default (zero), external through funcs (negative) and external through access (positive). It is possible to use multiple positive models, multiple negative models and a single default model at the same time.
• The negative external model (calling external read_byte and write_byte) is the most general, but not the best approach. If the externally defined buffer is indeed a C buffer, it is possible to bind it to an VHDL type explicitly. This allows VUnit to read/write as if it was a 'local' or 'shared' variable, i.e., without calling any external function.
  • This approach was used in the first 11 commits of #465: https://github.com/VUnit/vunit/pull/465/files/35a7940d9217d3d04ca33fda4e0713bdf4efed2f. I changed it in the last commit to make it look closer to the implementation in this PR: https://github.com/VUnit/vunit/pull/465/commits/f4049357acdbdf144dc9452b32fb5312075a82fa. I think that checking the last commit in that PR is helpful to understand the differences between both approaches: https://github.com/VUnit/vunit/pull/465/commits/f4049357acdbdf144dc9452b32fb5312075a82fa

[kraigher]

The reason the memory_t uses one integer per byte is that is stores permission flags and expected value along with the data byte as a packed integer word.

I think you are trying to solve to many problems at once here. Sure I agree that it could be interesting to have a general purpose dynamic length integer array that can inter-operate with an C-object. But that is a different feature than having an external memory model. Just substituting the integer vector in the memory_t with an external vector would not provide an external memory model. I say this because I assume the semantics of an external memory model would be to read and write bytes whereas the memory_t stores packed integers to represent permission flags and expected data in addition to the data bytes.

If this approach should be feasible the memory_t would need to store the data bytes separately in a byte array. In this case maybe this array could be shared with a C-object and could be considered to be an external memory model. So in this case the first task becomes to add a dynamic byte array data type to VUnit that can have an external implementation in a C-object. It is easier to review if such a task is done in isolation as a separate PR.

So in summary I can agree that having a general purpose approach with external data arrays could be elegant I think it needs to be done on the byte level and not on the integer level to work with the memory model.

[umarcor]

The reason the memory_t uses one integer per byte is that is stores permission flags and expected value along with the data byte as a packed integer word.

Then, I assume that this PR only works accidentaly. In the axi_dma example, the external memory is used but all the data is set/read from VUnit, so I didn't realize that the C part would see encoded content.

I think you are trying to solve to many problems at once here. Sure I agree that it could be interesting to have a general purpose dynamic length integer array that can inter-operate with an C-object. But that is a different feature than having an external memory model. Just substituting the integer vector in the memory_t with an external vector would not provide an external memory model.

This was a misunderstanding on my side. I thought that bytes were being saved in an array of integers in order to avoid type conversions to/from characters; and permissions and other metainfo were saved in p_meta. Thanks for the clarification.

Regarding a general purpose dynamic length integer array that can inter-operate with an C-object, the content in #476 is mostly a copy of the extension to the vector of integers in this PR:

• Do you want me to add both enhancements (external byte vector and external integer vector) to #476 and keep this PR to later implement the memory model?
• Should I open a separate PR fo external integer vectors?

I wish it was possible to use the same logic to manipulate external arrays of integers, characters or even custom record types. But I am afraid this is not possible with VHDL 1993. What about VHDL 2008 or 2019?

I say this because I assume the semantics of an external memory model would be to read and write bytes whereas the memory_t stores packed integers to represent permission flags and expected data in addition to the data bytes.

Although my first approach was to only support reading/writing bytes (because I thought that the internal memory model would use an array of bytes); after understanding the context, I think that we can support four 'models':

• Data and metadata in VHDL.
• Data in an external byte array and metadata in VHDL.
• Data in VHDL and metadata in an external integer array.
• Data and metada in an external integer array.

The first and the last one are implemented already. The second and the third will be possible after #476, as you suggested. I think that the implementation order should be 1, 2, 4, 3.