Re: [Qemu-devel] [PATCH 09/17] rtc: add qc annotations

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Tue, Jun 05, 2012 at 03:42:30PM +0300, Avi Kivity wrote:
> On 06/05/2012 01:40 PM, Jan Kiszka wrote:
> > On 2012-06-05 12:25, Avi Kivity wrote:
> >> On 06/05/2012 04:00 AM, Michael Roth wrote:
> >>> Add our annotations according to QIDL documentation.
> >>>
> >>> +qc_declaration typedef struct RTCState {
> >>> +    ISADevice _immutable dev;
> >>> +    MemoryRegion _immutable io;
> >>>      uint8_t cmos_data[128];
> >>>      uint8_t cmos_index;
> >>>      struct tm current_tm;
> >>>      int32_t base_year;
> >>> -    qemu_irq irq;
> >>> -    qemu_irq sqw_irq;
> >>> -    int it_shift;
> >>> +    qemu_irq _immutable irq;
> >>> +    qemu_irq _immutable sqw_irq;
> >> 
> >> How is qemu_irq immutable? We're raising and lowering it many times a
> >> second.  It's _derived, perhaps, but not immutable.
> > 
> > No, IRQState in its current form is immutable, doesn't contain any
> > volatile state.
> 
> Good point.  So it's just like any pointer: it depends on the pointed-to
> type.  If it saves its state, then great, but if the pointed-to type
> doesn't, then it's broken.
> 
> > 
> >> 
> >>> +    int32_t _immutable it_shift;
> >>>      /* periodic timer */
> >>>      QEMUTimer *periodic_timer;
> >>>      int64_t next_periodic_time;
> >>>      /* second update */
> >>>      int64_t next_second_time;
> >>> -    uint16_t irq_reinject_on_ack_count;
> >>> +    uint16_t _derived irq_reinject_on_ack_count;
> >> 
> >> It's not derived from anything.  It's _host, maybe.
> > 
> > I think it is _broken.

Agreed, using _derived was an error on my part

> 
> I think it's _complicated.  Migration involves downtime and so lost
> ticks.  In the case of RTC we probably need to trigger compensation code
> that will try to handle it according to policy.

We'd likely only be able to compensate based on calculated downtime or
something along that line. I think we'd still want to send accumulated ticks
as well, even if it's of little importance, since it's still guest state
of a sort (in the sense that it guest-perceivable) that we should avoid
discarding.

> 
> >>> +    LostTickPolicy _immutable lost_tick_policy;
> >> 
> >> _host; nothign prevents us from changing it dynamically in theory.
> > 
> > _host makes no sense to me. Either it is _immutable or _broken - or is
> > properly saved/restored. What should be the semantic of _host?
> 
> An emulated device manages some state, and also talks to a host device,
> often through an interface like BlockDriverState or CharState. _host is
> anything related to the host device that we should be able to
> reconstruct on resume.
> 
> Example of host state is a CharDriverState filename.  Even if we allow
> it to change, there is no point in migrating it since it belongs to the
> source namespace, not destination.
> 
> -- 
> error compiling committee.c: too many arguments to function
>