Re: [PATCH 1/1 V3] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

[Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>]

On 9/10/2013 7:47 AM, Jan Beulich wrote:
>>>> On 09.09.13 at 17:52, <suravee.suthikulpanit@amd.com> wrote:
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -147,9 +147,10 @@ static void amd_iommu_setup_domain_device(
>>   
>>           amd_iommu_flush_device(iommu, req_id);
>>   
>> -        AMD_IOMMU_DEBUG("Setup I/O page table: device id = %#x, "
>> +        AMD_IOMMU_DEBUG("Setup I/O page table: device id = %#x, type = %x, "
> Consistency please: Mixing hex and dec numbers in a single
> message almost requires disambiguating them. Please add the
> missing #.
Ok
>
>> @@ -175,6 +176,15 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn, struct pci_dev *pdev)
>>   
>>       if ( unlikely(!iommu) )
>>       {
>> +        /* Filter the bridge devices */
>> +        if ( (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
> Superfluous parentheses.
Ok
>
>> +        {
>> +            AMD_IOMMU_DEBUG("Skipping host bridge %04x:%02x:%02x.%u (type %x)\n",
>> +                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
>> +                        pdev->type);
> Saying "host bridge" _and_ being in a conditional only allowing host
> bridges in makes printing the type redundant.
Ok
>
>> --- a/xen/drivers/passthrough/vtd/intremap.c
>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>> @@ -439,6 +439,9 @@ static void set_msi_source_id(struct pci_dev *pdev, struct iremap_entry *ire)
>>       {
>>           unsigned int sq;
>>   
>> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>> +	break;
>> +
> This can't be right, as it re-introduces the issue XSA-49 was about.
> The easiest is perhaps to simply have this handled together with
> the immediately subsequent cases.
Ok, I'll merge them.
>
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -1433,6 +1433,12 @@ static int domain_context_mapping(
>>   
>>       switch ( pdev->type )
>>       {
>> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>> +	if (iommu_verbose)
> Hard tab and missing spaces.
Ok
>
>> +            dprintk(VTDPREFIX, "d%d:Hostbridge map skip %04x:%02x:%02x.%u\n",
>> +                domain->domain_id, seg, bus,
>> +                PCI_SLOT(devfn), PCI_FUNC(devfn));
>> +
> Fall-through in a switch statement needs to be annotated, to make
> clear it is intentional.
Ok.  I'll put break here.
>
>>       case DEV_TYPE_PCIe_BRIDGE:
>>       case DEV_TYPE_PCIe2PCI_BRIDGE:
>>       case DEV_TYPE_LEGACY_PCI_BRIDGE:
>> @@ -1563,6 +1569,12 @@ static int domain_context_unmap(
>>   
>>       switch ( pdev->type )
>>       {
>> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>> +	if (iommu_verbose)
> See above.
OK
>
>> +            dprintk(VTDPREFIX, "d%d:Hostbridge unmap skip %04x:%02x:%02x.%u\n",
>> +                domain->domain_id, seg, bus,
>> +                PCI_SLOT(devfn), PCI_FUNC(devfn));
>> +
> See above.
OK. I'll put "goto out" here.
>
>>       case DEV_TYPE_PCIe_BRIDGE:
>>       case DEV_TYPE_PCIe2PCI_BRIDGE:
>>       case DEV_TYPE_LEGACY_PCI_BRIDGE:
>> @@ -1885,7 +1897,8 @@ static int intel_iommu_add_device(u8 devfn, struct pci_dev *pdev)
>>       ret = domain_context_mapping(pdev->domain, devfn, pdev);
>>       if ( ret )
>>       {
>> -        dprintk(XENLOG_ERR VTDPREFIX, "d%d: context mapping failed\n",
>> +        if ( ret != -EPERM )
>> +            dprintk(XENLOG_ERR VTDPREFIX, "d%d: context mapping failed\n",
>>                   pdev->domain->domain_id);
> Where's that -EPERM coming from, and what's the reason for not
> printing the message in that case?
Sorry, I forgot to take this one out.  I was going to return -EPERM 
before for the case when we skipping
the host bridges.  But I don't think this is needed since I already 
printing message about skipping hostbridge for when iommu_verbose.

Thanks,

Suravee