On Fri, 26 Jun 2026 at 11:22, Paolo Bonzini <[email protected]> wrote:
>
> From: Magnus Kulke <[email protected]>
>
> In this change the we rewrite the existing MSR logic to make MSRs
> migratable:
>
> - we map them on existing QEMU fields in the CPU. A table and a macro
> MSHV_ENV_FIELD is used to associate a HV register name to the their msr
> index and their offset in the cpu state struct. The list is not
> exhaustive and will be extended in follow-up commits.
> - mshv_set/get_msrs() fns are called in the arch_load/store_vcpu_state()
> fns. they use use generic registers ioctl's and map the input/output
> via load/store_to/from_env() from/to the hv register content to the
> cpu state representation.
> - init_msrs() has been moved from mshv-vcpu to the msr source file
> - we need to perform some filtering of MSR because before writing and
> reading, because the hvcalls will fail if the partition doesn't
> support a given MSRs.
> - Some MSRs are partition-wide and so we will only write the to on the
> BSP.
>
> Signed-off-by: Magnus Kulke <[email protected]>
> Link:
> https://lore.kernel.org/r/[email protected]
> Signed-off-by: Paolo Bonzini <[email protected]>
Hi; Coverity complains about use of an uninitialized struct
field in this commit (CID 1660875):
> +int mshv_get_msrs(CPUState *cpu)
> +{
> + int ret = 0;
> + size_t n_assocs = ARRAY_SIZE(msr_env_map);
> + struct hv_register_assoc assocs[ARRAY_SIZE(msr_env_map)];
This array has no initializer, so the elements start out
with no initialized fields.
> + size_t i, j;
> + uint32_t name;
> +
> + set_hv_name_in_assocs(assocs, n_assocs);
This function call (as its name suggests) sets the "name" field
in each element of the array, but no other fields.
> +
> + /* Filter out MSRs that cannot be read */
> + for (i = 0, j = 0; i < n_assocs; i++) {
> + name = assocs[i].name;
> +
> + if (!msr_supported(name)) {
> + continue;
> + }
> +
> + if (j != i) {
> + assocs[j] = assocs[i];
Here we do a full structure copy, which accesses (copying)
the uninitialized fields in assocs[i] ("reserved1", "reserved2",
"value").
Either the function should ensure it's initializing all
the fields, or else if we know and expect only the "name"
field to be initialized we should copy only the "name"
field, not the whole struct.
> + }
> + j++;
> + }
> + n_assocs = j;
> +
> + ret = mshv_get_generic_regs(cpu, assocs, n_assocs);
> + if (ret < 0) {
> + error_report("Failed to get MSRs");
> + return -errno;
> + }
> +
> + store_in_env(cpu, assocs, n_assocs);
> +
> + return 0;
> +}
thanks
-- PMM
