Re: [RFC PATCH v1 00/19] Factor out HVF's instruction emulator

[Paolo Bonzini <pbonzini@redhat.com>]

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