Re: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 April 2016 10:50
> To: xen-devel
> Cc: Andrew Cooper; Paul Durrant
> Subject: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
> 
> ... as at least certain versions of Windows use such to update the
> MSI-X table. However, to not overly complicate the logic for now
> - only EFLAGS.DF=0 is being handled,
> - only updates not crossing MSI-X table entry boundaries are handled.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -351,9 +351,10 @@ static int msixtbl_range(struct vcpu *v,
>      ASSERT(r->type == IOREQ_TYPE_COPY);
>      if ( r->dir == IOREQ_WRITE )
>      {
> +        unsigned int size = r->size;
> +
>          if ( !r->data_is_ptr )
>          {
> -            unsigned int size = r->size;
>              uint64_t data = r->data;
> 
>              if ( size == 8 )
> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
>                   ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
>                    PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
>                   !(data & PCI_MSIX_VECTOR_BITMASK) )
> +            {
>                  v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> +                v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
> +            }
> +        }
> +        else if ( (size == 4 || size == 8) && !r->df &&
> +                  r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
> +                  !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )

That's quite an if statement. Any chance of making it more decipherable? I also think it's worth putting the restrictions you outline in the commit in a comment here so that it's clear that the code is not trying to handle all corner cases.

> +        {
> +            BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
> +                         (PCI_MSIX_ENTRY_SIZE - 1));
> +
> +            v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
> +                addr + size * r->count - 4;
> +            v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
> +                r->data + size * r->count - 4;

Does there need to be any explicit type promotion here since r->data is uint64_t?

  Paul

>          }
>      }
> 
> @@ -471,6 +487,7 @@ out:
>          for_each_vcpu ( d, v )
>          {
>              if ( (v->pause_flags & VPF_blocked_in_xen) &&
> +                 !v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa &&
>                   v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
>                   (gtable + msi_desc->msi_attrib.entry_nr *
>                             PCI_MSIX_ENTRY_SIZE +
> @@ -551,9 +568,29 @@ void msixtbl_pt_cleanup(struct domain *d
>  void msix_write_completion(struct vcpu *v)
>  {
>      unsigned long ctrl_address = v-
> >arch.hvm_vcpu.hvm_io.msix_unmask_address;
> +    unsigned long snoop_addr = v-
> >arch.hvm_vcpu.hvm_io.msix_snoop_address;
> 
>      v->arch.hvm_vcpu.hvm_io.msix_snoop_address = 0;
> 
> +    if ( !ctrl_address && snoop_addr &&
> +         v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa )
> +    {
> +        const struct msi_desc *desc;
> +        uint32_t data;
> +
> +        rcu_read_lock(&msixtbl_rcu_lock);
> +        desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, snoop_addr),
> +                                    snoop_addr);
> +        rcu_read_unlock(&msixtbl_rcu_lock);
> +
> +        if ( desc &&
> +             hvm_copy_from_guest_phys(&data,
> +                                      v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa,
> +                                      sizeof(data)) == HVMCOPY_okay &&
> +             !(data & PCI_MSIX_VECTOR_BITMASK) )
> +            ctrl_address = snoop_addr;
> +    }
> +
>      if ( !ctrl_address )
>          return;
> 
> --- unstable.orig/xen/include/asm-x86/hvm/vcpu.h	2016-04-27
> 14:47:25.000000000 +0200
> +++ unstable/xen/include/asm-x86/hvm/vcpu.h	2016-04-25
> 16:04:48.000000000 +0200
> @@ -86,6 +86,7 @@ struct hvm_vcpu_io {
> 
>      unsigned long msix_unmask_address;
>      unsigned long msix_snoop_address;
> +    unsigned long msix_snoop_gpa;
> 
>      const struct g2m_ioport *g2m_ioport;
>  };
> 
> 


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