On 08.06.2020 17:56, Roger Pau Monné wrote:
> On Mon, Jun 08, 2020 at 01:47:26PM +0200, Jan Beulich wrote:
>> On 08.06.2020 12:29, Roger Pau Monne wrote:
>>> Mediated access to the RTC was provided for PVHv1 dom0 using the PV
>>> code paths (guest_io_{write/read}), but those accesses where never
>>> implemented for PVHv2 dom0. This patch provides such mediated accesses
>>> to the RTC for PVH dom0, just like it's provided for a classic PV
>>> dom0.
>>>
>>> Pull out some of the RTC logic from guest_io_{read/write} into
>>> specific helpers that can be used by both PV and HVM guests. The
>>> setup of the handlers for PVH is done in rtc_init, which is already
>>> used to initialize the fully emulated RTC.
>>>
>>> Without this a Linux PVH dom0 will read garbage when trying to access
>>> the RTC, and one vCPU will be constantly looping in
>>> rtc_timer_do_work.
>>>
>>> Note that such issue doesn't happen on domUs because the ACPI
>>> NO_CMOS_RTC flag is set in FADT, which prevents the OS from accessing
>>> the RTC. Also the X86_EMU_RTC flag is not set for PVH dom0, as the
>>> accesses are not emulated but rather forwarded to the physical
>>> hardware.
>>>
>>> No functional change expected for classic PV dom0.
>>>
>>> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeul...@suse.com>
>> preferably with ...
>>
>>> @@ -1110,6 +1111,67 @@ static unsigned long get_cmos_time(void)
>>> return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
>>> }
>>>
>>> +/* Helpers for guest accesses to the physical RTC. */
>>> +unsigned int rtc_guest_read(unsigned int port)
>>> +{
>>> + const struct domain *currd = current->domain;
>>> + unsigned long flags;
>>> + unsigned int data = ~0;
>>> +
>>> + switch ( port )
>>> + {
>>> + case RTC_PORT(0):
>>> + /*
>>> + * All PV domains are allowed to read the latched value of the
>>> first
>>> + * RTC port. This is useful in order to store data when debugging.
>>> + */
>>
>> ... at least the 2nd sentence of this and ...
>>
>>> +void rtc_guest_write(unsigned int port, unsigned int data)
>>> +{
>>> + struct domain *currd = current->domain;
>>> + unsigned long flags;
>>> +
>>> + switch ( port )
>>> + {
>>> + case RTC_PORT(0):
>>> + /*
>>> + * All PV domains are allowed to write to the latched value of the
>>> + * first RTC port. This is useful in order to store data when
>>> + * debugging.
>>> + */
>>
>> ... this comment dropped again. This justification of the possible
>> usefulness is my very private guessing. Just like the original code
>> was, I think we could leave this uncommented altogether.
>
> Hm, as you wish. I would prefer to leave something similar to the
> first part of the comment, what about:
>
> /*
> * All PV domains (and PVH dom0) are allowed to write/read to the
> * latched value of the first RTC port, as there's no access to the
> * physical IO ports.
> */
Fine with me.
> I can adjust and then add a newline after the break in the RTC_PORT(0)
> case which I missed.
In this small a switch() I don't view this as mandatory, so I'd be fine
to also simply exchange the comments while committing. If there's a new
version by then (tomorrow, I guess), I'll use it of course.
Oh, wait - you mean after the RTC_PORT(1) cases, don't you? Yes, for
consistency having blank lines there would be nice indeed. But still
something that could be done while committing, if you like.
Jan
