On Mon, Jul 20, 2020 at 01:58:40PM +0200, Jan Beulich wrote:
> On 20.07.2020 12:52, Roger Pau Monné wrote:
> > 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?
>
> No, CF8 is special - partial accesses have no meaning as to the
> index selection for subsequent CFC accesses. Or else CF9
> couldn't be a standalone port with entirely different
> functionality..
Right:
Reviewed-by: Roger Pau Monné <roger....@citrix.com>
See below.
> >> @@ -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?
>
> Hmm, when choosing the name I decided that (a) it's a helper of
> the other function and (b) it's still guest driven data that we
> output.
Well, the fact that it's guest driven data shouldn't matter much,
because there are no guest-specific checks in the function anyway - it
might as well be used for non-guest driven data AFAICT? (even if it's
not the case ATM).
It's likely that if I have to change code here in the future I will
drop such prefix, but the change is correct regardless of the naming,
so I'm not going to insist.
> >> +{
> >> + 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?
>
> Well, yes, I guess I should. But then if I edit this moved code,
> I guess I'll also get rid of the stray casts.
Was going to also ask for that, but I assumed there might we some
value in making the truncations explicit here. Feel free to drop those
also if you end up making the above adjustments.
Thanks, Roger.
