Re: [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/01/2020 14:25, Jan Beulich wrote:
> On 17.12.2019 21:15, Andrew Cooper wrote:
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -123,16 +123,12 @@ struct xc_dom_image {
>>  
>>      /* other state info */
>>      uint32_t f_active[XENFEAT_NR_SUBMAPS];
>> +
>>      /*
>> -     * p2m_host maps guest physical addresses an offset from
>> -     * rambase_pfn (see below) into gfns.
> The removal of this part of the comment and ...
>
>> -     * For a pure PV guest this means that it maps GPFNs into MFNs for
>> -     * a hybrid guest this means that it maps GPFNs to GPFNS.
>> -     *
>> -     * Note that the input is offset by rambase.
>> +     * pv_p2m is specific to x86 PV guests, and maps GFNs to MFNs.  It is
>> +     * eventually copied into guest context.
>>       */
>> -    xen_pfn_t *p2m_host;
>> +    xen_pfn_t *pv_p2m;
>>  
>>      /* physical memory
>>       *
>> @@ -433,9 +429,12 @@ static inline xen_pfn_t xc_dom_p2m(struct xc_dom_image 
>> *dom, xen_pfn_t pfn)
>>  {
>>      if ( xc_dom_translated(dom) )
>>          return pfn;
>> -    if (pfn < dom->rambase_pfn || pfn >= dom->rambase_pfn + 
>> dom->total_pages)
>> +
>> +    /* x86 PV only now. */
>> +    if ( pfn >= dom->total_pages )
>>          return INVALID_MFN;
>> -    return dom->p2m_host[pfn - dom->rambase_pfn];
>> +
>> +    return dom->pv_p2m[pfn];
>>  }
> ... the dropping of all uses of rambase_pfn here make it look
> like you're doing away with that concept altogether. Is this
> really correct?

Well - it is effectively dead code here, because of the
xc_dom_translated() in context above, and it being 0 outside of ARM.

The (nonzero) value is used by other bits of ARM code.

>  If so, I guess you want to
> - say a word in this regard in the description, the more that
>   you don't fully get rid of this (the structure field and
>   some uses elsewhere remain),
> - drop/adjust the respective comment fragment next to the
>   field a little further down in the structure.

The domain builder is made almost exclusively of bitrot, but I don't
have an ARM system to do any testing on.  Given how fragile the code is,
I'm not comfortable doing speculative deletion of code I'm not certain
is unused.

>
>> @@ -1245,11 +1245,11 @@ static int meminit_pv(struct xc_dom_image *dom)
>>          return -EINVAL;
>>      }
>>  
>> -    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->p2m_size);
>> -    if ( dom->p2m_host == NULL )
>> +    dom->pv_p2m = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->p2m_size);
> Since you're touching the line anyway, perhaps better
> sizeof(*dom->pv_p2m)?

Will fix.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel