| Message ID | 1489608329-7275-5-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Oleksandr, On 15/03/17 20:05, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Update IOMMU mapping if the IOMMU doesn't share page table with the CPU. > The best place to do so on ARM is p2m_set_entry(). Use mfn as an indicator > of the required action. If mfn is valid call iommu_map_pages(), > otherwise - iommu_unmap_pages(). Can you explain in the commit message why you do this change in p2m_set_entry and not __p2m_set_entry? > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 1fc6ca3..84d3a09 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m, > if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base ) > p2m_free_entry(p2m, orig_pte, level); > > - if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || p2m_valid(*entry)) ) > + if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) && > + (p2m_valid(orig_pte) || p2m_valid(*entry)) ) > rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order); > else > rc = 0; > @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m, > p2m_type_t t, > p2m_access_t a) > { > + unsigned long orig_nr = nr; > + gfn_t orig_sgfn = sgfn; > + mfn_t orig_smfn = smfn; > int rc = 0; > > while ( nr ) > @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m, > nr -= (1 << order); > } > > + if ( likely(!rc) ) I would do if ( unlikely(rc) ) return rc; /* IOMMU map/unmap ... */ This would remove one indentation layer of if. > + { > + /* > + * It's time to update IOMMU mapping if the latter doesn't > + * share page table with the CPU. Pass the whole memory block to let > + * the IOMMU code decide what to do with it. > + * The reason to update IOMMU mapping outside "while loop" is that > + * the IOMMU might not support the pages/superpages the CPU can deal > + * with (4K, 2M, 1G) and as result this will lead to non-optimal > + * mapping. My worry with this solution is someone may decide to use __p2m_set_entry (see relinquish) directly because he knows the correct order. So the IOMMU would be correctly handled when page table are shared and not when they are "unshared". I would probably extend __p2m_get_entry with a new parameter indication whether we want to map the page in the IOMMU directly or not. Also, do you expect the IOMMU to support a page granularity bigger than the one supported by Xen? If not, we should probably have a check somewhere, to prevent potential security issue as we would map more than expected. > + * Also we assume that the IOMMU mapping should be updated only > + * if CPU mapping passed successfully. > + */ > + if ( need_iommu(p2m->domain) && !iommu_use_hap_pt(p2m->domain) ) > + { > + if ( !mfn_eq(orig_smfn, INVALID_MFN) ) > + { > + unsigned int flags = p2m_get_iommu_flags(t); > + > + rc = iommu_map_pages(p2m->domain, > + gfn_x(orig_sgfn), > + mfn_x(orig_smfn), > + orig_nr, > + flags); > + } > + else > + { > + rc = iommu_unmap_pages(p2m->domain, > + gfn_x(orig_sgfn), > + orig_nr); > + } > + } > + } > + > return rc; > } > > Cheers,
On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi, Julien > > On 15/03/17 20:05, Oleksandr Tyshchenko wrote: >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Update IOMMU mapping if the IOMMU doesn't share page table with the CPU. >> The best place to do so on ARM is p2m_set_entry(). Use mfn as an indicator >> of the required action. If mfn is valid call iommu_map_pages(), >> otherwise - iommu_unmap_pages(). > > > Can you explain in the commit message why you do this change in > p2m_set_entry and not __p2m_set_entry? ok > > >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 39 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 1fc6ca3..84d3a09 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m, >> if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base ) >> p2m_free_entry(p2m, orig_pte, level); >> >> - if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || >> p2m_valid(*entry)) ) >> + if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) && >> + (p2m_valid(orig_pte) || p2m_valid(*entry)) ) >> rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << >> page_order); >> else >> rc = 0; >> @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m, >> p2m_type_t t, >> p2m_access_t a) >> { >> + unsigned long orig_nr = nr; >> + gfn_t orig_sgfn = sgfn; >> + mfn_t orig_smfn = smfn; >> int rc = 0; >> >> while ( nr ) >> @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m, >> nr -= (1 << order); >> } >> >> + if ( likely(!rc) ) > > > I would do > > if ( unlikely(rc) ) > return rc; > > /* IOMMU map/unmap ... */ > > This would remove one indentation layer of if. Agree. > >> + { >> + /* >> + * It's time to update IOMMU mapping if the latter doesn't >> + * share page table with the CPU. Pass the whole memory block to >> let >> + * the IOMMU code decide what to do with it. >> + * The reason to update IOMMU mapping outside "while loop" is >> that >> + * the IOMMU might not support the pages/superpages the CPU can >> deal >> + * with (4K, 2M, 1G) and as result this will lead to non-optimal >> + * mapping. > > > My worry with this solution is someone may decide to use __p2m_set_entry > (see relinquish) directly because he knows the correct order. So the IOMMU > would be correctly handled when page table are shared and not when they are > "unshared". As for relinquish_p2m_mapping(), I was thinking about it, but I decided not to remove IOMMU mapping here since the whole IOMMU page table would be removed during complete_domain_destroy(). But, I agree that nothing prevent someone to use __p2m_set_entry directly in future. > > I would probably extend __p2m_get_entry with a new parameter indication > whether we want to map the page in the IOMMU directly or not. Sorry, I didn't get your point here. Did you mean __p2m_set_entry? > > Also, do you expect the IOMMU to support a page granularity bigger than the > one supported by Xen? If not, we should probably have a check somewhere, to > prevent potential security issue as we would map more than expected. At the moment I keep in mind IPMMU only. And it supports the same page granularity as the CPU (4K, 2M, 1G). Could you, please, explain what a proposed check should check? With the current solution we pass the whole memory block to the IOMMU code and the IOMMU driver should handle this properly. If we pass, for example 1,1 GB, but the IOMMU driver supports 4K pages only then it has to split the memory block into 4K pages and process them. > > >> + * Also we assume that the IOMMU mapping should be updated only >> + * if CPU mapping passed successfully. >> + */ >> + if ( need_iommu(p2m->domain) && !iommu_use_hap_pt(p2m->domain) ) >> + { >> + if ( !mfn_eq(orig_smfn, INVALID_MFN) ) >> + { >> + unsigned int flags = p2m_get_iommu_flags(t); >> + >> + rc = iommu_map_pages(p2m->domain, >> + gfn_x(orig_sgfn), >> + mfn_x(orig_smfn), >> + orig_nr, >> + flags); >> + } >> + else >> + { >> + rc = iommu_unmap_pages(p2m->domain, >> + gfn_x(orig_sgfn), >> + orig_nr); >> + } >> + } >> + } >> + >> return rc; >> } >> >> > > Cheers, > > -- > Julien Grall
On 21/04/17 15:18, Oleksandr Tyshchenko wrote: > On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@arm.com> wrote: >> Hi Oleksandr, > Hi, Julien Hi Oleksandr, >> >> On 15/03/17 20:05, Oleksandr Tyshchenko wrote: >>> >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> Update IOMMU mapping if the IOMMU doesn't share page table with the CPU. >>> The best place to do so on ARM is p2m_set_entry(). Use mfn as an indicator >>> of the required action. If mfn is valid call iommu_map_pages(), >>> otherwise - iommu_unmap_pages(). >> >> >> Can you explain in the commit message why you do this change in >> p2m_set_entry and not __p2m_set_entry? > ok > >> >> >>> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> --- >>> xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 39 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>> index 1fc6ca3..84d3a09 100644 >>> --- a/xen/arch/arm/p2m.c >>> +++ b/xen/arch/arm/p2m.c >>> @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m, >>> if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base ) >>> p2m_free_entry(p2m, orig_pte, level); >>> >>> - if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || >>> p2m_valid(*entry)) ) >>> + if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) && >>> + (p2m_valid(orig_pte) || p2m_valid(*entry)) ) >>> rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << >>> page_order); >>> else >>> rc = 0; >>> @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m, >>> p2m_type_t t, >>> p2m_access_t a) >>> { >>> + unsigned long orig_nr = nr; >>> + gfn_t orig_sgfn = sgfn; >>> + mfn_t orig_smfn = smfn; >>> int rc = 0; >>> >>> while ( nr ) >>> @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m, >>> nr -= (1 << order); >>> } >>> >>> + if ( likely(!rc) ) >> >> >> I would do >> >> if ( unlikely(rc) ) >> return rc; >> >> /* IOMMU map/unmap ... */ >> >> This would remove one indentation layer of if. > Agree. > >> >>> + { >>> + /* >>> + * It's time to update IOMMU mapping if the latter doesn't >>> + * share page table with the CPU. Pass the whole memory block to >>> let >>> + * the IOMMU code decide what to do with it. >>> + * The reason to update IOMMU mapping outside "while loop" is >>> that >>> + * the IOMMU might not support the pages/superpages the CPU can >>> deal >>> + * with (4K, 2M, 1G) and as result this will lead to non-optimal >>> + * mapping. >> >> >> My worry with this solution is someone may decide to use __p2m_set_entry >> (see relinquish) directly because he knows the correct order. So the IOMMU >> would be correctly handled when page table are shared and not when they are >> "unshared". > As for relinquish_p2m_mapping(), I was thinking about it, but I > decided not to remove IOMMU mapping here since > the whole IOMMU page table would be removed during complete_domain_destroy(). > But, I agree that nothing prevent someone to use __p2m_set_entry > directly in future. > >> >> I would probably extend __p2m_get_entry with a new parameter indication >> whether we want to map the page in the IOMMU directly or not. > Sorry, I didn't get your point here. Did you mean __p2m_set_entry? Yes sorry. > >> >> Also, do you expect the IOMMU to support a page granularity bigger than the >> one supported by Xen? If not, we should probably have a check somewhere, to >> prevent potential security issue as we would map more than expected. > At the moment I keep in mind IPMMU only. And it supports the same page > granularity as the CPU > (4K, 2M, 1G). Could you, please, explain what a proposed check should check? We should check that the smallest granularity supported by the IOMMU is inferior or equal to PAGE_SIZE. If not, then there will be more rework required in Xen to support correctly those IOMMUs. For instance, Xen PAGE_SIZE is currently 4KB. If the IOMMUs only support 64KB, then you will end up to map a bigger chunk than expected leading a guest device to access memory it should not access. Supporting such IOMMU will require much more work in Xen than this small changes. > With the current solution we pass the whole memory block to the IOMMU > code and the IOMMU driver should handle this properly. > If we pass, for example 1,1 GB, but the IOMMU driver supports 4K pages > only then it has to split the memory block into 4K pages and process > them. The IOMMU driver will likely duplicate the same logic as in p2m_set_entry and today does not seem to be necessary. By that I mean splitting into smaller chunk. You may need to split again those chunks if the IOMMU does not support the granularity. But this is less likely on current hardware. My main concern is to make sure __p2m_get_entry works as expected with all the configurations. Currently, what you describe will require much more work than this series. I will be ok whether you rework __p2m_get_entry or not. Cheers,
On Fri, Apr 21, 2017 at 7:27 PM, Julien Grall <julien.grall@arm.com> wrote: > > > On 21/04/17 15:18, Oleksandr Tyshchenko wrote: >> >> On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@arm.com> >> wrote: >>> >>> Hi Oleksandr, >> >> Hi, Julien > > > Hi Oleksandr, Hi, Julien > > >>> >>> On 15/03/17 20:05, Oleksandr Tyshchenko wrote: >>>> >>>> >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> Update IOMMU mapping if the IOMMU doesn't share page table with the CPU. >>>> The best place to do so on ARM is p2m_set_entry(). Use mfn as an >>>> indicator >>>> of the required action. If mfn is valid call iommu_map_pages(), >>>> otherwise - iommu_unmap_pages(). >>> >>> >>> >>> Can you explain in the commit message why you do this change in >>> p2m_set_entry and not __p2m_set_entry? >> >> ok >> >>> >>> >>>> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> --- >>>> xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 39 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>> index 1fc6ca3..84d3a09 100644 >>>> --- a/xen/arch/arm/p2m.c >>>> +++ b/xen/arch/arm/p2m.c >>>> @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m, >>>> if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base ) >>>> p2m_free_entry(p2m, orig_pte, level); >>>> >>>> - if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || >>>> p2m_valid(*entry)) ) >>>> + if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) && >>>> + (p2m_valid(orig_pte) || p2m_valid(*entry)) ) >>>> rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << >>>> page_order); >>>> else >>>> rc = 0; >>>> @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m, >>>> p2m_type_t t, >>>> p2m_access_t a) >>>> { >>>> + unsigned long orig_nr = nr; >>>> + gfn_t orig_sgfn = sgfn; >>>> + mfn_t orig_smfn = smfn; >>>> int rc = 0; >>>> >>>> while ( nr ) >>>> @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m, >>>> nr -= (1 << order); >>>> } >>>> >>>> + if ( likely(!rc) ) >>> >>> >>> >>> I would do >>> >>> if ( unlikely(rc) ) >>> return rc; >>> >>> /* IOMMU map/unmap ... */ >>> >>> This would remove one indentation layer of if. >> >> Agree. >> >>> >>>> + { >>>> + /* >>>> + * It's time to update IOMMU mapping if the latter doesn't >>>> + * share page table with the CPU. Pass the whole memory block >>>> to >>>> let >>>> + * the IOMMU code decide what to do with it. >>>> + * The reason to update IOMMU mapping outside "while loop" is >>>> that >>>> + * the IOMMU might not support the pages/superpages the CPU can >>>> deal >>>> + * with (4K, 2M, 1G) and as result this will lead to >>>> non-optimal >>>> + * mapping. >>> >>> >>> >>> My worry with this solution is someone may decide to use __p2m_set_entry >>> (see relinquish) directly because he knows the correct order. So the >>> IOMMU >>> would be correctly handled when page table are shared and not when they >>> are >>> "unshared". >> >> As for relinquish_p2m_mapping(), I was thinking about it, but I >> decided not to remove IOMMU mapping here since >> the whole IOMMU page table would be removed during >> complete_domain_destroy(). >> But, I agree that nothing prevent someone to use __p2m_set_entry >> directly in future. >> >>> >>> I would probably extend __p2m_get_entry with a new parameter indication >>> whether we want to map the page in the IOMMU directly or not. >> >> Sorry, I didn't get your point here. Did you mean __p2m_set_entry? > > > Yes sorry. > >> >>> >>> Also, do you expect the IOMMU to support a page granularity bigger than >>> the >>> one supported by Xen? If not, we should probably have a check somewhere, >>> to >>> prevent potential security issue as we would map more than expected. >> >> At the moment I keep in mind IPMMU only. And it supports the same page >> granularity as the CPU >> (4K, 2M, 1G). Could you, please, explain what a proposed check should >> check? > > > We should check that the smallest granularity supported by the IOMMU is > inferior or equal to PAGE_SIZE. If not, then there will be more rework > required in Xen to support correctly those IOMMUs. > > For instance, Xen PAGE_SIZE is currently 4KB. If the IOMMUs only support > 64KB, then you will end up to map a bigger chunk than expected leading a > guest device to access memory it should not access. > > Supporting such IOMMU will require much more work in Xen than this small > changes. Aha. Seems, I understand what you meant. I already have a check in IPMMU driver: /* * both the virtual address and the physical one, as well as * the size of the mapping, must be aligned (at least) to the * size of the smallest page supported by the hardware */ if (!IS_ALIGNED(iova | paddr | size, min_pagesz)) { printk("unaligned: iova 0x%lx pa 0x%"PRIx64" size 0x%zx min_pagesz 0x%x\n", iova, paddr, size, min_pagesz); return -EINVAL; } where min_pagesz - is a minimum page size supported by hardware. Hope, that this check can catch such case. > > >> With the current solution we pass the whole memory block to the IOMMU >> code and the IOMMU driver should handle this properly. >> If we pass, for example 1,1 GB, but the IOMMU driver supports 4K pages >> only then it has to split the memory block into 4K pages and process >> them. > > > The IOMMU driver will likely duplicate the same logic as in p2m_set_entry > and today does not seem to be necessary. By that I mean splitting into > smaller chunk. > > You may need to split again those chunks if the IOMMU does not support the > granularity. But this is less likely on current hardware. > > My main concern is to make sure __p2m_get_entry works as expected with all > the configurations. Currently, what you describe will require much more work > than this series. I will be ok whether you rework __p2m_get_entry or not. As we have already found common ground: I will rework the patch by adding updating IOMMU mapping to __p2m_set_entry and add new argument to it to to show whether we have to update IOMMU mapping or not. So, if we call __p2m_set_entry directly, we have to force it to update IOMMU mapping. > > Cheers, > > -- > Julien Grall
Hi Oleksandr, On 21/04/17 19:44, Oleksandr Tyshchenko wrote: > On Fri, Apr 21, 2017 at 7:27 PM, Julien Grall <julien.grall@arm.com> wrote: >> On 21/04/17 15:18, Oleksandr Tyshchenko wrote: >>> >>> On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@arm.com> >>> wrote: > > Aha. Seems, I understand what you meant. I already have a check in IPMMU driver: > > /* > * both the virtual address and the physical one, as well as > * the size of the mapping, must be aligned (at least) to the > * size of the smallest page supported by the hardware > */ > if (!IS_ALIGNED(iova | paddr | size, min_pagesz)) { > printk("unaligned: iova 0x%lx pa 0x%"PRIx64" size 0x%zx min_pagesz 0x%x\n", > iova, paddr, size, min_pagesz); > return -EINVAL; > } > > where min_pagesz - is a minimum page size supported by hardware. > Hope, that this check can catch such case. I think will cover what I meant. Although, can't we do this check when the IOMMU is initialized at boot time rather than for every mapping? For instance you know that if the mimimum page granularity supported by the IOMMU is bigger than PAGE_SIZE, than you will likely get into trouble later on. So rather than randomly crashing at runtime, you can disable the IOMMU/panic at boot time. Cheers,
On Mon, Apr 24, 2017 at 2:41 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi, Julien > > On 21/04/17 19:44, Oleksandr Tyshchenko wrote: >> >> On Fri, Apr 21, 2017 at 7:27 PM, Julien Grall <julien.grall@arm.com> >> wrote: >>> >>> On 21/04/17 15:18, Oleksandr Tyshchenko wrote: >>>> >>>> >>>> On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@arm.com> >>>> wrote: >> >> >> Aha. Seems, I understand what you meant. I already have a check in IPMMU >> driver: >> >> /* >> * both the virtual address and the physical one, as well as >> * the size of the mapping, must be aligned (at least) to the >> * size of the smallest page supported by the hardware >> */ >> if (!IS_ALIGNED(iova | paddr | size, min_pagesz)) { >> printk("unaligned: iova 0x%lx pa 0x%"PRIx64" size 0x%zx min_pagesz >> 0x%x\n", >> iova, paddr, size, min_pagesz); >> return -EINVAL; >> } >> >> where min_pagesz - is a minimum page size supported by hardware. >> Hope, that this check can catch such case. > > > I think will cover what I meant. Although, can't we do this check when the > IOMMU is initialized at boot time rather than for every mapping? I think, yes. > > For instance you know that if the mimimum page granularity supported by the > IOMMU is bigger than PAGE_SIZE, than you will likely get into trouble later > on. > > So rather than randomly crashing at runtime, you can disable the IOMMU/panic > at boot time. Agree. > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 1fc6ca3..84d3a09 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m, if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base ) p2m_free_entry(p2m, orig_pte, level); - if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || p2m_valid(*entry)) ) + if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) && + (p2m_valid(orig_pte) || p2m_valid(*entry)) ) rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order); else rc = 0; @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m, p2m_type_t t, p2m_access_t a) { + unsigned long orig_nr = nr; + gfn_t orig_sgfn = sgfn; + mfn_t orig_smfn = smfn; int rc = 0; while ( nr ) @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m, nr -= (1 << order); } + if ( likely(!rc) ) + { + /* + * It's time to update IOMMU mapping if the latter doesn't + * share page table with the CPU. Pass the whole memory block to let + * the IOMMU code decide what to do with it. + * The reason to update IOMMU mapping outside "while loop" is that + * the IOMMU might not support the pages/superpages the CPU can deal + * with (4K, 2M, 1G) and as result this will lead to non-optimal + * mapping. + * Also we assume that the IOMMU mapping should be updated only + * if CPU mapping passed successfully. + */ + if ( need_iommu(p2m->domain) && !iommu_use_hap_pt(p2m->domain) ) + { + if ( !mfn_eq(orig_smfn, INVALID_MFN) ) + { + unsigned int flags = p2m_get_iommu_flags(t); + + rc = iommu_map_pages(p2m->domain, + gfn_x(orig_sgfn), + mfn_x(orig_smfn), + orig_nr, + flags); + } + else + { + rc = iommu_unmap_pages(p2m->domain, + gfn_x(orig_sgfn), + orig_nr); + } + } + } + return rc; }