Re: [v3][PATCH 15/16] xen/vtd: enable USB device assignment

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2015/6/11 18:22, Tian, Kevin wrote:
>> From: Chen, Tiejun
>> Sent: Thursday, June 11, 2015 9:15 AM
>>
>> Before we refine RMRR mechanism, USB RMRR may conflict with guest bios
>> region so we always ignore USB RMRR.
>
> If USB RMRR conflicts with guest bios, the conflict is always there
> before and after your refinement. :-)

Yeah :)

>
>> Now this can be gone when we enable
>> pci_force to check/reserve RMRR.

So what about this?

USB RMRR may conflict with guest bios region so we always ignore
USB RMRR. But now this can be checked to handle after we introduce
our policy mechanism.

>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>
> Acked-by: Kevin Tian <kevin.tian@intel.com> except one small comment below
>
>> ---
>>   xen/drivers/passthrough/vtd/dmar.h  |  1 -
>>   xen/drivers/passthrough/vtd/iommu.c | 11 ++---------
>>   xen/drivers/passthrough/vtd/utils.c |  7 -------
>>   3 files changed, 2 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/vtd/dmar.h b/xen/drivers/passthrough/vtd/dmar.h
>> index af1feef..af205f5 100644
>> --- a/xen/drivers/passthrough/vtd/dmar.h
>> +++ b/xen/drivers/passthrough/vtd/dmar.h
>> @@ -129,7 +129,6 @@ do {                                                \
>>
>>   int vtd_hw_check(void);
>>   void disable_pmr(struct iommu *iommu);
>> -int is_usb_device(u16 seg, u8 bus, u8 devfn);
>>   int is_igd_drhd(struct acpi_drhd_unit *drhd);
>>
>>   #endif /* _DMAR_H_ */
>> diff --git a/xen/drivers/passthrough/vtd/iommu.c
>> b/xen/drivers/passthrough/vtd/iommu.c
>> index d7c9e1c..d3233b8 100644
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -2229,11 +2229,9 @@ static int reassign_device_ownership(
>>       /*
>>        * If the device belongs to the hardware domain, and it has RMRR, don't
>>        * remove it from the hardware domain, because BIOS may use RMRR at
>> -     * booting time. Also account for the special casing of USB below (in
>> -     * intel_iommu_assign_device()).
>> +     * booting time.
>
> this code is run-time right?

According to one associated commit,

commit 8b99f4400b695535153dcd5d949b3f63602ca8bf
Author: Jan Beulich <jbeulich@suse.com>
Date:   Fri Oct 10 10:54:21 2014 +0200

     VT-d: fix RMRR related error handling

     - reassign_device_ownership() now tears down RMRR mappings (for other
       than Dom0)
     - to facilitate that, rmrr_identity_mapping() now deals with both
       establishing and tearing down of these mappings (the open coded
       equivalent in intel_iommu_remove_device() is being replaced at once)
     - intel_iommu_assign_device() now unrolls the assignment upon RMRR
       mapping errors
     - intel_iommu_add_device() now returns consistent values upon RMRR
       mapping failures (was: failure when last iteration ran into a
       problem, success otherwise)
     - intel_iommu_remove_device() no longer special cases Dom0 (it only
       ever gets called for devices removed from the _system_, not a domain)
     - rmrr_identity_mapping() now returns a proper error indicator instead
       of -1 when intel_iommu_map_page() failed

     Signed-off-by: Jan Beulich <jbeulich@suse.com>
     Acked-by: Kevin Tian <kevin.tian@intel.com>

This chunk of codes resides inside intel_iommu_remove_device() so I 
think this shouldn't be for a running domain.

Thanks
Tiejun

>
>>        */
>> -    if ( !is_hardware_domain(source) &&
>> -         !is_usb_device(pdev->seg, pdev->bus, pdev->devfn) )
>> +    if ( !is_hardware_domain(source) )
>>       {
>>           const struct acpi_rmrr_unit *rmrr;
>>           u16 bdf;
>> @@ -2283,13 +2281,8 @@ static int intel_iommu_assign_device(
>>       if ( ret )
>>           return ret;
>>
>> -    /* FIXME: Because USB RMRR conflicts with guest bios region,
>> -     * ignore USB RMRR temporarily.
>> -     */
>>       seg = pdev->seg;
>>       bus = pdev->bus;
>> -    if ( is_usb_device(seg, bus, pdev->devfn) )
>> -        return 0;
>>
>>       /* Setup rmrr identity mapping */
>>       for_each_rmrr_device( rmrr, bdf, i )
>> diff --git a/xen/drivers/passthrough/vtd/utils.c b/xen/drivers/passthrough/vtd/utils.c
>> index bd14c02..b8a077f 100644
>> --- a/xen/drivers/passthrough/vtd/utils.c
>> +++ b/xen/drivers/passthrough/vtd/utils.c
>> @@ -29,13 +29,6 @@
>>   #include "extern.h"
>>   #include <asm/io_apic.h>
>>
>> -int is_usb_device(u16 seg, u8 bus, u8 devfn)
>> -{
>> -    u16 class = pci_conf_read16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>> -                                PCI_CLASS_DEVICE);
>> -    return (class == 0xc03);
>> -}
>> -
>>   /* Disable vt-d protected memory registers. */
>>   void disable_pmr(struct iommu *iommu)
>>   {
>> --
>> 1.9.1
>
>