RE: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking

2009-05-19 Thread Han, Weidong 
Ingo Molnar wrote:
> * Han, Weidong  wrote:
> 
>> Ingo Molnar wrote:
>>> * Han, Weidong  wrote:
>>> 
 Siddha, Suresh B wrote:
> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
>> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
>> acpi_dmar_header *header, " 0x%Lx\n",
>> scope->enumeration_id, drhd->address);
>> 
>> +bus = pci_find_bus(drhd->segment, scope->bus);
>> +path = (struct acpi_dmar_pci_path *)(scope + 
>> 1); +  count =
>> (scope->length - +sizeof(struct 
>> acpi_dmar_device_scope))
>> +/ sizeof(struct acpi_dmar_pci_path);
>> +
>> +while (count) {
>> +if (pdev)
>> +pci_dev_put(pdev);
>> +
>> +if (!bus)
>> +break;
>> +
>> +pdev = pci_get_slot(bus,
>> +PCI_DEVFN(path->dev, path->fn));
>> +if (!pdev)
>> +break;
> 
> ir_parse_ioapic_scope() happens very early in the boot. So, I
> don't think we can do the pci related discovery here.
> 
 
 Thanks for your pointing it out. It should enable the source-id
 checking for io-apic's after the pci subsystem is up. I will
 change it.
>>> 
>>> Note, there's ways to do early PCI quirks too, check
>>> arch/x86/kernel/early-quirks.c. It's done by reading the PCI
>>> configuration space directly via a careful early-capable subset of
>>> the PCI config space APIs. 
>>> 
>>> But it's a method of last resort.
>>> 
>> 
>> Thanks for your reminder. It can use direct PCI access here as
>> follows. It's easy and clean. I think it's better than adding the
>> source-id checking for io-apic's after the pci subsystem is up. I
>> will send out updated patches after some tests.   
>> 
>> @@ -634,6 +695,24 @@ static int ir_parse_ioapic_scope(struct
>>acpi_dmar_header *header, " 0x%Lx\n",
>>scope->enumeration_id, drhd->address);
>> 
>> +   bus = scope->bus;
>> +   path = (struct acpi_dmar_pci_path *)(scope +
>> 1); +   count = (scope->length -
>> +sizeof(struct
>> acpi_dmar_device_scope)) +   /
>> sizeof(struct acpi_dmar_pci_path); + +   while
>> (--count > 0) { +   /* Access PCI
>> directly due to the PCI +* subsystem
>> isn't initialized yet. +*/
>> +   bus = read_pci_config_byte(bus,
>> path->dev, +   path->fn,
>> PCI_SECONDARY_BUS); +   path++;
>> +   }
>> +
>> +   ir_ioapic[ir_ioapic_num].bus = bus;
>> +   ir_ioapic[ir_ioapic_num].devfn =
>> +   PCI_DEVFN(path->dev,
>> path->fn); 
> 
> looks good IMO, beyond the obligatory comment-style nitpick [*] :-)
> Also, the function above seems to be way too large - please split it
> into a couple of natural helper functions.
> 
> Thanks,
> 
>   Ingo
> 
> [*]
> 
> Please use the customary comment style:
> 
>   /*
>* Comment .
>* .. goes here:
>*/
> 
> specified in Documentation/CodingStyle.

I have sent out the updated patches. Thanks!

Regards,
Weidong--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking

2009-05-19 Thread Ingo Molnar 

* Han, Weidong  wrote:

> Ingo Molnar wrote:
> > * Han, Weidong  wrote:
> > 
> >> Siddha, Suresh B wrote:
> >>> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
>  @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
>  acpi_dmar_header *header, " 0x%Lx\n",
>  scope->enumeration_id, drhd->address);
>  
>  +bus = pci_find_bus(drhd->segment, scope->bus);
>  +path = (struct acpi_dmar_pci_path *)(scope + 
>  1); +  count =
>  (scope->length - +sizeof(struct 
>  acpi_dmar_device_scope))
>  +/ sizeof(struct acpi_dmar_pci_path);
>  +
>  +while (count) {
>  +if (pdev)
>  +pci_dev_put(pdev);
>  +
>  +if (!bus)
>  +break;
>  +
>  +pdev = pci_get_slot(bus,
>  +PCI_DEVFN(path->dev, path->fn));
>  +if (!pdev)
>  +break;
> >>> 
> >>> ir_parse_ioapic_scope() happens very early in the boot. So, I
> >>> don't think we can do the pci related discovery here.
> >>> 
> >> 
> >> Thanks for your pointing it out. It should enable the source-id
> >> checking for io-apic's after the pci subsystem is up. I will
> >> change it.
> > 
> > Note, there's ways to do early PCI quirks too, check
> > arch/x86/kernel/early-quirks.c. It's done by reading the PCI
> > configuration space directly via a careful early-capable subset of
> > the PCI config space APIs.
> > 
> > But it's a method of last resort.
> > 
> 
> Thanks for your reminder. It can use direct PCI access here as follows. It's 
> easy and clean. I think it's better than adding the source-id checking for 
> io-apic's after the pci subsystem is up. I will send out updated patches 
> after some tests.
>  
> @@ -634,6 +695,24 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header 
> *header,
>" 0x%Lx\n", scope->enumeration_id,
>drhd->address);
> 
> +   bus = scope->bus;
> +   path = (struct acpi_dmar_pci_path *)(scope + 1);
> +   count = (scope->length -
> +sizeof(struct acpi_dmar_device_scope))
> +   / sizeof(struct acpi_dmar_pci_path);
> +
> +   while (--count > 0) {
> +   /* Access PCI directly due to the PCI
> +* subsystem isn't initialized yet.
> +*/
> +   bus = read_pci_config_byte(bus, path->dev,
> +   path->fn, PCI_SECONDARY_BUS);
> +   path++;
> +   }
> +
> +   ir_ioapic[ir_ioapic_num].bus = bus;
> +   ir_ioapic[ir_ioapic_num].devfn =
> +   PCI_DEVFN(path->dev, path->fn);

looks good IMO, beyond the obligatory comment-style nitpick [*] :-) 
Also, the function above seems to be way too large - please split it 
into a couple of natural helper functions.

Thanks,

Ingo

[*]

Please use the customary comment style:

  /*
   * Comment .
   * .. goes here:
   */

specified in Documentation/CodingStyle.



--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking

2009-05-18 Thread Han, Weidong 
Ingo Molnar wrote:
> * Han, Weidong  wrote:
> 
>> Siddha, Suresh B wrote:
>>> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
 @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
   acpi_dmar_header *header, " 0x%Lx\n",
   scope->enumeration_id, drhd->address);
 
 +  bus = pci_find_bus(drhd->segment, scope->bus);
 +  path = (struct acpi_dmar_pci_path *)(scope + 1); +  
 count =
 (scope->length - +  sizeof(struct 
 acpi_dmar_device_scope))
 +  / sizeof(struct acpi_dmar_pci_path);
 +
 +  while (count) {
 +  if (pdev)
 +  pci_dev_put(pdev);
 +
 +  if (!bus)
 +  break;
 +
 +  pdev = pci_get_slot(bus,
 +  PCI_DEVFN(path->dev, path->fn));
 +  if (!pdev)
 +  break;
>>> 
>>> ir_parse_ioapic_scope() happens very early in the boot. So, I
>>> don't think we can do the pci related discovery here.
>>> 
>> 
>> Thanks for your pointing it out. It should enable the source-id
>> checking for io-apic's after the pci subsystem is up. I will
>> change it.
> 
> Note, there's ways to do early PCI quirks too, check
> arch/x86/kernel/early-quirks.c. It's done by reading the PCI
> configuration space directly via a careful early-capable subset of
> the PCI config space APIs.
> 
> But it's a method of last resort.
> 

Thanks for your reminder. It can use direct PCI access here as follows. It's 
easy and clean. I think it's better than adding the source-id checking for 
io-apic's after the pci subsystem is up. I will send out updated patches after 
some tests.
 
@@ -634,6 +695,24 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header 
*header,
   " 0x%Lx\n", scope->enumeration_id,
   drhd->address);

+   bus = scope->bus;
+   path = (struct acpi_dmar_pci_path *)(scope + 1);
+   count = (scope->length -
+sizeof(struct acpi_dmar_device_scope))
+   / sizeof(struct acpi_dmar_pci_path);
+
+   while (--count > 0) {
+   /* Access PCI directly due to the PCI
+* subsystem isn't initialized yet.
+*/
+   bus = read_pci_config_byte(bus, path->dev,
+   path->fn, PCI_SECONDARY_BUS);
+   path++;
+   }
+
+   ir_ioapic[ir_ioapic_num].bus = bus;
+   ir_ioapic[ir_ioapic_num].devfn =
+   PCI_DEVFN(path->dev, path->fn);
ir_ioapic[ir_ioapic_num].iommu = iommu;
ir_ioapic[ir_ioapic_num].id = scope->enumeration_id;
ir_ioapic_num++;




--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking

2009-05-11 Thread Ingo Molnar 

* Han, Weidong  wrote:

> Siddha, Suresh B wrote:
> > On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
> >> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
> >>   acpi_dmar_header *header, " 0x%Lx\n",
> >>   scope->enumeration_id, drhd->address);
> >> 
> >> +  bus = pci_find_bus(drhd->segment, scope->bus);
> >> +  path = (struct acpi_dmar_pci_path *)(scope + 1); +  
> >> count =
> >> (scope->length - +  sizeof(struct 
> >> acpi_dmar_device_scope))
> >> +  / sizeof(struct acpi_dmar_pci_path);
> >> +
> >> +  while (count) {
> >> +  if (pdev)
> >> +  pci_dev_put(pdev);
> >> +
> >> +  if (!bus)
> >> +  break;
> >> +
> >> +  pdev = pci_get_slot(bus,
> >> +  PCI_DEVFN(path->dev, path->fn));
> >> +  if (!pdev)
> >> +  break;
> > 
> > ir_parse_ioapic_scope() happens very early in the boot. So, I 
> > don't think we can do the pci related discovery here.
> > 
> 
> Thanks for your pointing it out. It should enable the source-id 
> checking for io-apic's after the pci subsystem is up. I will 
> change it.

Note, there's ways to do early PCI quirks too, check 
arch/x86/kernel/early-quirks.c. It's done by reading the PCI 
configuration space directly via a careful early-capable subset of 
the PCI config space APIs.

But it's a method of last resort.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking

2009-05-10 Thread Han, Weidong 
Siddha, Suresh B wrote:
> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
>> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
>> acpi_dmar_header *header, " 0x%Lx\n",
>> scope->enumeration_id, drhd->address);
>> 
>> +bus = pci_find_bus(drhd->segment, scope->bus);
>> +path = (struct acpi_dmar_pci_path *)(scope + 1); +  
>> count =
>> (scope->length - +sizeof(struct 
>> acpi_dmar_device_scope))
>> +/ sizeof(struct acpi_dmar_pci_path);
>> +
>> +while (count) {
>> +if (pdev)
>> +pci_dev_put(pdev);
>> +
>> +if (!bus)
>> +break;
>> +
>> +pdev = pci_get_slot(bus,
>> +PCI_DEVFN(path->dev, path->fn));
>> +if (!pdev)
>> +break;
> 
> ir_parse_ioapic_scope() happens very early in the boot. So, I don't
> think we can do the pci related discovery here.
> 

Thanks for your pointing it out. It should enable the source-id checking for 
io-apic's after the pci subsystem is up. I will change it.

Regards,
Weidong--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking

2009-05-07 Thread Suresh Siddha 
On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header 
> *header,
>  " 0x%Lx\n", scope->enumeration_id,
>  drhd->address);
>  
> + bus = pci_find_bus(drhd->segment, scope->bus);
> + path = (struct acpi_dmar_pci_path *)(scope + 1);
> + count = (scope->length -
> +  sizeof(struct acpi_dmar_device_scope))
> + / sizeof(struct acpi_dmar_pci_path);
> +
> + while (count) {
> + if (pdev)
> + pci_dev_put(pdev);
> +
> + if (!bus)
> + break;
> +
> + pdev = pci_get_slot(bus,
> + PCI_DEVFN(path->dev, path->fn));
> + if (!pdev)
> + break;

ir_parse_ioapic_scope() happens very early in the boot. So, I don't
think we can do the pci related discovery here.

thanks,
suresh

> +
> + path++;
> + count--;
> + bus = pdev->subordinate;
> + }
> +
> + if (pdev) { /* PCI discoverable IOAPIC*/
> + ir_ioapic[ir_ioapic_num].bus =
> + pdev->bus->number;
> + ir_ioapic[ir_ioapic_num].devfn = pdev->devfn;
> + } else { /* Not PCI discoverable IOAPIC */
> + if (!bus)
> + ir_ioapic[ir_ioapic_num].bus =
> + scope->bus;
> + else
> + ir_ioapic[ir_ioapic_num].bus =
> + bus->number;
> + ir_ioapic[ir_ioapic_num].devfn =
> + PCI_DEVFN(path->dev, path->fn);
> + }
> +
>   ir_ioapic[ir_ioapic_num].iommu = iommu;
>   ir_ioapic[ir_ioapic_num].id = scope->enumeration_id;
>   ir_ioapic_num++;


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html