Re: [Xen-devel] [PATCH v6] interrupts: allow guest to set/clear MSI-X mask bit

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.08.13 at 13:16, Joby Poriyath <joby.poriyath@xxxxxxxxxx> wrote:
> On Fri, Aug 16, 2013 at 10:55:39AM +0100, Jan Beulich wrote:
>> >>> On 15.08.13 at 17:47, Joby Poriyath <joby.poriyath@xxxxxxxxxx> wrote:
>> > @@ -404,7 +436,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq 
> *pirq, uint64_t gtable)
>> >  
>> >      entry = new_entry;
>> >      new_entry = NULL;
>> > -    add_msixtbl_entry(d, pdev, gtable, entry);
>> > +    add_msixtbl_entry(d, pdev, gtable, entry, pirq);
>> >  
>> >  found:
>> >      atomic_inc(&entry->refcnt);
>> 
>> Just noticed this "found" label here, which made me go back and
>> look at the whole function: Did you consider the case of there
>> already being an entry, and hence add_msixtbl_entry() not
>> getting called, and thus entry->pirq not getting set to what got
>> passed in here? I'm assuming that this is only ever the case if
>> for the entry found entry->pirq == pirq, but if I'm right with
>> this, adding a respective ASSERT() here would seem desirable.
> 
> If there was an entry, that was there only because it was added using 
> "add_msixtbl_entry" function, and hence entry->pirq would have been 
> initialized with a valid pirq. And modification of msixtbl_list is 
> protected with a spinlock on msixtbl_list_lock. So in any case (adding
> for the first time, or finding an entry), "entry" would have been 
> correctly initialized. So looks like it's safe.
> 
> Or, am I getting this wrong? 

"a valid pirq" != "the same pirq"

I went through the code there and at the call site, and as said I
think the pirq should be the same, but I couldn't see proof of that.

Jan


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