[Qemu-devel] [PATCH] hw/pxb-pcie: fix PCI Express hotplug support

[Qemu-devel] [PATCH] hw/pxb-pcie: fix PCI Express hotplug support

[Marcel Apfelbaum]

Add the missing osc method for pxb-pcie devices

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---

Note: The patch is based on the fact that Q35's osc method is quite generic.

Thanks,
Marcel

 hw/i386/acpi-build.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1c928ab..555aab3 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1964,6 +1964,11 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
             aml_append(dev, aml_name_decl("_UID", aml_int(bus_num)));
             aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
             aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
+            if (pci_bus_is_express(bus)) {
+                aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
+                aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
+                aml_append(dev, build_q35_osc_method());
+            }
 
             if (numa_node != NUMA_NODE_UNASSIGNED) {
                 aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));
-- 
2.5.5

Re: [Qemu-devel] [PATCH] hw/pxb-pcie: fix PCI Express hotplug support

[Michael S. Tsirkin]

On Fri, Feb 03, 2017 at 12:05:52AM +0200, Marcel Apfelbaum wrote:
> Add the missing osc method for pxb-pcie devices
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> 
> Note: The patch is based on the fact that Q35's osc method is quite generic.
> 
> Thanks,
> Marcel
> 
>  hw/i386/acpi-build.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1c928ab..555aab3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1964,6 +1964,11 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              aml_append(dev, aml_name_decl("_UID", aml_int(bus_num)));
>              aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
>              aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
> +            if (pci_bus_is_express(bus)) {
> +                aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
> +                aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
> +                aml_append(dev, build_q35_osc_method());
> +            }

I think we want to move this stuff into build_q35_osc_method:
"SUPP" seems to be 0, CTRL should be a variable passed in.


>              if (numa_node != NUMA_NODE_UNASSIGNED) {
>                  aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));


In fact build_q35_osc_method needs to be cleaned up.
It stores result in "CTRL" which should be documented.

"SUPP" seems to be unused.

A bunch of comments missing.

More importantly, its an unserialized method but it creates a bunch of
fields so attempts to run two of these in parallel will crash.
I see the same in ACPI spec which probably means it should be
revisited with a critical eye.


> -- 
> 2.5.5

Re: [Qemu-devel] [PATCH] hw/pxb-pcie: fix PCI Express hotplug support

[Marcel Apfelbaum]

On 02/03/2017 12:34 AM, Michael S. Tsirkin wrote:
> On Fri, Feb 03, 2017 at 12:05:52AM +0200, Marcel Apfelbaum wrote:
>> Add the missing osc method for pxb-pcie devices
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>
>> Note: The patch is based on the fact that Q35's osc method is quite generic.
>>
>> Thanks,
>> Marcel
>>
>>  hw/i386/acpi-build.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 1c928ab..555aab3 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -1964,6 +1964,11 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>>              aml_append(dev, aml_name_decl("_UID", aml_int(bus_num)));
>>              aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
>>              aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
>> +            if (pci_bus_is_express(bus)) {
>> +                aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
>> +                aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
>> +                aml_append(dev, build_q35_osc_method());
>> +            }
>

Hi Michael,
Thanks for the review.

> I think we want to move this stuff into build_q35_osc_method:
> "SUPP" seems to be 0, CTRL should be a variable passed in.
>

Sure

>
>>              if (numa_node != NUMA_NODE_UNASSIGNED) {
>>                  aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));
>
>
> In fact build_q35_osc_method needs to be cleaned up.
> It stores result in "CTRL" which should be documented.
>
> "SUPP" seems to be unused.
>
> A bunch of comments missing.
>
> More importantly, its an unserialized method but it creates a bunch of
> fields so attempts to run two of these in parallel will crash.

I don't see how this code will run in parallel. (the code that calls the _OSC method)

> I see the same in ACPI spec which probably means it should be
> revisited with a critical eye.
>

I am in PTO next week, then I'll revisit the build_q35_osc_method and (try to) clean it up.

Thanks,
Marcel

>
>> --
>> 2.5.5

Re: [Qemu-devel] [PATCH] hw/pxb-pcie: fix PCI Express hotplug support

[Michael S. Tsirkin]

On Fri, Feb 03, 2017 at 12:44:17AM +0200, Marcel Apfelbaum wrote:
> On 02/03/2017 12:34 AM, Michael S. Tsirkin wrote:
> > On Fri, Feb 03, 2017 at 12:05:52AM +0200, Marcel Apfelbaum wrote:
> > > Add the missing osc method for pxb-pcie devices
> > > 
> > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > ---
> > > 
> > > Note: The patch is based on the fact that Q35's osc method is quite generic.
> > > 
> > > Thanks,
> > > Marcel
> > > 
> > >  hw/i386/acpi-build.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 1c928ab..555aab3 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1964,6 +1964,11 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >              aml_append(dev, aml_name_decl("_UID", aml_int(bus_num)));
> > >              aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > >              aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
> > > +            if (pci_bus_is_express(bus)) {
> > > +                aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
> > > +                aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
> > > +                aml_append(dev, build_q35_osc_method());
> > > +            }
> > 
> 
> Hi Michael,
> Thanks for the review.
> 
> > I think we want to move this stuff into build_q35_osc_method:
> > "SUPP" seems to be 0, CTRL should be a variable passed in.
> > 
> 
> Sure
> 
> > 
> > >              if (numa_node != NUMA_NODE_UNASSIGNED) {
> > >                  aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));
> > 
> > 
> > In fact build_q35_osc_method needs to be cleaned up.
> > It stores result in "CTRL" which should be documented.
> > 
> > "SUPP" seems to be unused.
> > 
> > A bunch of comments missing.
> > 
> > More importantly, its an unserialized method but it creates a bunch of
> > fields so attempts to run two of these in parallel will crash.
> 
> I don't see how this code will run in parallel. (the code that calls the _OSC method)

If the method is not serialized, it should be callable in parallel.
I'm guessing OSPMs do not do it for _OSC.

> > I see the same in ACPI spec which probably means it should be
> > revisited with a critical eye.
> > 
> 
> I am in PTO next week, then I'll revisit the build_q35_osc_method and (try to) clean it up.
> 
> Thanks,
> Marcel
> 
> > 
> > > --
> > > 2.5.5