| Message ID | 1461827144-6937-16-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2016-04-28 09:05, Peter Xu wrote: > This patch introduces Intel VT-d IEC (Interrupt Entry Cache) > invalidation notifier list. When vIOMMU receives IEC invalidate request, > all the registered units will be notified with specific invalidation > requests. This should be designed to be IOMMU-agnostic, i.e. become reusable for the AMD implementation. I suspect we will have the same need for route invalidations there as well... Jan > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/i386/intel_iommu.c | 56 ++++++++++++++++++++++++++++++++++++------ > hw/i386/intel_iommu_internal.h | 24 +++++++++++++++--- > include/hw/i386/intel_iommu.h | 22 +++++++++++++++++ > 3 files changed, 91 insertions(+), 11 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index a8a57db..7122e5b 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -900,6 +900,22 @@ static void vtd_root_table_setup(IntelIOMMUState *s) > (s->root_extended ? "(extended)" : "")); > } > > +static void vtd_iec_notify_all(IntelIOMMUState *s, bool global, > + uint32_t index, uint32_t mask) > +{ > + VTD_IEC_Notifier *notifier; > + > + VTD_DPRINTF(INV, "notify IEC invalidate: global=%d, index=%u, mask=%u", > + global, index, mask); > + > + QLIST_FOREACH(notifier, &s->iec_notifiers, list) { > + if (notifier->iec_notify) { > + notifier->iec_notify(notifier->private, global, > + index, mask); > + } > + } > +} > + > static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s) > { > uint64_t value = 0; > @@ -907,7 +923,8 @@ static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s) > s->intr_size = 1UL << ((value & VTD_IRTA_SIZE_MASK) + 1); > s->intr_root = value & VTD_IRTA_ADDR_MASK; > > - /* TODO: invalidate interrupt entry cache */ > + /* Notify global invalidation */ > + vtd_iec_notify_all(s, true, 0, 0); > > VTD_DPRINTF(CSR, "int remap table addr 0x%"PRIx64 " size %"PRIu32, > s->intr_root, s->intr_size); > @@ -1409,6 +1426,21 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc) > return true; > } > > +static bool vtd_process_inv_iec_desc(IntelIOMMUState *s, > + VTDInvDesc *inv_desc) > +{ > + VTD_DPRINTF(INV, "inv ir glob %d index %d mask %d", > + inv_desc->iec.granularity, > + inv_desc->iec.index, > + inv_desc->iec.index_mask); > + > + vtd_iec_notify_all(s, inv_desc->iec.granularity, > + inv_desc->iec.index, > + inv_desc->iec.index_mask); > + > + return true; > +} > + > static bool vtd_process_inv_desc(IntelIOMMUState *s) > { > VTDInvDesc inv_desc; > @@ -1449,12 +1481,12 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s) > break; > > case VTD_INV_DESC_IEC: > - VTD_DPRINTF(INV, "Interrupt Entry Cache Invalidation " > - "not implemented yet"); > - /* > - * Since currently we do not cache interrupt entries, we can > - * just mark this descriptor as "good" and move on. > - */ > + VTD_DPRINTF(INV, "Invalidation Interrupt Entry Cache " > + "Descriptor hi 0x%"PRIx64 " lo 0x%"PRIx64, > + inv_desc.hi, inv_desc.lo); > + if (!vtd_process_inv_iec_desc(s, &inv_desc)) { > + return false; > + } > break; > > default: > @@ -2209,6 +2241,15 @@ static const MemoryRegionOps vtd_mem_ir_ops = { > }, > }; > > +void vtd_iec_register_notifier(IntelIOMMUState *s, vtd_iec_notify_fn fn, > + void *data) > +{ > + VTD_IEC_Notifier *notifier = g_new0(VTD_IEC_Notifier, 1); > + notifier->iec_notify = fn; > + notifier->private = data; > + QLIST_INSERT_HEAD(&s->iec_notifiers, notifier, list); > +} > + > VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) > { > uintptr_t key = (uintptr_t)bus; > @@ -2371,6 +2412,7 @@ static void vtd_realize(DeviceState *dev, Error **errp) > g_free, g_free); > s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, > g_free, g_free); > + QLIST_INIT(&s->iec_notifiers); > vtd_init(s); > } > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > index e1a08cb..10c20fe 100644 > --- a/hw/i386/intel_iommu_internal.h > +++ b/hw/i386/intel_iommu_internal.h > @@ -296,12 +296,28 @@ typedef enum VTDFaultReason { > > #define VTD_CONTEXT_CACHE_GEN_MAX 0xffffffffUL > > +/* Interrupt Entry Cache Invalidation Descriptor: VT-d 6.5.2.7. */ > +struct VTDInvDescIEC { > + uint32_t type:4; /* Should always be 0x4 */ > + uint32_t granularity:1; /* If set, it's global IR invalidation */ > + uint32_t resved_1:22; > + uint32_t index_mask:5; /* 2^N for continuous int invalidation */ > + uint32_t index:16; /* Start index to invalidate */ > + uint32_t reserved_2:16; > +}; > +typedef struct VTDInvDescIEC VTDInvDescIEC; > + > /* Queued Invalidation Descriptor */ > -struct VTDInvDesc { > - uint64_t lo; > - uint64_t hi; > +union VTDInvDesc { > + struct { > + uint64_t lo; > + uint64_t hi; > + }; > + union { > + VTDInvDescIEC iec; > + }; > }; > -typedef struct VTDInvDesc VTDInvDesc; > +typedef union VTDInvDesc VTDInvDesc; > > /* Masks for struct VTDInvDesc */ > #define VTD_INV_DESC_TYPE 0xf > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 5910e6f..4fe92cf 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -203,6 +203,24 @@ struct VTD_MSIMessage { > /* When IR is enabled, all MSI/MSI-X data bits should be zero */ > #define VTD_IR_MSI_DATA (0) > > +/** > + * vtd_iec_notify_fn - IEC (Interrupt Entry Cache) notifier hook, > + * triggered when IR invalidation happens. > + * @private: private data > + * @global: whether this is a global IEC invalidation > + * @index: IRTE index to invalidate (start from) > + * @mask: invalidation mask > + */ > +typedef void (*vtd_iec_notify_fn)(void *private, bool global, > + uint32_t index, uint32_t mask); > + > +struct VTD_IEC_Notifier { > + vtd_iec_notify_fn iec_notify; > + void *private; > + QLIST_ENTRY(VTD_IEC_Notifier) list; > +}; > +typedef struct VTD_IEC_Notifier VTD_IEC_Notifier; > + > /* The iommu (DMAR) device state struct */ > struct IntelIOMMUState { > SysBusDevice busdev; > @@ -243,6 +261,7 @@ struct IntelIOMMUState { > bool intr_enabled; /* Whether guest enabled IR */ > dma_addr_t intr_root; /* Interrupt remapping table pointer */ > uint32_t intr_size; /* Number of IR table entries */ > + QLIST_HEAD(, VTD_IEC_Notifier) iec_notifiers; /* IEC notify list */ > }; > > /* Find the VTD Address space associated with the given bus pointer, > @@ -252,5 +271,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn); > /* Get default IOMMU object */ > IntelIOMMUState *vtd_iommu_get(void); > int vtd_int_remap(void *iommu, MSIMessage *src, MSIMessage *dst); > +/* Register IEC invalidate notifier */ > +void vtd_iec_register_notifier(IntelIOMMUState *s, vtd_iec_notify_fn fn, > + void *data); > > #endif >
On Thu, Apr 28, 2016 at 09:26:01AM +0200, Jan Kiszka wrote: > On 2016-04-28 09:05, Peter Xu wrote: > > This patch introduces Intel VT-d IEC (Interrupt Entry Cache) > > invalidation notifier list. When vIOMMU receives IEC invalidate request, > > all the registered units will be notified with specific invalidation > > requests. > > This should be designed to be IOMMU-agnostic, i.e. become reusable for > the AMD implementation. I suspect we will have the same need for route > invalidations there as well... Yes possibly... I feel like there are lots of things that can be shared between Intel and AMD IOMMUs. I just do not know what is the most suitable "extent" that we should abstract these shared functionalities between the two, and how. For example, AFAIU, a better solution for current IOMMU codes (including Intel and AMD) is to provide a common framework (like... X86IOMMU?), abstract these shared things out into a framework, like per device name spaces, iotlb, IEC notifications, etc... However, that will need a lot of further work. Also, I still do not know whether this is a good idea even in the future. So, will this be a good point that we start to think about common code blocks for both Intel and AMD IOMMU? Thanks, -- peterx
On 2016-04-28 10:29, Peter Xu wrote: > On Thu, Apr 28, 2016 at 09:26:01AM +0200, Jan Kiszka wrote: >> On 2016-04-28 09:05, Peter Xu wrote: >>> This patch introduces Intel VT-d IEC (Interrupt Entry Cache) >>> invalidation notifier list. When vIOMMU receives IEC invalidate request, >>> all the registered units will be notified with specific invalidation >>> requests. >> >> This should be designed to be IOMMU-agnostic, i.e. become reusable for >> the AMD implementation. I suspect we will have the same need for route >> invalidations there as well... > > Yes possibly... > > I feel like there are lots of things that can be shared between > Intel and AMD IOMMUs. I just do not know what is the most suitable > "extent" that we should abstract these shared functionalities > between the two, and how. A rough indicator: if you add something that has "vtd" in its name to non-vtd code, think twice about some reusable abstraction ;). So, something like "vtd_get_iommu" could already be named and designed to have two provides (e.g. allow an IOMMU to register itself as provider, return that registered instance(s) when requested). > > For example, AFAIU, a better solution for current IOMMU > codes (including Intel and AMD) is to provide a common framework > (like... X86IOMMU?), abstract these shared things out into a > framework, like per device name spaces, iotlb, IEC notifications, > etc... However, that will need a lot of further work. Also, I still > do not know whether this is a good idea even in the future. > > So, will this be a good point that we start to think about common > code blocks for both Intel and AMD IOMMU? The core iommu code can still be refactored later on. I'm now more concerned about the hooks you add to generic code, see above. Jan
On Thu, Apr 28, 2016 at 10:36:19AM +0200, Jan Kiszka wrote: > On 2016-04-28 10:29, Peter Xu wrote: > > On Thu, Apr 28, 2016 at 09:26:01AM +0200, Jan Kiszka wrote: > >> On 2016-04-28 09:05, Peter Xu wrote: > >>> This patch introduces Intel VT-d IEC (Interrupt Entry Cache) > >>> invalidation notifier list. When vIOMMU receives IEC invalidate request, > >>> all the registered units will be notified with specific invalidation > >>> requests. > >> > >> This should be designed to be IOMMU-agnostic, i.e. become reusable for > >> the AMD implementation. I suspect we will have the same need for route > >> invalidations there as well... > > > > Yes possibly... > > > > I feel like there are lots of things that can be shared between > > Intel and AMD IOMMUs. I just do not know what is the most suitable > > "extent" that we should abstract these shared functionalities > > between the two, and how. > > A rough indicator: if you add something that has "vtd" in its name to > non-vtd code, think twice about some reusable abstraction ;). So, > something like "vtd_get_iommu" could already be named and designed to > have two provides (e.g. allow an IOMMU to register itself as provider, > return that registered instance(s) when requested). Yes, thanks for the hints. :) Before that, I was considering that the AMD guy who is going to add its support will better consider this and finally make sure the two coops well (anyway, I know nothing about AMD IOMMU before reading recent patches, and there is still no amd_iommu.c yet for me to read at..). But you are right, best to start consider it from the very beginning. > > > > > For example, AFAIU, a better solution for current IOMMU > > codes (including Intel and AMD) is to provide a common framework > > (like... X86IOMMU?), abstract these shared things out into a > > framework, like per device name spaces, iotlb, IEC notifications, > > etc... However, that will need a lot of further work. Also, I still > > do not know whether this is a good idea even in the future. > > > > So, will this be a good point that we start to think about common > > code blocks for both Intel and AMD IOMMU? > > The core iommu code can still be refactored later on. I'm now more > concerned about the hooks you add to generic code, see above. Will try to make them look better in v6. Hopefully there will have no vtd_*() in common codes. ;) Thanks! -- peterx
On Thu, Apr 28, 2016 at 11:49 AM, Peter Xu <peterx@redhat.com> wrote: > On Thu, Apr 28, 2016 at 10:36:19AM +0200, Jan Kiszka wrote: >> On 2016-04-28 10:29, Peter Xu wrote: >> > On Thu, Apr 28, 2016 at 09:26:01AM +0200, Jan Kiszka wrote: >> >> On 2016-04-28 09:05, Peter Xu wrote: >> >>> This patch introduces Intel VT-d IEC (Interrupt Entry Cache) >> >>> invalidation notifier list. When vIOMMU receives IEC invalidate request, >> >>> all the registered units will be notified with specific invalidation >> >>> requests. >> >> >> >> This should be designed to be IOMMU-agnostic, i.e. become reusable for >> >> the AMD implementation. I suspect we will have the same need for route >> >> invalidations there as well... >> > >> > Yes possibly... >> > >> > I feel like there are lots of things that can be shared between >> > Intel and AMD IOMMUs. I just do not know what is the most suitable >> > "extent" that we should abstract these shared functionalities >> > between the two, and how. >> >> A rough indicator: if you add something that has "vtd" in its name to >> non-vtd code, think twice about some reusable abstraction ;). So, >> something like "vtd_get_iommu" could already be named and designed to >> have two provides (e.g. allow an IOMMU to register itself as provider, >> return that registered instance(s) when requested). > > Yes, thanks for the hints. :) > > Before that, I was considering that the AMD guy who is going to add > its support will better consider this and finally make sure the two > coops well (anyway, I know nothing about AMD IOMMU before reading > recent patches, and there is still no amd_iommu.c yet for me to read > at..). But you are right, best to start consider it from the very > beginning. I think AMD IOMMU could be a benefit greatly from the Intel IOMMU cache implementation. There could be a few differences but I think much of the code could be reused. The thing is, AMD IOMMU spec doesn't mention anything to do with it's cache design except the available interface (for system programmers) and I have a bit of a hard time trying to design a cache from scratch. Trying to do this will however require me to dig too much into Intel IOMMU and I would prefer to reserve that for later, probably. > >> >> > >> > For example, AFAIU, a better solution for current IOMMU >> > codes (including Intel and AMD) is to provide a common framework >> > (like... X86IOMMU?), abstract these shared things out into a >> > framework, like per device name spaces, iotlb, IEC notifications, >> > etc... However, that will need a lot of further work. Also, I still >> > do not know whether this is a good idea even in the future. >> > >> > So, will this be a good point that we start to think about common >> > code blocks for both Intel and AMD IOMMU? >> >> The core iommu code can still be refactored later on. I'm now more >> concerned about the hooks you add to generic code, see above. > > Will try to make them look better in v6. Hopefully there will have > no vtd_*() in common codes. ;) > > Thanks! > > -- peterx
On Thu, Apr 28, 2016 at 06:56:05PM +0300, David Kiarie wrote: [...] > I think AMD IOMMU could be a benefit greatly from the Intel IOMMU > cache implementation. There could be a few differences but I think > much of the code could be reused. The thing is, AMD IOMMU spec doesn't > mention anything to do with it's cache design except the available > interface (for system programmers) and I have a bit of a hard time > trying to design a cache from scratch. Trying to do this will however > require me to dig too much into Intel IOMMU and I would prefer to > reserve that for later, probably. Sure. I will not touch that part in this series. My first step would be trying to remove vtd_* stuffs in common codes only, as Jan has suggested. Thanks, -- peterx
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index a8a57db..7122e5b 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -900,6 +900,22 @@ static void vtd_root_table_setup(IntelIOMMUState *s) (s->root_extended ? "(extended)" : "")); } +static void vtd_iec_notify_all(IntelIOMMUState *s, bool global, + uint32_t index, uint32_t mask) +{ + VTD_IEC_Notifier *notifier; + + VTD_DPRINTF(INV, "notify IEC invalidate: global=%d, index=%u, mask=%u", + global, index, mask); + + QLIST_FOREACH(notifier, &s->iec_notifiers, list) { + if (notifier->iec_notify) { + notifier->iec_notify(notifier->private, global, + index, mask); + } + } +} + static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s) { uint64_t value = 0; @@ -907,7 +923,8 @@ static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s) s->intr_size = 1UL << ((value & VTD_IRTA_SIZE_MASK) + 1); s->intr_root = value & VTD_IRTA_ADDR_MASK; - /* TODO: invalidate interrupt entry cache */ + /* Notify global invalidation */ + vtd_iec_notify_all(s, true, 0, 0); VTD_DPRINTF(CSR, "int remap table addr 0x%"PRIx64 " size %"PRIu32, s->intr_root, s->intr_size); @@ -1409,6 +1426,21 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc) return true; } +static bool vtd_process_inv_iec_desc(IntelIOMMUState *s, + VTDInvDesc *inv_desc) +{ + VTD_DPRINTF(INV, "inv ir glob %d index %d mask %d", + inv_desc->iec.granularity, + inv_desc->iec.index, + inv_desc->iec.index_mask); + + vtd_iec_notify_all(s, inv_desc->iec.granularity, + inv_desc->iec.index, + inv_desc->iec.index_mask); + + return true; +} + static bool vtd_process_inv_desc(IntelIOMMUState *s) { VTDInvDesc inv_desc; @@ -1449,12 +1481,12 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s) break; case VTD_INV_DESC_IEC: - VTD_DPRINTF(INV, "Interrupt Entry Cache Invalidation " - "not implemented yet"); - /* - * Since currently we do not cache interrupt entries, we can - * just mark this descriptor as "good" and move on. - */ + VTD_DPRINTF(INV, "Invalidation Interrupt Entry Cache " + "Descriptor hi 0x%"PRIx64 " lo 0x%"PRIx64, + inv_desc.hi, inv_desc.lo); + if (!vtd_process_inv_iec_desc(s, &inv_desc)) { + return false; + } break; default: @@ -2209,6 +2241,15 @@ static const MemoryRegionOps vtd_mem_ir_ops = { }, }; +void vtd_iec_register_notifier(IntelIOMMUState *s, vtd_iec_notify_fn fn, + void *data) +{ + VTD_IEC_Notifier *notifier = g_new0(VTD_IEC_Notifier, 1); + notifier->iec_notify = fn; + notifier->private = data; + QLIST_INSERT_HEAD(&s->iec_notifiers, notifier, list); +} + VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn) { uintptr_t key = (uintptr_t)bus; @@ -2371,6 +2412,7 @@ static void vtd_realize(DeviceState *dev, Error **errp) g_free, g_free); s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, g_free, g_free); + QLIST_INIT(&s->iec_notifiers); vtd_init(s); } diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h index e1a08cb..10c20fe 100644 --- a/hw/i386/intel_iommu_internal.h +++ b/hw/i386/intel_iommu_internal.h @@ -296,12 +296,28 @@ typedef enum VTDFaultReason { #define VTD_CONTEXT_CACHE_GEN_MAX 0xffffffffUL +/* Interrupt Entry Cache Invalidation Descriptor: VT-d 6.5.2.7. */ +struct VTDInvDescIEC { + uint32_t type:4; /* Should always be 0x4 */ + uint32_t granularity:1; /* If set, it's global IR invalidation */ + uint32_t resved_1:22; + uint32_t index_mask:5; /* 2^N for continuous int invalidation */ + uint32_t index:16; /* Start index to invalidate */ + uint32_t reserved_2:16; +}; +typedef struct VTDInvDescIEC VTDInvDescIEC; + /* Queued Invalidation Descriptor */ -struct VTDInvDesc { - uint64_t lo; - uint64_t hi; +union VTDInvDesc { + struct { + uint64_t lo; + uint64_t hi; + }; + union { + VTDInvDescIEC iec; + }; }; -typedef struct VTDInvDesc VTDInvDesc; +typedef union VTDInvDesc VTDInvDesc; /* Masks for struct VTDInvDesc */ #define VTD_INV_DESC_TYPE 0xf diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 5910e6f..4fe92cf 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -203,6 +203,24 @@ struct VTD_MSIMessage { /* When IR is enabled, all MSI/MSI-X data bits should be zero */ #define VTD_IR_MSI_DATA (0) +/** + * vtd_iec_notify_fn - IEC (Interrupt Entry Cache) notifier hook, + * triggered when IR invalidation happens. + * @private: private data + * @global: whether this is a global IEC invalidation + * @index: IRTE index to invalidate (start from) + * @mask: invalidation mask + */ +typedef void (*vtd_iec_notify_fn)(void *private, bool global, + uint32_t index, uint32_t mask); + +struct VTD_IEC_Notifier { + vtd_iec_notify_fn iec_notify; + void *private; + QLIST_ENTRY(VTD_IEC_Notifier) list; +}; +typedef struct VTD_IEC_Notifier VTD_IEC_Notifier; + /* The iommu (DMAR) device state struct */ struct IntelIOMMUState { SysBusDevice busdev; @@ -243,6 +261,7 @@ struct IntelIOMMUState { bool intr_enabled; /* Whether guest enabled IR */ dma_addr_t intr_root; /* Interrupt remapping table pointer */ uint32_t intr_size; /* Number of IR table entries */ + QLIST_HEAD(, VTD_IEC_Notifier) iec_notifiers; /* IEC notify list */ }; /* Find the VTD Address space associated with the given bus pointer, @@ -252,5 +271,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn); /* Get default IOMMU object */ IntelIOMMUState *vtd_iommu_get(void); int vtd_int_remap(void *iommu, MSIMessage *src, MSIMessage *dst); +/* Register IEC invalidate notifier */ +void vtd_iec_register_notifier(IntelIOMMUState *s, vtd_iec_notify_fn fn, + void *data); #endif
This patch introduces Intel VT-d IEC (Interrupt Entry Cache) invalidation notifier list. When vIOMMU receives IEC invalidate request, all the registered units will be notified with specific invalidation requests. Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/i386/intel_iommu.c | 56 ++++++++++++++++++++++++++++++++++++------ hw/i386/intel_iommu_internal.h | 24 +++++++++++++++--- include/hw/i386/intel_iommu.h | 22 +++++++++++++++++ 3 files changed, 91 insertions(+), 11 deletions(-)