| Message ID | 29c39a6f9ce3734bf693581348ed78f4bff3e0ef.1516746211.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add and connect the ZynqMP RTC | expand |
On 23 January 2018 at 22:24, Alistair Francis <alistair.francis@xilinx.com> wrote: > Allow the guest to determine the time set from the QEMU command line. > > This includes adding a trace event to debug the new time. > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > static const VMStateDescription vmstate_rtc = { > .name = TYPE_XLNX_ZYNQMP_RTC, > .version_id = 1, > .minimum_version_id = 1, > + .post_load = rtc_post_load, > .fields = (VMStateField[]) { > VMSTATE_UINT32_ARRAY(regs, XlnxZynqMPRTC, XLNX_ZYNQMP_RTC_R_MAX), > + VMSTATE_INT32(current_tm.tm_sec, XlnxZynqMPRTC), > + VMSTATE_INT32(current_tm.tm_min, XlnxZynqMPRTC), > + VMSTATE_INT32(current_tm.tm_hour, XlnxZynqMPRTC), > + VMSTATE_INT32(current_tm.tm_wday, XlnxZynqMPRTC), > + VMSTATE_INT32(current_tm.tm_mday, XlnxZynqMPRTC), > + VMSTATE_INT32(current_tm.tm_mon, XlnxZynqMPRTC), > + VMSTATE_INT32(current_tm.tm_year, XlnxZynqMPRTC), I'm still not sure about having the current_tm struct fields here rather than whatever the hardware's natural representation of the current time is. Can you explain why you think this is the best approach? thanks -- PMM
On Thu, Jan 25, 2018 at 3:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 23 January 2018 at 22:24, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> Allow the guest to determine the time set from the QEMU command line. >> >> This includes adding a trace event to debug the new time. >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> static const VMStateDescription vmstate_rtc = { >> .name = TYPE_XLNX_ZYNQMP_RTC, >> .version_id = 1, >> .minimum_version_id = 1, >> + .post_load = rtc_post_load, >> .fields = (VMStateField[]) { >> VMSTATE_UINT32_ARRAY(regs, XlnxZynqMPRTC, XLNX_ZYNQMP_RTC_R_MAX), >> + VMSTATE_INT32(current_tm.tm_sec, XlnxZynqMPRTC), >> + VMSTATE_INT32(current_tm.tm_min, XlnxZynqMPRTC), >> + VMSTATE_INT32(current_tm.tm_hour, XlnxZynqMPRTC), >> + VMSTATE_INT32(current_tm.tm_wday, XlnxZynqMPRTC), >> + VMSTATE_INT32(current_tm.tm_mday, XlnxZynqMPRTC), >> + VMSTATE_INT32(current_tm.tm_mon, XlnxZynqMPRTC), >> + VMSTATE_INT32(current_tm.tm_year, XlnxZynqMPRTC), > > I'm still not sure about having the current_tm struct fields > here rather than whatever the hardware's natural representation > of the current time is. Can you explain why you think this > is the best approach? This means that the only thing we have to track is the time that the user set, either through the QEMU command line arguments as an override or the original starting time. Then from there we can always calculate the correct offset based on our current time. I don't see a nicer way, otherwise we need to recalculate the natural representation before and after migration using something more complex. This way it's always the same logic, we just run it at init and after migration. Alistair > > thanks > -- PMM
On 26 January 2018 at 01:09, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Thu, Jan 25, 2018 at 3:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> I'm still not sure about having the current_tm struct fields >> here rather than whatever the hardware's natural representation >> of the current time is. Can you explain why you think this >> is the best approach? > > This means that the only thing we have to track is the time that the > user set, either through the QEMU command line arguments as an > override or the original starting time. Then from there we can always > calculate the correct offset based on our current time. > > I don't see a nicer way, otherwise we need to recalculate the natural > representation before and after migration using something more > complex. This way it's always the same logic, we just run it at init > and after migration. Doing it this way round means that it gets complicated when the guest writes to the RTC, though. At the moment I can't see anywhere that sets the current_tm fields except for reset and post-migration. Isn't something missing here? I'm wondering if this choice of migration state is going to paint us into a corner for handling guest-writes-to-rtc. thanks -- PMM
On Thu, Feb 8, 2018 at 7:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 January 2018 at 01:09, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> On Thu, Jan 25, 2018 at 3:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> I'm still not sure about having the current_tm struct fields >>> here rather than whatever the hardware's natural representation >>> of the current time is. Can you explain why you think this >>> is the best approach? >> >> This means that the only thing we have to track is the time that the >> user set, either through the QEMU command line arguments as an >> override or the original starting time. Then from there we can always >> calculate the correct offset based on our current time. >> >> I don't see a nicer way, otherwise we need to recalculate the natural >> representation before and after migration using something more >> complex. This way it's always the same logic, we just run it at init >> and after migration. > > Doing it this way round means that it gets complicated when the > guest writes to the RTC, though. At the moment I can't see anywhere > that sets the current_tm fields except for reset and post-migration. > Isn't something missing here? I'm wondering if this choice of > migration state is going to paint us into a corner for handling > guest-writes-to-rtc. I don't have guest write support. The main focus was just allowing users to set the time when they boot QEMU. We haven't seen any request to allow guests to set the time, so I didn't bother. Wouldn't adding guest write support just be as simple as converting the value written to a current_tm field? Alistair > > thanks > -- PMM >
On 8 February 2018 at 17:21, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Thu, Feb 8, 2018 at 7:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Doing it this way round means that it gets complicated when the >> guest writes to the RTC, though. At the moment I can't see anywhere >> that sets the current_tm fields except for reset and post-migration. >> Isn't something missing here? I'm wondering if this choice of >> migration state is going to paint us into a corner for handling >> guest-writes-to-rtc. > > I don't have guest write support. The main focus was just allowing > users to set the time when they boot QEMU. We haven't seen any request > to allow guests to set the time, so I didn't bother. > > Wouldn't adding guest write support just be as simple as converting > the value written to a current_tm field? Yes, but then why are we using the current_tm format at all? It's not what the guest wants to see for reads, it's not what writes would naturally be, and it's not what you're using at runtime (you convert it immediately to a tick_offset and ignore the current_tm stuff). I guess it can be made to work, it just seems like a weird format to use for migration. I've also just reread the patch and am a bit confused -- don't you need some code that writes the correct value for the current guest time to the current_tm fields before migration ? thanks -- PMM
diff --git a/include/hw/timer/xlnx-zynqmp-rtc.h b/include/hw/timer/xlnx-zynqmp-rtc.h index 87649836cc..2867563bdd 100644 --- a/include/hw/timer/xlnx-zynqmp-rtc.h +++ b/include/hw/timer/xlnx-zynqmp-rtc.h @@ -79,6 +79,9 @@ typedef struct XlnxZynqMPRTC { qemu_irq irq_rtc_int; qemu_irq irq_addr_error_int; + struct tm current_tm; + uint32_t tick_offset; + uint32_t regs[XLNX_ZYNQMP_RTC_R_MAX]; RegisterInfo regs_info[XLNX_ZYNQMP_RTC_R_MAX]; } XlnxZynqMPRTC; diff --git a/hw/timer/xlnx-zynqmp-rtc.c b/hw/timer/xlnx-zynqmp-rtc.c index 707f145027..1d229870c5 100644 --- a/hw/timer/xlnx-zynqmp-rtc.c +++ b/hw/timer/xlnx-zynqmp-rtc.c @@ -29,6 +29,10 @@ #include "hw/register.h" #include "qemu/bitops.h" #include "qemu/log.h" +#include "hw/ptimer.h" +#include "qemu/cutils.h" +#include "sysemu/sysemu.h" +#include "trace.h" #include "hw/timer/xlnx-zynqmp-rtc.h" #ifndef XLNX_ZYNQMP_RTC_ERR_DEBUG @@ -47,6 +51,19 @@ static void addr_error_int_update_irq(XlnxZynqMPRTC *s) qemu_set_irq(s->irq_addr_error_int, pending); } +static uint32_t rtc_get_count(XlnxZynqMPRTC *s) +{ + int64_t now = qemu_clock_get_ns(rtc_clock); + return s->tick_offset + now / NANOSECONDS_PER_SECOND; +} + +static uint64_t current_time_postr(RegisterInfo *reg, uint64_t val64) +{ + XlnxZynqMPRTC *s = XLNX_ZYNQMP_RTC(reg->opaque); + + return rtc_get_count(s); +} + static void rtc_int_status_postw(RegisterInfo *reg, uint64_t val64) { XlnxZynqMPRTC *s = XLNX_ZYNQMP_RTC(reg->opaque); @@ -97,13 +114,17 @@ static uint64_t addr_error_int_dis_prew(RegisterInfo *reg, uint64_t val64) static const RegisterAccessInfo rtc_regs_info[] = { { .name = "SET_TIME_WRITE", .addr = A_SET_TIME_WRITE, + .unimp = MAKE_64BIT_MASK(0, 32), },{ .name = "SET_TIME_READ", .addr = A_SET_TIME_READ, .ro = 0xffffffff, + .post_read = current_time_postr, },{ .name = "CALIB_WRITE", .addr = A_CALIB_WRITE, + .unimp = MAKE_64BIT_MASK(0, 32), },{ .name = "CALIB_READ", .addr = A_CALIB_READ, .ro = 0x1fffff, },{ .name = "CURRENT_TIME", .addr = A_CURRENT_TIME, .ro = 0xffffffff, + .post_read = current_time_postr, },{ .name = "CURRENT_TICK", .addr = A_CURRENT_TICK, .ro = 0xffff, },{ .name = "ALARM", .addr = A_ALARM, @@ -143,6 +164,10 @@ static void rtc_reset(DeviceState *dev) register_reset(&s->regs_info[i]); } + trace_xlnx_zynqmp_rtc_gettime(s->current_tm.tm_year, s->current_tm.tm_mon, + s->current_tm.tm_mday, s->current_tm.tm_hour, + s->current_tm.tm_min, s->current_tm.tm_sec); + rtc_int_update_irq(s); addr_error_int_update_irq(s); } @@ -178,14 +203,41 @@ static void rtc_init(Object *obj) sysbus_init_mmio(sbd, &s->iomem); sysbus_init_irq(sbd, &s->irq_rtc_int); sysbus_init_irq(sbd, &s->irq_addr_error_int); + + qemu_get_timedate(&s->current_tm, 0); + s->tick_offset = mktimegm(&s->current_tm) - + qemu_clock_get_ns(rtc_clock) / NANOSECONDS_PER_SECOND; +} + +static int rtc_post_load(void *opaque, int version_id) +{ + XlnxZynqMPRTC *s = opaque; + + /* The tick_offset is added to the current time to determine the guest + * time. After migration we don't want to use the original time as that + * will indicate to the guest that time has passed, so we need to + * recalculate the tick_offset here. + */ + s->tick_offset = mktimegm(&s->current_tm) - + qemu_clock_get_ns(rtc_clock) / NANOSECONDS_PER_SECOND; + + return 0; } static const VMStateDescription vmstate_rtc = { .name = TYPE_XLNX_ZYNQMP_RTC, .version_id = 1, .minimum_version_id = 1, + .post_load = rtc_post_load, .fields = (VMStateField[]) { VMSTATE_UINT32_ARRAY(regs, XlnxZynqMPRTC, XLNX_ZYNQMP_RTC_R_MAX), + VMSTATE_INT32(current_tm.tm_sec, XlnxZynqMPRTC), + VMSTATE_INT32(current_tm.tm_min, XlnxZynqMPRTC), + VMSTATE_INT32(current_tm.tm_hour, XlnxZynqMPRTC), + VMSTATE_INT32(current_tm.tm_wday, XlnxZynqMPRTC), + VMSTATE_INT32(current_tm.tm_mday, XlnxZynqMPRTC), + VMSTATE_INT32(current_tm.tm_mon, XlnxZynqMPRTC), + VMSTATE_INT32(current_tm.tm_year, XlnxZynqMPRTC), VMSTATE_END_OF_LIST(), } }; diff --git a/hw/timer/trace-events b/hw/timer/trace-events index 640722b5d1..e6e042fddb 100644 --- a/hw/timer/trace-events +++ b/hw/timer/trace-events @@ -60,3 +60,6 @@ systick_write(uint64_t addr, uint32_t value, unsigned size) "systick write addr cmsdk_apb_timer_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB timer read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u" cmsdk_apb_timer_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB timer write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u" cmsdk_apb_timer_reset(void) "CMSDK APB timer: reset" + +# hw/timer/xlnx-zynqmp-rtc.c +xlnx_zynqmp_rtc_gettime(int year, int month, int day, int hour, int min, int sec) "Get time from host: %d-%d-%d %2d:%02d:%02d"
Allow the guest to determine the time set from the QEMU command line. This includes adding a trace event to debug the new time. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- V5: - Recalculate tick_offset after migration V4: - Use the .unimp property V3: - Store an offset value - Use mktimegm() - Log unimplemented writes V2: - Convert DB_PRINT() macro to trace include/hw/timer/xlnx-zynqmp-rtc.h | 3 +++ hw/timer/xlnx-zynqmp-rtc.c | 52 ++++++++++++++++++++++++++++++++++++++ hw/timer/trace-events | 3 +++ 3 files changed, 58 insertions(+)