[Qemu-devel] [PATCH v3 0/6] spapr_pci: DT field fixes and PCI DT node creation in QEMU

[Qemu-devel] [PATCH v3 0/6] spapr_pci: DT field fixes and PCI DT node creation in QEMU

[Nikunj A Dadhania]

The patch series creates PCI device tree(DT) nodes in QEMU. The new
hotplug code needs the device node creation in QEMU. While during
boot, nodes were created in SLOF. It makes more sense to consolidate
the code to one place for better maintainability.

Based on David's spapr-next 
https://github.com/dgibson/qemu/tree/spapr-next

Also, patches for populating ibm,loc-code was getting very complicated
with use of RTAS/HCALL

Changelog V2:
 * Fix device tree for 64-bit encoding
 * Fix the class code, was failing xhci
 * Remove macro duplication
 * Fix DT fields generation for boot time device (Michael Roth)

Changelog v1:
 * Correct indent problems reported by checkpatch(David Gibson)
 * Declare sPAPRFDT structure as local (David Gibson)
 * Re-arrange code to avoid multiple indentation (Alexey Kardashevskiy)

Michael Roth (1):
  spapr_pci: fix boot-time device tree fields for pci hotplug

Nikunj A Dadhania (5):
  spapr_pci: remove duplicate macros
  spapr_pci: encode missing 64-bit memory address space
  spapr_pci: encode class code including Prog IF register
  spapr_pci: enumerate and add PCI device tree
  spapr_pci: populate ibm,loc-code

 hw/ppc/spapr_pci.c | 221 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 198 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/6] spapr_pci: remove duplicate macros

[Nikunj A Dadhania]

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2e7590c..4df3a33 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1475,17 +1475,6 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
     return PCI_HOST_BRIDGE(dev);
 }
 
-/* Macros to operate with address in OF binding to PCI */
-#define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
-#define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
-#define b_p(x)          b_x((x), 30, 1) /* 1 if prefetchable */
-#define b_t(x)          b_x((x), 29, 1) /* 1 if the address is aliased */
-#define b_ss(x)         b_x((x), 24, 2) /* the space code */
-#define b_bbbbbbbb(x)   b_x((x), 16, 8) /* bus number */
-#define b_ddddd(x)      b_x((x), 11, 5) /* device number */
-#define b_fff(x)        b_x((x), 8, 3)  /* function number */
-#define b_rrrrrrrr(x)   b_x((x), 0, 8)  /* register number */
-
 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
                           void *fdt)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 1/6] spapr_pci: remove duplicate macros

[Thomas Huth]

On Tue,  5 May 2015 14:23:51 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2e7590c..4df3a33 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1475,17 +1475,6 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>      return PCI_HOST_BRIDGE(dev);
>  }
>  
> -/* Macros to operate with address in OF binding to PCI */
> -#define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
> -#define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
> -#define b_p(x)          b_x((x), 30, 1) /* 1 if prefetchable */
> -#define b_t(x)          b_x((x), 29, 1) /* 1 if the address is aliased */
> -#define b_ss(x)         b_x((x), 24, 2) /* the space code */
> -#define b_bbbbbbbb(x)   b_x((x), 16, 8) /* bus number */
> -#define b_ddddd(x)      b_x((x), 11, 5) /* device number */
> -#define b_fff(x)        b_x((x), 8, 3)  /* function number */
> -#define b_rrrrrrrr(x)   b_x((x), 0, 8)  /* register number */

Seems like the duplication is not in master yet, only in spapr-next,
and has apparently been introduced by commit 2c938a6692c01818e 
(spapr_pci: enable basic hotplug operations) ...
So an alternative would be to fix up that patch instead. David?

Anyway, the duplication should go away, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 1/6] spapr_pci: remove duplicate macros

[Nikunj A Dadhania]

Thomas Huth <thuth@redhat.com> writes:

> On Tue,  5 May 2015 14:23:51 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_pci.c | 11 -----------
>>  1 file changed, 11 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 2e7590c..4df3a33 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -1475,17 +1475,6 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>>      return PCI_HOST_BRIDGE(dev);
>>  }
>>  
>> -/* Macros to operate with address in OF binding to PCI */
>> -#define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
>> -#define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
>> -#define b_p(x)          b_x((x), 30, 1) /* 1 if prefetchable */
>> -#define b_t(x)          b_x((x), 29, 1) /* 1 if the address is aliased */
>> -#define b_ss(x)         b_x((x), 24, 2) /* the space code */
>> -#define b_bbbbbbbb(x)   b_x((x), 16, 8) /* bus number */
>> -#define b_ddddd(x)      b_x((x), 11, 5) /* device number */
>> -#define b_fff(x)        b_x((x), 8, 3)  /* function number */
>> -#define b_rrrrrrrr(x)   b_x((x), 0, 8)  /* register number */
>
> Seems like the duplication is not in master yet, only in spapr-next,
> and has apparently been introduced by commit 2c938a6692c01818e 
> (spapr_pci: enable basic hotplug operations) ...
> So an alternative would be to fix up that patch instead. David?

Yes, that would be fine with me as well.

> Anyway, the duplication should go away, so:
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Regards
Nikunj

Re: [Qemu-devel] [PATCH v3 1/6] spapr_pci: remove duplicate macros

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]

On Wed, May 06, 2015 at 11:11:05AM +0530, Nikunj A Dadhania wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
> > On Tue,  5 May 2015 14:23:51 +0530
> > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
> >
> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/spapr_pci.c | 11 -----------
> >>  1 file changed, 11 deletions(-)
> >> 
> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> index 2e7590c..4df3a33 100644
> >> --- a/hw/ppc/spapr_pci.c
> >> +++ b/hw/ppc/spapr_pci.c
> >> @@ -1475,17 +1475,6 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
> >>      return PCI_HOST_BRIDGE(dev);
> >>  }
> >>  
> >> -/* Macros to operate with address in OF binding to PCI */
> >> -#define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
> >> -#define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
> >> -#define b_p(x)          b_x((x), 30, 1) /* 1 if prefetchable */
> >> -#define b_t(x)          b_x((x), 29, 1) /* 1 if the address is aliased */
> >> -#define b_ss(x)         b_x((x), 24, 2) /* the space code */
> >> -#define b_bbbbbbbb(x)   b_x((x), 16, 8) /* bus number */
> >> -#define b_ddddd(x)      b_x((x), 11, 5) /* device number */
> >> -#define b_fff(x)        b_x((x), 8, 3)  /* function number */
> >> -#define b_rrrrrrrr(x)   b_x((x), 0, 8)  /* register number */
> >
> > Seems like the duplication is not in master yet, only in spapr-next,
> > and has apparently been introduced by commit 2c938a6692c01818e 
> > (spapr_pci: enable basic hotplug operations) ...
> > So an alternative would be to fix up that patch instead. David?
> 
> Yes, that would be fine with me as well.

I've amended the patch in spapr-next.

> 
> > Anyway, the duplication should go away, so:
> >
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Regards
> Nikunj
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 1/6] spapr_pci: remove duplicate macros

[Nikunj A Dadhania]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Wed, May 06, 2015 at 11:11:05AM +0530, Nikunj A Dadhania wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>> > On Tue,  5 May 2015 14:23:51 +0530
>> > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>> >
>> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> >> ---
>> >>  hw/ppc/spapr_pci.c | 11 -----------
>> >>  1 file changed, 11 deletions(-)
>> >> 
>> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> >> index 2e7590c..4df3a33 100644
>> >> --- a/hw/ppc/spapr_pci.c
>> >> +++ b/hw/ppc/spapr_pci.c
>> >> @@ -1475,17 +1475,6 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>> >>      return PCI_HOST_BRIDGE(dev);
>> >>  }
>> >>  
>> >> -/* Macros to operate with address in OF binding to PCI */
>> >> -#define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
>> >> -#define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
>> >> -#define b_p(x)          b_x((x), 30, 1) /* 1 if prefetchable */
>> >> -#define b_t(x)          b_x((x), 29, 1) /* 1 if the address is aliased */
>> >> -#define b_ss(x)         b_x((x), 24, 2) /* the space code */
>> >> -#define b_bbbbbbbb(x)   b_x((x), 16, 8) /* bus number */
>> >> -#define b_ddddd(x)      b_x((x), 11, 5) /* device number */
>> >> -#define b_fff(x)        b_x((x), 8, 3)  /* function number */
>> >> -#define b_rrrrrrrr(x)   b_x((x), 0, 8)  /* register number */
>> >
>> > Seems like the duplication is not in master yet, only in spapr-next,
>> > and has apparently been introduced by commit 2c938a6692c01818e 
>> > (spapr_pci: enable basic hotplug operations) ...
>> > So an alternative would be to fix up that patch instead. David?
>> 
>> Yes, that would be fine with me as well.
>
> I've amended the patch in spapr-next.

Great, I will rebase and drop this patch.
>
>> 
>> > Anyway, the duplication should go away, so:
>> >
>> > Reviewed-by: Thomas Huth <thuth@redhat.com>
>> 
>> Regards
>> Nikunj
>> 
>
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

[Qemu-devel] [PATCH v3 2/6] spapr_pci: encode missing 64-bit memory address space

[Nikunj A Dadhania]

The properties reg/assigned-resources need to encode 64-bit memory
address space as part of phys.hi dword.

  00 if configuration space
  01 if IO region,
  10 if 32-bit MEM region
  11 if 64-bit MEM region

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 4df3a33..ea1a092 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -786,7 +786,13 @@ typedef struct ResourceProps {
  * phys.hi = 0xYYXXXXZZ, where:
  *   0xYY = npt000ss
  *          |||   |
- *          |||   +-- space code: 1 if IO region, 2 if MEM region
+ *          |||   +-- space code
+ *          |||               |
+ *          |||               +  00 if configuration space
+ *          |||               +  01 if IO region,
+ *          |||               +  10 if 32-bit MEM region
+ *          |||               +  11 if 64-bit MEM region
+ *          |||
  *          ||+------ for non-relocatable IO: 1 if aliased
  *          ||        for relocatable IO: 1 if below 64KB
  *          ||        for MEM: 1 if below 1MB
@@ -846,6 +852,8 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
         reg->phys_hi = cpu_to_be32(dev_id | b_rrrrrrrr(pci_bar(d, i)));
         if (d->io_regions[i].type & PCI_BASE_ADDRESS_SPACE_IO) {
             reg->phys_hi |= cpu_to_be32(b_ss(1));
+        } else if (d->io_regions[i].type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+            reg->phys_hi |= cpu_to_be32(b_ss(3));
         } else {
             reg->phys_hi |= cpu_to_be32(b_ss(2));
         }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 2/6] spapr_pci: encode missing 64-bit memory address space

[Thomas Huth]

On Tue,  5 May 2015 14:23:52 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> The properties reg/assigned-resources need to encode 64-bit memory
> address space as part of phys.hi dword.
> 
>   00 if configuration space
>   01 if IO region,
>   10 if 32-bit MEM region
>   11 if 64-bit MEM region
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 4df3a33..ea1a092 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -786,7 +786,13 @@ typedef struct ResourceProps {
>   * phys.hi = 0xYYXXXXZZ, where:
>   *   0xYY = npt000ss
>   *          |||   |
> - *          |||   +-- space code: 1 if IO region, 2 if MEM region
> + *          |||   +-- space code
> + *          |||               |
> + *          |||               +  00 if configuration space
> + *          |||               +  01 if IO region,
> + *          |||               +  10 if 32-bit MEM region
> + *          |||               +  11 if 64-bit MEM region
> + *          |||
>   *          ||+------ for non-relocatable IO: 1 if aliased
>   *          ||        for relocatable IO: 1 if below 64KB
>   *          ||        for MEM: 1 if below 1MB
> @@ -846,6 +852,8 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
>          reg->phys_hi = cpu_to_be32(dev_id | b_rrrrrrrr(pci_bar(d, i)));
>          if (d->io_regions[i].type & PCI_BASE_ADDRESS_SPACE_IO) {
>              reg->phys_hi |= cpu_to_be32(b_ss(1));
> +        } else if (d->io_regions[i].type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> +            reg->phys_hi |= cpu_to_be32(b_ss(3));
>          } else {
>              reg->phys_hi |= cpu_to_be32(b_ss(2));
>          }

Reviewed-by: Thomas Huth <thuth@redhat.com>

BTW, does this also require the new version of SLOF already?

Re: [Qemu-devel] [PATCH v3 2/6] spapr_pci: encode missing 64-bit memory address space

[Nikunj A Dadhania]

Thomas Huth <thuth@redhat.com> writes:

> On Tue,  5 May 2015 14:23:52 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> The properties reg/assigned-resources need to encode 64-bit memory
>> address space as part of phys.hi dword.
>> 
>>   00 if configuration space
>>   01 if IO region,
>>   10 if 32-bit MEM region
>>   11 if 64-bit MEM region
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_pci.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 4df3a33..ea1a092 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -786,7 +786,13 @@ typedef struct ResourceProps {
>>   * phys.hi = 0xYYXXXXZZ, where:
>>   *   0xYY = npt000ss
>>   *          |||   |
>> - *          |||   +-- space code: 1 if IO region, 2 if MEM region
>> + *          |||   +-- space code
>> + *          |||               |
>> + *          |||               +  00 if configuration space
>> + *          |||               +  01 if IO region,
>> + *          |||               +  10 if 32-bit MEM region
>> + *          |||               +  11 if 64-bit MEM region
>> + *          |||
>>   *          ||+------ for non-relocatable IO: 1 if aliased
>>   *          ||        for relocatable IO: 1 if below 64KB
>>   *          ||        for MEM: 1 if below 1MB
>> @@ -846,6 +852,8 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
>>          reg->phys_hi = cpu_to_be32(dev_id | b_rrrrrrrr(pci_bar(d, i)));
>>          if (d->io_regions[i].type & PCI_BASE_ADDRESS_SPACE_IO) {
>>              reg->phys_hi |= cpu_to_be32(b_ss(1));
>> +        } else if (d->io_regions[i].type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>> +            reg->phys_hi |= cpu_to_be32(b_ss(3));
>>          } else {
>>              reg->phys_hi |= cpu_to_be32(b_ss(2));
>>          }
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
> BTW, does this also require the new version of SLOF already?

Not yet, only after patch 4/6 newer SLOF would be needed.

This fixes the hotplug case for device requesting 64-bit bars, like
nec-usb-xhci.

Regards,
Nikunj

Re: [Qemu-devel] [PATCH v3 2/6] spapr_pci: encode missing 64-bit memory address space

[Thomas Huth]

On Wed, 06 May 2015 11:14:32 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> Thomas Huth <thuth@redhat.com> writes:
[...]
> > BTW, does this also require the new version of SLOF already?
> 
> Not yet, only after patch 4/6 newer SLOF would be needed.

Ok, ... but it will also still work with old SLOF version? If
not, I think you should eventually also include a patch in this
series to update the slof.bin of QEMU.

 Thomas

Re: [Qemu-devel] [PATCH v3 2/6] spapr_pci: encode missing 64-bit memory address space

[Nikunj A Dadhania]

Thomas Huth <thuth@redhat.com> writes:

> On Wed, 06 May 2015 11:14:32 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> Thomas Huth <thuth@redhat.com> writes:
> [...]
>> > BTW, does this also require the new version of SLOF already?
>> 
>> Not yet, only after patch 4/6 newer SLOF would be needed.
>
> Ok, ... but it will also still work with old SLOF version? If
> not, I think you should eventually also include a patch in this
> series to update the slof.bin of QEMU.

Alexey, can you pull the SLOF changes to your tree. I have it ready
here:

The following changes since commit bf641aa923237b5c71b472f536246b3f97d7dffb:

  scsi: handle report-luns failure (2015-04-29 15:20:31 +0530)

are available in the git repository at:

  https://github.com/nikunjad/SLOF master

for you to fetch changes up to ad8d7045375ac47cf69080fcbd24788ed32000f8:

  version: update to 20150429 (2015-04-29 15:27:04 +0530)

----------------------------------------------------------------
Nikunj A Dadhania (5):
      pci: program correct bridge limit registers during probe
      pci: Support 64-bit address translation
      usb: support 64-bit pci bars
      pci: Use QEMU created PCI device nodes
      version: update to 20150429

 VERSION                         |  2 +-
 board-qemu/slof/pci-phb.fs      | 44 +++++++++++++++++++++++++++++++++++++++++++-
 slof/fs/devices/pci-class_0c.fs | 10 ++++++++--
 slof/fs/pci-properties.fs       |  6 +++++-
 slof/fs/pci-scan.fs             |  6 +++---
 slof/fs/translate.fs            |  6 ++----
 6 files changed, 62 insertions(+), 12 deletions(-)

[Qemu-devel] [PATCH v3 3/6] spapr_pci: encode class code including Prog IF register

[Nikunj A Dadhania]

Current code missed the Prog IF register. All Class Code, Subclass,
and Prog IF registers are needed to identify the accurate device type.

For example: USB controllers use the PROG IF for denoting: USB
FullSpeed, HighSpeed or SuperSpeed.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ea1a092..8b02a3e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -899,8 +899,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
     _FDT(fdt_setprop_cell(fdt, offset, "revision-id",
                           pci_default_read_config(dev, PCI_REVISION_ID, 1)));
     _FDT(fdt_setprop_cell(fdt, offset, "class-code",
-                          pci_default_read_config(dev, PCI_CLASS_DEVICE, 2)
-                            << 8));
+                          pci_default_read_config(dev, PCI_CLASS_PROG, 3)));
     if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) {
         _FDT(fdt_setprop_cell(fdt, offset, "interrupts",
                  pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 3/6] spapr_pci: encode class code including Prog IF register

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1876 bytes --]

On Tue, May 05, 2015 at 02:23:53PM +0530, Nikunj A Dadhania wrote:
> Current code missed the Prog IF register. All Class Code, Subclass,
> and Prog IF registers are needed to identify the accurate device type.
> 
> For example: USB controllers use the PROG IF for denoting: USB
> FullSpeed, HighSpeed or SuperSpeed.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

Um.. I'm guessing the CLASS_PROG register essentially includes the
CLASS_DEVICE value?  Otherwise it looks like you're losing the
CLASS_DEVICE value.

For the benefit of those who don't remember the PCI spec from memory,
can you explain in more detail what the situation is with the several
class registers and how they overlap / interact.

> ---
>  hw/ppc/spapr_pci.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index ea1a092..8b02a3e 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -899,8 +899,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>      _FDT(fdt_setprop_cell(fdt, offset, "revision-id",
>                            pci_default_read_config(dev, PCI_REVISION_ID, 1)));
>      _FDT(fdt_setprop_cell(fdt, offset, "class-code",
> -                          pci_default_read_config(dev, PCI_CLASS_DEVICE, 2)
> -                            << 8));
> +                          pci_default_read_config(dev, PCI_CLASS_PROG, 3)));
>      if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) {
>          _FDT(fdt_setprop_cell(fdt, offset, "interrupts",
>                   pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)));

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 3/6] spapr_pci: encode class code including Prog IF register

[Thomas Huth]

On Tue, 5 May 2015 22:55:00 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, May 05, 2015 at 02:23:53PM +0530, Nikunj A Dadhania wrote:
> > Current code missed the Prog IF register. All Class Code, Subclass,
> > and Prog IF registers are needed to identify the accurate device type.
> > 
> > For example: USB controllers use the PROG IF for denoting: USB
> > FullSpeed, HighSpeed or SuperSpeed.
> > 
> > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> 
> Um.. I'm guessing the CLASS_PROG register essentially includes the
> CLASS_DEVICE value?  Otherwise it looks like you're losing the
> CLASS_DEVICE value.
> 
> For the benefit of those who don't remember the PCI spec from memory,
> can you explain in more detail what the situation is with the several
> class registers and how they overlap / interact.

In the PCI local bus spec, the "Class Code" is a 3-bytes field starting
at offset 9 in the config space. The QEMU defines seem to split this up
into PCI_CLASS_PROG (9) and PCI_CLASS_DEVICE (10), but originally they
belong together.

> > ---
> >  hw/ppc/spapr_pci.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index ea1a092..8b02a3e 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -899,8 +899,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> >      _FDT(fdt_setprop_cell(fdt, offset, "revision-id",
> >                            pci_default_read_config(dev, PCI_REVISION_ID, 1)));
> >      _FDT(fdt_setprop_cell(fdt, offset, "class-code",
> > -                          pci_default_read_config(dev, PCI_CLASS_DEVICE, 2)
> > -                            << 8));
> > +                          pci_default_read_config(dev, PCI_CLASS_PROG, 3)));
> >      if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) {
> >          _FDT(fdt_setprop_cell(fdt, offset, "interrupts",
> >                   pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)));
> 

Patch looks fine to me.

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH v3 4/6] spapr_pci: enumerate and add PCI device tree

[Nikunj A Dadhania]

All the PCI enumeration and device node creation was off-loaded to
SLOF. With PCI hotplug support, code needed to be added to add device
node. This creates multiple copy of the code one in SLOF and other in
hotplug code. To unify this, the patch adds the pci device node
creation in Qemu. For backward compatibility, a flag
"qemu,phb-enumerated" is added to the phb, suggesting to SLOF to not
do device node creation.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 103 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 8b02a3e..103284a 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -23,6 +23,7 @@
  * THE SOFTWARE.
  */
 #include "hw/hw.h"
+#include "hw/sysbus.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
@@ -35,6 +36,7 @@
 #include "qemu/error-report.h"
 #include "qapi/qmp/qerror.h"
 
+#include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/ppc/spapr_drc.h"
 #include "sysemu/device_tree.h"
@@ -945,7 +947,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
      * processed by OF beforehand
      */
     _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
-    _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, strlen(drc_name)));
+    if (drc_name) {
+        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
+                         strlen(drc_name)));
+    }
     _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
 
     _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
@@ -1001,10 +1006,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
     void *fdt = NULL;
     int fdt_start_offset = 0;
 
-    /* boot-time devices get their device tree node created by SLOF, but for
-     * hotplugged devices we need QEMU to generate it so the guest can fetch
-     * it via RTAS
-     */
     if (dev->hotplugged) {
         fdt = spapr_create_pci_child_dt(phb, pdev, drc_index, drc_name,
                                         &fdt_start_offset);
@@ -1482,6 +1483,89 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
     return PCI_HOST_BRIDGE(dev);
 }
 
+typedef struct sPAPRFDT {
+    void *fdt;
+    int node_off;
+    uint32_t index;
+} sPAPRFDT;
+
+static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
+                                          void *opaque)
+{
+    PCIBus *sec_bus;
+    sPAPRFDT *p = opaque;
+    int ret, offset;
+    int slot = PCI_SLOT(pdev->devfn);
+    int func = PCI_FUNC(pdev->devfn);
+    char nodename[512];
+    sPAPRFDT s_fdt;
+
+    if (func) {
+        sprintf(nodename, "pci@%d,%d", slot, func);
+    } else {
+        sprintf(nodename, "pci@%d", slot);
+    }
+    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
+    g_assert(!ret);
+
+    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
+         PCI_HEADER_TYPE_BRIDGE)) {
+        return;
+    }
+
+    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
+    if (!sec_bus) {
+        return;
+    }
+
+    s_fdt.fdt = p->fdt;
+    s_fdt.node_off = offset;
+    s_fdt.index = p->index;
+    pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
+                        spapr_populate_pci_devices_dt,
+                        &s_fdt);
+}
+
+static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
+                                           void *opaque)
+{
+    unsigned short *bus_no = (unsigned short *) opaque;
+    unsigned short primary = *bus_no;
+    unsigned short secondary;
+    unsigned short subordinate = 0xff;
+
+    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
+         PCI_HEADER_TYPE_BRIDGE)) {
+        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
+        secondary = *bus_no + 1;
+        pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
+        pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
+        pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, secondary, 1);
+        *bus_no = *bus_no + 1;
+        if (sec_bus) {
+            pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
+            pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
+            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1);
+            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
+                                spapr_phb_pci_enumerate_bridge,
+                                bus_no);
+            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
+        }
+    }
+}
+
+static void spapr_phb_pci_enumerate(sPAPRPHBState *phb)
+{
+    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
+    unsigned short bus_no = 0;
+
+    pci_for_each_device(bus, pci_bus_num(bus),
+                        spapr_phb_pci_enumerate_bridge,
+                        &bus_no);
+
+}
+
 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
                           void *fdt)
@@ -1521,6 +1605,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
         cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
     uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
     sPAPRTCETable *tcet;
+    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
+    sPAPRFDT s_fdt;
 
     /* Start populating the FDT */
     sprintf(nodename, "pci@%" PRIx64, phb->buid);
@@ -1570,6 +1656,18 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
                  tcet->liobn, tcet->bus_offset,
                  tcet->nb_table << tcet->page_shift);
 
+    /* Walk the bridges and program the bus numbers*/
+    spapr_phb_pci_enumerate(phb);
+    _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1));
+
+    /* Populate tree nodes with PCI devices attached */
+    s_fdt.fdt = fdt;
+    s_fdt.node_off = bus_off;
+    s_fdt.index = phb->index;
+    pci_for_each_device(bus, pci_bus_num(bus),
+                        spapr_populate_pci_devices_dt,
+                        &s_fdt);
+
     ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
                                 SPAPR_DR_CONNECTOR_TYPE_PCI);
     if (ret) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 4/6] spapr_pci: enumerate and add PCI device tree

[Thomas Huth]

On Tue,  5 May 2015 14:23:54 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> All the PCI enumeration and device node creation was off-loaded to
> SLOF. With PCI hotplug support, code needed to be added to add device
> node. This creates multiple copy of the code one in SLOF and other in
> hotplug code. To unify this, the patch adds the pci device node
> creation in Qemu. For backward compatibility, a flag
> "qemu,phb-enumerated" is added to the phb, suggesting to SLOF to not
> do device node creation.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 103 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 8b02a3e..103284a 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -23,6 +23,7 @@
>   * THE SOFTWARE.
>   */
>  #include "hw/hw.h"
> +#include "hw/sysbus.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/msi.h"
>  #include "hw/pci/msix.h"
> @@ -35,6 +36,7 @@
>  #include "qemu/error-report.h"
>  #include "qapi/qmp/qerror.h"
>  
> +#include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_bus.h"
>  #include "hw/ppc/spapr_drc.h"
>  #include "sysemu/device_tree.h"
> @@ -945,7 +947,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>       * processed by OF beforehand
>       */
>      _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
> -    _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, strlen(drc_name)));
> +    if (drc_name) {
> +        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
> +                         strlen(drc_name)));
> +    }
>      _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
>  
>      _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> @@ -1001,10 +1006,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
>      void *fdt = NULL;
>      int fdt_start_offset = 0;
>  
> -    /* boot-time devices get their device tree node created by SLOF, but for
> -     * hotplugged devices we need QEMU to generate it so the guest can fetch
> -     * it via RTAS
> -     */
>      if (dev->hotplugged) {
>          fdt = spapr_create_pci_child_dt(phb, pdev, drc_index, drc_name,
>                                          &fdt_start_offset);
> @@ -1482,6 +1483,89 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>      return PCI_HOST_BRIDGE(dev);
>  }
>  
> +typedef struct sPAPRFDT {
> +    void *fdt;
> +    int node_off;
> +    uint32_t index;
> +} sPAPRFDT;
> +
> +static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
> +                                          void *opaque)
> +{
> +    PCIBus *sec_bus;
> +    sPAPRFDT *p = opaque;
> +    int ret, offset;
> +    int slot = PCI_SLOT(pdev->devfn);
> +    int func = PCI_FUNC(pdev->devfn);
> +    char nodename[512];

That's quite a big array ....

> +    sPAPRFDT s_fdt;
> +
> +    if (func) {
> +        sprintf(nodename, "pci@%d,%d", slot, func);
> +    } else {
> +        sprintf(nodename, "pci@%d", slot);
> +    }

... just for holding such a small string. Could you maybe use
a smaller array size for nodename (especially since you call this
function recursively)?

> +    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);

The above code sequence looks pretty much similar to
spapr_create_pci_child_dt() ... maybe it's worth the effort to merge
the common code of both functions into a separate helper function
to avoid the duplication? ... not sure if this is worth the effort,
it's just a suggestion.

> +    g_assert(!ret);

You know that assert statements can simply be disabled by a
preprocessor switch - and in that case there is no more error handling
here at all and the code continues silently with a partial initialized
node! So it's maybe better to do a proper error handling here instead?

> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
> +         PCI_HEADER_TYPE_BRIDGE)) {
> +        return;
> +    }
> +
> +    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> +    if (!sec_bus) {
> +        return;
> +    }
> +
> +    s_fdt.fdt = p->fdt;
> +    s_fdt.node_off = offset;
> +    s_fdt.index = p->index;
> +    pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> +                        spapr_populate_pci_devices_dt,
> +                        &s_fdt);
> +}
> +
> +static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
> +                                           void *opaque)
> +{
> +    unsigned short *bus_no = (unsigned short *) opaque;

opaque is a void pointer, so I think you don't need the typecast here.

> +    unsigned short primary = *bus_no;
> +    unsigned short secondary;
> +    unsigned short subordinate = 0xff;

Is there a special reason for using unsigned short variables here?
"unsigned int" would sound much more natural to me.

> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
> +         PCI_HEADER_TYPE_BRIDGE)) {
> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> +        secondary = *bus_no + 1;
> +        pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
> +        pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
> +        pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, secondary, 1);
> +        *bus_no = *bus_no + 1;
> +        if (sec_bus) {
> +            pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
> +            pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
> +            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1);

You write all value here again ... I think you could either drop the
write to the PRIMARY and SECONDARY BUS registers, or you could use a
proper if-else instead.

> +            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> +                                spapr_phb_pci_enumerate_bridge,
> +                                bus_no);
> +            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
> +        }
> +    }
> +}
> +
> +static void spapr_phb_pci_enumerate(sPAPRPHBState *phb)
> +{
> +    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
> +    unsigned short bus_no = 0;

Again, why "short" ?

> +    pci_for_each_device(bus, pci_bus_num(bus),
> +                        spapr_phb_pci_enumerate_bridge,
> +                        &bus_no);
> +
> +}
> +
>  int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                            uint32_t xics_phandle,
>                            void *fdt)
> @@ -1521,6 +1605,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>          cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
>      uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
>      sPAPRTCETable *tcet;
> +    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
> +    sPAPRFDT s_fdt;
>  
>      /* Start populating the FDT */
>      sprintf(nodename, "pci@%" PRIx64, phb->buid);
> @@ -1570,6 +1656,18 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                   tcet->liobn, tcet->bus_offset,
>                   tcet->nb_table << tcet->page_shift);
>  
> +    /* Walk the bridges and program the bus numbers*/
> +    spapr_phb_pci_enumerate(phb);
> +    _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1));
> +
> +    /* Populate tree nodes with PCI devices attached */
> +    s_fdt.fdt = fdt;
> +    s_fdt.node_off = bus_off;
> +    s_fdt.index = phb->index;
> +    pci_for_each_device(bus, pci_bus_num(bus),
> +                        spapr_populate_pci_devices_dt,
> +                        &s_fdt);
> +
>      ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
>                                  SPAPR_DR_CONNECTOR_TYPE_PCI);
>      if (ret) {

 Thomas

Re: [Qemu-devel] [PATCH v3 4/6] spapr_pci: enumerate and add PCI device tree

[Nikunj A Dadhania]

Thomas Huth <thuth@redhat.com> writes:

> On Tue,  5 May 2015 14:23:54 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> All the PCI enumeration and device node creation was off-loaded to
>> SLOF. With PCI hotplug support, code needed to be added to add device
>> node. This creates multiple copy of the code one in SLOF and other in
>> hotplug code. To unify this, the patch adds the pci device node
>> creation in Qemu. For backward compatibility, a flag
>> "qemu,phb-enumerated" is added to the phb, suggesting to SLOF to not
>> do device node creation.
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 103 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 8b02a3e..103284a 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -23,6 +23,7 @@
>>   * THE SOFTWARE.
>>   */
>>  #include "hw/hw.h"
>> +#include "hw/sysbus.h"
>>  #include "hw/pci/pci.h"
>>  #include "hw/pci/msi.h"
>>  #include "hw/pci/msix.h"
>> @@ -35,6 +36,7 @@
>>  #include "qemu/error-report.h"
>>  #include "qapi/qmp/qerror.h"
>>  
>> +#include "hw/pci/pci_bridge.h"
>>  #include "hw/pci/pci_bus.h"
>>  #include "hw/ppc/spapr_drc.h"
>>  #include "sysemu/device_tree.h"
>> @@ -945,7 +947,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>>       * processed by OF beforehand
>>       */
>>      _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
>> -    _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name, strlen(drc_name)));
>> +    if (drc_name) {
>> +        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
>> +                         strlen(drc_name)));
>> +    }
>>      _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
>>  
>>      _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
>> @@ -1001,10 +1006,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
>>      void *fdt = NULL;
>>      int fdt_start_offset = 0;
>>  
>> -    /* boot-time devices get their device tree node created by SLOF, but for
>> -     * hotplugged devices we need QEMU to generate it so the guest can fetch
>> -     * it via RTAS
>> -     */
>>      if (dev->hotplugged) {
>>          fdt = spapr_create_pci_child_dt(phb, pdev, drc_index, drc_name,
>>                                          &fdt_start_offset);
>> @@ -1482,6 +1483,89 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>>      return PCI_HOST_BRIDGE(dev);
>>  }
>>  
>> +typedef struct sPAPRFDT {
>> +    void *fdt;
>> +    int node_off;
>> +    uint32_t index;
>> +} sPAPRFDT;
>> +
>> +static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
>> +                                          void *opaque)
>> +{
>> +    PCIBus *sec_bus;
>> +    sPAPRFDT *p = opaque;
>> +    int ret, offset;
>> +    int slot = PCI_SLOT(pdev->devfn);
>> +    int func = PCI_FUNC(pdev->devfn);
>> +    char nodename[512];
>
> That's quite a big array ....
>
>> +    sPAPRFDT s_fdt;
>> +
>> +    if (func) {
>> +        sprintf(nodename, "pci@%d,%d", slot, func);
>> +    } else {
>> +        sprintf(nodename, "pci@%d", slot);
>> +    }
>
> ... just for holding such a small string. Could you maybe use
> a smaller array size for nodename (especially since you call this
> function recursively)?

Sure, will change this to 32 bytes, that should be sufficient. 

>
>> +    offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
>> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
>
> The above code sequence looks pretty much similar to
> spapr_create_pci_child_dt() ... maybe it's worth the effort to merge
> the common code of both functions into a separate helper function
> to avoid the duplication? ... not sure if this is worth the effort,
> it's just a suggestion.

I had thought about it earlier but something was not matching. Let me
have a relook, things have changed.

>
>> +    g_assert(!ret);
>
> You know that assert statements can simply be disabled by a
> preprocessor switch - and in that case there is no more error handling
> here at all and the code continues silently with a partial initialized
> node!

Thanks for bringing this to notice, assert() ?

> So it's maybe better to do a proper error handling here instead?

Will change this error handling here.

>
>> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
>> +         PCI_HEADER_TYPE_BRIDGE)) {
>> +        return;
>> +    }
>> +
>> +    sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>> +    if (!sec_bus) {
>> +        return;
>> +    }
>> +
>> +    s_fdt.fdt = p->fdt;
>> +    s_fdt.node_off = offset;
>> +    s_fdt.index = p->index;
>> +    pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
>> +                        spapr_populate_pci_devices_dt,
>> +                        &s_fdt);
>> +}
>> +
>> +static void spapr_phb_pci_enumerate_bridge(PCIBus *bus, PCIDevice *pdev,
>> +                                           void *opaque)
>> +{
>> +    unsigned short *bus_no = (unsigned short *) opaque;
>
> opaque is a void pointer, so I think you don't need the typecast here.

Ok, QEMU has strong type checking, so by default I typecast them every time.

>
>> +    unsigned short primary = *bus_no;
>> +    unsigned short secondary;
>> +    unsigned short subordinate = 0xff;
>
> Is there a special reason for using unsigned short variables here?

No special reason, actually unsigned char should do, as the max is bus
number can only be 255

> "unsigned int" would sound much more natural to me.
>
>> +    if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) ==
>> +         PCI_HEADER_TYPE_BRIDGE)) {
>> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>> +        secondary = *bus_no + 1;
>> +        pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
>> +        pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
>> +        pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, secondary, 1);
>> +        *bus_no = *bus_no + 1;
>> +        if (sec_bus) {
>> +            pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
>> +            pci_default_write_config(pdev, PCI_SECONDARY_BUS, secondary, 1);
>> +            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1);
>
> You write all value here again ... I think you c\ould either drop the
> write to the PRIMARY and SECONDARY BUS registers, or you could use a
> proper if-else instead.

I will drop the PRIMARY and SECONDARY here, that would do.

>
>> +            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
>> +                                spapr_phb_pci_enumerate_bridge,
>> +                                bus_no);
>> +            pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
>> +        }
>> +    }
>> +}
>> +
>> +static void spapr_phb_pci_enumerate(sPAPRPHBState *phb)
>> +{
>> +    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
>> +    unsigned short bus_no = 0;
>
> Again, why "short" ?
>
>> +    pci_for_each_device(bus, pci_bus_num(bus),
>> +                        spapr_phb_pci_enumerate_bridge,
>> +                        &bus_no);
>> +
>> +}
>> +
>>  int spapr_populate_pci_dt(sPAPRPHBState *phb,
>>                            uint32_t xics_phandle,
>>                            void *fdt)
>> @@ -1521,6 +1605,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>>          cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
>>      uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
>>      sPAPRTCETable *tcet;
>> +    PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
>> +    sPAPRFDT s_fdt;
>>  
>>      /* Start populating the FDT */
>>      sprintf(nodename, "pci@%" PRIx64, phb->buid);
>> @@ -1570,6 +1656,18 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>>                   tcet->liobn, tcet->bus_offset,
>>                   tcet->nb_table << tcet->page_shift);
>>  
>> +    /* Walk the bridges and program the bus numbers*/
>> +    spapr_phb_pci_enumerate(phb);
>> +    _FDT(fdt_setprop_cell(fdt, bus_off, "qemu,phb-enumerated", 0x1));
>> +
>> +    /* Populate tree nodes with PCI devices attached */
>> +    s_fdt.fdt = fdt;
>> +    s_fdt.node_off = bus_off;
>> +    s_fdt.index = phb->index;
>> +    pci_for_each_device(bus, pci_bus_num(bus),
>> +                        spapr_populate_pci_devices_dt,
>> +                        &s_fdt);
>> +
>>      ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
>>                                  SPAPR_DR_CONNECTOR_TYPE_PCI);
>>      if (ret) {
>
>  Thomas

Regards
Nikunj

[Qemu-devel] [PATCH v3 5/6] spapr_pci: fix boot-time device tree fields for pci hotplug

[Nikunj A Dadhania]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

We need to set the proper drc_index values in ibm,my-drc-index
fields in order to allow a PCI device that was present at
boot-time to be unplugged.

Previously SLOF handles this, but with QEMU handling the DT we
need to do it there as well.

This patch slightly changes how SLOF handled it in the past,
which was to allows add an ibm,my-drc-index value based on
PCI slot/devices topology. Now we only add it when the slot
supports hotplug and has a DR connector, which is more inline
with PAPR.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 103284a..cbd5661 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -951,7 +951,9 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
         _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
                          strlen(drc_name)));
     }
-    _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
+    if (drc_index) {
+        _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
+    }
 
     _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
                           RESOURCE_CELLS_ADDRESS));
@@ -1483,6 +1485,20 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
     return PCI_HOST_BRIDGE(dev);
 }
 
+static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
+                                            PCIDevice *pdev)
+{
+    sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
+    sPAPRDRConnectorClass *drck;
+
+    if (!drc) {
+        return 0;
+    }
+
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    return drck->get_index(drc);
+}
+
 typedef struct sPAPRFDT {
     void *fdt;
     int node_off;
@@ -1499,6 +1515,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
     int func = PCI_FUNC(pdev->devfn);
     char nodename[512];
     sPAPRFDT s_fdt;
+    uint32_t drc_index = spapr_phb_get_pci_drc_index(p->sphb, pdev);
 
     if (func) {
         sprintf(nodename, "pci@%d,%d", slot, func);
@@ -1506,7 +1523,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
         sprintf(nodename, "pci@%d", slot);
     }
     offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
-    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, drc_index, NULL);
     g_assert(!ret);
 
     if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 5/6] spapr_pci: fix boot-time device tree fields for pci hotplug

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3403 bytes --]

On Tue, May 05, 2015 at 02:23:55PM +0530, Nikunj A Dadhania wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> We need to set the proper drc_index values in ibm,my-drc-index
> fields in order to allow a PCI device that was present at
> boot-time to be unplugged.
> 
> Previously SLOF handles this, but with QEMU handling the DT we
> need to do it there as well.
> 
> This patch slightly changes how SLOF handled it in the past,
> which was to allows add an ibm,my-drc-index value based on
> PCI slot/devices topology. Now we only add it when the slot
> supports hotplug and has a DR connector, which is more inline
> with PAPR.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 103284a..cbd5661 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -951,7 +951,9 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>          _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
>                           strlen(drc_name)));
>      }
> -    _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> +    if (drc_index) {
> +        _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> +    }
>  
>      _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
>                            RESOURCE_CELLS_ADDRESS));
> @@ -1483,6 +1485,20 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>      return PCI_HOST_BRIDGE(dev);
>  }
>  
> +static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
> +                                            PCIDevice *pdev)
> +{
> +    sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
> +    sPAPRDRConnectorClass *drck;
> +
> +    if (!drc) {
> +        return 0;
> +    }
> +
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    return drck->get_index(drc);
> +}
> +
>  typedef struct sPAPRFDT {
>      void *fdt;
>      int node_off;
> @@ -1499,6 +1515,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
>      int func = PCI_FUNC(pdev->devfn);
>      char nodename[512];
>      sPAPRFDT s_fdt;
> +    uint32_t drc_index = spapr_phb_get_pci_drc_index(p->sphb, pdev);

The line above causes a compile error, since sPAPRFDT doesn't have an
sphb member.

Its fixed in the next patch, but can you adjust the series so it won't
break bisection.


>      if (func) {
>          sprintf(nodename, "pci@%d,%d", slot, func);
> @@ -1506,7 +1523,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
>          sprintf(nodename, "pci@%d", slot);
>      }
>      offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
> -    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, drc_index, NULL);
>      g_assert(!ret);
>  
>      if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 5/6] spapr_pci: fix boot-time device tree fields for pci hotplug

[Nikunj A Dadhania]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Tue, May 05, 2015 at 02:23:55PM +0530, Nikunj A Dadhania wrote:
>> From: Michael Roth <mdroth@linux.vnet.ibm.com>
>> 
>> We need to set the proper drc_index values in ibm,my-drc-index
>> fields in order to allow a PCI device that was present at
>> boot-time to be unplugged.
>> 
>> Previously SLOF handles this, but with QEMU handling the DT we
>> need to do it there as well.
>> 
>> This patch slightly changes how SLOF handled it in the past,
>> which was to allows add an ibm,my-drc-index value based on
>> PCI slot/devices topology. Now we only add it when the slot
>> supports hotplug and has a DR connector, which is more inline
>> with PAPR.
>> 
>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_pci.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 103284a..cbd5661 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -951,7 +951,9 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>>          _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
>>                           strlen(drc_name)));
>>      }
>> -    _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
>> +    if (drc_index) {
>> +        _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
>> +    }
>>  
>>      _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
>>                            RESOURCE_CELLS_ADDRESS));
>> @@ -1483,6 +1485,20 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>>      return PCI_HOST_BRIDGE(dev);
>>  }
>>  
>> +static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
>> +                                            PCIDevice *pdev)
>> +{
>> +    sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
>> +    sPAPRDRConnectorClass *drck;
>> +
>> +    if (!drc) {
>> +        return 0;
>> +    }
>> +
>> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>> +    return drck->get_index(drc);
>> +}
>> +
>>  typedef struct sPAPRFDT {
>>      void *fdt;
>>      int node_off;
>> @@ -1499,6 +1515,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
>>      int func = PCI_FUNC(pdev->devfn);
>>      char nodename[512];
>>      sPAPRFDT s_fdt;
>> +    uint32_t drc_index = spapr_phb_get_pci_drc_index(p->sphb, pdev);
>
> The line above causes a compile error, since sPAPRFDT doesn't have an
> sphb member.
>
> Its fixed in the next patch, but can you adjust the series so it won't
> break bisection.

Sure, will rearrange the patch.

>
>
>>      if (func) {
>>          sprintf(nodename, "pci@%d,%d", slot, func);
>> @@ -1506,7 +1523,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
>>          sprintf(nodename, "pci@%d", slot);
>>      }
>>      offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
>> -    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
>> +    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, drc_index, NULL);
>>      g_assert(!ret);
>>  
>>      if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
>
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

[Qemu-devel] [PATCH v3 6/6] spapr_pci: populate ibm,loc-code

[Nikunj A Dadhania]

Each hardware instance has a platform unique location code.  The OF
device tree that describes a part of a hardware entity must include
the “ibm,loc-code” property with a value that represents the location
code for that hardware entity.

Populate ibm,loc-code.
1) PCI passthru devices need to identify with its own ibm,loc-code
   available on the host.
2) Emulated devices encode as following:
   qemu_<name>:<phb-index>:<slot>.<fn>

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 75 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cbd5661..eacf0bd 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -744,6 +744,70 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &phb->iommu_as;
 }
 
+static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value)
+{
+    char *host;
+    char path[PATH_MAX];
+
+    host = object_property_get_str(OBJECT(pdev), "host", NULL);
+    if (!host) {
+        return false;
+    }
+
+    snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", host);
+    g_free(host);
+
+    return g_file_get_contents(path, value, NULL, NULL);
+}
+
+static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
+{
+    char path[PATH_MAX], *buf = NULL;
+
+    /* We have a vfio host bridge lets get the path. */
+    if (!spapr_phb_vfio_get_devspec_value(pdev, &buf)) {
+        return NULL;
+    }
+
+    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
+    g_free(buf);
+
+    if (g_file_get_contents(path, &buf, NULL, NULL)) {
+        return buf;
+    } else {
+        return NULL;
+    }
+}
+
+static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
+{
+    char *path = g_malloc(PATH_MAX);
+
+    if (!path) {
+        return NULL;
+    }
+
+    /*
+     * For non-vfio devices and failures make up the location code out
+     * of the name, slot and function.
+     *
+     *       qemu_<name>:<phb-index>:<slot>.<fn>
+     */
+    snprintf(path, PATH_MAX, "qemu_%s:%02d:%02d.%1d", pdev->name,
+             sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    return path;
+}
+
+
+static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
+{
+    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != NULL) {
+        return spapr_phb_vfio_get_loc_code(sphb, pdev);
+    } else {
+        return spapr_phb_get_loc_code(sphb, pdev);
+    }
+}
+
 /* Macros to operate with address in OF binding to PCI */
 #define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
 #define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
@@ -881,12 +945,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
 }
 
 static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
-                                       int phb_index, int drc_index,
-                                       const char *drc_name)
+                                       sPAPRPHBState *phb, int drc_index)
 {
     ResourceProps rp;
     bool is_bridge = false;
     int pci_status;
+    char *buf = NULL;
 
     if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
         PCI_HEADER_TYPE_BRIDGE) {
@@ -947,9 +1011,10 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
      * processed by OF beforehand
      */
     _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
-    if (drc_name) {
-        _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
-                         strlen(drc_name)));
+    buf = spapr_ibm_get_loc_code(phb, dev);
+    if (buf) {
+        _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
+        g_free(buf);
     }
     if (drc_index) {
         _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
@@ -988,8 +1053,7 @@ static void *spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
         sprintf(nodename, "pci@%d", slot);
     }
     offset = fdt_add_subnode(fdt, 0, nodename);
-    ret = spapr_populate_pci_child_dt(dev, fdt, offset, phb->index, drc_index,
-                                      drc_name);
+    ret = spapr_populate_pci_child_dt(dev, fdt, offset, phb, drc_index);
     g_assert(!ret);
 
     *dt_offset = offset;
@@ -1502,7 +1566,7 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
 typedef struct sPAPRFDT {
     void *fdt;
     int node_off;
-    uint32_t index;
+    sPAPRPHBState *sphb;
 } sPAPRFDT;
 
 static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
@@ -1523,7 +1587,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
         sprintf(nodename, "pci@%d", slot);
     }
     offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
-    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, drc_index, NULL);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->sphb, drc_index);
     g_assert(!ret);
 
     if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
@@ -1538,7 +1602,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
 
     s_fdt.fdt = p->fdt;
     s_fdt.node_off = offset;
-    s_fdt.index = p->index;
+    s_fdt.sphb = p->sphb;
     pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
                         spapr_populate_pci_devices_dt,
                         &s_fdt);
@@ -1680,7 +1744,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
     /* Populate tree nodes with PCI devices attached */
     s_fdt.fdt = fdt;
     s_fdt.node_off = bus_off;
-    s_fdt.index = phb->index;
+    s_fdt.sphb = phb;
     pci_for_each_device(bus, pci_bus_num(bus),
                         spapr_populate_pci_devices_dt,
                         &s_fdt);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 6/6] spapr_pci: populate ibm,loc-code

[Thomas Huth]

On Tue,  5 May 2015 14:23:56 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> Each hardware instance has a platform unique location code.  The OF
> device tree that describes a part of a hardware entity must include
> the “ibm,loc-code” property with a value that represents the location
> code for that hardware entity.
> 
> Populate ibm,loc-code.
> 1) PCI passthru devices need to identify with its own ibm,loc-code
>    available on the host.
> 2) Emulated devices encode as following:
>    qemu_<name>:<phb-index>:<slot>.<fn>
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 75 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cbd5661..eacf0bd 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -744,6 +744,70 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>      return &phb->iommu_as;
>  }
>  
> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value)
> +{
> +    char *host;
> +    char path[PATH_MAX];
> +
> +    host = object_property_get_str(OBJECT(pdev), "host", NULL);
> +    if (!host) {
> +        return false;
> +    }
> +
> +    snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", host);
> +    g_free(host);
> +
> +    return g_file_get_contents(path, value, NULL, NULL);
> +}
> +
> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
> +{
> +    char path[PATH_MAX], *buf = NULL;
> +
> +    /* We have a vfio host bridge lets get the path. */
> +    if (!spapr_phb_vfio_get_devspec_value(pdev, &buf)) {
> +        return NULL;
> +    }
> +
> +    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
> +    g_free(buf);
> +
> +    if (g_file_get_contents(path, &buf, NULL, NULL)) {
> +        return buf;
> +    } else {
> +        return NULL;
> +    }

Minor idea for an optimization: g_file_get_contents() should set buf to
NULL in case of errors anyway, so you could omit the "if" and
"return NULLL" and simply always "return buf" here.

> +}
> +
> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
> +{
> +    char *path = g_malloc(PATH_MAX);
> +
> +    if (!path) {
> +        return NULL;
> +    }
> +
> +    /*
> +     * For non-vfio devices and failures make up the location code out

This comment ("and failures") ...

> +     * of the name, slot and function.
> +     *
> +     *       qemu_<name>:<phb-index>:<slot>.<fn>
> +     */
> +    snprintf(path, PATH_MAX, "qemu_%s:%02d:%02d.%1d", pdev->name,
> +             sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +    return path;
> +}
> +
> +
> +static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
> +{
> +    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != NULL) {
> +        return spapr_phb_vfio_get_loc_code(sphb, pdev);
> +    } else {
> +        return spapr_phb_get_loc_code(sphb, pdev);
> +    }

... does not match quite the behavior here, as far as I can see. I
guess you also wanted to fall back to spapr_phb_get_loc_code() in case 
spapr_phb_vfio_get_loc_code() did not work as expected (eg. when the
access to the /sys or /proc filesystem failed)?

> +}
> +
>  /* Macros to operate with address in OF binding to PCI */
>  #define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
>  #define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
> @@ -881,12 +945,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
>  }
>  
>  static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> -                                       int phb_index, int drc_index,
> -                                       const char *drc_name)
> +                                       sPAPRPHBState *phb, int drc_index)

As David already noted, the sPAPRPHBState related hunks likely rather
belong to an earlier patch already?

 Thomas

Re: [Qemu-devel] [PATCH v3 6/6] spapr_pci: populate ibm,loc-code

[Nikunj A Dadhania]

Thomas Huth <thuth@redhat.com> writes:

> On Tue,  5 May 2015 14:23:56 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> Each hardware instance has a platform unique location code.  The OF
>> device tree that describes a part of a hardware entity must include
>> the “ibm,loc-code” property with a value that represents the location
>> code for that hardware entity.
>> 
>> Populate ibm,loc-code.
>> 1) PCI passthru devices need to identify with its own ibm,loc-code
>>    available on the host.
>> 2) Emulated devices encode as following:
>>    qemu_<name>:<phb-index>:<slot>.<fn>
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 75 insertions(+), 11 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index cbd5661..eacf0bd 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -744,6 +744,70 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>      return &phb->iommu_as;
>>  }
>>  
>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value)
>> +{
>> +    char *host;
>> +    char path[PATH_MAX];
>> +
>> +    host = object_property_get_str(OBJECT(pdev), "host", NULL);
>> +    if (!host) {
>> +        return false;
>> +    }
>> +
>> +    snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", host);
>> +    g_free(host);
>> +
>> +    return g_file_get_contents(path, value, NULL, NULL);
>> +}
>> +
>> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
>> +{
>> +    char path[PATH_MAX], *buf = NULL;
>> +
>> +    /* We have a vfio host bridge lets get the path. */
>> +    if (!spapr_phb_vfio_get_devspec_value(pdev, &buf)) {
>> +        return NULL;
>> +    }
>> +
>> +    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
>> +    g_free(buf);
>> +
>> +    if (g_file_get_contents(path, &buf, NULL, NULL)) {
>> +        return buf;
>> +    } else {
>> +        return NULL;
>> +    }
>
> Minor idea for an optimization: g_file_get_contents() should set buf to
> NULL in case of errors anyway, so you could omit the "if" and
> "return NULLL" and simply always "return buf" here.

Sure

>
>> +}
>> +
>> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
>> +{
>> +    char *path = g_malloc(PATH_MAX);
>> +
>> +    if (!path) {
>> +        return NULL;
>> +    }
>> +
>> +    /*
>> +     * For non-vfio devices and failures make up the location code out
>
> This comment ("and failures") ...

Oh right, one option is to drop that...

>
>> +     * of the name, slot and function.
>> +     *
>> +     *       qemu_<name>:<phb-index>:<slot>.<fn>
>> +     */
>> +    snprintf(path, PATH_MAX, "qemu_%s:%02d:%02d.%1d", pdev->name,
>> +             sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> +    return path;
>> +}
>> +
>> +
>> +static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
>> +{
>> +    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != NULL) {
>> +        return spapr_phb_vfio_get_loc_code(sphb, pdev);
>> +    } else {
>> +        return spapr_phb_get_loc_code(sphb, pdev);
>> +    }
>
> ... does not match quite the behavior here, as far as I can see. I
> guess you also wanted to fall back to spapr_phb_get_loc_code() in case 
> spapr_phb_vfio_get_loc_code() did not work as expected (eg. when the
> access to the /sys or /proc filesystem failed)?

... In current case, if getting vfio device loc_code fails, we would not
 add any ibm,loc_code info. I thought that would be the proper
 behaviour. As "qemu_" would indicate it as an emulated device, which is
 not true. Or encode it as follows:

"vfio_<name>:<phb-index>:<slot>.<fn>"

So more checks would be needed here in that case:

if (vfio) {
   buf = vfio_get_loc_code()
   if (!buf) 
      buf = cook_vfio_get_loc_code()
   return buf;
} else { ... }

>
>> +}
>> +
>>  /* Macros to operate with address in OF binding to PCI */
>>  #define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
>>  #define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
>> @@ -881,12 +945,12 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
>>  }
>>  
>>  static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>> -                                       int phb_index, int drc_index,
>> -                                       const char *drc_name)
>> +                                       sPAPRPHBState *phb, int drc_index)
>
> As David already noted, the sPAPRPHBState related hunks likely rather
> belong to an earlier patch already?

Yes, I will take care, this resuled while juggling with the patches :-)

Regards
Nikunj

Re: [Qemu-devel] [PATCH v3 6/6] spapr_pci: populate ibm,loc-code

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4815 bytes --]

On Wed, May 06, 2015 at 11:44:59AM +0530, Nikunj A Dadhania wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
> > On Tue,  5 May 2015 14:23:56 +0530
> > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
> >
> >> Each hardware instance has a platform unique location code.  The OF
> >> device tree that describes a part of a hardware entity must include
> >> the “ibm,loc-code” property with a value that represents the location
> >> code for that hardware entity.
> >> 
> >> Populate ibm,loc-code.
> >> 1) PCI passthru devices need to identify with its own ibm,loc-code
> >>    available on the host.
> >> 2) Emulated devices encode as following:
> >>    qemu_<name>:<phb-index>:<slot>.<fn>
> >> 
> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/spapr_pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 75 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> index cbd5661..eacf0bd 100644
> >> --- a/hw/ppc/spapr_pci.c
> >> +++ b/hw/ppc/spapr_pci.c
> >> @@ -744,6 +744,70 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> >>      return &phb->iommu_as;
> >>  }
> >>  
> >> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value)
> >> +{
> >> +    char *host;
> >> +    char path[PATH_MAX];
> >> +
> >> +    host = object_property_get_str(OBJECT(pdev), "host", NULL);
> >> +    if (!host) {
> >> +        return false;
> >> +    }
> >> +
> >> +    snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", host);
> >> +    g_free(host);
> >> +
> >> +    return g_file_get_contents(path, value, NULL, NULL);
> >> +}
> >> +
> >> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
> >> +{
> >> +    char path[PATH_MAX], *buf = NULL;
> >> +
> >> +    /* We have a vfio host bridge lets get the path. */
> >> +    if (!spapr_phb_vfio_get_devspec_value(pdev, &buf)) {
> >> +        return NULL;
> >> +    }
> >> +
> >> +    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
> >> +    g_free(buf);
> >> +
> >> +    if (g_file_get_contents(path, &buf, NULL, NULL)) {
> >> +        return buf;
> >> +    } else {
> >> +        return NULL;
> >> +    }
> >
> > Minor idea for an optimization: g_file_get_contents() should set buf to
> > NULL in case of errors anyway, so you could omit the "if" and
> > "return NULLL" and simply always "return buf" here.
> 
> Sure
> 
> >
> >> +}
> >> +
> >> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
> >> +{
> >> +    char *path = g_malloc(PATH_MAX);
> >> +
> >> +    if (!path) {
> >> +        return NULL;
> >> +    }
> >> +
> >> +    /*
> >> +     * For non-vfio devices and failures make up the location code out
> >
> > This comment ("and failures") ...
> 
> Oh right, one option is to drop that...
> 
> >
> >> +     * of the name, slot and function.
> >> +     *
> >> +     *       qemu_<name>:<phb-index>:<slot>.<fn>
> >> +     */
> >> +    snprintf(path, PATH_MAX, "qemu_%s:%02d:%02d.%1d", pdev->name,
> >> +             sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> >> +    return path;
> >> +}
> >> +
> >> +
> >> +static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
> >> +{
> >> +    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != NULL) {
> >> +        return spapr_phb_vfio_get_loc_code(sphb, pdev);
> >> +    } else {
> >> +        return spapr_phb_get_loc_code(sphb, pdev);
> >> +    }
> >
> > ... does not match quite the behavior here, as far as I can see. I
> > guess you also wanted to fall back to spapr_phb_get_loc_code() in case 
> > spapr_phb_vfio_get_loc_code() did not work as expected (eg. when the
> > access to the /sys or /proc filesystem failed)?
> 
> ... In current case, if getting vfio device loc_code fails, we would not
>  add any ibm,loc_code info. I thought that would be the proper
>  behaviour. As "qemu_" would indicate it as an emulated device, which is
>  not true. Or encode it as follows:
> 
> "vfio_<name>:<phb-index>:<slot>.<fn>"
> 
> So more checks would be needed here in that case:
> 
> if (vfio) {
>    buf = vfio_get_loc_code()
>    if (!buf) 
>       buf = cook_vfio_get_loc_code()
>    return buf;
> } else { ... }

If you can't determine the loc code, I think just leaving it out is
probably the best option for now.  We can add fancier fallbacks in
future if they seem like they make sense.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3 6/6] spapr_pci: populate ibm,loc-code

[Nikunj A Dadhania]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Wed, May 06, 2015 at 11:44:59AM +0530, Nikunj A Dadhania wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>> > On Tue,  5 May 2015 14:23:56 +0530
>> > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>> >
>> >> Each hardware instance has a platform unique location code.  The OF
>> >> device tree that describes a part of a hardware entity must include
>> >> the “ibm,loc-code” property with a value that represents the location
>> >> code for that hardware entity.
>> >> 
>> >> Populate ibm,loc-code.
>> >> 1) PCI passthru devices need to identify with its own ibm,loc-code
>> >>    available on the host.
>> >> 2) Emulated devices encode as following:
>> >>    qemu_<name>:<phb-index>:<slot>.<fn>
>> >> 
>> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> >> ---
>> >>  hw/ppc/spapr_pci.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-------
>> >>  1 file changed, 75 insertions(+), 11 deletions(-)
>> >> 
>> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> >> index cbd5661..eacf0bd 100644
>> >> --- a/hw/ppc/spapr_pci.c
>> >> +++ b/hw/ppc/spapr_pci.c
>> >> @@ -744,6 +744,70 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> >>      return &phb->iommu_as;
>> >>  }
>> >>  
>> >> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value)
>> >> +{
>> >> +    char *host;
>> >> +    char path[PATH_MAX];
>> >> +
>> >> +    host = object_property_get_str(OBJECT(pdev), "host", NULL);
>> >> +    if (!host) {
>> >> +        return false;
>> >> +    }
>> >> +
>> >> +    snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", host);
>> >> +    g_free(host);
>> >> +
>> >> +    return g_file_get_contents(path, value, NULL, NULL);
>> >> +}
>> >> +
>> >> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
>> >> +{
>> >> +    char path[PATH_MAX], *buf = NULL;
>> >> +
>> >> +    /* We have a vfio host bridge lets get the path. */
>> >> +    if (!spapr_phb_vfio_get_devspec_value(pdev, &buf)) {
>> >> +        return NULL;
>> >> +    }
>> >> +
>> >> +    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
>> >> +    g_free(buf);
>> >> +
>> >> +    if (g_file_get_contents(path, &buf, NULL, NULL)) {
>> >> +        return buf;
>> >> +    } else {
>> >> +        return NULL;
>> >> +    }
>> >
>> > Minor idea for an optimization: g_file_get_contents() should set buf to
>> > NULL in case of errors anyway, so you could omit the "if" and
>> > "return NULLL" and simply always "return buf" here.
>> 
>> Sure
>> 
>> >
>> >> +}
>> >> +
>> >> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
>> >> +{
>> >> +    char *path = g_malloc(PATH_MAX);
>> >> +
>> >> +    if (!path) {
>> >> +        return NULL;
>> >> +    }
>> >> +
>> >> +    /*
>> >> +     * For non-vfio devices and failures make up the location code out
>> >
>> > This comment ("and failures") ...
>> 
>> Oh right, one option is to drop that...
>> 
>> >
>> >> +     * of the name, slot and function.
>> >> +     *
>> >> +     *       qemu_<name>:<phb-index>:<slot>.<fn>
>> >> +     */
>> >> +    snprintf(path, PATH_MAX, "qemu_%s:%02d:%02d.%1d", pdev->name,
>> >> +             sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> >> +    return path;
>> >> +}
>> >> +
>> >> +
>> >> +static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
>> >> +{
>> >> +    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != NULL) {
>> >> +        return spapr_phb_vfio_get_loc_code(sphb, pdev);
>> >> +    } else {
>> >> +        return spapr_phb_get_loc_code(sphb, pdev);
>> >> +    }
>> >
>> > ... does not match quite the behavior here, as far as I can see. I
>> > guess you also wanted to fall back to spapr_phb_get_loc_code() in case 
>> > spapr_phb_vfio_get_loc_code() did not work as expected (eg. when the
>> > access to the /sys or /proc filesystem failed)?
>> 
>> ... In current case, if getting vfio device loc_code fails, we would not
>>  add any ibm,loc_code info. I thought that would be the proper
>>  behaviour. As "qemu_" would indicate it as an emulated device, which is
>>  not true. Or encode it as follows:
>> 
>> "vfio_<name>:<phb-index>:<slot>.<fn>"
>> 
>> So more checks would be needed here in that case:
>> 
>> if (vfio) {
>>    buf = vfio_get_loc_code()
>>    if (!buf) 
>>       buf = cook_vfio_get_loc_code()
>>    return buf;
>> } else { ... }
>
> If you can't determine the loc code, I think just leaving it out is
> probably the best option for now.  We can add fancier fallbacks in
> future if they seem like they make sense.

I already have the fancy version now, will send it for review.

Regards
Nikunj

[Qemu-devel] [PATCH v3 0/6] spapr_pci: DT field fixes and PCI DT node creation in QEMU

[Nikunj A Dadhania]

The patch series creates PCI device tree(DT) nodes in QEMU. The new
hotplug code needs the device node creation in QEMU. While during
boot, nodes were created in SLOF. It makes more sense to consolidate
the code to one place for better maintainability.

Based on David's spapr-next 
https://github.com/dgibson/qemu/tree/spapr-next

Also, patches for populating ibm,loc-code was getting very complicated
with use of RTAS/HCALL

Changelog V2:
 * Fix device tree for 64-bit encoding
 * Fix the class code, was failing xhci
 * Remove macro duplication
 * Fix DT fields generation for boot time device (Michael Roth)

Changelog v1:
 * Correct indent problems reported by checkpatch(David Gibson)
 * Declare sPAPRFDT structure as local (David Gibson)
 * Re-arrange code to avoid multiple indentation (Alexey Kardashevskiy)

Michael Roth (1):
  spapr_pci: fix boot-time device tree fields for pci hotplug

Nikunj A Dadhania (5):
  spapr_pci: remove duplicate macros
  spapr_pci: encode missing 64-bit memory address space
  spapr_pci: encode class code including Prog IF register
  spapr_pci: enumerate and add PCI device tree
  spapr_pci: populate ibm,loc-code

 hw/ppc/spapr_pci.c | 221 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 198 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 5/6] spapr_pci: fix boot-time device tree fields for pci hotplug

[Nikunj A Dadhania]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

We need to set the proper drc_index values in ibm,my-drc-index
fields in order to allow a PCI device that was present at
boot-time to be unplugged.

Previously SLOF handles this, but with QEMU handling the DT we
need to do it there as well.

This patch slightly changes how SLOF handled it in the past,
which was to allows add an ibm,my-drc-index value based on
PCI slot/devices topology. Now we only add it when the slot
supports hotplug and has a DR connector, which is more inline
with PAPR.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 103284a..cbd5661 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -951,7 +951,9 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
         _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
                          strlen(drc_name)));
     }
-    _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
+    if (drc_index) {
+        _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
+    }
 
     _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
                           RESOURCE_CELLS_ADDRESS));
@@ -1483,6 +1485,20 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
     return PCI_HOST_BRIDGE(dev);
 }
 
+static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
+                                            PCIDevice *pdev)
+{
+    sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
+    sPAPRDRConnectorClass *drck;
+
+    if (!drc) {
+        return 0;
+    }
+
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    return drck->get_index(drc);
+}
+
 typedef struct sPAPRFDT {
     void *fdt;
     int node_off;
@@ -1499,6 +1515,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
     int func = PCI_FUNC(pdev->devfn);
     char nodename[512];
     sPAPRFDT s_fdt;
+    uint32_t drc_index = spapr_phb_get_pci_drc_index(p->sphb, pdev);
 
     if (func) {
         sprintf(nodename, "pci@%d,%d", slot, func);
@@ -1506,7 +1523,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
         sprintf(nodename, "pci@%d", slot);
     }
     offset = fdt_add_subnode(p->fdt, p->node_off, nodename);
-    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, 0, NULL);
+    ret = spapr_populate_pci_child_dt(pdev, p->fdt, offset, p->index, drc_index, NULL);
     g_assert(!ret);
 
     if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
-- 
1.8.3.1