Re: [V8 PATCH 2/8] pvh dom0: construct_dom0 changes

[George Dunlap <george.dunlap@eu.citrix.com>]

On 04/05/2014 01:53 AM, Mukesh Rathor wrote:
> On Thu, 27 Mar 2014 16:30:37 +0100
> Tim Deegan <tim@xen.org> wrote:
>
>> At 15:04 +0000 on 27 Mar (1395929085), Jan Beulich wrote:
>>>>>> On 27.03.14 at 11:55, <george.dunlap@eu.citrix.com> wrote:
>>>> On 03/27/2014 10:14 AM, Jan Beulich wrote:
>>>>>>>> On 26.03.14 at 20:05, <george.dunlap@eu.citrix.com> wrote:
>>>>>> --- a/xen/arch/x86/mm/hap/hap.c
>>>>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>>>>> @@ -589,6 +589,21 @@ int hap_domctl(struct domain *d,
>>>>>> xen_domctl_shadow_op_t
>>>> *sc,
>>>>>>         }
>>>>>>     }
>>>>>>     
>>>>>> +void __init hap_set_pvh_alloc_for_dom0(struct domain *d,
>>>>>> +                                       unsigned long num_pages)
>>>>>> +{
>>>>>> +    int rc;
>>>>>> +    unsigned long memkb = num_pages * (PAGE_SIZE / 1024);
>>>>>> +
>>>>>> +    /* Copied from: libxl_get_required_shadow_memory() */
>>>>>> +    memkb = 4 * (256 * d->max_vcpus + 2 * (memkb / 1024));
>>>>>> +    num_pages = ( (memkb + 1023) / 1024) << (20 - PAGE_SHIFT);
>>>>>> +    paging_lock(d);
>>>>>> +    rc = hap_set_allocation(d, num_pages, NULL);
>>>>>> +    paging_unlock(d);
>>>>>>
>>>>>>
>>>>>> The calculation for how many pages of shadow memory are needed
>>>>>> logically belongs in domain_build.c, not hap.c.
>>>>> Iirc it was me requesting it to be moved here, on the basis that
>>>>> the calculation should match the one for other domains, and
>>>>> hence be done alongside that for other domains. domain_build.c
>>>>> shouldn't carry HAP-specific knowledge imo.
>>>> I thought that might be the case, which is why I apologized in
>>>> advance for not reading the previous thread.
>>>>
>>>> The calculation should indeed match that done for other domains;
>>>> however, as the comment clearly says, this calculation is done in
>>>> libxl, not in Xen at all.  Everything done to build dom0 in Xen
>>>> which corresponds to things done by the toolstack to build a domU
>>>> logically belongs in domain_build.c.
>>>>
>>>> Putting it in hap.c would be wrong in any case; what would you do
>>>> when we enable shadow mode for HAP dom0?
>>> The function calls hap_set_allocation(), so it's already
>>> HAP-specific.
>> It really ought to be calling a paging_set_allocation() wrapper;
>> there's nothing _inherently_ HAP-specific about PVH.  Right now that
>> wrapper doesn't exist, because that path goes via paging_domctl() for
>> other domains.
> Correct, PVH requires HAP at present, and I like when the code makes
> that clear. In general, my approach is to make change in future when
> demanded, thus, when shadow is added, it can be rearranged with small
> effort.
>
> Anyways, I'm flexible. Jan, final word: Leave it as is (my first pref),
> move to domain_build.c, or put a wrapper(my last preference) ?

I'm fine with the code in domain_build.c calling hap_set_allocation() 
directly for the time being, and implementing a paging_set_allocation() 
wrapper later; but I think the code doing the calculation should be in 
domain_build.c.

  -George