[Xen-devel] RE: [Xen-changelog] [xen-unstable]vt-d: Fixpanic in msi_msg_read_remap_rte with acpi=off

["Cui, Dexuan" <dexuan.cui@xxxxxxxxx>]

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@xxxxxxxxxx] 
Sent: 2009å10æ17æ 1:40
To: Cui, Dexuan
Cc: Keir Fraser; xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: RE: [Xen-changelog] [xen-unstable]vt-d: Fixpanic in 
msi_msg_read_remap_rte with acpi=off

---- "Dexuan Cui" <dexuan.cui@xxxxxxxxx> wrote:

> From: "Dexuan Cui" <dexuan.cui@xxxxxxxxx>
> To: "Miroslav Rezanina" <mrezanin@xxxxxxxxxx>
> Cc: "Keir Fraser" <keir.fraser@xxxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxx
> 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

Attachment: disable_vtd_when_acpi_is_off_V2.patch
Description: disable_vtd_when_acpi_is_off_V2.patch

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel