Re: [Xen-devel] [PATCH v2 09/30] x86/vtd: fix and simplify mapping RMRR regions

2016-09-30 Thread Jan Beulich 
>>> On 30.09.16 at 17:02,  wrote:
> On Fri, Sep 30, 2016 at 07:21:41AM -0600, Jan Beulich wrote:
>> >>> On 30.09.16 at 13:27,  wrote:
>> > On Thu, Sep 29, 2016 at 08:18:36AM -0600, Jan Beulich wrote:
>> >> >>> On 27.09.16 at 17:57,  wrote:
>> >> > +{
>> >> > +int rc;
>> >> > +
>> >> > +while ( nr_pages > 0 )
>> >> > +{
>> >> > +rc = (map ? map_mmio_regions : unmap_mmio_regions)
>> >> > + (d, _gfn(pfn), nr_pages, _mfn(pfn));
>> >> > +if ( rc == 0 )
>> >> > +break;
>> >> > +if ( rc < 0 )
>> >> > +{
>> >> > +printk(XENLOG_ERR
>> >> > +"Failed to %smap %#lx - %#lx into domain %d memory 
>> >> > map: 
> %d\n",
>> >> > +   map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, 
>> >> > rc);
>> >> > +return rc;
>> >> > +}
>> >> > +nr_pages -= rc;
>> >> > +pfn += rc;
>> >> > +process_pending_softirqs();
>> >> > +}
>> >> > +
>> >> > +return rc;
>> >> 
>> >> The way this is coded it appears to possibly return non-zero even in
>> >> success case. I think this would therefore better be a for ( ; ; ) loop.
>> > 
>> > I don't think this is possible, {un}map_mmio_regions will return < 0 on 
>> > error, > 0 if there are pending pages to map, and 0 when all the requested 
>> > pages have been mapped successfully.
>> 
>> Right - hence the "appears to" in my reply; it took me a while to
>> figure it's not actually possible, and hence my desire to make this
>> more obvious to the reader.
> 
> Ah, OK, I misunderstood you then. What about changing the last return rc
> to return 0? This would make it more obvious, because I'm not really sure a 
> for loop would change much (IMHO the problem is the return semantics used by 
> {un}map_mmio_regions).

Well, my suggestion was not to use "a for loop" but specifically
"for ( ; ; )" to clarify the only loop exit condition is the single break
statement. That break statement could of course also become a
"return 0". I'd rather not see the return at the end of the function
become "return 0"; if anything (i.e. if you follow my suggestion) it
could be deleted altogether.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/30] x86/vtd: fix and simplify mapping RMRR regions

2016-09-30 Thread Roger Pau Monne 
On Fri, Sep 30, 2016 at 07:21:41AM -0600, Jan Beulich wrote:
> >>> On 30.09.16 at 13:27,  wrote:
> > On Thu, Sep 29, 2016 at 08:18:36AM -0600, Jan Beulich wrote:
> >> >>> On 27.09.16 at 17:57,  wrote:
> >> > +{
> >> > +int rc;
> >> > +
> >> > +while ( nr_pages > 0 )
> >> > +{
> >> > +rc = (map ? map_mmio_regions : unmap_mmio_regions)
> >> > + (d, _gfn(pfn), nr_pages, _mfn(pfn));
> >> > +if ( rc == 0 )
> >> > +break;
> >> > +if ( rc < 0 )
> >> > +{
> >> > +printk(XENLOG_ERR
> >> > +"Failed to %smap %#lx - %#lx into domain %d memory map: 
> >> > %d\n",
> >> > +   map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, 
> >> > rc);
> >> > +return rc;
> >> > +}
> >> > +nr_pages -= rc;
> >> > +pfn += rc;
> >> > +process_pending_softirqs();
> >> > +}
> >> > +
> >> > +return rc;
> >> 
> >> The way this is coded it appears to possibly return non-zero even in
> >> success case. I think this would therefore better be a for ( ; ; ) loop.
> > 
> > I don't think this is possible, {un}map_mmio_regions will return < 0 on 
> > error, > 0 if there are pending pages to map, and 0 when all the requested 
> > pages have been mapped successfully.
> 
> Right - hence the "appears to" in my reply; it took me a while to
> figure it's not actually possible, and hence my desire to make this
> more obvious to the reader.

Ah, OK, I misunderstood you then. What about changing the last return rc
to return 0? This would make it more obvious, because I'm not really sure a 
for loop would change much (IMHO the problem is the return semantics used by 
{un}map_mmio_regions).

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/30] x86/vtd: fix and simplify mapping RMRR regions

2016-09-30 Thread Jan Beulich 
>>> On 30.09.16 at 13:27,  wrote:
> On Thu, Sep 29, 2016 at 08:18:36AM -0600, Jan Beulich wrote:
>> >>> On 27.09.16 at 17:57,  wrote:
>> > +{
>> > +int rc;
>> > +
>> > +while ( nr_pages > 0 )
>> > +{
>> > +rc = (map ? map_mmio_regions : unmap_mmio_regions)
>> > + (d, _gfn(pfn), nr_pages, _mfn(pfn));
>> > +if ( rc == 0 )
>> > +break;
>> > +if ( rc < 0 )
>> > +{
>> > +printk(XENLOG_ERR
>> > +"Failed to %smap %#lx - %#lx into domain %d memory map: 
>> > %d\n",
>> > +   map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, 
>> > rc);
>> > +return rc;
>> > +}
>> > +nr_pages -= rc;
>> > +pfn += rc;
>> > +process_pending_softirqs();
>> > +}
>> > +
>> > +return rc;
>> 
>> The way this is coded it appears to possibly return non-zero even in
>> success case. I think this would therefore better be a for ( ; ; ) loop.
> 
> I don't think this is possible, {un}map_mmio_regions will return < 0 on 
> error, > 0 if there are pending pages to map, and 0 when all the requested 
> pages have been mapped successfully.

Right - hence the "appears to" in my reply; it took me a while to
figure it's not actually possible, and hence my desire to make this
more obvious to the reader.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/30] x86/vtd: fix and simplify mapping RMRR regions

2016-09-30 Thread Roger Pau Monne 
On Thu, Sep 29, 2016 at 08:18:36AM -0600, Jan Beulich wrote:
> >>> On 27.09.16 at 17:57,  wrote:
> > The current code used by Intel VTd will only map RMRR regions for the
> > hardware domain, but will fail to map RMRR regions for unprivileged domains
> > unless the page tables are shared between EPT and IOMMU.
> 
> Okay, if that's the case it surely should get fixed.
> 
> > Fix this and
> > simplify the code, removing the {set/clear}_identity_p2m_entry helpers and
> > just using the normal MMIO mapping functions.
> 
> This simplification, however, goes too far. Namely ...
> 
> > -int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
> > -   p2m_access_t p2ma, unsigned int flag)
> > -{
> > -p2m_type_t p2mt;
> > -p2m_access_t a;
> > -mfn_t mfn;
> > -struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > -int ret;
> > -
> > -if ( !paging_mode_translate(p2m->domain) )
> > -{
> > -if ( !need_iommu(d) )
> > -return 0;
> > -return iommu_map_page(d, gfn, gfn, 
> > IOMMUF_readable|IOMMUF_writable);
> > -}
> > -
> > -gfn_lock(p2m, gfn, 0);
> > -
> > -mfn = p2m->get_entry(p2m, gfn, , , 0, NULL, NULL);
> > -
> > -if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
> > -ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
> > -p2m_mmio_direct, p2ma);
> > -else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
> > -{
> > -ret = 0;
> > -/*
> > - * PVH fixme: during Dom0 PVH construction, p2m entries are being 
> > set
> > - * but iomem regions are not mapped with IOMMU. This makes sure 
> > that
> > - * RMRRs are correctly mapped with IOMMU.
> > - */
> > -if ( is_hardware_domain(d) && !iommu_use_hap_pt(d) )
> > -ret = iommu_map_page(d, gfn, gfn, 
> > IOMMUF_readable|IOMMUF_writable);
> > -}
> > -else
> > -{
> > -if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED )
> > -ret = 0;
> > -else
> > -ret = -EBUSY;
> > -printk(XENLOG_G_WARNING
> > -   "Cannot setup identity map d%d:%lx,"
> > -   " gfn already mapped to %lx.\n",
> > -   d->domain_id, gfn, mfn_x(mfn));
> 
> ... this logic (and its clear side counterpart) should not be removed
> without replacement. Note in this context how you render "flag" an
> unused parameter of rmrr_identity_mapping().

OK, so I'm just going to fix {set/clear}_identity_p2m_entry, because leaving 
the current logic while using something like modify_mmio_11 (or whatever we 
end up calling it) it's too complex IMHO.

> > --- a/xen/include/xen/p2m-common.h
> > +++ b/xen/include/xen/p2m-common.h
> > @@ -2,6 +2,7 @@
> >  #define _XEN_P2M_COMMON_H
> >  
> >  #include 
> > +#include 
> >  
> >  /*
> >   * Additional access types, which are used to further restrict
> > @@ -46,6 +47,35 @@ int unmap_mmio_regions(struct domain *d,
> > mfn_t mfn);
> >  
> >  /*
> > + * Preemptive Helper for mapping/unmapping MMIO regions.
> > + */
> 
> Single line comment.
> 
> > +static inline int modify_mmio_11(struct domain *d, unsigned long pfn,
> > + unsigned long nr_pages, bool map)
> 
> Why do you make this an inline function? And I have to admit that I
> dislike this strange use of number 11 - what's wrong with continuing
> to use the term "direct map" in one way or another?

I've renamed it to modify_mmio_direct and moved it to common/memory.c, since 
none of the files in passthrough/ seemed like a good place to put it.

> > +{
> > +int rc;
> > +
> > +while ( nr_pages > 0 )
> > +{
> > +rc = (map ? map_mmio_regions : unmap_mmio_regions)
> > + (d, _gfn(pfn), nr_pages, _mfn(pfn));
> > +if ( rc == 0 )
> > +break;
> > +if ( rc < 0 )
> > +{
> > +printk(XENLOG_ERR
> > +"Failed to %smap %#lx - %#lx into domain %d memory map: 
> > %d\n",
> 
> "Failed to identity %smap [%#lx,%#lx) for d%d: %d\n"
> 
> And I think XENLOG_WARNING would do - whether this actually is
> a problem depends on further factors.

Done.

> > +   map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc);
> > +return rc;
> > +}
> > +nr_pages -= rc;
> > +pfn += rc;
> > +process_pending_softirqs();
> 
> Is this what you call "preemptive"? 

Right, I've removed preemptive from the comment since it makes no sense.

> > +}
> > +
> > +return rc;
> 
> The way this is coded it appears to possibly return non-zero even in
> success case. I think this would therefore better be a for ( ; ; ) loop.

I don't think this is possible, {un}map_mmio_regions will return < 0 on 
error, > 0 if there are pending pages to map, and 0 when all the requested 
pages have been mapped successfully.

Thanks, Roger.

Re: [Xen-devel] [PATCH v2 09/30] x86/vtd: fix and simplify mapping RMRR regions

2016-09-29 Thread Jan Beulich 
>>> On 27.09.16 at 17:57,  wrote:
> The current code used by Intel VTd will only map RMRR regions for the
> hardware domain, but will fail to map RMRR regions for unprivileged domains
> unless the page tables are shared between EPT and IOMMU.

Okay, if that's the case it surely should get fixed.

> Fix this and
> simplify the code, removing the {set/clear}_identity_p2m_entry helpers and
> just using the normal MMIO mapping functions.

This simplification, however, goes too far. Namely ...

> -int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
> -   p2m_access_t p2ma, unsigned int flag)
> -{
> -p2m_type_t p2mt;
> -p2m_access_t a;
> -mfn_t mfn;
> -struct p2m_domain *p2m = p2m_get_hostp2m(d);
> -int ret;
> -
> -if ( !paging_mode_translate(p2m->domain) )
> -{
> -if ( !need_iommu(d) )
> -return 0;
> -return iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
> -}
> -
> -gfn_lock(p2m, gfn, 0);
> -
> -mfn = p2m->get_entry(p2m, gfn, , , 0, NULL, NULL);
> -
> -if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
> -ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
> -p2m_mmio_direct, p2ma);
> -else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
> -{
> -ret = 0;
> -/*
> - * PVH fixme: during Dom0 PVH construction, p2m entries are being set
> - * but iomem regions are not mapped with IOMMU. This makes sure that
> - * RMRRs are correctly mapped with IOMMU.
> - */
> -if ( is_hardware_domain(d) && !iommu_use_hap_pt(d) )
> -ret = iommu_map_page(d, gfn, gfn, 
> IOMMUF_readable|IOMMUF_writable);
> -}
> -else
> -{
> -if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED )
> -ret = 0;
> -else
> -ret = -EBUSY;
> -printk(XENLOG_G_WARNING
> -   "Cannot setup identity map d%d:%lx,"
> -   " gfn already mapped to %lx.\n",
> -   d->domain_id, gfn, mfn_x(mfn));

... this logic (and its clear side counterpart) should not be removed
without replacement. Note in this context how you render "flag" an
unused parameter of rmrr_identity_mapping().

> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -2,6 +2,7 @@
>  #define _XEN_P2M_COMMON_H
>  
>  #include 
> +#include 
>  
>  /*
>   * Additional access types, which are used to further restrict
> @@ -46,6 +47,35 @@ int unmap_mmio_regions(struct domain *d,
> mfn_t mfn);
>  
>  /*
> + * Preemptive Helper for mapping/unmapping MMIO regions.
> + */

Single line comment.

> +static inline int modify_mmio_11(struct domain *d, unsigned long pfn,
> + unsigned long nr_pages, bool map)

Why do you make this an inline function? And I have to admit that I
dislike this strange use of number 11 - what's wrong with continuing
to use the term "direct map" in one way or another?

> +{
> +int rc;
> +
> +while ( nr_pages > 0 )
> +{
> +rc = (map ? map_mmio_regions : unmap_mmio_regions)
> + (d, _gfn(pfn), nr_pages, _mfn(pfn));
> +if ( rc == 0 )
> +break;
> +if ( rc < 0 )
> +{
> +printk(XENLOG_ERR
> +"Failed to %smap %#lx - %#lx into domain %d memory map: 
> %d\n",

"Failed to identity %smap [%#lx,%#lx) for d%d: %d\n"

And I think XENLOG_WARNING would do - whether this actually is
a problem depends on further factors.

> +   map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc);
> +return rc;
> +}
> +nr_pages -= rc;
> +pfn += rc;
> +process_pending_softirqs();

Is this what you call "preemptive"? 

> +}
> +
> +return rc;

The way this is coded it appears to possibly return non-zero even in
success case. I think this would therefore better be a for ( ; ; ) loop.

Jan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v2 09/30] x86/vtd: fix and simplify mapping RMRR regions

2016-09-27 Thread Roger Pau Monne 
The current code used by Intel VTd will only map RMRR regions for the
hardware domain, but will fail to map RMRR regions for unprivileged domains
unless the page tables are shared between EPT and IOMMU. Fix this and
simplify the code, removing the {set/clear}_identity_p2m_entry helpers and
just using the normal MMIO mapping functions. Introduce a new MMIO
mapping/unmapping helper, that takes care of checking for pending IRQs if
the mapped region is big enough that it cannot be done in one shot.

Signed-off-by: Roger Pau Monné 
---
Cc: George Dunlap 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Kevin Tian 
Cc: Feng Wu 
---
 xen/arch/x86/mm/p2m.c   | 86 -
 xen/drivers/passthrough/vtd/iommu.c | 21 +
 xen/include/asm-x86/p2m.h   |  5 ---
 xen/include/xen/p2m-common.h| 30 +
 4 files changed, 42 insertions(+), 100 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 9526fff..44492ae 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1029,56 +1029,6 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long 
gfn, mfn_t mfn,
 return set_typed_p2m_entry(d, gfn, mfn, order, p2m_mmio_direct, access);
 }
 
-int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
-   p2m_access_t p2ma, unsigned int flag)
-{
-p2m_type_t p2mt;
-p2m_access_t a;
-mfn_t mfn;
-struct p2m_domain *p2m = p2m_get_hostp2m(d);
-int ret;
-
-if ( !paging_mode_translate(p2m->domain) )
-{
-if ( !need_iommu(d) )
-return 0;
-return iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
-}
-
-gfn_lock(p2m, gfn, 0);
-
-mfn = p2m->get_entry(p2m, gfn, , , 0, NULL, NULL);
-
-if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
-ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
-p2m_mmio_direct, p2ma);
-else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
-{
-ret = 0;
-/*
- * PVH fixme: during Dom0 PVH construction, p2m entries are being set
- * but iomem regions are not mapped with IOMMU. This makes sure that
- * RMRRs are correctly mapped with IOMMU.
- */
-if ( is_hardware_domain(d) && !iommu_use_hap_pt(d) )
-ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
-}
-else
-{
-if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED )
-ret = 0;
-else
-ret = -EBUSY;
-printk(XENLOG_G_WARNING
-   "Cannot setup identity map d%d:%lx,"
-   " gfn already mapped to %lx.\n",
-   d->domain_id, gfn, mfn_x(mfn));
-}
-
-gfn_unlock(p2m, gfn, 0);
-return ret;
-}
-
 /*
  * Returns:
  *0for success
@@ -1127,42 +1077,6 @@ int clear_mmio_p2m_entry(struct domain *d, unsigned long 
gfn, mfn_t mfn,
 return rc;
 }
 
-int clear_identity_p2m_entry(struct domain *d, unsigned long gfn)
-{
-p2m_type_t p2mt;
-p2m_access_t a;
-mfn_t mfn;
-struct p2m_domain *p2m = p2m_get_hostp2m(d);
-int ret;
-
-if ( !paging_mode_translate(d) )
-{
-if ( !need_iommu(d) )
-return 0;
-return iommu_unmap_page(d, gfn);
-}
-
-gfn_lock(p2m, gfn, 0);
-
-mfn = p2m->get_entry(p2m, gfn, , , 0, NULL, NULL);
-if ( p2mt == p2m_mmio_direct && mfn_x(mfn) == gfn )
-{
-ret = p2m_set_entry(p2m, gfn, INVALID_MFN, PAGE_ORDER_4K,
-p2m_invalid, p2m->default_access);
-gfn_unlock(p2m, gfn, 0);
-}
-else
-{
-gfn_unlock(p2m, gfn, 0);
-printk(XENLOG_G_WARNING
-   "non-identity map d%d:%lx not cleared (mapped to %lx)\n",
-   d->domain_id, gfn, mfn_x(mfn));
-ret = 0;
-}
-
-return ret;
-}
-
 /* Returns: 0 for success, -errno for failure */
 int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
 {
diff --git a/xen/drivers/passthrough/vtd/iommu.c 
b/xen/drivers/passthrough/vtd/iommu.c
index 919993e..714a19e 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1896,6 +1896,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t 
map,
 unsigned long end_pfn = PAGE_ALIGN_4K(rmrr->end_address) >> PAGE_SHIFT_4K;
 struct mapped_rmrr *mrmrr;
 struct domain_iommu *hd = dom_iommu(d);
+int ret = 0;
 
 ASSERT(pcidevs_locked());
 ASSERT(rmrr->base_address < rmrr->end_address);
@@ -1909,8 +1910,6 @@ static int rmrr_identity_mapping(struct domain *d, bool_t 
map,
 if ( mrmrr->base == rmrr->base_address &&
  mrmrr->end == rmrr->end_address )
 {
-int ret = 0;
-
 if ( map )
 {