Xen devel - Mar 2011 - use of struct hvm_mirq_dpci_mapping.gmsi vs. HVM_IRQ_DPCI_*_MSI flags

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''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).

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.

Thanks, Jan


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

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
>

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

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



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

>>
>> 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.
>>
Jan, sorry for the late reply.  I was out of the office in the past week.

Are you proposing the following data structure change?

struct hvm_mirq_dpci_mapping {
    uint32_t flags;
    int pending;
    union {
        struct timer *hvm_timer;
        struct list_head_digl_list;
        struct domain *dom;
        struct hvm_gmsi_info gmsi;
    };
}
>> 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).
>>
Are you saying above structure change will allow you to change the code to
combine
memory allocation of hvm_irq_dpci->mirq and hvm_irq_dpci->hvm_timer into a
single
allocation of array of pointers?

If my understanding of the proposal correctly, this will only work if all the
elements
inside of the union are not used at the same time.

Please confirm/correct my understanding, Haitao and I will discuss this
tomorrow.

Allen


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

>>> On 27.04.11 at 04:49, "Kay, Allen M"
<allen.m.kay@intel.com> wrote:
>> >
>>> 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.
>>>
> 
> Jan, sorry for the late reply.  I was out of the office in the past week.
> 
> Are you proposing the following data structure change?
> 
> struct hvm_mirq_dpci_mapping {
>     uint32_t flags;
>     int pending;
>     union {
>         struct timer *hvm_timer;
>         struct list_head_digl_list;
>         struct domain *dom;
>         struct hvm_gmsi_info gmsi;
>     };
> }
No - afaics timer, digl_list, and dom must be usable at the same
time, so only gmsi is an actual overlay (union) candidate. But
then again there''s not that much of a significance to this
anymore once these won''t get allocated as arrays, so it''s more
of a second level optimization.

Also, with my current (not yet posted) implementation there
won''t be arrays of pointers either, instead there''ll be a
radix
tree (indexed by guest pirq) with pointers attached. So it''ll be
a per-domain structure

struct hvm_irq_dpci {
    /* Guest IRQ to guest device/intx mapping. */
    struct list_head girq[NR_HVM_IRQS];
    /* Record of mapped ISA IRQs */
    DECLARE_BITMAP(isairq_map, NR_ISAIRQS);
    /* Record of mapped Links */
    uint8_t link_cnt[NR_LINK];
    struct tasklet dirq_tasklet;
};

and a per-guest-pirq one

struct hvm_pirq_dpci {
    uint32_t flags;
    bool_t masked;
    uint16_t pending;
    struct list_head digl_list;
    struct domain *dom;
    struct hvm_gmsi_info gmsi;
    struct timer timer;
};

which possibly in a second step could become

struct hvm_pirq_dpci {
    uint32_t flags;
    bool_t masked;
    uint16_t pending;
    union {
        struct {
            struct list_head digl_list;
            struct domain *dom;
            struct timer timer;
        } pci;
        struct {
            uint32_t gvec;
            uint32_t gflags;
            int dest_vcpu_id; /* -1 :multi-dest, non-negative: dest_vcpu_id */
        } msi;
    };
};

But clarification on the current (perhaps vs intended) use of
HVM_IRQ_DPCI_*_MSI would still be much appreciated (and if,
as suspected, there''s need to clean this up, I''d like the
cleanup
to be done before the patches I have pending).

Also, there is one more open question (quoting
the mail titled "pt_irq_time_out() dropping d->event_lock before
calling pirq_guest_eoi()"):

"What is the reason for this? irq_desc''s lock nests inside
d->event_lock,
and not having to drop the lock early would not only allow the two loops
to be folded, but also to call a short cut version of pirq_guest_eoi()
that already obtained the pirq->irq mapping (likely going to be created
when splitting the d->nr_pirqs sized arrays I''m working on
currently)."

In my pending patches I imply that this separation is indeed
unnecessary.

Jan


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

>
>
> But clarification on the current (perhaps vs intended) use of
> HVM_IRQ_DPCI_*_MSI would still be much appreciated (and if,
> as suspected, there''s need to clean this up, I''d like the
cleanup
> to be done before the patches I have pending).
>
> Jan, I think the meaning of the flags are pretty straight forward. But I
agree with you, we need to clean this up. I don''t believe all the flags
are
necessary at the moment (given the fact that they are introduced by
host-MSI-to-guest-INTx translation). But it is still OK for me if they did
not cause trouble and were not wrongly used.
I wonder whether the original patch has carefully considered the usage of
these flags when it tried to introduce these flags by nature.

Basically, I think it is up to you to decide their (flags) future. You are
already very careful on this. :)

Shan Haitao


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

Jan,

Haitao and I had a discussion on this.

1) Proposed data structure change: looks good to us.

2) HVM_IRQ_DPCI_GUEST_MSI and HVM_IRQ_DPCI_MACH_MSI usage:  These are for handle
cases various combination of host and guest interrupt types -
host_msi/guest_msi, host_intx/guest_intx, host_msi/guest_intx.  The last one
requires translation flag.  It is for supporting non MSI capable guests.  The
engineer originally worked on this is no longer working on this project.  You
are welcome to clean up if necessary.  However, testing various host/guest
interrupt combinations and make sure everything still works is quite a bit of
work.

3) I believe the locking mechanism was originally implemented by
Espen@netrome(?) so we are not sure about why the unlock is needed between two
iterations.  We have also encountered several which we would like to clean up. 
However, we left it as low priority task as the locking mechanisms are quite
complex and the amount of testing required after the cleanup is a quite a bit of
work.

Allen

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@novell.com] 
Sent: Tuesday, April 26, 2011 11:43 PM
To: Kay, Allen M
Cc: Haitao Shan; xen-devel@lists.xensource.com
Subject: RE: [Xen-devel] use of struct hvm_mirq_dpci_mapping.gmsi vs.
HVM_IRQ_DPCI_*_MSI flags
>>> On 27.04.11 at 04:49, "Kay, Allen M"
<allen.m.kay@intel.com> wrote:
>> >
>>> 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.
>>>
> 
> Jan, sorry for the late reply.  I was out of the office in the past week.
> 
> Are you proposing the following data structure change?
> 
> struct hvm_mirq_dpci_mapping {
>     uint32_t flags;
>     int pending;
>     union {
>         struct timer *hvm_timer;
>         struct list_head_digl_list;
>         struct domain *dom;
>         struct hvm_gmsi_info gmsi;
>     };
> }
No - afaics timer, digl_list, and dom must be usable at the same
time, so only gmsi is an actual overlay (union) candidate. But
then again there''s not that much of a significance to this
anymore once these won''t get allocated as arrays, so it''s more
of a second level optimization.

Also, with my current (not yet posted) implementation there
won''t be arrays of pointers either, instead there''ll be a
radix
tree (indexed by guest pirq) with pointers attached. So it''ll be
a per-domain structure

struct hvm_irq_dpci {
    /* Guest IRQ to guest device/intx mapping. */
    struct list_head girq[NR_HVM_IRQS];
    /* Record of mapped ISA IRQs */
    DECLARE_BITMAP(isairq_map, NR_ISAIRQS);
    /* Record of mapped Links */
    uint8_t link_cnt[NR_LINK];
    struct tasklet dirq_tasklet;
};

and a per-guest-pirq one

struct hvm_pirq_dpci {
    uint32_t flags;
    bool_t masked;
    uint16_t pending;
    struct list_head digl_list;
    struct domain *dom;
    struct hvm_gmsi_info gmsi;
    struct timer timer;
};

which possibly in a second step could become

struct hvm_pirq_dpci {
    uint32_t flags;
    bool_t masked;
    uint16_t pending;
    union {
        struct {
            struct list_head digl_list;
            struct domain *dom;
            struct timer timer;
        } pci;
        struct {
            uint32_t gvec;
            uint32_t gflags;
            int dest_vcpu_id; /* -1 :multi-dest, non-negative: dest_vcpu_id */
        } msi;
    };
};

But clarification on the current (perhaps vs intended) use of
HVM_IRQ_DPCI_*_MSI would still be much appreciated (and if,
as suspected, there''s need to clean this up, I''d like the
cleanup
to be done before the patches I have pending).

Also, there is one more open question (quoting
the mail titled "pt_irq_time_out() dropping d->event_lock before
calling pirq_guest_eoi()"):

"What is the reason for this? irq_desc''s lock nests inside
d->event_lock,
and not having to drop the lock early would not only allow the two loops
to be folded, but also to call a short cut version of pirq_guest_eoi()
that already obtained the pirq->irq mapping (likely going to be created
when splitting the d->nr_pirqs sized arrays I''m working on
currently)."

In my pending patches I imply that this separation is indeed
unnecessary.

Jan


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

>>> On 28.04.11 at 22:27, "Kay, Allen M"
<allen.m.kay@intel.com> wrote:
> 2) HVM_IRQ_DPCI_GUEST_MSI and HVM_IRQ_DPCI_MACH_MSI usage:  These are for 
> handle cases various combination of host and guest interrupt types - 
> host_msi/guest_msi, host_intx/guest_intx, host_msi/guest_intx.  The last
one
> requires translation flag.  It is for supporting non MSI capable guests. 
The
> engineer originally worked on this is no longer working on this project. 
You
> are welcome to clean up if necessary.  However, testing various host/guest 
> interrupt combinations and make sure everything still works is quite a bit
of
> work.
The problem is that from what I can tell the current use is inconsistent,
and this inconsistency is causing problems with the data layout change.
Iirc there''s no problem as long as there''s not going to be a
union for
overlaying the PCI/gMSI fields, so I''m going to leave off that part for
the first step.

As to testing - I''ll have to rely on someone at your end doing the full
testing of these changes anyway; I simply don''t have the hardware
(and time) to do all that.
> 3) I believe the locking mechanism was originally implemented by 
> Espen@netrome(?) so we are not sure about why the unlock is needed between 
> two iterations.  We have also encountered several which we would like to 
> clean up.  However, we left it as low priority task as the locking
mechanisms
> are quite complex and the amount of testing required after the cleanup is a
> quite a bit of work.
Then we''ll have to see how it goes with the change - see above for
the testing part.

Jan


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