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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2023 10:34, Roger Pau Monné wrote:
> 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.

That's true for PV Dom0, but not necessarily for PVH. Plus by denying
access to the aliases we also guard against bugs in Dom0, if some
component thinks there's something else at those ports (as they
indeed were used for other purposes by various vendors).

>> @@ -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?

No, the smallest sensible increment is the lowest bit set in
pic_alias_mask. There's specifically no shifting involved here (just
mentioning it because you use the word). E.g. if the aliasing was at
bits 2 and 3 (pic_alias_mask=0x0c), we'd need to deny access to 20/21,
24/25, 28/29, and 2C/2D, i.e. at an increment of 4.

>> --- 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?

Late hwdom "inherits" Dom0's permissions (really the permissions of the
two domains are swapped in late_hwdom_init(), leaving Dom0 with no
permissions at all by default).

>> +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.

I can certainly switch, no problem. For the variable it would be i8259A then,
though.

Jan