On 2/12/2025 3:01 PM, Nuno Das Neves wrote: > On 2/11/2025 9:47 PM, Easwar Hariharan wrote: >> On 2/11/2025 2:21 PM, Nuno Das Neves wrote: >>> Introduce CONFIG_MSHV_ROOT as a tristate to enable root partition >>> booting and future mshv driver functionality. >>> >>> Change hv_root_partition into a function which always returns false >>> if CONFIG_MSHV_ROOT=n. >>> >>> Introduce hv_current_partition_type to store the type of partition >>> (guest, root, or other kinds in future), and hv_identify_partition_type() >>> to it up early in Hyper-V initialization. >> >> ...to *set* it up early? >> > Yep! Thanks for catching that > >>> >>> Signed-off-by: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> >>> --- >>> Depends on >>> https://lore.kernel.org/linux-hyperv/1738955002-20821-3-git-send-email-nunodasneves@xxxxxxxxxxxxxxxxxxx/ >>> >>> arch/arm64/hyperv/mshyperv.c | 2 ++ >>> arch/x86/hyperv/hv_init.c | 10 ++++---- >>> arch/x86/kernel/cpu/mshyperv.c | 24 ++---------------- >>> drivers/clocksource/hyperv_timer.c | 4 +-- >>> drivers/hv/Kconfig | 12 +++++++++ >>> drivers/hv/Makefile | 3 ++- >>> drivers/hv/hv.c | 10 ++++---- >>> drivers/hv/hv_common.c | 32 +++++++++++++++++++----- >>> drivers/hv/vmbus_drv.c | 2 +- >>> drivers/iommu/hyperv-iommu.c | 4 +-- >>> include/asm-generic/mshyperv.h | 39 +++++++++++++++++++++++++----- >>> 11 files changed, 92 insertions(+), 50 deletions(-) >>> >> >> <snip> >> >>> + >>> +void hv_identify_partition_type(void) >>> +{ >>> + /* >>> + * Check partition creation and cpu management privileges >>> + * >>> + * Hyper-V should never specify running as root and as a Confidential >>> + * VM. But to protect against a compromised/malicious Hyper-V trying >>> + * to exploit root behavior to expose Confidential VM memory, ignore >>> + * the root partition setting if also a Confidential VM. >>> + */ >>> + if ((ms_hyperv.priv_high & HV_CREATE_PARTITIONS) && >>> + (ms_hyperv.priv_high & HV_CPU_MANAGEMENT) && >>> + !(ms_hyperv.priv_high & HV_ISOLATION)) { >>> + hv_current_partition_type = HV_PARTITION_TYPE_ROOT; >>> + pr_info("Hyper-V: running as root partition\n"); >>> + } else { >>> + hv_current_partition_type = HV_PARTITION_TYPE_GUEST; >>> + } >>> +} >> >> This should assume GUEST as default and modify to ROOT if all the checks pass. >> > It is doing that, isn't it? > > In fact the 'else' branch here is redundant and just there for additional clarity. > > hv_current_partition_type is zeroed (so GUEST) by default, but I could make that explicit > if you prefer: Yes, explicit is better, but see comment below. > +enum hv_partition_type hv_current_partition_type = HV_PARTITION_TYPE_GUEST; > > How does that sound? Am I misunderstanding something here? I'd suggest centralizing that in this function, instead of having it spread in 2 places. Since your commit message hints at future partition types, it's ideal to have this function be a central clearing house, which I suppose is the intent. The preferred pattern in general, and what I'm suggesting, is something like this: void hv_identify_partition_type(void) { /* Assume guest role */ hv_current_partition_type = HV_PARTITION_TYPE_GUEST; /* * Check partition creation and cpu management privileges * * Hyper-V should never specify running as root and as a Confidential * VM. But to protect against a compromised/malicious Hyper-V trying * to exploit root behavior to expose Confidential VM memory, ignore * the root partition setting if also a Confidential VM. */ if ((ms_hyperv.priv_high & HV_CREATE_PARTITIONS) && (ms_hyperv.priv_high & HV_CPU_MANAGEMENT) && !(ms_hyperv.priv_high & HV_ISOLATION)) { hv_current_partition_type = HV_PARTITION_TYPE_ROOT; pr_info("Hyper-V: running as root partition\n"); } } > >> <snip> >> >>> +static inline int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages) >>> +{ >>> + return hv_result(U64_MAX); >>> +} >> >> Is there value in perhaps #defining hv_result_<whatever this is> as U64_MAX and returning that for documentation? >> For e.g. assuming this is something like EOPNOTSUPP >> >> #define HV_RESULT_NOT_SUPP U64_MAX >> >> static inline int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages) >> { return hv_result(HV_RESULT_NOT_SUPP); } >> > The idea here was to copy what hv_do_hypercall does returning U64_MAX in case the hypercall > page is missing, which will hv_result() into an invalid status code. A special value for > that status could make this pattern clearer. Agreed, having a name for that status would be helpful, but we don't want to diverge too much from the hypervisor definitions, especially if we're going to change it later again anyway. I'd want to call out that this isn't a "real" > Hyper-V status code somehow. HV_STATUS's are 16 bits, so it would look more like: > > /* "LINUX" because this isn't really a status from the hypervisor.. */ > #define HV_STATUS_LINUX_FAIL 0xFFFF > static inline int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages) > { return HV_STATUS_LINUX_FAIL; } > > Another option: there is another patch coming (which you know of) which maps hypercall > status codes to regular Linux errors like -EOPNOTSUPP. I could simply merge that patch > with this one (or make this a series for v2), and that would result in less churn. > (And leave alone the current use of U64_MAX in hv_do_hypercall, for now). > I think that second option is a good idea. The hypervisor status should remain restricted to the functions that are hv_do_hypercall() or call it directly, while the rest of the code uses standard errno values. I'd suggest making it a series so each commit does 1 thing. Thanks, Easwar (he/him)
