Hi Peter,
On 05/17/2018 12:02 PM, Peter Xu wrote:
> On Thu, May 17, 2018 at 11:46:22AM +0200, Auger Eric wrote:
>> Hi Peter,
>>
>> On 05/04/2018 05:08 AM, Peter Xu wrote:
>>> That is not really necessary. Removing that node struct and put the
>>> list entry directly into VTDAddressSpace. It simplfies the code a lot.
>>>
>>> Signed-off-by: Peter Xu <pet...@redhat.com>
>>> ---
>>> include/hw/i386/intel_iommu.h | 9 ++------
>>> hw/i386/intel_iommu.c | 41 ++++++++++-------------------------
>>> 2 files changed, 14 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
>>> index 45ec8919b6..220697253f 100644
>>> --- a/include/hw/i386/intel_iommu.h
>>> +++ b/include/hw/i386/intel_iommu.h
>>> @@ -67,7 +67,6 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
>>> typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
>>> typedef struct VTDIrq VTDIrq;
>>> typedef struct VTD_MSIMessage VTD_MSIMessage;
>>> -typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
>>>
>>> /* Context-Entry */
>>> struct VTDContextEntry {
>>> @@ -93,6 +92,7 @@ struct VTDAddressSpace {
>>> MemoryRegion iommu_ir; /* Interrupt region: 0xfeeXXXXX */
>>> IntelIOMMUState *iommu_state;
>>> VTDContextCacheEntry context_cache_entry;
>>> + QLIST_ENTRY(VTDAddressSpace) next;
>>> };
>>>
>>> struct VTDBus {
>>> @@ -253,11 +253,6 @@ struct VTD_MSIMessage {
>>> /* When IR is enabled, all MSI/MSI-X data bits should be zero */
>>> #define VTD_IR_MSI_DATA (0)
>>>
>>> -struct IntelIOMMUNotifierNode {
>>> - VTDAddressSpace *vtd_as;
>>> - QLIST_ENTRY(IntelIOMMUNotifierNode) next;
>>> -};
>>> -
>>> /* The iommu (DMAR) device state struct */
>>> struct IntelIOMMUState {
>>> X86IOMMUState x86_iommu;
>>> @@ -295,7 +290,7 @@ struct IntelIOMMUState {
>>> GHashTable *vtd_as_by_busptr; /* VTDBus objects indexed by PCIBus*
>>> reference */
>>> VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed
>>> by bus number */
>>> /* list of registered notifiers */
>>> - QLIST_HEAD(, IntelIOMMUNotifierNode) notifiers_list;
>>> + QLIST_HEAD(, VTDAddressSpace) notifiers_list;
>> Wouldn't it make sense to rename notifiers_list into something more
>> understandable like address_spaces?
>
> But address_spaces might be a bit confusing too on the other side as
> "a list of all VT-d address spaces". How about something like:
>
> address_spaces_with_notifiers
Hum I missed not all of them belonged to that list. a bit long now?
vtd_as_with_notifiers?
Thanks
Eric
>
> ?
>
> [...]
>
>>> - /* update notifier node with new flags */
>>> - QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
>>> - if (node->vtd_as == vtd_as) {
>>> - if (new == IOMMU_NOTIFIER_NONE) {
>>> - QLIST_REMOVE(node, next);
>>> - g_free(node);
>>> - }
>>> - return;
>>> - }
>>> + /* Insert new ones */
>> s/ones/one
>>
>>> + QLIST_INSERT_HEAD(&s->notifiers_list, vtd_as, next);
>>> + } else if (new == IOMMU_NOTIFIER_NONE) {
>>> + /* Remove old ones */
>> same. Not sure the comments are worth.
>
> Will remove two "s"s there. Thanks,
>
