Add RTC device emulation to vmm.ko

[neel]

Summary

Replace bhyve's minimal RTC emulation with a fully featured one in vmm.ko.

The new RTC emulation supports all interrupt modes: periodic, update ended
and alarm. It is also capable of maintaining the date/time and NVRAM contents
across virtual machine reset. Also, the date/time fields can now be modified
by the guest.

Since bhyve now emulates both the PIT and the RTC there is no need for
"Legacy Replacement Routing" in the HPET so get rid of it.

The RTC device state can be inspected via bhyvectl as follows:
bhyvectl --vm=vm --get-rtc-time
bhyvectl --vm=vm --set-rtc-time=<unix_time_secs>
bhyvectl --vm=vm --rtc-nvram-offset=<offset> --get-rtc-nvram
bhyvectl --vm=vm --rtc-nvram-offset=<offset> --set-rtc-nvram=<value>

Test Plan

https://people.freebsd.org/~neel/bhyve/tests/rtc.tar.gz

[tychon]

I'm still in the process of reviewing this, however at first glance I'm not sure I see any reason to remove the HPET Legacy Routing support.

When you first mentioned it, I assumed that some interface change which made it difficult to support and that it added the to the test matrix. Now seeing the implementation it appears entirely independent from the RTC support.

Since it's not broken and provides additional functionality -- which I can't help bug imagine that some guest out there is relying on -- I don't see the justification from deleting it as there are several other examples of "redundant mechanisms".

[neel]

In D1385#3, @tychon wrote:

I'm still in the process of reviewing this, however at first glance I'm not sure I see any reason to remove the HPET Legacy Routing support.

When you first mentioned it, I assumed that some interface change which made it difficult to support and that it added the to the test matrix. Now seeing the implementation it appears entirely independent from the RTC support.

The reason it appears independent is because I removed LegacyRouting :-)

Since it's not broken and provides additional functionality -- which I can't help bug imagine that some guest out there is relying on -- I don't see the justification from deleting it as there are several other examples of "redundant mechanisms".

LegacyRouting is an optional capability so guests shouldn't be upset if they don't see this capability. I haven't run into any issues with the different guests that I have tested.

Also, LegacyRouting doesn't obviate the need for a complete PIT and RTC device emulation since we'll always need to support guests that depend on them.

The interrupt routing gets complicated with LegacyRouting. For e.g., if LegacyRouting is enabled then RTC periodic interrupts are disconnected from the PIC but the alarm and update-ended interrupts are now routed to the SCI. This is not insurmountable but I don't see a tangible benefit.

In any case the most important reason to deprecate LegacyRouting is because doubles the test matrix.

[tychon]

In D1385#4, @neel wrote:

In D1385#3, @tychon wrote:

I'm still in the process of reviewing this, however at first glance I'm not sure I see any reason to remove the HPET Legacy Routing support.

When you first mentioned it, I assumed that some interface change which made it difficult to support and that it added the to the test matrix. Now seeing the implementation it appears entirely independent from the RTC support.

The reason it appears independent is because I removed LegacyRouting :-)

Since it's not broken and provides additional functionality -- which I can't help bug imagine that some guest out there is relying on -- I don't see the justification from deleting it as there are several other examples of "redundant mechanisms".

LegacyRouting is an optional capability so guests shouldn't be upset if they don't see this capability. I haven't run into any issues with the different guests that I have tested.

Also, LegacyRouting doesn't obviate the need for a complete PIT and RTC device emulation since we'll always need to support guests that depend on them.

The interrupt routing gets complicated with LegacyRouting. For e.g., if LegacyRouting is enabled then RTC periodic interrupts are disconnected from the PIC but the alarm and update-ended interrupts are now routed to the SCI. This is not insurmountable but I don't see a tangible benefit.

In any case the most important reason to deprecate LegacyRouting is because doubles the test matrix.

That is a valid reason... out it goes.

Then with respect to the other code, I'm a bit curious about the VM_RTC_READ/VM_RTC_WRITE interface. I'd sort of expected a size or length parameter -- perhaps length so the NVRAM component can be bulk import/exported -- but even size would be fine. Another alternative is to make the 'value' or 'retval' uint8_t so it's obvious it's a byte-based.

Besides that it looks good to me!

[neel]

In D1385#5, @tychon wrote:

In D1385#4, @neel wrote:

In D1385#3, @tychon wrote:

I'm still in the process of reviewing this, however at first glance I'm not sure I see any reason to remove the HPET Legacy Routing support.

When you first mentioned it, I assumed that some interface change which made it difficult to support and that it added the to the test matrix. Now seeing the implementation it appears entirely independent from the RTC support.

The reason it appears independent is because I removed LegacyRouting :-)

Since it's not broken and provides additional functionality -- which I can't help bug imagine that some guest out there is relying on -- I don't see the justification from deleting it as there are several other examples of "redundant mechanisms".

LegacyRouting is an optional capability so guests shouldn't be upset if they don't see this capability. I haven't run into any issues with the different guests that I have tested.

Also, LegacyRouting doesn't obviate the need for a complete PIT and RTC device emulation since we'll always need to support guests that depend on them.

The interrupt routing gets complicated with LegacyRouting. For e.g., if LegacyRouting is enabled then RTC periodic interrupts are disconnected from the PIC but the alarm and update-ended interrupts are now routed to the SCI. This is not insurmountable but I don't see a tangible benefit.

In any case the most important reason to deprecate LegacyRouting is because doubles the test matrix.

That is a valid reason... out it goes.

Then with respect to the other code, I'm a bit curious about the VM_RTC_READ/VM_RTC_WRITE interface. I'd sort of expected a size or length parameter -- perhaps length so the NVRAM component can be bulk import/exported -- but even size would be fine. Another alternative is to make the 'value' or 'retval' uint8_t so it's obvious it's a byte-based.

Does the following seem alright?

int vm_rtc_write(struct vmctx *ctx, int offset, uint8_t value);
int vm_rtc_read(struct vmctx *ctx, int offset, uint8_t *retval);
int vm_rtc_size(struct vmctx *ctx); /* return size of the nvram including the RTC control/status registers */

Besides that it looks good to me!

Thanks for taking a look.

[tychon]

In D1385#6, @neel wrote:

In D1385#5, @tychon wrote:

Then with respect to the other code, I'm a bit curious about the VM_RTC_READ/VM_RTC_WRITE interface. I'd sort of expected a size or length parameter -- perhaps length so the NVRAM component can be bulk import/exported -- but even size would be fine. Another alternative is to make the 'value' or 'retval' uint8_t so it's obvious it's a byte-based.

Does the following seem alright?

int vm_rtc_write(struct vmctx *ctx, int offset, uint8_t value);
int vm_rtc_read(struct vmctx *ctx, int offset, uint8_t *retval);
int vm_rtc_size(struct vmctx *ctx); /* return size of the nvram including the RTC control/status registers */

In rereading what I wrote, maybe I was too vague. I think that vm_rtc_write() and vm_rtc_read() are sufficient as the size of the device is well known or at least the consumer of these interfaces would be best served by knowing what they were reading or writing.

My suggestion of 'size' was along the lines of a read/write interface which allowed the access of multiple bytes at once. For one at a time interface, I think vm_rtc_size() can be omitted for simplicity.

Besides that it looks good to me!

Thanks for taking a look.

No problem, my hope is to help just a bit and not just make things go really S L O W L Y ;-)

[neel]

In D1385#7, @tychon wrote:

In D1385#6, @neel wrote:

In D1385#5, @tychon wrote:

Then with respect to the other code, I'm a bit curious about the VM_RTC_READ/VM_RTC_WRITE interface. I'd sort of expected a size or length parameter -- perhaps length so the NVRAM component can be bulk import/exported -- but even size would be fine. Another alternative is to make the 'value' or 'retval' uint8_t so it's obvious it's a byte-based.

Does the following seem alright?

int vm_rtc_write(struct vmctx *ctx, int offset, uint8_t value);
int vm_rtc_read(struct vmctx *ctx, int offset, uint8_t *retval);
int vm_rtc_size(struct vmctx *ctx); /* return size of the nvram including the RTC control/status registers */

In rereading what I wrote, maybe I was too vague. I think that vm_rtc_write() and vm_rtc_read() are sufficient as the size of the device is well known or at least the consumer of these interfaces would be best served by knowing what they were reading or writing.

My suggestion of 'size' was along the lines of a read/write interface which allowed the access of multiple bytes at once. For one at a time interface, I think vm_rtc_size() can be omitted for simplicity.

Ok, in that case I'll just change the parameter types to 'uint8_t' to make it obvious that this is a single-byte API.

I was planning to implement something along these lines when bhyve supports suspend/resume:
int vm_rtc_state_save(struct vmctx *ctx, struct vm_rtc_state *state);
int vm_rtc_state_restore(struct vmctx *ctx, struct vm_rtc_state *state);

'struct vm_rtc_state' would hold all 128 bytes of the RTC+NVRAM and possibly some additional information that would be useful when resurrecting the RTC state.

Besides that it looks good to me!

Thanks for taking a look.

No problem, my hope is to help just a bit and not just make things go really S L O W L Y ;-)

[neel]

Tycho, can you accept the revision in phabric?

The workflow seems to require that step before I can close it.

[tychon]

In D1385#8, @neel wrote:

Ok, in that case I'll just change the parameter types to 'uint8_t' to make it obvious that this is a single-byte API.

I was planning to implement something along these lines when bhyve supports suspend/resume:
int vm_rtc_state_save(struct vmctx *ctx, struct vm_rtc_state *state);
int vm_rtc_state_restore(struct vmctx *ctx, struct vm_rtc_state *state);

'struct vm_rtc_state' would hold all 128 bytes of the RTC+NVRAM and possibly some additional information that would be useful when resurrecting the RTC state.

Sounds perfect!

I can't wait to give this a whirl.

Tycho