[PATCH 0/4] ICH9 root PCI hotplug

[PATCH 0/4] ICH9 root PCI hotplug

[Thierry Escande]

Hi,

This series fixes acpi_hotplug_bridge accessor names, adds new accessors
for acpi-root-pci-hotplug property, and enables root PCI hotplug by
default for Q35 machine.

Thierry Escande (4):
  ich9: Remove unused hotplug field from ICH9LPCPMRegs struct
  ich9: Renamed use_acpi_hotplug_bridge accessors
  ich9: Add accessors for acpi-root-pci-hotplug
  ich9: Enable root PCI hotplug by default

 hw/acpi/ich9.c         | 23 +++++++++++++++++++++--
 include/hw/acpi/ich9.h |  1 -
 2 files changed, 21 insertions(+), 3 deletions(-)

-- 
2.42.0



Thierry Escande | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH 2/4] ich9: Renamed use_acpi_hotplug_bridge accessors

[Thierry Escande]

The accessors were named as ich9_pm_get/set_acpi_pci_hotplug(). This
patch renames them as ich9_pm_get/set_acpi_hotplug_bridge().

Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
---
 hw/acpi/ich9.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 25e2c7243e..e21732b547 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -398,14 +398,15 @@ static void ich9_pm_set_enable_tco(Object *obj, bool value, Error **errp)
     s->pm.enable_tco = value;
 }
 
-static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
+static bool ich9_pm_get_acpi_hotplug_bridge(Object *obj, Error **errp)
 {
     ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
 
     return s->pm.acpi_pci_hotplug.use_acpi_hotplug_bridge;
 }
 
-static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value, Error **errp)
+static void ich9_pm_set_acpi_hotplug_bridge(Object *obj, bool value,
+                                            Error **errp)
 {
     ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
 
@@ -461,8 +462,8 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
                              ich9_pm_get_enable_tco,
                              ich9_pm_set_enable_tco);
     object_property_add_bool(obj, ACPI_PM_PROP_ACPI_PCIHP_BRIDGE,
-                             ich9_pm_get_acpi_pci_hotplug,
-                             ich9_pm_set_acpi_pci_hotplug);
+                             ich9_pm_get_acpi_hotplug_bridge,
+                             ich9_pm_set_acpi_hotplug_bridge);
     object_property_add_bool(obj, "x-keep-pci-slot-hpc",
                              ich9_pm_get_keep_pci_slot_hpc,
                              ich9_pm_set_keep_pci_slot_hpc);
-- 
2.42.0



Thierry Escande | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH 1/4] ich9: Remove unused hotplug field from ICH9LPCPMRegs struct

[Thierry Escande]

This patch removes the unused field use_acpi_hotplug_bridge from the
ICH9LPCPMRegs structure as it is now part of AcpiPciHpState.

Hotplug fields have been removed from piix4 state structure by commit
6536e427ce49 (pcihp: move fields enabling hotplug into AcpiPciHpState)
so do the same for ich9.

Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
---
 include/hw/acpi/ich9.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 2faf7f0cae..4bc9aa1d11 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -57,7 +57,6 @@ typedef struct ICH9LPCPMRegs {
     CPUHotplugState cpuhp_state;
 
     bool keep_pci_slot_hpc;
-    bool use_acpi_hotplug_bridge;
     AcpiPciHpState acpi_pci_hotplug;
     MemHotplugState acpi_memory_hotplug;
 
-- 
2.42.0



Thierry Escande | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH 1/4] ich9: Remove unused hotplug field from ICH9LPCPMRegs struct

[Philippe Mathieu-Daudé]

On 15/11/23 18:18, Thierry Escande wrote:
> This patch removes the unused field use_acpi_hotplug_bridge from the
> ICH9LPCPMRegs structure as it is now part of AcpiPciHpState.
> 
> Hotplug fields have been removed from piix4 state structure by commit
> 6536e427ce49 (pcihp: move fields enabling hotplug into AcpiPciHpState)
> so do the same for ich9.
> 
> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
> ---
>   include/hw/acpi/ich9.h | 1 -
>   1 file changed, 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/4] ich9: Remove unused hotplug field from ICH9LPCPMRegs struct

[Igor Mammedov]

On Wed, 15 Nov 2023 17:18:53 +0000
Thierry Escande <thierry.escande@vates.tech> wrote:

> This patch removes the unused field use_acpi_hotplug_bridge from the
> ICH9LPCPMRegs structure as it is now part of AcpiPciHpState.
> 
> Hotplug fields have been removed from piix4 state structure by commit
> 6536e427ce49 (pcihp: move fields enabling hotplug into AcpiPciHpState)
> so do the same for ich9.
> 
> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>

Reviewed-by:Igor Mammedov <imammedo@redhat.com>

> ---
>  include/hw/acpi/ich9.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 2faf7f0cae..4bc9aa1d11 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -57,7 +57,6 @@ typedef struct ICH9LPCPMRegs {
>      CPUHotplugState cpuhp_state;
>  
>      bool keep_pci_slot_hpc;
> -    bool use_acpi_hotplug_bridge;
>      AcpiPciHpState acpi_pci_hotplug;
>      MemHotplugState acpi_memory_hotplug;
>  

[PATCH 3/4] ich9: Add accessors for acpi-root-pci-hotplug

[Thierry Escande]

This patch adds accessors for acpi-root-pci-hotplug property. PCI
hotplug can be enabled/disabled with:
  -global ICH9-LPC.acpi-root-pci-hotplug=on|off

Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
---
 hw/acpi/ich9.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index e21732b547..e38c9bb998 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -413,6 +413,20 @@ static void ich9_pm_set_acpi_hotplug_bridge(Object *obj, bool value,
     s->pm.acpi_pci_hotplug.use_acpi_hotplug_bridge = value;
 }
 
+static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
+{
+    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
+
+    return s->pm.acpi_pci_hotplug.use_acpi_root_pci_hotplug;
+}
+
+static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value, Error **errp)
+{
+    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
+
+    s->pm.acpi_pci_hotplug.use_acpi_root_pci_hotplug = value;
+}
+
 static bool ich9_pm_get_keep_pci_slot_hpc(Object *obj, Error **errp)
 {
     ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
@@ -464,6 +478,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
     object_property_add_bool(obj, ACPI_PM_PROP_ACPI_PCIHP_BRIDGE,
                              ich9_pm_get_acpi_hotplug_bridge,
                              ich9_pm_set_acpi_hotplug_bridge);
+    object_property_add_bool(obj, ACPI_PM_PROP_ACPI_PCI_ROOTHP,
+                             ich9_pm_get_acpi_pci_hotplug,
+                             ich9_pm_set_acpi_pci_hotplug);
     object_property_add_bool(obj, "x-keep-pci-slot-hpc",
                              ich9_pm_get_keep_pci_slot_hpc,
                              ich9_pm_set_keep_pci_slot_hpc);
-- 
2.42.0



Thierry Escande | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH 4/4] ich9: Enable root PCI hotplug by default

[Thierry Escande]

This patch initializes use_acpi_root_pci_hotplug to true and enables
device PCI hotplug on q35 machine by default.

Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
---
 hw/acpi/ich9.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index e38c9bb998..ad2c8ad8ed 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -450,6 +450,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
     pm->disable_s4 = 0;
     pm->s4_val = 2;
     pm->acpi_pci_hotplug.use_acpi_hotplug_bridge = true;
+    pm->acpi_pci_hotplug.use_acpi_root_pci_hotplug = true;
     pm->keep_pci_slot_hpc = true;
     pm->enable_tco = true;
 
-- 
2.42.0



Thierry Escande | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH 4/4] ich9: Enable root PCI hotplug by default

[Igor Mammedov]

On Wed, 15 Nov 2023 17:18:54 +0000
Thierry Escande <thierry.escande@vates.tech> wrote:

> This patch initializes use_acpi_root_pci_hotplug to true and enables
> device PCI hotplug on q35 machine by default.
> 
> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
> ---
>  hw/acpi/ich9.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index e38c9bb998..ad2c8ad8ed 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -450,6 +450,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
>      pm->disable_s4 = 0;
>      pm->s4_val = 2;
>      pm->acpi_pci_hotplug.use_acpi_hotplug_bridge = true;
> +    pm->acpi_pci_hotplug.use_acpi_root_pci_hotplug = true;
>      pm->keep_pci_slot_hpc = true;
>      pm->enable_tco = true;
>  

That probably is not enough,
due to:
        if (pm->pcihp_bridge_en) {                                               
            build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);                 
        }                                                                        
        build_q35_pci0_int(dsdt);  

Re: [PATCH 4/4] ich9: Enable root PCI hotplug by default

[Igor Mammedov]

On Wed, 15 Nov 2023 17:18:54 +0000
Thierry Escande <thierry.escande@vates.tech> wrote:

> This patch initializes use_acpi_root_pci_hotplug to true and enables
> device PCI hotplug on q35 machine by default.
> 
> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
> ---
>  hw/acpi/ich9.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index e38c9bb998..ad2c8ad8ed 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -450,6 +450,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
>      pm->disable_s4 = 0;
>      pm->s4_val = 2;
>      pm->acpi_pci_hotplug.use_acpi_hotplug_bridge = true;
> +    pm->acpi_pci_hotplug.use_acpi_root_pci_hotplug = true;
>      pm->keep_pci_slot_hpc = true;
>      pm->enable_tco = true;
>  

PS: also you'd need to add compat logic to preserve
disabled state for already existing machine types

Re: [PATCH 0/4] ICH9 root PCI hotplug

[Igor Mammedov]

On Wed, 15 Nov 2023 17:18:53 +0000
Thierry Escande <thierry.escande@vates.tech> wrote:

> Hi,
> 
> This series fixes acpi_hotplug_bridge accessor names, adds new accessors
> for acpi-root-pci-hotplug property, and enables root PCI hotplug by
> default for Q35 machine.

hotplug on Q35 hostbridge is not implemented intentionally
to keep machine close to the real world.

PCIe spec 3.1a, 1.3.2.3. Root Complex Integrated Endpoint Rules
"
A Root Complex Integrated Endpoint may not be hot-plugged independent of the Root
Complex as a whole.
"
)

PS:
but patch 1/4 is good cleanup, pls include Reviewed-by's and resend it
as a separate patch after 8.2 has been released (so it wouldn't get lost in the traffic).

> 
> Thierry Escande (4):
>   ich9: Remove unused hotplug field from ICH9LPCPMRegs struct
>   ich9: Renamed use_acpi_hotplug_bridge accessors
>   ich9: Add accessors for acpi-root-pci-hotplug
>   ich9: Enable root PCI hotplug by default
> 
>  hw/acpi/ich9.c         | 23 +++++++++++++++++++++--
>  include/hw/acpi/ich9.h |  1 -
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 

Re: [PATCH 0/4] ICH9 root PCI hotplug

[Michael S. Tsirkin]

On Fri, Nov 24, 2023 at 03:01:35PM +0100, Igor Mammedov wrote:
> On Wed, 15 Nov 2023 17:18:53 +0000
> Thierry Escande <thierry.escande@vates.tech> wrote:
> 
> > Hi,
> > 
> > This series fixes acpi_hotplug_bridge accessor names, adds new accessors
> > for acpi-root-pci-hotplug property, and enables root PCI hotplug by
> > default for Q35 machine.
> 
> hotplug on Q35 hostbridge is not implemented intentionally
> to keep machine close to the real world.

> PCIe spec 3.1a, 1.3.2.3. Root Complex Integrated Endpoint Rules
> "
> A Root Complex Integrated Endpoint may not be hot-plugged independent of the Root
> Complex as a whole.
> "
> )

To be more precise close to native hotplug.
But we used ACPI for several years now and it seems to be fine.
Maybe it's time we lifted the limitation?


> 
> PS:
> but patch 1/4 is good cleanup, pls include Reviewed-by's and resend it
> as a separate patch after 8.2 has been released (so it wouldn't get lost in the traffic).
> 
> > 
> > Thierry Escande (4):
> >   ich9: Remove unused hotplug field from ICH9LPCPMRegs struct
> >   ich9: Renamed use_acpi_hotplug_bridge accessors
> >   ich9: Add accessors for acpi-root-pci-hotplug
> >   ich9: Enable root PCI hotplug by default
> > 
> >  hw/acpi/ich9.c         | 23 +++++++++++++++++++++--
> >  include/hw/acpi/ich9.h |  1 -
> >  2 files changed, 21 insertions(+), 3 deletions(-)
> > 

Re: [PATCH 0/4] ICH9 root PCI hotplug

[Thierry Escande]

On 24/11/2023 15:54, Michael S. Tsirkin wrote:
> On Fri, Nov 24, 2023 at 03:01:35PM +0100, Igor Mammedov wrote:
>> On Wed, 15 Nov 2023 17:18:53 +0000
>> Thierry Escande <thierry.escande@vates.tech> wrote:
>>
>>> Hi,
>>>
>>> This series fixes acpi_hotplug_bridge accessor names, adds new accessors
>>> for acpi-root-pci-hotplug property, and enables root PCI hotplug by
>>> default for Q35 machine.
>>
>> hotplug on Q35 hostbridge is not implemented intentionally
>> to keep machine close to the real world.
> 
>> PCIe spec 3.1a, 1.3.2.3. Root Complex Integrated Endpoint Rules
>> "
>> A Root Complex Integrated Endpoint may not be hot-plugged independent of the Root
>> Complex as a whole.
>> "
>> )
> 
> To be more precise close to native hotplug.
> But we used ACPI for several years now and it seems to be fine.
> Maybe it's time we lifted the limitation?

And for what it's worth, lifting this limitation would allow PCIe 
devices passthrough on Q35 VMs with Xen as such devices are hotplugged.

> 
> 
>>
>> PS:
>> but patch 1/4 is good cleanup, pls include Reviewed-by's and resend it
>> as a separate patch after 8.2 has been released (so it wouldn't get lost in the traffic).
>>
>>>
>>> Thierry Escande (4):
>>>    ich9: Remove unused hotplug field from ICH9LPCPMRegs struct
>>>    ich9: Renamed use_acpi_hotplug_bridge accessors
>>>    ich9: Add accessors for acpi-root-pci-hotplug
>>>    ich9: Enable root PCI hotplug by default
>>>
>>>   hw/acpi/ich9.c         | 23 +++++++++++++++++++++--
>>>   include/hw/acpi/ich9.h |  1 -
>>>   2 files changed, 21 insertions(+), 3 deletions(-)
>>>
>

Re: [PATCH 0/4] ICH9 root PCI hotplug

[Michael S. Tsirkin]

On Wed, Nov 29, 2023 at 09:47:13AM +0000, Thierry Escande wrote:
> On 24/11/2023 15:54, Michael S. Tsirkin wrote:
> > On Fri, Nov 24, 2023 at 03:01:35PM +0100, Igor Mammedov wrote:
> > > On Wed, 15 Nov 2023 17:18:53 +0000
> > > Thierry Escande <thierry.escande@vates.tech> wrote:
> > > 
> > > > Hi,
> > > > 
> > > > This series fixes acpi_hotplug_bridge accessor names, adds new accessors
> > > > for acpi-root-pci-hotplug property, and enables root PCI hotplug by
> > > > default for Q35 machine.
> > > 
> > > hotplug on Q35 hostbridge is not implemented intentionally
> > > to keep machine close to the real world.
> > 
> > > PCIe spec 3.1a, 1.3.2.3. Root Complex Integrated Endpoint Rules
> > > "
> > > A Root Complex Integrated Endpoint may not be hot-plugged independent of the Root
> > > Complex as a whole.
> > > "
> > > )
> > 
> > To be more precise close to native hotplug.
> > But we used ACPI for several years now and it seems to be fine.
> > Maybe it's time we lifted the limitation?
> 
> And for what it's worth, lifting this limitation would allow PCIe devices
> passthrough on Q35 VMs with Xen as such devices are hotplugged.


More motivation in the commit log would be a good idea here.

> > 
> > 
> > > 
> > > PS:
> > > but patch 1/4 is good cleanup, pls include Reviewed-by's and resend it
> > > as a separate patch after 8.2 has been released (so it wouldn't get lost in the traffic).
> > > 
> > > > 
> > > > Thierry Escande (4):
> > > >    ich9: Remove unused hotplug field from ICH9LPCPMRegs struct
> > > >    ich9: Renamed use_acpi_hotplug_bridge accessors
> > > >    ich9: Add accessors for acpi-root-pci-hotplug
> > > >    ich9: Enable root PCI hotplug by default
> > > > 
> > > >   hw/acpi/ich9.c         | 23 +++++++++++++++++++++--
> > > >   include/hw/acpi/ich9.h |  1 -
> > > >   2 files changed, 21 insertions(+), 3 deletions(-)
> > > > 
> > 

Re: [PATCH 0/4] ICH9 root PCI hotplug

[Igor Mammedov]

On Wed, 29 Nov 2023 09:47:13 +0000
Thierry Escande <thierry.escande@vates.tech> wrote:

> On 24/11/2023 15:54, Michael S. Tsirkin wrote:
> > On Fri, Nov 24, 2023 at 03:01:35PM +0100, Igor Mammedov wrote:  
> >> On Wed, 15 Nov 2023 17:18:53 +0000
> >> Thierry Escande <thierry.escande@vates.tech> wrote:
> >>  
> >>> Hi,
> >>>
> >>> This series fixes acpi_hotplug_bridge accessor names, adds new accessors
> >>> for acpi-root-pci-hotplug property, and enables root PCI hotplug by
> >>> default for Q35 machine.  
> >>
> >> hotplug on Q35 hostbridge is not implemented intentionally
> >> to keep machine close to the real world.  
> >   
> >> PCIe spec 3.1a, 1.3.2.3. Root Complex Integrated Endpoint Rules
> >> "
> >> A Root Complex Integrated Endpoint may not be hot-plugged independent of the Root
> >> Complex as a whole.
> >> "
> >> )  
> > 
> > To be more precise close to native hotplug.
> > But we used ACPI for several years now and it seems to be fine.
> > Maybe it's time we lifted the limitation?  

Pardon me for being conservative, but adding ACPI PCI hotplug to
PCIe hostbridge complicates its maintenance not to mention that
on guest side it likely to cause issues as well (since it's
architecturally not possible).

I've grudgingly accepted ACPI hotplug on Q35 root-ports/bridges
(since it possible for them to support hotplug in principle
(aka guest OSes account somewhat for that)), and even that wasn't
smooth ride despite of it working on PC machine for ages.
I'd expect more issues to pop up in case of PCIe hostbridge.

This why I dislike the idea very much.

For those who are desperate there is PC machine which supports
hotplug to root bus. For Q35 one can use root-ports for that.

Anyways, it's up to you.

> And for what it's worth, lifting this limitation would allow PCIe 
> devices passthrough on Q35 VMs with Xen as such devices are hotplugged.

maybe ask Xen folks to fix their impl. and make it work with root-ports
like the rest of the world or at least provide a very good justification
why they aren't able to that.

   
> >>
> >> PS:
> >> but patch 1/4 is good cleanup, pls include Reviewed-by's and resend it
> >> as a separate patch after 8.2 has been released (so it wouldn't get lost in the traffic).
> >>  
> >>>
> >>> Thierry Escande (4):
> >>>    ich9: Remove unused hotplug field from ICH9LPCPMRegs struct
> >>>    ich9: Renamed use_acpi_hotplug_bridge accessors
> >>>    ich9: Add accessors for acpi-root-pci-hotplug
> >>>    ich9: Enable root PCI hotplug by default
> >>>
> >>>   hw/acpi/ich9.c         | 23 +++++++++++++++++++++--
> >>>   include/hw/acpi/ich9.h |  1 -
> >>>   2 files changed, 21 insertions(+), 3 deletions(-)
> >>>  
> >  
>