From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: use of struct hvm_mirq_dpci_mapping.gmsi vs. HVM_IRQ_DPCI_*_MSI flags Date: Tue, 26 Apr 2011 09:48:14 +0100 Message-ID: <4DB6A2EE020000780003E1DC@vpn.id2.novell.com> References: <4D94A88C0200007800039637@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Allen M Kay Cc: Haitao Shan , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org >>> On 21.04.11 at 09:14, Haitao Shan wrote: > See comments below. >=20 > 2011/3/31 Jan Beulich >=20 >> 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. >=20 >=20 >> >> 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_MS= I, >> 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. Allen? > Thanks! >=20 >> >> 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 >=20 >=20 >> >> Thanks, Jan >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com=20 >> http://lists.xensource.com/xen-devel=20 >>