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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 28 April 2016 13:31
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel
> Subject: RE: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
> 
> >>> On 28.04.16 at 13:58, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 28 April 2016 12:44
> >> To: Paul Durrant
> >> Cc: Andrew Cooper; xen-devel
> >> Subject: RE: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
> >>
> >> >>> On 28.04.16 at 13:17, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> Sent: 28 April 2016 10:50
> >> >> @@ -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
> >>
> >> I don't see how I could be doing this.
> >>
> >> > 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.
> >>
> >> Sure. Question is whether by mixing code and comments things
> >> would get better readable (to at least somewhat address your
> >> request above), or whether that instead would make things
> >> worse. Thoughts?
> >
> > I think mentioning why you're only tackling the !r->df case would be worth
> > commenting on and if the && !r->df were on a separate line then the
> comment could
> > go inline.
> 
> That's what I did.
> 
> > Also, do you really need to check r->count (seems like a count of 0
> > should have been picked up before the code got here)
> 
> I've tried to fine where r->count == 0 would be dealt with, but
> could spot the location (other than relying on x86_emulate.c
> never passing such down), so since I want to subtract 1 from it
> (or really 4 from its product with "size") I wanted to be on the
> safe side. If you prefer, I could replace this by a respective
> ASSERT()...
> 
> > and then TBH I'm not
> > even sure what !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1))
> is even
> > checking so how about an illustratively named macro?
> 
> How about this (without macro)?
> 
>         else if ( (size == 4 || size == 8) &&
>                   /* Only support forward REP MOVS for now. */
>                   !r->df &&
>                   /*
>                    * Only fully support accesses to a single table entry for
>                    * now (if multiple ones get written to in one go, only the
>                    * final one gets dealt with).
>                    */
>                   r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
>                   !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
>         {
> 

That looks a lot better to me :-)

  Paul

> Jan


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