From mboxrd@z Thu Jan 1 00:00:00 1970 From: Haitao Shan Subject: Re: use of struct hvm_mirq_dpci_mapping.gmsi vs. HVM_IRQ_DPCI_*_MSI flags Date: Thu, 21 Apr 2011 15:14:40 +0800 Message-ID: References: <4D94A88C0200007800039637@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0053360692==" Return-path: In-Reply-To: <4D94A88C0200007800039637@vpn.id2.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: "xen-devel@lists.xensource.com" , "Kay, Allen M" List-Id: xen-devel@lists.xenproject.org --===============0053360692== Content-Type: multipart/alternative; boundary=bcaec520f43fac901804a1687e4d --bcaec520f43fac901804a1687e4d Content-Type: text/plain; charset=ISO-8859-1 See comments below. 2011/3/31 Jan Beulich > pt_irq_create_bind_vtd() initializes this substructure only when setting > .flags to HVM_IRQ_DPCI_MACH_MSI|HVM_IRQ_DPCI_GUEST_MSI (the > PT_IRQ_TYPE_MSI case), while the other path will not set > HVM_IRQ_DPCI_GUEST_MSI but may also set HVM_IRQ_DPCI_MACH_MSI. > Yet hvm_dpci_msi_eoi() and hvm_migrate_pirqs() check for > HVM_IRQ_DPCI_MACH_MSI, i.e. may run into an uninitialized > .gmsi.* field. What am I missing here? > I think these fields are introduced by MSI-to-gINTx patch. MACH_MSI means the host (physical) is using MSI, while GUEST_MSI is just what we can guess from its name. I agree only checking MACH_MSI is not enough. > > I'm largely asking because I think struct hvm_mirq_dpci_mapping.dom > and .digl_list could actually overlay .gmsi, as much as struct > hvm_irq_dpci.hvm_timer could actually rather be folded into struct > hvm_mirq_dpci_mapping (and then also overlay .gmsi). The overlay > distinction bit would, based on initialization, be HVM_IRQ_DPCI_GUEST_MSI, > but according to use it wouldn't be clear which of the two > HVM_IRQ_DPCI_*_MSI bits is actually the correct one. > > Having a single structure only would make it a lot easier to > convert struct hvm_mirq_dpci_mapping * in struct hvm_irq_dpci to > a sparse struct hvm_mirq_dpci_mapping ** (populating slots only > as they get used), thus shrinking the currently two d->nr_pirqs > sized array allocations in pt_irq_create_bind_vtd() to a single one > with only pointer size array elements (allowing up to about 512 > domain pirqs rather than currently slightly above 80 without > exceeding PAGE_SIZE on allocation). > I also agree. But I think better Allen could do the final judgement. Thanks! > > Also I'm wondering why the PT_IRQ_TYPE_MSI path of > pt_irq_create_bind_vtd() checks that on re-use of an IRQ the > flags are indicating the same kind of interrupt, while the other > path doesn't bother doing so. > The purpuse is described in the check in notes: # HG changeset patch # User Keir Fraser # Date 1239806337 -3600 # Node ID 3e64dfebabd7340f5852ad112c858efcebc9cae5 # Parent b2c43b0fba713912d8ced348b5d628743e52d8be passthrough: allow pt_bind_irq for msi update Extend pt_bind_irq to handle the update of msi guest vector and flag. Unbind and rebind using separate hypercalls may not be viable sometime. For example, the guest may update MSI address/data on fly without disabling it first (e.g. change delivery/destination), implement these updates in such a way may result in interrupt loss. Signed-off-by: Qing He > > Thanks, Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel > --bcaec520f43fac901804a1687e4d Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable See comments below.

2011/3/31 Jan Beulich= <JBeulich@nove= ll.com>
pt_irq_create_bind_vtd() initializes this substructure only when setting .flags to HVM_IRQ_DPCI_MACH_MSI|HVM_IRQ_DPCI_GUEST_MSI (the
PT_IRQ_TYPE_MSI case), while the other path will not set
HVM_IRQ_DPCI_GUEST_MSI but may also set HVM_IRQ_DPCI_MACH_MSI.
Yet hvm_dpci_msi_eoi() and hvm_migrate_pirqs() check for
HVM_IRQ_DPCI_MACH_MSI, i.e. may run into an uninitialized
.gmsi.* field. What am I missing here?
I think these f= ields are introduced by MSI-to-gINTx patch. MACH_MSI means the host (physic= al) is using MSI, while GUEST_MSI is just what we can guess from its name.<= /div>
I agree only checking MACH_MSI is not enough.
=A0

I'm largely asking because I think struct hvm_mirq_dpci_mapping.dom
and .digl_list could actually overlay .gmsi, as much as struct
hvm_irq_dpci.hvm_timer could actually rather be folded into struct
hvm_mirq_dpci_mapping (and then also overlay .gmsi). The overlay
distinction bit would, based on initialization, be HVM_IRQ_DPCI_GUEST_MSI,<= br> but according to use it wouldn't be clear which of the two
HVM_IRQ_DPCI_*_MSI bits is actually the correct one.

Having a single structure only would make it a lot easier to
convert struct hvm_mirq_dpci_mapping * in struct hvm_irq_dpci to
a sparse struct hvm_mirq_dpci_mapping ** (populating slots only
as they get used), thus shrinking the currently two d->nr_pirqs
sized array allocations in pt_irq_create_bind_vtd() to a single one
with only pointer size array elements (allowing up to about 512
domain pirqs rather than currently slightly above 80 without
exceeding PAGE_SIZE on allocation).
I also agree. But = =A0I think better Allen could do the final judgement. Thanks!

Also I'm wondering why the PT_IRQ_TYPE_MSI path of
pt_irq_create_bind_vtd() checks that on re-use of an IRQ the
flags are indicating the same kind of interrupt, while the other
path doesn't bother doing so.
The purpuse is descr= ibed in the check in notes:
=A0

Thanks, Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.= com
http://l= ists.xensource.com/xen-devel

--bcaec520f43fac901804a1687e4d-- --===============0053360692== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --===============0053360692==--