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

[Miroslav Rezanina <mrezanin@xxxxxxxxxx>]

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

> From: "Dexuan Cui" <dexuan.cui@xxxxxxxxx>
> To: "Miroslav Rezanina" <mrezanin@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, "Keir Fraser" <keir.fraser@xxxxxxxxxxxxx>
> 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@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
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team - XEN kernel


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