[Qemu-devel] [PATCH] ACPI: call acpi_set_pci_info when only acpi enabled

[Qemu-devel] [PATCH] ACPI: call acpi_set_pci_info when only acpi enabled

[Bruce Rogers]

Commit f0c9d64a exposed an issue with the code order in acpi_setup.
As of that commit, a xenfv machine type guest will no longer start
if using pci passthrough. Re-order the code in that function to
allow acpi_set_pci_info to be called before bailing on the other,
non-related conditions. With this change I can again use pci
passthrough and xenfv together.

Signed-off-by: Bruce Rogers <brogers@suse.com>
---
 hw/i386/acpi-build.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2073108..1ec072f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2834,6 +2834,13 @@ void acpi_setup(void)
     AcpiBuildState *build_state;
     Object *vmgenid_dev;
 
+    if (!acpi_enabled) {
+        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
+        return;
+    }
+
+    acpi_set_pci_info();
+
     if (!pcms->fw_cfg) {
         ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
         return;
@@ -2844,15 +2851,8 @@ void acpi_setup(void)
         return;
     }
 
-    if (!acpi_enabled) {
-        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
-        return;
-    }
-
     build_state = g_malloc0(sizeof *build_state);
 
-    acpi_set_pci_info();
-
     acpi_build_tables_init(&tables);
     acpi_build(&tables, MACHINE(pcms));
 
-- 
1.9.0

Re: [Qemu-devel] [PATCH] ACPI: call acpi_set_pci_info when only acpi enabled

[Igor Mammedov]

On Wed, 26 Apr 2017 13:07:02 -0600
Bruce Rogers <brogers@suse.com> wrote:

> Commit f0c9d64a exposed an issue with the code order in acpi_setup.
> As of that commit, a xenfv machine type guest will no longer start
> if using pci passthrough. Re-order the code in that function to
> allow acpi_set_pci_info to be called before bailing on the other,
> non-related conditions. With this change I can again use pci
> passthrough and xenfv together.
> 
> Signed-off-by: Bruce Rogers <brogers@suse.com>
it doesn't look right,
acpi_set_pci_info() is supposed to affect only ACPI based hotplug

could you elaborate more on what's going on and
what error you see at startup?

> ---
>  hw/i386/acpi-build.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 2073108..1ec072f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2834,6 +2834,13 @@ void acpi_setup(void)
>      AcpiBuildState *build_state;
>      Object *vmgenid_dev;
>  
> +    if (!acpi_enabled) {
> +        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> +        return;
> +    }
> +
> +    acpi_set_pci_info();
> +
>      if (!pcms->fw_cfg) {
>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>          return;
> @@ -2844,15 +2851,8 @@ void acpi_setup(void)
>          return;
>      }
>  
> -    if (!acpi_enabled) {
> -        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> -        return;
> -    }
> -
>      build_state = g_malloc0(sizeof *build_state);
>  
> -    acpi_set_pci_info();
> -
>      acpi_build_tables_init(&tables);
>      acpi_build(&tables, MACHINE(pcms));
>  

Re: [Qemu-devel] [PATCH] ACPI: call acpi_set_pci_info when only acpi enabled

[Bruce Rogers]

>>> On 4/27/2017 at 03:11 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
> On Wed, 26 Apr 2017 13:07:02 -0600
> Bruce Rogers <brogers@suse.com> wrote:
> 
>> Commit f0c9d64a exposed an issue with the code order in acpi_setup.
>> As of that commit, a xenfv machine type guest will no longer start
>> if using pci passthrough. Re-order the code in that function to
>> allow acpi_set_pci_info to be called before bailing on the other,
>> non-related conditions. With this change I can again use pci
>> passthrough and xenfv together.
>> 
>> Signed-off-by: Bruce Rogers <brogers@suse.com>
> it doesn't look right,
> acpi_set_pci_info() is supposed to affect only ACPI based hotplug
> 
> could you elaborate more on what's going on and
> what error you see at startup?

I am using libvirt, driving the creation of the Xen HVM guest via
libxl. libxl dynamically attaches the pci device via QMP. In the
context of qmp_device_add(), we get a failure in hw/acpi/pcihp.c:
acpi_pcihp_device_plug_cb() when it checks for bsel, and errors
with the message: "Unsupported bus. Bus doesn't have property
'acpi-pcihp-bsel' set". I guess it wasn't clear from my description
that hotplug was involved.

Bruce

> 
>> ---
>>  hw/i386/acpi-build.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 2073108..1ec072f 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2834,6 +2834,13 @@ void acpi_setup(void)
>>      AcpiBuildState *build_state;
>>      Object *vmgenid_dev;
>>  
>> +    if (!acpi_enabled) {
>> +        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
>> +        return;
>> +    }
>> +
>> +    acpi_set_pci_info();
>> +
>>      if (!pcms->fw_cfg) {
>>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>>          return;
>> @@ -2844,15 +2851,8 @@ void acpi_setup(void)
>>          return;
>>      }
>>  
>> -    if (!acpi_enabled) {
>> -        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
>> -        return;
>> -    }
>> -
>>      build_state = g_malloc0(sizeof *build_state);
>>  
>> -    acpi_set_pci_info();
>> -
>>      acpi_build_tables_init(&tables);
>>      acpi_build(&tables, MACHINE(pcms));
>>  

Re: [Qemu-devel] [PATCH] ACPI: call acpi_set_pci_info when only acpi enabled

[Igor Mammedov]

On Thu, 27 Apr 2017 09:44:31 -0600
"Bruce Rogers" <brogers@suse.com> wrote:

> >>> On 4/27/2017 at 03:11 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
> > On Wed, 26 Apr 2017 13:07:02 -0600
> > Bruce Rogers <brogers@suse.com> wrote:
> > 
> >> Commit f0c9d64a exposed an issue with the code order in acpi_setup.
> >> As of that commit, a xenfv machine type guest will no longer start
> >> if using pci passthrough. Re-order the code in that function to
> >> allow acpi_set_pci_info to be called before bailing on the other,
> >> non-related conditions. With this change I can again use pci
> >> passthrough and xenfv together.
> >> 
> >> Signed-off-by: Bruce Rogers <brogers@suse.com>
> > it doesn't look right,
> > acpi_set_pci_info() is supposed to affect only ACPI based hotplug
> > 
> > could you elaborate more on what's going on and
> > what error you see at startup?
> 
> I am using libvirt, driving the creation of the Xen HVM guest via
> libxl. libxl dynamically attaches the pci device via QMP. In the
> context of qmp_device_add(), we get a failure in hw/acpi/pcihp.c:
> acpi_pcihp_device_plug_cb() when it checks for bsel, and errors
> with the message: "Unsupported bus. Bus doesn't have property
> 'acpi-pcihp-bsel' set". I guess it wasn't clear from my description
> that hotplug was involved.
> 
is dev->hotplugged in acpi_pcihp_device_plug_cb() true at that time?

the point is that bsel is needed only when there is supporting ACPI code
and useless otherwise, so acpi_pcihp_device_plug_cb() probably shouldn't
run under xenfv. I'd try to add compat prop to PIIX4_PM and disable
acpi_pcihp_device_plug_cb() for xenfv via machine compat property.

> Bruce
> 
> > 
> >> ---
> >>  hw/i386/acpi-build.c | 14 +++++++-------
> >>  1 file changed, 7 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 2073108..1ec072f 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -2834,6 +2834,13 @@ void acpi_setup(void)
> >>      AcpiBuildState *build_state;
> >>      Object *vmgenid_dev;
> >>  
> >> +    if (!acpi_enabled) {
> >> +        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> >> +        return;
> >> +    }
> >> +
> >> +    acpi_set_pci_info();
> >> +
> >>      if (!pcms->fw_cfg) {
> >>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> >>          return;
> >> @@ -2844,15 +2851,8 @@ void acpi_setup(void)
> >>          return;
> >>      }
> >>  
> >> -    if (!acpi_enabled) {
> >> -        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> >> -        return;
> >> -    }
> >> -
> >>      build_state = g_malloc0(sizeof *build_state);
> >>  
> >> -    acpi_set_pci_info();
> >> -
> >>      acpi_build_tables_init(&tables);
> >>      acpi_build(&tables, MACHINE(pcms));
> >>  
> 
> 

Re: [Qemu-devel] [PATCH] ACPI: call acpi_set_pci_info when only acpi enabled

[Bruce Rogers]

>>> On 4/27/2017 at 10:08 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
> On Thu, 27 Apr 2017 09:44:31 -0600
> "Bruce Rogers" <brogers@suse.com> wrote:
> 
>> >>> On 4/27/2017 at 03:11 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
>> > On Wed, 26 Apr 2017 13:07:02 -0600
>> > Bruce Rogers <brogers@suse.com> wrote:
>> > 
>> >> Commit f0c9d64a exposed an issue with the code order in acpi_setup.
>> >> As of that commit, a xenfv machine type guest will no longer start
>> >> if using pci passthrough. Re-order the code in that function to
>> >> allow acpi_set_pci_info to be called before bailing on the other,
>> >> non-related conditions. With this change I can again use pci
>> >> passthrough and xenfv together.
>> >> 
>> >> Signed-off-by: Bruce Rogers <brogers@suse.com>
>> > it doesn't look right,
>> > acpi_set_pci_info() is supposed to affect only ACPI based hotplug
>> > 
>> > could you elaborate more on what's going on and
>> > what error you see at startup?
>> 
>> I am using libvirt, driving the creation of the Xen HVM guest via
>> libxl. libxl dynamically attaches the pci device via QMP. In the
>> context of qmp_device_add(), we get a failure in hw/acpi/pcihp.c:
>> acpi_pcihp_device_plug_cb() when it checks for bsel, and errors
>> with the message: "Unsupported bus. Bus doesn't have property
>> 'acpi-pcihp-bsel' set". I guess it wasn't clear from my description
>> that hotplug was involved.
>> 
> is dev->hotplugged in acpi_pcihp_device_plug_cb() true at that time?

Yes it is. It gets set in hw/core/qdev.c:device_initfn().

> 
> the point is that bsel is needed only when there is supporting ACPI code
> and useless otherwise, so acpi_pcihp_device_plug_cb() probably shouldn't
> run under xenfv. I'd try to add compat prop to PIIX4_PM and disable
> acpi_pcihp_device_plug_cb() for xenfv via machine compat property.

I'll look into this then. I'm not too familiar yet with this area of the code.
I simply bisected the problem, which pointed to the commit id referenced
in the patch, and came up with what seemed like the most obvious solution.

> 
>> Bruce
>> 
>> > 
>> >> ---
>> >>  hw/i386/acpi-build.c | 14 +++++++-------
>> >>  1 file changed, 7 insertions(+), 7 deletions(-)
>> >> 
>> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> >> index 2073108..1ec072f 100644
>> >> --- a/hw/i386/acpi-build.c
>> >> +++ b/hw/i386/acpi-build.c
>> >> @@ -2834,6 +2834,13 @@ void acpi_setup(void)
>> >>      AcpiBuildState *build_state;
>> >>      Object *vmgenid_dev;
>> >>  
>> >> +    if (!acpi_enabled) {
>> >> +        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
>> >> +        return;
>> >> +    }
>> >> +
>> >> +    acpi_set_pci_info();
>> >> +
>> >>      if (!pcms->fw_cfg) {
>> >>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>> >>          return;
>> >> @@ -2844,15 +2851,8 @@ void acpi_setup(void)
>> >>          return;
>> >>      }
>> >>  
>> >> -    if (!acpi_enabled) {
>> >> -        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
>> >> -        return;
>> >> -    }
>> >> -
>> >>      build_state = g_malloc0(sizeof *build_state);
>> >>  
>> >> -    acpi_set_pci_info();
>> >> -
>> >>      acpi_build_tables_init(&tables);
>> >>      acpi_build(&tables, MACHINE(pcms));
>> >>  
>> 
>> 

Re: [Qemu-devel] [PATCH] ACPI: call acpi_set_pci_info when only acpi enabled

[Bruce Rogers]

>>> On 4/27/2017 at 10:08 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
> On Thu, 27 Apr 2017 09:44:31 -0600
> "Bruce Rogers" <brogers@suse.com> wrote:
> 
>> >>> On 4/27/2017 at 03:11 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
>> > On Wed, 26 Apr 2017 13:07:02 -0600
>> > Bruce Rogers <brogers@suse.com> wrote:
>> > 
>> >> Commit f0c9d64a exposed an issue with the code order in acpi_setup.
>> >> As of that commit, a xenfv machine type guest will no longer start
>> >> if using pci passthrough. Re-order the code in that function to
>> >> allow acpi_set_pci_info to be called before bailing on the other,
>> >> non-related conditions. With this change I can again use pci
>> >> passthrough and xenfv together.
>> >> 
>> >> Signed-off-by: Bruce Rogers <brogers@suse.com>
>> > it doesn't look right,
>> > acpi_set_pci_info() is supposed to affect only ACPI based hotplug
>> > 
>> > could you elaborate more on what's going on and
>> > what error you see at startup?
>> 
>> I am using libvirt, driving the creation of the Xen HVM guest via
>> libxl. libxl dynamically attaches the pci device via QMP. In the
>> context of qmp_device_add(), we get a failure in hw/acpi/pcihp.c:
>> acpi_pcihp_device_plug_cb() when it checks for bsel, and errors
>> with the message: "Unsupported bus. Bus doesn't have property
>> 'acpi-pcihp-bsel' set". I guess it wasn't clear from my description
>> that hotplug was involved.
>> 
> is dev->hotplugged in acpi_pcihp_device_plug_cb() true at that time?
> 
> the point is that bsel is needed only when there is supporting ACPI code
> and useless otherwise, so acpi_pcihp_device_plug_cb() probably shouldn't
> run under xenfv. I'd try to add compat prop to PIIX4_PM and disable
> acpi_pcihp_device_plug_cb() for xenfv via machine compat property.

So what would be wrong with simply conditionalizing the call to 
 acpi_pcihp_device_plug_cb() with a check for xen in piix4_device_plug_cb()
as follows:

         }
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-        acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp);
+        if (!xen_enabled()) {
+            acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
+                                      errp);
+        }
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         if (s->cpu_hotplug_legacy) {
             legacy_acpi_cpu_plug_cb(hotplug_dev, &s->gpe_cpu, dev, errp);

Wouldn't that be the solution?

Bruce

Re: [Qemu-devel] [PATCH] ACPI: call acpi_set_pci_info when only acpi enabled

[Igor Mammedov]

On Thu, 27 Apr 2017 10:51:23 -0600
"Bruce Rogers" <brogers@suse.com> wrote:

> 
> 
> >>> On 4/27/2017 at 10:08 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
> > On Thu, 27 Apr 2017 09:44:31 -0600
> > "Bruce Rogers" <brogers@suse.com> wrote:
> > 
> >> >>> On 4/27/2017 at 03:11 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
> >> > On Wed, 26 Apr 2017 13:07:02 -0600
> >> > Bruce Rogers <brogers@suse.com> wrote:
> >> > 
> >> >> Commit f0c9d64a exposed an issue with the code order in acpi_setup.
> >> >> As of that commit, a xenfv machine type guest will no longer start
> >> >> if using pci passthrough. Re-order the code in that function to
> >> >> allow acpi_set_pci_info to be called before bailing on the other,
> >> >> non-related conditions. With this change I can again use pci
> >> >> passthrough and xenfv together.
> >> >> 
> >> >> Signed-off-by: Bruce Rogers <brogers@suse.com>
> >> > it doesn't look right,
> >> > acpi_set_pci_info() is supposed to affect only ACPI based hotplug
> >> > 
> >> > could you elaborate more on what's going on and
> >> > what error you see at startup?
> >> 
> >> I am using libvirt, driving the creation of the Xen HVM guest via
> >> libxl. libxl dynamically attaches the pci device via QMP. In the
> >> context of qmp_device_add(), we get a failure in hw/acpi/pcihp.c:
> >> acpi_pcihp_device_plug_cb() when it checks for bsel, and errors
> >> with the message: "Unsupported bus. Bus doesn't have property
> >> 'acpi-pcihp-bsel' set". I guess it wasn't clear from my description
> >> that hotplug was involved.
> >> 
> > is dev->hotplugged in acpi_pcihp_device_plug_cb() true at that time?
> > 
> > the point is that bsel is needed only when there is supporting ACPI code
> > and useless otherwise, so acpi_pcihp_device_plug_cb() probably shouldn't
> > run under xenfv. I'd try to add compat prop to PIIX4_PM and disable
> > acpi_pcihp_device_plug_cb() for xenfv via machine compat property.
> 
> So what would be wrong with simply conditionalizing the call to 
>  acpi_pcihp_device_plug_cb() with a check for xen in piix4_device_plug_cb()
> as follows:
> 
>          }
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> -        acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp);
> +        if (!xen_enabled()) {
> +            acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> +                                      errp);
> +        }
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          if (s->cpu_hotplug_legacy) {
>              legacy_acpi_cpu_plug_cb(hotplug_dev, &s->gpe_cpu, dev, errp);
> 
> Wouldn't that be the solution?
it should work

is it possible to see unplug on that device later under xen?

> 
> Bruce
> 
> 

Re: [Qemu-devel] [PATCH] ACPI: call acpi_set_pci_info when only acpi enabled

[Stefano Stabellini]

On Thu, 27 Apr 2017, Igor Mammedov wrote:
> On Thu, 27 Apr 2017 10:51:23 -0600
> "Bruce Rogers" <brogers@suse.com> wrote:
> 
> > 
> > 
> > >>> On 4/27/2017 at 10:08 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
> > > On Thu, 27 Apr 2017 09:44:31 -0600
> > > "Bruce Rogers" <brogers@suse.com> wrote:
> > > 
> > >> >>> On 4/27/2017 at 03:11 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
> > >> > On Wed, 26 Apr 2017 13:07:02 -0600
> > >> > Bruce Rogers <brogers@suse.com> wrote:
> > >> > 
> > >> >> Commit f0c9d64a exposed an issue with the code order in acpi_setup.
> > >> >> As of that commit, a xenfv machine type guest will no longer start
> > >> >> if using pci passthrough. Re-order the code in that function to
> > >> >> allow acpi_set_pci_info to be called before bailing on the other,
> > >> >> non-related conditions. With this change I can again use pci
> > >> >> passthrough and xenfv together.
> > >> >> 
> > >> >> Signed-off-by: Bruce Rogers <brogers@suse.com>
> > >> > it doesn't look right,
> > >> > acpi_set_pci_info() is supposed to affect only ACPI based hotplug
> > >> > 
> > >> > could you elaborate more on what's going on and
> > >> > what error you see at startup?
> > >> 
> > >> I am using libvirt, driving the creation of the Xen HVM guest via
> > >> libxl. libxl dynamically attaches the pci device via QMP. In the
> > >> context of qmp_device_add(), we get a failure in hw/acpi/pcihp.c:
> > >> acpi_pcihp_device_plug_cb() when it checks for bsel, and errors
> > >> with the message: "Unsupported bus. Bus doesn't have property
> > >> 'acpi-pcihp-bsel' set". I guess it wasn't clear from my description
> > >> that hotplug was involved.
> > >> 
> > > is dev->hotplugged in acpi_pcihp_device_plug_cb() true at that time?
> > > 
> > > the point is that bsel is needed only when there is supporting ACPI code
> > > and useless otherwise, so acpi_pcihp_device_plug_cb() probably shouldn't
> > > run under xenfv. I'd try to add compat prop to PIIX4_PM and disable
> > > acpi_pcihp_device_plug_cb() for xenfv via machine compat property.
> > 
> > So what would be wrong with simply conditionalizing the call to 
> >  acpi_pcihp_device_plug_cb() with a check for xen in piix4_device_plug_cb()
> > as follows:
> > 
> >          }
> >      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > -        acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp);
> > +        if (!xen_enabled()) {
> > +            acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> > +                                      errp);
> > +        }
> >      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >          if (s->cpu_hotplug_legacy) {
> >              legacy_acpi_cpu_plug_cb(hotplug_dev, &s->gpe_cpu, dev, errp);
> > 
> > Wouldn't that be the solution?
> it should work
> 
> is it possible to see unplug on that device later under xen?

Yes, it would be possible. I guess we need to do the same for
acpi_pcihp_device_unplug_cb?

Re: [Qemu-devel] [PATCH] ACPI: call acpi_set_pci_info when only acpi enabled

[Bruce Rogers]

>>> On 4/27/2017 at 12:08 PM, Stefano Stabellini <sstabellini@kernel.org> wrote: 
> On Thu, 27 Apr 2017, Igor Mammedov wrote:
>> On Thu, 27 Apr 2017 10:51:23 -0600
>> "Bruce Rogers" <brogers@suse.com> wrote:
>> 
>> > 
>> > 
>> > >>> On 4/27/2017 at 10:08 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
>> > > On Thu, 27 Apr 2017 09:44:31 -0600
>> > > "Bruce Rogers" <brogers@suse.com> wrote:
>> > > 
>> > >> >>> On 4/27/2017 at 03:11 AM, Igor Mammedov <imammedo@redhat.com> wrote: 
>> > >> > On Wed, 26 Apr 2017 13:07:02 -0600
>> > >> > Bruce Rogers <brogers@suse.com> wrote:
>> > >> > 
>> > >> >> Commit f0c9d64a exposed an issue with the code order in acpi_setup.
>> > >> >> As of that commit, a xenfv machine type guest will no longer start
>> > >> >> if using pci passthrough. Re-order the code in that function to
>> > >> >> allow acpi_set_pci_info to be called before bailing on the other,
>> > >> >> non-related conditions. With this change I can again use pci
>> > >> >> passthrough and xenfv together.
>> > >> >> 
>> > >> >> Signed-off-by: Bruce Rogers <brogers@suse.com>
>> > >> > it doesn't look right,
>> > >> > acpi_set_pci_info() is supposed to affect only ACPI based hotplug
>> > >> > 
>> > >> > could you elaborate more on what's going on and
>> > >> > what error you see at startup?
>> > >> 
>> > >> I am using libvirt, driving the creation of the Xen HVM guest via
>> > >> libxl. libxl dynamically attaches the pci device via QMP. In the
>> > >> context of qmp_device_add(), we get a failure in hw/acpi/pcihp.c:
>> > >> acpi_pcihp_device_plug_cb() when it checks for bsel, and errors
>> > >> with the message: "Unsupported bus. Bus doesn't have property
>> > >> 'acpi-pcihp-bsel' set". I guess it wasn't clear from my description
>> > >> that hotplug was involved.
>> > >> 
>> > > is dev->hotplugged in acpi_pcihp_device_plug_cb() true at that time?
>> > > 
>> > > the point is that bsel is needed only when there is supporting ACPI code
>> > > and useless otherwise, so acpi_pcihp_device_plug_cb() probably shouldn't
>> > > run under xenfv. I'd try to add compat prop to PIIX4_PM and disable
>> > > acpi_pcihp_device_plug_cb() for xenfv via machine compat property.
>> > 
>> > So what would be wrong with simply conditionalizing the call to 
>> >  acpi_pcihp_device_plug_cb() with a check for xen in piix4_device_plug_cb()
>> > as follows:
>> > 
>> >          }
>> >      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> > -        acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, 
> errp);
>> > +        if (!xen_enabled()) {
>> > +            acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
>> > +                                      errp);
>> > +        }
>> >      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> >          if (s->cpu_hotplug_legacy) {
>> >              legacy_acpi_cpu_plug_cb(hotplug_dev, &s->gpe_cpu, dev, errp);
>> > 
>> > Wouldn't that be the solution?
>> it should work
>> 
>> is it possible to see unplug on that device later under xen?
> 
> Yes, it would be possible. I guess we need to do the same for
> acpi_pcihp_device_unplug_cb?

Thanks guys, I'll send a new patch based on this discussion.

Bruce