From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43532) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c4D3Z-0001an-Ht for qemu-devel@nongnu.org; Tue, 08 Nov 2016 15:31:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c4D3W-00085l-7X for qemu-devel@nongnu.org; Tue, 08 Nov 2016 15:31:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57850) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c4D3W-00085M-0I for qemu-devel@nongnu.org; Tue, 08 Nov 2016 15:31:42 -0500 Date: Tue, 8 Nov 2016 22:31:38 +0200 From: "Michael S. Tsirkin" Message-ID: <20161108203138.qjftoznapxipqotz@redhat.com> References: <1478502595-8484-1-git-send-email-jasowang@redhat.com> <1478502595-8484-6-git-send-email-jasowang@redhat.com> <20161107233549.GA2793@pxdev.xzpeter.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V3 05/10] intel_iommu: support device iotlb descriptor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: Peter Xu , Eduardo Habkost , qemu-devel@nongnu.org, vkaplans@redhat.com, wexu@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, Richard Henderson On Tue, Nov 08, 2016 at 02:54:19PM +0800, Jason Wang wrote: >=20 >=20 > On 2016=E5=B9=B411=E6=9C=8808=E6=97=A5 07:35, Peter Xu wrote: > > On Mon, Nov 07, 2016 at 03:09:50PM +0800, Jason Wang wrote: > >=20 > > [...] > >=20 > > > +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... > >=20 > > [...] > >=20 > > > + entry.target_as =3D &vtd_dev_as->as; > > > + entry.addr_mask =3D sz - 1; > > > + entry.iova =3D addr; > > > + memory_region_notify_iommu(entry.target_as->root, entry); > > ... here we need to assign entry.perm explicitly to IOMMU_NONE, right= ? > >=20 > > 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. > >=20 > > [...] >=20 > 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. > >=20 > > > +static bool x86_iommu_device_iotlb_prop_get(Object *o, Error **err= p) > > > +{ > > > + X86IOMMUState *s =3D X86_IOMMU_DEVICE(o); > > > + return s->dt_supported; > > > +} > > > + > > > +static void x86_iommu_device_iotlb_prop_set(Object *o, bool value,= Error **errp) > > > +{ > > > + X86IOMMUState *s =3D X86_IOMMU_DEVICE(o); > > > + s->dt_supported =3D value; > > > +} > > > + > > > static void x86_iommu_instance_init(Object *o) > > > { > > > X86IOMMUState *s =3D X86_IOMMU_DEVICE(o); > > > @@ -114,6 +126,11 @@ static void x86_iommu_instance_init(Object *o) > > > s->intr_supported =3D false; > > > object_property_add_bool(o, "intremap", x86_iommu_intremap_pr= op_get, > > > x86_iommu_intremap_prop_set, NULL); > > > + s->dt_supported =3D 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": > >=20 > > static Property vtd_properties[] =3D { > > DEFINE_PROP_UINT32("device-iotlb", X86IOMMUState, dt_support= ed, false), > > DEFINE_PROP_END_OF_LIST(), > > }; > >=20 > > However not worth a repost. > >=20 > > Thanks, > >=20 > > -- peterx > >=20 >=20 > We may want to share this with AMD IOMMU. (Looking at AMD IOMMU codes, = its > device-iotlb support is buggy). >=20 > Thanks