IVDB DTE_ALL Settings

IVDB DTE_ALL Settings

[David Kiarie]

I hope am not messing something up,...

The Linux AMD IOMMU driver does not seem to be applying DTE settings listed in the ALL field correctly.

The relevant code:

  switch (e->type) {

  case IVHD_DEV_ALL:

    DUMP_printk("  DEV_ALL\t\t\t first devid: %02x:%02x.%x"
                " last device %02x:%02x.%x flags: %02x\n",
                PCI_BUS_NUM(iommu->first_device),
                PCI_SLOT(iommu->first_device),
                PCI_FUNC(iommu->first_device),
                PCI_BUS_NUM(iommu->last_device),
                PCI_SLOT(iommu->last_device),
                PCI_FUNC(iommu->last_device),
                         e->flags);

               for (dev_i = iommu->first_device;
                    dev_i <= iommu->last_device; ++dev_i)
                    set_dev_entry_from_acpi(iommu, dev_i,
                                    e->flags, 0);
                    break;

  case IVHD_DEV_SELECT:

Basically, the iommu->last/first_device fields are initialized in another state of the driver much later in the code(during PCI initialization); the values 
iommu->first/last_device are used before initialization which breaks the logic of this code.

On the other hand, this might be a lame bug as I have not seen any IOMMU using the ALL settings but I haven't seen many.

Cheers,
David.

Re: IVDB DTE_ALL Settings

[Joerg Roedel]

Hi David,

On Tue, Aug 04, 2015 at 05:57:45AM +0300, David Kiarie wrote:
> Basically, the iommu->last/first_device fields are initialized in
> another state of the driver much later in the code(during PCI
> initialization); the values iommu->first/last_device are used before
> initialization which breaks the logic of this code.

You are right, this is probably broken for some time, on the other side
it doesn't matter for now as I've never seen an IVRS table with an
IVHD_ALL field. I'll do a patch to fix this, thanks for the report.



	Joerg

Re: IVDB DTE_ALL Settings

[Valentine Sinitsyn]

Hi Joerg,

On 04.08.2015 21:56, Joerg Roedel wrote:
> Hi David,
>
> On Tue, Aug 04, 2015 at 05:57:45AM +0300, David Kiarie wrote:
>> Basically, the iommu->last/first_device fields are initialized in
>> another state of the driver much later in the code(during PCI
>> initialization); the values iommu->first/last_device are used before
>> initialization which breaks the logic of this code.
>
> You are right, this is probably broken for some time, on the other side
> it doesn't matter for now as I've never seen an IVRS table with an
> IVHD_ALL field. I'll do a patch to fix this, thanks for the report.
Not directly related to the issue David discovered, but as we discuss 
IVRS handling: the specification says that ACPI tables data overrides 
what's in hardware registers (Sect. 5). To the best of my understanding, 
this means e.g. I should take HATS value not from EFR but from IVHD Type 
10 block. However, amd_iommu driver code checks for features directly - 
for what reason? I'm not suggesting this is a bug, but rather asking 
your opinion here.

Thanks,
Valentine

Re: IVDB DTE_ALL Settings

[Joerg Roedel]

On Tue, Aug 04, 2015 at 10:05:45PM +0500, Valentine Sinitsyn wrote:
> Not directly related to the issue David discovered, but as we
> discuss IVRS handling: the specification says that ACPI tables data
> overrides what's in hardware registers (Sect. 5). To the best of my
> understanding, this means e.g. I should take HATS value not from EFR
> but from IVHD Type 10 block. However, amd_iommu driver code checks
> for features directly - for what reason? I'm not suggesting this is
> a bug, but rather asking your opinion here.

In my experience the hardware is more trustable than the ACPI table. We
had several issues with broken BIOSes that screw up the IVRS ACPI table,
so my trust in the values there is limited.



	Joerg

Re: IVDB DTE_ALL Settings

[Valentine Sinitsyn]

Hi Joerg,

On 05.08.2015 14:48, Joerg Roedel wrote:
> On Tue, Aug 04, 2015 at 10:05:45PM +0500, Valentine Sinitsyn wrote:
>> Not directly related to the issue David discovered, but as we
>> discuss IVRS handling: the specification says that ACPI tables data
>> overrides what's in hardware registers (Sect. 5). To the best of my
>> understanding, this means e.g. I should take HATS value not from EFR
>> but from IVHD Type 10 block. However, amd_iommu driver code checks
>> for features directly - for what reason? I'm not suggesting this is
>> a bug, but rather asking your opinion here.
>
> In my experience the hardware is more trustable than the ACPI table. We
> had several issues with broken BIOSes that screw up the IVRS ACPI table,
> so my trust in the values there is limited.
Makes sense. Thank you for sharing.

Valentine