Re: [Qemu-devel] [PATCH 18/19] hw: remove CPU dependencies

[Paolo Bonzini]

Il 04/02/2013 23:30, Peter Maydell ha scritto:
> On 4 February 2013 17:30, Paolo Bonzini <address@hidden> wrote:
>> Some devices or headers are using poisoned identifiers.
>> For .c files, some are useless and we remove them.
>>
>> For headers, wrap the definitions in the headers with
>>
> 
> Truncated sentence?
> 
>> Signed-off-by: Paolo Bonzini <address@hidden>
> 
>> --- a/hw/pci/host-apb.c
>> +++ b/hw/pci/host-apb.c
>> @@ -92,7 +92,7 @@ static void apb_config_writel (void *opaque, hwaddr addr,
>>  {
>>      APBState *s = opaque;
>>
>> -    APB_DPRINTF("%s: addr " TARGET_FMT_lx " val %" PRIx64 "\n", __func__, 
>> addr, val);
>> +    APB_DPRINTF("%s: addr " TARGET_FMT_plx " val %" PRIx64 "\n", __func__, 
>> addr, val);
>>
>>      switch (addr & 0xffff) {
>>      case 0x30 ... 0x4f: /* DMA error registers */
>> @@ -201,7 +201,7 @@ static uint64_t apb_config_readl (void *opaque,
>>          val = 0;
>>          break;
>>      }
>> -    APB_DPRINTF("%s: addr " TARGET_FMT_lx " -> %x\n", __func__, addr, val);
>> +    APB_DPRINTF("%s: addr " TARGET_FMT_plx " -> %x\n", __func__, addr, val);
>>
>>      return val;
>>  }
>> @@ -218,7 +218,7 @@ static void apb_pci_config_write(void *opaque, hwaddr 
>> addr,
>>      APBState *s = opaque;
>>
>>      val = qemu_bswap_len(val, size);
>> -    APB_DPRINTF("%s: addr " TARGET_FMT_lx " val %" PRIx64 "\n", __func__, 
>> addr, val);
>> +    APB_DPRINTF("%s: addr " TARGET_FMT_plx " val %" PRIx64 "\n", __func__, 
>> addr, val);
>>      pci_data_write(s->bus, addr, val, size);
>>  }
>>
>> @@ -230,7 +230,7 @@ static uint64_t apb_pci_config_read(void *opaque, hwaddr 
>> addr,
>>
>>      ret = pci_data_read(s->bus, addr, size);
>>      ret = qemu_bswap_len(ret, size);
>> -    APB_DPRINTF("%s: addr " TARGET_FMT_lx " -> %x\n", __func__, addr, ret);
>> +    APB_DPRINTF("%s: addr " TARGET_FMT_plx " -> %x\n", __func__, addr, ret);
>>      return ret;
>>  }
> 
> This appears to just be fixing format string errors in debug : should
> be a separate patch.

Yes.

>> diff --git a/include/hw/arm/exynos4210.h b/include/hw/arm/exynos4210.h
>> index bb9a1dd..8798cbe 100644
>> --- a/include/hw/arm/exynos4210.h
>> +++ b/include/hw/arm/exynos4210.h
>> @@ -84,7 +84,10 @@ typedef struct Exynos4210Irq {
>>      qemu_irq board_irqs[EXYNOS4210_MAX_INT_COMBINER_IN_IRQ];
>>  } Exynos4210Irq;
>>
>> -typedef struct Exynos4210State {
>> +typedef struct Exynos4210State Exynos4210State;
>> +
>> +#ifdef NEED_CPU_H
>> +struct Exynos4210State {
>>      ARMCPU *cpu[EXYNOS4210_NCPUS];
>>      Exynos4210Irq irqs;
>>      qemu_irq *irq_table;
>> @@ -98,16 +101,17 @@ typedef struct Exynos4210State {
>>      MemoryRegion boot_secondary;
>>      MemoryRegion bootreg_mem;
>>      i2c_bus *i2c_if[EXYNOS4210_I2C_NUMBER];
>> -} Exynos4210State;
>> +};
>>
>>  void exynos4210_write_secondary(ARMCPU *cpu,
>>          const struct arm_boot_info *info);
>> +#endif
> 
> etc
> 
> There's a lot of extra ifdeffery being added here. That makes
> me wonder if it's really the right way to do this...

No, it's not.  The right way is either to have a .h file for each
device, or to qdevify things so that devices do not need an _init
function in the .c file (instead it can be an inline in the .h file).

Again, it's just getting us out of the local optimum.  But it's close to
the end of the series so that it can be easily dropped.

Paolo