[4/5] x86/PV: remove the emulated PIT
diff mbox

Message ID 1452688338-70075-5-git-send-email-roger.pau@citrix.com
State New, archived
Headers show

Commit Message

Roger Pau Monné Jan. 13, 2016, 12:32 p.m. UTC
The HVMlite series removed the initialization of the emulated PIT for PV
guests, but the handler was still reachable, which means a PV guests can
crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
PV PIT handler and move the PIT initialization to HVM guests only.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/domain.c         |  3 ---
 xen/arch/x86/hvm/hvm.c        |  2 ++
 xen/arch/x86/hvm/i8254.c      | 27 ---------------------------
 xen/arch/x86/traps.c          | 12 ++----------
 xen/include/asm-x86/hvm/vpt.h |  1 -
 5 files changed, 4 insertions(+), 41 deletions(-)

Comments

Jan Beulich Jan. 13, 2016, 4:36 p.m. UTC | #1
>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
> The HVMlite series removed the initialization of the emulated PIT for PV
> guests, but the handler was still reachable, which means a PV guests can
> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
> PV PIT handler and move the PIT initialization to HVM guests only.

As said on IRC - this is needed for Dom0 to be able to drive the
PC speaker. You'll need to provide a fix for the suppressed
initialization instead, at least for Dom0. (As an aside, your patch
orphans hwdom_pit_access().)

Jan
Roger Pau Monné Jan. 14, 2016, 8:25 a.m. UTC | #2
El 13/01/16 a les 17.36, Jan Beulich ha escrit:
>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>> The HVMlite series removed the initialization of the emulated PIT for PV
>> guests, but the handler was still reachable, which means a PV guests can
>> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
>> PV PIT handler and move the PIT initialization to HVM guests only.
> 
> As said on IRC - this is needed for Dom0 to be able to drive the
> PC speaker. You'll need to provide a fix for the suppressed
> initialization instead, at least for Dom0. (As an aside, your patch
> orphans hwdom_pit_access().)

Thanks for the clarification. AFAICT I can leave the usage of
hwdom_pit_access for Dom0, and completely remove PIT access for DomU, is
that right?

Roger.
Jan Beulich Jan. 14, 2016, 9:11 a.m. UTC | #3
>>> On 14.01.16 at 09:25, <roger.pau@citrix.com> wrote:
> El 13/01/16 a les 17.36, Jan Beulich ha escrit:
>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>> The HVMlite series removed the initialization of the emulated PIT for PV
>>> guests, but the handler was still reachable, which means a PV guests can
>>> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
>>> PV PIT handler and move the PIT initialization to HVM guests only.
>> 
>> As said on IRC - this is needed for Dom0 to be able to drive the
>> PC speaker. You'll need to provide a fix for the suppressed
>> initialization instead, at least for Dom0. (As an aside, your patch
>> orphans hwdom_pit_access().)
> 
> Thanks for the clarification. AFAICT I can leave the usage of
> hwdom_pit_access for Dom0, and completely remove PIT access for DomU, is
> that right?

I don't think so - see the explanation Tim gave on IRC. Afaict the
mention of BIOS here isn't related to a virtual BIOS, but to that
of a passed through graphics card.

Jan
Roger Pau Monné Jan. 14, 2016, 10:59 a.m. UTC | #4
El 14/01/16 a les 10.11, Jan Beulich ha escrit:
>>>> On 14.01.16 at 09:25, <roger.pau@citrix.com> wrote:
>> El 13/01/16 a les 17.36, Jan Beulich ha escrit:
>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>> The HVMlite series removed the initialization of the emulated PIT for PV
>>>> guests, but the handler was still reachable, which means a PV guests can
>>>> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
>>>> PV PIT handler and move the PIT initialization to HVM guests only.
>>>
>>> As said on IRC - this is needed for Dom0 to be able to drive the
>>> PC speaker. You'll need to provide a fix for the suppressed
>>> initialization instead, at least for Dom0. (As an aside, your patch
>>> orphans hwdom_pit_access().)
>>
>> Thanks for the clarification. AFAICT I can leave the usage of
>> hwdom_pit_access for Dom0, and completely remove PIT access for DomU, is
>> that right?
> 
> I don't think so - see the explanation Tim gave on IRC. Afaict the
> mention of BIOS here isn't related to a virtual BIOS, but to that
> of a passed through graphics card.

I'm sorry but I still don't fully understand why that's needed, and it
arises a couple of questions. First of all, the only reference that I
can find about BIOS and i8254 usage is regarding VGA BIOS POST [0],
where they mention that the VGA POST method might make use of the i8254.

This seems reasonable, but I still don't understand why we provide an
emulated i8254 to DomUs. They don't have access to the low 1MB, which is
where the VGA BIOS resides, so there's no way they can call into the VGA
POST at all.

Also, allowing a VGA BIOS access to the i8254 used by the OS seems like
asking for trouble, so that same paper [0] describes that they provide
an emulated i8254 for the VGA BIOS to use (because they run the VGA BIOS
code inside x86emu), in order to prevent it from messing with the OS
setup. I have to admit that greatly depends on whether the OS makes use
of the i8254 or not.

Anyway, I would be in favour of adding an emulated i8254 to the hardware
domain (and let it make use of the PC speaker), but I don't see any
reason to provide it to DomUs.

Roger.

[0] https://2008.asiabsdcon.org/papers/P9A-paper.pdf
Jan Beulich Jan. 14, 2016, 12:38 p.m. UTC | #5
>>> On 14.01.16 at 11:59, <roger.pau@citrix.com> wrote:
> El 14/01/16 a les 10.11, Jan Beulich ha escrit:
>>>>> On 14.01.16 at 09:25, <roger.pau@citrix.com> wrote:
>>> El 13/01/16 a les 17.36, Jan Beulich ha escrit:
>>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>>> The HVMlite series removed the initialization of the emulated PIT for PV
>>>>> guests, but the handler was still reachable, which means a PV guests can
>>>>> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
>>>>> PV PIT handler and move the PIT initialization to HVM guests only.
>>>>
>>>> As said on IRC - this is needed for Dom0 to be able to drive the
>>>> PC speaker. You'll need to provide a fix for the suppressed
>>>> initialization instead, at least for Dom0. (As an aside, your patch
>>>> orphans hwdom_pit_access().)
>>>
>>> Thanks for the clarification. AFAICT I can leave the usage of
>>> hwdom_pit_access for Dom0, and completely remove PIT access for DomU, is
>>> that right?
>> 
>> I don't think so - see the explanation Tim gave on IRC. Afaict the
>> mention of BIOS here isn't related to a virtual BIOS, but to that
>> of a passed through graphics card.
> 
> I'm sorry but I still don't fully understand why that's needed, and it
> arises a couple of questions. First of all, the only reference that I
> can find about BIOS and i8254 usage is regarding VGA BIOS POST [0],
> where they mention that the VGA POST method might make use of the i8254.
> 
> This seems reasonable, but I still don't understand why we provide an
> emulated i8254 to DomUs. They don't have access to the low 1MB, which is
> where the VGA BIOS resides, so there's no way they can call into the VGA
> POST at all.

All of this arrangement predates me, but see the original change
introducing this: "Provide PV guests with emulated PIT", which
suggests this wasn't just for Dom0. I'm hesitant to accept removal
of code when we don't know exactly by whom and for what purpose
it might be used. When I enabled Dom0 speaker control, I
intentionally retained the original code for DomU purposes.

Jan
Roger Pau Monné Jan. 14, 2016, 2:03 p.m. UTC | #6
El 14/01/16 a les 13.38, Jan Beulich ha escrit:
>>>> On 14.01.16 at 11:59, <roger.pau@citrix.com> wrote:
>> El 14/01/16 a les 10.11, Jan Beulich ha escrit:
>>>>>> On 14.01.16 at 09:25, <roger.pau@citrix.com> wrote:
>>>> El 13/01/16 a les 17.36, Jan Beulich ha escrit:
>>>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>>>> The HVMlite series removed the initialization of the emulated PIT for PV
>>>>>> guests, but the handler was still reachable, which means a PV guests can
>>>>>> crash Xen if it pokes at IO ports 0x42, 0x43 or 0x61. Completely remove the
>>>>>> PV PIT handler and move the PIT initialization to HVM guests only.
>>>>>
>>>>> As said on IRC - this is needed for Dom0 to be able to drive the
>>>>> PC speaker. You'll need to provide a fix for the suppressed
>>>>> initialization instead, at least for Dom0. (As an aside, your patch
>>>>> orphans hwdom_pit_access().)
>>>>
>>>> Thanks for the clarification. AFAICT I can leave the usage of
>>>> hwdom_pit_access for Dom0, and completely remove PIT access for DomU, is
>>>> that right?
>>>
>>> I don't think so - see the explanation Tim gave on IRC. Afaict the
>>> mention of BIOS here isn't related to a virtual BIOS, but to that
>>> of a passed through graphics card.
>>
>> I'm sorry but I still don't fully understand why that's needed, and it
>> arises a couple of questions. First of all, the only reference that I
>> can find about BIOS and i8254 usage is regarding VGA BIOS POST [0],
>> where they mention that the VGA POST method might make use of the i8254.
>>
>> This seems reasonable, but I still don't understand why we provide an
>> emulated i8254 to DomUs. They don't have access to the low 1MB, which is
>> where the VGA BIOS resides, so there's no way they can call into the VGA
>> POST at all.
> 
> All of this arrangement predates me, but see the original change
> introducing this: "Provide PV guests with emulated PIT", which
> suggests this wasn't just for Dom0. I'm hesitant to accept removal
> of code when we don't know exactly by whom and for what purpose
> it might be used. When I enabled Dom0 speaker control, I
> intentionally retained the original code for DomU purposes.

What about we do the following: enable the PIT for PV(H) guests
(DomU/Dom0), and completely remove it for HVMlite guests for the moment?

We might consider enabling it for HVMlite, but the plan is that this
could be done on a per-domain basis using the flags in the
xen_arch_domainconfig struct.

Roger.
Jan Beulich Jan. 14, 2016, 2:36 p.m. UTC | #7
>>> On 14.01.16 at 15:03, <roger.pau@citrix.com> wrote:
> What about we do the following: enable the PIT for PV(H) guests
> (DomU/Dom0), and completely remove it for HVMlite guests for the moment?
> 
> We might consider enabling it for HVMlite, but the plan is that this
> could be done on a per-domain basis using the flags in the
> xen_arch_domainconfig struct.

That sounds okay.

Jan

Patch
diff mbox

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 159d960..868ef49 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -647,9 +647,6 @@  int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     tsc_set_info(d, TSC_MODE_DEFAULT, 0UL, 0, 0);
     spin_lock_init(&d->arch.vtsc_lock);
 
-    /* PV/PVH guests get an emulated PIT too for video BIOSes to use. */
-    pit_init(d, cpu_khz);
-
     return 0;
 
  fail:
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 05c3ca1..28c6cd9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1622,6 +1622,8 @@  int hvm_domain_initialise(struct domain *d)
 
     msixtbl_init(d);
 
+    pit_init(d, cpu_khz);
+
     register_portio_handler(d, 0xe9, 1, hvm_print_line);
     register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
 
diff --git a/xen/arch/x86/hvm/i8254.c b/xen/arch/x86/hvm/i8254.c
index b517cd6..83eae33 100644
--- a/xen/arch/x86/hvm/i8254.c
+++ b/xen/arch/x86/hvm/i8254.c
@@ -558,33 +558,6 @@  static int handle_speaker_io(
     return X86EMUL_OKAY;
 }
 
-int pv_pit_handler(int port, int data, int write)
-{
-    ioreq_t ioreq = {
-        .size = 1,
-        .type = IOREQ_TYPE_PIO,
-        .addr = port,
-        .dir  = write ? IOREQ_WRITE : IOREQ_READ,
-        .data = data
-    };
-
-    if ( is_hardware_domain(current->domain) && hwdom_pit_access(&ioreq) )
-    {
-        /* nothing to do */;
-    }
-    else
-    {
-        uint32_t val = data;
-        if ( port == 0x61 )
-            handle_speaker_io(ioreq.dir, port, 1, &val);
-        else
-            handle_pit_io(ioreq.dir, port, 1, &val);
-        ioreq.data = val;
-    }
-
-    return !write ? ioreq.data : 0;
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e105b95..a9d7e83 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1839,11 +1839,7 @@  uint32_t guest_io_read(unsigned int port, unsigned int bytes,
         unsigned int size = 1;
         uint32_t sub_data = ~0;
 
-        if ( (port == 0x42) || (port == 0x43) || (port == 0x61) )
-        {
-            sub_data = pv_pit_handler(port, 0, 0);
-        }
-        else if ( (port == RTC_PORT(0)) )
+        if ( (port == RTC_PORT(0)) )
         {
             sub_data = currd->arch.cmos_idx;
         }
@@ -1908,11 +1904,7 @@  void guest_io_write(unsigned int port, unsigned int bytes, uint32_t data,
     {
         unsigned int size = 1;
 
-        if ( (port == 0x42) || (port == 0x43) || (port == 0x61) )
-        {
-            pv_pit_handler(port, (uint8_t)data, 1);
-        }
-        else if ( (port == RTC_PORT(0)) )
+        if ( (port == RTC_PORT(0)) )
         {
             currd->arch.cmos_idx = data;
         }
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 495d669..557bb4a 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -172,7 +172,6 @@  void create_periodic_time(
     uint64_t period, uint8_t irq, time_cb *cb, void *data);
 void destroy_periodic_time(struct periodic_time *pt);
 
-int pv_pit_handler(int port, int data, int write);
 void pit_reset(struct domain *d);
 
 void pit_init(struct domain *d, unsigned long cpu_khz);