| Message ID | 1452688338-70075-5-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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.
>>> 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
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
>>> 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
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.
>>> 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
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);
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(-)