Re: [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.11.2022 21:25, Julien Grall wrote:
> Hi Jan,
> 
> On 21/11/2022 16:40, Jan Beulich wrote:
>> On 21.11.2022 17:23, Carlo Nonato wrote:
>>> On Mon, Nov 21, 2022 at 4:14 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 21.11.2022 15:50, Carlo Nonato wrote:
>>>>> I want to ask you some questions about this patch because in the previous
>>>>> version me and Julien have discussed how cache colors should be passed in
>>>>> domain creation. You should be able to read that discussion, anyway here 
>>>>> is
>>>>> a link to it
>>>>>
>>>>> https://marc.info/?l=xen-devel&m=166151802002263
>>>>
>>>> I've looked at the first few entries, without finding an answer to ...
>>>>
>>>>> In short, using struct xen_arch_domainconfig works fine only when domctl
>>>>> hypercall is issued. That struct contains a XEN_GUEST_HANDLE so it
>>>>> should point to guest memory and must not be used when creating a domain
>>>>> from Xen itself (i.e. dom0 or dom0less domains). The easy way to go is 
>>>>> then
>>>>> changing the domain_create() signature to require also a color array and 
>>>>> its
>>>>> length to be passed in for these latter cases.
>>>>> Are you ok with that? See below for more comments.
>>>>
>>>> ... my immediate question: Does supplying the colors necessarily need to
>>>> done right at domain creation? Wouldn't it suffice to be done before first
>>>> allocating memory to the new domain, i.e. via a separate domctl (and then
>>>> for Xen-created domains via a separate Xen-internal function, which the
>>>> new domctl handling would also call)? Or do colors also affect the
>>>> allocation of struct domain itself (and pointers hanging off of it)?
>>>
>>> This would be really good. The only problem I can see is the p2m allocation
>>> which is done during domain creation. With the current set of patches it
>>> results in a "Failed to allocate P2M pages" since we want to have p2m tables
>>> allocated with the same color of the domain and a null page is returned
>>> because we have no colors.
>>
>> Hmm, I see. It would seem to me that this p2m init really is happening
>> too early. Ideally domain_create would merely mean creating a largely
>> empty container, with stuff being populated subsequently as necessary.
> 
> The vGIC is not optional. So to me it sounds wrong to defer the decision 
> to after the domain is created.
> 
> It is not clear to me how you would check that mandatory components have 
> been properly initialized.

There could be final checking right before unpausing a domain for the
first time.

>> But I guess this is too much of a re-work to be done in the context
>> here, plus of course I may be overlooking something which actually
>> makes it necessary for domain creation to be structured the way it is
>> right now. (Imo the reason for the early minimal population of the p2m,
>> added only quite recently, wasn't a good one, and the vGIC init would
>> better be deferred. Yet once again I may lack details on why that's not
>> possible.)
> 
> See above for the theoritical part. For the practice part, we need to 
> know the vGIC version at domain creation because it impact the maximum 
> of vCPU we can expose.

Sure - I didn't suggest to leave that information out of domain_create.

> It is also not very clear where this could be initialized. Are you 
> suggesting to add an extra mandatory hypercall? FAOD, I don't think 
> p2m_set_allocation() would be the right place.

I agree with the latter - that would at best be a bodge. And yes, I was
thinking of having a separate domctl for that purpose.

Jan