Re: [PATCH v10 05/11] common/domain: add a domain context record for shared_info...

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.10.2020 20:57, Paul Durrant wrote:
> @@ -1671,6 +1672,118 @@ int continue_hypercall_on_cpu(
>      return 0;
>  }
>  
> +static int save_shared_info(struct domain *d, struct domain_ctxt_state *c,
> +                            bool dry_run)
> +{
> +#ifdef CONFIG_COMPAT
> +    struct domain_context_shared_info s = {
> +        .flags = has_32bit_shinfo(d) ? DOMAIN_CONTEXT_32BIT_SHARED_INFO : 0,
> +    };
> +    size_t size = has_32bit_shinfo(d) ?
> +        sizeof(struct compat_shared_info) :
> +        sizeof(struct shared_info);
> +#else
> +    struct domain_context_shared_info s = {};
> +    size_t size = sizeof(struct shared_info);

All of these would imo better be expressed in terms of d->shared_info.
While chances are zero that these types will change in any way, it
still sets a bad precedent for people seeing this and then introducing
similar disconnects elsewhere. (Same in the load handling then.)

> +static int load_shared_info(struct domain *d, struct domain_ctxt_state *c)
> +{
> +    struct domain_context_shared_info s = {};
> +    size_t size;
> +    unsigned int i;
> +    int rc;
> +
> +    rc = domain_load_ctxt_rec_begin(c, DOMAIN_CONTEXT_SHARED_INFO, &i);
> +    if ( rc )
> +        return rc;
> +
> +    if ( i ) /* expect only a single instance */
> +        return -ENXIO;
> +
> +    rc = domain_load_ctxt_rec_data(c, &s, offsetof(typeof(s), buffer));
> +    if ( rc )
> +        return rc;
> +
> +    if ( s.flags & ~DOMAIN_CONTEXT_32BIT_SHARED_INFO )
> +        return -EINVAL;
> +
> +    if ( s.flags & DOMAIN_CONTEXT_32BIT_SHARED_INFO )
> +    {
> +#ifdef CONFIG_COMPAT
> +        d->arch.has_32bit_shinfo = true;
> +        size = sizeof(struct compat_shared_info);

I realize this has been more or less this way already in prior
versions, but aren't you introducing a way to have a degenerate
64-bit PV guest with 32-bit shared info (or vice versa), in that
shared info bitness isn't strictly tied to guest bitness anymore?
Rejecting this case may not need to live here, but it needs to be
present / added somewhere.

> +#else
> +        return -EINVAL;
> +#endif
> +    }
> +    else
> +        size = sizeof(struct shared_info);
> +
> +    if ( is_pv_domain(d) )
> +    {
> +        shared_info_t *shinfo = xzalloc(shared_info_t);
> +
> +        if ( !shinfo )
> +            return -ENOMEM;
> +
> +        rc = domain_load_ctxt_rec_data(c, shinfo, size);
> +        if ( rc )
> +            goto out;
> +
> +        memcpy(&shared_info(d, vcpu_info), &__shared_info(d, shinfo, 
> vcpu_info),
> +               sizeof(shared_info(d, vcpu_info)));
> +        memcpy(&shared_info(d, arch), &__shared_info(d, shinfo, arch),
> +               sizeof(shared_info(d, arch)));
> +
> +        memset(&shared_info(d, evtchn_pending), 0,
> +               sizeof(shared_info(d, evtchn_pending)));
> +        memset(&shared_info(d, evtchn_mask), 0xff,
> +               sizeof(shared_info(d, evtchn_mask)));
> +
> +#ifdef CONFIG_X86
> +        shared_info(d, arch.pfn_to_mfn_frame_list_list) = 0;
> +#endif
> +        for ( i = 0; i < XEN_LEGACY_MAX_VCPUS; i++ )
> +            shared_info(d, vcpu_info[i].evtchn_pending_sel) = 0;

Again I realize this has been this way in earlier versions, and
it was also me to ask for streamlining the code, but is this
actually correct? I ask in particular in light of this comment

/*
 * Compat field is never larger than native field, so cast to that as it
 * is the largest memory range it is safe for the caller to modify without
 * further discrimination between compat and native cases.
 */

in xen/shared.h, next to the __shared_info() #define. I can't
help thinking that you'll fill only half of some of the fields
in the 64-bit case.

> @@ -58,6 +59,16 @@ struct domain_context_start {
>      uint32_t xen_major, xen_minor;
>  };
>  
> +struct domain_context_shared_info {
> +    uint32_t flags;
> +
> +#define _DOMAIN_CONTEXT_32BIT_SHARED_INFO 0

Is this separate constant actually needed for anything?

Speaking of which - wouldn't all your additions to this header
better be proper name spacing citizens, by having xen_ / XEN_
prefixes?

Jan