Re: [PATCH] hvf: arm: Allow creating VMs with > 63GB of RAM on macOS 15+

[Danny Canter]

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);

On Aug 13, 2024, at 2:31 AM, Peter Maydell <peter.maydell@linaro.org> wrote:

On Mon, 12 Aug 2024 at 23:18, Danny Canter <danny_canter@apple.com> wrote:

On Aug 12, 2024, at 10:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:

This is unfortunately probably going to imply a bit of extra
plumbing to be implemented for hvf -- that MachineClass::kvm_type
method is (as the name suggests) KVM specific. (Multi-patch
patchset for that, where we add the plumbing in as its own
separate patch (and/or whatever other split of functionality
into coherent chunks makes sense), rather than one-big-patch, please.)

That’s perfectly fine, I’ll try and see how the plumbing was done
for KVM and try and emulate where it makes sense
for HVF. Agree though, that’d definitely push this into multi-patch
territory. Curious if you think what’s here today should
be multiple patches or the current work seems fine in one?

I think it was fine as one patch. My personal preference
when I write code tends to go for more-smaller-patches
over fewer-larger-patches, so I might have for example
split out "Add hvf_arch_vm_create()" into its own
patch, but that's very borderline, and I wouldn't ask for
that change at code review time unless the patch as a whole
was too big and unwieldy and I was looking for places to
suggest a split into multiple patches.

-- PMM