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.mayd...@linaro.org> wrote:
>
> On Mon, 12 Aug 2024 at 23:18, Danny Canter <danny_can...@apple.com> wrote:
>> On Aug 12, 2024, at 10:52 AM, Peter Maydell <peter.mayd...@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
