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:27 PM, Ian Campbell wrote:
> On Mon, 2014-05-19 at 17:24 +0100, Julien Grall wrote:
>> Grant mapping can be used for DMA request. The dev_bus_addr returned by the
> 
>                                              ^Currently (?)
> 
>> hypercall is the MFN (not the IPA). Currently Linux is using this address (via
>> swiotlb) to program the DMA.
> 
> Rather than talking specifically about Linux and swiotlb I think it is
> correct to say "Guests expect to be able to use the returned address for
> DMA".
> 
>> When the device is protected by IOMMU the request will fail. We have to
>> add 1:1 mapping in the domain p2m to allow DMA request working.
>>
>> This is valid because DOM0 has its memory mapped 1:1 and therefore we know
>> that RAM and devices cannot clash.
> 
> Is it worth mentioning now that in the future when a domain only has
> access to protected I/O devices we would instead return
> dev_bus_addr==IPA and intend to drop this extra 1:1 mapping?

I plan to modify dev_bus_addr with my non-PCI passthrough to return the IPA.

The code has been written to remove easily the 1:1 workaround (see macro
is_domain_direct_mapped.

I can make a mention about it in the commit message.

>> The grant mapping code already handle this case for x86 PV guests. Reuse the
>> same code path for ARM guest.
> 
> In particular do you mean that iommu_{,un}map_page handles the reference
> counting needed when an mfn is mapped via multiple grant mapping? I
> think it must be the callers of those functions. Could you say that here
> please?

The reference counting is done in common/grant_table (see the wrc/rdc
logic before calling iommu_{,un}map_page).

>> 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.

> 
> Is gnttab the only possible user?

For ARM yes.

>> +     */
>> +    if ( !is_domain_direct_mapped(d) || gfn != mfn )
>> +        return -EINVAL;
>> +
>> +    /* We only support readable and writable flags */
>> +    if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
>> +        return -EINVAL;
>> +
>> +    /* The function guest_physmap_add_entry replace the current mapping
> 
> "replaces"
> 
>> +     * if there is already one...
> 
> ... I feel like you intended to describe a consequence of that here. I
> can't see the relationship between the comment and the selection of rw
> vs ro mappings.

This was intend to be just above guest_physmap_add_entry. I will move
the comment.

>> +     */
>> +    t = (flags & IOMMUF_writable)? p2m_iommu_map_rw : p2m_iommu_map_ro;
>> +
>> +    /* Grant mapping can be used for DMA request. The dev_bus_addr returned by
>> +     * the hypercall is the MFN (not the IPA). For device protected by
> 
> "Grant mappings... DMA requests... For devices" (all plural)
> 
>> +     * an IOMMU, Xen needs to add a 1:1 mapping in the domain p2m to
>> +     * allow DMA request working.
> 
> "to allow DMA requests to work"
> 
>> +     * 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.

> 
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index bd71abe..b68d5b8 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -45,6 +45,8 @@ typedef enum {
>>      p2m_map_foreign,    /* Ram pages from foreign domain */
>>      p2m_grant_map_rw,   /* Read/write grant mapping */
>>      p2m_grant_map_ro,   /* Read-only grant mapping */
>> +    p2m_iommu_map_rw,   /* Read/write iommu mapping */
>> +    p2m_iommu_map_ro,   /* Read-only iommu mapping */
> 
> Why do we need new p2m types, rather than using e.g. the grant map type
> (which I suppose is what the non-1:1 map uses)?
> Could you explain the reason in the commit log too please.

The iommu_map_page could be reuse more generically in long-term. Using
p2m_grant_map_{ro,rw} would be confusing here.


What about introducing "dummy type" such as p2m_notype_{ro,rw} which
could be use in such case?

Regards,

-- 
Julien Grall