RE: [Xen-devel] use of struct hvm_mirq_dpci_mapping.gmsi vs. HVM_IRQ_DPCI_*_MSI flags

["Kay, Allen M" <allen.m.kay@xxxxxxxxx>]

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@xxxxxxxxxx] 
Sent: Tuesday, April 26, 2011 11:43 PM
To: Kay, Allen M
Cc: Haitao Shan; xen-devel@xxxxxxxxxxxxxxxxxxx
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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel