| Message ID | 1740126987-8483-1-git-send-email-liuwe@linux.microsoft.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Factor out HVF's instruction emulator | expand |
On 2/21/25 09:36, Wei Liu wrote: > This patch series attempts to make the instruction emulator in HVF a common > component for the i386 target. It removes HVF specific code by either using a > set of hooks or moving it to better locations. The new incoming MSHV > accelerator will implement the hooks, and where necessary, enhance the emulator > and / or add new hooks. Good! > This patch series is in RFC state. The patches have been lightly tested by > running a Linux VM on an Intel-based Mac. We hope to get some feedback on the > overall approach, and let the community bikeshed a bit about names and > location. For the bikeshedding my only suggestion is to replace mmio_buf with emu_mmio_buf, and replace x86-insn-emul, with just "emulate" or something like that. That is, no need to repeat x86 inside the target/i386 directory, especially since the filenames also start with x86. > First two patches fix issues in the existing code. They can be applied > regardless of the discussion around the overall approach. These four can also be applied: target/i386/hvf: use x86_segment in x86_decode.c target/i386/hvf: move and rename {load, store}_regs target/i386/hvf: move and rename simulate_{rdmsr, wrmsr} target/i386/hvf: drop some dead code > The checkpatch script complains about a few things. Some are from the original > code I didn't touch. For the code I changed or moved, it complains that some > lines are long (>80). Seeing that the rule was not followed strictly in the old > code base, I held off fixing that class of issues. The other thing it complains > is there is no entry for the new directory in MAINTAINERS. We can fix these > issues if they are deemed important. Yes, no problem. The new directory thing is just a warning but I think you could add a new entry with both MSHV and HVF people on it. > Please let us know what you think. The alternative is to duplicate the > instruction emulator code in the mshv accelerator. That looks to be a worse > option. Yes, definitely. Paolo
On Fri, 21 Feb 2025 at 14:02, Wei Liu <liuwe@linux.microsoft.com> wrote: > > Hi, > > Microsoft's Linux Systems Group developed a Linux driver for the Microsoft > Hypervisor (MSHV for short). The driver is being upstreamed. The first > supported VMM is Cloud Hypervisor. QEMU will be the second supported > VMM. > > The plan is to write an mshv accelerator in QEMU. The accelerator is still in > the works. > > MSHV doesn't emulate instructions. VMMs are supposed to bring their own > instruction emulator. The path we've chosen is to reuse what's already in QEMU. > The instruction emulator in HVF looks good for what we need. > > This patch series attempts to make the instruction emulator in HVF a common > component for the i386 target. It removes HVF specific code by either using a > set of hooks or moving it to better locations. The new incoming MSHV > accelerator will implement the hooks, and where necessary, enhance the emulator > and / or add new hooks. If you want to make the hvf decoder more widely used you might want to look at this old patch to it that was never applied (issues in code review not addressed by the submitter): https://lore.kernel.org/qemu-devel/CAFEAcA8yaBOD3KXc-DY94oqzC5wkCENPkePgVCybqR=9NmdQFQ@mail.gmail.com/ which is trying to fix a problem where an overlong string of prefix bytes causes the decoder to misbehave. (PS: if in the future you should ever find yourself wanting to do an equivalent "decode loads/stores the hypervisor doesn't handle" for Arm, use decodetree, not a hand-rolled decoder...) thanks -- PMM
On Fri, Feb 21, 2025 at 05:36:39PM +0100, Paolo Bonzini wrote: > On 2/21/25 09:36, Wei Liu wrote: > > This patch series attempts to make the instruction emulator in HVF a common > > component for the i386 target. It removes HVF specific code by either using a > > set of hooks or moving it to better locations. The new incoming MSHV > > accelerator will implement the hooks, and where necessary, enhance the emulator > > and / or add new hooks. > > Good! > > > This patch series is in RFC state. The patches have been lightly tested by > > running a Linux VM on an Intel-based Mac. We hope to get some feedback on the > > overall approach, and let the community bikeshed a bit about names and > > location. > > For the bikeshedding my only suggestion is to replace mmio_buf with > emu_mmio_buf, and replace x86-insn-emul, with just "emulate" or something > like that. That is, no need to repeat x86 inside the target/i386 directory, > especially since the filenames also start with x86. > No problem. We can make the changes in the next version. > > First two patches fix issues in the existing code. They can be applied > > regardless of the discussion around the overall approach. > > These four can also be applied: > > target/i386/hvf: use x86_segment in x86_decode.c > target/i386/hvf: move and rename {load, store}_regs > target/i386/hvf: move and rename simulate_{rdmsr, wrmsr} > target/i386/hvf: drop some dead code > > > The checkpatch script complains about a few things. Some are from the original > > code I didn't touch. For the code I changed or moved, it complains that some > > lines are long (>80). Seeing that the rule was not followed strictly in the old > > code base, I held off fixing that class of issues. The other thing it complains > > is there is no entry for the new directory in MAINTAINERS. We can fix these > > issues if they are deemed important. > > Yes, no problem. The new directory thing is just a warning but I think you > could add a new entry with both MSHV and HVF people on it. > Okay, that works, too. > > Please let us know what you think. The alternative is to duplicate the > > instruction emulator code in the mshv accelerator. That looks to be a worse > > option. > Yes, definitely. Thank you for the feedback. Wei.
On Fri, Feb 21, 2025 at 04:53:26PM +0000, Peter Maydell wrote: > On Fri, 21 Feb 2025 at 14:02, Wei Liu <liuwe@linux.microsoft.com> wrote: > > > > Hi, > > > > Microsoft's Linux Systems Group developed a Linux driver for the Microsoft > > Hypervisor (MSHV for short). The driver is being upstreamed. The first > > supported VMM is Cloud Hypervisor. QEMU will be the second supported > > VMM. > > > > The plan is to write an mshv accelerator in QEMU. The accelerator is still in > > the works. > > > > MSHV doesn't emulate instructions. VMMs are supposed to bring their own > > instruction emulator. The path we've chosen is to reuse what's already in QEMU. > > The instruction emulator in HVF looks good for what we need. > > > > This patch series attempts to make the instruction emulator in HVF a common > > component for the i386 target. It removes HVF specific code by either using a > > set of hooks or moving it to better locations. The new incoming MSHV > > accelerator will implement the hooks, and where necessary, enhance the emulator > > and / or add new hooks. > > If you want to make the hvf decoder more widely used you might want > to look at this old patch to it that was never applied (issues in > code review not addressed by the submitter): > > https://lore.kernel.org/qemu-devel/CAFEAcA8yaBOD3KXc-DY94oqzC5wkCENPkePgVCybqR=9NmdQFQ@mail.gmail.com/ > > which is trying to fix a problem where an overlong string of > prefix bytes causes the decoder to misbehave. > Thanks for the information. > (PS: if in the future you should ever find yourself wanting to do an > equivalent "decode loads/stores the hypervisor doesn't handle" > for Arm, use decodetree, not a hand-rolled decoder...) > Noted. Yep, we have plans to add ARM64 support in the future. Thanks, Wei. > thanks > -- PMM
Hi, Microsoft's Linux Systems Group developed a Linux driver for the Microsoft Hypervisor (MSHV for short). The driver is being upstreamed. The first supported VMM is Cloud Hypervisor. QEMU will be the second supported VMM. The plan is to write an mshv accelerator in QEMU. The accelerator is still in the works. MSHV doesn't emulate instructions. VMMs are supposed to bring their own instruction emulator. The path we've chosen is to reuse what's already in QEMU. The instruction emulator in HVF looks good for what we need. This patch series attempts to make the instruction emulator in HVF a common component for the i386 target. It removes HVF specific code by either using a set of hooks or moving it to better locations. The new incoming MSHV accelerator will implement the hooks, and where necessary, enhance the emulator and / or add new hooks. This patch series is in RFC state. The patches have been lightly tested by running a Linux VM on an Intel-based Mac. We hope to get some feedback on the overall approach, and let the community bikeshed a bit about names and location. First two patches fix issues in the existing code. They can be applied regardless of the discussion around the overall approach. The checkpatch script complains about a few things. Some are from the original code I didn't touch. For the code I changed or moved, it complains that some lines are long (>80). Seeing that the rule was not followed strictly in the old code base, I held off fixing that class of issues. The other thing it complains is there is no entry for the new directory in MAINTAINERS. We can fix these issues if they are deemed important. Please let us know what you think. The alternative is to duplicate the instruction emulator code in the mshv accelerator. That looks to be a worse option. Thanks, Wei. Wei Liu (19): target/i386/hvf: fix a typo in a type name target/i386/hvf: fix the declaration of hvf_handle_io target/i386/hvf: use x86_segment in x86_decode.c target/i386/hvf: introduce x86_emul_ops target/i386/hvf: remove HVF specific calls from x86_decode.c target/i386/hvf: move and rename {load,store}_regs target/i386/hvf: provide and use handle_io in emul_ops target/i386: rename hvf_mmio_buf to mmio_buf target/i386/hvf: use emul_ops->read_mem in x86_emu.c taret/i386/hvf: provide and use write_mem in emul_ops target/i386/hvf: move and rename simulate_{rdmsr,wrmsr} target/i386/hvf: provide and use simulate_{wrmsr,rdmsr} in emul_ops target/i386: rename lazy flags field and its type target/i386/hvf: drop unused headers target/i386/hvf: drop some dead code target/i386/hvf: rename some include guards target/i386: add a directory for x86 instruction emulator target/i386/x86-insn-emul: add a panic.h target/i386: move x86 instruction emulator out of hvf target/i386/cpu.h | 8 +- target/i386/hvf/hvf-i386.h | 4 +- target/i386/hvf/hvf.c | 334 ++++++++++++++++-- target/i386/hvf/meson.build | 3 - target/i386/hvf/vmx.h | 2 +- target/i386/hvf/x86.c | 8 +- target/i386/hvf/x86_cpuid.c | 2 +- target/i386/hvf/x86_descr.c | 8 +- target/i386/hvf/x86_descr.h | 8 +- target/i386/hvf/x86_mmu.c | 2 +- target/i386/hvf/x86_task.c | 32 +- target/i386/hvf/x86_task.h | 2 +- target/i386/hvf/x86hvf.c | 2 +- target/i386/hvf/x86hvf.h | 3 + target/i386/meson.build | 1 + target/i386/x86-insn-emul/meson.build | 5 + target/i386/x86-insn-emul/panic.h | 45 +++ target/i386/{hvf => x86-insn-emul}/x86.h | 12 +- .../i386/{hvf => x86-insn-emul}/x86_decode.c | 18 +- .../i386/{hvf => x86-insn-emul}/x86_decode.h | 4 +- target/i386/{hvf => x86-insn-emul}/x86_emu.c | 329 ++--------------- target/i386/{hvf => x86-insn-emul}/x86_emu.h | 20 +- .../i386/{hvf => x86-insn-emul}/x86_flags.c | 56 +-- .../i386/{hvf => x86-insn-emul}/x86_flags.h | 6 +- 24 files changed, 497 insertions(+), 417 deletions(-) create mode 100644 target/i386/x86-insn-emul/meson.build create mode 100644 target/i386/x86-insn-emul/panic.h rename target/i386/{hvf => x86-insn-emul}/x86.h (96%) rename target/i386/{hvf => x86-insn-emul}/x86_decode.c (99%) rename target/i386/{hvf => x86-insn-emul}/x86_decode.h (99%) rename target/i386/{hvf => x86-insn-emul}/x86_emu.c (78%) rename target/i386/{hvf => x86-insn-emul}/x86_emu.h (72%) rename target/i386/{hvf => x86-insn-emul}/x86_flags.c (83%) rename target/i386/{hvf => x86-insn-emul}/x86_flags.h (97%)