[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
From: |
Jan Beulich |
Subject: |
Re: [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes |
Date: |
Tue, 16 Jun 2015 15:04:29 +0100 |
>>> On 16.06.15 at 15:35, <address@hidden> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
>>
>> pirq = entry->pirq;
>>
>> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
>> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>
> I admit I am having difficulties understanding the full purpose of these
> checks. Please add a comment on them.
The comment would (pointlessly imo) re-state what the code already
says:
> I guess the intention is only to make changes using the latest values,
> the ones in entry->latch, when the right conditions are met, otherwise
> keep using the old values. Is that right?
>
> In that case, don't we want to use the latest values on MASKBIT ->
> !MASKBIT transitions? In general when unmasking?
This is what we want. And with that, the questions you ask further
down should be answered too: The function gets invoked with the
pre-change mask flag state in ->latch[], and updates the values
used for actually setting up when that one has the entry masked
(or mask-all is set). The actual new value gets written to ->latch[]
after the call.
>> @@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
>> offset = addr % PCI_MSIX_ENTRY_SIZE;
>>
>> if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
>> - const volatile uint32_t *vec_ctrl;
>> -
>> if (get_entry_value(entry, offset) == val
>> && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
>> return;
>> }
>>
>> + entry->updated = true;
>> + } else if (msix->enabled && entry->updated &&
>> + !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> + const volatile uint32_t *vec_ctrl;
>> +
>> /*
>> * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
>> * up-to-date. Read from hardware directly.
>> */
>> vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
>> + PCI_MSIX_ENTRY_VECTOR_CTRL;
>> + set_entry_value(entry, offset, *vec_ctrl);
>
> Why are you calling set_entry_value with the hardware vec_ctrl value? It
> doesn't look correct to me. In any case, if you wanted to do it,
> shouldn't you just set/unset PCI_MSIX_ENTRY_CTRL_MASKBIT instead of the
> whole *vec_ctrl?
The comment above the code explains it: What we have stored locally
may not reflect reality, as we may not have seen all writes (and this
indeed isn't just a "may"). And if out cached value isn't valid anymore,
why would we not want to update all of it, rather than just the mask
bit?
>> - if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> - if (!entry->warned) {
>> - entry->warned = true;
>> - XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X
>> is"
>> - " already enabled.\n", entry_nr);
>> - }
>> - return;
>> - }
>> -
>> - entry->updated = true;
>> + xen_pt_msix_update_one(s, entry_nr);
>
> Shouldn't we call xen_pt_msix_update_one only if (*vec_ctrl &
> PCI_MSIX_ENTRY_CTRL_MASKBIT)? In other words, only when we see a
> MASKBIT -> !MASKBIT transition?
The combination of the !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)
check in the if() surrounding this call and the
(entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)
check inside the function guarantee just that (i.e. the function
invocation is benign in the other case, as entry->addr/entry->data
would remain unchanged).
Jan
- [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up, Jan Beulich, 2015/06/05
- [Qemu-devel] [PATCH 3/6] xen/MSI-X: really enforce alignment, Jan Beulich, 2015/06/05
- [Qemu-devel] [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits, Jan Beulich, 2015/06/05
- [Qemu-devel] [PATCH 5/6] xen/pass-through: log errno values rather than function return ones, Jan Beulich, 2015/06/05
- [Qemu-devel] [PATCH 6/6] xen/pass-through: constify some static data, Jan Beulich, 2015/06/05