Patchwork [1/2] piix: use type-safe cast instead of directly access of parent dev

login
register
mail settings
Submitter Hu Tao
Date June 14, 2013, 6:49 a.m.
Message ID <1371192585-536-1-git-send-email-hutao@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/251236/
State New
Headers show

Comments

Hu Tao - June 14, 2013, 6:49 a.m.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/pci-host/piix.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
Andreas Färber - June 14, 2013, 9:20 a.m.
Hi,

Am 14.06.2013 08:49, schrieb Hu Tao:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/pci-host/piix.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index f9e68c3..ba9a8f0 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -121,22 +121,24 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
>  static void i440fx_update_memory_mappings(PCII440FXState *d)
>  {
>      int i;
> +    PCIDevice *pd = PCI_DEVICE(d);
>  
>      memory_region_transaction_begin();
>      for (i = 0; i < 13; i++) {
>          pam_update(&d->pam_regions[i], i,
> -                   d->dev.config[I440FX_PAM + ((i + 1) / 2)]);
> +                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
[snip]

These two patches will likely conflict with Peter C.'s recent PCI
patchset doing the same thing.

They look okay, although in one case changing to "dev" variable will
force yet another change when later switching to QOM realize with
DeviceState *dev.

Regards,
Andreas
Hu Tao - June 17, 2013, 1:56 a.m.
On Fri, Jun 14, 2013 at 11:20:55AM +0200, Andreas Färber wrote:
> Hi,
> 
> Am 14.06.2013 08:49, schrieb Hu Tao:
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  hw/pci-host/piix.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index f9e68c3..ba9a8f0 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -121,22 +121,24 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
> >  static void i440fx_update_memory_mappings(PCII440FXState *d)
> >  {
> >      int i;
> > +    PCIDevice *pd = PCI_DEVICE(d);
> >  
> >      memory_region_transaction_begin();
> >      for (i = 0; i < 13; i++) {
> >          pam_update(&d->pam_regions[i], i,
> > -                   d->dev.config[I440FX_PAM + ((i + 1) / 2)]);
> > +                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
> [snip]
> 
> These two patches will likely conflict with Peter C.'s recent PCI
> patchset doing the same thing.

Thanks for reminding! I applied these two on top of Peter's patchset,
only patch 2 conflicts on one hunk.

> 
> They look okay, although in one case changing to "dev" variable will
> force yet another change when later switching to QOM realize with
> DeviceState *dev.

Do you mean q35_host_init(SysBusDevice *dev)?  Would you like to hold it?
Hu Tao - June 17, 2013, 3:27 a.m.
Andreas,

On Mon, Jun 17, 2013 at 09:56:47AM +0800, Hu Tao wrote:
> On Fri, Jun 14, 2013 at 11:20:55AM +0200, Andreas Färber wrote:
> > Hi,
> > 
> > Am 14.06.2013 08:49, schrieb Hu Tao:
> > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > ---
> > >  hw/pci-host/piix.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > index f9e68c3..ba9a8f0 100644
> > > --- a/hw/pci-host/piix.c
> > > +++ b/hw/pci-host/piix.c
> > > @@ -121,22 +121,24 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
> > >  static void i440fx_update_memory_mappings(PCII440FXState *d)
> > >  {
> > >      int i;
> > > +    PCIDevice *pd = PCI_DEVICE(d);
> > >  
> > >      memory_region_transaction_begin();
> > >      for (i = 0; i < 13; i++) {
> > >          pam_update(&d->pam_regions[i], i,
> > > -                   d->dev.config[I440FX_PAM + ((i + 1) / 2)]);
> > > +                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
> > [snip]
> > 
> > These two patches will likely conflict with Peter C.'s recent PCI
> > patchset doing the same thing.
> 
> Thanks for reminding! I applied these two on top of Peter's patchset,
> only patch 2 conflicts on one hunk.
> 
> > 
> > They look okay, although in one case changing to "dev" variable will
> > force yet another change when later switching to QOM realize with
> > DeviceState *dev.
> 
> Do you mean q35_host_init(SysBusDevice *dev)?  Would you like to hold it?

I'm trying to switch SysbusDevice to QOM realize if you're not working
on it.
Peter Crosthwaite - June 17, 2013, 8:14 a.m.
Hi Hu,

On Mon, Jun 17, 2013 at 1:27 PM, Hu Tao <hutao@cn.fujitsu.com> wrote:
> Andreas,
>
> On Mon, Jun 17, 2013 at 09:56:47AM +0800, Hu Tao wrote:
>> On Fri, Jun 14, 2013 at 11:20:55AM +0200, Andreas Färber wrote:
>> > Hi,
>> >
>> > Am 14.06.2013 08:49, schrieb Hu Tao:
>> > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>> > > ---
>> > >  hw/pci-host/piix.c | 11 +++++++----
>> > >  1 file changed, 7 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> > > index f9e68c3..ba9a8f0 100644
>> > > --- a/hw/pci-host/piix.c
>> > > +++ b/hw/pci-host/piix.c
>> > > @@ -121,22 +121,24 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
>> > >  static void i440fx_update_memory_mappings(PCII440FXState *d)
>> > >  {
>> > >      int i;
>> > > +    PCIDevice *pd = PCI_DEVICE(d);
>> > >
>> > >      memory_region_transaction_begin();
>> > >      for (i = 0; i < 13; i++) {
>> > >          pam_update(&d->pam_regions[i], i,
>> > > -                   d->dev.config[I440FX_PAM + ((i + 1) / 2)]);
>> > > +                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
>> > [snip]
>> >
>> > These two patches will likely conflict with Peter C.'s recent PCI
>> > patchset doing the same thing.
>>
>> Thanks for reminding! I applied these two on top of Peter's patchset,
>> only patch 2 conflicts on one hunk.
>>
>> >
>> > They look okay, although in one case changing to "dev" variable will
>> > force yet another change when later switching to QOM realize with
>> > DeviceState *dev.
>>
>> Do you mean q35_host_init(SysBusDevice *dev)?  Would you like to hold it?
>
> I'm trying to switch SysbusDevice to QOM realize if you're not working
> on it.

Thats a big job. FYI, I have already done hw/block/nand.c so you can
skip that one. Will send patches shortly.

Regards,
Peter

>
Hu Tao - June 17, 2013, 8:43 a.m.
On Mon, Jun 17, 2013 at 06:14:47PM +1000, Peter Crosthwaite wrote:
> Hi Hu,
> 
> On Mon, Jun 17, 2013 at 1:27 PM, Hu Tao <hutao@cn.fujitsu.com> wrote:
> > Andreas,
> >
> > On Mon, Jun 17, 2013 at 09:56:47AM +0800, Hu Tao wrote:
> >> On Fri, Jun 14, 2013 at 11:20:55AM +0200, Andreas Färber wrote:
> >> > Hi,
> >> >
> >> > Am 14.06.2013 08:49, schrieb Hu Tao:
> >> > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >> > > ---
> >> > >  hw/pci-host/piix.c | 11 +++++++----
> >> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >> > > index f9e68c3..ba9a8f0 100644
> >> > > --- a/hw/pci-host/piix.c
> >> > > +++ b/hw/pci-host/piix.c
> >> > > @@ -121,22 +121,24 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
> >> > >  static void i440fx_update_memory_mappings(PCII440FXState *d)
> >> > >  {
> >> > >      int i;
> >> > > +    PCIDevice *pd = PCI_DEVICE(d);
> >> > >
> >> > >      memory_region_transaction_begin();
> >> > >      for (i = 0; i < 13; i++) {
> >> > >          pam_update(&d->pam_regions[i], i,
> >> > > -                   d->dev.config[I440FX_PAM + ((i + 1) / 2)]);
> >> > > +                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
> >> > [snip]
> >> >
> >> > These two patches will likely conflict with Peter C.'s recent PCI
> >> > patchset doing the same thing.
> >>
> >> Thanks for reminding! I applied these two on top of Peter's patchset,
> >> only patch 2 conflicts on one hunk.
> >>
> >> >
> >> > They look okay, although in one case changing to "dev" variable will
> >> > force yet another change when later switching to QOM realize with
> >> > DeviceState *dev.
> >>
> >> Do you mean q35_host_init(SysBusDevice *dev)?  Would you like to hold it?
> >
> > I'm trying to switch SysbusDevice to QOM realize if you're not working
> > on it.
> 
> Thats a big job. FYI, I have already done hw/block/nand.c so you can
> skip that one. Will send patches shortly.

OK. Thanks.

Patch

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index f9e68c3..ba9a8f0 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -121,22 +121,24 @@  static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
 static void i440fx_update_memory_mappings(PCII440FXState *d)
 {
     int i;
+    PCIDevice *pd = PCI_DEVICE(d);
 
     memory_region_transaction_begin();
     for (i = 0; i < 13; i++) {
         pam_update(&d->pam_regions[i], i,
-                   d->dev.config[I440FX_PAM + ((i + 1) / 2)]);
+                   pd->config[I440FX_PAM + ((i + 1) / 2)]);
     }
-    smram_update(&d->smram_region, d->dev.config[I440FX_SMRAM], d->smm_enabled);
+    smram_update(&d->smram_region, pd->config[I440FX_SMRAM], d->smm_enabled);
     memory_region_transaction_commit();
 }
 
 static void i440fx_set_smm(int val, void *arg)
 {
     PCII440FXState *d = arg;
+    PCIDevice *pd = PCI_DEVICE(d);
 
     memory_region_transaction_begin();
-    smram_set_smm(&d->smm_enabled, val, d->dev.config[I440FX_SMRAM],
+    smram_set_smm(&d->smm_enabled, val, pd->config[I440FX_SMRAM],
                   &d->smram_region);
     memory_region_transaction_commit();
 }
@@ -158,9 +160,10 @@  static void i440fx_write_config(PCIDevice *dev,
 static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
 {
     PCII440FXState *d = opaque;
+    PCIDevice *pd = PCI_DEVICE(d);
     int ret, i;
 
-    ret = pci_device_load(&d->dev, f);
+    ret = pci_device_load(pd, f);
     if (ret < 0)
         return ret;
     i440fx_update_memory_mappings(d);