Re: [PATCH v9 08/15] tools/libxc: x86 PV save code

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/04/15 12:13, Ian Campbell wrote:
> On Fri, 2015-04-10 at 18:16 +0100, Andrew Cooper wrote:
>> Save the x86 PV specific parts of a domain.  This is the X86_PV_INFO record,
>> the P2M_FRAMES, the X86_PV_SHARED_INFO, the three different VCPU context
>> records, and the MSR records.
>>
>> The normalise_page callback used by the common code when writing the PAGE_DATA
>> records, converts MFNs in page tables to PFNs.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
>
> With one question:
>
>> +#ifdef __x86_64__
>> +            /* 64bit toolstack, 32bit guest.  Expand any INVALID_MFN. */
>> +            uint32_t s = ((uint32_t *)src)[x];
>> +
>> +            dst[x] = s == ~0U ? INVALID_MFN : s;
>> +#else
>> +            /* 32bit toolstack, 64bit guest.  Truncate their pointers */
>> +            dst[x] = ((uint64_t *)src)[x];
>> +#endif
> Would it not be better to propagate an error instead of truncating? Or
> at least log the first instance of such?

There are valid truncations to be performed, from 64bit INVALID_MFN to
32bit INVALID_MFN.

For the invalid truncations, that is a little harder.  The post-trucated
values are the ones which are checked for validity (so is at least safe
from that point of view), and will fail because of a p2m/m2p mismatch,
completely aborting the migration.

Truncation will only actually occur if a domain has pages above the 16TB
(44 bits) boundary, and a 32bit dom0 is simply not going to function on
a server that size.

In other words, I don't think there is a practical situation where
truncation would be an issue.

~Andrew