[v3.1,08/15] x86/vtd: fix mapping of RMRR regions

Commit Message

Roger Pau Monné Nov. 4, 2016, 3:33 p.m. UTC

On Fri, Nov 04, 2016 at 07:16:08AM -0600, Jan Beulich wrote:
> >>> On 04.11.16 at 14:03, <[e-mail]> wrote:
> > On Fri, Nov 04, 2016 at 06:53:09AM -0600, Jan Beulich wrote:
> >> >>> On 04.11.16 at 13:25, <[e-mail]> wrote:
> >> > On Fri, Nov 04, 2016 at 04:34:58AM -0600, Jan Beulich wrote:
> >> >> >>> On 04.11.16 at 10:45, <[e-mail]> wrote:
> >> >> > case p2m_invalid:
> >> >> > case p2m_mmio_dm:
> >> >> >     ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
> >> >> >                         p2m_mmio_direct, p2ma);
> >> >> >     if ( ret )
> >> >> >         break;
> >> >> >     if ( !iommu_use_hap_pt(d) )
> >> >> >         ret = iommu_map_page(d, gfn, gfn, 
> > IOMMUF_readable|IOMMUF_writable);
> >> >> >     break;
> >> >> > case p2m_mmio_direct:
> >> >> >     if ( a != p2ma || gfn != mfn )
> >> >> >     {
> >> >> >         printk(XENLOG_G_WARNING
> >> >> >                "Cannot setup identity map d%d:%lx, already mapped with "
> >> >> >                "different access type or mfn\n", d->domain_id, gfn);
> >> >> >         ret = (flag & XEN_DOMCTL_DEV_RDM_RELAXED) ? 0 : -EBUSY;
> >> >> >         break;
> >> >> >     }
> >> >> >     if ( !iommu_use_hap_pt(d) )
> >> >> >         ret = iommu_map_page(d, gfn, gfn, 
> > IOMMUF_readable|IOMMUF_writable);
> >> >> 
> >> >> Well, since according to what I've said above this code should
> >> >> really not be here, I think the code structuring question is moot
> >> >> now. The conditional call to iommu_map_page() really just needs
> >> >> adding alongside the p2m_set_entry() call.
> >> > 
> >> > OK, so if the gfn is already mapped into the p2m we don't care whether it 
> >> > has a valid IOMMU mapping or not?
> >> 
> >> We do care, but it is the responsibility of whoever established the
> >> first mapping to make sure it's present in both P2M and IOMMU.
> >> IOW if the GFN is already mapped, we should be able to imply that
> >> it's mapped in both places.
> > 
> > But how is the first caller that established the mapping supposed to know if 
> > it needs an IOMMU entry or not? (p2m_mmio_direct types don't get an IOMMU 
> > mapping at all)
> 
> And it's that fact stated in parentheses which I'd like to question.
> I don't see what's wrong with e.g. DMAing right into / out of a
> video frame buffer.

Right, so what about the following patch. It would fix my issues, and also 
remove the PVH hack in set_identity_p2m_entry:

---
x86/iommu: add IOMMU entries for p2m_mmio_direct pages

There's nothing wrong with allowing the domain to perform DMA transfers to 
MMIO areas that it already can access from the CPU, and this allows us to 
remove the hack in set_identity_p2m_entry for PVH Dom0.

Signed-off-by: Roger Pau Monné <[e-mail]>
---
 xen/arch/x86/mm/p2m.c     |    9 ---------
 xen/include/asm-x86/p2m.h |    1 +
 2 files changed, 1 insertion(+), 9 deletions(-)

     case p2m_ram_ro:

Comments

Jan Beulich Nov. 4, 2016, 4:13 p.m. UTC | #1

>>> On 04.11.16 at 16:33, <[e-mail]> wrote:
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -834,6 +834,7 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
>      case p2m_grant_map_rw:
>      case p2m_ram_logdirty:
>      case p2m_map_foreign:
> +    case p2m_mmio_direct:
>          flags =  IOMMUF_readable | IOMMUF_writable;
>          break;
>      case p2m_ram_ro:

Generally this may be the route to go. But if we want to do so, we
need to throughly understand why this type wasn't included here
before (and I don't know myself).

Jan

Roger Pau Monné Nov. 4, 2016, 4:19 p.m. UTC | #2

On Fri, Nov 04, 2016 at 10:13:08AM -0600, Jan Beulich wrote:
> >>> On 04.11.16 at 16:33, <[e-mail]> wrote:
> > --- a/xen/include/asm-x86/p2m.h
> > +++ b/xen/include/asm-x86/p2m.h
> > @@ -834,6 +834,7 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
> >      case p2m_grant_map_rw:
> >      case p2m_ram_logdirty:
> >      case p2m_map_foreign:
> > +    case p2m_mmio_direct:
> >          flags =  IOMMUF_readable | IOMMUF_writable;
> >          break;
> >      case p2m_ram_ro:
> 
> Generally this may be the route to go. But if we want to do so, we
> need to throughly understand why this type wasn't included here
> before (and I don't know myself).

It was me that introduced p2m_get_iommu_flags, and I just didn't think it 
would be useful at that point, that's why it wasn't included.

The patch that introduced p2m_get_iommu_flags was focused on fixing DMA'ing 
from grant-pages on HVM guests with passed-through hardware, this was needed 
for driver domains and later on by classic PVH Dom0

Roger.

Jan Beulich Nov. 4, 2016, 5:08 p.m. UTC | #3

>>> On 04.11.16 at 17:19, <[e-mail]> wrote:
> On Fri, Nov 04, 2016 at 10:13:08AM -0600, Jan Beulich wrote:
>> >>> On 04.11.16 at 16:33, <[e-mail]> wrote:
>> > --- a/xen/include/asm-x86/p2m.h
>> > +++ b/xen/include/asm-x86/p2m.h
>> > @@ -834,6 +834,7 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
>> >      case p2m_grant_map_rw:
>> >      case p2m_ram_logdirty:
>> >      case p2m_map_foreign:
>> > +    case p2m_mmio_direct:
>> >          flags =  IOMMUF_readable | IOMMUF_writable;
>> >          break;
>> >      case p2m_ram_ro:
>> 
>> Generally this may be the route to go. But if we want to do so, we
>> need to throughly understand why this type wasn't included here
>> before (and I don't know myself).
> 
> It was me that introduced p2m_get_iommu_flags, and I just didn't think it 
> would be useful at that point, that's why it wasn't included.

But there must have been logic to set the permissions before that?

Jan

Roger Pau Monné Nov. 4, 2016, 5:25 p.m. UTC | #4

On Fri, Nov 04, 2016 at 11:08:21AM -0600, Jan Beulich wrote:
> >>> On 04.11.16 at 17:19, <[e-mail]> wrote:
> > On Fri, Nov 04, 2016 at 10:13:08AM -0600, Jan Beulich wrote:
> >> >>> On 04.11.16 at 16:33, <[e-mail]> wrote:
> >> > --- a/xen/include/asm-x86/p2m.h
> >> > +++ b/xen/include/asm-x86/p2m.h
> >> > @@ -834,6 +834,7 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
> >> >      case p2m_grant_map_rw:
> >> >      case p2m_ram_logdirty:
> >> >      case p2m_map_foreign:
> >> > +    case p2m_mmio_direct:
> >> >          flags =  IOMMUF_readable | IOMMUF_writable;
> >> >          break;
> >> >      case p2m_ram_ro:
> >> 
> >> Generally this may be the route to go. But if we want to do so, we
> >> need to throughly understand why this type wasn't included here
> >> before (and I don't know myself).
> > 
> > It was me that introduced p2m_get_iommu_flags, and I just didn't think it 
> > would be useful at that point, that's why it wasn't included.
> 
> But there must have been logic to set the permissions before that?

Previous to that only p2m_ram_rw would get IOMMU mappings. I've tracked this 
back to ff635e12, but there's no mention there about why only p2m_ram_rw 
would get IOMMU mappings.

Considering that on hw with shared page-tables this is already available, I 
don't see an issue with also doing it for the non-shared pt case.

Roger.

Jan Beulich Nov. 7, 2016, 8:36 a.m. UTC | #5

>>> On 04.11.16 at 18:25, <[e-mail]> wrote:
> On Fri, Nov 04, 2016 at 11:08:21AM -0600, Jan Beulich wrote:
>> >>> On 04.11.16 at 17:19, <[e-mail]> wrote:
>> > On Fri, Nov 04, 2016 at 10:13:08AM -0600, Jan Beulich wrote:
>> >> >>> On 04.11.16 at 16:33, <[e-mail]> wrote:
>> >> > --- a/xen/include/asm-x86/p2m.h
>> >> > +++ b/xen/include/asm-x86/p2m.h
>> >> > @@ -834,6 +834,7 @@ static inline unsigned int 
> p2m_get_iommu_flags(p2m_type_t p2mt)
>> >> >      case p2m_grant_map_rw:
>> >> >      case p2m_ram_logdirty:
>> >> >      case p2m_map_foreign:
>> >> > +    case p2m_mmio_direct:
>> >> >          flags =  IOMMUF_readable | IOMMUF_writable;
>> >> >          break;
>> >> >      case p2m_ram_ro:
>> >> 
>> >> Generally this may be the route to go. But if we want to do so, we
>> >> need to throughly understand why this type wasn't included here
>> >> before (and I don't know myself).
>> > 
>> > It was me that introduced p2m_get_iommu_flags, and I just didn't think it 
>> > would be useful at that point, that's why it wasn't included.
>> 
>> But there must have been logic to set the permissions before that?
> 
> Previous to that only p2m_ram_rw would get IOMMU mappings. I've tracked this 
> back to ff635e12, but there's no mention there about why only p2m_ram_rw 
> would get IOMMU mappings.
> 
> Considering that on hw with shared page-tables this is already available, I 
> don't see an issue with also doing it for the non-shared pt case.

True.

Jan

Patch

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6a45185..7e33ab6 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1053,16 +1053,7 @@  int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
         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 )
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 7035860..b562da3 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -834,6 +834,7 @@  static inline unsigned int 
p2m_get_iommu_flags(p2m_type_t p2mt)
     case p2m_grant_map_rw:
     case p2m_ram_logdirty:
     case p2m_map_foreign:
+    case p2m_mmio_direct:
         flags =  IOMMUF_readable | IOMMUF_writable;
         break;