[Qemu-devel] [PATCH] hw/arm/virt: Add linux,pci-domain property

[Qemu-devel] [PATCH] hw/arm/virt: Add linux,pci-domain property

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This allows to pin the host controller in the Linux PCI domain space.
Linux requires that property to be available consistently or not at all,
in which case the domain number becomes unstable on additions/removals.
Adding it here won't make a difference in practice for most setups as we
only expose one controller.

However, enabling Jailhouse on top may introduce another controller, and
that one would like to have stable address as well. So the property is
needed for the first controller as well.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 94dcb125d3..943371b75e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1023,6 +1023,7 @@ static void create_pcie(const VirtMachineState *vms, qemu_irq *pic)
     qemu_fdt_setprop_string(vms->fdt, nodename, "device_type", "pci");
     qemu_fdt_setprop_cell(vms->fdt, nodename, "#address-cells", 3);
     qemu_fdt_setprop_cell(vms->fdt, nodename, "#size-cells", 2);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "linux,pci-domain", 0);
     qemu_fdt_setprop_cells(vms->fdt, nodename, "bus-range", 0,
                            nr_pcie_buses - 1);
     qemu_fdt_setprop(vms->fdt, nodename, "dma-coherent", NULL, 0);
-- 
2.13.6

Re: [Qemu-devel] [PATCH] hw/arm/virt: Add linux, pci-domain property

[Peter Maydell]

On 23 April 2018 at 06:18, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> This allows to pin the host controller in the Linux PCI domain space.
> Linux requires that property to be available consistently or not at all,
> in which case the domain number becomes unstable on additions/removals.
> Adding it here won't make a difference in practice for most setups as we
> only expose one controller.
>
> However, enabling Jailhouse on top may introduce another controller, and
> that one would like to have stable address as well. So the property is
> needed for the first controller as well.

Am I right in thinking that for ACPI the PCI domain number is
communicated via the _SEG method? If so, looks like we already
set that, and we set it to 0, which matches what we're doing here
in the DT, so that's good.

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/arm/virt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 94dcb125d3..943371b75e 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1023,6 +1023,7 @@ static void create_pcie(const VirtMachineState *vms, qemu_irq *pic)
>      qemu_fdt_setprop_string(vms->fdt, nodename, "device_type", "pci");
>      qemu_fdt_setprop_cell(vms->fdt, nodename, "#address-cells", 3);
>      qemu_fdt_setprop_cell(vms->fdt, nodename, "#size-cells", 2);
> +    qemu_fdt_setprop_cell(vms->fdt, nodename, "linux,pci-domain", 0);
>      qemu_fdt_setprop_cells(vms->fdt, nodename, "bus-range", 0,
>                             nr_pcie_buses - 1);
>      qemu_fdt_setprop(vms->fdt, nodename, "dma-coherent", NULL, 0);
> --

Drew -- is minor changes to the DTC something we can do without
having to condition it on machine version? I forget...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: Add linux, pci-domain property

[Jan Kiszka]

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

On 2018-04-23 15:11, Peter Maydell wrote:
> On 23 April 2018 at 06:18, Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> This allows to pin the host controller in the Linux PCI domain space.
>> Linux requires that property to be available consistently or not at all,
>> in which case the domain number becomes unstable on additions/removals.
>> Adding it here won't make a difference in practice for most setups as we
>> only expose one controller.
>>
>> However, enabling Jailhouse on top may introduce another controller, and
>> that one would like to have stable address as well. So the property is
>> needed for the first controller as well.
> 
> Am I right in thinking that for ACPI the PCI domain number is
> communicated via the _SEG method? If so, looks like we already
> set that, and we set it to 0, which matches what we're doing here
> in the DT, so that's good.

Looking at the related arm64 kernel code (not the spec), it seems __SEG
is involved, yet.

Jan

> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/arm/virt.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 94dcb125d3..943371b75e 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1023,6 +1023,7 @@ static void create_pcie(const VirtMachineState *vms, qemu_irq *pic)
>>      qemu_fdt_setprop_string(vms->fdt, nodename, "device_type", "pci");
>>      qemu_fdt_setprop_cell(vms->fdt, nodename, "#address-cells", 3);
>>      qemu_fdt_setprop_cell(vms->fdt, nodename, "#size-cells", 2);
>> +    qemu_fdt_setprop_cell(vms->fdt, nodename, "linux,pci-domain", 0);
>>      qemu_fdt_setprop_cells(vms->fdt, nodename, "bus-range", 0,
>>                             nr_pcie_buses - 1);
>>      qemu_fdt_setprop(vms->fdt, nodename, "dma-coherent", NULL, 0);
>> --
> 
> Drew -- is minor changes to the DTC something we can do without
> having to condition it on machine version? I forget...
> 
> thanks
> -- PMM
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH] hw/arm/virt: Add linux, pci-domain property

[Andrew Jones]

On Mon, Apr 23, 2018 at 02:11:40PM +0100, Peter Maydell wrote:
> On 23 April 2018 at 06:18, Jan Kiszka <jan.kiszka@web.de> wrote:
> > From: Jan Kiszka <jan.kiszka@siemens.com>
> >
> > This allows to pin the host controller in the Linux PCI domain space.
> > Linux requires that property to be available consistently or not at all,
> > in which case the domain number becomes unstable on additions/removals.
> > Adding it here won't make a difference in practice for most setups as we
> > only expose one controller.
> >
> > However, enabling Jailhouse on top may introduce another controller, and
> > that one would like to have stable address as well. So the property is
> > needed for the first controller as well.
> 
> Am I right in thinking that for ACPI the PCI domain number is
> communicated via the _SEG method? If so, looks like we already
> set that, and we set it to 0, which matches what we're doing here
> in the DT, so that's good.

_SEG and linux,pci-domain are similar in definition, but don't appear to
be equivalent, as the same _SEG number is permitted across multiple host
bridges, but linux,pci-domain must be unique for each host bridge. I
think the (_SEG, _BBN) pair may form the equivalent. We also set _BBN
to zero, so I think we're fine anyway.

> 
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> >  hw/arm/virt.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 94dcb125d3..943371b75e 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1023,6 +1023,7 @@ static void create_pcie(const VirtMachineState *vms, qemu_irq *pic)
> >      qemu_fdt_setprop_string(vms->fdt, nodename, "device_type", "pci");
> >      qemu_fdt_setprop_cell(vms->fdt, nodename, "#address-cells", 3);
> >      qemu_fdt_setprop_cell(vms->fdt, nodename, "#size-cells", 2);
> > +    qemu_fdt_setprop_cell(vms->fdt, nodename, "linux,pci-domain", 0);
> >      qemu_fdt_setprop_cells(vms->fdt, nodename, "bus-range", 0,
> >                             nr_pcie_buses - 1);
> >      qemu_fdt_setprop(vms->fdt, nodename, "dma-coherent", NULL, 0);
> > --
> 
> Drew -- is minor changes to the DTC something we can do without
> having to condition it on machine version? I forget...

Yes, and even less minor changes than this one can be made to ACPI and DT
generation, per the "if a firwmare update could change it, then don't
worry about it" policy.

(I've CC'ed Igor in case he wants to chime in to correct anything I've
 said.)

Thanks,
drew

Re: [Qemu-devel] [PATCH] hw/arm/virt: Add linux, pci-domain property

[Andrew Jones]

On Tue, Apr 24, 2018 at 04:12:58PM +0200, Andrew Jones wrote:
> On Mon, Apr 23, 2018 at 02:11:40PM +0100, Peter Maydell wrote:
> > On 23 April 2018 at 06:18, Jan Kiszka <jan.kiszka@web.de> wrote:
> > > From: Jan Kiszka <jan.kiszka@siemens.com>
> > >
> > > This allows to pin the host controller in the Linux PCI domain space.
> > > Linux requires that property to be available consistently or not at all,
> > > in which case the domain number becomes unstable on additions/removals.
> > > Adding it here won't make a difference in practice for most setups as we
> > > only expose one controller.
> > >
> > > However, enabling Jailhouse on top may introduce another controller, and
> > > that one would like to have stable address as well. So the property is
> > > needed for the first controller as well.
> > 
> > Am I right in thinking that for ACPI the PCI domain number is
> > communicated via the _SEG method? If so, looks like we already
> > set that, and we set it to 0, which matches what we're doing here
> > in the DT, so that's good.
> 
> _SEG and linux,pci-domain are similar in definition, but don't appear to
> be equivalent, as the same _SEG number is permitted across multiple host
> bridges, but linux,pci-domain must be unique for each host bridge. I
> think the (_SEG, _BBN) pair may form the equivalent. We also set _BBN
> to zero, so I think we're fine anyway.
> 
> > 
> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > ---
> > >  hw/arm/virt.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index 94dcb125d3..943371b75e 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -1023,6 +1023,7 @@ static void create_pcie(const VirtMachineState *vms, qemu_irq *pic)
> > >      qemu_fdt_setprop_string(vms->fdt, nodename, "device_type", "pci");
> > >      qemu_fdt_setprop_cell(vms->fdt, nodename, "#address-cells", 3);
> > >      qemu_fdt_setprop_cell(vms->fdt, nodename, "#size-cells", 2);
> > > +    qemu_fdt_setprop_cell(vms->fdt, nodename, "linux,pci-domain", 0);
> > >      qemu_fdt_setprop_cells(vms->fdt, nodename, "bus-range", 0,
> > >                             nr_pcie_buses - 1);
> > >      qemu_fdt_setprop(vms->fdt, nodename, "dma-coherent", NULL, 0);
> > > --
> > 
> > Drew -- is minor changes to the DTC something we can do without
> > having to condition it on machine version? I forget...
> 
> Yes, and even less minor changes than this one can be made to ACPI and DT
> generation, per the "if a firwmare update could change it, then don't
> worry about it" policy.
> 
> (I've CC'ed Igor in case he wants to chime in to correct anything I've
>  said.)

Eh, now I've CC'ed him...

Re: [Qemu-devel] [PATCH] hw/arm/virt: Add linux, pci-domain property

[Peter Maydell]

On 24 April 2018 at 15:12, Andrew Jones <drjones@redhat.com> wrote:
> On Mon, Apr 23, 2018 at 02:11:40PM +0100, Peter Maydell wrote:
>> On 23 April 2018 at 06:18, Jan Kiszka <jan.kiszka@web.de> wrote:
>> > From: Jan Kiszka <jan.kiszka@siemens.com>
>> >
>> > This allows to pin the host controller in the Linux PCI domain space.
>> > Linux requires that property to be available consistently or not at all,
>> > in which case the domain number becomes unstable on additions/removals.
>> > Adding it here won't make a difference in practice for most setups as we
>> > only expose one controller.
>> >
>> > However, enabling Jailhouse on top may introduce another controller, and
>> > that one would like to have stable address as well. So the property is
>> > needed for the first controller as well.
>>
>> Am I right in thinking that for ACPI the PCI domain number is
>> communicated via the _SEG method? If so, looks like we already
>> set that, and we set it to 0, which matches what we're doing here
>> in the DT, so that's good.
>
> _SEG and linux,pci-domain are similar in definition, but don't appear to
> be equivalent, as the same _SEG number is permitted across multiple host
> bridges, but linux,pci-domain must be unique for each host bridge. I
> think the (_SEG, _BBN) pair may form the equivalent. We also set _BBN
> to zero, so I think we're fine anyway.
>
>>
>> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> > ---
>> >  hw/arm/virt.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> > index 94dcb125d3..943371b75e 100644
>> > --- a/hw/arm/virt.c
>> > +++ b/hw/arm/virt.c
>> > @@ -1023,6 +1023,7 @@ static void create_pcie(const VirtMachineState *vms, qemu_irq *pic)
>> >      qemu_fdt_setprop_string(vms->fdt, nodename, "device_type", "pci");
>> >      qemu_fdt_setprop_cell(vms->fdt, nodename, "#address-cells", 3);
>> >      qemu_fdt_setprop_cell(vms->fdt, nodename, "#size-cells", 2);
>> > +    qemu_fdt_setprop_cell(vms->fdt, nodename, "linux,pci-domain", 0);
>> >      qemu_fdt_setprop_cells(vms->fdt, nodename, "bus-range", 0,
>> >                             nr_pcie_buses - 1);
>> >      qemu_fdt_setprop(vms->fdt, nodename, "dma-coherent", NULL, 0);
>> > --
>>
>> Drew -- is minor changes to the DTC something we can do without
>> having to condition it on machine version? I forget...
>
> Yes, and even less minor changes than this one can be made to ACPI and DT
> generation, per the "if a firwmare update could change it, then don't
> worry about it" policy.

Cool; applied to target-arm.next, then.

-- PMM