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

["Jan Beulich" <JBeulich@novell.com>]

>>> On 21.04.11 at 09:14, Haitao Shan <maillists.shan@gmail.com> wrote:
> 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.

Allen?

> 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 
>>