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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-05-21 at 14:42 +0100, Julien Grall wrote:
> 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.

Thanks.

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

OK. I take it you plan to insert this into the commit message too.

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

> > Is gnttab the only possible user?
> 
> For ARM yes.

OK

(out of curiosity what are the other users on x86?)

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

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

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.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel