Re: [PATCH 02/12] xen/arm: add cache coloring initialization for domains

[Julien Grall <julien@xen.org>]

On 07/11/2022 13:44, Carlo Nonato wrote:
> Hi Julien,

Hi Carlo,

> On Tue, Oct 25, 2022 at 1:15 PM Julien Grall <julien@xen.org> wrote:
>> On 25/10/2022 11:53, Carlo Nonato wrote:
>>> Hi Julien,
>>>
>>> On Fri, Oct 21, 2022 at 8:02 PM Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi Carlo,
>>>>
>>>> On 26/08/2022 13:51, Carlo Nonato wrote:
>>>>> This commit adds array pointers to domains as well as to the hypercall
>>>>> and configuration structure employed in domain creation. The latter is used
>>>>> both by the toolstack and by Xen itself to pass configuration data to the
>>>>> domain creation function, so the XEN_GUEST_HANDLE macro must be adopted to be
>>>>> able to access guest memory in the first case. This implies special care for
>>>>> the copy of the configuration data into the domain data, meaning that a
>>>>> discrimination variable for the two possible code paths (coming from Xen or
>>>>> from the toolstack) is needed.
>>>>
>>>> So this means that a toolstack could set from_guest. I know the
>>>> toolstack is trusted... However, we should try to limit when the trust
>>>> when this is possible.
>>>>
>>>> In this case, I would consider to modify the prototype of
>>>> domain_create() to pass internal information.
>>>
>>> Doing as you said isn't a bit too invasive? I should also change all the call
>>> sites of domain_create() and this means x86 too.
>>
>> Yes there will be a few calls to modify. But this is better than hacking
>> the hypercall interface to cater for internal use.
>>
>>> Isn't there an easier way to spot a guest address? Maybe just looking at the
>>> address value...
>>
>> HVM/Arm guest have a separate address space. So it is not possible to
>> differentiate between guest vs hypervisor address.
>>
>>> Or maybe adding an internal flag to the do_domctl() path.
>> IIUC, this flag would indicate whether the XEN_GUEST_HANDLE() is an
>> hypervisor or guest address. Is that correct?
>>
>> If so, I dislike it. I am not sure what the other maintainers think, but
>> personally updating domain_create() is my preferred way.
> 
> Sorry to bother you again on this topic, but I thought of a way to get rid of
> the "from_guest" field which I hope is simple enough to convince you.
> I can call copy_from_guest() *only* in domctl.c, overwriting the colors
> pointer with a new, Xen allocated, array.
> This lets me simplify the logic in domain_coloring_init() since all the arrays
> coming to it via the domainconfig struct are allocated in Xen memory only.
> It's still a bit of a hack since I'm using the XEN_GUEST_HANDLE as a normal
> Xen pointer, but it's by far less hacky than before and doesn't have the trust
> problem.

You don't have the trust problem but you are still mixing guest handle 
and xen pointer. I continue dislike this because this a gross hack that 
may save you some effort today but will be a nightmare to 
review/use/maintain (the developer will have to remember whether the 
field contain a guest address or xen address).

Cheers,

-- 
Julien Grall