From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50212) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c40If-0004No-LT for qemu-devel@nongnu.org; Tue, 08 Nov 2016 01:54:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c40Ic-0001eP-6v for qemu-devel@nongnu.org; Tue, 08 Nov 2016 01:54:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33122) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c40Ib-0001eF-VY for qemu-devel@nongnu.org; Tue, 08 Nov 2016 01:54:26 -0500 References: <1478502595-8484-1-git-send-email-jasowang@redhat.com> <1478502595-8484-6-git-send-email-jasowang@redhat.com> <20161107233549.GA2793@pxdev.xzpeter.org> From: Jason Wang Message-ID: Date: Tue, 8 Nov 2016 14:54:19 +0800 MIME-Version: 1.0 In-Reply-To: <20161107233549.GA2793@pxdev.xzpeter.org> Content-Type: text/plain; charset=utf-8; format=flowed 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: Peter Xu Cc: Eduardo Habkost , mst@redhat.com, qemu-devel@nongnu.org, vkaplans@redhat.com, wexu@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, Richard Henderson 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: > > [...] > >> +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 =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? > > 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=20 on top to fix this. > >> +static bool x86_iommu_device_iotlb_prop_get(Object *o, Error **errp) >> +{ >> + X86IOMMUState *s =3D X86_IOMMU_DEVICE(o); >> + return s->dt_supported; >> +} >> + >> +static void x86_iommu_device_iotlb_prop_set(Object *o, bool value, Er= ror **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_prop_= 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": > > static Property vtd_properties[] =3D { > 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,=20 its device-iotlb support is buggy). Thanks