[PATCH v2 0/3] x86: Dom0 I/O port access permissions

[PATCH v2 0/3] x86: Dom0 I/O port access permissions

[Jan Beulich]

Following on from the CMOS/RTC port aliasing change, there are some
more missing restrictions; in particular there's more port aliasing to
be aware of. But first of all introduce a command line option to allow
suppressing this probing of aliases, as was requested.

Of course an alternative to all of this would be to do away with all
policy-only ioports_deny_access() in dom0_setup_permissions(), leaving
in place only ones which are truly required for functionality reasons.

1: allow to suppress port-alias probing
2: detect PIC aliasing on ports other than 0x[2A][01]
3: detect PIT aliasing on ports other than 0x4[0-3]

Jan

[PATCH v2 1/3] x86: allow to suppress port-alias probing

[Jan Beulich]

By default there's already no use for this when we run in shim mode.
Plus there may also be a need to suppress the probing in case of issues
with it. Before introducing further port alias probing, introduce a
command line option allowing to bypass it, default it to on when in shim
mode, and gate RTC/CMOS port alias probing on it.

Requested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
While "probe-port-aliases" is longish, shorter forms (e.g. "port-probe")
partially lose the intended meaning.
---
v2: New.

--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2000,6 +2000,17 @@ INVPCID is supported and not disabled vi
 This is a mask of C-states which are to be used preferably.  This option is
 applicable only on hardware were certain C-states are exclusive of one another.
 
+### probe-port-aliases (x86)
+> `= <boolean>`
+
+> Default: `true` outside of shim mode, `false` in shim mode
+
+Certain devices accessible by I/O ports may be accessible also through "alias"
+ports (originally a result of incomplete address decoding).  When such devices
+are solely under Xen's control, Xen disallows even Dom0 access to the "primary"
+ports.  When alias probing is active and aliases are detected, "alias" ports
+would then be treated similar to the "primary" ones.
+
 ### psr (Intel)
 > `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | cos_max:<integer> | cdp:<boolean> )`
 
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -47,6 +47,7 @@ extern unsigned long highmem_start;
 #endif
 
 extern int8_t opt_smt;
+extern int8_t opt_probe_port_aliases;
 
 #ifdef CONFIG_SHADOW_PAGING
 extern bool opt_dom0_shadow;
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -75,6 +75,9 @@ static bool __initdata opt_invpcid = tru
 boolean_param("invpcid", opt_invpcid);
 bool __read_mostly use_invpcid;
 
+int8_t __initdata opt_probe_port_aliases = -1;
+boolean_param("probe-port-aliases", opt_probe_port_aliases);
+
 /* Only used in asm code and within this source file */
 unsigned long asmlinkage __read_mostly cr4_pv32_mask;
 
@@ -1844,6 +1847,9 @@ void asmlinkage __init noreturn __start_
     /* Low mappings were only needed for some BIOS table parsing. */
     zap_low_mappings();
 
+    if ( opt_probe_port_aliases < 0 )
+        opt_probe_port_aliases = !pv_shim;
+
     init_apic_mappings();
 
     normalise_cpu_order();
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1253,7 +1253,8 @@ static int __init cf_check probe_cmos_al
 {
     unsigned int offs;
 
-    if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC )
+    if ( (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) ||
+         !opt_probe_port_aliases )
         return 0;
 
     for ( offs = 2; offs < 8; offs <<= 1 )

[PATCH v2 2/3] x86: detect PIC aliasing on ports other than 0x[2A][01]

[Jan Beulich]

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

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Use new command line option. s/pic/8252A/. Re-base over new earlier
    patch. Use ISOLATE_LSB().

--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -467,7 +467,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 )
@@ -480,10 +480,16 @@ 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 = ISOLATE_LSB(i8259A_alias_mask) ?: 2;
+          offs <= i8259A_alias_mask; offs += i )
+    {
+        if ( offs & ~i8259A_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);
+    }
 
     /* ELCR of both PICs. */
     rc |= ioports_deny_access(d, 0x4D0, 0x4D1);
--- 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>
 
@@ -333,6 +334,58 @@ void __init make_8259A_irq(unsigned int
     irq_to_desc(irq)->handler = &i8259A_irq_type;
 }
 
+unsigned int __initdata i8259A_alias_mask;
+
+static void __init probe_8259A_alias(void)
+{
+    unsigned int mask = 0x1e;
+    uint8_t val = 0;
+
+    if ( !opt_probe_port_aliases )
+        return;
+
+    /*
+     * 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 = ISOLATE_LSB(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);
+        i8259A_alias_mask = mask;
+    }
+}
+
 static struct irqaction __read_mostly cascade = { no_action, "cascade", NULL};
 
 void __init init_IRQ(void)
@@ -343,6 +396,8 @@ void __init init_IRQ(void)
 
     init_8259A(0);
 
+    probe_8259A_alias();
+
     for (irq = 0; platform_legacy_irq(irq); irq++) {
         struct irq_desc *desc = irq_to_desc(irq);
         
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -46,6 +46,8 @@ extern uint8_t kbd_shift_flags;
 extern unsigned long highmem_start;
 #endif
 
+extern unsigned int i8259A_alias_mask;
+
 extern int8_t opt_smt;
 extern int8_t opt_probe_port_aliases;
 

[PATCH v2 3/3] x86: detect PIT aliasing on ports other than 0x4[0-3]

[Jan Beulich]

... in order to also deny Dom0 access through the alias ports. Without
this it is only giving the impression of denying access to PIT. Unlike
for CMOS/RTC, do detection pretty early, to avoid disturbing normal
operation later on (even if typically we won't use much of the PIT).

Like for CMOS/RTC a fundamental assumption of the probing is that reads
from the probed alias port won't have side effects (beyond such that PIT
reads have anyway) in case it does not alias the PIT's.

At to the port 0x61 accesses: Unlike other accesses we do, this masks
off the top four bits (in addition to the bottom two ones), following
Intel chipset documentation saying that these (read-only) bits should
only be written with zero.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
If Xen was running on top of another instance of itself (in HVM mode,
not PVH, i.e. not as a shim), prior to 14f42af3f52d ('x86/vPIT: account
for "counter stopped" time') I'm afraid our vPIT logic would not have
allowed the "Try to further make sure ..." check to pass in the Xen
running on top: We don't respect the gate bit being clear when handling
counter reads. (There are more unhandled [and unmentioned as being so]
aspects of PIT behavior though, yet it's unclear in how far addressing
at least some of them would be useful.)
---
v2: Use new command line option. Re-base over changes to earlier
    patches. Use ISOLATE_LSB().

--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -495,7 +495,11 @@ int __init dom0_setup_permissions(struct
     rc |= ioports_deny_access(d, 0x4D0, 0x4D1);
 
     /* Interval Timer (PIT). */
-    rc |= ioports_deny_access(d, 0x40, 0x43);
+    for ( offs = 0, i = ISOLATE_LSB(pit_alias_mask) ?: 4;
+          offs <= pit_alias_mask; offs += i )
+        if ( !(offs & ~pit_alias_mask) )
+            rc |= ioports_deny_access(d, 0x40 + offs, 0x43 + offs);
+
     /* PIT Channel 2 / PC Speaker Control. */
     rc |= ioports_deny_access(d, 0x61, 0x61);
 
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -47,6 +47,7 @@ extern unsigned long highmem_start;
 #endif
 
 extern unsigned int i8259A_alias_mask;
+extern unsigned int pit_alias_mask;
 
 extern int8_t opt_smt;
 extern int8_t opt_probe_port_aliases;
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -425,6 +425,72 @@ static struct platform_timesource __init
     .resume = resume_pit,
 };
 
+unsigned int __initdata pit_alias_mask;
+
+static void __init probe_pit_alias(void)
+{
+    unsigned int mask = 0x1c;
+    uint8_t val = 0;
+
+    if ( !opt_probe_port_aliases )
+        return;
+
+    /*
+     * Use channel 2 in mode 0 for probing.  In this mode even a non-initial
+     * count is loaded independent of counting being / becoming enabled.  Thus
+     * we have a 16-bit value fully under our control, to write and then check
+     * whether we can also read it back unaltered.
+     */
+
+    /* Turn off speaker output and disable channel 2 counting. */
+    outb(inb(0x61) & 0x0c, 0x61);
+
+    outb((2 << 6) | (3 << 4) | (0 << 1), PIT_MODE); /* Mode 0, LSB/MSB. */
+
+    do {
+        uint8_t val2;
+        unsigned int offs;
+
+        outb(val, PIT_CH2);
+        outb(val ^ 0xff, PIT_CH2);
+
+        /* Wait for the Null Count bit to clear. */
+        do {
+            /* Latch status. */
+            outb((3 << 6) | (1 << 5) | (1 << 3), PIT_MODE);
+
+            /* Try to make sure we're actually having a PIT here. */
+            val2 = inb(PIT_CH2);
+            if ( (val2 & ~(3 << 6)) != ((3 << 4) | (0 << 1)) )
+                return;
+        } while ( val2 & (1 << 6) );
+
+        /*
+         * Try to further make sure we're actually having a PIT here.
+         *
+         * NB: Deliberately |, not ||, as we always want both reads.
+         */
+        val2 = inb(PIT_CH2);
+        if ( (val2 ^ val) | (inb(PIT_CH2) ^ val ^ 0xff) )
+            return;
+
+        for ( offs = ISOLATE_LSB(mask); offs <= mask; offs <<= 1 )
+        {
+            if ( !(mask & offs) )
+                continue;
+            val2 = inb(PIT_CH2 + offs);
+            if ( (val2 ^ val) | (inb(PIT_CH2 + offs) ^ val ^ 0xff) )
+                mask &= ~offs;
+        }
+    } while ( mask && (val += 0x0b) );  /* Arbitrary uneven number. */
+
+    if ( mask )
+    {
+        dprintk(XENLOG_INFO, "PIT aliasing mask: %02x\n", mask);
+        pit_alias_mask = mask;
+    }
+}
+
 /************************************************************
  * PLATFORM TIMER 2: HIGH PRECISION EVENT TIMER (HPET)
  */
@@ -2414,6 +2480,8 @@ void __init early_time_init(void)
     }
 
     preinit_pit();
+    probe_pit_alias();
+
     tmp = init_platform_timer();
     plt_tsc.frequency = tmp;
 

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

[Nicola Vetrini]

On 2023-12-18 15:48, 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.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Use new command line option. s/pic/8252A/. Re-base over new earlier
>     patch. Use ISOLATE_LSB().
> 

Hi,

coming back to this patch, which I believe didn't receive much feedback 
and thus wasn't committed, for a reason:

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

Here asm/setup is included, which provides the declaration for init_IRQ, 
defined below in the file

> 
>  void __init init_IRQ(void)
> @@ -343,6 +396,8 @@ void __init init_IRQ(void)
> 

which is defined here. This patch would, among other things, address a 
MISRA C Rule 8.4 violation ("A compatible declaration shall be visible 
when an object or function with external linkage is defined"). I did 
send a patch concerned only with the MISRA violation, but correctly it 
was pointed out that this one was doing that and more. Perhaps someone 
can have a look at this?

>      init_8259A(0);
> 
> +    probe_8259A_alias();
> +
>      for (irq = 0; platform_legacy_irq(irq); irq++) {
>          struct irq_desc *desc = irq_to_desc(irq);
> 
> --- a/xen/arch/x86/include/asm/setup.h
> +++ b/xen/arch/x86/include/asm/setup.h
> @@ -46,6 +46,8 @@ extern uint8_t kbd_shift_flags;
>  extern unsigned long highmem_start;
>  #endif
> 
> +extern unsigned int i8259A_alias_mask;
> +
>  extern int8_t opt_smt;
>  extern int8_t opt_probe_port_aliases;

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [PATCH v2 1/3] x86: allow to suppress port-alias probing

[Jason Andryuk]

On 2023-12-18 09:47, Jan Beulich wrote:
> By default there's already no use for this when we run in shim mode.
> Plus there may also be a need to suppress the probing in case of issues
> with it. Before introducing further port alias probing, introduce a
> command line option allowing to bypass it, default it to on when in shim
> mode, and gate RTC/CMOS port alias probing on it.
> 
> Requested-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

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

[Jason Andryuk]

On 2023-12-18 09:48, 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.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Code-wise
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

I appreciate what Jan is trying to achieve.  I also share Roger's 
concern about reading IO ports since we don't know what is there.

I also don't have a good alternative to suggest...

Regards,
Jason

Re: [PATCH v2 3/3] x86: detect PIT aliasing on ports other than 0x4[0-3]

[Jason Andryuk]

On 2023-12-18 09:48, 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 PIT. Unlike
> for CMOS/RTC, do detection pretty early, to avoid disturbing normal
> operation later on (even if typically we won't use much of the PIT).
> 
> Like for CMOS/RTC a fundamental assumption of the probing is that reads
> from the probed alias port won't have side effects (beyond such that PIT
> reads have anyway) in case it does not alias the PIT's.
> 
> At to the port 0x61 accesses: Unlike other accesses we do, this masks

s/At/As/

> off the top four bits (in addition to the bottom two ones), following
> Intel chipset documentation saying that these (read-only) bits should
> only be written with zero.

This might be more useful in a comment next to `& 0x0c`.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> If Xen was running on top of another instance of itself (in HVM mode,
> not PVH, i.e. not as a shim), prior to 14f42af3f52d ('x86/vPIT: account
> for "counter stopped" time') I'm afraid our vPIT logic would not have
> allowed the "Try to further make sure ..." check to pass in the Xen
> running on top: We don't respect the gate bit being clear when handling
> counter reads. (There are more unhandled [and unmentioned as being so]
> aspects of PIT behavior though, yet it's unclear in how far addressing
> at least some of them would be useful.)
> ---
> v2: Use new command line option. Re-base over changes to earlier
>      patches. Use ISOLATE_LSB().
> 
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -495,7 +495,11 @@ int __init dom0_setup_permissions(struct
>       rc |= ioports_deny_access(d, 0x4D0, 0x4D1);
>   
>       /* Interval Timer (PIT). */
> -    rc |= ioports_deny_access(d, 0x40, 0x43);
> +    for ( offs = 0, i = ISOLATE_LSB(pit_alias_mask) ?: 4;
> +          offs <= pit_alias_mask; offs += i )
> +        if ( !(offs & ~pit_alias_mask) )
> +            rc |= ioports_deny_access(d, 0x40 + offs, 0x43 + offs);
> +
>       /* PIT Channel 2 / PC Speaker Control. */
>       rc |= ioports_deny_access(d, 0x61, 0x61);
>   
> --- a/xen/arch/x86/include/asm/setup.h
> +++ b/xen/arch/x86/include/asm/setup.h
> @@ -47,6 +47,7 @@ extern unsigned long highmem_start;
>   #endif
>   
>   extern unsigned int i8259A_alias_mask;
> +extern unsigned int pit_alias_mask;
>   
>   extern int8_t opt_smt;
>   extern int8_t opt_probe_port_aliases;
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -425,6 +425,72 @@ static struct platform_timesource __init
>       .resume = resume_pit,
>   };
>   
> +unsigned int __initdata pit_alias_mask;
> +
> +static void __init probe_pit_alias(void)
> +{
> +    unsigned int mask = 0x1c;
> +    uint8_t val = 0;
> +
> +    if ( !opt_probe_port_aliases )
> +        return;
> +
> +    /*
> +     * Use channel 2 in mode 0 for probing.  In this mode even a non-initial
> +     * count is loaded independent of counting being / becoming enabled.  Thus
> +     * we have a 16-bit value fully under our control, to write and then check
> +     * whether we can also read it back unaltered.
> +     */
> +
> +    /* Turn off speaker output and disable channel 2 counting. */
> +    outb(inb(0x61) & 0x0c, 0x61);
> +
> +    outb((2 << 6) | (3 << 4) | (0 << 1), PIT_MODE); /* Mode 0, LSB/MSB. */

Channel 2, Lobyte/Hibyte, 0b000 Mode 0, (Binary)

#define PIT_MODE_CH2 (2 << 6)
#define PIT_MODE0_16BIT ((3 << 4) | (0 << 1))

outb(PIT_MODE_CH2 | PIT_MODE0_16BIT, PIT_MODE);

> +
> +    do {
> +        uint8_t val2;
> +        unsigned int offs;
> +
> +        outb(val, PIT_CH2);
> +        outb(val ^ 0xff, PIT_CH2);
> +
> +        /* Wait for the Null Count bit to clear. */
> +        do {
> +            /* Latch status. */
> +            outb((3 << 6) | (1 << 5) | (1 << 3), PIT_MODE);

Read-back, Latch status,  read back timer channel 2

> +
> +            /* Try to make sure we're actually having a PIT here. */
> +            val2 = inb(PIT_CH2);
> +            if ( (val2 & ~(3 << 6)) != ((3 << 4) | (0 << 1)) )

if ( (val2 & PIT_RB_MASK) != PIT_MODE0_16BIT )

I think particularly a define for PIT_MODE0_16BIT would be helpful to 
show what is expected to be the same.

> +                return;
> +        } while ( val2 & (1 << 6) );

I think Roger might have mentioned on an earlier version - would it make 
sense to have a counter to prevent looping forever?

Also, FYI, I tested the series.  My test machine didn't show any aliasing.

Thanks,
Jason

> +
> +        /*
> +         * Try to further make sure we're actually having a PIT here.
> +         *
> +         * NB: Deliberately |, not ||, as we always want both reads.
> +         */
> +        val2 = inb(PIT_CH2);
> +        if ( (val2 ^ val) | (inb(PIT_CH2) ^ val ^ 0xff) )
> +            return;
> +
> +        for ( offs = ISOLATE_LSB(mask); offs <= mask; offs <<= 1 )
> +        {
> +            if ( !(mask & offs) )
> +                continue;
> +            val2 = inb(PIT_CH2 + offs);
> +            if ( (val2 ^ val) | (inb(PIT_CH2 + offs) ^ val ^ 0xff) )
> +                mask &= ~offs;
> +        }
> +    } while ( mask && (val += 0x0b) );  /* Arbitrary uneven number. */
> +
> +    if ( mask )
> +    {
> +        dprintk(XENLOG_INFO, "PIT aliasing mask: %02x\n", mask);
> +        pit_alias_mask = mask;
> +    }
> +}
> +
>   /************************************************************
>    * PLATFORM TIMER 2: HIGH PRECISION EVENT TIMER (HPET)
>    */
> @@ -2414,6 +2480,8 @@ void __init early_time_init(void)
>       }
>   
>       preinit_pit();
> +    probe_pit_alias();
> +
>       tmp = init_platform_timer();
>       plt_tsc.frequency = tmp;
>   
> 
> 
> 

Re: [PATCH v2 3/3] x86: detect PIT aliasing on ports other than 0x4[0-3]

[Jan Beulich]

On 10.05.2024 19:40, Jason Andryuk wrote:
> On 2023-12-18 09:48, Jan Beulich wrote:
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -425,6 +425,72 @@ static struct platform_timesource __init
>>       .resume = resume_pit,
>>   };
>>   
>> +unsigned int __initdata pit_alias_mask;
>> +
>> +static void __init probe_pit_alias(void)
>> +{
>> +    unsigned int mask = 0x1c;
>> +    uint8_t val = 0;
>> +
>> +    if ( !opt_probe_port_aliases )
>> +        return;
>> +
>> +    /*
>> +     * Use channel 2 in mode 0 for probing.  In this mode even a non-initial
>> +     * count is loaded independent of counting being / becoming enabled.  Thus
>> +     * we have a 16-bit value fully under our control, to write and then check
>> +     * whether we can also read it back unaltered.
>> +     */
>> +
>> +    /* Turn off speaker output and disable channel 2 counting. */
>> +    outb(inb(0x61) & 0x0c, 0x61);
>> +
>> +    outb((2 << 6) | (3 << 4) | (0 << 1), PIT_MODE); /* Mode 0, LSB/MSB. */
> 
> Channel 2, Lobyte/Hibyte, 0b000 Mode 0, (Binary)
> 
> #define PIT_MODE_CH2 (2 << 6)
> #define PIT_MODE0_16BIT ((3 << 4) | (0 << 1))
> 
> outb(PIT_MODE_CH2 | PIT_MODE0_16BIT, PIT_MODE);

Hmm. I can certainly see the value of introducing such #define-s, but then
while doing so one ought to also adjust other code using constants as done
here (for consistency).

>> +
>> +    do {
>> +        uint8_t val2;
>> +        unsigned int offs;
>> +
>> +        outb(val, PIT_CH2);
>> +        outb(val ^ 0xff, PIT_CH2);
>> +
>> +        /* Wait for the Null Count bit to clear. */
>> +        do {
>> +            /* Latch status. */
>> +            outb((3 << 6) | (1 << 5) | (1 << 3), PIT_MODE);
> 
> Read-back, Latch status,  read back timer channel 2

Was this meant as a request to extend the comment? If so, not quite,
as the line doesn't include any read-back. If not, I'm in trouble seeing
what you mean to tell me here (somewhat similar also for the first line
of your earlier comment still visible in context above).

>> +
>> +            /* Try to make sure we're actually having a PIT here. */
>> +            val2 = inb(PIT_CH2);
>> +            if ( (val2 & ~(3 << 6)) != ((3 << 4) | (0 << 1)) )
> 
> if ( (val2 & PIT_RB_MASK) != PIT_MODE0_16BIT )
> 
> I think particularly a define for PIT_MODE0_16BIT would be helpful to 
> show what is expected to be the same.
> 
>> +                return;
>> +        } while ( val2 & (1 << 6) );
> 
> I think Roger might have mentioned on an earlier version - would it make 
> sense to have a counter to prevent looping forever?

Well, as before: The issue with bounding such loops is that the bound is
going to be entirely arbitrary (and hence easily too large / too small).

> Also, FYI, I tested the series.  My test machine didn't show any aliasing.

That likely was an AMD one then? It's only Intel chipsets I've seen aliasing
on so far, but there it's (almost) all of them (with newer data sheets even
stating that behavior). We could, beyond shim, make the option default in
patch 1 be "false" for systems with AMD CPUs (on the assumption that those
wouldn't have Intel chipsets).

Jan

Re: [PATCH v2 3/3] x86: detect PIT aliasing on ports other than 0x4[0-3]

[Jason Andryuk]

On 2024-05-14 03:43, Jan Beulich wrote:
> On 10.05.2024 19:40, Jason Andryuk wrote:
>> On 2023-12-18 09:48, Jan Beulich wrote:
>>> --- a/xen/arch/x86/time.c
>>> +++ b/xen/arch/x86/time.c
>>> @@ -425,6 +425,72 @@ static struct platform_timesource __init
>>>        .resume = resume_pit,
>>>    };
>>>    
>>> +unsigned int __initdata pit_alias_mask;
>>> +
>>> +static void __init probe_pit_alias(void)
>>> +{
>>> +    unsigned int mask = 0x1c;
>>> +    uint8_t val = 0;
>>> +
>>> +    if ( !opt_probe_port_aliases )
>>> +        return;
>>> +
>>> +    /*
>>> +     * Use channel 2 in mode 0 for probing.  In this mode even a non-initial
>>> +     * count is loaded independent of counting being / becoming enabled.  Thus
>>> +     * we have a 16-bit value fully under our control, to write and then check
>>> +     * whether we can also read it back unaltered.
>>> +     */
>>> +
>>> +    /* Turn off speaker output and disable channel 2 counting. */
>>> +    outb(inb(0x61) & 0x0c, 0x61);
>>> +
>>> +    outb((2 << 6) | (3 << 4) | (0 << 1), PIT_MODE); /* Mode 0, LSB/MSB. */
>>
>> Channel 2, Lobyte/Hibyte, 0b000 Mode 0, (Binary)
>>
>> #define PIT_MODE_CH2 (2 << 6)
>> #define PIT_MODE0_16BIT ((3 << 4) | (0 << 1))
>>
>> outb(PIT_MODE_CH2 | PIT_MODE0_16BIT, PIT_MODE);
> 
> Hmm. I can certainly see the value of introducing such #define-s, but then
> while doing so one ought to also adjust other code using constants as done
> here (for consistency).

I had to look up all these bit values, so I think it's nicer with 
#defines-s.  Particularly, using PIT_MODE0_16BIT for the programming and 
checking shows the relationship.  I wasn't looking to make more work for 
you.  This function is self-contained, so just using them here for the 
time being seems reasonable.

>>> +
>>> +    do {
>>> +        uint8_t val2;
>>> +        unsigned int offs;
>>> +
>>> +        outb(val, PIT_CH2);
>>> +        outb(val ^ 0xff, PIT_CH2);
>>> +
>>> +        /* Wait for the Null Count bit to clear. */
>>> +        do {
>>> +            /* Latch status. */
>>> +            outb((3 << 6) | (1 << 5) | (1 << 3), PIT_MODE);
>>
>> Read-back, Latch status,  read back timer channel 2
> 
> Was this meant as a request to extend the comment? If so, not quite,
> as the line doesn't include any read-back. If not, I'm in trouble seeing
> what you mean to tell me here (somewhat similar also for the first line
> of your earlier comment still visible in context above).

Sorry, these were my notes as I was interpreting the bits.  I should 
have removed them from the email before sending as they aren't 
actionable comments.  Read back was in reference to writing (3 << 6) to 
the mode - not the action of read backing back the value.

>>> +
>>> +            /* Try to make sure we're actually having a PIT here. */
>>> +            val2 = inb(PIT_CH2);
>>> +            if ( (val2 & ~(3 << 6)) != ((3 << 4) | (0 << 1)) )
>>
>> if ( (val2 & PIT_RB_MASK) != PIT_MODE0_16BIT )
>>
>> I think particularly a define for PIT_MODE0_16BIT would be helpful to
>> show what is expected to be the same.
>>
>>> +                return;
>>> +        } while ( val2 & (1 << 6) );
>>
>> I think Roger might have mentioned on an earlier version - would it make
>> sense to have a counter to prevent looping forever?
> 
> Well, as before: The issue with bounding such loops is that the bound is
> going to be entirely arbitrary (and hence easily too large / too small).

Ah, yes.  Your response had slipped my mind.

>> Also, FYI, I tested the series.  My test machine didn't show any aliasing.
> 
> That likely was an AMD one then? It's only Intel chipsets I've seen aliasing
> on so far, but there it's (almost) all of them (with newer data sheets even
> stating that behavior). We could, beyond shim, make the option default in
> patch 1 be "false" for systems with AMD CPUs (on the assumption that those
> wouldn't have Intel chipsets).

Indeed, it was an AMD system, but my sample size is 1.

I didn't realize this was motivated by aliasing being common on Intel 
chipsets.  I think that would be useful to include in the commit messages.

Thanks,
Jason