diff mbox

Re: [PATCH 2/3] piix: tag as not hotpluggable.

Message ID 20110105194406.GC28688@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Jan. 5, 2011, 7:44 p.m. UTC 
On Tue, Nov 30, 2010 at 02:26:03PM +0100, Gerd Hoffmann wrote:
> This patch tags all pci devices which belong to the piix3/4 chipsets as
> not hotpluggable (Host bridge, ISA bridge, IDE controller, ACPI bridge).
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/acpi_piix4.c |    2 ++
>  hw/ide/piix.c   |    2 ++
>  hw/piix4.c      |    1 +
>  hw/piix_pci.c   |    2 ++
>  4 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 173d781..273097d 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -428,6 +428,8 @@ static PCIDeviceInfo piix4_pm_info = {
>      .qdev.desc          = "PM",
>      .qdev.size          = sizeof(PIIX4PMState),
>      .qdev.vmsd          = &vmstate_acpi,
> +    .qdev.no_user       = 1,
> +    .no_hotplug         = 1,
>      .init               = piix4_pm_initfn,
>      .config_write       = pm_write_config,
>      .qdev.props         = (Property[]) {
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 07483e8..173ba4b 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -184,11 +184,13 @@ static PCIDeviceInfo piix_ide_info[] = {
>          .qdev.name    = "piix3-ide",
>          .qdev.size    = sizeof(PCIIDEState),
>          .qdev.no_user = 1,
> +        .no_hotplug   = 1,
>          .init         = pci_piix3_ide_initfn,
>      },{
>          .qdev.name    = "piix4-ide",
>          .qdev.size    = sizeof(PCIIDEState),
>          .qdev.no_user = 1,
> +        .no_hotplug   = 1,
>          .init         = pci_piix4_ide_initfn,
>      },{
>          /* end of list */
> diff --git a/hw/piix4.c b/hw/piix4.c
> index 5489386..1678898 100644
> --- a/hw/piix4.c
> +++ b/hw/piix4.c
> @@ -113,6 +113,7 @@ static PCIDeviceInfo piix4_info[] = {
>          .qdev.desc    = "ISA bridge",
>          .qdev.size    = sizeof(PCIDevice),
>          .qdev.no_user = 1,
> +        .qdev.no_hotplug = 1,
>          .init         = piix4_initfn,
>      },{
>          /* end of list */

This one breaks the build for me.  The below seems to help - but begs
the question: was this tested?

Thanks,

Comments

Gerd Hoffmann Jan. 6, 2011, 8:40 a.m. UTC | #1 
>> diff --git a/hw/piix4.c b/hw/piix4.c
>> index 5489386..1678898 100644
>> --- a/hw/piix4.c
>> +++ b/hw/piix4.c
>> @@ -113,6 +113,7 @@ static PCIDeviceInfo piix4_info[] = {
>>           .qdev.desc    = "ISA bridge",
>>           .qdev.size    = sizeof(PCIDevice),
>>           .qdev.no_user = 1,
>> +        .qdev.no_hotplug = 1,
>>           .init         = piix4_initfn,
>>       },{
>>           /* end of list */
>
> This one breaks the build for me.  The below seems to help - but begs
> the question: was this tested?

Tested on x86.  piix4 chipset is used by hw/mips_malta.c, looks like I 
forgot to do a testbuild with all targets enabled :-(

> diff --git a/hw/piix4.c b/hw/piix4.c
> index 1678898..00da049 100644
> --- a/hw/piix4.c
> +++ b/hw/piix4.c
> @@ -113,7 +113,7 @@ static PCIDeviceInfo piix4_info[] = {
>           .qdev.desc    = "ISA bridge",
>           .qdev.size    = sizeof(PCIDevice),
>           .qdev.no_user = 1,
> -        .qdev.no_hotplug = 1,
> +        .no_hotplug = 1,
>           .init         = piix4_initfn,
>       },{
>           /* end of list */

This is correct.  Do you just squash in the fix or do you want me respin 
the patch series?

cheers,
   Gerd
Michael S. Tsirkin Jan. 6, 2011, 9:29 a.m. UTC | #2 
On Thu, Jan 06, 2011 at 09:40:52AM +0100, Gerd Hoffmann wrote:
> >>diff --git a/hw/piix4.c b/hw/piix4.c
> >>index 5489386..1678898 100644
> >>--- a/hw/piix4.c
> >>+++ b/hw/piix4.c
> >>@@ -113,6 +113,7 @@ static PCIDeviceInfo piix4_info[] = {
> >>          .qdev.desc    = "ISA bridge",
> >>          .qdev.size    = sizeof(PCIDevice),
> >>          .qdev.no_user = 1,
> >>+        .qdev.no_hotplug = 1,
> >>          .init         = piix4_initfn,
> >>      },{
> >>          /* end of list */
> >
> >This one breaks the build for me.  The below seems to help - but begs
> >the question: was this tested?
> 
> Tested on x86.  piix4 chipset is used by hw/mips_malta.c, looks like
> I forgot to do a testbuild with all targets enabled :-(

Let's just patch whatever was tested for now?

> >diff --git a/hw/piix4.c b/hw/piix4.c
> >index 1678898..00da049 100644
> >--- a/hw/piix4.c
> >+++ b/hw/piix4.c
> >@@ -113,7 +113,7 @@ static PCIDeviceInfo piix4_info[] = {
> >          .qdev.desc    = "ISA bridge",
> >          .qdev.size    = sizeof(PCIDevice),
> >          .qdev.no_user = 1,
> >-        .qdev.no_hotplug = 1,
> >+        .no_hotplug = 1,
> >          .init         = piix4_initfn,
> >      },{
> >          /* end of list */
> 
> This is correct.  Do you just squash in the fix or do you want me
> respin the patch series?
> 
> cheers,
>   Gerd

Could you split the tested and untested parts to separate patches,
noting the status in the commit message?
We could then queue the tested parts directly and wait for
maintainer's ack for the rest.
Gerd Hoffmann Jan. 6, 2011, 2:14 p.m. UTC | #3 
Hi,

> Could you split the tested and untested parts to separate patches,
> noting the status in the commit message?

I think this is overkill given how simple the change is.  Respin goes to 
the list shortly.

cheers,
   Gerd
Michael S. Tsirkin Jan. 6, 2011, 2:34 p.m. UTC | #4 
On Thu, Jan 06, 2011 at 03:14:18PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >Could you split the tested and untested parts to separate patches,
> >noting the status in the commit message?
> 
> I think this is overkill given how simple the change is.

Yes but I don't know whether the untested devices this
patch touches are intended to be non hotpluggable.
I guess, I'll just ack the pci.c part and let Anthony deal
with this.

>  Respin
> goes to the list shortly.
> 
> cheers,
>   Gerd
Marcelo Tosatti Jan. 6, 2011, 6:45 p.m. UTC | #5 
On Thu, Jan 06, 2011 at 04:34:38PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jan 06, 2011 at 03:14:18PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > >Could you split the tested and untested parts to separate patches,
> > >noting the status in the commit message?
> > 
> > I think this is overkill given how simple the change is.
> 
> Yes but I don't know whether the untested devices this
> patch touches are intended to be non hotpluggable.
> I guess, I'll just ack the pci.c part and let Anthony deal
> with this.

Michael,

PCI hotplug is only supported on x86 via ACPI, and there the piix 
devices are not hotpluggable.
Michael S. Tsirkin Jan. 7, 2011, 11:42 a.m. UTC | #6 
On Thu, Jan 06, 2011 at 04:45:20PM -0200, Marcelo Tosatti wrote:
> On Thu, Jan 06, 2011 at 04:34:38PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Jan 06, 2011 at 03:14:18PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > >Could you split the tested and untested parts to separate patches,
> > > >noting the status in the commit message?
> > > 
> > > I think this is overkill given how simple the change is.
> > 
> > Yes but I don't know whether the untested devices this
> > patch touches are intended to be non hotpluggable.
> > I guess, I'll just ack the pci.c part and let Anthony deal
> > with this.
> 
> Michael,
> 
> PCI hotplug is only supported on x86 via ACPI, and there the piix 
> devices are not hotpluggable.

Aurelien, could you ack the piix4 bit please?
diff mbox

Patch

diff --git a/hw/piix4.c b/hw/piix4.c
index 1678898..00da049 100644
--- a/hw/piix4.c
+++ b/hw/piix4.c
@@ -113,7 +113,7 @@  static PCIDeviceInfo piix4_info[] = {
         .qdev.desc    = "ISA bridge",
         .qdev.size    = sizeof(PCIDevice),
         .qdev.no_user = 1,
-        .qdev.no_hotplug = 1,
+        .no_hotplug = 1,
         .init         = piix4_initfn,
     },{
         /* end of list */