Miroslav Rezanina
2009-Oct-16 17:39 UTC
[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: "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
Cui, Dexuan
2009-Oct-19 07:38 UTC
[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 -----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