Re: [PATCH v6 04/13] confidential guest support: Move side effect out of machine_set_memory_encryption()

[Greg Kurz <groug@kaod.org>]

On Tue, 12 Jan 2021 15:44:59 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> When the "memory-encryption" property is set, we also disable KSM
> merging for the guest, since it won't accomplish anything.
> 
> We want that, but doing it in the property set function itself is
> thereoretically incorrect, in the unlikely event of some configuration
> environment that set the property then cleared it again before
> constructing the guest.
> 
> More importantly, it makes some other cleanups we want more difficult.
> So, instead move this logic to machine_run_board_init() conditional on
> the final value of the property.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/core/machine.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index de3b8f1b31..8909117d80 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -437,14 +437,6 @@ static void machine_set_memory_encryption(Object *obj, const char *value,
>  
>      g_free(ms->memory_encryption);
>      ms->memory_encryption = g_strdup(value);
> -
> -    /*
> -     * With memory encryption, the host can't see the real contents of RAM,
> -     * so there's no point in it trying to merge areas.
> -     */
> -    if (value) {
> -        machine_set_mem_merge(obj, false, errp);
> -    }
>  }
>  
>  static bool machine_get_nvdimm(Object *obj, Error **errp)
> @@ -1166,6 +1158,15 @@ void machine_run_board_init(MachineState *machine)
>                      cc->deprecation_note);
>      }
>  
> +    if (machine->memory_encryption) {
> +        /*
> +         * With memory encryption, the host can't see the real
> +         * contents of RAM, so there's no point in it trying to merge
> +         * areas.
> +         */
> +        machine_set_mem_merge(OBJECT(machine), false, &error_abort);
> +    }
> +
>      machine_class->init(machine);
>      phase_advance(PHASE_MACHINE_INITIALIZED);
>  }