Re: [RFC PATCH 03/16] target/arm/kvm-rme: Initialize realm

[Richard Henderson]

On 1/27/23 05:07, Jean-Philippe Brucker wrote:
> +static inline int kvm_arm_rme_init(ConfidentialGuestSupport *cgs, Error **errp)
> +{
> +    return 0;
> +}
> +
> +static inline int kvm_arm_rme_vm_type(MachineState *ms)
> +{
> +    return 0;
> +}

Should the stubs really return 0, or g_assert_not_reached()?

> +static RmeGuest *cgs_to_rme(ConfidentialGuestSupport *cgs)
> +{
> +    if (!cgs) {
> +        return NULL;
> +    }
> +    return (RmeGuest *)object_dynamic_cast(OBJECT(cgs), TYPE_RME_GUEST);
> +}

Direct usage of object_dynamic_cast is usually not correct.

Normally one would use DECLARE_INSTANCE_CHECKER to define the entire function.  But if you 
prefer to type-check the input argument as ConfidentialGuestSupport I can see defining 
your own function.  But in that case I think you probably want to use OBJECT_CHECK, which 
asserts that the cast succeeds.

But then I see that cgs_to_rme is, in all instances so far, also used as a 
predicate.


> +bool kvm_arm_rme_enabled(void)
> +{
> +    ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
> +
> +    return !!cgs_to_rme(cgs);
> +}

Ah, hmm.  Well, probably wouldn't want an assert here.

At present I would expect exactly one object class to be present in the 
qemu-system-aarch64 binary that would pass the machine_check_confidential_guest_support 
test done by core code.  But we are hoping to move toward a heterogeneous model where e.g. 
the TYPE_SEV_GUEST object might be discoverable within the same executable.

Therefore, if cgs_to_rme above uses assert, this might use object_dynamic_cast here 
directly.  It seems like we ought to have a boolean test for this kind of thing, but no.


> +int kvm_arm_rme_init(ConfidentialGuestSupport *cgs, Error **errp)
> +{
> +    int ret;
> +    static Error *rme_mig_blocker;
> +    RmeGuest *guest = cgs_to_rme(cgs);
> +
> +    if (!guest) {
> +        /* Either no cgs, or another confidential guest type */
> +        return 0;
> +    }
> +
> +    if (!kvm_enabled()) {
> +        error_setg(errp, "KVM required for RME");
> +        return -ENODEV;
> +    }

I think this null check, and the kvm_enabled check, belong in virt.c.
You'll not get the error with the !CONFIG_KVM version of this function above.


r~