[PATCH] x86/HVM: handle_{mmio*, pio}() return value adjustments

[PATCH] x86/HVM: handle_{mmio*, pio}() return value adjustments

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3877 bytes --]

Don't ignore their return values. Don't indicate success to callers of
handle_pio() when in fact the domain has been crashed.

Make all three functions return bool. Adjust formatting of switch()
statements being touched anyway.

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

--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -78,7 +78,7 @@ void send_invalidate_req(void)
         gprintk(XENLOG_ERR, "Unsuccessful map-cache invalidate\n");
 }
 
-int handle_mmio(void)
+bool handle_mmio(void)
 {
     struct hvm_emulate_ctxt ctxt;
     struct vcpu *curr = current;
@@ -100,22 +100,21 @@ int handle_mmio(void)
     {
     case X86EMUL_UNHANDLEABLE:
         hvm_dump_emulation_state(XENLOG_G_WARNING "MMIO", &ctxt);
-        return 0;
+        return false;
+
     case X86EMUL_EXCEPTION:
         if ( ctxt.ctxt.event_pending )
             hvm_inject_event(&ctxt.ctxt.event);
         break;
-    default:
-        break;
     }
 
     hvm_emulate_writeback(&ctxt);
 
-    return 1;
+    return true;
 }
 
-int handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
-                                 struct npfec access)
+bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
+                                  struct npfec access)
 {
     struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
 
@@ -127,7 +126,7 @@ int handle_mmio_with_translation(unsigne
     return handle_mmio();
 }
 
-int handle_pio(uint16_t port, unsigned int size, int dir)
+bool handle_pio(uint16_t port, unsigned int size, int dir)
 {
     struct vcpu *curr = current;
     struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
@@ -155,19 +154,20 @@ int handle_pio(uint16_t port, unsigned i
                 memcpy(&guest_cpu_user_regs()->rax, &data, size);
         }
         break;
+
     case X86EMUL_RETRY:
         /* We should not advance RIP/EIP if the domain is shutting down */
         if ( curr->domain->is_shutting_down )
-            return 0;
-
+            return false;
         break;
+
     default:
         gdprintk(XENLOG_ERR, "Weird HVM ioemulation status %d.\n", rc);
         domain_crash(curr->domain);
-        break;
+        return false;
     }
 
-    return 1;
+    return true;
 }
 
 static bool_t dpci_portio_accept(const struct hvm_io_handler *handler,
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -156,13 +156,14 @@ bool_t handle_hvm_io_completion(struct v
     {
     case HVMIO_no_completion:
         break;
+
     case HVMIO_mmio_completion:
-        handle_mmio();
-        break;
+        return handle_mmio();
+
     case HVMIO_pio_completion:
-        (void)handle_pio(vio->io_req.addr, vio->io_req.size,
-                         vio->io_req.dir);
-        break;
+        return handle_pio(vio->io_req.addr, vio->io_req.size,
+                          vio->io_req.dir);
+
     case HVMIO_realmode_completion:
     {
         struct hvm_emulate_ctxt ctxt;
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -118,10 +118,10 @@ void relocate_portio_handler(
 
 void send_timeoffset_req(unsigned long timeoff);
 void send_invalidate_req(void);
-int handle_mmio(void);
-int handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
-                                 struct npfec);
-int handle_pio(uint16_t port, unsigned int size, int dir);
+bool handle_mmio(void);
+bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
+                                  struct npfec);
+bool handle_pio(uint16_t port, unsigned int size, int dir);
 void hvm_interrupt_post(struct vcpu *v, int vector, int type);
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq,
                   const union vioapic_redir_entry *ent);




[-- Attachment #2: x86-HVM-IO-emul-bail.patch --]
[-- Type: text/plain, Size: 3929 bytes --]

x86/HVM: handle_{mmio*,pio}() return value adjustments

Don't ignore their return values. Don't indicate success to callers of
handle_pio() when in fact the domain has been crashed.

Make all three functions return bool. Adjust formatting of switch()
statements being touched anyway.

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

--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -78,7 +78,7 @@ void send_invalidate_req(void)
         gprintk(XENLOG_ERR, "Unsuccessful map-cache invalidate\n");
 }
 
-int handle_mmio(void)
+bool handle_mmio(void)
 {
     struct hvm_emulate_ctxt ctxt;
     struct vcpu *curr = current;
@@ -100,22 +100,21 @@ int handle_mmio(void)
     {
     case X86EMUL_UNHANDLEABLE:
         hvm_dump_emulation_state(XENLOG_G_WARNING "MMIO", &ctxt);
-        return 0;
+        return false;
+
     case X86EMUL_EXCEPTION:
         if ( ctxt.ctxt.event_pending )
             hvm_inject_event(&ctxt.ctxt.event);
         break;
-    default:
-        break;
     }
 
     hvm_emulate_writeback(&ctxt);
 
-    return 1;
+    return true;
 }
 
-int handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
-                                 struct npfec access)
+bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
+                                  struct npfec access)
 {
     struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
 
@@ -127,7 +126,7 @@ int handle_mmio_with_translation(unsigne
     return handle_mmio();
 }
 
-int handle_pio(uint16_t port, unsigned int size, int dir)
+bool handle_pio(uint16_t port, unsigned int size, int dir)
 {
     struct vcpu *curr = current;
     struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
@@ -155,19 +154,20 @@ int handle_pio(uint16_t port, unsigned i
                 memcpy(&guest_cpu_user_regs()->rax, &data, size);
         }
         break;
+
     case X86EMUL_RETRY:
         /* We should not advance RIP/EIP if the domain is shutting down */
         if ( curr->domain->is_shutting_down )
-            return 0;
-
+            return false;
         break;
+
     default:
         gdprintk(XENLOG_ERR, "Weird HVM ioemulation status %d.\n", rc);
         domain_crash(curr->domain);
-        break;
+        return false;
     }
 
-    return 1;
+    return true;
 }
 
 static bool_t dpci_portio_accept(const struct hvm_io_handler *handler,
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -156,13 +156,14 @@ bool_t handle_hvm_io_completion(struct v
     {
     case HVMIO_no_completion:
         break;
+
     case HVMIO_mmio_completion:
-        handle_mmio();
-        break;
+        return handle_mmio();
+
     case HVMIO_pio_completion:
-        (void)handle_pio(vio->io_req.addr, vio->io_req.size,
-                         vio->io_req.dir);
-        break;
+        return handle_pio(vio->io_req.addr, vio->io_req.size,
+                          vio->io_req.dir);
+
     case HVMIO_realmode_completion:
     {
         struct hvm_emulate_ctxt ctxt;
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -118,10 +118,10 @@ void relocate_portio_handler(
 
 void send_timeoffset_req(unsigned long timeoff);
 void send_invalidate_req(void);
-int handle_mmio(void);
-int handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
-                                 struct npfec);
-int handle_pio(uint16_t port, unsigned int size, int dir);
+bool handle_mmio(void);
+bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
+                                  struct npfec);
+bool handle_pio(uint16_t port, unsigned int size, int dir);
 void hvm_interrupt_post(struct vcpu *v, int vector, int type);
 void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq,
                   const union vioapic_redir_entry *ent);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] x86/HVM: handle_{mmio*, pio}() return value adjustments

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 16 December 2016 09:31
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Subject: [PATCH] x86/HVM: handle_{mmio*,pio}() return value adjustments
> 
> Don't ignore their return values. Don't indicate success to callers of
> handle_pio() when in fact the domain has been crashed.
> 
> Make all three functions return bool. Adjust formatting of switch()
> statements being touched anyway.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -78,7 +78,7 @@ void send_invalidate_req(void)
>          gprintk(XENLOG_ERR, "Unsuccessful map-cache invalidate\n");
>  }
> 
> -int handle_mmio(void)
> +bool handle_mmio(void)
>  {
>      struct hvm_emulate_ctxt ctxt;
>      struct vcpu *curr = current;
> @@ -100,22 +100,21 @@ int handle_mmio(void)
>      {
>      case X86EMUL_UNHANDLEABLE:
>          hvm_dump_emulation_state(XENLOG_G_WARNING "MMIO", &ctxt);
> -        return 0;
> +        return false;
> +
>      case X86EMUL_EXCEPTION:
>          if ( ctxt.ctxt.event_pending )
>              hvm_inject_event(&ctxt.ctxt.event);
>          break;
> -    default:
> -        break;

Should there not be some sort of default case, even if it's simply to assert that it's not reachable?

>      }
> 
>      hvm_emulate_writeback(&ctxt);
> 
> -    return 1;
> +    return true;
>  }
> 
> -int handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
> -                                 struct npfec access)
> +bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
> +                                  struct npfec access)
>  {
>      struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
> 
> @@ -127,7 +126,7 @@ int handle_mmio_with_translation(unsigne
>      return handle_mmio();
>  }
> 
> -int handle_pio(uint16_t port, unsigned int size, int dir)
> +bool handle_pio(uint16_t port, unsigned int size, int dir)
>  {
>      struct vcpu *curr = current;
>      struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
> @@ -155,19 +154,20 @@ int handle_pio(uint16_t port, unsigned i
>                  memcpy(&guest_cpu_user_regs()->rax, &data, size);
>          }
>          break;
> +
>      case X86EMUL_RETRY:
>          /* We should not advance RIP/EIP if the domain is shutting down */
>          if ( curr->domain->is_shutting_down )
> -            return 0;
> -
> +            return false;
>          break;
> +
>      default:
>          gdprintk(XENLOG_ERR, "Weird HVM ioemulation status %d.\n", rc);
>          domain_crash(curr->domain);
> -        break;
> +        return false;
>      }
> 
> -    return 1;
> +    return true;
>  }
> 
>  static bool_t dpci_portio_accept(const struct hvm_io_handler *handler,
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -156,13 +156,14 @@ bool_t handle_hvm_io_completion(struct v

Do you not want to change this to from bool_t to bool while you're at it?

>      {
>      case HVMIO_no_completion:
>          break;
> +
>      case HVMIO_mmio_completion:
> -        handle_mmio();
> -        break;
> +        return handle_mmio();
> +
>      case HVMIO_pio_completion:
> -        (void)handle_pio(vio->io_req.addr, vio->io_req.size,
> -                         vio->io_req.dir);
> -        break;
> +        return handle_pio(vio->io_req.addr, vio->io_req.size,
> +                          vio->io_req.dir);
> +
>      case HVMIO_realmode_completion:
>      {
>          struct hvm_emulate_ctxt ctxt;
> --- a/xen/include/asm-x86/hvm/io.h
> +++ b/xen/include/asm-x86/hvm/io.h
> @@ -118,10 +118,10 @@ void relocate_portio_handler(
> 
>  void send_timeoffset_req(unsigned long timeoff);
>  void send_invalidate_req(void);
> -int handle_mmio(void);
> -int handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
> -                                 struct npfec);
> -int handle_pio(uint16_t port, unsigned int size, int dir);
> +bool handle_mmio(void);
> +bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
> +                                  struct npfec);
> +bool handle_pio(uint16_t port, unsigned int size, int dir);
>  void hvm_interrupt_post(struct vcpu *v, int vector, int type);
>  void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq,
>                    const union vioapic_redir_entry *ent);
> 
> 


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

Re: [PATCH] x86/HVM: handle_{mmio*, pio}() return value adjustments

[Jan Beulich]

>>> On 16.12.16 at 10:52, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 16 December 2016 09:31
>> @@ -100,22 +100,21 @@ int handle_mmio(void)
>>      {
>>      case X86EMUL_UNHANDLEABLE:
>>          hvm_dump_emulation_state(XENLOG_G_WARNING "MMIO", &ctxt);
>> -        return 0;
>> +        return false;
>> +
>>      case X86EMUL_EXCEPTION:
>>          if ( ctxt.ctxt.event_pending )
>>              hvm_inject_event(&ctxt.ctxt.event);
>>          break;
>> -    default:
>> -        break;
> 
> Should there not be some sort of default case, even if it's simply to assert 
> that it's not reachable?

A default case doing nothing when the switch expression is not of
an enum type is pointless. And it _is_ reachable (namely for
X86EMUL_OKAY).

>> --- a/xen/arch/x86/hvm/ioreq.c
>> +++ b/xen/arch/x86/hvm/ioreq.c
>> @@ -156,13 +156,14 @@ bool_t handle_hvm_io_completion(struct v
> 
> Do you not want to change this to from bool_t to bool while you're at it?

Well, while I first was inclined to do so, I then didn't want to do
too many things at once (iirc there would be a few more which then
would want updating at the same time).

Jan


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

Re: [PATCH] x86/HVM: handle_{mmio*, pio}() return value adjustments

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 16 December 2016 10:25
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [PATCH] x86/HVM: handle_{mmio*,pio}() return value
> adjustments
> 
> >>> On 16.12.16 at 10:52, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 16 December 2016 09:31
> >> @@ -100,22 +100,21 @@ int handle_mmio(void)
> >>      {
> >>      case X86EMUL_UNHANDLEABLE:
> >>          hvm_dump_emulation_state(XENLOG_G_WARNING "MMIO",
> &ctxt);
> >> -        return 0;
> >> +        return false;
> >> +
> >>      case X86EMUL_EXCEPTION:
> >>          if ( ctxt.ctxt.event_pending )
> >>              hvm_inject_event(&ctxt.ctxt.event);
> >>          break;
> >> -    default:
> >> -        break;
> >
> > Should there not be some sort of default case, even if it's simply to assert
> > that it's not reachable?
> 
> A default case doing nothing when the switch expression is not of
> an enum type is pointless. And it _is_ reachable (namely for
> X86EMUL_OKAY).

Then my preference would still be an explicit case for X86EMUL_OKAY and an unreachable default, but if you don't think it's worth it then the code is ok as-is.

> 
> >> --- a/xen/arch/x86/hvm/ioreq.c
> >> +++ b/xen/arch/x86/hvm/ioreq.c
> >> @@ -156,13 +156,14 @@ bool_t handle_hvm_io_completion(struct v
> >
> > Do you not want to change this to from bool_t to bool while you're at it?
> 
> Well, while I first was inclined to do so, I then didn't want to do
> too many things at once (iirc there would be a few more which then
> would want updating at the same time).
> 

Fair enough.

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> Jan


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

Re: [PATCH] x86/HVM: handle_{mmio*, pio}() return value adjustments

[Andrew Cooper]

On 16/12/16 09:31, Jan Beulich wrote:
> Don't ignore their return values. Don't indicate success to callers of
> handle_pio() when in fact the domain has been crashed.
>
> Make all three functions return bool. Adjust formatting of switch()
> statements being touched anyway.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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