Re: [Qemu-devel] [PULL 00/28] Misc patches for 2016-09-26

[Peter Xu]

On Tue, Sep 27, 2016 at 04:53:57AM -0400, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
> > From: "Peter Xu" <address@hidden>
> > To: "Peter Maydell" <address@hidden>, "Paolo Bonzini" <address@hidden>
> > Cc: "QEMU Developers" <address@hidden>
> > 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