The code currently in hv_proc.c is similar to the code in hv_call.c and mshv_root_hv_call.c from that series -- it's a wrapper around Hyper-V hypercalls. But a difference is that the code in hv_proc.c can't be a module because it is called from hv_smp_prepare_cpus(), which must be built-in. So it can't be added to the proposed hv_call.c without making all of hv_call.c built-in. Ideally, there would be a separate source file just for the code that must be built-in. I'm not familiar enough with root partition requirements to understand what hv_smp_prepare_cpus() and its calls to hv_call_add_logical_proc() and hv_call_create_vp() are doing. Evidently it is related to bringing up CPUs in the root partition, and not related to guest VMs. But those two hv_call_* functions would also be used for managing guest VMs from /dev/mshv. As for the name, I don't really like "common", even though I'm the one who created "hv_common.c" back when doing the initial arm64 support on Hyper-V. :-( My thinking is that anything that isn't under arch/x86 or arch/arm64 is by definition shared across architectures, so having "common" in the name is superfluous. Maybe just staying with hv_proc.c is OK. > > > And then later, perhaps move the entire irqdomain.c file as well? > Yes, may as well move it too. irqdomain.c is also in that category of needing to be built-in. But looking more closely, it is x86 specific and should stay where it is. I can't immediately tell whether it's feasible to make the Hyper-V IOMMU driver (and irqdomain.c) architecture neutral, or whether a separate arm64 implementation will be needed. My guess is the latter. > > > There's also an interesting question of whether to move them into > > drivers/hv, or create a new directory virt/hyperv. Hyper-V support > > started out 15 years ago structured as a driver, hence "drivers/hv". > > But over the time, the support has become significantly more than > > just a driver, so "virt/hyperv" might be a better location for > > non-driver code that had previously been under arch/x86 but is > > now common to all architectures. > > > I'd be fine with using "virt/hyperv", but I thought "virt" was only for > KVM. Now that I see the bigger picture from your previous patch series, keeping the files in drivers/hv seems OK to me. The 'mshv' code *is* a driver that implements /dev/mshv. :-) > > Another option would be to create subdirectories in "drivers/hv" to > organize the different modules more cleanly (i.e. when the /dev/mshv > driver code is introduced). Putting all the mshv and "running as root partition" files in a single sub-directory might make sense. Multiple sub-directories might be overkill. But I don't have a strong opinion either way. Putting them all directly in drivers/hv seems OK, as does one or more sub-directories. One thing I encountered back when first doing the arm64 support is that everything in the drivers/hv directory could be built as a module. The Hyper-V support under arch/x86 was always built-in, and the arch/arm64 code I added was as well. But there was no obvious place to put common code that must always be built-in. At the time, I thought about introducing virt/hyperv, but had only a single relatively small source code file, and introducing that new pathname with its own Makefile, etc. seemed like overkill. So drivers/hv/hv_common.c came into existence. I tweaked the existing drivers/hv/Makefile so hv_common.c is always built-in even when CONFIG_HYPERV=m. It's a little messy, but not too bad. The difference in what can be built as a module vs. must be built-in might be a factor in the directory structure, along with the CONFIG options that control whether the root partition code gets built at all (see below). The details will govern what works well and what ends up being a bit of a mess. > > > * Today, the code for running in the root partition is built along > > with the rest of the Hyper-V support, and so is present in kernels > > built for normal Linux guests on Hyper-V. I haven't thought about > > all the implications, but perhaps there's value in having a CONFIG > > option to build for the root partition, so that code can be dropped > > from normal kernels. There's a significant amount of new code still > > to come for mshv that could be excluded from normal guests in this > > way. Also, the tests of the hv_root_partition variable could be > > changed to a function the compiler detects is always "false" in a > > kernel built without the CONFIG option, in which case it can drop > > the code for where hv_root_partition is "true". > > > Using hv_root_partition is a good way to do it, since it won't require > many #ifdefs or moving the existing code around too much. > > I can certainly give it a try, and create a separate patch series > introducing the option. I suppose "CONFIG_HYPERV_ROOT" makes sense as a > name? Now that I see you have CONFIG_MSHV, CONFIG_MSHV_ROOT, and CONFIG_MSHV_VTL planned, could building the hv_root_partition code just be under control of CONFIG_MSHV_ROOT without introducing another CONFIG variable? Is any of the root partition code like hv_proc.c and irqdomain.c needed if CONFIG_MSHV_ROOT=n? Stubs will be needed for functions called from the generic kernel code when CONFIG_MSHV_ROOT=n, but those are easily supplied in asm/mshyperv.h. If hv_root_partition becomes a function whose return value is gated by CONFIG_MSHV_ROOT, then the compiler can know when the value is always "false" and drop even the code that calls the stubs. But I'm pretty sure the stubs are still needed to avoid compile errors, even when the compiler drops the code. With this approach, you can avoid #ifdefs except in asm/mshyperv.h for the stubs, and in the hv_root_partition() function. One more code structure topic: In the previous patch series, some of the mshv code is x86 specific. There is x86 assembler code, and references to x86 specific registers (all the HV_X64_* registers). Do you plan to put architecture specific code under drivers/hv, or under arch/[x86/arm64]/hyperv? We've made drivers/hv be architecture neutral -- currently there's only one "cheat" with an #ifdef CONFIG_ARM64 in the hv_balloon.c driver that turns off some functionality. As we discussed previously, the new Hyper-V #include files provide the union of x86 and arm64 definitions, with just an occasional #ifdef where needed. That was justified because that's how the definitions come from the Windows/Hyper-V world. But it seems like the mshv code should be structured with arch specific code is under arch, not in drivers/hv with #ifdefs. The Makefiles in arch/[x86,arm64]/hyperv will need to use CONFIG_MSHV, CONFIG_MSHV_ROOT, and CONFIG_MSHV_VTL to decide what to build, and whether to build as a module vs. built-in. Maybe putting the mshv code in a "mshv" subdirectory under both drivers/hv and arch/[x86,arm64]/hyperv would conceptually help tie together the arch neutral and arch specific portions. Or something like that .... > > > * The code currently in hv_proc.c is built for x86 only, and validly > > assumes the page size is 4K. But when the code moves to be > > common across architectures, that assumption is no longer > > valid in the general case. Perhaps the intent is that kernels for > > the root partition should always be built with page size 4K on > > arm64, but nothing enforces that intent. Personally, I think the code > > should be made to work with page sizes other than 4K so as to not > > leave technical debt. But I realize you may have other priorities. If > > there were a CONFIG option for building for the root partition, > > that option could be setup to enforce the 4K page size on arm64. > > > That makes sense. I suppose this can be done by selecting PAGE_SIZE_4KB > under HYPERV in drivers/hv/Kconfig? Since the PAGE_SIZE value is independently selectable in the .config file, I'm not sure if you can override it when CONFIG_MSHV_ROOT is set. But you can allow CONFIG_MSHV_ROOT to be set only if PAGE_SIZE_4KB is selected on arm64. I'd have to look in more detail to figure out the best way to create an appropriate dependency. > > I'm not how easy it will be to make the code work with different page > sizes, since we use alloc_page() and similar in a few places, assuming 4k. alloc_page() is not the problem as it is relatively easy to break up a 16K or 64K page into multiple 4K pages when depositing memory. And you can round up the amount of deposited memory to the larger boundary without doing any harm. But in your previous patch series, I see hv_call_withdraw_memory(), wherein Hyper-V gives back individual 4K pages in no particular order. That's the killer, as it is not feasible to re-assemble a random set of 4K pages into larger 16K or 64K pages for free_page(). So scratch that idea. :-( The root partition must run with a page size that matches the size Hyper-V uses to do memory deposits to and from a partition, and that's 4K. Michael [1] https://lore.kernel.org/lkml/1696010501-24584-1-git-send-email-nunodasneves@xxxxxxxxxxxxxxxxxxx/
