Re: use of struct hvm_mirq_dpci_mapping.gmsi vs. HVM_IRQ_DPCI_*_MSI flags

[Haitao Shan <maillists.shan@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2798 bytes --]

See comments below.

2011/3/31 Jan Beulich <JBeulich@novell.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 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 <keir.fraser@citrix.com>
# 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 <qing.he@intel.com>


>
> Thanks, Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

[-- Attachment #1.2: Type: text/html, Size: 4155 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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