Re: [PATCH v7 7/9] common/domain: add a domain context record for shared_info...

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.09.2020 19:29, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 26 August 2020 14:58
>>
>> On 18.08.2020 12:30, Paul Durrant wrote:
>>> v7:
>>>  - Only restore vcpu_info and arch sub-structures for PV domains, to match
>>>    processing of SHARED_INFO in xc_sr_restore_x86_pv.c
>>
>> Since you point out this original logic, ...
>>
>>> +static int load_shared_info(struct domain *d, struct domain_context *c)
>>> +{
>>> +    struct domain_shared_info_context ctxt;
>>> +    size_t hdr_size = offsetof(typeof(ctxt), buffer);
>>> +    unsigned int i;
>>> +    int rc;
>>> +
>>> +    rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, &i);
>>> +    if ( rc )
>>> +        return rc;
>>> +
>>> +    if ( i ) /* expect only a single instance */
>>> +        return -ENXIO;
>>> +
>>> +    rc = domain_load_data(c, &ctxt, hdr_size);
>>> +    if ( rc )
>>> +        return rc;
>>> +
>>> +    if ( ctxt.buffer_size > sizeof(shared_info_t) ||
>>> +         (ctxt.flags & ~DOMAIN_SAVE_32BIT_SHINFO) )
>>> +        return -EINVAL;
>>> +
>>> +    if ( ctxt.flags & DOMAIN_SAVE_32BIT_SHINFO )
>>> +    {
>>> +#ifdef CONFIG_COMPAT
>>> +        has_32bit_shinfo(d) = true;
>>> +#else
>>> +        return -EINVAL;
>>> +#endif
>>> +    }
>>> +
>>> +    if ( is_pv_domain(d) )
>>> +    {
>>> +        shared_info_t *shinfo = xmalloc(shared_info_t);
>>> +
>>> +        rc = domain_load_data(c, shinfo, sizeof(*shinfo));
>>> +        if ( rc )
>>> +        {
>>> +            xfree(shinfo);
>>> +            return rc;
>>> +        }
>>> +
>>> +#ifdef CONFIG_COMPAT
>>> +        if ( has_32bit_shinfo(d) )
>>> +        {
>>> +            memcpy(&d->shared_info->compat.vcpu_info,
>>> +                   &shinfo->compat.vcpu_info,
>>> +                   sizeof(d->shared_info->compat.vcpu_info));
>>> +            memcpy(&d->shared_info->compat.arch,
>>> +                   &shinfo->compat.arch,
>>> +                   sizeof(d->shared_info->compat.vcpu_info));
>>> +        }
>>> +        else
>>> +        {
>>> +            memcpy(&d->shared_info->native.vcpu_info,
>>> +                   &shinfo->native.vcpu_info,
>>> +                   sizeof(d->shared_info->native.vcpu_info));
>>> +            memcpy(&d->shared_info->native.arch,
>>> +                   &shinfo->native.arch,
>>> +                   sizeof(d->shared_info->native.arch));
>>> +        }
>>> +#else
>>> +        memcpy(&d->shared_info->vcpu_info,
>>> +               &shinfo->vcpu_info,
>>> +               sizeof(d->shared_info->vcpu_info));
>>> +        memcpy(&d->shared_info->arch,
>>> +               &shinfo->arch,
>>> +               sizeof(d->shared_info->shared));
>>> +#endif
>>
>> ... where does the rest of that logic (resetting of
>> arch.pfn_to_mfn_frame_list_list, evtchn_pending, evtchn_mask, and
>> evtchn_pending_sel) get done? Or why is it not needed anymore?
> 
> The resetting logic is still in xc_sr_restore_x86_pv.c (see patch #6).
> It's going to need to stay there anyway to deal with older streams so
> I made it common to both cases; it seems slightly separate from
> restoring the shared info.

I guess I at least don't fully agree: The resetting is part of restoring,
as it effectively determines which parts are restored and which parts
are simply set (not truly reset, but I agree the perception may change
depending on whose position you take). Hence at the very least this
aspect wants clearly spelling out in the description, I think. But
really I'd prefer if for old streams libxc took care of (all of) it,
and if for new streams all logic lived in the hypervisor.

Jan