| Message ID | 1692309711-5573-4-git-send-email-nunodasneves@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce /dev/mshv drivers | expand |
On 8/17/23 15:01, Nuno Das Neves wrote: > +static inline union hv_proximity_domain_info > +numa_node_to_proximity_domain_info(int node) > +{ > + union hv_proximity_domain_info proximity_domain_info; > + > + if (node != NUMA_NO_NODE) { > + proximity_domain_info.domain_id = node_to_pxm(node); > + proximity_domain_info.flags.reserved = 0; > + proximity_domain_info.flags.proximity_info_valid = 1; > + proximity_domain_info.flags.proximity_preferred = 1; > + } else { > + proximity_domain_info.as_uint64 = 0; > + } > + > + return proximity_domain_info; > +} Pop quiz: What are the rules for the 30 bits of uninitialized data of proximity_domain_info.flags in the (node != NUMA_NO_NODE) case? I actually don't know off the top of my head. I generally avoid bitfields, but if they were normal stack-allocated variable space, they'd be garbage. I'd also *much* rather see the "as_uint64 = 0" coded up as a memset() or even explicitly zeroing all the same variables as the other half of the if(). As it stands, it's not 100% obvious that proximity_domain_info is 64 bits and that .as_uint64=0 zeroes the whole thing. It *WOULD* be totally obvious if it were a memset().
On 8/17/2023 4:22 PM, Dave Hansen wrote: > On 8/17/23 15:01, Nuno Das Neves wrote: >> +static inline union hv_proximity_domain_info >> +numa_node_to_proximity_domain_info(int node) >> +{ >> + union hv_proximity_domain_info proximity_domain_info; >> + >> + if (node != NUMA_NO_NODE) { >> + proximity_domain_info.domain_id = node_to_pxm(node); >> + proximity_domain_info.flags.reserved = 0; >> + proximity_domain_info.flags.proximity_info_valid = 1; >> + proximity_domain_info.flags.proximity_preferred = 1; >> + } else { >> + proximity_domain_info.as_uint64 = 0; >> + } >> + >> + return proximity_domain_info; >> +} > > Pop quiz: What are the rules for the 30 bits of uninitialized data of > proximity_domain_info.flags in the (node != NUMA_NO_NODE) case? > > I actually don't know off the top of my head. I generally avoid > bitfields, but if they were normal stack-allocated variable space, > they'd be garbage. I'm not sure what you are getting at here - all the fields are initialized. > > I'd also *much* rather see the "as_uint64 = 0" coded up as a memset() or > even explicitly zeroing all the same variables as the other half of the > if(). As it stands, it's not 100% obvious that proximity_domain_info is > 64 bits and that .as_uint64=0 zeroes the whole thing. It *WOULD* be > totally obvious if it were a memset(). I agree that it could be made clearer with memset(). Now that I'm thinking about it, hv_proximity_domain_info should really just be a struct...then zeroing it is just: struct hv_proximity_domain_info proximity_domain_info = {}; and I can remove the else branch and zeroing the reserved bits.
On 8/17/23 17:17, Nuno Das Neves wrote: >>> + if (node != NUMA_NO_NODE) { >>> + proximity_domain_info.domain_id = node_to_pxm(node); >>> + proximity_domain_info.flags.reserved = 0; >>> + proximity_domain_info.flags.proximity_info_valid = 1; >>> + proximity_domain_info.flags.proximity_preferred = 1; >>> + } else { >>> + proximity_domain_info.as_uint64 = 0; >>> + } >>> + >>> + return proximity_domain_info; >>> +} >> Pop quiz: What are the rules for the 30 bits of uninitialized data of >> proximity_domain_info.flags in the (node != NUMA_NO_NODE) case? >> >> I actually don't know off the top of my head. I generally avoid >> bitfields, but if they were normal stack-allocated variable space, >> they'd be garbage. > I'm not sure what you are getting at here - all the fields are > initialized. Whoops, I somehow missed the reserved field initialization. But, yeah, a struct would be nice instead of the union here.
diff --git a/arch/x86/hyperv/hv_proc.c b/arch/x86/hyperv/hv_proc.c index 68a0843d4750..5ba5ca1b2089 100644 --- a/arch/x86/hyperv/hv_proc.c +++ b/arch/x86/hyperv/hv_proc.c @@ -121,7 +121,6 @@ int hv_call_add_logical_proc(int node, u32 lp_index, u32 apic_id) u64 status; unsigned long flags; int ret = HV_STATUS_SUCCESS; - int pxm = node_to_pxm(node); /* * When adding a logical processor, the hypervisor may return @@ -137,11 +136,8 @@ int hv_call_add_logical_proc(int node, u32 lp_index, u32 apic_id) input->lp_index = lp_index; input->apic_id = apic_id; - input->flags = 0; - input->proximity_domain_info.domain_id = pxm; - input->proximity_domain_info.flags.reserved = 0; - input->proximity_domain_info.flags.proximity_info_valid = 1; - input->proximity_domain_info.flags.proximity_preferred = 1; + input->proximity_domain_info = + numa_node_to_proximity_domain_info(node); status = hv_do_hypercall(HVCALL_ADD_LOGICAL_PROCESSOR, input, output); local_irq_restore(flags); diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c index 1f4fc5f8a819..0cf9f0574495 100644 --- a/drivers/acpi/numa/srat.c +++ b/drivers/acpi/numa/srat.c @@ -48,6 +48,7 @@ int node_to_pxm(int node) return PXM_INVAL; return node_to_pxm_map[node]; } +EXPORT_SYMBOL(node_to_pxm); static void __acpi_map_pxm_to_node(int pxm, int node) { diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h index 233c976344e5..447e7ebe67ee 100644 --- a/include/asm-generic/mshyperv.h +++ b/include/asm-generic/mshyperv.h @@ -21,6 +21,7 @@ #include <linux/types.h> #include <linux/atomic.h> #include <linux/bitops.h> +#include <acpi/acpi_numa.h> #include <linux/cpumask.h> #include <linux/nmi.h> #include <asm/ptrace.h> @@ -28,6 +29,23 @@ #define VTPM_BASE_ADDRESS 0xfed40000 +static inline union hv_proximity_domain_info +numa_node_to_proximity_domain_info(int node) +{ + union hv_proximity_domain_info proximity_domain_info; + + if (node != NUMA_NO_NODE) { + proximity_domain_info.domain_id = node_to_pxm(node); + proximity_domain_info.flags.reserved = 0; + proximity_domain_info.flags.proximity_info_valid = 1; + proximity_domain_info.flags.proximity_preferred = 1; + } else { + proximity_domain_info.as_uint64 = 0; + } + + return proximity_domain_info; +} + struct ms_hyperv_info { u32 features; u32 priv_high;