Aarch64 serial fix by sandreim · Pull Request #1233 · firecracker-microvm/firecracker

sandreim · 2019-08-28T16:11:18Z

Fixes #1147, #1144

Description of changes:

Added kernel configuration for aarch64 in resources folder (the previous configuration was missing CONFIG_SERIAL_OF_PLATFORM so the guest uart driver was not probing the device tree for ports)
Changed serial IRQ from level to edge triggered (aarch64).
Updated the device tree entry to use the clock node definition.
Changed the early console settings to 8bit mmio.
Rework serial device integration in VMM: no need to access the serial device object
Implemented RawIOHandler trait + get_raw_io_device() mapping the RawIOHandler trait
Implemented arch dependent get_serial_device()
Move stdin_handle from LegacyDeviceManager to VMM.

Note: S3 update of kernel and rootfs images are also required. I'll update them after PR is merged.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

acatangiu · 2019-08-29T08:14:39Z

nit: We like to use the 50/72 rule for our commit messages.

acatangiu

I think we should rename the legacy_device_manager to portio_device_manager so we get a clear-cut delimitation between mmio devices and portio devices, then completely remove the portio_device_manager from aarch64.

vmm/src/device_manager/mmio.rs

dianpopa

I agree with @acatangiu that

LegacyDeviceManager should be renamed to PortIODeviceManager.
PortIODeviceManager should be compiled out for aarch64 (as a consequence of this, you could also need to compile out some more things, like the i8042 usages). I would do a separate commit for this.
Remove the Option around the MMIODeviceManager, this will also benefit the x86_64 logic since we are always calling self.attach_virtio_devices()?; at start_microvm no matter what.

vmm/src/device_manager/mmio.rs

dianpopa · 2019-08-29T09:34:01Z

vmm/src/lib.rs

+                            // One lives in LegacyDeviceManager and the other in MMIODeviceManager
+                            // Use the one that is actually plugged in the Guest.
+                            #[cfg(target_arch = "aarch64")]
+                            let serial = self


What I would do here for more elegance would be this:
Have a helper function (maybe self.get_stdio_serial()) that has two implementations, one for x86_64 and one for aarch64. See the example of
fn attach_legacy_devices(&mut self) -> std::result::Result<(), StartMicrovmError> {
Additionally, if you decide to use the get_device function from the MMIODeviceManager, this will become self.mmio_device_manager.get_device(DeviceType::Serial, "uart").

* PortIODeviceManager should be compiled out for aarch64 (as a consequence of this, you could also need to compile out some more things, like the i8042 usages). I would do a separate commit for this. * Remove the `Option` around the `MMIODeviceManager`, this will also benefit the x86_64 logic since we are always calling `self.attach_virtio_devices()?;` at `start_microvm` no matter what.

I will perform the rename, but outcompiling this on aarch64 will backfire by lots of
#[cfg(target_arch = "x86_64")] in different parts of the codebase.
Also removing the Option depends on also initializing guest memory before instantiating the PortIODeviceManager generating even more code changes.
I think this should be done as a separate refactoring PR with a clear design defined and agreed by all.

arch/src/aarch64/fdt.rs

devices/src/bus.rs

devices/src/lib.rs

devices/src/bus.rs

vmm/src/lib.rs

vmm/src/device_manager/mmio.rs

dianpopa

💯

devices/src/bus.rs

vmm/src/lib.rs

Signed-off-by: Andrei Sandu <[email protected]>

- Remove data_len from Serial device implementation - Rename LegacyDeviceManager to PortIODeviceManager Signed-off-by: Andrei Sandu <[email protected]>

Use the device tree clock node definition (apb_pclk). Signed-off-by: Andrei Sandu <[email protected]>

vmm/src/device_manager/mmio.rs

vmm/src/lib.rs

- Implement RawIOHandler trait. - Add raw_io_handlers hashmap - Implmenet get_raw_io_device() mapping the RawIOHandler trait - Implement arch dependent get_serial_device() - x86_64 uses PortIODeviceManager - aarch uses MMIODeviceManafger - Move stdin_handle out of PortIODeviceManager. - Add unit test for get_raw_io_device(). Signed-off-by: Andrei Sandu <[email protected]>

Serial device was finally fully supported with firecracker on aarch64, see details here(firecracker-microvm/firecracker#1233). A set of kernel configs related with 8250 compatible serial ports should be turn on. Fixes: kata-containers#736 Signed-off-by: Penny Zheng <[email protected]> Signed-off-by: Haibo Xu <[email protected]>

ref: firecracker-microvm#1233

ref: firecracker-microvm#1233 Signed-off-by: Ján Mochňak <[email protected]>

ref: #1233 Signed-off-by: Ján Mochňak <[email protected]>

sandreim requested review from dhrgit and dianpopa August 28, 2019 16:12

acatangiu reviewed Aug 29, 2019

View reviewed changes

vmm/src/device_manager/mmio.rs Outdated Show resolved Hide resolved

dianpopa reviewed Aug 29, 2019

View reviewed changes

dianpopa assigned sandreim Aug 29, 2019

sandreim force-pushed the aarch64_serial_fix branch 2 times, most recently from 0152295 to 6af6b87 Compare August 30, 2019 13:15

sandreim requested review from acatangiu and dianpopa August 30, 2019 13:20

sandreim force-pushed the aarch64_serial_fix branch 2 times, most recently from 3dbc9f3 to aff5676 Compare September 2, 2019 11:55

dianpopa reviewed Sep 3, 2019

View reviewed changes

sandreim force-pushed the aarch64_serial_fix branch 3 times, most recently from b7497fb to 4c41e9f Compare September 3, 2019 15:27

sandreim added Codebase: Architecture and removed Codebase: Architecture labels Sep 3, 2019

sandreim requested a review from dianpopa September 3, 2019 20:02

dianpopa previously approved these changes Sep 4, 2019

View reviewed changes

acatangiu reviewed Sep 9, 2019

View reviewed changes

devices/src/bus.rs Show resolved Hide resolved

vmm/src/lib.rs Outdated Show resolved Hide resolved

sandreim added 3 commits September 9, 2019 12:45

Add aarch64 kernel config.

414d89c

Signed-off-by: Andrei Sandu <[email protected]>

Changed earlycon settings to use mmio 8 bit access.

6cf87c7

- Remove data_len from Serial device implementation - Rename LegacyDeviceManager to PortIODeviceManager Signed-off-by: Andrei Sandu <[email protected]>

Change serial irq to rising edge.

98e7924

Use the device tree clock node definition (apb_pclk). Signed-off-by: Andrei Sandu <[email protected]>

sandreim dismissed dianpopa’s stale review via 654c8fa September 9, 2019 09:47

sandreim force-pushed the aarch64_serial_fix branch 3 times, most recently from 0a32477 to 2263509 Compare September 9, 2019 10:48

acatangiu reviewed Sep 9, 2019

View reviewed changes

vmm/src/device_manager/mmio.rs Show resolved Hide resolved

vmm/src/lib.rs Outdated Show resolved Hide resolved

sandreim force-pushed the aarch64_serial_fix branch from 2263509 to 72be4a0 Compare September 9, 2019 13:21

acatangiu previously approved these changes Sep 9, 2019

View reviewed changes

sandreim dismissed acatangiu’s stale review via 9586cc9 September 9, 2019 13:32

sandreim force-pushed the aarch64_serial_fix branch from 72be4a0 to 9586cc9 Compare September 9, 2019 13:32

acatangiu approved these changes Sep 9, 2019

View reviewed changes

dianpopa approved these changes Sep 9, 2019

View reviewed changes

sandreim mentioned this pull request Sep 10, 2019

Improve aarch64 filesystem image hygene #1144

Closed

sandreim merged commit 60085d5 into firecracker-microvm:master Sep 10, 2019

Pennyzct mentioned this pull request Sep 25, 2019

Kernel: Support Firecracker on aarch64 kata-containers/packaging#736

Closed

mochja added a commit to mochja/firecracker that referenced this pull request May 28, 2022

docs: add serial console kernel config for aarch64

a4484bf

ref: firecracker-microvm#1233

mochja mentioned this pull request May 28, 2022

docs: add serial console kernel config for aarch64 #3009

Merged

9 tasks

mochja added a commit to mochja/firecracker that referenced this pull request May 30, 2022

docs: add serial console kernel config for aarch64

5bb8fc5

ref: firecracker-microvm#1233 Signed-off-by: Ján Mochňak <[email protected]>

luminitavoicu pushed a commit that referenced this pull request May 31, 2022

docs: add serial console kernel config for aarch64

4ed4646

ref: #1233 Signed-off-by: Ján Mochňak <[email protected]>

bchalios pushed a commit that referenced this pull request Sep 5, 2022

docs: add serial console kernel config for aarch64

ca57f50

ref: #1233 Signed-off-by: Ján Mochňak <[email protected]>

kzys mentioned this pull request Mar 9, 2023

[Bug] Issue with Go race detector - ThreadSanitizer: unsupported VMA range Found 39 - Supported 48 #3514

Closed

3 tasks

Conversation

sandreim commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acatangiu commented Aug 29, 2019

Uh oh!

acatangiu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dianpopa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dianpopa Aug 29, 2019

Choose a reason for hiding this comment

Uh oh!

sandreim Aug 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dianpopa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sandreim commented Aug 28, 2019 •

edited

Loading