Re: [PATCH 05/12] libxenguest: complete loops in xc_map_domain_meminfo()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.06.2021 20:30, Andrew Cooper wrote:
> On 25/06/2021 14:19, Jan Beulich wrote:
>> minfo->p2m_size may have more than 31 significant bits. Change the
>> induction variable to unsigned long, and (largely for signed-ness
>> consistency) a helper variable to unsigned int.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/tools/libs/guest/xg_domain.c
>> +++ b/tools/libs/guest/xg_domain.c
>> @@ -40,7 +40,7 @@ int xc_map_domain_meminfo(xc_interface *
>>      xc_dominfo_t info;
>>      shared_info_any_t *live_shinfo;
>>      xen_capabilities_info_t xen_caps = "";
>> -    int i;
>> +    unsigned long i;
>>  
>>      /* Only be initialized once */
>>      if ( minfo->pfn_type || minfo->p2m_table )
>> @@ -116,12 +116,12 @@ int xc_map_domain_meminfo(xc_interface *
>>      /* Retrieve PFN types in batches */
>>      for ( i = 0; i < minfo->p2m_size ; i+=1024 )
>>      {
>> -        int count = ((minfo->p2m_size - i ) > 1024 ) ?
>> -                        1024: (minfo->p2m_size - i);
>> +        unsigned int count = ((minfo->p2m_size - i) > 1024) ?
>> +                             1024 : (minfo->p2m_size - i);
> 
> min().

min() using 1024UL or MIN()? (I'll use the former unless you tell
me otherwise.)

> Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks.

> This whole infrastructure is almost abandoned, and broken.  Its used by
> xen-mfndump (debugging only) and xen-hptool mem-offline.
> 
> The mem-offline functionally cannot possibly work usefully.  It is PV
> only, despite not having an HVM check, and in particular reads the dead
> page in an attempt to restore the contents elsewhere.  There is also no
> thought given to writes from outside sources, such as DMA from
> passthrough or a different dom0 foreign mapping.
> 
> This is perhaps ok as an academic demonstration of "can I shuffle memory
> behind an alive VM in ideal circumstances", but will be killed by the
> dom0 kernel if you ever try running it to resolve a real memory error on
> a VM, because there is no possibility of recovering the data.
> 
> The mem-offline functionality needs deleting.  It isn't production
> ready, and can't credibly be made so.

I definitely agree; I'm merely trying to address an anomaly found
while auditing the code for certain properties, without any claim
that afterwards any of this would really work.

Jan