Re: [Qemu-devel] [PATCH 2/7] megasas: Enable MSI-X support

[Andreas Färber]

Am 17.04.2014 08:20, schrieb Hannes Reinecke:
> On 04/16/2014 07:52 PM, Michael S. Tsirkin wrote:
>> On Wed, Apr 16, 2014 at 07:47:37PM +0200, Andreas Färber wrote:
>>> Am 16.04.2014 19:40, schrieb Michael S. Tsirkin:
>>>> On Wed, Apr 16, 2014 at 06:48:08PM +0200, Andreas Färber wrote:
>>>>> Am 16.04.2014 18:32, schrieb Alexander Graf:
>>>>>>
>>>>>> On 16.04.14 16:44, Hannes Reinecke wrote:
>>>>>>> MSI-X support has been fixed in qemu, so we can enable it again.
>>>>>>>
>>>>>>> Signed-off-by: Hannes Reinecke <address@hidden>
>>>>>>> ---
>>>>>>>   hw/scsi/megasas.c | 19 ++++++-------------
>>>>>>>   1 file changed, 6 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>>>>>>> index 1781525..df45286 100644
>>>>>>> --- a/hw/scsi/megasas.c
>>>>>>> +++ b/hw/scsi/megasas.c
>>>>>>> @@ -2084,7 +2084,8 @@ static const VMStateDescription vmstate_megasas = 
>>>>>>> {
>>>>>>>       .minimum_version_id = 0,
>>>>>>>       .minimum_version_id_old = 0,
>>>>>>>       .fields      = (VMStateField[]) {
>>>>>>> -        VMSTATE_PCI_DEVICE(parent_obj, MegasasState),
>>>>>>> +        VMSTATE_PCIE_DEVICE(parent_obj, MegasasState),
>>>>>>> +        VMSTATE_MSIX(parent_obj, MegasasState),
>>>>>>
>>>>>> This requires a version change for vmstate, no?
>>>>>
>>>>> The PCI -> PCIE change yes. mst might have objections for bumping an x86
>>>>> PCI device?
>>>>
>>>> Yes.
>>>> It should be possible to make this conditional on having a pcie
>>>> capability, and disable pcie capability for old pc versions.
>>>
>>> I did have a patch merging their vmstate_ structs based on some test in
>>> the series I ping'ed, unfortunately the difference was mostly config
>>> space size, so not immediately obvious how to set via compat_props, but
>>> likely solvable somehow.
>>
>> Check pci_is_express ?
>>
>>>>> The MSIX addition should be safe AFAICT since old versions would not
>>>>> enable MSI-X.
>>>>
>>>> Yes but I don't see a code like this - where's code in PC
>>>> disabling msix for old versions?
>>>
>>> The default value for use_msix (use-msix?) in v2 is already false, so no
>>> need to set it to false again in compat_props?
>>>
>>> Andreas
>>
>> Ah, I didn't notice that. Sure, if it's off by default there's
>> no need for compat code.
>>
> And the net result is ... what?
> Do I need to change the code?

Yes, NAK for this patch as is.

Why are you changing VMSTATE_PCI_DEVICE() to VMSTATE_PCIE_DEVICE()? Was
it always a PCIe device and you're fixing that now? Then put it into its
own patch with proper explanation, setting pdc->is_express = 1 *and*
assure there via some compatibility property (?) that migration from
2.0.0-rc3 to the patched version succeeds (which would probably still
break backwards migration that mst was interested in though).

Anyway, this MSI-X patch adding only VMSTATE_MSIX() after
VMSTATE_PCI[E]_DEVICE() then becomes trivially correct IMO.

If you wanted to turn on MSI-X by default as in v1, then you would need
a trivial three-line struct entry in include/hw/i386/pc.h:PC_COMPAT_2_0
setting the property to the old default of false.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg