driver/qemudriver: add support for Q35 machine #961
Conversation
labgrid/driver/qemudriver.py
Outdated
| elif self.machine == "q35": | ||
| self._cmd.append("-drive") | ||
| self._cmd.append( | ||
| f"format={disk_format},file={disk_path}") |
There was a problem hiding this comment.
Is if=virtio the default now?
We should probably keep the root=... rootwait or is there a downside?
There was a problem hiding this comment.
No, if=ide is default for q35. Should I include that?
I think it is fine to include rootwait, but I would expect people to define root and rootfstype in local.yaml if it is not handled by their bootloader. By appending them to boot_args, you effectively overwrite any boot or rootfstype defined in local.yaml and thus hardcode them or did I miss anything? For example, if i spin up a pc machine, I will always have root=/dev/vda, no?
There was a problem hiding this comment.
No,
if=ideis default forq35. Should I include that?
I'd argue that we q35 should behave mostly similar to the existing pc config.
I think it is fine to include
rootwait, but I would expect people to definerootandrootfstypeinlocal.yamlif it is not handled by their bootloader. By appending them toboot_args, you effectively overwrite anybootorrootfstypedefined inlocal.yamland thus hardcode them or did I miss anything? For example, if i spin up apcmachine, I will always haveroot=/dev/vda, no?
The boot_args are only passed to qemu if a kernel is started directly:
https://github.com/labgrid-project/labgrid/blob/master/labgrid/driver/qemudriver.py#L174-L176
Also, custom boot_args are appended after the autogenerated ones
(https://github.com/labgrid-project/labgrid/blob/master/labgrid/driver/qemudriver.py#L172-L173), which allows the user to override them in the yaml.
There was a problem hiding this comment.
I'd argue that we q35 should behave mostly similar to the existing pc config.
If we keep the interface default for q35, i.e., if=ide, I agree.
The boot_args are only passed to qemu if a kernel is started directly:
https://github.com/labgrid-project/labgrid/blob/master/labgrid/driver/qemudriver.py#L174-L176
Also, custom boot_args are appended after the autogenerated ones
(https://github.com/labgrid-project/labgrid/blob/master/labgrid/driver/qemudriver.py#L172-L173), which allows the user to override them in the yaml.
Now it makes sense to me. Thanks for clarifying. I will include root=... rootwait to q35 also.
There was a problem hiding this comment.
Oh, I misread the diff and went ahead with the merge. Could you add the root= in a new PR? :)
There was a problem hiding this comment.
I was confused for a second. Yes, I will make a new PR.
Signed-off-by: Emil Kronborg Andersen <emil.kronborg@protonmail.com>
Codecov Report
@@ Coverage Diff @@
## master #961 +/- ##
========================================
- Coverage 57.1% 57.1% -0.1%
========================================
Files 150 150
Lines 11199 11202 +3
========================================
Hits 6402 6402
- Misses 4797 4800 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
2 participants
Signed-off-by: Emil Kronborg Andersen emil.kronborg@protonmail.com
Description
Allow usage of Q35-based machines in QEMU.
Checklist