register/mmio framework: add upper-bounds checks

[nfeske]

The Register respectively Mmio API has become predominant in Genode's drivers and the type-safe access to hardware registers has become a second nature. However, one point of unsafety remains. The Mmio constructor merely takes the base address of the locally mapped memory-mapped I/O range as argument, but not an upper bound. All Register definitions are assumed to comply with the size of the MMIO region. I'd argue that this assumption is too optimistic. As a safeguard we should better add a range check against the upper bound whenever a register is accessed.

There are two principle ways of implementing such a check.

• We could add a check a each register access. This would be almost transparent at the API level but introduce one conditional branch for each access. I'd be fine with it because I think of Mmio accesses to be rather slow anyway (compared to CPU caches).

• We could turn Mmio into a template that takes an expected minimum size as template argument. Whenever a Register is used, a static assert could check the Register offset + size against the Mmio template argument. So the individual range checks happen at compile time only. Only at the construction time of an Mmio object, the static Mmio size would be compared against the (dynamic) constructor argument. So there is only one single check at runtime and this check fails early on, which presumably make error diagnosis easy. The downside of this approach is that the change of Mmio to a template is a more invasive API change, compared to the first option.

The open question is: What should happen whenever a range violation occurs? I think that an error message, followed by a Mmio::Range_violation exception is the way to go. That is, I would not encourage programmers to handle such errors defensively but instead see the violation as an implementation bug (like the one I recently re-introduced but spotted by @alex-ab in https://github.com/genodelabs/genode/issues/2726#issuecomment-820227605). I'd take an error-message + abort over silent memory corruption any day.

While we are at it, how about changing Mmio constructor argument from addr_t to void * such that users of the class don't need to cast the return value of dataspace.local_addr<char>() during construction?

@m-stein Would you be interested in implementing this idea, along with a test?

[m-stein]

@nfeske I think the range check is a good idea. And I could do it like next week. I think, it shouldn't consume more than 5 days. I'd vote for the second approach. The offset and the size of registers is a compile-time thing and so should be their validation check as I see it. Like you, I think the check runtime size >= compiletime min-size should cause an error and an exception and should not be handled but fixed.

If you think the use of (void *) is benificial I can change it but I'd rather try to avoid pointers and reflect with addr_t that the value is used in arithmetics.

[nfeske]

Cool! Thanks @m-stein for taking care of this topic.

I share your allergy against raw pointers and try to avoid them whenever possible. In this case, however, the argument is a pointer. In each instance where Mmio is used, we cast a local pointer to an addr_t. Then, inside the register framework, we cast addr_t back to a pointer to de-reference it. Disguising it as an addr_t does not make it any less a pointer. :-) There is no safety gain by dressing the raw pointer in an innocent outfit.

Note that I'm completely fine with using addr_t internally inside the register framework. I'm only arguing about the publicly visible Mmio constructor.

BTW, the new generation of the Platform::Device API (discussed and implemented at https://github.com/genodelabs/genode/issues/4075#issuecomment-820215140) completely eliminates the raw pointers from the API surface used by typical drivers while leveraging the convenience of the Mmio API.

[m-stein]

@nfeske I've created a small proposal commit (958e6da665) that illustrates how I would change MMIO, attached MMIO, and register sets. I've only concentrated on two use cases...

build/arm_v7a$ make timer KERNEL=sel4 BOARD=imx6q_sabrelite
build/x86_64$ make run/log KERNEL=hw BOARD=pc

in order to demonstrate the use of the adapted tools.

I've added all three checks (static, constructor, access) as I think there are places where the correct MMIO size can be determined only dynamically and the static value is merely an upper bound.

What do you think?

[nfeske]

Thank you for the proposal, which is almost exactly what I had in mind. In the following, there just a few nitpicks.

I wonder, could we live without the runtime checks in the _read and _write functions when we have the static assertions in place? It would be nice to avoid the checks and error calls.

To go with the times, I'd define MAX_ITEM_BIT_OFF and MAX_ACCESS_OFF as static constexpr instead of enum values. This way, the values get a proper type. "Offset" should better not be abbreviated. (could the overly long name potentially be avoided by speaking of a LAST_BIT instead?)

static constexpr size_t MAX_ITEM_BIT_OFFSET = ... ;

This would also apply for Genode::Mmio::MMIO_SIZE, which I'd call SIZE because it is already scoped. So it's better to name the template argument MMIO_SIZE and the hosted value SIZE.

template <Genode::size_t MMIO_SIZE>
struct Genode::Mmio : ...
{
   static constexpr size_t SIZE = MMIO_SIZE;
   ...
};

The error message "dynamically set size of MMIO type exceeds its statically declared size" is too long. Imagine this message appearing in a Genode log. How about "MMIO range is unexpectedly too small"?

[nfeske]

As another minor remark, I would move the exception to Mmio::Range_violation.

[m-stein]

@nfeske Does fixup cbf9670bb3 meet your expectations?

[nfeske]

Does fixup https://github.com/genodelabs/genode/commit/cbf9670bb34ad139214a9c8a4d64b4fb84c65244 meet your expectations?

Thank you for the adjustments. That looks excellent, except for one tiny bit (quite literally):

When speaking of LAST_BIT I would expect this to mean the last bit, not the bit after the last bit. If I understand the code correctly, right now, MAX_ACCESS_OFFSET actually denotes the first offset after the valid range, which means that the static_assert one line below is off by one (using <= instead of <).

Generally, I find it most intuitive and robust to speak memory ranges as inclusive (first...last where last lies within the range). BTW, this also has the benefit of anticipating ranges at the very end of the addressable integer space (where an exclusively defined upper bound wound wrap to zero.

Hence, it would make sense to also speak of the last_access_offset (as the inclusive last legitimate access, not the first illegitimate one). Then the static_assert would be correct when using <=.

[m-stein]

@nfeske LAST_BIT is the first bit of the last array item and MAX_ACCESS_OFFSET the first byte of the last valid access integer. Say the array is located at offset 0x1000, is of 80 bits width (10 bytes), has 10 items of 8 bits width each, and is accessed via uint32_t. Then, MAX_INDEX is 9, ITEM_WIDTH_LOG2 is 3, LAST_BIT is 72, MAX_ACCESS_OFFSET is 8. So, the condition is static_assert(0x1000 + 8 + 4 <= REGISTER_SET_SIZE). Note that not the logical region of the array [0x1000..0x100a) is of interest but the region that is actually accessed [0x1000..0x100c).

I've adapted the naming in 2a8dfe5d06.

[chelmuth]

Is it correct that your compile-time check mimics dst(MAX_INDEX) + ACCESS_WIDTH <= REGISTER_SET_SIZE?

[m-stein]

@chelmuth That's correct.

[m-stein]

Well, actually not completely. ACCESS_WIDTH is in bits but the check needs bytes: dst(MAX_INDEX) + sizeof(access_t) <= REGISTER_SET_SIZE.

[nfeske]

LAST_BIT is the first bit [...]

The start of this sentence indicates to me that the terminology is not good yet because it defeats intuition.

What I learned from our conversation: For the static_assert, we must talk about the last legitimate access offset (at byte granularity), which depends on the access type and the size of the MMIO range. Talking about bits is distracting for the purpose of the static assert.

Hence, I would turn the existing dst function into a constexpr function like this (just a sketch, not tested):

struct Dst { off_t offset; uint8_t shift; };

static constexpr Dst _dst(unsigned long index)
{
    off_t   bit_offset  = off_t(index << ITEM_WIDTH_LOG2);
    off_t   byte_offset = bit_offset >> BYTE_WIDTH_LOG2;
    off_t   offset      = align_addr(byte_offset, ACCESS_WIDTH);
    uint8_t shift       = uint8_t(bit_offset - (offset << BYTE_WIDTH_LOG2));
    offset += OFFSET;

    return { .offset = offset, .shift = shift };
}

static_assert(_dst(MAX_INDEX).offset + sizeof(access_t) <= REGISTER_SET_SIZE);

This way, the static assertion become rather intuitive.

Please don't take what I've written above as definite. The only thing that's definite is that we must strive for more clarity. The exact terminology doesn't need to follow my concrete suggestions as long as it the resulting code explains itself well.

[m-stein]

@nfeske I think this is a good idea. I had to replace ACCESS_WIDTH as it's in bits and the function align_addr as it rounds up instead of down which is required here. I refrained from using off_t offset = byte_offset & _align_mask(sizeof(access_t)) as the function has a leading underscore. This is the patch: 562737e0c2.

[nfeske]

The patch looks good to me.

[chelmuth]

Please don't miss to adapt the documentation of Mmio() and Mmio_plain_access().

[m-stein]

@chelmuth Thanks for the hint (4794a6e1f6)!

[m-stein]

So, I'll start adapting the rest of the drivers.

[m-stein]

I've grep'd all genodelabs repos for Mmio|Register_set|Attached_mmio and made the results build again. For now, I've created separate commits for the build targets, so, the changes can be reviewed easier by original authors (will be merged together later). I would like to encourage the original authors also to review the "remove deprecated ..." commits as I made these changes based only on unsuccessful greps for a use context of the corresponding file.

Here are links to the topic branches:

https://github.com/m-stein/genode/commits/mmio_size_check https://github.com/m-stein/genode-allwinner/commits/mmio_size_check https://github.com/m-stein/genode-imx/commits/mmio_size_check https://github.com/m-stein/genode-riscv/commits/mmio_size_check https://github.com/m-stein/genode-rpi/commits/mmio_size_check https://github.com/m-stein/genode-world/commits/mmio_size_check https://github.com/m-stein/genode-zynq/commits/mmio_size_check

There are some things I'd like to add:

• I had to temporarily disable a static assert in c1ddfe8233 in order to be able to proceed. @atopia is working on a solution for that.
• Platform::Device::Mmio is often used without actually using the Genode::Mmio part of it. These places can be found by grep'ing for Mmio<0>. A typical pattern is to declare other types struct X : Genode::Mmio based on the range that gets mapped by an Platform::Device::Mmio object, sometimes even with the exact same range.
• AFAICT, Platform::Device_interface::Range could be replaced by Byte_range_ptr.
• AFAICT, it has become unnecessary that Platform::Device::Mmio inherits from Range as its Genode::Mmio part now stores the same range, available via range().
• Mmio_big_endian_access in server/vmm/fdt.cc and Mmio_plain_access util/mmio.h are almost the same, but I don't know if it is worth working on this.
• In the "(core|bootstrap)" commits, I refrained from trying to use Mmio sizes based on mappings or specs. I normally do Mmio({(char *)base, Mmio::SIZE}), unless a "realistic" size was already defined somewhere near.

[nfeske]

Thank you @m-stein for the implementation and the thorough adjustments. I very much welcome the changes. After browsing the adaptations, I can now clearly see the benefit of the range_at you introduced.

My minor (inline) remarks hint at some further opportunities to avoid casts. But those remarks are not pressing. From my point of view, I'd be fine with merging the changes to staging (and see possible effects on the CI).

Platform::Device_interface::Range could be replaced by Byte_range_ptr.

Yes. That was actually my intention all along when we introduced the Byte_range_ptr (the Platform::Device_interface::Range is older). It would be a natural subsequent step.

it has become unnecessary that Platform::Device::Mmio inherits from Range as its Genode::Mmio part now stores the same range, available via range().

I agree.

[m-stein]

@nfeske Thanks for the feedback! I'll leave the topics Device_interface::Range, Device::Mmio<0>, and Mmio_big_endian untreated for now as far as I can tell from your comment.

[m-stein]

I've added a fix-up 92450b3721 that slightly adapts several targets according to the pointers given by @nfeske.

[atopia]

@m-stein I just pushed https://github.com/atopia/genode/commit/5703f49249244942568d767beff16d0bc7dff7bb as a replacement for https://github.com/genodelabs/genode/commit/c1ddfe8233f8cb0b4c8e8f530266cab2fd82066b. Removing the Genode::uint64_t _reserved[1]; in line 80 of repos/base-hw/src/core/spec/x86_64/virtualization/svm.h should (together with the new Mmio size) satisfy the static_assert.

[skalk]

I've read the commit series regarding genode's main repository, and have no objections. I'm not completely convinced that the Byte_range_ptr is really useful within the Vmm::Ram class' API, but I can live with it. Thank you @m-stein for this big renovation work!

[m-stein]

@atopia Thank you for your contribution! However, when I cherry-pick 5703f49249 on my mmio_check_size branch and remove https://github.com/genodelabs/genode/commit/c1ddfe8233f8cb0b4c8e8f530266cab2fd82066b, the assertion still fails:

Board::Vmcb_control_area::total_size = 1024 sizeof(Board::Vmcb_control_area) = 992 sizeof(Genode::Mmio<0>) = 24 sizeof(Board::Vmcb_reserved_for_host) = 16 sizeof(Board::Vmcb_control_area) + sizeof(Genode::Mmio<0>) + sizeof(Board::Vmcb_reserved_for_host) = 1032

Do I miss something?

[m-stein]

@skalk Thanks for having reviewed my work! I see your point regarding Vmm::Ram. I didn't introduce Byte_range_ptr right at the beginning but only when I figured that I had problems understanding how to adapt the code that uses Vmm::Ram. However, I'd be fine with reverting that part as soon as the commits are through the nightly tests.

[atopia]

Do I miss something?

Yes, I crafted the patch so that it will pass tests standalone, i.e. in order to use it with the new Mmio framework you will need to remove the _reserved field in Vmcb_reserved_for_host so that its size is 8 bytes instead of 16. Sorry for the inconvenience, I wasn't sure if anyone wanted to give my patch a spin first, that's why I didn't do it for you.

[m-stein]

@atopia Thanks for the clarification! I've updated my mmio_size_check branch accordingly.

[jschlatow]

@m-stein Your changes in genode-zynq are flawless. I actually validated that your changes would fly on the Zybo Z7 board. I also appreciate the removal of deprecated includes.

[m-stein]

@jschlatow Thanks for having reviewed and even tested my commits! They should soon enter staging.

[nfeske]

Thanks @m-stein for the squashed form of the commits. I merged these from your respective merge_to_staging branches to the staging branches now.

[chelmuth]

@m-stein There are some depot build errors with current staging.

genodelabs/bin/x86_32/acpi_drv

/data/depot/checker/src/acpi_drv/2024-01-30/src/drivers/acpi/spec/x86/../../acpi.cc: In member function ‘void Ivrs::parse(Genode::Allocator&)’:
/data/depot/checker/src/acpi_drv/2024-01-30/src/drivers/acpi/spec/x86/../../acpi.cc:292:31: error: comparison of integer expressions of different signedness: ‘Genode::off_t’ {aka ‘long int’} 
  292 |                 while (offset < read<Ivrs::Length>()) {
      |                        ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~

genodelabs/bin/x86_32/pci_decode

/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc: In lambda function:
/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc:97:53: error: no matching function for call to ‘Pci::Config_type1::Config_type1(Genode::addr_t)’
   97 |                         Config_type1 bcfg(cfg.base());
      |                                                     ^
In file included from /data/depot/checker/api/os/2024-01-30/include/pci/config.h:20,
                 from /data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc:25:
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:9: note: candidate: ‘Genode::Mmio<<anonymous> >::Mmio(const Genode::Byte_range_ptr&) [with long unsigned int MMIO_SIZE = 69]’
   94 |         Mmio(Byte_range_ptr const &range)
      |         ^~~~
/data/depot/checker/api/os/2024-01-30/include/pci/config.h:715:28: note:   inherited here
  715 |         using Pci::Config::Config;
      |                            ^~~~~~
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:36: note:   no known conversion for argument 1 from ‘Genode::addr_t’ {aka ‘long unsigned int’} to ‘const Genode::Byte_range_ptr&
   94 |         Mmio(Byte_range_ptr const &range)
      |              ~~~~~~~~~~~~~~~~~~~~~~^~~~~
/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc: In lambda function:
/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc:145:51: error: no matching function for call to ‘Pci::Config_type0::Config_type0(Genode::addr_t)’
  145 |                                 C0 cfg0(cfg.base());
      |                                                   ^
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:9: note: candidate: ‘Genode::Mmio<<anonymous> >::Mmio(const Genode::Byte_range_ptr&) [with long unsigned int MMIO_SIZE = 69]’
   94 |         Mmio(Byte_range_ptr const &range)
      |         ^~~~
/data/depot/checker/api/os/2024-01-30/include/pci/config.h:676:28: note:   inherited here
  676 |         using Pci::Config::Config;
      |                            ^~~~~~
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:36: note:   no known conversion for argument 1 from ‘Genode::addr_t’ {aka ‘long unsigned int’} to ‘const Genode::Byte_range_ptr&
   94 |         Mmio(Byte_range_ptr const &range)
      |              ~~~~~~~~~~~~~~~~~~~~~~^~~~~
/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc: In lambda function:
/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc:299:39: error: no matching function for call to ‘Pci::Config::Config(Genode::addr_t&)’
  299 |                 Config cfg(config_base);
      |                                       ^
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:9: note: candidate: ‘Genode::Mmio<<anonymous> >::Mmio(const Genode::Byte_range_ptr&) [with long unsigned int MMIO_SIZE = 69]’
   94 |         Mmio(Byte_range_ptr const &range)
      |         ^~~~
/data/depot/checker/api/os/2024-01-30/include/pci/config.h:617:21: note:   inherited here
  617 |         using Mmio::Mmio;
      |                     ^~~~
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:36: note:   no known conversion for argument 1 from ‘Genode::addr_t’ {aka ‘long unsigned int’} to ‘const Genode::Byte_range_ptr&
   94 |         Mmio(Byte_range_ptr const &range)
      |              ~~~~~~~~~~~~~~~~~~~~~~^~~~~

genodelabs/bin/x86_64/pci_decode

/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc: In lambda function:
/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc:97:53: error: no matching function for call to ‘Pci::Config_type1::Config_type1(Genode::addr_t)’
   97 |                         Config_type1 bcfg(cfg.base());
      |                                                     ^
In file included from /data/depot/checker/api/os/2024-01-30/include/pci/config.h:20,
                 from /data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc:25:
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:9: note: candidate: ‘Genode::Mmio<<anonymous> >::Mmio(const Genode::Byte_range_ptr&) [with long unsigned int MMIO_SIZE = 69]’
   94 |         Mmio(Byte_range_ptr const &range)
      |         ^~~~
/data/depot/checker/api/os/2024-01-30/include/pci/config.h:715:28: note:   inherited here
  715 |         using Pci::Config::Config;
      |                            ^~~~~~
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:36: note:   no known conversion for argument 1 from ‘Genode::addr_t’ {aka ‘long unsigned int’} to ‘const Genode::Byte_range_ptr&
   94 |         Mmio(Byte_range_ptr const &range)
      |              ~~~~~~~~~~~~~~~~~~~~~~^~~~~
/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc: In lambda function:
/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc:145:51: error: no matching function for call to ‘Pci::Config_type0::Config_type0(Genode::addr_t)’
  145 |                                 C0 cfg0(cfg.base());
      |                                                   ^
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:9: note: candidate: ‘Genode::Mmio<<anonymous> >::Mmio(const Genode::Byte_range_ptr&) [with long unsigned int MMIO_SIZE = 69]’
   94 |         Mmio(Byte_range_ptr const &range)
      |         ^~~~
/data/depot/checker/api/os/2024-01-30/include/pci/config.h:676:28: note:   inherited here
  676 |         using Pci::Config::Config;
      |                            ^~~~~~
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:36: note:   no known conversion for argument 1 from ‘Genode::addr_t’ {aka ‘long unsigned int’} to ‘const Genode::Byte_range_ptr&
   94 |         Mmio(Byte_range_ptr const &range)
      |              ~~~~~~~~~~~~~~~~~~~~~~^~~~~
/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc: In lambda function:
/data/depot/checker/src/pci_decode/2024-01-30/src/app/pci_decode/main.cc:299:39: error: no matching function for call to ‘Pci::Config::Config(Genode::addr_t&)’
  299 |                 Config cfg(config_base);
      |                                       ^
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:9: note: candidate: ‘Genode::Mmio<<anonymous> >::Mmio(const Genode::Byte_range_ptr&) [with long unsigned int MMIO_SIZE = 69]’
   94 |         Mmio(Byte_range_ptr const &range)
      |         ^~~~
/data/depot/checker/api/os/2024-01-30/include/pci/config.h:617:21: note:   inherited here
  617 |         using Mmio::Mmio;
      |                     ^~~~
/data/depot/checker/api/base/2024-01-30/include/util/mmio.h:94:36: note:   no known conversion for argument 1 from ‘Genode::addr_t’ {aka ‘long unsigned int’} to ‘const Genode::Byte_range_ptr&
   94 |         Mmio(Byte_range_ptr const &range)
      |              ~~~~~~~~~~~~~~~~~~~~~~^~~~~

[chelmuth]

arm_v8a.imx8q_evk.hw.vmm_arm failed tonight possibly of changes in this issue.

[init -> vmm] Start virtual machine ...
[init -> vmm] Warning: Could not find address=0xc width=0x4
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0x800000c)
[init -> vmm] Warning: Invalid write access to register GICR_WAKER address=0x0 width=0x4
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0x80a0014)
[init -> vmm] Warning: Invalid write access to register GICR_IGROUPR0 address=0x0 width=0x4
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0x80b0080)
[init -> vmm] Warning: Invalid write access to register GICR_WAKER address=0x0 width=0x4
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0x80c0014)
[init -> vmm] Warning: Invalid write access to register GICR_IGROUPR0 address=0x0 width=0x4
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0x80d0080)
[init -> vmm] Warning: Invalid write access to register GICR_WAKER address=0x0 width=0x4
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0x80e0014)
[init -> vmm] Warning: Invalid write access to register GICR_IGROUPR0 address=0x0 width=0x4
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0x80f0080)
[init -> vmm] Warning: Invalid write access to register GICR_WAKER address=0x0 width=0x4
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0x8100014)
[init -> vmm] Warning: Invalid write access to register GICR_IGROUPR0 address=0x0 width=0x4
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0x8110080)
Error: illegal READ at address 0x100d000 by pager_object: pd='init -> vm' thread='ep' ip=0x1005f44
[init -> vmm] Warning: Could not find address=0x104 width=0x1
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0xa000304)
[init -> vmm] Warning: Could not find address=0x105 width=0x1
[init -> vmm] Warning: Will ignore invalid bus access (IPA=0xa000305)

[nfeske]

The vmm.run test shows two problems. The unexpected messages result from the Mmio change, best be investigated by @m-stein.

The illegal read is a apparently a bug in os/src/test/terminal_expect_send/, which got coincidentally triggered by the new output. As it is not related to the Mmio changes, @skalk could you have look into this one?

[chelmuth]

The issue in test-terminal_expect_send is at the following location.

https://github.com/genodelabs/genode/blob/c4679e7af644f2ff8d07b9b48bc2c79eb57a79cc/repos/os/src/test/terminal_expect_send/main.cc#L35-L61

line_idx may grow indefinitely if \n is not received. In the case at hand, many 0-bytes are received and I don't know how the tool should handle these and case where the line length exceeds MAX_LINE_LENGTH. The latter should also robustly handle \n and terminating 0 of the line string, which seems to be not correct currently with String<MAX_LINE_LENGTH>.

[chelmuth]

The vmm.run test shows two problems. The unexpected messages result from the Mmio change, best be investigated by @m-stein.

@m-stein you may use BOARD=virt_qemu_arm_v8a for initial testing (as I did).

[chelmuth]

@m-stein builder revealed an additional related error.

> make -C build/riscv test/plic-migv
../genode-riscv.git/src/test/plic-migv/main.cc:26:1: error: expected class-name before ‘{’ token
   26 | {
      | ^
../genode-riscv.git/src/test/plic-migv/main.cc:29:40: error: wrong number of template arguments (2, should be 1)
   29 |         struct Timer : Register<0x0, 32> { };
      |                                        ^
In file included from ../genode.git/repos/base/include/base/trace/buffer.h:102,
                 from ../genode.git/repos/base/include/base/trace/logger.h:17,
                 from ../genode.git/repos/base/include/base/thread.h:20,
                 from ../genode.git/repos/base/include/parent/parent.h:20,
                 from ../genode.git/repos/base/include/base/env.h:17,
                 from ../genode.git/repos/base/include/base/component.h:17,
                 from ../genode-riscv.git/src/test/plic-migv/main.cc:17:
../genode.git/repos/base/include/util/register.h:22:56: note: provided for ‘template<long unsigned int <anonymous> > struct Genode::Register’
   22 |         template <unsigned long>                struct Register;
      |                                                        ^~~~~~~~
../genode-riscv.git/src/test/plic-migv/main.cc:30:40: error: wrong number of template arguments (2, should be 1)
   30 |         struct Ctrl  : Register<0x4, 32>
      |                                        ^
../genode.git/repos/base/include/util/register.h:22:56: note: provided for ‘template<long unsigned int <anonymous> > struct Genode::Register’
   22 |         template <unsigned long>                struct Register;
      |                                                        ^~~~~~~~
../genode-riscv.git/src/test/plic-migv/main.cc:32:41: error: expected template-name before ‘<’ token
   32 |                 struct Enable : Bitfield<0, 1> { };
      |                                         ^
../genode-riscv.git/src/test/plic-migv/main.cc:32:41: error: expected ‘{’ before ‘<’ token
../genode-riscv.git/src/test/plic-migv/main.cc:32:41: error: expected unqualified-id before ‘<’ token
../genode-riscv.git/src/test/plic-migv/main.cc:34:40: error: wrong number of template arguments (2, should be 1)
   34 |         struct Cmp   : Register<0x8, 32>{ } ;
      |                                        ^
../genode.git/repos/base/include/util/register.h:22:56: note: provided for ‘template<long unsigned int <anonymous> > struct Genode::Register’
   22 |         template <unsigned long>                struct Register;
      |                                                        ^~~~~~~~
../genode-riscv.git/src/test/plic-migv/main.cc: In constructor ‘Genode::Timer0::Timer0(Genode::Env&, Genode::addr_t)’:
../genode-riscv.git/src/test/plic-migv/main.cc:40:19: error: class ‘Genode::Timer0’ does not have any field named ‘Attached_mmio’
   40 |                   Attached_mmio(env, base, 0x1000)
      |                   ^~~~~~~~~~~~~
../genode-riscv.git/src/test/plic-migv/main.cc: In member function ‘void Genode::Timer0::enable()’:
../genode-riscv.git/src/test/plic-migv/main.cc:43:34: error: ‘write’ was not declared in this scope
   43 |                 void enable()  { write<Ctrl::Enable>(0x1); }
      |                                  ^~~~~~~~~~~~~~~~~~~
../genode-riscv.git/src/test/plic-migv/main.cc: In member function ‘void Genode::Timer0::disable()’:
../genode-riscv.git/src/test/plic-migv/main.cc:44:34: error: ‘write’ was not declared in this scope
   44 |                 void disable() { write<Ctrl::Enable>(0); }
      |                                  ^~~~~~~~~~~~~~~~~~~
../genode-riscv.git/src/test/plic-migv/main.cc: In member function ‘void Genode::Timer0::program()’:
../genode-riscv.git/src/test/plic-migv/main.cc:49:36: error: ‘read’ was not declared in this scope
   49 |                         write<Cmp>(read<Timer>() + 100 * MS);
      |                                    ^~~~~~~~~~~
../genode-riscv.git/src/test/plic-migv/main.cc:49:25: error: ‘write’ was not declared in this scope
   49 |                         write<Cmp>(read<Timer>() + 100 * MS);
      |                         ^~~~~~~~~~
../genode-riscv.git/src/test/plic-migv/main.cc: In member function ‘unsigned int Genode::Timer0::time_ms()’:
../genode-riscv.git/src/test/plic-migv/main.cc:52:45: error: ‘read’ was not declared in this scope
   52 |                 unsigned time_ms() { return read<Timer>() / MS; }
      |                                             ^~~~~~~~~~~
../genode-riscv.git/src/test/plic-migv/main.cc: In member function ‘unsigned int Genode::Timer0::next_ms()’:
../genode-riscv.git/src/test/plic-migv/main.cc:53:45: error: ‘read’ was not declared in this scope
   53 |                 unsigned next_ms() { return read<Cmp>() / MS; }
      |                                             ^~~~~~~~~
make[2]: *** [../genode.git/repos/base/mk/generic.mk:62: main.o] Error 1

[m-stein]

@chelmuth @nfeske Thanks for pointing me to the problems! I'm at it.

[m-stein]

@nfeske @chelmuth Regarding vmm_arm.run: Without my changes, the VMM prints exactly the same warnings about "Could not find" and "Will ignore". The only difference is that, instead of the expected VM output, the illegal READ appears. I'll dive into it anyway in order to see if there's an MMIO-related issue here.

[m-stein]

@nfeske @chelmuth I've added two fixups (genode, genode-riscv) that should fix all above mentioned issues.

[chelmuth]

@m-stein thanks for fixups. According to a quick test run, the only open issue revealed here is with test-terminal_expect_send which is unrelated to MMIO. Great.

[skalk]

I've opened an issue for the test-terminal_expect_send bug.

[chelmuth]

With the build fix for acpi_drv, it fails when running on real hardware.

[init -> acpi_drv] Error: MMIO range is unexpectedly too small
[init -> acpi_drv] Error: Unknown exception occured - failure
[init -> acpi_drv] Error: Uncaught exception of type 'Genode::Mmio<14ul>::Range_violation'

[chelmuth]

The code location is https://github.com/genodelabs/genode/blob/a8b4fdc18c4aec61cec01d2a45e4959323f6907f/repos/os/src/drivers/acpi/acpi.cc#L236 and I see the following range_at() results.

[00000000001a5bb0,00000000001a5be0)
[00000000001a5bb8,00000000001a5be0)
[00000000001a5bd8,00000000001a5be0)

Indeed, the last range is too small but I have no clue what's happening there. @ssumpf @alex-ab may you chime in?

[ssumpf]

The code location is

https://github.com/genodelabs/genode/blob/a8b4fdc18c4aec61cec01d2a45e4959323f6907f/repos/os/src/drivers/acpi/acpi.cc#L236 and I see the following range_at() results.

[00000000001a5bb0,00000000001a5be0)
[00000000001a5bb8,00000000001a5be0)
[00000000001a5bd8,00000000001a5be0)

Indeed, the last range is too small but I have no clue what's happening there. @ssumpf @alex-ab may you chime in?

@chelmuth, @mstein, @alex-ab: This looks like a variable length structure to me, where the data starts after Dmar_rmrr::Limit (i.e. 24 or 0x18) and the number of scopes is defined by Dmarr_rmrr, so the range is only known at execution time. This btw. can happen in other drivers too.

[alex-ab]

@ssumpf: i may change/debug it or you are already on it ?

[ssumpf]

@ssumpf: i may change/debug it or you are already on it ?

Please go ahead.

[alex-ab]

I added a fixup. The Device_scope was specified too large. The maximum supported path count was considered as fixed part of the scope, which is not the case.

[m-stein]

@alex-ab Thanks for having fixed this so fast!

[chelmuth]

Thanks @alex-ab! Merged the commit just now.

[chelmuth]

ahci_drv and acpi_drv need some attention.

x86_64.nova: [2024-02-04 08:58:22] [init -> ahci_drv] Error: Uncaught exception of type 'Genode::Mmio<40ul>::Range_violation'
x86_32.nova: [2024-02-04 04:45:30] [init -> drivers -> acpi_drv] Error: Uncaught exception of type 'Genode::Mmio<136ul>::Range_violation'