[PATCH] x86/HVM: improve a few state load checks

[PATCH] x86/HVM: improve a few state load checks

[Jan Beulich]

Using plain int for instance numbers looks quite dangerous without
being aware that hvm_load_instance() returns an unsigned quantity. Make
this more explicit. Also replace uint16_t uses by unsigned int.

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

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -976,14 +976,13 @@ unsigned long hvm_cr4_guest_valid_bits(c
 
 static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
 {
-    int vcpuid;
+    unsigned int vcpuid = hvm_load_instance(h);
     struct vcpu *v;
     struct hvm_hw_cpu ctxt;
     struct segment_register seg;
     const char *errstr;
 
     /* Which vcpu is this? */
-    vcpuid = hvm_load_instance(h);
     if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
     {
         dprintk(XENLOG_G_ERR, "HVM restore: dom%u has no vcpu%u\n",
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -768,7 +768,7 @@ static int hvm_save_mtrr_msr(struct doma
 
 static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
 {
-    int vcpuid, i;
+    unsigned int vcpuid, i;
     struct vcpu *v;
     struct mtrr_state *mtrr_state;
     struct hvm_hw_mtrr hw_mtrr;
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -1048,11 +1048,10 @@ static int viridian_save_vcpu_ctxt(struc
 
 static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
 {
-    int vcpuid;
+    unsigned int vcpuid = hvm_load_instance(h);
     struct vcpu *v;
     struct hvm_viridian_vcpu_context ctxt;
 
-    vcpuid = hvm_load_instance(h);
     if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
     {
         dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no vcpu%u\n",
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1507,7 +1507,7 @@ static void lapic_load_fixup(struct vlap
 
 static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
 {
-    uint16_t vcpuid;
+    unsigned int vcpuid = hvm_load_instance(h);
     struct vcpu *v;
     struct vlapic *s;
 
@@ -1515,7 +1515,6 @@ static int lapic_load_hidden(struct doma
         return -ENODEV;
 
     /* Which vlapic to load? */
-    vcpuid = hvm_load_instance(h); 
     if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
     {
         dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n",
@@ -1542,7 +1541,7 @@ static int lapic_load_hidden(struct doma
 
 static int lapic_load_regs(struct domain *d, hvm_domain_context_t *h)
 {
-    uint16_t vcpuid;
+    unsigned int vcpuid = hvm_load_instance(h);
     struct vcpu *v;
     struct vlapic *s;
 
@@ -1550,7 +1549,6 @@ static int lapic_load_regs(struct domain
         return -ENODEV;
 
     /* Which vlapic to load? */
-    vcpuid = hvm_load_instance(h); 
     if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
     {
         dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n",
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -393,13 +393,12 @@ static int vpic_save(struct domain *d, h
 static int vpic_load(struct domain *d, hvm_domain_context_t *h)
 {
     struct hvm_hw_vpic *s;
-    uint16_t inst;
+    unsigned int inst = hvm_load_instance(h);
 
     if ( !has_vpic(d) )
         return -ENODEV;
 
     /* Which PIC is this? */
-    inst = hvm_load_instance(h);
     if ( inst > 1 )
         return -EINVAL;
     s = &d->arch.hvm_domain.vpic[inst];
--- a/xen/include/asm-x86/hvm/save.h
+++ b/xen/include/asm-x86/hvm/save.h
@@ -84,10 +84,10 @@ void _hvm_read_entry(struct hvm_domain_c
     _hvm_load_entry(_x, _h, _dst, 0)
 
 /* Unmarshalling: what is the instance ID of the next entry? */
-static inline uint16_t hvm_load_instance(struct hvm_domain_context *h)
+static inline unsigned int hvm_load_instance(const struct hvm_domain_context *h)
 {
-    struct hvm_save_descriptor *d 
-        = (struct hvm_save_descriptor *)&h->data[h->cur];
+    const struct hvm_save_descriptor *d = (const void *)&h->data[h->cur];
+
     return d->instance;
 }
 




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

Re: [PATCH] x86/HVM: improve a few state load checks

[Andrew Cooper]

On 16/07/18 14:58, Jan Beulich wrote:
> Using plain int for instance numbers looks quite dangerous without
> being aware that hvm_load_instance() returns an unsigned quantity. Make
> this more explicit. Also replace uint16_t uses by unsigned int.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

Re: [PATCH] x86/HVM: improve a few state load checks

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 16 July 2018 14:58
> 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: improve a few state load checks
> 
> Using plain int for instance numbers looks quite dangerous without
> being aware that hvm_load_instance() returns an unsigned quantity. Make
> this more explicit. Also replace uint16_t uses by unsigned int.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -976,14 +976,13 @@ unsigned long hvm_cr4_guest_valid_bits(c
> 
>  static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>  {
> -    int vcpuid;
> +    unsigned int vcpuid = hvm_load_instance(h);
>      struct vcpu *v;
>      struct hvm_hw_cpu ctxt;
>      struct segment_register seg;
>      const char *errstr;
> 
>      /* Which vcpu is this? */
> -    vcpuid = hvm_load_instance(h);
>      if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )

This open coded pattern:

vcpuid = hvm_load_instance(h);
if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
{ ...

seems to be repeated an awful lot. Is it time, perhaps, to introduce a helper function that incorporates the check?

  Paul

>      {
>          dprintk(XENLOG_G_ERR, "HVM restore: dom%u has no vcpu%u\n",
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -768,7 +768,7 @@ static int hvm_save_mtrr_msr(struct doma
> 
>  static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t
> *h)
>  {
> -    int vcpuid, i;
> +    unsigned int vcpuid, i;
>      struct vcpu *v;
>      struct mtrr_state *mtrr_state;
>      struct hvm_hw_mtrr hw_mtrr;
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -1048,11 +1048,10 @@ static int viridian_save_vcpu_ctxt(struc
> 
>  static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t
> *h)
>  {
> -    int vcpuid;
> +    unsigned int vcpuid = hvm_load_instance(h);
>      struct vcpu *v;
>      struct hvm_viridian_vcpu_context ctxt;
> 
> -    vcpuid = hvm_load_instance(h);
>      if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
>      {
>          dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no vcpu%u\n",
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1507,7 +1507,7 @@ static void lapic_load_fixup(struct vlap
> 
>  static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
>  {
> -    uint16_t vcpuid;
> +    unsigned int vcpuid = hvm_load_instance(h);
>      struct vcpu *v;
>      struct vlapic *s;
> 
> @@ -1515,7 +1515,6 @@ static int lapic_load_hidden(struct doma
>          return -ENODEV;
> 
>      /* Which vlapic to load? */
> -    vcpuid = hvm_load_instance(h);
>      if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
>      {
>          dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n",
> @@ -1542,7 +1541,7 @@ static int lapic_load_hidden(struct doma
> 
>  static int lapic_load_regs(struct domain *d, hvm_domain_context_t *h)
>  {
> -    uint16_t vcpuid;
> +    unsigned int vcpuid = hvm_load_instance(h);
>      struct vcpu *v;
>      struct vlapic *s;
> 
> @@ -1550,7 +1549,6 @@ static int lapic_load_regs(struct domain
>          return -ENODEV;
> 
>      /* Which vlapic to load? */
> -    vcpuid = hvm_load_instance(h);
>      if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
>      {
>          dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n",
> --- a/xen/arch/x86/hvm/vpic.c
> +++ b/xen/arch/x86/hvm/vpic.c
> @@ -393,13 +393,12 @@ static int vpic_save(struct domain *d, h
>  static int vpic_load(struct domain *d, hvm_domain_context_t *h)
>  {
>      struct hvm_hw_vpic *s;
> -    uint16_t inst;
> +    unsigned int inst = hvm_load_instance(h);
> 
>      if ( !has_vpic(d) )
>          return -ENODEV;
> 
>      /* Which PIC is this? */
> -    inst = hvm_load_instance(h);
>      if ( inst > 1 )
>          return -EINVAL;
>      s = &d->arch.hvm_domain.vpic[inst];
> --- a/xen/include/asm-x86/hvm/save.h
> +++ b/xen/include/asm-x86/hvm/save.h
> @@ -84,10 +84,10 @@ void _hvm_read_entry(struct hvm_domain_c
>      _hvm_load_entry(_x, _h, _dst, 0)
> 
>  /* Unmarshalling: what is the instance ID of the next entry? */
> -static inline uint16_t hvm_load_instance(struct hvm_domain_context *h)
> +static inline unsigned int hvm_load_instance(const struct
> hvm_domain_context *h)
>  {
> -    struct hvm_save_descriptor *d
> -        = (struct hvm_save_descriptor *)&h->data[h->cur];
> +    const struct hvm_save_descriptor *d = (const void *)&h->data[h->cur];
> +
>      return d->instance;
>  }
> 
> 
> 


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

Re: [PATCH] x86/HVM: improve a few state load checks

[Andrew Cooper]

On 16/07/18 15:05, Paul Durrant wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -976,14 +976,13 @@ unsigned long hvm_cr4_guest_valid_bits(c
>>
>>  static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>>  {
>> -    int vcpuid;
>> +    unsigned int vcpuid = hvm_load_instance(h);
>>      struct vcpu *v;
>>      struct hvm_hw_cpu ctxt;
>>      struct segment_register seg;
>>      const char *errstr;
>>
>>      /* Which vcpu is this? */
>> -    vcpuid = hvm_load_instance(h);
>>      if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
> This open coded pattern:
>
> vcpuid = hvm_load_instance(h);
> if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
> { ...
>
> seems to be repeated an awful lot. Is it time, perhaps, to introduce a helper function that incorporates the check?

In some copious free time when I can post v2 of my "fix max_vcpus"
series, all of this logic (and much more) will become redundant and get
culled.

I really wouldn't waste time re-factoring it at this point.

~Andrew

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

Re: [PATCH] x86/HVM: improve a few state load checks

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 16 July 2018 15:08
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> <JBeulich@suse.com>; xen-devel <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH] x86/HVM: improve a few state load checks
> 
> On 16/07/18 15:05, Paul Durrant wrote:
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -976,14 +976,13 @@ unsigned long hvm_cr4_guest_valid_bits(c
> >>
> >>  static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t
> *h)
> >>  {
> >> -    int vcpuid;
> >> +    unsigned int vcpuid = hvm_load_instance(h);
> >>      struct vcpu *v;
> >>      struct hvm_hw_cpu ctxt;
> >>      struct segment_register seg;
> >>      const char *errstr;
> >>
> >>      /* Which vcpu is this? */
> >> -    vcpuid = hvm_load_instance(h);
> >>      if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
> > This open coded pattern:
> >
> > vcpuid = hvm_load_instance(h);
> > if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
> > { ...
> >
> > seems to be repeated an awful lot. Is it time, perhaps, to introduce a
> helper function that incorporates the check?
> 
> In some copious free time when I can post v2 of my "fix max_vcpus"
> series, all of this logic (and much more) will become redundant and get
> culled.
> 
> I really wouldn't waste time re-factoring it at this point.

Ok. If that's the case then...

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

> 
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel