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