On Fri, Jul 17, 2020 at 03:10:43PM +0200, Jan Beulich wrote:
> Since we intercept RTC/CMOS port accesses, let's do so consistently in
> all cases, i.e. also for e.g. a dword access to [006E,0071]. To avoid
> the risk of unintended impact on Dom0 code actually doing so (despite
> the belief that none ought to exist), also extend
> guest_io_{read,write}() to decompose accesses where some ports are
> allowed to be directly accessed and some aren't.
Wouldn't the same apply to displaced accesses to port 0xcf8?
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -210,7 +210,7 @@ static bool admin_io_okay(unsigned int p
> return false;
>
> /* We also never permit direct access to the RTC/CMOS registers. */
> - if ( ((port & ~1) == RTC_PORT(0)) )
> + if ( port <= RTC_PORT(1) && port + bytes > RTC_PORT(0) )
> return false;
>
> return ioports_access_permitted(d, port, port + bytes - 1);
> @@ -297,6 +297,17 @@ static uint32_t guest_io_read(unsigned i
> if ( pci_cfg_ok(currd, port & 3, size, NULL) )
> sub_data = pci_conf_read(currd->arch.pci_cf8, port & 3,
> size);
> }
> + else if ( ioports_access_permitted(currd, port, port) )
> + {
> + if ( bytes > 1 && !(port & 1) &&
> + ioports_access_permitted(currd, port, port + 1) )
> + {
> + sub_data = inw(port);
> + size = 2;
> + }
> + else
> + sub_data = inb(port);
> + }
>
> if ( size == 4 )
> return sub_data;
> @@ -373,25 +384,31 @@ static int read_io(unsigned int port, un
> return X86EMUL_OKAY;
> }
>
> +static void _guest_io_write(unsigned int port, unsigned int bytes,
> + uint32_t data)
There's nothing guest specific about this function I think? If so you
could drop the _guest_ prefix and just name it io_write?
> +{
> + switch ( bytes )
> + {
> + case 1:
> + outb((uint8_t)data, port);
> + if ( amd_acpi_c1e_quirk )
> + amd_check_disable_c1e(port, (uint8_t)data);
> + break;
> + case 2:
> + outw((uint16_t)data, port);
> + break;
> + case 4:
> + outl(data, port);
> + break;
> + }
Newlines after break statements would be nice, and maybe add a
default: ASSERT_UNREACHABLE() case to be on the safe side?
Thanks, Roger.
