Removing the PVH assert in arch/x86/hvm/io.c:87

Removing the PVH assert in arch/x86/hvm/io.c:87

[Roger Pau Monné]

Hello,

I've just stumbled upon this assert while testing PVH on different
hardware. It was added in 7c4870 as a safe belt, but it turns out INS
and OUTS go through handle_mmio. So using this instructions from a PVH
guest basically kills Xen.

I've removed it and everything seems fine, so I'm considering sending a
patch for 4.5 in order to have it removed. I think the path that could
trigger the crash because of the missing vioapic stuff is already
guarded by the other chunk added in the same patch.

Roger.

Re: Removing the PVH assert in arch/x86/hvm/io.c:87

[Mukesh Rathor]

On Thu, 4 Dec 2014 17:35:59 +0100
Roger Pau Monné <roger.pau@citrix.com> wrote:

> Hello,
> 
> I've just stumbled upon this assert while testing PVH on different
> hardware. It was added in 7c4870 as a safe belt, but it turns out INS
> and OUTS go through handle_mmio. So using this instructions from a PVH
> guest basically kills Xen.

Right. Unf CR-moves/lmsw/clts intercepts also go thru handle_mmio, and
the suggestion was to clean it up first with another emulator function
for CR/IO intercepts. I attempted to do that before I left :

http://lists.xen.org/archives/html/xen-devel/2014-08/msg02204.html

See also:

http://lists.xen.org/archives/html/xen-devel/2014-08/msg00391.html

> I've removed it and everything seems fine, so I'm considering sending
> a patch for 4.5 in order to have it removed. I think the path that
> could trigger the crash because of the missing vioapic stuff is
> already guarded by the other chunk added in the same patch.

No, there used to be another path thru hvm_hap_nested_page_fault()
that would walk the back end handlers and crash xen. So you might
wanna check to make sure. I see hvm_hap_nested_page_fault() looks a
little different now, so not sure if its still broken... 

Mukesh


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Removing the PVH assert in arch/x86/hvm/io.c:87

[Jan Beulich]

>>> On 04.12.14 at 17:35, <roger.pau@citrix.com> wrote:
> I've just stumbled upon this assert while testing PVH on different
> hardware. It was added in 7c4870 as a safe belt, but it turns out INS
> and OUTS go through handle_mmio. So using this instructions from a PVH
> guest basically kills Xen.
> 
> I've removed it and everything seems fine, so I'm considering sending a
> patch for 4.5 in order to have it removed. I think the path that could
> trigger the crash because of the missing vioapic stuff is already
> guarded by the other chunk added in the same patch.

Iirc we settled on forbidding paths to handle_mmio() for PVH (hence
the ASSERT()). Sadly you provide way too little detail on what is
actually happening in your case: What's the use case of to-be-
emulated INS/OUTS in a PVH kernel? What's the call tree that gets
you into handle_mmio(), considering that both calls to
handle_mmio_with_translation() from hvm_hap_nested_page_fault()
as well as the one to handle_mmio() ought to be unreachable for PVH?

Jan

Re: Removing the PVH assert in arch/x86/hvm/io.c:87

[Roger Pau Monné]

El 05/12/14 a les 10.15, Jan Beulich ha escrit:
>>>> On 04.12.14 at 17:35, <roger.pau@citrix.com> wrote:
>> I've just stumbled upon this assert while testing PVH on different
>> hardware. It was added in 7c4870 as a safe belt, but it turns out INS
>> and OUTS go through handle_mmio. So using this instructions from a PVH
>> guest basically kills Xen.
>>
>> I've removed it and everything seems fine, so I'm considering sending a
>> patch for 4.5 in order to have it removed. I think the path that could
>> trigger the crash because of the missing vioapic stuff is already
>> guarded by the other chunk added in the same patch.
> 
> Iirc we settled on forbidding paths to handle_mmio() for PVH (hence
> the ASSERT()). Sadly you provide way too little detail on what is
> actually happening in your case: What's the use case of to-be-
> emulated INS/OUTS in a PVH kernel?

In this specific situation I'm seeing intsw instructions executed by the
FreeBSD ATA layer:

http://fxr.watson.org/fxr/source/dev/ata/ata-lowlevel.c#L740

> What's the call tree that gets
> you into handle_mmio(), considering that both calls to
> handle_mmio_with_translation() from hvm_hap_nested_page_fault()
> as well as the one to handle_mmio() ought to be unreachable for PVH?

You can get there from vmx_vmexit_handler if the exit reason is
EXIT_REASON_IO_INSTRUCTION.

Roger.

Re: Removing the PVH assert in arch/x86/hvm/io.c:87

[David Vrabel]

On 05/12/14 11:07, Roger Pau Monné wrote:
> El 05/12/14 a les 10.15, Jan Beulich ha escrit:
>>>>> On 04.12.14 at 17:35, <roger.pau@citrix.com> wrote:
>>> I've just stumbled upon this assert while testing PVH on different
>>> hardware. It was added in 7c4870 as a safe belt, but it turns out INS
>>> and OUTS go through handle_mmio. So using this instructions from a PVH
>>> guest basically kills Xen.
>>>
>>> I've removed it and everything seems fine, so I'm considering sending a
>>> patch for 4.5 in order to have it removed. I think the path that could
>>> trigger the crash because of the missing vioapic stuff is already
>>> guarded by the other chunk added in the same patch.
>>
>> Iirc we settled on forbidding paths to handle_mmio() for PVH (hence
>> the ASSERT()). Sadly you provide way too little detail on what is
>> actually happening in your case: What's the use case of to-be-
>> emulated INS/OUTS in a PVH kernel?
> 
> In this specific situation I'm seeing intsw instructions executed by the
> FreeBSD ATA layer:
> 
> http://fxr.watson.org/fxr/source/dev/ata/ata-lowlevel.c#L740

Why are you running this device driver at all in a PVH guest?  It should
only be using PV block devices.

David

Re: Removing the PVH assert in arch/x86/hvm/io.c:87

[Jan Beulich]

>>> On 05.12.14 at 12:07, <roger.pau@citrix.com> wrote:
> El 05/12/14 a les 10.15, Jan Beulich ha escrit:
>>>>> On 04.12.14 at 17:35, <roger.pau@citrix.com> wrote:
>>> I've just stumbled upon this assert while testing PVH on different
>>> hardware. It was added in 7c4870 as a safe belt, but it turns out INS
>>> and OUTS go through handle_mmio. So using this instructions from a PVH
>>> guest basically kills Xen.
>>>
>>> I've removed it and everything seems fine, so I'm considering sending a
>>> patch for 4.5 in order to have it removed. I think the path that could
>>> trigger the crash because of the missing vioapic stuff is already
>>> guarded by the other chunk added in the same patch.
>> 
>> Iirc we settled on forbidding paths to handle_mmio() for PVH (hence
>> the ASSERT()). Sadly you provide way too little detail on what is
>> actually happening in your case: What's the use case of to-be-
>> emulated INS/OUTS in a PVH kernel?
> 
> In this specific situation I'm seeing intsw instructions executed by the
> FreeBSD ATA layer:
> 
> http://fxr.watson.org/fxr/source/dev/ata/ata-lowlevel.c#L740 
> 
>> What's the call tree that gets
>> you into handle_mmio(), considering that both calls to
>> handle_mmio_with_translation() from hvm_hap_nested_page_fault()
>> as well as the one to handle_mmio() ought to be unreachable for PVH?
> 
> You can get there from vmx_vmexit_handler if the exit reason is
> EXIT_REASON_IO_INSTRUCTION.

A PVH guest without passed through device shouldn't access I/O
ports in the first place. Are you trying to hand an IDE or SATA
controller to the guest? Or is this happening with just Dom0, in
which case I'd suspect the I/O bitmap isn't being set up properly,
thus causing a VM exit when none is needed?

And yes, guarding the EXIT_REASON_IO_INSTRUCTION handling
in vmx_vmexit_handler() against PVH would seem necessary,
directing control flow to exit_and_crash. I'm pretty certain I had
pointed this out while reviewing the original PVH series.

Jan