Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
> From: Paolo Bonzini [mailto:pbonz...@redhat.com] > Sent: Tuesday, March 6, 2018 7:22 PM > Subject: Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce > pci_device_notify_iommu() > > On 06/03/2018 12:03, Liu, Yi L wrote: > > On Tue, Mar 06, 2018 at 11:18:43AM +0100, Paolo Bonzini wrote: > >> On 05/03/2018 09:42, Liu, Yi L wrote: > >>>> In general I think it's better to change your names from "assigned_dev" > >>>> to "sva_dev", because the point of the list is to only iterate over > >>>> devices that might be interested in using SVA. > >>> > >>> For "assigned_dev", my purpose is to distinguish "assigned devices" > >>> from emulated devices. Only the SVA usage on "assigned devices" is cared > >>> here. > >>> But it is true only SVA capable device is interested. So I may need > >>> to rename it as "assigned_sva_dev". How about your opinion? > >> > >> What you care about is not whether the device assigned, but rather > >> whether it called or not pci_setup_sva_ops. Currently only VFIO does > >> this, but that's not a requirement. Hence my suggestion of calling > >> it sva_dev. > > > > Yes, only VFIO calls pci_setup_sva_ops so far, but it should not limited to. > > I'll apply in next version. > > For what it's worth, I agree with David's suggestion for naming (so > pci_setup_pasid_ops, pasid_dev, etc.) Thanks, Paolo. I would follow suggestions from you two. Regards, Yi Liu
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On Tue, Mar 06, 2018 at 06:47:27PM +0800, Peter Xu wrote: > On Tue, Mar 06, 2018 at 11:19:13AM +0100, Paolo Bonzini wrote: > > On 05/03/2018 11:43, Peter Xu wrote: > > > On Mon, Mar 05, 2018 at 04:43:09PM +0800, Liu, Yi L wrote: > > >> On Fri, Mar 02, 2018 at 05:06:56PM +0100, Paolo Bonzini wrote: > > >>> On 01/03/2018 11:33, Liu, Yi L wrote: > > +pci_device_notify_iommu(pdev, PCI_NTY_DEV_ADD); > > + > > pci_setup_sva_ops(pdev, _pci_sva_ops); > > > > return; > > @@ -3134,6 +3136,7 @@ static void vfio_exitfn(PCIDevice *pdev) > > { > > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > > > > +pci_device_notify_iommu(pdev, PCI_NTY_DEV_DEL); > > >>> > > >>> Please make the names longer: PCI_IOMMU_NOTIFY_DEVICE_ADDED and > > >>> PCI_IOMMU_NOTIFY_DEVICE_REMOVED. (This is independent of my other > > >>> remark, about doing this in generic PCI code for all devices that > > >>> register SVA ops). > > >> > > >> Thanks for the suggestion, will appply. > > > > > > Isn't the name too generic if it's tailored for VFIO only? Would > > > something like PCI_IOMMU_NOTIFY_VFIO_ADD be a bit better? > > > > I don't think it's for VFIO only. It's just that VFIO is the only > > caller of pci_setup_sva_ops. > > Indeed. E.g., we can have emulated devices that also want to provide > the SVA ops. Excatly as Paolo commented. ^_^ Thanks, Yi Liu
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On 06/03/2018 12:03, Liu, Yi L wrote: > On Tue, Mar 06, 2018 at 11:18:43AM +0100, Paolo Bonzini wrote: >> On 05/03/2018 09:42, Liu, Yi L wrote: In general I think it's better to change your names from "assigned_dev" to "sva_dev", because the point of the list is to only iterate over devices that might be interested in using SVA. >>> >>> For "assigned_dev", my purpose is to distinguish "assigned devices" from >>> emulated devices. Only the SVA usage on "assigned devices" is cared here. >>> But it is true only SVA capable device is interested. So I may need to >>> rename it as "assigned_sva_dev". How about your opinion? >> >> What you care about is not whether the device assigned, but rather >> whether it called or not pci_setup_sva_ops. Currently only VFIO does >> this, but that's not a requirement. Hence my suggestion of calling it >> sva_dev. > > Yes, only VFIO calls pci_setup_sva_ops so far, but it should not limited to. > I'll apply in next version. For what it's worth, I agree with David's suggestion for naming (so pci_setup_pasid_ops, pasid_dev, etc.) Paolo
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On Tue, Mar 06, 2018 at 11:18:43AM +0100, Paolo Bonzini wrote: > On 05/03/2018 09:42, Liu, Yi L wrote: > >> In general I think it's better to change your names from "assigned_dev" > >> to "sva_dev", because the point of the list is to only iterate over > >> devices that might be interested in using SVA. > > > > For "assigned_dev", my purpose is to distinguish "assigned devices" from > > emulated devices. Only the SVA usage on "assigned devices" is cared here. > > But it is true only SVA capable device is interested. So I may need to > > rename it as "assigned_sva_dev". How about your opinion? > > What you care about is not whether the device assigned, but rather > whether it called or not pci_setup_sva_ops. Currently only VFIO does > this, but that's not a requirement. Hence my suggestion of calling it > sva_dev. Yes, only VFIO calls pci_setup_sva_ops so far, but it should not limited to. I'll apply in next version. Thanks, Yi Liu
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On Tue, Mar 06, 2018 at 11:19:13AM +0100, Paolo Bonzini wrote: > On 05/03/2018 11:43, Peter Xu wrote: > > On Mon, Mar 05, 2018 at 04:43:09PM +0800, Liu, Yi L wrote: > >> On Fri, Mar 02, 2018 at 05:06:56PM +0100, Paolo Bonzini wrote: > >>> On 01/03/2018 11:33, Liu, Yi L wrote: > +pci_device_notify_iommu(pdev, PCI_NTY_DEV_ADD); > + > pci_setup_sva_ops(pdev, _pci_sva_ops); > > return; > @@ -3134,6 +3136,7 @@ static void vfio_exitfn(PCIDevice *pdev) > { > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > > +pci_device_notify_iommu(pdev, PCI_NTY_DEV_DEL); > >>> > >>> Please make the names longer: PCI_IOMMU_NOTIFY_DEVICE_ADDED and > >>> PCI_IOMMU_NOTIFY_DEVICE_REMOVED. (This is independent of my other > >>> remark, about doing this in generic PCI code for all devices that > >>> register SVA ops). > >> > >> Thanks for the suggestion, will appply. > > > > Isn't the name too generic if it's tailored for VFIO only? Would > > something like PCI_IOMMU_NOTIFY_VFIO_ADD be a bit better? > > I don't think it's for VFIO only. It's just that VFIO is the only > caller of pci_setup_sva_ops. Indeed. E.g., we can have emulated devices that also want to provide the SVA ops. -- Peter Xu
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On 05/03/2018 11:43, Peter Xu wrote: > On Mon, Mar 05, 2018 at 04:43:09PM +0800, Liu, Yi L wrote: >> On Fri, Mar 02, 2018 at 05:06:56PM +0100, Paolo Bonzini wrote: >>> On 01/03/2018 11:33, Liu, Yi L wrote: +pci_device_notify_iommu(pdev, PCI_NTY_DEV_ADD); + pci_setup_sva_ops(pdev, _pci_sva_ops); return; @@ -3134,6 +3136,7 @@ static void vfio_exitfn(PCIDevice *pdev) { VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); +pci_device_notify_iommu(pdev, PCI_NTY_DEV_DEL); >>> >>> Please make the names longer: PCI_IOMMU_NOTIFY_DEVICE_ADDED and >>> PCI_IOMMU_NOTIFY_DEVICE_REMOVED. (This is independent of my other >>> remark, about doing this in generic PCI code for all devices that >>> register SVA ops). >> >> Thanks for the suggestion, will appply. > > Isn't the name too generic if it's tailored for VFIO only? Would > something like PCI_IOMMU_NOTIFY_VFIO_ADD be a bit better? I don't think it's for VFIO only. It's just that VFIO is the only caller of pci_setup_sva_ops. Paolo
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On 05/03/2018 09:42, Liu, Yi L wrote: >> In general I think it's better to change your names from "assigned_dev" >> to "sva_dev", because the point of the list is to only iterate over >> devices that might be interested in using SVA. > > For "assigned_dev", my purpose is to distinguish "assigned devices" from > emulated devices. Only the SVA usage on "assigned devices" is cared here. > But it is true only SVA capable device is interested. So I may need to > rename it as "assigned_sva_dev". How about your opinion? What you care about is not whether the device assigned, but rather whether it called or not pci_setup_sva_ops. Currently only VFIO does this, but that's not a requirement. Hence my suggestion of calling it sva_dev. Paolo
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On Mon, Mar 05, 2018 at 04:43:09PM +0800, Liu, Yi L wrote: > On Fri, Mar 02, 2018 at 05:06:56PM +0100, Paolo Bonzini wrote: > > On 01/03/2018 11:33, Liu, Yi L wrote: > > > +pci_device_notify_iommu(pdev, PCI_NTY_DEV_ADD); > > > + > > > pci_setup_sva_ops(pdev, _pci_sva_ops); > > > > > > return; > > > @@ -3134,6 +3136,7 @@ static void vfio_exitfn(PCIDevice *pdev) > > > { > > > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > > > > > > +pci_device_notify_iommu(pdev, PCI_NTY_DEV_DEL); > > > > Please make the names longer: PCI_IOMMU_NOTIFY_DEVICE_ADDED and > > PCI_IOMMU_NOTIFY_DEVICE_REMOVED. (This is independent of my other > > remark, about doing this in generic PCI code for all devices that > > register SVA ops). > > Thanks for the suggestion, will appply. Isn't the name too generic if it's tailored for VFIO only? Would something like PCI_IOMMU_NOTIFY_VFIO_ADD be a bit better? -- Peter Xu
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On Mon, Mar 05, 2018 at 04:27:43PM +0800, Peter Xu wrote: > On Thu, Mar 01, 2018 at 06:33:31PM +0800, Liu, Yi L wrote: > > [...] > > > -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque) > > +void pci_device_notify_iommu(PCIDevice *dev, PCIDevNotifyType type) > > { > > -bus->iommu_fn = fn; > > +PCIBus *bus = PCI_BUS(pci_get_bus(dev)); > > +PCIBus *iommu_bus = bus; > > + > > +while (iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) { > > +iommu_bus = PCI_BUS(pci_get_bus(iommu_bus->parent_dev)); > > +} > > +if (iommu_bus && iommu_bus->notify_fn) { > > +iommu_bus->notify_fn(bus, > > + iommu_bus->iommu_opaque, > > + dev->devfn, > > + type); > > We didn't really check the return code for notify function. What if > it failed? If we care, we'd better handle the failure; or we can just > define the notify_fn() to return void (now it's int). Good catch. I think we need to handle failure. User should be aware of it. I'll try to add accordingly in next version. > > +} > > +return; > > I saw many places in the series that you added explicit return for > "void" return-typed functions. IMHO all of them can be dropped. Thanks for spotting it, would fix them in next version. Regards, Yi Liu
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On Fri, Mar 02, 2018 at 05:06:56PM +0100, Paolo Bonzini wrote: > On 01/03/2018 11:33, Liu, Yi L wrote: > > +pci_device_notify_iommu(pdev, PCI_NTY_DEV_ADD); > > + > > pci_setup_sva_ops(pdev, _pci_sva_ops); > > > > return; > > @@ -3134,6 +3136,7 @@ static void vfio_exitfn(PCIDevice *pdev) > > { > > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > > > > +pci_device_notify_iommu(pdev, PCI_NTY_DEV_DEL); > > Please make the names longer: PCI_IOMMU_NOTIFY_DEVICE_ADDED and > PCI_IOMMU_NOTIFY_DEVICE_REMOVED. (This is independent of my other > remark, about doing this in generic PCI code for all devices that > register SVA ops). Thanks for the suggestion, will appply. Regards, Yi Liu
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On Fri, Mar 02, 2018 at 04:12:01PM +0100, Paolo Bonzini wrote: > On 01/03/2018 11:33, Liu, Yi L wrote: > > This patch adds pci_device_notify_iommu() for notify virtual IOMMU > > emulator when assigned device is added. And adds a new notify_func > > in PCIBus. vIOMMU emulator provides the instance of this notify_func. > > > > Reason: > > When virtual IOMMU is exposed to guest, vIOMMU emulator needs to > > programm host IOMMU to setup DMA mapping for assigned devices. This > > is a per-device operation, to be efficient, vIOMMU emulator needs > > to record the assigned devices. > > > > Example: devices assigned thru vfio, vfio_realize would call > > pci_device_notify_iommu() to notify vIOMMU emulator to record necessary > > info for assigned device. > > I think the notification should not be left to the individual device. > Instead, the add notification should be done in pci_setup_sva_ops, and > the delete notification should be done in pci_qdev_unrealize, before > calling pc->exit, and only if dev->sva_ops is not NULL. Agreed. I think it works together with your comments against "[PATCH v3 05/12] hw/pci: introduce PCISVAOps to PCIDevice". Would apply it in next version. > In general I think it's better to change your names from "assigned_dev" > to "sva_dev", because the point of the list is to only iterate over > devices that might be interested in using SVA. For "assigned_dev", my purpose is to distinguish "assigned devices" from emulated devices. Only the SVA usage on "assigned devices" is cared here. But it is true only SVA capable device is interested. So I may need to rename it as "assigned_sva_dev". How about your opinion? Thanks, Yi Liu
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On Thu, Mar 01, 2018 at 06:33:31PM +0800, Liu, Yi L wrote: [...] > -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque) > +void pci_device_notify_iommu(PCIDevice *dev, PCIDevNotifyType type) > { > -bus->iommu_fn = fn; > +PCIBus *bus = PCI_BUS(pci_get_bus(dev)); > +PCIBus *iommu_bus = bus; > + > +while (iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) { > +iommu_bus = PCI_BUS(pci_get_bus(iommu_bus->parent_dev)); > +} > +if (iommu_bus && iommu_bus->notify_fn) { > +iommu_bus->notify_fn(bus, > + iommu_bus->iommu_opaque, > + dev->devfn, > + type); We didn't really check the return code for notify function. What if it failed? If we care, we'd better handle the failure; or we can just define the notify_fn() to return void (now it's int). > +} > +return; I saw many places in the series that you added explicit return for "void" return-typed functions. IMHO all of them can be dropped. > +} Thanks, -- Peter Xu
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On 01/03/2018 11:33, Liu, Yi L wrote: > +pci_device_notify_iommu(pdev, PCI_NTY_DEV_ADD); > + > pci_setup_sva_ops(pdev, _pci_sva_ops); > > return; > @@ -3134,6 +3136,7 @@ static void vfio_exitfn(PCIDevice *pdev) > { > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > > +pci_device_notify_iommu(pdev, PCI_NTY_DEV_DEL); Please make the names longer: PCI_IOMMU_NOTIFY_DEVICE_ADDED and PCI_IOMMU_NOTIFY_DEVICE_REMOVED. (This is independent of my other remark, about doing this in generic PCI code for all devices that register SVA ops). Thanks, Paolo
Re: [Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu()
On 01/03/2018 11:33, Liu, Yi L wrote: > This patch adds pci_device_notify_iommu() for notify virtual IOMMU > emulator when assigned device is added. And adds a new notify_func > in PCIBus. vIOMMU emulator provides the instance of this notify_func. > > Reason: > When virtual IOMMU is exposed to guest, vIOMMU emulator needs to > programm host IOMMU to setup DMA mapping for assigned devices. This > is a per-device operation, to be efficient, vIOMMU emulator needs > to record the assigned devices. > > Example: devices assigned thru vfio, vfio_realize would call > pci_device_notify_iommu() to notify vIOMMU emulator to record necessary > info for assigned device. I think the notification should not be left to the individual device. Instead, the add notification should be done in pci_setup_sva_ops, and the delete notification should be done in pci_qdev_unrealize, before calling pc->exit, and only if dev->sva_ops is not NULL. In general I think it's better to change your names from "assigned_dev" to "sva_dev", because the point of the list is to only iterate over devices that might be interested in using SVA. Paolo