On Sat, 17 Aug 2024 at 01:37, Danny Canter <danny_can...@apple.com> wrote:
>
> Peter, thought I’d send this little snippet before getting the rest of V2
> done in case anyone hates this :). I tried to take a similar approach to
> kvm_type,
> but I’m not sure if this will be looked upon favorably so want an early
> opinion. The nice thing about kvm_type is at least it has differing meaning
> per
> platform so all the impls can do whatever they need, with the below it’s
> really only needed on ARM (and obviously macOS specific) so it's a bit odd,
> but couldn’t think of how else to be able to be able to get what we need out
> of the memmap during vm creation.
>
> How this would be used is almost exactly like how ARMs kvm_type is used. We
> set up hvf_get_physical_address_range to freeze the memory
> map and compute the highest gpa, then check if that exceeds our platforms
> largest IPA size and if so return a sane error message. If everything
> checks out we’d just set the IPA size on the VM config object and then create
> the VM. The current patch should mostly stay the same after that bit
> of plumbing I think besides removing the macOS 13 ifdef’s (and simplifying
> the copy and pasted loop you pointed out). x86’s
> hvf_get_physical_address_range can be NULL.
>
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -215,6 +215,10 @@ typedef struct {
> * Return the type of KVM corresponding to the kvm-type string option or
> * computed based on other criteria such as the host kernel capabilities.
> * kvm-type may be NULL if it is not needed.
> + * @hvf_get_physical_address_range:
> + * Returns the physical address range in bits to use for the HVF virtual
> + * machine based on the current boards memory map. This may be NULL if it
> + * is not needed.
> * @numa_mem_supported:
> * true if '--numa node.mem' option is supported and false otherwise
> * @hotplug_allowed:
> @@ -253,6 +257,7 @@ struct MachineClass {
> void (*reset)(MachineState *state, ShutdownCause reason);
> void (*wakeup)(MachineState *state);
> int (*kvm_type)(MachineState *machine, const char *arg);
> + unsigned int (*hvf_get_physical_address_range)(MachineState *machine);
My gut feeling was that this felt very specific compared
to the kvm_type method which lets different architectures
use it for whatever they need. But on the other hand we
have exactly one use for this for hvf right now and at
least for the foreseeable future it's unlikely we're going
to want to do more. And this API isn't a set-in-stone one,
so we can come back and generalize it later if we ever need
to do that. (Or, if we find we need this kind of hook for
a third hypervisor type, maybe we try to make it hypervisor
agnostic at that point.)
So I'm OK with this.
thanks
-- PMM
