Re: [PATCH] VT-d: improve RMRR validity checking

[Weidong Han <weidong.han@intel.com>]

Sander Eikelenboom wrote:
> Hello Weidong,
>
> Wouldn't it be more clear to add an option to iommu= for this case ?
>
> if iommu=on,..,..,security
>
> With the security option specified:
>      -it would be most strict in it's checks, since enforcing security with the iommu requires that as you have pointed out.
>      -warn,fail or panic incase it can't enable all to enforce the security.
>   
iommu=force is for security. It does as you described above. So I think 
"security" option is not necessary.
> Without the security option specified (default)
>      - it tries to work as with the security option specified
>      - but incase of problems makes the assumption the iommu's main task is not security, but making as much of vt-d working to keep the passthrough functionality
>      - it will only warn, that you will lose the security part, that it would be wise to let your bios be fixed, and not making it panic
>      - and keep vt-d enabled
>
>   
the default iommu=1 works like iommu=force if BIOS is correct. But in 
fact we encountered some buggy BIOS, and then we added some workarounds 
to make VT-d still be enabled,  or warn and disable VT-d if the issue is 
regarded as invalid and cannot be workarounded. These workarounds make 
Xen more defensive to VT-d BIOS issues. The panic only occurs when 
operating VT-d hardware fails, because it means the hardware is possibly 
malfunctional.

In short, default iommu=1 can workaround known VT-d BIOS issues we 
observed till now, while iommu=force ensures best security provided by VT-d.

Regards,
Weidong

> Regards,
>
> Sander
>
>
>
> Friday, January 22, 2010, 9:47:11 AM, you wrote:
>
>   
>> diff -r 207fba95a7d5 xen/drivers/passthrough/vtd/dmar.c
>> --- a/xen/drivers/passthrough/vtd/dmar.c        Fri Jan 22 13:12:45 2010 +0800
>> +++ b/xen/drivers/passthrough/vtd/dmar.c        Fri Jan 22 22:32:10 2010 +0800
>> @@ -396,8 +396,49 @@ acpi_parse_one_drhd(struct acpi_dmar_ent
>>  
>>      if ( ret )
>>          xfree(dmaru);
>> +    else if ( force_iommu || dmaru->include_all )
>> +        acpi_register_drhd_unit(dmaru);
>>      else
>> -        acpi_register_drhd_unit(dmaru);
>> +    {
>> +        u8 b, d, f;
>> +        int i, invalid_cnt = 0;
>> +
>> +        for ( i = 0; i < dmaru->scope.devices_cnt; i++ )
>> +        {
>> +            b = PCI_BUS(dmaru->scope.devices[i]);
>> +            d = PCI_SLOT(dmaru->scope.devices[i]);
>> +            f = PCI_FUNC(dmaru->scope.devices[i]);
>> +
>> +            if ( pci_device_detect(b, d, f) == 0 )
>> +            {
>> +                dprintk(XENLOG_WARNING VTDPREFIX,
>> +                    "  Non-existent device (%x:%x.%x) is reported "
>> +                    "in this DRHD's scope!\n", b, d, f);
>> +                invalid_cnt++;
>> +            }
>> +        }
>> +
>> +        if ( invalid_cnt )
>> +        {
>> +            xfree(dmaru);
>> +            if ( invalid_cnt == dmaru->scope.devices_cnt )
>> +            {
>> +                dprintk(XENLOG_WARNING VTDPREFIX,
>> +                    "  Ignore the DRHD due to all devices under "
>> +                    "its scope are not PCI discoverable!\n");
>> +            }
>> +            else
>> +            {
>> +                dprintk(XENLOG_WARNING VTDPREFIX,
>> +                    "  The DRHD is invalid due to some devices under "
>> +                    "its scope are not PCI discoverable!\n");
>> +                ret = -EINVAL;
>> +            }
>> +        }
>> +        else
>> +            acpi_register_drhd_unit(dmaru);
>> +    }
>> +
>>      return ret;
>>  }
>>  
>>     
>
>
>
>