Re: [Xen-devel] [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.09.13 at 16:10, Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
wrote:
> On 09/16/2013 03:03 AM, Jan Beulich wrote:
>>>>> On 14.09.13 at 01:30, <suravee.suthikulpanit@xxxxxxx> wrote:
>>> From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
>>>
>>> The current logic does not handle the case when HPET special->handle
>>> is invalid in IVRS. On such system, the following message is shown:
>>>
>>> (XEN) AMD-Vi: Failed to setup HPET MSI remapping: Wrong HPET
>>>
>>> This patch will allow the ivrs_hpet[<handle>]=<sbdf> to override the
>>> IVRS.
>>>
>>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
>>> ---
>>>   xen/drivers/passthrough/amd/iommu_acpi.c | 22 +++++++++++++++-------
>>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c
>>> b/xen/drivers/passthrough/amd/iommu_acpi.c
>>> index 0778af0..bfcc2eb 100644
>>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>>> @@ -671,6 +671,7 @@ static void __init parse_ivrs_hpet(char *str)
>>>       if ( !s || *s )
>>>           return;
>>>   
>>> +    hpet_sbdf.id = id;
>>>       hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
>>>       hpet_sbdf.seg = seg;
>>>       hpet_sbdf.cmdline = 1;
>>> @@ -787,19 +788,26 @@ static u16 __init parse_ivhd_device_special(
>>>           }
>>>           break;
>>>       case ACPI_IVHD_HPET:
>>> +        if ( hpet_sbdf.cmdline )
>>> +        {
>>> +            AMD_IOMMU_DEBUG("IVHD: Command line override present for HPET 
> %#x "
>>> +                            "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
>>> +                            hpet_sbdf.id, special->handle, seg, 
>>> PCI_BUS(bdf),
>>> +                            PCI_SLOT(bdf), PCI_FUNC(bdf));
>>> +            hpet_sbdf.iommu = iommu;
>> This (unconditional) assignment is what the earlier logic attempted
>> to avoid: We must not blindly set this (and in particular not blindly
>> overwrite a previously set valid value), and in order to do so we
>> need to know whether to trust devid or handle. I'm therefore going
>> to apply only the first hunk - being a clear and obvious bug fix - for
>> the time being.
>>
> But since we only allow one HPET in the system, and if users want to 
> override the one that is in the IVRS (the buggy one)
> we should allow this, right?  Otherwise, if the special->handle doesn't 
> match, it would end up causing this logic to complain
> about multiple HPETs.

We must not allow multiple HPET entries in the ACPI tables to
confuse us and store a bad IOMMU pointer. And yes, we also
must not wrongly complain about multiple HEPTs.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel