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 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. 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) 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?

  Paul

> 
> >> +        {
> >> +            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?
> 
> No, because both size and r->count did already get bounds
> checked to very narrow ranges.
> 
> Jan


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