| Message ID | 1400516640-7175-5-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
>>> On 19.05.14 at 18:24, <julien.grall@linaro.org> wrote: > Grant mapping can be used for DMA request. The dev_bus_addr returned by the > hypercall is the MFN (not the IPA). Currently Linux is using this address (via > swiotlb) to program the 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. Wasn't this 1:1 mapping meant as a temporary workaround? If so, does it really make sense to base further code on it? > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -727,7 +727,7 @@ __gnttab_map_grant_ref( > > double_gt_lock(lgt, rgt); > > - if ( !paging_mode_translate(ld) && need_iommu(ld) ) > + if ( gnttab_need_iommu_mapping(ld) && need_iommu(ld) ) I suppose that you want to keep the common bits of this condition common, but the two "need_iommu" in here look sort of redundant: With the name chosen, I think it would make more sense for the second condition to be moved into gnttab_need_iommu_mapping(). > --- a/xen/include/asm-arm/grant_table.h > +++ b/xen/include/asm-arm/grant_table.h > @@ -33,6 +33,8 @@ static inline int replace_grant_supported(void) > ( ((i >= nr_grant_frames(d->grant_table)) && \ > (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i])) > > +#define gnttab_need_iommu_mapping(d) is_domain_direct_mapped(d) While this one indeed doesn't need extra parentheses, ... > --- a/xen/include/asm-x86/grant_table.h > +++ b/xen/include/asm-x86/grant_table.h > @@ -65,6 +65,8 @@ static inline void gnttab_clear_flag(unsigned int nr, uint16_t *st) > /* Done implicitly when page tables are destroyed. */ > #define gnttab_release_host_mappings(domain) ( paging_mode_external(domain) ) > > +#define gnttab_need_iommu_mapping(d) !paging_mode_translate(d) ... this one does. Also, with all of this I don't see how other than Dom0 is being (or going to be) handled in this regard. Jan
On Tue, 2014-05-20 at 08:46 +0100, Jan Beulich wrote: > >>> On 19.05.14 at 18:24, <julien.grall@linaro.org> wrote: > > Grant mapping can be used for DMA request. The dev_bus_addr returned by the > > hypercall is the MFN (not the IPA). Currently Linux is using this address (via > > swiotlb) to program the 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. > > Wasn't this 1:1 mapping meant as a temporary workaround? If so, > does it really make sense to base further code on it? IOMMU on ARM has not become so prevalent (yet?) that we can get rid of it. I think we are mostly resigned to keeping it around for the time being. (sadly) Ian.
On 05/20/2014 08:46 AM, Jan Beulich wrote: >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -727,7 +727,7 @@ __gnttab_map_grant_ref( >> >> double_gt_lock(lgt, rgt); >> >> - if ( !paging_mode_translate(ld) && need_iommu(ld) ) >> + if ( gnttab_need_iommu_mapping(ld) && need_iommu(ld) ) > > I suppose that you want to keep the common bits of this condition > common, but the two "need_iommu" in here look sort of redundant: > With the name chosen, I think it would make more sense for the > second condition to be moved into gnttab_need_iommu_mapping(). Ok. I will move these bits in the macro. >> --- a/xen/include/asm-arm/grant_table.h >> +++ b/xen/include/asm-arm/grant_table.h >> @@ -33,6 +33,8 @@ static inline int replace_grant_supported(void) >> ( ((i >= nr_grant_frames(d->grant_table)) && \ >> (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i])) >> >> +#define gnttab_need_iommu_mapping(d) is_domain_direct_mapped(d) > > While this one indeed doesn't need extra parentheses, ... > >> --- a/xen/include/asm-x86/grant_table.h >> +++ b/xen/include/asm-x86/grant_table.h >> @@ -65,6 +65,8 @@ static inline void gnttab_clear_flag(unsigned int nr, uint16_t *st) >> /* Done implicitly when page tables are destroyed. */ >> #define gnttab_release_host_mappings(domain) ( paging_mode_external(domain) ) >> >> +#define gnttab_need_iommu_mapping(d) !paging_mode_translate(d) > > ... this one does. Oh right, I will fix it in the next version. > Also, with all of this I don't see how other than Dom0 is being (or > going to be) handled in this regard. Only DOM0 uses direct mapping (i.e 1:1 mapping). The other guests will use their own mapping. As the P2M is shared, we can't insert this 1:1 entry. Only protected devices (i.e the IOMMU has been programmed by Xen) will be passtrough to the guest. So we will be able to modify dev_bus_addr to return an IPA (guest address) rather than an MFN. For now, passthrough is not supported, therefore guest doesn't have DMA-capable device. I will take care of this solution with my non-PCI passthrough patch series. Regards,
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? > 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? > 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? Is gnttab the only possible user? > + */ > + 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. > + */ > + 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) > 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. Ian.
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,
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.
On Wed, 2014-05-21 at 14:42 +0100, Julien Grall wrote: > > Is gnttab the only possible user? > > For ARM yes. [...] > The iommu_map_page could be reuse more generically in long-term. Using > p2m_grant_map_{ro,rw} would be confusing here. BTW, those statements contradict each other. Ian.
On 05/21/2014 02:51 PM, Ian Campbell wrote: > On Wed, 2014-05-21 at 14:42 +0100, Julien Grall wrote: > >>> Is gnttab the only possible user? >> >> For ARM yes. > [...] >> The iommu_map_page could be reuse more generically in long-term. Using >> p2m_grant_map_{ro,rw} would be confusing here. > > BTW, those statements contradict each other. Right. This function could also be used if we want to support per-IOMMU p2m rather than sharing with the CPU.
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,
On Wed, 2014-05-21 at 14:54 +0100, Julien Grall wrote: > On 05/21/2014 02:51 PM, Ian Campbell wrote: > > On Wed, 2014-05-21 at 14:42 +0100, Julien Grall wrote: > > > >>> Is gnttab the only possible user? > >> > >> For ARM yes. > > [...] > >> The iommu_map_page could be reuse more generically in long-term. Using > >> p2m_grant_map_{ro,rw} would be confusing here. > > > > BTW, those statements contradict each other. > > Right. This function could also be used if we want to support per-IOMMU > p2m rather than sharing with the CPU. I suspected as much, thanks. I don't know how likely that is to be a thing which happens. (I'm not sure what the implications of that are for either the comment or the p2m type name...) Ian.
On Wed, 2014-05-21 at 15:01 +0100, Julien Grall wrote: > > 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. Will finding p2m_invalid under various circumstances not confuse something somewhere? How about a single real p2m_special and the virtual types you suggest? Or, TBH, lets just use the two types you propose and do all this when we run out... A comment to the affect that the types are only used for the rw bit might serve as a hint to us in a couple of years that we can easily get rid of them... Ian.
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 96bc0ef..810459a 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -227,6 +227,7 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr, e.p2m.write = 0; break; + case p2m_iommu_map_rw: case p2m_map_foreign: case p2m_grant_map_rw: case p2m_mmio_direct: @@ -234,6 +235,7 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr, e.p2m.write = 1; break; + case p2m_iommu_map_ro: case p2m_grant_map_ro: case p2m_invalid: e.p2m.xn = 1; diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 2c93d9c..7e549f2 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -727,7 +727,7 @@ __gnttab_map_grant_ref( double_gt_lock(lgt, rgt); - if ( !paging_mode_translate(ld) && need_iommu(ld) ) + if ( gnttab_need_iommu_mapping(ld) && need_iommu(ld) ) { unsigned int wrc, rdc; int err = 0; @@ -935,7 +935,7 @@ __gnttab_unmap_common( act->pin -= GNTPIN_hstw_inc; } - if ( !paging_mode_translate(ld) && need_iommu(ld) ) + if ( gnttab_need_iommu_mapping(ld) && need_iommu(ld) ) { unsigned int wrc, rdc; int err = 0; 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. + */ + 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 + * if there is already one... + */ + 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 + * an IOMMU, Xen needs to add a 1:1 mapping in the domain p2m to + * allow DMA request working. + * 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); + + return 0; +} + static const struct iommu_ops arm_smmu_iommu_ops = { .init = arm_smmu_iommu_domain_init, .hwdom_init = arm_smmu_iommu_hwdom_init, @@ -1544,6 +1586,8 @@ static const struct iommu_ops arm_smmu_iommu_ops = { .iotlb_flush_all = arm_smmu_iotlb_flush_all, .assign_dt_device = arm_smmu_attach_dev, .reassign_dt_device = arm_smmu_reassign_dt_dev, + .map_page = arm_smmu_map_page, + .unmap_page = arm_smmu_unmap_page, }; static int __init smmu_init(struct dt_device_node *dev, diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h index 6e0cc59..673bcdd 100644 --- a/xen/include/asm-arm/grant_table.h +++ b/xen/include/asm-arm/grant_table.h @@ -33,6 +33,8 @@ static inline int replace_grant_supported(void) ( ((i >= nr_grant_frames(d->grant_table)) && \ (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i])) +#define gnttab_need_iommu_mapping(d) is_domain_direct_mapped(d) + #endif /* __ASM_GRANT_TABLE_H__ */ /* * Local variables: 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 */ p2m_max_real_type, /* Types after this won't be store in the p2m */ } p2m_type_t; diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h index 3013869..e5ccf2b 100644 --- a/xen/include/asm-x86/grant_table.h +++ b/xen/include/asm-x86/grant_table.h @@ -65,6 +65,8 @@ static inline void gnttab_clear_flag(unsigned int nr, uint16_t *st) /* Done implicitly when page tables are destroyed. */ #define gnttab_release_host_mappings(domain) ( paging_mode_external(domain) ) +#define gnttab_need_iommu_mapping(d) !paging_mode_translate(d) + static inline int replace_grant_supported(void) { return 1;
Grant mapping can be used for DMA request. The dev_bus_addr returned by the hypercall is the MFN (not the IPA). Currently Linux is using this address (via swiotlb) to program the 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. The grant mapping code already handle this case for x86 PV guests. Reuse the same code path for ARM guest. Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Jan Beulich <jbeulich@suse.com> --- The patch has been heavily rework to use iommu_{,un}map_page. I dropped all the acks. Changes in v8: - Rework differently the 1:1 mapping by using iommu_{,un}map_page helpers. Changes in v5: - Update commit message Changes in v4: - Patch added --- xen/arch/arm/p2m.c | 2 ++ xen/common/grant_table.c | 4 ++-- xen/drivers/passthrough/arm/smmu.c | 44 ++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/grant_table.h | 2 ++ xen/include/asm-arm/p2m.h | 2 ++ xen/include/asm-x86/grant_table.h | 2 ++ 6 files changed, 54 insertions(+), 2 deletions(-)