From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulpanit Subject: Re: [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle Date: Fri, 13 Sep 2013 18:31:25 -0500 Message-ID: <5233A04D.9070601@amd.com> References: <1379005240-4857-1-git-send-email-suravee.suthikulpanit@amd.com> <5232F5D402000078000F2FDF@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5232F5D402000078000F2FDF@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 9/13/2013 4:24 AM, Jan Beulich wrote: >>>> On 12.09.13 at 19:00, wrote: >> --- a/xen/drivers/passthrough/amd/iommu_acpi.c >> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c >> @@ -664,19 +664,46 @@ static void __init parse_ivrs_hpet(char *str) >> >> ASSERT(*s == '['); >> id = simple_strtoul(s + 1, &s, 0); >> - if ( id != (typeof(hpet_sbdf.id))id || *s != ']' || *++s != '=' ) >> + if ( (*s != ']') || (*++s != '=') ) > No, unless you have a very good reason. Oh, sorry. This is my mistake. > >> return; >> >> s = parse_pci(s + 1, &seg, &bus, &dev, &func); >> if ( !s || *s ) >> return; >> >> + hpet_sbdf.id = id; > In essence this is the only change not contained in the patch I sent. > So I'd be inclined to commit this (and perhaps the one debug > message adjustment below) under your name, and my proposed > change as a separate one. Ok. > >> hpet_sbdf.bdf = PCI_BDF(bus, dev, func); >> hpet_sbdf.seg = seg; >> hpet_sbdf.cmdline = 1; >> } >> custom_param("ivrs_hpet[", parse_ivrs_hpet); >> >> +static bool_t is_ioapic_overidden(u16 seg, u16 bdf, u8 handle) > Missing __init annotation. And anyway, I can't really see why > putting this in a separate function is a significant benefit. It's > only being used in one place afaics. Sorry, I forgot the __init. I normally just like to keep to code that does a specific thing inside it's own function for the ease of reading. But this is purely coding style. I can put it in the caller function if you prefer. > >> +{ >> + bool_t ret = 0; >> + int apic = find_first_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf)); >> + >> + while ( apic < ARRAY_SIZE(ioapic_sbdf) ) >> + { >> + if ( ioapic_sbdf[apic].bdf == bdf && >> + ioapic_sbdf[apic].seg == seg ) >> + break; >> + apic = find_next_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf), >> + apic + 1); >> + } >> + >> + if ( apic < ARRAY_SIZE(ioapic_sbdf) ) >> + { >> + AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC %#x " >> + "(IVRS: %#x devID %04x:%02x:%02x.%u)\n", >> + apic, handle, seg, PCI_BUS(bdf), >> + PCI_SLOT(bdf), PCI_FUNC(bdf)); >> + ret = 1; >> + } >> + >> + return ret; >> +} >> + >> static u16 __init parse_ivhd_device_special( >> const struct acpi_ivrs_device8c *special, u16 seg, >> u16 header_length, u16 block_length, struct amd_iommu *iommu) >> @@ -698,16 +725,18 @@ static u16 __init parse_ivhd_device_special( >> return 0; >> } >> >> - AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle %#x\n", >> + AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle %#x used_id %#x\n", >> seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf), >> - special->variety, special->handle); >> + special->variety, special->handle, special->used_id); >> add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, iommu); >> >> switch ( special->variety ) >> { >> case ACPI_IVHD_IOAPIC: >> - if ( !iommu_intremap ) >> + if ( !iommu_intremap || >> + is_ioapic_overidden(seg, bdf, special->handle) ) >> break; >> + >> /* >> * Some BIOSes have IOAPIC broken entries so we check for IVRS >> * consistency here --- whether entry's IOAPIC ID is valid and >> @@ -725,10 +754,7 @@ static u16 __init parse_ivhd_device_special( >> return 0; >> } >> >> - if ( test_bit(special->handle, ioapic_cmdline) ) >> - AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC %#x\n", >> - special->handle); >> - else if ( ioapic_sbdf[special->handle].pin_2_idx ) >> + if ( ioapic_sbdf[special->handle].pin_2_idx ) > Again - no, unless you have a very good reason. I think with the check for command line stuff already handle prior to this point, I don't see why we still need to keep this here. It should not need to go through the logic here. > >> { >> if ( ioapic_sbdf[special->handle].bdf == bdf && >> ioapic_sbdf[special->handle].seg == seg ) >> @@ -770,6 +796,16 @@ 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; >> + break; >> + } >> + >> /* set device id of hpet */ >> if ( hpet_sbdf.iommu || >> (hpet_sbdf.cmdline && hpet_sbdf.id != special->handle) ) >> @@ -777,12 +813,10 @@ static u16 __init parse_ivhd_device_special( >> printk(XENLOG_WARNING "Only one IVHD HPET entry is supported\n"); >> break; >> } >> + >> hpet_sbdf.id = special->handle; >> - if ( !hpet_sbdf.cmdline ) >> - { >> - hpet_sbdf.bdf = bdf; >> - hpet_sbdf.seg = seg; >> - } >> + hpet_sbdf.bdf = bdf; >> + hpet_sbdf.seg = seg; > I don't see what benefit these HPET related changes provide. Or > if there is any that I overlook, then the previous uses of > hpet_sbdf.cmdline should all be eliminated. > > Jan > I found an issue when I was testing. if the "handle" for the IVRS HPET is not the same where: - IVRS Table HPET handle = 0x0 - HPET Table = 0x2 This throws the following error message: (XEN) AMD-Vi: Failed to setup HPET MSI remapping: Wrong HPET In this case, the handle is wrong and I am trying to change it. When I am trying to specify the command line option "ivrs_hpet[2]=00:14.0" this does not get used because the in the parse_ivrs_hpet() did not store the override id. So, I modify the logic in the parse_ivrs_hpet() to override the hpet_sbdf.bdf with the new ID specified from command line. Then it doesn't need to go though this logic here. Also, the check "hpet_sbdf.cmdline && hpet_sbdf.id != special->handle" should also be eliminated. I will split the patch send out V4 with updates, and thank you for your patience. Suravee