diff mbox

[v2] x86: fix printed messages in arch_set_info_hvm_guest

Message ID 20170809105045.51180-1-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Pau Monne Aug. 9, 2017, 10:50 a.m. UTC 
Append the target vCPU in the messages printed by
arch_set_info_hvm_guest.

While there constify the arguments of check_segment.

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/hvm/domain.c | 89 ++++++++++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 35 deletions(-)

Comments

Jan Beulich Aug. 14, 2017, 12:11 p.m. UTC | #1 
>>> On 09.08.17 at 12:50, <roger.pau@citrix.com> wrote:
> Append the target vCPU in the messages printed by
> arch_set_info_hvm_guest.

While this is a good idea, I'm not convinced of the use of these
messages in non-debug builds. And if they're to stay, I'd really
like to ask to make them as short as possible (without losing
information), e.g.

> @@ -37,7 +40,8 @@ static int check_segment(struct segment_reg
>      {
>          if ( seg != x86_seg_ds && seg != x86_seg_es )
>          {
> -            gprintk(XENLOG_ERR, "Null selector provided for CS, SS or TR\n");
> +            gprintk(XENLOG_ERR,
> +                    "Null selector provided for CS, SS or TR for %pv\n", v);

"CS, SS, or TR is null for %pv"

If, otoh, they'd be converted to gdprintk() I could live with them
being as long as they are; then I only dislike the frequent double
"for" that you introduce (but clearly that's a matter of taste,
unless native speakers said otherwise).

Jan
diff mbox

Patch

diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
index 60474649de..7cfb0f4051 100644
--- a/xen/arch/x86/hvm/domain.c
+++ b/xen/arch/x86/hvm/domain.c
@@ -24,12 +24,15 @@ 
 
 #include <public/hvm/hvm_vcpu.h>
 
-static int check_segment(struct segment_register *reg, enum x86_segment seg)
+static int check_segment(const struct vcpu *v,
+                         const struct segment_register *reg,
+                         const enum x86_segment seg)
 {
 
     if ( reg->pad != 0 )
     {
-        gprintk(XENLOG_ERR, "Segment attribute bits 12-15 are not zero\n");
+        gprintk(XENLOG_ERR,
+                "Segment attribute bits 12-15 are not zero for %pv\n", v);
         return -EINVAL;
     }
 
@@ -37,7 +40,8 @@  static int check_segment(struct segment_register *reg, enum x86_segment seg)
     {
         if ( seg != x86_seg_ds && seg != x86_seg_es )
         {
-            gprintk(XENLOG_ERR, "Null selector provided for CS, SS or TR\n");
+            gprintk(XENLOG_ERR,
+                    "Null selector provided for CS, SS or TR for %pv\n", v);
             return -EINVAL;
         }
         return 0;
@@ -47,26 +51,29 @@  static int check_segment(struct segment_register *reg, enum x86_segment seg)
     {
         if ( reg->s )
         {
-            gprintk(XENLOG_ERR, "Code or data segment provided for TR\n");
+            gprintk(XENLOG_ERR,
+                    "Code or data segment provided for TR for %pv\n", v);
             return -EINVAL;
         }
 
         if ( reg->type != SYS_DESC_tss_busy )
         {
-            gprintk(XENLOG_ERR, "Non-32-bit-TSS segment provided for TR\n");
+            gprintk(XENLOG_ERR,
+                    "Non-32-bit-TSS segment provided for TR for %pv\n", v);
             return -EINVAL;
         }
     }
     else if ( !reg->s )
     {
         gprintk(XENLOG_ERR,
-                "System segment provided for a code or data segment\n");
+                "System segment provided for a code or data segment for %pv\n",
+                v);
         return -EINVAL;
     }
 
     if ( !reg->p )
     {
-        gprintk(XENLOG_ERR, "Non-present segment provided\n");
+        gprintk(XENLOG_ERR, "Non-present segment provided for %pv\n", v);
         return -EINVAL;
     }
 
@@ -75,7 +82,8 @@  static int check_segment(struct segment_register *reg, enum x86_segment seg)
     case x86_seg_cs:
         if ( !(reg->type & 0x8) )
         {
-            gprintk(XENLOG_ERR, "Non-code segment provided for CS\n");
+            gprintk(XENLOG_ERR, "Non-code segment provided for CS for %pv\n",
+                    v);
             return -EINVAL;
         }
         break;
@@ -83,7 +91,8 @@  static int check_segment(struct segment_register *reg, enum x86_segment seg)
     case x86_seg_ss:
         if ( (reg->type & 0x8) || !(reg->type & 0x2) )
         {
-            gprintk(XENLOG_ERR, "Non-writeable segment provided for SS\n");
+            gprintk(XENLOG_ERR,
+                    "Non-writeable segment provided for SS for %pv\n", v);
             return -EINVAL;
         }
         break;
@@ -92,7 +101,8 @@  static int check_segment(struct segment_register *reg, enum x86_segment seg)
     case x86_seg_es:
         if ( (reg->type & 0x8) && !(reg->type & 0x2) )
         {
-            gprintk(XENLOG_ERR, "Non-readable segment provided for DS or ES\n");
+            gprintk(XENLOG_ERR,
+                    "Non-readable segment provided for DS or ES for %pv\n", v);
             return -EINVAL;
         }
         break;
@@ -141,7 +151,7 @@  int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
     /* Set accessed / busy bit for present segments. */                     \
     if ( s.p )                                                              \
         s.type |= (x86_seg_##s != x86_seg_tr ? 1 : 2);                      \
-    check_segment(&s, x86_seg_ ## s); })
+    check_segment(v, &s, x86_seg_ ## s); })
 
         rc = SEG(cs, regs);
         rc |= SEG(ds, regs);
@@ -159,36 +169,41 @@  int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
             limit = (limit << 12) | 0xfff;
         if ( regs->eip > limit )
         {
-            gprintk(XENLOG_ERR, "EIP (%#08x) outside CS limit (%#08x)\n",
-                    regs->eip, limit);
+            gprintk(XENLOG_ERR,
+                    "EIP (%#08x) outside CS limit (%#08x) for %pv\n",
+                    regs->eip, limit, v);
             return -EINVAL;
         }
 
         if ( ss.dpl != cs.dpl )
         {
-            gprintk(XENLOG_ERR, "SS.DPL (%u) is different than CS.DPL (%u)\n",
-                    ss.dpl, cs.dpl);
+            gprintk(XENLOG_ERR,
+                    "SS.DPL (%u) is different than CS.DPL (%u) for %pv\n",
+                    ss.dpl, cs.dpl, v);
             return -EINVAL;
         }
 
         if ( ds.p && ds.dpl > cs.dpl )
         {
-            gprintk(XENLOG_ERR, "DS.DPL (%u) is greater than CS.DPL (%u)\n",
-                    ds.dpl, cs.dpl);
+            gprintk(XENLOG_ERR,
+                    "DS.DPL (%u) is greater than CS.DPL (%u) for %pv\n",
+                    ds.dpl, cs.dpl, v);
             return -EINVAL;
         }
 
         if ( es.p && es.dpl > cs.dpl )
         {
-            gprintk(XENLOG_ERR, "ES.DPL (%u) is greater than CS.DPL (%u)\n",
-                    es.dpl, cs.dpl);
+            gprintk(XENLOG_ERR,
+                    "ES.DPL (%u) is greater than CS.DPL (%u) for %pv\n",
+                    es.dpl, cs.dpl, v);
             return -EINVAL;
         }
 
         if ( (regs->efer & EFER_LMA) && !(regs->efer & EFER_LME) )
         {
-            gprintk(XENLOG_ERR, "EFER.LMA set without EFER.LME (%#016lx)\n",
-                    regs->efer);
+            gprintk(XENLOG_ERR,
+                    "EFER.LMA set without EFER.LME (%#016lx) for %pv\n",
+                    regs->efer, v);
             return -EINVAL;
         }
 
@@ -217,29 +232,33 @@  int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
         /* Basic sanity checks. */
         if ( !is_canonical_address(regs->rip) )
         {
-            gprintk(XENLOG_ERR, "RIP contains a non-canonical address (%#lx)\n",
-                    regs->rip);
+            gprintk(XENLOG_ERR,
+                    "RIP contains a non-canonical address (%#lx) for %pv\n",
+                    regs->rip, v);
             return -EINVAL;
         }
 
         if ( !(regs->cr0 & X86_CR0_PG) )
         {
-            gprintk(XENLOG_ERR, "CR0 doesn't have paging enabled (%#016lx)\n",
-                    regs->cr0);
+            gprintk(XENLOG_ERR,
+                    "CR0 doesn't have paging enabled (%#016lx) for %pv\n",
+                    regs->cr0, v);
             return -EINVAL;
         }
 
         if ( !(regs->cr4 & X86_CR4_PAE) )
         {
-            gprintk(XENLOG_ERR, "CR4 doesn't have PAE enabled (%#016lx)\n",
-                    regs->cr4);
+            gprintk(XENLOG_ERR,
+                    "CR4 doesn't have PAE enabled (%#016lx) for %pv\n",
+                    regs->cr4, v);
             return -EINVAL;
         }
 
         if ( !(regs->efer & EFER_LME) )
         {
-            gprintk(XENLOG_ERR, "EFER doesn't have LME enabled (%#016lx)\n",
-                    regs->efer);
+            gprintk(XENLOG_ERR,
+                    "EFER doesn't have LME enabled (%#016lx) for %pv\n",
+                    regs->efer, v);
             return -EINVAL;
         }
 
@@ -274,16 +293,16 @@  int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
 
     if ( v->arch.hvm_vcpu.guest_cr[4] & ~hvm_cr4_guest_valid_bits(v, 0) )
     {
-        gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
-                v->arch.hvm_vcpu.guest_cr[4]);
+        gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx for %pv\n",
+                v->arch.hvm_vcpu.guest_cr[4], v);
         return -EINVAL;
     }
 
     errstr = hvm_efer_valid(v, v->arch.hvm_vcpu.guest_efer, -1);
     if ( errstr )
     {
-        gprintk(XENLOG_ERR, "Bad EFER value (%#016lx): %s\n",
-               v->arch.hvm_vcpu.guest_efer, errstr);
+        gprintk(XENLOG_ERR, "Bad EFER value (%#016lx): %s for %pv\n",
+                v->arch.hvm_vcpu.guest_efer, errstr, v);
         return -EINVAL;
     }
 
@@ -300,8 +319,8 @@  int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
                                  NULL, P2M_ALLOC);
         if ( !page )
         {
-            gprintk(XENLOG_ERR, "Invalid CR3: %#lx\n",
-                    v->arch.hvm_vcpu.guest_cr[3]);
+            gprintk(XENLOG_ERR, "Invalid CR3: %#lx for %pv\n",
+                    v->arch.hvm_vcpu.guest_cr[3], v);
             return -EINVAL;
         }