Re: [RFC PATCH 08/16] target/arm/kvm-rme: Populate the realm with boot images

[Richard Henderson]

On 1/27/23 05:07, Jean-Philippe Brucker wrote:
> Initialize the GPA space and populate it with boot images (kernel,
> initrd, firmware, etc). Populating has to be done at VM start time,
> because the images are loaded during reset by rom_reset()
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>   target/arm/kvm_arm.h |  6 ++++
>   target/arm/kvm-rme.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 85 insertions(+)
>
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index e4dc7fbb8d..cec6500603 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -371,6 +371,7 @@ int kvm_arm_set_irq(int cpu, int irqtype, int irq, int 
> level);
>   
>   int kvm_arm_rme_init(ConfidentialGuestSupport *cgs, Error **errp);
>   int kvm_arm_rme_vm_type(MachineState *ms);
> +void kvm_arm_rme_add_blob(hwaddr start, hwaddr src_size, hwaddr dst_size);
>   
>   bool kvm_arm_rme_enabled(void);
>   int kvm_arm_rme_vcpu_init(CPUState *cs);
> @@ -458,6 +459,11 @@ static inline int kvm_arm_rme_vm_type(MachineState *ms)
>   {
>       return 0;
>   }
> +
> +static inline void kvm_arm_rme_add_blob(hwaddr start, hwaddr src_size,
> +                                        hwaddr dst_size)
> +{
> +}
>   #endif
>   
>   static inline const char *gic_class_name(void)
> diff --git a/target/arm/kvm-rme.c b/target/arm/kvm-rme.c
> index 3833b187f9..c8c019f78a 100644
> --- a/target/arm/kvm-rme.c
> +++ b/target/arm/kvm-rme.c
> @@ -9,6 +9,7 @@
>   #include "exec/confidential-guest-support.h"
>   #include "hw/boards.h"
>   #include "hw/core/cpu.h"
> +#include "hw/loader.h"
>   #include "kvm_arm.h"
>   #include "migration/blocker.h"
>   #include "qapi/error.h"
> @@ -19,12 +20,22 @@
>   #define TYPE_RME_GUEST "rme-guest"
>   OBJECT_DECLARE_SIMPLE_TYPE(RmeGuest, RME_GUEST)
>   
> +#define RME_PAGE_SIZE qemu_real_host_page_size()
> +
>   typedef struct RmeGuest RmeGuest;
>   
>   struct RmeGuest {
>       ConfidentialGuestSupport parent_obj;
>   };
>   
> +struct RmeImage {
> +    hwaddr base;
> +    hwaddr src_size;
> +    hwaddr dst_size;
> +};
> +
> +static GSList *rme_images;
> +
>   static RmeGuest *cgs_to_rme(ConfidentialGuestSupport *cgs)
>   {
>       if (!cgs) {
> @@ -51,6 +62,38 @@ static int rme_create_rd(RmeGuest *guest, Error **errp)
>       return ret;
>   }
>   
> +static void rme_populate_realm(gpointer data, gpointer user_data)
> +{
> +    int ret;
> +    struct RmeImage *image = data;
> +    struct kvm_cap_arm_rme_init_ipa_args init_args = {
> +        .init_ipa_base = image->base,
> +        .init_ipa_size = image->dst_size,
> +    };
> +    struct kvm_cap_arm_rme_populate_realm_args populate_args = {
> +        .populate_ipa_base = image->base,
> +        .populate_ipa_size = image->src_size,
> +    };
> +
> +    ret = kvm_vm_enable_cap(kvm_state, KVM_CAP_ARM_RME, 0,
> +                            KVM_CAP_ARM_RME_INIT_IPA_REALM,
> +                            (intptr_t)&init_args);
> +    if (ret) {
> +        error_setg_errno(&error_fatal, -ret,
> +                         "RME: failed to initialize GPA range (0x%"HWADDR_PRIx", 
> 0x%"HWADDR_PRIx")",
> +                         image->base, image->dst_size);
> +    }

Using error_fatal with error_setg is discouraged.
This should be error_report(), exit().


> +
> +    ret = kvm_vm_enable_cap(kvm_state, KVM_CAP_ARM_RME, 0,
> +                            KVM_CAP_ARM_RME_POPULATE_REALM,
> +                            (intptr_t)&populate_args);
> +    if (ret) {
> +        error_setg_errno(&error_fatal, -ret,
> +                         "RME: failed to populate realm (0x%"HWADDR_PRIx", 
> 0x%"HWADDR_PRIx")",
> +                         image->base, image->src_size);
> +    }
> +}
> +
>   static void rme_vm_state_change(void *opaque, bool running, RunState state)
>   {
>       int ret;
> @@ -72,6 +115,9 @@ static void rme_vm_state_change(void *opaque, bool running, 
> RunState state)
>           }
>       }
>   
> +    g_slist_foreach(rme_images, rme_populate_realm, NULL);
> +    g_slist_free_full(g_steal_pointer(&rme_images), g_free);

I suppose this technically works because you clear the list, and thus the hook is called 
only on the first transition to RUNNING.  On all subsequent transitions the list is empty.

I see that i386 sev does this immediately during machine init, alongside the kernel setup. 
 Since kvm_init has already been called, that seems workable, rather than queuing 
anything for later.

But I think ideally this would be handled generically in (approximately) 
kvm_cpu_synchronize_post_init, looping over all blobs.  This would handle any usage of 
'-device loader,...', instead of the 4 specific things you handle in the next patch.


r~