> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> 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, <[email protected]> wrote:
> >> From: Jan Beulich [mailto:[email protected]]
> >> 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
[email protected]
http://lists.xen.org/xen-devel
