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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----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