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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.01.2020 16:02, Andrew Cooper wrote:
> 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.

My remark was about this (non-Arm) part of the comment:

     * An x86 PV guest has one or more blocks of physical RAM,
     * consisting of total_pages starting at rambase_pfn. The start
     * address and size of each block is controlled by vNUMA
     * structures.

I did in no way mean to ask for speculative deletion of code.

Jan

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