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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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