From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH resend 2/3] x86: enable multi-vector MSI Date: Wed, 17 Jul 2013 11:02:47 +0100 Message-ID: <51E66BC7.7090406@citrix.com> References: <51E535F902000078000E547F@nat28.tlf.novell.com> <51E5392502000078000E54AE@nat28.tlf.novell.com> <51E53046.1040809@citrix.com> <51E54F1F02000078000E55C0@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51E54F1F02000078000E55C0@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , xiantao.zhang@intel.com, xen-devel List-Id: xen-devel@lists.xenproject.org On 16/07/13 12:48, Jan Beulich wrote: >>>> On 16.07.13 at 13:36, Andrew Cooper wrote: >> On 16/07/13 11:14, Jan Beulich wrote: >>> +int get_free_pirqs(struct domain *d, unsigned int nr) >>> +{ >>> + unsigned int i, found = 0; >>> + >>> + ASSERT(spin_is_locked(&d->event_lock)); >>> + >>> + for ( i = d->nr_pirqs - 1; i >= nr_irqs_gsi; --i ) >>> + if ( is_free_pirq(d, pirq_info(d, i)) ) >>> + { >>> + pirq_get_info(d, i); >>> + if ( ++found == nr ) >>> + return i; >>> + } >>> + else >>> + found = 0; >>> + >>> + return -ENOSPC; >>> +} >>> + >> Is there any reason why this loop is backwards? Unless I am mistaken, >> guests can choose their own pirqs when binding them, reducing the >> likelyhood that the top of the available space will be free. > This just follows the behavior of get_free_pirq(). I'm not up to > having the two behave differently. > >>> @@ -210,8 +237,15 @@ int physdev_map_pirq(domid_t domid, int >>> done: >>> spin_unlock(&d->event_lock); >>> spin_unlock(&pcidevs_lock); >>> - if ( (ret != 0) && (type == MAP_PIRQ_TYPE_MSI) && (*index == -1) ) >>> - destroy_irq(irq); >>> + if ( ret != 0 ) >>> + switch ( type ) >>> + { >>> + case MAP_PIRQ_TYPE_MSI: >>> + if ( *index == -1 ) >>> + case MAP_PIRQ_TYPE_MULTI_MSI: >>> + destroy_irq(irq); >>> + break; >>> + } >> Do we not need to create and destroy entry_nr irqs in this function, or >> is a multi-vector-msi now considered as just as single irq ? >> >> I ask because this appears to lack the "repeating certain operations for >> all involved IRQs" described in the comment. > No, there's a single create_irq() in the function, and having a single > destroy_irq() here matches this. The remaining ones (both!) are in > map_domain_pirq(). Ok. Reviewed-by: Andrew Cooper > > Also, as a general remark, asking for changes in a series that was > posted 2.5 months ago (and deferred just because of the 4.3 > release process) seems a little strange to me. I had to repost > merely to collect ack-s, and didn't really expect further requests > for adjustments as there was ample time before to do so. > > Jan > 2.5 months ago, I was very busy with XSAs, hotfixes and a release of XenServer, so I appologies for not reviewing back then. ~Andrew