Re: [PATCH 4/7] x86: detect PIC aliasing on ports other than 0x[2A][01]

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 11, 2023 at 02:06:46PM +0200, Jan Beulich wrote:
> ... in order to also deny Dom0 access through the alias ports. Without
> this it is only giving the impression of denying access to both PICs.
> Unlike for CMOS/RTC, do detection very early, to avoid disturbing normal
> operation later on.
> 
> Like for CMOS/RTC a fundamental assumption of the probing is that reads
> from the probed alias port won't have side effects in case it does not
> alias the respective PIC's one.

I'm slightly concerned about this probing.

Also I'm unsure we can fully isolate the hardware domain like this.
Preventing access to the non-aliased ports is IMO helpful for domains
to realize the PIT is not available, but in any case such accesses
shouldn't happen in the first place, as dom0 must be modified to run
in such mode.

> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -479,7 +479,7 @@ static void __init process_dom0_ioports_
>  int __init dom0_setup_permissions(struct domain *d)
>  {
>      unsigned long mfn;
> -    unsigned int i;
> +    unsigned int i, offs;
>      int rc;
>  
>      if ( pv_shim )
> @@ -492,10 +492,17 @@ int __init dom0_setup_permissions(struct
>  
>      /* Modify I/O port access permissions. */
>  
> -    /* Master Interrupt Controller (PIC). */
> -    rc |= ioports_deny_access(d, 0x20, 0x21);
> -    /* Slave Interrupt Controller (PIC). */
> -    rc |= ioports_deny_access(d, 0xA0, 0xA1);
> +    for ( offs = 0, i = pic_alias_mask & -pic_alias_mask ?: 2;
> +          offs <= pic_alias_mask; offs += i )

I'm a bit lost with this, specifically:

i = pic_alias_mask & -pic_alias_mask ?: 2

Which is then used as the increment step in

offs += i

I could see the usage of pic_alias_mask & -pic_alias_mask in order to
find the first offset, but afterwards don't you need to increment at
single bit left shifts in order to test all possibly set bits in
pic_alias_mask?

> +    {
> +        if ( offs & ~pic_alias_mask )
> +            continue;
> +        /* Master Interrupt Controller (PIC). */
> +        rc |= ioports_deny_access(d, 0x20 + offs, 0x21 + offs);
> +        /* Slave Interrupt Controller (PIC). */
> +        rc |= ioports_deny_access(d, 0xA0 + offs, 0xA1 + offs);
> +    }
> +
>      /* Interval Timer (PIT). */
>      rc |= ioports_deny_access(d, 0x40, 0x43);
>      /* PIT Channel 2 / PC Speaker Control. */
> --- a/xen/arch/x86/i8259.c
> +++ b/xen/arch/x86/i8259.c
> @@ -19,6 +19,7 @@
>  #include <xen/delay.h>
>  #include <asm/apic.h>
>  #include <asm/asm_defns.h>
> +#include <asm/setup.h>
>  #include <io_ports.h>
>  #include <irq_vectors.h>
>  
> @@ -332,6 +333,55 @@ void __init make_8259A_irq(unsigned int
>      irq_to_desc(irq)->handler = &i8259A_irq_type;
>  }
>  
> +unsigned int __initdata pic_alias_mask;

Should this be __hwdom_initdata?  I see it gets used in an __init
function, so I guess this all permissions stuff is not really indented
for a late hardware domain to use?

> +
> +static void __init probe_pic_alias(void)
> +{
> +    unsigned int mask = 0x1e;
> +    uint8_t val = 0;
> +
> +    /*
> +     * The only properly r/w register is OCW1.  While keeping the master
> +     * fully masked (thus also masking anything coming through the slave),
> +     * write all possible 256 values to the slave's base port, and check
> +     * whether the same value can then be read back through any of the
> +     * possible alias ports.  Probing just the slave of course builds on the
> +     * assumption that aliasing is identical for master and slave.
> +     */
> +
> +    outb(0xff, 0x21); /* Fully mask master. */
> +
> +    do {
> +        unsigned int offs;
> +
> +        outb(val, 0xa1);
> +
> +        /* Try to make sure we're actually having a PIC here. */
> +        if ( inb(0xa1) != val )
> +        {
> +            mask = 0;
> +            break;
> +        }
> +
> +        for ( offs = mask & -mask; offs <= mask; offs <<= 1 )
> +        {
> +            if ( !(mask & offs) )
> +                continue;
> +            if ( inb(0xa1 + offs) != val )
> +                mask &= ~offs;
> +        }
> +    } while ( mask && (val += 0x0d) );  /* Arbitrary uneven number. */
> +
> +    outb(cached_A1, 0xa1); /* Restore slave IRQ mask. */
> +    outb(cached_21, 0x21); /* Restore master IRQ mask. */
> +
> +    if ( mask )
> +    {
> +        dprintk(XENLOG_INFO, "PIC aliasing mask: %02x\n", mask);
> +        pic_alias_mask = mask;
> +    }
> +}
> +
>  static struct irqaction __read_mostly cascade = { no_action, "cascade", 
> NULL};
>  
>  void __init init_IRQ(void)
> @@ -342,6 +392,8 @@ void __init init_IRQ(void)
>  
>      init_8259A(0);
>  
> +    probe_pic_alias();

Could we use 8259A instead of pic in the function name and mask
variable?  Just so that it's consistent with how we refer to the PIC
in other parts of the code.

Thanks, Roger.