Re: [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls

[Jan Beulich]

>>> On 16.06.15 at 16:03, <address@hidden> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> +    } else if (s->msix->enabled) {
>> +        if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
>> +            xen_pt_msix_disable(s);
>> +            s->msix->enabled = false;
>> +        } else if (!s->msix->maskall) {
> 
> Why are you changing the state of s->msix->maskall here?
> This is the value & PCI_MSIX_FLAGS_ENABLE case, nothing to do with
> maskall, right?

We're at an else if inside an else if here. The only case left
after the if() still seen above is that value has
PCI_MSIX_FLAGS_MASKALL set.

>> +            s->msix->maskall = true;
>> +            xen_pt_msix_maskall(s, true);
>> +        }
>>      }
>>  
>> -    debug_msix_enabled_old = s->msix->enabled;
>> -    s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
>>      if (s->msix->enabled != debug_msix_enabled_old) {
>>          XEN_PT_LOG(&s->dev, "%s MSI-X\n",
>>                     s->msix->enabled ? "enable" : "disable");
>>      }
>>  
>> +    xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, 
>> &dev_value);
> 
> I have to say that I don't like the asymmetry between reading and
> writing PCI config registers. If writes go via hypercalls, reads should
> go via hypercalls too.

We're not doing any cfg register write via hypercalls (not here,
and not elsewhere). What is being replaced by the patch are
write to two bits which happen to live in PCI config space. Plus,
reading directly, and doing writes via hypercall only when really
needed would still be the right thing from a performance pov.

>> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
>> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
>> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
>>          return -1;
>>      }
>>  
>> -    return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
>> -                           enabled);
> 
> Would it make sense to remove msi_msix_enable completely to avoid any
> further mistakes?

Perhaps, yes. I think I actually had suggested so quite a while back.
But I don't see myself wasting much more time on this, ehm, code.

Jan