Re: [Qemu-devel] [PATCH 11/12] qdev: add enum property types to QAPI schema

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Prepare for when QOM introspection will be able to piggyback on the QAPI
> schema for describing property types.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/core/qdev-properties.c | 18 +++------------
>  hw/i386/kvm/i8254.c       |  6 ++---
>  hw/timer/mc146818rtc.c    | 14 ++++++------
>  include/hw/block/block.h  |  6 -----
>  include/qemu-common.h     |  8 -------
>  qapi-schema.json          | 58 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 71 insertions(+), 39 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2c3a756..0a2ca05 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -449,34 +449,22 @@ PropertyInfo qdev_prop_macaddr = {
>  
>  /* --- lost tick policy --- */
>  
> -static const char *lost_tick_policy_table[LOST_TICK_MAX+1] = {
> -    [LOST_TICK_DISCARD] = "discard",
> -    [LOST_TICK_DELAY] = "delay",
> -    [LOST_TICK_MERGE] = "merge",
> -    [LOST_TICK_SLEW] = "slew",
> -    [LOST_TICK_MAX] = NULL,
> -};
> -
>  QEMU_BUILD_BUG_ON(sizeof(LostTickPolicy) != sizeof(int));
>  
>  PropertyInfo qdev_prop_losttickpolicy = {
>      .name  = "LostTickPolicy",
> -    .enum_table  = lost_tick_policy_table,
> +    .enum_table  = LostTickPolicy_lookup,
>      .get   = get_enum,
>      .set   = set_enum,
>  };
>  
>  /* --- BIOS CHS translation */
>  
> -static const char *bios_chs_trans_table[] = {
> -    [BIOS_ATA_TRANSLATION_AUTO] = "auto",
> -    [BIOS_ATA_TRANSLATION_NONE] = "none",
> -    [BIOS_ATA_TRANSLATION_LBA]  = "lba",
> -};
> +QEMU_BUILD_BUG_ON(sizeof(BiosAtaTranslation) != sizeof(int));

Recognized values before your patch: "auto", "none", "lba".

>  
>  PropertyInfo qdev_prop_bios_chs_trans = {
>      .name = "bios-chs-trans",
> -    .enum_table = bios_chs_trans_table,
> +    .enum_table = BiosAtaTranslation_lookup,
>      .get = get_enum,
>      .set = set_enum,
>  };
> diff --git a/hw/i386/kvm/i8254.c b/hw/i386/kvm/i8254.c
> index f8f3021..59373aa 100644
> --- a/hw/i386/kvm/i8254.c
> +++ b/hw/i386/kvm/i8254.c
> @@ -268,9 +268,9 @@ static void kvm_pit_realizefn(DeviceState *dev, Error 
> **errp)
>          return;
>      }
>      switch (s->lost_tick_policy) {
> -    case LOST_TICK_DELAY:
> +    case LOST_TICK_POLICY_DELAY:
>          break; /* enabled by default */
> -    case LOST_TICK_DISCARD:
> +    case LOST_TICK_POLICY_DISCARD:
>          if (kvm_check_extension(kvm_state, KVM_CAP_REINJECT_CONTROL)) {
>              struct kvm_reinject_control control = { .pit_reinject = 0 };
>  
> @@ -300,7 +300,7 @@ static void kvm_pit_realizefn(DeviceState *dev, Error 
> **errp)
>  static Property kvm_pit_properties[] = {
>      DEFINE_PROP_UINT32("iobase", PITCommonState, iobase,  -1),
>      DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", KVMPITState,
> -                               lost_tick_policy, LOST_TICK_DELAY),
> +                               lost_tick_policy, LOST_TICK_POLICY_DELAY),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 6fb124f..8509309 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -185,7 +185,7 @@ static void rtc_periodic_timer(void *opaque)
>      if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
>          s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
>  #ifdef TARGET_I386
> -        if (s->lost_tick_policy == LOST_TICK_SLEW) {
> +        if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
>              if (s->irq_reinject_on_ack_count >= RTC_REINJECT_ON_ACK_COUNT)
>                  s->irq_reinject_on_ack_count = 0;            
>              apic_reset_irq_delivered();
> @@ -708,7 +708,7 @@ static int rtc_post_load(void *opaque, int version_id)
>  
>  #ifdef TARGET_I386
>      if (version_id >= 2) {
> -        if (s->lost_tick_policy == LOST_TICK_SLEW) {
> +        if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
>              rtc_coalesced_timer_update(s);
>          }
>      }
> @@ -749,7 +749,7 @@ static void rtc_notify_clock_reset(Notifier *notifier, 
> void *data)
>      periodic_timer_update(s, now);
>      check_update_timer(s);
>  #ifdef TARGET_I386
> -    if (s->lost_tick_policy == LOST_TICK_SLEW) {
> +    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
>          rtc_coalesced_timer_update(s);
>      }
>  #endif
> @@ -774,7 +774,7 @@ static void rtc_reset(void *opaque)
>      qemu_irq_lower(s->irq);
>  
>  #ifdef TARGET_I386
> -    if (s->lost_tick_policy == LOST_TICK_SLEW) {
> +    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
>          s->irq_coalesced = 0;
>      }
>  #endif
> @@ -835,11 +835,11 @@ static void rtc_realizefn(DeviceState *dev, Error 
> **errp)
>  
>  #ifdef TARGET_I386
>      switch (s->lost_tick_policy) {
> -    case LOST_TICK_SLEW:
> +    case LOST_TICK_POLICY_SLEW:
>          s->coalesced_timer =
>              timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
>          break;
> -    case LOST_TICK_DISCARD:
> +    case LOST_TICK_POLICY_DISCARD:
>          break;
>      default:
>          error_setg(errp, "Invalid lost tick policy.");
> @@ -890,7 +890,7 @@ ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq 
> intercept_irq)
>  static Property mc146818rtc_properties[] = {
>      DEFINE_PROP_INT32("base_year", RTCState, base_year, 1980),
>      DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
> -                               lost_tick_policy, LOST_TICK_DISCARD),
> +                               lost_tick_policy, LOST_TICK_POLICY_DISCARD),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index dd11532..7c3d6c8 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -65,12 +65,6 @@ int blkconf_geometry(BlockConf *conf, int *trans,
>  
>  /* Hard disk geometry */
>  
> -#define BIOS_ATA_TRANSLATION_AUTO   0
> -#define BIOS_ATA_TRANSLATION_NONE   1
> -#define BIOS_ATA_TRANSLATION_LBA    2
> -#define BIOS_ATA_TRANSLATION_LARGE  3
> -#define BIOS_ATA_TRANSLATION_RECHS  4
> -
>  void hd_geometry_guess(BlockDriverState *bs,
>                         uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
>                         int *ptrans);
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 5054836..b0e34b2 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -261,14 +261,6 @@ typedef int (*DMA_transfer_handler) (void *opaque, int 
> nchan, int pos, int size)
>  
>  typedef uint64_t pcibus_t;
>  
> -typedef enum LostTickPolicy {
> -    LOST_TICK_DISCARD,
> -    LOST_TICK_DELAY,
> -    LOST_TICK_MERGE,
> -    LOST_TICK_SLEW,
> -    LOST_TICK_MAX
> -} LostTickPolicy;
> -
>  typedef struct PCIHostDeviceAddress {
>      unsigned int domain;
>      unsigned int bus;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 05ced9d..ed72938 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -28,6 +28,65 @@
>    'data': [ 'GenericError', 'CommandNotFound', 'DeviceEncrypted',
>              'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap' ] }
>  
> +
> +##
> +# LostTickPolicy:
> +#
> +# Policy for handling lost ticks in timer devices.
> +#
> +# @discard: throw away the missed tick(s) and continue with future injection
> +#           normally.  Guest time may be delayed, unless the OS has explicit
> +#           handling of lost ticks
> +#
> +# @delay: continue to deliver ticks at the normal rate.  Guest time will be
> +#         delayed due to the late tick
> +#
> +# @merge: merge the missed tick(s) into one tick and inject.  Guest time
> +#         may be delayed, depending on how the OS reacts to the merging
> +#         of ticks
> +#
> +# @slew: deliver ticks at a higher rate to catch up with the missed tick. The
> +#        guest time should not be delayed once catchup is complete.
> +#
> +# Since: 2.0
> +##
> +{ 'enum': 'LostTickPolicy',
> +  'data': ['discard', 'delay', 'merge', 'slew' ] }
> +
> +##
> +# BiosAtaTranslation:
> +#
> +# Policy that BIOS should use to interpret cylinder/head/sector
> +# addresses.  Note that Bochs BIOS and SeaBIOS will not actually
> +# translate logical CHS to physical; instead, they will use logical
> +# block addressing.
> +#
> +# @auto: If cylinder/heads/sizes are passed, choose between none and LBA
> +#        depending on the size of the disk.  If they are not passed,
> +#        choose none if QEMU can guess that the disk had 16 or fewer
> +#        heads, large if QEMU can guess that the disk had 131072 or
> +#        fewer tracks across all heads (i.e. cylinders*heads<131072),
> +#        otherwise LBA.
> +#
> +# @none: The physical disk geometry is equal to the logical geometry.
> +#
> +# @lba: Assume 63 sectors per track and one of 16, 32, 64, 128 or 255
> +#       heads (if fewer than 255 are enough to cover the whole disk
> +#       with 1024 cylinders/head).  The number of cylinders/head is
> +#       then computed based on the number of sectors and heads.
> +#
> +# @large: The number of cylinders per head is scaled down to 1024
> +#         by correspondingly scaling up the number of heads.
> +#
> +# @rechs: Same as @large, but first convert a 16-head geometry to
> +#         15-head, by proportionally scaling up the number of
> +#         cylinders/head.
> +#
> +# Since: 2.0
> +##
> +{ 'enum': 'BiosAtaTranslation',
> +  'data': ['auto', 'none', 'lba', 'large', 'rechs']}
> +
>  ##
>  # @add_client
>  #

Recognized values after your patch: same as before, plus "large",
"rechs".  Silent change!

Please keep distinct things separate: have one patch introducing new
values "large" and "rechs", and another patch to QAPIfy.

If you want the obsolete ways to specify translation accept the new
values, too, then the patch to do that (currently "[PATCH v2] block:
handle "rechs" and "large" translation options") should immediately
follow the first one, or be squashed into it.