| Message ID | 1575153570.3027165.1474966437735.JavaMail.zimbra@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 27, 2016 at 04:53:57AM -0400, Paolo Bonzini wrote: > > > ----- Original Message ----- > > From: "Peter Xu" <peterx@redhat.com> > > To: "Peter Maydell" <peter.maydell@linaro.org>, "Paolo Bonzini" <pbonzini@redhat.com> > > Cc: "QEMU Developers" <qemu-devel@nongnu.org> > > Sent: Tuesday, September 27, 2016 4:12:51 AM > > Subject: Re: [Qemu-devel] [PULL 00/28] Misc patches for 2016-09-26 > > > > On Mon, Sep 26, 2016 at 02:19:08PM -0700, Peter Maydell wrote: > > > > [...] > > > > > I also see this compile failure: > > > > > > CC i386-softmmu/hw/i386/amd_iommu.o > > > /home/petmay01/linaro/qemu-for-merges/hw/i386/amd_iommu.c: In function > > > ‘amdvi_init’: > > > /home/petmay01/linaro/qemu-for-merges/hw/i386/amd_iommu.c:1083:17: > > > error: ‘MemoryRegionIOMMUOps {aka struct MemoryRegionIOMMUOps}’ has no > > > member named ‘notify_started’ > > > s->iommu_ops.notify_started = amdvi_iommu_notify_started; > > > ^ > > > /home/petmay01/linaro/qemu-for-merges/rules.mak:60: recipe for target > > > 'hw/i386/amd_iommu.o' failed > > > > Paolo, > > > > Would you please help squash this into 02/28 of your PULL request to > > solve above error? > > Shall I also redo patch 3/3 for AMD IOMMU, like this: > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c > index a868539..6365682 100644 > --- a/hw/i386/amd_iommu.c > +++ b/hw/i386/amd_iommu.c > @@ -1072,9 +1072,12 @@ static void amdvi_iommu_notify_flag_changed(MemoryRegion *iommu, > { > AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu); > > - hw_error("device %02x.%02x.%x requires iommu notifier which is not " > - "currently supported", as->bus_num, PCI_SLOT(as->devfn), > - PCI_FUNC(as->devfn)); > + if (new & IOMMU_NOTIFIER_MAP) { > + error_report("device %02x.%02x.%x requires iommu notifier which is not " > + "currently supported", as->bus_num, PCI_SLOT(as->devfn), > + PCI_FUNC(as->devfn)); > + exit(1); > + } > } > > static void amdvi_init(AMDVIState *s) > > ? I think we should keep it as it is, because Jason's patchset will only support intel-iommu, not amd-iommu. For now, it won't have problem (just like Intel IOMMU one). But after Jason's patch is merged, people will be able to boot a guest with vhost and amd-iommu (which we actually do not support yet), and that might be problematic. Thanks, -- peterx
> I think we should keep it as it is, because Jason's patchset will only > support intel-iommu, not amd-iommu. For now, it won't have problem > (just like Intel IOMMU one). But after Jason's patch is merged, people > will be able to boot a guest with vhost and amd-iommu (which we > actually do not support yet), and that might be problematic. Let's fix Jason's patch instead. :) Paolo
On Tue, Sep 27, 2016 at 06:11:59AM -0400, Paolo Bonzini wrote: > > > I think we should keep it as it is, because Jason's patchset will only > > support intel-iommu, not amd-iommu. For now, it won't have problem > > (just like Intel IOMMU one). But after Jason's patch is merged, people > > will be able to boot a guest with vhost and amd-iommu (which we > > actually do not support yet), and that might be problematic. > > Let's fix Jason's patch instead. :) Then I think Jason will need to add vhost DMAR support for AMD as well, which I believe is not in his current plan. :) Anyway, I think we can do it either way. I can hardly believe if someone will like to run vhost with AMD IOMMU. So IMHO it's not a big thing for all cases. Thanks, -- peterx
On 2016年09月28日 11:12, Peter Xu wrote: > On Tue, Sep 27, 2016 at 06:11:59AM -0400, Paolo Bonzini wrote: >>> I think we should keep it as it is, because Jason's patchset will only >>> support intel-iommu, not amd-iommu. For now, it won't have problem >>> (just like Intel IOMMU one). But after Jason's patch is merged, people >>> will be able to boot a guest with vhost and amd-iommu (which we >>> actually do not support yet), and that might be problematic. >> Let's fix Jason's patch instead. :) > Then I think Jason will need to add vhost DMAR support for AMD as > well, which I believe is not in his current plan. :) Will try to add AMD IOMMU support in next version. Thanks > > Anyway, I think we can do it either way. I can hardly believe if > someone will like to run vhost with AMD IOMMU. So IMHO it's not a big > thing for all cases. > > Thanks, > > -- peterx >
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c index a868539..6365682 100644 --- a/hw/i386/amd_iommu.c +++ b/hw/i386/amd_iommu.c @@ -1072,9 +1072,12 @@ static void amdvi_iommu_notify_flag_changed(MemoryRegion *iommu, { AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu); - hw_error("device %02x.%02x.%x requires iommu notifier which is not " - "currently supported", as->bus_num, PCI_SLOT(as->devfn), - PCI_FUNC(as->devfn)); + if (new & IOMMU_NOTIFIER_MAP) { + error_report("device %02x.%02x.%x requires iommu notifier which is not " + "currently supported", as->bus_num, PCI_SLOT(as->devfn), + PCI_FUNC(as->devfn)); + exit(1); + } } static void amdvi_init(AMDVIState *s)