Re: [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.01.2021 19:25, Andrew Cooper wrote:
> On 19/10/2020 14:46, Jan Beulich wrote:
>> On 08.10.2020 20:57, Paul Durrant wrote:
>>> --- /dev/null
>>> +++ b/xen/include/public/save.h
>>> @@ -0,0 +1,66 @@
>>> +/*
>>> + * save.h
>>> + *
>>> + * Structure definitions for common PV/HVM domain state that is held by 
>>> Xen.
>>> + *
>>> + * Copyright Amazon.com Inc. or its affiliates.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>>> copy
>>> + * of this software and associated documentation files (the "Software"), to
>>> + * deal in the Software without restriction, including without limitation 
>>> the
>>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
>>> and/or
>>> + * sell copies of the Software, and to permit persons to whom the Software 
>>> is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included 
>>> in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>>> OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
>>> THE
>>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>> + * DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +#ifndef XEN_PUBLIC_SAVE_H
>>> +#define XEN_PUBLIC_SAVE_H
>>> +
>>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>>> +
>>> +#include "xen.h"
>>> +
>>> +/*
>>> + * C structures for the Domain Context v1 format.
>>> + * See docs/specs/domain-context.md
>>> + */
>>> +
>>> +struct domain_context_record {
>>> +    uint32_t type;
>>> +    uint32_t instance;
>>> +    uint64_t length;
>> Should this be uint64_aligned_t, such that alignof() will
>> produce consistent values regardless of bitness of the invoking
>> domain?
> 
> Does it matter?  Its just a bitstream, and can appear in the migration
> fd at any arbitrary alignment.
> 
> What matters is that the structure is aligned appropriately for the
> bitness of code operating on these fields.
> 
> Even with the tools ABI fixed to allow a 32-on-64-on-64  toolstack to
> function, I'm not sure that excess alignment would be appropriate.  Sure
> - it would be more efficient for 32bit code to align to the 8 byte
> boundary for the benefit of a 64bit Xen's copy_from_user(), but this
> alignment happens anyway because of how hypercall buffers work.

It _probably_ doesn't matter (hence me having put this as a
question), but a struct in the ABI doesn't mandate how it is
going to be used. I don't expect this to become the (or part
of an) argument to a hypercall. I also don't expect anyone
needing to use alignof() on it. But by not following the
common model for the tools-only parts of the public
interface we'd outright exclude such uses down the road.

Jan