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

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

[Jan Beulich]

Following on from the CMOS/RTC port aliasing change, there are quite
a few more missing restrictions, and there's more port aliasing to be
aware of.

The last two patches are pretty much RFC for now.

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: don't allow Dom0 access to port CF9
2: don't allow Dom0 access to port 92
3: PVH: deny Dom0 access to the ISA DMA controller
4: detect PIC aliasing on ports other than 0x[2A][01]
5: detect PIT aliasing on ports other than 0x4[0-3]
6: don't allow Dom0 (direct) access to port F0
7: don't allow Dom0 access to ELCR ports

Jan

[PATCH 1/7] x86: don't allow Dom0 access to port CF9

[Jan Beulich]

This allows to initiate machine reset, which we don't want to permit
Dom0 to invoke that way.

While there insert blank lines and convert the sibling PCI config space
port numbers to upper case, matching style earlier in the function.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -503,8 +503,13 @@ int __init dom0_setup_permissions(struct
     /* ACPI PM Timer. */
     if ( pmtmr_ioport )
         rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
-    /* PCI configuration space (NB. 0xcf8 has special treatment). */
-    rc |= ioports_deny_access(d, 0xcfc, 0xcff);
+
+    /* Reset control. */
+    rc |= ioports_deny_access(d, 0xCF9, 0xCF9);
+
+    /* PCI configuration space (NB. 0xCF8 has special treatment). */
+    rc |= ioports_deny_access(d, 0xCFC, 0xCFF);
+
 #ifdef CONFIG_HVM
     if ( is_hvm_domain(d) )
     {

[PATCH 2/7] x86: don't allow Dom0 access to port 92

[Jan Beulich]

Somewhat like port CF9 this may have a bit controlling the CPU's INIT#
signal, and it also may have a bit involved in the driving of A20M#.
Neither of these - just like CF9 - we want to allow Dom0 to drive.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -500,6 +500,10 @@ int __init dom0_setup_permissions(struct
     rc |= ioports_deny_access(d, 0x40, 0x43);
     /* PIT Channel 2 / PC Speaker Control. */
     rc |= ioports_deny_access(d, 0x61, 0x61);
+
+    /* INIT# and alternative A20M# control. */
+    rc |= ioports_deny_access(d, 0x92, 0x92);
+
     /* ACPI PM Timer. */
     if ( pmtmr_ioport )
         rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);

[PATCH 3/7] x86/PVH: deny Dom0 access to the ISA DMA controller

[Jan Beulich]

Unlike PV, a PVH Dom0 has no sensible way of driving the address and
page registers correctly, as it would need to translate guest physical
addresses to host ones. Rather than allowing data corruption to occur
from e.g. the use of a legacy floppy drive, disallow access altogether.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The possible aliases of the page registers (90-9F, except 92) aren't
covered. Unlike the possible alias range 10-1F, which I think is okay
to include here blindly, I guess we'd better probe for aliasing of these
if we wanted to deny access there as well. This is first and foremost
because the range having had wider use on PS/2, and who knows what's
been re-used in that range beyond port 92.

--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -517,6 +517,13 @@ int __init dom0_setup_permissions(struct
 #ifdef CONFIG_HVM
     if ( is_hvm_domain(d) )
     {
+        /* ISA DMA controller, channels 0-3 (incl possible aliases). */
+        rc |= ioports_deny_access(d, 0x00, 0x1F);
+        /* ISA DMA controller, page registers (incl various reserved ones). */
+        rc |= ioports_deny_access(d, 0x80 + !!hvm_port80_allowed, 0x8F);
+        /* ISA DMA controller, channels 4-7 (incl usual aliases). */
+        rc |= ioports_deny_access(d, 0xC0, 0xDF);
+
         /* HVM debug console IO port. */
         rc |= ioports_deny_access(d, XEN_HVM_DEBUGCONS_IOPORT,
                                   XEN_HVM_DEBUGCONS_IOPORT);

[PATCH 4/7] 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>

--- 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 )
+    {
+        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;
+
+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();
+
     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
@@ -52,6 +52,8 @@ extern uint8_t kbd_shift_flags;
 extern unsigned long highmem_start;
 #endif
 
+extern unsigned int pic_alias_mask;
+
 extern int8_t opt_smt;
 
 #ifdef CONFIG_SHADOW_PAGING

[PATCH 5/7] 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), I'm afraid our vPIT logic would not allow
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.)

--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -504,7 +504,11 @@ int __init dom0_setup_permissions(struct
     }
 
     /* Interval Timer (PIT). */
-    rc |= ioports_deny_access(d, 0x40, 0x43);
+    for ( offs = 0, i = pit_alias_mask & -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
@@ -53,6 +53,7 @@ extern unsigned long highmem_start;
 #endif
 
 extern unsigned int pic_alias_mask;
+extern unsigned int pit_alias_mask;
 
 extern int8_t opt_smt;
 
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -425,6 +425,69 @@ 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;
+
+    /*
+     * 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 = mask & -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)
  */
@@ -2390,6 +2453,8 @@ void __init early_time_init(void)
     }
 
     preinit_pit();
+    probe_pit_alias();
+
     tmp = init_platform_timer();
     plt_tsc.frequency = tmp;
 

[PATCH 6/7] x86: don't allow Dom0 (direct) access to port F0

[Jan Beulich]

This controls the driving of IGNNE# (if such emulation is enabled in
hardware), and hence would need proper handling in the hypervisor to be
safe to use by Dom0 (and fully emulating for PVH/HVM DomU-s).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: Really this disabling of access would want to be conditional upon
     the functionality actually being enabled. For AMD this looks to be
     uniformly HWCR[8], but for Intel this is chipset-specific.

Port F1 (and perhaps also further ones up to FF) ought to be applicable
to external coprocessors only, and hence are left alone here.

--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -515,6 +515,9 @@ int __init dom0_setup_permissions(struct
     /* INIT# and alternative A20M# control. */
     rc |= ioports_deny_access(d, 0x92, 0x92);
 
+    /* IGNNE# control. */
+    rc |= ioports_deny_access(d, 0xF0, 0xF0);
+
     /* ACPI PM Timer. */
     if ( pmtmr_ioport )
         rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);

[PATCH 7/7] x86: don't allow Dom0 access to ELCR ports

[Jan Beulich]

Much like the other PIC ports, Dom0 has no business touching these. Even
our own uses are somewhat questionable, as the corresponding IO-APIC
code in Linux is enclosed in a CONFIG_EISA conditional; I don't think
there are any x86-64 EISA systems.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: For Linux'es (matching our) construct_default_ioirq_mptable() we
     may need to permit read access at least for PVH, if such default
     table construction is assumed to be sensible there in the first
     place (we assume ACPI and no PIC for PVH Dom0, after all).

RFC: Linux further has ACPI boot code accessing ELCR
     (acpi_pic_sci_set_trigger() and acpi_register_gsi_pic()), which we
     have no equivalent of.

Taken together, perhaps the hiding needs to be limited to PVH Dom0?

--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -503,6 +503,9 @@ int __init dom0_setup_permissions(struct
         rc |= ioports_deny_access(d, 0xA0 + offs, 0xA1 + offs);
     }
 
+    /* ELCR of both PICs. */
+    rc |= ioports_deny_access(d, 0x4D0, 0x4D1);
+
     /* Interval Timer (PIT). */
     for ( offs = 0, i = pit_alias_mask & -pit_alias_mask ?: 4;
           offs <= pit_alias_mask; offs += i )

Re: [PATCH 1/7] x86: don't allow Dom0 access to port CF9

[Roger Pau Monné]

On Thu, May 11, 2023 at 02:05:11PM +0200, Jan Beulich wrote:
> This allows to initiate machine reset, which we don't want to permit
> Dom0 to invoke that way.
> 
> While there insert blank lines and convert the sibling PCI config space
> port numbers to upper case, matching style earlier in the function.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Should we also do something about port 0x64?

Thanks, Roger.

Re: [PATCH 2/7] x86: don't allow Dom0 access to port 92

[Roger Pau Monné]

On Thu, May 11, 2023 at 02:05:45PM +0200, Jan Beulich wrote:
> Somewhat like port CF9 this may have a bit controlling the CPU's INIT#
> signal, and it also may have a bit involved in the driving of A20M#.
> Neither of these - just like CF9 - we want to allow Dom0 to drive.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

I'm kind of concerned that such ports might be used for other stuff
not described in the specifications I'm looking at, I guess we will
find out.

> 
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -500,6 +500,10 @@ int __init dom0_setup_permissions(struct
>      rc |= ioports_deny_access(d, 0x40, 0x43);
>      /* PIT Channel 2 / PC Speaker Control. */
>      rc |= ioports_deny_access(d, 0x61, 0x61);
> +
> +    /* INIT# and alternative A20M# control. */
> +    rc |= ioports_deny_access(d, 0x92, 0x92);

I do wonder whether it would make sense to create an array of [start,
end] IO ports to deny access to, so that we could loop over them and
code a single call to ioports_deny_access().  Maybe that's over
engineering it.

Thanks, Roger.

Re: [PATCH 3/7] x86/PVH: deny Dom0 access to the ISA DMA controller

[Roger Pau Monné]

On Thu, May 11, 2023 at 02:06:25PM +0200, Jan Beulich wrote:
> Unlike PV, a PVH Dom0 has no sensible way of driving the address and
> page registers correctly, as it would need to translate guest physical
> addresses to host ones. Rather than allowing data corruption to occur
> from e.g. the use of a legacy floppy drive, disallow access altogether.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 1/7] x86: don't allow Dom0 access to port CF9

[Jan Beulich]

On 25.10.2023 14:36, Roger Pau Monné wrote:
> On Thu, May 11, 2023 at 02:05:11PM +0200, Jan Beulich wrote:
>> This allows to initiate machine reset, which we don't want to permit
>> Dom0 to invoke that way.
>>
>> While there insert blank lines and convert the sibling PCI config space
>> port numbers to upper case, matching style earlier in the function.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Should we also do something about port 0x64?

That would be more involved, as its reset (and A20) functionality is
only a small subset of what it is used for. IOW we'd have to intercept
accesses and pass through the majority of the operations.

Jan

Re: [PATCH 2/7] x86: don't allow Dom0 access to port 92

[Jan Beulich]

On 25.10.2023 14:49, Roger Pau Monné wrote:
> On Thu, May 11, 2023 at 02:05:45PM +0200, Jan Beulich wrote:
>> Somewhat like port CF9 this may have a bit controlling the CPU's INIT#
>> signal, and it also may have a bit involved in the driving of A20M#.
>> Neither of these - just like CF9 - we want to allow Dom0 to drive.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I'm kind of concerned that such ports might be used for other stuff
> not described in the specifications I'm looking at, I guess we will
> find out.

There's a small risk, but there's also a certain risk from not making
the port inaccessible.

>> --- a/xen/arch/x86/dom0_build.c
>> +++ b/xen/arch/x86/dom0_build.c
>> @@ -500,6 +500,10 @@ int __init dom0_setup_permissions(struct
>>      rc |= ioports_deny_access(d, 0x40, 0x43);
>>      /* PIT Channel 2 / PC Speaker Control. */
>>      rc |= ioports_deny_access(d, 0x61, 0x61);
>> +
>> +    /* INIT# and alternative A20M# control. */
>> +    rc |= ioports_deny_access(d, 0x92, 0x92);
> 
> I do wonder whether it would make sense to create an array of [start,
> end] IO ports to deny access to, so that we could loop over them and
> code a single call to ioports_deny_access().  Maybe that's over
> engineering it.

It would compact part of the invocations, but some aren't using build-
time constants, so wouldn't fit well with such a table approach. (I
would probably ack a patch doing such a partial consolidation, but
at least right now I don't think I would put time in making such a
patch.)

Jan

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

[Roger Pau Monné]

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@suse.com>
> 
> --- 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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

On Thu, May 11, 2023 at 02:07:12PM +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 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.

As said in previous patches, I think this is likely too much risk for
little benefit.  I understand the desire to uniformly deny access to
any ports that allow interaction with devices in use by Xen (or not
allowed to be used by dom0), but there's certainly a risk in
configuring such devices in the way that we do by finding a register
that can be read and written to.

I think if anything this alias detection should have a command line
option in order to disable it.

Do you also have figures if the greatly increased amount of accesses
add a noticeable boot time regression?

We are usually cautious is not performing more accesses than strictly
required.

> 
> 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), I'm afraid our vPIT logic would not allow
> 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.)
> 
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -504,7 +504,11 @@ int __init dom0_setup_permissions(struct
>      }
>  
>      /* Interval Timer (PIT). */
> -    rc |= ioports_deny_access(d, 0x40, 0x43);
> +    for ( offs = 0, i = pit_alias_mask & -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
> @@ -53,6 +53,7 @@ extern unsigned long highmem_start;
>  #endif
>  
>  extern unsigned int pic_alias_mask;
> +extern unsigned int pit_alias_mask;
>  
>  extern int8_t opt_smt;
>  
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -425,6 +425,69 @@ 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;
> +
> +    /*
> +     * 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) );

We should have some kind of timeout here, just in case...

> +
> +        /*
> +         * 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 = mask & -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;
> +    }

Is it fine to leave counting disabled for channel 2?

Shouldn't we restore the previous gate value?

Thanks, Roger.

Re: [PATCH 6/7] x86: don't allow Dom0 (direct) access to port F0

[Roger Pau Monné]

On Thu, May 11, 2023 at 02:07:40PM +0200, Jan Beulich wrote:
> This controls the driving of IGNNE# (if such emulation is enabled in
> hardware), and hence would need proper handling in the hypervisor to be
> safe to use by Dom0 (and fully emulating for PVH/HVM DomU-s).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> RFC: Really this disabling of access would want to be conditional upon
>      the functionality actually being enabled. For AMD this looks to be
>      uniformly HWCR[8], but for Intel this is chipset-specific.

I'm afraid I'm not able to find much information about this, I've
found something in the Intel PCH datasheets, but I don't have a clear
picture of whether this port could be used by other functionality.

From my reading of the spec, the initial value in 0xF0 (COPROC_ERR)
will inhibit the generation of an IRQ13, and hence if the behavior
that most modern OSes rely on?

Mostly wanted to check which kind of logic and OS would use to figure
out whether 0xF0 exists and control IGNNE#

Thanks, Roger.

Re: [PATCH 7/7] x86: don't allow Dom0 access to ELCR ports

[Roger Pau Monné]

On Thu, May 11, 2023 at 02:08:09PM +0200, Jan Beulich wrote:
> Much like the other PIC ports, Dom0 has no business touching these. Even
> our own uses are somewhat questionable, as the corresponding IO-APIC
> code in Linux is enclosed in a CONFIG_EISA conditional; I don't think
> there are any x86-64 EISA systems.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> RFC: For Linux'es (matching our) construct_default_ioirq_mptable() we
>      may need to permit read access at least for PVH, if such default
>      table construction is assumed to be sensible there in the first
>      place (we assume ACPI and no PIC for PVH Dom0, after all).

Unless I'm confused, thise ports only control the triggering mode of
the PICs, and hence a PVH dom0 should have no business with them, as
not having a PIC in the first place.

> 
> RFC: Linux further has ACPI boot code accessing ELCR
>      (acpi_pic_sci_set_trigger() and acpi_register_gsi_pic()), which we
>      have no equivalent of.

If access to the port is denied, ~0 will be returned, and hence all
IRQs will be assumed to be level.  Some bits reserved and must be 0
will also be returned as 1.

> 
> Taken together, perhaps the hiding needs to be limited to PVH Dom0?

I very much doubt Xen will ever use the PIC unless forced on the
command line, and we should likely remove such option and just
mandate a working IO-APIC in order to run Xen.

My only doubt is whether some Linux code will get confused by poking
at ELCR{1,2} and malfunction as a result of not being able to fetch a
sane mask.

As a last resort, we could make the register RO?

Thanks, Roger.

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

[Jan Beulich]

On 26.10.2023 12:25, Roger Pau Monné wrote:
> On Thu, May 11, 2023 at 02:07:12PM +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 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.
> 
> As said in previous patches, I think this is likely too much risk for
> little benefit.  I understand the desire to uniformly deny access to
> any ports that allow interaction with devices in use by Xen (or not
> allowed to be used by dom0), but there's certainly a risk in
> configuring such devices in the way that we do by finding a register
> that can be read and written to.
> 
> I think if anything this alias detection should have a command line
> option in order to disable it.

Well, we could have command line options (for each of the RTC/CMOS,
PIC, and PIT probing allowing the alias masks to be specified (so we
don't need to probe). A value of 1 would uniformly mean "no probing,
no aliases" (as all three decode the low bit, so aliasing can happen
there). We could further make the default of these variables (yes/no,
no actual mask values of course) controllable by a Kconfig setting.

> Do you also have figures if the greatly increased amount of accesses
> add a noticeable boot time regression?

I didn't measure anything, but nothing's really noticeable, no. And
the number of accesses in all three of CMOS/RTC, PIC, and PIT is of
roughly the same order, I think (and the RTC/CMOS one has been in
for a while).

> We are usually cautious is not performing more accesses than strictly
> required.

Sadly "strictly required" is an uncertain quantity here. The more
checking we do, the less likely that we'd identify a false positive
case of aliasing.

>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -425,6 +425,69 @@ 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;
>> +
>> +    /*
>> +     * 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) );
> 
> We should have some kind of timeout here, just in case...

Hmm, I indeed did consider the need for a timeout here. With what
we've done up to here we already assume a functioning PIT, verifying
simply as we go. The issue with truly using some form of timeout is
the determination of how long to wait at most.

>> +
>> +        /*
>> +         * 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 = mask & -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;
>> +    }
> 
> Is it fine to leave counting disabled for channel 2?
> 
> Shouldn't we restore the previous gate value?

See init_pit(), which also doesn't restore anything. The system is under
our control, and we have no other use for channel 2. (I really had
restore logic here initially, but then dropped it for said reason.)

Jan

Re: [PATCH 7/7] x86: don't allow Dom0 access to ELCR ports

[Jan Beulich]

On 26.10.2023 13:02, Roger Pau Monné wrote:
> On Thu, May 11, 2023 at 02:08:09PM +0200, Jan Beulich wrote:
>> Much like the other PIC ports, Dom0 has no business touching these. Even
>> our own uses are somewhat questionable, as the corresponding IO-APIC
>> code in Linux is enclosed in a CONFIG_EISA conditional; I don't think
>> there are any x86-64 EISA systems.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> RFC: For Linux'es (matching our) construct_default_ioirq_mptable() we
>>      may need to permit read access at least for PVH, if such default
>>      table construction is assumed to be sensible there in the first
>>      place (we assume ACPI and no PIC for PVH Dom0, after all).
> 
> Unless I'm confused, thise ports only control the triggering mode of
> the PICs, and hence a PVH dom0 should have no business with them, as
> not having a PIC in the first place.

It should have no business, but the code I'm referring to still reads
these ports.

>> RFC: Linux further has ACPI boot code accessing ELCR
>>      (acpi_pic_sci_set_trigger() and acpi_register_gsi_pic()), which we
>>      have no equivalent of.
> 
> If access to the port is denied, ~0 will be returned, and hence all
> IRQs will be assumed to be level.  Some bits reserved and must be 0
> will also be returned as 1.

As with any reserved bits, the party reading them would better ignore
their values (like we do).

>> Taken together, perhaps the hiding needs to be limited to PVH Dom0?
> 
> I very much doubt Xen will ever use the PIC unless forced on the
> command line, and we should likely remove such option and just
> mandate a working IO-APIC in order to run Xen.
> 
> My only doubt is whether some Linux code will get confused by poking
> at ELCR{1,2} and malfunction as a result of not being able to fetch a
> sane mask.

Over many years this code hasn't changed much, if at all. So simply
from it being okay right now this would hopefully be only a
theoretical concern.

> As a last resort, we could make the register RO?

If and when needed, perhaps.

Jan

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

[Roger Pau Monné]

On Thu, Oct 26, 2023 at 11:03:42AM +0200, Jan Beulich wrote:
> 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).

I think it would be safe to add a command line option to disable the
probing, as we would at least like to avoid it in pvshim mode.  Maybe
ut would be interesting to make it a Kconfig option so that exclusive
pvshim Kconfig can avoid all this?

Otherwise it will just make booting the pvshim slower.

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

Right, it took me a bit to realize.

We assume that aliases are based on fused address pins, so for example
we don't explicitly test for an alias at port 0x34, but expect one if
there's an alias at port 0x30 and another one at port 0x24.

Thanks, Roger.

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

[Roger Pau Monné]

On Thu, Oct 26, 2023 at 02:31:27PM +0200, Jan Beulich wrote:
> On 26.10.2023 12:25, Roger Pau Monné wrote:
> > On Thu, May 11, 2023 at 02:07:12PM +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 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.
> > 
> > As said in previous patches, I think this is likely too much risk for
> > little benefit.  I understand the desire to uniformly deny access to
> > any ports that allow interaction with devices in use by Xen (or not
> > allowed to be used by dom0), but there's certainly a risk in
> > configuring such devices in the way that we do by finding a register
> > that can be read and written to.
> > 
> > I think if anything this alias detection should have a command line
> > option in order to disable it.
> 
> Well, we could have command line options (for each of the RTC/CMOS,
> PIC, and PIT probing allowing the alias masks to be specified (so we
> don't need to probe). A value of 1 would uniformly mean "no probing,
> no aliases" (as all three decode the low bit, so aliasing can happen
> there). We could further make the default of these variables (yes/no,
> no actual mask values of course) controllable by a Kconfig setting.

If you want to make this more fine grained, or even allow the user to
provide custom masks that's all fine, but there's already
dom0_ioports_disable that allows disabling a list of IO port ranges.

What I would require is a way to avoid all the probing, so that we
could return to the previous behavior.

> >> --- a/xen/arch/x86/time.c
> >> +++ b/xen/arch/x86/time.c
> >> @@ -425,6 +425,69 @@ 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;
> >> +
> >> +    /*
> >> +     * 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) );
> > 
> > We should have some kind of timeout here, just in case...
> 
> Hmm, I indeed did consider the need for a timeout here. With what
> we've done up to here we already assume a functioning PIT, verifying
> simply as we go. The issue with truly using some form of timeout is
> the determination of how long to wait at most.

I would likely make it based on iterations, could you get some figures
on how many iterations it takes for the bit to be clear?

I would think something like 1000 should be enough, but really have no
idea.

> >> +
> >> +        /*
> >> +         * 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 = mask & -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;
> >> +    }
> > 
> > Is it fine to leave counting disabled for channel 2?
> > 
> > Shouldn't we restore the previous gate value?
> 
> See init_pit(), which also doesn't restore anything. The system is under
> our control, and we have no other use for channel 2. (I really had
> restore logic here initially, but then dropped it for said reason.)

It might be used by a PV dom0 (see hwdom_pit_access()), so I wonder
whether we should try to hand off as Xen found it.

Thanks, Roger.

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

[Jan Beulich]

On 26.10.2023 15:24, Roger Pau Monné wrote:
> On Thu, Oct 26, 2023 at 11:03:42AM +0200, Jan Beulich wrote:
>> 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).
> 
> I think it would be safe to add a command line option to disable the
> probing, as we would at least like to avoid it in pvshim mode.  Maybe
> ut would be interesting to make it a Kconfig option so that exclusive
> pvshim Kconfig can avoid all this?
> 
> Otherwise it will just make booting the pvshim slower.

I've taken note to introduce such an option (not sure yet whether just
cmdline or also Kconfig). Still
- Shouldn't we already be bypassing related init logic in shim mode?
- A Kconfig option interfacing with PV_SHIM_EXCLUSIVE will collide with
  my patch inverting that option's sense (and renaming it), so it would
  be nice to have that sorted/accepted first (see
  https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html).

>>>> @@ -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.
> 
> Right, it took me a bit to realize.
> 
> We assume that aliases are based on fused address pins, so for example
> we don't explicitly test for an alias at port 0x34, but expect one if
> there's an alias at port 0x30 and another one at port 0x24.

Well, I wouldn't have called it "fused pins", but "not decoded address
bits". But yes. The same was already assumed in the CMOS/RTC patch.

Jan

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

[Jan Beulich]

On 26.10.2023 15:57, Roger Pau Monné wrote:
> On Thu, Oct 26, 2023 at 02:31:27PM +0200, Jan Beulich wrote:
>> On 26.10.2023 12:25, Roger Pau Monné wrote:
>>> On Thu, May 11, 2023 at 02:07:12PM +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 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.
>>>
>>> As said in previous patches, I think this is likely too much risk for
>>> little benefit.  I understand the desire to uniformly deny access to
>>> any ports that allow interaction with devices in use by Xen (or not
>>> allowed to be used by dom0), but there's certainly a risk in
>>> configuring such devices in the way that we do by finding a register
>>> that can be read and written to.
>>>
>>> I think if anything this alias detection should have a command line
>>> option in order to disable it.
>>
>> Well, we could have command line options (for each of the RTC/CMOS,
>> PIC, and PIT probing allowing the alias masks to be specified (so we
>> don't need to probe). A value of 1 would uniformly mean "no probing,
>> no aliases" (as all three decode the low bit, so aliasing can happen
>> there). We could further make the default of these variables (yes/no,
>> no actual mask values of course) controllable by a Kconfig setting.
> 
> If you want to make this more fine grained, or even allow the user to
> provide custom masks that's all fine, but there's already
> dom0_ioports_disable that allows disabling a list of IO port ranges.
> 
> What I would require is a way to avoid all the probing, so that we
> could return to the previous behavior.
> 
>>>> --- a/xen/arch/x86/time.c
>>>> +++ b/xen/arch/x86/time.c
>>>> @@ -425,6 +425,69 @@ 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;
>>>> +
>>>> +    /*
>>>> +     * 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) );
>>>
>>> We should have some kind of timeout here, just in case...
>>
>> Hmm, I indeed did consider the need for a timeout here. With what
>> we've done up to here we already assume a functioning PIT, verifying
>> simply as we go. The issue with truly using some form of timeout is
>> the determination of how long to wait at most.
> 
> I would likely make it based on iterations, could you get some figures
> on how many iterations it takes for the bit to be clear?
> 
> I would think something like 1000 should be enough, but really have no
> idea.

Except that how long a given number of iterations takes is unknown. 1000
may be enough today or on the systems we test, but may not be tomorrow
or on other peoples' systems. Hence why I'm hesitant ...

Jan

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

[Roger Pau Monné]

On Thu, Oct 26, 2023 at 05:10:41PM +0200, Jan Beulich wrote:
> On 26.10.2023 15:57, Roger Pau Monné wrote:
> > On Thu, Oct 26, 2023 at 02:31:27PM +0200, Jan Beulich wrote:
> >> On 26.10.2023 12:25, Roger Pau Monné wrote:
> >>> On Thu, May 11, 2023 at 02:07:12PM +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 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.
> >>>
> >>> As said in previous patches, I think this is likely too much risk for
> >>> little benefit.  I understand the desire to uniformly deny access to
> >>> any ports that allow interaction with devices in use by Xen (or not
> >>> allowed to be used by dom0), but there's certainly a risk in
> >>> configuring such devices in the way that we do by finding a register
> >>> that can be read and written to.
> >>>
> >>> I think if anything this alias detection should have a command line
> >>> option in order to disable it.
> >>
> >> Well, we could have command line options (for each of the RTC/CMOS,
> >> PIC, and PIT probing allowing the alias masks to be specified (so we
> >> don't need to probe). A value of 1 would uniformly mean "no probing,
> >> no aliases" (as all three decode the low bit, so aliasing can happen
> >> there). We could further make the default of these variables (yes/no,
> >> no actual mask values of course) controllable by a Kconfig setting.
> > 
> > If you want to make this more fine grained, or even allow the user to
> > provide custom masks that's all fine, but there's already
> > dom0_ioports_disable that allows disabling a list of IO port ranges.
> > 
> > What I would require is a way to avoid all the probing, so that we
> > could return to the previous behavior.
> > 
> >>>> --- a/xen/arch/x86/time.c
> >>>> +++ b/xen/arch/x86/time.c
> >>>> @@ -425,6 +425,69 @@ 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;
> >>>> +
> >>>> +    /*
> >>>> +     * 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) );
> >>>
> >>> We should have some kind of timeout here, just in case...
> >>
> >> Hmm, I indeed did consider the need for a timeout here. With what
> >> we've done up to here we already assume a functioning PIT, verifying
> >> simply as we go. The issue with truly using some form of timeout is
> >> the determination of how long to wait at most.
> > 
> > I would likely make it based on iterations, could you get some figures
> > on how many iterations it takes for the bit to be clear?
> > 
> > I would think something like 1000 should be enough, but really have no
> > idea.
> 
> Except that how long a given number of iterations takes is unknown. 1000
> may be enough today or on the systems we test, but may not be tomorrow
> or on other peoples' systems. Hence why I'm hesitant ...

Hm, but getting stuck in a loop here can't be good either.  Let's do
it time wise if you prefer, 1s should be more than enough I would
think.

In any case the worse that could if the timeout is hit is that aliases
are not properly detected, but that's way better than the possibility
of getting stuck in an infinite loop.

Thanks, Roger.

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

[Roger Pau Monné]

On Thu, Oct 26, 2023 at 05:07:18PM +0200, Jan Beulich wrote:
> On 26.10.2023 15:24, Roger Pau Monné wrote:
> > On Thu, Oct 26, 2023 at 11:03:42AM +0200, Jan Beulich wrote:
> >> 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).
> > 
> > I think it would be safe to add a command line option to disable the
> > probing, as we would at least like to avoid it in pvshim mode.  Maybe
> > ut would be interesting to make it a Kconfig option so that exclusive
> > pvshim Kconfig can avoid all this?
> > 
> > Otherwise it will just make booting the pvshim slower.
> 
> I've taken note to introduce such an option (not sure yet whether just
> cmdline or also Kconfig). Still
> - Shouldn't we already be bypassing related init logic in shim mode?

Not sure what we bypass in pvshim mode, would be good to double
check.

> - A Kconfig option interfacing with PV_SHIM_EXCLUSIVE will collide with
>   my patch inverting that option's sense (and renaming it), so it would
>   be nice to have that sorted/accepted first (see
>   https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html).

It being Andrew the one that made the request, I would like to get his
opinion on it.  UNCONSTRAINED does seem a bit weird.

Maybe the issue is that PV_SHIM_EXCLUSIVE shouldn't have been a
Kconfig option in the first place, and instead a specific Kconfig
config file?

Maybe it's not possible to achieve the same using just a Kconfig
config file.

Thanks, Roger.

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

[Jan Beulich]

On 26.10.2023 17:19, Roger Pau Monné wrote:
> On Thu, Oct 26, 2023 at 05:07:18PM +0200, Jan Beulich wrote:
>> On 26.10.2023 15:24, Roger Pau Monné wrote:
>>> On Thu, Oct 26, 2023 at 11:03:42AM +0200, Jan Beulich wrote:
>>>> 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).
>>>
>>> I think it would be safe to add a command line option to disable the
>>> probing, as we would at least like to avoid it in pvshim mode.  Maybe
>>> ut would be interesting to make it a Kconfig option so that exclusive
>>> pvshim Kconfig can avoid all this?
>>>
>>> Otherwise it will just make booting the pvshim slower.
>>
>> I've taken note to introduce such an option (not sure yet whether just
>> cmdline or also Kconfig). Still
>> - Shouldn't we already be bypassing related init logic in shim mode?
> 
> Not sure what we bypass in pvshim mode, would be good to double
> check.
> 
>> - A Kconfig option interfacing with PV_SHIM_EXCLUSIVE will collide with
>>   my patch inverting that option's sense (and renaming it), so it would
>>   be nice to have that sorted/accepted first (see
>>   https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html).
> 
> It being Andrew the one that made the request, I would like to get his
> opinion on it.  UNCONSTRAINED does seem a bit weird.

I agree that the name is odd; I couldn't think of any better one (and
this already is the result of 3 or 4 rounds of renaming). I'll be more
than happy to consider other naming suggestions. The main issue with the
present option is, just to re-state it here, that we have grown negative
dependencies on it, which is a problem.

However, meanwhile I've realized that we don't really want to tie anything
here to PV_SHIM_EXCLUSIVE, at least not directly. What we care about is
whether we _actually_ run in shim mode, i.e. also when a full-fledged
hypervisor is in use. My plan is now to have said new command line option,
which - if not specified on the command line - I'd default to !pv_shim.

> Maybe the issue is that PV_SHIM_EXCLUSIVE shouldn't have been a
> Kconfig option in the first place, and instead a specific Kconfig
> config file?
> 
> Maybe it's not possible to achieve the same using just a Kconfig
> config file.

I'm afraid I don't understand what you mean by "Kconfig config file". It
can't really be just another .../Kconfig file somewhere in the tree, as
it doesn't really matter where an option like this would be defined.

Jan

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

[Jan Beulich]

On 26.10.2023 17:13, Roger Pau Monné wrote:
> On Thu, Oct 26, 2023 at 05:10:41PM +0200, Jan Beulich wrote:
>> On 26.10.2023 15:57, Roger Pau Monné wrote:
>>> On Thu, Oct 26, 2023 at 02:31:27PM +0200, Jan Beulich wrote:
>>>> On 26.10.2023 12:25, Roger Pau Monné wrote:
>>>>> On Thu, May 11, 2023 at 02:07:12PM +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 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.
>>>>>
>>>>> As said in previous patches, I think this is likely too much risk for
>>>>> little benefit.  I understand the desire to uniformly deny access to
>>>>> any ports that allow interaction with devices in use by Xen (or not
>>>>> allowed to be used by dom0), but there's certainly a risk in
>>>>> configuring such devices in the way that we do by finding a register
>>>>> that can be read and written to.
>>>>>
>>>>> I think if anything this alias detection should have a command line
>>>>> option in order to disable it.
>>>>
>>>> Well, we could have command line options (for each of the RTC/CMOS,
>>>> PIC, and PIT probing allowing the alias masks to be specified (so we
>>>> don't need to probe). A value of 1 would uniformly mean "no probing,
>>>> no aliases" (as all three decode the low bit, so aliasing can happen
>>>> there). We could further make the default of these variables (yes/no,
>>>> no actual mask values of course) controllable by a Kconfig setting.
>>>
>>> If you want to make this more fine grained, or even allow the user to
>>> provide custom masks that's all fine, but there's already
>>> dom0_ioports_disable that allows disabling a list of IO port ranges.
>>>
>>> What I would require is a way to avoid all the probing, so that we
>>> could return to the previous behavior.
>>>
>>>>>> --- a/xen/arch/x86/time.c
>>>>>> +++ b/xen/arch/x86/time.c
>>>>>> @@ -425,6 +425,69 @@ 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;
>>>>>> +
>>>>>> +    /*
>>>>>> +     * 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) );
>>>>>
>>>>> We should have some kind of timeout here, just in case...
>>>>
>>>> Hmm, I indeed did consider the need for a timeout here. With what
>>>> we've done up to here we already assume a functioning PIT, verifying
>>>> simply as we go. The issue with truly using some form of timeout is
>>>> the determination of how long to wait at most.
>>>
>>> I would likely make it based on iterations, could you get some figures
>>> on how many iterations it takes for the bit to be clear?
>>>
>>> I would think something like 1000 should be enough, but really have no
>>> idea.
>>
>> Except that how long a given number of iterations takes is unknown. 1000
>> may be enough today or on the systems we test, but may not be tomorrow
>> or on other peoples' systems. Hence why I'm hesitant ...
> 
> Hm, but getting stuck in a loop here can't be good either.

I certainly understand that. The command line option I've just added in
a prereq patch would allow bypassing the probing, but of course I agree
that we want to avoid hanging here nevertheless (if we can).

>  Let's do
> it time wise if you prefer, 1s should be more than enough I would
> think.

Yet time-wise is also problematic ahead of us having calibrated clocks.
And using the PIT itself (which runs at a known frequency) doesn't look
to be a good idea when we mean to deal with the case of a broken PIT,
or none at all.

Jan

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

[Roger Pau Monné]

On Mon, Oct 30, 2023 at 01:24:52PM +0100, Jan Beulich wrote:
> On 26.10.2023 17:19, Roger Pau Monné wrote:
> > On Thu, Oct 26, 2023 at 05:07:18PM +0200, Jan Beulich wrote:
> >> On 26.10.2023 15:24, Roger Pau Monné wrote:
> >>> On Thu, Oct 26, 2023 at 11:03:42AM +0200, Jan Beulich wrote:
> >>>> 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).
> >>>
> >>> I think it would be safe to add a command line option to disable the
> >>> probing, as we would at least like to avoid it in pvshim mode.  Maybe
> >>> ut would be interesting to make it a Kconfig option so that exclusive
> >>> pvshim Kconfig can avoid all this?
> >>>
> >>> Otherwise it will just make booting the pvshim slower.
> >>
> >> I've taken note to introduce such an option (not sure yet whether just
> >> cmdline or also Kconfig). Still
> >> - Shouldn't we already be bypassing related init logic in shim mode?
> > 
> > Not sure what we bypass in pvshim mode, would be good to double
> > check.
> > 
> >> - A Kconfig option interfacing with PV_SHIM_EXCLUSIVE will collide with
> >>   my patch inverting that option's sense (and renaming it), so it would
> >>   be nice to have that sorted/accepted first (see
> >>   https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html).
> > 
> > It being Andrew the one that made the request, I would like to get his
> > opinion on it.  UNCONSTRAINED does seem a bit weird.
> 
> I agree that the name is odd; I couldn't think of any better one (and
> this already is the result of 3 or 4 rounds of renaming). I'll be more
> than happy to consider other naming suggestions. The main issue with the
> present option is, just to re-state it here, that we have grown negative
> dependencies on it, which is a problem.
> 
> However, meanwhile I've realized that we don't really want to tie anything
> here to PV_SHIM_EXCLUSIVE, at least not directly. What we care about is
> whether we _actually_ run in shim mode, i.e. also when a full-fledged
> hypervisor is in use. My plan is now to have said new command line option,
> which - if not specified on the command line - I'd default to !pv_shim.

So that tests for aliases are run unless in pv shim mode.

> > Maybe the issue is that PV_SHIM_EXCLUSIVE shouldn't have been a
> > Kconfig option in the first place, and instead a specific Kconfig
> > config file?
> > 
> > Maybe it's not possible to achieve the same using just a Kconfig
> > config file.
> 
> I'm afraid I don't understand what you mean by "Kconfig config file". It
> can't really be just another .../Kconfig file somewhere in the tree, as
> it doesn't really matter where an option like this would be defined.

No, I was thinking of splitting what PV_SHIM_EXCLUSIVE actually
implies, for example by adding CONFIG_DOMCTL_HYPERCALLL or
CONFIG_PLATFORM_HYPERCALL and re-work the pvshim_defconfig config file
based on those, so that we don't end up with negative relations.

Note sure all usages of PV_SHIM_EXCLUSIVE can be split in such a way,
maybe we would need some compromise.

Thanks, Roger.

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

[Jan Beulich]

On 30.10.2023 16:14, Roger Pau Monné wrote:
> On Mon, Oct 30, 2023 at 01:24:52PM +0100, Jan Beulich wrote:
>> On 26.10.2023 17:19, Roger Pau Monné wrote:
>>> Maybe the issue is that PV_SHIM_EXCLUSIVE shouldn't have been a
>>> Kconfig option in the first place, and instead a specific Kconfig
>>> config file?
>>>
>>> Maybe it's not possible to achieve the same using just a Kconfig
>>> config file.
>>
>> I'm afraid I don't understand what you mean by "Kconfig config file". It
>> can't really be just another .../Kconfig file somewhere in the tree, as
>> it doesn't really matter where an option like this would be defined.
> 
> No, I was thinking of splitting what PV_SHIM_EXCLUSIVE actually
> implies, for example by adding CONFIG_DOMCTL_HYPERCALLL or
> CONFIG_PLATFORM_HYPERCALL and re-work the pvshim_defconfig config file
> based on those, so that we don't end up with negative relations.
> 
> Note sure all usages of PV_SHIM_EXCLUSIVE can be split in such a way,
> maybe we would need some compromise.

Wouldn't such a CONFIG_DOMCTL_HYPERCALL then still want to depend on
!PV_SHIM_EXCLUSIVE, which is the kind of dependency we want to avoid?
Aiui the two (splitting and inverting) are largely orthogonal aspects.

Jan

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

[Roger Pau Monné]

On Mon, Oct 30, 2023 at 04:19:22PM +0100, Jan Beulich wrote:
> On 30.10.2023 16:14, Roger Pau Monné wrote:
> > On Mon, Oct 30, 2023 at 01:24:52PM +0100, Jan Beulich wrote:
> >> On 26.10.2023 17:19, Roger Pau Monné wrote:
> >>> Maybe the issue is that PV_SHIM_EXCLUSIVE shouldn't have been a
> >>> Kconfig option in the first place, and instead a specific Kconfig
> >>> config file?
> >>>
> >>> Maybe it's not possible to achieve the same using just a Kconfig
> >>> config file.
> >>
> >> I'm afraid I don't understand what you mean by "Kconfig config file". It
> >> can't really be just another .../Kconfig file somewhere in the tree, as
> >> it doesn't really matter where an option like this would be defined.
> > 
> > No, I was thinking of splitting what PV_SHIM_EXCLUSIVE actually
> > implies, for example by adding CONFIG_DOMCTL_HYPERCALLL or
> > CONFIG_PLATFORM_HYPERCALL and re-work the pvshim_defconfig config file
> > based on those, so that we don't end up with negative relations.
> > 
> > Note sure all usages of PV_SHIM_EXCLUSIVE can be split in such a way,
> > maybe we would need some compromise.
> 
> Wouldn't such a CONFIG_DOMCTL_HYPERCALL then still want to depend on
> !PV_SHIM_EXCLUSIVE, which is the kind of dependency we want to avoid?
> Aiui the two (splitting and inverting) are largely orthogonal aspects.

No, CONFIG_DOMCTL_HYPERCALL could be promptless option enabled by
default and disabled by the pvshim_defconfig.

Thanks, Roger.

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

[Jan Beulich]

On 30.10.2023 16:23, Roger Pau Monné wrote:
> On Mon, Oct 30, 2023 at 04:19:22PM +0100, Jan Beulich wrote:
>> On 30.10.2023 16:14, Roger Pau Monné wrote:
>>> On Mon, Oct 30, 2023 at 01:24:52PM +0100, Jan Beulich wrote:
>>>> On 26.10.2023 17:19, Roger Pau Monné wrote:
>>>>> Maybe the issue is that PV_SHIM_EXCLUSIVE shouldn't have been a
>>>>> Kconfig option in the first place, and instead a specific Kconfig
>>>>> config file?
>>>>>
>>>>> Maybe it's not possible to achieve the same using just a Kconfig
>>>>> config file.
>>>>
>>>> I'm afraid I don't understand what you mean by "Kconfig config file". It
>>>> can't really be just another .../Kconfig file somewhere in the tree, as
>>>> it doesn't really matter where an option like this would be defined.
>>>
>>> No, I was thinking of splitting what PV_SHIM_EXCLUSIVE actually
>>> implies, for example by adding CONFIG_DOMCTL_HYPERCALLL or
>>> CONFIG_PLATFORM_HYPERCALL and re-work the pvshim_defconfig config file
>>> based on those, so that we don't end up with negative relations.
>>>
>>> Note sure all usages of PV_SHIM_EXCLUSIVE can be split in such a way,
>>> maybe we would need some compromise.
>>
>> Wouldn't such a CONFIG_DOMCTL_HYPERCALL then still want to depend on
>> !PV_SHIM_EXCLUSIVE, which is the kind of dependency we want to avoid?
>> Aiui the two (splitting and inverting) are largely orthogonal aspects.
> 
> No, CONFIG_DOMCTL_HYPERCALL could be promptless option enabled by
> default and disabled by the pvshim_defconfig.

pvshim_defconfig shouldn't play a role here. Anyone configuring a shim
build from scratch should also get a consistent set of settings. When
there's no prompt and default-enabling, I'm also having some difficulty
seeing how pvshim_defconfig could override that then.

Jan

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

[Roger Pau Monné]

On Mon, Oct 30, 2023 at 04:35:41PM +0100, Jan Beulich wrote:
> On 30.10.2023 16:23, Roger Pau Monné wrote:
> > On Mon, Oct 30, 2023 at 04:19:22PM +0100, Jan Beulich wrote:
> >> On 30.10.2023 16:14, Roger Pau Monné wrote:
> >>> On Mon, Oct 30, 2023 at 01:24:52PM +0100, Jan Beulich wrote:
> >>>> On 26.10.2023 17:19, Roger Pau Monné wrote:
> >>>>> Maybe the issue is that PV_SHIM_EXCLUSIVE shouldn't have been a
> >>>>> Kconfig option in the first place, and instead a specific Kconfig
> >>>>> config file?
> >>>>>
> >>>>> Maybe it's not possible to achieve the same using just a Kconfig
> >>>>> config file.
> >>>>
> >>>> I'm afraid I don't understand what you mean by "Kconfig config file". It
> >>>> can't really be just another .../Kconfig file somewhere in the tree, as
> >>>> it doesn't really matter where an option like this would be defined.
> >>>
> >>> No, I was thinking of splitting what PV_SHIM_EXCLUSIVE actually
> >>> implies, for example by adding CONFIG_DOMCTL_HYPERCALLL or
> >>> CONFIG_PLATFORM_HYPERCALL and re-work the pvshim_defconfig config file
> >>> based on those, so that we don't end up with negative relations.
> >>>
> >>> Note sure all usages of PV_SHIM_EXCLUSIVE can be split in such a way,
> >>> maybe we would need some compromise.
> >>
> >> Wouldn't such a CONFIG_DOMCTL_HYPERCALL then still want to depend on
> >> !PV_SHIM_EXCLUSIVE, which is the kind of dependency we want to avoid?
> >> Aiui the two (splitting and inverting) are largely orthogonal aspects.
> > 
> > No, CONFIG_DOMCTL_HYPERCALL could be promptless option enabled by
> > default and disabled by the pvshim_defconfig.
> 
> pvshim_defconfig shouldn't play a role here. Anyone configuring a shim
> build from scratch should also get a consistent set of settings.

Then we might have to expose those more fine grained options.

My sauggestion was to still allow enabling shim mode from by the user,
but that building a pv-shim exclusive image would require using the
pvshim_defconfig, to simply avoid exposing a bunch of fine grained
options that only make sense for pv-shim exclusive builds.

If that's not sensible, we might consider exposing those more fine
grained options.

Thanks, Roger.