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

From: Greg Kurz <groug@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: pair@us.ibm.com, "Marcelo Tosatti" <mtosatti@redhat.com>,
	brijesh.singh@amd.com, kvm@vger.kernel.org, david@redhat.com,
	qemu-devel@nongnu.org, frankja@linux.ibm.com,
	pragyansri.pathi@intel.com, mst@redhat.com,
	mdroth@linux.vnet.ibm.com, pasic@linux.ibm.com,
	"Christian Borntraeger" <borntraeger@de.ibm.com>,
	andi.kleen@intel.com, thuth@redhat.com,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	richard.henderson@linaro.org, dgilbert@redhat.com,
	qemu-s390x@nongnu.org, jun.nakajima@intel.com,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Cornelia Huck" <cohuck@redhat.com>,
	qemu-ppc@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>
Subject: Re: [PATCH v6 04/13] confidential guest support: Move side effect out of machine_set_memory_encryption()
Date: Tue, 12 Jan 2021 11:39:47 +0100	[thread overview]
Message-ID: <20210112113947.4419cca2@bahia.lan> (raw)
In-Reply-To: <20210112044508.427338-5-david@gibson.dropbear.id.au>

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);
>  }