Re: [PATCH for-next 07/16] xen/arm: Introduce copy_to_guest_phys_flush_dcache

[Julien Grall <julien.grall@linaro.org>]

Hi,

On 06/12/17 12:27, Julien Grall wrote:
> On 12/06/2017 01:26 AM, Stefano Stabellini wrote:
>> On Thu, 23 Nov 2017, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> On 23/11/17 18:49, Andrew Cooper wrote:
>>>> On 23/11/17 18:32, Julien Grall wrote:
>>>>> This new function will be used in a follow-up patch to copy data to 
>>>>> the
>>>>> guest
>>>>> using the IPA (aka guest physical address) and then clean the cache.
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>>> ---
>>>>>    xen/arch/arm/guestcopy.c           | 10 ++++++++++
>>>>>    xen/include/asm-arm/guest_access.h |  6 ++++++
>>>>>    2 files changed, 16 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
>>>>> index be53bee559..7958663970 100644
>>>>> --- a/xen/arch/arm/guestcopy.c
>>>>> +++ b/xen/arch/arm/guestcopy.c
>>>>> @@ -110,6 +110,16 @@ unsigned long raw_copy_from_guest(void *to, const
>>>>> void __user *from, unsigned le
>>>>>                          COPY_from_guest | COPY_linear);
>>>>>    }
>>>>>    +unsigned long copy_to_guest_phys_flush_dcache(struct domain *d,
>>>>> +                                              paddr_t gpa,
>>>>> +                                              void *buf,
>>>>> +                                              unsigned int len)
>>>>> +{
>>>>> +    /* P2M is shared between all vCPUs, so the vCPU used does not 
>>>>> matter.
>>>>> */
>>>>
>>>> Be very careful with this line of thinking.  It is only works after
>>>> DOMCTL_max_vcpus has succeeded, and before that point, it is a latent
>>>> NULL pointer dereference.
>>>
>>> I really don't expect that function been used before DOMCT_max_vcpus 
>>> is set.
>>> It is only used for hardware emulation or Xen loading image into the 
>>> hardware
>>> domain memory. I could add a check d->vcpus to be safe.
>>>
>>>>
>>>> Also, what about vcpus configured with alternative views?
>>>
>>> It is not important because the underlying call is get_page_from_gfn 
>>> that does
>>> not care about the alternative view (that function take a domain in
>>> parameter). I can update the comment.
>> Since this is a new function, would it make sense to take a struct
>> vcpu* as parameter, instead of a struct domain* ?
> 
> Well, I suggested this patch this way because likely everyone will use 
> with d->vcpus[0]. And then you would have to wonder why d->vcpus[0] and 
> not d->vcpus[1]...

Thinking a bit more to this, it might be better/safer to pass either a 
domain or a vCPU to copy_guest. I can see 2 solutions:
	1# Introduce a union that use the same parameter:
		union
		{
			struct
			{
				struct domain *d;
			} ipa;
			struct
			{
				struct vcpu *v;
			} gva;
		}
	  The structure here would be to ensure that it is clear that only 
domain (resp. vcpu) should be used with ipa (resp. gva).

	2# Have 2 parameters, vcpu and domain.

Any opinions?

Cheers,

-- 
Julien Grall

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