| Message ID | 1338858018-17189-10-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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. > + 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. > uint32_t irq_coalesced; > uint32_t period; > - QEMUTimer *coalesced_timer; > + QEMUTimer _broken *coalesced_timer; > QEMUTimer *second_timer; > QEMUTimer *second_timer2; > - Notifier clock_reset_notifier; > - LostTickPolicy lost_tick_policy; > - Notifier suspend_notifier; > + Notifier _broken clock_reset_notifier; Why broken? > + LostTickPolicy _immutable lost_tick_policy; _host; nothign prevents us from changing it dynamically in theory. > + Notifier _broken suspend_notifier; Why broken? > } RTCState; > > #endif /* !MC146818RTC_STATE_H */
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. > >> + 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. > >> uint32_t irq_coalesced; >> uint32_t period; >> - QEMUTimer *coalesced_timer; >> + QEMUTimer _broken *coalesced_timer; >> QEMUTimer *second_timer; >> QEMUTimer *second_timer2; >> - Notifier clock_reset_notifier; >> - LostTickPolicy lost_tick_policy; >> - Notifier suspend_notifier; >> + Notifier _broken clock_reset_notifier; > > Why broken? > >> + 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? > >> + Notifier _broken suspend_notifier; > > Why broken? > >> } RTCState; >> >> #endif /* !MC146818RTC_STATE_H */ > > Jan
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. 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. >>> + 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.
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 >
diff --git a/hw/mc146818rtc_state.h b/hw/mc146818rtc_state.h index f819e15..9347ee6 100644 --- a/hw/mc146818rtc_state.h +++ b/hw/mc146818rtc_state.h @@ -2,31 +2,32 @@ #define MC146818RTC_STATE_H #include "isa.h" +#include "qapi/qc.h" -typedef struct RTCState { - ISADevice dev; - MemoryRegion io; +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; + 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; uint32_t irq_coalesced; uint32_t period; - QEMUTimer *coalesced_timer; + QEMUTimer _broken *coalesced_timer; QEMUTimer *second_timer; QEMUTimer *second_timer2; - Notifier clock_reset_notifier; - LostTickPolicy lost_tick_policy; - Notifier suspend_notifier; + Notifier _broken clock_reset_notifier; + LostTickPolicy _immutable lost_tick_policy; + Notifier _broken suspend_notifier; } RTCState; #endif /* !MC146818RTC_STATE_H */
Add our annotations according to QIDL documentation. Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> --- hw/mc146818rtc_state.h | 23 ++++++++++++----------- 1 files changed, 12 insertions(+), 11 deletions(-)