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

[Wei Liu <wei.liu@kernel.org>]

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.