Re: [PATCH v8 4/4] xen/arm: grant: Add another entry to map MFN 1:1 in dom0 p2m

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

On 05/21/2014 02:50 PM, Ian Campbell wrote:
>>>> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
>>>> index 21b4572..9f85800 100644
>>>> --- a/xen/drivers/passthrough/arm/smmu.c
>>>> +++ b/xen/drivers/passthrough/arm/smmu.c
>>>> @@ -1536,6 +1536,48 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
>>>>      xfree(smmu_domain);
>>>>  }
>>>>  
>>>> +static int arm_smmu_map_page(struct domain *d, unsigned long gfn,
>>>> +                             unsigned long mfn, unsigned int flags)
>>>> +{
>>>> +    p2m_type_t t;
>>>> +
>>>> +    /* This function should only be used by gnttab code when the domain
>>>> +     * is direct mapped and gfn == mfn.
>>>
>>> Is gfn !+ mfn an ASSERT-worthy condition?
>>
>> The ASSERT would only be for debug build. I'd like to have a safe guard
>> for non-debug build just in case.
> 
> That's a BUG_ON then I think, assuming it would be a coding error in the
> hypervisor (rather than e.g. a guest trying to exploit the issue
> somehow).

The guest should not be able to exploit this issue. I will add a BUG_ON.

>>> Is gnttab the only possible user?
>>
>> For ARM yes.
> 
> OK
> 
> (out of curiosity what are the other users on x86?)

It's used for create the IOMMU PT.

>>>> +     * This is only valid when the domain is directed mapped
>>>> +     */
>>>> +    return guest_physmap_add_entry(d, gfn, mfn, 0, t);
>>>> +}
>>>> +
>>>> +static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
>>>> +{
>>>> +    /* This function should only be used by gnttab code when the domain
>>>> +     * is direct mapped
>>>> +     */
>>>> +    if ( !is_domain_direct_mapped(d) )
>>>> +        return -EINVAL;
>>>> +
>>>> +    guest_physmap_remove_page(d, gfn, gfn, 0);
>>>
>>> I think 0 here is really PAGE_ORDER_4K, is it? (other callers of this
>>> function seem to be inconsistent about this)
>>
>> Yes, assuming the guest page will always be 4K.
> 
> Even if not then PAGE_ORDER_4K will make good fodder for grep...

I will use it in the next version.

>> What about introducing "dummy type" such as p2m_notype_{ro,rw} which
>> could be use in such case?
> 
> notype is effectively "ram" I think, but that doesn't seem quite right
> either.
> 
> I'm just worried that p2m type bits are in limited supply so I want to
> be sure using new ones is justified.

We don't really need to store those type in the P2M. We only need them
to choose the page attributes.

We could introduce a virtual type (i.e value > p2m_max_real_type) and
store p2m_invalid in the P2M.

Regards,

-- 
Julien Grall