diff mbox

[V3,05/10] intel_iommu: support device iotlb descriptor

Message ID 1478502595-8484-6-git-send-email-jasowang@redhat.com
State New
Headers show

Commit Message

Jason Wang Nov. 7, 2016, 7:09 a.m. UTC 
This patch enables device IOTLB support for intel iommu. The major
work is to implement QI device IOTLB descriptor processing and notify
the device through iommu notifier.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/intel_iommu.c          | 81 +++++++++++++++++++++++++++++++++++++++---
 hw/i386/intel_iommu_internal.h | 13 +++++--
 hw/i386/x86-iommu.c            | 17 +++++++++
 include/hw/i386/x86-iommu.h    |  1 +
 4 files changed, 105 insertions(+), 7 deletions(-)

Comments

Peter Xu Nov. 7, 2016, 11:35 p.m. UTC | #1 
On Mon, Nov 07, 2016 at 03:09:50PM +0800, Jason Wang wrote:

[...]

> +static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
> +                                          VTDInvDesc *inv_desc)
> +{
> +    VTDAddressSpace *vtd_dev_as;
> +    IOMMUTLBEntry entry;

Since "entry" is allocated on the stack...

[...]

> +    entry.target_as = &vtd_dev_as->as;
> +    entry.addr_mask = sz - 1;
> +    entry.iova = addr;
> +    memory_region_notify_iommu(entry.target_as->root, entry);

... here we need to assign entry.perm explicitly to IOMMU_NONE, right?

Also I think it'll be nice that we set all the fields even not used,
to avoid rubbish from the stack passed down to notifier handlers.

[...]

> +static bool x86_iommu_device_iotlb_prop_get(Object *o, Error **errp)
> +{
> +    X86IOMMUState *s = X86_IOMMU_DEVICE(o);
> +    return s->dt_supported;
> +}
> +
> +static void x86_iommu_device_iotlb_prop_set(Object *o, bool value, Error **errp)
> +{
> +    X86IOMMUState *s = X86_IOMMU_DEVICE(o);
> +    s->dt_supported = value;
> +}
> +
>  static void x86_iommu_instance_init(Object *o)
>  {
>      X86IOMMUState *s = X86_IOMMU_DEVICE(o);
> @@ -114,6 +126,11 @@ static void x86_iommu_instance_init(Object *o)
>      s->intr_supported = false;
>      object_property_add_bool(o, "intremap", x86_iommu_intremap_prop_get,
>                               x86_iommu_intremap_prop_set, NULL);
> +    s->dt_supported = false;
> +    object_property_add_bool(o, "device-iotlb",
> +                             x86_iommu_device_iotlb_prop_get,
> +                             x86_iommu_device_iotlb_prop_set,
> +                             NULL);

Again, a nit-pick here is to use Property for "device-iotlb":

    static Property vtd_properties[] = {
        DEFINE_PROP_UINT32("device-iotlb", X86IOMMUState, dt_supported, false),
        DEFINE_PROP_END_OF_LIST(),
    };

However not worth a repost.

Thanks,

-- peterx
Jason Wang Nov. 8, 2016, 6:54 a.m. UTC | #2 
On 2016年11月08日 07:35, Peter Xu wrote:
> On Mon, Nov 07, 2016 at 03:09:50PM +0800, Jason Wang wrote:
>
> [...]
>
>> +static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
>> +                                          VTDInvDesc *inv_desc)
>> +{
>> +    VTDAddressSpace *vtd_dev_as;
>> +    IOMMUTLBEntry entry;
> Since "entry" is allocated on the stack...
>
> [...]
>
>> +    entry.target_as = &vtd_dev_as->as;
>> +    entry.addr_mask = sz - 1;
>> +    entry.iova = addr;
>> +    memory_region_notify_iommu(entry.target_as->root, entry);
> ... here we need to assign entry.perm explicitly to IOMMU_NONE, right?
>
> Also I think it'll be nice that we set all the fields even not used,
> to avoid rubbish from the stack passed down to notifier handlers.
>
> [...]

This is better, if no other comments on the series I will post a patch 
on top to fix this.

>
>> +static bool x86_iommu_device_iotlb_prop_get(Object *o, Error **errp)
>> +{
>> +    X86IOMMUState *s = X86_IOMMU_DEVICE(o);
>> +    return s->dt_supported;
>> +}
>> +
>> +static void x86_iommu_device_iotlb_prop_set(Object *o, bool value, Error **errp)
>> +{
>> +    X86IOMMUState *s = X86_IOMMU_DEVICE(o);
>> +    s->dt_supported = value;
>> +}
>> +
>>   static void x86_iommu_instance_init(Object *o)
>>   {
>>       X86IOMMUState *s = X86_IOMMU_DEVICE(o);
>> @@ -114,6 +126,11 @@ static void x86_iommu_instance_init(Object *o)
>>       s->intr_supported = false;
>>       object_property_add_bool(o, "intremap", x86_iommu_intremap_prop_get,
>>                                x86_iommu_intremap_prop_set, NULL);
>> +    s->dt_supported = false;
>> +    object_property_add_bool(o, "device-iotlb",
>> +                             x86_iommu_device_iotlb_prop_get,
>> +                             x86_iommu_device_iotlb_prop_set,
>> +                             NULL);
> Again, a nit-pick here is to use Property for "device-iotlb":
>
>      static Property vtd_properties[] = {
>          DEFINE_PROP_UINT32("device-iotlb", X86IOMMUState, dt_supported, false),
>          DEFINE_PROP_END_OF_LIST(),
>      };
>
> However not worth a repost.
>
> Thanks,
>
> -- peterx
>

We may want to share this with AMD IOMMU. (Looking at AMD IOMMU codes, 
its device-iotlb support is buggy).

Thanks
Michael S. Tsirkin Nov. 8, 2016, 8:31 p.m. UTC | #3 
On Tue, Nov 08, 2016 at 02:54:19PM +0800, Jason Wang wrote:
> 
> 
> On 2016年11月08日 07:35, Peter Xu wrote:
> > On Mon, Nov 07, 2016 at 03:09:50PM +0800, Jason Wang wrote:
> > 
> > [...]
> > 
> > > +static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
> > > +                                          VTDInvDesc *inv_desc)
> > > +{
> > > +    VTDAddressSpace *vtd_dev_as;
> > > +    IOMMUTLBEntry entry;
> > Since "entry" is allocated on the stack...
> > 
> > [...]
> > 
> > > +    entry.target_as = &vtd_dev_as->as;
> > > +    entry.addr_mask = sz - 1;
> > > +    entry.iova = addr;
> > > +    memory_region_notify_iommu(entry.target_as->root, entry);
> > ... here we need to assign entry.perm explicitly to IOMMU_NONE, right?
> > 
> > Also I think it'll be nice that we set all the fields even not used,
> > to avoid rubbish from the stack passed down to notifier handlers.
> > 
> > [...]
> 
> This is better, if no other comments on the series I will post a patch on
> top to fix this.

If you do, pls remember to use the fixup! prefix.

> > 
> > > +static bool x86_iommu_device_iotlb_prop_get(Object *o, Error **errp)
> > > +{
> > > +    X86IOMMUState *s = X86_IOMMU_DEVICE(o);
> > > +    return s->dt_supported;
> > > +}
> > > +
> > > +static void x86_iommu_device_iotlb_prop_set(Object *o, bool value, Error **errp)
> > > +{
> > > +    X86IOMMUState *s = X86_IOMMU_DEVICE(o);
> > > +    s->dt_supported = value;
> > > +}
> > > +
> > >   static void x86_iommu_instance_init(Object *o)
> > >   {
> > >       X86IOMMUState *s = X86_IOMMU_DEVICE(o);
> > > @@ -114,6 +126,11 @@ static void x86_iommu_instance_init(Object *o)
> > >       s->intr_supported = false;
> > >       object_property_add_bool(o, "intremap", x86_iommu_intremap_prop_get,
> > >                                x86_iommu_intremap_prop_set, NULL);
> > > +    s->dt_supported = false;
> > > +    object_property_add_bool(o, "device-iotlb",
> > > +                             x86_iommu_device_iotlb_prop_get,
> > > +                             x86_iommu_device_iotlb_prop_set,
> > > +                             NULL);
> > Again, a nit-pick here is to use Property for "device-iotlb":
> > 
> >      static Property vtd_properties[] = {
> >          DEFINE_PROP_UINT32("device-iotlb", X86IOMMUState, dt_supported, false),
> >          DEFINE_PROP_END_OF_LIST(),
> >      };
> > 
> > However not worth a repost.
> > 
> > Thanks,
> > 
> > -- peterx
> > 
> 
> We may want to share this with AMD IOMMU. (Looking at AMD IOMMU codes, its
> device-iotlb support is buggy).
> 
> Thanks
diff mbox

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index a2678bc..c1da0a6 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -738,11 +738,18 @@  static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
                     "context-entry hi 0x%"PRIx64 " lo 0x%"PRIx64,
                     ce->hi, ce->lo);
         return -VTD_FR_CONTEXT_ENTRY_INV;
-    } else if (ce->lo & VTD_CONTEXT_ENTRY_TT) {
-        VTD_DPRINTF(GENERAL, "error: unsupported Translation Type in "
-                    "context-entry hi 0x%"PRIx64 " lo 0x%"PRIx64,
-                    ce->hi, ce->lo);
-        return -VTD_FR_CONTEXT_ENTRY_INV;
+    } else {
+        switch (ce->lo & VTD_CONTEXT_ENTRY_TT) {
+        case VTD_CONTEXT_TT_MULTI_LEVEL:
+            /* fall through */
+        case VTD_CONTEXT_TT_DEV_IOTLB:
+            break;
+        default:
+            VTD_DPRINTF(GENERAL, "error: unsupported Translation Type in "
+                        "context-entry hi 0x%"PRIx64 " lo 0x%"PRIx64,
+                        ce->hi, ce->lo);
+            return -VTD_FR_CONTEXT_ENTRY_INV;
+        }
     }
     return 0;
 }
@@ -1437,7 +1444,59 @@  static bool vtd_process_inv_iec_desc(IntelIOMMUState *s,
     vtd_iec_notify_all(s, !inv_desc->iec.granularity,
                        inv_desc->iec.index,
                        inv_desc->iec.index_mask);
+    return true;
+}
+
+static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
+                                          VTDInvDesc *inv_desc)
+{
+    VTDAddressSpace *vtd_dev_as;
+    IOMMUTLBEntry entry;
+    struct VTDBus *vtd_bus;
+    hwaddr addr;
+    uint64_t sz;
+    uint16_t sid;
+    uint8_t devfn;
+    bool size;
+    uint8_t bus_num;
+
+    addr = VTD_INV_DESC_DEVICE_IOTLB_ADDR(inv_desc->hi);
+    sid = VTD_INV_DESC_DEVICE_IOTLB_SID(inv_desc->lo);
+    devfn = sid & 0xff;
+    bus_num = sid >> 8;
+    size = VTD_INV_DESC_DEVICE_IOTLB_SIZE(inv_desc->hi);
+
+    if ((inv_desc->lo & VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO) ||
+        (inv_desc->hi & VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI)) {
+        VTD_DPRINTF(GENERAL, "error: non-zero reserved field in Device "
+                    "IOTLB Invalidate Descriptor hi 0x%"PRIx64 " lo 0x%"PRIx64,
+                    inv_desc->hi, inv_desc->lo);
+        return false;
+    }
+
+    vtd_bus = vtd_find_as_from_bus_num(s, bus_num);
+    if (!vtd_bus) {
+        goto done;
+    }
+
+    vtd_dev_as = vtd_bus->dev_as[devfn];
+    if (!vtd_dev_as) {
+        goto done;
+    }
+
+    if (size) {
+        sz = 1 << (ctz64(~(addr | (VTD_PAGE_MASK_4K - 1))) + 1);
+        addr &= ~(sz - 1);
+    } else {
+        sz = VTD_PAGE_SIZE;
+    }
 
+    entry.target_as = &vtd_dev_as->as;
+    entry.addr_mask = sz - 1;
+    entry.iova = addr;
+    memory_region_notify_iommu(entry.target_as->root, entry);
+
+done:
     return true;
 }
 
@@ -1489,6 +1548,14 @@  static bool vtd_process_inv_desc(IntelIOMMUState *s)
         }
         break;
 
+    case VTD_INV_DESC_DEVICE:
+        VTD_DPRINTF(INV, "Device IOTLB Invalidation Descriptor hi 0x%"PRIx64
+                    " lo 0x%"PRIx64, inv_desc.hi, inv_desc.lo);
+        if (!vtd_process_device_iotlb_desc(s, &inv_desc)) {
+            return false;
+        }
+        break;
+
     default:
         VTD_DPRINTF(GENERAL, "error: unkonw Invalidation Descriptor type "
                     "hi 0x%"PRIx64 " lo 0x%"PRIx64 " type %"PRIu8,
@@ -2394,6 +2461,10 @@  static void vtd_init(IntelIOMMUState *s)
         assert(s->intr_eim != ON_OFF_AUTO_AUTO);
     }
 
+    if (x86_iommu->dt_supported) {
+        s->ecap |= VTD_ECAP_DT;
+    }
+
     vtd_reset_context_cache(s);
     vtd_reset_iotlb(s);
 
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 0829a50..b195d8a 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -183,6 +183,7 @@ 
 /* (offset >> 4) << 8 */
 #define VTD_ECAP_IRO                (DMAR_IOTLB_REG_OFFSET << 4)
 #define VTD_ECAP_QI                 (1ULL << 1)
+#define VTD_ECAP_DT                 (1ULL << 2)
 /* Interrupt Remapping support */
 #define VTD_ECAP_IR                 (1ULL << 3)
 #define VTD_ECAP_EIM                (1ULL << 4)
@@ -326,6 +327,7 @@  typedef union VTDInvDesc VTDInvDesc;
 #define VTD_INV_DESC_TYPE               0xf
 #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
 #define VTD_INV_DESC_IOTLB              0x2
+#define VTD_INV_DESC_DEVICE             0x3
 #define VTD_INV_DESC_IEC                0x4 /* Interrupt Entry Cache
                                                Invalidate Descriptor */
 #define VTD_INV_DESC_WAIT               0x5 /* Invalidation Wait Descriptor */
@@ -361,6 +363,13 @@  typedef union VTDInvDesc VTDInvDesc;
 #define VTD_INV_DESC_IOTLB_RSVD_LO      0xffffffff0000ff00ULL
 #define VTD_INV_DESC_IOTLB_RSVD_HI      0xf80ULL
 
+/* Mask for Device IOTLB Invalidate Descriptor */
+#define VTD_INV_DESC_DEVICE_IOTLB_ADDR(val) ((val) & 0xfffffffffffff000ULL)
+#define VTD_INV_DESC_DEVICE_IOTLB_SIZE(val) ((val) & 0x1)
+#define VTD_INV_DESC_DEVICE_IOTLB_SID(val) (((val) >> 32) & 0xFFFFULL)
+#define VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI 0xffeULL
+#define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0fff8
+
 /* Information about page-selective IOTLB invalidate */
 struct VTDIOTLBPageInvInfo {
     uint16_t domain_id;
@@ -399,8 +408,8 @@  typedef struct VTDRootEntry VTDRootEntry;
 #define VTD_CONTEXT_ENTRY_FPD       (1ULL << 1) /* Fault Processing Disable */
 #define VTD_CONTEXT_ENTRY_TT        (3ULL << 2) /* Translation Type */
 #define VTD_CONTEXT_TT_MULTI_LEVEL  0
-#define VTD_CONTEXT_TT_DEV_IOTLB    1
-#define VTD_CONTEXT_TT_PASS_THROUGH 2
+#define VTD_CONTEXT_TT_DEV_IOTLB    (1ULL << 2)
+#define VTD_CONTEXT_TT_PASS_THROUGH (2ULL << 2)
 /* Second Level Page Translation Pointer*/
 #define VTD_CONTEXT_ENTRY_SLPTPTR   (~0xfffULL)
 #define VTD_CONTEXT_ENTRY_RSVD_LO   (0xff0ULL | ~VTD_HAW_MASK)
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 2278af7..23dcd3f 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -106,6 +106,18 @@  static void x86_iommu_intremap_prop_set(Object *o, bool value, Error **errp)
     s->intr_supported = value;
 }
 
+static bool x86_iommu_device_iotlb_prop_get(Object *o, Error **errp)
+{
+    X86IOMMUState *s = X86_IOMMU_DEVICE(o);
+    return s->dt_supported;
+}
+
+static void x86_iommu_device_iotlb_prop_set(Object *o, bool value, Error **errp)
+{
+    X86IOMMUState *s = X86_IOMMU_DEVICE(o);
+    s->dt_supported = value;
+}
+
 static void x86_iommu_instance_init(Object *o)
 {
     X86IOMMUState *s = X86_IOMMU_DEVICE(o);
@@ -114,6 +126,11 @@  static void x86_iommu_instance_init(Object *o)
     s->intr_supported = false;
     object_property_add_bool(o, "intremap", x86_iommu_intremap_prop_get,
                              x86_iommu_intremap_prop_set, NULL);
+    s->dt_supported = false;
+    object_property_add_bool(o, "device-iotlb",
+                             x86_iommu_device_iotlb_prop_get,
+                             x86_iommu_device_iotlb_prop_set,
+                             NULL);
 }
 
 static const TypeInfo x86_iommu_info = {
diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
index 0c89d98..361c07c 100644
--- a/include/hw/i386/x86-iommu.h
+++ b/include/hw/i386/x86-iommu.h
@@ -73,6 +73,7 @@  typedef struct IEC_Notifier IEC_Notifier;
 struct X86IOMMUState {
     SysBusDevice busdev;
     bool intr_supported;        /* Whether vIOMMU supports IR */
+    bool dt_supported;          /* Whether vIOMMU supports DT */
     IommuType type;             /* IOMMU type - AMD/Intel     */
     QLIST_HEAD(, IEC_Notifier) iec_notifiers; /* IEC notify list */
 };