Miroslav Rezanina
2009-Oct-19 08:34 UTC
Re: [Xen-devel] RE: [Xen-changelog] [xen-unstable]vt-d: Fixpanic in msi_msg_read_remap_rte with acpi=off
----- "Dexuan Cui" <dexuan.cui@intel.com> wrote:> From: "Dexuan Cui" <dexuan.cui@intel.com> > To: "Miroslav Rezanina" <mrezanin@redhat.com> > Cc: xen-devel@lists.xensource.com, "Keir Fraser" <keir.fraser@eu.citrix.com> > Sent: Monday, October 19, 2009 9:38:12 AM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna > Subject: [Xen-devel] RE: [Xen-changelog] [xen-unstable]vt-d: Fixpanic in msi_msg_read_remap_rte with acpi=off > > Hi Miroslav, > I agree we should program defensively. > However, here when acpi=off and iommu=1, or when iommu=0, with my "if > ( acpi_disabled ) iommu_enabled = 0" checking, I believe xen''s > execution can''t reach the places you pointed out. > And when (by default acpi=on and ) iommu=1, normally the places can''t > return NULL either, otherwise the BIOS is buggy or Xen has a bug; in > these cases, we can''t simply ignore it silently -- I think we should > at least print a warning message. > So how about my attached new patch? > > Thanks, > -- Dexuan >Hi Dexuan, you''re right. We should print warning. In your patch, I do not understand why you put comment only in setup_dom0_devices function. There is more calling of domain_context_mapping and we check NULL also in domain_context_unmap and reassign_device_ownership. We should put warning in there too, shouldn''t we? Regards, Mirek> > > -----Original Message----- > From: Miroslav Rezanina [mailto:mrezanin@redhat.com] > Sent: 2009年10月17日 1:40 > To: Cui, Dexuan > Cc: Keir Fraser; xen-devel@lists.xensource.com > Subject: RE: [Xen-changelog] [xen-unstable]vt-d: Fixpanic in > msi_msg_read_remap_rte with acpi=off > > ---- "Dexuan Cui" <dexuan.cui@intel.com> wrote: > > > From: "Dexuan Cui" <dexuan.cui@intel.com> > > To: "Miroslav Rezanina" <mrezanin@redhat.com> > > Cc: "Keir Fraser" <keir.fraser@eu.citrix.com>, > xen-devel@lists.xensource.com > > Sent: Friday, October 16, 2009 4:16:55 PM GMT +01:00 Amsterdam / > Berlin / Bern / Rome / Stockholm / Vienna > > Subject: RE: [Xen-changelog] [xen-unstable]vt-d: Fixpanic in > msi_msg_read_remap_rte with acpi=off > > > > Hi Miroslav and Keir, > > When acpi=off and we set iommu_enabled=0, the execution of xen > can''t > > reach the acpi_find_matched_drhd_unit Miroslav explicitly points > out > > one by one. > > I think I can give a cleaner fix. Please see the attached patch. > > > > Thanks, > > -- Dexuan > > > Hi Dexuan, > acpi_find_matched_drhd_unit can theoretically return NULL. Is one > check so big overhead to risk > segmentation fault? > > > > diff -r 0705efd9c69e xen/drivers/passthrough/iommu.c > > --- a/xen/drivers/passthrough/iommu.c Fri Oct 16 09:04:53 2009 > +0100 > > +++ b/xen/drivers/passthrough/iommu.c Fri Oct 16 18:41:46 2009 > +0800 > > @@ -266,9 +266,13 @@ int iommu_setup(void) > > { > > int rc = -ENODEV; > > > > - rc = iommu_hardware_setup(); > > - > > - iommu_enabled = (rc == 0); > > + if ( acpi_disabled ) > > + iommu_enabled = 0; > > + else > > + { > > + rc = iommu_hardware_setup(); > > + iommu_enabled = (rc == 0); > > + } > > > > if ( force_iommu && !iommu_enabled ) > > panic("IOMMU setup failed, crash Xen for security > purpose!\n"); > > I miss this part, so this one should be add. However, I would leave > checks when calling acpi_find_matched_drhd_unit. > -- > Miroslav Rezanina > Software Engineer - Virtualization Team - XEN kernel > > -- > Miroslav Rezanina > Software Engineer - Virtualization Team - XEN kernel > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel-- Miroslav Rezanina Software Engineer - Virtualization Team - XEN kernel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Cui, Dexuan
2009-Oct-19 08:49 UTC
RE: [Xen-devel] RE: [Xen-changelog] [xen-unstable]vt-d: Fixpanic in msi_msg_read_remap_rte with acpi=off
Miroslav Rezanina wrote:> Hi Dexuan, > you're right. We should print warning. In your patch, I do not > understand > why you put comment only in setup_dom0_devices function. There is more > calling of domain_context_mapping and we check NULL also inIn other places, the retuen value of domain_context_mapping() has been checked properly, e.g., passing to the caller, so we wouldn't ignore the failure. :-)> domain_context_unmap and reassign_device_ownership. We should put > warning in there too, shouldn't we?domain_context_unmap() is invoked in 2 places: 1) in intel_iommu_remove_device(), the return value has been propagated properly; 2) in reassign_device_ownership(), invoking reassign_device_ownership implies the device has been successfully assigned and the domain_context_mapping() returned success, so here the domain_context_unmap() can't fail. The other returing place in reassign_device_ownership() has been propagated properly to the caller. Thanks, -- Dexuan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Oct-19 09:22 UTC
Re: [Xen-devel] RE: [Xen-changelog] [xen-unstable]vt-d: Fixpanic in msi_msg_read_remap_rte with acpi=off
On 19/10/2009 09:34, "Miroslav Rezanina" <mrezanin@redhat.com> wrote:>> Thanks, >> -- Dexuan >> > > Hi Dexuan, > you''re right. We should print warning. In your patch, I do not understand > why you put comment only in setup_dom0_devices function. There is more > calling of domain_context_mapping and we check NULL also in > domain_context_unmap > and reassign_device_ownership. We should put warning in there too, shouldn''t > we?The warnings are silly, if we believe find_matched_drhd_unit() should not return NULL in those cases. Since obviously we wouldn''t know what to do in that case: bailing and doing nothing, while convenient and requiring little thought to implement, probably causes other subtler problems later on since those remap functions are supposed to actually do something! Crashing immediately is the nice thing to do here: nice for the poor developer who may have to debug this case sometime in the future, in the hopefully unlikely event our belief turns out to be false. I''ll be applying Dexuan''s original replacement patch. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel