The first patch addresses a recent regression and hence ought to be considered for 4.14, despite it getting late. I noticed the problem while re-basing the 2nd patch here, which I decided to now re-post despite the original discussion on v1 not having lead to any clear result (i.e. the supposed "leave Dom0 handle the RTC" has never materialized over the past almost 5 years), while I still think that the changed code is at least a little bit of improvement. 1: restore pv_rtc_handler() invocation 2: detect CMOS aliasing on ports other than 0x70/0x71 Jan
This was lost when making the logic accessible to PVH Dom0.
While doing so make the access to the global function pointer safe
against races (as noticed by Roger): The only current user wants to be
invoked just once (but can tolerate to be invoked multiple times),
zapping the pointer at that point.
Fixes: 835d8d69d96a ("x86/rtc: provide mediated access to RTC for PVH dom0")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Latch pointer under lock.
v2: New.
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1148,6 +1148,8 @@ void rtc_guest_write(unsigned int port,
switch ( port )
{
+ typeof(pv_rtc_handler) hook;
+
case RTC_PORT(0):
/*
* All PV domains (and PVH dom0) are allowed to write to the latched
@@ -1160,6 +1162,14 @@ void rtc_guest_write(unsigned int port,
case RTC_PORT(1):
if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) )
break;
+
+ spin_lock_irqsave(&rtc_lock, flags);
+ hook = pv_rtc_handler;
+ spin_unlock_irqrestore(&rtc_lock, flags);
+
+ if ( hook )
+ hook(currd->arch.cmos_idx & 0x7f, data);
+
spin_lock_irqsave(&rtc_lock, flags);
outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0));
outb(data, RTC_PORT(1));
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 15 July 2020 12:57
> To: xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>;
> Roger Pau Monné <roger.pau@citrix.com>
> Subject: [PATCH v3 1/2] x86: restore pv_rtc_handler() invocation
>
> This was lost when making the logic accessible to PVH Dom0.
>
> While doing so make the access to the global function pointer safe
> against races (as noticed by Roger): The only current user wants to be
> invoked just once (but can tolerate to be invoked multiple times),
> zapping the pointer at that point.
>
> Fixes: 835d8d69d96a ("x86/rtc: provide mediated access to RTC for PVH dom0")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v3: Latch pointer under lock.
> v2: New.
>
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1148,6 +1148,8 @@ void rtc_guest_write(unsigned int port,
>
> switch ( port )
> {
> + typeof(pv_rtc_handler) hook;
> +
> case RTC_PORT(0):
> /*
> * All PV domains (and PVH dom0) are allowed to write to the latched
> @@ -1160,6 +1162,14 @@ void rtc_guest_write(unsigned int port,
> case RTC_PORT(1):
> if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) )
> break;
> +
> + spin_lock_irqsave(&rtc_lock, flags);
> + hook = pv_rtc_handler;
> + spin_unlock_irqrestore(&rtc_lock, flags);
> +
> + if ( hook )
> + hook(currd->arch.cmos_idx & 0x7f, data);
> +
> spin_lock_irqsave(&rtc_lock, flags);
> outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0));
> outb(data, RTC_PORT(1));
LGTM..
Release-acked-by: Paul Durrant <paul@xen.org>
On Wed, Jul 15, 2020 at 01:56:47PM +0200, Jan Beulich wrote:
> This was lost when making the logic accessible to PVH Dom0.
>
> While doing so make the access to the global function pointer safe
> against races (as noticed by Roger): The only current user wants to be
> invoked just once (but can tolerate to be invoked multiple times),
> zapping the pointer at that point.
>
> Fixes: 835d8d69d96a ("x86/rtc: provide mediated access to RTC for PVH dom0")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Thanks, sorry I have one comment below.
> ---
> v3: Latch pointer under lock.
> v2: New.
>
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1148,6 +1148,8 @@ void rtc_guest_write(unsigned int port,
>
> switch ( port )
> {
> + typeof(pv_rtc_handler) hook;
Nit: FWIW, given the current structure of the function I would just have placed
it together with the rest of the top-level local variables.
> +
> case RTC_PORT(0):
> /*
> * All PV domains (and PVH dom0) are allowed to write to the latched
> @@ -1160,6 +1162,14 @@ void rtc_guest_write(unsigned int port,
> case RTC_PORT(1):
> if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) )
> break;
> +
> + spin_lock_irqsave(&rtc_lock, flags);
> + hook = pv_rtc_handler;
> + spin_unlock_irqrestore(&rtc_lock, flags);
Given that clearing the pv_rtc_handler variable in handle_rtc_once is
not done while holding the rtc_lock, I'm not sure there's much point
in holding the lock here, ie: just doing something like:
hook = pv_rtc_handler;
if ( hook )
hook(currd->arch.cmos_idx & 0x7f, data);
Should be as safe as what you do.
We also assume that setting pv_rtc_handler to NULL is an atomic
operation.
Roger.
On 15.07.2020 14:13, Roger Pau Monné wrote: > On Wed, Jul 15, 2020 at 01:56:47PM +0200, Jan Beulich wrote: >> @@ -1160,6 +1162,14 @@ void rtc_guest_write(unsigned int port, >> case RTC_PORT(1): >> if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) ) >> break; >> + >> + spin_lock_irqsave(&rtc_lock, flags); >> + hook = pv_rtc_handler; >> + spin_unlock_irqrestore(&rtc_lock, flags); > > Given that clearing the pv_rtc_handler variable in handle_rtc_once is > not done while holding the rtc_lock, I'm not sure there's much point > in holding the lock here, ie: just doing something like: > > hook = pv_rtc_handler; > if ( hook ) > hook(currd->arch.cmos_idx & 0x7f, data); > > Should be as safe as what you do. No, the compiler is free to eliminate the local variable and read the global one twice (and it may change contents in between) then. I could use ACCESS_ONCE() or read_atomic() here, but then it would become quite clear that at the same time ... > We also assume that setting pv_rtc_handler to NULL is an atomic > operation. ... this (which isn't different from what we do elsewhere, and we really can't fix everything at the same time) ought to also become ACCESS_ONCE() (or write_atomic()). A compromise might be to use barrier() in place of the locking for now ... Jan
On Wed, Jul 15, 2020 at 02:36:49PM +0200, Jan Beulich wrote: > On 15.07.2020 14:13, Roger Pau Monné wrote: > > On Wed, Jul 15, 2020 at 01:56:47PM +0200, Jan Beulich wrote: > >> @@ -1160,6 +1162,14 @@ void rtc_guest_write(unsigned int port, > >> case RTC_PORT(1): > >> if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) ) > >> break; > >> + > >> + spin_lock_irqsave(&rtc_lock, flags); > >> + hook = pv_rtc_handler; > >> + spin_unlock_irqrestore(&rtc_lock, flags); > > > > Given that clearing the pv_rtc_handler variable in handle_rtc_once is > > not done while holding the rtc_lock, I'm not sure there's much point > > in holding the lock here, ie: just doing something like: > > > > hook = pv_rtc_handler; > > if ( hook ) > > hook(currd->arch.cmos_idx & 0x7f, data); > > > > Should be as safe as what you do. > > No, the compiler is free to eliminate the local variable and read > the global one twice (and it may change contents in between) then. > I could use ACCESS_ONCE() or read_atomic() here, but then it would > become quite clear that at the same time ... > > > We also assume that setting pv_rtc_handler to NULL is an atomic > > operation. > > ... this (which isn't different from what we do elsewhere, and we > really can't fix everything at the same time) ought to also become > ACCESS_ONCE() (or write_atomic()). > > A compromise might be to use barrier() in place of the locking for > now ... Oh, right. Didn't realize you did it in order to prevent optimizations. Using the lock seems also quite weird IMO, so I'm not sure it's much better than just using ACCESS_ONCE (or a barrier). Anyway, I don't want to delay this any longer, so: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Feel free to change to ACCESS_ONCE or barrier if you think it's clearer. Thanks.
On 15.07.2020 15:32, Roger Pau Monné wrote: > On Wed, Jul 15, 2020 at 02:36:49PM +0200, Jan Beulich wrote: >> On 15.07.2020 14:13, Roger Pau Monné wrote: >>> On Wed, Jul 15, 2020 at 01:56:47PM +0200, Jan Beulich wrote: >>>> @@ -1160,6 +1162,14 @@ void rtc_guest_write(unsigned int port, >>>> case RTC_PORT(1): >>>> if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) ) >>>> break; >>>> + >>>> + spin_lock_irqsave(&rtc_lock, flags); >>>> + hook = pv_rtc_handler; >>>> + spin_unlock_irqrestore(&rtc_lock, flags); >>> >>> Given that clearing the pv_rtc_handler variable in handle_rtc_once is >>> not done while holding the rtc_lock, I'm not sure there's much point >>> in holding the lock here, ie: just doing something like: >>> >>> hook = pv_rtc_handler; >>> if ( hook ) >>> hook(currd->arch.cmos_idx & 0x7f, data); >>> >>> Should be as safe as what you do. >> >> No, the compiler is free to eliminate the local variable and read >> the global one twice (and it may change contents in between) then. >> I could use ACCESS_ONCE() or read_atomic() here, but then it would >> become quite clear that at the same time ... >> >>> We also assume that setting pv_rtc_handler to NULL is an atomic >>> operation. >> >> ... this (which isn't different from what we do elsewhere, and we >> really can't fix everything at the same time) ought to also become >> ACCESS_ONCE() (or write_atomic()). >> >> A compromise might be to use barrier() in place of the locking for >> now ... > > Oh, right. Didn't realize you did it in order to prevent > optimizations. Using the lock seems also quite weird IMO, so I'm not > sure it's much better than just using ACCESS_ONCE (or a barrier). > Anyway, I don't want to delay this any longer, so: > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. > Feel free to change to ACCESS_ONCE or barrier if you think it's > clearer. I did so (also on the writer side), not the least based on guessing what Andrew would presumably have preferred. Jan
On Wed, Jul 15, 2020 at 03:51:17PM +0200, Jan Beulich wrote: > On 15.07.2020 15:32, Roger Pau Monné wrote: > > On Wed, Jul 15, 2020 at 02:36:49PM +0200, Jan Beulich wrote: > >> On 15.07.2020 14:13, Roger Pau Monné wrote: > >>> On Wed, Jul 15, 2020 at 01:56:47PM +0200, Jan Beulich wrote: > >>>> @@ -1160,6 +1162,14 @@ void rtc_guest_write(unsigned int port, > >>>> case RTC_PORT(1): > >>>> if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) ) > >>>> break; > >>>> + > >>>> + spin_lock_irqsave(&rtc_lock, flags); > >>>> + hook = pv_rtc_handler; > >>>> + spin_unlock_irqrestore(&rtc_lock, flags); > >>> > >>> Given that clearing the pv_rtc_handler variable in handle_rtc_once is > >>> not done while holding the rtc_lock, I'm not sure there's much point > >>> in holding the lock here, ie: just doing something like: > >>> > >>> hook = pv_rtc_handler; > >>> if ( hook ) > >>> hook(currd->arch.cmos_idx & 0x7f, data); > >>> > >>> Should be as safe as what you do. > >> > >> No, the compiler is free to eliminate the local variable and read > >> the global one twice (and it may change contents in between) then. > >> I could use ACCESS_ONCE() or read_atomic() here, but then it would > >> become quite clear that at the same time ... > >> > >>> We also assume that setting pv_rtc_handler to NULL is an atomic > >>> operation. > >> > >> ... this (which isn't different from what we do elsewhere, and we > >> really can't fix everything at the same time) ought to also become > >> ACCESS_ONCE() (or write_atomic()). > >> > >> A compromise might be to use barrier() in place of the locking for > >> now ... > > > > Oh, right. Didn't realize you did it in order to prevent > > optimizations. Using the lock seems also quite weird IMO, so I'm not > > sure it's much better than just using ACCESS_ONCE (or a barrier). > > Anyway, I don't want to delay this any longer, so: > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > > Feel free to change to ACCESS_ONCE or barrier if you think it's > > clearer. > > I did so (also on the writer side), not the least based on guessing > what Andrew would presumably have preferred. Thanks! Sorry I might be pedantic, but is the ACCESS_ONCE on the write side actually required? I'm not sure I see what ACCESS_ONCE protects against in handle_rtc_once. Roger.
On 15.07.2020 16:51, Roger Pau Monné wrote: > On Wed, Jul 15, 2020 at 03:51:17PM +0200, Jan Beulich wrote: >> On 15.07.2020 15:32, Roger Pau Monné wrote: >>> Feel free to change to ACCESS_ONCE or barrier if you think it's >>> clearer. >> >> I did so (also on the writer side), not the least based on guessing >> what Andrew would presumably have preferred. > > Thanks! Sorry I might be pedantic, but is the ACCESS_ONCE on the write > side actually required? I'm not sure I see what ACCESS_ONCE protects > against in handle_rtc_once. Well, this is all sort of a mess, I think. We have this mixture of ACCESS_ONCE() and read_atomic() / write_atomic(), but I don't think we use them consistently, and I'm not sure either is suitable to deal with all (theoretical) corner cases. read_atomic() / write_atomic() guarantee a single insn to be used to access a piece of data. I'm uncertain whether they also guarantee single access (i.e. that the compiler won't replicate the asm()-s). The wording in gcc doc is pretty precise, but not quite enough imo to be entirely certain. ACCESS_ONCE() guarantees single access, but doesn't guarantee that the compiler wouldn't split this single access into multiple insns. (It's just, like elsewhere, that it would be pretty silly of it if it did.) Yesterday, as said, I tried to in particular do what I expect/guess Andrew would have wanted done. This is despite me not being entirely convinced this is the right thing to do here, i.e. personally I would have preferred read_atomic() / write_atomic(), as I think the intention of what the gcc doc is saying is what we want (taking into consideration both uses of "volatile" in these helpers). Jan
On Thu, Jul 16, 2020 at 12:06:14PM +0200, Jan Beulich wrote:
> On 15.07.2020 16:51, Roger Pau Monné wrote:
> > On Wed, Jul 15, 2020 at 03:51:17PM +0200, Jan Beulich wrote:
> >> On 15.07.2020 15:32, Roger Pau Monné wrote:
> >>> Feel free to change to ACCESS_ONCE or barrier if you think it's
> >>> clearer.
> >>
> >> I did so (also on the writer side), not the least based on guessing
> >> what Andrew would presumably have preferred.
> >
> > Thanks! Sorry I might be pedantic, but is the ACCESS_ONCE on the write
> > side actually required? I'm not sure I see what ACCESS_ONCE protects
> > against in handle_rtc_once.
>
> Well, this is all sort of a mess, I think. We have this mixture of
> ACCESS_ONCE() and read_atomic() / write_atomic(), but I don't think
> we use them consistently, and I'm not sure either is suitable to
> deal with all (theoretical) corner cases.
>
> read_atomic() / write_atomic() guarantee a single insn to be used
> to access a piece of data. I'm uncertain whether they also guarantee
> single access (i.e. that the compiler won't replicate the asm()-s).
Yes, that would be my expectation from my reading of the manual, as
it prevents gcc from: "move it out of loops or omit it on the
assumption that the result from a previous call is still valid".
> The wording in gcc doc is pretty precise, but not quite enough imo
> to be entirely certain.
I agree it's not that precise.
> ACCESS_ONCE() guarantees single access, but doesn't guarantee that
> the compiler wouldn't split this single access into multiple insns.
> (It's just, like elsewhere, that it would be pretty silly of it if
> it did.)
>
> Yesterday, as said, I tried to in particular do what I expect/guess
> Andrew would have wanted done. This is despite me not being entirely
> convinced this is the right thing to do here, i.e. personally I
> would have preferred read_atomic() / write_atomic(), as I think the
> intention of what the gcc doc is saying is what we want (taking
> into consideration both uses of "volatile" in these helpers).
Well, gcc states:
"Note that the compiler can move even volatile asm instructions
relative to other code, including across jump instructions."
So I think we likely want to use {read/write}_atomic plus a compiler
barrier? I'm not sure anyway how the read of pv_rtc_handler could be
moved, but I guess I'm not that creative :).
AFAICT we require a write_atomic in handle_rtc_once in order to assure
a single instruction is used (no barrier required), and then we
require a read_atomic + a compiler barrier in rtc_guest_write in order
to prevent the compiler from optimizing the accesses to 'hook' in any
way? (that barrier might not be strictly required, as you say it's not
fully clear whether 'asm volatile' doesn't provide the necessary
protection here).
Thanks.
On 16.07.2020 12:31, Roger Pau Monné wrote:
> On Thu, Jul 16, 2020 at 12:06:14PM +0200, Jan Beulich wrote:
>> On 15.07.2020 16:51, Roger Pau Monné wrote:
>>> On Wed, Jul 15, 2020 at 03:51:17PM +0200, Jan Beulich wrote:
>>>> On 15.07.2020 15:32, Roger Pau Monné wrote:
>>>>> Feel free to change to ACCESS_ONCE or barrier if you think it's
>>>>> clearer.
>>>>
>>>> I did so (also on the writer side), not the least based on guessing
>>>> what Andrew would presumably have preferred.
>>>
>>> Thanks! Sorry I might be pedantic, but is the ACCESS_ONCE on the write
>>> side actually required? I'm not sure I see what ACCESS_ONCE protects
>>> against in handle_rtc_once.
>>
>> Well, this is all sort of a mess, I think. We have this mixture of
>> ACCESS_ONCE() and read_atomic() / write_atomic(), but I don't think
>> we use them consistently, and I'm not sure either is suitable to
>> deal with all (theoretical) corner cases.
>>
>> read_atomic() / write_atomic() guarantee a single insn to be used
>> to access a piece of data. I'm uncertain whether they also guarantee
>> single access (i.e. that the compiler won't replicate the asm()-s).
>
> Yes, that would be my expectation from my reading of the manual, as
> it prevents gcc from: "move it out of loops or omit it on the
> assumption that the result from a previous call is still valid".
>
>> The wording in gcc doc is pretty precise, but not quite enough imo
>> to be entirely certain.
>
> I agree it's not that precise.
>
>> ACCESS_ONCE() guarantees single access, but doesn't guarantee that
>> the compiler wouldn't split this single access into multiple insns.
>> (It's just, like elsewhere, that it would be pretty silly of it if
>> it did.)
>>
>> Yesterday, as said, I tried to in particular do what I expect/guess
>> Andrew would have wanted done. This is despite me not being entirely
>> convinced this is the right thing to do here, i.e. personally I
>> would have preferred read_atomic() / write_atomic(), as I think the
>> intention of what the gcc doc is saying is what we want (taking
>> into consideration both uses of "volatile" in these helpers).
>
> Well, gcc states:
>
> "Note that the compiler can move even volatile asm instructions
> relative to other code, including across jump instructions."
>
> So I think we likely want to use {read/write}_atomic plus a compiler
> barrier? I'm not sure anyway how the read of pv_rtc_handler could be
> moved, but I guess I'm not that creative :).
>
> AFAICT we require a write_atomic in handle_rtc_once in order to assure
> a single instruction is used (no barrier required), and then we
> require a read_atomic + a compiler barrier in rtc_guest_write in order
> to prevent the compiler from optimizing the accesses to 'hook' in any
> way? (that barrier might not be strictly required, as you say it's not
> fully clear whether 'asm volatile' doesn't provide the necessary
> protection here).
Yes, but I'd really like to wait for Andrew's input here before we
make any further adjustments. In any even what has gone in yesterday
is already better than what the original code was that needed
restoring.
Jan
On 16/07/2020 11:06, Jan Beulich wrote: > ACCESS_ONCE() guarantees single access, but doesn't guarantee that > the compiler wouldn't split this single access into multiple insns. ACCESS_ONCE() does guarantee single accesses for any natural integer size. There is a section about this specifically in Linux's memory-barriers.txt, and this isn't the first time I've pointed it out... ~Andrew
On 20.07.2020 17:28, Andrew Cooper wrote: > On 16/07/2020 11:06, Jan Beulich wrote: >> ACCESS_ONCE() guarantees single access, but doesn't guarantee that >> the compiler wouldn't split this single access into multiple insns. > > ACCESS_ONCE() does guarantee single accesses for any natural integer size. > > There is a section about this specifically in Linux's > memory-barriers.txt, and this isn't the first time I've pointed it out... There indeed is text stating this, but I can't find any word on why they believe this is the case. My understanding of volatile is that it guarantees no more (and also no less) accesses to any single storage location than indicated by the source. But it doesn't prevent "tearing" of accesses. And really, how could it, considering that volatile can also be applied to types that aren't basic ones, and hence in particular to ones that can't possibly be accessed by a single insn? I'd be glad to see you point out the aspect I'm missing here. Jan
On 20.07.2020 18:27, Jan Beulich wrote:
> On 20.07.2020 17:28, Andrew Cooper wrote:
>> On 16/07/2020 11:06, Jan Beulich wrote:
>>> ACCESS_ONCE() guarantees single access, but doesn't guarantee that
>>> the compiler wouldn't split this single access into multiple insns.
>>
>> ACCESS_ONCE() does guarantee single accesses for any natural integer size.
>>
>> There is a section about this specifically in Linux's
>> memory-barriers.txt, and this isn't the first time I've pointed it out...
>
> There indeed is text stating this, but I can't find any word on
> why they believe this is the case. My understanding of volatile
> is that it guarantees no more (and also no less) accesses to
> any single storage location than indicated by the source. But
> it doesn't prevent "tearing" of accesses. And really, how could
> it, considering that volatile can also be applied to types that
> aren't basic ones, and hence in particular to ones that can't
> possibly be accessed by a single insn?
To avoid a possible reference to *_ONCE() only accepting scalar
types - even the more explicit logic in the Linux constructs
permits "long long". Yet (I'm inclined to say of course) the
compiler makes no effort at all to carry out such a 64-bit
access as a single (atomic) insn on a 32-bit arch (i.e. cmpxchg8b
on ix86, if available). If there really was such a guarantee, it
surely would need to, or diagnose that it can't.
Furthermore I've looked at the current implementation of their
macros:
/*
* Use __READ_ONCE() instead of READ_ONCE() if you do not require any
* atomicity or dependency ordering guarantees. Note that this may result
* in tears!
*/
#define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x))
#define __READ_ONCE_SCALAR(x) \
({ \
__unqual_scalar_typeof(x) __x = __READ_ONCE(x); \
smp_read_barrier_depends(); \
(typeof(x))__x; \
})
#define READ_ONCE(x) \
({ \
compiletime_assert_rwonce_type(x); \
__READ_ONCE_SCALAR(x); \
})
The difference between __READ_ONCE() and READ_ONCE() effectively
is merely the smp_read_barrier_depends() afaics. Hence to me the
"tears" in the comment can only refer to "tear drops", not to
"torn accesses". The comment ahead of
compiletime_assert_rwonce_type() is also "interesting":
/*
* Yes, this permits 64-bit accesses on 32-bit architectures. These will
* actually be atomic in some cases (namely Armv7 + LPAE), but for others we
* rely on the access being split into 2x32-bit accesses for a 32-bit quantity
* (e.g. a virtual address) and a strong prevailing wind.
*/
(I'm struggling to see what extra effects this construct has over
the type enforcement by __unqual_scalar_typeof().)
Jan
... in order to also intercept accesses through the alias ports.
Also stop intercepting accesses to the CMOS ports if we won't ourselves
use the CMOS RTC.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Re-base over change to earlier patch.
v2: Re-base.
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -670,6 +670,80 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
return ret;
}
+#ifndef COMPAT
+#include <asm/mc146818rtc.h>
+
+unsigned int __read_mostly cmos_alias_mask;
+
+static int __init probe_cmos_alias(void)
+{
+ unsigned int i, offs;
+
+ if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC )
+ return 0;
+
+ for ( offs = 2; offs < 8; offs <<= 1 )
+ {
+ bool read = true;
+
+ for ( i = RTC_REG_D + 1; i < 0x80; ++i )
+ {
+ uint8_t normal, alt;
+ unsigned long flags;
+
+ if ( i == acpi_gbl_FADT.century )
+ continue;
+
+ spin_lock_irqsave(&rtc_lock, flags);
+
+ normal = CMOS_READ(i);
+ if ( inb(RTC_PORT(offs)) != i )
+ read = false;
+
+ alt = inb(RTC_PORT(offs + 1));
+
+ spin_unlock_irqrestore(&rtc_lock, flags);
+
+ if ( normal != alt )
+ break;
+
+ process_pending_softirqs();
+ }
+ if ( i == 0x80 )
+ {
+ cmos_alias_mask |= offs;
+ printk(XENLOG_INFO "CMOS aliased at %02x, index %s\n",
+ RTC_PORT(offs), read ? "r/w" : "w/o");
+ }
+ }
+
+ return 0;
+}
+__initcall(probe_cmos_alias);
+
+/* Has the administrator granted sufficient permission for this I/O access? */
+bool admin_io_okay(unsigned int port, unsigned int bytes,
+ const struct domain *d)
+{
+ /*
+ * Port 0xcf8 (CONFIG_ADDRESS) is only visible for DWORD accesses.
+ * We never permit direct access to that register.
+ */
+ if ( (port == 0xcf8) && (bytes == 4) )
+ return false;
+
+ /*
+ * We also never permit direct access to the RTC/CMOS registers
+ * if we may be accessing the RTC ones ourselves.
+ */
+ if ( !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) &&
+ ((port & ~(cmos_alias_mask | 1)) == RTC_PORT(0)) )
+ return false;
+
+ return ioports_access_permitted(d, port, port + bytes - 1);
+}
+#endif /* COMPAT */
+
/*
* Local variables:
* mode: C
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -198,24 +198,6 @@ static bool guest_io_okay(unsigned int p
return false;
}
-/* Has the administrator granted sufficient permission for this I/O access? */
-static bool admin_io_okay(unsigned int port, unsigned int bytes,
- const struct domain *d)
-{
- /*
- * Port 0xcf8 (CONFIG_ADDRESS) is only visible for DWORD accesses.
- * We never permit direct access to that register.
- */
- if ( (port == 0xcf8) && (bytes == 4) )
- return false;
-
- /* We also never permit direct access to the RTC/CMOS registers. */
- if ( ((port & ~1) == RTC_PORT(0)) )
- return false;
-
- return ioports_access_permitted(d, port, port + bytes - 1);
-}
-
static bool pci_cfg_ok(struct domain *currd, unsigned int start,
unsigned int size, uint32_t *write)
{
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -48,7 +48,7 @@
#include <xen/cpu.h>
#include <asm/nmi.h>
#include <asm/alternative.h>
-#include <asm/mc146818rtc.h>
+#include <asm/iocap.h>
#include <asm/cpuid.h>
#include <asm/spec_ctrl.h>
#include <asm/guest.h>
@@ -2009,37 +2009,33 @@ int __hwdom_init xen_in_range(unsigned l
static int __hwdom_init io_bitmap_cb(unsigned long s, unsigned long e,
void *ctx)
{
- struct domain *d = ctx;
+ const struct domain *d = ctx;
unsigned int i;
ASSERT(e <= INT_MAX);
for ( i = s; i <= e; i++ )
- __clear_bit(i, d->arch.hvm.io_bitmap);
+ if ( admin_io_okay(i, 1, d) )
+ __clear_bit(i, d->arch.hvm.io_bitmap);
return 0;
}
void __hwdom_init setup_io_bitmap(struct domain *d)
{
- int rc;
+ if ( !is_hvm_domain(d) )
+ return;
- if ( is_hvm_domain(d) )
- {
- bitmap_fill(d->arch.hvm.io_bitmap, 0x10000);
- rc = rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000,
- io_bitmap_cb, d);
- BUG_ON(rc);
- /*
- * NB: we need to trap accesses to 0xcf8 in order to intercept
- * 4 byte accesses, that need to be handled by Xen in order to
- * keep consistency.
- * Access to 1 byte RTC ports also needs to be trapped in order
- * to keep consistency with PV.
- */
- __set_bit(0xcf8, d->arch.hvm.io_bitmap);
- __set_bit(RTC_PORT(0), d->arch.hvm.io_bitmap);
- __set_bit(RTC_PORT(1), d->arch.hvm.io_bitmap);
- }
+ bitmap_fill(d->arch.hvm.io_bitmap, 0x10000);
+ if ( rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000,
+ io_bitmap_cb, d) )
+ BUG();
+
+ /*
+ * We need to trap 4-byte accesses to 0xcf8 (see admin_io_okay(),
+ * guest_io_read(), and guest_io_write()), which isn't covered by
+ * the admin_io_okay() check in io_bitmap_cb().
+ */
+ __set_bit(0xcf8, d->arch.hvm.io_bitmap);
}
/*
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1092,7 +1092,10 @@ static unsigned long get_cmos_time(void)
if ( seconds < 60 )
{
if ( rtc.sec != seconds )
+ {
cmos_rtc_probe = false;
+ acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC;
+ }
break;
}
@@ -1114,7 +1117,7 @@ unsigned int rtc_guest_read(unsigned int
unsigned long flags;
unsigned int data = ~0;
- switch ( port )
+ switch ( port & ~cmos_alias_mask )
{
case RTC_PORT(0):
/*
@@ -1126,11 +1129,12 @@ unsigned int rtc_guest_read(unsigned int
break;
case RTC_PORT(1):
- if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) )
+ if ( !ioports_access_permitted(currd, port - 1, port) )
break;
spin_lock_irqsave(&rtc_lock, flags);
- outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0));
- data = inb(RTC_PORT(1));
+ outb(currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(1))),
+ port - 1);
+ data = inb(port);
spin_unlock_irqrestore(&rtc_lock, flags);
break;
@@ -1146,9 +1150,10 @@ void rtc_guest_write(unsigned int port,
struct domain *currd = current->domain;
unsigned long flags;
- switch ( port )
+ switch ( port & ~cmos_alias_mask )
{
typeof(pv_rtc_handler) hook;
+ unsigned int idx;
case RTC_PORT(0):
/*
@@ -1160,19 +1165,21 @@ void rtc_guest_write(unsigned int port,
break;
case RTC_PORT(1):
- if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) )
+ if ( !ioports_access_permitted(currd, port - 1, port) )
break;
spin_lock_irqsave(&rtc_lock, flags);
hook = pv_rtc_handler;
spin_unlock_irqrestore(&rtc_lock, flags);
+ idx = currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(1)));
+
if ( hook )
- hook(currd->arch.cmos_idx & 0x7f, data);
+ hook(idx, data);
spin_lock_irqsave(&rtc_lock, flags);
- outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0));
- outb(data, RTC_PORT(1));
+ outb(idx, port - 1);
+ outb(data, port);
spin_unlock_irqrestore(&rtc_lock, flags);
break;
--- a/xen/include/asm-x86/iocap.h
+++ b/xen/include/asm-x86/iocap.h
@@ -18,4 +18,7 @@
(!rangeset_is_empty((d)->iomem_caps) || \
!rangeset_is_empty((d)->arch.ioport_caps))
+bool admin_io_okay(unsigned int port, unsigned int bytes,
+ const struct domain *d);
+
#endif /* __X86_IOCAP_H__ */
--- a/xen/include/asm-x86/mc146818rtc.h
+++ b/xen/include/asm-x86/mc146818rtc.h
@@ -9,6 +9,8 @@
extern spinlock_t rtc_lock; /* serialize CMOS RAM access */
+extern unsigned int cmos_alias_mask;
+
/**********************************************************************
* register summary
**********************************************************************/
On Wed, Jul 15, 2020 at 01:57:07PM +0200, Jan Beulich wrote: > ... in order to also intercept accesses through the alias ports. > > Also stop intercepting accesses to the CMOS ports if we won't ourselves > use the CMOS RTC. Will wait for v4 with the fixed additions to PVH dom0. Roger.
On Mon, Jul 20, 2020 at 01:11:18PM +0200, Roger Pau Monné wrote: > On Wed, Jul 15, 2020 at 01:57:07PM +0200, Jan Beulich wrote: > > ... in order to also intercept accesses through the alias ports. > > > > Also stop intercepting accesses to the CMOS ports if we won't ourselves > > use the CMOS RTC. > > Will wait for v4 with the fixed additions to PVH dom0. I think this is waiting for a v4, let me know if that's not the case. Thanks, Roger.
© 2016 - 2024 Red Hat, Inc.