[v2,01/12] viridian: don't blindly write to 32-bit registers is 'mode' is invalid

Commit Message

Paul Durrant Nov. 20, 2020, 9:48 a.m. UTC

From: Paul Durrant <[e-mail]>

If hvm_guest_x86_mode() returns something other than 8 or 4 then
viridian_hypercall() will return immediately but, on the way out, will write
back status as if 'mode' was 4. This patch simply makes it leave the registers
alone.

NOTE: The formatting of the 'out' label and the switch statement are also
      adjusted as per CODING_STYLE.

Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Wei Liu <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>
Cc: "Roger Pau Monné" <[e-mail]>

v2:
 - New in v2
---
 xen/arch/x86/hvm/viridian/viridian.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Jan Beulich Nov. 20, 2020, 2:20 p.m. UTC | #1

On 20.11.2020 10:48, Paul Durrant wrote:
> From: Paul Durrant <[e-mail]>
> 
> If hvm_guest_x86_mode() returns something other than 8 or 4 then
> viridian_hypercall() will return immediately but, on the way out, will write
> back status as if 'mode' was 4. This patch simply makes it leave the registers
> alone.

IOW 16-bit protected mode and real mode aren't allowed to make
hypercalls (supported also be the earlier switch() in the
function)? But then, to achieve what you want, wouldn't it be
more direct to simply "return HVM_HCALL_completed;" straight
from that earlier switch()'s default case? At which point the
switch() you modify could become if/else? Anyway - you're the
maintainer, I'm just wondering ...

Jan

Durrant, Paul Nov. 20, 2020, 2:25 p.m. UTC | #2

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 20 November 2020 14:20
> To: Paul Durrant <[e-mail]>
> Cc: Durrant, Paul <[e-mail]>; Wei Liu <[e-mail]>; Andrew Cooper
> <[e-mail]>; Roger Pau Monné <[e-mail]>; [e-mail]
> Subject: RE: [EXTERNAL] [PATCH v2 01/12] viridian: don't blindly write to 32-bit registers is 'mode'
> is invalid
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 20.11.2020 10:48, Paul Durrant wrote:
> > From: Paul Durrant <[e-mail]>
> >
> > If hvm_guest_x86_mode() returns something other than 8 or 4 then
> > viridian_hypercall() will return immediately but, on the way out, will write
> > back status as if 'mode' was 4. This patch simply makes it leave the registers
> > alone.
> 
> IOW 16-bit protected mode and real mode aren't allowed to make
> hypercalls (supported also be the earlier switch() in the
> function)?

I don't think running enlightened versions of OS/2 1.3 is really a use case :-)

> But then, to achieve what you want, wouldn't it be
> more direct to simply "return HVM_HCALL_completed;" straight
> from that earlier switch()'s default case? At which point the
> switch() you modify could become if/else? Anyway - you're the
> maintainer, I'm just wondering ...
> 

It could be done that way but I prefer the exit path via goto.

  Paul

> Jan

Patch

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index dc7183a54627..54035f75cb1a 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -692,13 +692,14 @@  int viridian_hypercall(struct cpu_user_regs *regs)
         break;
     }
 
-out:
+ out:
     output.result = status;
     switch (mode) {
     case 8:
         regs->rax = output.raw;
         break;
-    default:
+
+    case 4:
         regs->rdx = output.raw >> 32;
         regs->rax = (uint32_t)output.raw;
         break;